Re: [PATCH 2/3] dma-buf: Add DMA_BUF_IOCTL_GET_DMA_ADDR

["Christian König" <christian.koenig@amd.com>]

On 4/29/25 08:39, Simona Vetter wrote:
> Catching up after spring break, hence the late reply ...
> 
> On Fri, Apr 11, 2025 at 02:34:37PM -0400, Nicolas Dufresne wrote:
>> Le jeudi 10 avril 2025 à 16:53 +0200, Bastien Curutchet a écrit :
>>> There is no way to transmit the DMA address of a buffer to userspace.
>>> Some UIO users need this to handle DMA from userspace.
>>
>> To me this API is against all safe practice we've been pushing forward
>> and has no place in DMA_BUF API.
>>
>> If this is fine for the UIO subsystem to pass around physicial
>> addresses, then make this part of the UIO device ioctl.
> 
> Yeah, this has no business in dma-buf since the entire point of dma-buf
> was to stop all the nasty "just pass raw dma addr in userspace" hacks that
> preceeded it.
> 
> And over the years since dma-buf landed, we've removed a lot of these,
> like dri1 drivers. Or where that's not possible like with fbdev, hid the
> raw dma addr uapi behind a Kconfig.
> 
> I concur with the overall sentiment that this should be done in
> vfio/iommufd interfaces, maybe with some support added to map dma-buf. I
> think patches for that have been floating around for a while, but I lost a
> bit the status of where exactly they are.

My take away is that we need to have a documented way for special driver specific interfaces in DMA-buf.

In other words DMA-buf has some standardized rules of doing things which every implementation should follow. The implementations might of course still have bugs (e.g. allocate memory for a dma_fence operation), but at least we have documented what should be done and what's forbidden.

What is still missing in the documentation is the use case when you have for example vfio which wants to talk to iommufd through a specialized interface. This doesn't necessarily needs to be part of DMA-buf, but we should still document "do it this way" because that has already worked in the last ten use cases and we don't want people to re-invent the wheel in a new funky way which then later turns out to not work.

Regards,
Christian.

> 
> Cheers, Sima
> 
>>
>> regards,
>> Nicolas
>>
>>>
>>> Add a new dma_buf_ops operation that returns the DMA address.
>>> Add a new ioctl to transmit this DMA address to userspace.
>>>
>>> Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
>>> ---
>>>  drivers/dma-buf/dma-buf.c    | 21 +++++++++++++++++++++
>>>  include/linux/dma-buf.h      |  1 +
>>>  include/uapi/linux/dma-buf.h |  1 +
>>>  3 files changed, 23 insertions(+)
>>>
>>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>>> index
>>> 398418bd9731ad7a3a1f12eaea6a155fa77a22fe..cbbb518981e54e50f479c3d1fcf
>>> 6da6971f639c1 100644
>>> --- a/drivers/dma-buf/dma-buf.c
>>> +++ b/drivers/dma-buf/dma-buf.c
>>> @@ -454,6 +454,24 @@ static long dma_buf_import_sync_file(struct
>>> dma_buf *dmabuf,
>>>  }
>>>  #endif
>>>  
>>> +static int dma_buf_get_dma_addr(struct dma_buf *dmabuf, u64 __user
>>> *arg)
>>> +{
>>> +	u64 addr;
>>> +	int ret;
>>> +
>>> +	if (!dmabuf->ops->get_dma_addr)
>>> +		return -EINVAL;
>>> +
>>> +	ret = dmabuf->ops->get_dma_addr(dmabuf, &addr);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	if (copy_to_user(arg, &addr, sizeof(u64)))
>>> +		return -EFAULT;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  static long dma_buf_ioctl(struct file *file,
>>>  			  unsigned int cmd, unsigned long arg)
>>>  {
>>> @@ -504,6 +522,9 @@ static long dma_buf_ioctl(struct file *file,
>>>  		return dma_buf_import_sync_file(dmabuf, (const void
>>> __user *)arg);
>>>  #endif
>>>  
>>> +	case DMA_BUF_IOCTL_GET_DMA_ADDR:
>>> +		return dma_buf_get_dma_addr(dmabuf, (u64 __user
>>> *)arg);
>>> +
>>>  	default:
>>>  		return -ENOTTY;
>>>  	}
>>> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
>>> index
>>> 36216d28d8bdc01a9c9c47e27c392413f7f6c5fb..ed4bf15d3ce82e7a86323fff459
>>> 699a9bc8baa3b 100644
>>> --- a/include/linux/dma-buf.h
>>> +++ b/include/linux/dma-buf.h
>>> @@ -285,6 +285,7 @@ struct dma_buf_ops {
>>>  
>>>  	int (*vmap)(struct dma_buf *dmabuf, struct iosys_map *map);
>>>  	void (*vunmap)(struct dma_buf *dmabuf, struct iosys_map
>>> *map);
>>> +	int (*get_dma_addr)(struct dma_buf *dmabuf, u64 *addr);
>>>  };
>>>  
>>>  /**
>>> diff --git a/include/uapi/linux/dma-buf.h b/include/uapi/linux/dma-
>>> buf.h
>>> index
>>> 5a6fda66d9adf01438619e7e67fa69f0fec2d88d..f3aba46942042de6a2e3a4cca3e
>>> b3f87175e29c9 100644
>>> --- a/include/uapi/linux/dma-buf.h
>>> +++ b/include/uapi/linux/dma-buf.h
>>> @@ -178,5 +178,6 @@ struct dma_buf_import_sync_file {
>>>  #define DMA_BUF_SET_NAME_B	_IOW(DMA_BUF_BASE, 1, __u64)
>>>  #define DMA_BUF_IOCTL_EXPORT_SYNC_FILE	_IOWR(DMA_BUF_BASE, 2,
>>> struct dma_buf_export_sync_file)
>>>  #define DMA_BUF_IOCTL_IMPORT_SYNC_FILE	_IOW(DMA_BUF_BASE, 3, struct
>>> dma_buf_import_sync_file)
>>> +#define DMA_BUF_IOCTL_GET_DMA_ADDR	_IOR(DMA_BUF_BASE, 4, __u64
>>> *)
>>>  
>>>  #endif
>