Re: [PATCH v3 1/2] vfio: add dma-buf get_tph callback and DMA_BUF_TPH feature

[Zhiping Zhang <zhipingz@meta.com>]

On Tue, May 12, 2026 at 6:33 PM fengchengwen <fengchengwen@huawei.com> 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 <zhipingz@meta.com>
> >
> > ---
> >  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 -------- */
> >
> >  /**
>