[PATCH v3 0/8] PCI: Resource placement algorithm fixes

[PATCH v3 0/8] PCI: Resource placement algorithm fixes

[Ilpo Järvinen]

Hi,

This series generalizes resource placement algorithm. The commit
9036bd0efcb6 ("PCI: Align head space better") special cased one
composite resource remainder case to fix a regression (in a long chain
of regression fixes). Unfortunately, it introduced another regression
(or a few, to be more accurate).

Instead of building more special tricks, generalize the PCI resource
placement algorithm so it returns placements that gives better chances
for the greedy assignment algorithm to find suitable space for
subsequent assignments.

In short words, the new approach tries to find a placement for the
current resource that blocks as little of the free space range as
possible, considering both alignment and continuous free span of the
remaining free space.

In addition, the series corrects composite resource sizing to account
for gaps that have to be added due to alignment constaints and
remainder space not fully connecting (filling space all the way to
the bridge window align).

More detailed explanations in the patches themselves.

Unfortunately, this also causes yet another regression due to shuffling
resources around, even if the resulting arrangement seems just as valid
as any other. The workaround quirk is in the last patch.

I find it likely that after this series the resources are packed tight
enough to solve also
  https://bugzilla.kernel.org/show_bug.cgi?id=220016
from more than a year back but that remains to be seen (if we can still
get the reporter to test it).


I've another series coming up to improve pci=resource_alignment locking
as I realized the current approach allows parameter change to race with
the fitting and assignment algorithm.


v3:
- Add roundup/down_pow_of_two64() (new patch) to avoid problems if
  resource_size_t is 64-bit and unsigned long is 32-bit + use __ffs64()
  (multiple potential issues mentioned by sashiko)
- Do not print res->parent in parisc debug code as the resource is never
  in the resource tree (found by sashiko).
- Do not override user-alignment with the remainder moves (sashiko)
- Consider optional sizes properly when sizing bridge window gaps (sashiko)
- Move genoa quirk under arch/x86/
- Rename quirk's static resource variable

v2:
- Add patch to cleanup parisc debug print (do dyndbg conversion)
- Add patch to prevent remainder tricks when alignment override is used
  (found by sashiko)
- Do not use PCIBIOS_MIN_IO/MEM but 1U as min align. The former seem
  address space lower bounds even if parisc arch code misleadingly
  uses them for ALIGN() call.
- Fix res->end + 1 overflow case (found by sashiko)
- Tweak debug print formatting
- Reworked the iomem black hole quirk (now based on the bridge)


Ilpo Järvinen (8):
  resource: Mark free space assigned
  PCI/parisc: Clean up resource debug print & use dynamic debug
  PCI: Honor alignment overrides
  PCI: Fix nesting windows with remainder at the left edge
  linux/log2.h: Add 64-bit roundup/down_pow_of_two64() variants
  PCI: Place resources to either edge of the window
  PCI: Fix composite resource sizing
  PCI/quirks: Avoid certain address on Genoa systems

 arch/alpha/kernel/pci.c          |   3 +-
 arch/arm/kernel/bios32.c         |   5 +-
 arch/m68k/kernel/pcibios.c       |   4 +-
 arch/mips/pci/pci-generic.c      |   5 +-
 arch/mips/pci/pci-legacy.c       |   5 +-
 arch/parisc/kernel/pci.c         |  20 +--
 arch/powerpc/kernel/pci-common.c |   5 +-
 arch/sh/drivers/pci/pci.c        |   5 +-
 arch/x86/pci/fixup.c             |  36 +++++
 arch/x86/pci/i386.c              |   5 +-
 arch/xtensa/kernel/pci.c         |   5 +-
 drivers/char/agp/intel-gtt.c     |   5 +-
 drivers/gpu/drm/i915/i915_gmch.c |   5 +-
 drivers/pci/pci.c                |   5 +-
 drivers/pci/pci.h                |   4 +
 drivers/pci/setup-bus.c          |  88 +++++++++++-
 drivers/pci/setup-res.c          | 223 +++++++++++++++++++++++++++----
 include/linux/log2.h             |  52 +++++++
 include/linux/pci.h              |  33 ++++-
 kernel/resource.c                |   1 +
 20 files changed, 445 insertions(+), 69 deletions(-)


base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
-- 
2.47.3

[PATCH v3 1/8] resource: Mark free space assigned

[Ilpo Järvinen]

Free space ranges are (part of) assigned ranges. Since new resources
have IORESOURCE_UNSET, the flag gets copied also to full_avail.flags,
which hampers printing them with %pR.

Clear IORESOURCE_UNSET for the free space indicator resource.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Reviewed-by: Bradley Morgan <brads@mainlining.org>
---
 kernel/resource.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/resource.c b/kernel/resource.c
index e60539a55541..17e7fccd859c 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -731,6 +731,7 @@ static int __find_resource_space(struct resource *root, struct resource *old,
 	resource_alignf alignf = constraint->alignf;
 
 	full_avail.start = root->start;
+	full_avail.flags &= ~IORESOURCE_UNSET;
 	/*
 	 * Skip past an allocated resource that starts at 0, since the assignment
 	 * of this->start - 1 to full_avail->end below would cause an underflow.
-- 
2.47.3

Re: [PATCH v3 1/8] resource: Mark free space assigned

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] resource: incomplete fix for %pR format on candidate resource
--

commit 094e97a90a5417acb103a184b0b1c36691f9e6c3
Author: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

resource: Mark free space assigned

This commit clears the IORESOURCE_UNSET flag from the free space indicator
resource (full_avail). This prevents the flag from hampering the printing
of the resource with the %pR format specifier.

> diff --git a/kernel/resource.c b/kernel/resource.c
> index e60539a55541d..17e7fccd859cb 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -731,6 +731,7 @@ static int __find_resource_space(struct resource *root, struct resource *old,
>  	resource_alignf alignf = constraint->alignf;
>  
>  	full_avail.start = root->start;
> +	full_avail.flags &= ~IORESOURCE_UNSET;

[Severity: Low]
Is this fix incomplete?

Later in __find_resource_space(), the candidate resource 'avail' inherits
flags from the unassigned resource 'new':

kernel/resource.c:__find_resource_space() {
    ...
		/* Check for overflow after ALIGN() */
		avail.start = ALIGN(full_avail.start, constraint->align);
		avail.end = full_avail.end;
		avail.flags = new->flags;
    ...
}

Because 'new->flags' still has IORESOURCE_UNSET set, 'avail' inherits it.
When 'avail' is passed to alignf callbacks (such as pcibios_align_resource
in arch/parisc/kernel/pci.c), any %pR debug prints of this candidate
resource will incorrectly format it as a size rather than revealing the
candidate address range.

Does IORESOURCE_UNSET need to be cleared for 'avail.flags' as well to
fully resolve this %pR formatting issue?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013130948.6231-1-ilpo.jarvinen@linux.intel.com?part=1

[PATCH v3 2/8] PCI/parisc: Clean up resource debug print & use dynamic debug

[Ilpo Järvinen]

Remove adhoc RES_DBG() and use pci_dbg() instead.

Remove printing parent resource as the passed is a temporary resource
not within the resource tree so accessing ->parent does not make sense.

Reformat the message to the typical format used by PCI core. Print the
resource using %pR.

Correct format specifiers and remove non-sensical res->flags int cast.

Dynamic debug prints can be enabled by providing dyndbg command line
parameter or through /sys/kernel/debug/dynamic_debug/control so this
is more versatile way to debug things than the previous compile time
decision which could only be enable by changing the source code.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 arch/parisc/kernel/pci.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/arch/parisc/kernel/pci.c b/arch/parisc/kernel/pci.c
index b8007c7400d4..96950fe9c2b7 100644
--- a/arch/parisc/kernel/pci.c
+++ b/arch/parisc/kernel/pci.c
@@ -19,7 +19,6 @@
 #include <asm/io.h>
 #include <asm/superio.h>
 
-#define DEBUG_RESOURCES 0
 #define DEBUG_CONFIG 0
 
 #if DEBUG_CONFIG
@@ -28,13 +27,6 @@
 # define DBGC(x...)
 #endif
 
-
-#if DEBUG_RESOURCES
-#define DBG_RES(x...)	printk(KERN_DEBUG x)
-#else
-#define DBG_RES(x...)
-#endif
-
 struct pci_port_ops *pci_port __ro_after_init;
 struct pci_bios_ops *pci_bios __ro_after_init;
 
@@ -204,10 +196,9 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
 	struct pci_dev *dev = data;
 	resource_size_t align, start = res->start;
 
-	DBG_RES("pcibios_align_resource(%s, (%p) [%lx,%lx]/%x, 0x%lx, 0x%lx)\n",
-		pci_name(((struct pci_dev *) data)),
-		res->parent, res->start, res->end,
-		(int) res->flags, size, alignment);
+	pci_dbg(dev, "%pR: pcibios_align_resource(%lx, 0x%llx, 0x%llx)\n",
+		res, res->flags, (unsigned long long)size,
+		(unsigned long long)alignment);
 
 	/* If it's not IO, then it's gotta be MEM */
 	align = (res->flags & IORESOURCE_IO) ? PCIBIOS_MIN_IO : PCIBIOS_MIN_MEM;
-- 
2.47.3

Re: [PATCH v3 2/8] PCI/parisc: Clean up resource debug print & use dynamic debug

[sashiko-bot]

> Remove adhoc RES_DBG() and use pci_dbg() instead.
> 
> Remove printing parent resource as the passed is a temporary resource
> not within the resource tree so accessing ->parent does not make sense.
> 
> Reformat the message to the typical format used by PCI core. Print the
> resource using %pR.
> 
> Correct format specifiers and remove non-sensical res->flags int cast.
> 
> Dynamic debug prints can be enabled by providing dyndbg command line
> parameter or through /sys/kernel/debug/dynamic_debug/control so this
> is more versatile way to debug things than the previous compile time
> decision which could only be enable by changing the source code.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013130948.6231-1-ilpo.jarvinen@linux.intel.com?part=2

[PATCH v3 3/8] PCI: Honor alignment overrides

[Ilpo Järvinen]

pci=resource_alignment argument can override the default alignment for
the resource. The remainder code introduced in the commit 9036bd0efcb6
("PCI: Align head space better") can move remainder space (non-aligning
part of the size) before the aligning left edge which results in
violating the requested alignment.

Introduce struct pci_resreq_data to hold device and user-given alignment
to be able to honor it in pci_align_resource(). When user-given alignment
is found, any remainder movement is skipped.

Fixes: 9036bd0efcb6 ("PCI: Align head space better")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/linux-pci/20260923133202.07DF61F000FF@smtp.kernel.org/
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---

IMO, PPC rigging the user alignment override to enforce arch specific
min alignment is mixing things wrongly. It would be more appropriate
to do arch alignment overrides by overriding pci_resource_alignment().

Jani Nikula <jani.nikula@intel.com> did ack this with a condition that
there is a proper review done by somebody else so I've not yet included
his tag.

---
 arch/alpha/kernel/pci.c          |  3 ++-
 arch/arm/kernel/bios32.c         |  5 +++--
 arch/m68k/kernel/pcibios.c       |  4 ++--
 arch/mips/pci/pci-generic.c      |  5 +++--
 arch/mips/pci/pci-legacy.c       |  5 +++--
 arch/parisc/kernel/pci.c         |  5 +++--
 arch/powerpc/kernel/pci-common.c |  5 +++--
 arch/sh/drivers/pci/pci.c        |  5 +++--
 arch/x86/pci/i386.c              |  5 +++--
 arch/xtensa/kernel/pci.c         |  5 +++--
 drivers/char/agp/intel-gtt.c     |  5 ++++-
 drivers/gpu/drm/i915/i915_gmch.c |  5 +++--
 drivers/pci/pci.c                |  5 +++--
 drivers/pci/setup-res.c          | 19 +++++++++++++------
 include/linux/pci.h              | 32 ++++++++++++++++++++++++++++++--
 15 files changed, 81 insertions(+), 32 deletions(-)

diff --git a/arch/alpha/kernel/pci.c b/arch/alpha/kernel/pci.c
index 11df411b1d18..c5d725a9781f 100644
--- a/arch/alpha/kernel/pci.c
+++ b/arch/alpha/kernel/pci.c
@@ -128,7 +128,8 @@ pcibios_align_resource(void *data, const struct resource *res,
 		       const struct resource *empty_res,
 		       resource_size_t size, resource_size_t align)
 {
-	struct pci_dev *dev = data;
+	struct pci_resreq_data *rr = data;
+	struct pci_dev *dev = rr->dev;
 	struct pci_controller *hose = dev->sysdata;
 	unsigned long alignto;
 	resource_size_t start = res->start;
diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index ac0e890510da..3e61826f07fb 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -564,7 +564,8 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
 				       resource_size_t size,
 				       resource_size_t align)
 {
-	struct pci_dev *dev = data;
+	struct pci_resreq_data *rr = data;
+	struct pci_dev *dev = rr->dev;
 	resource_size_t start = res->start;
 	struct pci_host_bridge *host_bridge;
 
@@ -578,7 +579,7 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
 				start, size, align);
 
 	if (res->flags & IORESOURCE_MEM)
-		return pci_align_resource(dev, res, empty_res, size, align);
+		return pci_align_resource(rr, res, empty_res, size, align);
 
 	return start;
 }
diff --git a/arch/m68k/kernel/pcibios.c b/arch/m68k/kernel/pcibios.c
index 7a9e60df79c5..3024408412bc 100644
--- a/arch/m68k/kernel/pcibios.c
+++ b/arch/m68k/kernel/pcibios.c
@@ -31,14 +31,14 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
 				       resource_size_t size,
 				       resource_size_t align)
 {
-	struct pci_dev *dev = data;
+	struct pci_resreq_data *rr = data;
 	resource_size_t start = res->start;
 
 	if ((res->flags & IORESOURCE_IO) && (start & 0x300))
 		start = (start + 0x3ff) & ~0x3ff;
 
 	if (res->flags & IORESOURCE_MEM)
-		return pci_align_resource(dev, res, empty_res, size, align);
+		return pci_align_resource(rr, res, empty_res, size, align);
 
 	return start;
 }
diff --git a/arch/mips/pci/pci-generic.c b/arch/mips/pci/pci-generic.c
index c2e23d0c1d77..7f3ecb91b85b 100644
--- a/arch/mips/pci/pci-generic.c
+++ b/arch/mips/pci/pci-generic.c
@@ -25,7 +25,8 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
 				       const struct resource *empty_res,
 				       resource_size_t size, resource_size_t align)
 {
-	struct pci_dev *dev = data;
+	struct pci_resreq_data *rr = data;
+	struct pci_dev *dev = rr->dev;
 	resource_size_t start = res->start;
 	struct pci_host_bridge *host_bridge;
 
@@ -39,7 +40,7 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
 				start, size, align);
 
 	if (res->flags & IORESOURCE_MEM)
-		return pci_align_resource(dev, res, empty_res, size, align);
+		return pci_align_resource(rr, res, empty_res, size, align);
 
 	return start;
 }
diff --git a/arch/mips/pci/pci-legacy.c b/arch/mips/pci/pci-legacy.c
index dae6dafdd6e0..82d4b01db64e 100644
--- a/arch/mips/pci/pci-legacy.c
+++ b/arch/mips/pci/pci-legacy.c
@@ -55,7 +55,8 @@ pcibios_align_resource(void *data, const struct resource *res,
 		       const struct resource *empty_res,
 		       resource_size_t size, resource_size_t align)
 {
-	struct pci_dev *dev = data;
+	struct pci_resreq_data *rr = data;
+	struct pci_dev *dev = rr->dev;
 	struct pci_controller *hose = dev->sysdata;
 	resource_size_t start = res->start;
 
@@ -70,7 +71,7 @@ pcibios_align_resource(void *data, const struct resource *res,
 		if (start & 0x300)
 			start = (start + 0x3ff) & ~0x3ff;
 	} else if (res->flags & IORESOURCE_MEM) {
-		start = pci_align_resource(dev, res, empty_res, size, align);
+		start = pci_align_resource(rr, res, empty_res, size, align);
 
 		/* Make sure we start at our min on all hoses */
 		if (start < PCIBIOS_MIN_MEM + hose->mem_resource->start)
diff --git a/arch/parisc/kernel/pci.c b/arch/parisc/kernel/pci.c
index 96950fe9c2b7..36a3a439f609 100644
--- a/arch/parisc/kernel/pci.c
+++ b/arch/parisc/kernel/pci.c
@@ -193,7 +193,8 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
 				       resource_size_t size,
 				       resource_size_t alignment)
 {
-	struct pci_dev *dev = data;
+	struct pci_resreq_data *rr = data;
+	struct pci_dev *dev = rr->dev;
 	resource_size_t align, start = res->start;
 
 	pci_dbg(dev, "%pR: pcibios_align_resource(%lx, 0x%llx, 0x%llx)\n",
@@ -205,7 +206,7 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
 	if (align > alignment)
 		start = ALIGN(start, align);
 	else
-		start = pci_align_resource(dev, res, empty_res, size, alignment);
+		start = pci_align_resource(rr, res, empty_res, size, alignment);
 
 	return start;
 }
diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index 4fc52c21fe5d..23594759cfe2 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -1131,7 +1131,8 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
 				       resource_size_t size,
 				       resource_size_t align)
 {
-	struct pci_dev *dev = data;
+	struct pci_resreq_data *rr = data;
+	struct pci_dev *dev = rr->dev;
 	resource_size_t start = res->start;
 
 	if (res->flags & IORESOURCE_IO) {
@@ -1140,7 +1141,7 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
 		if (start & 0x300)
 			start = (start + 0x3ff) & ~0x3ff;
 	} else if (res->flags & IORESOURCE_MEM) {
-		start = pci_align_resource(dev, res, empty_res, size, align);
+		start = pci_align_resource(rr, res, empty_res, size, align);
 	}
 
 	return start;
diff --git a/arch/sh/drivers/pci/pci.c b/arch/sh/drivers/pci/pci.c
index 878a27a1acfb..279f2770ff4b 100644
--- a/arch/sh/drivers/pci/pci.c
+++ b/arch/sh/drivers/pci/pci.c
@@ -172,7 +172,8 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
 				       resource_size_t size,
 				       resource_size_t align)
 {
-	struct pci_dev *dev = data;
+	struct pci_resreq_data *rr = data;
+	struct pci_dev *dev = rr->dev;
 	struct pci_channel *hose = dev->sysdata;
 	resource_size_t start = res->start;
 
@@ -186,7 +187,7 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
 		if (start & 0x300)
 			start = (start + 0x3ff) & ~0x3ff;
 	} else if (res->flags & IORESOURCE_MEM) {
-		start = pci_align_resource(dev, res, empty_res, size, align);
+		start = pci_align_resource(rr, res, empty_res, size, align);
 	}
 
 	return start;
diff --git a/arch/x86/pci/i386.c b/arch/x86/pci/i386.c
index e2de26b82940..4178ae4380c2 100644
--- a/arch/x86/pci/i386.c
+++ b/arch/x86/pci/i386.c
@@ -156,7 +156,8 @@ pcibios_align_resource(void *data, const struct resource *res,
 		       const struct resource *empty_res,
 		       resource_size_t size, resource_size_t align)
 {
-	struct pci_dev *dev = data;
+	struct pci_resreq_data *rr = data;
+	struct pci_dev *dev = rr->dev;
 	resource_size_t start = res->start;
 
 	if (res->flags & IORESOURCE_IO) {
@@ -165,7 +166,7 @@ pcibios_align_resource(void *data, const struct resource *res,
 		if (start & 0x300)
 			start = (start + 0x3ff) & ~0x3ff;
 	} else if (res->flags & IORESOURCE_MEM) {
-		start = pci_align_resource(dev, res, empty_res, size, align);
+		start = pci_align_resource(rr, res, empty_res, size, align);
 
 		/* The low 1MB range is reserved for ISA cards */
 		if (start < BIOS_END)
diff --git a/arch/xtensa/kernel/pci.c b/arch/xtensa/kernel/pci.c
index 305031551136..4e8ad4a3c9fe 100644
--- a/arch/xtensa/kernel/pci.c
+++ b/arch/xtensa/kernel/pci.c
@@ -42,7 +42,8 @@ pcibios_align_resource(void *data, const struct resource *res,
 		       const struct resource *empty_res,
 		       resource_size_t size, resource_size_t align)
 {
-	struct pci_dev *dev = data;
+	struct pci_resreq_data *rr = data;
+	struct pci_dev *dev = rr->dev;
 	resource_size_t start = res->start;
 
 	if (res->flags & IORESOURCE_IO) {
@@ -55,7 +56,7 @@ pcibios_align_resource(void *data, const struct resource *res,
 		if (start & 0x300)
 			start = (start + 0x3ff) & ~0x3ff;
 	} else if (res->flags & IORESOURCE_MEM) {
-		start = pci_align_resource(dev, res, empty_res, size, align);
+		start = pci_align_resource(rr, res, empty_res, size, align);
 	}
 
 	return start;
diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
index bcc26785175d..2214347906a6 100644
--- a/drivers/char/agp/intel-gtt.c
+++ b/drivers/char/agp/intel-gtt.c
@@ -1038,10 +1038,13 @@ static struct agp_memory *intel_fake_agp_alloc_by_type(size_t pg_count,
 
 static int intel_alloc_chipset_flush_resource(void)
 {
+	struct pci_resreq_data rr;
 	int ret;
+
+	pci_init_pci_resreq_data(&rr, intel_private.bridge_dev);
 	ret = pci_bus_alloc_resource(intel_private.bridge_dev->bus, &intel_private.ifp_resource, PAGE_SIZE,
 				     PAGE_SIZE, PCIBIOS_MIN_MEM, 0,
-				     pcibios_align_resource, intel_private.bridge_dev);
+				     pcibios_align_resource, &rr);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/i915/i915_gmch.c b/drivers/gpu/drm/i915/i915_gmch.c
index b0ef6ef577a3..d35097bebc0f 100644
--- a/drivers/gpu/drm/i915/i915_gmch.c
+++ b/drivers/gpu/drm/i915/i915_gmch.c
@@ -38,6 +38,7 @@ static int mchbar_reg(struct drm_i915_private *i915)
 static int
 intel_alloc_mchbar_resource(struct drm_i915_private *i915)
 {
+	struct pci_resreq_data rr;
 	u32 temp_lo, temp_hi = 0;
 	u64 mchbar_addr;
 	int ret;
@@ -55,12 +56,12 @@ intel_alloc_mchbar_resource(struct drm_i915_private *i915)
 	/* Get some space for it */
 	i915->gmch.mch_res.name = "i915 MCHBAR";
 	i915->gmch.mch_res.flags = IORESOURCE_MEM;
+	pci_init_pci_resreq_data(&rr, i915->gmch.pdev);
 	ret = pci_bus_alloc_resource(i915->gmch.pdev->bus,
 				     &i915->gmch.mch_res,
 				     MCHBAR_SIZE, MCHBAR_SIZE,
 				     PCIBIOS_MIN_MEM,
-				     0, pcibios_align_resource,
-				     i915->gmch.pdev);
+				     0, pcibios_align_resource, &rr);
 	if (ret) {
 		drm_dbg(&i915->drm, "failed bus alloc: %d\n", ret);
 		i915->gmch.mch_res.start = 0;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b2879a6be5f8..1b7a4469c6ca 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -6444,8 +6444,8 @@ static DEFINE_SPINLOCK(resource_alignment_lock);
  * RETURNS: Resource alignment if it is specified.
  *          Zero if it is not specified.
  */
-static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
-							bool *resize)
+resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
+						 bool *resize)
 {
 	int align_order, count;
 	resource_size_t align = pcibios_default_alignment();
@@ -6497,6 +6497,7 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
 	spin_unlock(&resource_alignment_lock);
 	return align;
 }
+EXPORT_SYMBOL_GPL(pci_specified_resource_alignment);
 
 static void pci_request_resource_alignment(struct pci_dev *dev, int bar,
 					   resource_size_t align, bool resize)
diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index 376f09630a4a..441a62807719 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -265,17 +265,21 @@ resource_size_t pci_resource_alignment(const struct pci_dev *dev,
  * before res->start if there's enough free space there. This enables
  * tighter packing for resources.
  */
-resource_size_t pci_align_resource(struct pci_dev *dev,
+resource_size_t pci_align_resource(struct pci_resreq_data *rr,
 				   const struct resource *res,
 				   const struct resource *empty_res,
 				   resource_size_t size,
 				   resource_size_t align)
 {
+	struct pci_dev *dev = rr->dev;
 	resource_size_t remainder, start_addr;
 
 	if (!(res->flags & IORESOURCE_MEM))
 		return res->start;
 
+	if (rr->user_align)
+		return res->start;
+
 	if (IS_ALIGNED(size, align))
 		return res->start;
 
@@ -306,18 +310,21 @@ resource_size_t __weak pcibios_align_resource(void *data,
 					      resource_size_t size,
 					      resource_size_t align)
 {
-	struct pci_dev *dev = data;
+	struct pci_resreq_data *rr = data;
 
-	return pci_align_resource(dev, res, empty_res, size, align);
+	return pci_align_resource(rr, res, empty_res, size, align);
 }
 
 static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
 		int resno, resource_size_t size, resource_size_t align)
 {
 	struct resource *res = pci_resource_n(dev, resno);
+	struct pci_resreq_data rr;
 	resource_size_t min;
 	int ret;
 
+	pci_init_pci_resreq_data(&rr, dev);
+
 	min = (res->flags & IORESOURCE_IO) ? PCIBIOS_MIN_IO : PCIBIOS_MIN_MEM;
 
 	/*
@@ -329,7 +336,7 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
 	 */
 	ret = pci_bus_alloc_resource(bus, res, size, align, min,
 				     IORESOURCE_PREFETCH | IORESOURCE_MEM_64,
-				     pcibios_align_resource, dev);
+				     pcibios_align_resource, &rr);
 	if (ret == 0)
 		return 0;
 
@@ -341,7 +348,7 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
 	     (IORESOURCE_PREFETCH | IORESOURCE_MEM_64)) {
 		ret = pci_bus_alloc_resource(bus, res, size, align, min,
 					     IORESOURCE_PREFETCH,
-					     pcibios_align_resource, dev);
+					     pcibios_align_resource, &rr);
 		if (ret == 0)
 			return 0;
 	}
@@ -354,7 +361,7 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
 	 */
 	if (res->flags & (IORESOURCE_PREFETCH | IORESOURCE_MEM_64))
 		ret = pci_bus_alloc_resource(bus, res, size, align, min, 0,
-					     pcibios_align_resource, dev);
+					     pcibios_align_resource, &rr);
 
 	return ret;
 }
diff --git a/include/linux/pci.h b/include/linux/pci.h
index d31a8d107b1e..ea7d75871269 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1217,12 +1217,40 @@ int __must_check pcibios_enable_device(struct pci_dev *, int mask);
 /* Architecture-specific versions may override this (weak) */
 char *pcibios_setup(char *str);
 
-/* Used only when drivers/pci/setup.c is used */
+/* Resource assignment request handling */
+/**
+ * struct pci_resreq_data - Data passed to pcibios_align_resource()
+ * @dev: PCI device
+ * @user_align: User provided alignment override (pci=resource_alignment param)
+ */
+struct pci_resreq_data {
+	struct pci_dev *dev;
+	resource_size_t user_align;
+};
+
+resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
+						 bool *resize);
+/*
+ * TODO: Improve PCI resource assignment interface so that callers outside PCI
+ * core do not need to know about this detail.
+ */
+static inline void pci_init_pci_resreq_data(struct pci_resreq_data *rr,
+					    struct pci_dev *dev)
+{
+	bool user_aligned;
+
+	memset(rr, 0, sizeof(*rr));
+	rr->dev = dev;
+	rr->user_align = pci_specified_resource_alignment(dev, &user_aligned);
+	if (!user_aligned)
+		rr->user_align = 0;
+}
+
 resource_size_t pcibios_align_resource(void *data, const struct resource *res,
 				       const struct resource *empty_res,
 				       resource_size_t size,
 				       resource_size_t align);
-resource_size_t pci_align_resource(struct pci_dev *dev,
+resource_size_t pci_align_resource(struct pci_resreq_data *rr,
 				   const struct resource *res,
 				   const struct resource *empty_res,
 				   resource_size_t size,
-- 
2.47.3

Re: [PATCH v3 3/8] PCI: Honor alignment overrides

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pci: uninitialized variable `user_aligned` causes random bypass of resource alignment
--

commit dc4a5f31af386d2a1de50debe4823977860a1cb9
Author: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Subject: PCI: Honor alignment overrides

This commit introduces struct pci_resreq_data to hold device and user-given
alignment. It allows pci_align_resource() to honor user-given alignment by
skipping remainder movement when a user alignment is provided.

Link: https://lore.kernel.org/linux-pci/20260923133202.07DF61F000FF@smtp.kernel.org/

[ ... ]
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index d31a8d107b1ef..ea7d758712691 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
[ ... ]
> @@ -1217,12 +1217,40 @@ int __must_check pcibios_enable_device(struct pci_dev *, int mask);
>  /* Architecture-specific versions may override this (weak) */
>  char *pcibios_setup(char *str);
>  
> -/* Used only when drivers/pci/setup.c is used */
> +/* Resource assignment request handling */
> +/**
> + * struct pci_resreq_data - Data passed to pcibios_align_resource()
> + * @dev: PCI device
> + * @user_align: User provided alignment override (pci=resource_alignment param)
> + */
> +struct pci_resreq_data {
> +	struct pci_dev *dev;
> +	resource_size_t user_align;
> +};
> +
> +resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
> +						 bool *resize);
> +/*
> + * TODO: Improve PCI resource assignment interface so that callers outside PCI
> + * core do not need to know about this detail.
> + */
> +static inline void pci_init_pci_resreq_data(struct pci_resreq_data *rr,
> +					    struct pci_dev *dev)
> +{
> +	bool user_aligned;
> +
> +	memset(rr, 0, sizeof(*rr));
> +	rr->dev = dev;
> +	rr->user_align = pci_specified_resource_alignment(dev, &user_aligned);

[Severity: High]
Does this leave user_aligned uninitialized on the default execution path?

When pci_init_pci_resreq_data() calls pci_specified_resource_alignment(),
the output parameter is not always written to. For instance, if there
are no boot parameters, pci_specified_resource_alignment() exits early:

drivers/pci/pci.c:pci_specified_resource_alignment() {
    ...
	p = resource_alignment_param;
	if (!p || !*p)
		goto out;
    ...
out:
	spin_unlock(&resource_alignment_lock);
	return align;
}

This leaves user_aligned with uninitialized stack memory. If this memory
happens to be non-zero, it can cause the resource alignment logic to be
randomly bypassed later in pci_align_resource(), leading to potential PCI
resource exhaustion or allocation failures on architectures with a non-zero
default alignment.

Could user_aligned be initialized to false before being passed?

> +	if (!user_aligned)
> +		rr->user_align = 0;
> +}
> +
>  resource_size_t pcibios_align_resource(void *data, const struct resource *res,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013130948.6231-1-ilpo.jarvinen@linux.intel.com?part=3

Re: [PATCH v3 3/8] PCI: Honor alignment overrides

[Rodrigo Vivi]

On Tue, Oct 13, 2026 at 04:09:37PM +0300, Ilpo Järvinen wrote:
> pci=resource_alignment argument can override the default alignment for
> the resource. The remainder code introduced in the commit 9036bd0efcb6
> ("PCI: Align head space better") can move remainder space (non-aligning
> part of the size) before the aligning left edge which results in
> violating the requested alignment.
> 
> Introduce struct pci_resreq_data to hold device and user-given alignment
> to be able to honor it in pci_align_resource(). When user-given alignment
> is found, any remainder movement is skipped.
> 
> Fixes: 9036bd0efcb6 ("PCI: Align head space better")
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Link: https://lore.kernel.org/linux-pci/20260923133202.07DF61F000FF@smtp.kernel.org/
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
> 
> IMO, PPC rigging the user alignment override to enforce arch specific
> min alignment is mixing things wrongly. It would be more appropriate
> to do arch alignment overrides by overriding pci_resource_alignment().
> 
> Jani Nikula <jani.nikula@intel.com> did ack this with a condition that
> there is a proper review done by somebody else so I've not yet included
> his tag.
> 
> ---
>  arch/alpha/kernel/pci.c          |  3 ++-
>  arch/arm/kernel/bios32.c         |  5 +++--
>  arch/m68k/kernel/pcibios.c       |  4 ++--
>  arch/mips/pci/pci-generic.c      |  5 +++--
>  arch/mips/pci/pci-legacy.c       |  5 +++--
>  arch/parisc/kernel/pci.c         |  5 +++--
>  arch/powerpc/kernel/pci-common.c |  5 +++--
>  arch/sh/drivers/pci/pci.c        |  5 +++--
>  arch/x86/pci/i386.c              |  5 +++--
>  arch/xtensa/kernel/pci.c         |  5 +++--
>  drivers/char/agp/intel-gtt.c     |  5 ++++-
>  drivers/gpu/drm/i915/i915_gmch.c |  5 +++--

Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

>  drivers/pci/pci.c                |  5 +++--
>  drivers/pci/setup-res.c          | 19 +++++++++++++------
>  include/linux/pci.h              | 32 ++++++++++++++++++++++++++++++--
>  15 files changed, 81 insertions(+), 32 deletions(-)
> 
> diff --git a/arch/alpha/kernel/pci.c b/arch/alpha/kernel/pci.c
> index 11df411b1d18..c5d725a9781f 100644
> --- a/arch/alpha/kernel/pci.c
> +++ b/arch/alpha/kernel/pci.c
> @@ -128,7 +128,8 @@ pcibios_align_resource(void *data, const struct resource *res,
>  		       const struct resource *empty_res,
>  		       resource_size_t size, resource_size_t align)
>  {
> -	struct pci_dev *dev = data;
> +	struct pci_resreq_data *rr = data;
> +	struct pci_dev *dev = rr->dev;
>  	struct pci_controller *hose = dev->sysdata;
>  	unsigned long alignto;
>  	resource_size_t start = res->start;
> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> index ac0e890510da..3e61826f07fb 100644
> --- a/arch/arm/kernel/bios32.c
> +++ b/arch/arm/kernel/bios32.c
> @@ -564,7 +564,8 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
>  				       resource_size_t size,
>  				       resource_size_t align)
>  {
> -	struct pci_dev *dev = data;
> +	struct pci_resreq_data *rr = data;
> +	struct pci_dev *dev = rr->dev;
>  	resource_size_t start = res->start;
>  	struct pci_host_bridge *host_bridge;
>  
> @@ -578,7 +579,7 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
>  				start, size, align);
>  
>  	if (res->flags & IORESOURCE_MEM)
> -		return pci_align_resource(dev, res, empty_res, size, align);
> +		return pci_align_resource(rr, res, empty_res, size, align);
>  
>  	return start;
>  }
> diff --git a/arch/m68k/kernel/pcibios.c b/arch/m68k/kernel/pcibios.c
> index 7a9e60df79c5..3024408412bc 100644
> --- a/arch/m68k/kernel/pcibios.c
> +++ b/arch/m68k/kernel/pcibios.c
> @@ -31,14 +31,14 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
>  				       resource_size_t size,
>  				       resource_size_t align)
>  {
> -	struct pci_dev *dev = data;
> +	struct pci_resreq_data *rr = data;
>  	resource_size_t start = res->start;
>  
>  	if ((res->flags & IORESOURCE_IO) && (start & 0x300))
>  		start = (start + 0x3ff) & ~0x3ff;
>  
>  	if (res->flags & IORESOURCE_MEM)
> -		return pci_align_resource(dev, res, empty_res, size, align);
> +		return pci_align_resource(rr, res, empty_res, size, align);
>  
>  	return start;
>  }
> diff --git a/arch/mips/pci/pci-generic.c b/arch/mips/pci/pci-generic.c
> index c2e23d0c1d77..7f3ecb91b85b 100644
> --- a/arch/mips/pci/pci-generic.c
> +++ b/arch/mips/pci/pci-generic.c
> @@ -25,7 +25,8 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
>  				       const struct resource *empty_res,
>  				       resource_size_t size, resource_size_t align)
>  {
> -	struct pci_dev *dev = data;
> +	struct pci_resreq_data *rr = data;
> +	struct pci_dev *dev = rr->dev;
>  	resource_size_t start = res->start;
>  	struct pci_host_bridge *host_bridge;
>  
> @@ -39,7 +40,7 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
>  				start, size, align);
>  
>  	if (res->flags & IORESOURCE_MEM)
> -		return pci_align_resource(dev, res, empty_res, size, align);
> +		return pci_align_resource(rr, res, empty_res, size, align);
>  
>  	return start;
>  }
> diff --git a/arch/mips/pci/pci-legacy.c b/arch/mips/pci/pci-legacy.c
> index dae6dafdd6e0..82d4b01db64e 100644
> --- a/arch/mips/pci/pci-legacy.c
> +++ b/arch/mips/pci/pci-legacy.c
> @@ -55,7 +55,8 @@ pcibios_align_resource(void *data, const struct resource *res,
>  		       const struct resource *empty_res,
>  		       resource_size_t size, resource_size_t align)
>  {
> -	struct pci_dev *dev = data;
> +	struct pci_resreq_data *rr = data;
> +	struct pci_dev *dev = rr->dev;
>  	struct pci_controller *hose = dev->sysdata;
>  	resource_size_t start = res->start;
>  
> @@ -70,7 +71,7 @@ pcibios_align_resource(void *data, const struct resource *res,
>  		if (start & 0x300)
>  			start = (start + 0x3ff) & ~0x3ff;
>  	} else if (res->flags & IORESOURCE_MEM) {
> -		start = pci_align_resource(dev, res, empty_res, size, align);
> +		start = pci_align_resource(rr, res, empty_res, size, align);
>  
>  		/* Make sure we start at our min on all hoses */
>  		if (start < PCIBIOS_MIN_MEM + hose->mem_resource->start)
> diff --git a/arch/parisc/kernel/pci.c b/arch/parisc/kernel/pci.c
> index 96950fe9c2b7..36a3a439f609 100644
> --- a/arch/parisc/kernel/pci.c
> +++ b/arch/parisc/kernel/pci.c
> @@ -193,7 +193,8 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
>  				       resource_size_t size,
>  				       resource_size_t alignment)
>  {
> -	struct pci_dev *dev = data;
> +	struct pci_resreq_data *rr = data;
> +	struct pci_dev *dev = rr->dev;
>  	resource_size_t align, start = res->start;
>  
>  	pci_dbg(dev, "%pR: pcibios_align_resource(%lx, 0x%llx, 0x%llx)\n",
> @@ -205,7 +206,7 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
>  	if (align > alignment)
>  		start = ALIGN(start, align);
>  	else
> -		start = pci_align_resource(dev, res, empty_res, size, alignment);
> +		start = pci_align_resource(rr, res, empty_res, size, alignment);
>  
>  	return start;
>  }
> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
> index 4fc52c21fe5d..23594759cfe2 100644
> --- a/arch/powerpc/kernel/pci-common.c
> +++ b/arch/powerpc/kernel/pci-common.c
> @@ -1131,7 +1131,8 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
>  				       resource_size_t size,
>  				       resource_size_t align)
>  {
> -	struct pci_dev *dev = data;
> +	struct pci_resreq_data *rr = data;
> +	struct pci_dev *dev = rr->dev;
>  	resource_size_t start = res->start;
>  
>  	if (res->flags & IORESOURCE_IO) {
> @@ -1140,7 +1141,7 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
>  		if (start & 0x300)
>  			start = (start + 0x3ff) & ~0x3ff;
>  	} else if (res->flags & IORESOURCE_MEM) {
> -		start = pci_align_resource(dev, res, empty_res, size, align);
> +		start = pci_align_resource(rr, res, empty_res, size, align);
>  	}
>  
>  	return start;
> diff --git a/arch/sh/drivers/pci/pci.c b/arch/sh/drivers/pci/pci.c
> index 878a27a1acfb..279f2770ff4b 100644
> --- a/arch/sh/drivers/pci/pci.c
> +++ b/arch/sh/drivers/pci/pci.c
> @@ -172,7 +172,8 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
>  				       resource_size_t size,
>  				       resource_size_t align)
>  {
> -	struct pci_dev *dev = data;
> +	struct pci_resreq_data *rr = data;
> +	struct pci_dev *dev = rr->dev;
>  	struct pci_channel *hose = dev->sysdata;
>  	resource_size_t start = res->start;
>  
> @@ -186,7 +187,7 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
>  		if (start & 0x300)
>  			start = (start + 0x3ff) & ~0x3ff;
>  	} else if (res->flags & IORESOURCE_MEM) {
> -		start = pci_align_resource(dev, res, empty_res, size, align);
> +		start = pci_align_resource(rr, res, empty_res, size, align);
>  	}
>  
>  	return start;
> diff --git a/arch/x86/pci/i386.c b/arch/x86/pci/i386.c
> index e2de26b82940..4178ae4380c2 100644
> --- a/arch/x86/pci/i386.c
> +++ b/arch/x86/pci/i386.c
> @@ -156,7 +156,8 @@ pcibios_align_resource(void *data, const struct resource *res,
>  		       const struct resource *empty_res,
>  		       resource_size_t size, resource_size_t align)
>  {
> -	struct pci_dev *dev = data;
> +	struct pci_resreq_data *rr = data;
> +	struct pci_dev *dev = rr->dev;
>  	resource_size_t start = res->start;
>  
>  	if (res->flags & IORESOURCE_IO) {
> @@ -165,7 +166,7 @@ pcibios_align_resource(void *data, const struct resource *res,
>  		if (start & 0x300)
>  			start = (start + 0x3ff) & ~0x3ff;
>  	} else if (res->flags & IORESOURCE_MEM) {
> -		start = pci_align_resource(dev, res, empty_res, size, align);
> +		start = pci_align_resource(rr, res, empty_res, size, align);
>  
>  		/* The low 1MB range is reserved for ISA cards */
>  		if (start < BIOS_END)
> diff --git a/arch/xtensa/kernel/pci.c b/arch/xtensa/kernel/pci.c
> index 305031551136..4e8ad4a3c9fe 100644
> --- a/arch/xtensa/kernel/pci.c
> +++ b/arch/xtensa/kernel/pci.c
> @@ -42,7 +42,8 @@ pcibios_align_resource(void *data, const struct resource *res,
>  		       const struct resource *empty_res,
>  		       resource_size_t size, resource_size_t align)
>  {
> -	struct pci_dev *dev = data;
> +	struct pci_resreq_data *rr = data;
> +	struct pci_dev *dev = rr->dev;
>  	resource_size_t start = res->start;
>  
>  	if (res->flags & IORESOURCE_IO) {
> @@ -55,7 +56,7 @@ pcibios_align_resource(void *data, const struct resource *res,
>  		if (start & 0x300)
>  			start = (start + 0x3ff) & ~0x3ff;
>  	} else if (res->flags & IORESOURCE_MEM) {
> -		start = pci_align_resource(dev, res, empty_res, size, align);
> +		start = pci_align_resource(rr, res, empty_res, size, align);
>  	}
>  
>  	return start;
> diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
> index bcc26785175d..2214347906a6 100644
> --- a/drivers/char/agp/intel-gtt.c
> +++ b/drivers/char/agp/intel-gtt.c
> @@ -1038,10 +1038,13 @@ static struct agp_memory *intel_fake_agp_alloc_by_type(size_t pg_count,
>  
>  static int intel_alloc_chipset_flush_resource(void)
>  {
> +	struct pci_resreq_data rr;
>  	int ret;
> +
> +	pci_init_pci_resreq_data(&rr, intel_private.bridge_dev);
>  	ret = pci_bus_alloc_resource(intel_private.bridge_dev->bus, &intel_private.ifp_resource, PAGE_SIZE,
>  				     PAGE_SIZE, PCIBIOS_MIN_MEM, 0,
> -				     pcibios_align_resource, intel_private.bridge_dev);
> +				     pcibios_align_resource, &rr);
>  
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/i915/i915_gmch.c b/drivers/gpu/drm/i915/i915_gmch.c
> index b0ef6ef577a3..d35097bebc0f 100644
> --- a/drivers/gpu/drm/i915/i915_gmch.c
> +++ b/drivers/gpu/drm/i915/i915_gmch.c
> @@ -38,6 +38,7 @@ static int mchbar_reg(struct drm_i915_private *i915)
>  static int
>  intel_alloc_mchbar_resource(struct drm_i915_private *i915)
>  {
> +	struct pci_resreq_data rr;
>  	u32 temp_lo, temp_hi = 0;
>  	u64 mchbar_addr;
>  	int ret;
> @@ -55,12 +56,12 @@ intel_alloc_mchbar_resource(struct drm_i915_private *i915)
>  	/* Get some space for it */
>  	i915->gmch.mch_res.name = "i915 MCHBAR";
>  	i915->gmch.mch_res.flags = IORESOURCE_MEM;
> +	pci_init_pci_resreq_data(&rr, i915->gmch.pdev);
>  	ret = pci_bus_alloc_resource(i915->gmch.pdev->bus,
>  				     &i915->gmch.mch_res,
>  				     MCHBAR_SIZE, MCHBAR_SIZE,
>  				     PCIBIOS_MIN_MEM,
> -				     0, pcibios_align_resource,
> -				     i915->gmch.pdev);
> +				     0, pcibios_align_resource, &rr);
>  	if (ret) {
>  		drm_dbg(&i915->drm, "failed bus alloc: %d\n", ret);
>  		i915->gmch.mch_res.start = 0;
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b2879a6be5f8..1b7a4469c6ca 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -6444,8 +6444,8 @@ static DEFINE_SPINLOCK(resource_alignment_lock);
>   * RETURNS: Resource alignment if it is specified.
>   *          Zero if it is not specified.
>   */
> -static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
> -							bool *resize)
> +resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
> +						 bool *resize)
>  {
>  	int align_order, count;
>  	resource_size_t align = pcibios_default_alignment();
> @@ -6497,6 +6497,7 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
>  	spin_unlock(&resource_alignment_lock);
>  	return align;
>  }
> +EXPORT_SYMBOL_GPL(pci_specified_resource_alignment);
>  
>  static void pci_request_resource_alignment(struct pci_dev *dev, int bar,
>  					   resource_size_t align, bool resize)
> diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
> index 376f09630a4a..441a62807719 100644
> --- a/drivers/pci/setup-res.c
> +++ b/drivers/pci/setup-res.c
> @@ -265,17 +265,21 @@ resource_size_t pci_resource_alignment(const struct pci_dev *dev,
>   * before res->start if there's enough free space there. This enables
>   * tighter packing for resources.
>   */
> -resource_size_t pci_align_resource(struct pci_dev *dev,
> +resource_size_t pci_align_resource(struct pci_resreq_data *rr,
>  				   const struct resource *res,
>  				   const struct resource *empty_res,
>  				   resource_size_t size,
>  				   resource_size_t align)
>  {
> +	struct pci_dev *dev = rr->dev;
>  	resource_size_t remainder, start_addr;
>  
>  	if (!(res->flags & IORESOURCE_MEM))
>  		return res->start;
>  
> +	if (rr->user_align)
> +		return res->start;
> +
>  	if (IS_ALIGNED(size, align))
>  		return res->start;
>  
> @@ -306,18 +310,21 @@ resource_size_t __weak pcibios_align_resource(void *data,
>  					      resource_size_t size,
>  					      resource_size_t align)
>  {
> -	struct pci_dev *dev = data;
> +	struct pci_resreq_data *rr = data;
>  
> -	return pci_align_resource(dev, res, empty_res, size, align);
> +	return pci_align_resource(rr, res, empty_res, size, align);
>  }
>  
>  static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
>  		int resno, resource_size_t size, resource_size_t align)
>  {
>  	struct resource *res = pci_resource_n(dev, resno);
> +	struct pci_resreq_data rr;
>  	resource_size_t min;
>  	int ret;
>  
> +	pci_init_pci_resreq_data(&rr, dev);
> +
>  	min = (res->flags & IORESOURCE_IO) ? PCIBIOS_MIN_IO : PCIBIOS_MIN_MEM;
>  
>  	/*
> @@ -329,7 +336,7 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
>  	 */
>  	ret = pci_bus_alloc_resource(bus, res, size, align, min,
>  				     IORESOURCE_PREFETCH | IORESOURCE_MEM_64,
> -				     pcibios_align_resource, dev);
> +				     pcibios_align_resource, &rr);
>  	if (ret == 0)
>  		return 0;
>  
> @@ -341,7 +348,7 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
>  	     (IORESOURCE_PREFETCH | IORESOURCE_MEM_64)) {
>  		ret = pci_bus_alloc_resource(bus, res, size, align, min,
>  					     IORESOURCE_PREFETCH,
> -					     pcibios_align_resource, dev);
> +					     pcibios_align_resource, &rr);
>  		if (ret == 0)
>  			return 0;
>  	}
> @@ -354,7 +361,7 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
>  	 */
>  	if (res->flags & (IORESOURCE_PREFETCH | IORESOURCE_MEM_64))
>  		ret = pci_bus_alloc_resource(bus, res, size, align, min, 0,
> -					     pcibios_align_resource, dev);
> +					     pcibios_align_resource, &rr);
>  
>  	return ret;
>  }
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index d31a8d107b1e..ea7d75871269 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1217,12 +1217,40 @@ int __must_check pcibios_enable_device(struct pci_dev *, int mask);
>  /* Architecture-specific versions may override this (weak) */
>  char *pcibios_setup(char *str);
>  
> -/* Used only when drivers/pci/setup.c is used */
> +/* Resource assignment request handling */
> +/**
> + * struct pci_resreq_data - Data passed to pcibios_align_resource()
> + * @dev: PCI device
> + * @user_align: User provided alignment override (pci=resource_alignment param)
> + */
> +struct pci_resreq_data {
> +	struct pci_dev *dev;
> +	resource_size_t user_align;
> +};
> +
> +resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
> +						 bool *resize);
> +/*
> + * TODO: Improve PCI resource assignment interface so that callers outside PCI
> + * core do not need to know about this detail.
> + */
> +static inline void pci_init_pci_resreq_data(struct pci_resreq_data *rr,
> +					    struct pci_dev *dev)
> +{
> +	bool user_aligned;
> +
> +	memset(rr, 0, sizeof(*rr));
> +	rr->dev = dev;
> +	rr->user_align = pci_specified_resource_alignment(dev, &user_aligned);
> +	if (!user_aligned)
> +		rr->user_align = 0;
> +}
> +
>  resource_size_t pcibios_align_resource(void *data, const struct resource *res,
>  				       const struct resource *empty_res,
>  				       resource_size_t size,
>  				       resource_size_t align);
> -resource_size_t pci_align_resource(struct pci_dev *dev,
> +resource_size_t pci_align_resource(struct pci_resreq_data *rr,
>  				   const struct resource *res,
>  				   const struct resource *empty_res,
>  				   resource_size_t size,
> -- 
> 2.47.3
> 

[PATCH v3 4/8] PCI: Fix nesting windows with remainder at the left edge

[Ilpo Järvinen]

If a bridge window has its non-aligning remainder at the left edge of
the window, assigning a nested bridge window of same size will fail
because resource side code will only allow starting a candicate range
from an aligning address. In such case, aligned address is somewhere in
the middle, not at the left edge of the window. Since the entire
available space is required for full-sized nested bridge window,
resource side code rejects the free space.

To solve this problem, PCI side code has to bypass the resource side
alignment code by giving a smaller alignment and must check the
aligment requirement itself. This allows resource side code to consider
the entire available space as candidate to fit the required size.

Wrap pcibios_align_resource() with PCI core function that performs the
alignment checks and left edge recalculation if needed. As the
alignment given for the resource side code is artificially small, the
real alignment has to be passed through alignf_data.

Fixes: 9036bd0efcb6 ("PCI: Align head space better")
Tested-by: Nikolas Joshua Britton <nbritton@exabit.io>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---

I've not seen a report about this problem (thus no log quotes) but with
nested thunderbolt topologies it seems well within realms of possibility
of occurring.
---
 drivers/pci/setup-res.c | 60 +++++++++++++++++++++++++++++++++++++----
 include/linux/pci.h     |  1 +
 2 files changed, 56 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index 441a62807719..2ca3784aa323 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -13,6 +13,8 @@
  *	     Resource sorting
  */
 
+#include <linux/align.h>
+#include <linux/bug.h>
 #include <linux/kernel.h>
 #include <linux/export.h>
 #include <linux/pci.h>
@@ -260,6 +262,12 @@ resource_size_t pci_resource_alignment(const struct pci_dev *dev,
 	return resource_alignment(res);
 }
 
+static resource_size_t pci_resreq_remainder(resource_size_t size,
+					    resource_size_t align)
+{
+	return size - ALIGN_DOWN(size, align);
+}
+
 /*
  * For mem bridge windows, try to relocate tail remainder space to space
  * before res->start if there's enough free space there. This enables
@@ -280,10 +288,17 @@ resource_size_t pci_align_resource(struct pci_resreq_data *rr,
 	if (rr->user_align)
 		return res->start;
 
-	if (IS_ALIGNED(size, align))
+	remainder = pci_resreq_remainder(size, align);
+	if (!remainder)
+		return res->start;
+
+	/*
+	 * Size constraints forced an early start move in
+	 * pci_check_and_align_resource()?
+	 */
+	if (!IS_ALIGNED(res->start, align))
 		return res->start;
 
-	remainder = size - ALIGN_DOWN(size, align);
 	/* Don't mess with size that doesn't align with window size granularity */
 	if (!IS_ALIGNED(remainder, pci_min_window_alignment(dev->bus, res->flags)))
 		return res->start;
@@ -315,6 +330,33 @@ resource_size_t __weak pcibios_align_resource(void *data,
 	return pci_align_resource(rr, res, empty_res, size, align);
 }
 
+static resource_size_t pci_resreq_check(void *data,
+					const struct resource *res,
+					const struct resource *empty_res,
+					resource_size_t size,
+					resource_size_t min_align)
+{
+	struct pci_resreq_data *rr = data;
+	resource_size_t align = rr->real_align;
+	resource_size_t remainder, start;
+	struct resource tmp = *res;
+
+	if (res->flags & IORESOURCE_MEM && !IS_ALIGNED(res->start, align)) {
+		resource_set_range(&tmp, ALIGN(res->start, align), size);
+		if (!__resource_contains_unbound(empty_res, &tmp)) {
+			remainder = pci_resreq_remainder(size, align);
+			resource_set_range(&tmp, tmp.start - remainder, size);
+			if (!__resource_contains_unbound(empty_res, &tmp))
+				return tmp.start;	/* caller skips range */
+		}
+	}
+
+	start = pcibios_align_resource(rr, &tmp, empty_res, size, align);
+	WARN_ON_ONCE(!IS_ALIGNED(start, min_align));
+
+	return start;
+}
+
 static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
 		int resno, resource_size_t size, resource_size_t align)
 {
@@ -324,9 +366,17 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
 	int ret;
 
 	pci_init_pci_resreq_data(&rr, dev);
+	rr.real_align = align;
 
 	min = (res->flags & IORESOURCE_IO) ? PCIBIOS_MIN_IO : PCIBIOS_MIN_MEM;
 
+	if (!rr.user_align &&
+	    (res->flags & IORESOURCE_MEM) && pci_resreq_remainder(size, align)) {
+		align = 1U;
+		if (pci_resource_is_bridge_win(resno))
+			align = pci_min_window_alignment(bus, res->flags);
+	}
+
 	/*
 	 * First, try exact prefetching match.  Even if a 64-bit
 	 * prefetchable bridge window is below 4GB, we can't put a 32-bit
@@ -336,7 +386,7 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
 	 */
 	ret = pci_bus_alloc_resource(bus, res, size, align, min,
 				     IORESOURCE_PREFETCH | IORESOURCE_MEM_64,
-				     pcibios_align_resource, &rr);
+				     pci_resreq_check, &rr);
 	if (ret == 0)
 		return 0;
 
@@ -348,7 +398,7 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
 	     (IORESOURCE_PREFETCH | IORESOURCE_MEM_64)) {
 		ret = pci_bus_alloc_resource(bus, res, size, align, min,
 					     IORESOURCE_PREFETCH,
-					     pcibios_align_resource, &rr);
+					     pci_resreq_check, &rr);
 		if (ret == 0)
 			return 0;
 	}
@@ -361,7 +411,7 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
 	 */
 	if (res->flags & (IORESOURCE_PREFETCH | IORESOURCE_MEM_64))
 		ret = pci_bus_alloc_resource(bus, res, size, align, min, 0,
-					     pcibios_align_resource, &rr);
+					     pci_resreq_check, &rr);
 
 	return ret;
 }
diff --git a/include/linux/pci.h b/include/linux/pci.h
index ea7d75871269..d7b431c1ac0d 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1226,6 +1226,7 @@ char *pcibios_setup(char *str);
 struct pci_resreq_data {
 	struct pci_dev *dev;
 	resource_size_t user_align;
+	resource_size_t real_align;
 };
 
 resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
-- 
2.47.3

Re: [PATCH v3 4/8] PCI: Fix nesting windows with remainder at the left edge

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pci: uninitialized variable causes random bypass of remainder logic
--

commit d763cceb79214019a0738e421be1a8a6485d37bc
Author: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

PCI: Fix nesting windows with remainder at the left edge

If a bridge window has its non-aligning remainder at the left edge of
the window, assigning a nested bridge window of same size will fail
because resource side code will only allow starting a candicate range
from an aligning address. In such case, aligned address is somewhere in
the middle, not at the left edge of the window. Since the entire
available space is required for full-sized nested bridge window,
resource side code rejects the free space.

> diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
> index 441a62807719d..2ca3784aa323f 100644
> --- a/drivers/pci/setup-res.c
> +++ b/drivers/pci/setup-res.c
[ ... ]
> @@ -324,9 +366,17 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
>  	int ret;
>  
>  	pci_init_pci_resreq_data(&rr, dev);
> +	rr.real_align = align;
>  
>  	min = (res->flags & IORESOURCE_IO) ? PCIBIOS_MIN_IO : PCIBIOS_MIN_MEM;
>  
> +	if (!rr.user_align &&
> +	    (res->flags & IORESOURCE_MEM) && pci_resreq_remainder(size, align)) {
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does the remainder logic rely on rr.user_align being reliably initialized to 0
for non-overridden resources?

In include/linux/pci.h, pci_init_pci_resreq_data() declares an uninitialized
variable bool user_aligned; and passes it by reference:

include/linux/pci.h:pci_init_pci_resreq_data() {
    bool user_aligned;
    ...
    rr->user_align = pci_specified_resource_alignment(dev, &user_aligned);
    if (!user_aligned)
        rr->user_align = 0;
}

If the user did not specify an alignment parameter,
pci_specified_resource_alignment() returns early without writing to
user_aligned:

drivers/pci/pci.c:pci_specified_resource_alignment() {
    ...
    if (!p || !*p)
        goto out;
    ...
out:
    return pcibios_default_alignment();
}

If the stack garbage in user_aligned evaluates to true, rr->user_align
incorrectly retains pcibios_default_alignment(). On architectures where
pcibios_default_alignment() is non-zero (like powernv), rr.user_align
becomes non-zero, causing !rr.user_align to evaluate to false. This skips
the align = 1U override randomly, breaking the remainder alignment logic
for nested bridge windows.

> +		align = 1U;
> +		if (pci_resource_is_bridge_win(resno))
> +			align = pci_min_window_alignment(bus, res->flags);
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013130948.6231-1-ilpo.jarvinen@linux.intel.com?part=4

[PATCH v3 5/8] linux/log2.h: Add 64-bit roundup/down_pow_of_two64() variants

[Ilpo Järvinen]

Generic sounding roundup_pow_of_two() and rounddown_pow_of_two() are
internally based on fls() that limits types to unsigned long. The type
limitation is unexpected and undocumented.

Worse yet, the typing seems also inconsistent between
__builtin_constant_p() and __roundup/down_pow_of_two() branches, the
former using full 64-bit size and the latter truncating type.

The most sensible way seems to be eventually changing types such that
the return type for roundup/down_pow_of_two() is same as input but it
requires auditing all the existing callers that have mixed input/output
types.

As a stop gap solution, add roundup/down_pow_of_two64() that do not
unexpectedly truncate 64-bit input.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 include/linux/log2.h | 52 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/include/linux/log2.h b/include/linux/log2.h
index e17ceb32e0c9..a5c8416d0382 100644
--- a/include/linux/log2.h
+++ b/include/linux/log2.h
@@ -57,6 +57,16 @@ unsigned long __roundup_pow_of_two(unsigned long n)
 	return 1UL << fls_long(n - 1);
 }
 
+/**
+ * __roundup_pow_of_two64() - round up to nearest power of two
+ * @n: value to round up
+ */
+static inline __attribute__((const))
+u64 __roundup_pow_of_two64(u64 n)
+{
+	return 1UL << fls64(n - 1);
+}
+
 /**
  * __rounddown_pow_of_two() - round down to nearest power of two
  * @n: value to round down
@@ -67,6 +77,16 @@ unsigned long __rounddown_pow_of_two(unsigned long n)
 	return 1UL << (fls_long(n) - 1);
 }
 
+/**
+ * __rounddown_pow_of_two64() - round down to nearest power of two
+ * @n: value to round down
+ */
+static inline __attribute__((const))
+u64 __rounddown_pow_of_two64(u64 n)
+{
+	return 1UL << (fls64(n) - 1);
+}
+
 /**
  * const_ilog2 - log base 2 of 32-bit or a 64-bit constant unsigned value
  * @n: parameter
@@ -163,6 +183,23 @@ unsigned long __rounddown_pow_of_two(unsigned long n)
 	__ilog2_u64(n)			\
  )
 
+/**
+ * roundup_pow_of_two64 - round the given value up to nearest power of two
+ * @n: parameter
+ *
+ * round the given value up to the nearest power of two
+ * - the result is undefined when n == 0
+ * - this can be used to initialise global variables from constant data
+ */
+#define roundup_pow_of_two64(n)			\
+(						\
+	__builtin_constant_p(n) ? (		\
+		((n) == 1) ? 1 :		\
+		(1UL << (ilog2((n) - 1) + 1))	\
+				   ) :		\
+	__roundup_pow_of_two64(n)		\
+ )
+
 /**
  * roundup_pow_of_two - round the given value up to nearest power of two
  * @n: parameter
@@ -180,6 +217,21 @@ unsigned long __rounddown_pow_of_two(unsigned long n)
 	__roundup_pow_of_two(n)			\
  )
 
+/**
+ * rounddown_pow_of_two64 - round the given value down to nearest power of two
+ * @n: parameter
+ *
+ * round the given value down to the nearest power of two
+ * - the result is undefined when n == 0
+ * - this can be used to initialise global variables from constant data
+ */
+#define rounddown_pow_of_two64(n)		\
+(						\
+	__builtin_constant_p(n) ? (		\
+		(1UL << ilog2(n))) :		\
+	__rounddown_pow_of_two64(n)		\
+ )
+
 /**
  * rounddown_pow_of_two - round the given value down to nearest power of two
  * @n: parameter
-- 
2.47.3

Re: [PATCH v3 5/8] linux/log2.h: Add 64-bit roundup/down_pow_of_two64() variants

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] log2: 64-bit roundup/down_pow_of_two64() truncates and UBs on 32-bit archs
--

commit 56bd761df1693290739ce316beb7a19a30f72c63
Author: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
linux/log2.h: Add 64-bit roundup/down_pow_of_two64() variants

This commit introduces new 64-bit variants of the power-of-two rounding
functions and macros to prevent unexpected truncation of 64-bit inputs
that occurs with the existing helpers.

[Severity: High]
In include/linux/log2.h, the newly added 64-bit functions
__roundup_pow_of_two64 at line 69, __rounddown_pow_of_two64 at line 89,
and their macro equivalents roundup_pow_of_two64 at line 198 and
rounddown_pow_of_two64 at line 231 all use 1UL as the base for their bit
shift operations.

Since these functions and macros are explicitly designed to process u64
values, can this cause undefined behavior and truncation on 32-bit
architectures?

On platforms where sizeof(unsigned long) is 4, shifting 1UL by 32 or
greater will result in undefined behavior, and the upper 32 bits will be
truncated before the result is promoted to u64.

Would it be safer to use 1ULL as the shift base to ensure correct 64-bit
math across all architectures?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013130948.6231-1-ilpo.jarvinen@linux.intel.com?part=5

[PATCH v3 6/8] PCI: Place resources to either edge of the window

[Ilpo Järvinen]

PCI resource assignment phase based on a greedy algorithm. The
resources are assigned in descending order of required alignment.
Because the PCI BARs are power-of-two sized resources, it mostly works
(bridge windows and VF BARs are composite resources that might not
necessarily have power-of-two size but internally they are still
composed of BARs).

The resource assignment prior to the commit 9036bd0efcb6 ("PCI: Align
head space better") placed resource to/towards the left edge of the
window. The commit 9036bd0efcb6 ("PCI: Align head space better")
altered assignment for resources whose size is not a perfect multiple
of the required alignment by moving the remainder before the left edge
of the window if possible.

The behavior after the commit 9036bd0efcb6 ("PCI: Align head space
better") may result in problems when a bridge window consists of one
large alignment resource and a composite one with a smaller alignment
(this is a typical setup for GPUs PF BAR and VF BARs). The resource
with largest alignment is assigned first and placed such that the
remainder space is left of the assigned resource, which effectively
splits the remaining space into two. While the VF BAR could fit to the
remaining space, it requires continuous free space that is no longer
there because the large resource is now in the middle.

In this log exceprt, BAR 2 is placed in the middle of the window blocking
the larger VF BAR 2 from fitting anywhere:

pci 0000:03:01.0: bridge window [mem 0xa9f8000000-0xbfffffffff 64bit pref]: assigned
pci 0000:04:00.0: BAR 2 [mem 0xb000000000-0xb7ffffffff 64bit pref]: assigned
pci 0000:04:00.0: VF BAR 2 [mem size 0xe00000000 64bit pref]: can't assign; no space
pci 0000:04:00.0: VF BAR 2 [mem size 0xe00000000 64bit pref]: failed to assign

The old behavior, despite being greedy, naturally consumed space from
the left edge leaving the remainder space adjacent to the other free
space. When remainder space is at the left edge of the window, the
greedy algorithm should instead assign to the right edge of the window.
(If both ends do align, either end works equally.)

While defining window edge aware resource assignment algorithm, one
additional thing is useful to note. In DT setups the bridge
windows/root bus resources are often very precisely sized, with right
edge of the window having much smaller alignment that the left edge.
They also often come with a small BAR that should be placed to the
right edge to not block bridge window placed to the left edge of the
window. The bridge window may be entirely optional at this point
because of hotplug. The resource assignment fallback phase assigns
mandatory resources first and in such a case, small BAR gets assigned
first.

One example where mandatory BAR 0 blocks bridge windows from fitting
(both bridge windows wouldn't fit because of qemu not providing enough
space for both bridge windows but one should fit like it was originally
setup by the platform):

pci_bus 0000:0a: root bus resource [mem 0x10a00000-0x10c00fff window]
pci 0000:0a:00.0: [1b36:000c] type 01 class 0x060400 PCIe Root Port
pci 0000:0a:00.0: BAR 0 [mem 0x10c00000-0x10c00fff]
pci 0000:0a:00.0: PCI bridge to [bus 0b-0d]
pci 0000:0a:00.0:   bridge window [mem 0x10a00000-0x10bfffff]
pci 0000:0a:00.0: enabling Extended Tags
pci 0000:0a:00.0: bridge window [mem 0x00100000-0x000fffff 64bit pref] to [bus 0b-0d] add_size 200000 add_align 100000
pci 0000:0a:00.0: bridge window [mem 0x00100000-0x000fffff] to [bus 0b-0d] add_size 200000 add_align 100000
pci 0000:0a:00.0: bridge window [mem 0x10a00000-0x10bfffff]: assigned
pci 0000:0a:00.0: bridge window [mem size 0x00200000 64bit pref]: can't assign; no space
pci 0000:0a:00.0: bridge window [mem size 0x00200000 64bit pref]: failed to assign
pci 0000:0a:00.0: BAR 0 [mem 0x10c00000-0x10c00fff]: assigned
pci 0000:0a:00.0: bridge window [mem 0x10a00000-0x10bfffff]: releasing
pci 0000:0a:00.0: BAR 0 [mem 0x10c00000-0x10c00fff]: releasing
pci 0000:0a:00.0: BAR 0 [mem 0x10a00000-0x10a00fff]: assigned
pci 0000:0a:00.0: bridge window [mem size 0x00200000]: can't assign; no space
pci 0000:0a:00.0: bridge window [mem size 0x00200000]: failed to assign
pci 0000:0a:00.0: bridge window [mem size 0x00200000 64bit pref]: can't assign; no space
pci 0000:0a:00.0: bridge window [mem size 0x00200000 64bit pref]: failed to assign
pci_bus 0000:0a: Some PCI device resources are unassigned, try booting with pci=realloc

To achieve best generalization of the approach, the solution should
also avoid consuming space from the window edge that can fit largest
aligning resources in the future, whenever possible.

Fixes: 9036bd0efcb6 ("PCI: Align head space better")
Reported-by: Alexey Fomenko <alexey.fomenko@intel.com>
Tested-by: Alexey Fomenko <alexey.fomenko@intel.com>
Reported-by: Eric Auger <eauger@redhat.com>
Tested-by: Eric Auger <eric.auger@redhat.com>
Tested-by: Nikolas Joshua Britton <nbritton@exabit.io>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---

Both reports are private discussions (Eric's report started as public
and produced another fix but further problem was only visible in the
privately send logs after that). Thus no links.
---
 drivers/pci/pci.h       |   4 +
 drivers/pci/setup-bus.c |   4 -
 drivers/pci/setup-res.c | 170 ++++++++++++++++++++++++++++++++++------
 3 files changed, 148 insertions(+), 30 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index ba3c3fddddc2..82a5fd267f5b 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -106,6 +106,10 @@ struct pcie_tlp_log;
 #define PCI_BUS_BRIDGE_MEM_WINDOW	1
 #define PCI_BUS_BRIDGE_PREF_MEM_WINDOW	2
 
+#define PCI_RES_TYPE_MASK \
+	(IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_PREFETCH |\
+	 IORESOURCE_MEM_64)
+
 #define PCI_EXP_AER_FLAGS	(PCI_EXP_DEVCTL_CERE | PCI_EXP_DEVCTL_NFERE | \
 				 PCI_EXP_DEVCTL_FERE | PCI_EXP_DEVCTL_URRE)
 
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index e8c94aa1d3c1..7ca0e9f4ffb6 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -31,10 +31,6 @@
 #include <linux/acpi.h>
 #include "pci.h"
 
-#define PCI_RES_TYPE_MASK \
-	(IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_PREFETCH |\
-	 IORESOURCE_MEM_64)
-
 unsigned int pci_flags;
 EXPORT_SYMBOL_GPL(pci_flags);
 
diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index 2ca3784aa323..0833ea88bfa1 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -17,6 +17,9 @@
 #include <linux/bug.h>
 #include <linux/kernel.h>
 #include <linux/export.h>
+#include <linux/limits.h>
+#include <linux/log2.h>
+#include <linux/minmax.h>
 #include <linux/pci.h>
 #include <linux/errno.h>
 #include <linux/ioport.h>
@@ -262,16 +265,84 @@ resource_size_t pci_resource_alignment(const struct pci_dev *dev,
 	return resource_alignment(res);
 }
 
+static resource_size_t pci_max_natural_size(const struct resource *res,
+					    resource_size_t *max_align)
+{
+	resource_size_t size = resource_size(res);
+	resource_size_t powof2, natural_start;
+
+	*max_align = 1;
+	if (!size)
+		return 0;
+
+	powof2 = rounddown_pow_of_two64(size);
+	natural_start = ALIGN(res->start, powof2);
+	/* end = ~0 first overflows, then -1 brings it back */
+	if (natural_start >= ALIGN_DOWN(res->end + 1, powof2) - 1) {
+		powof2 = max(powof2 / 2, 1U);
+		natural_start = ALIGN(res->start, powof2);
+	}
+
+	if (natural_start) {
+		*max_align <<= __ffs64(natural_start);
+	} else {
+		/*
+		 * Zero address has infinite alignment, return the largest
+		 * representable number even if it's not a power of two.
+		 */
+		*max_align = RESOURCE_SIZE_MAX;
+	}
+
+	return powof2;
+}
+
 static resource_size_t pci_resreq_remainder(resource_size_t size,
 					    resource_size_t align)
 {
 	return size - ALIGN_DOWN(size, align);
 }
 
+/**
+ * pci_align_resource - Places resource into empty space range
+ * @rr: PCI resource request data
+ * @res: Candidate range calculated by caller (not among @dev's resources!)
+ * @empty_res: Full free space range
+ * @size: Required size for the resource
+ * @align: Required alignment (see below for details)
+ *
+ * Places resource inside @empty_res honoring @size and @align.
+ *
+ * Returns: start address for the resource.
+ */
 /*
- * For mem bridge windows, try to relocate tail remainder space to space
- * before res->start if there's enough free space there. This enables
- * tighter packing for resources.
+ * Following candidate logic only applies to mem resources currently. For
+ * composite resources not divisable by @align, @align does not apply to
+ * non-aligning remainder part giving some leeway for its placement. There
+ * are 4 candidate positions:
+ *
+ *           W0WWW0WWW0WWW
+ * 1.         AAAAr
+ * 2.        rAAAA
+ * 3.             AAAAr
+ * 4.                rAAAA
+ *
+ * W = bridge window
+ * 0 = bridge window offset matching align
+ * A = aligning part of size (size rounded down by align)
+ * r = non-aligning remainder
+ *
+ * Cases 1 & 3 and 2 & 4 may degenerate to the same candidate.
+ *
+ * Select resource placement based on the remaining free space. Pick the
+ * candidate with which the remaining free space has (in decreasing order of
+ * priority):
+ *
+ *   1. the largest naturally aligning power-of-two-sized address range within,
+ *   2. the largest continous free space,
+ *   3. the largest alignment of the start address for the naturally aligning
+ *      free space range (from check 1).
+ *
+ * Cases 1 & 2 check only right edge free space and 3 & 4 the left edge.
  */
 resource_size_t pci_align_resource(struct pci_resreq_data *rr,
 				   const struct resource *res,
@@ -280,38 +351,85 @@ resource_size_t pci_align_resource(struct pci_resreq_data *rr,
 				   resource_size_t align)
 {
 	struct pci_dev *dev = rr->dev;
-	resource_size_t remainder, start_addr;
+	unsigned long type = res->flags & PCI_RES_TYPE_MASK;
+	resource_size_t best_natural_size = 0, best_size = 0, best_maxalign = 0;
+	resource_size_t aligning, remainder;
+	struct resource candidate[4];
+	unsigned int i;
+	int best = -1;
 
 	if (!(res->flags & IORESOURCE_MEM))
 		return res->start;
 
-	if (rr->user_align)
-		return res->start;
-
 	remainder = pci_resreq_remainder(size, align);
-	if (!remainder)
-		return res->start;
+	aligning = size - remainder;
+
+	candidate[0] = DEFINE_RES(ALIGN(empty_res->start, align), size, type);
+	candidate[1] = DEFINE_RES(ALIGN(empty_res->start, align) - remainder,
+				  size, type);
+	candidate[2] = DEFINE_RES(ALIGN_DOWN(empty_res->end + 1 - remainder, align) -
+				  aligning, size, type);
+	candidate[3] = DEFINE_RES(ALIGN_DOWN(empty_res->end + 1, align) - size,
+				  size, type);
+
+	for (i = 0; i < ARRAY_SIZE(candidate); i++) {
+		struct resource remaining;
+		resource_size_t natural_size, size, maxalign;
+
+		if ((candidate[i].start > candidate[i].end) ||
+		    !__resource_contains_unbound(empty_res, &candidate[i])) {
+			pci_dbg(dev, "%pR: candidate %u not within free space %pR\n",
+				&candidate[i], i, empty_res);
+			continue;
+		}
 
-	/*
-	 * Size constraints forced an early start move in
-	 * pci_check_and_align_resource()?
-	 */
-	if (!IS_ALIGNED(res->start, align))
-		return res->start;
+		if (rr->user_align &&
+		    !IS_ALIGNED(candidate[i].start, rr->user_align)) {
+			pci_dbg(dev, "%pR: candidate %u skipped due to resource_alignment=%llx\n",
+				&candidate[i], i,
+				(unsigned long long)rr->user_align);
+			continue;
+		}
 
-	/* Don't mess with size that doesn't align with window size granularity */
-	if (!IS_ALIGNED(remainder, pci_min_window_alignment(dev->bus, res->flags)))
-		return res->start;
-	/* Try to place remainder that doesn't fill align before */
-	if (res->start < remainder)
-		return res->start;
-	start_addr = res->start - remainder;
-	if (empty_res->start > start_addr)
+		remaining.flags = type;
+		if (i <= 1) {
+			remaining.start = candidate[i].end + 1;
+			remaining.end = empty_res->end;
+		} else {
+			remaining.start = empty_res->start;
+			remaining.end = candidate[i].start - 1;
+		}
+
+		natural_size = pci_max_natural_size(&remaining, &maxalign);
+		size = resource_size(&remaining);
+		pci_dbg(dev, "%pR: candidate %u free space naturalsize=%llx size=%llx maxalign=%llx\n",
+			&candidate[i], i,
+			(unsigned long long)natural_size,
+			(unsigned long long)size,
+			(unsigned long long)maxalign);
+		if ((best < 0) ||
+		    (natural_size > best_natural_size) ||
+		    (natural_size == best_natural_size && size > best_size) ||
+		    (natural_size == best_natural_size && size == best_size &&
+		     maxalign > best_maxalign)) {
+			best = i;
+			best_natural_size = natural_size;
+			best_size = size;
+			best_maxalign = maxalign;
+		}
+	}
+
+	/* None fits? Return some address and let the caller deal with it. */
+	if (best == -1)
 		return res->start;
 
-	pci_dbg(dev, "%pR: moving candidate start address below align to %llx\n",
-		res, (unsigned long long)start_addr);
-	return start_addr;
+	pci_dbg(dev, "%pR: candidate %u selected (free space %pR), size: %llx + %llx, align: %llx\n",
+		&candidate[best], best, empty_res,
+		(unsigned long long)aligning,
+		(unsigned long long)remainder,
+		(unsigned long long)align);
+
+	return candidate[best].start;
 }
 
 /*
-- 
2.47.3

Re: [PATCH v3 6/8] PCI: Place resources to either edge of the window

[sashiko-bot]

> PCI resource assignment phase based on a greedy algorithm. The
> resources are assigned in descending order of required alignment.
> Because the PCI BARs are power-of-two sized resources, it mostly works
> (bridge windows and VF BARs are composite resources that might not
> necessarily have power-of-two size but internally they are still
> composed of BARs).
> 
> The resource assignment prior to the commit 9036bd0efcb6 ("PCI: Align
> head space better") placed resource to/towards the left edge of the
> window. The commit 9036bd0efcb6 ("PCI: Align head space better")
> altered assignment for resources whose size is not a perfect multiple
> of the required alignment by moving the remainder before the left edge
> of the window if possible.
> 
> The behavior after the commit 9036bd0efcb6 ("PCI: Align head space
> [ ... ]
> Tested-by: Alexey Fomenko <alexey.fomenko@intel.com>
> Reported-by: Eric Auger <eauger@redhat.com>
> Tested-by: Eric Auger <eric.auger@redhat.com>
> Tested-by: Nikolas Joshua Britton <nbritton@exabit.io>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013130948.6231-1-ilpo.jarvinen@linux.intel.com?part=6

[PATCH v3 7/8] PCI: Fix composite resource sizing

[Ilpo Järvinen]

The bridge window sizing algorithm aims to pack child resources back to
back. With multiple composite child resources whose sizes do not align
to the calculated minimal alignment for the bridge window, back-to-back
placement may not be possible. The non-aligning remainder placement is
limited because it must be adjacent to the rest of the composite
resource.

Effectively, two remainder parts may be placed into the same align
sized block, but sum of their size might not match align. In such case,
a gap is required to meet the alignment requirement of both resources.

Add bridge window gap size calculator. Basic rules:

1) Gaps are only necessary if there is more than one non-aligning
   composite resource within a single bridge window.

2) If there are only two remainder parts that amount to less than align
   together, the required gap is the difference of align and the sum of
   remainder sizes.

3) On other cases, round each remainder part to align to get the gap
   size. Sometimes, smaller size may be possible but due to how sizing
   and assignment are made in different phases, it is not always
   possible to predict where each resource is assigned. Thus, the
   sizing has to play safe.

The gap is calculated based on the minimal alignment for the bridge
window, which may be different for the case with only required
resources and the case with optional resources.

Fixes: 9036bd0efcb6 ("PCI: Align head space better")
Reported-by: Bjorn Helgaas <bhelgaas@google.com>
Tested-by: Bjorn Helgaas <bhelgaas@google.com>
Reported-by: Nikolas Joshua Britton <nbritton@exabit.io>
Tested-by: Nikolas Joshua Britton <nbritton@exabit.io>
Link: https://lore.kernel.org/linux-pci/20260903063124.9316-1-nbritton@exabit.io/
Reported-by: Maciej Grochowski <Maciej.Grochowski@sony.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/setup-bus.c | 84 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 81 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 7ca0e9f4ffb6..2980b499ef75 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1164,6 +1164,79 @@ static inline resource_size_t calculate_mem_align(resource_size_t *aligns,
 	return min_align;
 }
 
+/*
+ * Bridge window gap size calculator.
+ *
+ * Calculates gap (empty space) necessary because of non-aligning composite
+ * resources (VF BARs, bridge windows).
+ *
+ * Rules:
+ *
+ * 1) Gaps are only necessary if there is more than one non-aligning
+ *    composite resource within a single bridge window.
+ *
+ * 2) If there are only two remainder parts that amount to less than win_align
+ *    together, the required gap is the difference of win_align and the sum of
+ *    remainder sizes.
+ *
+ * 3) On other cases, round each remainder part to win_align to get the gap
+ *    size. Sometimes, tighter packing might be possible but due to how
+ *    sizing and assignment are made in different phases, it is not always
+ *    possible to predict where each resource is assigned. Thus, the sizing
+ *    has to play safe even if it may overestimate in some cases.
+ */
+static resource_size_t calculate_win_gap_size(struct pci_bus *bus,
+					      struct resource *b_res,
+					      resource_size_t win_align,
+					      bool optional,
+					      struct list_head *realloc_head)
+{
+	resource_size_t safe_gap = 0, remainders = 0;
+	unsigned int nonaligning = 0;
+	struct pci_dev *dev;
+
+	list_for_each_entry(dev, &bus->devices, bus_list) {
+		struct resource *r;
+		int i;
+
+		pci_dev_for_each_resource(dev, r, i) {
+			resource_size_t r_size, remainder, aligning;
+
+			if (!pdev_resources_assignable(dev) ||
+			    !pdev_resource_should_fit(dev, r))
+				continue;
+			if (b_res != pbus_select_window(bus, r))
+				continue;
+
+			if (!optional && pci_resource_is_optional(dev, i))
+				continue;
+
+			r_size = resource_size(r);
+			if (realloc_head && optional)
+				r_size += get_res_add_size(realloc_head, r);
+			if (r_size <= win_align)
+				continue;
+
+			aligning = ALIGN_DOWN(r_size, win_align);
+			remainder = r_size - aligning;
+			if (!remainder)
+				continue;
+
+			nonaligning++;
+			remainders += remainder;
+			safe_gap += win_align - remainder;
+		}
+	}
+
+	if (nonaligning == 2 && (remainders <= win_align))
+		return win_align - remainders;
+
+	if (nonaligning >= 2)
+		return safe_gap;
+
+	return 0;
+}
+
 /*
  * Calculate bridge window head alignment that leaves no gaps in between
  * resources.
@@ -1281,6 +1354,7 @@ static void pbus_size_mem(struct pci_bus *bus, struct resource *b_res,
 	int order, max_order;
 	resource_size_t children_add_size = 0;
 	resource_size_t add_align = 0;
+	resource_size_t gap_size;
 
 	if (!b_res)
 		return;
@@ -1345,7 +1419,9 @@ static void pbus_size_mem(struct pci_bus *bus, struct resource *b_res,
 	win_align = pci_min_window_alignment(bus, b_res->flags);
 	min_align = calculate_head_align(aligns, max_order);
 	min_align = max(min_align, win_align);
-	size0 = calculate_memsize(size, realloc_head ? 0 : add_size,
+	gap_size = calculate_win_gap_size(bus, b_res, min_align, !realloc_head,
+					  realloc_head);
+	size0 = calculate_memsize(size + gap_size, realloc_head ? 0 : add_size,
 				  0, win_align);
 
 	if (size0) {
@@ -1355,8 +1431,10 @@ static void pbus_size_mem(struct pci_bus *bus, struct resource *b_res,
 
 	if (realloc_head && (add_size > 0 || children_add_size > 0)) {
 		add_align = max(min_align, add_align);
-		size1 = calculate_memsize(size, add_size, children_add_size,
-					  win_align);
+		gap_size = calculate_win_gap_size(bus, b_res, add_align, true,
+						  realloc_head);
+		size1 = calculate_memsize(size + gap_size, add_size,
+					  children_add_size, win_align);
 	}
 
 	if (!size0 && !size1) {
-- 
2.47.3

Re: [PATCH v3 7/8] PCI: Fix composite resource sizing

[sashiko-bot]

> The bridge window sizing algorithm aims to pack child resources back to
> back. With multiple composite child resources whose sizes do not align
> to the calculated minimal alignment for the bridge window, back-to-back
> placement may not be possible. The non-aligning remainder placement is
> limited because it must be adjacent to the rest of the composite
> resource.
> 
> Effectively, two remainder parts may be placed into the same align
> sized block, but sum of their size might not match align. In such case,
> a gap is required to meet the alignment requirement of both resources.
> 
> Add bridge window gap size calculator. Basic rules:
> 
> 1) Gaps are only necessary if there is more than one non-aligning
>    composite resource within a single bridge window.
> [ ... ]
> Reported-by: Nikolas Joshua Britton <nbritton@exabit.io>
> Tested-by: Nikolas Joshua Britton <nbritton@exabit.io>
> Link: https://lore.kernel.org/linux-pci/20260903063124.9316-1-nbritton@exabit.io/
> Reported-by: Maciej Grochowski <Maciej.Grochowski@sony.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013130948.6231-1-ilpo.jarvinen@linux.intel.com?part=7

[PATCH v3 8/8] PCI/quirks: Avoid certain address on Genoa systems

[Ilpo Järvinen]

While testing the resource placement changes, tests hit a case where
igb fails to probe when BAR 0 is placed at 0x9c000000:

   90000000-9cffffff : PCI Bus 0000:a0
  -  90000000-902fffff : PCI Bus 0000:a1
  -    90000000-900fffff : 0000:a1:00.0
  -      90000000-900fffff : igb
  -    90100000-901fffff : 0000:a1:00.0
  -    90200000-90203fff : 0000:a1:00.0
  -      90200000-90203fff : igb
  +  9be00000-9c0fffff : PCI Bus 0000:a1
  +    9be00000-9befffff : 0000:a1:00.0
  +    9bf00000-9bf03fff : 0000:a1:00.0
  +    9c000000-9c0fffff : 0000:a1:00.0
     9c100000-9c17ffff : amd_iommu
     9c180000-9c1803ff : IOAPIC 8

  -       Region 0: Memory at 90000000 (32-bit, non-prefetchable) [size=1M]
  -       Region 3: Memory at 90200000 (32-bit, non-prefetchable) [size=16K]
  -       Expansion ROM at 90100000 [disabled] [size=1M]
  +       Region 0: Memory at 9c000000 (32-bit, non-prefetchable) [size=1M]
  +       Region 3: Memory at 9bf00000 (32-bit, non-prefetchable) [size=16K]
  +       Expansion ROM at 9be00000 [disabled] [size=1M]

igb 0000:a1:00.0 0000:a1:00.0 (uninitialized): PCIe link lost
------------[ cut here ]------------
igb: Failed to read reg 0x18!
WARNING: drivers/net/ethernet/intel/igb/igb_main.c:724 at igb_rd32.cold+0x3c/0x4f [igb], CPU#32: kworker/32:1/706
...
 igb_get_invariants_82575+0xff/0xf00 [igb]
 igb_probe+0x3c8/0x1190 [igb]
 local_pci_probe+0x3b/0x80

It turns out there is a 64kB iomem black hole at 9c000000 that returns
~0 and this is where igb's BAR 0 resides. If another BAR of the same
card is placed into that address, it is similarly black holed.

Mark the problematic 64kB range reserved using a quirk bound to the
bridge found in the problematic system.

Closes: https://bugzilla.kernel.org/show_bug.cgi?id=222074
Suggested-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
Cc: Borislav Petkov <bp@alien8.de>

As an alternative to this, I've also tested reverting the commit
07eab0901ede ("efi/x86: Remove EfiMemoryMappedIO from E820 map") and
that also resolves the issue by retaining "9c000000-9cffffff : Reserved"
entry in the resource tree.

---
 arch/x86/pci/fixup.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
index b301c6c8df75..cc463bf492b6 100644
--- a/arch/x86/pci/fixup.c
+++ b/arch/x86/pci/fixup.c
@@ -822,6 +822,42 @@ DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_AMD, 0x1571, pci_amd_enable_64bit_bar);
 DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_AMD, 0x15b1, pci_amd_enable_64bit_bar);
 DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_AMD, 0x1601, pci_amd_enable_64bit_bar);
 
+/*
+ * TODO: Remove when/if root cause is found.
+ *
+ * Genoa appears to have a 64kB iomem black hole starting at 0x9c000000
+ * address for which all reads return ~0 blocking an overlapping BAR from
+ * working.
+ *
+ * Work around the problem by reserving the space prior to making any iomem
+ * allocations that could overlap with the black hole.
+ */
+static struct resource genoa_black_hole = DEFINE_RES_MEM_NAMED(0x9c000000,
+							       SZ_64K,
+							       "reserved");
+
+static void genoa_iomem_black_hole(struct pci_dev *dev)
+{
+	struct resource *r;
+
+	pci_bus_for_each_resource(dev->bus, r) {
+		if (!r || !r->flags || !resource_assigned(r))
+			continue;
+
+		if (!__resource_contains_unbound(r, &genoa_black_hole))
+			continue;
+
+		if (resource_assigned(&genoa_black_hole))
+			pci_dbg(dev, "iomem black hole workaround already applied\n");
+		else if (!insert_resource(r, &genoa_black_hole))
+			pci_info(dev, "iomem black hole workaround enabled\n");
+		else
+			pci_dbg(dev, "iomem black hole workaround add failed\n");
+		return;
+	}
+}
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AMD, 0x14ab, genoa_iomem_black_hole);
+
 #define RS690_LOWER_TOP_OF_DRAM2	0x30
 #define RS690_LOWER_TOP_OF_DRAM2_VALID	0x1
 #define RS690_UPPER_TOP_OF_DRAM2	0x31
-- 
2.47.3

Re: [PATCH v3 8/8] PCI/quirks: Avoid certain address on Genoa systems

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] pci: dangling parent pointer in static resource on host bridge hot-remove
--

commit a726f0ceef0b80da9ff2163d336b376a4bbb5424
Author: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

PCI/quirks: Avoid certain address on Genoa systems

This commit works around a hardware black hole issue on Genoa systems where
a 64kB region at 0x9c000000 returns ~0. It does this by adding a PCI fixup
quirk that reserves this address space before any overlapping allocations
are made.

Closes: https://bugzilla.kernel.org/show_bug.cgi?id=222074

> diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
> index b301c6c8df753..cc463bf492b63 100644
> --- a/arch/x86/pci/fixup.c
> +++ b/arch/x86/pci/fixup.c

[ ... ]

> @@ -822,6 +822,42 @@ DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_AMD, 0x1571, pci_amd_enable_64bit_bar);
> +static struct resource genoa_black_hole = DEFINE_RES_MEM_NAMED(0x9c000000,
> +							       SZ_64K,
> +							       "reserved");
> +
> +static void genoa_iomem_black_hole(struct pci_dev *dev)
> +{
> +	struct resource *r;
> +
> +	pci_bus_for_each_resource(dev->bus, r) {
> +		if (!r || !r->flags || !resource_assigned(r))
> +			continue;
> +
> +		if (!__resource_contains_unbound(r, &genoa_black_hole))
> +			continue;
> +
> +		if (resource_assigned(&genoa_black_hole))

[Severity: Medium]
Does this resource_assigned() check evaluate to true incorrectly after a host
bridge hot-remove?

When the quirk inserts the static genoa_black_hole resource into the
dynamically allocated host bridge resource window:

insert_resource(r, &genoa_black_hole)
    genoa_black_hole.parent = r;

If the ACPI host bridge is later hot-removed, the PCI core releases its
resource windows by calling release_resource(r). This function unlinks r
from the tree but intentionally does not clear the parent pointers of its
children.

As a result, genoa_black_hole.parent is left as a dangling pointer to the
freed resource r.

If the host bridge is rescanned or re-added, the device is rediscovered
and triggers the quirk again. The quirk checks resource_assigned(), which
evaluates:

static inline bool resource_assigned(const struct resource *res)
{
    return res->parent;
}

Because the pointer is dangling, the check evaluates to true, causing the
quirk to incorrectly skip re-insertion. The rescanned bus is therefore left
vulnerable to the hardware black hole, potentially leading to link loss or
failed initializations.

> +			pci_dbg(dev, "iomem black hole workaround already applied\n");
> +		else if (!insert_resource(r, &genoa_black_hole))
> +			pci_info(dev, "iomem black hole workaround enabled\n");
> +		else
> +			pci_dbg(dev, "iomem black hole workaround add failed\n");
> +		return;
> +	}
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013130948.6231-1-ilpo.jarvinen@linux.intel.com?part=8

Re: [PATCH v3 8/8] PCI/quirks: Avoid certain address on Genoa systems

[Mario Limonciello]

[-- Attachment #1: Type: text/plain, Size: 4409 bytes --]



On 10/13/26 08:09, Ilpo Järvinen wrote:
> While testing the resource placement changes, tests hit a case where
> igb fails to probe when BAR 0 is placed at 0x9c000000:
> 
>     90000000-9cffffff : PCI Bus 0000:a0
>    -  90000000-902fffff : PCI Bus 0000:a1
>    -    90000000-900fffff : 0000:a1:00.0
>    -      90000000-900fffff : igb
>    -    90100000-901fffff : 0000:a1:00.0
>    -    90200000-90203fff : 0000:a1:00.0
>    -      90200000-90203fff : igb
>    +  9be00000-9c0fffff : PCI Bus 0000:a1
>    +    9be00000-9befffff : 0000:a1:00.0
>    +    9bf00000-9bf03fff : 0000:a1:00.0
>    +    9c000000-9c0fffff : 0000:a1:00.0
>       9c100000-9c17ffff : amd_iommu
>       9c180000-9c1803ff : IOAPIC 8
> 
>    -       Region 0: Memory at 90000000 (32-bit, non-prefetchable) [size=1M]
>    -       Region 3: Memory at 90200000 (32-bit, non-prefetchable) [size=16K]
>    -       Expansion ROM at 90100000 [disabled] [size=1M]
>    +       Region 0: Memory at 9c000000 (32-bit, non-prefetchable) [size=1M]
>    +       Region 3: Memory at 9bf00000 (32-bit, non-prefetchable) [size=16K]
>    +       Expansion ROM at 9be00000 [disabled] [size=1M]
> 
> igb 0000:a1:00.0 0000:a1:00.0 (uninitialized): PCIe link lost
> ------------[ cut here ]------------
> igb: Failed to read reg 0x18!
> WARNING: drivers/net/ethernet/intel/igb/igb_main.c:724 at igb_rd32.cold+0x3c/0x4f [igb], CPU#32: kworker/32:1/706
> ...
>   igb_get_invariants_82575+0xff/0xf00 [igb]
>   igb_probe+0x3c8/0x1190 [igb]
>   local_pci_probe+0x3b/0x80
> 
> It turns out there is a 64kB iomem black hole at 9c000000 that returns
> ~0 and this is where igb's BAR 0 resides. If another BAR of the same
> card is placed into that address, it is similarly black holed.
> 
> Mark the problematic 64kB range reserved using a quirk bound to the
> bridge found in the problematic system.
> 
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=222074
> Suggested-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
> Cc: Borislav Petkov <bp@alien8.de>
> 
> As an alternative to this, I've also tested reverting the commit
> 07eab0901ede ("efi/x86: Remove EfiMemoryMappedIO from E820 map") and
> that also resolves the issue by retaining "9c000000-9cffffff : Reserved"
> entry in the resource tree.
> 
> ---
>   arch/x86/pci/fixup.c | 36 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 36 insertions(+)
> 
> diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
> index b301c6c8df75..cc463bf492b6 100644
> --- a/arch/x86/pci/fixup.c
> +++ b/arch/x86/pci/fixup.c
> @@ -822,6 +822,42 @@ DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_AMD, 0x1571, pci_amd_enable_64bit_bar);
>   DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_AMD, 0x15b1, pci_amd_enable_64bit_bar);
>   DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_AMD, 0x1601, pci_amd_enable_64bit_bar);
>   
> +/*
> + * TODO: Remove when/if root cause is found.
> + *
> + * Genoa appears to have a 64kB iomem black hole starting at 0x9c000000
> + * address for which all reads return ~0 blocking an overlapping BAR from
> + * working.
> + *
> + * Work around the problem by reserving the space prior to making any iomem
> + * allocations that could overlap with the black hole.
> + */
> +static struct resource genoa_black_hole = DEFINE_RES_MEM_NAMED(0x9c000000,
> +							       SZ_64K,
> +							       "reserved");
> +
> +static void genoa_iomem_black_hole(struct pci_dev *dev)
> +{
> +	struct resource *r;
> +
> +	pci_bus_for_each_resource(dev->bus, r) {
> +		if (!r || !r->flags || !resource_assigned(r))
> +			continue;
> +
> +		if (!__resource_contains_unbound(r, &genoa_black_hole))
> +			continue;
> +
> +		if (resource_assigned(&genoa_black_hole))
> +			pci_dbg(dev, "iomem black hole workaround already applied\n");
> +		else if (!insert_resource(r, &genoa_black_hole))
> +			pci_info(dev, "iomem black hole workaround enabled\n");
> +		else
> +			pci_dbg(dev, "iomem black hole workaround add failed\n");
> +		return;
> +	}
> +}
> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AMD, 0x14ab, genoa_iomem_black_hole);
> +
>   #define RS690_LOWER_TOP_OF_DRAM2	0x30
>   #define RS690_LOWER_TOP_OF_DRAM2_VALID	0x1
>   #define RS690_UPPER_TOP_OF_DRAM2	0x31

I guess it's good we know the root cause is 07eab0901ede. If we're going 
down the path of making holes in that we may well put the hole in 
efi_remove_e820_mmio() like this (attached).


[-- Attachment #2: 0001-efi-x86-Preserve-Genoa-MMIO-guard-regions-in-E820.patch --]
[-- Type: text/x-patch, Size: 3785 bytes --]

From 17d564bd829934c9627f6ff5e1cc10f301115406 Mon Sep 17 00:00:00 2001
From: Mario Limonciello <mario.limonciello@amd.com>
Date: Tue, 13 Oct 2026 11:54:33 -0500
Subject: [PATCH] efi/x86: Preserve Genoa MMIO guard regions in E820

Commit 07eab0901ede ("efi/x86: Remove EfiMemoryMappedIO from E820 map")
removes every EFI MMIO descriptor of at least 256KB from the E820 map.

This makes PCI host bridge apertures described as EFI MMIO available
for BAR assignment, but assumes that all large descriptors are
allocatable.

On an AMD EPYC 9354 system, firmware supplies a 16MB EFI MMIO descriptor
at the end of each PCI root window.  The first 64KB at 0x9c000000 is not
described by ACPI and reads as all ones.  Removing the descriptor from
E820 allows PCI to assign an igb BAR there, which makes the device
unusable.  The IOMMU and IOAPIC regions farther into the descriptor are
reserved separately.

For Genoa, retain the first 64KB of each matching 16MB descriptor in
E820 while removing the remainder.  This keeps the inaccessible prefix
out of PCI allocation without hard-coding its physical address,
covers every root window, and preserves the BAR space made available
by 07eab0901ede.

Fixes: 07eab0901ede ("efi/x86: Remove EfiMemoryMappedIO from E820 map")
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=222074
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 arch/x86/platform/efi/efi.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 0c39adb96b912..676dca172e351 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -44,10 +44,12 @@
 #include <linux/io.h>
 #include <linux/reboot.h>
 #include <linux/bcd.h>
+#include <linux/sizes.h>
 
 #include <asm/setup.h>
 #include <asm/efi.h>
 #include <asm/e820/api.h>
+#include <asm/processor.h>
 #include <asm/time.h>
 #include <asm/tlbflush.h>
 #include <asm/x86_init.h>
@@ -322,7 +324,7 @@ static void __init efi_clean_memmap(void)
 static void __init efi_remove_e820_mmio(void)
 {
 	efi_memory_desc_t *md;
-	u64 size, start, end;
+	u64 size, start, end, remove_start, remove_size;
 	int i = 0;
 
 	for_each_efi_memory_desc(md) {
@@ -330,10 +332,34 @@ static void __init efi_remove_e820_mmio(void)
 			size = md->num_pages << EFI_PAGE_SHIFT;
 			start = md->phys_addr;
 			end = start + size - 1;
-			if (size >= 256*1024) {
+			remove_start = start;
+			remove_size = size;
+
+			/*
+			 * Genoa firmware places a 16MB MMIO descriptor at the end
+			 * of every PCI root window.  The first 64KB of one such
+			 * descriptor is an undocumented hardware hole, while
+			 * resources such as the IOMMU and IOAPIC are described
+			 * separately farther into the descriptor.  Keep the prefix
+			 * of each such descriptor out of PCI allocation, but make
+			 * the rest available.
+			 */
+			if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
+			    boot_cpu_data.x86 == 0x19 &&
+			    boot_cpu_data.x86_model >= 0x10 &&
+			    boot_cpu_data.x86_model <= 0x1f &&
+			    size == SZ_16M) {
+				pr_info("Preserve mem%02u: MMIO range=[0x%08llx-0x%08llx] (64KB) in e820 map\n",
+					i, start, start + SZ_64K - 1);
+				remove_start += SZ_64K;
+				remove_size -= SZ_64K;
+			}
+
+			if (size >= SZ_256K) {
 				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);
+					i, remove_start, end, remove_size >> 20);
+				e820__range_remove(remove_start, remove_size,
+						   E820_TYPE_RESERVED);
 			} else {
 				pr_info("Not removing mem%02u: MMIO range=[0x%08llx-0x%08llx] (%lluKB) from e820 map\n",
 					i, start, end, size >> 10);
-- 
2.43.0

Re: [PATCH v3 8/8] PCI/quirks: Avoid certain address on Genoa systems

[Mario Limonciello]

On 10/13/26 12:00, Mario Limonciello wrote:
> 
> 
> On 10/13/26 08:09, Ilpo Järvinen wrote:
>> While testing the resource placement changes, tests hit a case where
>> igb fails to probe when BAR 0 is placed at 0x9c000000:
>>
>>     90000000-9cffffff : PCI Bus 0000:a0
>>    -  90000000-902fffff : PCI Bus 0000:a1
>>    -    90000000-900fffff : 0000:a1:00.0
>>    -      90000000-900fffff : igb
>>    -    90100000-901fffff : 0000:a1:00.0
>>    -    90200000-90203fff : 0000:a1:00.0
>>    -      90200000-90203fff : igb
>>    +  9be00000-9c0fffff : PCI Bus 0000:a1
>>    +    9be00000-9befffff : 0000:a1:00.0
>>    +    9bf00000-9bf03fff : 0000:a1:00.0
>>    +    9c000000-9c0fffff : 0000:a1:00.0
>>       9c100000-9c17ffff : amd_iommu
>>       9c180000-9c1803ff : IOAPIC 8
>>
>>    -       Region 0: Memory at 90000000 (32-bit, non-prefetchable) 
>> [size=1M]
>>    -       Region 3: Memory at 90200000 (32-bit, non-prefetchable) 
>> [size=16K]
>>    -       Expansion ROM at 90100000 [disabled] [size=1M]
>>    +       Region 0: Memory at 9c000000 (32-bit, non-prefetchable) 
>> [size=1M]
>>    +       Region 3: Memory at 9bf00000 (32-bit, non-prefetchable) 
>> [size=16K]
>>    +       Expansion ROM at 9be00000 [disabled] [size=1M]
>>
>> igb 0000:a1:00.0 0000:a1:00.0 (uninitialized): PCIe link lost
>> ------------[ cut here ]------------
>> igb: Failed to read reg 0x18!
>> WARNING: drivers/net/ethernet/intel/igb/igb_main.c:724 at 
>> igb_rd32.cold+0x3c/0x4f [igb], CPU#32: kworker/32:1/706
>> ...
>>   igb_get_invariants_82575+0xff/0xf00 [igb]
>>   igb_probe+0x3c8/0x1190 [igb]
>>   local_pci_probe+0x3b/0x80
>>
>> It turns out there is a 64kB iomem black hole at 9c000000 that returns
>> ~0 and this is where igb's BAR 0 resides. If another BAR of the same
>> card is placed into that address, it is similarly black holed.
>>
>> Mark the problematic 64kB range reserved using a quirk bound to the
>> bridge found in the problematic system.
>>
>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=222074
>> Suggested-by: Mario Limonciello <mario.limonciello@amd.com>
>> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>> ---
>> Cc: Borislav Petkov <bp@alien8.de>
>>
>> As an alternative to this, I've also tested reverting the commit
>> 07eab0901ede ("efi/x86: Remove EfiMemoryMappedIO from E820 map") and
>> that also resolves the issue by retaining "9c000000-9cffffff : Reserved"
>> entry in the resource tree.
>>
>> ---
>>   arch/x86/pci/fixup.c | 36 ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 36 insertions(+)
>>
>> diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
>> index b301c6c8df75..cc463bf492b6 100644
>> --- a/arch/x86/pci/fixup.c
>> +++ b/arch/x86/pci/fixup.c
>> @@ -822,6 +822,42 @@ DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_AMD, 
>> 0x1571, pci_amd_enable_64bit_bar);
>>   DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_AMD, 0x15b1, 
>> pci_amd_enable_64bit_bar);
>>   DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_AMD, 0x1601, 
>> pci_amd_enable_64bit_bar);
>> +/*
>> + * TODO: Remove when/if root cause is found.
>> + *
>> + * Genoa appears to have a 64kB iomem black hole starting at 0x9c000000
>> + * address for which all reads return ~0 blocking an overlapping BAR 
>> from
>> + * working.
>> + *
>> + * Work around the problem by reserving the space prior to making any 
>> iomem
>> + * allocations that could overlap with the black hole.
>> + */
>> +static struct resource genoa_black_hole = 
>> DEFINE_RES_MEM_NAMED(0x9c000000,
>> +                                   SZ_64K,
>> +                                   "reserved");
>> +
>> +static void genoa_iomem_black_hole(struct pci_dev *dev)
>> +{
>> +    struct resource *r;
>> +
>> +    pci_bus_for_each_resource(dev->bus, r) {
>> +        if (!r || !r->flags || !resource_assigned(r))
>> +            continue;
>> +
>> +        if (!__resource_contains_unbound(r, &genoa_black_hole))
>> +            continue;
>> +
>> +        if (resource_assigned(&genoa_black_hole))
>> +            pci_dbg(dev, "iomem black hole workaround already 
>> applied\n");
>> +        else if (!insert_resource(r, &genoa_black_hole))
>> +            pci_info(dev, "iomem black hole workaround enabled\n");
>> +        else
>> +            pci_dbg(dev, "iomem black hole workaround add failed\n");
>> +        return;
>> +    }
>> +}
>> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AMD, 0x14ab, 
>> genoa_iomem_black_hole);
>> +
>>   #define RS690_LOWER_TOP_OF_DRAM2    0x30
>>   #define RS690_LOWER_TOP_OF_DRAM2_VALID    0x1
>>   #define RS690_UPPER_TOP_OF_DRAM2    0x31
> 
> I guess it's good we know the root cause is 07eab0901ede. If we're going 
> down the path of making holes in that we may well put the hole in 
> efi_remove_e820_mmio() like this (attached).

I don't really like that though.  If Genoa is affected I would expect 
plenty of other machines are too they just haven't been found yet.

Can we roll back 07eab0901ede and add quirks for the busted machines 
instead?

Re: [PATCH v3 8/8] PCI/quirks: Avoid certain address on Genoa systems

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 5581 bytes --]

On Tue, 13 Oct 2026, Mario Limonciello wrote:
> On 10/13/26 12:00, Mario Limonciello wrote:
> > On 10/13/26 08:09, Ilpo Järvinen wrote:
> > > While testing the resource placement changes, tests hit a case where
> > > igb fails to probe when BAR 0 is placed at 0x9c000000:
> > > 
> > >     90000000-9cffffff : PCI Bus 0000:a0
> > >    -  90000000-902fffff : PCI Bus 0000:a1
> > >    -    90000000-900fffff : 0000:a1:00.0
> > >    -      90000000-900fffff : igb
> > >    -    90100000-901fffff : 0000:a1:00.0
> > >    -    90200000-90203fff : 0000:a1:00.0
> > >    -      90200000-90203fff : igb
> > >    +  9be00000-9c0fffff : PCI Bus 0000:a1
> > >    +    9be00000-9befffff : 0000:a1:00.0
> > >    +    9bf00000-9bf03fff : 0000:a1:00.0
> > >    +    9c000000-9c0fffff : 0000:a1:00.0
> > >       9c100000-9c17ffff : amd_iommu
> > >       9c180000-9c1803ff : IOAPIC 8
> > > 
> > >    -       Region 0: Memory at 90000000 (32-bit, non-prefetchable)
> > > [size=1M]
> > >    -       Region 3: Memory at 90200000 (32-bit, non-prefetchable)
> > > [size=16K]
> > >    -       Expansion ROM at 90100000 [disabled] [size=1M]
> > >    +       Region 0: Memory at 9c000000 (32-bit, non-prefetchable)
> > > [size=1M]
> > >    +       Region 3: Memory at 9bf00000 (32-bit, non-prefetchable)
> > > [size=16K]
> > >    +       Expansion ROM at 9be00000 [disabled] [size=1M]
> > > 
> > > igb 0000:a1:00.0 0000:a1:00.0 (uninitialized): PCIe link lost
> > > ------------[ cut here ]------------
> > > igb: Failed to read reg 0x18!
> > > WARNING: drivers/net/ethernet/intel/igb/igb_main.c:724 at
> > > igb_rd32.cold+0x3c/0x4f [igb], CPU#32: kworker/32:1/706
> > > ...
> > >   igb_get_invariants_82575+0xff/0xf00 [igb]
> > >   igb_probe+0x3c8/0x1190 [igb]
> > >   local_pci_probe+0x3b/0x80
> > > 
> > > It turns out there is a 64kB iomem black hole at 9c000000 that returns
> > > ~0 and this is where igb's BAR 0 resides. If another BAR of the same
> > > card is placed into that address, it is similarly black holed.
> > > 
> > > Mark the problematic 64kB range reserved using a quirk bound to the
> > > bridge found in the problematic system.
> > > 
> > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=222074
> > > Suggested-by: Mario Limonciello <mario.limonciello@amd.com>
> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > ---
> > > Cc: Borislav Petkov <bp@alien8.de>
> > > 
> > > As an alternative to this, I've also tested reverting the commit
> > > 07eab0901ede ("efi/x86: Remove EfiMemoryMappedIO from E820 map") and
> > > that also resolves the issue by retaining "9c000000-9cffffff : Reserved"
> > > entry in the resource tree.
> > > 
> > > ---
> > >   arch/x86/pci/fixup.c | 36 ++++++++++++++++++++++++++++++++++++
> > >   1 file changed, 36 insertions(+)
> > > 
> > > diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
> > > index b301c6c8df75..cc463bf492b6 100644
> > > --- a/arch/x86/pci/fixup.c
> > > +++ b/arch/x86/pci/fixup.c
> > > @@ -822,6 +822,42 @@ DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_AMD, 0x1571,
> > > pci_amd_enable_64bit_bar);
> > >   DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_AMD, 0x15b1,
> > > pci_amd_enable_64bit_bar);
> > >   DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_AMD, 0x1601,
> > > pci_amd_enable_64bit_bar);
> > > +/*
> > > + * TODO: Remove when/if root cause is found.
> > > + *
> > > + * Genoa appears to have a 64kB iomem black hole starting at 0x9c000000
> > > + * address for which all reads return ~0 blocking an overlapping BAR from
> > > + * working.
> > > + *
> > > + * Work around the problem by reserving the space prior to making any
> > > iomem
> > > + * allocations that could overlap with the black hole.
> > > + */
> > > +static struct resource genoa_black_hole =
> > > DEFINE_RES_MEM_NAMED(0x9c000000,
> > > +                                   SZ_64K,
> > > +                                   "reserved");
> > > +
> > > +static void genoa_iomem_black_hole(struct pci_dev *dev)
> > > +{
> > > +    struct resource *r;
> > > +
> > > +    pci_bus_for_each_resource(dev->bus, r) {
> > > +        if (!r || !r->flags || !resource_assigned(r))
> > > +            continue;
> > > +
> > > +        if (!__resource_contains_unbound(r, &genoa_black_hole))
> > > +            continue;
> > > +
> > > +        if (resource_assigned(&genoa_black_hole))
> > > +            pci_dbg(dev, "iomem black hole workaround already
> > > applied\n");
> > > +        else if (!insert_resource(r, &genoa_black_hole))
> > > +            pci_info(dev, "iomem black hole workaround enabled\n");
> > > +        else
> > > +            pci_dbg(dev, "iomem black hole workaround add failed\n");
> > > +        return;
> > > +    }
> > > +}
> > > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AMD, 0x14ab,
> > > genoa_iomem_black_hole);
> > > +
> > >   #define RS690_LOWER_TOP_OF_DRAM2    0x30
> > >   #define RS690_LOWER_TOP_OF_DRAM2_VALID    0x1
> > >   #define RS690_UPPER_TOP_OF_DRAM2    0x31
> > 
> > I guess it's good we know the root cause is 07eab0901ede. If we're going
> > down the path of making holes in that we may well put the hole in
> > efi_remove_e820_mmio() like this (attached).
> 
> I don't really like that though.  If Genoa is affected I would expect plenty
> of other machines are too they just haven't been found yet.

That's what I'm worried as well. My own testing can only cover very little 
ground here.

> Can we roll back 07eab0901ede and add quirks for the busted machines instead?


-- 
 i.