Re: [PATCH v4 6/7] PCI: Place resources to either edge of the window

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] pci: unaligned placement in pci_align_resource() when size is less than align
- [Medium] pci: pci_max_natural_size() underreports max_align after halving powof2
- [Medium] pci: WARN_ON_ONCE() in pci_resreq_check() when user_align is set with remainder
- [Medium] pci: pci_align_resource() bypasses user_align when returning on best == -1
- [Medium] PCI: unaligned candidate start addresses in pci_align_resource()
--

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

PCI: Place resources to either edge of the window

This commit updates pci_align_resource() to evaluate four candidate positions
at the left and right edges of an available free space range. It selects the
candidate that leaves the largest naturally aligned power-of-two range, the
largest contiguous free space, and the highest start alignment in the
remaining window space.

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

[ ... ]

> @@ -262,16 +265,84 @@ resource_size_t pci_resource_alignment(const struct pci_dev *dev,
>  	return resource_alignment(res);
>  }
>  
> +static resource_size_t pci_max_natural_size(const struct resource *res,
> +					    resource_size_t *max_align)
> +{
> +	resource_size_t size = resource_size(res);
> +	resource_size_t powof2, natural_start;
> +
> +	*max_align = 1;
> +	if (!size)
> +		return 0;
> +
> +	powof2 = rounddown_pow_of_two64(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);
> +		natural_start = ALIGN(res->start, powof2);
> +	}
> +
> +	if (natural_start) {
> +		*max_align <<= __ffs64(natural_start);
> +	} else {
> +		/*
> +		 * Zero address has infinite alignment, return the largest
> +		 * representable number even if it's not a power of two.
> +		 */
> +		*max_align = RESOURCE_SIZE_MAX;
> +	}
> +
> +	return powof2;
> +}

[Severity: Medium]
When the initial powof2 does not fit within res and pci_max_natural_size()
halves powof2, could unconditionally overwriting natural_start with
ALIGN(res->start, powof2) underreport max_align?

If the remaining range contains two naturally aligned blocks of the halved
powof2 size (for example, when res starts at an odd multiple of the halved
powof2 and has size equal to two such blocks), ALIGN(res->start, powof2)
selects the first block at the odd multiple (which only has alignment equal
to the halved powof2) rather than the second block at the original
natural_start (which is at an even multiple and has at least twice the
alignment).

For example, if a root bus window is [0x10900000-0x10b00fff] and a 4KB BAR 0
is assigned before a 2MB, 1MB-aligned bridge window:

1. candidate[0] places BAR 0 at the left edge [0x10900000-0x10900fff],
   leaving remaining = [0x10901000-0x10b00fff] (size 0x200000). Halving
   powof2 to 0x100000 gives natural_start = ALIGN(0x10901000, 0x100000) =
   0x10a00000, so natural_size = 0x100000, size = 0x200000, and
   maxalign = 0x200000.

2. candidate[2] places BAR 0 at the right edge [0x10b00000-0x10b00fff],
   leaving remaining = [0x10900000-0x10afffff] (size 0x200000), which
   preserves both 1MB blocks. However, halving powof2 to 0x100000 overwrites
   natural_start with ALIGN(0x10900000, 0x100000) = 0x10900000, reporting
   natural_size = 0x100000, size = 0x200000, and maxalign = 0x100000.

Because natural_size and size tie, maxalign breaks the tie in favor of
candidate[0], placing BAR 0 at the left edge and blocking the 2MB bridge
window from fitting. Should pci_max_natural_size() keep the original
natural_start when a halved powof2 block starting at that address still fits
within res?

> +
>  static resource_size_t pci_resreq_remainder(resource_size_t size,
>  					    resource_size_t align)
>  {
>  	return size - ALIGN_DOWN(size, align);
>  }

[ ... ]

> @@ -280,38 +351,85 @@ 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;
> +	resource_size_t best_natural_size = 0, best_size = 0, best_maxalign = 0;
> +	resource_size_t aligning, remainder;
> +	struct resource candidate[4];
> +	unsigned int i;
> +	int best = -1;
>  
>  	if (!(res->flags & IORESOURCE_MEM))
>  		return res->start;
>  
> -	if (rr->user_align)
> -		return res->start;
> -
>  	remainder = pci_resreq_remainder(size, align);
> -	if (!remainder)
> -		return res->start;
> +	aligning = size - remainder;
> +
> +	candidate[0] = DEFINE_RES(ALIGN(empty_res->start, align), size, type);
> +	candidate[1] = DEFINE_RES(ALIGN(empty_res->start, align) - remainder,
> +				  size, type);
> +	candidate[2] = DEFINE_RES(ALIGN_DOWN(empty_res->end + 1 - remainder, align) -
> +				  aligning, size, type);
> +	candidate[3] = DEFINE_RES(ALIGN_DOWN(empty_res->end + 1, align) - size,
> +				  size, type);

[Severity: High]
What happens here when a memory resource has size < align and
rr->user_align == 0?

In pci_resreq_remainder(size, align), ALIGN_DOWN(size, align) evaluates to 0
whenever size < align, so pci_resreq_remainder() returns size instead of 0:

drivers/pci/setup-res.c:pci_resreq_remainder() {
	return size - ALIGN_DOWN(size, align);
}

Because pci_resreq_remainder(size, align) is non-zero and rr.user_align == 0,
__pci_assign_resource() lowers the alignment constraint passed to
pci_bus_alloc_resource() to 1U (or pci_min_window_alignment() for bridge
windows):

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

Then in pci_align_resource(), remainder = size and aligning = 0, which treats
the entire resource as a non-aligning remainder with a zero-sized aligning
part:

- candidate[1] is placed at ALIGN(empty_res->start, align) - size
- candidate[3] is placed at ALIGN_DOWN(empty_res->end + 1, align) - size

Neither candidate is aligned to align, and candidate[3] leaves align - size
more contiguous free space on the left than the aligned candidate[2] while
preserving empty_res->start (giving a higher maxalign than candidate[0]), so
pci_align_resource() selects candidate[3] (or candidate[1]) and assigns the
resource to an unaligned start address.

This can be reached when size < align with rr->user_align == 0 via:

- Platforms implementing pcibios_default_alignment() (such as PowerNV
  pnv_pci_default_alignment() returning 64KB), where
  pci_reassigndev_resource_alignment() -> pci_request_resource_alignment()
  with resize == false sets IORESOURCE_STARTALIGN to 64KB while keeping a
  smaller BAR size (e.g., 4KB) and leaving rr->user_align == 0.
- CardBus bridges, where pci_resource_alignment() returns
  pci_cardbus_resource_alignment() (64MB) for all memory resources including
  the 4KB BAR 0.
- SR-IOV VF BARs where pcibios_iov_resource_alignment() returns an alignment
  larger than total_VFs * vf_bar_size, or resources in
  reassign_resources_sorted() where add_align > size.

Should pci_resreq_remainder() return 0 when size < align?

> +
> +	for (i = 0; i < ARRAY_SIZE(candidate); i++) {
> +		struct resource remaining;
> +		resource_size_t natural_size, size, maxalign;
> +
> +		if ((candidate[i].start > candidate[i].end) ||
> +		    !__resource_contains_unbound(empty_res, &candidate[i])) {
> +			pci_dbg(dev, "%pR: candidate %u not within free space %pR\n",
> +				&candidate[i], i, empty_res);
> +			continue;
> +		}
>  
> -	/*
> -	 * Size constraints forced an early start move in
> -	 * pci_check_and_align_resource()?
> -	 */
> -	if (!IS_ALIGNED(res->start, align))
> -		return res->start;
> +		if (rr->user_align &&
> +		    !IS_ALIGNED(candidate[i].start, rr->user_align)) {
> +			pci_dbg(dev, "%pR: candidate %u skipped due to resource_alignment=%llx\n",
> +				&candidate[i], i,
> +				(unsigned long long)rr->user_align);
> +			continue;
> +		}

[Severity: Medium]
Can removing the early return on rr->user_align trigger the WARN_ON_ONCE() in
pci_resreq_check() when rr->user_align is set on a device that also has a
composite resource with a non-zero remainder?

In __pci_assign_resource(), min_align (align) is only lowered when
!rr.user_align:

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

If pci=resource_alignment=... matches a bridge or SR-IOV PF device with
rr->user_align smaller than rr->real_align (for example, rr->user_align = 4KB
while assigning a bridge window or VF BAR with real_align = 2MB, size = 3MB,
and remainder = 1MB), __pci_assign_resource() leaves min_align at 2MB.

In pci_align_resource(), candidate[1] and candidate[3] have 1MB-aligned start
addresses, which pass IS_ALIGNED(candidate[i].start, rr->user_align), so
pci_align_resource() can return a 1MB-aligned start address to
pci_resreq_check():

drivers/pci/setup-res.c:pci_resreq_check() {
	start = pcibios_align_resource(rr, &tmp, empty_res, size, align);
	WARN_ON_ONCE(!IS_ALIGNED(start, min_align));
}

With start aligned to 1MB and min_align equal to 2MB, won't this trigger
WARN_ON_ONCE(!IS_ALIGNED(start, min_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;

[Severity: Medium]
Is it safe to remove the pci_min_window_alignment() check on remainder without
checking candidate alignment for bridge windows?

In drivers/pci/setup-bus.c:pci_bus_distribute_available_resources(),
remove_dev_resources() advances available[i].start by non-bridge BAR sizes on
the bus (such as a 4KB BAR 0 on a PCIe port). When available[i] is split
across downstream bridges via resource_set_size(), available[i].end becomes
unaligned by that sub-1MB offset, and the recursive call for the child bus
aligns available[i].start up before calling adjust_bridge_window():

drivers/pci/setup-bus.c:pci_bus_distribute_available_resources() {
		align = pci_resource_alignment(bridge, res);
		if (!resource_assigned(res) && align)
			available[i].start = min(ALIGN(available[i].start, align),
						 available[i].end + 1);
		...
		adjust_bridge_window(bridge, res, add_list,
				     resource_size(&available[i]));
}

This can leave a child bridge window with a size whose remainder is not a
multiple of pci_min_window_alignment(dev->bus, res->flags) (1MB).

When __pci_assign_resource() assigns that bridge window, it sets min_align to
pci_min_window_alignment(bus, res->flags). In pci_align_resource(),
candidate[1] (ALIGN(empty_res->start, align) - remainder) and candidate[3]
(ALIGN_DOWN(empty_res->end + 1, align) - size) will have start addresses that
are not aligned to pci_min_window_alignment(). If candidate[1] or
candidate[3] is selected, pci_resreq_check() will trigger
WARN_ON_ONCE(!IS_ALIGNED(start, min_align)) and assign a bridge window whose
base address is not 1MB-aligned.

> -	/* 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)
> +		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;
> +		}
> +
> +		natural_size = pci_max_natural_size(&remaining, &maxalign);
> +		size = resource_size(&remaining);
> +		pci_dbg(dev, "%pR: candidate %u free space naturalsize=%llx size=%llx maxalign=%llx\n",
> +			&candidate[i], i,
> +			(unsigned long long)natural_size,
> +			(unsigned long long)size,
> +			(unsigned long long)maxalign);
> +		if ((best < 0) ||
> +		    (natural_size > best_natural_size) ||
> +		    (natural_size == best_natural_size && size > best_size) ||
> +		    (natural_size == best_natural_size && size == best_size &&
> +		     maxalign > best_maxalign)) {
> +			best = i;
> +			best_natural_size = natural_size;
> +			best_size = size;
> +			best_maxalign = maxalign;
> +		}
> +	}
> +
> +	/* None fits? Return some address and let the caller deal with it. */
> +	if (best == -1)
>  		return res->start;

[Severity: Medium]
If all candidates in empty_res are rejected because none of them satisfies
IS_ALIGNED(candidate[i].start, rr->user_align), won't returning res->start
here cause the caller to accept an unaligned allocation?

When pci_resreq_check() calls pcibios_align_resource() -> pci_align_resource(),
res is tmp, which has already been aligned to align inside empty_res. If
empty_res is large enough to contain [res->start, res->start + size - 1],
 returning res->start hands back a range that lies completely within
empty_res (full_avail).

In kernel/resource.c:__find_resource_space(), the caller only checks whether
the returned range fits inside full_avail:

kernel/resource.c:__find_resource_space() {
			alloc.start = alignf(alignf_data, &avail, size, align);
			alloc.end = alloc.start + size - 1;
			if (alloc.start <= alloc.end &&
			    __resource_contains_unbound(&full_avail, &alloc)) {
				new->start = alloc.start;
				new->end = alloc.end;
				return 0;
			}
}

Because [res->start, res->start + size - 1] is contained in full_avail, won't
__find_resource_space() succeed and allocate the resource at res->start in
violation of rr->user_align instead of skipping empty_res?

>  
> -	pci_dbg(dev, "%pR: moving candidate start address below align to %llx\n",
> -		res, (unsigned long long)start_addr);
> -	return start_addr;
> +	pci_dbg(dev, "%pR: 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;
>  }

[ ... ]

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