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

[Lukas Wunner <lukas@wunner.de>]

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