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/6/25 17:59, Cédric Le Goater wrote:
> 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);

One more thing which needs a fix :

'info' is uninitialized if !VIRTIO_GPU_VFIO_BLOB.


Thanks,

C.



>> +        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)
>