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

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

On Tue, Jan 21, 2025 at 12:20:17PM +0100, Jan Beulich wrote:
> 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.

Oh, I see.  So you either want patch 3 ahead of this, or both patches
merged into a single one.

Thanks, Roger.