Re: [PATCH v10 4/5] vfio/pci: Add PCIe TPH configuration space virtualization

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Hardware configuration space is accessed while the device may be in a suspended (D3) power state.
- [High] Valid sub-dword (1-byte or 2-byte) writes to the TPH control register are silently dropped.
- [High] The guest's requested configuration for the TPH Requester Enable field is ignored and improperly overridden.
- [High] Unchecked return values from `pcie_enable_tph()` lead to state desynchronization and permanently broken TPH after reset or mode changes.
- [High] Concurrent config space writes to the TPH control register cause data races on `struct pci_dev` bitfields.
--

commit 11f3d46be6bcfc08d1a080f85c6eb8e89f3bdbb3
Author: Chengwen Feng <fengchengwen@huawei.com>
Subject: vfio/pci: Add PCIe TPH configuration space virtualization

This commit adds support for virtualizing the PCIe TPH (Transaction
Processing Hints) control register in vfio-pci. It introduces a module
parameter to globally control TPH support and mediates TPH control
register writes to only allow valid mode settings.

> diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> index a10ed733f0e3a..907b7a16ac0c6 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c
[ ... ]
> @@ -313,6 +316,31 @@ static int vfio_virt_config_read(struct vfio_pci_core_device *vdev, int pos,
>  	return count;
>  }
>  
> +static int vfio_pci_tph_config_write(struct vfio_pci_core_device *vdev, int pos,
> +				     int count, struct perm_bits *perm,
> +				     int offset, __le32 val)
> +{
> +	u32 data = le32_to_cpu(val);
> +
> +	if (!enable_unsafe_tph)
> +		return count;
> +
> +	if (count != 4 || offset != PCI_TPH_CTRL)
> +		return count;

Does this hardcoded check drop valid sub-dword (1-byte or 2-byte) writes from
a guest OS attempting to change the ST Mode?

The PCIe specification allows configuration space accesses of 1, 2, or 4
bytes.

> +
> +	/* Only permit write TPH mode. */
> +	data &= PCI_TPH_CTRL_MODE_SEL_MASK;

Does masking with PCI_TPH_CTRL_MODE_SEL_MASK ignore the Requester Enable
(REQ_EN) field (bits 8-9)?

If a guest clears REQ_EN to explicitly disable TPH but leaves the ST Mode
non-zero (e.g., IV Mode), data evaluates as non-zero. The handler would
then call pcie_enable_tph(), which forces REQ_EN to the host's supported
value and enables TPH on the physical hardware instead of disabling it.

> +	if (data > PCI_TPH_ST_DS_MODE)
> +		return count;
> +
> +	if (data != PCI_TPH_ST_NS_MODE)
> +		pcie_enable_tph(vdev->pdev, data);

Is it safe to ignore the return value of pcie_enable_tph()?

If a guest attempts to change from one ST mode to another, or attempts to
re-enable TPH after a Function Level Reset (FLR) where hardware registers
are cleared but pdev->tph_enabled remains 1, pcie_enable_tph() returns
-EBUSY or -EINVAL.

Ignoring this might leave the guest assuming success while TPH remains
broken until the device is closed.

Additionally, pcie_enable_tph() and pcie_disable_tph() modify the
tph_enabled:1 bitfield in struct pci_dev. Since VFIO only holds a read
lock during config writes, can concurrent writes to the TPH capability
cause a read-modify-write data race on adjacent bitfields in the same
machine word (such as msi_enabled), potentially corrupting PCI device state?

> +	else
> +		pcie_disable_tph(vdev->pdev);
> +
> +	return count;
> +}
> +
[ ... ]
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 3f8d093aacf8a..cc13fc8eea9da 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
[ ... ]
> @@ -736,6 +738,9 @@ void vfio_pci_core_close_device(struct vfio_device *core_vdev)
>  #endif
>  	vfio_pci_dma_buf_cleanup(vdev);
>  
> +	/* Disable TPH when userspace closes the device FD */
> +	pcie_disable_tph(vdev->pdev);
> +

Could the device still be in the suspended (D3) power state from previous
runtime PM or guest operations when pcie_disable_tph() is called here,
since it executes before vfio_pci_core_disable()?

Accessing hardware configuration space while the device is in D3 causes
unsupported requests (UR) and master aborts.

>  	vfio_pci_core_disable(vdev);
>  
[ ... ]
> @@ -2205,6 +2210,9 @@ int vfio_pci_core_register_device(struct vfio_pci_core_device *vdev)
>  	if (!disable_idle_d3)
>  		pm_runtime_put(dev);
>  
> +	/* Disable TPH when taking over ownership of the device */
> +	pcie_disable_tph(pdev);
> +

Can pcie_disable_tph() safely access the hardware configuration space here?

Since pm_runtime_put() was called immediately prior, the device may have
already transitioned to the D3 state, which could trigger unsupported
requests if the configuration space is accessed.

>  	ret = vfio_register_group_dev(&vdev->vdev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515094040.38496-1-fengchengwen@huawei.com?part=4