On 21.01.2025 12:09, Roger Pau Monné wrote:
> On Wed, Apr 24, 2024 at 04:27:10PM +0200, Jan Beulich wrote:
>> On 18.04.2024 13:57, Teddy Astie wrote:
>>> All hardware with VT-d/AMD-Vi has CMPXCHG16B support. Check this at
>>> initialisation time, and remove the effectively-dead logic for the
>>> non-cx16 case.
>>
>> As before: What about Xen itself running virtualized, and the underlying
>> hypervisor surfacing an IOMMU but not CX16? It may be okay to ignore the
>> IOMMU in such an event, but by not mentioning the case you give the
>> appearance of not having considered it at all.
>>
>>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
>>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
>>> @@ -305,6 +305,12 @@ static int __init cf_check iov_detect(void)
>>> if ( !iommu_enable && !iommu_intremap )
>>> return 0;
>>>
>>> + if ( unlikely(!cpu_has_cx16) )
>>> + {
>>> + printk("AMD-Vi: CPU doesn't support CMPXCHG16B, disabling\n");
>>> + return -ENODEV;
>>> + }
>>> +
>>> if ( (init_done ? amd_iommu_init_late()
>>> : amd_iommu_init(false)) != 0 )
>>> {
>>
>> I did previously point out (and that's even visible in patch context here)
>> that the per-vendor .setup() hooks aren't necessarily the first thing to
>> run. Please can you make sure you address (verbally or by code) prior to
>> submitting new versions?
>
> I've re-visiting this as part of my other IOMMU IRTE atomic update
> fix.
>
> Would you prefer the check for CX16 be in the common x86
> iommu_hardware_setup()? That would be kind of layering violation, as
> in principle a further IOMMU implementation on x86 might not require
> CX16. However I find it very unlikely, and hence I would be fine in
> placing the check in iommu_hardware_setup() if you prefer it there.
No. The check needs replicating into the other hook that may run first,
->supports_x2apic(). And the ->enable_x2apic() hooks may want to gain
respective assertions then.
Jan
