[PATCH 0/3] x86/PCI: Clip only partial E820 overlaps

[PATCH 0/3] x86/PCI: Clip only partial E820 overlaps

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

This is based on Hans' extensive debugging and patch at
https://lore.kernel.org/r/20220228105259.230903-1-hdegoede@redhat.com
and applies on 7e57714cd0ad ("Linux 5.17-rc6").

This is basically the same idea (applying the 4dc2287c1805 workaround only
when an E820 region *partially* overlaps a host bridge window), but I think
it's a little simpler.

This also adds a little dmesg output when clipping, which should make
future debugging easier.

I bcc'd several folks who didn't have public email addresses in the RedHat
bugzilla or Launchpad.  If you review or test this, I'd be happy to
acknowledge that.

Bjorn Helgaas (3):
  x86/PCI: Eliminate remove_e820_regions() common subexpressions
  x86/PCI: Log host bridge window clipping for E820 regions
  x86/PCI: Preserve host bridge windows completely covered by E820

 arch/x86/include/asm/e820/api.h |  5 +++++
 arch/x86/kernel/resource.c      | 34 ++++++++++++++++++++++++++-------
 arch/x86/pci/acpi.c             |  5 +++++
 3 files changed, 37 insertions(+), 7 deletions(-)

-- 
2.25.1

[PATCH 1/3] x86/PCI: Eliminate remove_e820_regions() common subexpressions

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Add local variables to reduce repetition later.  No functional change
intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 arch/x86/kernel/resource.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/resource.c b/arch/x86/kernel/resource.c
index 9b9fb7882c20..8ffe68437744 100644
--- a/arch/x86/kernel/resource.c
+++ b/arch/x86/kernel/resource.c
@@ -27,12 +27,14 @@ static void remove_e820_regions(struct resource *avail)
 {
 	int i;
 	struct e820_entry *entry;
+	u64 e820_start, e820_end;
 
 	for (i = 0; i < e820_table->nr_entries; i++) {
 		entry = &e820_table->entries[i];
+		e820_start = entry->addr;
+		e820_end = entry->addr + entry->size - 1;
 
-		resource_clip(avail, entry->addr,
-			      entry->addr + entry->size - 1);
+		resource_clip(avail, e820_start, e820_end);
 	}
 }
 
-- 
2.25.1

[PATCH 2/3] x86/PCI: Log host bridge window clipping for E820 regions

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

ACPI firmware advertises PCI host bridge resources via PNP0A03 _CRS
methods.  Some BIOSes include non-window address space in _CRS, and if we
allocate that non-window space for PCI devices, they don't work.

4dc2287c1805 ("x86: avoid E820 regions when allocating address space")
works around this issue by clipping out any regions mentioned in the E820
table in the allocate_resource() path, but the implementation has several
issues:

  - The clipping is done for *all* allocations, not just those for PCI
    address space,

  - The clipping is done at each allocation instead of being done once when
    setting up the host bridge windows, and

  - The host bridge windows logged in dmesg do not reflect the clipping,
    and in fact there is *no* indication in dmesg, which complicates
    debugging.

Rework the implementation so we only clip PCI host bridge windows, we do it
once when setting them up, we a log message when a window is clipped, and
we reflect the clip when printing the host bridge windows.

I intend this only to improve the logging, not to fix any issues.

Example output changes:

    BIOS-e820: [mem 0x00000000b0000000-0x00000000c00fffff] reserved
  + acpi PNP0A08:00: clipped [mem 0xc0000000-0xfebfffff window] to [mem 0xc0100000-0xfebfffff window] for e820 entry [mem 0xb0000000-0xc00fffff]
  - pci_bus 0000:00: root bus resource [mem 0xc0000000-0xfebfffff window]
  + pci_bus 0000:00: root bus resource [mem 0xc0100000-0xfebfffff window]

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 arch/x86/include/asm/e820/api.h |  5 +++++
 arch/x86/kernel/resource.c      | 17 ++++++++++++-----
 arch/x86/pci/acpi.c             |  5 +++++
 3 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/e820/api.h b/arch/x86/include/asm/e820/api.h
index e8f58ddd06d9..5a39ed59b6db 100644
--- a/arch/x86/include/asm/e820/api.h
+++ b/arch/x86/include/asm/e820/api.h
@@ -4,6 +4,9 @@
 
 #include <asm/e820/types.h>
 
+struct device;
+struct resource;
+
 extern struct e820_table *e820_table;
 extern struct e820_table *e820_table_kexec;
 extern struct e820_table *e820_table_firmware;
@@ -43,6 +46,8 @@ extern void e820__register_nosave_regions(unsigned long limit_pfn);
 
 extern int  e820__get_entry_type(u64 start, u64 end);
 
+extern void remove_e820_regions(struct device *dev, struct resource *avail);
+
 /*
  * Returns true iff the specified range [start,end) is completely contained inside
  * the ISA region.
diff --git a/arch/x86/kernel/resource.c b/arch/x86/kernel/resource.c
index 8ffe68437744..7378ea146976 100644
--- a/arch/x86/kernel/resource.c
+++ b/arch/x86/kernel/resource.c
@@ -1,4 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0
+#include <linux/dev_printk.h>
 #include <linux/ioport.h>
 #include <asm/e820/api.h>
 
@@ -23,18 +24,27 @@ static void resource_clip(struct resource *res, resource_size_t start,
 		res->start = end + 1;
 }
 
-static void remove_e820_regions(struct resource *avail)
+void remove_e820_regions(struct device *dev, struct resource *avail)
 {
+	struct resource orig = *avail;
 	int i;
 	struct e820_entry *entry;
 	u64 e820_start, e820_end;
 
+	if (!(avail->flags & IORESOURCE_MEM))
+		return;
+
 	for (i = 0; i < e820_table->nr_entries; i++) {
 		entry = &e820_table->entries[i];
 		e820_start = entry->addr;
 		e820_end = entry->addr + entry->size - 1;
 
 		resource_clip(avail, e820_start, e820_end);
+		if (orig.start != avail->start || orig.end != avail->end) {
+			dev_info(dev, "clipped %pR to %pR for e820 entry [mem %#010Lx-%#010Lx]\n",
+				 &orig, avail, e820_start, e820_end);
+			orig = *avail;
+		}
 	}
 }
 
@@ -45,9 +55,6 @@ void arch_remove_reservations(struct resource *avail)
 	 * the low 1MB unconditionally, as this area is needed for some ISA
 	 * cards requiring a memory range, e.g. the i82365 PCMCIA controller.
 	 */
-	if (avail->flags & IORESOURCE_MEM) {
+	if (avail->flags & IORESOURCE_MEM)
 		resource_clip(avail, BIOS_ROM_BASE, BIOS_ROM_END);
-
-		remove_e820_regions(avail);
-	}
 }
diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
index 052f1d78a562..562c81a51ea0 100644
--- a/arch/x86/pci/acpi.c
+++ b/arch/x86/pci/acpi.c
@@ -8,6 +8,7 @@
 #include <linux/pci-acpi.h>
 #include <asm/numa.h>
 #include <asm/pci_x86.h>
+#include <asm/e820/api.h>
 
 struct pci_root_info {
 	struct acpi_pci_root_info common;
@@ -299,6 +300,10 @@ static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
 	int status;
 
 	status = acpi_pci_probe_root_resources(ci);
+
+	resource_list_for_each_entry(entry, &ci->resources)
+		remove_e820_regions(&device->dev, entry->res);
+
 	if (pci_use_crs) {
 		resource_list_for_each_entry_safe(entry, tmp, &ci->resources)
 			if (resource_is_pcicfg_ioport(entry->res))
-- 
2.25.1

[PATCH 3/3] x86/PCI: Preserve host bridge windows completely covered by E820

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Many folks have reported PCI devices not working.  It could affect any
device, but most reports are for Thunderbolt controllers on Lenovo Yoga and
Clevo Barebone laptops and the touchpad on Lenovo IdeaPads.

In every report, a region in the E820 table entirely encloses a PCI host
bridge window from _CRS, and because of 4dc2287c1805 ("x86: avoid E820
regions when allocating address space"), we ignore the entire window,
preventing us from assigning space to PCI devices.

For example, the dmesg log [2] from bug report [1] shows:

  BIOS-e820: [mem 0x000000004bc50000-0x00000000cfffffff] reserved
  pci_bus 0000:00: root bus resource [mem 0x65400000-0xbfffffff window]
  pci 0000:00:15.0: BAR 0: no space for [mem size 0x00001000 64bit]

The efi=debug dmesg log [3] from the same report shows the EFI memory map
entries that created the E820 map:

  efi: mem47: [Reserved |   |WB|WT|WC|UC] range=[0x4bc50000-0x5fffffff]
  efi: mem48: [Reserved |   |WB|  |  |UC] range=[0x60000000-0x60ffffff]
  efi: mem49: [Reserved |   |  |  |  |  ] range=[0x61000000-0x653fffff]
  efi: mem50: [MMIO     |RUN|  |  |  |UC] range=[0x65400000-0xcfffffff]

4dc2287c1805 ("x86: avoid E820 regions when allocating address space")
works around issues where _CRS contains non-window address space that can't
be used for PCI devices.  It does this by removing E820 regions from host
bridge windows.  But in these reports, the E820 region covers the entire
window, so 4dc2287c1805 makes it completely unusable.

Per UEFI v2.8, sec 7.2, the EfiMemoryMappedIO type means:

  Used by system firmware to request that a memory-mapped IO region be
  mapped by the OS to a virtual address so it can be accessed by EFI
  runtime services.

A host bridge window is definitely a memory-mapped IO region, and EFI
runtime services may need to access it, so I don't think we can argue that
this is a firmware defect.

Instead, change the 4dc2287c1805 strategy so it only removes E820 regions
when they overlap *part* of a host bridge window on the assumption that a
partial overlap is really register space, not part of the window proper.

If an E820 region covers the entire window from _CRS, assume the _CRS
window is correct and do nothing.

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1868899
[2] https://bugzilla.redhat.com/attachment.cgi?id=1711424
[3] https://bugzilla.redhat.com/attachment.cgi?id=1861407

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=206459
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=214259
BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1868899
BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1871793
BugLink: https://bugs.launchpad.net/bugs/1878279
BugLink: https://bugs.launchpad.net/bugs/1931715
BugLink: https://bugs.launchpad.net/bugs/1932069
BugLink: https://bugs.launchpad.net/bugs/1921649
Fixes: 4dc2287c1805 ("x86: avoid E820 regions when allocating address space")
Link: https://lore.kernel.org/r/20220228105259.230903-1-hdegoede@redhat.com
Based-on-patch-by: Hans de Goede <hdegoede@redhat.com>
Reported-by: Benoit Grégoire <benoitg@coeus.ca>   # BZ 206459
Reported-by: wse@tuxedocomputers.com              # BZ 214259
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 arch/x86/kernel/resource.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/x86/kernel/resource.c b/arch/x86/kernel/resource.c
index 7378ea146976..405f0af53e3d 100644
--- a/arch/x86/kernel/resource.c
+++ b/arch/x86/kernel/resource.c
@@ -39,6 +39,17 @@ void remove_e820_regions(struct device *dev, struct resource *avail)
 		e820_start = entry->addr;
 		e820_end = entry->addr + entry->size - 1;
 
+		/*
+		 * If an E820 entry covers just part of the resource, we
+		 * assume E820 is telling us about something like host
+		 * bridge register space that is unavailable for PCI
+		 * devices.  But if it covers the *entire* resource, it's
+		 * more likely just telling us that this is MMIO space, and
+		 * that doesn't need to be removed.
+		 */
+		if (e820_start <= avail->start && avail->end <= e820_end)
+			continue;
+
 		resource_clip(avail, e820_start, e820_end);
 		if (orig.start != avail->start || orig.end != avail->end) {
 			dev_info(dev, "clipped %pR to %pR for e820 entry [mem %#010Lx-%#010Lx]\n",
-- 
2.25.1

Re: [PATCH 3/3] x86/PCI: Preserve host bridge windows completely covered by E820

[Hans de Goede]

Hi Bjorn,

On 3/4/22 04:51, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Many folks have reported PCI devices not working.  It could affect any
> device, but most reports are for Thunderbolt controllers on Lenovo Yoga and
> Clevo Barebone laptops and the touchpad on Lenovo IdeaPads.
> 
> In every report, a region in the E820 table entirely encloses a PCI host
> bridge window from _CRS, and because of 4dc2287c1805 ("x86: avoid E820
> regions when allocating address space"), we ignore the entire window,
> preventing us from assigning space to PCI devices.
> 
> For example, the dmesg log [2] from bug report [1] shows:
> 
>   BIOS-e820: [mem 0x000000004bc50000-0x00000000cfffffff] reserved
>   pci_bus 0000:00: root bus resource [mem 0x65400000-0xbfffffff window]
>   pci 0000:00:15.0: BAR 0: no space for [mem size 0x00001000 64bit]
> 
> The efi=debug dmesg log [3] from the same report shows the EFI memory map
> entries that created the E820 map:
> 
>   efi: mem47: [Reserved |   |WB|WT|WC|UC] range=[0x4bc50000-0x5fffffff]
>   efi: mem48: [Reserved |   |WB|  |  |UC] range=[0x60000000-0x60ffffff]
>   efi: mem49: [Reserved |   |  |  |  |  ] range=[0x61000000-0x653fffff]
>   efi: mem50: [MMIO     |RUN|  |  |  |UC] range=[0x65400000-0xcfffffff]
> 
> 4dc2287c1805 ("x86: avoid E820 regions when allocating address space")
> works around issues where _CRS contains non-window address space that can't
> be used for PCI devices.  It does this by removing E820 regions from host
> bridge windows.  But in these reports, the E820 region covers the entire
> window, so 4dc2287c1805 makes it completely unusable.
> 
> Per UEFI v2.8, sec 7.2, the EfiMemoryMappedIO type means:
> 
>   Used by system firmware to request that a memory-mapped IO region be
>   mapped by the OS to a virtual address so it can be accessed by EFI
>   runtime services.
> 
> A host bridge window is definitely a memory-mapped IO region, and EFI
> runtime services may need to access it, so I don't think we can argue that
> this is a firmware defect.
> 
> Instead, change the 4dc2287c1805 strategy so it only removes E820 regions
> when they overlap *part* of a host bridge window on the assumption that a
> partial overlap is really register space, not part of the window proper.
> 
> If an E820 region covers the entire window from _CRS, assume the _CRS
> window is correct and do nothing.
> 
> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1868899
> [2] https://bugzilla.redhat.com/attachment.cgi?id=1711424
> [3] https://bugzilla.redhat.com/attachment.cgi?id=1861407
> 
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=206459
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=214259
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1868899
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1871793
> BugLink: https://bugs.launchpad.net/bugs/1878279
> BugLink: https://bugs.launchpad.net/bugs/1931715
> BugLink: https://bugs.launchpad.net/bugs/1932069
> BugLink: https://bugs.launchpad.net/bugs/1921649
> Fixes: 4dc2287c1805 ("x86: avoid E820 regions when allocating address space")
> Link: https://lore.kernel.org/r/20220228105259.230903-1-hdegoede@redhat.com
> Based-on-patch-by: Hans de Goede <hdegoede@redhat.com>
> Reported-by: Benoit Grégoire <benoitg@coeus.ca>   # BZ 206459
> Reported-by: wse@tuxedocomputers.com              # BZ 214259
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  arch/x86/kernel/resource.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/x86/kernel/resource.c b/arch/x86/kernel/resource.c
> index 7378ea146976..405f0af53e3d 100644
> --- a/arch/x86/kernel/resource.c
> +++ b/arch/x86/kernel/resource.c
> @@ -39,6 +39,17 @@ void remove_e820_regions(struct device *dev, struct resource *avail)
>  		e820_start = entry->addr;
>  		e820_end = entry->addr + entry->size - 1;
>  
> +		/*
> +		 * If an E820 entry covers just part of the resource, we
> +		 * assume E820 is telling us about something like host
> +		 * bridge register space that is unavailable for PCI
> +		 * devices.  But if it covers the *entire* resource, it's
> +		 * more likely just telling us that this is MMIO space, and
> +		 * that doesn't need to be removed.
> +		 */
> +		if (e820_start <= avail->start && avail->end <= e820_end)
> +			continue;
> +

IMHO it would be good to add some logging here, since hitting this is
somewhat of a special case. For the Fedora test kernels I did I changed
this to:

		if (e820_start <= avail->start && avail->end <= e820_end) {
			dev_info(dev, "resource %pR fully covered by e820 entry [mem %#010Lx-%#010Lx]\n",
				 avail, e820_start, e820_end);
			continue;
		}

And I expect/hope to see this new info message on the ideapad with the
touchpad issue.

Regards,

Hans



>  		resource_clip(avail, e820_start, e820_end);
>  		if (orig.start != avail->start || orig.end != avail->end) {
>  			dev_info(dev, "clipped %pR to %pR for e820 entry [mem %#010Lx-%#010Lx]\n",

Re: [PATCH 3/3] x86/PCI: Preserve host bridge windows completely covered by E820

[Bjorn Helgaas]

On Fri, Mar 04, 2022 at 03:16:42PM +0100, Hans de Goede wrote:
> Hi Bjorn,
> 
> On 3/4/22 04:51, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > Many folks have reported PCI devices not working.  It could affect any
> > device, but most reports are for Thunderbolt controllers on Lenovo Yoga and
> > Clevo Barebone laptops and the touchpad on Lenovo IdeaPads.
> > 
> > In every report, a region in the E820 table entirely encloses a PCI host
> > bridge window from _CRS, and because of 4dc2287c1805 ("x86: avoid E820
> > regions when allocating address space"), we ignore the entire window,
> > preventing us from assigning space to PCI devices.
> > 
> > For example, the dmesg log [2] from bug report [1] shows:
> > 
> >   BIOS-e820: [mem 0x000000004bc50000-0x00000000cfffffff] reserved
> >   pci_bus 0000:00: root bus resource [mem 0x65400000-0xbfffffff window]
> >   pci 0000:00:15.0: BAR 0: no space for [mem size 0x00001000 64bit]
> > 
> > The efi=debug dmesg log [3] from the same report shows the EFI memory map
> > entries that created the E820 map:
> > 
> >   efi: mem47: [Reserved |   |WB|WT|WC|UC] range=[0x4bc50000-0x5fffffff]
> >   efi: mem48: [Reserved |   |WB|  |  |UC] range=[0x60000000-0x60ffffff]
> >   efi: mem49: [Reserved |   |  |  |  |  ] range=[0x61000000-0x653fffff]
> >   efi: mem50: [MMIO     |RUN|  |  |  |UC] range=[0x65400000-0xcfffffff]
> > 
> > 4dc2287c1805 ("x86: avoid E820 regions when allocating address space")
> > works around issues where _CRS contains non-window address space that can't
> > be used for PCI devices.  It does this by removing E820 regions from host
> > bridge windows.  But in these reports, the E820 region covers the entire
> > window, so 4dc2287c1805 makes it completely unusable.
> > 
> > Per UEFI v2.8, sec 7.2, the EfiMemoryMappedIO type means:
> > 
> >   Used by system firmware to request that a memory-mapped IO region be
> >   mapped by the OS to a virtual address so it can be accessed by EFI
> >   runtime services.
> > 
> > A host bridge window is definitely a memory-mapped IO region, and EFI
> > runtime services may need to access it, so I don't think we can argue that
> > this is a firmware defect.
> > 
> > Instead, change the 4dc2287c1805 strategy so it only removes E820 regions
> > when they overlap *part* of a host bridge window on the assumption that a
> > partial overlap is really register space, not part of the window proper.
> > 
> > If an E820 region covers the entire window from _CRS, assume the _CRS
> > window is correct and do nothing.
> > 
> > [1] https://bugzilla.redhat.com/show_bug.cgi?id=1868899
> > [2] https://bugzilla.redhat.com/attachment.cgi?id=1711424
> > [3] https://bugzilla.redhat.com/attachment.cgi?id=1861407
> > 
> > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=206459
> > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=214259
> > BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1868899
> > BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1871793
> > BugLink: https://bugs.launchpad.net/bugs/1878279
> > BugLink: https://bugs.launchpad.net/bugs/1931715
> > BugLink: https://bugs.launchpad.net/bugs/1932069
> > BugLink: https://bugs.launchpad.net/bugs/1921649
> > Fixes: 4dc2287c1805 ("x86: avoid E820 regions when allocating address space")
> > Link: https://lore.kernel.org/r/20220228105259.230903-1-hdegoede@redhat.com
> > Based-on-patch-by: Hans de Goede <hdegoede@redhat.com>
> > Reported-by: Benoit Grégoire <benoitg@coeus.ca>   # BZ 206459
> > Reported-by: wse@tuxedocomputers.com              # BZ 214259
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >  arch/x86/kernel/resource.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/arch/x86/kernel/resource.c b/arch/x86/kernel/resource.c
> > index 7378ea146976..405f0af53e3d 100644
> > --- a/arch/x86/kernel/resource.c
> > +++ b/arch/x86/kernel/resource.c
> > @@ -39,6 +39,17 @@ void remove_e820_regions(struct device *dev, struct resource *avail)
> >  		e820_start = entry->addr;
> >  		e820_end = entry->addr + entry->size - 1;
> >  
> > +		/*
> > +		 * If an E820 entry covers just part of the resource, we
> > +		 * assume E820 is telling us about something like host
> > +		 * bridge register space that is unavailable for PCI
> > +		 * devices.  But if it covers the *entire* resource, it's
> > +		 * more likely just telling us that this is MMIO space, and
> > +		 * that doesn't need to be removed.
> > +		 */
> > +		if (e820_start <= avail->start && avail->end <= e820_end)
> > +			continue;
> > +
> 
> IMHO it would be good to add some logging here, since hitting this is
> somewhat of a special case. For the Fedora test kernels I did I changed
> this to:
> 
> 		if (e820_start <= avail->start && avail->end <= e820_end) {
> 			dev_info(dev, "resource %pR fully covered by e820 entry [mem %#010Lx-%#010Lx]\n",
> 				 avail, e820_start, e820_end);
> 			continue;
> 		}
> 
> And I expect/hope to see this new info message on the ideapad with the
> touchpad issue.

Right, I would expect the same.

We could add something like this.  But both the e820 entry and the
host bridge window are already in the dmesg log, so it doesn't really
add new information, and I don't think there's anything *wrong* with
this situation (per the UEFI text above), so I don't think we need to
call attention to it.

I think what might add useful information would be to always log the
EFI "RUN" entries.  IIUC, currently the "efi: mem47: ..." lines are
only emitted when booting with "efi=debug"?

I think the "RUN" lines indicate regions that must be virtually mapped
so EFI runtime services can use them, and it seems like it might be
more generally useful to always mention them.

Bjorn

Re: [PATCH 3/3] x86/PCI: Preserve host bridge windows completely covered by E820

[Hans de Goede]

Hi,

On 3/4/22 16:32, Bjorn Helgaas wrote:
> On Fri, Mar 04, 2022 at 03:16:42PM +0100, Hans de Goede wrote:
>> Hi Bjorn,
>>
>> On 3/4/22 04:51, Bjorn Helgaas wrote:
>>> From: Bjorn Helgaas <bhelgaas@google.com>
>>>
>>> Many folks have reported PCI devices not working.  It could affect any
>>> device, but most reports are for Thunderbolt controllers on Lenovo Yoga and
>>> Clevo Barebone laptops and the touchpad on Lenovo IdeaPads.
>>>
>>> In every report, a region in the E820 table entirely encloses a PCI host
>>> bridge window from _CRS, and because of 4dc2287c1805 ("x86: avoid E820
>>> regions when allocating address space"), we ignore the entire window,
>>> preventing us from assigning space to PCI devices.
>>>
>>> For example, the dmesg log [2] from bug report [1] shows:
>>>
>>>   BIOS-e820: [mem 0x000000004bc50000-0x00000000cfffffff] reserved
>>>   pci_bus 0000:00: root bus resource [mem 0x65400000-0xbfffffff window]
>>>   pci 0000:00:15.0: BAR 0: no space for [mem size 0x00001000 64bit]
>>>
>>> The efi=debug dmesg log [3] from the same report shows the EFI memory map
>>> entries that created the E820 map:
>>>
>>>   efi: mem47: [Reserved |   |WB|WT|WC|UC] range=[0x4bc50000-0x5fffffff]
>>>   efi: mem48: [Reserved |   |WB|  |  |UC] range=[0x60000000-0x60ffffff]
>>>   efi: mem49: [Reserved |   |  |  |  |  ] range=[0x61000000-0x653fffff]
>>>   efi: mem50: [MMIO     |RUN|  |  |  |UC] range=[0x65400000-0xcfffffff]
>>>
>>> 4dc2287c1805 ("x86: avoid E820 regions when allocating address space")
>>> works around issues where _CRS contains non-window address space that can't
>>> be used for PCI devices.  It does this by removing E820 regions from host
>>> bridge windows.  But in these reports, the E820 region covers the entire
>>> window, so 4dc2287c1805 makes it completely unusable.
>>>
>>> Per UEFI v2.8, sec 7.2, the EfiMemoryMappedIO type means:
>>>
>>>   Used by system firmware to request that a memory-mapped IO region be
>>>   mapped by the OS to a virtual address so it can be accessed by EFI
>>>   runtime services.
>>>
>>> A host bridge window is definitely a memory-mapped IO region, and EFI
>>> runtime services may need to access it, so I don't think we can argue that
>>> this is a firmware defect.
>>>
>>> Instead, change the 4dc2287c1805 strategy so it only removes E820 regions
>>> when they overlap *part* of a host bridge window on the assumption that a
>>> partial overlap is really register space, not part of the window proper.
>>>
>>> If an E820 region covers the entire window from _CRS, assume the _CRS
>>> window is correct and do nothing.
>>>
>>> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1868899
>>> [2] https://bugzilla.redhat.com/attachment.cgi?id=1711424
>>> [3] https://bugzilla.redhat.com/attachment.cgi?id=1861407
>>>
>>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=206459
>>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=214259
>>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1868899
>>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1871793
>>> BugLink: https://bugs.launchpad.net/bugs/1878279
>>> BugLink: https://bugs.launchpad.net/bugs/1931715
>>> BugLink: https://bugs.launchpad.net/bugs/1932069
>>> BugLink: https://bugs.launchpad.net/bugs/1921649
>>> Fixes: 4dc2287c1805 ("x86: avoid E820 regions when allocating address space")
>>> Link: https://lore.kernel.org/r/20220228105259.230903-1-hdegoede@redhat.com
>>> Based-on-patch-by: Hans de Goede <hdegoede@redhat.com>
>>> Reported-by: Benoit Grégoire <benoitg@coeus.ca>   # BZ 206459
>>> Reported-by: wse@tuxedocomputers.com              # BZ 214259
>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>> ---
>>>  arch/x86/kernel/resource.c | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>>>
>>> diff --git a/arch/x86/kernel/resource.c b/arch/x86/kernel/resource.c
>>> index 7378ea146976..405f0af53e3d 100644
>>> --- a/arch/x86/kernel/resource.c
>>> +++ b/arch/x86/kernel/resource.c
>>> @@ -39,6 +39,17 @@ void remove_e820_regions(struct device *dev, struct resource *avail)
>>>  		e820_start = entry->addr;
>>>  		e820_end = entry->addr + entry->size - 1;
>>>  
>>> +		/*
>>> +		 * If an E820 entry covers just part of the resource, we
>>> +		 * assume E820 is telling us about something like host
>>> +		 * bridge register space that is unavailable for PCI
>>> +		 * devices.  But if it covers the *entire* resource, it's
>>> +		 * more likely just telling us that this is MMIO space, and
>>> +		 * that doesn't need to be removed.
>>> +		 */
>>> +		if (e820_start <= avail->start && avail->end <= e820_end)
>>> +			continue;
>>> +
>>
>> IMHO it would be good to add some logging here, since hitting this is
>> somewhat of a special case. For the Fedora test kernels I did I changed
>> this to:
>>
>> 		if (e820_start <= avail->start && avail->end <= e820_end) {
>> 			dev_info(dev, "resource %pR fully covered by e820 entry [mem %#010Lx-%#010Lx]\n",
>> 				 avail, e820_start, e820_end);
>> 			continue;
>> 		}
>>
>> And I expect/hope to see this new info message on the ideapad with the
>> touchpad issue.
> 
> Right, I would expect the same.
> 
> We could add something like this.  But both the e820 entry and the
> host bridge window are already in the dmesg log, so it doesn't really
> add new information

Well it adds the information that the workaround (to the workaround)
which we added for this case is working as expected and it allows
seeing that is the case in a single glance.

Yes we can derive this is happening from the other logs, but it won't
"stand out" unless you are specifically looking for it. Having
a separate line which stands-out (a bit) might be helpful to spot
this when debugging something else which seems unrelated, but
possibly is actually related.

Anyways, I'll leave what to do here up to you.

>, and I don't think there's anything *wrong* with
> this situation (per the UEFI text above),

Right, which is why I suggest using dev_info and not dev_warn.

> so I don't think we need to
> call attention to it.
> 
> I think what might add useful information would be to always log the
> EFI "RUN" entries.  IIUC, currently the "efi: mem47: ..." lines are
> only emitted when booting with "efi=debug"?
> 
> I think the "RUN" lines indicate regions that must be virtually mapped
> so EFI runtime services can use them, and it seems like it might be
> more generally useful to always mention them.

I'm not sure about always logging the EFI memmap I agree it might
be useful sometimes, but it is easy to enable then and the initial
boot code of the kernel already is pretty "chatty".

Regards,

Hans

Re: [PATCH 3/3] x86/PCI: Preserve host bridge windows completely covered by E820

[Bjorn Helgaas]

On Fri, Mar 04, 2022 at 04:46:11PM +0100, Hans de Goede wrote:
> On 3/4/22 16:32, Bjorn Helgaas wrote:

> > I think what might add useful information would be to always log the
> > EFI "RUN" entries.  IIUC, currently the "efi: mem47: ..." lines are
> > only emitted when booting with "efi=debug"?
> > 
> > I think the "RUN" lines indicate regions that must be virtually mapped
> > so EFI runtime services can use them, and it seems like it might be
> > more generally useful to always mention them.
> 
> I'm not sure about always logging the EFI memmap I agree it might
> be useful sometimes, but it is easy to enable then and the initial
> boot code of the kernel already is pretty "chatty".

Yeah.  I didn't mean all of the EFI memmap, just the parts that we're
sharing with firmware.  But I guess everybody probably has different
parts of the map they think would be interesting :)

Re: [PATCH 3/3] x86/PCI: Preserve host bridge windows completely covered by E820

[Hans de Goede]

Hi,

On 3/4/22 16:46, Hans de Goede wrote:
> Hi,
> 
> On 3/4/22 16:32, Bjorn Helgaas wrote:
>> On Fri, Mar 04, 2022 at 03:16:42PM +0100, Hans de Goede wrote:
>>> Hi Bjorn,
>>>
>>> On 3/4/22 04:51, Bjorn Helgaas wrote:
>>>> From: Bjorn Helgaas <bhelgaas@google.com>
>>>>
>>>> Many folks have reported PCI devices not working.  It could affect any
>>>> device, but most reports are for Thunderbolt controllers on Lenovo Yoga and
>>>> Clevo Barebone laptops and the touchpad on Lenovo IdeaPads.
>>>>
>>>> In every report, a region in the E820 table entirely encloses a PCI host
>>>> bridge window from _CRS, and because of 4dc2287c1805 ("x86: avoid E820
>>>> regions when allocating address space"), we ignore the entire window,
>>>> preventing us from assigning space to PCI devices.
>>>>
>>>> For example, the dmesg log [2] from bug report [1] shows:
>>>>
>>>>   BIOS-e820: [mem 0x000000004bc50000-0x00000000cfffffff] reserved
>>>>   pci_bus 0000:00: root bus resource [mem 0x65400000-0xbfffffff window]
>>>>   pci 0000:00:15.0: BAR 0: no space for [mem size 0x00001000 64bit]
>>>>
>>>> The efi=debug dmesg log [3] from the same report shows the EFI memory map
>>>> entries that created the E820 map:
>>>>
>>>>   efi: mem47: [Reserved |   |WB|WT|WC|UC] range=[0x4bc50000-0x5fffffff]
>>>>   efi: mem48: [Reserved |   |WB|  |  |UC] range=[0x60000000-0x60ffffff]
>>>>   efi: mem49: [Reserved |   |  |  |  |  ] range=[0x61000000-0x653fffff]
>>>>   efi: mem50: [MMIO     |RUN|  |  |  |UC] range=[0x65400000-0xcfffffff]
>>>>
>>>> 4dc2287c1805 ("x86: avoid E820 regions when allocating address space")
>>>> works around issues where _CRS contains non-window address space that can't
>>>> be used for PCI devices.  It does this by removing E820 regions from host
>>>> bridge windows.  But in these reports, the E820 region covers the entire
>>>> window, so 4dc2287c1805 makes it completely unusable.
>>>>
>>>> Per UEFI v2.8, sec 7.2, the EfiMemoryMappedIO type means:
>>>>
>>>>   Used by system firmware to request that a memory-mapped IO region be
>>>>   mapped by the OS to a virtual address so it can be accessed by EFI
>>>>   runtime services.
>>>>
>>>> A host bridge window is definitely a memory-mapped IO region, and EFI
>>>> runtime services may need to access it, so I don't think we can argue that
>>>> this is a firmware defect.
>>>>
>>>> Instead, change the 4dc2287c1805 strategy so it only removes E820 regions
>>>> when they overlap *part* of a host bridge window on the assumption that a
>>>> partial overlap is really register space, not part of the window proper.
>>>>
>>>> If an E820 region covers the entire window from _CRS, assume the _CRS
>>>> window is correct and do nothing.
>>>>
>>>> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1868899
>>>> [2] https://bugzilla.redhat.com/attachment.cgi?id=1711424
>>>> [3] https://bugzilla.redhat.com/attachment.cgi?id=1861407
>>>>
>>>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=206459
>>>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=214259
>>>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1868899
>>>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1871793
>>>> BugLink: https://bugs.launchpad.net/bugs/1878279
>>>> BugLink: https://bugs.launchpad.net/bugs/1931715
>>>> BugLink: https://bugs.launchpad.net/bugs/1932069
>>>> BugLink: https://bugs.launchpad.net/bugs/1921649
>>>> Fixes: 4dc2287c1805 ("x86: avoid E820 regions when allocating address space")
>>>> Link: https://lore.kernel.org/r/20220228105259.230903-1-hdegoede@redhat.com
>>>> Based-on-patch-by: Hans de Goede <hdegoede@redhat.com>
>>>> Reported-by: Benoit Grégoire <benoitg@coeus.ca>   # BZ 206459
>>>> Reported-by: wse@tuxedocomputers.com              # BZ 214259
>>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>>> ---
>>>>  arch/x86/kernel/resource.c | 11 +++++++++++
>>>>  1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/arch/x86/kernel/resource.c b/arch/x86/kernel/resource.c
>>>> index 7378ea146976..405f0af53e3d 100644
>>>> --- a/arch/x86/kernel/resource.c
>>>> +++ b/arch/x86/kernel/resource.c
>>>> @@ -39,6 +39,17 @@ void remove_e820_regions(struct device *dev, struct resource *avail)
>>>>  		e820_start = entry->addr;
>>>>  		e820_end = entry->addr + entry->size - 1;
>>>>  
>>>> +		/*
>>>> +		 * If an E820 entry covers just part of the resource, we
>>>> +		 * assume E820 is telling us about something like host
>>>> +		 * bridge register space that is unavailable for PCI
>>>> +		 * devices.  But if it covers the *entire* resource, it's
>>>> +		 * more likely just telling us that this is MMIO space, and
>>>> +		 * that doesn't need to be removed.
>>>> +		 */
>>>> +		if (e820_start <= avail->start && avail->end <= e820_end)
>>>> +			continue;
>>>> +
>>>
>>> IMHO it would be good to add some logging here, since hitting this is
>>> somewhat of a special case. For the Fedora test kernels I did I changed
>>> this to:
>>>
>>> 		if (e820_start <= avail->start && avail->end <= e820_end) {
>>> 			dev_info(dev, "resource %pR fully covered by e820 entry [mem %#010Lx-%#010Lx]\n",
>>> 				 avail, e820_start, e820_end);
>>> 			continue;
>>> 		}
>>>
>>> And I expect/hope to see this new info message on the ideapad with the
>>> touchpad issue.
>>
>> Right, I would expect the same.
>>
>> We could add something like this.  But both the e820 entry and the
>> host bridge window are already in the dmesg log, so it doesn't really
>> add new information
> 
> Well it adds the information that the workaround (to the workaround)
> which we added for this case is working as expected and it allows
> seeing that is the case in a single glance.

So I just got the first report back from the Fedora test 5.16.12 kernel
with this series added. Good news on the ideapad this wotks fine to
fix the touchpad issue (as expected).

What is interesting is that the above dev_info message which I added
triggers *twice*:

[    0.327837] acpi PNP0A08:00: resource [mem 0x000a0000-0x000bffff window] fully covered by e820 entry [mem 0x0009f000-0x000fffff]
[    0.327843] acpi PNP0A08:00: resource [mem 0x65400000-0xbfffffff window] fully covered by e820 entry [mem 0x4bc50000-0xcfffffff]

Notice that it also stops from the mem-window for ISA io getting fully
clipped, which I did not realize also was a potential issue.

I hope this also shows that having the dev_info here is good,
at least IMHO this confirms that having the dev_info for this
is a good thing.

I'm still waiting for testing results on the X1C2 which had the
suspend/resume regressions with my bios-date based approach.

Regards,

Hans

Re: [PATCH 3/3] x86/PCI: Preserve host bridge windows completely covered by E820

[Hans de Goede]

Hi Bjorn, Rafael,

On 3/5/22 11:37, Hans de Goede wrote:
> Hi,
> 
> On 3/4/22 16:46, Hans de Goede wrote:
>> Hi,
>>
>> On 3/4/22 16:32, Bjorn Helgaas wrote:
>>> On Fri, Mar 04, 2022 at 03:16:42PM +0100, Hans de Goede wrote:
>>>> Hi Bjorn,
>>>>
>>>> On 3/4/22 04:51, Bjorn Helgaas wrote:
>>>>> From: Bjorn Helgaas <bhelgaas@google.com>
>>>>>
>>>>> Many folks have reported PCI devices not working.  It could affect any
>>>>> device, but most reports are for Thunderbolt controllers on Lenovo Yoga and
>>>>> Clevo Barebone laptops and the touchpad on Lenovo IdeaPads.
>>>>>
>>>>> In every report, a region in the E820 table entirely encloses a PCI host
>>>>> bridge window from _CRS, and because of 4dc2287c1805 ("x86: avoid E820
>>>>> regions when allocating address space"), we ignore the entire window,
>>>>> preventing us from assigning space to PCI devices.
>>>>>
>>>>> For example, the dmesg log [2] from bug report [1] shows:
>>>>>
>>>>>   BIOS-e820: [mem 0x000000004bc50000-0x00000000cfffffff] reserved
>>>>>   pci_bus 0000:00: root bus resource [mem 0x65400000-0xbfffffff window]
>>>>>   pci 0000:00:15.0: BAR 0: no space for [mem size 0x00001000 64bit]
>>>>>
>>>>> The efi=debug dmesg log [3] from the same report shows the EFI memory map
>>>>> entries that created the E820 map:
>>>>>
>>>>>   efi: mem47: [Reserved |   |WB|WT|WC|UC] range=[0x4bc50000-0x5fffffff]
>>>>>   efi: mem48: [Reserved |   |WB|  |  |UC] range=[0x60000000-0x60ffffff]
>>>>>   efi: mem49: [Reserved |   |  |  |  |  ] range=[0x61000000-0x653fffff]
>>>>>   efi: mem50: [MMIO     |RUN|  |  |  |UC] range=[0x65400000-0xcfffffff]
>>>>>
>>>>> 4dc2287c1805 ("x86: avoid E820 regions when allocating address space")
>>>>> works around issues where _CRS contains non-window address space that can't
>>>>> be used for PCI devices.  It does this by removing E820 regions from host
>>>>> bridge windows.  But in these reports, the E820 region covers the entire
>>>>> window, so 4dc2287c1805 makes it completely unusable.
>>>>>
>>>>> Per UEFI v2.8, sec 7.2, the EfiMemoryMappedIO type means:
>>>>>
>>>>>   Used by system firmware to request that a memory-mapped IO region be
>>>>>   mapped by the OS to a virtual address so it can be accessed by EFI
>>>>>   runtime services.
>>>>>
>>>>> A host bridge window is definitely a memory-mapped IO region, and EFI
>>>>> runtime services may need to access it, so I don't think we can argue that
>>>>> this is a firmware defect.
>>>>>
>>>>> Instead, change the 4dc2287c1805 strategy so it only removes E820 regions
>>>>> when they overlap *part* of a host bridge window on the assumption that a
>>>>> partial overlap is really register space, not part of the window proper.
>>>>>
>>>>> If an E820 region covers the entire window from _CRS, assume the _CRS
>>>>> window is correct and do nothing.
>>>>>
>>>>> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1868899
>>>>> [2] https://bugzilla.redhat.com/attachment.cgi?id=1711424
>>>>> [3] https://bugzilla.redhat.com/attachment.cgi?id=1861407
>>>>>
>>>>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=206459
>>>>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=214259
>>>>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1868899
>>>>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1871793
>>>>> BugLink: https://bugs.launchpad.net/bugs/1878279
>>>>> BugLink: https://bugs.launchpad.net/bugs/1931715
>>>>> BugLink: https://bugs.launchpad.net/bugs/1932069
>>>>> BugLink: https://bugs.launchpad.net/bugs/1921649
>>>>> Fixes: 4dc2287c1805 ("x86: avoid E820 regions when allocating address space")
>>>>> Link: https://lore.kernel.org/r/20220228105259.230903-1-hdegoede@redhat.com
>>>>> Based-on-patch-by: Hans de Goede <hdegoede@redhat.com>
>>>>> Reported-by: Benoit Grégoire <benoitg@coeus.ca>   # BZ 206459
>>>>> Reported-by: wse@tuxedocomputers.com              # BZ 214259
>>>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>>>> ---
>>>>>  arch/x86/kernel/resource.c | 11 +++++++++++
>>>>>  1 file changed, 11 insertions(+)
>>>>>
>>>>> diff --git a/arch/x86/kernel/resource.c b/arch/x86/kernel/resource.c
>>>>> index 7378ea146976..405f0af53e3d 100644
>>>>> --- a/arch/x86/kernel/resource.c
>>>>> +++ b/arch/x86/kernel/resource.c
>>>>> @@ -39,6 +39,17 @@ void remove_e820_regions(struct device *dev, struct resource *avail)
>>>>>  		e820_start = entry->addr;
>>>>>  		e820_end = entry->addr + entry->size - 1;
>>>>>  
>>>>> +		/*
>>>>> +		 * If an E820 entry covers just part of the resource, we
>>>>> +		 * assume E820 is telling us about something like host
>>>>> +		 * bridge register space that is unavailable for PCI
>>>>> +		 * devices.  But if it covers the *entire* resource, it's
>>>>> +		 * more likely just telling us that this is MMIO space, and
>>>>> +		 * that doesn't need to be removed.
>>>>> +		 */
>>>>> +		if (e820_start <= avail->start && avail->end <= e820_end)
>>>>> +			continue;
>>>>> +
>>>>
>>>> IMHO it would be good to add some logging here, since hitting this is
>>>> somewhat of a special case. For the Fedora test kernels I did I changed
>>>> this to:
>>>>
>>>> 		if (e820_start <= avail->start && avail->end <= e820_end) {
>>>> 			dev_info(dev, "resource %pR fully covered by e820 entry [mem %#010Lx-%#010Lx]\n",
>>>> 				 avail, e820_start, e820_end);
>>>> 			continue;
>>>> 		}
>>>>
>>>> And I expect/hope to see this new info message on the ideapad with the
>>>> touchpad issue.
>>>
>>> Right, I would expect the same.
>>>
>>> We could add something like this.  But both the e820 entry and the
>>> host bridge window are already in the dmesg log, so it doesn't really
>>> add new information
>>
>> Well it adds the information that the workaround (to the workaround)
>> which we added for this case is working as expected and it allows
>> seeing that is the case in a single glance.
> 
> So I just got the first report back from the Fedora test 5.16.12 kernel
> with this series added. Good news on the ideapad this wotks fine to
> fix the touchpad issue (as expected).
> 
> What is interesting is that the above dev_info message which I added
> triggers *twice*:
> 
> [    0.327837] acpi PNP0A08:00: resource [mem 0x000a0000-0x000bffff window] fully covered by e820 entry [mem 0x0009f000-0x000fffff]
> [    0.327843] acpi PNP0A08:00: resource [mem 0x65400000-0xbfffffff window] fully covered by e820 entry [mem 0x4bc50000-0xcfffffff]
> 
> Notice that it also stops from the mem-window for ISA io getting fully
> clipped, which I did not realize also was a potential issue.
> 
> I hope this also shows that having the dev_info here is good,
> at least IMHO this confirms that having the dev_info for this
> is a good thing.
> 
> I'm still waiting for testing results on the X1C2 which had the
> suspend/resume regressions with my bios-date based approach.

I have heard back from the X1C2 user, he does not have access to
the machine atm he will get back to me in a couple of days.

I don't really expect any surprises there though, so given where
we are in the kernel-cycle and that we already have confirmation
that it fixes the ideapad touchpad issues I think we should move
forward with this patch-set now.

Rafael, can you drop my variant of this patch?  (this series is
a cleaner implementation of basically the same method to fix
things)

Bjorn, I assume you will merge this series through your tree?

Regards,

Hans

Re: [PATCH 3/3] x86/PCI: Preserve host bridge windows completely covered by E820

[Rafael J. Wysocki]

On Mon, Mar 7, 2022 at 11:33 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Bjorn, Rafael,
>
> On 3/5/22 11:37, Hans de Goede wrote:
> > Hi,
> >
> > On 3/4/22 16:46, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 3/4/22 16:32, Bjorn Helgaas wrote:
> >>> On Fri, Mar 04, 2022 at 03:16:42PM +0100, Hans de Goede wrote:
> >>>> Hi Bjorn,
> >>>>
> >>>> On 3/4/22 04:51, Bjorn Helgaas wrote:
> >>>>> From: Bjorn Helgaas <bhelgaas@google.com>
> >>>>>
> >>>>> Many folks have reported PCI devices not working.  It could affect any
> >>>>> device, but most reports are for Thunderbolt controllers on Lenovo Yoga and
> >>>>> Clevo Barebone laptops and the touchpad on Lenovo IdeaPads.
> >>>>>
> >>>>> In every report, a region in the E820 table entirely encloses a PCI host
> >>>>> bridge window from _CRS, and because of 4dc2287c1805 ("x86: avoid E820
> >>>>> regions when allocating address space"), we ignore the entire window,
> >>>>> preventing us from assigning space to PCI devices.
> >>>>>
> >>>>> For example, the dmesg log [2] from bug report [1] shows:
> >>>>>
> >>>>>   BIOS-e820: [mem 0x000000004bc50000-0x00000000cfffffff] reserved
> >>>>>   pci_bus 0000:00: root bus resource [mem 0x65400000-0xbfffffff window]
> >>>>>   pci 0000:00:15.0: BAR 0: no space for [mem size 0x00001000 64bit]
> >>>>>
> >>>>> The efi=debug dmesg log [3] from the same report shows the EFI memory map
> >>>>> entries that created the E820 map:
> >>>>>
> >>>>>   efi: mem47: [Reserved |   |WB|WT|WC|UC] range=[0x4bc50000-0x5fffffff]
> >>>>>   efi: mem48: [Reserved |   |WB|  |  |UC] range=[0x60000000-0x60ffffff]
> >>>>>   efi: mem49: [Reserved |   |  |  |  |  ] range=[0x61000000-0x653fffff]
> >>>>>   efi: mem50: [MMIO     |RUN|  |  |  |UC] range=[0x65400000-0xcfffffff]
> >>>>>
> >>>>> 4dc2287c1805 ("x86: avoid E820 regions when allocating address space")
> >>>>> works around issues where _CRS contains non-window address space that can't
> >>>>> be used for PCI devices.  It does this by removing E820 regions from host
> >>>>> bridge windows.  But in these reports, the E820 region covers the entire
> >>>>> window, so 4dc2287c1805 makes it completely unusable.
> >>>>>
> >>>>> Per UEFI v2.8, sec 7.2, the EfiMemoryMappedIO type means:
> >>>>>
> >>>>>   Used by system firmware to request that a memory-mapped IO region be
> >>>>>   mapped by the OS to a virtual address so it can be accessed by EFI
> >>>>>   runtime services.
> >>>>>
> >>>>> A host bridge window is definitely a memory-mapped IO region, and EFI
> >>>>> runtime services may need to access it, so I don't think we can argue that
> >>>>> this is a firmware defect.
> >>>>>
> >>>>> Instead, change the 4dc2287c1805 strategy so it only removes E820 regions
> >>>>> when they overlap *part* of a host bridge window on the assumption that a
> >>>>> partial overlap is really register space, not part of the window proper.
> >>>>>
> >>>>> If an E820 region covers the entire window from _CRS, assume the _CRS
> >>>>> window is correct and do nothing.
> >>>>>
> >>>>> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1868899
> >>>>> [2] https://bugzilla.redhat.com/attachment.cgi?id=1711424
> >>>>> [3] https://bugzilla.redhat.com/attachment.cgi?id=1861407
> >>>>>
> >>>>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=206459
> >>>>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=214259
> >>>>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1868899
> >>>>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1871793
> >>>>> BugLink: https://bugs.launchpad.net/bugs/1878279
> >>>>> BugLink: https://bugs.launchpad.net/bugs/1931715
> >>>>> BugLink: https://bugs.launchpad.net/bugs/1932069
> >>>>> BugLink: https://bugs.launchpad.net/bugs/1921649
> >>>>> Fixes: 4dc2287c1805 ("x86: avoid E820 regions when allocating address space")
> >>>>> Link: https://lore.kernel.org/r/20220228105259.230903-1-hdegoede@redhat.com
> >>>>> Based-on-patch-by: Hans de Goede <hdegoede@redhat.com>
> >>>>> Reported-by: Benoit Grégoire <benoitg@coeus.ca>   # BZ 206459
> >>>>> Reported-by: wse@tuxedocomputers.com              # BZ 214259
> >>>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> >>>>> ---
> >>>>>  arch/x86/kernel/resource.c | 11 +++++++++++
> >>>>>  1 file changed, 11 insertions(+)
> >>>>>
> >>>>> diff --git a/arch/x86/kernel/resource.c b/arch/x86/kernel/resource.c
> >>>>> index 7378ea146976..405f0af53e3d 100644
> >>>>> --- a/arch/x86/kernel/resource.c
> >>>>> +++ b/arch/x86/kernel/resource.c
> >>>>> @@ -39,6 +39,17 @@ void remove_e820_regions(struct device *dev, struct resource *avail)
> >>>>>           e820_start = entry->addr;
> >>>>>           e820_end = entry->addr + entry->size - 1;
> >>>>>
> >>>>> +         /*
> >>>>> +          * If an E820 entry covers just part of the resource, we
> >>>>> +          * assume E820 is telling us about something like host
> >>>>> +          * bridge register space that is unavailable for PCI
> >>>>> +          * devices.  But if it covers the *entire* resource, it's
> >>>>> +          * more likely just telling us that this is MMIO space, and
> >>>>> +          * that doesn't need to be removed.
> >>>>> +          */
> >>>>> +         if (e820_start <= avail->start && avail->end <= e820_end)
> >>>>> +                 continue;
> >>>>> +
> >>>>
> >>>> IMHO it would be good to add some logging here, since hitting this is
> >>>> somewhat of a special case. For the Fedora test kernels I did I changed
> >>>> this to:
> >>>>
> >>>>            if (e820_start <= avail->start && avail->end <= e820_end) {
> >>>>                    dev_info(dev, "resource %pR fully covered by e820 entry [mem %#010Lx-%#010Lx]\n",
> >>>>                             avail, e820_start, e820_end);
> >>>>                    continue;
> >>>>            }
> >>>>
> >>>> And I expect/hope to see this new info message on the ideapad with the
> >>>> touchpad issue.
> >>>
> >>> Right, I would expect the same.
> >>>
> >>> We could add something like this.  But both the e820 entry and the
> >>> host bridge window are already in the dmesg log, so it doesn't really
> >>> add new information
> >>
> >> Well it adds the information that the workaround (to the workaround)
> >> which we added for this case is working as expected and it allows
> >> seeing that is the case in a single glance.
> >
> > So I just got the first report back from the Fedora test 5.16.12 kernel
> > with this series added. Good news on the ideapad this wotks fine to
> > fix the touchpad issue (as expected).
> >
> > What is interesting is that the above dev_info message which I added
> > triggers *twice*:
> >
> > [    0.327837] acpi PNP0A08:00: resource [mem 0x000a0000-0x000bffff window] fully covered by e820 entry [mem 0x0009f000-0x000fffff]
> > [    0.327843] acpi PNP0A08:00: resource [mem 0x65400000-0xbfffffff window] fully covered by e820 entry [mem 0x4bc50000-0xcfffffff]
> >
> > Notice that it also stops from the mem-window for ISA io getting fully
> > clipped, which I did not realize also was a potential issue.
> >
> > I hope this also shows that having the dev_info here is good,
> > at least IMHO this confirms that having the dev_info for this
> > is a good thing.
> >
> > I'm still waiting for testing results on the X1C2 which had the
> > suspend/resume regressions with my bios-date based approach.
>
> I have heard back from the X1C2 user, he does not have access to
> the machine atm he will get back to me in a couple of days.
>
> I don't really expect any surprises there though, so given where
> we are in the kernel-cycle and that we already have confirmation
> that it fixes the ideapad touchpad issues I think we should move
> forward with this patch-set now.
>
> Rafael, can you drop my variant of this patch?  (this series is
> a cleaner implementation of basically the same method to fix
> things)

Done.

> Bjorn, I assume you will merge this series through your tree?

Same here, and please feel free to add

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

to all of the patches in this series.

Thanks!

Re: [PATCH 3/3] x86/PCI: Preserve host bridge windows completely covered by E820

[Bjorn Helgaas]

On Sat, Mar 05, 2022 at 11:37:23AM +0100, Hans de Goede wrote:
> On 3/4/22 16:46, Hans de Goede wrote:
> > On 3/4/22 16:32, Bjorn Helgaas wrote:
> >> On Fri, Mar 04, 2022 at 03:16:42PM +0100, Hans de Goede wrote:
> >>> On 3/4/22 04:51, Bjorn Helgaas wrote:
> >>>> From: Bjorn Helgaas <bhelgaas@google.com>
> >>>>
> >>>> Many folks have reported PCI devices not working.  It could affect any
> >>>> device, but most reports are for Thunderbolt controllers on Lenovo Yoga and
> >>>> Clevo Barebone laptops and the touchpad on Lenovo IdeaPads.
> >>>> ...

> >>>> diff --git a/arch/x86/kernel/resource.c b/arch/x86/kernel/resource.c
> >>>> index 7378ea146976..405f0af53e3d 100644
> >>>> --- a/arch/x86/kernel/resource.c
> >>>> +++ b/arch/x86/kernel/resource.c
> >>>> @@ -39,6 +39,17 @@ void remove_e820_regions(struct device *dev, struct resource *avail)
> >>>>  		e820_start = entry->addr;
> >>>>  		e820_end = entry->addr + entry->size - 1;
> >>>>  
> >>>> +		/*
> >>>> +		 * If an E820 entry covers just part of the resource, we
> >>>> +		 * assume E820 is telling us about something like host
> >>>> +		 * bridge register space that is unavailable for PCI
> >>>> +		 * devices.  But if it covers the *entire* resource, it's
> >>>> +		 * more likely just telling us that this is MMIO space, and
> >>>> +		 * that doesn't need to be removed.
> >>>> +		 */
> >>>> +		if (e820_start <= avail->start && avail->end <= e820_end)
> >>>> +			continue;
> >>>> +
> >>>
> >>> IMHO it would be good to add some logging here, since hitting this is
> >>> somewhat of a special case. For the Fedora test kernels I did I changed
> >>> this to:
> >>>
> >>> 		if (e820_start <= avail->start && avail->end <= e820_end) {
> >>> 			dev_info(dev, "resource %pR fully covered by e820 entry [mem %#010Lx-%#010Lx]\n",
> >>> 				 avail, e820_start, e820_end);
> >>> 			continue;
> >>> 		}
> >>>
> >>> And I expect/hope to see this new info message on the ideapad with the
> >>> touchpad issue.

I added this logging.

> So I just got the first report back from the Fedora test 5.16.12 kernel
> with this series added. Good news on the ideapad this wotks fine to
> fix the touchpad issue (as expected).

Any "Tested-by" I could add?  If we can, I'd really like to give some
credit to the folks who suffered through this and helped resolve it.

Bjorn

Re: [PATCH 3/3] x86/PCI: Preserve host bridge windows completely covered by E820

[Hans de Goede]

Hi Bjorn,

On 3/9/22 19:15, Bjorn Helgaas wrote:
> On Sat, Mar 05, 2022 at 11:37:23AM +0100, Hans de Goede wrote:
>> On 3/4/22 16:46, Hans de Goede wrote:
>>> On 3/4/22 16:32, Bjorn Helgaas wrote:
>>>> On Fri, Mar 04, 2022 at 03:16:42PM +0100, Hans de Goede wrote:
>>>>> On 3/4/22 04:51, Bjorn Helgaas wrote:
>>>>>> From: Bjorn Helgaas <bhelgaas@google.com>
>>>>>>
>>>>>> Many folks have reported PCI devices not working.  It could affect any
>>>>>> device, but most reports are for Thunderbolt controllers on Lenovo Yoga and
>>>>>> Clevo Barebone laptops and the touchpad on Lenovo IdeaPads.
>>>>>> ...
> 
>>>>>> diff --git a/arch/x86/kernel/resource.c b/arch/x86/kernel/resource.c
>>>>>> index 7378ea146976..405f0af53e3d 100644
>>>>>> --- a/arch/x86/kernel/resource.c
>>>>>> +++ b/arch/x86/kernel/resource.c
>>>>>> @@ -39,6 +39,17 @@ void remove_e820_regions(struct device *dev, struct resource *avail)
>>>>>>  		e820_start = entry->addr;
>>>>>>  		e820_end = entry->addr + entry->size - 1;
>>>>>>  
>>>>>> +		/*
>>>>>> +		 * If an E820 entry covers just part of the resource, we
>>>>>> +		 * assume E820 is telling us about something like host
>>>>>> +		 * bridge register space that is unavailable for PCI
>>>>>> +		 * devices.  But if it covers the *entire* resource, it's
>>>>>> +		 * more likely just telling us that this is MMIO space, and
>>>>>> +		 * that doesn't need to be removed.
>>>>>> +		 */
>>>>>> +		if (e820_start <= avail->start && avail->end <= e820_end)
>>>>>> +			continue;
>>>>>> +
>>>>>
>>>>> IMHO it would be good to add some logging here, since hitting this is
>>>>> somewhat of a special case. For the Fedora test kernels I did I changed
>>>>> this to:
>>>>>
>>>>> 		if (e820_start <= avail->start && avail->end <= e820_end) {
>>>>> 			dev_info(dev, "resource %pR fully covered by e820 entry [mem %#010Lx-%#010Lx]\n",
>>>>> 				 avail, e820_start, e820_end);
>>>>> 			continue;
>>>>> 		}
>>>>>
>>>>> And I expect/hope to see this new info message on the ideapad with the
>>>>> touchpad issue.
> 
> I added this logging.
> 
>> So I just got the first report back from the Fedora test 5.16.12 kernel
>> with this series added. Good news on the ideapad this wotks fine to
>> fix the touchpad issue (as expected).
> 
> Any "Tested-by" I could add?  If we can, I'd really like to give some
> credit to the folks who suffered through this and helped resolve it.

Good point, the reporter of:
https://bugzilla.redhat.com/show_bug.cgi?id=1868899

has done most of the ideapad with touchpad issues testing for me
and has been very helpful. I agree he deserves credit for this.

I've asked him if he is ok with adding a Tested-by tag and if yes,
which email we should use.

Regards,

Hans

Re: [PATCH 3/3] x86/PCI: Preserve host bridge windows completely covered by E820

[Hans de Goede]

Hi Bjorn,

On 3/10/22 13:28, Hans de Goede wrote:
> Hi Bjorn,
> 
> On 3/9/22 19:15, Bjorn Helgaas wrote:
>> On Sat, Mar 05, 2022 at 11:37:23AM +0100, Hans de Goede wrote:
>>> On 3/4/22 16:46, Hans de Goede wrote:
>>>> On 3/4/22 16:32, Bjorn Helgaas wrote:
>>>>> On Fri, Mar 04, 2022 at 03:16:42PM +0100, Hans de Goede wrote:
>>>>>> On 3/4/22 04:51, Bjorn Helgaas wrote:
>>>>>>> From: Bjorn Helgaas <bhelgaas@google.com>
>>>>>>>
>>>>>>> Many folks have reported PCI devices not working.  It could affect any
>>>>>>> device, but most reports are for Thunderbolt controllers on Lenovo Yoga and
>>>>>>> Clevo Barebone laptops and the touchpad on Lenovo IdeaPads.
>>>>>>> ...
>>
>>>>>>> diff --git a/arch/x86/kernel/resource.c b/arch/x86/kernel/resource.c
>>>>>>> index 7378ea146976..405f0af53e3d 100644
>>>>>>> --- a/arch/x86/kernel/resource.c
>>>>>>> +++ b/arch/x86/kernel/resource.c
>>>>>>> @@ -39,6 +39,17 @@ void remove_e820_regions(struct device *dev, struct resource *avail)
>>>>>>>  		e820_start = entry->addr;
>>>>>>>  		e820_end = entry->addr + entry->size - 1;
>>>>>>>  
>>>>>>> +		/*
>>>>>>> +		 * If an E820 entry covers just part of the resource, we
>>>>>>> +		 * assume E820 is telling us about something like host
>>>>>>> +		 * bridge register space that is unavailable for PCI
>>>>>>> +		 * devices.  But if it covers the *entire* resource, it's
>>>>>>> +		 * more likely just telling us that this is MMIO space, and
>>>>>>> +		 * that doesn't need to be removed.
>>>>>>> +		 */
>>>>>>> +		if (e820_start <= avail->start && avail->end <= e820_end)
>>>>>>> +			continue;
>>>>>>> +
>>>>>>
>>>>>> IMHO it would be good to add some logging here, since hitting this is
>>>>>> somewhat of a special case. For the Fedora test kernels I did I changed
>>>>>> this to:
>>>>>>
>>>>>> 		if (e820_start <= avail->start && avail->end <= e820_end) {
>>>>>> 			dev_info(dev, "resource %pR fully covered by e820 entry [mem %#010Lx-%#010Lx]\n",
>>>>>> 				 avail, e820_start, e820_end);
>>>>>> 			continue;
>>>>>> 		}
>>>>>>
>>>>>> And I expect/hope to see this new info message on the ideapad with the
>>>>>> touchpad issue.
>>
>> I added this logging.
>>
>>> So I just got the first report back from the Fedora test 5.16.12 kernel
>>> with this series added. Good news on the ideapad this wotks fine to
>>> fix the touchpad issue (as expected).
>>
>> Any "Tested-by" I could add?  If we can, I'd really like to give some
>> credit to the folks who suffered through this and helped resolve it.
> 
> Good point, the reporter of:
> https://bugzilla.redhat.com/show_bug.cgi?id=1868899
> 
> has done most of the ideapad with touchpad issues testing for me
> and has been very helpful. I agree he deserves credit for this.
> 
> I've asked him if he is ok with adding a Tested-by tag and if yes,
> which email we should use.

If you can add the following tag that would be great:

Tested-by: Matt Hansen <2lprbe78@duck.com>

Regards,

Hans

Re: [PATCH 3/3] x86/PCI: Preserve host bridge windows completely covered by E820

[Bjorn Helgaas]

[+cc Matt]

On Fri, Mar 11, 2022 at 08:52:31AM +0100, Hans de Goede wrote:
> On 3/10/22 13:28, Hans de Goede wrote:
> > On 3/9/22 19:15, Bjorn Helgaas wrote:
> >> On Sat, Mar 05, 2022 at 11:37:23AM +0100, Hans de Goede wrote:
> >>> On 3/4/22 16:46, Hans de Goede wrote:
> >>>> On 3/4/22 16:32, Bjorn Helgaas wrote:
> >>>>> On Fri, Mar 04, 2022 at 03:16:42PM +0100, Hans de Goede wrote:
> >>>>>> On 3/4/22 04:51, Bjorn Helgaas wrote:
> >>>>>>> From: Bjorn Helgaas <bhelgaas@google.com>
> >>>>>>>
> >>>>>>> Many folks have reported PCI devices not working.  It could affect any
> >>>>>>> device, but most reports are for Thunderbolt controllers on Lenovo Yoga and
> >>>>>>> Clevo Barebone laptops and the touchpad on Lenovo IdeaPads.
> >>>>>>> ...
> >>
> >>>>>>> diff --git a/arch/x86/kernel/resource.c b/arch/x86/kernel/resource.c
> >>>>>>> index 7378ea146976..405f0af53e3d 100644
> >>>>>>> --- a/arch/x86/kernel/resource.c
> >>>>>>> +++ b/arch/x86/kernel/resource.c
> >>>>>>> @@ -39,6 +39,17 @@ void remove_e820_regions(struct device *dev, struct resource *avail)
> >>>>>>>  		e820_start = entry->addr;
> >>>>>>>  		e820_end = entry->addr + entry->size - 1;
> >>>>>>>  
> >>>>>>> +		/*
> >>>>>>> +		 * If an E820 entry covers just part of the resource, we
> >>>>>>> +		 * assume E820 is telling us about something like host
> >>>>>>> +		 * bridge register space that is unavailable for PCI
> >>>>>>> +		 * devices.  But if it covers the *entire* resource, it's
> >>>>>>> +		 * more likely just telling us that this is MMIO space, and
> >>>>>>> +		 * that doesn't need to be removed.
> >>>>>>> +		 */
> >>>>>>> +		if (e820_start <= avail->start && avail->end <= e820_end)
> >>>>>>> +			continue;
> >>>>>>> +
> >>>>>>
> >>>>>> IMHO it would be good to add some logging here, since hitting this is
> >>>>>> somewhat of a special case. For the Fedora test kernels I did I changed
> >>>>>> this to:
> >>>>>>
> >>>>>> 		if (e820_start <= avail->start && avail->end <= e820_end) {
> >>>>>> 			dev_info(dev, "resource %pR fully covered by e820 entry [mem %#010Lx-%#010Lx]\n",
> >>>>>> 				 avail, e820_start, e820_end);
> >>>>>> 			continue;
> >>>>>> 		}
> >>>>>>
> >>>>>> And I expect/hope to see this new info message on the ideapad with the
> >>>>>> touchpad issue.
> >>
> >> I added this logging.
> >>
> >>> So I just got the first report back from the Fedora test 5.16.12 kernel
> >>> with this series added. Good news on the ideapad this wotks fine to
> >>> fix the touchpad issue (as expected).
> >>
> >> Any "Tested-by" I could add?  If we can, I'd really like to give some
> >> credit to the folks who suffered through this and helped resolve it.
> > 
> > Good point, the reporter of:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1868899
> > 
> > has done most of the ideapad with touchpad issues testing for me
> > and has been very helpful. I agree he deserves credit for this.
> > 
> > I've asked him if he is ok with adding a Tested-by tag and if yes,
> > which email we should use.
> 
> If you can add the following tag that would be great:
> 
> Tested-by: Matt Hansen <2lprbe78@duck.com>

Done, thank you very much, Matt!  Many people will benefit from your
work.

Re: [PATCH 3/3] x86/PCI: Preserve host bridge windows completely covered by E820

[Hans de Goede]

Hi,

On 3/4/22 16:46, Hans de Goede wrote:
> Hi,
> 
> On 3/4/22 16:32, Bjorn Helgaas wrote:
>> On Fri, Mar 04, 2022 at 03:16:42PM +0100, Hans de Goede wrote:
>>> Hi Bjorn,
>>>
>>> On 3/4/22 04:51, Bjorn Helgaas wrote:
>>>> From: Bjorn Helgaas <bhelgaas@google.com>
>>>>
>>>> Many folks have reported PCI devices not working.  It could affect any
>>>> device, but most reports are for Thunderbolt controllers on Lenovo Yoga and
>>>> Clevo Barebone laptops and the touchpad on Lenovo IdeaPads.
>>>>
>>>> In every report, a region in the E820 table entirely encloses a PCI host
>>>> bridge window from _CRS, and because of 4dc2287c1805 ("x86: avoid E820
>>>> regions when allocating address space"), we ignore the entire window,
>>>> preventing us from assigning space to PCI devices.
>>>>
>>>> For example, the dmesg log [2] from bug report [1] shows:
>>>>
>>>>   BIOS-e820: [mem 0x000000004bc50000-0x00000000cfffffff] reserved
>>>>   pci_bus 0000:00: root bus resource [mem 0x65400000-0xbfffffff window]
>>>>   pci 0000:00:15.0: BAR 0: no space for [mem size 0x00001000 64bit]
>>>>
>>>> The efi=debug dmesg log [3] from the same report shows the EFI memory map
>>>> entries that created the E820 map:
>>>>
>>>>   efi: mem47: [Reserved |   |WB|WT|WC|UC] range=[0x4bc50000-0x5fffffff]
>>>>   efi: mem48: [Reserved |   |WB|  |  |UC] range=[0x60000000-0x60ffffff]
>>>>   efi: mem49: [Reserved |   |  |  |  |  ] range=[0x61000000-0x653fffff]
>>>>   efi: mem50: [MMIO     |RUN|  |  |  |UC] range=[0x65400000-0xcfffffff]
>>>>
>>>> 4dc2287c1805 ("x86: avoid E820 regions when allocating address space")
>>>> works around issues where _CRS contains non-window address space that can't
>>>> be used for PCI devices.  It does this by removing E820 regions from host
>>>> bridge windows.  But in these reports, the E820 region covers the entire
>>>> window, so 4dc2287c1805 makes it completely unusable.
>>>>
>>>> Per UEFI v2.8, sec 7.2, the EfiMemoryMappedIO type means:
>>>>
>>>>   Used by system firmware to request that a memory-mapped IO region be
>>>>   mapped by the OS to a virtual address so it can be accessed by EFI
>>>>   runtime services.
>>>>
>>>> A host bridge window is definitely a memory-mapped IO region, and EFI
>>>> runtime services may need to access it, so I don't think we can argue that
>>>> this is a firmware defect.
>>>>
>>>> Instead, change the 4dc2287c1805 strategy so it only removes E820 regions
>>>> when they overlap *part* of a host bridge window on the assumption that a
>>>> partial overlap is really register space, not part of the window proper.
>>>>
>>>> If an E820 region covers the entire window from _CRS, assume the _CRS
>>>> window is correct and do nothing.
>>>>
>>>> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1868899
>>>> [2] https://bugzilla.redhat.com/attachment.cgi?id=1711424
>>>> [3] https://bugzilla.redhat.com/attachment.cgi?id=1861407
>>>>
>>>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=206459
>>>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=214259
>>>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1868899
>>>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1871793
>>>> BugLink: https://bugs.launchpad.net/bugs/1878279
>>>> BugLink: https://bugs.launchpad.net/bugs/1931715
>>>> BugLink: https://bugs.launchpad.net/bugs/1932069
>>>> BugLink: https://bugs.launchpad.net/bugs/1921649
>>>> Fixes: 4dc2287c1805 ("x86: avoid E820 regions when allocating address space")
>>>> Link: https://lore.kernel.org/r/20220228105259.230903-1-hdegoede@redhat.com
>>>> Based-on-patch-by: Hans de Goede <hdegoede@redhat.com>
>>>> Reported-by: Benoit Grégoire <benoitg@coeus.ca>   # BZ 206459
>>>> Reported-by: wse@tuxedocomputers.com              # BZ 214259
>>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>>> ---
>>>>  arch/x86/kernel/resource.c | 11 +++++++++++
>>>>  1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/arch/x86/kernel/resource.c b/arch/x86/kernel/resource.c
>>>> index 7378ea146976..405f0af53e3d 100644
>>>> --- a/arch/x86/kernel/resource.c
>>>> +++ b/arch/x86/kernel/resource.c
>>>> @@ -39,6 +39,17 @@ void remove_e820_regions(struct device *dev, struct resource *avail)
>>>>  		e820_start = entry->addr;
>>>>  		e820_end = entry->addr + entry->size - 1;
>>>>  
>>>> +		/*
>>>> +		 * If an E820 entry covers just part of the resource, we
>>>> +		 * assume E820 is telling us about something like host
>>>> +		 * bridge register space that is unavailable for PCI
>>>> +		 * devices.  But if it covers the *entire* resource, it's
>>>> +		 * more likely just telling us that this is MMIO space, and
>>>> +		 * that doesn't need to be removed.
>>>> +		 */
>>>> +		if (e820_start <= avail->start && avail->end <= e820_end)
>>>> +			continue;
>>>> +
>>>
>>> IMHO it would be good to add some logging here, since hitting this is
>>> somewhat of a special case. For the Fedora test kernels I did I changed
>>> this to:
>>>
>>> 		if (e820_start <= avail->start && avail->end <= e820_end) {
>>> 			dev_info(dev, "resource %pR fully covered by e820 entry [mem %#010Lx-%#010Lx]\n",
>>> 				 avail, e820_start, e820_end);
>>> 			continue;
>>> 		}
>>>
>>> And I expect/hope to see this new info message on the ideapad with the
>>> touchpad issue.
>>
>> Right, I would expect the same.
>>
>> We could add something like this.  But both the e820 entry and the
>> host bridge window are already in the dmesg log, so it doesn't really
>> add new information
> 
> Well it adds the information that the workaround (to the workaround)
> which we added for this case is working as expected and it allows
> seeing that is the case in a single glance.

I just got a report back from the Fedora test 5.16.12 kernel
with this series added on the X1C2 which had the suspend/resume
regression with my DMI_BIOS_DATE based approach. Everything still
works well there and it shows the new log messages from 2/3 in action:

[    0.326504] acpi PNP0A08:00: clipped [mem 0xdfa00000-0xfebfffff window] to [mem 0xdfa10000-0xfebfffff window] for e820 entry [mem 0xdceff000-0xdfa0ffff]
[    0.326515] acpi PNP0A08:00: clipped [mem 0xdfa10000-0xfebfffff window] to [mem 0xdfa10000-0xf7ffffff window] for e820 entry [mem 0xf8000000-0xfbffffff]

Regards,

Hans

Re: [PATCH 0/3] x86/PCI: Clip only partial E820 overlaps

[Hans de Goede]

Hi Bjorn,

On 3/4/22 04:51, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> This is based on Hans' extensive debugging and patch at
> https://lore.kernel.org/r/20220228105259.230903-1-hdegoede@redhat.com
> and applies on 7e57714cd0ad ("Linux 5.17-rc6").
> 
> This is basically the same idea (applying the 4dc2287c1805 workaround only
> when an E820 region *partially* overlaps a host bridge window), but I think
> it's a little simpler.
> 
> This also adds a little dmesg output when clipping, which should make
> future debugging easier.
> 
> I bcc'd several folks who didn't have public email addresses in the RedHat
> bugzilla or Launchpad.  If you review or test this, I'd be happy to
> acknowledge that.
> 
> Bjorn Helgaas (3):
>   x86/PCI: Eliminate remove_e820_regions() common subexpressions
>   x86/PCI: Log host bridge window clipping for E820 regions
>   x86/PCI: Preserve host bridge windows completely covered by E820

Thanks, I agree that this is better then my fix I also like the logging
added to 2/3 which lets us know if the commit 4dc2287c1805 workaround
is active.

I have one small remark on 3/3. Regardless of that getting addressed
the entire series is:

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

I've done a Fedora test kernel build of 5.16.12 with these 3 patches
added and asked the reporters of:

https://bugzilla.redhat.com/show_bug.cgi?id=1868899
(ideapad touchpad bug)

and:

https://bugzilla.redhat.com/show_bug.cgi?id=2029207
(Lenovo x1 carbon gen 2 regression with my bios-data based fix_

to test the rpms and to collect dmesg. On the X1C2 this should show
the new logging from 2/3 "in action" and on the ideapad the touchpad
should still work...

Regards,

Hans

Re: [PATCH 0/3] x86/PCI: Clip only partial E820 overlaps

[Mika Westerberg]

Hi,

On Fri, Mar 04, 2022 at 03:15:16PM +0100, Hans de Goede wrote:
> Hi Bjorn,
> 
> On 3/4/22 04:51, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > This is based on Hans' extensive debugging and patch at
> > https://lore.kernel.org/r/20220228105259.230903-1-hdegoede@redhat.com
> > and applies on 7e57714cd0ad ("Linux 5.17-rc6").
> > 
> > This is basically the same idea (applying the 4dc2287c1805 workaround only
> > when an E820 region *partially* overlaps a host bridge window), but I think
> > it's a little simpler.
> > 
> > This also adds a little dmesg output when clipping, which should make
> > future debugging easier.
> > 
> > I bcc'd several folks who didn't have public email addresses in the RedHat
> > bugzilla or Launchpad.  If you review or test this, I'd be happy to
> > acknowledge that.
> > 
> > Bjorn Helgaas (3):
> >   x86/PCI: Eliminate remove_e820_regions() common subexpressions
> >   x86/PCI: Log host bridge window clipping for E820 regions
> >   x86/PCI: Preserve host bridge windows completely covered by E820
> 
> Thanks, I agree that this is better then my fix I also like the logging
> added to 2/3 which lets us know if the commit 4dc2287c1805 workaround
> is active.
> 
> I have one small remark on 3/3. Regardless of that getting addressed
> the entire series is:
> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Looks good to me too :)

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>