> > +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;
> > +
> > + if (!pcie_tph_supported(vdev->pdev))
> > + return -EOPNOTSUPP;
>
> This tests for the TPH capability, but the TPH capability is only a
> requirement for functions that generate TLPs with TPH, ie. a requester.
> This feature is about providing TPH steering tags when the device is a
> completer. Bits 13:12 of the Device Capabilities 2 register indicate
> if the device is supported as a TPH completer.
>
> Additionally these bits indicate if the device supports standard and
> extended TPH, which means we should not only fail if the device reports
> 00b, but should reject extended steering tags unless the device reports
> 11b.
>
You are right: pcie_tph_supported is not correct here.
Let me use a helper function to return something like this:
PCI_TPH_COMP_{NONE, TPH_ONLY, EXT_TPH}.
> > +
> > + 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;
> > +
> > + /* PCIe TLP Processing Hint is a 2-bit field. */
> > + if (set_tph.ph & ~0x3)
> > + return -EINVAL;
>
> Sashiko notes what appears to be a false positive here, the uAPI states
> ph is to be in the range [0, 3] and nowhere else says that it's allowed
> to be garbage for a clear operation.
>
Agreed - i will leave it as is.
> > +
> > + 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;
> > + if (priv->vdev != vdev) {
> > + ret = -EINVAL;
> > + goto out_put;
> > + }
>
> Sashiko notes this may need READ_ONCE()/WRITE_ONCE() semantics, but
> that may get fixed as part of the resv lock usage.
>
Ack.
> > +
> > + scoped_guard(mutex, &priv->tph_lock) {
> > + 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_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;
> ...
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index 5de618a3a5ee..0ca26721849b 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -1534,6 +1534,51 @@ 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, and the device must expose
> > the
> > + * TPH Extended Capability (otherwise the ioctl returns -EOPNOTSUPP).
> > + *
> > + * 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.
> > + *
> > + * This publishes the PCI SIG-defined ST/PH tuple for a VFIO-owned PCIe
> > + * completer. The dma-buf core treats the tuple as opaque completer-owned
> > + * metadata; an importer simply requests the namespace it supports and
> > places
> > + * the returned value on generated TLPs.
> > + *
> > + * @flags == 0 clears any previously published metadata.
>
> This is overselling the invalidation. It only flags the fields as
> invalid for future get_tph() requests, it does nothing to clear
> previously published metadata from importers. Thanks,
>
> Alex
>
Good catch, let me re-word.
Thanks,
Zhiping
