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.

Teddy Astie (3):
  VT-d: Disable IOMMU if cx16 isn't supported
  AMD-Vi: Disable IOMMU if cx16 isn't supported
  VT-d: Cleanup MAP_SINGLE_DEVICE and related code

 xen/arch/x86/apic.c                         |  6 ++
 xen/drivers/passthrough/amd/iommu_map.c     | 42 ++++------
 xen/drivers/passthrough/amd/pci_amd_iommu.c |  6 ++
 xen/drivers/passthrough/vtd/intremap.c      | 65 ++++-----------
 xen/drivers/passthrough/vtd/iommu.c         | 92 +++++++--------------
 xen/drivers/passthrough/vtd/vtd.h           |  5 +-
 6 files changed, 71 insertions(+), 145 deletions(-)

-- 
2.44.0



Teddy Astie | Vates XCP-ng Intern

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

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

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

On 12/04/2024 4:51 pm, Teddy Astie wrote:
> Le 12/04/2024 à 16:49, Andrew Cooper a écrit :
>> 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) ?

Yes, with the changes in the appropriate patch.

~Andrew