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@xxxxxxxxx> > Link: https://cdrdv2.intel.com/v1/dl/getContent/837176 > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx> 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 >
