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

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

New issues:
- [High] The refactored quirk attempts to isolate the x2 mitigation but fails because it applies `no_inc_mrrs = 1` globally to the entire host bridge, still penalizing parallel x4 branches.
- [High] The local fixup to disable Extended Tags on x2 endpoints is immediately overwritten and undone by the PCI core during device enumeration.
- [Medium] The local endpoint fixup unlawfully modifies RsvdP (Reserved and Preserved) PCIe capabilities on SR-IOV Virtual Functions (VFs).
- [Low] Unmatched closing parenthesis in the `pci_info` log message.

Pre-existing issues:
- [Critical] A guest VM can bypass the quirk and re-enable Extended Tags via VFIO, leading to hardware instability and host Denial of Service.
--

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

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

This commit attempts to fix collateral damage caused by the Xeon 6 x2 quirk by
changing the mitigation to be per-device instead of global to the host bridge.
It removes the global bridge->no_ext_tags flag and uses a local endpoint fixup
to disable extended tags and lock MRRS to 128B for devices downstream of an
affected x2 root port.

> diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
> index b301c6c8df753..6370b08d99aef 100644
> --- a/arch/x86/pci/fixup.c
> +++ b/arch/x86/pci/fixup.c
> @@ -320,9 +311,8 @@ static void pci_xeon_x2_bifurc_quirk(struct pci_dev *pdev)
>  	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 x2 PCIe link)\n");
> +	bridge->no_inc_mrrs = 1;

[Severity: High]
Does setting bridge->no_inc_mrrs = 1 here still apply the restriction globally
to the entire host bridge?

When pcie_set_readrq() is called by unrelated x4 devices sharing this bridge, it
will check this flag:

drivers/pci/pci.c:pcie_set_readrq() {
    ...
	if (bridge->no_inc_mrrs) {
    ...
}

Would this cause the MRRS limitation to still inadvertently impact the parallel
x4 branches that this patch intends to protect?

> +	pci_info(pdev, "Blocking devices on this bridge from increasing MRRS for performance reasons due to x2 PCIe link)\n");

[Severity: Low]
There appears to be an unmatched closing parenthesis at the end of this log
message string.

[ ... ]

> @@ -334,6 +324,50 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x0db7, pci_xeon_x2_bifurc_quirk);
> +static void pci_xeon6_x2_local_endpoint_fixup(struct pci_dev *pdev)
> +{
> +	/* Skip bridges/switches; only target actual endpoints */
> +	if (pci_is_bridge(pdev))
> +		return;
> +
> +	/* Only apply to devices under the x2 branch; leaves x4 branches completely untouched */
> +	if (!is_descendant_of_xeon6_x2_rp(pdev))
> +		return;
> +
> +	pci_info(pdev, "Applying local Xeon 6 x2 quirk: Disabling Extended Tags and locking MRRS to 128B\n");
> +
> +	pcie_capability_clear_word(pdev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_EXT_TAG);

[Severity: High]
Will the clearing of the EXT_TAG bit here be undone by the PCI core later in
the boot process?

Because the global bridge->no_ext_tags flag was removed earlier in the patch,
when the PCI core later evaluates extended tags during pci_device_add(), it
will encounter this in pci_configure_extended_tags():

drivers/pci/probe.c:pci_configure_extended_tags() {
    ...
	if (!(ctl & PCI_EXP_DEVCTL_EXT_TAG)) {
		pci_info(dev, "enabling Extended Tags\n");
		pcie_capability_set_word(dev, PCI_EXP_DEVCTL,
					 PCI_EXP_DEVCTL_EXT_TAG);
	}
    ...
}

Could this unconditionally re-enable Extended Tags on the endpoint and defeat
the mitigation?

[Severity: Critical]
This is a pre-existing issue, but since the commit message mentions concerns
about vfio/VM usage re-enabling extended tags, is it possible for a guest VM
to bypass the quirk?

Looking at init_pci_cap_exp_perm() in drivers/vfio/pci/vfio_pci_config.c,
PCI_EXP_DEVCTL_EXT_TAG is not included in the NO_WRITE mask:

drivers/vfio/pci/vfio_pci_config.c:init_pci_cap_exp_perm() {
    ...
	p_setw(perm, PCI_EXP_DEVCTL,
	       PCI_EXP_DEVCTL_BCR_FLR | PCI_EXP_DEVCTL_PAYLOAD |
	       PCI_EXP_DEVCTL_READRQ | PCI_EXP_DEVCTL_PHANTOM, NO_WRITE);
    ...
}

Would this allow an unprivileged guest VM to write to its virtualized
PCI_EXP_DEVCTL register and have the write pass through to the physical
hardware, thereby re-enabling Extended Tags?

> +	pcie_capability_clear_word(pdev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_READRQ);

[Severity: Medium]
Does this code modify the PCI_EXP_DEVCTL register on SR-IOV Virtual Functions?

According to the PCIe SR-IOV Specification, Extended Tag Enable and
Max_Read_Request_Size are RsvdP (Reserved and Preserved) fields on VFs and must
not be modified, as they track the PF.

Should there be a check for !pdev->is_virtfn before clearing these bits?

> +}

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