Re: [PATCH v7 4/6] 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] PCIe configuration accesses for TPH in device register/close paths occur while the device may be suspended (D3).
- [High] Missing Runtime PM wake-up in TPH ioctl handlers allows hardware access on suspended devices.
--

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

vfio/pci: Add PCIe TPH enable/disable support

This commit adds support to enable and disable the PCIe TPH function
with mode selection, restricting unsafe modes. It also disables TPH when
ownership is taken or when userspace closes the device file descriptor.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index e7efa8f230bec..7a5dc2bfe2e9d 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);
> +

Is it safe to call pcie_disable_tph() here before vfio_pci_core_disable()
wakes the device up?

A device might be in a suspended state when its file descriptor is closed.
vfio_pci_core_disable() explicitly handles waking the device up via
pm_runtime_resume(&pdev->dev).

If the device is in D3, won't accessing the configuration space cause
dropped writes or PCIe bus faults?

>  	vfio_pci_core_disable(vdev);
>  
>  	mutex_lock(&vdev->igate);

[ ... ]

>  static int vfio_pci_ioctl_tph(struct vfio_pci_core_device *vdev,
>  			      void __user *uarg)
>  {
>  	struct vfio_device_pci_tph_op op = {0};
>  	size_t minsz = sizeof(op.argsz) + sizeof(op.op);
>  
> +	guard(mutex)(&vdev->igate);
> +

Could this ioctl be called while the device is suspended?

Neither vfio_pci_core_ioctl() nor vfio_pci_ioctl_tph() appears to call
pm_runtime_resume_and_get() before dispatching these ioctls.

Since userspace can place the device into a low-power state, accessing
the TPH configuration space registers via pcie_tph_get_st_modes(),
pcie_enable_tph(), or pcie_disable_tph() might read invalid data or
drop writes.

>  	if (copy_from_user(&op, uarg, minsz))
>  		return -EFAULT;
>  
>  	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);
>  	default:
>  		/* Other ops are not implemented yet */
>  		return -EINVAL;

[ ... ]

> @@ -2258,6 +2303,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);
> +

Does this call to pcie_disable_tph() happen after the device may have
already entered a low-power state?

pm_runtime_put() can asynchronously drop the PM usage counter and allow
the device to enter D3.

Shouldn't the TPH disablement occur before dropping the PM reference to
avoid accessing the configuration space of a suspended device?

>  	ret = vfio_register_group_dev(&vdev->vdev);
>  	if (ret)
>  		goto out_power;

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