Re: [PATCH v1 6/7] vfio/device: Add support for VFIO_DEVICE_FEATURE_DMA_BUF

["Cédric Le Goater" <clg@redhat.com>]

Hello Vivek,

On 10/4/25 01:35, Vivek Kasireddy wrote:
> In order to implement VFIO_DEVICE_FEATURE_DMA_BUF, we first need
> to identify the VFIO region and index the buffer (represented by
> iovec) belongs to and then translate its addresses to offsets
> within that region.
> 
> The qemu_ram_block_from_host() API gives us both the region and the
> offset info we need to populate the dma ranges in order to invoke
> this feature.
> 
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> ---
>   hw/vfio/device.c              | 43 +++++++++++++++++++++++++++++++++++
>   include/hw/vfio/vfio-device.h |  3 +++
>   2 files changed, 46 insertions(+)
> 
> diff --git a/hw/vfio/device.c b/hw/vfio/device.c
> index 64f8750389..49070929ac 100644
> --- a/hw/vfio/device.c
> +++ b/hw/vfio/device.c
> @@ -21,6 +21,7 @@
>   #include "qemu/osdep.h"
>   #include <sys/ioctl.h>
>   
> +#include "system/ramblock.h"
>   #include "hw/vfio/vfio-device.h"
>   #include "hw/vfio/pci.h"
>   #include "hw/hw.h"
> @@ -592,3 +593,45 @@ static VFIODeviceIOOps vfio_device_io_ops_ioctl = {
>       .region_read = vfio_device_io_region_read,
>       .region_write = vfio_device_io_region_write,
>   };
> +
> +int vfio_device_create_dmabuf(VFIODevice *vdev,

a 'vbasedev' name is preferred for VFIODevice variables/parameters.

> +                              struct iovec *iov, unsigned int iov_cnt)
> +{
> +    g_autofree struct vfio_device_feature *feature;

g_autofree variables should be set: 'feature = NULL'

> +    struct vfio_device_feature_dma_buf *dma_buf;
> +    ram_addr_t offset;
> +    RAMBlock *rb;
> +    size_t argsz;
> +    int i, index;
> +
> +    argsz = sizeof(*feature) + sizeof (*dma_buf) +
> +            sizeof(struct vfio_region_dma_range) * iov_cnt;
> +    feature = g_malloc0(argsz);
> +    dma_buf = (struct vfio_device_feature_dma_buf *)feature->data;
> +
> +    for (i = 0; i < iov_cnt; i++) {
> +        rcu_read_lock();

Is it needed ?

> +        rb = qemu_ram_block_from_host(iov[i].iov_base, false, &offset);
> +        rcu_read_unlock();
> +
> +        if (!rb) {
> +            return -1;

wouldn't an errno be more appropriate ?

> +        }
> +
> +        index = vfio_get_region_index_from_mr(rb->mr);
> +        if (index < 0) {
> +            return -1;
> +        }
> +
> +        dma_buf->region_index = index;
> +        dma_buf->dma_ranges[i].offset = offset;
> +        dma_buf->dma_ranges[i].length = iov[i].iov_len;
> +    }
> +
> +    dma_buf->nr_ranges = iov_cnt;
> +    dma_buf->open_flags = O_RDONLY | O_CLOEXEC;
> +    feature->argsz = argsz;
> +    feature->flags = VFIO_DEVICE_FEATURE_GET | VFIO_DEVICE_FEATURE_DMA_BUF;
> +
> +    return ioctl(vdev->fd, VFIO_DEVICE_FEATURE, feature);
Please use :

    return vbasedev->io_ops->device_feature(vbasedev, feature);

This would be returning an errno. Callers should be aware.

> +}
> diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
> index bdb106c937..74b3c4eef7 100644
> --- a/include/hw/vfio/vfio-device.h
> +++ b/include/hw/vfio/vfio-device.h
> @@ -279,6 +279,9 @@ int vfio_device_get_irq_info(VFIODevice *vbasedev, int index,
>                                   struct vfio_irq_info *info);
>   
>   int vfio_get_region_index_from_mr(MemoryRegion *mr);
> +
> +int vfio_device_create_dmabuf(VFIODevice *vbasedev,
> +                              struct iovec *iov, unsigned int iov_cnt);


Since this is an external routine, please add documentation.


Thanks,

C.


>   #endif
>   
>   /* Returns 0 on success, or a negative errno. */