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.
Thanks, Roger.
