On Tue, May 12, 2026 at 6:33 PM fengchengwen <[email protected]> wrote: > > > > Hi Zhiping, > > I have several suggestions: > > 1. In struct vfio_device_feature_dma_buf_tph, steering_tag is defined as > __u16, but PCIe TPH base steering tag is only 8-bit. We can use __u8 > for steering_tag to shrink structure size and reduce reserved padding. > > 2. The flags field seems unnecessary. We can use value 0 of steering_tag > or steering_tag_ext to indicate the corresponding ST entry is not > available, which simplifies the uAPI design. > > 3. All TPH metadata fields (st, ext st, ph) fit within 32 bits. We can > wrap them into a union with atomic_t, then use atomic read/write > instead of memory_lock plus smp_load_acquire/smp_store_release. This > makes lockless access cleaner and avoids ordering maintenance. > > For details, see the text. >
Hi Feng, Thanks for the detailed review. 1) Regular TPH uses an 8-bit ST, while Extended TPH uses a 16-bit ST, so so Extended TPH in Flit mode TLP can carry a 16-bit steering tag in the request. 2) I still think I need an explicit validity indication rather than using `0` to mean "not present". ST and Extended ST can coexist with different values (including the value 0). 3) I also do not think packing the fields into `atomic_t` removes the need for `memory_lock` here, because the write side still needs the lock for `priv->vdev` ownership/lifetime checks and the dma-buf list/cleanup paths. Open for discussion, though. Thanks, Zhiping > On 5/13/2026 2:47 AM, Zhiping Zhang wrote: > > Add a dma-buf callback that returns raw TPH metadata from the exporter > > so peer devices can reuse the steering tag and processing hint > > associated with a VFIO-exported buffer. Add a new > > VFIO_DEVICE_FEATURE_DMA_BUF_TPH ioctl that takes the fd from > > VFIO_DEVICE_FEATURE_DMA_BUF along with the TPH values, validates the fd > > is a vfio-exported dma-buf belonging to this device, and stores the TPH > > metadata under memory_lock. The existing VFIO_DEVICE_FEATURE_DMA_BUF > > uAPI is unchanged. > > > > 8-bit ST and 16-bit Extended ST are distinct namespaces in the PCIe TPH > > ST table (firmware reports them as separate fields with separate > > validity bits in the ACPI _DSM ST table), so the uAPI carries both > > values along with a flags field that indicates which value(s) are > > valid for this device. The exporter selects the value that matches the > > importer's requested width and returns -EOPNOTSUPP if that width is > > not present, instead of substituting a value across namespaces. > > > > Publish the TPH fields under memory_lock and gate readers on a > > release/acquire on the flags field; this lets get_tph() run lockless > > and avoids inverting the memory_lock -> dma_resv_lock ordering set up > > by vfio_pci_dma_buf_move(). Convert the @revoked bitfield to a plain bool > > so concurrent updates of @revoked (under dma_resv_lock) and the new TPH > > fields (under memory_lock) cannot race on a shared bitfield byte. > > The commit log includes many implementation details, why not remove or simply > it. > > > > > Signed-off-by: Zhiping Zhang <[email protected]> > > > > --- > > drivers/vfio/pci/vfio_pci_core.c | 3 + > > drivers/vfio/pci/vfio_pci_dmabuf.c | 113 ++++++++++++++++++++++++++++- > > drivers/vfio/pci/vfio_pci_priv.h | 11 +++ > > include/linux/dma-buf.h | 21 ++++++ > > include/uapi/linux/vfio.h | 35 +++++++++ > > 5 files changed, 182 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/vfio/pci/vfio_pci_core.c > > b/drivers/vfio/pci/vfio_pci_core.c > > index 3f8d093aacf8..94aa6dd95701 100644 > > --- a/drivers/vfio/pci/vfio_pci_core.c > > +++ b/drivers/vfio/pci/vfio_pci_core.c > > @@ -1534,6 +1534,9 @@ 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_DMA_BUF_TPH: > > + return vfio_pci_core_feature_dma_buf_tph(vdev, flags, arg, > > + argsz); > > default: > > return -ENOTTY; > > } > > diff --git a/drivers/vfio/pci/vfio_pci_dmabuf.c > > b/drivers/vfio/pci/vfio_pci_dmabuf.c > > index f87fd32e4a01..28247602e359 100644 > > --- a/drivers/vfio/pci/vfio_pci_dmabuf.c > > +++ b/drivers/vfio/pci/vfio_pci_dmabuf.c > > @@ -19,7 +19,23 @@ struct vfio_pci_dma_buf { > > u32 nr_ranges; > > struct kref kref; > > struct completion comp; > > - u8 revoked : 1; > > + /* > > + * TPH metadata published by VFIO_DEVICE_FEATURE_DMA_BUF_TPH and > > + * consumed by the @get_tph dma-buf callback. > > + * > > + * @tph_flags is the publish/consume gate: writers populate > > + * @steering_tag, @steering_tag_ext and @ph first, then store > > + * @tph_flags with smp_store_release(); readers do > > + * smp_load_acquire(&tph_flags) before accessing the value fields. > > + * @tph_flags == 0 means "TPH not set". Writers serialize via > > + * vdev->memory_lock; readers are lockless to avoid AB-BA against > > + * the dma_resv_lock held by importers. > > + */ > > + u32 tph_flags; > > As subsequent comments, can proceed without tph_flags > > > + u16 steering_tag; > > + u16 steering_tag_ext; > > + u8 ph; > > struct dma_buf_tph { > union { > atomic_t val; > struct { > u16 st_ext; > u8 st; > u8 ph; > }; > }; > }; > Set and get are done with atomic operation, no need for lock > > > + bool revoked; > > }; > > > > static int vfio_pci_dma_buf_attach(struct dma_buf *dmabuf, > > @@ -69,6 +85,35 @@ vfio_pci_dma_buf_map(struct dma_buf_attachment > > *attachment, > > return ret; > > } > > > > ... > > > > > + /** > > + * @get_tph: > > + * @dmabuf: DMA buffer for which to retrieve TPH metadata > > + * @steering_tag: Returns the raw TPH steering tag for @st_width > > + * @ph: Returns the TPH processing hint (2-bit value) > > + * @st_width: Consumer's supported steering tag width in bits (8 or > > 16) > > + * > > + * Return the TPH (TLP Processing Hints) metadata associated with this > > + * DMA buffer for the requested steering-tag width. 8-bit ST and > > 16-bit > > + * Extended ST are distinct namespaces in the PCIe TPH ST table, so > > the > > + * exporter must select the value that matches @st_width and must not > > + * substitute one for the other. > > + * > > + * Return 0 on success, -EOPNOTSUPP if no metadata is available for > > the > > + * requested width, or -EINVAL if @st_width is not 8 or 16. > > + * > > + * This callback is optional. > > + */ > > + int (*get_tph)(struct dma_buf *dmabuf, u16 *steering_tag, u8 *ph, > > + u8 st_width); > > how about rename steering_tag to st? > > > + > > /** > > * @map_dma_buf: > > * > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > > index 5de618a3a5ee..53b2bbd9fc1e 100644 > > --- a/include/uapi/linux/vfio.h > > +++ b/include/uapi/linux/vfio.h > > @@ -1534,6 +1534,41 @@ struct vfio_device_feature_dma_buf { > > */ > > #define VFIO_DEVICE_FEATURE_MIG_PRECOPY_INFOv2 12 > > > > +/** > > + * Upon VFIO_DEVICE_FEATURE_SET associate TPH (TLP Processing Hints) > > metadata > > + * with a vfio-exported dma-buf. The dma-buf must have been created by > > + * VFIO_DEVICE_FEATURE_DMA_BUF on this device. > > + * > > + * dmabuf_fd is the file descriptor returned by > > VFIO_DEVICE_FEATURE_DMA_BUF. > > + * > > + * 8-bit ST (steering_tag) and 16-bit Extended ST (steering_tag_ext) are > > + * distinct namespaces in the PCIe TPH ST table; userspace should populate > > + * the value(s) it has from the firmware ST table for this device and set > > + * the matching VFIO_DMA_BUF_TPH_ST / VFIO_DMA_BUF_TPH_ST_EXT bit in > > @flags. > > + * An importer requests a specific width and receives the matching value; > > + * if the requested width is not present, the importer is told TPH is > > + * unavailable for this dma-buf. > > + * > > + * ph is the 2-bit TLP Processing Hint and must be in the range [0, 3]. > > + * > > + * The user must set TPH on the dma-buf before the importer consumes it. > > + * > > + * Return: 0 on success, -errno on failure. > > -1 and errno is set on failure. > > > + */ > > +#define VFIO_DEVICE_FEATURE_DMA_BUF_TPH 13 > > + > > +#define VFIO_DMA_BUF_TPH_ST (1 << 0) /* steering_tag valid */ > > +#define VFIO_DMA_BUF_TPH_ST_EXT (1 << 1) /* steering_tag_ext > > valid */ > > It could be represented by judge whether steering_tag/ext == 0 > > > + > > +struct vfio_device_feature_dma_buf_tph { > > + __s32 dmabuf_fd; > > + __u32 flags; > > + __u16 steering_tag; > > + __u16 steering_tag_ext; > > + __u8 ph; > > + __u8 reserved[3]; > > How about: > struct vfio_device_feature_dma_buf_tph { > __s32 dmabuf_fd; > __u16 st_ext; > __u8 st; > __u8 ph; > } > If st_ext is not zero means it valid, and also with st field. > > Thanks > > > +}; > > + > > /* -------- API for Type1 VFIO IOMMU -------- */ > > > > /** >
