[PATCH v2] PCI: Fix Intel Xeon 6 x2 quirk collateral damage on adjacent x4 endpoints

[PATCH v2] PCI: Fix Intel Xeon 6 x2 quirk collateral damage on adjacent x4 endpoints

[Rick Warner]

commit a22250fe933d ("PCI: Add Extended Tag + MRRS quirk for Xeon 6")
introduced a traffic mitigation quirk for Intel Xeon 6 root ports that
negotiate down to an x2 lane width. However, that patch manipulated host
bridge properties globally via 'bridge->no_ext_tags = 1' and by hooking
'bridge->enable_device'.

Because multiple unrelated root ports can reside under the exact same
global pci_host_bridge domain structure, this overly aggressive mitigation
causes severe collateral damage. When a low-speed or bifurcated secondary
device (such as an onboard ASMedia SATA controller or BMC graphics link)
matches the x2 condition, the kernel strips away Extended Tags and forces
a 128B MRRS restriction across that ENTIRE host bridge. This instantly
breaks or starves adjacent high-performance, unrelated x4 endpoints
(such as NVMe drives), resulting in controller timeouts, initialization
failures, and missing drives at boot.

Fix this by refactoring the quirk logic to be completely per-device and
downstream-isolated. Remove the broad host bridge no_ext_tags and
enable_device = limit_mrrs_to_128 assignments, instead setting only
no_inc_mrrs = 1 to prevent increases from vfio, VM, etc usage.

DECLARE_PCI_FIXUP_HEADER then registers pci_xeon6_x2_local_endpoint_fixup
to check every pci device as it initializes to see if it is downstream of
an affected x2 port, and if so, the device has extended tags disabled and
mrrs set to 128. This enforces Intel's stability parameters locally on the
bottlenecked lane branches while fully protecting the parallel x4 channels.

Fixes: a22250fe933d ("PCI: Add Extended Tag + MRRS quirk for Xeon 6")
Signed-off-by: Rick Warner <rick@microway.com>
---
v2
Original submission did a single pci bus walk and didn't handle hot
plugged devices.  This has been rewritten to handle the quirk per
device during initialization by checking if it's a descendant of a x2 port.

This was tested successfully on a Gigabyte MS74-HB0 motherboard with a
Seagate ZP4000GM30063 M.2 drive in the 2nd slot. With the stock 7.0 kernel,
the drive fails to initialize and is unavailable once booted. Testing showed
that extended tags (vs mrrs) are the key to getting this drive working.

Concerns were raised about vfio/VM usage being able to re-enable extended 
tags if bridge->no_ext_tags is not set.  I'm not sure how to best address 
that if it's needed. The best option I've come up with is adding
DECLARE_PCI_FIXUP_FINAL calls for 0x0db0-0xdb9 that set no_ext_tags after
the initial pci bus walk is done. That would enable the already 
initialized devices to maintain their configured extended tag support 
but would block all newly hotplugged devices on that bridge from using
extended tags, even if they weren't downstream of a x2 port. That might be
good enough, as it would at least solve this issue for NVME drives not
initializing. Otherwise I think bigger changes might be required to 
handle it more cleanly.

 arch/x86/pci/fixup.c | 58 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 46 insertions(+), 12 deletions(-)

diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
index b301c6c8df75..0ab92a5cc0d1 100644
--- a/arch/x86/pci/fixup.c
+++ b/arch/x86/pci/fixup.c
@@ -301,15 +301,6 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_MCH_PC1,	pcie_r
  *
  * https://cdrdv2.intel.com/v1/dl/getContent/837176
  */
-static int limit_mrrs_to_128(struct pci_host_bridge *b, struct pci_dev *pdev)
-{
-	int readrq = pcie_get_readrq(pdev);
-
-	if (readrq > 128)
-		pcie_set_readrq(pdev, 128);
-
-	return 0;
-}
 
 static void pci_xeon_x2_bifurc_quirk(struct pci_dev *pdev)
 {
@@ -320,9 +311,8 @@ static void pci_xeon_x2_bifurc_quirk(struct pci_dev *pdev)
 	if (FIELD_GET(PCI_EXP_LNKCAP_MLW, linkcap) != 0x2)
 		return;
 
-	bridge->no_ext_tags = 1;
-	bridge->enable_device = limit_mrrs_to_128;
-	pci_info(pdev, "Disabling Extended Tags and limiting MRRS to 128B (performance reasons due to x2 PCIe link)\n");
+	bridge->no_inc_mrrs = 1;
+	pci_info(pdev, "Blocking devices on this bridge from increasing MRRS for performance reasons due to x2 PCIe link)\n");
 }
 
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x0db0, pci_xeon_x2_bifurc_quirk);
@@ -334,6 +324,50 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x0db7, pci_xeon_x2_bifurc_quirk);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x0db8, pci_xeon_x2_bifurc_quirk);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x0db9, pci_xeon_x2_bifurc_quirk);
 
+/* Helper to check if a device descends from an affected Xeon 6 x2 Root Port */
+static bool is_descendant_of_xeon6_x2_rp(struct pci_dev *pdev)
+{
+	u32 linkcap;
+	struct pci_dev *upstream = pci_upstream_bridge(pdev);
+
+	while (upstream) {
+		if (upstream->vendor == PCI_VENDOR_ID_INTEL &&
+		    upstream->device >= 0x0db0 &&
+		    upstream->device <= 0x0db9) {
+			pcie_capability_read_dword(upstream, PCI_EXP_LNKCAP, &linkcap);
+			if (FIELD_GET(PCI_EXP_LNKCAP_MLW, linkcap) != 0x2)
+				return false;
+			else
+				return true;
+		}
+		upstream = pci_upstream_bridge(upstream);
+	}
+	return false;
+}
+
+static void pci_xeon6_x2_local_endpoint_fixup(struct pci_dev *pdev)
+{
+	/* Skip bridges/switches; only target actual endpoints */
+	if (pci_is_bridge(pdev))
+		return;
+
+	/* Only apply to devices under the x2 branch; leaves x4 branches completely untouched */
+	if (!is_descendant_of_xeon6_x2_rp(pdev))
+		return;
+
+	pci_info(pdev, "Applying local Xeon 6 x2 quirk: Disabling Extended Tags and locking MRRS to 128B\n");
+
+	pcie_capability_clear_word(pdev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_EXT_TAG);
+	pcie_capability_clear_word(pdev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_READRQ);
+}
+
+/*
+ * HEADER fixups run for EVERY endpoint during its initial discovery phase.
+ * This natively catches boot devices, hotplugged devices, and SR-IOV VFs.
+ */
+DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID, pci_xeon6_x2_local_endpoint_fixup);
+
+
 /*
  * Fixup to mark boot BIOS video selected by BIOS before it changes
  *
-- 
2.52.0

Re: [PATCH v2] PCI: Fix Intel Xeon 6 x2 quirk collateral damage on adjacent x4 endpoints

[sashiko-bot]

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

New issues:
- [High] The refactored quirk attempts to isolate the x2 mitigation but fails because it applies `no_inc_mrrs = 1` globally to the entire host bridge, still penalizing parallel x4 branches.
- [High] The local fixup to disable Extended Tags on x2 endpoints is immediately overwritten and undone by the PCI core during device enumeration.
- [Medium] The local endpoint fixup unlawfully modifies RsvdP (Reserved and Preserved) PCIe capabilities on SR-IOV Virtual Functions (VFs).
- [Low] Unmatched closing parenthesis in the `pci_info` log message.

Pre-existing issues:
- [Critical] A guest VM can bypass the quirk and re-enable Extended Tags via VFIO, leading to hardware instability and host Denial of Service.
--

--- Patch [1]: [PATCH v2] PCI: Fix Intel Xeon 6 x2 quirk collateral damage on adjacent x4 endpoints ---
commit d621c370d145fdc2f0765dea4356628c461c0bda
Author: Rick Warner <rick@microway.com>

PCI: Fix Intel Xeon 6 x2 quirk collateral damage on adjacent x4 endpoints

This commit attempts to fix collateral damage caused by the Xeon 6 x2 quirk by
changing the mitigation to be per-device instead of global to the host bridge.
It removes the global bridge->no_ext_tags flag and uses a local endpoint fixup
to disable extended tags and lock MRRS to 128B for devices downstream of an
affected x2 root port.

> diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
> index b301c6c8df753..6370b08d99aef 100644
> --- a/arch/x86/pci/fixup.c
> +++ b/arch/x86/pci/fixup.c
> @@ -320,9 +311,8 @@ static void pci_xeon_x2_bifurc_quirk(struct pci_dev *pdev)
>  	if (FIELD_GET(PCI_EXP_LNKCAP_MLW, linkcap) != 0x2)
>  		return;
>  
> -	bridge->no_ext_tags = 1;
> -	bridge->enable_device = limit_mrrs_to_128;
> -	pci_info(pdev, "Disabling Extended Tags and limiting MRRS to 128B (performance reasons due to x2 PCIe link)\n");
> +	bridge->no_inc_mrrs = 1;

[Severity: High]
Does setting bridge->no_inc_mrrs = 1 here still apply the restriction globally
to the entire host bridge?

When pcie_set_readrq() is called by unrelated x4 devices sharing this bridge, it
will check this flag:

drivers/pci/pci.c:pcie_set_readrq() {
    ...
	if (bridge->no_inc_mrrs) {
    ...
}

Would this cause the MRRS limitation to still inadvertently impact the parallel
x4 branches that this patch intends to protect?

> +	pci_info(pdev, "Blocking devices on this bridge from increasing MRRS for performance reasons due to x2 PCIe link)\n");

[Severity: Low]
There appears to be an unmatched closing parenthesis at the end of this log
message string.

[ ... ]

> @@ -334,6 +324,50 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x0db7, pci_xeon_x2_bifurc_quirk);
> +static void pci_xeon6_x2_local_endpoint_fixup(struct pci_dev *pdev)
> +{
> +	/* Skip bridges/switches; only target actual endpoints */
> +	if (pci_is_bridge(pdev))
> +		return;
> +
> +	/* Only apply to devices under the x2 branch; leaves x4 branches completely untouched */
> +	if (!is_descendant_of_xeon6_x2_rp(pdev))
> +		return;
> +
> +	pci_info(pdev, "Applying local Xeon 6 x2 quirk: Disabling Extended Tags and locking MRRS to 128B\n");
> +
> +	pcie_capability_clear_word(pdev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_EXT_TAG);

[Severity: High]
Will the clearing of the EXT_TAG bit here be undone by the PCI core later in
the boot process?

Because the global bridge->no_ext_tags flag was removed earlier in the patch,
when the PCI core later evaluates extended tags during pci_device_add(), it
will encounter this in pci_configure_extended_tags():

drivers/pci/probe.c:pci_configure_extended_tags() {
    ...
	if (!(ctl & PCI_EXP_DEVCTL_EXT_TAG)) {
		pci_info(dev, "enabling Extended Tags\n");
		pcie_capability_set_word(dev, PCI_EXP_DEVCTL,
					 PCI_EXP_DEVCTL_EXT_TAG);
	}
    ...
}

Could this unconditionally re-enable Extended Tags on the endpoint and defeat
the mitigation?

[Severity: Critical]
This is a pre-existing issue, but since the commit message mentions concerns
about vfio/VM usage re-enabling extended tags, is it possible for a guest VM
to bypass the quirk?

Looking at init_pci_cap_exp_perm() in drivers/vfio/pci/vfio_pci_config.c,
PCI_EXP_DEVCTL_EXT_TAG is not included in the NO_WRITE mask:

drivers/vfio/pci/vfio_pci_config.c:init_pci_cap_exp_perm() {
    ...
	p_setw(perm, PCI_EXP_DEVCTL,
	       PCI_EXP_DEVCTL_BCR_FLR | PCI_EXP_DEVCTL_PAYLOAD |
	       PCI_EXP_DEVCTL_READRQ | PCI_EXP_DEVCTL_PHANTOM, NO_WRITE);
    ...
}

Would this allow an unprivileged guest VM to write to its virtualized
PCI_EXP_DEVCTL register and have the write pass through to the physical
hardware, thereby re-enabling Extended Tags?

> +	pcie_capability_clear_word(pdev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_READRQ);

[Severity: Medium]
Does this code modify the PCI_EXP_DEVCTL register on SR-IOV Virtual Functions?

According to the PCIe SR-IOV Specification, Extended Tag Enable and
Max_Read_Request_Size are RsvdP (Reserved and Preserved) fields on VFs and must
not be modified, as they track the PF.

Should there be a check for !pdev->is_virtfn before clearing these bits?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804204845.171483-1-rick@microway.com?part=1