Received-SPF: pass (google.com: domain of linux-kernel+bounces-150557-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Thu, 18 Apr 2024 12:15:54 -0500
From: Bjorn Helgaas <helgaas@kernel.org>
To: Hans de Goede <hdegoede@redhat.com>
Cc: linux-pci@vger.kernel.org, Mateusz Kaduk <mateusz.kaduk@gmail.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>, Tj <linux@iam.tj>,
	Andy Shevchenko <andy.shevchenko@gmail.com>, x86@kernel.org,
	linux-kernel@vger.kernel.org, Bjorn Helgaas <bhelgaas@google.com>,
	stable@vger.kernel.org
Subject: Re: [PATCH 1/1] x86/pci: Skip early E820 check for ECAM region
Message-ID: <20240418171554.GA243364@bhelgaas>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <c1897030-77a4-416d-86c4-6d4d560e5fd8@redhat.com>

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