Re: [PATCH v2] PCI: Reserve prefetchable window headroom for Resizable BARs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Fatal 64-bit size truncation using `roundup_pow_of_two()` on 32-bit platforms.
- [High] Stripping `IORESOURCE_PREFETCH` silently breaks write-combining (WC) mappings for control BARs.
- [High] ReBAR headroom reservation is erroneously added as a mandatory size requirement instead of an optional one, potentially causing total resource allocation failures.
- [High] BAR demotion permanently strips the PREFETCH flag, making fallback to the prefetchable window impossible if 32-bit space is exhausted.
- [Medium] Hardcoded BAR 0 incorrectly assumes Resizable BAR capabilities only exist on the first BAR, breaking headroom reservation for GPUs with different topologies.
- [Medium] Upstream bridge windows are permanently pinned if a non-display device shares the switch topology.
--

commit ab6d281d10e22bc05fa969730e7cb1fb4d1cc31e
Author: Geramy Loveless <gloveless@jqluv.com>

PCI: Reserve prefetchable window headroom for Resizable BARs

Firmware typically sizes prefetchable bridge windows for the boot-time
BAR size. Behind a fixed PCIe switch fabric, there is then no room for a
driver to grow a Resizable BAR afterwards. This patch reserves prefetchable
window headroom and demotes small prefetchable BARs below 4 GiB to enable
ReBAR on cascaded switch fabrics.

> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index de9bbccda21fd..57a5926a273c6 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
[ ... ]
> +	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> +		r = pci_resource_n(dev, i);
> +		if ((r->flags & (IORESOURCE_MEM | IORESOURCE_PREFETCH)) !=
> +		    (IORESOURCE_MEM | IORESOURCE_PREFETCH))
> +			continue;
> +		if (r->flags & IORESOURCE_PCI_FIXED)
> +			continue;
> +		if (pci_resource_alignment(dev, r) >= max_align)
> +			continue;		/* an alignment-setting BAR, keep it */
> +		if (resource_size(r) > PCI_BAR_PACK_CAP)
> +			continue;		/* not a small control BAR */
> +
> +		r->flags &= ~IORESOURCE_PREFETCH;

[Severity: High]
Does stripping this flag silently break write-combining mappings? When the
driver calls pci_ioremap_wc(), architecture code like x86 PAT checks the
IORESOURCE_PREFETCH flag to permit write-combining. Removing it forces
critical MMIO regions like GPU doorbells to fall back to strict uncacheable
mappings, which might degrade performance.

[Severity: High]
If the 32-bit non-prefetchable window is exhausted, does clearing this flag
prevent a graceful fallback to the prefetchable window? The commit message
states that it will gracefully fall back, but pci_bus_alloc_resource()
strictly forbids allocating a non-prefetching resource from a prefetching
area, which might leave the device unassigned and unusable.

> +		pci_info(dev, "%pR: assigned from the non-prefetchable window to pack the prefetchable window\n",
> +			 r);
> +	}
> +}
> +DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID, quirk_pci_pack_prefetch);
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index e8c94aa1d3c12..aa02b8454269a 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
[ ... ]
> @@ -1343,15 +1392,29 @@ static void pbus_size_mem(struct pci_bus *bus, struct resource *b_res,
>  				aligns[order] += align;
>  			if (order > max_order)
>  				max_order = order;
> +
> +			size += pci_rebar_reserve_size(dev, i, &rebar_align);

[Severity: High]
Could this cause total resource allocation failures? The ReBAR headroom is
being added directly to the size accumulator, which dictates the mandatory
size required for the bridge window. If it were added to children_add_size,
it would dictate the optional reallocation size. If a parent bridge lacks
sufficient contiguous address space, this might cause the entire bridge
window allocation to fail completely.

>  		}
>  	}
>  
>  	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);
> +	min_align = max(min_align, rebar_align);
>  	size0 = calculate_memsize(size, realloc_head ? 0 : add_size,
>  				  0, win_align);
>  
> +	/*
> +	 * A window that reserves ReBAR headroom is larger than its own
> +	 * alignment (e.g. 32 GiB BAR + doorbell => 33 GiB > 32 GiB). The
> +	 * parent only reserves alignment padding for a child window when
> +	 * its size <= its alignment, so round such a window's alignment up
> +	 * to a power of two >= its size; sibling 32 GiB BARs behind one
> +	 * switch then each land in a properly aligned slot.
> +	 */
> +	if (rebar_align && size0)
> +		min_align = max(min_align, roundup_pow_of_two(size0));

[Severity: High]
Does this cause a fatal 64-bit size truncation on 32-bit platforms? The
size0 variable is passed into the roundup_pow_of_two() macro, which only
safely supports unsigned long. On 32-bit platforms with 64-bit physical
addresses, this might assign the bridge window an undersized alignment.

> +
>  	if (size0) {
>  		resource_set_range(b_res, min_align, size0);
>  		b_res->flags &= ~IORESOURCE_DISABLED;
[ ... ]
> +static int pci_release_rebar_bars_cb(struct pci_dev *dev, void *data)
> +{
> +	struct resource *r;
> +	unsigned int i;
> +
> +	if ((dev->class >> 16) != PCI_BASE_CLASS_DISPLAY)
> +		return 0;
> +	if (pci_rebar_get_max_size(dev, 0) <= pci_rebar_get_current_size(dev, 0))

[Severity: Medium]
Does this hardcoded index correctly handle devices that might have a
Resizable BAR at a non-zero index? By explicitly checking only BAR 0, this
might skip headroom reservation for devices like Intel DG2 GPUs that have
their ReBAR on a different index, causing them to fail with -ENOSPC later.

> +		return 0;
> +	pci_dev_for_each_resource(dev, r, i) {
> +		if (i >= PCI_BRIDGE_RESOURCES)
> +			break;
> +		if (r->parent && (r->flags & IORESOURCE_MEM_64) &&
> +		    (r->flags & IORESOURCE_PREFETCH))
> +			pci_release_resource(dev, i);
> +	}
> +	return 0;
> +}
> +
> +/*
> + * Release the now-empty prefetchable bridge windows bottom-up so the sizing
> + * pass re-sizes them (with pci_rebar_pref_reserve() reservation) to fit the
> + * BARs a driver will later grow.
> + */
> +static void pci_release_rebar_windows(struct pci_bus *bus)
> +{
> +	struct pci_dev *dev;
> +	struct resource *w;
> +
> +	list_for_each_entry(dev, &bus->devices, bus_list)
> +		if (dev->subordinate)
> +			pci_release_rebar_windows(dev->subordinate);
> +
> +	if (!bus->self)
> +		return;
> +	w = &bus->self->resource[PCI_BRIDGE_PREF_MEM_WINDOW];
> +	if (w->parent && (w->flags & IORESOURCE_MEM_64) && !w->child)

[Severity: Medium]
Will this permanently pin the upstream bridge window if a non-display device
shares the switch topology? Since pci_release_rebar_bars_cb() restricts BAR
releases strictly to PCI_BASE_CLASS_DISPLAY devices, any non-display sibling
with a prefetchable BAR will retain it. This leaves w->child populated, which
prevents the upstream window from being released and resized.

> +		pci_release_resource(bus->self, PCI_BRIDGE_PREF_MEM_WINDOW);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/5524e9e5-ecac-41c8-b11a-60a85f82fa17@jqluv.com?part=1