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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] PCI: pci_resreq_remainder() treats size < align as tail remainder
- [Medium] PCI: wrong bus passed to pci_min_window_alignment() in __pci_assign_resource()
--

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

PCI: Fix nesting windows with remainder at the left edge

This commit wraps pcibios_align_resource() with pci_resreq_check() and passes
a smaller alignment constraint to the resource allocator when a memory
resource has a non-zero remainder. This allows nested bridge windows with a
remainder at the left edge to fit into parent windows of the same size while
preserving the real alignment in pci_resreq_data.

> diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
> index 441a62807719d..2ca3784aa323f 100644
> --- a/drivers/pci/setup-res.c
> +++ b/drivers/pci/setup-res.c

[ ... ]

> @@ -260,6 +262,12 @@ resource_size_t pci_resource_alignment(const struct pci_dev *dev,
>  	return resource_alignment(res);
>  }
>  
> +static resource_size_t pci_resreq_remainder(resource_size_t size,
> +					    resource_size_t align)
> +{
> +	return size - ALIGN_DOWN(size, align);
> +}
> +
>  /*
>   * For mem bridge windows, try to relocate tail remainder space to space
>   * before res->start if there's enough free space there. This enables
> @@ -280,10 +288,17 @@ resource_size_t pci_align_resource(struct pci_resreq_data *rr,
>  	if (rr->user_align)
>  		return res->start;
>  
> -	if (IS_ALIGNED(size, align))
> +	remainder = pci_resreq_remainder(size, align);
> +	if (!remainder)
> +		return res->start;
> +
> +	/*
> +	 * Size constraints forced an early start move in
> +	 * pci_check_and_align_resource()?
> +	 */
> +	if (!IS_ALIGNED(res->start, align))
>  		return res->start;
>  
> -	remainder = size - ALIGN_DOWN(size, align);
>  	/* Don't mess with size that doesn't align with window size granularity */
>  	if (!IS_ALIGNED(remainder, pci_min_window_alignment(dev->bus, res->flags)))
>  		return res->start;
> @@ -315,6 +330,33 @@ resource_size_t __weak pcibios_align_resource(void *data,
>  	return pci_align_resource(rr, res, empty_res, size, align);
>  }
>  
> +static resource_size_t pci_resreq_check(void *data,
> +					const struct resource *res,
> +					const struct resource *empty_res,
> +					resource_size_t size,
> +					resource_size_t min_align)
> +{
> +	struct pci_resreq_data *rr = data;
> +	resource_size_t align = rr->real_align;
> +	resource_size_t remainder, start;
> +	struct resource tmp = *res;
> +
> +	if (res->flags & IORESOURCE_MEM && !IS_ALIGNED(res->start, align)) {
> +		resource_set_range(&tmp, ALIGN(res->start, align), size);
> +		if (!__resource_contains_unbound(empty_res, &tmp)) {
> +			remainder = pci_resreq_remainder(size, align);
> +			resource_set_range(&tmp, tmp.start - remainder, size);
> +			if (!__resource_contains_unbound(empty_res, &tmp))
> +				return tmp.start;	/* caller skips range */
> +		}
> +	}

[Severity: High]
What happens in pci_resreq_remainder() and pci_resreq_check() when a memory
resource has size < align and rr.user_align is 0?

When size < align, ALIGN_DOWN(size, align) evaluates to 0, so
pci_resreq_remainder(size, align) returns size instead of 0. That treats the
entire resource as a movable remainder even though it has no align-sized
portion.

This can occur with rr.user_align == 0 in paths such as:

1. On PowerNV, pcibios_default_alignment() -> pnv_pci_default_alignment()
   returns PAGE_SIZE (for example, 64 KiB). When no pci=resource_alignment=
   parameter matches the device, pci_reassigndev_resource_alignment() ->
   pci_specified_resource_alignment(dev, &resize) returns align = PAGE_SIZE
   with resize = false. For a 4 KiB BAR, pci_request_resource_alignment()
   sets IORESOURCE_STARTALIGN with r->start = PAGE_SIZE while keeping
   size = 4 KiB, and pci_init_pci_resreq_data() leaves rr.user_align = 0.

2. For a CardBus bridge (PCI_CLASS_BRIDGE_CARDBUS), BAR 0 has size = 4 KiB
   while pci_resource_alignment() -> pci_cardbus_resource_alignment()
   returns 64 MiB with rr.user_align == 0.

In __pci_assign_resource(), because pci_resreq_remainder(size, align) is
non-zero and !pci_resource_is_bridge_win(resno), the alignment constraint
passed to pci_bus_alloc_resource() is lowered to 1U.

Then in pci_resreq_check(), if an unaligned free range empty_res cannot fit
[ALIGN(res->start, align), ALIGN(res->start, align) + size - 1], subtracting
remainder (which equals size) shifts tmp.start to
ALIGN(res->start, align) - size. If that range fits within empty_res, the
resource is assigned just below the alignment boundary at an unaligned
address (for example, 0x8000f000 in [0x80001000, 0x8000ffff] after an earlier
4 KiB BAR at 0x80000000), violating the required alignment.

Later in the series, commit "PCI: Place resources to either edge of the
window" also uses pci_resreq_remainder() in pci_align_resource() to construct
candidates at ALIGN(empty_res->start, align) - remainder and
ALIGN_DOWN(empty_res->end + 1, align) - size, which similarly selects
unaligned addresses when size < align.

Should pci_resreq_remainder() return 0 when size < align?

> +
> +	start = pcibios_align_resource(rr, &tmp, empty_res, size, align);
> +	WARN_ON_ONCE(!IS_ALIGNED(start, min_align));
> +
> +	return start;
> +}
> +
>  static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
>  		int resno, resource_size_t size, resource_size_t align)
>  {
> @@ -324,9 +366,17 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
>  	int ret;
>  
>  	pci_init_pci_resreq_data(&rr, dev);
> +	rr.real_align = align;
>  
>  	min = (res->flags & IORESOURCE_IO) ? PCIBIOS_MIN_IO : PCIBIOS_MIN_MEM;
>  
> +	if (!rr.user_align &&
> +	    (res->flags & IORESOURCE_MEM) && pci_resreq_remainder(size, align)) {
> +		align = 1U;
> +		if (pci_resource_is_bridge_win(resno))
> +			align = pci_min_window_alignment(bus, res->flags);
> +	}

[Severity: Medium]
Should pci_min_window_alignment() here (and in pci_align_resource()) be
passed dev->subordinate instead of bus?

When sizing bridge windows in pbus_size_io() and pbus_size_mem(),
pci_min_window_alignment() is called with the bridge's subordinate bus
(where bus->self is the bridge device dev):

drivers/pci/setup-bus.c:pbus_size_mem() {
	...
	win_align = pci_min_window_alignment(bus, b_res->flags);
}

On PowerNV, pnv_pci_window_alignment() walks up from bridge = bus->self
counting PCI_EXP_TYPE_PCI_BRIDGE devices:

arch/powerpc/platforms/powernv/pci-ioda.c:pnv_pci_window_alignment() {
	...
	bridge = bus->self;
	while (bridge) {
		if (pci_pcie_type(bridge) == PCI_EXP_TYPE_PCI_BRIDGE) {
			num_pci_bridges++;
			if (num_pci_bridges >= 2)
				return 1;
		}

		bridge = bridge->bus->self;
	}
	...
}

In __pci_assign_resource(), dev is the bridge whose window res is being
assigned, while bus is dev->bus (or an ancestor bus). For a second-level
PCI_EXP_TYPE_PCI_BRIDGE bridge on PowerNV, pbus_size_mem(dev->subordinate,
...) sizes the window with a 1 MiB minimum window alignment
(num_pci_bridges >= 2), whereas calling pci_min_window_alignment(bus,
res->flags) with dev->bus sees num_pci_bridges == 1 and returns m32_segsize
or m64_segsize (for example, 16 MiB or 64 MiB) as min_align.

If pci_resreq_check() (or pci_align_resource() in the subsequent commit
"PCI: Place resources to either edge of the window") places the window at an
address shifted by remainder that is aligned to dev->subordinate's 1 MiB
window alignment but not to the parent bus's segment size, can that reject
valid candidates or trigger WARN_ON_ONCE(!IS_ALIGNED(start, min_align)) in
pci_resreq_check()?

[ ... ]

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