Re: [PATCH for-4.20?] x86/dom0: be less restrictive with the Interrupt Address Range

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

On Thu, Feb 13, 2025 at 10:06:26AM +0100, Jan Beulich wrote:
> On 12.02.2025 16:38, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/dom0_build.c
> > +++ b/xen/arch/x86/dom0_build.c
> > @@ -555,10 +555,6 @@ int __init dom0_setup_permissions(struct domain *d)
> >          if ( !rangeset_contains_singleton(mmio_ro_ranges, mfn) )
> >              rc |= iomem_deny_access(d, mfn, mfn);
> >      }
> > -    /* MSI range. */
> > -    rc |= iomem_deny_access(d, paddr_to_pfn(MSI_ADDR_BASE_LO),
> > -                            paddr_to_pfn(MSI_ADDR_BASE_LO +
> > -                                         MSI_ADDR_DEST_ID_MASK));
> 
> For this one, yes, I can see the LAPIC counterpart a few lines up from here
> (as the description says). In arch_iommu_hwdom_init(), however, I can't.
> Where's that?

We crossed emails, as a bit before this reply from yours I sent:

https://lore.kernel.org/xen-devel/Z62xS26FBClpsol9@macbook.local/

> > --- a/xen/drivers/passthrough/x86/iommu.c
> > +++ b/xen/drivers/passthrough/x86/iommu.c
> > @@ -475,11 +475,6 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain 
> > *d)
> >      if ( rc )
> >          panic("IOMMU failed to remove Xen ranges: %d\n", rc);
> >  
> > -    /* Remove any overlap with the Interrupt Address Range. */
> > -    rc = rangeset_remove_range(map, 0xfee00, 0xfeeff);
> > -    if ( rc )
> > -        panic("IOMMU failed to remove Interrupt Address Range: %d\n", rc);
> 
> Besides being puzzled by the use of literal numbers here, why was this
> necessary in the first place? Or in other words, why do we not respect the
> domain's ->iomem_caps here (irrespective of iommu_hwdom_{inclusive,reserved}),
> thus making sure we don't allow access to anything dom0_setup_permissions()
> denies? There is iomem_access_permitted() checking in identity_map() for PV,
> but no equivalent for PVH that I could spot. If that was checked somewhere, my
> question on the earlier hunk would then also be addressed, of course.

Indeed, I wondered the same when adjusting this code, I think I might
go ahead and add a pre-patch that switches the code in
arch_iommu_hwdom_init() to use ->iomem_caps and remove all the
open-coded filtering if feasible.

> Further, with the expectation for the UCSI region to eventually be marked
> ACPI_NVS, how's that going to help here? The loop over the E820 map a few
> lines up from here doesn't make any mappings for E820_{ACPI,NVS}. (later) Oh,
> pvh_setup_acpi() does map them, and it running after iommu_hwdom_init() the
> mappings should be made in both page tables (if not shared).

That code is not very well laid out, we should do the mappings in a
single place preferably.

> Which gets me to
> a tangential question though: Am I failing to spot where we avoid, for the
> shared page tables case, doing all the work arch_iommu_hwdom_init() does?

Even in the shared page-table case Xen needs to perform the work done
by arch_iommu_hwdom_init(), as it maps reserved (E820_RESERVED)
regions into dom0 p2m.  PVH dom0 mandates strict mode, so
arch_iommu_hwdom_init() just maps E820_RESERVED for PVH.

Thanks, Roger.