[PATCH] PCI: pcie_bus_perf: Allow MRRS above MPS when completions are safe

[PATCH] PCI: pcie_bus_perf: Allow MRRS above MPS when completions are safe

[Kai-Heng Feng]

In PCIE_BUS_PERFORMANCE mode, pcie_write_mrrs() sets MRRS equal to MPS
for all endpoints. This is overly conservative when completions from the
root complex cannot exceed what every receiver on the path can accept.

Completions are sized by the completer up to the root port's MPS. Cap
MRRS when an intermediate bridge or the requester itself has lower MPS
than the root port (e.g. root MPS=512, endpoint MPS=256). Otherwise set
MRRS from pcie_mpss (clamped to 4096), which raises MRRS above MPS on
paths such as Grace where root MPS is 256 and the endpoint MPSS is
larger.

Comparing only ancestor bridge MPS to the endpoint's MPS is not enough:
under top-down MPS configuration every ancestor already has MPS >= the
endpoint, so that check never triggers. Also keep the pcie_set_readrq()
clamp topology-aware; dropping it unconditionally would let drivers
request completions larger than the endpoint can receive on direct-attach
paths. Fail closed for non-PCIe or unidentifiable roots.

Update the pcie_bus_perf description in kernel-parameters.txt to match.

Tested-on: NVIDIA Grace GH200 (lego-cg1-qct-055) with pci=pcie_bus_perf
  ConnectX-7: MPS=256 MPSS=512  MRRS 256 -> 512
  SAS HBA:    MPS=256 MPSS=1024 MRRS 256 -> 1024
  Without pci=pcie_bus_perf: no change vs stock defaults

Link: https://lore.kernel.org/linux-pci/20180123174821.GF5317@bhelgaas-glaptop.roam.corp.google.com/T/
Signed-off-by: Kai-Heng Feng <kaihengf@nvidia.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
---
 .../admin-guide/kernel-parameters.txt         |  9 +-
 drivers/pci/pci.c                             |  8 +-
 drivers/pci/pci.h                             |  1 +
 drivers/pci/probe.c                           | 87 +++++++++++++++++--
 4 files changed, 93 insertions(+), 12 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 3d35270dddef..902bc631b98b 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -5150,9 +5150,12 @@ Kernel parameters
 				supported by all devices below the root complex.
 		pcie_bus_perf	Set device MPS to the largest allowable MPS
 				based on its parent bus. Also set MRRS (Max
-				Read Request Size) to the largest supported
-				value (no larger than the MPS that the device
-				or bus can support) for best performance.
+				Read Request Size) appropriately for best
+				performance: raise it above the device MPS when
+				completions from the root complex cannot exceed
+				what every bridge and the device on the path can
+				accept; otherwise keep MRRS capped to the
+				device MPS.
 		pcie_bus_peer2peer	Set every device's MPS to 128B, which
 				every device is guaranteed to support. This
 				configuration allows peer-to-peer DMA between
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 77b17b13ee61..3e17594e181d 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5845,14 +5845,14 @@ int pcie_set_readrq(struct pci_dev *dev, int rq)
 		return -EINVAL;
 
 	/*
-	 * If using the "performance" PCIe config, we clamp the read rq
-	 * size to the max packet size to keep the host bridge from
-	 * generating requests larger than we can cope with.
+	 * If using the "performance" PCIe config, clamp the read rq size when
+	 * the path cannot safely accept root-sized completions (intermediate
+	 * bridge or this device has lower MPS than the root port).
 	 */
 	if (pcie_bus_config == PCIE_BUS_PERFORMANCE) {
 		int mps = pcie_get_mps(dev);
 
-		if (mps < rq)
+		if (pcie_path_needs_mrrs_cap(dev) && mps < rq)
 			rq = mps;
 	}
 
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 4469e1a77f3c..48572fd5b62d 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -118,6 +118,7 @@ extern struct mutex pci_rescan_remove_lock;
 bool pcie_cap_has_lnkctl(const struct pci_dev *dev);
 bool pcie_cap_has_lnkctl2(const struct pci_dev *dev);
 bool pcie_cap_has_rtctl(const struct pci_dev *dev);
+bool pcie_path_needs_mrrs_cap(struct pci_dev *dev);
 
 /* Standard Capability finder */
 /**
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index dd0abbc63e18..7ab2bd66f3a5 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2961,6 +2961,76 @@ static void pcie_write_mps(struct pci_dev *dev, int mps)
 		pci_err(dev, "Failed attempting to set the MPS\n");
 }
 
+/*
+ * Check whether MRRS must be capped to the device's MPS.
+ *
+ * In PCIE_BUS_PERFORMANCE mode, MPS is configured top-down. Completions are
+ * sized by the completer (root complex) up to the root port's MPS. Every
+ * receiver on the path -- each intermediate bridge and the requester itself --
+ * must accept that completion size, so MRRS must be capped whenever
+ * MPS(root port) exceeds MPS of any of those receivers.
+ *
+ * Comparing only ancestor bridge MPS to the endpoint's MPS is not enough:
+ * under top-down configuration every ancestor already has MPS >= the
+ * endpoint, so that check never triggers. Required checks are:
+ *   - intermediate bridge MPS < root port MPS, and
+ *   - requester MPS < root port MPS (needed by pcie_set_readrq() callers
+ *     that request values larger than MPS on direct-attach paths).
+ *
+ * When the path cannot be characterised (unknown root, non-PCIe bridge),
+ * fail closed and keep the cap.
+ *
+ * Note: this assumes no peer-to-peer DMA, which is the standing assumption for
+ * PCIE_BUS_PERFORMANCE mode.
+ */
+bool pcie_path_needs_mrrs_cap(struct pci_dev *dev)
+{
+	struct pci_bus *bus = dev->bus;
+	int root_mps = -1;
+	int min_inter_mps = -1;
+
+	while (bus->parent) {
+		struct pci_dev *bridge = bus->self;
+
+		if (bridge) {
+			int mps;
+
+			if (!pci_is_pcie(bridge))
+				return true;	/* unknown; keep the cap */
+
+			mps = pcie_get_mps(bridge);
+
+			/*
+			 * The bridge directly above the root bus is treated as
+			 * the root port. Everything between the device and
+			 * that bridge must forward completion TLPs.
+			 */
+			if (bus->parent->parent) {
+				if (min_inter_mps < 0 || mps < min_inter_mps)
+					min_inter_mps = mps;
+			} else {
+				root_mps = mps;
+			}
+		}
+		bus = bus->parent;
+	}
+
+	/* No identifiable root port MPS: keep the conservative cap. */
+	if (root_mps < 0)
+		return true;
+
+	if (min_inter_mps >= 0 && min_inter_mps < root_mps)
+		return true;
+
+	/*
+	 * Direct-attach and uniform-vs-root paths still need a cap when this
+	 * device's own MPS is below the root port's -- otherwise drivers
+	 * calling pcie_set_readrq() with a large value can request completions
+	 * larger than the requester can receive.
+	 */
+	return pcie_get_mps(dev) < root_mps;
+}
+
 static void pcie_write_mrrs(struct pci_dev *dev)
 {
 	int rc, mrrs;
@@ -2973,12 +3043,19 @@ static void pcie_write_mrrs(struct pci_dev *dev)
 		return;
 
 	/*
-	 * For max performance, the MRRS must be set to the largest supported
-	 * value.  However, it cannot be configured larger than the MPS the
-	 * device or the bus can support.  This should already be properly
-	 * configured by a prior call to pcie_write_mps().
+	 * Prefer a larger MRRS when completions from the root complex cannot
+	 * exceed what every receiver on the path (bridges and this device)
+	 * can accept. Otherwise cap to the device's MPS.
+	 *
+	 * There is no Max_Read_Request_Size capability field; pcie_mpss is
+	 * used as a conservative upper bound (clamped to the architectural
+	 * 4096B MRRS maximum). Hardware that rejects the value is handled by
+	 * the shrink loop below.
 	 */
-	mrrs = pcie_get_mps(dev);
+	if (pcie_path_needs_mrrs_cap(dev))
+		mrrs = pcie_get_mps(dev);
+	else
+		mrrs = min(128 << dev->pcie_mpss, 4096);
 
 	/*
 	 * MRRS is a R/W register.  Invalid values can be written, but a
-- 
2.50.1 (Apple Git-155)

Re: [PATCH] PCI: pcie_bus_perf: Allow MRRS above MPS when completions are safe

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The topology walk in `pcie_path_needs_mrrs_cap()` fails to traverse across nested PCIe domains like Intel VMD, allowing MRRS to be unsafely raised and causing dropped Completion TLPs.

Pre-existing issues:
- [High] In PCIE_BUS_PERFORMANCE mode, devices on a virtual root bus (like Intel VMD) are assigned an MPS larger than the physical host endpoint can handle, leading to dropped Memory Writes.
--

--- Patch [1]: [PATCH] PCI: pcie_bus_perf: Allow MRRS above MPS when completions are safe ---
commit bf577b720721bb4bea4fbd0d9d55339ceaede57f
Author: Kai-Heng Feng <kaihengf@nvidia.com>

PCI: pcie_bus_perf: Allow MRRS above MPS when completions are safe

In PCIE_BUS_PERFORMANCE mode, pcie_write_mrrs() sets MRRS equal to MPS
for all endpoints. This patch allows MRRS to exceed MPS when it is safe
to do so based on root complex completion sizing.

Link: https://lore.kernel.org/linux-pci/20180123174821.GF5317@bhelgaas-glaptop.roam.corp.google.com/T/

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 77b17b13ee615..3e17594e181d9 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5845,14 +5845,14 @@ int pcie_set_readrq(struct pci_dev *dev, int rq)
>  		return -EINVAL;
>  
>  	/*
> -	 * If using the "performance" PCIe config, we clamp the read rq
> -	 * size to the max packet size to keep the host bridge from
> -	 * generating requests larger than we can cope with.
> +	 * If using the "performance" PCIe config, clamp the read rq size when
> +	 * the path cannot safely accept root-sized completions (intermediate
> +	 * bridge or this device has lower MPS than the root port).
>  	 */
>  	if (pcie_bus_config == PCIE_BUS_PERFORMANCE) {
>  		int mps = pcie_get_mps(dev);
>  
> -		if (mps < rq)
> +		if (pcie_path_needs_mrrs_cap(dev) && mps < rq)

[Severity: High]
Will this check bypass the clamp for nested PCIe domains like Intel VMD?

If pcie_path_needs_mrrs_cap() terminates its walk at the virtual root bus
because bus->parent is NULL, it will evaluate as safe. 

Could a driver calling pcie_set_readrq() then bypass the clamp and unsafely
raise MRRS above the physical host path's MPS?

>  			rq = mps;
>  	}

> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index dd0abbc63e18d..7ab2bd66f3a5f 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2961,6 +2961,76 @@ static void pcie_write_mps(struct pci_dev *dev, int mps)
>  		pci_err(dev, "Failed attempting to set the MPS\n");
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does the top-down
configuration in pcie_write_mps() fail across nested PCI domains?

Looking at pcie_write_mps(), virtual root ports bypass the inheritance check:

drivers/pci/probe.c:pcie_write_mps() {
    ...
    if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT &&
        dev->bus->self)
            mps = min(mps, pcie_get_mps(dev->bus->self));
    ...
}

For a virtual root port in a VMD domain, pci_pcie_type(dev) equals
PCI_EXP_TYPE_ROOT_PORT, bypassing this inheritance and getting configured
with its max MPSS (e.g., 512) while ignoring the physical host topology.

If the physical VMD endpoint on the host bus has an MPS of 256, could an
inner device inherit an MPS of 512? When the inner device transmits a
512-byte Memory Write, will the physical VMD endpoint drop it as a
Malformed TLP since it exceeds its primary side MPS?

> +/*
> + * Check whether MRRS must be capped to the device's MPS.
[ ... ]
> + */
> +bool pcie_path_needs_mrrs_cap(struct pci_dev *dev)
> +{
> +	struct pci_bus *bus = dev->bus;
> +	int root_mps = -1;
> +	int min_inter_mps = -1;
> +
> +	while (bus->parent) {

[Severity: High]
Does this topology walk fail to traverse across nested PCIe domains?

The loop relies on bus->parent to reach the Root Port. For nested domains
like Intel VMD, the virtual root bus has bus->parent == NULL, terminating
the walk prematurely and ignoring the physical host topology.

> +		struct pci_dev *bridge = bus->self;
[ ... ]
> @@ -2973,12 +3043,19 @@ static void pcie_write_mrrs(struct pci_dev *dev)
[ ... ]
> -	mrrs = pcie_get_mps(dev);
> +	if (pcie_path_needs_mrrs_cap(dev))
> +		mrrs = pcie_get_mps(dev);
> +	else
> +		mrrs = min(128 << dev->pcie_mpss, 4096);

[Severity: High]
If pcie_path_needs_mrrs_cap() evaluates the virtual domain as safe, could
this unsafely raise MRRS above the physical host path's MPS?

If the VMD physical endpoint on the host bus has an MPS of 256 but the
inner device's MRRS is raised to 512, will the host Root Complex respond
with a 512-byte Completion TLP that the VMD endpoint drops as a Malformed
TLP?

>  	/*
>  	 * MRRS is a R/W register.  Invalid values can be written, but a

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814134547.23148-1-kaihengf@nvidia.com?part=1