[PATCH v5 00/14] PCI: Resource placement algorithm fixes

[PATCH v5 00/14] PCI: Resource placement algorithm fixes

[Ilpo Järvinen]

V4 got quite many comments from sashiko, some of them were a bit
sketchy (bogus scenarios such as IOV BARs on a bridge) but there was
still good feedback hidden under the clearly flawed facade.

There have been major changes so any retesting is appreciated (the
4 candidates patch is largely the same so I kept the Tested-by tags but
everything around has been changing so even that is on the borderline
when it comes to the validity of the existing tags).

I ended up dropping the nested window fix which I had anyway realized
would not be necessary with the current resource code. I had earlier
planned to change the resource side code to only try start addresses
that qualify to size+align requirements and had thus kept the nested
win fix. But it just makes PCI side more complicated requiring extra
tweaks and checks. With that patch dropped, pci_align_resource() can
alone decide the placement making the code much simpler.

Second major change is rewriting the gap calculation, which required
including pbus_size_mem() refactoring that I was planning on doing
anyway.

As with v4, the quirk is not included and one approach should still
be selected to solve the Genoa/Zen4 iomem blackhole but that's not
likely the one I proposed within my series so I left it out.

This series also does not attempt to address cases where user alignment
override does not propagate to window size calculations and thus not
into the align given to resource placement code (reported by sashiko).
These are pre-existing issues and while they should be fixed
eventually, I've no plans on increasing the scope and complexity of
this series that is focused on solving real regressions caused by the
commit 9036bd0efcb6 ("PCI: Align head space better").

For those who haven't read the previous version intros (no changes
from v4 in this):

This series generalizes the 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 placement 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 constraints 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. I've left my quirk out of this version as it seems
the least likely approach to be used to resolve the issue. Some approach
should still be applied but that decision seems largely independent of
this series. The main part of the quirk discussion is here:

  https://lore.kernel.org/linux-pci/20261013130948.6231-9-ilpo.jarvinen@linux.intel.com/

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 planning on making another series to properly honor
pci=resource_alignment and improve locking as I realized the current
approach allows parameter change to race with the fitting and
assignment algorithm.

v5:
- Reordered series, "simple" patches first
- Added patch to skip resource with IORESOURCE_DISABLED
- Added patches to fix __ffs*() (__ffs64 & zero handling)
- Fix 2 more 1UL -> 1ULL, renamed roundup/down funcs to _u64
- Rework considering user/arch min align, always take it into
  account during placement if present (sashiko)
- Rename pci_init_pci_resreq_data() -> pci_init_resreq_data() and make
  it non-inline func
- Drop nested window fix (unnecessary and w/o complexity is less)
- Rename .user_align -> .min_align
- Use struct to store candidate selection parameters
- Rework candidate selection precedence
- Calculate odd-multiple starting "2 blocks resource" using both blocks
- Add a tie breaker to candidate selection using alignment of the left
  edge
- Do not use candidate logic if there's no full block or
  rr->min_align > align (the latter is due to pre-existing problems
  elsewhere).
- If no candidate is possible, return right edge of the empty res
  (not strictly necessary because one of the candidates is at res->start
   and the caller should use large enough align if user align is present
   but it doesn't hurt either)
- Refactor pbus_size_mem() to support the new gap calculation
- Reworked gap calculation entirely
- Added debug print to sizing function
- Fix many grammar errors / typos / consistency improvements

v4:
- Drop my own quirk patch (one approach should still be picked!)
- Use 1ULL to not lose bits in 32-bit case (sashiko)
- Copy avail flags from full_avail.flags (sashiko)
- Always return "resize" from pci_specified_resource_alignment() (sashiko)

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 (14):
  resource: Mark free space assigned
  parisc/PCI: Clean up resource debug print & use dynamic debug
  PCI: Don't size or assign disabled BARs
  PCI: Use __ffs64() for resource_size_t typed align
  PCI: Prevent zero align from reaching __ffs64()
  linux/log2.h: Add 64-bit roundup/down_pow_of_two_u64() variants
  PCI: Derive aligns[] array size in a define
  PCI: Honor alignment overrides
  PCI: Place resources to either edge of the window
  PCI: Separate align/raw size calculation from pbus_size_mem()
  PCI: Calculate bridge window with optional sizes from scratch
  PCI: Move size/align calculation fully into __pbus_size_mem()
  PCI: Fix composite resource sizing
  PCI: Add debug printing to window sizing

 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/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                |   4 +-
 drivers/pci/pci.h                |   6 +
 drivers/pci/setup-bus.c          | 263 ++++++++++++++++++++-----------
 drivers/pci/setup-res.c          | 246 ++++++++++++++++++++++++++---
 include/linux/log2.h             |  52 ++++++
 include/linux/pci.h              |  15 +-
 kernel/resource.c                |   3 +-
 19 files changed, 510 insertions(+), 151 deletions(-)


base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
-- 
2.47.3

[PATCH v5 01/14] 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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/resource.c b/kernel/resource.c
index 0c315c917fbb..eb38074ce5da 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.
@@ -754,7 +755,7 @@ static int __find_resource_space(struct resource *root, struct resource *old,
 		/* Check for overflow after ALIGN() */
 		avail.start = ALIGN(full_avail.start, constraint->align);
 		avail.end = full_avail.end;
-		avail.flags = new->flags;
+		avail.flags = full_avail.flags;
 		if (avail.start >= full_avail.start) {
 			alloc.flags = avail.flags;
 			if (alignf) {
-- 
2.47.3

[PATCH v5 02/14] parisc/PCI: Clean up resource debug print & use dynamic debug

[Ilpo Järvinen]

Remove ad hoc DBG_RES() and use pci_dbg() instead.

Remove printing parent resource as the passed resource is a temporary
one 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 nonsensical 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
a more versatile way to debug things than the previous compile time
decision which could only be enabled by changing the source code.

Acked-by: Helge Deller <deller@gmx.de>
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

[PATCH v5 03/14] PCI: Don't size or assign disabled BARs

[Ilpo Järvinen]

When a 64-bit BAR is larger than 4GB but pci_bus_addr_t or
resource_size_t is 32-bit, __pci_read_base() marks the BAR with
IORESOURCE_UNSET | IORESOURCE_DISABLED and sets its size to zero while
retaining the type flags. pdev_resource_assignable() only excludes
disabled bridge windows so such a BAR is still included in resource
sizing.

pbus_size_mem() then calls __ffs() on its zero alignment, which is
undefined behavior. pdev_sort_resources() catches the zero alignment but
logs a bogus "alignment must not be zero" warning.

Exclude all IORESOURCE_DISABLED resources in pdev_resource_assignable().

A disabled expansion ROM is still assigned because ROM disabling is
tracked with IORESOURCE_ROM_ENABLE, not with IORESOURCE_DISABLED.

Fixes: 23b13bc76f35 ("PCI: Fail safely if we can't handle BARs larger than 4GB")
Cc: stable@vger.kernel.org
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/setup-bus.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index ed16ef7c26fa..0c27cec83c0d 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -301,20 +301,18 @@ static bool pdev_resources_assignable(struct pci_dev *dev)
 	return true;
 }
 
-static bool pdev_resource_assignable(struct pci_dev *dev, struct resource *res)
+static bool pdev_resource_assignable(struct resource *res)
 {
-	int idx = pci_resource_num(dev, res);
-
 	if (!res->flags)
 		return false;
 
-	if (pci_resource_is_bridge_win(idx) && res->flags & IORESOURCE_DISABLED)
+	if (res->flags & IORESOURCE_DISABLED)
 		return false;
 
 	return true;
 }
 
-static bool pdev_resource_should_fit(struct pci_dev *dev, struct resource *res)
+static bool pdev_resource_should_fit(struct resource *res)
 {
 	if (resource_assigned(res))
 		return false;
@@ -322,7 +320,7 @@ static bool pdev_resource_should_fit(struct pci_dev *dev, struct resource *res)
 	if (res->flags & IORESOURCE_PCI_FIXED)
 		return false;
 
-	return pdev_resource_assignable(dev, res);
+	return pdev_resource_assignable(res);
 }
 
 /* Sort resources by alignment */
@@ -340,7 +338,7 @@ static void pdev_sort_resources(struct pci_dev *dev, struct list_head *head)
 		resource_size_t r_align;
 		struct list_head *n;
 
-		if (!pdev_resource_should_fit(dev, r))
+		if (!pdev_resource_should_fit(r))
 			continue;
 
 		r_align = pci_resource_alignment(dev, r);
@@ -1094,7 +1092,7 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t add_size,
 			if (resource_assigned(r) || !(r->flags & IORESOURCE_IO))
 				continue;
 
-			if (!pdev_resource_assignable(dev, r))
+			if (!pdev_resource_assignable(r))
 				continue;
 
 			r_size = resource_size(r);
@@ -1305,7 +1303,7 @@ static void pbus_size_mem(struct pci_bus *bus, struct resource *b_res,
 			resource_size_t r_size;
 
 			if (!pdev_resources_assignable(dev) ||
-			    !pdev_resource_should_fit(dev, r))
+			    !pdev_resource_should_fit(r))
 				continue;
 			if (b_res != pbus_select_window(bus, r))
 				continue;
-- 
2.47.3

[PATCH v5 04/14] PCI: Use __ffs64() for resource_size_t typed align

[Ilpo Järvinen]

__ffs() inputs unsigned long. With some 32-bit configurations
resource_size_t can be 64 bits wide while unsigned long is only 32 bits
wide.

To prevent value truncation, convert __ffs() to __ffs64() when the
input is resource_size_t.

In practice >=4GB resource align is required for the problem to
manifest itself which might not be present on systems running 32-bit
kernels so the problem has not been noticed.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/setup-bus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 0c27cec83c0d..f4358322f766 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1315,7 +1315,7 @@ static void pbus_size_mem(struct pci_bus *bus, struct resource *b_res,
 			 * keep "order" from being negative for smaller
 			 * resources.
 			 */
-			order = max_t(int, __ffs(align) - __ffs(SZ_1M), 0);
+			order = max_t(int, __ffs64(align) - __ffs(SZ_1M), 0);
 			if (order >= ARRAY_SIZE(aligns)) {
 				pci_warn(dev, "%s %pR: disabling; bad alignment %#llx\n",
 					 r_name, r, (unsigned long long) align);
-- 
2.47.3

[PATCH v5 05/14] PCI: Prevent zero align from reaching __ffs64()

[Ilpo Järvinen]

The bridge window sizing algorithm calculates order from alignment
using __ffs64(). __ffs*() is undefined for zero input.

pci_resource_alignment() & resource_alignment() have a number potential
ways ways to return 0, in fact, resource_alignment() intentionally does
that if neither SIZEALIGN nor STARTALIGN is set.

Thus, ensure zero align is always properly dealt with in
pbus_size_mem(). pdev_sort_resources() already has a similar check for
values returned from pci_resource_alignment() (though it is not using
__ffs*()).

Cc: stable@vger.kernel.org
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/setup-bus.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index f4358322f766..9d167e7773a1 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1309,6 +1309,8 @@ static void pbus_size_mem(struct pci_bus *bus, struct resource *b_res,
 				continue;
 
 			align = pci_resource_alignment(dev, r);
+			if (!align)
+				goto bad_alignment;
 			/*
 			 * aligns[0] is for 1MB (since bridge memory
 			 * windows are always at least 1MB aligned), so
@@ -1317,6 +1319,7 @@ static void pbus_size_mem(struct pci_bus *bus, struct resource *b_res,
 			 */
 			order = max_t(int, __ffs64(align) - __ffs(SZ_1M), 0);
 			if (order >= ARRAY_SIZE(aligns)) {
+bad_alignment:
 				pci_warn(dev, "%s %pR: disabling; bad alignment %#llx\n",
 					 r_name, r, (unsigned long long) align);
 				r->flags = 0;
-- 
2.47.3

[PATCH v5 06/14] linux/log2.h: Add 64-bit roundup/down_pow_of_two_u64() variants

[Ilpo Järvinen]

Generic sounding roundup_pow_of_two() and rounddown_pow_of_two() are
internally based on fls_long() 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 the same as the input
but it requires auditing all the existing callers that have mixed
input/output types.

As a stopgap solution, add roundup/down_pow_of_two_u64() 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..87fd91be2075 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_two_u64() - round up to nearest power of two
+ * @n: value to round up
+ */
+static inline __attribute__((const))
+u64 __roundup_pow_of_two_u64(u64 n)
+{
+	return 1ULL << 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_two_u64() - round down to nearest power of two
+ * @n: value to round down
+ */
+static inline __attribute__((const))
+u64 __rounddown_pow_of_two_u64(u64 n)
+{
+	return 1ULL << (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_two_u64 - 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_two_u64(n)			\
+(						\
+	__builtin_constant_p(n) ? (		\
+		((n) == 1) ? 1 :		\
+		(1ULL << (ilog2((n) - 1) + 1))	\
+				   ) :		\
+	__roundup_pow_of_two_u64(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_two_u64 - 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_two_u64(n)		\
+(						\
+	__builtin_constant_p(n) ? (		\
+		(1ULL << ilog2(n))) :		\
+	__rounddown_pow_of_two_u64(n)		\
+ )
+
 /**
  * rounddown_pow_of_two - round the given value down to nearest power of two
  * @n: parameter
-- 
2.47.3

[PATCH v5 07/14] PCI: Derive aligns[] array size in a define

[Ilpo Järvinen]

The aligns[] array in pbus_size_mem() is sized with a literal with a
comment explaining the number. Add a define PCI_RES_ALIGN_RANGE that
derives the value from those sizes.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/setup-bus.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 9d167e7773a1..c3c229ed320a 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -19,6 +19,7 @@
 #include <linux/bug.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
+#include <linux/log2.h>
 #include <linux/minmax.h>
 #include <linux/module.h>
 #include <linux/pci.h>
@@ -1256,6 +1257,10 @@ static bool pbus_size_mem_optional(struct pci_dev *dev, int resno,
 	return true;
 }
 
+/* Alignments from 1MB to 128TB */
+#define PCI_RES_MAX_ALIGN	SZ_128T
+#define PCI_RES_ALIGN_RANGE	(ilog2(PCI_RES_MAX_ALIGN) - ilog2(SZ_1M) + 1)
+
 /**
  * pbus_size_mem() - Size the memory window of a given bus
  *
@@ -1279,7 +1284,7 @@ static void pbus_size_mem(struct pci_bus *bus, struct resource *b_res,
 {
 	struct pci_dev *dev;
 	resource_size_t min_align, win_align, align, size, size0, size1 = 0;
-	resource_size_t aligns[28] = {}; /* Alignments from 1MB to 128TB */
+	resource_size_t aligns[PCI_RES_ALIGN_RANGE] = {};
 	int order, max_order;
 	resource_size_t children_add_size = 0;
 	resource_size_t add_align = 0;
-- 
2.47.3

[PATCH v5 08/14] 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.

PowerNV will not be able to use the current remainder logic after this
change because on it pcibios_default_alignment() always results in
setting min_align but this code is in a transient state and the problem
will be fixed by an upcoming change that rewrites the resource
placement algorithm with a more generic approach.

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/
Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---

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                |  4 ++--
 drivers/pci/pci.h                |  2 ++
 drivers/pci/setup-res.c          | 33 ++++++++++++++++++++++++++------
 include/linux/pci.h              | 15 +++++++++++++--
 16 files changed, 79 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..06990fad8e48 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_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..f11ed5509095 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_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 7c6f2aba8144..8c40f86d75ee 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -6507,8 +6507,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();
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 77efa4c5c960..e6e2afcceb33 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -515,6 +515,8 @@ void devm_pci_alloc_host_bridge_release(void *data);
 struct device *pci_get_host_bridge_device(struct pci_dev *dev);
 void pci_put_host_bridge_device(struct device *dev);
 
+resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
+						 bool *resize);
 void pci_resize_resource_set_size(struct pci_dev *dev, int resno, int size);
 int pci_do_resource_release_and_resize(struct pci_dev *dev, int resno, int size,
 				       int exclude_bars);
diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index 376f09630a4a..9c55b9f9462c 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -248,6 +248,20 @@ static int pci_revert_fw_address(struct resource *res, struct pci_dev *dev,
 	return 0;
 }
 
+/*
+ * TODO: Improve PCI resource assignment interface so that callers outside PCI
+ * core do not need to know about this detail.
+ */
+void pci_init_resreq_data(struct pci_resreq_data *rr, struct pci_dev *dev)
+{
+	bool dummy;
+
+	memset(rr, 0, sizeof(*rr));
+	rr->dev = dev;
+	rr->min_align = pci_specified_resource_alignment(dev, &dummy);
+}
+EXPORT_SYMBOL_GPL(pci_init_resreq_data);
+
 resource_size_t pci_resource_alignment(const struct pci_dev *dev,
 				       const struct resource *res)
 {
@@ -265,17 +279,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->min_align)
+		return res->start;
+
 	if (IS_ALIGNED(size, align))
 		return res->start;
 
@@ -306,18 +324,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_resreq_data(&rr, dev);
+
 	min = (res->flags & IORESOURCE_IO) ? PCIBIOS_MIN_IO : PCIBIOS_MIN_MEM;
 
 	/*
@@ -329,7 +350,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 +362,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 +375,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 aa3765a20a96..c945f5753e7c 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1221,12 +1221,23 @@ 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 */
+/**
+ * struct pci_resreq_data - Data passed to pcibios_align_resource()
+ * @dev: PCI device
+ * @min_align: Minimum alignment bound (from pci=resource_alignment or arch)
+ */
+struct pci_resreq_data {
+	struct pci_dev *dev;
+	resource_size_t min_align;
+};
+
+void pci_init_resreq_data(struct pci_resreq_data *rr, struct pci_dev *dev);
+
 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 v5 09/14] PCI: Place resources to either edge of the window

[Ilpo Järvinen]

PCI resource assignment phase is 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 resources towards the left edge of the
window. Tha commit 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 GPU's PF BAR and VF BARs). The resource
with the 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 contiguous free space that is no longer
there because the large resource is now in the middle.

In this log excerpt, 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 than 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
set up 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 selection algorithm
avoids consuming space from the window edge that can fit largest
aligning resources in the future, whenever possible. This approach has
certain level of preference towards the right edge to keep the largest
alignment available (which is usually at the left edge), which
unfortunately may uncover platform issues.

The candidate selection compares the properties of the remaining free
space using cmp_free_candidate() that compares (in decreasing order of
priority):

 1. the largest contiguous free space,
 2. the largest naturally aligning power-of-two-sized address range
    within,
 3. the largest alignment of the start address for the naturally
    aligning free space range (from check 2).
 4. the largest alignment of the start address of the free space.

This precedence is a balancing act. In cases where not all resources
fit, having check 2 before 1 could be helpful for getting optional
resources assigned (when retrying assignments with mandatory resources
first and then assigning optional resources on best-effort basis). It
may be useful to revisit the order or add logic to pick the precedence
on a case-by-case basis once we get large scale experience and problem
reports.

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 <eric.auger@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 sent logs after that). Thus no links.
---
 drivers/pci/pci.h       |   4 +
 drivers/pci/setup-bus.c |   4 -
 drivers/pci/setup-res.c | 219 ++++++++++++++++++++++++++++++++++++----
 3 files changed, 204 insertions(+), 23 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index e6e2afcceb33..31ffae9ce557 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -115,6 +115,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 c3c229ed320a..5f8320cac504 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -32,10 +32,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 9c55b9f9462c..ea256eb806a5 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -13,8 +13,12 @@
  *	     Resource sorting
  */
 
+#include <linux/align.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>
@@ -274,10 +278,112 @@ 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 powof2, natural_start, natural_end;
+	resource_size_t size = resource_size(res);
+
+	*max_align = 1;
+	if (!size)
+		return 0;
+
+	powof2 = rounddown_pow_of_two_u64(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);
+		/*
+		 * Corner case: with start at an odd alignment multiple, there
+		 * are two powof2 sized blocks.
+		 */
+		natural_start = ALIGN(res->start, powof2);
+		natural_end = ALIGN_DOWN(res->end + 1 - powof2, powof2);
+		*max_align <<= max(natural_start ? __ffs64(natural_start) : 0,
+				   natural_end ? __ffs64(natural_end) : 0);
+
+	} else if (natural_start) {
+		*max_align <<= __ffs64(natural_start);
+	}
+
+	/* Normal PCI resources cannot have align 1. */
+	if (*max_align == 1) {
+		/*
+		 * 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;
+}
+
+struct free_candidate_info {
+	resource_size_t size;
+	resource_size_t natural_size;
+	resource_size_t max_align;
+	resource_size_t left_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.
+ * 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 contiguous free space,
+ *   2. the largest naturally aligning power-of-two-sized address range within,
+ *   3. the largest alignment of the start address for the naturally aligning
+ *      free space range (from check 2),
+ *   4. the largest alignment of the start address of the free space.
+ */
+static int cmp_free_candidate(const struct free_candidate_info *a,
+			      const struct free_candidate_info *b)
+{
+	if (a->size != b->size)
+		return a->size < b->size ? -1 : 1;
+	if (a->natural_size != b->natural_size)
+		return a->natural_size < b->natural_size ? -1 : 1;
+	if (a->max_align != b->max_align)
+		return a->max_align < b->max_align ? -1 : 1;
+	if (a->left_align != b->left_align)
+		return a->left_align < b->left_align ? -1 : 1;
+
+	return 0;
+}
+
+/**
+ * pci_align_resource - Places resource into empty space range
+ * @rr: PCI resource request data
+ * @res: Candidate range calculated by caller (not among @rr->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.
+ */
+/*
+ * Following candidate logic only applies to mem resources currently. For
+ * composite resources not divisible by @align, @align does not apply to
+ * non-aligning remainder part giving some leeway for its placement. There
+ * are 4 candidate positions:
+ *
+ *           W0WWW0WWW0WWW
+ * 0.         AAAAr
+ * 1.        rAAAA
+ * 2.             AAAAr
+ * 3.                rAAAA
+ *
+ * W = bridge window
+ * 0 = bridge window offset matching align
+ * A = aligning part of size (size rounded down by align)
+ * r = non-aligning remainder
+ *
+ * Candidates 0 & 2 and 1 & 3 may degenerate to the same address range.
+ *
+ * Select resource placement based on the remaining free space. Candidates 0
+ * & 1 check only right edge free space and 2 & 3 the left edge.
  */
 resource_size_t pci_align_resource(struct pci_resreq_data *rr,
 				   const struct resource *res,
@@ -286,31 +392,106 @@ 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;
+	struct free_candidate_info best_info = {};
+	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->min_align)
-		return res->start;
-
-	if (IS_ALIGNED(size, align))
+	/*
+	 * Not using large enough align when minimum alignment is in use is
+	 * a caller-side problem which likely caused also mis-sizing the
+	 * corresponding window so do not attempt to solve it here (nor
+	 * possibly make the problem worse by applying remainder relocation
+	 * below).
+	 *
+	 * Practical impact of this is expected to be around zero (or the
+	 * callers would have been already fixed).
+	 *
+	 * TODO: Fix all callers and remove this.
+	 */
+	if (rr->min_align > align) {
+		pci_dbg(dev, "%pR: minimum alignment exceeds align (%llx > %llx)\n",
+			res,
+			(unsigned long long)rr->min_align,
+			(unsigned long long)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;
-	/* 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)
+	aligning = size - remainder;
+	if (!aligning)
 		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;
+	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 free_candidate_info info;
+		struct resource remaining;
+
+		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;
+		}
+
+		if (rr->min_align &&
+		    !IS_ALIGNED(candidate[i].start, rr->min_align)) {
+			pci_dbg(dev, "%pR: candidate %u skipped due to alignment lower bound %llx\n",
+				&candidate[i], i,
+				(unsigned long long)rr->min_align);
+			continue;
+		}
+
+		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;
+		}
+
+		info.size = resource_size(&remaining);
+		info.natural_size = pci_max_natural_size(&remaining,
+							 &info.max_align);
+		info.left_align = remaining.start ?
+				  (1ULL << __ffs64(remaining.start)) :
+				  RESOURCE_SIZE_MAX;
+		pci_dbg(dev, "%pR: candidate %u free space size=%llx natural_size=%llx max_align=%llx left_align=%llx\n",
+			&candidate[i], i,
+			(unsigned long long)info.size,
+			(unsigned long long)info.natural_size,
+			(unsigned long long)info.max_align,
+			(unsigned long long)info.left_align);
+		if ((best < 0) || cmp_free_candidate(&info, &best_info) > 0) {
+			best = i;
+			best_info = info;
+		}
+	}
+
+	/* None fits? Return address where size definitely won't fit. */
+	if (best < 0)
+		return empty_res->end;
+
+	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

[PATCH v5 10/14] PCI: Separate align/raw size calculation from pbus_size_mem()

[Ilpo Järvinen]

pbus_size_mem() contains many steps, the first one being calculation
of required size and alignment. Refactor it into a separate function
to be able to reuse the calculation.

Rename size to winsize to better differentiate from r_size (resource
size).

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/setup-bus.c | 94 +++++++++++++++++++++++------------------
 1 file changed, 54 insertions(+), 40 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 5f8320cac504..a8915b2354ea 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1257,51 +1257,27 @@ static bool pbus_size_mem_optional(struct pci_dev *dev, int resno,
 #define PCI_RES_MAX_ALIGN	SZ_128T
 #define PCI_RES_ALIGN_RANGE	(ilog2(PCI_RES_MAX_ALIGN) - ilog2(SZ_1M) + 1)
 
-/**
- * pbus_size_mem() - Size the memory window of a given bus
- *
- * @bus:		The bus
- * @b_res:		The bridge window resource
- * @add_size:		Additional memory window
- * @realloc_head:	Track the additional memory window on this list
- *
- * Calculate the size of the bridge window @b_res and minimal alignment
- * which guarantees that all child resources fit in this size.
- *
- * Set the bus resource start/end to indicate the required size if there an
- * available unassigned bus resource of the desired @type.
- *
- * Add optional resource requests to the @realloc_head list if it is
- * supplied.
- */
-static void pbus_size_mem(struct pci_bus *bus, struct resource *b_res,
-			  resource_size_t add_size,
-			  struct list_head *realloc_head)
+static resource_size_t __pbus_size_mem(struct pci_bus *bus,
+				       struct resource *b_res,
+				       resource_size_t *head_align,
+				       resource_size_t *add_align,
+				       resource_size_t *children_add_size,
+				       struct list_head *realloc_head)
 {
 	struct pci_dev *dev;
-	resource_size_t min_align, win_align, align, size, size0, size1 = 0;
+	resource_size_t winsize = 0, win_align;
 	resource_size_t aligns[PCI_RES_ALIGN_RANGE] = {};
-	int order, max_order;
-	resource_size_t children_add_size = 0;
-	resource_size_t add_align = 0;
-
-	if (!b_res)
-		return;
-
-	/* If resource is already assigned, nothing more to do */
-	if (resource_assigned(b_res))
-		return;
+	int max_order = 0;
 
-	max_order = 0;
-	size = 0;
+	win_align = pci_min_window_alignment(bus, b_res->flags);
 
 	list_for_each_entry(dev, &bus->devices, bus_list) {
 		struct resource *r;
-		int i;
+		int order, i;
 
 		pci_dev_for_each_resource(dev, r, i) {
 			const char *r_name = pci_resource_name(dev, i);
-			resource_size_t r_size;
+			resource_size_t r_size, align;
 
 			if (!pdev_resources_assignable(dev) ||
 			    !pdev_resource_should_fit(r))
@@ -1328,12 +1304,12 @@ static void pbus_size_mem(struct pci_bus *bus, struct resource *b_res,
 			}
 
 			if (pbus_size_mem_optional(dev, i, align,
-						   realloc_head, &add_align,
-						   &children_add_size))
+						   realloc_head, add_align,
+						   children_add_size))
 				continue;
 
 			r_size = resource_size(r);
-			size += max(r_size, align);
+			winsize += max(r_size, align);
 
 			/*
 			 * If resource's size is larger than its alignment,
@@ -1348,9 +1324,47 @@ static void pbus_size_mem(struct pci_bus *bus, struct resource *b_res,
 		}
 	}
 
+	*head_align = calculate_head_align(aligns, max_order);
+	*head_align = max(*head_align, win_align);
+
+	return winsize;
+}
+
+/**
+ * pbus_size_mem() - Size the memory window of a given bus
+ *
+ * @bus:		The bus
+ * @b_res:		The bridge window resource
+ * @add_size:		Additional memory window
+ * @realloc_head:	Track the additional memory window on this list
+ *
+ * Calculate the size of the bridge window @b_res and minimal alignment
+ * which guarantees that all child resources fit in this size.
+ *
+ * Set the bus resource start/end to indicate the required size if there an
+ * available unassigned bus resource of the desired @type.
+ *
+ * Add optional resource requests to the @realloc_head list if it is
+ * supplied.
+ */
+static void pbus_size_mem(struct pci_bus *bus, struct resource *b_res,
+			  resource_size_t add_size,
+			  struct list_head *realloc_head)
+{
+	resource_size_t min_align, win_align, size, size0, size1 = 0;
+	resource_size_t children_add_size = 0;
+	resource_size_t add_align = 0;
+
+	if (!b_res)
+		return;
+
+	/* If resource is already assigned, nothing more to do */
+	if (resource_assigned(b_res))
+		return;
+
 	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);
+	size = __pbus_size_mem(bus, b_res, &min_align, &add_align,
+			       &children_add_size, realloc_head);
 	size0 = calculate_memsize(size, realloc_head ? 0 : add_size,
 				  0, win_align);
 
-- 
2.47.3

[PATCH v5 11/14] PCI: Calculate bridge window with optional sizes from scratch

[Ilpo Järvinen]

pbus_size_mem() calls __pbus_size_mem() to calculate minimum head
alignment and size the bridge window. A single pass in
__pbus_size_mem() attempts to calculate both size with only mandatory
resources and also with optional size using add_align and
children_add_size.

The discovered resource sizes are passed to calculate_head_align() but
that only makes one calculation with mandatory resources in mind. That
means the alignment requirement for the case with optional resources is
determined from the maximum alignment, not from the required minimum
alignment.

To make the calculation more consistent, remove add_align and
children_add_size parameters, and make the caller decide if optional
resources should be included or not. Then, pbus_size_mem() can just
make a second call to __pbus_size_mem() to recalculate the alignment
and size with optional sizes.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/setup-bus.c | 76 ++++++++++++++++++++++-------------------
 1 file changed, 40 insertions(+), 36 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index a8915b2354ea..6623adfdde85 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1013,10 +1013,9 @@ static resource_size_t calculate_iosize(resource_size_t size,
 
 static resource_size_t calculate_memsize(resource_size_t size,
 					 resource_size_t min_size,
-					 resource_size_t children_add_size,
 					 resource_size_t align)
 {
-	size = max(size + children_add_size, min_size);
+	size = max(size, min_size);
 	return ALIGN(size, align);
 }
 
@@ -1208,19 +1207,21 @@ static resource_size_t calculate_head_align(resource_size_t *aligns,
  * Account an optional resource or the optional part of the resource in bridge
  * window size.
  *
- * Return: %true if the resource is entirely optional.
+ * Return: %true if the resource is skipped due to optionality.
  */
 static bool pbus_size_mem_optional(struct pci_dev *dev, int resno,
-				   resource_size_t align,
-				   struct list_head *realloc_head,
-				   resource_size_t *add_align,
-				   resource_size_t *children_add_size)
+				   resource_size_t *size,
+				   resource_size_t *align,
+				   bool add_optional,
+				   struct list_head *realloc_head)
 {
 	struct resource *res = pci_resource_n(dev, resno);
 	bool optional = pci_resource_is_optional(dev, resno);
-	resource_size_t r_size = resource_size(res);
 	struct pci_dev_resource *dev_res = NULL;
 
+	if (!add_optional)
+		return optional;
+
 	if (!realloc_head)
 		return false;
 
@@ -1232,8 +1233,8 @@ static bool pbus_size_mem_optional(struct pci_dev *dev, int resno,
 	if (pci_resource_is_bridge_win(resno)) {
 		dev_res = res_to_dev_res(realloc_head, res);
 		if (dev_res) {
-			*children_add_size += dev_res->add_size;
-			*add_align = max(*add_align, dev_res->min_align);
+			*size += dev_res->add_size;
+			*align = max(*align, dev_res->min_align);
 		}
 	}
 
@@ -1246,22 +1247,28 @@ static bool pbus_size_mem_optional(struct pci_dev *dev, int resno,
 	 * on the list.
 	 */
 	if (!dev_res)
-		pci_dev_res_add_to_list(realloc_head, dev, res, 0, align);
-	*children_add_size += r_size;
-	*add_align = max(align, *add_align);
+		pci_dev_res_add_to_list(realloc_head, dev, res, 0, *align);
 
-	return true;
+	return false;
 }
 
 /* Alignments from 1MB to 128TB */
 #define PCI_RES_MAX_ALIGN	SZ_128T
 #define PCI_RES_ALIGN_RANGE	(ilog2(PCI_RES_MAX_ALIGN) - ilog2(SZ_1M) + 1)
 
+/*
+ * Since bridge memory windows are always at least 1MB aligned, keep order
+ * from being negative for smaller resources.
+ */
+static int pci_align_to_order(resource_size_t align)
+{
+	return max_t(int, __ffs64(align) - __ffs(SZ_1M), 0);
+}
+
 static resource_size_t __pbus_size_mem(struct pci_bus *bus,
 				       struct resource *b_res,
 				       resource_size_t *head_align,
-				       resource_size_t *add_align,
-				       resource_size_t *children_add_size,
+				       bool add_optional,
 				       struct list_head *realloc_head)
 {
 	struct pci_dev *dev;
@@ -1288,13 +1295,7 @@ static resource_size_t __pbus_size_mem(struct pci_bus *bus,
 			align = pci_resource_alignment(dev, r);
 			if (!align)
 				goto bad_alignment;
-			/*
-			 * aligns[0] is for 1MB (since bridge memory
-			 * windows are always at least 1MB aligned), so
-			 * keep "order" from being negative for smaller
-			 * resources.
-			 */
-			order = max_t(int, __ffs64(align) - __ffs(SZ_1M), 0);
+			order = pci_align_to_order(align);
 			if (order >= ARRAY_SIZE(aligns)) {
 bad_alignment:
 				pci_warn(dev, "%s %pR: disabling; bad alignment %#llx\n",
@@ -1303,12 +1304,17 @@ static resource_size_t __pbus_size_mem(struct pci_bus *bus,
 				continue;
 			}
 
-			if (pbus_size_mem_optional(dev, i, align,
-						   realloc_head, add_align,
-						   children_add_size))
-				continue;
-
 			r_size = resource_size(r);
+			/*
+			 * This has side-effects on size so it should not be
+			 * called while the resource can still be rejected
+			 * (such as the order check above). As align could
+			 * have changed, the order has to be recalculated.
+			 */
+			if (pbus_size_mem_optional(dev, i, &r_size, &align,
+						   add_optional, realloc_head))
+				continue;
+			order = pci_align_to_order(align);
 			winsize += max(r_size, align);
 
 			/*
@@ -1352,7 +1358,6 @@ static void pbus_size_mem(struct pci_bus *bus, struct resource *b_res,
 			  struct list_head *realloc_head)
 {
 	resource_size_t min_align, win_align, size, size0, size1 = 0;
-	resource_size_t children_add_size = 0;
 	resource_size_t add_align = 0;
 
 	if (!b_res)
@@ -1363,20 +1368,19 @@ static void pbus_size_mem(struct pci_bus *bus, struct resource *b_res,
 		return;
 
 	win_align = pci_min_window_alignment(bus, b_res->flags);
-	size = __pbus_size_mem(bus, b_res, &min_align, &add_align,
-			       &children_add_size, realloc_head);
-	size0 = calculate_memsize(size, realloc_head ? 0 : add_size,
-				  0, win_align);
+	size = __pbus_size_mem(bus, b_res, &min_align, !realloc_head,
+			       realloc_head);
+	size0 = calculate_memsize(size, realloc_head ? 0 : add_size, win_align);
 
 	if (size0) {
 		resource_set_range(b_res, min_align, size0);
 		b_res->flags &= ~IORESOURCE_DISABLED;
 	}
 
-	if (realloc_head && (add_size > 0 || children_add_size > 0)) {
+	if (realloc_head) {
+		size = __pbus_size_mem(bus, b_res, &add_align, true, realloc_head);
 		add_align = max(min_align, add_align);
-		size1 = calculate_memsize(size, add_size, children_add_size,
-					  win_align);
+		size1 = calculate_memsize(size, add_size, win_align);
 	}
 
 	if (!size0 && !size1) {
-- 
2.47.3

[PATCH v5 12/14] PCI: Move size/align calculation fully into __pbus_size_mem()

[Ilpo Järvinen]

Move entire size and alignment calculation into __pbus_size_mem()
further simplifying the caller side code.

Also fold calculate_memsize() into __pbus_size_mem() as it has become
too simple to justify a separate function.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/setup-bus.c | 25 +++++++------------------
 1 file changed, 7 insertions(+), 18 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 6623adfdde85..b6be6cd16006 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1011,14 +1011,6 @@ static resource_size_t calculate_iosize(resource_size_t size,
 	return ALIGN(max(size, old_size), align);
 }
 
-static resource_size_t calculate_memsize(resource_size_t size,
-					 resource_size_t min_size,
-					 resource_size_t align)
-{
-	size = max(size, min_size);
-	return ALIGN(size, align);
-}
-
 resource_size_t __weak pcibios_window_alignment(struct pci_bus *bus,
 						unsigned long type)
 {
@@ -1269,6 +1261,7 @@ static resource_size_t __pbus_size_mem(struct pci_bus *bus,
 				       struct resource *b_res,
 				       resource_size_t *head_align,
 				       bool add_optional,
+				       resource_size_t min_size,
 				       struct list_head *realloc_head)
 {
 	struct pci_dev *dev;
@@ -1333,7 +1326,7 @@ static resource_size_t __pbus_size_mem(struct pci_bus *bus,
 	*head_align = calculate_head_align(aligns, max_order);
 	*head_align = max(*head_align, win_align);
 
-	return winsize;
+	return ALIGN(max(winsize, min_size), win_align);
 }
 
 /**
@@ -1357,8 +1350,7 @@ static void pbus_size_mem(struct pci_bus *bus, struct resource *b_res,
 			  resource_size_t add_size,
 			  struct list_head *realloc_head)
 {
-	resource_size_t min_align, win_align, size, size0, size1 = 0;
-	resource_size_t add_align = 0;
+	resource_size_t min_align, size0, size1 = 0, add_align = 0;
 
 	if (!b_res)
 		return;
@@ -1367,20 +1359,17 @@ static void pbus_size_mem(struct pci_bus *bus, struct resource *b_res,
 	if (resource_assigned(b_res))
 		return;
 
-	win_align = pci_min_window_alignment(bus, b_res->flags);
-	size = __pbus_size_mem(bus, b_res, &min_align, !realloc_head,
-			       realloc_head);
-	size0 = calculate_memsize(size, realloc_head ? 0 : add_size, win_align);
-
+	size0 = __pbus_size_mem(bus, b_res, &min_align, !realloc_head,
+				realloc_head ? 0 : add_size, realloc_head);
 	if (size0) {
 		resource_set_range(b_res, min_align, size0);
 		b_res->flags &= ~IORESOURCE_DISABLED;
 	}
 
 	if (realloc_head) {
-		size = __pbus_size_mem(bus, b_res, &add_align, true, realloc_head);
+		size1 = __pbus_size_mem(bus, b_res, &add_align, true,
+					add_size, realloc_head);
 		add_align = max(min_align, add_align);
-		size1 = calculate_memsize(size, add_size, win_align);
 	}
 
 	if (!size0 && !size1) {
-- 
2.47.3

[PATCH v5 13/14] PCI: Fix composite resource sizing

[Ilpo Järvinen]

The bridge window sizing algorithm aims to pack child resources back to
back. With composite child resources whose sizes are not a power of two,
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.

Add bridge window gap size calculation.

When there are only two resources and at least one of them is
composite, the gap end address aligns to meet the smaller of the two
alignment requirements. If there is only one composite resource, the
smaller alignment may be that of the composite resource.

Other combinations fall back to filling each resource's align sized
blocks fully. 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 it safe.

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

I removed Bjorn's Tested-by tag as this patch has been through a major
rework.

There is one known failure mode (pointed out by AI review) with >= 3
resources, each with a different alignment requirement following a
certain pattern and the smallest align one must also be a composite
resource. Additional requirement is that the window must have an
odd-multiple start address.

While that corner case could be addressed by always forcing larger gaps
(at the cost of everyone paying the extra gap "tax"), I see no reason
to overengineer this for corner cases that might not occur in real
systems. If issues are reported, this can be revisited and perhaps more
complex fallback logic added (to avoid incurring the gap "tax"
unilaterally).

Adding the gap logic fixes known regressions seen in real systems, so
it is an important improvement even as is.
---
 drivers/pci/setup-bus.c | 57 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 55 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index b6be6cd16006..86c9557bc2fb 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1267,6 +1267,9 @@ static resource_size_t __pbus_size_mem(struct pci_bus *bus,
 	struct pci_dev *dev;
 	resource_size_t winsize = 0, win_align;
 	resource_size_t aligns[PCI_RES_ALIGN_RANGE] = {};
+	resource_size_t partial_blocks = 0, max_partial = 0;
+	unsigned int nonaligning = 0, rescount = 0;
+	resource_size_t remainders = 0, gap = 0;
 	int max_order = 0;
 
 	win_align = pci_min_window_alignment(bus, b_res->flags);
@@ -1277,7 +1280,7 @@ static resource_size_t __pbus_size_mem(struct pci_bus *bus,
 
 		pci_dev_for_each_resource(dev, r, i) {
 			const char *r_name = pci_resource_name(dev, i);
-			resource_size_t r_size, align;
+			resource_size_t r_size, align, remainder;
 
 			if (!pdev_resources_assignable(dev) ||
 			    !pdev_resource_should_fit(r))
@@ -1320,13 +1323,63 @@ static resource_size_t __pbus_size_mem(struct pci_bus *bus,
 				aligns[order] += align;
 			if (order > max_order)
 				max_order = order;
+
+			rescount++;
+
+			if (r_size <= align)
+				continue;
+			remainder = r_size - ALIGN_DOWN(r_size, align);
+			if (!remainder)
+				continue;
+
+			nonaligning++;
+			remainders += remainder;
+			partial_blocks += align;
+			max_partial = max(max_partial, align);
 		}
 	}
 
+	if (nonaligning && rescount == 2) {
+		resource_size_t second_align = max_partial;
+
+		/*
+		 * With two resources having differing alignment
+		 * requirements, the gap size can be optimized by aligning it
+		 * only up to the smaller of the alignment requirements.
+		 *
+		 * This also allows two composite remainders to share the
+		 * same block when the resources have the same alignment
+		 * requirement which is common with multi-GPU setups where
+		 * each bridge window contains a small BAR and a large BAR.
+		 * The small BARs can share the same align block.
+		 *
+		 * As this sizing phase cannot influence resource assignment
+		 * order and placement, this is only guaranteed to work with
+		 * two non-aligning resources.
+		 */
+		if (nonaligning == 2) {
+			second_align = partial_blocks - max_partial;
+		} else {
+			for (unsigned int i = 0; i < max_order; i++) {
+				if (!aligns[i])
+					continue;
+
+				second_align = min(second_align,
+						   1ULL << (i + __ffs(SZ_1M)));
+				break;
+			}
+		}
+
+		gap = ALIGN(remainders, second_align) - remainders;
+	} else if (nonaligning >= 2 ||
+		   (nonaligning && rescount > nonaligning)) {
+		gap = partial_blocks - remainders;
+	}
+
 	*head_align = calculate_head_align(aligns, max_order);
 	*head_align = max(*head_align, win_align);
 
-	return ALIGN(max(winsize, min_size), win_align);
+	return ALIGN(max(winsize + gap, min_size), win_align);
 }
 
 /**
-- 
2.47.3

[PATCH v5 14/14] PCI: Add debug printing to window sizing

[Ilpo Järvinen]

Bridge window sizing prints nothing about its decisions. Some values
used for the calculations are not easy to derive from other log lines
that are already available.

Add window sizing debug print to make troubleshooting easier.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/setup-bus.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 86c9557bc2fb..22da18347488 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1265,7 +1265,7 @@ static resource_size_t __pbus_size_mem(struct pci_bus *bus,
 				       struct list_head *realloc_head)
 {
 	struct pci_dev *dev;
-	resource_size_t winsize = 0, win_align;
+	resource_size_t winsize = 0, win_align, final_winsize;
 	resource_size_t aligns[PCI_RES_ALIGN_RANGE] = {};
 	resource_size_t partial_blocks = 0, max_partial = 0;
 	unsigned int nonaligning = 0, rescount = 0;
@@ -1379,7 +1379,28 @@ static resource_size_t __pbus_size_mem(struct pci_bus *bus,
 	*head_align = calculate_head_align(aligns, max_order);
 	*head_align = max(*head_align, win_align);
 
-	return ALIGN(max(winsize + gap, min_size), win_align);
+	final_winsize = ALIGN(max(winsize + gap, min_size), win_align);
+
+	if (bus->self) {
+		int idx = pci_resource_num(bus->self, b_res);
+		const char *r_name = pci_resource_name(bus->self, idx);
+		struct resource print_res = *b_res; /* b_res update is delayed */
+
+		resource_set_range(&print_res, *head_align, final_winsize);
+		if (final_winsize) {
+			print_res.flags &= ~(IORESOURCE_UNSET |
+					     IORESOURCE_DISABLED);
+		} else {
+			print_res.flags |= IORESOURCE_DISABLED;
+		}
+
+		pci_dbg(bus->self, "%s %pR: non-aligning %u/%u gap %llx partials %llx\n",
+			r_name, &print_res, nonaligning, rescount,
+			(unsigned long long)gap,
+			(unsigned long long)partial_blocks);
+	}
+
+	return final_winsize;
 }
 
 /**
-- 
2.47.3

Re: [PATCH v5 00/14] PCI: Resource placement algorithm fixes

[Nikolas Joshua Britton]

On Fri, 6 Nov 2026, Ilpo Järvinen wrote:
> There have been major changes so any retesting is appreciated (the
> 4 candidates patch is largely the same so I kept the Tested-by tags but
> everything around has been changing so even that is on the borderline
> when it comes to the validity of the existing tags).

I retested v5 on the Mac Pro 7,1 from my report (two Vega II Duo cards,
four Vega 20 dies behind two root ports, BAR0 resized to 32G on every
die).

v7.3 and later don't boot on this machine for reasons unrelated to this
series, so I tested on v7.2.15 plus 5012a6953208 ("PCI: Do not add
hotplug reservation multiple times"), which 11/14 and 12/14 depend on.
All 14 patches then apply as posted, and drivers/pci/setup-bus.c and
drivers/pci/setup-res.c come out byte-identical to v7.4-rc1 + v5.

Every BAR is assigned, both root port windows are 96G, and all four dies
get their 32G BAR0. The first root port:

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

The second root port (14:00.0) has the same layout at 0xbe800000000.

Compared with v4, the two die chains swapped ends of the window: 08:08.0
and the windows nested below it (09:00.0, 0a:00.0) now sit at the top,
with the 2M BAR2 at their left edge. That is the case the dropped
nested-window fix covered, and it still assigns without that patch.
amdgpu binds all four dies and compute across them works.

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

Re: [PATCH v5 00/14] PCI: Resource placement algorithm fixes

[Ilpo Järvinen]

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

On Fri, 6 Nov 2026, Nikolas Joshua Britton wrote:

> On Fri, 6 Nov 2026, Ilpo Järvinen wrote:
> > There have been major changes so any retesting is appreciated (the
> > 4 candidates patch is largely the same so I kept the Tested-by tags but
> > everything around has been changing so even that is on the borderline
> > when it comes to the validity of the existing tags).
> 
> I retested v5 on the Mac Pro 7,1 from my report (two Vega II Duo cards,
> four Vega 20 dies behind two root ports, BAR0 resized to 32G on every
> die).
> 
> v7.3 and later don't boot on this machine for reasons unrelated to this
> series, so I tested on v7.2.15 plus 5012a6953208 ("PCI: Do not add
> hotplug reservation multiple times"), which 11/14 and 12/14 depend on.
> All 14 patches then apply as posted, and drivers/pci/setup-bus.c and
> drivers/pci/setup-res.c come out byte-identical to v7.4-rc1 + v5.
> 
> Every BAR is assigned, both root port windows are 96G, and all four dies
> get their 32G BAR0. The first root port:
> 
>   pci 0000:06:00.0: bridge window [mem 0x9e800000000-0x9ffffffffff 64bit pref]: assigned
>   pci 0000:07:00.0: bridge window [mem 0x9e800000000-0x9ffffffffff 64bit pref]: assigned
>   pci 0000:08:08.0: bridge window [mem 0x9f7ffe00000-0x9ffffffffff 64bit pref]: assigned
>   pci 0000:08:10.0: bridge window [mem 0x9e800000000-0x9f0001fffff 64bit pref]: assigned
>   pci 0000:0b:00.0: BAR 0 [mem 0x9f800000000-0x9ffffffffff 64bit pref]: assigned
>   pci 0000:0b:00.0: BAR 2 [mem 0x9f7ffe00000-0x9f7ffffffff 64bit pref]: assigned
>   pci 0000:0e:00.0: BAR 0 [mem 0x9e800000000-0x9efffffffff 64bit pref]: assigned
>   pci 0000:0e:00.0: BAR 2 [mem 0x9f000000000-0x9f0001fffff 64bit pref]: assigned
> 
> The second root port (14:00.0) has the same layout at 0xbe800000000.
> 
> Compared with v4, the two die chains swapped ends of the window: 08:08.0
> and the windows nested below it (09:00.0, 0a:00.0) now sit at the top,
> with the 2M BAR2 at their left edge. That is the case the dropped
> nested-window fix covered, and it still assigns without that patch.
> amdgpu binds all four dies and compute across them works.

Also the candidate selection precedence change impacts ordering. TBH, I 
liked the v4 order better because it really tried to preserve the left 
edge for large blocks better but there were just so many corner cases with 
it so v5 now uses the largest contiguous size before the largest natural 
size. I might revisit this at some point but getting these regressions 
addressed in mainline is more important than perfection and my 
preferences.

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

Thanks for testing.

All looks promising, sashiko came up nothing interesting. It took 
quite long to get to that point but I think the overall feel I've the 
series feels much more polished now than what v4 was.

-- 
 i.