linux-kernel - Re: [PATCH v2 1/1] PCI: Add Extended Tag + MRRS quirk for Xeon 6

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aBM3cLA_sw7iWoJf@wunner.de>
Date: Thu, 1 May 2025 10:57:20 +0200
From: Lukas Wunner <lukas@...ner.de>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Dan Williams <dan.j.williams@...el.com>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
	Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
	"H. Peter Anvin" <hpa@...or.com>, linux-pci@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/1] PCI: Add Extended Tag + MRRS quirk for Xeon 6

On Tue, Apr 22, 2025 at 04:02:07PM +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).
[...]
> 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. Reject >128B MRRS only when it is going to be written
> by the kernel (this assumes FW configured a good initial value for MRRS
> in case the kernel is not touching MRRS at all).

I note that there's the existing quirk_brcm_5719_limit_mrrs(),
which limits MRRS to 2048 on certain revisions of Broadcom
Ethernet adapters.  This became necessary to work around an
internal FIFO problem, see commit 2c55a3d08ade ("tg3: Scale back
code that modifies MRRS") and commit 0b471506712d ("tg3: Recode
PCI MRRS adjustment as a PCI quirk").

The quirk works by overriding the MRRS which was originally set
on enumeration by pcie_bus_configure_settings().  The overriding
happens at enable time, i.e. when a driver starts to makes use
of the device:

do_pci_enable_device()
  pci_host_bridge_enable_device()
  pcibios_enable_device()
  pci_fixup_device()
    quirk_brcm_5719_limit_mrrs()

Now if you look further above in do_pci_enable_device(), there's
a call to pci_host_bridge_enable_device(), which invokes the
->enable_device() callback in struct pci_host_bridge.
Currently there's only a single host brige driver implementing
that callback, controller/dwc/pci-imx6.c.

One option would be to set that callback on the host bridge
if a Granite Rapids Root Port is found.  And then enforce the
mrrs limit in the callback.  That approach may be more acceptable
upstream than adding a custom "only_128b_mrrs" bit to struct
pci_host_bridge.

Another option would be to amend x86's pcibios_enable_device()
to check whether there's a Granite Rapids Root Port above the
device and enforce the mrrs limit if so.

The only downside I see is that the Broadcom quirk will run
afterwards and increase the MRRS again.  But it's highly unlikely
that one of these old Broadcom chips is used on a present-day
Granite Rapids server, so it may not be a problem in practice.
And the worst thing that can happen is suboptimal performance.

Thanks,

Lukas