Claude review: RDMA/mlx5: get tph for p2p access when registering dma-buf mr

Claude review: RDMA/mlx5: get tph for p2p access when registering dma-buf mr

[Claude Code Review Bot]

Patch Review

**Critical: Steering tag index resource leak**

```c
+	ret = mlx5_st_alloc_index_by_tag(dev->mdev, steering_tag, st_index);
+	if (ret) {
+		*ph = MLX5_IB_NO_PH;
+		mlx5_ib_dbg(dev, "st_alloc_index_by_tag failed (%d)\n", ret);
+	}
```

`mlx5_st_alloc_index_by_tag()` allocates a refcounted entry in `st->idx_xa` (in non-direct mode). The resulting `st_index` is passed into `alloc_cacheable_mr()` and written into the hardware MKC, but it is **never stored** in a way that allows `mlx5_st_dealloc_index()` to be called when the MR is destroyed. Looking at `__mlx5_ib_dereg_mr()`, there is no call to `mlx5_st_dealloc_index()` — that only happens through the DMAH path (`mlx5_ib_dealloc_dmah()`).

This means every MR registered via the new dma-buf TPH fallback path leaks an xarray entry. Over time, the steering tag table fills up and `xa_alloc()` will eventually fail.

The MR needs to track whether it owns an st_index allocation and call `mlx5_st_dealloc_index()` during `__mlx5_ib_dereg_mr()`.

**Redundant `dma_buf_get()`/`dma_buf_put()`**

```c
+static void get_tph_mr_dmabuf(struct mlx5_ib_dev *dev, int fd, u16 *st_index,
+			      u8 *ph)
+{
+	...
+	dmabuf = dma_buf_get(fd);
+	if (IS_ERR(dmabuf))
+		return;
+	...
+	dma_buf_put(dmabuf);
+}
```

This function does `dma_buf_get(fd)` / `dma_buf_put(dmabuf)` solely to query TPH. The caller `reg_user_mr_dmabuf()` will immediately afterward call `ib_umem_dmabuf_get()`, which does another `dma_buf_get()` on the same fd. This is a wasted get/put cycle. Consider restructuring so the TPH query happens after the dmabuf is already acquired (e.g., query from `umem_dmabuf->dmabuf`), avoiding the redundant fd lookup.

**Direct ops call bypasses dma-buf framework**

```c
+	if (!dmabuf->ops->get_tph)
+		goto end_dbuf_put;
+
+	ret = dmabuf->ops->get_tph(dmabuf, &steering_tag, ph, st_width);
```

This calls through the ops table directly rather than through a framework wrapper function. Every other dma-buf operation (map, unmap, pin, unpin, attach, etc.) goes through a wrapper in `drivers/dma-buf/dma-buf.c`. The NULL check should be inside the wrapper, not at the call site. This also means the call bypasses any future framework-level tracing, locking, or validation.

**`ret = 0` initialization in `mlx5_st_create()`**

```c
-	int ret;
+	int ret = 0;
```

This fix addresses a real compiler warning, but it would be better in a separate patch since it's an independent bugfix unrelated to the TPH-from-dma-buf feature. It could also be submitted as a standalone fix that can be backported independently.

**`MODULE_IMPORT_NS("DMA_BUF")`**

```c
+MODULE_IMPORT_NS("DMA_BUF");
```

This is needed because `dma_buf_get()` and `dma_buf_put()` are exported in the `DMA_BUF` namespace. Correct.

**`st_width` derivation**

```c
+	st_width = (pdev->tph_req_type == PCI_TPH_REQ_EXT_TPH) ? 16 : 8;
```

This checks `tph_req_type` but doesn't first verify that TPH is actually enabled/supported on this device. If `pdev->tph_req_type` is `PCI_TPH_REQ_DISABLE` or 0 (no TPH), the function will derive `st_width = 8` and proceed to allocate a steering tag that the device can't use. The function should early-return if the device doesn't support TPH (or `dev->st` is NULL), before even querying the dma-buf.

**Error handling: `ph` set to `MLX5_IB_NO_PH` on alloc failure but steering_tag left as-is**

```c
+	ret = mlx5_st_alloc_index_by_tag(dev->mdev, steering_tag, st_index);
+	if (ret) {
+		*ph = MLX5_IB_NO_PH;
```

When `mlx5_st_alloc_index_by_tag()` fails, `*ph` is reset to `MLX5_IB_NO_PH` but `*st_index` is left in an indeterminate state (it was passed as a pointer and `mlx5_st_alloc_index_by_tag()` may or may not have written to it). The caller initialized `st_index = MLX5_MKC_PCIE_TPH_NO_STEERING_TAG_INDEX`, which should be fine if `mlx5_st_alloc_index_by_tag()` doesn't modify `*st_index` on error, but this isn't guaranteed — the xarray path could partially succeed. Safer to explicitly reset `*st_index = MLX5_MKC_PCIE_TPH_NO_STEERING_TAG_INDEX` in the error path.

**Refactoring of `mlx5_st_alloc_index` is clean**

The split into `mlx5_st_alloc_index_by_tag()` (takes raw tag) and `mlx5_st_alloc_index()` (resolves CPU → tag, then calls `_by_tag`) is a clean refactor. The `#ifndef CONFIG_PCIE_TPH` static inline stub returning `-EOPNOTSUPP` is correct.

---
Generated by Claude Code Patch Reviewer

Claude review: RDMA/mlx5: get tph for p2p access when registering dma-buf mr

[Claude Code Review Bot]

Patch Review

**Double `dma_buf_get` on the same fd**

`get_tph_mr_dmabuf()` calls `dma_buf_get(fd)` to temporarily resolve the fd just to query TPH metadata:

```c
dmabuf = dma_buf_get(fd);
if (IS_ERR(dmabuf))
    return;
...
dma_buf_put(dmabuf);
```

The same fd is resolved again later by the main MR registration path (`ib_umem_dmabuf_get_pinned` or similar). This is functionally correct but does an unnecessary extra get/put cycle. Consider whether the dma_buf pointer could be passed from the caller instead, though I understand it may not be available at this point in the flow.

**Direct callback invocation instead of helper**

The code calls `dmabuf->ops->get_tph()` directly:

```c
if (!dmabuf->ops->get_tph)
    goto end_dbuf_put;

ret = dmabuf->ops->get_tph(dmabuf, &steering_tag, ph, st_width);
```

This is the standard pattern for optional dma-buf callbacks (same as how `pin`/`unpin` are called). It would be worth considering whether a `dma_buf_get_tph()` inline helper in `<linux/dma-buf.h>` would be better for encapsulation, especially if other importers will use this callback. But for an initial implementation with a single consumer, this is fine.

**Error handling in `get_tph_mr_dmabuf` is correct**

If `mlx5_st_alloc_index_by_tag` fails, `*ph` is reset to `MLX5_IB_NO_PH`:

```c
ret = mlx5_st_alloc_index_by_tag(dev->mdev, steering_tag, st_index);
if (ret) {
    *ph = MLX5_IB_NO_PH;
    ...
}
```

`*st_index` was never modified by the failed call, so it retains the caller's default (`MLX5_MKC_PCIE_TPH_NO_STEERING_TAG_INDEX`). This is correct — a failed TPH query silently falls back to no-TPH behavior.

**`ret = 0` initialization in `mlx5_st_create` is a real fix**

Looking at the existing code, when the `xa_for_each` loop finds a cached tag match:

```c
xa_for_each(&st->idx_xa, index, idx_data) {
    if (tag == idx_data->tag) {
        refcount_inc(&idx_data->usecount);
        *st_index = index;
        goto end;
    }
}
...
end:
    mutex_unlock(&st->lock);
    return ret;  // <-- was uninitialized on cache hit path
```

`ret` is genuinely uninitialized on the cache-hit `goto end` path. The `ret = 0` init in the refactored `mlx5_st_alloc_index_by_tag` fixes this bug. This is correct and should be called out more prominently — it's a real bug fix, not just a clang warning suppression.

**Refactoring of `mlx5_st_alloc_index` is clean**

The split into `mlx5_st_alloc_index_by_tag` (takes raw tag) + `mlx5_st_alloc_index` (wraps with `pcie_tph_get_cpu_st`) is a straightforward factoring. The original function body moves to `_by_tag`, and the original becomes a thin wrapper. The `#else` stub for `!CONFIG_PCIE_TPH` is correctly added.

**`MODULE_IMPORT_NS("DMA_BUF")` is needed**

`dma_buf_get` is exported under the `DMA_BUF` namespace, so this is required.

---
Generated by Claude Code Patch Reviewer

Claude review: RDMA/mlx5: get tph for p2p access when registering dma-buf mr

[Claude Code Review Bot]

Patch Review

**The `mlx5_st_alloc_index` refactoring is clean.** The `pcie_tph_get_cpu_st()` call is properly lifted into the wrapper, and the new `mlx5_st_alloc_index_by_tag()` takes the raw tag directly:

```c
int mlx5_st_alloc_index_by_tag(struct mlx5_core_dev *dev, u16 tag,
			       u16 *st_index)
{
	...
	if (st->direct_mode) {
		*st_index = tag;
		return 0;
	}
	...
```

**The `ret = 0` initialization in `mlx5_st_alloc_index_by_tag` is necessary.** After the refactoring, the cached path (`xa_for_each` finding a match and jumping to `end:`) returns `ret` without having set it — previously `pcie_tph_get_cpu_st()` returning 0 served as the implicit initialization, but that call is now in the wrapper.

**The `ret = 0` initialization in `mlx5_st_create` is unnecessary but harmless.** In the current code, `ret` is assigned by `pcie_enable_tph()` (line 58) and only used after that point. The function never reaches a point where `ret` is used without being set. This is likely a compiler warning suppression for clang. Fine, but might be better as a separate patch since the commit message describes it as "Initialize ret in mlx5_st_create()" alongside unrelated functional changes.

**Issue: Bitfield sharing in `mlx5_ib_mr`.** The new `dmabuf_st_owned:1` is packed into the same byte as `revoked:1` and `dmabuf_faulted:1`:

```c
u8 revoked :1;
u8 dmabuf_faulted:1;
u8 dmabuf_st_owned:1;
u16 dmabuf_st_index;
```

Patch 1's commit message explicitly calls out converting `revoked` from a bitfield to a bool in `vfio_pci_dma_buf` because "`revoked` is written under `dma_resv_lock`; the new TPH fields are written under `memory_lock`." The same class of concern applies here: `revoked` is set under `dma_resv_lock`, `dmabuf_faulted` is set in the page fault path, and `dmabuf_st_owned` is set/cleared during registration/cleanup. If any pair is updated concurrently under different locks, the non-atomic RMW on the shared byte is a bug. This pre-exists (revoked + dmabuf_faulted already share a byte), but the patch should not make it worse without confirming the locking is safe.

**TOCTOU fix is good.** Using `umem_dmabuf->attach->dmabuf` (already resolved) instead of `dma_buf_get(fd)` eliminates a real race:

```c
} else {
	get_tph_mr_dmabuf(dev, umem_dmabuf->attach->dmabuf,
			  &st_index, &ph);
}
```

**ST index cleanup covers both paths.** The FRMR cache path and the destroy_mkey path both call `mlx5_ib_mr_put_dmabuf_st()`:

```c
// FRMR cache path (line ~1380):
if (mr->ibmr.frmr.pool && !mlx5_umr_revoke_mr_with_lock(mr) &&
    !ib_frmr_pool_push(mr->ibmr.device, &mr->ibmr)) {
	mlx5_ib_mr_put_dmabuf_st(mr);
	return 0;
}

// destroy_mkey path (line ~1468):
if (!ret)
	mlx5_ib_mr_put_dmabuf_st(mr);
```

This is correct — the ST index must be freed once the firmware mkey is revoked or destroyed.

**Error path cleanup in `reg_user_mr_dmabuf` is correct:**

```c
if (IS_ERR(mr)) {
	if (!dmah && st_index != MLX5_MKC_PCIE_TPH_NO_STEERING_TAG_INDEX)
		mlx5_st_dealloc_index(dev->mdev, st_index);
	ib_umem_release(&umem_dmabuf->umem);
	return ERR_CAST(mr);
}
```

**`pcie_tph_get_st_width` uses `EXPORT_SYMBOL`** (not GPL), consistent with the other exports in `drivers/pci/tph.c`. The stub for `!CONFIG_PCIE_TPH` correctly returns 0.

**Minor: `get_tph_mr_dmabuf` is a void function that silently falls through on failure.** This is intentional — the caller initializes `st_index` and `ph` to the no-TPH defaults, and the function leaves them unchanged on failure. The `mlx5_ib_dbg` calls provide observability. The pattern is fine, but the function's contract (modifies outputs only on success, leaves them at caller's defaults on failure) could be clearer in the comment, since the `*ph = MLX5_IB_NO_PH` reset in error paths suggests the function *does* touch the outputs on failure in some cases. The inconsistency between "leave as default" and "reset to default" is confusing — on `get_tph` callback failure, `*ph` is explicitly reset to `MLX5_IB_NO_PH`, but on `!get_tph` or `!st_width`, the values are left untouched. The effect is the same (the caller initialized `ph = MLX5_IB_NO_PH`), but it reads as if the function is inconsistent about its own contract.

---
Generated by Claude Code Patch Reviewer

Claude review: RDMA/mlx5: get tph for p2p access when registering dma-buf mr

[Claude Code Review Bot]

Patch Review

**kfree leak fix (st.c)**

```c
if (refcount_dec_and_test(&idx_data->usecount)) {
	xa_erase(&st->idx_xa, st_index);
+	kfree(idx_data);
	/* We leave PCI config space as was before ... */
}
```

Confirmed — the current tree at `st.c:176-179` has `xa_erase` without `kfree`, leaking `idx_data`. This is a real pre-existing bug fix. Should arguably be a separate patch with its own Fixes: tag since it affects existing users of `mlx5_st_dealloc_index`, not just the new dma-buf path.

**Refactoring mlx5_st_alloc_index (st.c)**

The split into `mlx5_st_alloc_index_by_tag` + a thin wrapper `mlx5_st_alloc_index` is clean. The `int ret = 0` initialization silences the clang warning for the `direct_mode` early-return path where `ret` was previously uninitialized. The export is `_GPL`, matching existing conventions.

**get_tph_mr_dmabuf (mr.c)**

```c
req_type = pcie_tph_enabled_req_type(dev->mdev->pdev);
switch (req_type) {
case PCI_TPH_REQ_TPH_ONLY:
	st_width = 8;
	break;
case PCI_TPH_REQ_EXT_TPH:
	st_width = 16;
	break;
default:
	return;
}
```

This correctly maps the enabled requester type to the ST width for the callback.

**Concern**: After `get_tph` fails, the code does:

```c
if (ret) {
	mlx5_ib_dbg(dev, "get_tph failed (%d)\n", ret);
	*ph = MLX5_IB_NO_PH;
	return;
}
```

This resets `*ph` to `MLX5_IB_NO_PH` but `*st_index` remains `MLX5_MKC_PCIE_TPH_NO_STEERING_TAG_INDEX` from the caller's initialization, so the caller won't attempt deallocation. Correct.

**ST index lifecycle (mr.c)**

The `dmabuf_st_owned` flag and `mlx5_ib_mr_put_dmabuf_st` cleanup function correctly handle ownership:

1. On `alloc_cacheable_mr` failure, immediate dealloc before returning error.
2. On success, ownership transferred to MR via `dmabuf_st_owned = 1`.
3. In `mlx5r_handle_mkey_cleanup`, the ST is released after the mkey is revoked but before the MR re-enters the FRMR pool — this ordering is critical and correct.
4. In the non-FRMR cleanup path, `mlx5_ib_mr_put_dmabuf_st` is called after successful mkey cleanup.

```c
if (mr->ibmr.frmr.pool && !mlx5_umr_revoke_mr_with_lock(mr)) {
	mlx5_ib_mr_put_dmabuf_st(mr);
	if (!ib_frmr_pool_push(mr->ibmr.device, &mr->ibmr))
		return 0;
}
```

This ensures the ST index is freed before an MR gets recycled via the FRMR pool, preventing a reused MR from referencing a stale firmware ST entry. Good fix.

**MODULE_IMPORT_NS("DMA_BUF")**

```c
MODULE_IMPORT_NS("DMA_BUF");
```

Required for using dma_buf symbols. Correct.

**Bitfield placement (mlx5_ib.h)**

```c
u8 revoked :1;
u8 dmabuf_faulted:1;
+u8 dmabuf_st_owned:1;
+u16 dmabuf_st_index;
```

Adding these fields inside the anonymous struct within the union is fine, but the `u16 dmabuf_st_index` after three 1-bit `u8` bitfields may introduce padding depending on compiler/ABI. This should be checked — the anonymous struct contains other fields too (`null_mmkey`), so it likely has room, but it's worth verifying that the union size doesn't unexpectedly grow. Not a blocking issue.

**Summary of concerns:**

1. **(Minor)** The `kfree(idx_data)` leak fix in `st.c` is a pre-existing bug — consider splitting it into its own patch with a `Fixes:` tag so it can be backported independently.
2. **(Minor)** The `revoked` type change from bitfield to `bool` in patch 1 is unrelated to TPH and should be mentioned in the commit message.
3. **(Observation)** No `PROBE`/`GET` support for `VFIO_DEVICE_FEATURE_DMA_BUF_TPH` — userspace can't discover whether the feature is available. The `vfio_check_feature` call only allows `SET`. This may be intentional (probe via attempting SET, or probe via the existing DMA_BUF feature), but is worth documenting in the uAPI comment.

---
Generated by Claude Code Patch Reviewer

[PATCH v5 0/4] vfio/dma-buf: add TPH support for peer-to-peer access

[Zhiping Zhang]

This series adds TLP Processing Hints (TPH) support to the VFIO dma-buf
export path, allowing importing drivers (e.g. mlx5) to use the
exporter's steering tag when performing peer-to-peer DMA into a
VFIO-owned device.

Patch 1 exposes the enabled TPH requester type through a small PCI/TPH
helper so callers don't reach into pci_dev internals.
Patch 2 adds the optional dma_buf_ops::get_tph callback to the dma-buf
framework so importers can fetch TPH metadata from an exporter.
Patch 3 implements get_tph in vfio-pci and adds the new uAPI
(VFIO_DEVICE_FEATURE_DMA_BUF_TPH) for userspace to attach the metadata.
Patch 4 wires up the mlx5 RDMA driver as a consumer.

Previous link:
v4: https://lore.kernel.org/linux-pci/20260519201401.1558410-1-zhipingz@meta.com/
v3: https://lore.kernel.org/linux-pci/20260512184755.4137227-1-zhipingz@meta.com/
v2: https://lore.kernel.org/linux-pci/20260430200704.352228-1-zhipingz@meta.com/

Zhiping Zhang (4):
  PCI/TPH: expose the enabled TPH requester type
  dma-buf: add optional get_tph() callback
  vfio/pci: implement get_tph and DMA_BUF_TPH feature
  RDMA/mlx5: get tph for p2p access when registering dma-buf mr

 drivers/infiniband/hw/mlx5/mlx5_ib.h          |   6 +
 drivers/infiniband/hw/mlx5/mr.c               |  86 +++++++++++++-
 .../net/ethernet/mellanox/mlx5/core/lib/st.c  |  28 +++--
 drivers/pci/tph.c                             |  12 ++
 drivers/vfio/pci/vfio_pci_core.c              |   3 +
 drivers/vfio/pci/vfio_pci_dmabuf.c            | 110 +++++++++++++++++-
 drivers/vfio/pci/vfio_pci_priv.h              |  12 ++
 include/linux/dma-buf.h                       |  21 ++++
 include/linux/mlx5/driver.h                   |   7 ++
 include/linux/pci-tph.h                       |   2 +
 include/uapi/linux/vfio.h                     |  37 ++++++
 11 files changed, 311 insertions(+), 13 deletions(-)

-- 
2.53.0-Meta

[PATCH v5 1/4] PCI/TPH: expose the enabled TPH requester type

[Zhiping Zhang]

Add pcie_tph_enabled_req_type() so drivers can query the enabled TPH
requester mode without reaching into pci_dev internals.

This keeps pci_dev::tph_req_type inside the PCI/TPH code and provides a
!CONFIG_PCIE_TPH stub for callers.

Signed-off-by: Zhiping Zhang <zhipingz@meta.com>
---
 drivers/pci/tph.c       | 12 ++++++++++++
 include/linux/pci-tph.h |  2 ++
 2 files changed, 14 insertions(+)

diff --git a/drivers/pci/tph.c b/drivers/pci/tph.c
index 91145e8d9d95..6c4492075ae9 100644
--- a/drivers/pci/tph.c
+++ b/drivers/pci/tph.c
@@ -174,6 +174,18 @@ u32 pcie_tph_get_st_table_loc(struct pci_dev *pdev)
 }
 EXPORT_SYMBOL(pcie_tph_get_st_table_loc);
 
+/**
+ * pcie_tph_enabled_req_type - Return the device's enabled TPH requester type
+ * @pdev: PCI device to query
+ *
+ * Return: PCI_TPH_REQ_DISABLE, PCI_TPH_REQ_TPH_ONLY or PCI_TPH_REQ_EXT_TPH.
+ */
+u8 pcie_tph_enabled_req_type(struct pci_dev *pdev)
+{
+	return pdev->tph_req_type;
+}
+EXPORT_SYMBOL(pcie_tph_enabled_req_type);
+
 /*
  * Return the size of ST table. If ST table is not in TPH Requester Extended
  * Capability space, return 0. Otherwise return the ST Table Size + 1.
diff --git a/include/linux/pci-tph.h b/include/linux/pci-tph.h
index be68cd17f2f8..fe572737b409 100644
--- a/include/linux/pci-tph.h
+++ b/include/linux/pci-tph.h
@@ -30,6 +30,7 @@ void pcie_disable_tph(struct pci_dev *pdev);
 int pcie_enable_tph(struct pci_dev *pdev, int mode);
 u16 pcie_tph_get_st_table_size(struct pci_dev *pdev);
 u32 pcie_tph_get_st_table_loc(struct pci_dev *pdev);
+u8 pcie_tph_enabled_req_type(struct pci_dev *pdev);
 #else
 static inline int pcie_tph_set_st_entry(struct pci_dev *pdev,
 					unsigned int index, u16 tag)
@@ -41,6 +42,7 @@ static inline int pcie_tph_get_cpu_st(struct pci_dev *dev,
 static inline void pcie_disable_tph(struct pci_dev *pdev) { }
 static inline int pcie_enable_tph(struct pci_dev *pdev, int mode)
 { return -EINVAL; }
+static inline u8 pcie_tph_enabled_req_type(struct pci_dev *pdev) { return 0; }
 #endif
 
 #endif /* LINUX_PCI_TPH_H */
-- 
2.53.0-Meta

[PATCH v5 2/4] dma-buf: add optional get_tph() callback

[Zhiping Zhang]

Add an optional dma-buf get_tph callback so an exporter can return TPH
(TLP Processing Hints) metadata to an importer.

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. The importer
passes its supported steering-tag width and the exporter returns the
matching value, or -EOPNOTSUPP if no metadata is available for that
width.

The first user is VFIO_DEVICE_FEATURE_DMA_BUF_TPH in vfio-pci, with the
mlx5 RDMA driver as the first importer.

Signed-off-by: Zhiping Zhang <zhipingz@meta.com>
---
 include/linux/dma-buf.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

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:
 	 *
-- 
2.53.0-Meta

[PATCH v5 3/4] vfio/pci: implement get_tph and DMA_BUF_TPH feature

[Zhiping Zhang]

Implement the dma-buf get_tph callback for vfio-pci-exported dma-bufs
and add VFIO_DEVICE_FEATURE_DMA_BUF_TPH so userspace can attach TPH
metadata to such a dma-buf.

8-bit ST and 16-bit Extended ST are distinct PCIe TPH namespaces; the
uAPI carries both with explicit validity flags, and get_tph() returns
the value matching the importer's requested width (or -EOPNOTSUPP).

The TPH descriptor is published under a new per-dma-buf mutex
(priv->lock) and read by get_tph() under the same mutex. The same
mutex serialises with the priv->vdev clear in
vfio_pci_dma_buf_cleanup() so a SET racing with device teardown
cannot observe a half-detached dma-buf. memory_lock remain on the
existing dma-buf paths; the outer order is memory_lock -> priv->lock.

Signed-off-by: Zhiping Zhang <zhipingz@meta.com>
---
 drivers/vfio/pci/vfio_pci_core.c   |   3 +
 drivers/vfio/pci/vfio_pci_dmabuf.c | 110 ++++++++++++++++++++++++++++-
 drivers/vfio/pci/vfio_pci_priv.h   |  12 ++++
 include/uapi/linux/vfio.h          |  37 ++++++++++
 4 files changed, 161 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index 050e7542952e..4fa36f2f7555 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -1569,6 +1569,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 1a177ce7de54..3ea2978c376c 100644
--- a/drivers/vfio/pci/vfio_pci_dmabuf.c
+++ b/drivers/vfio/pci/vfio_pci_dmabuf.c
@@ -19,7 +19,19 @@ struct vfio_pci_dma_buf {
 	u32 nr_ranges;
 	struct kref kref;
 	struct completion comp;
-	u8 revoked : 1;
+	/*
+	 * @lock serializes TPH SET vs get_tph and the priv->vdev clear in
+	 * vfio_pci_dma_buf_cleanup(). It nests inside memory_lock:
+	 * the outer order across these paths is
+	 * memory_lock -> priv->lock.
+	 */
+	struct mutex lock;
+	u8 tph_st_valid:1;	/* priv->lock */
+	u8 tph_st_ext_valid:1;	/* priv->lock */
+	u8 tph_ph:2;		/* priv->lock */
+	u8 tph_st;		/* priv->lock */
+	u16 tph_st_ext;		/* priv->lock */
+	u8 revoked:1;		/* dma_resv_lock */
 };
 
 static int vfio_pci_dma_buf_attach(struct dma_buf *dmabuf,
@@ -69,6 +81,38 @@ 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;
+	int ret = 0;
+
+	mutex_lock(&priv->lock);
+	switch (st_width) {
+	case 8:
+		if (!priv->tph_st_valid) {
+			ret = -EOPNOTSUPP;
+			break;
+		}
+		*steering_tag = priv->tph_st;
+		*ph = priv->tph_ph;
+		break;
+	case 16:
+		if (!priv->tph_st_ext_valid) {
+			ret = -EOPNOTSUPP;
+			break;
+		}
+		*steering_tag = priv->tph_st_ext;
+		*ph = priv->tph_ph;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+	mutex_unlock(&priv->lock);
+	return ret;
+}
+
 static void vfio_pci_dma_buf_unmap(struct dma_buf_attachment *attachment,
 				   struct sg_table *sgt,
 				   enum dma_data_direction dir)
@@ -95,12 +139,14 @@ static void vfio_pci_dma_buf_release(struct dma_buf *dmabuf)
 		up_write(&priv->vdev->memory_lock);
 		vfio_device_put_registration(&priv->vdev->vdev);
 	}
+	mutex_destroy(&priv->lock);
 	kfree(priv->phys_vec);
 	kfree(priv);
 }
 
 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,
@@ -265,6 +311,7 @@ int vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags,
 		ret = -ENOMEM;
 		goto err_free_ranges;
 	}
+	mutex_init(&priv->lock);
 	priv->phys_vec = kzalloc_objs(*priv->phys_vec, get_dma_buf.nr_ranges);
 	if (!priv->phys_vec) {
 		ret = -ENOMEM;
@@ -327,12 +374,71 @@ int vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags,
 err_free_phys:
 	kfree(priv->phys_vec);
 err_free_priv:
+	mutex_destroy(&priv->lock);
 	kfree(priv);
 err_free_ranges:
 	kfree(dma_ranges);
 	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;
+	mutex_lock(&priv->lock);
+	if (priv->vdev != vdev) {
+		ret = -EINVAL;
+		goto out_unlock;
+	}
+
+	priv->tph_st = set_tph.steering_tag;
+	priv->tph_st_ext = set_tph.steering_tag_ext;
+	priv->tph_ph = set_tph.ph;
+	priv->tph_st_valid = !!(set_tph.flags & VFIO_DMA_BUF_TPH_ST);
+	priv->tph_st_ext_valid = !!(set_tph.flags & VFIO_DMA_BUF_TPH_ST_EXT);
+	ret = 0;
+
+out_unlock:
+	mutex_unlock(&priv->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;
@@ -398,7 +504,9 @@ void vfio_pci_dma_buf_cleanup(struct vfio_pci_core_device *vdev)
 			continue;
 
 		list_del_init(&priv->dmabufs_elm);
+		mutex_lock(&priv->lock);
 		priv->vdev = NULL;
+		mutex_unlock(&priv->lock);
 		vfio_device_put_registration(&vdev->vdev);
 		fput(priv->dmabuf->file);
 	}
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/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 5de618a3a5ee..55cac3b7122c 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -1534,6 +1534,43 @@ 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].
+ *
+ * Userspace is responsible for setting TPH on the dma-buf before handing the
+ * fd to the importer. Calling SET again replaces the previously published
+ * values; racing a SET against an importer that is already consuming the
+ * dma-buf is a userspace ordering problem.
+ *
+ * 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;
+};
+
 /* -------- API for Type1 VFIO IOMMU -------- */
 
 /**
-- 
2.53.0-Meta

[PATCH v5 4/4] RDMA/mlx5: get tph for p2p access when registering dma-buf mr

[Zhiping Zhang]

Query dma-buf TPH metadata when registering a dma-buf MR for peer-to-
peer access and translate the returned steering tag into an mlx5 ST
index. Keep the DMAH path as the first priority and only fall back to
DMA-buf metadata when no DMAH is supplied.

Track per-MR ownership of the allocated ST index and release it on MR
setup failure, destroy, and FRMR-pool reuse. Release the ST index before
the MR is pushed back into the FRMR pool, and free mlx5_st_idx_data when
its refcount reaches zero so repeated allocation/deallocation does not
leak memory.

Signed-off-by: Zhiping Zhang <zhipingz@meta.com>
---
 drivers/infiniband/hw/mlx5/mlx5_ib.h          |  6 ++
 drivers/infiniband/hw/mlx5/mr.c               | 86 ++++++++++++++++++-
 .../net/ethernet/mellanox/mlx5/core/lib/st.c  | 28 ++++--
 include/linux/mlx5/driver.h                   |  7 ++
 4 files changed, 115 insertions(+), 12 deletions(-)

diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
index e156dc4d7529..4ab867392267 100644
--- a/drivers/infiniband/hw/mlx5/mlx5_ib.h
+++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
@@ -721,6 +721,12 @@ struct mlx5_ib_mr {
 			u8 revoked :1;
 			/* Indicates previous dmabuf page fault occurred */
 			u8 dmabuf_faulted:1;
+			/* Set when the MR owns dmabuf_st_index and must
+			 * release it via mlx5_st_dealloc_index() once the
+			 * firmware mkey is no longer referencing it.
+			 */
+			u8 dmabuf_st_owned:1;
+			u16 dmabuf_st_index;
 			struct mlx5_ib_mkey null_mmkey;
 		};
 	};
diff --git a/drivers/infiniband/hw/mlx5/mr.c b/drivers/infiniband/hw/mlx5/mr.c
index 3b6da45061a5..8059b5e4da97 100644
--- a/drivers/infiniband/hw/mlx5/mr.c
+++ b/drivers/infiniband/hw/mlx5/mr.c
@@ -38,6 +38,7 @@
 #include <linux/delay.h>
 #include <linux/dma-buf.h>
 #include <linux/dma-resv.h>
+#include <linux/pci-tph.h>
 #include <rdma/frmr_pools.h>
 #include <rdma/ib_umem_odp.h>
 #include "dm.h"
@@ -46,6 +47,8 @@
 #include "data_direct.h"
 #include "dmah.h"
 
+MODULE_IMPORT_NS("DMA_BUF");
+
 static int mkey_max_umr_order(struct mlx5_ib_dev *dev)
 {
 	if (MLX5_CAP_GEN(dev->mdev, umr_extended_translation_offset))
@@ -899,6 +902,63 @@ static struct dma_buf_attach_ops mlx5_ib_dmabuf_attach_ops = {
 	.invalidate_mappings = mlx5_ib_dmabuf_invalidate_cb,
 };
 
+/*
+ * Query TPH metadata from @dmabuf and translate the raw steering tag into
+ * an mlx5 ST index. On success, returns 0 and the caller becomes the
+ * owner of *@st_index (must be released with mlx5_st_dealloc_index()
+ * once the firmware mkey no longer references it). On any failure
+ * *@st_index and *@ph are left as the no-TPH defaults set by the caller.
+ *
+ * @dmabuf must already be referenced by the caller (e.g. via the umem's
+ * attachment) so we don't re-resolve the user's fd here and avoid a
+ * dup2() TOCTOU between umem creation and TPH lookup.
+ */
+static void get_tph_mr_dmabuf(struct mlx5_ib_dev *dev, struct dma_buf *dmabuf,
+			      u16 *st_index, u8 *ph)
+{
+	u8 req_type;
+	u16 steering_tag;
+	u8 st_width;
+	int ret;
+
+	if (!dmabuf->ops->get_tph)
+		return;
+
+	req_type = pcie_tph_enabled_req_type(dev->mdev->pdev);
+	switch (req_type) {
+	case PCI_TPH_REQ_TPH_ONLY:
+		st_width = 8;
+		break;
+	case PCI_TPH_REQ_EXT_TPH:
+		st_width = 16;
+		break;
+	default:
+		return;
+	}
+
+	ret = dmabuf->ops->get_tph(dmabuf, &steering_tag, ph, st_width);
+	if (ret) {
+		mlx5_ib_dbg(dev, "get_tph failed (%d)\n", ret);
+		*ph = MLX5_IB_NO_PH;
+		return;
+	}
+
+	ret = mlx5_st_alloc_index_by_tag(dev->mdev, steering_tag, st_index);
+	if (ret) {
+		*ph = MLX5_IB_NO_PH;
+		mlx5_ib_dbg(dev, "st_alloc_index_by_tag failed (%d)\n", ret);
+	}
+}
+
+static void mlx5_ib_mr_put_dmabuf_st(struct mlx5_ib_mr *mr)
+{
+	if (mr->umem && mr->dmabuf_st_owned) {
+		mlx5_st_dealloc_index(mr_to_mdev(mr)->mdev,
+				      mr->dmabuf_st_index);
+		mr->dmabuf_st_owned = 0;
+	}
+}
+
 static struct ib_mr *
 reg_user_mr_dmabuf(struct ib_pd *pd, struct device *dma_device,
 		   u64 offset, u64 length, u64 virt_addr,
@@ -941,16 +1001,26 @@ reg_user_mr_dmabuf(struct ib_pd *pd, struct device *dma_device,
 		ph = dmah->ph;
 		if (dmah->valid_fields & BIT(IB_DMAH_CPU_ID_EXISTS))
 			st_index = mdmah->st_index;
+	} else {
+		get_tph_mr_dmabuf(dev, umem_dmabuf->attach->dmabuf,
+				  &st_index, &ph);
 	}
 
 	mr = alloc_cacheable_mr(pd, &umem_dmabuf->umem, virt_addr,
 				access_flags, access_mode,
 				st_index, ph);
 	if (IS_ERR(mr)) {
+		if (!dmah && st_index != MLX5_MKC_PCIE_TPH_NO_STEERING_TAG_INDEX)
+			mlx5_st_dealloc_index(dev->mdev, st_index);
 		ib_umem_release(&umem_dmabuf->umem);
 		return ERR_CAST(mr);
 	}
 
+	if (!dmah && st_index != MLX5_MKC_PCIE_TPH_NO_STEERING_TAG_INDEX) {
+		mr->dmabuf_st_index = st_index;
+		mr->dmabuf_st_owned = 1;
+	}
+
 	mlx5_ib_dbg(dev, "mkey 0x%x\n", mr->mmkey.key);
 
 	atomic_add(ib_umem_num_pages(mr->umem), &dev->mdev->priv.reg_pages);
@@ -1377,9 +1447,17 @@ static int mlx5r_handle_mkey_cleanup(struct mlx5_ib_mr *mr)
 	bool is_odp = is_odp_mr(mr);
 	int ret;
 
-	if (mr->ibmr.frmr.pool && !mlx5_umr_revoke_mr_with_lock(mr) &&
-	    !ib_frmr_pool_push(mr->ibmr.device, &mr->ibmr))
-		return 0;
+	if (mr->ibmr.frmr.pool && !mlx5_umr_revoke_mr_with_lock(mr)) {
+		/*
+		 * The mkey has been revoked: firmware no longer references
+		 * dmabuf_st_index, so release it before this mr can re-enter
+		 * the FRMR cache for reuse by another registration.
+		 */
+		mlx5_ib_mr_put_dmabuf_st(mr);
+
+		if (!ib_frmr_pool_push(mr->ibmr.device, &mr->ibmr))
+			return 0;
+	}
 
 	if (is_odp)
 		mutex_lock(&to_ib_umem_odp(mr->umem)->umem_mutex);
@@ -1400,6 +1478,8 @@ static int mlx5r_handle_mkey_cleanup(struct mlx5_ib_mr *mr)
 		dma_resv_unlock(
 			to_ib_umem_dmabuf(mr->umem)->attach->dmabuf->resv);
 	}
+	if (!ret)
+		mlx5_ib_mr_put_dmabuf_st(mr);
 	return ret;
 }
 
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/st.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/st.c
index 997be91f0a13..8929c17c88bc 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lib/st.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/st.c
@@ -29,7 +29,7 @@ struct mlx5_st *mlx5_st_create(struct mlx5_core_dev *dev)
 	u8 direct_mode = 0;
 	u16 num_entries;
 	u32 tbl_loc;
-	int ret;
+	int ret = 0;
 
 	if (!MLX5_CAP_GEN(dev, mkey_pcie_tph))
 		return NULL;
@@ -92,23 +92,18 @@ void mlx5_st_destroy(struct mlx5_core_dev *dev)
 	kfree(st);
 }
 
-int mlx5_st_alloc_index(struct mlx5_core_dev *dev, enum tph_mem_type mem_type,
-			unsigned int cpu_uid, u16 *st_index)
+int mlx5_st_alloc_index_by_tag(struct mlx5_core_dev *dev, u16 tag,
+			       u16 *st_index)
 {
 	struct mlx5_st_idx_data *idx_data;
 	struct mlx5_st *st = dev->st;
 	unsigned long index;
 	u32 xa_id;
-	u16 tag;
-	int ret;
+	int ret = 0;
 
 	if (!st)
 		return -EOPNOTSUPP;
 
-	ret = pcie_tph_get_cpu_st(dev->pdev, mem_type, cpu_uid, &tag);
-	if (ret)
-		return ret;
-
 	if (st->direct_mode) {
 		*st_index = tag;
 		return 0;
@@ -152,6 +147,20 @@ int mlx5_st_alloc_index(struct mlx5_core_dev *dev, enum tph_mem_type mem_type,
 	mutex_unlock(&st->lock);
 	return ret;
 }
+EXPORT_SYMBOL_GPL(mlx5_st_alloc_index_by_tag);
+
+int mlx5_st_alloc_index(struct mlx5_core_dev *dev, enum tph_mem_type mem_type,
+			unsigned int cpu_uid, u16 *st_index)
+{
+	u16 tag;
+	int ret;
+
+	ret = pcie_tph_get_cpu_st(dev->pdev, mem_type, cpu_uid, &tag);
+	if (ret)
+		return ret;
+
+	return mlx5_st_alloc_index_by_tag(dev, tag, st_index);
+}
 EXPORT_SYMBOL_GPL(mlx5_st_alloc_index);
 
 int mlx5_st_dealloc_index(struct mlx5_core_dev *dev, u16 st_index)
@@ -175,6 +184,7 @@ int mlx5_st_dealloc_index(struct mlx5_core_dev *dev, u16 st_index)
 
 	if (refcount_dec_and_test(&idx_data->usecount)) {
 		xa_erase(&st->idx_xa, st_index);
+		kfree(idx_data);
 		/* We leave PCI config space as was before, no mkey will refer to it */
 	}
 
diff --git a/include/linux/mlx5/driver.h b/include/linux/mlx5/driver.h
index 04b96c5abb57..523a9ab0ae1e 100644
--- a/include/linux/mlx5/driver.h
+++ b/include/linux/mlx5/driver.h
@@ -1166,10 +1166,17 @@ int mlx5_dm_sw_icm_dealloc(struct mlx5_core_dev *dev, enum mlx5_sw_icm_type type
 			   u64 length, u16 uid, phys_addr_t addr, u32 obj_id);
 
 #ifdef CONFIG_PCIE_TPH
+int mlx5_st_alloc_index_by_tag(struct mlx5_core_dev *dev, u16 tag,
+			       u16 *st_index);
 int mlx5_st_alloc_index(struct mlx5_core_dev *dev, enum tph_mem_type mem_type,
 			unsigned int cpu_uid, u16 *st_index);
 int mlx5_st_dealloc_index(struct mlx5_core_dev *dev, u16 st_index);
 #else
+static inline int mlx5_st_alloc_index_by_tag(struct mlx5_core_dev *dev,
+					     u16 tag, u16 *st_index)
+{
+	return -EOPNOTSUPP;
+}
 static inline int mlx5_st_alloc_index(struct mlx5_core_dev *dev,
 				      enum tph_mem_type mem_type,
 				      unsigned int cpu_uid, u16 *st_index)
-- 
2.53.0-Meta

Claude review: vfio/dma-buf: add TPH support for peer-to-peer access

[Claude Code Review Bot]

Overall Series Review

Subject: vfio/dma-buf: add TPH support for peer-to-peer access
Author: Zhiping Zhang <zhipingz@meta.com>
Patches: 5
Reviewed: 2026-05-27T14:35:49.137197

---

This v5 series adds TLP Processing Hints (TPH) support to the VFIO dma-buf export path, enabling RDMA drivers (mlx5) to use steering tags for peer-to-peer DMA into VFIO-owned devices. The series is cleanly structured across four patches with clear layering: PCI helper -> dma-buf callback -> VFIO implementation -> mlx5 consumer.

**Strengths:**
- Good separation of concerns across subsystems
- The uAPI design with explicit validity flags for 8-bit vs 16-bit ST namespaces is correct for the PCIe spec
- Proper locking documentation and analysis in patch 3
- The `kfree(idx_data)` fix in patch 4 catches a real memory leak in the existing code
- The refactoring of `mlx5_st_alloc_index` into `mlx5_st_alloc_index_by_tag` is well done

**Concerns:**
- The `vfio_pci_dma_buf_cleanup()` locking change in patch 3 has a subtlety worth verifying (see below)
- The `EXPORT_SYMBOL` (non-GPL) choice in patch 1 should be reviewed for consistency
- Minor struct layout and documentation nits

---

---
Generated by Claude Code Patch Reviewer

Claude review: PCI/TPH: expose the enabled TPH requester type

[Claude Code Review Bot]

Patch Review

A small, clean accessor to avoid drivers reaching into `pci_dev` internals.

**Observation: EXPORT_SYMBOL vs EXPORT_SYMBOL_GPL**

```c
u8 pcie_tph_enabled_req_type(struct pci_dev *pdev)
{
	return pdev->tph_req_type;
}
EXPORT_SYMBOL(pcie_tph_enabled_req_type);
```

The existing `pcie_tph_get_st_table_loc` also uses plain `EXPORT_SYMBOL`, so this is consistent with the file's convention. However, many newer PCI subsystem exports use `EXPORT_SYMBOL_GPL`. The PCI maintainers may have a preference here — worth confirming, but not a blocker since the file is internally consistent.

**Stub return value:** The `!CONFIG_PCIE_TPH` stub returns `0`, which maps to `PCI_TPH_REQ_DISABLE`. This is correct — callers will see TPH as disabled, which is the safe default.

No other issues. Clean patch.

---
Generated by Claude Code Patch Reviewer

Claude review: dma-buf: add optional get_tph() callback

[Claude Code Review Bot]

Patch Review

Adds an optional callback to `struct dma_buf_ops`.

**Documentation nit:** The kerneldoc comment mixes `@member` doc style with `@param` style:

```c
/**
 * @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)
```

The `@get_tph:` is the ops member name, but the subsequent `@dmabuf`, `@steering_tag`, etc. are function parameters. Standard kerneldoc for ops struct callbacks typically documents the callback signature under the `@member:` block without using `@` for the individual callback parameters (since they aren't struct members). This could confuse kernel-doc tooling.

**Interface design question:** `ph` is documented as a 2-bit value and is shared between 8-bit and 16-bit ST namespaces — i.e., there's only one PH regardless of ST width. This matches the PCIe spec where PH is per-TLP, not per-ST-entry. Good.

**No wrapper function:** The callback is invoked directly via `dmabuf->ops->get_tph()` in the mlx5 consumer (patch 4) with a NULL check. There's no `dma_buf_get_tph()` wrapper in `dma-buf.c`. This is acceptable for an optional internal callback, but a thin wrapper would be more consistent with other dma-buf APIs (`dma_buf_pin`, `dma_buf_unpin`, etc.) and would centralize the NULL check. Worth considering.

---
Generated by Claude Code Patch Reviewer

Claude review: vfio/pci: implement get_tph and DMA_BUF_TPH feature

[Claude Code Review Bot]

Patch Review

This is the core patch. It adds the TPH metadata storage to `struct vfio_pci_dma_buf`, a `get_tph` callback, and the `VFIO_DEVICE_FEATURE_DMA_BUF_TPH` ioctl.

**Bitfield packing concern:**

```c
struct mutex lock;
u8 tph_st_valid:1;
u8 tph_st_ext_valid:1;
u8 tph_ph:2;
u8 tph_st;
u16 tph_st_ext;
u8 revoked:1;
```

The `revoked` bitfield was previously adjacent to a `u8` boundary. Now with the new fields inserted before it, the ordering of `tph_ph:2` (2 bits) followed by `tph_st` (full u8) followed by `u16` then another `u8` bitfield should be fine structurally. But `revoked` is documented as protected by `dma_resv_lock` while the TPH fields are protected by `priv->lock` — mixing bitfields at different locking granularities could be a problem if the compiler packs `tph_ph:2` and `revoked:1` into the same storage unit and concurrent access to different bitfields within the same storage unit is undefined behavior in C. Here `tph_st_valid:1`, `tph_st_ext_valid:1`, `tph_ph:2` are all `u8` bitfields, and `revoked:1` is a separate `u8` bitfield with `u8 tph_st` and `u16 tph_st_ext` intervening — the non-bitfield members should force them into separate storage units, so this should be safe. But it's worth verifying the compiler doesn't merge them, or alternatively making `revoked` a plain `u8` to be explicit.

**cleanup path locking:**

```c
/* In vfio_pci_dma_buf_cleanup() */
list_del_init(&priv->dmabufs_elm);
mutex_lock(&priv->lock);
priv->vdev = NULL;
mutex_unlock(&priv->lock);
```

In the original code, `priv->vdev = NULL` was set under `dma_resv_lock()`. In the patched `vfio_pci_dma_buf_cleanup()`, the `dma_resv_lock` section (which sets `priv->revoked = true` and invalidates mappings) is removed from the context shown, and only `priv->lock` protects the `vdev = NULL` write. Looking at the original code, `vfio_pci_dma_buf_cleanup` does `dma_resv_lock`, sets `priv->vdev = NULL`, sets revoked, invalidates, waits, then `dma_resv_unlock`. The patch only wraps the `vdev = NULL` in `priv->lock`, but the original path also has the `dma_resv_lock` around it. Since the purpose of `priv->lock` is to synchronize `get_tph` and `SET_TPH` against the `vdev` going away, this is correct — `get_tph` takes `priv->lock`, checks `vdev`, and the cleanup nulls `vdev` under the same lock. The `dma_resv_lock` in the cleanup path is still there for the revocation/invalidation part. The ordering claim "memory_lock -> priv->lock" is maintained because `vfio_pci_dma_buf_cleanup` holds `memory_lock` (via `down_write`) before taking `priv->lock`.

Wait — looking at the diff more carefully, the `mutex_lock(&priv->lock)` / `priv->vdev = NULL` / `mutex_unlock(&priv->lock)` is added in the `vfio_pci_dma_buf_cleanup` function which already holds `memory_lock`. But examining the original code, the full cleanup path does `dma_resv_lock` then sets `priv->vdev = NULL` and `priv->revoked = true` under that lock. The patch only wraps the `priv->vdev = NULL` in `priv->lock` but the `dma_resv_lock` section with the revocation appears earlier in the same function (not shown in the diff context). So this code path now has `memory_lock -> dma_resv_lock -> (unlock dma_resv) -> priv->lock`. This looks correct for the stated purpose.

**uAPI struct layout:**

```c
struct vfio_device_feature_dma_buf_tph {
	__s32	dmabuf_fd;
	__u32	flags;
	__u8	steering_tag;
	__u8	ph;
	__u16	steering_tag_ext;
};
```

Total size is 12 bytes, naturally aligned with no padding holes. Good.

**Input validation is thorough:** flags validated, ph range checked, dmabuf ownership verified. The `!set_tph.flags` check correctly rejects a no-op SET. Good.

**One potential concern:** `vfio_pci_core_feature_dma_buf_tph` only supports `VFIO_DEVICE_FEATURE_SET`. There's no `PROBE` support. This means userspace cannot discover at runtime whether the kernel supports `VFIO_DEVICE_FEATURE_DMA_BUF_TPH` without attempting a SET. VFIO typically provides PROBE for features. This should probably be addressed:

```c
ret = vfio_check_feature(flags, argsz, VFIO_DEVICE_FEATURE_SET,
			 sizeof(set_tph));
```

If PROBE is sent, this returns `-EINVAL` rather than indicating support. Consider adding PROBE handling that returns 0 when the device supports TPH.

---
Generated by Claude Code Patch Reviewer

Claude review: RDMA/mlx5: get tph for p2p access when registering dma-buf mr

[Claude Code Review Bot]

Patch Review

This patch wires up the mlx5 RDMA driver as a TPH consumer, refactors `mlx5_st_alloc_index` to expose a `by_tag` variant, and fixes a memory leak.

**Memory leak fix is correct and important:**

```c
if (refcount_dec_and_test(&idx_data->usecount)) {
	xa_erase(&st->idx_xa, st_index);
+	kfree(idx_data);
	/* We leave PCI config space as was before, no mkey will refer to it */
}
```

The existing code erases from the xarray but never frees the `idx_data`. This is a real leak on every ST deallocation that drops the last reference. This fix should arguably be a standalone patch or at least called out more prominently in the commit message since it fixes a pre-existing bug independent of the TPH feature.

**`ret = 0` initialization changes:**

```c
-	int ret;
+	int ret = 0;
```

In `mlx5_st_create()` and `mlx5_st_alloc_index_by_tag()`, `ret` is initialized to 0. In `mlx5_st_create()`, this appears unnecessary as `ret` is always assigned before use. In `mlx5_st_alloc_index_by_tag()`, it's needed because the `direct_mode` early-return path doesn't set `ret` (it returns 0 directly), but the main path always assigns `ret`. So the initialization is harmless but slightly misleading.

**ST ownership tracking:**

```c
if (!dmah && st_index != MLX5_MKC_PCIE_TPH_NO_STEERING_TAG_INDEX) {
	mr->dmabuf_st_index = st_index;
	mr->dmabuf_st_owned = 1;
}
```

The `!dmah` guard is correct — when DMAH supplies the ST index, the DMAH path owns the lifecycle. The ownership flag avoids double-free. Good.

**Error path cleanup:**

```c
if (IS_ERR(mr)) {
+	if (!dmah && st_index != MLX5_MKC_PCIE_TPH_NO_STEERING_TAG_INDEX)
+		mlx5_st_dealloc_index(dev->mdev, st_index);
	ib_umem_release(&umem_dmabuf->umem);
	return ERR_CAST(mr);
}
```

Correct — if MR allocation fails, the ST index must be released since no MR will own it.

**FRMR pool reuse path:**

```c
if (mr->ibmr.frmr.pool && !mlx5_umr_revoke_mr_with_lock(mr)) {
+	mlx5_ib_mr_put_dmabuf_st(mr);
+
	if (!ib_frmr_pool_push(mr->ibmr.device, &mr->ibmr))
		return 0;
}
```

The ST index is released after the mkey is revoked (firmware no longer references it) but before the MR goes back to the FRMR cache. This is the correct ordering. If `ib_frmr_pool_push` fails, the MR falls through to `destroy_mkey` and the later `mlx5_ib_mr_put_dmabuf_st(mr)` call — but since `dmabuf_st_owned` was already cleared by the first call, the second is a no-op. Correct.

**`mlx5_ib_mr_put_dmabuf_st` checks `mr->umem`:**

```c
static void mlx5_ib_mr_put_dmabuf_st(struct mlx5_ib_mr *mr)
{
	if (mr->umem && mr->dmabuf_st_owned) {
```

The `mr->umem` check guards against non-dma-buf MRs. Since `dmabuf_st_owned` is only set for dma-buf MRs that got a valid ST, the `mr->umem` check is slightly redundant but provides defense-in-depth. Fine.

**Direct callback invocation:**

```c
if (!dmabuf->ops->get_tph)
	return;
ret = dmabuf->ops->get_tph(dmabuf, &steering_tag, ph, st_width);
```

As noted in patch 2 review, calling through the ops table directly rather than a wrapper is functional but differs from other dma-buf callback patterns. The NULL check is present so it's safe.

**MODULE_IMPORT_NS("DMA_BUF") addition:**

The `mr.c` file adds `MODULE_IMPORT_NS("DMA_BUF")`. The `vfio_pci_dmabuf.c` file already had this. The `mr.c` file needs this because it now calls `dmabuf->ops->get_tph` (accessing dma-buf internals). This is correct if `get_tph` is exported in the DMA_BUF namespace, though since it's a direct callback invocation rather than an exported symbol, the necessity depends on whether other dma-buf symbols used by mr.c are in that namespace.

---
Generated by Claude Code Patch Reviewer