diff mbox series

[1/1] x86/pci: Skip early E820 check for ECAM region

Message ID 20240417204012.215030-2-helgaas@kernel.org (mailing list archive)
State Accepted
Delegated to: Bjorn Helgaas
Headers show
Series x86/pci: Skip early E820 check for ECAM region | expand

Commit Message

Bjorn Helgaas April 17, 2024, 8:40 p.m. UTC
From: Bjorn Helgaas <bhelgaas@google.com>

Arul, Mateusz, Imcarneiro91, and Aman reported a regression caused by
07eab0901ede ("efi/x86: Remove EfiMemoryMappedIO from E820 map").  On the
Lenovo Legion 9i laptop, that commit removes the area containing ECAM from
E820, which means the early E820 validation started failing, which meant we
didn't enable ECAM in the "early MCFG" path

The lack of ECAM caused many ACPI methods to fail, resulting in the
embedded controller, PS/2, audio, trackpad, and battery devices not being
detected.  The _OSC method also failed, so Linux could not take control of
the PCIe hotplug, PME, and AER features:

  # pci_mmcfg_early_init()

  PCI: ECAM [mem 0xc0000000-0xce0fffff] (base 0xc0000000) for domain 0000 [bus 00-e0]
  PCI: not using ECAM ([mem 0xc0000000-0xce0fffff] not reserved)

  ACPI Error: AE_ERROR, Returned by Handler for [PCI_Config] (20230628/evregion-300)
  ACPI: Interpreter enabled
  ACPI: Ignoring error and continuing table load
  ACPI BIOS Error (bug): Could not resolve symbol [\_SB.PC00.RP01._SB.PC00], AE_NOT_FOUND (20230628/dswload2-162)
  ACPI Error: AE_NOT_FOUND, During name lookup/catalog (20230628/psobject-220)
  ACPI: Skipping parse of AML opcode: OpcodeName unavailable (0x0010)
  ACPI BIOS Error (bug): Could not resolve symbol [\_SB.PC00.RP01._SB.PC00], AE_NOT_FOUND (20230628/dswload2-162)
  ACPI Error: AE_NOT_FOUND, During name lookup/catalog (20230628/psobject-220)
  ...
  ACPI Error: Aborting method \_SB.PC00._OSC due to previous error (AE_NOT_FOUND) (20230628/psparse-529)
  acpi PNP0A08:00: _OSC: platform retains control of PCIe features (AE_NOT_FOUND)

  # pci_mmcfg_late_init()

  PCI: ECAM [mem 0xc0000000-0xce0fffff] (base 0xc0000000) for domain 0000 [bus 00-e0]
  PCI: [Firmware Info]: ECAM [mem 0xc0000000-0xce0fffff] not reserved in ACPI motherboard resources
  PCI: ECAM [mem 0xc0000000-0xce0fffff] is EfiMemoryMappedIO; assuming valid
  PCI: ECAM [mem 0xc0000000-0xce0fffff] reserved to work around lack of ACPI motherboard _CRS

Per PCI Firmware r3.3, sec 4.1.2, ECAM space must be reserved by a PNP0C02
resource, but it need not be mentioned in E820, so we shouldn't look at
E820 to validate the ECAM space described by MCFG.

946f2ee5c731 ("[PATCH] i386/x86-64: Check that MCFG points to an e820
reserved area") added a sanity check of E820 to work around buggy MCFG
tables, but that over-aggressive validation causes failures like this one.

Keep the E820 validation check only for older BIOSes (pre-2016) so the
buggy 2006-era machines don't break.  Skip the early E820 check for 2016
and newer BIOSes.

Fixes: 07eab0901ede ("efi/x86: Remove EfiMemoryMappedIO from E820 map")
Reported-by: Mateusz Kaduk <mateusz.kaduk@gmail.com>
Reported-by: Arul <...>
Reported-by: Imcarneiro91 <...>
Reported-by: Aman <...>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218444
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Tested-by: Mateusz Kaduk <mateusz.kaduk@gmail.com>
Cc: stable@vger.kernel.org
---
 arch/x86/pci/mmconfig-shared.c | 35 +++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

Comments

Andy Shevchenko April 17, 2024, 8:47 p.m. UTC | #1
On Wed, Apr 17, 2024 at 11:40 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> Arul, Mateusz, Imcarneiro91, and Aman reported a regression caused by
> 07eab0901ede ("efi/x86: Remove EfiMemoryMappedIO from E820 map").  On the
> Lenovo Legion 9i laptop, that commit removes the area containing ECAM from
> E820, which means the early E820 validation started failing, which meant we
> didn't enable ECAM in the "early MCFG" path
>
> The lack of ECAM caused many ACPI methods to fail, resulting in the
> embedded controller, PS/2, audio, trackpad, and battery devices not being
> detected.  The _OSC method also failed, so Linux could not take control of
> the PCIe hotplug, PME, and AER features:
>
>   # pci_mmcfg_early_init()
>
>   PCI: ECAM [mem 0xc0000000-0xce0fffff] (base 0xc0000000) for domain 0000 [bus 00-e0]
>   PCI: not using ECAM ([mem 0xc0000000-0xce0fffff] not reserved)
>
>   ACPI Error: AE_ERROR, Returned by Handler for [PCI_Config] (20230628/evregion-300)
>   ACPI: Interpreter enabled
>   ACPI: Ignoring error and continuing table load
>   ACPI BIOS Error (bug): Could not resolve symbol [\_SB.PC00.RP01._SB.PC00], AE_NOT_FOUND (20230628/dswload2-162)
>   ACPI Error: AE_NOT_FOUND, During name lookup/catalog (20230628/psobject-220)
>   ACPI: Skipping parse of AML opcode: OpcodeName unavailable (0x0010)
>   ACPI BIOS Error (bug): Could not resolve symbol [\_SB.PC00.RP01._SB.PC00], AE_NOT_FOUND (20230628/dswload2-162)
>   ACPI Error: AE_NOT_FOUND, During name lookup/catalog (20230628/psobject-220)
>   ...
>   ACPI Error: Aborting method \_SB.PC00._OSC due to previous error (AE_NOT_FOUND) (20230628/psparse-529)
>   acpi PNP0A08:00: _OSC: platform retains control of PCIe features (AE_NOT_FOUND)
>
>   # pci_mmcfg_late_init()
>
>   PCI: ECAM [mem 0xc0000000-0xce0fffff] (base 0xc0000000) for domain 0000 [bus 00-e0]
>   PCI: [Firmware Info]: ECAM [mem 0xc0000000-0xce0fffff] not reserved in ACPI motherboard resources
>   PCI: ECAM [mem 0xc0000000-0xce0fffff] is EfiMemoryMappedIO; assuming valid
>   PCI: ECAM [mem 0xc0000000-0xce0fffff] reserved to work around lack of ACPI motherboard _CRS
>
> Per PCI Firmware r3.3, sec 4.1.2, ECAM space must be reserved by a PNP0C02
> resource, but it need not be mentioned in E820, so we shouldn't look at
> E820 to validate the ECAM space described by MCFG.
>
> 946f2ee5c731 ("[PATCH] i386/x86-64: Check that MCFG points to an e820
> reserved area") added a sanity check of E820 to work around buggy MCFG
> tables, but that over-aggressive validation causes failures like this one.
>
> Keep the E820 validation check only for older BIOSes (pre-2016) so the
> buggy 2006-era machines don't break.  Skip the early E820 check for 2016
> and newer BIOSes.

> Fixes: 07eab0901ede ("efi/x86: Remove EfiMemoryMappedIO from E820 map")
> Reported-by: Mateusz Kaduk <mateusz.kaduk@gmail.com>

> Reported-by: Arul <...>
> Reported-by: Imcarneiro91 <...>
> Reported-by: Aman <...>

Isn't bugzilla public enough? You may take emails from there, no?

...

> +               /*
> +                * 946f2ee5c731 ("Check that MCFG points to an e820
> +                * reserved area") added this E820 check in 2006 to work
> +                * around BIOS defects.
> +                *
> +                * Per PCI Firmware r3.3, sec 4.1.2, ECAM space must be
> +                * reserved by a PNP0C02 resource, but it need not be
> +                * mentioned in E820.  Before the ACPI interpreter is
> +                * available, we can't check for PNP0C02 resources, so
> +                * there's no reliable way to verify the region in this
> +                * early check.  Keep it only for the old machines that
> +                * motivated 946f2ee5c731.
> +                */

> +               if (dmi_get_bios_year() < 2016 && raw_pci_ops)

I probably missed something, but where does 2016 come from?
(I've been following the bz discussion)

> +                       return is_mmconf_reserved(e820__mapped_all, cfg, dev,
> +                                                 "E820 entry");
> +
> +               return true;
> +       }

...

>         if (pci_mmcfg_running_state)
>                 return true;
>
> -       /* Don't try to do this check unless configuration
> -          type 1 is available. how about type 2 ?*/
> -       if (raw_pci_ops)
> -               return is_mmconf_reserved(e820__mapped_all, cfg, dev,
> -                                         "E820 entry");
> -
>         return false;

Not strictly related to this patch, but now it can simply

  return pci_mmcfg_running_state;


In any case, LGTM,
Reviewed-by: Andy Shevchenko <andy@kernel.org>
Bjorn Helgaas April 17, 2024, 9:09 p.m. UTC | #2
On Wed, Apr 17, 2024 at 11:47:27PM +0300, Andy Shevchenko wrote:
> On Wed, Apr 17, 2024 at 11:40 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > Reported-by: Mateusz Kaduk <mateusz.kaduk@gmail.com>
> 
> > Reported-by: Arul <...>
> > Reported-by: Imcarneiro91 <...>
> > Reported-by: Aman <...>
> 
> Isn't bugzilla public enough? You may take emails from there, no?

Good question.  I think email addresses in bugzilla are only visible
to logged-in users, so I wanted permission before publishing them.  I
got that from Mateusz and am hoping to hear from the others because I
want everybody to get credit for their contribution.

> > +               /*
> > +                * 946f2ee5c731 ("Check that MCFG points to an e820
> > +                * reserved area") added this E820 check in 2006 to work
> > +                * around BIOS defects.
> > +                *
> > +                * Per PCI Firmware r3.3, sec 4.1.2, ECAM space must be
> > +                * reserved by a PNP0C02 resource, but it need not be
> > +                * mentioned in E820.  Before the ACPI interpreter is
> > +                * available, we can't check for PNP0C02 resources, so
> > +                * there's no reliable way to verify the region in this
> > +                * early check.  Keep it only for the old machines that
> > +                * motivated 946f2ee5c731.
> > +                */
> 
> > +               if (dmi_get_bios_year() < 2016 && raw_pci_ops)
> 
> I probably missed something, but where does 2016 come from?
> (I've been following the bz discussion)

I made it up based on the fact that 946f2ee5c731 was added in 2006,
and I just added 10 years.  I would love to get rid of the E820 checks
altogether because they're really completely bogus and an ongoing
headache, but I don't know the details of the machines that
946f2ee5c731 fixed.  I'm open to other suggestions.  The Lenovo BIOS
is from 2023, so it would have to be something earlier than that:

  DMI: LENOVO 83AG/LNVNB161216, BIOS MHCN40WW 12/15/2023

> >         if (pci_mmcfg_running_state)
> >                 return true;
> >
> > -       /* Don't try to do this check unless configuration
> > -          type 1 is available. how about type 2 ?*/
> > -       if (raw_pci_ops)
> > -               return is_mmconf_reserved(e820__mapped_all, cfg, dev,
> > -                                         "E820 entry");
> > -
> >         return false;
> 
> Not strictly related to this patch, but now it can simply
> 
>   return pci_mmcfg_running_state;

Good point, changed locally.

> In any case, LGTM,
> Reviewed-by: Andy Shevchenko <andy@kernel.org>

Thanks!

Bjorn
Kuppuswamy Sathyanarayanan April 18, 2024, 3:10 a.m. UTC | #3
On 4/17/24 1:40 PM, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> Arul, Mateusz, Imcarneiro91, and Aman reported a regression caused by
> 07eab0901ede ("efi/x86: Remove EfiMemoryMappedIO from E820 map").  On the
> Lenovo Legion 9i laptop, that commit removes the area containing ECAM from
> E820, which means the early E820 validation started failing, which meant we
> didn't enable ECAM in the "early MCFG" path
>
> The lack of ECAM caused many ACPI methods to fail, resulting in the
> embedded controller, PS/2, audio, trackpad, and battery devices not being
> detected.  The _OSC method also failed, so Linux could not take control of
> the PCIe hotplug, PME, and AER features:
>
>   # pci_mmcfg_early_init()
>
>   PCI: ECAM [mem 0xc0000000-0xce0fffff] (base 0xc0000000) for domain 0000 [bus 00-e0]
>   PCI: not using ECAM ([mem 0xc0000000-0xce0fffff] not reserved)
>
>   ACPI Error: AE_ERROR, Returned by Handler for [PCI_Config] (20230628/evregion-300)
>   ACPI: Interpreter enabled
>   ACPI: Ignoring error and continuing table load
>   ACPI BIOS Error (bug): Could not resolve symbol [\_SB.PC00.RP01._SB.PC00], AE_NOT_FOUND (20230628/dswload2-162)
>   ACPI Error: AE_NOT_FOUND, During name lookup/catalog (20230628/psobject-220)
>   ACPI: Skipping parse of AML opcode: OpcodeName unavailable (0x0010)
>   ACPI BIOS Error (bug): Could not resolve symbol [\_SB.PC00.RP01._SB.PC00], AE_NOT_FOUND (20230628/dswload2-162)
>   ACPI Error: AE_NOT_FOUND, During name lookup/catalog (20230628/psobject-220)
>   ...
>   ACPI Error: Aborting method \_SB.PC00._OSC due to previous error (AE_NOT_FOUND) (20230628/psparse-529)
>   acpi PNP0A08:00: _OSC: platform retains control of PCIe features (AE_NOT_FOUND)
>
>   # pci_mmcfg_late_init()
>
>   PCI: ECAM [mem 0xc0000000-0xce0fffff] (base 0xc0000000) for domain 0000 [bus 00-e0]
>   PCI: [Firmware Info]: ECAM [mem 0xc0000000-0xce0fffff] not reserved in ACPI motherboard resources
>   PCI: ECAM [mem 0xc0000000-0xce0fffff] is EfiMemoryMappedIO; assuming valid
>   PCI: ECAM [mem 0xc0000000-0xce0fffff] reserved to work around lack of ACPI motherboard _CRS
>
> Per PCI Firmware r3.3, sec 4.1.2, ECAM space must be reserved by a PNP0C02
> resource, but it need not be mentioned in E820, so we shouldn't look at
> E820 to validate the ECAM space described by MCFG.
>
> 946f2ee5c731 ("[PATCH] i386/x86-64: Check that MCFG points to an e820
> reserved area") added a sanity check of E820 to work around buggy MCFG
> tables, but that over-aggressive validation causes failures like this one.
>
> Keep the E820 validation check only for older BIOSes (pre-2016) so the
> buggy 2006-era machines don't break.  Skip the early E820 check for 2016
> and newer BIOSes.
>
> Fixes: 07eab0901ede ("efi/x86: Remove EfiMemoryMappedIO from E820 map")
> Reported-by: Mateusz Kaduk <mateusz.kaduk@gmail.com>
> Reported-by: Arul <...>
> Reported-by: Imcarneiro91 <...>
> Reported-by: Aman <...>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218444
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> Tested-by: Mateusz Kaduk <mateusz.kaduk@gmail.com>
> Cc: stable@vger.kernel.org
> ---

LGTM

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

>  arch/x86/pci/mmconfig-shared.c | 35 +++++++++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
> index 0cc9520666ef..53c7afa606c3 100644
> --- a/arch/x86/pci/mmconfig-shared.c
> +++ b/arch/x86/pci/mmconfig-shared.c
> @@ -518,7 +518,34 @@ static bool __ref pci_mmcfg_reserved(struct device *dev,
>  {
>  	struct resource *conflict;
>  
> -	if (!early && !acpi_disabled) {
> +	if (early) {
> +
> +		/*
> +		 * Don't try to do this check unless configuration type 1
> +		 * is available.  How about type 2?

I don't understand why above question is included in the comment. Do
you think it is better to drop that part of the comment?

> +		 */
> +
> +		/*
> +		 * 946f2ee5c731 ("Check that MCFG points to an e820
> +		 * reserved area") added this E820 check in 2006 to work
> +		 * around BIOS defects.
> +		 *
> +		 * Per PCI Firmware r3.3, sec 4.1.2, ECAM space must be
> +		 * reserved by a PNP0C02 resource, but it need not be
> +		 * mentioned in E820.  Before the ACPI interpreter is
> +		 * available, we can't check for PNP0C02 resources, so
> +		 * there's no reliable way to verify the region in this
> +		 * early check.  Keep it only for the old machines that
> +		 * motivated 946f2ee5c731.
> +		 */
> +		if (dmi_get_bios_year() < 2016 && raw_pci_ops)
> +			return is_mmconf_reserved(e820__mapped_all, cfg, dev,
> +						  "E820 entry");
> +
> +		return true;
> +	}
> +
> +	if (!acpi_disabled) {
>  		if (is_mmconf_reserved(is_acpi_reserved, cfg, dev,
>  				       "ACPI motherboard resource"))
>  			return true;
> @@ -554,12 +581,6 @@ static bool __ref pci_mmcfg_reserved(struct device *dev,
>  	if (pci_mmcfg_running_state)
>  		return true;
>  
> -	/* Don't try to do this check unless configuration
> -	   type 1 is available. how about type 2 ?*/
> -	if (raw_pci_ops)
> -		return is_mmconf_reserved(e820__mapped_all, cfg, dev,
> -					  "E820 entry");
> -
>  	return false;
>  }
>
Hans de Goede April 18, 2024, 8:14 a.m. UTC | #4
Hi all,

On 4/17/24 10:40 PM, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Arul, Mateusz, Imcarneiro91, and Aman reported a regression caused by
> 07eab0901ede ("efi/x86: Remove EfiMemoryMappedIO from E820 map").  On the
> Lenovo Legion 9i laptop, that commit removes the area containing ECAM from
> E820, which means the early E820 validation started failing, which meant we
> didn't enable ECAM in the "early MCFG" path
> 
> The lack of ECAM caused many ACPI methods to fail, resulting in the
> embedded controller, PS/2, audio, trackpad, and battery devices not being
> detected.  The _OSC method also failed, so Linux could not take control of
> the PCIe hotplug, PME, and AER features:
> 
>   # pci_mmcfg_early_init()
> 
>   PCI: ECAM [mem 0xc0000000-0xce0fffff] (base 0xc0000000) for domain 0000 [bus 00-e0]
>   PCI: not using ECAM ([mem 0xc0000000-0xce0fffff] not reserved)
> 
>   ACPI Error: AE_ERROR, Returned by Handler for [PCI_Config] (20230628/evregion-300)
>   ACPI: Interpreter enabled
>   ACPI: Ignoring error and continuing table load
>   ACPI BIOS Error (bug): Could not resolve symbol [\_SB.PC00.RP01._SB.PC00], AE_NOT_FOUND (20230628/dswload2-162)
>   ACPI Error: AE_NOT_FOUND, During name lookup/catalog (20230628/psobject-220)
>   ACPI: Skipping parse of AML opcode: OpcodeName unavailable (0x0010)
>   ACPI BIOS Error (bug): Could not resolve symbol [\_SB.PC00.RP01._SB.PC00], AE_NOT_FOUND (20230628/dswload2-162)
>   ACPI Error: AE_NOT_FOUND, During name lookup/catalog (20230628/psobject-220)
>   ...
>   ACPI Error: Aborting method \_SB.PC00._OSC due to previous error (AE_NOT_FOUND) (20230628/psparse-529)
>   acpi PNP0A08:00: _OSC: platform retains control of PCIe features (AE_NOT_FOUND)
> 
>   # pci_mmcfg_late_init()
> 
>   PCI: ECAM [mem 0xc0000000-0xce0fffff] (base 0xc0000000) for domain 0000 [bus 00-e0]
>   PCI: [Firmware Info]: ECAM [mem 0xc0000000-0xce0fffff] not reserved in ACPI motherboard resources
>   PCI: ECAM [mem 0xc0000000-0xce0fffff] is EfiMemoryMappedIO; assuming valid
>   PCI: ECAM [mem 0xc0000000-0xce0fffff] reserved to work around lack of ACPI motherboard _CRS
> 
> Per PCI Firmware r3.3, sec 4.1.2, ECAM space must be reserved by a PNP0C02
> resource, but it need not be mentioned in E820, so we shouldn't look at
> E820 to validate the ECAM space described by MCFG.
> 
> 946f2ee5c731 ("[PATCH] i386/x86-64: Check that MCFG points to an e820
> reserved area") added a sanity check of E820 to work around buggy MCFG
> tables, but that over-aggressive validation causes failures like this one.
> 
> Keep the E820 validation check only for older BIOSes (pre-2016) so the
> buggy 2006-era machines don't break.  Skip the early E820 check for 2016
> and newer BIOSes.

I know a fix for this has been long in the making so I don't want to throw
a spanner into the works, but I wonder why is the is_efi_mmio() check inside
the if (!early && !acpi_disabled) {} block (before this patch) ?

is_efi_mmio() only relies on EFI memdescriptors and those are setup pretty
early. Assuming that the EFI memdescriptors are indeed setup before
pci_mmcfg_reserved(..., ..., early=true) gets called we could simply move
the is_efi_mmio(&cfg->res) outside (below) the if (!early && !acpi_disabled)
{} so that it always runs before the is_mmconf_reserved(e820__mapped_all, ...)
check.

Looking at the dmesg above the is_efi_mmio() check does succeed, so this
should fix the issue without needing a BIOS year check ?

Regards,

Hans





> Fixes: 07eab0901ede ("efi/x86: Remove EfiMemoryMappedIO from E820 map")
> Reported-by: Mateusz Kaduk <mateusz.kaduk@gmail.com>
> Reported-by: Arul <...>
> Reported-by: Imcarneiro91 <...>
> Reported-by: Aman <...>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218444
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> Tested-by: Mateusz Kaduk <mateusz.kaduk@gmail.com>
> Cc: stable@vger.kernel.org
> ---
>  arch/x86/pci/mmconfig-shared.c | 35 +++++++++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
> index 0cc9520666ef..53c7afa606c3 100644
> --- a/arch/x86/pci/mmconfig-shared.c
> +++ b/arch/x86/pci/mmconfig-shared.c
> @@ -518,7 +518,34 @@ static bool __ref pci_mmcfg_reserved(struct device *dev,
>  {
>  	struct resource *conflict;
>  
> -	if (!early && !acpi_disabled) {
> +	if (early) {
> +
> +		/*
> +		 * Don't try to do this check unless configuration type 1
> +		 * is available.  How about type 2?
> +		 */
> +
> +		/*
> +		 * 946f2ee5c731 ("Check that MCFG points to an e820
> +		 * reserved area") added this E820 check in 2006 to work
> +		 * around BIOS defects.
> +		 *
> +		 * Per PCI Firmware r3.3, sec 4.1.2, ECAM space must be
> +		 * reserved by a PNP0C02 resource, but it need not be
> +		 * mentioned in E820.  Before the ACPI interpreter is
> +		 * available, we can't check for PNP0C02 resources, so
> +		 * there's no reliable way to verify the region in this
> +		 * early check.  Keep it only for the old machines that
> +		 * motivated 946f2ee5c731.
> +		 */
> +		if (dmi_get_bios_year() < 2016 && raw_pci_ops)
> +			return is_mmconf_reserved(e820__mapped_all, cfg, dev,
> +						  "E820 entry");
> +
> +		return true;
> +	}
> +
> +	if (!acpi_disabled) {
>  		if (is_mmconf_reserved(is_acpi_reserved, cfg, dev,
>  				       "ACPI motherboard resource"))
>  			return true;
> @@ -554,12 +581,6 @@ static bool __ref pci_mmcfg_reserved(struct device *dev,
>  	if (pci_mmcfg_running_state)
>  		return true;
>  
> -	/* Don't try to do this check unless configuration
> -	   type 1 is available. how about type 2 ?*/
> -	if (raw_pci_ops)
> -		return is_mmconf_reserved(e820__mapped_all, cfg, dev,
> -					  "E820 entry");
> -
>  	return false;
>  }
>
Bjorn Helgaas April 18, 2024, 5:10 p.m. UTC | #5
On Wed, Apr 17, 2024 at 08:10:28PM -0700, Kuppuswamy Sathyanarayanan wrote:
> 
> On 4/17/24 1:40 PM, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> >
> > Arul, Mateusz, Imcarneiro91, and Aman reported a regression caused by
> > 07eab0901ede ("efi/x86: Remove EfiMemoryMappedIO from E820 map").  On the
> > Lenovo Legion 9i laptop, that commit removes the area containing ECAM from
> > E820, which means the early E820 validation started failing, which meant we
> > didn't enable ECAM in the "early MCFG" path
> ...

> LGTM
> 
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

Thanks for taking a look!

> > -	if (!early && !acpi_disabled) {
> > +	if (early) {
> > +
> > +		/*
> > +		 * Don't try to do this check unless configuration type 1
> > +		 * is available.  How about type 2?
> 
> I don't understand why above question is included in the comment. Do
> you think it is better to drop that part of the comment?

The "How about type 2?" questio was added by bb63b4219976 ("x86 pci:
remove checking type for mmconfig probe").  I only moved it and fixed
the capitalization and formatting.

> > -	/* Don't try to do this check unless configuration
> > -	   type 1 is available. how about type 2 ?*/
> > -	if (raw_pci_ops)
> > -		return is_mmconf_reserved(e820__mapped_all, cfg, dev,
> > -					  "E820 entry");
> > -
> >  	return false;
> >  }
Bjorn Helgaas April 18, 2024, 5:15 p.m. UTC | #6
On Thu, Apr 18, 2024 at 10:14:21AM +0200, Hans de Goede wrote:
> On 4/17/24 10:40 PM, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > Arul, Mateusz, Imcarneiro91, and Aman reported a regression caused by
> > 07eab0901ede ("efi/x86: Remove EfiMemoryMappedIO from E820 map").  On the
> > Lenovo Legion 9i laptop, that commit removes the area containing ECAM from
> > E820, which means the early E820 validation started failing, which meant we
> > didn't enable ECAM in the "early MCFG" path
> > 
> > The lack of ECAM caused many ACPI methods to fail, resulting in the
> > embedded controller, PS/2, audio, trackpad, and battery devices not being
> > detected.  The _OSC method also failed, so Linux could not take control of
> > the PCIe hotplug, PME, and AER features:
> > 
> >   # pci_mmcfg_early_init()
> > 
> >   PCI: ECAM [mem 0xc0000000-0xce0fffff] (base 0xc0000000) for domain 0000 [bus 00-e0]
> >   PCI: not using ECAM ([mem 0xc0000000-0xce0fffff] not reserved)
> > 
> >   ACPI Error: AE_ERROR, Returned by Handler for [PCI_Config] (20230628/evregion-300)
> >   ACPI: Interpreter enabled
> >   ACPI: Ignoring error and continuing table load
> >   ACPI BIOS Error (bug): Could not resolve symbol [\_SB.PC00.RP01._SB.PC00], AE_NOT_FOUND (20230628/dswload2-162)
> >   ACPI Error: AE_NOT_FOUND, During name lookup/catalog (20230628/psobject-220)
> >   ACPI: Skipping parse of AML opcode: OpcodeName unavailable (0x0010)
> >   ACPI BIOS Error (bug): Could not resolve symbol [\_SB.PC00.RP01._SB.PC00], AE_NOT_FOUND (20230628/dswload2-162)
> >   ACPI Error: AE_NOT_FOUND, During name lookup/catalog (20230628/psobject-220)
> >   ...
> >   ACPI Error: Aborting method \_SB.PC00._OSC due to previous error (AE_NOT_FOUND) (20230628/psparse-529)
> >   acpi PNP0A08:00: _OSC: platform retains control of PCIe features (AE_NOT_FOUND)
> > 
> >   # pci_mmcfg_late_init()
> > 
> >   PCI: ECAM [mem 0xc0000000-0xce0fffff] (base 0xc0000000) for domain 0000 [bus 00-e0]
> >   PCI: [Firmware Info]: ECAM [mem 0xc0000000-0xce0fffff] not reserved in ACPI motherboard resources
> >   PCI: ECAM [mem 0xc0000000-0xce0fffff] is EfiMemoryMappedIO; assuming valid
> >   PCI: ECAM [mem 0xc0000000-0xce0fffff] reserved to work around lack of ACPI motherboard _CRS
> > 
> > Per PCI Firmware r3.3, sec 4.1.2, ECAM space must be reserved by a PNP0C02
> > resource, but it need not be mentioned in E820, so we shouldn't look at
> > E820 to validate the ECAM space described by MCFG.
> > 
> > 946f2ee5c731 ("[PATCH] i386/x86-64: Check that MCFG points to an e820
> > reserved area") added a sanity check of E820 to work around buggy MCFG
> > tables, but that over-aggressive validation causes failures like this one.
> > 
> > Keep the E820 validation check only for older BIOSes (pre-2016) so the
> > buggy 2006-era machines don't break.  Skip the early E820 check for 2016
> > and newer BIOSes.
> 
> I know a fix for this has been long in the making so I don't want to throw
> a spanner into the works, but I wonder why is the is_efi_mmio() check inside
> the if (!early && !acpi_disabled) {} block (before this patch) ?
> 
> is_efi_mmio() only relies on EFI memdescriptors and those are setup pretty
> early. Assuming that the EFI memdescriptors are indeed setup before
> pci_mmcfg_reserved(..., ..., early=true) gets called we could simply move
> the is_efi_mmio(&cfg->res) outside (below) the if (!early && !acpi_disabled)
> {} so that it always runs before the is_mmconf_reserved(e820__mapped_all, ...)
> check.
> 
> Looking at the dmesg above the is_efi_mmio() check does succeed, so this
> should fix the issue without needing a BIOS year check ?

As far as I know there is no spec requirement that an area described
by MCFG appear in either the E820 map or the EFI map.

I would like to get away from relying on these things that the spec
doesn't require because they are so prone to breakage.

I would love to just get rid of this early usage of
pci_mmcfg_reserved() completely; I'm just afraid of breaking some
ancient 2006-era machine that still happens to be running.

Did I understand your question correctly?

Bjorn
Hans de Goede April 19, 2024, 7:25 a.m. UTC | #7
Hi Bjorn,

On 4/18/24 7:15 PM, Bjorn Helgaas wrote:
> On Thu, Apr 18, 2024 at 10:14:21AM +0200, Hans de Goede wrote:
>> On 4/17/24 10:40 PM, Bjorn Helgaas wrote:
>>> From: Bjorn Helgaas <bhelgaas@google.com>
>>>
>>> Arul, Mateusz, Imcarneiro91, and Aman reported a regression caused by
>>> 07eab0901ede ("efi/x86: Remove EfiMemoryMappedIO from E820 map").  On the
>>> Lenovo Legion 9i laptop, that commit removes the area containing ECAM from
>>> E820, which means the early E820 validation started failing, which meant we
>>> didn't enable ECAM in the "early MCFG" path
>>>
>>> The lack of ECAM caused many ACPI methods to fail, resulting in the
>>> embedded controller, PS/2, audio, trackpad, and battery devices not being
>>> detected.  The _OSC method also failed, so Linux could not take control of
>>> the PCIe hotplug, PME, and AER features:
>>>
>>>   # pci_mmcfg_early_init()
>>>
>>>   PCI: ECAM [mem 0xc0000000-0xce0fffff] (base 0xc0000000) for domain 0000 [bus 00-e0]
>>>   PCI: not using ECAM ([mem 0xc0000000-0xce0fffff] not reserved)
>>>
>>>   ACPI Error: AE_ERROR, Returned by Handler for [PCI_Config] (20230628/evregion-300)
>>>   ACPI: Interpreter enabled
>>>   ACPI: Ignoring error and continuing table load
>>>   ACPI BIOS Error (bug): Could not resolve symbol [\_SB.PC00.RP01._SB.PC00], AE_NOT_FOUND (20230628/dswload2-162)
>>>   ACPI Error: AE_NOT_FOUND, During name lookup/catalog (20230628/psobject-220)
>>>   ACPI: Skipping parse of AML opcode: OpcodeName unavailable (0x0010)
>>>   ACPI BIOS Error (bug): Could not resolve symbol [\_SB.PC00.RP01._SB.PC00], AE_NOT_FOUND (20230628/dswload2-162)
>>>   ACPI Error: AE_NOT_FOUND, During name lookup/catalog (20230628/psobject-220)
>>>   ...
>>>   ACPI Error: Aborting method \_SB.PC00._OSC due to previous error (AE_NOT_FOUND) (20230628/psparse-529)
>>>   acpi PNP0A08:00: _OSC: platform retains control of PCIe features (AE_NOT_FOUND)
>>>
>>>   # pci_mmcfg_late_init()
>>>
>>>   PCI: ECAM [mem 0xc0000000-0xce0fffff] (base 0xc0000000) for domain 0000 [bus 00-e0]
>>>   PCI: [Firmware Info]: ECAM [mem 0xc0000000-0xce0fffff] not reserved in ACPI motherboard resources
>>>   PCI: ECAM [mem 0xc0000000-0xce0fffff] is EfiMemoryMappedIO; assuming valid
>>>   PCI: ECAM [mem 0xc0000000-0xce0fffff] reserved to work around lack of ACPI motherboard _CRS
>>>
>>> Per PCI Firmware r3.3, sec 4.1.2, ECAM space must be reserved by a PNP0C02
>>> resource, but it need not be mentioned in E820, so we shouldn't look at
>>> E820 to validate the ECAM space described by MCFG.
>>>
>>> 946f2ee5c731 ("[PATCH] i386/x86-64: Check that MCFG points to an e820
>>> reserved area") added a sanity check of E820 to work around buggy MCFG
>>> tables, but that over-aggressive validation causes failures like this one.
>>>
>>> Keep the E820 validation check only for older BIOSes (pre-2016) so the
>>> buggy 2006-era machines don't break.  Skip the early E820 check for 2016
>>> and newer BIOSes.
>>
>> I know a fix for this has been long in the making so I don't want to throw
>> a spanner into the works, but I wonder why is the is_efi_mmio() check inside
>> the if (!early && !acpi_disabled) {} block (before this patch) ?
>>
>> is_efi_mmio() only relies on EFI memdescriptors and those are setup pretty
>> early. Assuming that the EFI memdescriptors are indeed setup before
>> pci_mmcfg_reserved(..., ..., early=true) gets called we could simply move
>> the is_efi_mmio(&cfg->res) outside (below) the if (!early && !acpi_disabled)
>> {} so that it always runs before the is_mmconf_reserved(e820__mapped_all, ...)
>> check.
>>
>> Looking at the dmesg above the is_efi_mmio() check does succeed, so this
>> should fix the issue without needing a BIOS year check ?
> 
> As far as I know there is no spec requirement that an area described
> by MCFG appear in either the E820 map or the EFI map.
> 
> I would like to get away from relying on these things that the spec
> doesn't require because they are so prone to breakage.
> 
> I would love to just get rid of this early usage of
> pci_mmcfg_reserved() completely; I'm just afraid of breaking some
> ancient 2006-era machine that still happens to be running.

Ok, yes if you want to move away from the early checks at all
then adding the BIOS year check makes sense.

With that clarified the patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans
Bjorn Helgaas May 2, 2024, 9:43 p.m. UTC | #8
On Wed, Apr 17, 2024 at 03:40:12PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Arul, Mateusz, Imcarneiro91, and Aman reported a regression caused by
> 07eab0901ede ("efi/x86: Remove EfiMemoryMappedIO from E820 map").  On the
> Lenovo Legion 9i laptop, that commit removes the area containing ECAM from
> E820, which means the early E820 validation started failing, which meant we
> didn't enable ECAM in the "early MCFG" path
> 
> The lack of ECAM caused many ACPI methods to fail, resulting in the
> embedded controller, PS/2, audio, trackpad, and battery devices not being
> detected.  The _OSC method also failed, so Linux could not take control of
> the PCIe hotplug, PME, and AER features:
> 
>   # pci_mmcfg_early_init()
> 
>   PCI: ECAM [mem 0xc0000000-0xce0fffff] (base 0xc0000000) for domain 0000 [bus 00-e0]
>   PCI: not using ECAM ([mem 0xc0000000-0xce0fffff] not reserved)
> 
>   ACPI Error: AE_ERROR, Returned by Handler for [PCI_Config] (20230628/evregion-300)
>   ACPI: Interpreter enabled
>   ACPI: Ignoring error and continuing table load
>   ACPI BIOS Error (bug): Could not resolve symbol [\_SB.PC00.RP01._SB.PC00], AE_NOT_FOUND (20230628/dswload2-162)
>   ACPI Error: AE_NOT_FOUND, During name lookup/catalog (20230628/psobject-220)
>   ACPI: Skipping parse of AML opcode: OpcodeName unavailable (0x0010)
>   ACPI BIOS Error (bug): Could not resolve symbol [\_SB.PC00.RP01._SB.PC00], AE_NOT_FOUND (20230628/dswload2-162)
>   ACPI Error: AE_NOT_FOUND, During name lookup/catalog (20230628/psobject-220)
>   ...
>   ACPI Error: Aborting method \_SB.PC00._OSC due to previous error (AE_NOT_FOUND) (20230628/psparse-529)
>   acpi PNP0A08:00: _OSC: platform retains control of PCIe features (AE_NOT_FOUND)
> 
>   # pci_mmcfg_late_init()
> 
>   PCI: ECAM [mem 0xc0000000-0xce0fffff] (base 0xc0000000) for domain 0000 [bus 00-e0]
>   PCI: [Firmware Info]: ECAM [mem 0xc0000000-0xce0fffff] not reserved in ACPI motherboard resources
>   PCI: ECAM [mem 0xc0000000-0xce0fffff] is EfiMemoryMappedIO; assuming valid
>   PCI: ECAM [mem 0xc0000000-0xce0fffff] reserved to work around lack of ACPI motherboard _CRS
> 
> Per PCI Firmware r3.3, sec 4.1.2, ECAM space must be reserved by a PNP0C02
> resource, but it need not be mentioned in E820, so we shouldn't look at
> E820 to validate the ECAM space described by MCFG.
> 
> 946f2ee5c731 ("[PATCH] i386/x86-64: Check that MCFG points to an e820
> reserved area") added a sanity check of E820 to work around buggy MCFG
> tables, but that over-aggressive validation causes failures like this one.
> 
> Keep the E820 validation check only for older BIOSes (pre-2016) so the
> buggy 2006-era machines don't break.  Skip the early E820 check for 2016
> and newer BIOSes.
> 
> Fixes: 07eab0901ede ("efi/x86: Remove EfiMemoryMappedIO from E820 map")
> Reported-by: Mateusz Kaduk <mateusz.kaduk@gmail.com>
> Reported-by: Arul <...>
> Reported-by: Imcarneiro91 <...>
> Reported-by: Aman <...>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218444
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> Tested-by: Mateusz Kaduk <mateusz.kaduk@gmail.com>
> Cc: stable@vger.kernel.org

I applied this to pci/enumeration for v6.10, thanks everybody for
your testing and review.

> ---
>  arch/x86/pci/mmconfig-shared.c | 35 +++++++++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
> index 0cc9520666ef..53c7afa606c3 100644
> --- a/arch/x86/pci/mmconfig-shared.c
> +++ b/arch/x86/pci/mmconfig-shared.c
> @@ -518,7 +518,34 @@ static bool __ref pci_mmcfg_reserved(struct device *dev,
>  {
>  	struct resource *conflict;
>  
> -	if (!early && !acpi_disabled) {
> +	if (early) {
> +
> +		/*
> +		 * Don't try to do this check unless configuration type 1
> +		 * is available.  How about type 2?
> +		 */
> +
> +		/*
> +		 * 946f2ee5c731 ("Check that MCFG points to an e820
> +		 * reserved area") added this E820 check in 2006 to work
> +		 * around BIOS defects.
> +		 *
> +		 * Per PCI Firmware r3.3, sec 4.1.2, ECAM space must be
> +		 * reserved by a PNP0C02 resource, but it need not be
> +		 * mentioned in E820.  Before the ACPI interpreter is
> +		 * available, we can't check for PNP0C02 resources, so
> +		 * there's no reliable way to verify the region in this
> +		 * early check.  Keep it only for the old machines that
> +		 * motivated 946f2ee5c731.
> +		 */
> +		if (dmi_get_bios_year() < 2016 && raw_pci_ops)
> +			return is_mmconf_reserved(e820__mapped_all, cfg, dev,
> +						  "E820 entry");
> +
> +		return true;
> +	}
> +
> +	if (!acpi_disabled) {
>  		if (is_mmconf_reserved(is_acpi_reserved, cfg, dev,
>  				       "ACPI motherboard resource"))
>  			return true;
> @@ -554,12 +581,6 @@ static bool __ref pci_mmcfg_reserved(struct device *dev,
>  	if (pci_mmcfg_running_state)
>  		return true;
>  
> -	/* Don't try to do this check unless configuration
> -	   type 1 is available. how about type 2 ?*/
> -	if (raw_pci_ops)
> -		return is_mmconf_reserved(e820__mapped_all, cfg, dev,
> -					  "E820 entry");
> -
>  	return false;
>  }
>  
> -- 
> 2.34.1
>
diff mbox series

Patch

diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
index 0cc9520666ef..53c7afa606c3 100644
--- a/arch/x86/pci/mmconfig-shared.c
+++ b/arch/x86/pci/mmconfig-shared.c
@@ -518,7 +518,34 @@  static bool __ref pci_mmcfg_reserved(struct device *dev,
 {
 	struct resource *conflict;
 
-	if (!early && !acpi_disabled) {
+	if (early) {
+
+		/*
+		 * Don't try to do this check unless configuration type 1
+		 * is available.  How about type 2?
+		 */
+
+		/*
+		 * 946f2ee5c731 ("Check that MCFG points to an e820
+		 * reserved area") added this E820 check in 2006 to work
+		 * around BIOS defects.
+		 *
+		 * Per PCI Firmware r3.3, sec 4.1.2, ECAM space must be
+		 * reserved by a PNP0C02 resource, but it need not be
+		 * mentioned in E820.  Before the ACPI interpreter is
+		 * available, we can't check for PNP0C02 resources, so
+		 * there's no reliable way to verify the region in this
+		 * early check.  Keep it only for the old machines that
+		 * motivated 946f2ee5c731.
+		 */
+		if (dmi_get_bios_year() < 2016 && raw_pci_ops)
+			return is_mmconf_reserved(e820__mapped_all, cfg, dev,
+						  "E820 entry");
+
+		return true;
+	}
+
+	if (!acpi_disabled) {
 		if (is_mmconf_reserved(is_acpi_reserved, cfg, dev,
 				       "ACPI motherboard resource"))
 			return true;
@@ -554,12 +581,6 @@  static bool __ref pci_mmcfg_reserved(struct device *dev,
 	if (pci_mmcfg_running_state)
 		return true;
 
-	/* Don't try to do this check unless configuration
-	   type 1 is available. how about type 2 ?*/
-	if (raw_pci_ops)
-		return is_mmconf_reserved(e820__mapped_all, cfg, dev,
-					  "E820 entry");
-
 	return false;
 }