Re: [PATCH v8 5/7] vfio/pci: Add PCIe TPH enable/disable support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] TPH operations access the device's PCI configuration space without ensuring the device is in the D0 power state, risking accesses while suspended (D3).
- [High] `vdev->igate` is insufficient to prevent bitfield tearing on `pdev->tph_enabled` against other PCI subsystems.
--

commit e6a89cf03d6c51144466205cce4a3bf38731b5db
Author: Chengwen Feng <fengchengwen@huawei.com>

vfio/pci: Add PCIe TPH enable/disable support

This patch adds support for enabling and disabling the TPH function via
mode selection. It also disables TPH when ownership is taken or when
userspace closes the device FD.

> Serialize all TPH operations under vdev->igate mutex using scope-based
> automatic locking to prevent hardware control and bitfield races.

If pdev->tph_enabled is a bitfield inside struct pci_dev that shares a
memory word with other core PCI bitfields (like state_saved or
is_busmaster), will vdev->igate prevent bitfield tearing?

Since other kernel subsystems modify these neighboring bitfields without
holding the VFIO-private igate mutex (such as the PCI PM core modifying
state_saved during runtime suspend), concurrent updates could result in
data races and lost state updates.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 0e97b128fd637..bfc7e87d190f4 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -738,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);
> +
>  	vfio_pci_core_disable(vdev);

Is it safe to access the device's PCI configuration space here?

Since vfio_pci_core_disable() is explicitly responsible for waking up the
device via pm_runtime_resume(), pcie_disable_tph() is being called before
the device is ensured to be in the D0 power state. 

Accessing the config space of a suspended device could cause PCIe errors
like Unsupported Requests.

[ ... ]

>  	switch (op.op) {
>  	case VFIO_PCI_TPH_GET_CAP:
>  		return vfio_pci_tph_get_cap(vdev, &op, uarg + minsz);
> +	case VFIO_PCI_TPH_ENABLE:
> +		return vfio_pci_tph_enable(vdev, &op, uarg + minsz);
> +	case VFIO_PCI_TPH_DISABLE:
> +		return vfio_pci_tph_disable(vdev);

Are these TPH capability accesses safe without a pm_runtime_resume_and_get()
and pm_runtime_put() wrapper?

Userspace could potentially access the config space while the device is idle
and runtime suspended.

[ ... ]

> @@ -2257,6 +2302,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);
> +

Could this pcie_disable_tph() execute while the device is suspending?

It is called after pm_runtime_put(), meaning the PM core may have already
started asynchronously suspending the device, potentially leading to config
space accesses while the device is in D3.

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