diff mbox series

[v2,6/7] PCI: Fix composite resource sizing

Message ID 20261002113319.6652-7-ilpo.jarvinen@linux.intel.com
State New
Headers show
Series PCI: Resource placement algorithm fixes | expand

Commit Message

Ilpo Järvinen Oct. 2, 2026, 11:33 a.m. UTC
The bridge window sizing algorithm aims to pack child resources back to
back. With multiple composite child resources whose sizes do not align
to the calculated minimal alignment for the bridge window, back-to-back
placement may not be possible. The non-aligning remainder placement is
limited because it must be adjacent to the rest of the composite
resource.

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

Add bridge window gap size calculator. Basic rules:

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

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

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

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

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

Comments

sashiko-bot@kernel.org Oct. 2, 2026, 11:51 a.m. UTC | #1
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] pci: undersized bridge windows during hotplug due to ignored optional resources
- [High] pci: gap calculation ignores expanded sizes, undersizing optional bridge windows
- [High] pci: gap optimization incorrectly assumes perfect packing, causing allocation failures
--

commit 17592808ea429ea63c8337b7e33d67bb459cf0c2
Author: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Subject: PCI: Fix composite resource sizing

This commit adds a bridge window gap size calculator to correctly pad
alignments for non-aligning composite resources back to back. The gap is
calculated based on the minimal alignment for the bridge window.

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
[ ... ]
> +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);

[Severity: High]
During the optional sizing pass, calculate_win_gap_size() is called with
optional=true, but the gap requirement is evaluated using the unexpanded base
size from resource_size(r). It does not appear to consider the requested
add_size expansions from realloc_head.

If a child bridge's base size perfectly aligns to the parent's add_align
(resulting in a 0 gap here), but the child's expanded size is unaligned, won't
this result in an undersized parent optional window because it failed to
provision the padding gap for the expanded size?

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

[Severity: High]
Does this optimization assume perfect packing of fragmented remainders,
violating the strict alignment constraints of the resources themselves?

If both resources require win_align alignment (e.g., both have size 5MB and
alignment 4MB), the second resource cannot be placed adjacent to the first
and must be bumped to the next 4MB boundary. Wouldn't this reduction in the
gap cause the second resource to fail assignment because the bridge window
is undersized?

> +
> +	if (nonaligning >= 2)
> +		return safe_gap;
> +
> +	return 0;
> +}
[ ... ]
> @@ -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);

[Severity: High]
When a device with optional resources (like SR-IOV BARs) is sized via a path
where realloc_head is NULL (e.g., during hotplug via pci_bus_size_bridges()),
pbus_size_mem() accumulates the sizes of those optional resources into the
base size because pbus_size_mem_optional() returns false.

However, the gap calculator is invoked here with optional=false, which
unconditionally drops optional resources. Doesn't this cause their required
alignment padding gaps to be omitted from the bridge window entirely, leaving
the bridge window undersized?

> +	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) {
diff mbox series

Patch

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) {