mbox series

[XEN,v2,0/3] x86/iommu: Drop IOMMU support when cx16 isn't supported

Message ID cover.1712580356.git.teddy.astie@vates.tech (mailing list archive)
Headers show
Series x86/iommu: Drop IOMMU support when cx16 isn't supported | expand

Message

Teddy Astie April 8, 2024, 1:02 p.m. UTC
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

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

Comments

Andrew Cooper April 11, 2024, 8:05 p.m. UTC | #1
On 08/04/2024 2:02 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
>
> 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(-)
>

Sorry, but you've sent out two copies of each patch in this series, and
it's not clear if they're identical or not.

Please could you send out another version, making sure there's only one
of each patch.

Also, you need to swap ENOSYS with ENODEV, as per Jan's review on v1.

Thanks,

~Andrew
Marek Marczykowski-Górecki April 11, 2024, 11:54 p.m. UTC | #2
On Thu, Apr 11, 2024 at 09:05:08PM +0100, Andrew Cooper wrote:
> Sorry, but you've sent out two copies of each patch in this series, and
> it's not clear if they're identical or not.

FWIW I've got just one copy.
Teddy Astie April 12, 2024, 8:31 a.m. UTC | #3
Le 11/04/2024 à 22:05, Andrew Cooper a écrit :
> On 08/04/2024 2:02 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
>>
>> 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(-)
>>
> 
> Sorry, but you've sent out two copies of each patch in this series, and
> it's not clear if they're identical or not.
> 
> Please could you send out another version, making sure there's only one
> of each patch.
> 
> Also, you need to swap ENOSYS with ENODEV, as per Jan's review on v1.
> 
> Thanks,
> 
> ~Andrew

Hello,

Not entirely sure why it got sent twice, as marek said he only received 
it once. Will double-check next time to avoid this issue in case I 
wrongfully sent it twice.

Will also swap ENOSYS with ENODEV in the next version.

Thanks,

Teddy


Teddy Astie | Vates XCP-ng Intern

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech
Andrew Cooper April 12, 2024, 9:39 a.m. UTC | #4
On 12/04/2024 9:31 am, Teddy Astie wrote:
> Le 11/04/2024 à 22:05, Andrew Cooper a écrit :
>> On 08/04/2024 2:02 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
>>>
>>> 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(-)
>>>
>> Sorry, but you've sent out two copies of each patch in this series, and
>> it's not clear if they're identical or not.
>>
>> Please could you send out another version, making sure there's only one
>> of each patch.
>>
>> Also, you need to swap ENOSYS with ENODEV, as per Jan's review on v1.
>>
>> Thanks,
>>
>> ~Andrew
> Hello,
>
> Not entirely sure why it got sent twice, as marek said he only received 
> it once. Will double-check next time to avoid this issue in case I 
> wrongfully sent it twice.

Huh, lore agrees.  I seem to have both a direct and list copy which
weren't correctly deduplicated.

Sorry for the noise.

> Will also swap ENOSYS with ENODEV in the next version.

Thanks.

~Andrew