[RFC 0/2] x86/PCI: Ignore EFI memmap MMIO entries

[RFC 0/2] x86/PCI: Ignore EFI memmap MMIO entries

[Hans de Goede]

Hi All,

Here is a new attempt at fixing the issue where on some laptops
there are EFI memmap MMIO entries covering the entire PCI bridge
mem window, causing Linux to be unable to find free space to
assign to unassigned BARs.

This is marked as RFC atm because I'm waiting for feedback from
testers.

Regards,

Hans


Hans de Goede (2):
  x86/e820: Map EFI_MEMORY_MAPPED_IO to a new E820_TYPE_MMIO type
  x86/PCI: Ignore EFI memmap MMIO entries

 arch/x86/include/asm/e820/types.h       |  3 +++
 arch/x86/include/asm/iommu.h            |  3 ++-
 arch/x86/kernel/e820.c                  |  5 +++++
 arch/x86/kernel/resource.c              |  4 ++++
 arch/x86/mm/ioremap.c                   |  1 +
 arch/x86/pci/mmconfig-shared.c          | 15 +++++++++++----
 arch/x86/platform/efi/efi.c             |  5 ++++-
 drivers/firmware/efi/libstub/x86-stub.c |  5 ++++-
 8 files changed, 34 insertions(+), 7 deletions(-)

-- 
2.33.1

[RFC 1/2] x86/e820: Map EFI_MEMORY_MAPPED_IO to a new E820_TYPE_MMIO type

[Hans de Goede]

Map EFI_MEMORY_MAPPED_IO to a new E820_TYPE_MMIO type. The EFI memory-map
has a special type for Memory Mapped IO, add a new E820_TYPE_MMIO type and
when translating the EFI memory-map to an e820_table map
EFI_MEMORY_MAPPED_IO to this new E820_TYPE_MMIO type.

This is a preparation patch for making arch_remove_reservations() treat
EFI_MEMORY_MAPPED_IO ranges differently then other reserved ranged.

All users of E820_TYPE_* have been audited and modified where necessary
so that this patch should not introduce any functional changes.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 arch/x86/include/asm/e820/types.h       |  3 +++
 arch/x86/include/asm/iommu.h            |  3 ++-
 arch/x86/kernel/e820.c                  |  5 +++++
 arch/x86/mm/ioremap.c                   |  1 +
 arch/x86/pci/mmconfig-shared.c          | 15 +++++++++++----
 arch/x86/platform/efi/efi.c             |  5 ++++-
 drivers/firmware/efi/libstub/x86-stub.c |  5 ++++-
 7 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/e820/types.h b/arch/x86/include/asm/e820/types.h
index 314f75d886d0..d91d4d28fe50 100644
--- a/arch/x86/include/asm/e820/types.h
+++ b/arch/x86/include/asm/e820/types.h
@@ -44,6 +44,9 @@ enum e820_type {
 	 * might alter over the S3 transition:
 	 */
 	E820_TYPE_RESERVED_KERN	= 128,
+
+	/* Used for EFI_MEMORY_MAPPED_IO when translating the EFI memmap */
+	E820_TYPE_MMIO = 129,
 };
 
 /*
diff --git a/arch/x86/include/asm/iommu.h b/arch/x86/include/asm/iommu.h
index bf1ed2ddc74b..ed7329137fef 100644
--- a/arch/x86/include/asm/iommu.h
+++ b/arch/x86/include/asm/iommu.h
@@ -18,7 +18,8 @@ arch_rmrr_sanity_check(struct acpi_dmar_reserved_memory *rmrr)
 	u64 start = rmrr->base_address;
 	u64 end = rmrr->end_address + 1;
 
-	if (e820__mapped_all(start, end, E820_TYPE_RESERVED))
+	if (e820__mapped_all(start, end, E820_TYPE_RESERVED) ||
+	    e820__mapped_all(start, end, E820_TYPE_MMIO))
 		return 0;
 
 	pr_err(FW_BUG "No firmware reserved region can cover this RMRR [%#018Lx-%#018Lx], contact BIOS vendor for fixes\n",
diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index bc0657f0deed..5fbd2505b10e 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -196,6 +196,7 @@ static void __init e820_print_type(enum e820_type type)
 	case E820_TYPE_UNUSABLE:	pr_cont("unusable");			break;
 	case E820_TYPE_PMEM:		/* Fall through: */
 	case E820_TYPE_PRAM:		pr_cont("persistent (type %u)", type);	break;
+	case E820_TYPE_MMIO:		pr_cont("MMIO");			break;
 	default:			pr_cont("type %u", type);		break;
 	}
 }
@@ -1064,6 +1065,7 @@ static const char *__init e820_type_to_string(struct e820_entry *entry)
 	case E820_TYPE_PMEM:		return "Persistent Memory";
 	case E820_TYPE_RESERVED:	return "Reserved";
 	case E820_TYPE_SOFT_RESERVED:	return "Soft Reserved";
+	case E820_TYPE_MMIO:		return "Memory Mapped IO";
 	default:			return "Unknown E820 type";
 	}
 }
@@ -1080,6 +1082,7 @@ static unsigned long __init e820_type_to_iomem_type(struct e820_entry *entry)
 	case E820_TYPE_PMEM:		/* Fall-through: */
 	case E820_TYPE_RESERVED:	/* Fall-through: */
 	case E820_TYPE_SOFT_RESERVED:	/* Fall-through: */
+	case E820_TYPE_MMIO:		/* Fall-through: */
 	default:			return IORESOURCE_MEM;
 	}
 }
@@ -1091,6 +1094,7 @@ static unsigned long __init e820_type_to_iores_desc(struct e820_entry *entry)
 	case E820_TYPE_NVS:		return IORES_DESC_ACPI_NV_STORAGE;
 	case E820_TYPE_PMEM:		return IORES_DESC_PERSISTENT_MEMORY;
 	case E820_TYPE_PRAM:		return IORES_DESC_PERSISTENT_MEMORY_LEGACY;
+	case E820_TYPE_MMIO:		/* Fall-through: */
 	case E820_TYPE_RESERVED:	return IORES_DESC_RESERVED;
 	case E820_TYPE_SOFT_RESERVED:	return IORES_DESC_SOFT_RESERVED;
 	case E820_TYPE_RESERVED_KERN:	/* Fall-through: */
@@ -1113,6 +1117,7 @@ static bool __init do_mark_busy(enum e820_type type, struct resource *res)
 	switch (type) {
 	case E820_TYPE_RESERVED:
 	case E820_TYPE_SOFT_RESERVED:
+	case E820_TYPE_MMIO:
 	case E820_TYPE_PRAM:
 	case E820_TYPE_PMEM:
 		return false;
diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index 026031b3b782..7987e9c899fa 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -552,6 +552,7 @@ static bool memremap_should_map_decrypted(resource_size_t phys_addr,
 	/* Check if the address is outside kernel usable area */
 	switch (e820__get_entry_type(phys_addr, phys_addr + size - 1)) {
 	case E820_TYPE_RESERVED:
+	case E820_TYPE_MMIO:
 	case E820_TYPE_ACPI:
 	case E820_TYPE_NVS:
 	case E820_TYPE_UNUSABLE:
diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
index 758cbfe55daa..3c19353bae10 100644
--- a/arch/x86/pci/mmconfig-shared.c
+++ b/arch/x86/pci/mmconfig-shared.c
@@ -425,7 +425,7 @@ static acpi_status find_mboard_resource(acpi_handle handle, u32 lvl,
 	return AE_OK;
 }
 
-static bool is_acpi_reserved(u64 start, u64 end, enum e820_type not_used)
+static bool is_acpi_reserved(u64 start, u64 end)
 {
 	struct resource mcfg_res;
 
@@ -442,7 +442,14 @@ static bool is_acpi_reserved(u64 start, u64 end, enum e820_type not_used)
 	return mcfg_res.flags;
 }
 
-typedef bool (*check_reserved_t)(u64 start, u64 end, enum e820_type type);
+static bool is_e820_reserved(u64 start, u64 end)
+{
+	int type = e820__get_entry_type(start, end);
+
+	return type == E820_TYPE_RESERVED || type == E820_TYPE_MMIO;
+}
+
+typedef bool (*check_reserved_t)(u64 start, u64 end);
 
 static bool __ref is_mmconf_reserved(check_reserved_t is_reserved,
 				     struct pci_mmcfg_region *cfg,
@@ -454,7 +461,7 @@ static bool __ref is_mmconf_reserved(check_reserved_t is_reserved,
 	int num_buses;
 	char *method = with_e820 ? "E820" : "ACPI motherboard resources";
 
-	while (!is_reserved(addr, addr + size, E820_TYPE_RESERVED)) {
+	while (!is_reserved(addr, addr + size)) {
 		size >>= 1;
 		if (size < (16UL<<20))
 			break;
@@ -527,7 +534,7 @@ pci_mmcfg_check_reserved(struct device *dev, struct pci_mmcfg_region *cfg, int e
 	/* 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, 1);
+		return is_mmconf_reserved(is_e820_reserved, cfg, dev, 1);
 
 	return false;
 }
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 147c30a81f15..23b361447417 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -173,10 +173,13 @@ static void __init do_add_efi_memmap(void)
 		case EFI_PERSISTENT_MEMORY:
 			e820_type = E820_TYPE_PMEM;
 			break;
+		case EFI_MEMORY_MAPPED_IO:
+			e820_type = E820_TYPE_MMIO;
+			break;
 		default:
 			/*
 			 * EFI_RESERVED_TYPE EFI_RUNTIME_SERVICES_CODE
-			 * EFI_RUNTIME_SERVICES_DATA EFI_MEMORY_MAPPED_IO
+			 * EFI_RUNTIME_SERVICES_DATA
 			 * EFI_MEMORY_MAPPED_IO_PORT_SPACE EFI_PAL_CODE
 			 */
 			e820_type = E820_TYPE_RESERVED;
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index 01ddd4502e28..d0795adc2534 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -470,12 +470,15 @@ setup_e820(struct boot_params *params, struct setup_data *e820ext, u32 e820ext_s
 		case EFI_RESERVED_TYPE:
 		case EFI_RUNTIME_SERVICES_CODE:
 		case EFI_RUNTIME_SERVICES_DATA:
-		case EFI_MEMORY_MAPPED_IO:
 		case EFI_MEMORY_MAPPED_IO_PORT_SPACE:
 		case EFI_PAL_CODE:
 			e820_type = E820_TYPE_RESERVED;
 			break;
 
+		case EFI_MEMORY_MAPPED_IO:
+			e820_type = E820_TYPE_MMIO;
+			break;
+
 		case EFI_UNUSABLE_MEMORY:
 			e820_type = E820_TYPE_UNUSABLE;
 			break;
-- 
2.33.1

[RFC 2/2] x86/PCI: Ignore EFI memmap MMIO entries

[Hans de Goede]

Linux excludes E820 reserved addresses when allocating addresses from the
PCI host bridge window. This behavior is needed for at least 2 reasons:

1. Some BIOS-es contain a bug where they add addresses which map to system
RAM in the PCI host bridge window returned by the ACPI _CRS method, see
commit 4dc2287c1805 ("x86: avoid E820 regions when allocating address
space").

2. At least the Lenovo X1 carbon gen 2 BIOS has an overlap between an
E820 reserved range and the ACPI _CRS providing the PCI bridge windows:
 BIOS-e820: [mem 0x00000000dceff000-0x00000000dfa0ffff] reserved
 pci_bus 0000:00: root bus resource [mem 0xdfa00000-0xfebfffff window]
If Linux assigns the overlapping 0xdfa00000-0xdfa0ffff range to a PCI BAR
then the system fails to resume after a suspend.

Recently (2019) some systems have shown-up with EFI memmap MMIO entries
covering the entire _CRS returned PCI bridge memory window. These memmap
entries get converted into e820_table entries, causing all attempts to
assign memory to PCI BARs which have not been setup by the BIOS to fail.
For example see these dmesg snippets from a Lenovo IdeaPad 3 15IIL 81WE:

 efi: mem63: [MMIO   |RUN|  |  |  |  |  |  |  |  |   |  |  |  |UC] range=
     [0x0000000065400000-0x00000000cfffffff] (1708MB)
 [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]
 pci 0000:00:15.0: BAR 0: failed to assign [mem size 0x00001000 64bit]

Since the problem is specifically caused by EFI memmap entries with
a MMIO type, use the new E820_TYPE_MMIO marking of e820 entries
translated from MMIO EFI memmap entries to skip these entries when
excluding e820 reservations in arch_remove_reservations(), fixing the
problem of not being able to find free space for unassigned BARs.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=206459
BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1868899
BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1871793
BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=2029207
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
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 arch/x86/kernel/resource.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/kernel/resource.c b/arch/x86/kernel/resource.c
index 9b9fb7882c20..bd501f787a10 100644
--- a/arch/x86/kernel/resource.c
+++ b/arch/x86/kernel/resource.c
@@ -31,6 +31,10 @@ static void remove_e820_regions(struct resource *avail)
 	for (i = 0; i < e820_table->nr_entries; i++) {
 		entry = &e820_table->entries[i];
 
+		/* Some fw reserves the entire PCI bridge window as MMIO */
+		if (entry->type == E820_TYPE_MMIO)
+			continue;
+
 		resource_clip(avail, entry->addr,
 			      entry->addr + entry->size - 1);
 	}
-- 
2.33.1

Re: [RFC 0/2] x86/PCI: Ignore EFI memmap MMIO entries

[Hans de Goede]

Hi All,

On 2/14/22 16:17, Hans de Goede wrote:
> Hi All,
> 
> Here is a new attempt at fixing the issue where on some laptops
> there are EFI memmap MMIO entries covering the entire PCI bridge
> mem window, causing Linux to be unable to find free space to
> assign to unassigned BARs.
> 
> This is marked as RFC atm because I'm waiting for feedback from
> testers.

Unfortunately the troublesome 0xdfa00000-0xdfa0ffff region on
the Lenovo X1 carbon gen 2 is marked as MMIO by the EFI memmap,
so the approach from this series won't work.

Interestingly enough this RFC series does seem to help to fix
the suspend/resume on this x1c2, since for some reason merely
splitting the original:

BIOS-e820: [mem 0x00000000dceff000-0x00000000dfa0ffff] reserved

range into:

BIOS-e820: [mem 0x00000000dceff000-0x00000000df9fffff] reserved
BIOS-e820: [mem 0x00000000dfa00000-0x00000000dfa0ffff] MMIO

causes the PCI resource allocation code to pick slightly
different resources avoiding the troublesome overlap, see:
https://bugzilla.redhat.com/show_bug.cgi?id=2029207
for logs.

But I don't think we should rely in this, since from a
arch_remove_reservations() pov the troublesome overlap area
which is now marked as MMIO is fair game for PCI bars with
the change to allow MMIO areas for PCI bars, so things seem
to mostly work by sheer luck after this RFC series.

So now I have yet another plan to fix this (see below) I'll get
that tested and assuming it works post that as a proper patch.

Regards,

Hans



diff --git a/arch/x86/include/asm/pci_x86.h b/arch/x86/include/asm/pci_x86.h
index 490411dba438..573e1323f490 100644
--- a/arch/x86/include/asm/pci_x86.h
+++ b/arch/x86/include/asm/pci_x86.h
@@ -64,6 +64,8 @@ void pcibios_scan_specific_bus(int busn);
 
 /* pci-irq.c */
 
+struct pci_dev;
+
 struct irq_info {
 	u8 bus, devfn;			/* Bus, device and function */
 	struct {
@@ -232,3 +234,9 @@ static inline void mmio_config_writel(void __iomem *pos, u32 val)
 # define x86_default_pci_init_irq	NULL
 # define x86_default_pci_fixup_irqs	NULL
 #endif
+
+#if defined CONFIG_PCI && defined CONFIG_ACPI
+extern bool pci_use_e820;
+#else
+#define pci_use_e820 true
+#endif
diff --git a/arch/x86/kernel/resource.c b/arch/x86/kernel/resource.c
index 9b9fb7882c20..e8dc9bc327bd 100644
--- a/arch/x86/kernel/resource.c
+++ b/arch/x86/kernel/resource.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <linux/ioport.h>
 #include <asm/e820/api.h>
+#include <asm/pci_x86.h>
 
 static void resource_clip(struct resource *res, resource_size_t start,
 			  resource_size_t end)
@@ -28,6 +29,9 @@ static void remove_e820_regions(struct resource *avail)
 	int i;
 	struct e820_entry *entry;
 
+	if (!pci_use_e820)
+		return;
+
 	for (i = 0; i < e820_table->nr_entries; i++) {
 		entry = &e820_table->entries[i];
 
diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
index 052f1d78a562..7167934819b3 100644
--- a/arch/x86/pci/acpi.c
+++ b/arch/x86/pci/acpi.c
@@ -1,4 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0
+#include <linux/efi.h>
 #include <linux/pci.h>
 #include <linux/acpi.h>
 #include <linux/init.h>
@@ -21,6 +22,7 @@ struct pci_root_info {
 
 static bool pci_use_crs = true;
 static bool pci_ignore_seg;
+bool pci_use_e820 = true;
 
 static int __init set_use_crs(const struct dmi_system_id *id)
 {
@@ -291,6 +293,28 @@ static bool resource_is_pcicfg_ioport(struct resource *res)
 		res->start == 0xCF8 && res->end == 0xCFF;
 }
 
+static bool resource_matches_efi_mmio_region(const struct resource *res)
+{
+	unsigned long long start, end;
+	efi_memory_desc_t *md;
+
+	if (!efi_enabled(EFI_MEMMAP))
+		return false;
+
+	for_each_efi_memory_desc(md) {
+		if (md->type != EFI_MEMORY_MAPPED_IO)
+			continue;
+
+		start = md->phys_addr;
+		end = start + (md->num_pages << EFI_PAGE_SHIFT) - 1;
+
+		if (res->start >= start && res->end <= end)
+			return true;
+	}
+
+	return false;
+}
+
 static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
 {
 	struct acpi_device *device = ci->bridge;
@@ -300,9 +324,16 @@ static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
 
 	status = acpi_pci_probe_root_resources(ci);
 	if (pci_use_crs) {
-		resource_list_for_each_entry_safe(entry, tmp, &ci->resources)
+		resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
 			if (resource_is_pcicfg_ioport(entry->res))
 				resource_list_destroy_entry(entry);
+			if (resource_matches_efi_mmio_region(entry->res)) {
+				dev_info(&device->dev,
+					"host bridge window %pR is marked by EFI as MMIO\n",
+					entry->res);
+				pci_use_e820 = false;
+			}
+		}
 		return status;
 	}
 

Re: [RFC 0/2] x86/PCI: Ignore EFI memmap MMIO entries

[Rafael J. Wysocki]

On Tue, Feb 15, 2022 at 5:12 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi All,
>
> On 2/14/22 16:17, Hans de Goede wrote:
> > Hi All,
> >
> > Here is a new attempt at fixing the issue where on some laptops
> > there are EFI memmap MMIO entries covering the entire PCI bridge
> > mem window, causing Linux to be unable to find free space to
> > assign to unassigned BARs.
> >
> > This is marked as RFC atm because I'm waiting for feedback from
> > testers.
>
> Unfortunately the troublesome 0xdfa00000-0xdfa0ffff region on
> the Lenovo X1 carbon gen 2 is marked as MMIO by the EFI memmap,
> so the approach from this series won't work.
>
> Interestingly enough this RFC series does seem to help to fix
> the suspend/resume on this x1c2, since for some reason merely
> splitting the original:
>
> BIOS-e820: [mem 0x00000000dceff000-0x00000000dfa0ffff] reserved
>
> range into:
>
> BIOS-e820: [mem 0x00000000dceff000-0x00000000df9fffff] reserved
> BIOS-e820: [mem 0x00000000dfa00000-0x00000000dfa0ffff] MMIO
>
> causes the PCI resource allocation code to pick slightly
> different resources avoiding the troublesome overlap, see:
> https://bugzilla.redhat.com/show_bug.cgi?id=2029207
> for logs.
>
> But I don't think we should rely in this, since from a
> arch_remove_reservations() pov the troublesome overlap area
> which is now marked as MMIO is fair game for PCI bars with
> the change to allow MMIO areas for PCI bars, so things seem
> to mostly work by sheer luck after this RFC series.
>
> So now I have yet another plan to fix this (see below) I'll get
> that tested and assuming it works post that as a proper patch.
>
> Regards,
>
> Hans
>
>
>
> diff --git a/arch/x86/include/asm/pci_x86.h b/arch/x86/include/asm/pci_x86.h
> index 490411dba438..573e1323f490 100644
> --- a/arch/x86/include/asm/pci_x86.h
> +++ b/arch/x86/include/asm/pci_x86.h
> @@ -64,6 +64,8 @@ void pcibios_scan_specific_bus(int busn);
>
>  /* pci-irq.c */
>
> +struct pci_dev;
> +
>  struct irq_info {
>         u8 bus, devfn;                  /* Bus, device and function */
>         struct {
> @@ -232,3 +234,9 @@ static inline void mmio_config_writel(void __iomem *pos, u32 val)
>  # define x86_default_pci_init_irq      NULL
>  # define x86_default_pci_fixup_irqs    NULL
>  #endif
> +
> +#if defined CONFIG_PCI && defined CONFIG_ACPI
> +extern bool pci_use_e820;
> +#else
> +#define pci_use_e820 true
> +#endif
> diff --git a/arch/x86/kernel/resource.c b/arch/x86/kernel/resource.c
> index 9b9fb7882c20..e8dc9bc327bd 100644
> --- a/arch/x86/kernel/resource.c
> +++ b/arch/x86/kernel/resource.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include <linux/ioport.h>
>  #include <asm/e820/api.h>
> +#include <asm/pci_x86.h>
>
>  static void resource_clip(struct resource *res, resource_size_t start,
>                           resource_size_t end)
> @@ -28,6 +29,9 @@ static void remove_e820_regions(struct resource *avail)
>         int i;
>         struct e820_entry *entry;
>
> +       if (!pci_use_e820)
> +               return;
> +
>         for (i = 0; i < e820_table->nr_entries; i++) {
>                 entry = &e820_table->entries[i];
>
> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> index 052f1d78a562..7167934819b3 100644
> --- a/arch/x86/pci/acpi.c
> +++ b/arch/x86/pci/acpi.c
> @@ -1,4 +1,5 @@
>  // SPDX-License-Identifier: GPL-2.0
> +#include <linux/efi.h>
>  #include <linux/pci.h>
>  #include <linux/acpi.h>
>  #include <linux/init.h>
> @@ -21,6 +22,7 @@ struct pci_root_info {
>
>  static bool pci_use_crs = true;
>  static bool pci_ignore_seg;
> +bool pci_use_e820 = true;
>
>  static int __init set_use_crs(const struct dmi_system_id *id)
>  {
> @@ -291,6 +293,28 @@ static bool resource_is_pcicfg_ioport(struct resource *res)
>                 res->start == 0xCF8 && res->end == 0xCFF;
>  }
>
> +static bool resource_matches_efi_mmio_region(const struct resource *res)

I would call this resource_is_efi_mmio() FWIW.

> +{
> +       unsigned long long start, end;
> +       efi_memory_desc_t *md;
> +
> +       if (!efi_enabled(EFI_MEMMAP))
> +               return false;
> +
> +       for_each_efi_memory_desc(md) {
> +               if (md->type != EFI_MEMORY_MAPPED_IO)
> +                       continue;
> +
> +               start = md->phys_addr;
> +               end = start + (md->num_pages << EFI_PAGE_SHIFT) - 1;
> +
> +               if (res->start >= start && res->end <= end)
> +                       return true;
> +       }
> +
> +       return false;
> +}
> +
>  static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
>  {
>         struct acpi_device *device = ci->bridge;
> @@ -300,9 +324,16 @@ static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
>
>         status = acpi_pci_probe_root_resources(ci);
>         if (pci_use_crs) {
> -               resource_list_for_each_entry_safe(entry, tmp, &ci->resources)
> +               resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
>                         if (resource_is_pcicfg_ioport(entry->res))
>                                 resource_list_destroy_entry(entry);
> +                       if (resource_matches_efi_mmio_region(entry->res)) {

I would add a pci_use_e820 check to this.

> +                               dev_info(&device->dev,
> +                                       "host bridge window %pR is marked by EFI as MMIO\n",
> +                                       entry->res);
> +                               pci_use_e820 = false;
> +                       }
> +               }
>                 return status;
>         }

Overall, it looks reasonable to me.

Re: [RFC 0/2] x86/PCI: Ignore EFI memmap MMIO entries

[Hans de Goede]

Hi,

On 2/15/22 18:20, Rafael J. Wysocki wrote:
> On Tue, Feb 15, 2022 at 5:12 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi All,
>>
>> On 2/14/22 16:17, Hans de Goede wrote:
>>> Hi All,
>>>
>>> Here is a new attempt at fixing the issue where on some laptops
>>> there are EFI memmap MMIO entries covering the entire PCI bridge
>>> mem window, causing Linux to be unable to find free space to
>>> assign to unassigned BARs.
>>>
>>> This is marked as RFC atm because I'm waiting for feedback from
>>> testers.
>>
>> Unfortunately the troublesome 0xdfa00000-0xdfa0ffff region on
>> the Lenovo X1 carbon gen 2 is marked as MMIO by the EFI memmap,
>> so the approach from this series won't work.
>>
>> Interestingly enough this RFC series does seem to help to fix
>> the suspend/resume on this x1c2, since for some reason merely
>> splitting the original:
>>
>> BIOS-e820: [mem 0x00000000dceff000-0x00000000dfa0ffff] reserved
>>
>> range into:
>>
>> BIOS-e820: [mem 0x00000000dceff000-0x00000000df9fffff] reserved
>> BIOS-e820: [mem 0x00000000dfa00000-0x00000000dfa0ffff] MMIO
>>
>> causes the PCI resource allocation code to pick slightly
>> different resources avoiding the troublesome overlap, see:
>> https://bugzilla.redhat.com/show_bug.cgi?id=2029207
>> for logs.
>>
>> But I don't think we should rely in this, since from a
>> arch_remove_reservations() pov the troublesome overlap area
>> which is now marked as MMIO is fair game for PCI bars with
>> the change to allow MMIO areas for PCI bars, so things seem
>> to mostly work by sheer luck after this RFC series.
>>
>> So now I have yet another plan to fix this (see below) I'll get
>> that tested and assuming it works post that as a proper patch.
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>> diff --git a/arch/x86/include/asm/pci_x86.h b/arch/x86/include/asm/pci_x86.h
>> index 490411dba438..573e1323f490 100644
>> --- a/arch/x86/include/asm/pci_x86.h
>> +++ b/arch/x86/include/asm/pci_x86.h
>> @@ -64,6 +64,8 @@ void pcibios_scan_specific_bus(int busn);
>>
>>  /* pci-irq.c */
>>
>> +struct pci_dev;
>> +
>>  struct irq_info {
>>         u8 bus, devfn;                  /* Bus, device and function */
>>         struct {
>> @@ -232,3 +234,9 @@ static inline void mmio_config_writel(void __iomem *pos, u32 val)
>>  # define x86_default_pci_init_irq      NULL
>>  # define x86_default_pci_fixup_irqs    NULL
>>  #endif
>> +
>> +#if defined CONFIG_PCI && defined CONFIG_ACPI
>> +extern bool pci_use_e820;
>> +#else
>> +#define pci_use_e820 true
>> +#endif
>> diff --git a/arch/x86/kernel/resource.c b/arch/x86/kernel/resource.c
>> index 9b9fb7882c20..e8dc9bc327bd 100644
>> --- a/arch/x86/kernel/resource.c
>> +++ b/arch/x86/kernel/resource.c
>> @@ -1,6 +1,7 @@
>>  // SPDX-License-Identifier: GPL-2.0
>>  #include <linux/ioport.h>
>>  #include <asm/e820/api.h>
>> +#include <asm/pci_x86.h>
>>
>>  static void resource_clip(struct resource *res, resource_size_t start,
>>                           resource_size_t end)
>> @@ -28,6 +29,9 @@ static void remove_e820_regions(struct resource *avail)
>>         int i;
>>         struct e820_entry *entry;
>>
>> +       if (!pci_use_e820)
>> +               return;
>> +
>>         for (i = 0; i < e820_table->nr_entries; i++) {
>>                 entry = &e820_table->entries[i];
>>
>> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
>> index 052f1d78a562..7167934819b3 100644
>> --- a/arch/x86/pci/acpi.c
>> +++ b/arch/x86/pci/acpi.c
>> @@ -1,4 +1,5 @@
>>  // SPDX-License-Identifier: GPL-2.0
>> +#include <linux/efi.h>
>>  #include <linux/pci.h>
>>  #include <linux/acpi.h>
>>  #include <linux/init.h>
>> @@ -21,6 +22,7 @@ struct pci_root_info {
>>
>>  static bool pci_use_crs = true;
>>  static bool pci_ignore_seg;
>> +bool pci_use_e820 = true;
>>
>>  static int __init set_use_crs(const struct dmi_system_id *id)
>>  {
>> @@ -291,6 +293,28 @@ static bool resource_is_pcicfg_ioport(struct resource *res)
>>                 res->start == 0xCF8 && res->end == 0xCFF;
>>  }
>>
>> +static bool resource_matches_efi_mmio_region(const struct resource *res)
> 
> I would call this resource_is_efi_mmio() FWIW.

Ack, fixed in my local tree.

> 
>> +{
>> +       unsigned long long start, end;
>> +       efi_memory_desc_t *md;
>> +
>> +       if (!efi_enabled(EFI_MEMMAP))
>> +               return false;
>> +
>> +       for_each_efi_memory_desc(md) {
>> +               if (md->type != EFI_MEMORY_MAPPED_IO)
>> +                       continue;
>> +
>> +               start = md->phys_addr;
>> +               end = start + (md->num_pages << EFI_PAGE_SHIFT) - 1;
>> +
>> +               if (res->start >= start && res->end <= end)
>> +                       return true;
>> +       }
>> +
>> +       return false;
>> +}
>> +
>>  static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
>>  {
>>         struct acpi_device *device = ci->bridge;
>> @@ -300,9 +324,16 @@ static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
>>
>>         status = acpi_pci_probe_root_resources(ci);
>>         if (pci_use_crs) {
>> -               resource_list_for_each_entry_safe(entry, tmp, &ci->resources)
>> +               resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
>>                         if (resource_is_pcicfg_ioport(entry->res))
>>                                 resource_list_destroy_entry(entry);
>> +                       if (resource_matches_efi_mmio_region(entry->res)) {
> 
> I would add a pci_use_e820 check to this.

I'm not sure about that, this code path should run only once per bridge and if multiple
bridges are affected then it would be good to have this info level message for all of
them.

OTOH I guess we only expect this to affect the main window for the root PCI bridge
and then the windows for any bridges below that will also automatically fit within
the same EFI memmap entry, resulting in what is more or less a false-positive
logging of the message.

I've build a test-kernel for both the reporter of the original touchpad (i2c-controller
PCI bar assignment) issue as well as the suspend/resume regression on the x1c2 reporter
to test, which does not have your suggestion. I'll check the logs there and if there
are indeed duplicate log messages I'll implement your suggestion.

Regards,

Hans



> 
>> +                               dev_info(&device->dev,
>> +                                       "host bridge window %pR is marked by EFI as MMIO\n",
>> +                                       entry->res);
>> +                               pci_use_e820 = false;
>> +                       }
>> +               }
>>                 return status;
>>         }
> 
> Overall, it looks reasonable to me.
>