Re: [PATCH v4 3/7] PCI: Honor alignment overrides

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] PCI: pci_align_resource() still misaligns resources when size < align
--

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

PCI: Honor alignment overrides

This commit introduces struct pci_resreq_data to pass the PCI device and
user-specified resource alignment into pci_align_resource() so that
remainder movement is skipped when a user alignment override is active.

Link: https://lore.kernel.org/linux-pci/20260923133202.07DF61F000FF@smtp.kernel.org/

[ ... ]

> diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
> index 376f09630a4ab..441a62807719d 100644
> --- a/drivers/pci/setup-res.c
> +++ b/drivers/pci/setup-res.c
> @@ -265,17 +265,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->user_align)
> +		return res->start;
> +
>  	if (IS_ALIGNED(size, align))
>  		return res->start;
>  

[ ... ]

> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index d31a8d107b1ef..ea7d758712691 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1217,12 +1217,40 @@ 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 */
> +/* Resource assignment request handling */
> +/**
> + * struct pci_resreq_data - Data passed to pcibios_align_resource()
> + * @dev: PCI device
> + * @user_align: User provided alignment override (pci=resource_alignment param)
> + */
> +struct pci_resreq_data {
> +	struct pci_dev *dev;
> +	resource_size_t user_align;
> +};
> +
> +resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
> +						 bool *resize);
> +/*
> + * TODO: Improve PCI resource assignment interface so that callers outside PCI
> + * core do not need to know about this detail.
> + */
> +static inline void pci_init_pci_resreq_data(struct pci_resreq_data *rr,
> +					    struct pci_dev *dev)
> +{
> +	bool user_aligned;
> +
> +	memset(rr, 0, sizeof(*rr));
> +	rr->dev = dev;
> +	rr->user_align = pci_specified_resource_alignment(dev, &user_aligned);
> +	if (!user_aligned)
> +		rr->user_align = 0;
> +}

[Severity: High]
Can pci_align_resource() still misalign resources when size < align?

Looking at pci_reassigndev_resource_alignment() ->
pci_request_resource_alignment() in drivers/pci/pci.c:

	if (resize) {
		r->start = 0;
		r->end = align - 1;
	} else {
		r->flags &= ~IORESOURCE_SIZEALIGN;
		r->flags |= IORESOURCE_STARTALIGN;
		resource_set_range(r, align, size);
	}

When a device matches pci=resource_alignment= so resize (user_aligned) is
true, BARs in 0..PCI_ROM_RESOURCE with size < align are resized so that
size == align. For those BARs, IS_ALIGNED(size, align) in
pci_align_resource() is already true, so rr->user_align is not needed to
skip remainder calculation for them.

Conversely, when resize (user_aligned) is false because align comes from
pcibios_default_alignment() (such as pnv_pci_default_alignment() returning
PAGE_SIZE on PowerNV), pci_request_resource_alignment() takes the else
branch and sets IORESOURCE_STARTALIGN while keeping size < align. However,
pci_init_pci_resreq_data() clears rr->user_align = 0 when !user_aligned.

Whenever pci_align_resource() in drivers/pci/setup-res.c is called with
size < align and rr->user_align == 0 (whether for IORESOURCE_STARTALIGN
BARs via pcibios_default_alignment(), even if moved to
pci_resource_alignment(), or for a bridge window whose alignment was raised
above its size):

	if (IS_ALIGNED(size, align))
		return res->start;

	remainder = size - ALIGN_DOWN(size, align);

ALIGN_DOWN(size, align) evaluates to 0 and remainder equals size. Because
there is no aligning body, subtracting remainder shifts the entire resource
to res->start - size (and in the later commits "PCI: Fix nesting windows
with remainder at the left edge" and "PCI: Place resources to either edge
of the window", lowers constraint->align to 1U and selects candidate[1] or
candidate[3] at aligned_edge - size), placing the entire resource below the
aligned boundary at an address that is only size-aligned rather than
align-aligned.

Additionally, when a PCI-to-PCI bridge is specified in
pci=resource_alignment= to allow expanding its bridge windows,
pci_reassigndev_resource_alignment() only unsets
PCI_BRIDGE_RESOURCES..PCI_NUM_RESOURCES - 1 without requesting user_align
on those windows. Because pci_init_pci_resreq_data() sets rr->user_align on
the bridge pci_dev for all of its resources, wouldn't this disable
remainder handling on the bridge windows (and in the later commit "PCI:
Place resources to either edge of the window", reject bridge window
candidates not aligned to rr->user_align)?

Would checking size <= align in pci_align_resource() (or restricting
remainder relocation to composite bridge window resources where size >
align) avoid shifting resources with size < align below their alignment
boundary?

[ ... ]

>

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