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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.12.2019 14:15, Roger Pau Monné wrote:
> 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?

And IS_*() or is_*() ought to have an object to pass to. What would
the object be here? The domain isn't applicable. Maybe something
like FULL_QUARANTINE_MODE() (without any parameters) ...

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

Makes sense, as long as we can find a half way suitable name.

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

Will do.

Jan

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