Re: [PATCH v8 4/7] vfio/pci: Add PCIe TPH interface with capability query

[fengchengwen <fengchengwen@huawei.com>]

On 5/9/2026 6:40 AM, Alex Williamson wrote:
> On Fri, 8 May 2026 14:40:50 +0800
> Chengwen Feng <fengchengwen@huawei.com> wrote:
> 
>> Add VFIO_DEVICE_PCI_TPH IOCTL to allow userspace to query device TPH
>> capabilities, supported modes, and steering tag table information.
>>
>> Add module parameter 'enable_unsafe_tph_ds_mode' to restrict unsafe
>> device-specific TPH mode to trusted userspace only.
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> ---
>>  drivers/vfio/pci/vfio_pci.c      |  13 ++-
>>  drivers/vfio/pci/vfio_pci_core.c |  56 ++++++++++++-
>>  include/linux/vfio_pci_core.h    |   3 +-
>>  include/uapi/linux/vfio.h        | 133 +++++++++++++++++++++++++++++++
>>  4 files changed, 202 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
>> index 0c771064c0b8..40bf5aa9fd0b 100644
>> --- a/drivers/vfio/pci/vfio_pci.c
>> +++ b/drivers/vfio/pci/vfio_pci.c
>> @@ -60,6 +60,12 @@ static bool disable_denylist;
>>  module_param(disable_denylist, bool, 0444);
>>  MODULE_PARM_DESC(disable_denylist, "Disable use of device denylist. Disabling the denylist allows binding to devices with known errata that may lead to exploitable stability or security issues when accessed by untrusted users.");
>>  
>> +#ifdef CONFIG_PCIE_TPH
>> +static bool enable_unsafe_tph_ds_mode;
>> +module_param(enable_unsafe_tph_ds_mode, bool, 0444);
>> +MODULE_PARM_DESC(enable_unsafe_tph_ds_mode, "Enable UNSAFE TPH device-specific (DS) mode. This mode provides weak isolation, cannot be safely used for virtual machines. If you do not know what this is for, step away. (default: false)");
>> +#endif
>> +
> 
> Why is the "unsafe" aspect of this keyed on mode rather than storage
> location?
> 
> Currently the user cannot enable TPH, the capability is read-only, but
> the user does have direct access to the MSI-X table.  We rely on an
> agreement that the user needs to use SET_IRQS to allocate host vectors
> and we use interrupt remapping as protection against abuse, but there's
> no mediation of STs written directly to the MSI-X table.  If the device
> supports IV mode with ST in the MSI-X table, nothing prevents the user
> from writing those ST entries directly to the MSI-X table.  Therefore
> doesn't it have the same security concern as DS mode?


Agree, from this perspective, even if it is in MSI-X table, it is still unsafe.
So TPH is unsafe as a whole, not just DS mode.

> 
> Further, config space lives in the device and various devices are known
> to have alternate means for accessing their config space.
> Virtualization of config space is more to present the device in the VM
> address space and bridge features between guest and host.  It's not
> great as a security barrier.
> 
> Maybe it's really neither the mode nor storage location, and we need to
> decide if TPH as a whole introduces any new security considerations.

I will adjust the module parameter to control TPH globally instead of only DS mode.

> It seems arguable whether we can actually prevent a device from
> including arbitrary STs on TLPs in any case and maybe we're really only
> exposing a curated programming interface.
> 
> ...
>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> index 5de618a3a5ee..81da2bd0c21b 100644
>> --- a/include/uapi/linux/vfio.h
>> +++ b/include/uapi/linux/vfio.h

...

>> +#define VFIO_DEVICE_PCI_TPH	_IO(VFIO_TYPE, VFIO_BASE + 22)
>> +
> 
> This seems like the wrong shape to me and introduces yet another ioctl
> multiplexer.  We already have that via the device feature interface.
> I'd propose this only needs one new DEVICE_FEATURE ioctl, TPH_ST.  The
> uAPI would look like:
> 
> struct vfio_device_feature_tph_st {
> 	__u32 flags;
> #define VFIO_TPH_ST_MEM_TYPE_PM	(1 << 0)
> 	__u16 index;
> 	__u16 count;
> 	__u32 data[]; /* host CPU# on SET, ST value on GET */
> }
> 
> The user can SET multiple STs at once that have the same mem_type
> (assuming that's a reasonable limitation).  On SET, each {cpu#,

Agree, using the same mem_type for a batch is a good idea.

Because it could set multiple index, so how about:

struct vfio_pci_tph_entry {
	__u32 cpu;
	__u16 val;	/* ST index on SET, ST value on GET */
	__u16 reserved;
}

struct vfio_device_feature_tph_st {
	__u32 op;
#define VFIO_TPH_OP_GET_ST	0
#define VFIO_TPH_OP_SET_ST	1
	__u32 flags;
#define VFIO_TPH_ST_MEM_TYPE_PM	(1 << 0)
	__u16 count;
	__u16 reserved1;
	struct vfio_pci_tph_entry ents[];
}

> mem_type} is translated to a host value and stored internally.  A GET

Should we store internally? How about writing directly to the device?

> returns that translated ST value for DS use cases.
> 
> The user can use PROBE to determine if this feature is available.
> 
> We already provide the TPH capability read-only in config space, we can
> use that rather than an explicit INFO/GET_CAP interface.

OK

> 
> When the feature is available, the TPH control register is virtualized.
> On enabling TPH via config space, vfio will store the translated ST
> values to the appropriate location, or none, and enable TPH.  On SET
> while already enabled, vfio will update both the internal table and the
> device location (or none).  Thanks,
> 
> Alex