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

[Alex Williamson <alex@shazbot.org>]

On Sat, 9 May 2026 11:28:03 +0800
fengchengwen <fengchengwen@huawei.com> wrote:
> 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:
> >> 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.

I'm not convinced that's the right solution either.  It's a usage
barrier if the TPH capability isn't exposed R/W, but does it guarantee
the device won't make use of such TLPs anyway?  If the device has
config space backdoors or can otherwise be manipulated to send these
hints, a vfio-pci module option is just security theater.  It's also a
burden for users and for each variant driver for devices supporting TPH.

We do however need to consider how changing the behavior of the
capability affects existing users, like QEMU.  We may need to consider
two device features, one that only supports SET with no payload to
enable virtualized access to the TPH capability and another that
provides the ST handling interface.

> > 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;
> }

In the structure I proposed the user can set/get contiguous index
ranges according to index and count, where the data field can then just
be a u32 array.  Why does the user need to be able to set/get
arbitrary, non-contiguous indexes?

In a VM use case we'd likely be trapping individual writes, therefore
we'd be intercepting one index at a time.
 
> struct vfio_device_feature_tph_st {
> 	__u32 op;
> #define VFIO_TPH_OP_GET_ST	0
> #define VFIO_TPH_OP_SET_ST	1

The vfio device feature interface already handles set/get, we don't
need this.

> 	__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?

Perhaps we should, but I think we need a flag to indicate whether we're
virtualizing a write to hardware (capability or MSI-X table) or SET'ing
an index that userspace will later retrieve for DS mode via GET.
Otherwise we don't know until the mode bits are written which location,
if any, the capability is actually using.  For example the device can
support either MSI-X or capability locations, but enable DS mode.  For
consistency though, it might make sense to write to an internal table
regardless.  Thanks,

Alex