Hi Akihiko,
> Subject: Re: [RFC 3/6] virtio-gpu-udmabuf: Create dmabuf for blobs
> associated with VFIO devices
>
> On 2025/09/03 7:42, 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_create_dmabuf().
> >
> > Note that we only call virtio_gpu_remap_udmabuf() if the blob is
> > backed by a memfd. This is because the VFIO dmabuf implementation
> > may not support mmap.
> >
> > Cc: Marc-André Lureau <marcandre.lur...@redhat.com>
> > Cc: Alex Bennée <alex.ben...@linaro.org>
> > Cc: Akihiko Odaki <od...@rsg.ci.i.u-tokyo.ac.jp>
> > Cc: Dmitry Osipenko <dmitry.osipe...@collabora.com>
> > Signed-off-by: Vivek Kasireddy <vivek.kasire...@intel.com>
> > ---
> > hw/display/virtio-gpu-udmabuf.c | 60 ++++++++++++++++++++++++++++--
> ---
> > 1 file changed, 52 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/display/virtio-gpu-udmabuf.c b/hw/display/virtio-gpu-
> udmabuf.c
> > index d804f321aa..0390a8f488 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,32 @@
> > #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)
> > +{
> > + res->dmabuf_fd = vfio_device_create_dmabuf(vdev, res->iov, res-
> >iov_cnt);
> > + if (res->dmabuf_fd < 0) {
> > + warn_report("%s: VFIO_DEVICE_FEATURE_DMA_BUF: %s", __func__,
> > + strerror(errno));
> > + }
> > +}
> > +
> > +static VFIODevice *vfio_device_lookup(MemoryRegion *mr)
> > +{
> > + VFIODevice *vdev;
> > +
> > + if (QLIST_EMPTY(&vfio_device_list)) {
> > + return NULL;
> > + }
>
> I think this QLIST_EMPTY() check can be removed.
Yeah, agreed. I'll remove it.
>
> > +
> > + QLIST_FOREACH(vdev, &vfio_device_list, next) {
> > + if (vdev->dev == mr->dev) {
> > + return vdev;
> > + }
> > + }
> > + return NULL;
> > +}
> > +
> > static void virtio_gpu_create_udmabuf(struct virtio_gpu_simple_resource
> *res)
> > {
> > struct udmabuf_create_list *list;
> > @@ -130,6 +157,9 @@ bool virtio_gpu_have_udmabuf(void)
> >
> > void virtio_gpu_init_udmabuf(struct virtio_gpu_simple_resource *res)
> > {
> > + bool memfd_blob = false;
> > + ram_addr_t offset;
> > + RAMBlock *rb;
> > void *pdata = NULL;
> >
> > res->dmabuf_fd = -1;
> > @@ -137,15 +167,31 @@ void virtio_gpu_init_udmabuf(struct
> virtio_gpu_simple_resource *res)
> > res->iov[0].iov_len < 4096) {
> > pdata = res->iov[0].iov_base;
> > } else {
> > - virtio_gpu_create_udmabuf(res);
> > + rb = qemu_ram_block_from_host(res->iov[0].iov_base, false,
> &offset);
> > + if (rb && memory_region_is_ram_device(rb->mr)) {
> > + VFIODevice *vdev = vfio_device_lookup(rb->mr);
> > +
> > + if (!vdev) {
> > + warn_report("Could not find device to create dmabuf");
>
> It is odd to print a warning only when the memory region is a RAM device
> not backed by VFIO while it prints no warning for the other incompatible
> memory regions. It is better to keep the behavior for the incompatible
> memory regions consistent.
Ok, I'll have the warning printed in other incompatible cases as well.
Thanks,
Vivek
>
> > + return;
> > + }
> > + vfio_create_dmabuf(vdev, res);
> > + } else {
> > + virtio_gpu_create_udmabuf(res);
> > + memfd_blob = true;
> > + }
> > +
> > if (res->dmabuf_fd < 0) {
> > return;
> > }
> > - virtio_gpu_remap_udmabuf(res);
> > - if (!res->remapped) {
> > - return;
> > +
> > + if (memfd_blob) {
> > + virtio_gpu_remap_udmabuf(res);
> > + if (!res->remapped) {
> > + return;
> > + }
> > + pdata = res->remapped;
> > }
> > - pdata = res->remapped;
> > }
> >
> > res->blob = pdata;
> > @@ -153,9 +199,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)
