[PATCH v3 1/1] PCI: Add Extended Tag + MRRS quirk for Xeon 6

[PATCH v3 1/1] PCI: Add Extended Tag + MRRS quirk for Xeon 6

[Ilpo Järvinen]

When bifurcated to x2, Xeon 6 Root Port performance is sensitive to the
configuration of Extended Tags, Max Read Request Size (MRRS), and 10-Bit
Tag Requester (note: there is currently no 10-Bit Tag support in the
kernel). While those can be configured to the recommended values by FW,
kernel may decide to overwrite the initial values.

Add a quirk that disallows enabling Extended Tags and setting MRRS
larger than 128B for devices under Xeon 6 Root Ports if the Root Port
is bifurcated to x2. Use the host bridge's enable_device hook to
overwrite MRRS if it's set to >128B for the device to be enabled.

The earlier attempts to implement this quirk polluted PCI core code
with the checks necessary to support this quirk. Using the
enable_device hook keeps the quirk well-contained, away from the PCI
core code.

Suggested-by: Lukas Wunner <lukas@wunner.de>
Link: https://cdrdv2.intel.com/v1/dl/getContent/837176
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---

Ingo gave his Ack on v2 but since the used approach has once again
changed, I didn't add his Ack.

Mani did express his concern on using enable_device hook but suggested
I send the patch anyway.

We're also looking into using _HPX Type 3 (suggested by Bjorn) eventually
for this class of problems where FW settings get overwritten by the
kernel (but it will take time to make it the sanctioned solution). In
the meantime, this is a real problem for Xeon 6 out there so it warrants
adding the quirk (which is now pretty well-contained).

v3:
- Use enable_device hook to overwrite MRRS to 128B if needed. (Lukas)
- Typo fix to comment (Ingo)

v2:
- Explain in changelog why FW cannot solve this on its own
- Moved the quirk under arch/x86/pci/
- Don't NULL check value from pci_find_host_bridge()
- Added comment above the quirk about the performance degradation
- Removed all setup chain 128B quirk overrides expect for MRRS write
  itself (assumes a sane initial value is set by FW)


 arch/x86/pci/fixup.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
index e7e71490bd25..38369124de45 100644
--- a/arch/x86/pci/fixup.c
+++ b/arch/x86/pci/fixup.c
@@ -294,6 +294,46 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_MCH_PB1,	pcie_r
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_MCH_PC,	pcie_rootport_aspm_quirk);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_MCH_PC1,	pcie_rootport_aspm_quirk);
 
+/*
+ * PCIe devices underneath Xeon6 PCIe Root Port bifurcated to 2x have slower
+ * performance with Extended Tags and MRRS > 128B. Work around the performance
+ * problems by disabling Extended Tags and limiting MRRS to 128B.
+ *
+ * https://cdrdv2.intel.com/v1/dl/getContent/837176
+ */
+static int limit_mrrs_to_128(struct pci_host_bridge *bridge, struct pci_dev *pdev)
+{
+	int readrq = pcie_get_readrq(pdev);
+
+	if (readrq > 128)
+		pcie_set_readrq(pdev, 128);
+
+	return 0;
+}
+
+static void quirk_pcie2x_no_tags_no_mrrs(struct pci_dev *pdev)
+{
+	struct pci_host_bridge *bridge = pci_find_host_bridge(pdev->bus);
+	u32 linkcap;
+
+	pcie_capability_read_dword(pdev, PCI_EXP_LNKCAP, &linkcap);
+	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 2x PCIe link)\n");
+}
+
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x0db0, quirk_pcie2x_no_tags_no_mrrs);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x0db1, quirk_pcie2x_no_tags_no_mrrs);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x0db2, quirk_pcie2x_no_tags_no_mrrs);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x0db3, quirk_pcie2x_no_tags_no_mrrs);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x0db6, quirk_pcie2x_no_tags_no_mrrs);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x0db7, quirk_pcie2x_no_tags_no_mrrs);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x0db8, quirk_pcie2x_no_tags_no_mrrs);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x0db9, quirk_pcie2x_no_tags_no_mrrs);
+
 /*
  * Fixup to mark boot BIOS video selected by BIOS before it changes
  *

base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
-- 
2.39.5

Re: [PATCH v3 1/1] PCI: Add Extended Tag + MRRS quirk for Xeon 6

[Lukas Wunner]

On Tue, Jun 10, 2025 at 02:48:02PM +0300, Ilpo Järvinen wrote:
> When bifurcated to x2, Xeon 6 Root Port performance is sensitive to the
> configuration of Extended Tags, Max Read Request Size (MRRS), and 10-Bit
> Tag Requester (note: there is currently no 10-Bit Tag support in the
> kernel). While those can be configured to the recommended values by FW,
> kernel may decide to overwrite the initial values.
> 
> Add a quirk that disallows enabling Extended Tags and setting MRRS
> larger than 128B for devices under Xeon 6 Root Ports if the Root Port
> is bifurcated to x2. Use the host bridge's enable_device hook to
> overwrite MRRS if it's set to >128B for the device to be enabled.
> 
> The earlier attempts to implement this quirk polluted PCI core code
> with the checks necessary to support this quirk. Using the
> enable_device hook keeps the quirk well-contained, away from the PCI
> core code.
> 
> Suggested-by: Lukas Wunner <lukas@wunner.de>
> Link: https://cdrdv2.intel.com/v1/dl/getContent/837176
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Reviewed-by: Lukas Wunner <lukas@wunner.de>

Patch looks perfect to me and could go in either through pci or tip.


For the record, link to previous attempt to solve this:

https://lore.kernel.org/r/20250422130207.3124-1-ilpo.jarvinen@linux.intel.com/

Thanks,

Lukas

Re: [PATCH v3 1/1] PCI: Add Extended Tag + MRRS quirk for Xeon 6

[Dan Williams]

Ilpo Järvinen wrote:
> When bifurcated to x2, Xeon 6 Root Port performance is sensitive to the
> configuration of Extended Tags, Max Read Request Size (MRRS), and 10-Bit
> Tag Requester (note: there is currently no 10-Bit Tag support in the
> kernel). While those can be configured to the recommended values by FW,
> kernel may decide to overwrite the initial values.
> 
> Add a quirk that disallows enabling Extended Tags and setting MRRS
> larger than 128B for devices under Xeon 6 Root Ports if the Root Port
> is bifurcated to x2. Use the host bridge's enable_device hook to
> overwrite MRRS if it's set to >128B for the device to be enabled.
> 
> The earlier attempts to implement this quirk polluted PCI core code
> with the checks necessary to support this quirk. Using the
> enable_device hook keeps the quirk well-contained, away from the PCI
> core code.
> 
> Suggested-by: Lukas Wunner <lukas@wunner.de>
> Link: https://cdrdv2.intel.com/v1/dl/getContent/837176
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
> 
> Ingo gave his Ack on v2 but since the used approach has once again
> changed, I didn't add his Ack.
> 
> Mani did express his concern on using enable_device hook but suggested
> I send the patch anyway.
> 
> We're also looking into using _HPX Type 3 (suggested by Bjorn) eventually
> for this class of problems where FW settings get overwritten by the
> kernel (but it will take time to make it the sanctioned solution). In
> the meantime, this is a real problem for Xeon 6 out there so it warrants
> adding the quirk (which is now pretty well-contained).
> 
> v3:
> - Use enable_device hook to overwrite MRRS to 128B if needed. (Lukas)
> - Typo fix to comment (Ingo)
> 
> v2:
> - Explain in changelog why FW cannot solve this on its own
> - Moved the quirk under arch/x86/pci/
> - Don't NULL check value from pci_find_host_bridge()
> - Added comment above the quirk about the performance degradation
> - Removed all setup chain 128B quirk overrides expect for MRRS write
>   itself (assumes a sane initial value is set by FW)
> 
> 
>  arch/x86/pci/fixup.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)

Thanks for the work on this Ilpo, and thanks for the cleanup suggestions
Lukas. This version looks reasonable to carry the kernel until _HPX can
be relied upon to help the kernel apply quirks like this.

You can add:

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

Re: [PATCH v3 1/1] PCI: Add Extended Tag + MRRS quirk for Xeon 6

[Bjorn Helgaas]

On Tue, Jun 10, 2025 at 02:48:02PM +0300, Ilpo Järvinen wrote:
> When bifurcated to x2, Xeon 6 Root Port performance is sensitive to the
> configuration of Extended Tags, Max Read Request Size (MRRS), and 10-Bit
> Tag Requester (note: there is currently no 10-Bit Tag support in the
> kernel). While those can be configured to the recommended values by FW,
> kernel may decide to overwrite the initial values.
> 
> Add a quirk that disallows enabling Extended Tags and setting MRRS
> larger than 128B for devices under Xeon 6 Root Ports if the Root Port
> is bifurcated to x2. Use the host bridge's enable_device hook to
> overwrite MRRS if it's set to >128B for the device to be enabled.
> 
> The earlier attempts to implement this quirk polluted PCI core code
> with the checks necessary to support this quirk. Using the
> enable_device hook keeps the quirk well-contained, away from the PCI
> core code.
> 
> Suggested-by: Lukas Wunner <lukas@wunner.de>
> Link: https://cdrdv2.intel.com/v1/dl/getContent/837176
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Applied to pci/enumeration for v6.18, thanks.

> ---
> 
> Ingo gave his Ack on v2 but since the used approach has once again
> changed, I didn't add his Ack.
> 
> Mani did express his concern on using enable_device hook but suggested
> I send the patch anyway.

Yeah, I kind of agree, and there's also nothing that enforces
ownership of the .enable_device() pointer.  But it feels like it might
be overengineering to deal with all that.

> We're also looking into using _HPX Type 3 (suggested by Bjorn) eventually
> for this class of problems where FW settings get overwritten by the
> kernel (but it will take time to make it the sanctioned solution). In
> the meantime, this is a real problem for Xeon 6 out there so it warrants
> adding the quirk (which is now pretty well-contained).
> 
> v3:
> - Use enable_device hook to overwrite MRRS to 128B if needed. (Lukas)
> - Typo fix to comment (Ingo)
> 
> v2:
> - Explain in changelog why FW cannot solve this on its own
> - Moved the quirk under arch/x86/pci/
> - Don't NULL check value from pci_find_host_bridge()
> - Added comment above the quirk about the performance degradation
> - Removed all setup chain 128B quirk overrides expect for MRRS write
>   itself (assumes a sane initial value is set by FW)
> 
> 
>  arch/x86/pci/fixup.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
> index e7e71490bd25..38369124de45 100644
> --- a/arch/x86/pci/fixup.c
> +++ b/arch/x86/pci/fixup.c
> @@ -294,6 +294,46 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_MCH_PB1,	pcie_r
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_MCH_PC,	pcie_rootport_aspm_quirk);
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_MCH_PC1,	pcie_rootport_aspm_quirk);
>  
> +/*
> + * PCIe devices underneath Xeon6 PCIe Root Port bifurcated to 2x have slower
> + * performance with Extended Tags and MRRS > 128B. Work around the performance
> + * problems by disabling Extended Tags and limiting MRRS to 128B.
> + *
> + * https://cdrdv2.intel.com/v1/dl/getContent/837176
> + */
> +static int limit_mrrs_to_128(struct pci_host_bridge *bridge, struct pci_dev *pdev)
> +{
> +	int readrq = pcie_get_readrq(pdev);
> +
> +	if (readrq > 128)
> +		pcie_set_readrq(pdev, 128);
> +
> +	return 0;
> +}
> +
> +static void quirk_pcie2x_no_tags_no_mrrs(struct pci_dev *pdev)
> +{
> +	struct pci_host_bridge *bridge = pci_find_host_bridge(pdev->bus);
> +	u32 linkcap;
> +
> +	pcie_capability_read_dword(pdev, PCI_EXP_LNKCAP, &linkcap);
> +	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 2x PCIe link)\n");
> +}
> +
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x0db0, quirk_pcie2x_no_tags_no_mrrs);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x0db1, quirk_pcie2x_no_tags_no_mrrs);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x0db2, quirk_pcie2x_no_tags_no_mrrs);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x0db3, quirk_pcie2x_no_tags_no_mrrs);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x0db6, quirk_pcie2x_no_tags_no_mrrs);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x0db7, quirk_pcie2x_no_tags_no_mrrs);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x0db8, quirk_pcie2x_no_tags_no_mrrs);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x0db9, quirk_pcie2x_no_tags_no_mrrs);
> +
>  /*
>   * Fixup to mark boot BIOS video selected by BIOS before it changes
>   *
> 
> base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
> -- 
> 2.39.5
>