On Tue, Jun 16, 2026 at 12:37:29PM +0100, Matt Evans wrote:
> 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(-)
> >>
> >
[...]
> >> +
> >> + /* 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.
>
Ahh okay. I missed the changelogs in the cover letter.
> 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.
Fair enough. Since its documented it clearly in the uAPI header.
Thanks,
Praan
