Re: [XEN PATCH v5 1/3] x86/iommu: Disable IOMMU if cx16 isn't supported

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.