Re: [PATCH for-4.22 2/2] x86/mcfg: relax memory map checks on newer firmware

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 04/06/2026 11:46 am, Roger Pau Monne wrote:
> Per PCI Firmware 3.3 specification, section 4.1.2, ECAM space must be
> reserved by declaring a motherboard resource, but there's no requirement to
> mention it in E820, so we shouldn't look at E820 to validate the ECAM space
> described by MCFG.  The specification additionally states that: the
> resources can optionally be returned in Int15 E820h or EFIGetMemoryMap as
> reserved memory.

I'd take out the ", so we shouldn't ..." clause.  I see it came from the
Linux commit, but it wasn't great there either.  It's a piece of opinion
in the middle of quotes from a spec.

Then, I think you want a new paragraph between these two, saying
explicitly that some Lenvovo systems do not mark MMCFG in the memory map.

> The more strict logic was introduced in Linux in 2006 as 946f2ee5c731
> ("[PATCH] i386/x86-64: Check that MCFG points to an e820 reserved area").
> This was picked up by Xen when MCFG support was added in 3b35911d709e
> ("Enable pci mmcfg and ATS for x86_64").  Apply the same approach that
> Linux has done in 199f968f1484 ("x86/pci: Skip early E820 check for ECAM
> region") and relax the strict reserved region checking so it's only done
> for firmware manufactured prior to 2016.
>
> When dom0 is booted it can always prevent access to misconfigured MCFG
> regions by using the PHYSDEVOP_pci_mmcfg_reserved hypercall.  This brings
> Xen's early usage of MCFG (prior to ACPI AML parsing) in line with the
> implementation in Linux.
>
> This fixes an issue with detection of extended capabilities when running
> Xen on a Lenovo system that doesn't list the MCFG area as an
> EfiMemoryMappedIO region in the EFI memory map.
>
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> I'm not sure what's the best way to attribute the logic here with the
> Linux commit that introduced this, more than referencing it in the commit
> message text.  The code is too different for me to attempt to label this
> change as a backport of the original Linux commit.

I think your commit message is adequate.  It does provide all the reasoning.

> ---
>  xen/arch/x86/x86_64/mmconfig-shared.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/x86_64/mmconfig-shared.c 
> b/xen/arch/x86/x86_64/mmconfig-shared.c
> index d0cbc151705d..e24a78c8d1d3 100644
> --- a/xen/arch/x86/x86_64/mmconfig-shared.c
> +++ b/xen/arch/x86/x86_64/mmconfig-shared.c
> @@ -13,6 +13,7 @@
>   */
>  
>  #include <xen/acpi.h>
> +#include <xen/dmi.h>
>  #include <xen/init.h>
>  #include <xen/mm.h>
>  #include <xen/param.h>
> @@ -369,12 +370,15 @@ static bool __init pci_mmcfg_reject_broken(void)
>      typeof(pci_mmcfg_config[0]) *cfg;
>      int i;
>      bool valid = true;
> +    int year;
>  
>      if ((pci_mmcfg_config_num == 0) ||
>          (pci_mmcfg_config == NULL) ||
>          (pci_mmcfg_config[0].address == 0))
>          return 0;
>  
> +    dmi_get_date(DMI_BIOS_DATE, &year, NULL, NULL);
> +
>      for (i = 0; i < pci_mmcfg_config_num; i++) {
>          u64 addr, size;
>  
> @@ -390,7 +394,13 @@ static bool __init pci_mmcfg_reject_broken(void)
>                 (unsigned int)cfg->start_bus_number,
>                 (unsigned int)cfg->end_bus_number);
>  
> -        if ( !is_mmconf_reserved(addr, size, i, cfg) ||
> +        /*
> +         * For firmware from 2016 or later relax the checking and also 
> consider
> +         * MCFG regions in holes on the memory map as valid.
> +         */
> +        if ( ((year < 2016 || !is_memory_hole(maddr_to_mfn(addr),
> +                                              maddr_to_mfn(addr + size - 
> 1))) &&
> +              !is_mmconf_reserved(addr, size, i, cfg)) ||

This comment is half stale already, as "relax" is really only relevant
to the prior behaviour.

"For firmwares prior to 2016, confirm that MMCFG is marked as reserved. 
For 2016 and later, also allow MMCFG being in a hole."

It's also worth saying that this fix is different to Linux's.  Linux
simply ignores the E820 on anything newer than 2016.  Personally I
prefer the more cautious approach of saying reserved-or-hole, but this
should be called out in the commit message, I think.

~Andrew