Re: [PATCH v3 9/9] vfio/pci: Add mmap() attributes to DMABUF feature

[Matt Evans <matt@ozlabs.org>]

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 <matt@ozlabs.org>
>> ---
>>  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/20260602131417.41366391@shazbot.org/
   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 <praan@google.com>

Thanks!


Matt


> 
> Thanks,
> Praan
> 
> [1] https://lore.kernel.org/all/20260602131417.41366391@shazbot.org/
> [2] https://elixir.bootlin.com/linux/v7.1-rc6/source/include/uapi/asm-generic/errno-base.h#L6
>