Re: [Xen-devel] [PATCH] IOMMU: make DMA containment of quarantined devices optional

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Dec 12, 2019 at 10:28:26AM +0100, Jan Beulich wrote:
> On 11.12.2019 16:54, Roger Pau Monné wrote:
> > On Wed, Dec 11, 2019 at 01:53:00PM +0100, Jan Beulich wrote:
> >> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> >> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> >> @@ -85,18 +85,36 @@ int get_dma_requestor_id(uint16_t seg, u
> >>      return req_id;
> >>  }
> >>  
> >> -static void amd_iommu_setup_domain_device(
> >> +static int __must_check allocate_domain_resources(struct domain_iommu *hd)
> >> +{
> >> +    int rc;
> >> +
> >> +    spin_lock(&hd->arch.mapping_lock);
> >> +    rc = amd_iommu_alloc_root(hd);
> >> +    spin_unlock(&hd->arch.mapping_lock);
> >> +
> >> +    return rc;
> >> +}
> >> +
> >> +static int __must_check amd_iommu_setup_domain_device(
> >>      struct domain *domain, struct amd_iommu *iommu,
> >>      uint8_t devfn, struct pci_dev *pdev)
> >>  {
> >>      struct amd_iommu_dte *table, *dte;
> >>      unsigned long flags;
> >> -    int req_id, valid = 1;
> >> +    int req_id, valid = 1, rc;
> >>      u8 bus = pdev->bus;
> >> -    const struct domain_iommu *hd = dom_iommu(domain);
> >> +    struct domain_iommu *hd = dom_iommu(domain);
> >> +
> >> +    /* dom_io is used as a sentinel for quarantined devices */
> >> +    if ( domain == dom_io && !hd->arch.root_table )
> > 
> > This condition (and it's Intel counterpart) would be better in a macro
> > IMO, so that vendor code regardless of the implementation can use the
> > same macro (and to avoid having to add the same comment in all
> > instances), ie: IS_DEVICE_QUARANTINED or some such would be fine IMO.
> 
> The "device" in the name suggested is inapplicable, as there's no
> device involved here. The conditional also isn't about
> "quarantined", but about the extended for thereof.

Maybe IS_QUARANTINE_FULL or IS_FULLY_QUARANTINED or something similar
in order to match the command line option then?

The comment above explicitly mentions that dom_io is used as a
sentinel for quarantined devices, hence the DEVICE in the name didn't
seem that far off.

> I further don't
> understand "vendor code" in your remark: Different macros would be
> needed for either vendor anyway.

Yes, but both macros would have the same name, hence you wouldn't need
to think whether you are in AMD or Intel code as the macro would
always have the same name.

> (I did actually consider having
> some kind of predicate helper, but I couldn't come up with a
> sufficiently good name. I also think such an abstraction should
> then have been introduced when these conditionals were first added
> in their then still vendor independent form.)

I would prefer some kind of macro, as I think there's quite a lot of
replication of those two checks, and IMO it's easy to by mistake use
the wrong one when moving between Intel and AMD code (the more that
it would build fine but lead to runtime issues).

> 
> >> --- a/xen/drivers/passthrough/iommu.c
> >> +++ b/xen/drivers/passthrough/iommu.c
> >> @@ -30,13 +30,17 @@ bool_t __initdata iommu_enable = 1;
> >>  bool_t __read_mostly iommu_enabled;
> >>  bool_t __read_mostly force_iommu;
> >>  bool_t __read_mostly iommu_verbose;
> >> -bool __read_mostly iommu_quarantine = true;
> >>  bool_t __read_mostly iommu_igfx = 1;
> >>  bool_t __read_mostly iommu_snoop = 1;
> >>  bool_t __read_mostly iommu_qinval = 1;
> >>  bool_t __read_mostly iommu_intremap = 1;
> >>  bool_t __read_mostly iommu_crash_disable;
> >>  
> >> +#define IOMMU_quarantine_none  false
> >> +#define IOMMU_quarantine_basic true
> >> +#define IOMMU_quarantine_full  2
> >> +uint8_t __read_mostly iommu_quarantine = IOMMU_quarantine_basic;
> > 
> > I don't really like to use booleans with non-boolean variables.
> > Wouldn't it be better to just use plain numbers, or even better an
> > enum?
> 
> No option is really good here, I think. I did consider using an
> enum, but I wanted to restrict the variable to 8 bits.

IMO I wouldn't be that worried about using 8 vs 32 bits.

> If I was
> to use an enum, of course I'd also want to have the variable this
> (correct) type. And I'd also like to avoid the packed attribute
> here. The above seemed to least bad option; I could be convinced
> to use 0/1 instead of false/true, though.

Yes please, 0/1 would be fine for me.

> 
> >> --- a/xen/include/xen/iommu.h
> >> +++ b/xen/include/xen/iommu.h
> >> @@ -53,8 +53,9 @@ static inline bool_t dfn_eq(dfn_t x, dfn
> >>  }
> >>  
> >>  extern bool_t iommu_enable, iommu_enabled;
> >> -extern bool force_iommu, iommu_quarantine, iommu_verbose, iommu_igfx;
> >> +extern bool force_iommu, iommu_verbose, iommu_igfx;
> >>  extern bool_t iommu_snoop, iommu_qinval, iommu_intremap, iommu_intpost;
> >> +extern uint8_t iommu_quarantine;
> > 
> > Exporting this variable without the paired defines seems pointless,
> > how are external callers supposed to figure out the quarantine mode
> > without the IOMMU_quarantine_* defines?
> 
> Why pointless? Outside of the file knowing the IOMMU_quarantine_*
> defines the variable continues to have boolean meaning.

Do you think you could add a comment to that effect?

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel