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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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. */

> +    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." ?


> -    if ( pci_add_segment(0) )
> -        panic("Could not initialize PCI segment 0\n");
> +    pci_setup();
>  
>      /*
>       * IOMMU-related ACPI table parsing has to happen before APIC probing, 
> for
> diff --git a/xen/arch/x86/x86_64/mmconfig-shared.c 
> b/xen/arch/x86/x86_64/mmconfig-shared.c
> index f1a3d42c5b21..fbe2676f8636 100644
> --- a/xen/arch/x86/x86_64/mmconfig-shared.c
> +++ b/xen/arch/x86/x86_64/mmconfig-shared.c
> @@ -402,6 +402,9 @@ void __init acpi_mmcfg_init(void)
>  {
>      bool valid = true;
>  
> +    if ( acpi_disabled )
> +        return;

This is fine for the patch, making things consistent with the prior
behaviour.

However, I think it's well gone time we drop support for pre-APCI
systems, including things like acpi_disabled, etc.

Nothing good can possibly come of these codepaths these days.

~Andrew