[PATCH 2/5] PCI: Fix nesting windows with remainder at the left edge

[PATCH 2/5] 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")
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 | 64 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 59 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index 376f09630a4a..eacce9e2486b 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
@@ -276,10 +284,17 @@ resource_size_t pci_align_resource(struct pci_dev *dev,
 	if (!(res->flags & IORESOURCE_MEM))
 		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;
@@ -311,15 +326,54 @@ resource_size_t __weak pcibios_align_resource(void *data,
 	return pci_align_resource(dev, res, empty_res, size, align);
 }
 
+struct pci_resreq_data {
+	struct pci_dev *dev;
+	resource_size_t real_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->dev, &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)
 {
+	struct pci_resreq_data rr = { .dev = dev, .real_align = align };
 	struct resource *res = pci_resource_n(dev, resno);
 	resource_size_t min;
 	int ret;
 
 	min = (res->flags & IORESOURCE_IO) ? PCIBIOS_MIN_IO : PCIBIOS_MIN_MEM;
 
+	if ((res->flags & IORESOURCE_MEM) && pci_resreq_remainder(size, align)) {
+		align = min;
+		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
@@ -329,7 +383,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);
+				     pci_resreq_check, &rr);
 	if (ret == 0)
 		return 0;
 
@@ -341,7 +395,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);
+					     pci_resreq_check, &rr);
 		if (ret == 0)
 			return 0;
 	}
@@ -354,7 +408,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);
+					     pci_resreq_check, &rr);
 
 	return ret;
 }
-- 
2.47.3

Re: [PATCH 2/5] PCI: Fix nesting windows with remainder at the left edge

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] pci: __pci_assign_resource assigns address bound min to alignment
- [High] pci: pci_resreq_check incorrectly shifts normal BARs out of alignment
--

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

PCI: Fix nesting windows with remainder at the left edge

This commit attempts to fix a bridge window assignment failure in nested
topologies where the non-aligning remainder sits at the left edge. It
introduces pci_resreq_check() to wrap pcibios_align_resource() and recalculate
the start address if there are constraints, bypassing standard alignment checks.

Fixes: 9036bd0efcb6 ("PCI: Align head space better")

> diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
> index 376f09630a4ab..eacce9e2486bf 100644
> --- a/drivers/pci/setup-res.c
> +++ b/drivers/pci/setup-res.c

[ ... ]

> @@ -311,15 +326,54 @@ resource_size_t __weak pcibios_align_resource(void *data,
>  	return pci_align_resource(dev, res, empty_res, size, align);
>  }
>  
> +struct pci_resreq_data {
> +	struct pci_dev *dev;
> +	resource_size_t real_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);

[Severity: High]
Does this subtraction incorrectly shift normal device BARs out of alignment?

When a user overrides alignment via the pci=resource_alignment= parameter,
the requested alignment can be much larger than the size, resulting in a
non-zero remainder. For normal device BARs, subtracting this remainder shifts
the start address out of the requested alignment boundaries.

Could this silent violation of user alignment requests break use cases like
VFIO pass-through that rely on IOMMU page alignment to isolate device memory?

> +			if (!__resource_contains_unbound(empty_res, &tmp))
> +				return tmp.start;	/* caller skips range */
> +		}
> +	}
> +
> +	start = pcibios_align_resource(rr->dev, &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)
>  {
> +	struct pci_resreq_data rr = { .dev = dev, .real_align = align };
>  	struct resource *res = pci_resource_n(dev, resno);
>  	resource_size_t min;
>  	int ret;
>  
>  	min = (res->flags & IORESOURCE_IO) ? PCIBIOS_MIN_IO : PCIBIOS_MIN_MEM;
>  
> +	if ((res->flags & IORESOURCE_MEM) && pci_resreq_remainder(size, align)) {
> +		align = min;
> +		if (pci_resource_is_bridge_win(resno))
> +			align = pci_min_window_alignment(bus, res->flags);
> +	}

[Severity: High]
Can this logic unintentionally catch normal device BARs with overridden
alignments and cause resource allocation to fail?

If pci=resource_alignment= is used, pci_resreq_remainder() evaluates to
non-zero for a normal device BAR. This block is then entered and align is
overwritten with min (which is PCIBIOS_MIN_MEM).

On architectures where PCIBIOS_MIN_MEM is 0 (like ARM64), this destroys the
original alignment constraint and assigns an alignment of 0. Passing a 0
alignment to allocate_resource() causes ALIGN(x, 0) to evaluate to 0, which
can trigger a catastrophic allocation failure.

Should this condition specifically check if the resource is a bridge window
before proceeding?

> +
>  	/*
>  	 * First, try exact prefetching match.  Even if a 64-bit
>  	 * prefetchable bridge window is below 4GB, we can't put a 32-bit

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

[PATCH 3/5] 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.

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>
Fixes: 9036bd0efcb6 ("PCI: Align head space better")
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.

Unfortunately, those debug resource prints cannot currently use
pci_resource_name() because the original resource (the one within
pci_dev's resource array) isn't available. I'll change that eventually
so that pci_dev's resource is passed to pcibios_align_resource() but
that will require changing its signature again which is a bit tedious
as it requires touching all those arch/ functions.

---
 drivers/pci/pci.h       |   4 +
 drivers/pci/setup-bus.c |   4 -
 drivers/pci/setup-res.c | 157 ++++++++++++++++++++++++++++++++++------
 3 files changed, 137 insertions(+), 28 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index ba3c3fddddc2..691711e56597 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -109,6 +109,10 @@ struct pcie_tlp_log;
 #define PCI_EXP_AER_FLAGS	(PCI_EXP_DEVCTL_CERE | PCI_EXP_DEVCTL_NFERE | \
 				 PCI_EXP_DEVCTL_FERE | PCI_EXP_DEVCTL_URRE)
 
+#define PCI_RES_TYPE_MASK \
+	(IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_PREFETCH |\
+	 IORESOURCE_MEM_64)
+
 extern const unsigned char pcie_link_speed[];
 unsigned char pcie_get_link_speed(unsigned int speed);
 
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 eacce9e2486b..1ab5d167ab5c 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,80 @@ 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_two(size);
+	natural_start = ALIGN(res->start, powof2);
+	if (natural_start >= ALIGN_DOWN(res->end + 1, powof2)) {
+		powof2 = max(powof2 / 2, 1U);
+		natural_start = ALIGN(res->start, powof2);
+	}
+
+	if (natural_start) {
+		*max_align <<= __ffs(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);
 }
 
-/*
- * 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.
+/**
+ * pci_align_resource - Places resource into empty space range
+ * @dev: PCI device resource belongs to
+ * @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. Following
+ * special 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_dev *dev,
 				   const struct resource *res,
@@ -279,35 +346,77 @@ resource_size_t pci_align_resource(struct pci_dev *dev,
 				   resource_size_t size,
 				   resource_size_t align)
 {
-	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;
 
 	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 %pR not within free space %pR\n",
+				res, i, &candidate[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;
+		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;
+		}
 
-	/* 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)
+		natural_size = pci_max_natural_size(&remaining, &maxalign);
+		size = resource_size(&remaining);
+		pci_dbg(dev, "%pR: candidate %u %pR, free space naturalsize=%llx size=%llx maxalign=%llx\n",
+			res, i, &candidate[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: picked candidate %u (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 3/5] PCI: Place resources to either edge of the window

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] pci: undefined behavior from 64-bit resource sizes on 32-bit systems
- [Medium] pci: res->end + 1 overflow breaks natural alignment logic
--

commit 65c3dea851ed27dd4bf3576e0a9387eaa7359798
Author: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Subject: PCI: Place resources to either edge of the window

This commit updates the PCI resource assignment phase to use a window edge
aware resource assignment algorithm. It alters the greedy assignment logic
to place resources either to the left or right edge of the window to avoid
fragmenting contiguous free space.

> diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
> index eacce9e2486bf..1ab5d167ab5c6 100644
> --- a/drivers/pci/setup-res.c
> +++ b/drivers/pci/setup-res.c
[ ... ]
> @@ -262,16 +265,80 @@ 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_two(size);

[Severity: High]
Does this code inadvertently truncate 64-bit resource sizes on 32-bit systems?

When a 32-bit kernel is built with a 64-bit phys_addr_t (like x86 PAE or
ARM LPAE), passing a 64-bit size into rounddown_pow_of_two() casts it to an
unsigned long. If the size is a multiple of 4GB (which is common for large
GPU BARs), the lower 32 bits are silently truncated to 0, causing powof2 to
be evaluated as 0.

> +	natural_start = ALIGN(res->start, powof2);
> +	if (natural_start >= ALIGN_DOWN(res->end + 1, powof2)) {

[Severity: Medium]
Could this boundary check overflow if the window ends exactly at ~0U?

If a 32-bit system maps PCI space just below the 4GB boundary, res->end
could be 0xFFFFFFFF. In that scenario, res->end + 1 wraps to 0, which
causes ALIGN_DOWN(0, powof2) to return 0. The check natural_start >= 0 then
unconditionally evaluates to true for unsigned types, incorrectly forcing
the algorithm to halve powof2 and potentially failing to pack memory windows
tightly.

> +		powof2 = max(powof2 / 2, 1U);
> +		natural_start = ALIGN(res->start, powof2);
> +	}
> +
> +	if (natural_start) {
> +		*max_align <<= __ffs(natural_start);

[Severity: High]
Does this trigger undefined behavior when passing a truncated 64-bit value?

Similar to rounddown_pow_of_two(), __ffs() takes an unsigned long. If a
64-bit natural_start is a multiple of 4GB, the cast truncates it to 0.
Passing 0 to __ffs() invokes undefined behavior, which can lead to
unpredictable results or kernel panics during boot (such as garbage shift
amounts or arch-specific trap instructions on zero inputs).

> +	} 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;
> +}

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

Re: [PATCH 3/5] PCI: Place resources to either edge of the window

[Ilpo Järvinen]

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

On Wed, 23 Sep 2026, sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] pci: undefined behavior from 64-bit resource sizes on 32-bit systems
> - [Medium] pci: res->end + 1 overflow breaks natural alignment logic
> --
> 
> commit 65c3dea851ed27dd4bf3576e0a9387eaa7359798
> Author: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Subject: PCI: Place resources to either edge of the window
> 
> This commit updates the PCI resource assignment phase to use a window edge
> aware resource assignment algorithm. It alters the greedy assignment logic
> to place resources either to the left or right edge of the window to avoid
> fragmenting contiguous free space.
> 
> > diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
> > index eacce9e2486bf..1ab5d167ab5c6 100644
> > --- a/drivers/pci/setup-res.c
> > +++ b/drivers/pci/setup-res.c
> [ ... ]
> > @@ -262,16 +265,80 @@ 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_two(size);
> 
> [Severity: High]
> Does this code inadvertently truncate 64-bit resource sizes on 32-bit systems?
> 
> When a 32-bit kernel is built with a 64-bit phys_addr_t (like x86 PAE or
> ARM LPAE), passing a 64-bit size into rounddown_pow_of_two() casts it to an
> unsigned long. If the size is a multiple of 4GB (which is common for large
> GPU BARs), the lower 32 bits are silently truncated to 0, causing powof2 to
> be evaluated as 0.

This is a pre-existing issue. PCI core already uses __ffs() which is based 
on unsigned long (at least on x86).

There's also a serious discussion in x86 space about future of 32-bit 
support where keeping it wastes development effort:

https://lore.kernel.org/all/bd252483-fb0a-4818-bfaf-b9b6ebd187fe@intel.com/

The non-trivial cost here is avoiding the problematic generic helpers 
(custom coding them) or fixing ffs() and round*_pow_of_two() when input 
size > sizeof(unsigned long), for ability to run >=4GB BARs on a 32-bit 
platform. Yeah, modern GPUs have big BARs but running them on a 32-bit 
platform doesn't sound that great combination.


That being said, I think round*_pow_of_two() interface should be made
to return same type as the input because it simply doesn't make sense to 
convert the value to some other type. I briefly looked into that earlier 
but it requires auditting that all the callers can handle that (IIRC, 
there were even some build failures from making just the type change and
there could be logic flaws as well if something relies on type expansion
to unsigned long).

> > +	natural_start = ALIGN(res->start, powof2);
> > +	if (natural_start >= ALIGN_DOWN(res->end + 1, powof2)) {
> 
> [Severity: Medium]
> Could this boundary check overflow if the window ends exactly at ~0U?
> 
> If a 32-bit system maps PCI space just below the 4GB boundary, res->end
> could be 0xFFFFFFFF. In that scenario, res->end + 1 wraps to 0, which
> causes ALIGN_DOWN(0, powof2) to return 0. The check natural_start >= 0 then
> unconditionally evaluates to true for unsigned types, incorrectly forcing
> the algorithm to halve powof2 and potentially failing to pack memory windows
> tightly.

This will be fixed by subtracting 1 from the aligned result to underflow 
it back from 0 to ~0.

> > +		powof2 = max(powof2 / 2, 1U);
> > +		natural_start = ALIGN(res->start, powof2);
> > +	}
> > +
> > +	if (natural_start) {
> > +		*max_align <<= __ffs(natural_start);
> 
> [Severity: High]
> Does this trigger undefined behavior when passing a truncated 64-bit value?
> 
> Similar to rounddown_pow_of_two(), __ffs() takes an unsigned long. If a
> 64-bit natural_start is a multiple of 4GB, the cast truncates it to 0.
> Passing 0 to __ffs() invokes undefined behavior, which can lead to
> unpredictable results or kernel panics during boot (such as garbage shift
> amounts or arch-specific trap instructions on zero inputs).

Sashiko knows no shame with rather bold claims. I'm pretty sure the answer 
should have fit into its context window, no matter how small that was. ;-)

> > +	} 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;
> > +}
> 
> 

-- 
 i.

[PATCH 4/5] 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>
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 | 79 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 76 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 7ca0e9f4ffb6..9d828a59bd00 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1164,6 +1164,76 @@ 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)
+{
+	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 (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 +1351,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 +1416,8 @@ 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, false);
+	size0 = calculate_memsize(size + gap_size, realloc_head ? 0 : add_size,
 				  0, win_align);
 
 	if (size0) {
@@ -1355,8 +1427,9 @@ 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);
+		size1 = calculate_memsize(size + gap_size, add_size,
+					  children_add_size, win_align);
 	}
 
 	if (!size0 && !size1) {
-- 
2.47.3

Re: [PATCH 4/5] PCI: Fix composite resource sizing

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pci: bridge window gap calculation Rule 2 underestimates required size
--

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

PCI: Fix composite resource sizing

This commit modifies the bridge window sizing algorithm to add a gap size
calculator. The calculator determines the empty space required because of
non-aligning composite child resources, such as VF BARs and bridge windows,
to ensure alignment constraints are met when resources are placed.

Link: https://lore.kernel.org/linux-pci/20260903063124.9316-1-nbritton@exabit.io/

> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 7ca0e9f4ffb63..9d828a59bd002 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
[ ... ]
> @@ -1164,6 +1164,76 @@ static inline resource_size_t calculate_mem_align(resource_size_t *aligns,
[ ... ]
> +static resource_size_t calculate_win_gap_size(struct pci_bus *bus,
> +					      struct resource *b_res,
> +					      resource_size_t win_align,
> +					      bool optional)
> +{
> +	resource_size_t safe_gap = 0, remainders = 0;
> +	unsigned int nonaligning = 0;
> +	struct pci_dev *dev;
[ ... ]
> +			nonaligning++;
> +			remainders += remainder;
> +			safe_gap += win_align - remainder;
> +		}
> +	}
> +
> +	if (nonaligning == 2 && (remainders <= win_align))
> +		return win_align - remainders;

[Severity: High]
Does this logic underestimate the required gap size by assuming two remainder
parts can perfectly share a single alignment block's padding without verifying
if their individual alignment constraints allow tight packing?

If a parent PCI bus contains exactly two child resources (e.g., downstream
bridge windows) whose sizes are not multiples of the bus's max alignment, and
their size remainders sum to less than or equal to the max alignment, this
bypass avoids the safe_gap fallback.

For example, if two 10MB windows require 4MB alignment, this calculates a gap
of (4 - (2+2)) = 0MB, resulting in a 20MB parent window. However, because both
resources require 4MB alignment, if the first is placed at 0..10MB, the second
must be placed at 12MB. This requires a 22MB total span, which exceeds the
calculated 20MB window.

Could this underestimation cause PCI resource assignment to fail during
enumeration for devices or downstream bridges, leaving them uninitialized?

> +
> +	if (nonaligning >= 2)
> +		return safe_gap;
> +
> +	return 0;
> +}

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

Re: [PATCH 4/5] PCI: Fix composite resource sizing

[Ilpo Järvinen]

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

On Wed, 23 Sep 2026, sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] pci: bridge window gap calculation Rule 2 underestimates required size
> --
> 
> commit f9f2d66b1a5c96f99a75fc82cb40a5f073968dfb
> Author: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> 
> PCI: Fix composite resource sizing
> 
> This commit modifies the bridge window sizing algorithm to add a gap size
> calculator. The calculator determines the empty space required because of
> non-aligning composite child resources, such as VF BARs and bridge windows,
> to ensure alignment constraints are met when resources are placed.
> 
> Link: https://lore.kernel.org/linux-pci/20260903063124.9316-1-nbritton@exabit.io/
> 
> > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> > index 7ca0e9f4ffb63..9d828a59bd002 100644
> > --- a/drivers/pci/setup-bus.c
> > +++ b/drivers/pci/setup-bus.c
> [ ... ]
> > @@ -1164,6 +1164,76 @@ static inline resource_size_t calculate_mem_align(resource_size_t *aligns,
> [ ... ]
> > +static resource_size_t calculate_win_gap_size(struct pci_bus *bus,
> > +					      struct resource *b_res,
> > +					      resource_size_t win_align,
> > +					      bool optional)
> > +{
> > +	resource_size_t safe_gap = 0, remainders = 0;
> > +	unsigned int nonaligning = 0;
> > +	struct pci_dev *dev;
> [ ... ]
> > +			nonaligning++;
> > +			remainders += remainder;
> > +			safe_gap += win_align - remainder;
> > +		}
> > +	}
> > +
> > +	if (nonaligning == 2 && (remainders <= win_align))
> > +		return win_align - remainders;
> 
> [Severity: High]
> Does this logic underestimate the required gap size by assuming two remainder
> parts can perfectly share a single alignment block's padding without verifying
> if their individual alignment constraints allow tight packing?
> 
> If a parent PCI bus contains exactly two child resources (e.g., downstream
> bridge windows) whose sizes are not multiples of the bus's max alignment, and
> their size remainders sum to less than or equal to the max alignment, this
> bypass avoids the safe_gap fallback.
> 
> For example, if two 10MB windows require 4MB alignment, this calculates a gap
> of (4 - (2+2)) = 0MB, resulting in a 20MB parent window. However, because both
> resources require 4MB alignment, if the first is placed at 0..10MB, the second
> must be placed at 12MB. This requires a 22MB total span, which exceeds the

This is bogus, sashiko doesn't understand how the new placement algorithm 
works when it comes to remainder parts. The remainder for the second 
resource would start at 10MB and the aligning part will start exactly at 
12MB filling all the constraints and fitting to 20MB.

Thus, the calculation is correct. I suspect this case would work even 
without this patch because no gap is needed in the first place (untested).

> calculated 20MB window.
>
> Could this underestimation cause PCI resource assignment to fail during
> enumeration for devices or downstream bridges, leaving them uninitialized?
>
> > +
> > +	if (nonaligning >= 2)
> > +		return safe_gap;
> > +
> > +	return 0;
> > +}

-- 
 i.

[PATCH 5/5] PCI/quirks: Avoid certain BAR 0 address with igb

[Ilpo Järvinen]

While testing the resource placement changes, my 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

Apparently, the igb driver bails out, after its initial sanity check
detects an unexpected ~0 read. Hacking around the sanity check just
results in more failures down the road so the sanity check itself is not
the cause for the failure.

The resource placement looks valid so the actual placement patches seem
to work normally.

All other possible 1M address I could test (with a hack patch) did work.

Add quirk to reshuffle igb resources, use BAR 3 to block the problematic
address.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---

I know this is ugly and I don't like it either but do not know better
way to avoid the regression.

I've tried with iommu=off and that did not resolve the issue.

I also managed to prove igb works with the same resource layout in
another system. So identifying the case should probably be tightened
by matching with more devices than the one used by igb. This is open
to discussion.

---
 drivers/pci/quirks.c | 56 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index de9bbccda21f..e6f3e2ab1fd4 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -6288,6 +6288,62 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1536, rom_bar_overlap_defect);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1537, rom_bar_overlap_defect);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1538, rom_bar_overlap_defect);
 
+/*
+ * The igb driver probe (due to reads returning ~0 unexpected) when BAR 0
+ * appears at 0x9c000000. The cause is unknown.
+ *
+ * Use BAR 3 to block 0x9c000000 address.
+ */
+static void bar0_address_breakage(struct pci_dev *dev)
+{
+	struct resource *bar0 = pci_resource_n(dev, 0);
+	struct resource *bar3 = pci_resource_n(dev, 3);
+	resource_size_t broken_addr = 0x9c000000;
+	struct resource *res;
+	int i, ret;
+
+	if (bar0->start != broken_addr)
+		return;
+
+	/*
+	 * HW BAR sizes seems to vary. Exclude non-1M BAR 0 case and
+	 * sanity check BAR 0 & 3 before attempting this quirk.
+	 */
+	if (resource_type(bar0) != IORESOURCE_MEM ||
+	    resource_size(bar0) != SZ_1M ||
+	    resource_type(bar3) != IORESOURCE_MEM)
+		return;
+
+	pci_info(dev, "%pR: relocating BAR\n", bar0);
+
+	pci_dev_for_each_resource(dev, res, i) {
+		if (!resource_assigned(res) ||
+		    resource_type(res) != IORESOURCE_MEM)
+			continue;
+
+		pci_release_resource(dev, i);
+	}
+
+	resource_set_range(bar3, broken_addr, resource_size(bar3));
+	bar3->flags &= ~IORESOURCE_UNSET;
+	pci_claim_resource(dev, 3);
+	if (!resource_assigned(bar3)) {
+		bar3->flags |= IORESOURCE_UNSET;
+		pci_warn(dev, "resource relocation failed\n");
+	}
+
+	pci_dev_for_each_resource(dev, res, i) {
+		if (resource_assigned(res) ||
+		    resource_type(res) != IORESOURCE_MEM)
+			continue;
+
+		ret = pci_assign_resource(dev, i);
+		if (ret)
+			pci_warn(dev, "resource relocation failed\n");
+	}
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1533, bar0_address_breakage);
+
 #ifdef CONFIG_PCIEASPM
 /*
  * Several Intel DG2 graphics devices advertise that they can only tolerate
-- 
2.47.3

Re: [PATCH 5/5] PCI/quirks: Avoid certain BAR 0 address with igb

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] pci: skipping hardware update in bar0_address_breakage() causes desync
- [High] pci: ignoring pci_release_resource() return corrupts tree
--

commit 140645b0838c61616e4734fb4c852db528a2c054
Author: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Date:   Wed Sep 23 13:17:55 2026 +0000

    PCI/quirks: Avoid certain BAR 0 address with igb
    
This patch adds a quirk to reshuffle igb resources when BAR 0 is placed at 
0x9c000000, which previously caused the driver to fail during probe due to 
unexpected reads. The quirk uses BAR 3 to block the problematic address by 
relocating it.

> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index de9bbccda21fd..e6f3e2ab1fd4e 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
[ ... ]
> @@ -6288,6 +6288,62 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1536, rom_bar_overlap_defect);
[ ... ]
> +static void bar0_address_breakage(struct pci_dev *dev)
> +{
[ ... ]
> +	pci_dev_for_each_resource(dev, res, i) {
> +		if (!resource_assigned(res) ||
> +		    resource_type(res) != IORESOURCE_MEM)
> +			continue;
> +
> +		pci_release_resource(dev, i);

[Severity: High]
Can ignoring the return value of pci_release_resource() cause tree
corruption?

If a resource fails to release (e.g., returning -EBUSY), it remains actively
linked in the system's resource sibling tree. The code below then modifies
its boundaries unconditionally via resource_set_range().

Does modifying the start and end bounds of a node while it is still linked
violate the strict address-ordered sibling list invariants of the resource
tree, and cause pci_claim_resource() to corrupt pointers by re-inserting it?

> +	}
> +
> +	resource_set_range(bar3, broken_addr, resource_size(bar3));
> +	bar3->flags &= ~IORESOURCE_UNSET;
> +	pci_claim_resource(dev, 3);
> +	if (!resource_assigned(bar3)) {
> +		bar3->flags |= IORESOURCE_UNSET;
> +		pci_warn(dev, "resource relocation failed\n");
> +	}
> +
> +	pci_dev_for_each_resource(dev, res, i) {
> +		if (resource_assigned(res) ||
> +		    resource_type(res) != IORESOURCE_MEM)
> +			continue;
> +
> +		ret = pci_assign_resource(dev, i);

[Severity: High]
Does skipping BAR 3 in this loop cause a desync between the software
resource tree and the physical hardware?

Since pci_claim_resource() updates the software resource tree but does not
write to the physical hardware PCI configuration registers, the hardware BAR
needs to be updated.

However, because BAR 3 was claimed and marked assigned in software earlier,
the resource_assigned() check skips it here. This permanently bypasses the
hardware register update. 

Would this leave the OS thinking BAR 3 is at 0x9c000000 while the physical
hardware remains at its old address, causing the MSI-X table reads to fail
and creating a possible PCIe bus conflict?

> +		if (ret)
> +			pci_warn(dev, "resource relocation failed\n");
> +	}
> +}

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

Re: [PATCH 5/5] PCI/quirks: Avoid certain BAR 0 address with igb

[Bjorn Helgaas]

[+cc Rafael, ACPI resource question]

On Wed, Sep 23, 2026 at 04:17:55PM +0300, Ilpo Järvinen wrote:
> While testing the resource placement changes, my 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
> 
> Apparently, the igb driver bails out, after its initial sanity check
> detects an unexpected ~0 read. Hacking around the sanity check just
> results in more failures down the road so the sanity check itself is not
> the cause for the failure.
> 
> The resource placement looks valid so the actual placement patches seem
> to work normally.
> 
> All other possible 1M address I could test (with a hack patch) did work.

Super weird.  Is it possible there's some other device there?  It's
conceivable ACPI might have a _CRS method describing it.  I think
there are ACPI devices for which we don't reserve space mentioned in
_CRS.  Maybe Rafael knows a debug option to log everything in _CRS?

Does igb seem sensitive about this exact address on a variety of
machines?  If so I would expect some kind of igb hardware erratum for
it.

Do other non-igb devices work at that address?

> Add quirk to reshuffle igb resources, use BAR 3 to block the problematic
> address.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
> 
> I know this is ugly and I don't like it either but do not know better
> way to avoid the regression.
> 
> I've tried with iommu=off and that did not resolve the issue.
> 
> I also managed to prove igb works with the same resource layout in
> another system. So identifying the case should probably be tightened
> by matching with more devices than the one used by igb. This is open
> to discussion.

I guess this answers one of my questions above.

> ---
>  drivers/pci/quirks.c | 56 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index de9bbccda21f..e6f3e2ab1fd4 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -6288,6 +6288,62 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1536, rom_bar_overlap_defect);
>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1537, rom_bar_overlap_defect);
>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1538, rom_bar_overlap_defect);
>  
> +/*
> + * The igb driver probe (due to reads returning ~0 unexpected) when BAR 0
> + * appears at 0x9c000000. The cause is unknown.

I suppose this is missing "fails"?  "igb driver probe fails"?

> + * Use BAR 3 to block 0x9c000000 address.
> + */
> +static void bar0_address_breakage(struct pci_dev *dev)
> +{
> +	struct resource *bar0 = pci_resource_n(dev, 0);
> +	struct resource *bar3 = pci_resource_n(dev, 3);
> +	resource_size_t broken_addr = 0x9c000000;
> +	struct resource *res;
> +	int i, ret;
> +
> +	if (bar0->start != broken_addr)
> +		return;
> +
> +	/*
> +	 * HW BAR sizes seems to vary. Exclude non-1M BAR 0 case and
> +	 * sanity check BAR 0 & 3 before attempting this quirk.
> +	 */
> +	if (resource_type(bar0) != IORESOURCE_MEM ||
> +	    resource_size(bar0) != SZ_1M ||
> +	    resource_type(bar3) != IORESOURCE_MEM)
> +		return;
> +
> +	pci_info(dev, "%pR: relocating BAR\n", bar0);
> +
> +	pci_dev_for_each_resource(dev, res, i) {
> +		if (!resource_assigned(res) ||
> +		    resource_type(res) != IORESOURCE_MEM)
> +			continue;
> +
> +		pci_release_resource(dev, i);
> +	}
> +
> +	resource_set_range(bar3, broken_addr, resource_size(bar3));
> +	bar3->flags &= ~IORESOURCE_UNSET;
> +	pci_claim_resource(dev, 3);
> +	if (!resource_assigned(bar3)) {
> +		bar3->flags |= IORESOURCE_UNSET;
> +		pci_warn(dev, "resource relocation failed\n");
> +	}
> +
> +	pci_dev_for_each_resource(dev, res, i) {
> +		if (resource_assigned(res) ||
> +		    resource_type(res) != IORESOURCE_MEM)
> +			continue;
> +
> +		ret = pci_assign_resource(dev, i);
> +		if (ret)
> +			pci_warn(dev, "resource relocation failed\n");
> +	}
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1533, bar0_address_breakage);
> +
>  #ifdef CONFIG_PCIEASPM
>  /*
>   * Several Intel DG2 graphics devices advertise that they can only tolerate
> -- 
> 2.47.3
> 

Re: [PATCH 5/5] PCI/quirks: Avoid certain BAR 0 address with igb

[Ilpo Järvinen]

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

On Thu, 24 Sep 2026, Bjorn Helgaas wrote:

> [+cc Rafael, ACPI resource question]
> 
> On Wed, Sep 23, 2026 at 04:17:55PM +0300, Ilpo Järvinen wrote:
> > While testing the resource placement changes, my 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
> > 
> > Apparently, the igb driver bails out, after its initial sanity check
> > detects an unexpected ~0 read. Hacking around the sanity check just
> > results in more failures down the road so the sanity check itself is not
> > the cause for the failure.
> > 
> > The resource placement looks valid so the actual placement patches seem
> > to work normally.
> > 
> > All other possible 1M address I could test (with a hack patch) did work.
> 
> Super weird.  Is it possible there's some other device there?  It's
> conceivable ACPI might have a _CRS method describing it.  I think
> there are ACPI devices for which we don't reserve space mentioned in
> _CRS.  Maybe Rafael knows a debug option to log everything in _CRS?

Now that you mentioned it, there certainly something going on with 
that address:

[    0.000000] BIOS-e820: [mem 0x0000000070000000-0x000000008fffffff]  device reserved
[    0.000000] BIOS-e820: [gap 0x0000000090000000-0x000000009bffffff]
[    0.000000] BIOS-e820: [mem 0x000000009c000000-0x000000009cffffff]  device reserved
[    0.000000] BIOS-e820: [gap 0x000000009d000000-0x00000000a8ffffff]
[    0.000000] BIOS-e820: [mem 0x00000000a9000000-0x00000000a9ffffff]  device reserved
...
[    0.000000] efi: Remove mem48: MMIO range=[0x80000000-0x8fffffff] (256MB) from e820 map
[    0.000000] e820: remove [mem 0x80000000-0x8fffffff] device reserved
[    0.000000] efi: Remove mem49: MMIO range=[0x9c000000-0x9cffffff] (16MB) from e820 map
[    0.000000] e820: remove [mem 0x9c000000-0x9cffffff] device reserved
[    0.000000] efi: Remove mem50: MMIO range=[0xa9000000-0xa9ffffff] (16MB) from e820 map
[    0.000000] e820: remove [mem 0xa9000000-0xa9ffffff] device reserved

But given the comment above efi_remove_e820_mmio() it sounds like this is 
a red herring.

In any case, the address is inside the provided root bus resource:

[    4.854840] ACPI: PCI Root Bridge [PC05] (domain 0000 [bus a0-bf])
...
[    4.854848] PCI host bridge to bus 0000:a0
[    4.854848] pci_bus 0000:a0: root bus resource [io  0x6000-0x6fff window]
[    4.854848] pci_bus 0000:a0: root bus resource [mem 0x90000000-0x9cffffff window]
[    4.854848] pci_bus 0000:a0: root bus resource [mem 0x71d60000000-0x7fcffffffff window]
[    4.854848] pci_bus 0000:a0: root bus resource [bus a0-bf]

> Does igb seem sensitive about this exact address on a variety of
> machines?  If so I would expect some kind of igb hardware erratum for
> it.

I've not heard anything to that effect.

But existance of the sanity check itself in the igb driver looks almost 
like a smoking gun so I don't know what to think of it.

I'm also inclined to think that the placement approaches out there so far 
might not have covered that many addresses but used the left edge of the 
window. So this series, when it often moves resources to right edge of the 
window, goes to what might not be on well-charted territory.

> Do other non-igb devices work at that address?

Unfortunately there are not other devices underneath the RP so I might not 
be able to test this.

> > Add quirk to reshuffle igb resources, use BAR 3 to block the problematic
> > address.
> > 
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > ---
> > 
> > I know this is ugly and I don't like it either but do not know better
> > way to avoid the regression.
> > 
> > I've tried with iommu=off and that did not resolve the issue.
> > 
> > I also managed to prove igb works with the same resource layout in
> > another system. So identifying the case should probably be tightened
> > by matching with more devices than the one used by igb. This is open
> > to discussion.
> 
> I guess this answers one of my questions above.

Yeah.

But then there's the sanity check in igb which hints otherwise.

> > ---
> >  drivers/pci/quirks.c | 56 ++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 56 insertions(+)
> > 
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index de9bbccda21f..e6f3e2ab1fd4 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -6288,6 +6288,62 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1536, rom_bar_overlap_defect);
> >  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1537, rom_bar_overlap_defect);
> >  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1538, rom_bar_overlap_defect);
> >  
> > +/*
> > + * The igb driver probe (due to reads returning ~0 unexpected) when BAR 0
> > + * appears at 0x9c000000. The cause is unknown.
> 
> I suppose this is missing "fails"?  "igb driver probe fails"?

Obviously, thanks.

-- 
 i.
 
> > + * Use BAR 3 to block 0x9c000000 address.
> > + */
> > +static void bar0_address_breakage(struct pci_dev *dev)
> > +{
> > +	struct resource *bar0 = pci_resource_n(dev, 0);
> > +	struct resource *bar3 = pci_resource_n(dev, 3);
> > +	resource_size_t broken_addr = 0x9c000000;
> > +	struct resource *res;
> > +	int i, ret;
> > +
> > +	if (bar0->start != broken_addr)
> > +		return;
> > +
> > +	/*
> > +	 * HW BAR sizes seems to vary. Exclude non-1M BAR 0 case and
> > +	 * sanity check BAR 0 & 3 before attempting this quirk.
> > +	 */
> > +	if (resource_type(bar0) != IORESOURCE_MEM ||
> > +	    resource_size(bar0) != SZ_1M ||
> > +	    resource_type(bar3) != IORESOURCE_MEM)
> > +		return;
> > +
> > +	pci_info(dev, "%pR: relocating BAR\n", bar0);
> > +
> > +	pci_dev_for_each_resource(dev, res, i) {
> > +		if (!resource_assigned(res) ||
> > +		    resource_type(res) != IORESOURCE_MEM)
> > +			continue;
> > +
> > +		pci_release_resource(dev, i);
> > +	}
> > +
> > +	resource_set_range(bar3, broken_addr, resource_size(bar3));
> > +	bar3->flags &= ~IORESOURCE_UNSET;
> > +	pci_claim_resource(dev, 3);
> > +	if (!resource_assigned(bar3)) {
> > +		bar3->flags |= IORESOURCE_UNSET;
> > +		pci_warn(dev, "resource relocation failed\n");
> > +	}
> > +
> > +	pci_dev_for_each_resource(dev, res, i) {
> > +		if (resource_assigned(res) ||
> > +		    resource_type(res) != IORESOURCE_MEM)
> > +			continue;
> > +
> > +		ret = pci_assign_resource(dev, i);
> > +		if (ret)
> > +			pci_warn(dev, "resource relocation failed\n");
> > +	}
> > +}
> > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1533, bar0_address_breakage);
> > +
> >  #ifdef CONFIG_PCIEASPM
> >  /*
> >   * Several Intel DG2 graphics devices advertise that they can only tolerate
> > -- 
> > 2.47.3
> > 
> 

Re: [PATCH 5/5] PCI/quirks: Avoid certain BAR 0 address with igb

[David Laight]

On Mon, 28 Sep 2026 15:20:13 +0300 (EEST)
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> On Thu, 24 Sep 2026, Bjorn Helgaas wrote:
> 
> > [+cc Rafael, ACPI resource question]
> > 
> > On Wed, Sep 23, 2026 at 04:17:55PM +0300, Ilpo Järvinen wrote:  
> > > While testing the resource placement changes, my 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

Is that valid?
I'm no expect but I wouldn't expect an address range to cross a power of 2 boundary.

David

> > >   +    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
> > > 
> > > Apparently, the igb driver bails out, after its initial sanity check
> > > detects an unexpected ~0 read. Hacking around the sanity check just
> > > results in more failures down the road so the sanity check itself is not
> > > the cause for the failure.
> > > 
> > > The resource placement looks valid so the actual placement patches seem
> > > to work normally.
> > > 
> > > All other possible 1M address I could test (with a hack patch) did work.  
> > 
> > Super weird.  Is it possible there's some other device there?  It's
> > conceivable ACPI might have a _CRS method describing it.  I think
> > there are ACPI devices for which we don't reserve space mentioned in
> > _CRS.  Maybe Rafael knows a debug option to log everything in _CRS?  
> 
> Now that you mentioned it, there certainly something going on with 
> that address:
> 
> [    0.000000] BIOS-e820: [mem 0x0000000070000000-0x000000008fffffff]  device reserved
> [    0.000000] BIOS-e820: [gap 0x0000000090000000-0x000000009bffffff]
> [    0.000000] BIOS-e820: [mem 0x000000009c000000-0x000000009cffffff]  device reserved
> [    0.000000] BIOS-e820: [gap 0x000000009d000000-0x00000000a8ffffff]
> [    0.000000] BIOS-e820: [mem 0x00000000a9000000-0x00000000a9ffffff]  device reserved
> ...
> [    0.000000] efi: Remove mem48: MMIO range=[0x80000000-0x8fffffff] (256MB) from e820 map
> [    0.000000] e820: remove [mem 0x80000000-0x8fffffff] device reserved
> [    0.000000] efi: Remove mem49: MMIO range=[0x9c000000-0x9cffffff] (16MB) from e820 map
> [    0.000000] e820: remove [mem 0x9c000000-0x9cffffff] device reserved
> [    0.000000] efi: Remove mem50: MMIO range=[0xa9000000-0xa9ffffff] (16MB) from e820 map
> [    0.000000] e820: remove [mem 0xa9000000-0xa9ffffff] device reserved
> 
> But given the comment above efi_remove_e820_mmio() it sounds like this is 
> a red herring.
> 
> In any case, the address is inside the provided root bus resource:
> 
> [    4.854840] ACPI: PCI Root Bridge [PC05] (domain 0000 [bus a0-bf])
> ...
> [    4.854848] PCI host bridge to bus 0000:a0
> [    4.854848] pci_bus 0000:a0: root bus resource [io  0x6000-0x6fff window]
> [    4.854848] pci_bus 0000:a0: root bus resource [mem 0x90000000-0x9cffffff window]
> [    4.854848] pci_bus 0000:a0: root bus resource [mem 0x71d60000000-0x7fcffffffff window]
> [    4.854848] pci_bus 0000:a0: root bus resource [bus a0-bf]
> 
> > Does igb seem sensitive about this exact address on a variety of
> > machines?  If so I would expect some kind of igb hardware erratum for
> > it.  
> 
> I've not heard anything to that effect.
> 
> But existance of the sanity check itself in the igb driver looks almost 
> like a smoking gun so I don't know what to think of it.
> 
> I'm also inclined to think that the placement approaches out there so far 
> might not have covered that many addresses but used the left edge of the 
> window. So this series, when it often moves resources to right edge of the 
> window, goes to what might not be on well-charted territory.
> 
> > Do other non-igb devices work at that address?  
> 
> Unfortunately there are not other devices underneath the RP so I might not 
> be able to test this.
> 
> > > Add quirk to reshuffle igb resources, use BAR 3 to block the problematic
> > > address.
> > > 
> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > ---
> > > 
> > > I know this is ugly and I don't like it either but do not know better
> > > way to avoid the regression.
> > > 
> > > I've tried with iommu=off and that did not resolve the issue.
> > > 
> > > I also managed to prove igb works with the same resource layout in
> > > another system. So identifying the case should probably be tightened
> > > by matching with more devices than the one used by igb. This is open
> > > to discussion.  
> > 
> > I guess this answers one of my questions above.  
> 
> Yeah.
> 
> But then there's the sanity check in igb which hints otherwise.
> 
> > > ---
> > >  drivers/pci/quirks.c | 56 ++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 56 insertions(+)
> > > 
> > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > > index de9bbccda21f..e6f3e2ab1fd4 100644
> > > --- a/drivers/pci/quirks.c
> > > +++ b/drivers/pci/quirks.c
> > > @@ -6288,6 +6288,62 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1536, rom_bar_overlap_defect);
> > >  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1537, rom_bar_overlap_defect);
> > >  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1538, rom_bar_overlap_defect);
> > >  
> > > +/*
> > > + * The igb driver probe (due to reads returning ~0 unexpected) when BAR 0
> > > + * appears at 0x9c000000. The cause is unknown.  
> > 
> > I suppose this is missing "fails"?  "igb driver probe fails"?  
> 
> Obviously, thanks.
> 

Re: [PATCH 5/5] PCI/quirks: Avoid certain BAR 0 address with igb

[Ilpo Järvinen]

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

On Mon, 28 Sep 2026, David Laight wrote:

> On Mon, 28 Sep 2026 15:20:13 +0300 (EEST)
> Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> 
> > On Thu, 24 Sep 2026, Bjorn Helgaas wrote:
> > 
> > > [+cc Rafael, ACPI resource question]
> > > 
> > > On Wed, Sep 23, 2026 at 04:17:55PM +0300, Ilpo Järvinen wrote:  
> > > > While testing the resource placement changes, my 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
> 
> Is that valid?
> I'm no expect but I wouldn't expect an address range to cross a power of 2 boundary.

Hi,

Bridge windows are defined by a base/limit pair, both with 1M granularity. 
There's nothing that disallows a bridge window crossing a power of 2 
boundary in the spec (AFAIK).

Note that after this quirk is applied reshuffling the device BAR 
resources, the same crossing is still there and the device works.

Perhaps you mixed this up with BAR resources that indeed must to be 
naturally aligned.

-- 
 i.


> David
> 
> > > >   +    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
> > > > 
> > > > Apparently, the igb driver bails out, after its initial sanity check
> > > > detects an unexpected ~0 read. Hacking around the sanity check just
> > > > results in more failures down the road so the sanity check itself is not
> > > > the cause for the failure.
> > > > 
> > > > The resource placement looks valid so the actual placement patches seem
> > > > to work normally.
> > > > 
> > > > All other possible 1M address I could test (with a hack patch) did work.  
> > > 
> > > Super weird.  Is it possible there's some other device there?  It's
> > > conceivable ACPI might have a _CRS method describing it.  I think
> > > there are ACPI devices for which we don't reserve space mentioned in
> > > _CRS.  Maybe Rafael knows a debug option to log everything in _CRS?  
> > 
> > Now that you mentioned it, there certainly something going on with 
> > that address:
> > 
> > [    0.000000] BIOS-e820: [mem 0x0000000070000000-0x000000008fffffff]  device reserved
> > [    0.000000] BIOS-e820: [gap 0x0000000090000000-0x000000009bffffff]
> > [    0.000000] BIOS-e820: [mem 0x000000009c000000-0x000000009cffffff]  device reserved
> > [    0.000000] BIOS-e820: [gap 0x000000009d000000-0x00000000a8ffffff]
> > [    0.000000] BIOS-e820: [mem 0x00000000a9000000-0x00000000a9ffffff]  device reserved
> > ...
> > [    0.000000] efi: Remove mem48: MMIO range=[0x80000000-0x8fffffff] (256MB) from e820 map
> > [    0.000000] e820: remove [mem 0x80000000-0x8fffffff] device reserved
> > [    0.000000] efi: Remove mem49: MMIO range=[0x9c000000-0x9cffffff] (16MB) from e820 map
> > [    0.000000] e820: remove [mem 0x9c000000-0x9cffffff] device reserved
> > [    0.000000] efi: Remove mem50: MMIO range=[0xa9000000-0xa9ffffff] (16MB) from e820 map
> > [    0.000000] e820: remove [mem 0xa9000000-0xa9ffffff] device reserved
> > 
> > But given the comment above efi_remove_e820_mmio() it sounds like this is 
> > a red herring.
> > 
> > In any case, the address is inside the provided root bus resource:
> > 
> > [    4.854840] ACPI: PCI Root Bridge [PC05] (domain 0000 [bus a0-bf])
> > ...
> > [    4.854848] PCI host bridge to bus 0000:a0
> > [    4.854848] pci_bus 0000:a0: root bus resource [io  0x6000-0x6fff window]
> > [    4.854848] pci_bus 0000:a0: root bus resource [mem 0x90000000-0x9cffffff window]
> > [    4.854848] pci_bus 0000:a0: root bus resource [mem 0x71d60000000-0x7fcffffffff window]
> > [    4.854848] pci_bus 0000:a0: root bus resource [bus a0-bf]
> > 
> > > Does igb seem sensitive about this exact address on a variety of
> > > machines?  If so I would expect some kind of igb hardware erratum for
> > > it.  
> > 
> > I've not heard anything to that effect.
> > 
> > But existance of the sanity check itself in the igb driver looks almost 
> > like a smoking gun so I don't know what to think of it.
> > 
> > I'm also inclined to think that the placement approaches out there so far 
> > might not have covered that many addresses but used the left edge of the 
> > window. So this series, when it often moves resources to right edge of the 
> > window, goes to what might not be on well-charted territory.
> > 
> > > Do other non-igb devices work at that address?  
> > 
> > Unfortunately there are not other devices underneath the RP so I might not 
> > be able to test this.
> > 
> > > > Add quirk to reshuffle igb resources, use BAR 3 to block the problematic
> > > > address.
> > > > 
> > > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > > ---
> > > > 
> > > > I know this is ugly and I don't like it either but do not know better
> > > > way to avoid the regression.
> > > > 
> > > > I've tried with iommu=off and that did not resolve the issue.
> > > > 
> > > > I also managed to prove igb works with the same resource layout in
> > > > another system. So identifying the case should probably be tightened
> > > > by matching with more devices than the one used by igb. This is open
> > > > to discussion.  
> > > 
> > > I guess this answers one of my questions above.  
> > 
> > Yeah.
> > 
> > But then there's the sanity check in igb which hints otherwise.
> > 
> > > > ---
> > > >  drivers/pci/quirks.c | 56 ++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 56 insertions(+)
> > > > 
> > > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > > > index de9bbccda21f..e6f3e2ab1fd4 100644
> > > > --- a/drivers/pci/quirks.c
> > > > +++ b/drivers/pci/quirks.c
> > > > @@ -6288,6 +6288,62 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1536, rom_bar_overlap_defect);
> > > >  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1537, rom_bar_overlap_defect);
> > > >  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1538, rom_bar_overlap_defect);
> > > >  
> > > > +/*
> > > > + * The igb driver probe (due to reads returning ~0 unexpected) when BAR 0
> > > > + * appears at 0x9c000000. The cause is unknown.  
> > > 
> > > I suppose this is missing "fails"?  "igb driver probe fails"?  
> > 
> > Obviously, thanks.
> > 
> 

Re: [PATCH 5/5] PCI/quirks: Avoid certain BAR 0 address with igb

[David Laight]

On Mon, 28 Sep 2026 17:30:41 +0300 (EEST)
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> On Mon, 28 Sep 2026, David Laight wrote:
> 
> > On Mon, 28 Sep 2026 15:20:13 +0300 (EEST)
> > Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> >   
> > > On Thu, 24 Sep 2026, Bjorn Helgaas wrote:
> > >   
> > > > [+cc Rafael, ACPI resource question]
> > > > 
> > > > On Wed, Sep 23, 2026 at 04:17:55PM +0300, Ilpo Järvinen wrote:    
> > > > > While testing the resource placement changes, my 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  
> > 
> > Is that valid?
> > I'm no expect but I wouldn't expect an address range to cross a power of 2 boundary.  
> 
> Hi,
> 
> Bridge windows are defined by a base/limit pair, both with 1M granularity. 
> There's nothing that disallows a bridge window crossing a power of 2 
> boundary in the spec (AFAIK).
> 
> Note that after this quirk is applied reshuffling the device BAR 
> resources, the same crossing is still there and the device works.
> 
> Perhaps you mixed this up with BAR resources that indeed must to be 
> naturally aligned.
> 

I did say I wasn't an expert :-)
The PCIe book I've got is just over 20 years old (only 1000 pages).
I know rather more about the TLPs - we had a problem with some fpga logic
that didn't like getting two completions for a single read.

But it would be consistent with something 'not liking' BAR0 being at
9bxxxxxx and BAR1 at 9cxxxxxx.

David

Re: [PATCH 0/5] PCI: Resource placement algorithm fixes

[Nikolas Joshua Britton]

On Wed, 23 Sep 2026, Ilpo Järvinen wrote:
> 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).

Hi Ilpo,

Thanks, the series fixes the Mac Pro 7,1 case from my report. That is
two Radeon Pro Vega II Duo cards, each with two GPU dies behind one root
port, and BAR0 resized to 32 GB by setting the ReBAR control and
rescanning the root port's bus (booted with pci=realloc).

I tested all five patches on top of v7.2.8, where they apply without
fuzz, against plain v7.2.8 built with the same config.

With plain v7.2.8 it fails as in the report: the root port window is
64G+4M, and the second die gets no BAR:

  pci 0000:06:00.0: bridge window [mem 0x90000000000-0x910003fffff 64bit pref]: assigned
  pci 0000:0b:00.0: BAR 0 [mem 0x90000000000-0x907ffffffff 64bit pref]: assigned
  pci 0000:0e:00.0: BAR 0 [mem size 0x800000000 64bit pref]: can't assign; no space

With the series, all four dies get their 32 GB BAR0, amdgpu binds all
four, and they form one XGMI hive. Each root port window is now 96G
(my one-liner gave 128G). The second sub-bridge window starts with the
die's 2M BAR2 at its left edge, and the two nested bridges below it
(0c:00.0, 0d:00.0) carry the same range:

  pci 0000:06:00.0: bridge window [mem 0x9e800000000-0x9ffffffffff 64bit pref]: assigned
  pci 0000:08:08.0: bridge window [mem 0x9e800000000-0x9f0001fffff 64bit pref]: assigned
  pci 0000:08:10.0: bridge window [mem 0x9f7ffe00000-0x9ffffffffff 64bit pref]: assigned
  pci 0000:0b:00.0: BAR 0 [mem 0x9e800000000-0x9efffffffff 64bit pref]: assigned
  pci 0000:0e:00.0: BAR 2 [mem 0x9f7ffe00000-0x9f7ffffffff 64bit pref]: assigned
  pci 0000:0e:00.0: BAR 0 [mem 0x9f800000000-0x9ffffffffff 64bit pref]: assigned

The second card (root port 16:00.0) is laid out the same way. No other
device lost a resource: the only "can't assign" messages left are for
the same I/O windows that fail on every kernel on this machine.

I used v7.2.8 rather than your v7.3-rc1 base because v7.3-rc4 powers
this machine off during boot, with or without the series. I haven't
looked into that yet.

Tested-by: Nikolas Joshua Britton <nbritton@exabit.io>

Re: [PATCH 0/5] PCI: Resource placement algorithm fixes

[Ilpo Järvinen]

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

On Sat, 26 Sep 2026, Nikolas Joshua Britton wrote:

> On Wed, 23 Sep 2026, Ilpo Järvinen wrote:
> > 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).
> 
> Hi Ilpo,
> 
> Thanks, the series fixes the Mac Pro 7,1 case from my report. That is
> two Radeon Pro Vega II Duo cards, each with two GPU dies behind one root
> port, and BAR0 resized to 32 GB by setting the ReBAR control and
> rescanning the root port's bus (booted with pci=realloc).
> 
> I tested all five patches on top of v7.2.8, where they apply without
> fuzz, against plain v7.2.8 built with the same config.
> 
> With plain v7.2.8 it fails as in the report: the root port window is
> 64G+4M, and the second die gets no BAR:
> 
>   pci 0000:06:00.0: bridge window [mem 0x90000000000-0x910003fffff 64bit pref]: assigned
>   pci 0000:0b:00.0: BAR 0 [mem 0x90000000000-0x907ffffffff 64bit pref]: assigned
>   pci 0000:0e:00.0: BAR 0 [mem size 0x800000000 64bit pref]: can't assign; no space
> 
> With the series, all four dies get their 32 GB BAR0, amdgpu binds all
> four, and they form one XGMI hive. Each root port window is now 96G
> (my one-liner gave 128G). The second sub-bridge window starts with the
> die's 2M BAR2 at its left edge, and the two nested bridges below it
> (0c:00.0, 0d:00.0) carry the same range:
> 
>   pci 0000:06:00.0: bridge window [mem 0x9e800000000-0x9ffffffffff 64bit pref]: assigned
>   pci 0000:08:08.0: bridge window [mem 0x9e800000000-0x9f0001fffff 64bit pref]: assigned
>   pci 0000:08:10.0: bridge window [mem 0x9f7ffe00000-0x9ffffffffff 64bit pref]: assigned
>   pci 0000:0b:00.0: BAR 0 [mem 0x9e800000000-0x9efffffffff 64bit pref]: assigned
>   pci 0000:0e:00.0: BAR 2 [mem 0x9f7ffe00000-0x9f7ffffffff 64bit pref]: assigned
>   pci 0000:0e:00.0: BAR 0 [mem 0x9f800000000-0x9ffffffffff 64bit pref]: assigned
>
> The second card (root port 16:00.0) is laid out the same way. No other
> device lost a resource: the only "can't assign" messages left are for
> the same I/O windows that fail on every kernel on this machine.

I should probably one day make the io assign fail messages debug level if 
there's no io window at parent. It's just noise for majority of 
systems and there's nothing kernel can do about it (nor can the user).

> I used v7.2.8 rather than your v7.3-rc1 base because v7.3-rc4 powers
> this machine off during boot, with or without the series. I haven't
> looked into that yet.

7.2.8 test should be fine for this.

> Tested-by: Nikolas Joshua Britton <nbritton@exabit.io>

Thanks for testing.

-- 
 i.