Re: [PATCH v19 14/18] vfio/pci: Implement VFIO_DEVICE_FEATURE_TPH and valid TPH config write support

[Alex Williamson <alex@shazbot.org>]

On Thu, 2 Jul 2026 20:42:20 +0800
Chengwen Feng <fengchengwen@huawei.com> wrote:

> Implement vfio_pci_core_feature_tph() to handle VFIO_DEVICE_FEATURE_TPH:
> - SET permits TPH feature usage for the device;
> - GET reports available TPH capabilities according to tph_policy and
>   root-port DSM support.
> 
> Previously VFIO dropped all writes to the TPH capability register.
> Now implement vfio_tph_config_write() to support legitimate TPH_CTRL
> modification after TPH feature is permitted.
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
>  drivers/vfio/pci/vfio_pci_config.c | 54 ++++++++++++++++++++++++++++++
>  drivers/vfio/pci/vfio_pci_core.c   | 36 ++++++++++++++++++++
>  include/linux/vfio_pci_core.h      |  1 +
>  3 files changed, 91 insertions(+)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> index 06d7b2fbf866..388dd6fed16b 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c
> @@ -1150,6 +1150,60 @@ static int vfio_tph_config_write(struct vfio_pci_core_device *vdev, int pos,
>  				 int count, struct perm_bits *perm,
>  				 int offset, __le32 val)
>  {
> +	u16 start = vfio_find_cap_start(vdev, pos);
> +	struct pci_dev *pdev = vdev->pdev;
> +	u32 org_ctrl, new_ctrl, cap;
> +	u8 mode, req, org_req;
> +	__le32 org_val = 0;
> +	bool extended;
> +	int ret;
> +
> +	if (!vdev->tph_permit)

We need to work on this variable name, tph_opt_in or something.

> +		return count;
> +
> +	down_write(&vdev->memory_lock);

Why is memory_lock involved in this sequence?  Taking the write lock
doesn't enable memory if it's already disabled, nor do we test the
memory state.

> +
> +	org_ctrl = le32_to_cpu(*(__le32 *)&vdev->vconfig[start + PCI_TPH_CTRL]);
> +	vfio_default_config_read(vdev, pos, count, perm, offset, &org_val);
> +
> +	ret = vfio_default_config_write(vdev, pos, count, perm, offset, val);
> +	if (ret != count)
> +		goto out;
> +
> +	new_ctrl = le32_to_cpu(*(__le32 *)&vdev->vconfig[start + PCI_TPH_CTRL]);
> +	if (new_ctrl == org_ctrl)
> +		goto out; /* Only care about changes in TPH_CTRL. */

We didn't need memory_lock for any of this.

> +
> +	cap = le32_to_cpu(*(__le32 *)&vdev->vconfig[start + PCI_TPH_CAP]);
> +	mode = FIELD_GET(PCI_TPH_CTRL_MODE_SEL_MASK, new_ctrl);
> +	req = FIELD_GET(PCI_TPH_CTRL_REQ_EN_MASK, new_ctrl);
> +	if (mode > PCI_TPH_ST_DS_MODE || !(cap & (1u << mode)) || req == 0x2 ||
> +		(req == PCI_TPH_REQ_EXT_TPH && !(cap & PCI_TPH_CAP_EXT_TPH)))
> +		goto restore; /* Drop invalid or unsupported write value */

Nor did we need memory_lock for this.

> +
> +	org_req = FIELD_GET(PCI_TPH_CTRL_REQ_EN_MASK, org_ctrl);
> +	if (req == org_req)
> +		goto out; /* Only care about requester enable */

What if mode is changed while enabled, don't we create an inconsistent
state?

> +
> +	ret = vfio_pci_set_power_state(vdev, PCI_D0);

We already did a runtime PM resume before calling vfio_pci_config_rw(),
why do we need this?

> +	if (ret)
> +		goto restore; /* Drop this write */
> +
> +	if (req == PCI_TPH_REQ_TPH_ONLY || req == PCI_TPH_REQ_EXT_TPH) {
> +		extended = !!(req == PCI_TPH_REQ_EXT_TPH);
> +		ret = pcie_enable_tph_explicit(pdev, mode, extended);
> +		if (ret)
> +			goto restore;
> +	} else if (req == PCI_TPH_REQ_DISABLE) {
> +		pcie_disable_tph(vdev->pdev);
> +	}

This sequence is really the only thing that appears to need memory_lock
and it should be using vfio_pci_memory_lock_and_enable() along with
vfio_pci_memory_unlock_and_restore().

> +
> +	goto out;
> +
> +restore:
> +	vfio_default_config_write(vdev, pos, count, perm, offset, org_val);
> +out:
> +	up_write(&vdev->memory_lock);
>  	return count;
>  }
>  
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 3f11a9624b9c..14944d3ea86e 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -30,6 +30,7 @@
>  #include <linux/sched/mm.h>
>  #include <linux/iommufd.h>
>  #include <linux/pci-p2pdma.h>
> +#include <linux/pci-tph.h>
>  #include <linux/seq_file.h>
>  #if IS_ENABLED(CONFIG_EEH)
>  #include <asm/eeh.h>
> @@ -610,6 +611,7 @@ int vfio_pci_core_enable(struct vfio_pci_core_device *vdev)
>  		goto out_disable_device;
>  
>  	vdev->reset_works = !ret;
> +	vdev->tph_permit = false;
>  	pci_save_state(pdev);
>  	vdev->pci_saved_state = pci_store_saved_state(pdev);
>  	if (!vdev->pci_saved_state)
> @@ -1607,6 +1609,38 @@ static int vfio_pci_core_feature_token(struct vfio_pci_core_device *vdev,
>  	return 0;
>  }
>  
> +static int vfio_pci_core_feature_tph(struct vfio_pci_core_device *vdev,
> +				     u32 flags,
> +				     struct vfio_device_feature_tph __user *arg,
> +				     size_t argsz)
> +{
> +	struct vfio_device_feature_tph tph = {0};
> +	int ret;
> +
> +	if (!pcie_tph_supported(vdev->pdev, false))
> +		return -EOPNOTSUPP;
> +
> +	ret = vfio_check_feature(flags, argsz,
> +			VFIO_DEVICE_FEATURE_GET | VFIO_DEVICE_FEATURE_SET,
> +			sizeof(tph));
> +	if (ret <= 0)
> +		return ret;

That's not the convention for testing vfio_check_feature() return.

> +
> +	if (flags & VFIO_DEVICE_FEATURE_SET) {

		if (tph.flags)
			return -EINVAL;

> +		vdev->tph_permit = 1;
> +		return 0;
> +	}
> +

	if (!vdev->tph_permit)
		return -EINVAL;

> +	tph.flags = VFIO_DEVICE_TPH_CAP_DMABUF;
> +	if (vdev->tph_policy != VFIO_PCI_TPH_POLICY_NO_ST &&
> +		pcie_tph_dsm_supported(vdev->pdev))
> +		tph.flags |= VFIO_DEVICE_TPH_CAP_CPU;
> +	if (vdev->tph_policy == VFIO_PCI_TPH_POLICY_LITERAL)
> +		tph.flags |= VFIO_DEVICE_TPH_CAP_LITERAL;

These should also account for the device capabilities, CPU is not
supported unless the device supports somewhere to program CPU values,
ie. IV or DS mode.  Same for LITERAL, not exposed unless the device has
a way to use it, (IV or DS) and POLICY_LITERAL.  Thanks,

Alex

> +
> +	return copy_to_user(arg, &tph, sizeof(tph)) ? -EFAULT : 0;
> +}
> +
>  int vfio_pci_core_ioctl_feature(struct vfio_device *device, u32 flags,
>  				void __user *arg, size_t argsz)
>  {
> @@ -1625,6 +1659,8 @@ int vfio_pci_core_ioctl_feature(struct vfio_device *device, u32 flags,
>  		return vfio_pci_core_feature_token(vdev, flags, arg, argsz);
>  	case VFIO_DEVICE_FEATURE_DMA_BUF:
>  		return vfio_pci_core_feature_dma_buf(vdev, flags, arg, argsz);
> +	case VFIO_DEVICE_FEATURE_TPH:
> +		return vfio_pci_core_feature_tph(vdev, flags, arg, argsz);
>  	default:
>  		return -ENOTTY;
>  	}
> diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h
> index a0641286dd90..9a6e82c5c00c 100644
> --- a/include/linux/vfio_pci_core.h
> +++ b/include/linux/vfio_pci_core.h
> @@ -150,6 +150,7 @@ struct vfio_pci_core_device {
>  	struct rw_semaphore	memory_lock;
>  	struct list_head	dmabufs;
>  	u8			tph_policy;
> +	bool			tph_permit;
>  };
>  
>  enum vfio_pci_io_width {