Hi Praan, On 16/06/2026 09:47, Pranjal Shrivastava wrote: > On Wed, Jun 10, 2026 at 04:43:23PM +0100, Matt Evans wrote: >> A new VFIO feature, VFIO_DEVICE_FEATURE_DMA_BUF_MEMATTR, is added to >> set CPU-facing memory type attributes for a DMABUF exported from >> vfio-pci. These are used for subsequent mmap()s of the buffer. >> >> There are two attributes supported: >> - The default, VFIO_DEVICE_FEATURE_DMA_BUF_MEMATTR_NC >> - VFIO_DEVICE_FEATURE_DMA_BUF_MEMATTR_WC, which results in WC >> PTEs for the DMABUF's BAR region. >> >> Signed-off-by: Matt Evans <[email protected]> >> --- >> drivers/vfio/pci/vfio_pci_core.c | 2 ++ >> drivers/vfio/pci/vfio_pci_dmabuf.c | 57 +++++++++++++++++++++++++++++- >> drivers/vfio/pci/vfio_pci_priv.h | 14 ++++++++ >> include/uapi/linux/vfio.h | 27 ++++++++++++++ >> 4 files changed, 99 insertions(+), 1 deletion(-) >> > >> +int vfio_pci_core_feature_dma_buf_memattr( >> + struct vfio_pci_core_device *vdev, u32 flags, >> + struct vfio_device_feature_dma_buf_memattr __user *arg, >> + size_t argsz) >> +{ >> + struct vfio_device_feature_dma_buf_memattr db_attr; >> + struct vfio_pci_dma_buf *priv; >> + struct dma_buf *dmabuf; >> + int ret; >> + >> + if (!vdev->pci_ops || !vdev->pci_ops->get_dmabuf_phys) >> + return -EOPNOTSUPP; >> + >> + ret = vfio_check_feature(flags, argsz, >> + VFIO_DEVICE_FEATURE_SET, >> + sizeof(db_attr)); >> + if (ret != 1) >> + return ret; >> + >> + if (copy_from_user(&db_attr, arg, sizeof(db_attr))) >> + return -EFAULT; >> + >> + dmabuf = dma_buf_get(db_attr.dmabuf_fd); >> + if (IS_ERR(dmabuf)) >> + return PTR_ERR(dmabuf); >> + >> + /* Verify DMABUF: see comments in vfio_pci_dma_buf_revoke() */ >> + priv = dmabuf->priv; >> + if (dmabuf->ops != &vfio_pci_dmabuf_ops || >> + READ_ONCE(priv->vdev) != vdev) { >> + ret = -ENODEV; >> + goto out_put_buf; >> + } >> + >> + switch (db_attr.memattr) { >> + case VFIO_DEVICE_FEATURE_DMA_BUF_MEMATTR_NC: >> + case VFIO_DEVICE_FEATURE_DMA_BUF_MEMATTR_WC: >> + WRITE_ONCE(priv->memattr, db_attr.memattr); >> + ret = 0; >> + break; >> + >> + default: >> + ret = -ENOENT; > > Nit: Looks like the agreement [1] was on -EOPNOTSUPP / -EINVAL but we > took -ENOENT here and in the doc string? Was that intentional? > > I tend to agree with Alex's suggestion here, we'd prefer one of those > two (-EINVAL / -EOPNOTSUPP) since it clearly communicates to the user > that "You sent a wrong arg" or "We don't support this" >
Yes, it was intentional. This was noted in the v3 changelog entry in the cover letter: - Removed GET on vfio_pci_core_feature_dma_buf_memattr(), removed unnecessary taking of memory_lock, fixed error return values. In particular, removes ENOTSUPP, and uses ENOENT to indicate an unknown attribute enum value was passed to SET. In the discussion here, https://lore.kernel.org/all/[email protected]/ we'd agreed on EOPNOTSUPP before I realised that's already used elsewhere. ENOENT uniquely indicates an unknown attribute. EINVAL/EOPNOTSUPP would indeed be semantically perfect, but after posting my reply there I remembered they are already overloaded with a load of different meanings. I think uniqueness is important here so that memattr issues (for example any future arch-specific porting issues) show up as an immediately-understandable error value. > -ENOENT means no such file or directory [2] to the user. Users may not > be kernel engineers who'd wanna peek into the code and they may simply > look at the uAPI files which doesn't give them an answer as to what > went wrong. But surely when they look at the uAPI header they will then see "* ENOENT: The given memattr is not supported." and understand what went wrong. > >> + } >> + >> out_put_buf: >> dma_buf_put(dmabuf); >> > > Apart from that, > Reviewed-by: Pranjal Shrivastava <[email protected]> Thanks! Matt > > Thanks, > Praan > > [1] https://lore.kernel.org/all/[email protected]/ > [2] > https://elixir.bootlin.com/linux/v7.1-rc6/source/include/uapi/asm-generic/errno-base.h#L6 >
