On Wed, May 13, 2026 at 11:08 PM Zhiping Zhang <[email protected]> wrote: > > 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. > >
sure, will do! > > > > > > 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? > > I'd like to keep "steering_tag" in this signature if you don't mind. The verbose name keeps the signature self-documenting. > > > + > > > /** > > > * @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. > > This documents the kernel-side return value; the -1 / errno form is what the libc ioctl(2) wrapper presents to userspace. > > > + */ > > > +#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 -------- */ > > > > > > /** > >
