Re: [XEN PATCH v3 0/3] x86/iommu: Drop IOMMU support when cx16 isn't supported

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 12/04/2024 1:33 pm, Teddy Astie wrote:
> All hardware that supports VT-d/AMD-Vi that exists also supports cx16 (aside
> specifically crafted virtual machines).
>
> Some IOMMU code paths in Xen consider cases where VT-d/AMD-Vi is supported
> while cx16 isn't, those paths may be bugged and are barely tested, dead code
> in practice.
>
> Disable IOMMU in case we have IOMMU hardware but no cx16, then cleanup
> no-cx16 handling logic from VT-d and AMD-Vi drivers.
>
> Teddy
>
> Changed in v2:
>
>  * Added cleanup no-cx16 code for x2APIC
>  * Fixed commit and code formatting
>  * Added missing Suggested-by note
>
> Changed in v3:
>
>  * Use -ENODEV instead of -ENOSYS.

Hmm - I've still got a double copy of patch 2.  No idea what's going on,
but I'll make do with what b4 thinks is on the list.

A couple of things.

1) You introduced trailing whitespace in patch 1 in the middle line here:

> + ASSERT(spin_is_locked(&iommu->intremap.lock)); + + old_ire = *entry;

2) In your commit messages, the grammar is a bit awkward.  It would be
clearer to say:

"All hardware with VT-d/AMD-Vi has CMPXCHG16 support.  Check this at
initialisation time, and remove the effectively-dead logic for the
non-cx16 case."

just as you've done in the cover letter.


3) In patch 1, you shouldn't modify x2apic_bsp_setup() like that. 
x2APIC && no-IOMMU is a legal combination.

Instead, you should put a cx16 check in both driver's supports_x2apic()
hooks.


4) In patch 3, you should drop the Suggested-by me.  You found that one
all yourself.

~Andrew