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

[Teddy Astie <teddy.astie@xxxxxxxxxx>]

Le 12/04/2024 à 16:49, Andrew Cooper a écrit :

> 1) You introduced trailing whitespace in patch 1 in the middle line here:
>
>> + ASSERT(spin_is_locked(&iommu->intremap.lock)); + + old_ire = *entry;

Good catch, will fix

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

Yes

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

In this case, you mean both intel_iommu_supports_eim and iov_supports_xt
(AMD) ?

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

Will change this.

Teddy

---


Teddy Astie | Vates XCP-ng Intern

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech