Re: [PATCH v19 13/18] vfio/pci: Introduce VFIO_DEVICE_FEATURE_TPH family uapi for PCI TPH control

[Alex Williamson <alex@shazbot.org>]

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

> Add user API definitions for PCI TPH device feature set, including three
> feature IDs:
> - VFIO_DEVICE_FEATURE_TPH: Global TPH enable and capability query
> - VFIO_DEVICE_FEATURE_TPH_RESOLVE: Resolve PH/ST from specified source
>   (GET only)
> - VFIO_DEVICE_FEATURE_TPH_ST: Batch program ST table entries (SET only)
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
>  include/uapi/linux/vfio.h | 62 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
> 
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 5de618a3a5ee..ed04b6280cd6 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -1534,6 +1534,68 @@ struct vfio_device_feature_dma_buf {
>   */
>  #define VFIO_DEVICE_FEATURE_MIG_PRECOPY_INFOv2  12
>  
> +/* PCIe TPH device feature definitions for VFIO_DEVICE_FEATURE ioctl */
> +#define VFIO_DEVICE_FEATURE_TPH		13
> +#define VFIO_DEVICE_FEATURE_TPH_RESOLVE	14
> +#define VFIO_DEVICE_FEATURE_TPH_ST	15
> +
> +/*
> + * VFIO_DEVICE_FEATURE_TPH - Control and query PCI TPH capabilities
> + *
> + * SET: Opt-in to TPH feature; flags must be zero.

We clearly need to define what it actually means to opt-in, as this
series is not implementing it as it should be defined.  The user needs
to opt-in via this feature before we modify the device ABI in any way.
The extent to which the user may enable TPH features is bound both by
the device capabilities and the module parameters set by the system
admin.

> + * GET: Return supported TPH capability bits in flags.
> + */
> +struct vfio_device_feature_tph {
> +	__u32 flags;
> +};
> +
> +/* Capability bits returned by GET on VFIO_DEVICE_FEATURE_TPH */
> +#define VFIO_DEVICE_TPH_CAP_DMABUF	(1u << 0) /* DMABUF sources resolvable */
> +#define VFIO_DEVICE_TPH_CAP_CPU		(1u << 1) /* CPU sources resolvable via _DSM */
> +#define VFIO_DEVICE_TPH_CAP_LITERAL	(1u << 2) /* LITERAL ST source supported */
> +
> +/*
> + * VFIO_DEVICE_FEATURE_TPH_RESOLVE - Resolve PH/ST tag value from source
> + * Note: GET only
> + */
> +struct vfio_device_feature_tph_resolve {
> +	__u32 flags;	/* IN: source type + namespace modifiers */
> +	__u32 src;	/* IN: dma-buf fd or CPU ID based on flags */
> +	__u8 valid;	/* OUT: bitmap indicating valid output fields */
> +#define VFIO_DEVICE_TPH_VALID_PH (1u << 0) /* ph holds valid processing hint */
> +#define VFIO_DEVICE_TPH_VALID_ST (1u << 1) /* st holds valid steering tag */
> +	__u8 ph;	/* OUT: TPH processing hint */
> +	__u16 st;	/* OUT: raw steering tag value */
> +};
> +
> +/*
> + * VFIO_DEVICE_FEATURE_TPH_ST - Batch program architected ST table entries
> + * Return value: number of successfully written entries
> + * Note: SET only
> + */
> +struct vfio_device_feature_tph_st {
> +	__u32 flags;		/* IN: source type + namespace modifiers */
> +	__u16 start;		/* IN: first ST table index / MSI-X vector */
> +	__u16 count;		/* IN: number of contiguous entries to program */
> +	__aligned_u64 dests;	/* IN: user pointer to array of source u32 values */
> +};
> +
> +/* Common flags shared by TPH_RESOLVE and TPH_ST features */
> +#define VFIO_DEVICE_TPH_SRC_NONE           (1u << 0) /* TPH_ST only: set ST=0 */
> +#define VFIO_DEVICE_TPH_SRC_DMABUF	   (1u << 1) /* src = dma-buf fd */
> +#define VFIO_DEVICE_TPH_SRC_CPU_VOLATILE   (1u << 2) /* src = volatile memory CPU ID */
> +#define VFIO_DEVICE_TPH_SRC_CPU_PERSISTENT (1u << 3) /* src = persistent memory CPU ID */
> +#define VFIO_DEVICE_TPH_SRC_LITERAL        (1u << 4) /* TPH_ST only: src = raw literal ST */
> +#define VFIO_DEVICE_TPH_EXTENDED           (1u << 30) /* Use extended 16-bit ST namespace */
> +#define VFIO_DEVICE_TPH_REQUIRE_ST         (1u << 31) /* TPH_ST only: fail if resolved ST=0 */

I don't see the value in jumping to the end of the flags field for
these last two.  Adding DMABUF is a good change from the proposal, but
I wouldn't go out of our way to try to preserve some grouping of source
flags from modifiers.

We also seem to have avoided any statement or decision to what extent
our uAPI is defined by the current implementation.  Particularly the
ordering of TPH_ST versus manipulating the control bits in config space
are entirely undefined here.

We're also not addressing that the TPH_ST policy counters the overall
feature policy to return 0/-errno and not updating the documentation to
bring that to the forefront.  Thanks,

Alex

> +
> +/* Mask for mutually exclusive source type bits, only one may be set */
> +#define VFIO_DEVICE_TPH_SRC_MASK	(VFIO_DEVICE_TPH_SRC_NONE | \
> +					 VFIO_DEVICE_TPH_SRC_DMABUF | \
> +					 VFIO_DEVICE_TPH_SRC_CPU_VOLATILE | \
> +					 VFIO_DEVICE_TPH_SRC_CPU_PERSISTENT | \
> +					 VFIO_DEVICE_TPH_SRC_LITERAL)
> +
>  /* -------- API for Type1 VFIO IOMMU -------- */
>  
>  /**