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

[sashiko-bot@kernel.org]

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