Re: [PATCH v1 7/7] virtio-gpu-udmabuf: Create dmabuf for blobs associated with VFIO devices

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

On 10/4/25 01:36, Vivek Kasireddy wrote:
> In addition to memfd, a blob resource can also have its backing
> storage in a VFIO device region. Therefore, we first need to figure
> out if the blob is backed by a VFIO device region or a memfd before
> we can call the right API to get a dmabuf fd created.
> 
> So, once we have the ramblock and the associated mr, we rely on
> memory_region_is_ram_device() to tell us where the backing storage
> is located. If the blob resource is VFIO backed, we try to find the
> right VFIO device that contains the blob and then invoke the API
> vfio_device_create_dmabuf().
> 
> Note that in virtio_gpu_remap_udmabuf(), we first try to test if
> the VFIO dmabuf exporter supports mmap or not. If it doesn't, we
> use the VFIO device fd directly to create the CPU mapping.
> 
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Cc: Alex Bennée <alex.bennee@linaro.org>
> Cc: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> ---
>   hw/display/Kconfig              |   5 ++
>   hw/display/virtio-gpu-udmabuf.c | 143 ++++++++++++++++++++++++++++++--
>   2 files changed, 141 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/display/Kconfig b/hw/display/Kconfig
> index 1e95ab28ef..0d090f25f5 100644
> --- a/hw/display/Kconfig
> +++ b/hw/display/Kconfig
> @@ -106,6 +106,11 @@ config VIRTIO_VGA
>       depends on VIRTIO_PCI
>       select VGA
>   
> +config VIRTIO_GPU_VFIO_BLOB
> +    bool
> +    default y
> +    depends on VFIO
> +
>   config VHOST_USER_GPU
>       bool
>       default y
> diff --git a/hw/display/virtio-gpu-udmabuf.c b/hw/display/virtio-gpu-udmabuf.c
> index d804f321aa..bd06b4f300 100644
> --- a/hw/display/virtio-gpu-udmabuf.c
> +++ b/hw/display/virtio-gpu-udmabuf.c
> @@ -18,6 +18,7 @@
>   #include "ui/console.h"
>   #include "hw/virtio/virtio-gpu.h"
>   #include "hw/virtio/virtio-gpu-pixman.h"
> +#include "hw/vfio/vfio-device.h"
>   #include "trace.h"
>   #include "system/ramblock.h"
>   #include "system/hostmem.h"
> @@ -27,6 +28,33 @@
>   #include "standard-headers/linux/udmabuf.h"
>   #include "standard-headers/drm/drm_fourcc.h"
>   
> +static void vfio_create_dmabuf(VFIODevice *vdev,
> +                               struct virtio_gpu_simple_resource *res)
> +{
> +#if defined(VIRTIO_GPU_VFIO_BLOB)
> +    res->dmabuf_fd = vfio_device_create_dmabuf(vdev, res->iov, res->iov_cnt);

I didn't realize an fd was returned until this patch. I'd suggest
renaming vfio_device_create_dmabuf() to vfio_device_create_dmabuf_fd(),
or something explicit IMO.

> +    if (res->dmabuf_fd < 0) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: VFIO_DEVICE_FEATURE_DMA_BUF: %s\n",
> +                      __func__, strerror(errno));
> +    }
> +#endif
> +}
> +
> +static VFIODevice *vfio_device_lookup(MemoryRegion *mr)
> +{
> +#if defined(VIRTIO_GPU_VFIO_BLOB)
> +    VFIODevice *vdev;
> +
> +    QLIST_FOREACH(vdev, &vfio_device_list, next) {
Hmm, I'm not sure we want to expose the VFIOdevice list to other
subsystems. I understand the need, and it's faster than iterating
over QOM devices, but I’d prefer that an API be provided for this
purpose.

I missed how much vfio_device_list has proliferated. Needs a check.


> +        if (vdev->dev == mr->dev) {
> +            return vdev;
> +        }
> +    }
> +#endif
> +    return NULL;
> +}
> +
>   static void virtio_gpu_create_udmabuf(struct virtio_gpu_simple_resource *res)
>   {
>       struct udmabuf_create_list *list;
> @@ -68,11 +96,73 @@ static void virtio_gpu_create_udmabuf(struct virtio_gpu_simple_resource *res)
>       g_free(list);
>   }
>   
> -static void virtio_gpu_remap_udmabuf(struct virtio_gpu_simple_resource *res)
> +static void *vfio_dmabuf_mmap(struct virtio_gpu_simple_resource *res,
> +                              VFIODevice *vdev)
> +{
> +    struct vfio_region_info *info;
> +    ram_addr_t offset, len = 0;
> +    void *map, *submap;
> +    int i, ret = -1;
> +    RAMBlock *rb;
> +
> +    /*
> +     * We first reserve a contiguous chunk of address space for the entire
> +     * dmabuf, then replace it with smaller mappings that correspond to the
> +     * individual segments of the dmabuf.
> +     */
> +    map = mmap(NULL, res->blob_size, PROT_READ, MAP_SHARED, vdev->fd, 0);
> +    if (map == MAP_FAILED) {
> +        return map;
> +    }
> +
> +    for (i = 0; i < res->iov_cnt; i++) {
> +        rcu_read_lock();
> +        rb = qemu_ram_block_from_host(res->iov[i].iov_base, false, &offset);
> +        rcu_read_unlock();
> +
> +        if (!rb) {
> +            goto err;
> +        }
> +
> +#if defined(VIRTIO_GPU_VFIO_BLOB)
> +        ret = vfio_get_region_index_from_mr(rb->mr);
> +        if (ret < 0) {
> +            goto err;
> +        }
> +
> +        ret = vfio_device_get_region_info(vdev, ret, &info);
> +#endif
> +        if (ret < 0) {
> +            goto err;
> +        }

"hmm" again. Not this patch fault but we lack proper documentation
for the VFIO API. Something to work on. Since this patch is using
vfio_device_get_region_info() could you please add documentation
for it ?


Thanks,

C.



> +        submap = mmap(map + len, res->iov[i].iov_len, PROT_READ,
> +                      MAP_SHARED | MAP_FIXED, vdev->fd,
> +                      info->offset + offset);
> +        if (submap == MAP_FAILED) {
> +            goto err;
> +        }
> +
> +        len += res->iov[i].iov_len;
> +    }
> +    return map;
> +err:
> +    munmap(map, res->blob_size);
> +    return MAP_FAILED;
> +}
> +
> +static void virtio_gpu_remap_udmabuf(struct virtio_gpu_simple_resource *res,
> +                                     VFIODevice *vdev)
>   {
>       res->remapped = mmap(NULL, res->blob_size, PROT_READ,
>                            MAP_SHARED, res->dmabuf_fd, 0);
>       if (res->remapped == MAP_FAILED) {
> +        if (vdev) {
> +            res->remapped = vfio_dmabuf_mmap(res, vdev);
> +            if (res->remapped != MAP_FAILED) {
> +                return;
> +            }
> +        }
>           warn_report("%s: dmabuf mmap failed: %s", __func__,
>                       strerror(errno));
>           res->remapped = NULL;
> @@ -130,18 +220,59 @@ bool virtio_gpu_have_udmabuf(void)
>   
>   void virtio_gpu_init_udmabuf(struct virtio_gpu_simple_resource *res)
>   {
> +    VFIODevice *vdev = NULL;
>       void *pdata = NULL;
> +    ram_addr_t offset;
> +    RAMBlock *rb;
>   
>       res->dmabuf_fd = -1;
>       if (res->iov_cnt == 1 &&
>           res->iov[0].iov_len < 4096) {
>           pdata = res->iov[0].iov_base;
>       } else {
> -        virtio_gpu_create_udmabuf(res);
> -        if (res->dmabuf_fd < 0) {
> +        rcu_read_lock();
> +        rb = qemu_ram_block_from_host(res->iov[0].iov_base, false, &offset);
> +        rcu_read_unlock();
> +
> +        if (!rb) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "%s: Could not find ram block for host address\n",
> +                          __func__);
>               return;
>           }
> -        virtio_gpu_remap_udmabuf(res);
> +
> +        if (memory_region_is_ram_device(rb->mr)) {
> +            vdev = vfio_device_lookup(rb->mr);
> +            if (!vdev) {
> +                qemu_log_mask(LOG_GUEST_ERROR,
> +                              "%s: Could not find device to create dmabuf\n",
> +                              __func__);
> +                return;
> +            }
> +
> +            vfio_create_dmabuf(vdev, res);
> +            if (res->dmabuf_fd < 0) {
> +                qemu_log_mask(LOG_GUEST_ERROR,
> +                              "%s: Could not create dmabuf from vfio device\n",
> +                              __func__);
> +                return;
> +            }
> +        } else if (memory_region_is_ram(rb->mr) && virtio_gpu_have_udmabuf()) {
> +            virtio_gpu_create_udmabuf(res);
> +            if (res->dmabuf_fd < 0) {
> +                qemu_log_mask(LOG_GUEST_ERROR,
> +                              "%s: Could not create dmabuf from memfd\n",
> +                              __func__);
> +                return;
> +            }
> +        } else {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "%s: memory region cannot be used to create dmabuf\n",
> +                          __func__);
> +            return;
> +        }
> +
> +        virtio_gpu_remap_udmabuf(res, vdev);
>           if (!res->remapped) {
>               return;
>           }
> @@ -153,9 +284,7 @@ void virtio_gpu_init_udmabuf(struct virtio_gpu_simple_resource *res)
>   
>   void virtio_gpu_fini_udmabuf(struct virtio_gpu_simple_resource *res)
>   {
> -    if (res->remapped) {
> -        virtio_gpu_destroy_udmabuf(res);
> -    }
> +    virtio_gpu_destroy_udmabuf(res);
>   }
>   
>   static void virtio_gpu_free_dmabuf(VirtIOGPU *g, VGPUDMABuf *dmabuf)