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

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

New issues:
- [High] The one-time bus walk completely misses dynamically hotplugged downstream endpoints, breaking the workaround and exposing the system to hardware instability.

Pre-existing issues:
- [High] Endpoint drivers and VFIO guests can trivially overwrite the manual 128B MRRS clamp and Extended Tag restrictions.
--

--- Patch [1]: [PATCH] PCI: Fix Intel Xeon 6 x2 quirk collateral damage on adjacent x4 endpoints ---
commit 437863bd6a6bd870ba3147da1f484eb59fd2282e
Author: Rick Warner <[email protected]>

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

This commit refactors the Intel Xeon 6 x2 traffic mitigation quirk to apply
locally to subordinate devices rather than globally on the host bridge. It
removes the host bridge property assignments and instead performs a one-time
bus walk during enumeration to manually clear Extended Tags and clamp MRRS
directly on endpoints.

> diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
> index b301c6c8df753..c2bdf580692aa 100644
> --- a/arch/x86/pci/fixup.c
> +++ b/arch/x86/pci/fixup.c
> @@ -301,38 +301,43 @@ 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)
> +static int pci_xeon_x2_bifurc_quirk_perdev(struct pci_dev *pdev, void *data)
>  {
> -	int readrq = pcie_get_readrq(pdev);
> -
> -	if (readrq > 128)
> -		pcie_set_readrq(pdev, 128);
> -
> +	/* Only clamp actual endpoints, skip downstream switches/bridges */
> +	if (!pci_is_bridge(pdev)) {
> +		pci_info(pdev, "Disabling Extended Tags and Clamping MRRS to 128B due to upstream Xeon 6 x2 bifurcation\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);

[Severity: High]
This is a pre-existing issue, but can endpoint drivers and VFIO guests
trivially overwrite these manual register changes?

This code clears the MRRS and Extended Tag bits directly via configuration
writes, but it does not set a subsystem constraint such as no_inc_mrrs on the
bridge. During device probe or VFIO guest execution, could a subsequent call
to pcie_set_readrq() unconditionally overwrite this 128B limit and reintroduce
the hardware instability?

> +	}
>  	return 0;
>  }
>  
>  static void pci_xeon_x2_bifurc_quirk(struct pci_dev *pdev)
>  {

[ ... ]

> +	/*
> +	 * Walk down only this specific root port's downstream bus tree
> +	 * and clear extended tags specifically for devices on this branch.
> +	 */
> +	if (pdev->subordinate)
> +		pci_walk_bus(pdev->subordinate, pci_xeon_x2_bifurc_quirk_perdev, NULL);

[Severity: High]
Does this one-time bus walk completely miss dynamically hotplugged downstream
endpoints?

By removing the dynamic enable_device hook, this mitigation relies entirely
on the DECLARE_PCI_FIXUP_FINAL sweep which executes exclusively once at boot
during root port enumeration. If a device is dynamically added later via
hotplug or if an SR-IOV VF is instantiated, the root port is not
re-enumerated and the callback won't be triggered for the new endpoints.
Could this leave hotplugged devices unmitigated, allowing them to negotiate
full MRRS and break the workaround?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1