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

[Alex Williamson <alex@shazbot.org>]

On Wed, 15 Jul 2026 13:39:55 -0700
Zhiping Zhang <zhipingz@meta.com> wrote:
>
> Depends on (submitted separately):
>   net/mlx5: free mlx5_st_idx_data on final dealloc
>     https://lore.kernel.org/linux-rdma/20260612170406.3339093-1-zhipingz@meta.com
>   PCI/TPH: fold reserved completer encoding in get_rp_completer_type()
>     https://lore.kernel.org/linux-pci/20260715202409.3767494-1-zhipingz@meta.com

Sashiko failed to apply[1], it can't handle such dependencies.  The
first dependency already exists in linux-next, I applied the latter
manually, therefore the patch indexes in the below local sashiko run
results are off by one.  This used Claude Opus 4.8 rather than the
standard Gemini 3.1 Pro Preview used in the on-list reviews, other LLMs
may find more or less.

We should probably reevaluate the best merge path for these, it doesn't
make a lot of sense to me to have one patch routed through Bjorn's tree
for a self-contained fix where no known hardware makes use of the
reserved encoding, then depend on that change in the first patch of
this series.  We need Bjorn's ack here anyway, the Fixes: tag is
academic in current code.  Suggest adding that patch into this series
on the re-spin and prodding required subsystem maintainers for review.

I'd upgrade the severity of the first issue below since it affects the
uAPI.  Thanks,

Alex

[1]https://sashiko.dev/#/patchset/20260715204008.3911275-1-zhipingz@meta.com

Inline Review:
--- Patch [4]: vfio/pci: implement get_pci_tph and DMA_BUF_TPH feature ---
commit 3a3a7e88fb6acb9853e8f8bea216613aeea43b4f
Author: Zhiping Zhang <zhipingz@meta.com>

vfio/pci: implement get_pci_tph and DMA_BUF_TPH feature

This patch implements the dma-buf get_pci_tph callback for vfio-pci
exported dma-bufs and adds VFIO_DEVICE_FEATURE_DMA_BUF_TPH so userspace
can publish TPH metadata for a VFIO-owned device. It gates the feature on
the device reporting TPH Completer support in Device Capabilities 2.

Link: https://lore.kernel.org/r/20260715204008.3911275-4-zhipingz@meta.com

> diff --git a/drivers/vfio/pci/vfio_pci_dmabuf.c b/drivers/vfio/pci/vfio_pci_dmabuf.c
> index c16f460c01d6..7db84d27dfec 100644
> --- a/drivers/vfio/pci/vfio_pci_dmabuf.c
> +++ b/drivers/vfio/pci/vfio_pci_dmabuf.c

[ ... ]

> +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;
> +	u8 comp;
> +	int ret;
> +
> +	comp = pcie_tph_completer_type(vdev->pdev);
> +	if (comp == PCI_EXP_DEVCAP2_TPH_COMP_NONE)
> +		return -EOPNOTSUPP;
> +
> +	ret = vfio_check_feature(flags, argsz, VFIO_DEVICE_FEATURE_SET,
> +				 sizeof(set_tph));
> +	if (ret != 1)
> +		return ret;

[Severity: Low]
Should the PROBE path here also require that the device can actually
export a vfio dma-buf?

The only gate before vfio_check_feature() is the completer-type check:

	comp = pcie_tph_completer_type(vdev->pdev);
	if (comp == PCI_EXP_DEVCAP2_TPH_COMP_NONE)
		return -EOPNOTSUPP;

vfio_check_feature() returns 0 for a PROBE request, so on a device that
reports TPH Completer support in Device Capabilities 2 but whose pci_ops
has no get_dmabuf_phys, a VFIO_DEVICE_FEATURE_PROBE for
VFIO_DEVICE_FEATURE_DMA_BUF_TPH reports success even though no vfio
dma-buf can ever be created to carry the TPH metadata. Every real SET
would then fail later, either at dma_buf_get() or at the
dmabuf->ops != &vfio_pci_dmabuf_ops ownership check.

The sibling base feature vfio_pci_core_feature_dma_buf() opens with:

	if (!vdev->pci_ops || !vdev->pci_ops->get_dmabuf_phys)
		return -EOPNOTSUPP;

so its PROBE reports unsupported when no dma-buf can be created.

The commit message says PROBE was made to "follow the same hardware gate
so the feature only probes as supported when the device can really
consume it." Would adding the same get_dmabuf_phys check make PROBE
consistent with vfio_pci_core_feature_dma_buf() and match that stated
intent?

--- Patch [5]: RDMA/mlx5: get tph for p2p access when registering dma-buf mr ---
commit e612584ae6697d1ebf77baf0fb1487a41e4c6511
Author: Zhiping Zhang <zhipingz@meta.com>

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

This patch wires up mlx5_ib to query dma_buf_get_pci_tph() metadata at MR
registration time for p2p access and program requester-side TPH on the
outbound mkey.  As part of it, mlx5_st_alloc_index() is split so that a new
mlx5_st_alloc_index_by_tag() helper does the table lookup while the old
entry point still handles the ACPI steering-tag query.

Link: https://lore.kernel.org/r/20260715204008.3911275-5-zhipingz@meta.com

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/st.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/st.c
> index 7cedc348790d..877b37b4e639 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/st.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/st.c
> @@ -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,8 +147,46 @@ 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);

[Severity: Low]
This refactor moves the if (!st) return -EOPNOTSUPP; short-circuit into
mlx5_st_alloc_index_by_tag(), so the remaining mlx5_st_alloc_index() now
calls pcie_tph_get_cpu_st() before that guard is reached.

Before this patch mlx5_st_alloc_index() checked !st first and returned
-EOPNOTSUPP without ever calling pcie_tph_get_cpu_st().  Now the order is
reversed:

mlx5_st_alloc_index()
    pcie_tph_get_cpu_st(dev->pdev, ...)   /* runs first */
    mlx5_st_alloc_index_by_tag()
        if (!st) return -EOPNOTSUPP;      /* guard now lives here */

For a device where dev->st is NULL (TPH not supported, e.g.
!MLX5_CAP_GEN(mkey_pcie_tph), no pdev->tph_cap, or a failed mlx5_st_create()),
the DMAH-with-CPU-id path mlx5_ib_alloc_dmah() -> mlx5_st_alloc_index() now
reaches pcie_tph_get_cpu_st(), which performs an ACPI _DSM invocation
(acpi_get_cpu_uid() + pcie_find_root_port() + tph_invoke_dsm()) that was
previously skipped entirely.

Is this added firmware query on a device that does not support ST intended?

There is also an errno change: when the _DSM lookup itself fails,
mlx5_st_alloc_index() now returns pcie_tph_get_cpu_st()'s error (e.g.
-ENODEV or -EINVAL) to userspace instead of the prior -EOPNOTSUPP.  When the
lookup succeeds the by_tag re-check still returns -EOPNOTSUPP, so the two
differ only on the sub-path where the _DSM fails.

Would it be worth restoring the !st early-out in mlx5_st_alloc_index() before
the pcie_tph_get_cpu_st() call, or noting this behavioral change in the
commit message, which currently presents the change only as extracting
mlx5_st_alloc_index_by_tag()?