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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] PCI: missing bridge window gap in calculate_win_gap_size() when r_align != win_align
- [Medium] PCI: missing bridge window gap in calculate_win_gap_size() for single remainder
- [Medium] PCI: calculate_win_gap_size() underestimates gap with small resources
--

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

PCI: Fix composite resource sizing

This commit adds calculate_win_gap_size() to pbus_size_mem() to account for
alignment gaps required when packing multiple non-aligning composite child
resources into a PCI 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..2980b499ef759 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -1164,6 +1164,79 @@ static inline resource_size_t calculate_mem_align(resource_size_t *aligns,
>  	return min_align;
>  }
>  
> +/*
> + * Bridge window gap size calculator.
> + *
> + * Calculates gap (empty space) necessary because of non-aligning composite
> + * resources (VF BARs, bridge windows).
> + *
> + * Rules:
> + *
> + * 1) Gaps are only necessary if there is more than one non-aligning
> + *    composite resource within a single bridge window.
> + *
> + * 2) If there are only two remainder parts that amount to less than win_align
> + *    together, the required gap is the difference of win_align and the sum of
> + *    remainder sizes.
> + *
> + * 3) On other cases, round each remainder part to win_align to get the gap
> + *    size. Sometimes, tighter packing might be possible but due to how
> + *    sizing and assignment are made in different phases, it is not always
> + *    possible to predict where each resource is assigned. Thus, the sizing
> + *    has to play safe even if it may overestimate in some cases.
> + */
> +static resource_size_t calculate_win_gap_size(struct pci_bus *bus,
> +					      struct resource *b_res,
> +					      resource_size_t win_align,
> +					      bool optional,
> +					      struct list_head *realloc_head)
> +{
> +	resource_size_t safe_gap = 0, remainders = 0;
> +	unsigned int nonaligning = 0;
> +	struct pci_dev *dev;
> +
> +	list_for_each_entry(dev, &bus->devices, bus_list) {
> +		struct resource *r;
> +		int i;
> +
> +		pci_dev_for_each_resource(dev, r, i) {
> +			resource_size_t r_size, remainder, aligning;
> +
> +			if (!pdev_resources_assignable(dev) ||
> +			    !pdev_resource_should_fit(dev, r))
> +				continue;
> +			if (b_res != pbus_select_window(bus, r))
> +				continue;
> +
> +			if (!optional && pci_resource_is_optional(dev, i))
> +				continue;
> +
> +			r_size = resource_size(r);
> +			if (realloc_head && optional)
> +				r_size += get_res_add_size(realloc_head, r);
> +			if (r_size <= win_align)
> +				continue;
> +
> +			aligning = ALIGN_DOWN(r_size, win_align);
> +			remainder = r_size - aligning;
> +			if (!remainder)
> +				continue;

[Severity: High]
Should calculate_win_gap_size() check and split each child resource against
its own required alignment pci_resource_alignment(dev, r) rather than the
parent bridge window's win_align?

During resource assignment in __pci_bus_assign_resources() ->
pbus_assign_resources_sorted() -> pci_assign_resource() ->
pci_align_resource(), each child resource r is aligned and split into an
aligning part and an adjacent remainder using its own alignment r_align =
pci_resource_alignment(dev, r).

However, during sizing in __pci_bus_size_bridges() -> pbus_size_mem(),
resources with r_size > align are excluded from aligns[order] while still
updating max_order, and pbus_size_mem() then passes min_align (for size0) or
add_align (for size1) as win_align to calculate_win_gap_size():

drivers/pci/setup-bus.c:pbus_size_mem() {
	...
			if (r_size <= align)
				aligns[order] += align;
			if (order > max_order)
				max_order = order;
		}
	}

	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);
	gap_size = calculate_win_gap_size(bus, b_res, min_align, !realloc_head,
					  realloc_head);
	...
	if (realloc_head && (add_size > 0 || children_add_size > 0)) {
		add_align = max(min_align, add_align);
		gap_size = calculate_win_gap_size(bus, b_res, add_align, true,
						  realloc_head);
}

When a child resource's r_align differs from win_align, this can omit the
required gap in three ways and cause pci_assign_resource() to fail with
-ENOSPC:

1. r_align < r_size <= win_align: Suppose a bus has a mandatory 64M BAR
(r_size = 64M, r_align = 64M) and two mandatory child bridge windows each
with r_size = 20M and r_align = 16M. pbus_size_mem() computes size = 104M
and min_align = 64M. In calculate_win_gap_size(), both 20M composite windows
are skipped by:

	if (r_size <= win_align)
		continue;

because 20M <= 64M, returning gap_size = 0 and sizing b_res to 104M instead
of the required 112M (64M + 20M + 20M + 8M gap). During assignment in a
104M window, the 64M BAR consumes the 64M-aligned region and leaves a 40M
region; placing the first 20M@16M window leaves no 16M-aligned block with
adjacent room for the second 20M@16M window.

2. min_align < r_size <= add_align when sizing optional resources for size1:
Suppose a bus has two mandatory 20M@16M child bridge windows (or three
18M@16M windows) plus an optional resource (such as a 32M SR-IOV BAR) that
raises add_align to 32M. The first call with min_align = 16M includes the
required 8M (or 42M) gap in size0, but the second call with add_align = 32M
skips the mandatory 20M (or 18M) windows because r_size <= 32M, dropping
their required gap from size1.

3. win_align < r_align when calculate_head_align() reduces min_align:
Suppose a bus has two mandatory composite windows of r_size = 80M (or 96M)
with r_align = 64M and one 32M BAR (r_size = 32M, r_align = 32M). Because
the composite windows have r_size > 64M, aligns[6] remains 0 while
max_order = 6 and aligns[5] = 32M, so calculate_head_align() reduces
min_align from 64M to 32M. Passing min_align = 32M as win_align computes
remainders modulo 32M (16M + 16M = 32M -> win_align - remainders = 0 for
80M, or remainder = 0 for 96M), returning gap_size = 0 even though two
64M-aligned composite resources and the 32M BAR cannot fit into the
resulting 192M window.

> +
> +			nonaligning++;
> +			remainders += remainder;
> +			safe_gap += win_align - remainder;
> +		}
> +	}
> +
> +	if (nonaligning == 2 && (remainders <= win_align))
> +		return win_align - remainders;

[Severity: Medium]
Can returning win_align - remainders here underestimate the required gap
when smaller resources with r_size <= win_align are also present in the same
bridge window?

When a bridge window contains two non-aligning composite resources whose
remainders r1 and r2 sum to <= win_align plus one or more smaller resources
(skipped by r_size <= win_align) whose total size S is less than
min_align / 2, calculate_head_align() leaves min_align unchanged and Rule 2
sizes the bridge window to A1 + A2 + win_align + S. This assumes both
remainders r1 and r2 will share a single win_align block and leave a
contiguous S-sized region for the smaller resources.

However, during pbus_assign_resources_sorted() -> pci_assign_resource() ->
pci_align_resource(), candidate[2] in drivers/pci/setup-res.c evaluates
placing the first composite resource's remainder at the right end of the
window:

drivers/pci/setup-res.c:pci_align_resource() {
	...
	candidate[2] = DEFINE_RES(ALIGN_DOWN(empty_res->end + 1 - remainder, align) -
				  aligning, size, type);
}

When r1 <= S and r2 + S > win_align, candidate[2] places the first composite
resource's remainder r1 into the trailing S-sized tail because doing so
maximizes the power-of-two natural_size of the remaining free space on the
left. Once the second composite resource is placed, the space intended for S
is split into two fragments (S - r1 at the right tail and win_align - r2 on
the left), neither of which is large enough to fit the smaller resource,
causing its assignment to fail with -EBUSY.

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

[Severity: Medium]
Is an alignment gap also needed when nonaligning == 1 and the single
composite resource's remainder is not aligned to a smaller sibling
resource's alignment?

Consider a bus with one composite child bridge window r1 of size 84M
(64M + 16M + 4M, align = 64M) and one normal sibling BAR r2 of size 16M
(align = 16M). In pbus_size_mem(), r1 has r_size > align so aligns[6] is 0
with max_order = 6, while r2 sets aligns[4] = 16M. Because 16M < 64M / 2,
calculate_head_align() returns min_align = 64M.

In calculate_win_gap_size(), r1 has remainder = 20M (nonaligning = 1) and r2
is skipped because 16M <= 64M. Because nonaligning < 2, it returns 0 and
pbus_size_mem() sizes b_res to 84M + 16M = 100M with min_align = 64M.

When b_res (size 100M, align 64M, aligning = 64M, remainder = 36M) is
assigned by pci_align_resource():

drivers/pci/setup-res.c:pci_align_resource() {
	...
	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);
}

every candidate places b_res at either [K, K + 100M) or [K - 36M, K + 64M)
for some 64M-aligned address K, so b_res contains only a single 64M-aligned
block [K, K + 64M).

During pbus_assign_resources_sorted(), r1 (align 64M) is assigned before r2
(align 16M), consuming [K, K + 64M) and placing its adjacent 20M remainder
at [K + 64M, K + 84M) (or [K - 20M, K)). That leaves only [K + 84M, K + 100M)
(or [K - 36M, K - 20M)) for r2; both 16M intervals are shifted by 20M (only
4M-aligned) and straddle a 16M boundary, so __find_resource_space() fails to
assign r2 with -EBUSY.

[ ... ]

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