[PATCH v2 0/4] PCI: Continue E820 vs host bridge window saga

[PATCH v2 0/4] PCI: Continue E820 vs host bridge window saga

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

When allocating space for PCI BARs, Linux avoids allocating space mentioned
in the E820 map.  This was originally done by 4dc2287c1805 ("x86: avoid
E820 regions when allocating address space") to work around BIOS defects
that included unusable space in host bridge _CRS.

Some recent machines use EfiMemoryMappedIO for PCI MMCONFIG and host bridge
apertures, and bootloaders and EFI stubs convert those to E820 regions,
which means we can't allocate space for hot-added PCI devices (often a
dock) or for devices the BIOS didn't configure (often a touchpad)

The current strategy is to add DMI quirks that disable the E820 filtering
on these machines and to disable it entirely starting with 2023 BIOSes:

  d341838d776a ("x86/PCI: Disable E820 reserved region clipping via quirks")
  0ae084d5a674 ("x86/PCI: Disable E820 reserved region clipping starting in 2023")

But the quirks are problematic because it's really hard to list all the
machines that need them.

This series is an attempt at a more generic approach.  I'm told by firmware
folks that EfiMemoryMappedIO means "the OS should map this area so EFI
runtime services can use it in virtual mode," but does not prevent the OS
from using it.

The first patch removes large EfiMemoryMappedIO areas from the E820 map.
This doesn't affect any virtual mapping of those areas (that would have to
be done directly from the EFI memory map) but it means Linux can allocate
space for PCI MMIO.

The rest are basically cosmetic log message changes.

Changes from v1 to v2:
- Remove only large (>= 256KB) EfiMemoryMappedIO areas from E820 to avoid
  the Lenovo X1 Carbon suspend/resume problems.  This machine includes
  64KB of non-window space in the PNP0A03 _CRS, and a corresponding
  EfiMemoryMappedIO area seems to be the only clue to avoid it (see
  https://bugzilla.redhat.com/show_bug.cgi?id=2029207).  Interdiff below.


Bjorn Helgaas (4):
  efi/x86: Remove EfiMemoryMappedIO from E820 map
  PCI: Skip allocate_resource() if too little space available
  x86/PCI: Tidy E820 removal messages
  x86/PCI: Fix log message typo

 arch/x86/kernel/resource.c  |  8 +++++--
 arch/x86/pci/acpi.c         |  2 +-
 arch/x86/platform/efi/efi.c | 46 +++++++++++++++++++++++++++++++++++++
 drivers/pci/bus.c           |  4 ++++
 4 files changed, 57 insertions(+), 3 deletions(-)

-- 
2.25.1

  diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
  index 4728f60119da..dee1852e95cd 100644
  --- a/arch/x86/platform/efi/efi.c
  +++ b/arch/x86/platform/efi/efi.c
  @@ -315,8 +315,12 @@ static void __init efi_clean_memmap(void)
    * PCI host bridge windows, which means Linux can't allocate BAR space for
    * hot-added devices.
    *
  - * Remove any EfiMemoryMappedIO regions from the E820 map to avoid this
  + * Remove large EfiMemoryMappedIO regions from the E820 map to avoid this
    * problem.
  + *
  + * Retain small EfiMemoryMappedIO regions because on some platforms, these
  + * describe non-window space that's included in host bridge _CRS.  If we
  + * assign that space to PCI devices, they don't work.
    */
   static void __init efi_remove_e820_mmio(void)
   {
  @@ -327,11 +331,17 @@ static void __init efi_remove_e820_mmio(void)
	  for_each_efi_memory_desc(md) {
		  if (md->type == EFI_MEMORY_MAPPED_IO) {
			  size = md->num_pages << EFI_PAGE_SHIFT;
  -			start = md->phys_addr;
  -			end = start + size - 1;
  -			pr_info("Remove mem%02u: MMIO range=[0x%08llx-0x%08llx] (%lluMB) from e820 map\n",
  -				i, start, end, size >> 20);
  -			e820__range_remove(start, size, E820_TYPE_RESERVED, 1);
  +			if (size >= 256*1024) {
  +				start = md->phys_addr;
  +				end = start + size - 1;
  +				pr_info("Remove mem%02u: MMIO range=[0x%08llx-0x%08llx] (%lluMB) from e820 map\n",
  +					i, start, end, size >> 20);
  +				e820__range_remove(start, size,
  +						   E820_TYPE_RESERVED, 1);
  +			} else {
  +				pr_info("Not removing mem%02u: MMIO range=[0x%08llx-0x%08llx] (%lluKB) from e820 map\n",
  +					i, start, end, size >> 10);
  +			}
		  }
		  i++;
	  }

[PATCH v2 1/4] efi/x86: Remove EfiMemoryMappedIO from E820 map

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Firmware can use EfiMemoryMappedIO to request that MMIO regions be mapped
by the OS so they can be accessed by EFI runtime services, but should have
no other significance to the OS (UEFI r2.10, sec 7.2).  However, most
bootloaders and EFI stubs convert EfiMemoryMappedIO regions to
E820_TYPE_RESERVED entries, which prevent Linux from allocating space from
them (see remove_e820_regions()).

Some platforms use EfiMemoryMappedIO entries for PCI MMCONFIG space and PCI
host bridge windows, which means Linux can't allocate BAR space for
hot-added devices.

Remove large EfiMemoryMappedIO regions from the E820 map to avoid this
problem.

Leave small (< 256KB) EfiMemoryMappedIO regions alone because on some
platforms, these describe non-window space that's included in host bridge
_CRS.  If we assign that space to PCI devices, they don't work.  On the
Lenovo X1 Carbon, this leads to suspend/resume failures.

The previous solution to the problem of allocating BARs in these regions
was to add pci_crs_quirks[] entries to disable E820 checking for these
machines (see d341838d776a ("x86/PCI: Disable E820 reserved region clipping
via quirks")):

  Acer   DMI_PRODUCT_NAME    Spin SP513-54N
  Clevo  DMI_BOARD_NAME      X170KM-G
  Lenovo DMI_PRODUCT_VERSION *IIL*

Florent reported the BAR allocation issue on the Clevo NL4XLU.  We could
add another quirk for the NL4XLU, but I hope this generic change can solve
it for many machines without having to add quirks.

This change has been tested on Clevo X170KM-G (Konrad) and Lenovo Ideapad
Slim 3 (Matt) and solves the problem even when overriding the existing
quirks by booting with "pci=use_e820".

Link: https://bugzilla.kernel.org/show_bug.cgi?id=216565     Clevo NL4XLU
Link: https://bugzilla.kernel.org/show_bug.cgi?id=206459#c78 Clevo X170KM-G
Link: https://bugzilla.redhat.com/show_bug.cgi?id=1868899    Ideapad Slim 3
Link: https://bugzilla.redhat.com/show_bug.cgi?id=2029207    X1 Carbon
Reported-by: Florent DELAHAYE <kernelorg@undead.fr>
Tested-by: Konrad J Hambrick <kjhambrick@gmail.com>
Tested-by: Matt Hansen <2lprbe78@duck.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Cc: Hans de Goede <hdegoede@redhat.com>
---
 arch/x86/platform/efi/efi.c | 46 +++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index ebc98a68c400..dee1852e95cd 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -303,6 +303,50 @@ static void __init efi_clean_memmap(void)
 	}
 }
 
+/*
+ * Firmware can use EfiMemoryMappedIO to request that MMIO regions be
+ * mapped by the OS so they can be accessed by EFI runtime services, but
+ * should have no other significance to the OS (UEFI r2.10, sec 7.2).
+ * However, most bootloaders and EFI stubs convert EfiMemoryMappedIO
+ * regions to E820_TYPE_RESERVED entries, which prevent Linux from
+ * allocating space from them (see remove_e820_regions()).
+ *
+ * Some platforms use EfiMemoryMappedIO entries for PCI MMCONFIG space and
+ * PCI host bridge windows, which means Linux can't allocate BAR space for
+ * hot-added devices.
+ *
+ * Remove large EfiMemoryMappedIO regions from the E820 map to avoid this
+ * problem.
+ *
+ * Retain small EfiMemoryMappedIO regions because on some platforms, these
+ * describe non-window space that's included in host bridge _CRS.  If we
+ * assign that space to PCI devices, they don't work.
+ */
+static void __init efi_remove_e820_mmio(void)
+{
+	efi_memory_desc_t *md;
+	u64 size, start, end;
+	int i = 0;
+
+	for_each_efi_memory_desc(md) {
+		if (md->type == EFI_MEMORY_MAPPED_IO) {
+			size = md->num_pages << EFI_PAGE_SHIFT;
+			if (size >= 256*1024) {
+				start = md->phys_addr;
+				end = start + size - 1;
+				pr_info("Remove mem%02u: MMIO range=[0x%08llx-0x%08llx] (%lluMB) from e820 map\n",
+					i, start, end, size >> 20);
+				e820__range_remove(start, size,
+						   E820_TYPE_RESERVED, 1);
+			} else {
+				pr_info("Not removing mem%02u: MMIO range=[0x%08llx-0x%08llx] (%lluKB) from e820 map\n",
+					i, start, end, size >> 10);
+			}
+		}
+		i++;
+	}
+}
+
 void __init efi_print_memmap(void)
 {
 	efi_memory_desc_t *md;
@@ -474,6 +518,8 @@ void __init efi_init(void)
 	set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
 	efi_clean_memmap();
 
+	efi_remove_e820_mmio();
+
 	if (efi_enabled(EFI_DBG))
 		efi_print_memmap();
 }
-- 
2.25.1

[PATCH v2 2/4] PCI: Skip allocate_resource() if too little space available

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

pci_bus_alloc_from_region() allocates MMIO space by iterating through all
the resources available on the bus.  The available resource might be
reduced if the caller requires 32-bit space or we're avoiding BIOS or E820
areas.

Don't bother calling allocate_resource() if we need more space than is
available in this resource.  This prevents some pointless and annoying
messages about avoided areas.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Cc: Hans de Goede <hdegoede@redhat.com>
---
 drivers/pci/bus.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 3cef835b375f..83ae838ceb5f 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -197,6 +197,10 @@ static int pci_bus_alloc_from_region(struct pci_bus *bus, struct resource *res,
 
 		max = avail.end;
 
+		/* Don't bother if available space isn't large enough */
+		if (size > max - min_used + 1)
+			continue;
+
 		/* Ok, try it out.. */
 		ret = allocate_resource(r, res, size, min_used, max,
 					align, alignf, alignf_data);
-- 
2.25.1

[PATCH v2 3/4] x86/PCI: Tidy E820 removal messages

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

These messages:

  clipped [mem size 0x00000000 64bit] to [mem size 0xfffffffffffa0000 64bit] for e820 entry [mem 0x0009f000-0x000fffff]

aren't as useful as they could be because (a) the resource is often
IORESOURCE_UNSET, so we print the size instead of the start/end and (b) we
print the available resource even if it is empty after removing the E820
entry.

Print the available space by hand to avoid the IORESOURCE_UNSET problem and
only if it's non-empty.  No functional change intended.

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

diff --git a/arch/x86/kernel/resource.c b/arch/x86/kernel/resource.c
index bba1abd05bfe..7543a13c8520 100644
--- a/arch/x86/kernel/resource.c
+++ b/arch/x86/kernel/resource.c
@@ -42,8 +42,12 @@ static void remove_e820_regions(struct resource *avail)
 
 		resource_clip(avail, e820_start, e820_end);
 		if (orig.start != avail->start || orig.end != avail->end) {
-			pr_info("clipped %pR to %pR for e820 entry [mem %#010Lx-%#010Lx]\n",
-				 &orig, avail, e820_start, e820_end);
+			pr_info("resource: avoiding allocation from e820 entry [mem %#010Lx-%#010Lx]\n",
+				e820_start, e820_end);
+			if (avail->end > avail->start)
+				pr_info("resource: remaining [mem %#010llx-%#010llx] available\n",
+					(unsigned long long) avail->start,
+					(unsigned long long) avail->end);
 			orig = *avail;
 		}
 	}
-- 
2.25.1

[PATCH v2 4/4] x86/PCI: Fix log message typo

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Add missing word in the log message:

  - ... so future kernels can this automatically
  + ... so future kernels can do this automatically

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Cc: Hans de Goede <hdegoede@redhat.com>
---
 arch/x86/pci/acpi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
index 2f82480fd430..83dfea9e9894 100644
--- a/arch/x86/pci/acpi.c
+++ b/arch/x86/pci/acpi.c
@@ -245,7 +245,7 @@ void __init pci_acpi_crs_quirks(void)
 	printk(KERN_INFO "PCI: %s E820 reservations for host bridge windows\n",
 	       pci_use_e820 ? "Using" : "Ignoring");
 	if (pci_probe & (PCI_NO_E820 | PCI_USE_E820))
-		printk(KERN_INFO "PCI: Please notify linux-pci@vger.kernel.org so future kernels can this automatically\n");
+		printk(KERN_INFO "PCI: Please notify linux-pci@vger.kernel.org so future kernels can do this automatically\n");
 }
 
 #ifdef	CONFIG_PCI_MMCONFIG
-- 
2.25.1

Re: [PATCH v2 0/4] PCI: Continue E820 vs host bridge window saga

[Hans de Goede]

Hi,

On 12/8/22 20:03, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> When allocating space for PCI BARs, Linux avoids allocating space mentioned
> in the E820 map.  This was originally done by 4dc2287c1805 ("x86: avoid
> E820 regions when allocating address space") to work around BIOS defects
> that included unusable space in host bridge _CRS.
> 
> Some recent machines use EfiMemoryMappedIO for PCI MMCONFIG and host bridge
> apertures, and bootloaders and EFI stubs convert those to E820 regions,
> which means we can't allocate space for hot-added PCI devices (often a
> dock) or for devices the BIOS didn't configure (often a touchpad)
> 
> The current strategy is to add DMI quirks that disable the E820 filtering
> on these machines and to disable it entirely starting with 2023 BIOSes:
> 
>   d341838d776a ("x86/PCI: Disable E820 reserved region clipping via quirks")
>   0ae084d5a674 ("x86/PCI: Disable E820 reserved region clipping starting in 2023")
> 
> But the quirks are problematic because it's really hard to list all the
> machines that need them.
> 
> This series is an attempt at a more generic approach.  I'm told by firmware
> folks that EfiMemoryMappedIO means "the OS should map this area so EFI
> runtime services can use it in virtual mode," but does not prevent the OS
> from using it.
> 
> The first patch removes large EfiMemoryMappedIO areas from the E820 map.
> This doesn't affect any virtual mapping of those areas (that would have to
> be done directly from the EFI memory map) but it means Linux can allocate
> space for PCI MMIO.
> 
> The rest are basically cosmetic log message changes.
> 
> Changes from v1 to v2:
> - Remove only large (>= 256KB) EfiMemoryMappedIO areas from E820 to avoid
>   the Lenovo X1 Carbon suspend/resume problems.  This machine includes
>   64KB of non-window space in the PNP0A03 _CRS, and a corresponding
>   EfiMemoryMappedIO area seems to be the only clue to avoid it (see
>   https://bugzilla.redhat.com/show_bug.cgi?id=2029207).  Interdiff below.

Thanks, v2 looks good to me:

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

for the series.

I have also started a Fedora kernel test-build with this series added
and asked the reporters of the ideapad touchpad + x1 carbon suspend/resume:

https://bugzilla.redhat.com/show_bug.cgi?id=1868899
https://bugzilla.redhat.com/show_bug.cgi?id=2029207

bugs to test.

Regards,

Hans



> Bjorn Helgaas (4):
>   efi/x86: Remove EfiMemoryMappedIO from E820 map
>   PCI: Skip allocate_resource() if too little space available
>   x86/PCI: Tidy E820 removal messages
>   x86/PCI: Fix log message typo
> 
>  arch/x86/kernel/resource.c  |  8 +++++--
>  arch/x86/pci/acpi.c         |  2 +-
>  arch/x86/platform/efi/efi.c | 46 +++++++++++++++++++++++++++++++++++++
>  drivers/pci/bus.c           |  4 ++++
>  4 files changed, 57 insertions(+), 3 deletions(-)
> 

Re: [PATCH v2 1/4] efi/x86: Remove EfiMemoryMappedIO from E820 map

[Hans de Goede]

Hi,

One comment (logging bug in patch) below:

On 12/8/22 20:03, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Firmware can use EfiMemoryMappedIO to request that MMIO regions be mapped
> by the OS so they can be accessed by EFI runtime services, but should have
> no other significance to the OS (UEFI r2.10, sec 7.2).  However, most
> bootloaders and EFI stubs convert EfiMemoryMappedIO regions to
> E820_TYPE_RESERVED entries, which prevent Linux from allocating space from
> them (see remove_e820_regions()).
> 
> Some platforms use EfiMemoryMappedIO entries for PCI MMCONFIG space and PCI
> host bridge windows, which means Linux can't allocate BAR space for
> hot-added devices.
> 
> Remove large EfiMemoryMappedIO regions from the E820 map to avoid this
> problem.
> 
> Leave small (< 256KB) EfiMemoryMappedIO regions alone because on some
> platforms, these describe non-window space that's included in host bridge
> _CRS.  If we assign that space to PCI devices, they don't work.  On the
> Lenovo X1 Carbon, this leads to suspend/resume failures.
> 
> The previous solution to the problem of allocating BARs in these regions
> was to add pci_crs_quirks[] entries to disable E820 checking for these
> machines (see d341838d776a ("x86/PCI: Disable E820 reserved region clipping
> via quirks")):
> 
>   Acer   DMI_PRODUCT_NAME    Spin SP513-54N
>   Clevo  DMI_BOARD_NAME      X170KM-G
>   Lenovo DMI_PRODUCT_VERSION *IIL*
> 
> Florent reported the BAR allocation issue on the Clevo NL4XLU.  We could
> add another quirk for the NL4XLU, but I hope this generic change can solve
> it for many machines without having to add quirks.
> 
> This change has been tested on Clevo X170KM-G (Konrad) and Lenovo Ideapad
> Slim 3 (Matt) and solves the problem even when overriding the existing
> quirks by booting with "pci=use_e820".
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216565     Clevo NL4XLU
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206459#c78 Clevo X170KM-G
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=1868899    Ideapad Slim 3
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2029207    X1 Carbon
> Reported-by: Florent DELAHAYE <kernelorg@undead.fr>
> Tested-by: Konrad J Hambrick <kjhambrick@gmail.com>
> Tested-by: Matt Hansen <2lprbe78@duck.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> ---
>  arch/x86/platform/efi/efi.c | 46 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index ebc98a68c400..dee1852e95cd 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -303,6 +303,50 @@ static void __init efi_clean_memmap(void)
>  	}
>  }
>  
> +/*
> + * Firmware can use EfiMemoryMappedIO to request that MMIO regions be
> + * mapped by the OS so they can be accessed by EFI runtime services, but
> + * should have no other significance to the OS (UEFI r2.10, sec 7.2).
> + * However, most bootloaders and EFI stubs convert EfiMemoryMappedIO
> + * regions to E820_TYPE_RESERVED entries, which prevent Linux from
> + * allocating space from them (see remove_e820_regions()).
> + *
> + * Some platforms use EfiMemoryMappedIO entries for PCI MMCONFIG space and
> + * PCI host bridge windows, which means Linux can't allocate BAR space for
> + * hot-added devices.
> + *
> + * Remove large EfiMemoryMappedIO regions from the E820 map to avoid this
> + * problem.
> + *
> + * Retain small EfiMemoryMappedIO regions because on some platforms, these
> + * describe non-window space that's included in host bridge _CRS.  If we
> + * assign that space to PCI devices, they don't work.
> + */
> +static void __init efi_remove_e820_mmio(void)
> +{
> +	efi_memory_desc_t *md;
> +	u64 size, start, end;
> +	int i = 0;
> +
> +	for_each_efi_memory_desc(md) {
> +		if (md->type == EFI_MEMORY_MAPPED_IO) {
> +			size = md->num_pages << EFI_PAGE_SHIFT;
> +			if (size >= 256*1024) {
> +				start = md->phys_addr;
> +				end = start + size - 1;
> +				pr_info("Remove mem%02u: MMIO range=[0x%08llx-0x%08llx] (%lluMB) from e820 map\n",
> +					i, start, end, size >> 20);
> +				e820__range_remove(start, size,
> +						   E820_TYPE_RESERVED, 1);
> +			} else {
> +				pr_info("Not removing mem%02u: MMIO range=[0x%08llx-0x%08llx] (%lluKB) from e820 map\n",
> +					i, start, end, size >> 10);

The logging in this else is re-using the start and end from the previous section which was actually removed.

E.g. Matt's latest log from:
https://bugzilla.redhat.com/show_bug.cgi?id=1868899
has:

[    0.000000] e820: remove [mem 0xfc800000-0xfe7fffff] reserved
[    0.000000] efi: Not removing mem46: MMIO range=[0xfc800000-0xfe7fffff] (4KB) from e820 map
[    0.000000] efi: Not removing mem47: MMIO range=[0xfc800000-0xfe7fffff] (32KB) from e820 map
[    0.000000] efi: Not removing mem49: MMIO range=[0xfc800000-0xfe7fffff] (8KB) from e820 map
[    0.000000] efi: Not removing mem50: MMIO range=[0xfc800000-0xfe7fffff] (4KB) from e820 map

Notice how all the "Not removing ..." lines log the same range as
the actually removed map entry above them.

Regards,

Hans








> +			}
> +		}
> +		i++;
> +	}
> +}
> +
>  void __init efi_print_memmap(void)
>  {
>  	efi_memory_desc_t *md;
> @@ -474,6 +518,8 @@ void __init efi_init(void)
>  	set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
>  	efi_clean_memmap();
>  
> +	efi_remove_e820_mmio();
> +
>  	if (efi_enabled(EFI_DBG))
>  		efi_print_memmap();
>  }

Re: [PATCH v2 1/4] efi/x86: Remove EfiMemoryMappedIO from E820 map

[Hans de Goede]

Hi,

On 12/9/22 09:06, Hans de Goede wrote:
> Hi,
> 
> One comment (logging bug in patch) below:
> 
> On 12/8/22 20:03, Bjorn Helgaas wrote:
>> From: Bjorn Helgaas <bhelgaas@google.com>
>>
>> Firmware can use EfiMemoryMappedIO to request that MMIO regions be mapped
>> by the OS so they can be accessed by EFI runtime services, but should have
>> no other significance to the OS (UEFI r2.10, sec 7.2).  However, most
>> bootloaders and EFI stubs convert EfiMemoryMappedIO regions to
>> E820_TYPE_RESERVED entries, which prevent Linux from allocating space from
>> them (see remove_e820_regions()).
>>
>> Some platforms use EfiMemoryMappedIO entries for PCI MMCONFIG space and PCI
>> host bridge windows, which means Linux can't allocate BAR space for
>> hot-added devices.
>>
>> Remove large EfiMemoryMappedIO regions from the E820 map to avoid this
>> problem.
>>
>> Leave small (< 256KB) EfiMemoryMappedIO regions alone because on some
>> platforms, these describe non-window space that's included in host bridge
>> _CRS.  If we assign that space to PCI devices, they don't work.  On the
>> Lenovo X1 Carbon, this leads to suspend/resume failures.
>>
>> The previous solution to the problem of allocating BARs in these regions
>> was to add pci_crs_quirks[] entries to disable E820 checking for these
>> machines (see d341838d776a ("x86/PCI: Disable E820 reserved region clipping
>> via quirks")):
>>
>>   Acer   DMI_PRODUCT_NAME    Spin SP513-54N
>>   Clevo  DMI_BOARD_NAME      X170KM-G
>>   Lenovo DMI_PRODUCT_VERSION *IIL*
>>
>> Florent reported the BAR allocation issue on the Clevo NL4XLU.  We could
>> add another quirk for the NL4XLU, but I hope this generic change can solve
>> it for many machines without having to add quirks.
>>
>> This change has been tested on Clevo X170KM-G (Konrad) and Lenovo Ideapad
>> Slim 3 (Matt) and solves the problem even when overriding the existing
>> quirks by booting with "pci=use_e820".
>>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216565     Clevo NL4XLU
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206459#c78 Clevo X170KM-G
>> Link: https://bugzilla.redhat.com/show_bug.cgi?id=1868899    Ideapad Slim 3
>> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2029207    X1 Carbon
>> Reported-by: Florent DELAHAYE <kernelorg@undead.fr>
>> Tested-by: Konrad J Hambrick <kjhambrick@gmail.com>
>> Tested-by: Matt Hansen <2lprbe78@duck.com>
>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>> Cc: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  arch/x86/platform/efi/efi.c | 46 +++++++++++++++++++++++++++++++++++++
>>  1 file changed, 46 insertions(+)
>>
>> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
>> index ebc98a68c400..dee1852e95cd 100644
>> --- a/arch/x86/platform/efi/efi.c
>> +++ b/arch/x86/platform/efi/efi.c
>> @@ -303,6 +303,50 @@ static void __init efi_clean_memmap(void)
>>  	}
>>  }
>>  
>> +/*
>> + * Firmware can use EfiMemoryMappedIO to request that MMIO regions be
>> + * mapped by the OS so they can be accessed by EFI runtime services, but
>> + * should have no other significance to the OS (UEFI r2.10, sec 7.2).
>> + * However, most bootloaders and EFI stubs convert EfiMemoryMappedIO
>> + * regions to E820_TYPE_RESERVED entries, which prevent Linux from
>> + * allocating space from them (see remove_e820_regions()).
>> + *
>> + * Some platforms use EfiMemoryMappedIO entries for PCI MMCONFIG space and
>> + * PCI host bridge windows, which means Linux can't allocate BAR space for
>> + * hot-added devices.
>> + *
>> + * Remove large EfiMemoryMappedIO regions from the E820 map to avoid this
>> + * problem.
>> + *
>> + * Retain small EfiMemoryMappedIO regions because on some platforms, these
>> + * describe non-window space that's included in host bridge _CRS.  If we
>> + * assign that space to PCI devices, they don't work.
>> + */
>> +static void __init efi_remove_e820_mmio(void)
>> +{
>> +	efi_memory_desc_t *md;
>> +	u64 size, start, end;
>> +	int i = 0;
>> +
>> +	for_each_efi_memory_desc(md) {
>> +		if (md->type == EFI_MEMORY_MAPPED_IO) {
>> +			size = md->num_pages << EFI_PAGE_SHIFT;
>> +			if (size >= 256*1024) {
>> +				start = md->phys_addr;
>> +				end = start + size - 1;
>> +				pr_info("Remove mem%02u: MMIO range=[0x%08llx-0x%08llx] (%lluMB) from e820 map\n",
>> +					i, start, end, size >> 20);
>> +				e820__range_remove(start, size,
>> +						   E820_TYPE_RESERVED, 1);
>> +			} else {
>> +				pr_info("Not removing mem%02u: MMIO range=[0x%08llx-0x%08llx] (%lluKB) from e820 map\n",
>> +					i, start, end, size >> 10);
> 
> The logging in this else is re-using the start and end from the previous section which was actually removed.
> 
> E.g. Matt's latest log from:
> https://bugzilla.redhat.com/show_bug.cgi?id=1868899
> has:
> 
> [    0.000000] e820: remove [mem 0xfc800000-0xfe7fffff] reserved
> [    0.000000] efi: Not removing mem46: MMIO range=[0xfc800000-0xfe7fffff] (4KB) from e820 map
> [    0.000000] efi: Not removing mem47: MMIO range=[0xfc800000-0xfe7fffff] (32KB) from e820 map
> [    0.000000] efi: Not removing mem49: MMIO range=[0xfc800000-0xfe7fffff] (8KB) from e820 map
> [    0.000000] efi: Not removing mem50: MMIO range=[0xfc800000-0xfe7fffff] (4KB) from e820 map
> 
> Notice how all the "Not removing ..." lines log the same range as
> the actually removed map entry above them.

I realize the fix is very obvious, but since I just fixed this in my
local tree anyways, here is my fix for this:

--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -331,9 +331,9 @@ static void __init efi_remove_e820_mmio(void)
 	for_each_efi_memory_desc(md) {
 		if (md->type == EFI_MEMORY_MAPPED_IO) {
 			size = md->num_pages << EFI_PAGE_SHIFT;
+			start = md->phys_addr;
+			end = start + size - 1;
 			if (size >= 256*1024) {
-				start = md->phys_addr;
-				end = start + size - 1;
 				pr_info("Remove mem%02u: MMIO range=[0x%08llx-0x%08llx] (%lluMB) from e820 map\n",
 					i, start, end, size >> 20);
 				e820__range_remove(start, size,

Regards,

Hans






>> +			}
>> +		}
>> +		i++;
>> +	}
>> +}
>> +
>>  void __init efi_print_memmap(void)
>>  {
>>  	efi_memory_desc_t *md;
>> @@ -474,6 +518,8 @@ void __init efi_init(void)
>>  	set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
>>  	efi_clean_memmap();
>>  
>> +	efi_remove_e820_mmio();
>> +
>>  	if (efi_enabled(EFI_DBG))
>>  		efi_print_memmap();
>>  }
> 

Re: [PATCH v2 3/4] x86/PCI: Tidy E820 removal messages

[Andy Shevchenko]

On Thu, Dec 08, 2022 at 01:03:40PM -0600, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> These messages:
> 
>   clipped [mem size 0x00000000 64bit] to [mem size 0xfffffffffffa0000 64bit] for e820 entry [mem 0x0009f000-0x000fffff]
> 
> aren't as useful as they could be because (a) the resource is often
> IORESOURCE_UNSET, so we print the size instead of the start/end and (b) we
> print the available resource even if it is empty after removing the E820
> entry.
> 
> Print the available space by hand to avoid the IORESOURCE_UNSET problem and
> only if it's non-empty.  No functional change intended.

...

> +			if (avail->end > avail->start)
> +				pr_info("resource: remaining [mem %#010llx-%#010llx] available\n",
> +					(unsigned long long) avail->start,
> +					(unsigned long long) avail->end);

Is there any point why we do not use %pa for resource_size_t parameters?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/4] x86/PCI: Fix log message typo

[Andy Shevchenko]

On Thu, Dec 08, 2022 at 01:03:41PM -0600, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Add missing word in the log message:
> 
>   - ... so future kernels can this automatically
>   + ... so future kernels can do this automatically

...

>  	printk(KERN_INFO "PCI: %s E820 reservations for host bridge windows\n",
>  	       pci_use_e820 ? "Using" : "Ignoring");
>  	if (pci_probe & (PCI_NO_E820 | PCI_USE_E820))
> -		printk(KERN_INFO "PCI: Please notify linux-pci@vger.kernel.org so future kernels can this automatically\n");
> +		printk(KERN_INFO "PCI: Please notify linux-pci@vger.kernel.org so future kernels can do this automatically\n");

Wondering if we can change printk(KERN_LVL) to pr_lvl() in this file.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/4] efi/x86: Remove EfiMemoryMappedIO from E820 map

[Bjorn Helgaas]

On Fri, Dec 09, 2022 at 12:04:53PM +0100, Hans de Goede wrote:
> On 12/9/22 09:06, Hans de Goede wrote:
> > One comment (logging bug in patch) below:
> > ...

> > The logging in this else is re-using the start and end from the previous section which was actually removed.
> > 
> > E.g. Matt's latest log from:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1868899
> > has:
> > 
> > [    0.000000] e820: remove [mem 0xfc800000-0xfe7fffff] reserved
> > [    0.000000] efi: Not removing mem46: MMIO range=[0xfc800000-0xfe7fffff] (4KB) from e820 map
> > [    0.000000] efi: Not removing mem47: MMIO range=[0xfc800000-0xfe7fffff] (32KB) from e820 map
> > [    0.000000] efi: Not removing mem49: MMIO range=[0xfc800000-0xfe7fffff] (8KB) from e820 map
> > [    0.000000] efi: Not removing mem50: MMIO range=[0xfc800000-0xfe7fffff] (4KB) from e820 map
> > 
> > Notice how all the "Not removing ..." lines log the same range as
> > the actually removed map entry above them.
> 
> I realize the fix is very obvious, but since I just fixed this in my
> local tree anyways, here is my fix for this:

Thank you!  Incorporated.

> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -331,9 +331,9 @@ static void __init efi_remove_e820_mmio(void)
>  	for_each_efi_memory_desc(md) {
>  		if (md->type == EFI_MEMORY_MAPPED_IO) {
>  			size = md->num_pages << EFI_PAGE_SHIFT;
> +			start = md->phys_addr;
> +			end = start + size - 1;
>  			if (size >= 256*1024) {
> -				start = md->phys_addr;
> -				end = start + size - 1;
>  				pr_info("Remove mem%02u: MMIO range=[0x%08llx-0x%08llx] (%lluMB) from e820 map\n",
>  					i, start, end, size >> 20);
>  				e820__range_remove(start, size,
> 

Re: [PATCH v2 3/4] x86/PCI: Tidy E820 removal messages

[Bjorn Helgaas]

On Fri, Dec 09, 2022 at 08:42:06PM +0200, Andy Shevchenko wrote:
> On Thu, Dec 08, 2022 at 01:03:40PM -0600, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > These messages:
> > 
> >   clipped [mem size 0x00000000 64bit] to [mem size 0xfffffffffffa0000 64bit] for e820 entry [mem 0x0009f000-0x000fffff]
> > 
> > aren't as useful as they could be because (a) the resource is often
> > IORESOURCE_UNSET, so we print the size instead of the start/end and (b) we
> > print the available resource even if it is empty after removing the E820
> > entry.
> > 
> > Print the available space by hand to avoid the IORESOURCE_UNSET problem and
> > only if it's non-empty.  No functional change intended.
> 
> ...
> 
> > +			if (avail->end > avail->start)
> > +				pr_info("resource: remaining [mem %#010llx-%#010llx] available\n",
> > +					(unsigned long long) avail->start,
> > +					(unsigned long long) avail->end);
> 
> Is there any point why we do not use %pa for resource_size_t parameters?

Only my ignorance :)  Thanks for pointing that out; I changed it to
this and added a comment about why:


--- a/arch/x86/kernel/resource.c
+++ b/arch/x86/kernel/resource.c
@@ -42,8 +42,16 @@ static void remove_e820_regions(struct resource *avail)
 
 		resource_clip(avail, e820_start, e820_end);
 		if (orig.start != avail->start || orig.end != avail->end) {
-			pr_info("clipped %pR to %pR for e820 entry [mem %#010Lx-%#010Lx]\n",
-				 &orig, avail, e820_start, e820_end);
+			pr_info("resource: avoiding allocation from e820 entry [mem %#010Lx-%#010Lx]\n",
+				e820_start, e820_end);
+			if (avail->end > avail->start)
+				/*
+				 * Use %pa instead of %pR because "avail"
+				 * is typically IORESOURCE_UNSET, so %pR
+				 * shows the size instead of addresses.
+				 */
+				pr_info("resource: remaining [mem %pa-%pa] available\n",
+					&avail->start, &avail->end);
 			orig = *avail;
 		}
 	}

Re: [PATCH v2 4/4] x86/PCI: Fix log message typo

[Bjorn Helgaas]

On Fri, Dec 09, 2022 at 08:43:06PM +0200, Andy Shevchenko wrote:
> On Thu, Dec 08, 2022 at 01:03:41PM -0600, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > Add missing word in the log message:
> > 
> >   - ... so future kernels can this automatically
> >   + ... so future kernels can do this automatically
> 
> ...
> 
> >  	printk(KERN_INFO "PCI: %s E820 reservations for host bridge windows\n",
> >  	       pci_use_e820 ? "Using" : "Ignoring");
> >  	if (pci_probe & (PCI_NO_E820 | PCI_USE_E820))
> > -		printk(KERN_INFO "PCI: Please notify linux-pci@vger.kernel.org so future kernels can this automatically\n");
> > +		printk(KERN_INFO "PCI: Please notify linux-pci@vger.kernel.org so future kernels can do this automatically\n");
> 
> Wondering if we can change printk(KERN_LVL) to pr_lvl() in this file.

Sure!  How about this?


commit 7058cdb558d5 ("x86/PCI: Use pr_info() when possible")
Author: Bjorn Helgaas <bhelgaas@google.com>
Date:   Fri Dec 9 14:41:27 2022 -0600

    x86/PCI: Use pr_info() when possible
    
    Use pr_info() and similar when possible.  No functional change intended.
    
    Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
index 83dfea9e9894..ea2eb2ec90e2 100644
--- a/arch/x86/pci/acpi.c
+++ b/arch/x86/pci/acpi.c
@@ -1,4 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
+
+#define pr_fmt(fmt) "PCI: " fmt
+
 #include <linux/pci.h>
 #include <linux/acpi.h>
 #include <linux/init.h>
@@ -37,15 +40,15 @@ static int __init set_nouse_crs(const struct dmi_system_id *id)
 
 static int __init set_ignore_seg(const struct dmi_system_id *id)
 {
-	printk(KERN_INFO "PCI: %s detected: ignoring ACPI _SEG\n", id->ident);
+	pr_info("%s detected: ignoring ACPI _SEG\n", id->ident);
 	pci_ignore_seg = true;
 	return 0;
 }
 
 static int __init set_no_e820(const struct dmi_system_id *id)
 {
-	printk(KERN_INFO "PCI: %s detected: not clipping E820 regions from _CRS\n",
-	       id->ident);
+	pr_info("%s detected: not clipping E820 regions from _CRS\n",
+	        id->ident);
 	pci_use_e820 = false;
 	return 0;
 }
@@ -231,10 +234,9 @@ void __init pci_acpi_crs_quirks(void)
 	else if (pci_probe & PCI_USE__CRS)
 		pci_use_crs = true;
 
-	printk(KERN_INFO "PCI: %s host bridge windows from ACPI; "
-	       "if necessary, use \"pci=%s\" and report a bug\n",
-	       pci_use_crs ? "Using" : "Ignoring",
-	       pci_use_crs ? "nocrs" : "use_crs");
+	pr_info("%s host bridge windows from ACPI; if necessary, use \"pci=%s\" and report a bug\n",
+	        pci_use_crs ? "Using" : "Ignoring",
+	        pci_use_crs ? "nocrs" : "use_crs");
 
 	/* "pci=use_e820"/"pci=no_e820" on the kernel cmdline takes precedence */
 	if (pci_probe & PCI_NO_E820)
@@ -242,19 +244,17 @@ void __init pci_acpi_crs_quirks(void)
 	else if (pci_probe & PCI_USE_E820)
 		pci_use_e820 = true;
 
-	printk(KERN_INFO "PCI: %s E820 reservations for host bridge windows\n",
-	       pci_use_e820 ? "Using" : "Ignoring");
+	pr_info("%s E820 reservations for host bridge windows\n",
+	        pci_use_e820 ? "Using" : "Ignoring");
 	if (pci_probe & (PCI_NO_E820 | PCI_USE_E820))
-		printk(KERN_INFO "PCI: Please notify linux-pci@vger.kernel.org so future kernels can do this automatically\n");
+		pr_info("Please notify linux-pci@vger.kernel.org so future kernels can do this automatically\n");
 }
 
 #ifdef	CONFIG_PCI_MMCONFIG
 static int check_segment(u16 seg, struct device *dev, char *estr)
 {
 	if (seg) {
-		dev_err(dev,
-			"%s can't access PCI configuration "
-			"space under this host bridge.\n",
+		dev_err(dev, "%s can't access configuration space under this host bridge\n",
 			estr);
 		return -EIO;
 	}
@@ -264,9 +264,7 @@ static int check_segment(u16 seg, struct device *dev, char *estr)
 	 * just can't access extended configuration space of
 	 * devices under this host bridge.
 	 */
-	dev_warn(dev,
-		 "%s can't access extended PCI configuration "
-		 "space under this bridge.\n",
+	dev_warn(dev, "%s can't access extended configuration space under this bridge\n",
 		 estr);
 
 	return 0;
@@ -421,9 +419,8 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
 		root->segment = domain = 0;
 
 	if (domain && !pci_domains_supported) {
-		printk(KERN_WARNING "pci_bus %04x:%02x: "
-		       "ignored (multiple domains not supported)\n",
-		       domain, busnum);
+		pr_warn("pci_bus %04x:%02x: ignored (multiple domains not supported)\n",
+		        domain, busnum);
 		return NULL;
 	}
 
@@ -491,7 +488,7 @@ int __init pci_acpi_init(void)
 	if (acpi_noirq)
 		return -ENODEV;
 
-	printk(KERN_INFO "PCI: Using ACPI for IRQ routing\n");
+	pr_info("Using ACPI for IRQ routing\n");
 	acpi_irq_penalty_init();
 	pcibios_enable_irq = acpi_pci_irq_enable;
 	pcibios_disable_irq = acpi_pci_irq_disable;
@@ -503,7 +500,7 @@ int __init pci_acpi_init(void)
 		 * also do it here in case there are still broken drivers that
 		 * don't use pci_enable_device().
 		 */
-		printk(KERN_INFO "PCI: Routing PCI interrupts for all devices because \"pci=routeirq\" specified\n");
+		pr_info("Routing PCI interrupts for all devices because \"pci=routeirq\" specified\n");
 		for_each_pci_dev(dev)
 			acpi_pci_irq_enable(dev);
 	}

Re: [PATCH v2 3/4] x86/PCI: Tidy E820 removal messages

[Andy Shevchenko]

On Fri, Dec 09, 2022 at 02:34:28PM -0600, Bjorn Helgaas wrote:
> On Fri, Dec 09, 2022 at 08:42:06PM +0200, Andy Shevchenko wrote:
> > On Thu, Dec 08, 2022 at 01:03:40PM -0600, Bjorn Helgaas wrote:

...

> > > +			if (avail->end > avail->start)
> > > +				pr_info("resource: remaining [mem %#010llx-%#010llx] available\n",
> > > +					(unsigned long long) avail->start,
> > > +					(unsigned long long) avail->end);
> > 
> > Is there any point why we do not use %pa for resource_size_t parameters?
> 
> Only my ignorance :)  Thanks for pointing that out; I changed it to
> this and added a comment about why:

> +			pr_info("resource: avoiding allocation from e820 entry [mem %#010Lx-%#010Lx]\n",
> +				e820_start, e820_end);
> +			if (avail->end > avail->start)
> +				/*
> +				 * Use %pa instead of %pR because "avail"
> +				 * is typically IORESOURCE_UNSET, so %pR
> +				 * shows the size instead of addresses.
> +				 */
> +				pr_info("resource: remaining [mem %pa-%pa] available\n",
> +					&avail->start, &avail->end);

LGTM, thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/4] x86/PCI: Fix log message typo

[Andy Shevchenko]

On Fri, Dec 09, 2022 at 02:51:31PM -0600, Bjorn Helgaas wrote:
> On Fri, Dec 09, 2022 at 08:43:06PM +0200, Andy Shevchenko wrote:
> > On Thu, Dec 08, 2022 at 01:03:41PM -0600, Bjorn Helgaas wrote:

...

> > Wondering if we can change printk(KERN_LVL) to pr_lvl() in this file.
> 
> Sure!  How about this?

LGTM, you can add
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
to it if you wish.

> commit 7058cdb558d5 ("x86/PCI: Use pr_info() when possible")
> Author: Bjorn Helgaas <bhelgaas@google.com>
> Date:   Fri Dec 9 14:41:27 2022 -0600
> 
>     x86/PCI: Use pr_info() when possible
>     
>     Use pr_info() and similar when possible.  No functional change intended.
>     
>     Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>

I prefer @linux.intel.com.

>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> index 83dfea9e9894..ea2eb2ec90e2 100644
> --- a/arch/x86/pci/acpi.c
> +++ b/arch/x86/pci/acpi.c
> @@ -1,4 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
> +
> +#define pr_fmt(fmt) "PCI: " fmt
> +
>  #include <linux/pci.h>
>  #include <linux/acpi.h>
>  #include <linux/init.h>
> @@ -37,15 +40,15 @@ static int __init set_nouse_crs(const struct dmi_system_id *id)
>  
>  static int __init set_ignore_seg(const struct dmi_system_id *id)
>  {
> -	printk(KERN_INFO "PCI: %s detected: ignoring ACPI _SEG\n", id->ident);
> +	pr_info("%s detected: ignoring ACPI _SEG\n", id->ident);
>  	pci_ignore_seg = true;
>  	return 0;
>  }
>  
>  static int __init set_no_e820(const struct dmi_system_id *id)
>  {
> -	printk(KERN_INFO "PCI: %s detected: not clipping E820 regions from _CRS\n",
> -	       id->ident);
> +	pr_info("%s detected: not clipping E820 regions from _CRS\n",
> +	        id->ident);
>  	pci_use_e820 = false;
>  	return 0;
>  }
> @@ -231,10 +234,9 @@ void __init pci_acpi_crs_quirks(void)
>  	else if (pci_probe & PCI_USE__CRS)
>  		pci_use_crs = true;
>  
> -	printk(KERN_INFO "PCI: %s host bridge windows from ACPI; "
> -	       "if necessary, use \"pci=%s\" and report a bug\n",
> -	       pci_use_crs ? "Using" : "Ignoring",
> -	       pci_use_crs ? "nocrs" : "use_crs");
> +	pr_info("%s host bridge windows from ACPI; if necessary, use \"pci=%s\" and report a bug\n",
> +	        pci_use_crs ? "Using" : "Ignoring",
> +	        pci_use_crs ? "nocrs" : "use_crs");
>  
>  	/* "pci=use_e820"/"pci=no_e820" on the kernel cmdline takes precedence */
>  	if (pci_probe & PCI_NO_E820)
> @@ -242,19 +244,17 @@ void __init pci_acpi_crs_quirks(void)
>  	else if (pci_probe & PCI_USE_E820)
>  		pci_use_e820 = true;
>  
> -	printk(KERN_INFO "PCI: %s E820 reservations for host bridge windows\n",
> -	       pci_use_e820 ? "Using" : "Ignoring");
> +	pr_info("%s E820 reservations for host bridge windows\n",
> +	        pci_use_e820 ? "Using" : "Ignoring");
>  	if (pci_probe & (PCI_NO_E820 | PCI_USE_E820))
> -		printk(KERN_INFO "PCI: Please notify linux-pci@vger.kernel.org so future kernels can do this automatically\n");
> +		pr_info("Please notify linux-pci@vger.kernel.org so future kernels can do this automatically\n");
>  }
>  
>  #ifdef	CONFIG_PCI_MMCONFIG
>  static int check_segment(u16 seg, struct device *dev, char *estr)
>  {
>  	if (seg) {
> -		dev_err(dev,
> -			"%s can't access PCI configuration "
> -			"space under this host bridge.\n",
> +		dev_err(dev, "%s can't access configuration space under this host bridge\n",
>  			estr);
>  		return -EIO;
>  	}
> @@ -264,9 +264,7 @@ static int check_segment(u16 seg, struct device *dev, char *estr)
>  	 * just can't access extended configuration space of
>  	 * devices under this host bridge.
>  	 */
> -	dev_warn(dev,
> -		 "%s can't access extended PCI configuration "
> -		 "space under this bridge.\n",
> +	dev_warn(dev, "%s can't access extended configuration space under this bridge\n",
>  		 estr);
>  
>  	return 0;
> @@ -421,9 +419,8 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
>  		root->segment = domain = 0;
>  
>  	if (domain && !pci_domains_supported) {
> -		printk(KERN_WARNING "pci_bus %04x:%02x: "
> -		       "ignored (multiple domains not supported)\n",
> -		       domain, busnum);
> +		pr_warn("pci_bus %04x:%02x: ignored (multiple domains not supported)\n",
> +		        domain, busnum);
>  		return NULL;
>  	}
>  
> @@ -491,7 +488,7 @@ int __init pci_acpi_init(void)
>  	if (acpi_noirq)
>  		return -ENODEV;
>  
> -	printk(KERN_INFO "PCI: Using ACPI for IRQ routing\n");
> +	pr_info("Using ACPI for IRQ routing\n");
>  	acpi_irq_penalty_init();
>  	pcibios_enable_irq = acpi_pci_irq_enable;
>  	pcibios_disable_irq = acpi_pci_irq_disable;
> @@ -503,7 +500,7 @@ int __init pci_acpi_init(void)
>  		 * also do it here in case there are still broken drivers that
>  		 * don't use pci_enable_device().
>  		 */
> -		printk(KERN_INFO "PCI: Routing PCI interrupts for all devices because \"pci=routeirq\" specified\n");
> +		pr_info("Routing PCI interrupts for all devices because \"pci=routeirq\" specified\n");
>  		for_each_pci_dev(dev)
>  			acpi_pci_irq_enable(dev);
>  	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/4] x86/PCI: Fix log message typo

[Bjorn Helgaas]

On Fri, Dec 09, 2022 at 11:35:57PM +0200, Andy Shevchenko wrote:
> On Fri, Dec 09, 2022 at 02:51:31PM -0600, Bjorn Helgaas wrote:
> > On Fri, Dec 09, 2022 at 08:43:06PM +0200, Andy Shevchenko wrote:
> > > On Thu, Dec 08, 2022 at 01:03:41PM -0600, Bjorn Helgaas wrote:
> 
> ...
> 
> > > Wondering if we can change printk(KERN_LVL) to pr_lvl() in this file.
> > 
> > Sure!  How about this?
> 
> LGTM, you can add
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> to it if you wish.

Done, thanks!

> >     Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> 
> I prefer @linux.intel.com.

Oops, sorry, I should have known that.  I had copied that from
the From: line of your email
(https://lore.kernel.org/r/Y5OBupWBghHfvG/h@smile.fi.intel.com)

Bjorn

Re: [PATCH v2 4/4] x86/PCI: Fix log message typo

[Andy Shevchenko]

On Fri, Dec 09, 2022 at 03:52:11PM -0600, Bjorn Helgaas wrote:
> On Fri, Dec 09, 2022 at 11:35:57PM +0200, Andy Shevchenko wrote:
> > On Fri, Dec 09, 2022 at 02:51:31PM -0600, Bjorn Helgaas wrote:
> > > On Fri, Dec 09, 2022 at 08:43:06PM +0200, Andy Shevchenko wrote:

...

> > >     Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> > 
> > I prefer @linux.intel.com.
> 
> Oops, sorry, I should have known that.  I had copied that from
> the From: line of your email
> (https://lore.kernel.org/r/Y5OBupWBghHfvG/h@smile.fi.intel.com)

I understand. It's not your fault.

-- 
With Best Regards,
Andy Shevchenko

Re:[PATCH v2 1/4] efi/x86: Remove EfiMemoryMappedIO from E820 map

[Baowen Zheng]

Hello Bjorn Helgaas:

When we tested our NIC, we found we will fail to access pcie extent configuration space.
Actually we can only access the 256 Bytes of the pcie configuration space.
You can make check with command of "lspci -s XXXX -vv"
It seems relates to this change, could you please help to make a check?


On Thu, 8 Dec 2022 13:03:38 Bjorn Helgaas wrote:
>Firmware can use EfiMemoryMappedIO to request that MMIO regions be mapped
>by the OS so they can be accessed by EFI runtime services, but should have
>no other significance to the OS (UEFI r2.10, sec 7.2).  However, most
>bootloaders and EFI stubs convert EfiMemoryMappedIO regions to
>E820_TYPE_RESERVED entries, which prevent Linux from allocating space from
>them (see remove_e820_regions()).
>
>Some platforms use EfiMemoryMappedIO entries for PCI MMCONFIG space and PCI
>host bridge windows, which means Linux can't allocate BAR space for
>hot-added devices.
>
>Remove large EfiMemoryMappedIO regions from the E820 map to avoid this
>problem.
>

...

>
>Link: https://bugzilla.kernel.org/show_bug.cgi?id=216565     Clevo NL4XLU
>Link: https://bugzilla.kernel.org/show_bug.cgi?id=206459#c78 Clevo X170KM-G
>Link: https://bugzilla.redhat.com/show_bug.cgi?id=1868899    Ideapad Slim 3
>Link: https://bugzilla.redhat.com/show_bug.cgi?id=2029207    X1 Carbon
>Reported-by: Florent DELAHAYE <kernelorg@undead.fr>
>Tested-by: Konrad J Hambrick <kjhambrick@gmail.com>
>Tested-by: Matt Hansen <2lprbe78@duck.com>
>Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>Cc: Hans de Goede <hdegoede@redhat.com>
>---
> arch/x86/platform/efi/efi.c | 46 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 46 insertions(+)
>
>diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
>index ebc98a68c400..dee1852e95cd 100644
>--- a/arch/x86/platform/efi/efi.c
>+++ b/arch/x86/platform/efi/efi.c
>@@ -303,6 +303,50 @@ static void __init efi_clean_memmap(void)
> 	}
> }
> 
>+/*
>+ * Firmware can use EfiMemoryMappedIO to request that MMIO regions be
>+ * mapped by the OS so they can be accessed by EFI runtime services, but
>+ * should have no other significance to the OS (UEFI r2.10, sec 7.2).
>+ * However, most bootloaders and EFI stubs convert EfiMemoryMappedIO
>+ * regions to E820_TYPE_RESERVED entries, which prevent Linux from
>+ * allocating space from them (see remove_e820_regions()).
>+ *
>+ * Some platforms use EfiMemoryMappedIO entries for PCI MMCONFIG space and
>+ * PCI host bridge windows, which means Linux can't allocate BAR space for
>+ * hot-added devices.
>+ *
>+ * Remove large EfiMemoryMappedIO regions from the E820 map to avoid this
>+ * problem.
>+ *
>+ * Retain small EfiMemoryMappedIO regions because on some platforms, these
>+ * describe non-window space that's included in host bridge _CRS.  If we
>+ * assign that space to PCI devices, they don't work.
>+ */
>+static void __init efi_remove_e820_mmio(void)
>+{
>+	efi_memory_desc_t *md;
>+	u64 size, start, end;
>+	int i = 0;
>+
>+	for_each_efi_memory_desc(md) {
>+		if (md->type == EFI_MEMORY_MAPPED_IO) {
>+			size = md->num_pages << EFI_PAGE_SHIFT;
>+			if (size >= 256*1024) {
>+				start = md->phys_addr;
>+				end = start + size - 1;
>+				pr_info("Remove mem%02u: MMIO range=[0x%08llx-0x%08llx] (%lluMB) from e820 map\n",
>+					i, start, end, size >> 20);
>+				e820__range_remove(start, size,
>+						   E820_TYPE_RESERVED, 1);
>+			} else {
>+				pr_info("Not removing mem%02u: MMIO range=[0x%08llx-0x%08llx] (%lluKB) from e820 map\n",
>+					i, start, end, size >> 10);
>+			}
>+		}
>+		i++;
>+	}
>+}
>+
> void __init efi_print_memmap(void)
> {
> 	efi_memory_desc_t *md;
>@@ -474,6 +518,8 @@ void __init efi_init(void)
> 	set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
> 	efi_clean_memmap();
> 
>+	efi_remove_e820_mmio();
>+
> 	if (efi_enabled(EFI_DBG))
> 		efi_print_memmap();
> }

Re: [PATCH v2 1/4] efi/x86: Remove EfiMemoryMappedIO from E820 map

[Bjorn Helgaas]

On Fri, Jan 13, 2023 at 06:46:03PM +0800, Baowen Zheng wrote:
> Hello Bjorn Helgaas:
> 
> When we tested our NIC, we found we will fail to access pcie extent configuration space.
> Actually we can only access the 256 Bytes of the pcie configuration space.
> You can make check with command of "lspci -s XXXX -vv"
> It seems relates to this change, could you please help to make a check?

Can you test these patches?

  https://lore.kernel.org/linux-pci/20230110180243.1590045-1-helgaas@kernel.org/

Let me know if they don't fix the problem.

Bjorn