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

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

On Thu, Jun 04, 2026 at 12:08:30PM +0100, Andrew Cooper wrote:
> 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.

OK, I will move the last paragraph here then.

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

I've adjusted the in-code comment and reworded the commit message to
notice this divergence with Linux.

Thanks, Roger.