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

[Jan Beulich <jbeulich@xxxxxxxx>]

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. I further don't
understand "vendor code" in your remark: Different macros would be
needed for either vendor anyway. (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.)

>> --- 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. 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.

>> --- 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.

Jan

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