Thread (4 messages) 4 messages, 2 authors, 2d ago

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

From: Alex Williamson <alex@shazbot.org>
Date: 2026-05-21 22:04:18
Also in: dri-devel, kvm, linux-pci, linux-rdma

On Tue, 19 May 2026 13:13:49 -0700
Zhiping Zhang [off-list ref] wrote:
Add a dma-buf get_tph callback for exporters to return TPH
(TLP Processing Hints) metadata, and add VFIO_DEVICE_FEATURE_DMA_BUF_TPH
so userspace can attach that metadata to a VFIO-exported dma-buf.
This should be two patches, the first extending the dma-buf framework
for the get_tph callback for explicit approval from dma-buf maintainers
(who are not even copied here).  The second the vfio-pci implementation
of get_tph.
 
8-bit ST and 16-bit Extended ST are distinct PCIe TPH namespaces; the
uAPI carries both with explicit validity flags so importers get the
value matching their requested width. SET is write-once per dma-buf;
the existing VFIO_DEVICE_FEATURE_DMA_BUF uAPI is unchanged.
I didn't see what motivated this write-once change, I thought we
understood that it was a userspace problem that the tph values need to
be set before providing the dma-buf fd to the importer and that races
relative to that are a userspace ordering problem.  Write-once seems
unnecessarily restrictive and there's no justification provided here.
 
quoted hunk
Signed-off-by: Zhiping Zhang <redacted>
---
 drivers/vfio/pci/vfio_pci_core.c   |   3 +
 drivers/vfio/pci/vfio_pci_dmabuf.c | 134 +++++++++++++++++++++++++++--
 drivers/vfio/pci/vfio_pci_priv.h   |  12 +++
 include/linux/dma-buf.h            |  21 +++++
 include/uapi/linux/vfio.h          |  35 ++++++++
 5 files changed, 198 insertions(+), 7 deletions(-)
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..be1c65385670 100644
--- a/drivers/vfio/pci/vfio_pci_dmabuf.c
+++ b/drivers/vfio/pci/vfio_pci_dmabuf.c
@@ -19,7 +19,24 @@ 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 publish a non-zero
+	 * value only once per dma-buf and serialize via vdev->memory_lock;
+	 * readers stay lockless to avoid AB-BA against the dma_resv_lock held
+	 * by importers.
+	 */
Can you outline the ABBA hazard, I'm not seeing it.  You're acquiring
memory_lock in the feature SET and dma_resv_lock doesn't appear to be
held when calling .get_tph().  There's a lot of lockless complication
here balanced on this claim of avoiding a hazard that doesn't appear
present.
+	u32 tph_flags;
+	u16 steering_tag_ext;
+	u8 steering_tag;
+	u8 ph;
+	bool revoked;
If we still used memory_lock for tph, these could be:

	u8 tph_st_valid:1; /* memory_lock */
	u8 tph_st_ext_valid:1; /* memory_lock */
	u8 tph_ph:2; /* memory_lock */
	u8 tph_st;
	u16 tph_st_ext;
	u8 revoked:1; /* dma_resv_lock */

The existing change of @revoked from bitfield to bool has no rationale
noted for it in the commit log.
quoted hunk
 };
 
 static int vfio_pci_dma_buf_attach(struct dma_buf *dmabuf,
@@ -69,6 +86,36 @@ vfio_pci_dma_buf_map(struct dma_buf_attachment *attachment,
 	return ret;
 }
 
+static int vfio_pci_dma_buf_get_tph(struct dma_buf *dmabuf, u16 *steering_tag,
+				    u8 *ph, u8 st_width)
+{
+	struct vfio_pci_dma_buf *priv = dmabuf->priv;
+	u32 flags;
+
+	/* Pair with the smp_store_release() in VFIO_DEVICE_FEATURE_DMA_BUF_TPH. */
+	flags = smp_load_acquire(&priv->tph_flags);
+	if (!flags)
+		return -EOPNOTSUPP;
+
+	switch (st_width) {
+	case 8:
+		if (!(flags & VFIO_DMA_BUF_TPH_ST))
+			return -EOPNOTSUPP;
+		*steering_tag = priv->steering_tag;
+		break;
+	case 16:
+		if (!(flags & VFIO_DMA_BUF_TPH_ST_EXT))
+			return -EOPNOTSUPP;
+		*steering_tag = priv->steering_tag_ext;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	*ph = priv->ph;
+	return 0;
+}
+
 static void vfio_pci_dma_buf_unmap(struct dma_buf_attachment *attachment,
 				   struct sg_table *sgt,
 				   enum dma_data_direction dir)
@@ -84,16 +131,17 @@ static void vfio_pci_dma_buf_unmap(struct dma_buf_attachment *attachment,
 static void vfio_pci_dma_buf_release(struct dma_buf *dmabuf)
 {
 	struct vfio_pci_dma_buf *priv = dmabuf->priv;
+	struct vfio_pci_core_device *vdev = READ_ONCE(priv->vdev);
 
 	/*
 	 * Either this or vfio_pci_dma_buf_cleanup() will remove from the list.
 	 * The refcount prevents both.
 	 */
-	if (priv->vdev) {
-		down_write(&priv->vdev->memory_lock);
+	if (vdev) {
+		down_write(&vdev->memory_lock);
 		list_del_init(&priv->dmabufs_elm);
-		up_write(&priv->vdev->memory_lock);
-		vfio_device_put_registration(&priv->vdev->vdev);
+		up_write(&vdev->memory_lock);
+		vfio_device_put_registration(&vdev->vdev);
 	}
 	kfree(priv->phys_vec);
 	kfree(priv);

This seems unnecessary.  I think this is just because priv->vdev is now
(unnecessarily) set via WRITE_ONCE, right?  These are very well ordered
paths, prior to exposing the dma-buf, while the device is opened, during
release, after release. They don't seem to need the READ/WRITE_ONCE
treatment.  This looks like noise from trying to make it lockless.

quoted hunk
@@ -101,6 +149,7 @@ static void vfio_pci_dma_buf_release(struct dma_buf *dmabuf)
 
 static const struct dma_buf_ops vfio_pci_dmabuf_ops = {
 	.attach = vfio_pci_dma_buf_attach,
+	.get_tph = vfio_pci_dma_buf_get_tph,
 	.map_dma_buf = vfio_pci_dma_buf_map,
 	.unmap_dma_buf = vfio_pci_dma_buf_unmap,
 	.release = vfio_pci_dma_buf_release,
@@ -269,7 +318,7 @@ int vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags,
 		goto err_free_priv;
 	}
 
-	priv->vdev = vdev;
+	WRITE_ONCE(priv->vdev, vdev);
 	priv->nr_ranges = get_dma_buf.nr_ranges;
 	priv->size = length;
 	ret = vdev->pci_ops->get_dmabuf_phys(vdev, &priv->provider,
@@ -331,6 +380,77 @@ int vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags,
 	return ret;
 }
 
+int vfio_pci_core_feature_dma_buf_tph(struct vfio_pci_core_device *vdev,
+				      u32 flags,
+				      struct vfio_device_feature_dma_buf_tph __user *arg,
+				      size_t argsz)
+{
+	struct vfio_device_feature_dma_buf_tph set_tph;
+	struct vfio_pci_dma_buf *priv;
+	struct dma_buf *dmabuf;
+	int ret;
+
+	ret = vfio_check_feature(flags, argsz, VFIO_DEVICE_FEATURE_SET,
+				 sizeof(set_tph));
+	if (ret != 1)
+		return ret;
+
+	if (copy_from_user(&set_tph, arg, sizeof(set_tph)))
+		return -EFAULT;
+
+	if (set_tph.flags & ~(VFIO_DMA_BUF_TPH_ST | VFIO_DMA_BUF_TPH_ST_EXT))
+		return -EINVAL;
+
+	if (!set_tph.flags)
+		return -EINVAL;
+
+	/* PCIe TLP Processing Hint is a 2-bit field. */
+	if (set_tph.ph & ~0x3)
+		return -EINVAL;
+
+	dmabuf = dma_buf_get(set_tph.dmabuf_fd);
+	if (IS_ERR(dmabuf))
+		return PTR_ERR(dmabuf);
+
+	if (dmabuf->ops != &vfio_pci_dmabuf_ops) {
+		ret = -EINVAL;
+		goto out_put;
+	}
+
+	priv = dmabuf->priv;
+	down_write(&vdev->memory_lock);
+	if (READ_ONCE(priv->vdev) != vdev) {
+		ret = -EINVAL;
+		goto out_unlock;
+	}
+
+	/*
+	 * TPH metadata is write-once per dma-buf so that lockless readers only
+	 * have to observe a single release-published transition from 0 -> flags.
+	 */
+	if (READ_ONCE(priv->tph_flags)) {
+		ret = -EBUSY;
+		goto out_unlock;
+	}
+
+	priv->steering_tag = set_tph.steering_tag;
+	priv->steering_tag_ext = set_tph.steering_tag_ext;
+	priv->ph = set_tph.ph;
+	/*
+	 * Publish the TPH values before the gate flag, so that lockless
+	 * readers in vfio_pci_dma_buf_get_tph() see fully-initialized
+	 * fields once they observe a non-zero tph_flags.
+	 */
+	smp_store_release(&priv->tph_flags, set_tph.flags);
+	ret = 0;
+
+out_unlock:
+	up_write(&vdev->memory_lock);
+out_put:
+	dma_buf_put(dmabuf);
+	return ret;
+}
+
 void vfio_pci_dma_buf_move(struct vfio_pci_core_device *vdev, bool revoked)
 {
 	struct vfio_pci_dma_buf *priv;
@@ -388,7 +508,7 @@ void vfio_pci_dma_buf_cleanup(struct vfio_pci_core_device *vdev)
 
 		dma_resv_lock(priv->dmabuf->resv, NULL);
 		list_del_init(&priv->dmabufs_elm);
-		priv->vdev = NULL;
+		WRITE_ONCE(priv->vdev, NULL);
 		priv->revoked = true;
 		dma_buf_invalidate_mappings(priv->dmabuf);
 		dma_resv_wait_timeout(priv->dmabuf->resv,
diff --git a/drivers/vfio/pci/vfio_pci_priv.h b/drivers/vfio/pci/vfio_pci_priv.h
index fca9d0dfac90..c58f369be4b3 100644
--- a/drivers/vfio/pci/vfio_pci_priv.h
+++ b/drivers/vfio/pci/vfio_pci_priv.h
@@ -118,6 +118,10 @@ static inline bool vfio_pci_is_vga(struct pci_dev *pdev)
 int vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags,
 				  struct vfio_device_feature_dma_buf __user *arg,
 				  size_t argsz);
+int vfio_pci_core_feature_dma_buf_tph(struct vfio_pci_core_device *vdev,
+				      u32 flags,
+				      struct vfio_device_feature_dma_buf_tph __user *arg,
+				      size_t argsz);
 void vfio_pci_dma_buf_cleanup(struct vfio_pci_core_device *vdev);
 void vfio_pci_dma_buf_move(struct vfio_pci_core_device *vdev, bool revoked);
 #else
@@ -128,6 +132,14 @@ vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags,
 {
 	return -ENOTTY;
 }
+
+static inline int
+vfio_pci_core_feature_dma_buf_tph(struct vfio_pci_core_device *vdev, u32 flags,
+				  struct vfio_device_feature_dma_buf_tph __user *arg,
+				  size_t argsz)
+{
+	return -ENOTTY;
+}
 static inline void vfio_pci_dma_buf_cleanup(struct vfio_pci_core_device *vdev)
 {
 }
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index d1203da56fc5..49eb6ad644a2 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -113,6 +113,27 @@ struct dma_buf_ops {
 	 */
 	void (*unpin)(struct dma_buf_attachment *attach);
 
+	/**
+	 * @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 and may
+	 * both be present with different values, 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);
+
 	/**
 	 * @map_dma_buf:
 	 *
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 5de618a3a5ee..a9cb6cbc6ade 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 and may both be present with
+ * different values. 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.
+ * TPH metadata is write-once per dma-buf; a second SET returns -EBUSY.
+ *
+ * Return: 0 on success, -errno 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 */
+
+struct vfio_device_feature_dma_buf_tph {
+	__s32	dmabuf_fd;
+	__u32	flags;
+	__u8	steering_tag;
+	__u8	ph;
+	__u16	steering_tag_ext;
+};
Sure is tempting to make the ph field the first 2-bits of u8 flags.
Thanks,

Alex
+
 /* -------- API for Type1 VFIO IOMMU -------- */
 
 /**
  
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help