Re: [PATCH] x86/iommu: setup MMCFG ahead of IOMMU

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

On Tue, Aug 19, 2025 at 07:23:57PM +0100, Andrew Cooper wrote:
> On 19/08/2025 6:18 pm, Roger Pau Monne wrote:
> > Otherwise the PCI accesses to segments different than the first one done by
> > the IOMMU initialization code would silently fail by returning all ones.
> >
> > Introduce a new helper, called pci_setup(), and move both the creation of
> > PCI segment 0 internal data structures, plus the parsing of ACPI MMCFG
> > table to it.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> And moving acpi_mmcfg_init() slightly earlier from acpi_boot_init() into
> pci_setup().
> 
> > diff --git a/xen/arch/x86/pci.c b/xen/arch/x86/pci.c
> > index 26bb7f6a3c3a..e75a29e851a7 100644
> > --- a/xen/arch/x86/pci.c
> > +++ b/xen/arch/x86/pci.c
> > @@ -139,6 +142,19 @@ int pci_sanitize_bar_memory(struct rangeset *r)
> >      return 0;
> >  }
> >  
> > +void __init pci_setup(void)
> > +{
> > +    /*
> > +     * Ahead of any ACPI table parsing make sure we have control structures
> > +     * for PCI segment 0.
> > +     */
> > +    if ( pci_add_segment(0) )
> > +        panic("Could not initialize PCI segment 0\n");
> > +
> > +    /* Parse ACPI MMCFG ahead of IOMMU, so accesses to segments > 0 is 
> > setup. */
> 
> "ahead of IOMMU" isn't helpful here because the relevant context is in
> the caller.  Instead, I'd just say:
> 
> /* Parse ACPI MMCFG to see if other segments are available. */

Sure.

> > +    acpi_mmcfg_init();
> > +}
> > +
> >  /*
> >   * Local variables:
> >   * mode: C
> > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> > index 6fb42c5a5f95..bd648323bfed 100644
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -1938,11 +1938,10 @@ void asmlinkage __init noreturn __start_xen(void)
> >      setup_system_domains();
> >  
> >      /*
> > -     * Ahead of any ACPI table parsing make sure we have control structures
> > -     * for PCI segment 0.
> > +     * Initialize PCI (create segment 0, setup MMCFG access) ahead of IOMMU
> > +     * setup, as it requires access to the PCI config space.
> >       */
> 
> Again, this isn't terribly clear IMO.
> 
> "ahead of IOMMU setup, as the IOMMUs might not all live on segment 0." ?

It's not just IOMMUs, but for example on VT-d we also need to poke at
the config space of bridges, and when such bridges live in segment > 0
that results in garbage being returned.

I'm not sure acpi_iommu_init() accesses the IOMMU PCI device config
space, but it does at least access the config space of bridges in
order to detect hierarchy.  See how acpi_parse_dev_scope() performs
PCI reads.

What about using:

/*
 * Initialize PCI (create segment 0, setup MMCFG access) ahead of IOMMU
 * setup, as devices in segment > 0 must also be discoverable.
 */

Thanks, Roger.