Linux PCI subsystem development
 help / color / mirror / Atom feed
From: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
To: Maciej Grochowski <maciej.grochowski@sony.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>,
	linux-pci@vger.kernel.org,  Jonathan Corbet <corbet@lwn.net>
Subject: Re: [RFC PATCH 1/2] PCI: Add pci=hpreserve= to reserve memory windows for a hotplug bridge
Date: Mon, 5 Oct 2026 11:39:53 +0300 (EEST)	[thread overview]
Message-ID: <8b41ee8a-8138-cfe7-8e58-19de2f1baee9@linux.intel.com> (raw)
In-Reply-To: <20261002193111.51637-2-maciej.grochowski@sony.com>

On Fri, 2 Oct 2026, Maciej Grochowski wrote:

> A hotplug bridge is sized for the devices that are present when its
> windows are sized, plus the optional hpmmiosize/hpmmioprefsize space.
> Some topologies have devices that are known to appear later.  After a
> reset of a two-level PCIe switch hierarchy, for example, pciehp
> re-enumerates the upstream switch before the link to the nested switch
> is back up, so the downstream port leading to the nested switch is
> sized with nothing below it.  When the nested switch appears, its BARs
> do not fit.
> 
> On a two-level Microchip PM50052 (Switchtec PFX) hierarchy whose nested
> switch has an NTB function with a 1 GiB prefetchable BAR, a hard reset
> of the top-level switch on v7.3-rc2 leaves downstream port 82:03.0
> with a 206 MiB prefetchable window, an equal share of its
> parent's window.  The nested switch returns about 0.6 s later and
> fails:
> 
>   pci 0000:8c:00.1: BAR 2 [mem size 0x40000000 64bit pref]: can't assign; no space
>   switchtec 0000:8c:00.1: probe with driver switchtec failed with error -16
> 
> The global hpmmioprefsize cannot express this.  It applies to every
> hotplug bridge, it is optional space that is dropped when it does not
> fit, and it has no alignment.  Here the root port window is 1036 MiB:
> exactly the 1 GiB + 4 MiB + 4 MiB of the nested switch plus the 4 MiB
> BAR of the top switch's management function.  Only one port may get
> the space, and it has to be 1 GiB aligned.
> 
> Add "pci=hpreserve=<reservation>@<pci_dev>[; ...]" to request a minimum

Hi,

Why is this a new parameter instead of adding the per device functionality 
into hpmmio{,pref}size=x ?

> size and alignment for the memory windows of specific hotplug bridges,
> using the usual device specification.  A full path is more robust than
> a bare BDF when downstream bus numbers change.  pbus_size_mem() folds
> the reservation into the required size and alignment of the window,
> so it is assigned like the resources of a device already present below
> the bridge.  The runtime distribution of spare space can shrink only
> the optional part, leaving the required reservation intact.
> 
> Without the parameter nothing changes.
> 
> Tested-by: Maciej Grochowski <maciej.grochowski@sony.com>

The usual assumption the patch submitter has actually tested the patch so 
normal custom is to not include self tested-bys.

-- 
 i.

> Signed-off-by: Maciej Grochowski <maciej.grochowski@sony.com>
> ---
>  .../admin-guide/kernel-parameters.txt         |  34 ++++++
>  drivers/pci/pci.c                             | 106 ++++++++++++++++++
>  drivers/pci/pci.h                             |  19 ++++
>  drivers/pci/setup-bus.c                       |  33 ++++++
>  4 files changed, 192 insertions(+)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 33cd30996e47..19b98e22b10e 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -5232,6 +5232,40 @@ Kernel parameters
>  		hpbussize=nn	The minimum amount of additional bus numbers
>  				reserved for buses below a hotplug bridge.
>  				Default is 1.
> +		hpreserve=
> +				Format:
> +				<reservation>@<pci_dev>[; ...]
> +				where <reservation> is one or more of
> +				<key>=<value> separated by colons.
> +				Reserve resources for the hotplug bridges
> +				specified (in the format described above)
> +				for devices that are not present yet, e.g.
> +				a switch that is expected behind the bridge
> +				but whose link comes up only after the
> +				bridge windows have been sized.  Unlike
> +				hpmmiosize and hpmmioprefsize, which add
> +				optional space that is dropped if it does
> +				not fit, the reservation is required space:
> +				the bridge window is sized to include it,
> +				like the resources of devices already
> +				present below the bridge.
> +				Only hotplug bridges are affected.
> +				Keys:
> +				  mmio=nn[KMG]	Minimum size of the
> +					non-prefetchable memory window.
> +				  mmioalign=nn[KMG]  Minimum alignment of
> +					the non-prefetchable memory window.
> +				  mmiopref=nn[KMG]  Minimum size of the
> +					prefetchable memory window.
> +				  mmioprefalign=nn[KMG]  Minimum alignment
> +					of the prefetchable memory window.
> +				Alignments must be powers of two.  For
> +				example,
> +				  pci=hpreserve=mmiopref=1032M:mmioprefalign=1G@0000:80:01.1/00.0/03.0
> +				reserves a 1 GiB-aligned 1032 MiB
> +				prefetchable window below the downstream
> +				port at devfn 03.0 of the switch behind root
> +				port 0000:80:01.1.
>  		realloc=	Enable/disable reallocating PCI bridge resources
>  				if allocations done by BIOS are too small to
>  				accommodate resources required by all child
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b2879a6be5f8..f6922cf0d96e 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -412,6 +412,109 @@ static int pci_dev_str_match(struct pci_dev *dev, const char *p,
>  	return 1;
>  }
>  
> +/*
> + * "pci=hpreserve=<reservation>@<pci_dev>[; ...]", set in pci_setup() and
> + * copied in pci_realloc_setup_params().
> + */
> +static const char *hotplug_reserve_param;
> +
> +static bool pci_hotplug_reserve_key(const char *p, size_t len, const char *key)
> +{
> +	return strlen(key) == len && !strncmp(p, key, len);
> +}
> +
> +/*
> + * Parse one "<key>=<value>[:<key>=<value>]*@" reservation into @res.
> + * Returns a pointer to the device specification following the '@', or
> + * NULL if the reservation cannot be parsed.
> + */
> +static const char *pci_parse_hotplug_reserve(const char *p,
> +					     struct pci_hotplug_reserve *res)
> +{
> +	unsigned long long val;
> +	size_t len;
> +	char *end;
> +
> +	memset(res, 0, sizeof(*res));
> +
> +	for (;;) {
> +		len = strcspn(p, "=:@;");
> +		if (p[len] != '=')
> +			return NULL;
> +
> +		val = memparse(p + len + 1, &end);
> +		if (end == p + len + 1)
> +			return NULL;
> +
> +		if (pci_hotplug_reserve_key(p, len, "mmio"))
> +			res->mmio_size = val;
> +		else if (pci_hotplug_reserve_key(p, len, "mmioalign"))
> +			res->mmio_align = val;
> +		else if (pci_hotplug_reserve_key(p, len, "mmiopref"))
> +			res->mmio_pref_size = val;
> +		else if (pci_hotplug_reserve_key(p, len, "mmioprefalign"))
> +			res->mmio_pref_align = val;
> +		else
> +			return NULL;
> +
> +		p = end + 1;
> +		if (*end == '@')
> +			break;
> +		if (*end != ':')
> +			return NULL;
> +	}
> +
> +	if ((res->mmio_align && !is_power_of_2(res->mmio_align)) ||
> +	    (res->mmio_pref_align && !is_power_of_2(res->mmio_pref_align)))
> +		return NULL;
> +
> +	return p;
> +}
> +
> +/**
> + * pci_get_hotplug_reserve - get the resources reserved for a hotplug bridge
> + * @bridge: the bridge
> + * @res: filled in with the reservation for @bridge
> + *
> + * Look up @bridge in the "pci=hpreserve=" kernel parameter.  Only hotplug
> + * bridges are considered.
> + *
> + * Return: true if a reservation was requested for @bridge.
> + */
> +bool pci_get_hotplug_reserve(struct pci_dev *bridge,
> +			     struct pci_hotplug_reserve *res)
> +{
> +	const char *p = hotplug_reserve_param;
> +	int ret;
> +
> +	if (!p || !bridge->is_hotplug_bridge)
> +		return false;
> +
> +	while (*p) {
> +		p = pci_parse_hotplug_reserve(p, res);
> +		if (!p) {
> +			pr_err_once("PCI: Can't parse hpreserve parameter\n");
> +			return false;
> +		}
> +
> +		ret = pci_dev_str_match(bridge, p, &p);
> +		if (ret == 1)
> +			return true;
> +		if (ret < 0) {
> +			pr_err_once("PCI: Can't parse hpreserve parameter\n");
> +			return false;
> +		}
> +
> +		if (*p != ';' && *p != ',') {
> +			/* End of param or invalid format */
> +			break;
> +		}
> +		p++;
> +	}
> +
> +	return false;
> +}
> +
>  static u8 __pci_find_next_cap(struct pci_bus *bus, unsigned int devfn,
>  			      u8 pos, int cap)
>  {
> @@ -6840,6 +6943,8 @@ static int __init pci_setup(char *str)
>  					simple_strtoul(str + 10, &str, 0);
>  				if (pci_hotplug_bus_size > 0xff)
>  					pci_hotplug_bus_size = DEFAULT_HOTPLUG_BUS_SIZE;
> +			} else if (!strncmp(str, "hpreserve=", 10)) {
> +				hotplug_reserve_param = str + 10;
>  			} else if (!strncmp(str, "pcie_bus_tune_off", 17)) {
>  				pcie_bus_config = PCIE_BUS_TUNE_OFF;
>  			} else if (!strncmp(str, "pcie_bus_safe", 13)) {
> @@ -6879,6 +6984,7 @@ static int __init pci_realloc_setup_params(void)
>  					   GFP_KERNEL);
>  	disable_acs_redir_param = kstrdup(disable_acs_redir_param, GFP_KERNEL);
>  	config_acs_param = kstrdup(config_acs_param, GFP_KERNEL);
> +	hotplug_reserve_param = kstrdup(hotplug_reserve_param, GFP_KERNEL);
>  
>  	return 0;
>  }
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index ba3c3fddddc2..5003d2c71200 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -410,6 +410,25 @@ extern unsigned long pci_hotplug_mmio_size;
>  extern unsigned long pci_hotplug_mmio_pref_size;
>  extern unsigned long pci_hotplug_bus_size;
>  
> +/**
> + * struct pci_hotplug_reserve - resources reserved for a hotplug bridge
> + * @mmio_size: minimum size of the non-prefetchable memory window
> + * @mmio_align: minimum alignment of the non-prefetchable memory window
> + * @mmio_pref_size: minimum size of the prefetchable memory window
> + * @mmio_pref_align: minimum alignment of the prefetchable memory window
> + *
> + * Requested with "pci=hpreserve=".  A value of zero means no reservation.
> + */
> +struct pci_hotplug_reserve {
> +	resource_size_t mmio_size;
> +	resource_size_t mmio_align;
> +	resource_size_t mmio_pref_size;
> +	resource_size_t mmio_pref_align;
> +};
> +
> +bool pci_get_hotplug_reserve(struct pci_dev *bridge,
> +			     struct pci_hotplug_reserve *res);
> +
>  static inline bool pci_is_cardbus_bridge(struct pci_dev *dev)
>  {
>  	return dev->hdr_type == PCI_HEADER_TYPE_CARDBUS;
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index e8c94aa1d3c1..105bf8694cdc 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -1258,6 +1258,38 @@ static bool pbus_size_mem_optional(struct pci_dev *dev, int resno,
>  	return true;
>  }
>  
> +/*
> + * Apply a "pci=hpreserve=" reservation to the memory window @b_res of the
> + * bridge leading to @bus.  Unlike the hpmmiosize/hpmmioprefsize space, the
> + * reservation is sized as required space, so it is in place before the
> + * devices that need it appear behind the bridge.
> + */
> +static void pbus_size_mem_reserve(struct pci_bus *bus, struct resource *b_res,
> +				  resource_size_t *size,
> +				  resource_size_t *min_align)
> +{
> +	bool pref = b_res->flags & IORESOURCE_PREFETCH;
> +	struct pci_hotplug_reserve res;
> +	resource_size_t rsize, ralign;
> +
> +	if (!bus->self || !pci_get_hotplug_reserve(bus->self, &res))
> +		return;
> +
> +	rsize = pref ? res.mmio_pref_size : res.mmio_size;
> +	ralign = pref ? res.mmio_pref_align : res.mmio_align;
> +	if (rsize <= *size && ralign <= *min_align)
> +		return;
> +
> +	*size = max(*size, rsize);
> +	if (!*size)
> +		return;
> +	*min_align = max(*min_align, ralign);
> +
> +	pci_info(bus->self, "bridge window to %pR: reserving %s size %#llx align %#llx\n",
> +		 &bus->busn_res, pref ? "prefetchable" : "non-prefetchable",
> +		 (unsigned long long)*size, (unsigned long long)*min_align);
> +}
> +
>  /**
>   * pbus_size_mem() - Size the memory window of a given bus
>   *
> @@ -1349,6 +1381,7 @@ static void pbus_size_mem(struct pci_bus *bus, struct resource *b_res,
>  	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);
> +	pbus_size_mem_reserve(bus, b_res, &size, &min_align);
>  	size0 = calculate_memsize(size, realloc_head ? 0 : add_size,
>  				  0, win_align);
>  
> 

  parent reply	other threads:[~2026-10-05  8:40 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-10-02 19:31 [RFC PATCH 0/2] PCI: Reserve resources for a delayed hotplug subtree Maciej Grochowski
2026-10-02 19:31 ` [RFC PATCH 1/2] PCI: Add pci=hpreserve= to reserve memory windows for a hotplug bridge Maciej Grochowski
2026-10-03  1:33   ` sashiko-bot
2026-10-05  8:39   ` Ilpo Järvinen [this message]
2026-10-07 13:35     ` Maciej Grochowski
2026-10-02 19:31 ` [RFC PATCH 2/2] PCI: Allow pci=hpreserve= to reserve bus numbers " Maciej Grochowski
2026-10-03  1:33   ` sashiko-bot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8b41ee8a-8138-cfe7-8e58-19de2f1baee9@linux.intel.com \
    --to=ilpo.jarvinen@linux.intel.com \
    --cc=bhelgaas@google.com \
    --cc=corbet@lwn.net \
    --cc=linux-pci@vger.kernel.org \
    --cc=maciej.grochowski@sony.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox