[RFC 0/6] vfio: Implement VFIO_DEVICE_FEATURE_DMA_BUF and use it in virtio-gpu

[RFC 0/6] vfio: Implement VFIO_DEVICE_FEATURE_DMA_BUF and use it in virtio-gpu

[Vivek Kasireddy]

The virtio-gpu driver running in the Guest VM can create Guest blob
resources (by importing dmabufs) that are backed by System RAM. This
is made possible by making use of memfd memory backend and udmabuf
driver on the Host side. However, in order to create Guest blobs
that are backed by vfio-pci device regions, we have to implement
VFIO_DEVICE_FEATURE_DMA_BUF and leverage it in virtio-gpu.

So, while creating the blobs we use memory_region_is_ram_device() to
figure out if the blob is backed by memfd or a vfio-pci device. If
it is determined that the blob is backed by vfio-pci device region,
instead of calling into udmabuf driver to create dmabuf we would
now call into vfio-pci driver to have a dmabuf created on the Host.

Tested with an SRIOV enabled Intel dGPU by running Gnome Wayland
(in the VM) and Qemu with the following (relevant) parameters:
-device vfio-pci,host=0000:03:00.1
-device virtio-vga,max_outputs=1,xres=1920,yres=1080,blob=true
-display gtk,gl=on

Associated vfio-pci kernel driver series:
https://lore.kernel.org/dri-devel/cover.1754311439.git.leon@kernel.org/
Associated virtio-gpu kernel driver series (merged):
https://lore.kernel.org/dri-devel/20241126031643.3490496-1-vivek.kasireddy@intel.com/

---
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>
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Cédric Le Goater <clg@redhat.com>
Cc: Leon Romanovsky <leon@kernel.org>
Cc: Leon Romanovsky <leonro@nvidia.com>
Cc: Jason Gunthorpe <jgg@nvidia.com>
Cc: Dongwon Kim <dongwon.kim@intel.com>

Vivek Kasireddy (6):
  linux-headers: Update vfio.h to include VFIO_DEVICE_FEATURE_DMA_BUF
  vfio: Add support for VFIO_DEVICE_FEATURE_DMA_BUF
  virtio-gpu-udmabuf: Create dmabuf for blobs associated with VFIO
    devices
  virtio-gpu: Don't rely on res->blob to identify blob resources
  virtio-gpu: Recreate the resource's dmabuf if new backing is attached
  virtio-gpu: Find the host addr given gpa associated with a ram device

 hw/display/virtio-gpu-udmabuf.c | 60 ++++++++++++++++++++++++++++-----
 hw/display/virtio-gpu.c         | 58 +++++++++++++++++++++++++------
 hw/vfio/region.c                | 49 +++++++++++++++++++++++++++
 include/hw/vfio/vfio-device.h   |  3 ++
 linux-headers/linux/vfio.h      | 25 ++++++++++++++
 5 files changed, 176 insertions(+), 19 deletions(-)

-- 
2.50.1

[RFC 1/6] linux-headers: Update vfio.h to include VFIO_DEVICE_FEATURE_DMA_BUF

[Vivek Kasireddy]

Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 linux-headers/linux/vfio.h | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/linux-headers/linux/vfio.h b/linux-headers/linux/vfio.h
index 79bf8c0cc5..d2177c9cba 100644
--- a/linux-headers/linux/vfio.h
+++ b/linux-headers/linux/vfio.h
@@ -1468,6 +1468,31 @@ struct vfio_device_feature_bus_master {
 };
 #define VFIO_DEVICE_FEATURE_BUS_MASTER 10
 
+/**
+ * Upon VFIO_DEVICE_FEATURE_GET create a dma_buf fd for the
+ * regions selected.
+ *
+ * open_flags are the typical flags passed to open(2), eg O_RDWR, O_CLOEXEC,
+ * etc. offset/length specify a slice of the region to create the dmabuf from.
+ * nr_ranges is the total number of (P2P DMA) ranges that comprise the dmabuf.
+ *
+ * Return: The fd number on success, -1 and errno is set on failure.
+ */
+#define VFIO_DEVICE_FEATURE_DMA_BUF 11
+
+struct vfio_region_dma_range {
+	__u64 offset;
+	__u64 length;
+};
+
+struct vfio_device_feature_dma_buf {
+	__u32	region_index;
+	__u32	open_flags;
+	__u32   flags;
+	__u32   nr_ranges;
+	struct vfio_region_dma_range dma_ranges[];
+};
+
 /* -------- API for Type1 VFIO IOMMU -------- */
 
 /**
-- 
2.50.1

[RFC 2/6] vfio: Add support for VFIO_DEVICE_FEATURE_DMA_BUF

[Vivek Kasireddy]

In order to implement VFIO_DEVICE_FEATURE_DMA_BUF, we first need
to identify the PCI region 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/region.c              | 49 +++++++++++++++++++++++++++++++++++
 include/hw/vfio/vfio-device.h |  3 +++
 2 files changed, 52 insertions(+)

diff --git a/hw/vfio/region.c b/hw/vfio/region.c
index d04c57db63..b58188147c 100644
--- a/hw/vfio/region.c
+++ b/hw/vfio/region.c
@@ -28,6 +28,7 @@
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "qemu/units.h"
+#include "system/ramblock.h"
 #include "monitor/monitor.h"
 #include "vfio-helpers.h"
 
@@ -401,3 +402,51 @@ void vfio_region_mmaps_set_enabled(VFIORegion *region, bool enabled)
     trace_vfio_region_mmaps_set_enabled(memory_region_name(region->mem),
                                         enabled);
 }
+
+int vfio_device_create_dmabuf(VFIODevice *vdev,
+                              struct iovec *iov, unsigned int iov_cnt)
+{
+    g_autofree struct vfio_device_feature *feature;
+    struct vfio_device_feature_dma_buf *dma_buf;
+    VFIORegion *region;
+    ram_addr_t offset;
+    MemoryRegion *mr;
+    RAMBlock *rb;
+    size_t argsz;
+    int i;
+
+    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();
+        rb = qemu_ram_block_from_host(iov[i].iov_base, false, &offset);
+        rcu_read_unlock();
+
+        if (!rb) {
+            return -1;
+        }
+
+        mr = rb->mr;
+        if (mr->ops != &vfio_region_ops) {
+            mr = mr->container;
+            if (mr->ops != &vfio_region_ops) {
+                return -1;
+            }
+        }
+
+        region = mr->opaque;
+        dma_buf->region_index = region->nr;
+        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);
+}
diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
index 6e4d5ccdac..e127abd5f0 100644
--- a/include/hw/vfio/vfio-device.h
+++ b/include/hw/vfio/vfio-device.h
@@ -277,6 +277,9 @@ bool vfio_device_has_region_cap(VFIODevice *vbasedev, int region, uint16_t cap_t
 
 int vfio_device_get_irq_info(VFIODevice *vbasedev, int index,
                                 struct vfio_irq_info *info);
+
+int vfio_device_create_dmabuf(VFIODevice *vbasedev,
+                              struct iovec *iov, unsigned int iov_cnt);
 #endif
 
 /* Returns 0 on success, or a negative errno. */
-- 
2.50.1

[RFC 3/6] virtio-gpu-udmabuf: Create dmabuf for blobs associated with VFIO devices

[Vivek Kasireddy]

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.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/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;
+    }
+
+    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");
+                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)
-- 
2.50.1

[RFC 4/6] virtio-gpu: Don't rely on res->blob to identify blob resources

[Vivek Kasireddy]

The res->blob pointer is only valid for blobs that have their
backing storage in memfd. Therefore, we cannot use it to determine
if a resource is a blob or not. Instead, we could use res->blob_size
to make this determination as it is non-zero for blob resources
regardless of where their backing storage is located.

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/virtio-gpu.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 0a1a625b0e..2f9133c3b6 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -57,7 +57,7 @@ void virtio_gpu_update_cursor_data(VirtIOGPU *g,
     }
 
     if (res->blob_size) {
-        if (res->blob_size < (s->current_cursor->width *
+        if (!res->blob || res->blob_size < (s->current_cursor->width *
                               s->current_cursor->height * 4)) {
             return;
         }
@@ -144,7 +144,7 @@ virtio_gpu_find_check_resource(VirtIOGPU *g, uint32_t resource_id,
     }
 
     if (require_backing) {
-        if (!res->iov || (!res->image && !res->blob)) {
+        if (!res->iov || (!res->image && !res->blob_size)) {
             qemu_log_mask(LOG_GUEST_ERROR, "%s: no backing storage %d\n",
                           caller, resource_id);
             if (error) {
@@ -444,7 +444,7 @@ static void virtio_gpu_transfer_to_host_2d(VirtIOGPU *g,
 
     res = virtio_gpu_find_check_resource(g, t2d.resource_id, true,
                                          __func__, &cmd->error);
-    if (!res || res->blob) {
+    if (!res || res->blob_size) {
         return;
     }
 
@@ -507,7 +507,7 @@ static void virtio_gpu_resource_flush(VirtIOGPU *g,
         return;
     }
 
-    if (res->blob) {
+    if (res->blob_size) {
         for (i = 0; i < g->parent_obj.conf.max_outputs; i++) {
             scanout = &g->parent_obj.scanout[i];
             if (scanout->resource_id == res->resource_id &&
@@ -538,7 +538,7 @@ static void virtio_gpu_resource_flush(VirtIOGPU *g,
         }
     }
 
-    if (!res->blob &&
+    if (!res->blob_size &&
         (rf.r.x > res->width ||
         rf.r.y > res->height ||
         rf.r.width > res->width ||
@@ -634,7 +634,7 @@ static bool virtio_gpu_do_set_scanout(VirtIOGPU *g,
 
     g->parent_obj.enable = 1;
 
-    if (res->blob) {
+    if (res->blob_size) {
         if (console_has_gl(scanout->con)) {
             if (!virtio_gpu_update_dmabuf(g, scanout_id, res, fb, r)) {
                 virtio_gpu_update_scanout(g, scanout_id, res, fb, r);
@@ -645,13 +645,16 @@ static bool virtio_gpu_do_set_scanout(VirtIOGPU *g,
             return true;
         }
 
+        if (!res->blob) {
+            return false;
+        }
         data = res->blob;
     } else {
         data = (uint8_t *)pixman_image_get_data(res->image);
     }
 
     /* create a surface for this scanout */
-    if ((res->blob && !console_has_gl(scanout->con)) ||
+    if ((res->blob_size && !console_has_gl(scanout->con)) ||
         !scanout->ds ||
         surface_data(scanout->ds) != data + fb->offset ||
         scanout->width != r->width ||
@@ -899,7 +902,7 @@ void virtio_gpu_cleanup_mapping(VirtIOGPU *g,
     g_free(res->addrs);
     res->addrs = NULL;
 
-    if (res->blob) {
+    if (res->blob_size) {
         virtio_gpu_fini_udmabuf(res);
     }
 }
-- 
2.50.1

[RFC 5/6] virtio-gpu: Recreate the resource's dmabuf if new backing is attached

[Vivek Kasireddy]

There are cases when a blob resource's backing might get detached
and re-attached again such as when the underlying object is getting
migrated in the Guest. In these situations, we need to obtain a new
dmabuf fd, which can be done by calling virtio_gpu_init_udmabuf().

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/virtio-gpu.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 2f9133c3b6..1654a417b8 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -938,6 +938,10 @@ virtio_gpu_resource_attach_backing(VirtIOGPU *g,
         cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
         return;
     }
+
+    if (res->blob_size && res->dmabuf_fd < 0) {
+        virtio_gpu_init_udmabuf(res);
+    }
 }
 
 static void
-- 
2.50.1

[RFC 6/6] virtio-gpu: Find the host addr given gpa associated with a ram device

[Vivek Kasireddy]

If the Guest provides a gpa (guest physical address) associated with
a PCI region, then we can obtain the hva (host virtual address) via
gpa2hva() API instead of dma_memory_map(). Note that we would still
call dma_memory_unmap() (to unref mr) regardless of how we obtained
the hva.

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/virtio-gpu.c | 35 ++++++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 1654a417b8..4af8390bb5 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -32,6 +32,7 @@
 #include "qemu/module.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
+#include "monitor/monitor.h"
 
 #define VIRTIO_GPU_VM_VERSION 1
 
@@ -799,6 +800,36 @@ static void virtio_gpu_set_scanout_blob(VirtIOGPU *g,
                               &fb, res, &ss.r, &cmd->error);
 }
 
+static void *map_gpa2hva(VirtIOGPU *g,
+                         struct virtio_gpu_ctrl_command *cmd,
+                         uint64_t gpa, hwaddr *len)
+{
+    MemoryRegion *mr = NULL;
+    Error *errp = NULL;
+    void *map;
+
+    if (cmd->cmd_hdr.type != VIRTIO_GPU_CMD_RESOURCE_CREATE_BLOB) {
+        return dma_memory_map(VIRTIO_DEVICE(g)->dma_as, gpa, len,
+                              DMA_DIRECTION_TO_DEVICE,
+                              MEMTXATTRS_UNSPECIFIED);
+    }
+
+    map = gpa2hva(&mr, gpa, 1, &errp);
+    if (errp) {
+        error_report_err(errp);
+        return NULL;
+    }
+
+    if (!memory_region_is_ram_device(mr)) {
+        memory_region_unref(mr);
+        map = dma_memory_map(VIRTIO_DEVICE(g)->dma_as, gpa, len,
+                             DMA_DIRECTION_TO_DEVICE,
+                             MEMTXATTRS_UNSPECIFIED);
+    }
+
+    return map;
+}
+
 int virtio_gpu_create_mapping_iov(VirtIOGPU *g,
                                   uint32_t nr_entries, uint32_t offset,
                                   struct virtio_gpu_ctrl_command *cmd,
@@ -840,9 +871,7 @@ int virtio_gpu_create_mapping_iov(VirtIOGPU *g,
 
         do {
             len = l;
-            map = dma_memory_map(VIRTIO_DEVICE(g)->dma_as, a, &len,
-                                 DMA_DIRECTION_TO_DEVICE,
-                                 MEMTXATTRS_UNSPECIFIED);
+            map = map_gpa2hva(g, cmd, a, &len);
             if (!map) {
                 qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to map MMIO memory for"
                               " element %d\n", __func__, e);
-- 
2.50.1

Re: [RFC 3/6] virtio-gpu-udmabuf: Create dmabuf for blobs associated with VFIO devices

[Akihiko Odaki]

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.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/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.

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

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

Re: [RFC 4/6] virtio-gpu: Don't rely on res->blob to identify blob resources

[Akihiko Odaki]

On 2025/09/03 7:42, Vivek Kasireddy wrote:
> The res->blob pointer is only valid for blobs that have their
> backing storage in memfd. Therefore, we cannot use it to determine
> if a resource is a blob or not. Instead, we could use res->blob_size
> to make this determination as it is non-zero for blob resources
> regardless of where their backing storage is located.

I guess this change needs to be applied before "[RFC 3/6] 
virtio-gpu-udmabuf: Create dmabuf for blobs associated with VFIO 
devices"; without this patch, the "create dmabuf" patch will probably 
create an invalid blob.

> 
> 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/virtio-gpu.c | 19 +++++++++++--------
>   1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 0a1a625b0e..2f9133c3b6 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -57,7 +57,7 @@ void virtio_gpu_update_cursor_data(VirtIOGPU *g,
>       }
>   
>       if (res->blob_size) {
> -        if (res->blob_size < (s->current_cursor->width *
> +        if (!res->blob || res->blob_size < (s->current_cursor->width *

I doubt that rejecting a valid blob due to an implementation concern 
(whether the backing storage is in memfd) is tolerated in the specification.

>                                 s->current_cursor->height * 4)) {
>               return;
>           }
> @@ -144,7 +144,7 @@ virtio_gpu_find_check_resource(VirtIOGPU *g, uint32_t resource_id,
>       }
>   
>       if (require_backing) {
> -        if (!res->iov || (!res->image && !res->blob)) {
> +        if (!res->iov || (!res->image && !res->blob_size)) {
>               qemu_log_mask(LOG_GUEST_ERROR, "%s: no backing storage %d\n",
>                             caller, resource_id);
>               if (error) {
> @@ -444,7 +444,7 @@ static void virtio_gpu_transfer_to_host_2d(VirtIOGPU *g,
>   
>       res = virtio_gpu_find_check_resource(g, t2d.resource_id, true,
>                                            __func__, &cmd->error);
> -    if (!res || res->blob) {
> +    if (!res || res->blob_size) {
>           return;
>       }
>   
> @@ -507,7 +507,7 @@ static void virtio_gpu_resource_flush(VirtIOGPU *g,
>           return;
>       }
>   
> -    if (res->blob) {
> +    if (res->blob_size) {
>           for (i = 0; i < g->parent_obj.conf.max_outputs; i++) {
>               scanout = &g->parent_obj.scanout[i];
>               if (scanout->resource_id == res->resource_id &&
> @@ -538,7 +538,7 @@ static void virtio_gpu_resource_flush(VirtIOGPU *g,
>           }
>       }
>   
> -    if (!res->blob &&
> +    if (!res->blob_size &&
>           (rf.r.x > res->width ||
>           rf.r.y > res->height ||
>           rf.r.width > res->width ||
> @@ -634,7 +634,7 @@ static bool virtio_gpu_do_set_scanout(VirtIOGPU *g,
>   
>       g->parent_obj.enable = 1;
>   
> -    if (res->blob) {
> +    if (res->blob_size) {
>           if (console_has_gl(scanout->con)) {
>               if (!virtio_gpu_update_dmabuf(g, scanout_id, res, fb, r)) {
>                   virtio_gpu_update_scanout(g, scanout_id, res, fb, r);
> @@ -645,13 +645,16 @@ static bool virtio_gpu_do_set_scanout(VirtIOGPU *g,
>               return true;
>           }
>   
> +        if (!res->blob) {
> +            return false;
> +        }
>           data = res->blob;
>       } else {
>           data = (uint8_t *)pixman_image_get_data(res->image);
>       }
>   
>       /* create a surface for this scanout */
> -    if ((res->blob && !console_has_gl(scanout->con)) ||
> +    if ((res->blob_size && !console_has_gl(scanout->con)) ||
>           !scanout->ds ||
>           surface_data(scanout->ds) != data + fb->offset ||
>           scanout->width != r->width ||
> @@ -899,7 +902,7 @@ void virtio_gpu_cleanup_mapping(VirtIOGPU *g,
>       g_free(res->addrs);
>       res->addrs = NULL;
>   
> -    if (res->blob) {
> +    if (res->blob_size) {
>           virtio_gpu_fini_udmabuf(res);
>       }
>   }

Re: [RFC 5/6] virtio-gpu: Recreate the resource's dmabuf if new backing is attached

[Akihiko Odaki]

On 2025/09/03 7:42, Vivek Kasireddy wrote:
> There are cases when a blob resource's backing might get detached
> and re-attached again such as when the underlying object is getting
> migrated in the Guest. In these situations, we need to obtain a new
> dmabuf fd, which can be done by calling virtio_gpu_init_udmabuf().

It sounds like a bug fix. Perhaps you may add a Fixes: tag and reorder 
patches to make this change come first.

> 
> 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/virtio-gpu.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 2f9133c3b6..1654a417b8 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -938,6 +938,10 @@ virtio_gpu_resource_attach_backing(VirtIOGPU *g,
>           cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>           return;
>       }
> +
> +    if (res->blob_size && res->dmabuf_fd < 0) {
> +        virtio_gpu_init_udmabuf(res);
> +    }
>   }
>   
>   static void

Re: [RFC 6/6] virtio-gpu: Find the host addr given gpa associated with a ram device

[Akihiko Odaki]

On 2025/09/03 7:42, Vivek Kasireddy wrote:
> If the Guest provides a gpa (guest physical address) associated with
> a PCI region, then we can obtain the hva (host virtual address) via
> gpa2hva() API instead of dma_memory_map(). Note that we would still
> call dma_memory_unmap() (to unref mr) regardless of how we obtained
> the hva.

I think address_space_translate() should be used instead. The guest 
passes addresses that are valid in the DMA address space, which may not 
be a GPA if an IOMMU is in effect. address_space_translate() allows you 
specifying the DMA address space.

The motivation for this change should also be described in the patch 
message; otherwise a reader may wonder why dma_memory_map() does not 
suffice.

While I added comments for each patch, the overall implementation 
direction of this series looks good to me.

Regards,
Akihiko Odaki

> 
> 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/virtio-gpu.c | 35 ++++++++++++++++++++++++++++++++---
>   1 file changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 1654a417b8..4af8390bb5 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -32,6 +32,7 @@
>   #include "qemu/module.h"
>   #include "qapi/error.h"
>   #include "qemu/error-report.h"
> +#include "monitor/monitor.h"
>   
>   #define VIRTIO_GPU_VM_VERSION 1
>   
> @@ -799,6 +800,36 @@ static void virtio_gpu_set_scanout_blob(VirtIOGPU *g,
>                                 &fb, res, &ss.r, &cmd->error);
>   }
>   
> +static void *map_gpa2hva(VirtIOGPU *g,
> +                         struct virtio_gpu_ctrl_command *cmd,
> +                         uint64_t gpa, hwaddr *len)
> +{
> +    MemoryRegion *mr = NULL;
> +    Error *errp = NULL;
> +    void *map;
> +
> +    if (cmd->cmd_hdr.type != VIRTIO_GPU_CMD_RESOURCE_CREATE_BLOB) {
> +        return dma_memory_map(VIRTIO_DEVICE(g)->dma_as, gpa, len,
> +                              DMA_DIRECTION_TO_DEVICE,
> +                              MEMTXATTRS_UNSPECIFIED);
> +    }
> +
> +    map = gpa2hva(&mr, gpa, 1, &errp);
> +    if (errp) {
> +        error_report_err(errp);
> +        return NULL;
> +    }
> +
> +    if (!memory_region_is_ram_device(mr)) {
> +        memory_region_unref(mr);
> +        map = dma_memory_map(VIRTIO_DEVICE(g)->dma_as, gpa, len,
> +                             DMA_DIRECTION_TO_DEVICE,
> +                             MEMTXATTRS_UNSPECIFIED);
> +    }
> +
> +    return map;
> +}
> +
>   int virtio_gpu_create_mapping_iov(VirtIOGPU *g,
>                                     uint32_t nr_entries, uint32_t offset,
>                                     struct virtio_gpu_ctrl_command *cmd,
> @@ -840,9 +871,7 @@ int virtio_gpu_create_mapping_iov(VirtIOGPU *g,
>   
>           do {
>               len = l;
> -            map = dma_memory_map(VIRTIO_DEVICE(g)->dma_as, a, &len,
> -                                 DMA_DIRECTION_TO_DEVICE,
> -                                 MEMTXATTRS_UNSPECIFIED);
> +            map = map_gpa2hva(g, cmd, a, &len);
>               if (!map) {
>                   qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to map MMIO memory for"
>                                 " element %d\n", __func__, e);

RE: [RFC 6/6] virtio-gpu: Find the host addr given gpa associated with a ram device

[Kasireddy, Vivek]

Hi Akihiko,

> Subject: Re: [RFC 6/6] virtio-gpu: Find the host addr given gpa associated
> with a ram device
> 
> > If the Guest provides a gpa (guest physical address) associated with
> > a PCI region, then we can obtain the hva (host virtual address) via
> > gpa2hva() API instead of dma_memory_map(). Note that we would still
> > call dma_memory_unmap() (to unref mr) regardless of how we obtained
> > the hva.
> 
> I think address_space_translate() should be used instead. The guest
> passes addresses that are valid in the DMA address space, which may not
> be a GPA if an IOMMU is in effect. address_space_translate() allows you
> specifying the DMA address space.
Thank you for your suggestion. Looks like address_space_translate() followed
by memory_region_get_ram_ptr() also makes it work and is much cleaner. I'll
include this change in the next version.

> 
> The motivation for this change should also be described in the patch
> message; otherwise a reader may wonder why dma_memory_map() does
> not
> suffice.
Sure, I'll improve the commit message to explain why dma_memory_map()
does not work in this case.

> 
> While I added comments for each patch, the overall implementation
> direction of this series looks good to me.
Thank you for taking a look!

Thanks,
Vivek

> 
> Regards,
> Akihiko Odaki
> 
> >
> > 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/virtio-gpu.c | 35 ++++++++++++++++++++++++++++++++---
> >   1 file changed, 32 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> > index 1654a417b8..4af8390bb5 100644
> > --- a/hw/display/virtio-gpu.c
> > +++ b/hw/display/virtio-gpu.c
> > @@ -32,6 +32,7 @@
> >   #include "qemu/module.h"
> >   #include "qapi/error.h"
> >   #include "qemu/error-report.h"
> > +#include "monitor/monitor.h"
> >
> >   #define VIRTIO_GPU_VM_VERSION 1
> >
> > @@ -799,6 +800,36 @@ static void
> virtio_gpu_set_scanout_blob(VirtIOGPU *g,
> >                                 &fb, res, &ss.r, &cmd->error);
> >   }
> >
> > +static void *map_gpa2hva(VirtIOGPU *g,
> > +                         struct virtio_gpu_ctrl_command *cmd,
> > +                         uint64_t gpa, hwaddr *len)
> > +{
> > +    MemoryRegion *mr = NULL;
> > +    Error *errp = NULL;
> > +    void *map;
> > +
> > +    if (cmd->cmd_hdr.type !=
> VIRTIO_GPU_CMD_RESOURCE_CREATE_BLOB) {
> > +        return dma_memory_map(VIRTIO_DEVICE(g)->dma_as, gpa, len,
> > +                              DMA_DIRECTION_TO_DEVICE,
> > +                              MEMTXATTRS_UNSPECIFIED);
> > +    }
> > +
> > +    map = gpa2hva(&mr, gpa, 1, &errp);
> > +    if (errp) {
> > +        error_report_err(errp);
> > +        return NULL;
> > +    }
> > +
> > +    if (!memory_region_is_ram_device(mr)) {
> > +        memory_region_unref(mr);
> > +        map = dma_memory_map(VIRTIO_DEVICE(g)->dma_as, gpa, len,
> > +                             DMA_DIRECTION_TO_DEVICE,
> > +                             MEMTXATTRS_UNSPECIFIED);
> > +    }
> > +
> > +    return map;
> > +}
> > +
> >   int virtio_gpu_create_mapping_iov(VirtIOGPU *g,
> >                                     uint32_t nr_entries, uint32_t offset,
> >                                     struct virtio_gpu_ctrl_command *cmd,
> > @@ -840,9 +871,7 @@ int virtio_gpu_create_mapping_iov(VirtIOGPU *g,
> >
> >           do {
> >               len = l;
> > -            map = dma_memory_map(VIRTIO_DEVICE(g)->dma_as, a, &len,
> > -                                 DMA_DIRECTION_TO_DEVICE,
> > -                                 MEMTXATTRS_UNSPECIFIED);
> > +            map = map_gpa2hva(g, cmd, a, &len);
> >               if (!map) {
> >                   qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to map MMIO
> memory for"
> >                                 " element %d\n", __func__, e);

RE: [RFC 3/6] virtio-gpu-udmabuf: Create dmabuf for blobs associated with VFIO devices

[Kasireddy, Vivek]

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.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/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)

RE: [RFC 4/6] virtio-gpu: Don't rely on res->blob to identify blob resources

[Kasireddy, Vivek]

Hi Akihiko,

> Subject: Re: [RFC 4/6] virtio-gpu: Don't rely on res->blob to identify blob
> resources
> 
> On 2025/09/03 7:42, Vivek Kasireddy wrote:
> > The res->blob pointer is only valid for blobs that have their
> > backing storage in memfd. Therefore, we cannot use it to determine
> > if a resource is a blob or not. Instead, we could use res->blob_size
> > to make this determination as it is non-zero for blob resources
> > regardless of where their backing storage is located.
> 
> I guess this change needs to be applied before "[RFC 3/6]
> virtio-gpu-udmabuf: Create dmabuf for blobs associated with VFIO
> devices"; without this patch, the "create dmabuf" patch will probably
> create an invalid blob.
Ok, makes sense. I'll move it earlier in the patch series.

> 
> >
> > 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/virtio-gpu.c | 19 +++++++++++--------
> >   1 file changed, 11 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> > index 0a1a625b0e..2f9133c3b6 100644
> > --- a/hw/display/virtio-gpu.c
> > +++ b/hw/display/virtio-gpu.c
> > @@ -57,7 +57,7 @@ void virtio_gpu_update_cursor_data(VirtIOGPU *g,
> >       }
> >
> >       if (res->blob_size) {
> > -        if (res->blob_size < (s->current_cursor->width *
> > +        if (!res->blob || res->blob_size < (s->current_cursor->width *
> 
> I doubt that rejecting a valid blob due to an implementation concern
> (whether the backing storage is in memfd) is tolerated in the specification.
Are you suggesting that the whole if (res->blob_size < (s->current_cursor->width *...
check needs to be removed? I think it is just a sanity check to ensure that the blob
size is big enough for cursor.

Thanks,
Vivek

> 
> >                                 s->current_cursor->height * 4)) {
> >               return;
> >           }
> > @@ -144,7 +144,7 @@ virtio_gpu_find_check_resource(VirtIOGPU *g,
> uint32_t resource_id,
> >       }
> >
> >       if (require_backing) {
> > -        if (!res->iov || (!res->image && !res->blob)) {
> > +        if (!res->iov || (!res->image && !res->blob_size)) {
> >               qemu_log_mask(LOG_GUEST_ERROR, "%s: no backing storage
> %d\n",
> >                             caller, resource_id);
> >               if (error) {
> > @@ -444,7 +444,7 @@ static void
> virtio_gpu_transfer_to_host_2d(VirtIOGPU *g,
> >
> >       res = virtio_gpu_find_check_resource(g, t2d.resource_id, true,
> >                                            __func__, &cmd->error);
> > -    if (!res || res->blob) {
> > +    if (!res || res->blob_size) {
> >           return;
> >       }
> >
> > @@ -507,7 +507,7 @@ static void virtio_gpu_resource_flush(VirtIOGPU *g,
> >           return;
> >       }
> >
> > -    if (res->blob) {
> > +    if (res->blob_size) {
> >           for (i = 0; i < g->parent_obj.conf.max_outputs; i++) {
> >               scanout = &g->parent_obj.scanout[i];
> >               if (scanout->resource_id == res->resource_id &&
> > @@ -538,7 +538,7 @@ static void virtio_gpu_resource_flush(VirtIOGPU *g,
> >           }
> >       }
> >
> > -    if (!res->blob &&
> > +    if (!res->blob_size &&
> >           (rf.r.x > res->width ||
> >           rf.r.y > res->height ||
> >           rf.r.width > res->width ||
> > @@ -634,7 +634,7 @@ static bool virtio_gpu_do_set_scanout(VirtIOGPU
> *g,
> >
> >       g->parent_obj.enable = 1;
> >
> > -    if (res->blob) {
> > +    if (res->blob_size) {
> >           if (console_has_gl(scanout->con)) {
> >               if (!virtio_gpu_update_dmabuf(g, scanout_id, res, fb, r)) {
> >                   virtio_gpu_update_scanout(g, scanout_id, res, fb, r);
> > @@ -645,13 +645,16 @@ static bool
> virtio_gpu_do_set_scanout(VirtIOGPU *g,
> >               return true;
> >           }
> >
> > +        if (!res->blob) {
> > +            return false;
> > +        }
> >           data = res->blob;
> >       } else {
> >           data = (uint8_t *)pixman_image_get_data(res->image);
> >       }
> >
> >       /* create a surface for this scanout */
> > -    if ((res->blob && !console_has_gl(scanout->con)) ||
> > +    if ((res->blob_size && !console_has_gl(scanout->con)) ||
> >           !scanout->ds ||
> >           surface_data(scanout->ds) != data + fb->offset ||
> >           scanout->width != r->width ||
> > @@ -899,7 +902,7 @@ void virtio_gpu_cleanup_mapping(VirtIOGPU *g,
> >       g_free(res->addrs);
> >       res->addrs = NULL;
> >
> > -    if (res->blob) {
> > +    if (res->blob_size) {
> >           virtio_gpu_fini_udmabuf(res);
> >       }
> >   }

RE: [RFC 5/6] virtio-gpu: Recreate the resource's dmabuf if new backing is attached

[Kasireddy, Vivek]

> Subject: Re: [RFC 5/6] virtio-gpu: Recreate the resource's dmabuf if new
> backing is attached
> 
> On 2025/09/03 7:42, Vivek Kasireddy wrote:
> > There are cases when a blob resource's backing might get detached
> > and re-attached again such as when the underlying object is getting
> > migrated in the Guest. In these situations, we need to obtain a new
> > dmabuf fd, which can be done by calling virtio_gpu_init_udmabuf().
> 
> It sounds like a bug fix. Perhaps you may add a Fixes: tag and reorder
> patches to make this change come first.
Sure, I can do that.

Thanks,
Vivek

> 
> >
> > 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/virtio-gpu.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> > index 2f9133c3b6..1654a417b8 100644
> > --- a/hw/display/virtio-gpu.c
> > +++ b/hw/display/virtio-gpu.c
> > @@ -938,6 +938,10 @@ virtio_gpu_resource_attach_backing(VirtIOGPU
> *g,
> >           cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
> >           return;
> >       }
> > +
> > +    if (res->blob_size && res->dmabuf_fd < 0) {
> > +        virtio_gpu_init_udmabuf(res);
> > +    }
> >   }
> >
> >   static void

Re: [RFC 4/6] virtio-gpu: Don't rely on res->blob to identify blob resources

[Akihiko Odaki]

On 2025/09/13 11:48, Kasireddy, Vivek wrote:
> Hi Akihiko,
> 
>> Subject: Re: [RFC 4/6] virtio-gpu: Don't rely on res->blob to identify blob
>> resources
>>
>> On 2025/09/03 7:42, Vivek Kasireddy wrote:
>>> The res->blob pointer is only valid for blobs that have their
>>> backing storage in memfd. Therefore, we cannot use it to determine
>>> if a resource is a blob or not. Instead, we could use res->blob_size
>>> to make this determination as it is non-zero for blob resources
>>> regardless of where their backing storage is located.
>>
>> I guess this change needs to be applied before "[RFC 3/6]
>> virtio-gpu-udmabuf: Create dmabuf for blobs associated with VFIO
>> devices"; without this patch, the "create dmabuf" patch will probably
>> create an invalid blob.
> Ok, makes sense. I'll move it earlier in the patch series.
> 
>>
>>>
>>> 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/virtio-gpu.c | 19 +++++++++++--------
>>>    1 file changed, 11 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
>>> index 0a1a625b0e..2f9133c3b6 100644
>>> --- a/hw/display/virtio-gpu.c
>>> +++ b/hw/display/virtio-gpu.c
>>> @@ -57,7 +57,7 @@ void virtio_gpu_update_cursor_data(VirtIOGPU *g,
>>>        }
>>>
>>>        if (res->blob_size) {
>>> -        if (res->blob_size < (s->current_cursor->width *
>>> +        if (!res->blob || res->blob_size < (s->current_cursor->width *
>>
>> I doubt that rejecting a valid blob due to an implementation concern
>> (whether the backing storage is in memfd) is tolerated in the specification.
> Are you suggesting that the whole if (res->blob_size < (s->current_cursor->width *...
> check needs to be removed? I think it is just a sanity check to ensure that the blob
> size is big enough for cursor.

I referred to !res->blob, the new condition. It rejects a valid blob 
that is backed by VFIO.

Regards,
Akihiko Odaki

Re: [RFC 3/6] virtio-gpu-udmabuf: Create dmabuf for blobs associated with VFIO devices

[Akihiko Odaki]

On 2025/09/13 11:43, Kasireddy, Vivek wrote:
> 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.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/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.
By the way, this condition should be reported with LOG_GUEST_ERROR 
instead of warn_report(). LOG_GUEST_ERROR is for the situation where the 
guest does something wrong while the host does the correct thing. It is 
the guest's fault if it passes an incompatible address.

Regards,
Akihiko Odaki

RE: [RFC 4/6] virtio-gpu: Don't rely on res->blob to identify blob resources

[Kasireddy, Vivek]

Hi Akihiko,

> Subject: Re: [RFC 4/6] virtio-gpu: Don't rely on res->blob to identify blob
> resources
> 
> On 2025/09/13 11:48, Kasireddy, Vivek wrote:
> > Hi Akihiko,
> >
> >> Subject: Re: [RFC 4/6] virtio-gpu: Don't rely on res->blob to identify blob
> >> resources
> >>
> >> On 2025/09/03 7:42, Vivek Kasireddy wrote:
> >>> The res->blob pointer is only valid for blobs that have their
> >>> backing storage in memfd. Therefore, we cannot use it to determine
> >>> if a resource is a blob or not. Instead, we could use res->blob_size
> >>> to make this determination as it is non-zero for blob resources
> >>> regardless of where their backing storage is located.
> >>
> >> I guess this change needs to be applied before "[RFC 3/6]
> >> virtio-gpu-udmabuf: Create dmabuf for blobs associated with VFIO
> >> devices"; without this patch, the "create dmabuf" patch will probably
> >> create an invalid blob.
> > Ok, makes sense. I'll move it earlier in the patch series.
> >
> >>
> >>>
> >>> 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/virtio-gpu.c | 19 +++++++++++--------
> >>>    1 file changed, 11 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> >>> index 0a1a625b0e..2f9133c3b6 100644
> >>> --- a/hw/display/virtio-gpu.c
> >>> +++ b/hw/display/virtio-gpu.c
> >>> @@ -57,7 +57,7 @@ void virtio_gpu_update_cursor_data(VirtIOGPU *g,
> >>>        }
> >>>
> >>>        if (res->blob_size) {
> >>> -        if (res->blob_size < (s->current_cursor->width *
> >>> +        if (!res->blob || res->blob_size < (s->current_cursor->width *
> >>
> >> I doubt that rejecting a valid blob due to an implementation concern
> >> (whether the backing storage is in memfd) is tolerated in the specification.
> > Are you suggesting that the whole if (res->blob_size < (s->current_cursor-
> >width *...
> > check needs to be removed? I think it is just a sanity check to ensure that the
> blob
> > size is big enough for cursor.
> 
> I referred to !res->blob, the new condition. It rejects a valid blob
> that is backed by VFIO.
The problem is that for blobs backed by VFIO, res->blob can be NULL but this function
(virtio_gpu_update_cursor_data) is relying on res->blob always being valid. That's why
the new condition !res->blob is needed here to check the validity of res->blob.

Thanks,
Vivek

> 
> Regards,
> Akihiko Odaki

Re: [RFC 4/6] virtio-gpu: Don't rely on res->blob to identify blob resources

[Akihiko Odaki]

On 2025/09/15 15:33, Kasireddy, Vivek wrote:
> Hi Akihiko,
> 
>> Subject: Re: [RFC 4/6] virtio-gpu: Don't rely on res->blob to identify blob
>> resources
>>
>> On 2025/09/13 11:48, Kasireddy, Vivek wrote:
>>> Hi Akihiko,
>>>
>>>> Subject: Re: [RFC 4/6] virtio-gpu: Don't rely on res->blob to identify blob
>>>> resources
>>>>
>>>> On 2025/09/03 7:42, Vivek Kasireddy wrote:
>>>>> The res->blob pointer is only valid for blobs that have their
>>>>> backing storage in memfd. Therefore, we cannot use it to determine
>>>>> if a resource is a blob or not. Instead, we could use res->blob_size
>>>>> to make this determination as it is non-zero for blob resources
>>>>> regardless of where their backing storage is located.
>>>>
>>>> I guess this change needs to be applied before "[RFC 3/6]
>>>> virtio-gpu-udmabuf: Create dmabuf for blobs associated with VFIO
>>>> devices"; without this patch, the "create dmabuf" patch will probably
>>>> create an invalid blob.
>>> Ok, makes sense. I'll move it earlier in the patch series.
>>>
>>>>
>>>>>
>>>>> 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/virtio-gpu.c | 19 +++++++++++--------
>>>>>     1 file changed, 11 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
>>>>> index 0a1a625b0e..2f9133c3b6 100644
>>>>> --- a/hw/display/virtio-gpu.c
>>>>> +++ b/hw/display/virtio-gpu.c
>>>>> @@ -57,7 +57,7 @@ void virtio_gpu_update_cursor_data(VirtIOGPU *g,
>>>>>         }
>>>>>
>>>>>         if (res->blob_size) {
>>>>> -        if (res->blob_size < (s->current_cursor->width *
>>>>> +        if (!res->blob || res->blob_size < (s->current_cursor->width *
>>>>
>>>> I doubt that rejecting a valid blob due to an implementation concern
>>>> (whether the backing storage is in memfd) is tolerated in the specification.
>>> Are you suggesting that the whole if (res->blob_size < (s->current_cursor-
>>> width *...
>>> check needs to be removed? I think it is just a sanity check to ensure that the
>> blob
>>> size is big enough for cursor.
>>
>> I referred to !res->blob, the new condition. It rejects a valid blob
>> that is backed by VFIO.
> The problem is that for blobs backed by VFIO, res->blob can be NULL but this function
> (virtio_gpu_update_cursor_data) is relying on res->blob always being valid. That's why
> the new condition !res->blob is needed here to check the validity of res->blob.

I understand the host-side implementation difficulty to support this 
operation for VFIO, but the guest may not be prepared for the failure of 
the operation so we shouldn't simply reject it.

By the way, perhaps it may be possible to provide res->blob for VFIO. 
Since "[RFC 3/6] virtio-gpu-udmabuf: Create dmabuf for blobs associated 
with VFIO devices" checks memory_region_is_ram_device(), the VFIO 
backend providing the blob should support mmap().

Regards,
Akihiko Odaki

RE: [RFC 4/6] virtio-gpu: Don't rely on res->blob to identify blob resources

[Kasireddy, Vivek]

> Subject: Re: [RFC 4/6] virtio-gpu: Don't rely on res->blob to identify blob
> resources
> 
> On 2025/09/15 15:33, Kasireddy, Vivek wrote:
> > Hi Akihiko,
> >
> >> Subject: Re: [RFC 4/6] virtio-gpu: Don't rely on res->blob to identify blob
> >> resources
> >>
> >> On 2025/09/13 11:48, Kasireddy, Vivek wrote:
> >>> Hi Akihiko,
> >>>
> >>>> Subject: Re: [RFC 4/6] virtio-gpu: Don't rely on res->blob to identify
> blob
> >>>> resources
> >>>>
> >>>> On 2025/09/03 7:42, Vivek Kasireddy wrote:
> >>>>> The res->blob pointer is only valid for blobs that have their
> >>>>> backing storage in memfd. Therefore, we cannot use it to determine
> >>>>> if a resource is a blob or not. Instead, we could use res->blob_size
> >>>>> to make this determination as it is non-zero for blob resources
> >>>>> regardless of where their backing storage is located.
> >>>>
> >>>> I guess this change needs to be applied before "[RFC 3/6]
> >>>> virtio-gpu-udmabuf: Create dmabuf for blobs associated with VFIO
> >>>> devices"; without this patch, the "create dmabuf" patch will probably
> >>>> create an invalid blob.
> >>> Ok, makes sense. I'll move it earlier in the patch series.
> >>>
> >>>>
> >>>>>
> >>>>> 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/virtio-gpu.c | 19 +++++++++++--------
> >>>>>     1 file changed, 11 insertions(+), 8 deletions(-)
> >>>>>
> >>>>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> >>>>> index 0a1a625b0e..2f9133c3b6 100644
> >>>>> --- a/hw/display/virtio-gpu.c
> >>>>> +++ b/hw/display/virtio-gpu.c
> >>>>> @@ -57,7 +57,7 @@ void virtio_gpu_update_cursor_data(VirtIOGPU
> *g,
> >>>>>         }
> >>>>>
> >>>>>         if (res->blob_size) {
> >>>>> -        if (res->blob_size < (s->current_cursor->width *
> >>>>> +        if (!res->blob || res->blob_size < (s->current_cursor->width *
> >>>>
> >>>> I doubt that rejecting a valid blob due to an implementation concern
> >>>> (whether the backing storage is in memfd) is tolerated in the
> specification.
> >>> Are you suggesting that the whole if (res->blob_size < (s-
> >current_cursor-
> >>> width *...
> >>> check needs to be removed? I think it is just a sanity check to ensure
> that the
> >> blob
> >>> size is big enough for cursor.
> >>
> >> I referred to !res->blob, the new condition. It rejects a valid blob
> >> that is backed by VFIO.
> > The problem is that for blobs backed by VFIO, res->blob can be NULL but
> this function
> > (virtio_gpu_update_cursor_data) is relying on res->blob always being
> valid. That's why
> > the new condition !res->blob is needed here to check the validity of res-
> >blob.
> 
> I understand the host-side implementation difficulty to support this
> operation for VFIO, but the guest may not be prepared for the failure of
> the operation so we shouldn't simply reject it.
I think the worst case scenario here is Guest VM thinks its cursor is being
drawn using the image it provided but nothing gets drawn. I guess we need
to separately figure out if there are any alternate solutions to address this
issue (such as rendering the cursor on the Host side).

> 
> By the way, perhaps it may be possible to provide res->blob for VFIO.
> Since "[RFC 3/6] virtio-gpu-udmabuf: Create dmabuf for blobs associated
> with VFIO devices" checks memory_region_is_ram_device(), the VFIO
> backend providing the blob should support mmap().
The problem is that for dmabuf implementations in the kernel, providing
mmap() support is optional. And, the current vfio-pci implementation (that
provides dmabuf feature) does not support it as there was some pushback.
So, relying on res->blob always being valid is not going to work regardless.

Thanks,
Vivek

> 
> Regards,
> Akihiko Odaki

Re: [RFC 3/6] virtio-gpu-udmabuf: Create dmabuf for blobs associated with VFIO devices

[Alex Bennée]

Vivek Kasireddy <vivek.kasireddy@intel.com> writes:

> 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.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/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;
> +    }
> +
> +    QLIST_FOREACH(vdev, &vfio_device_list, next) {
> +        if (vdev->dev == mr->dev) {
> +            return vdev;
> +        }
> +    }
> +    return NULL;
> +}
> +

Also fails if VFIO isn't enabled:

  /usr/bin/ld: libsystem.a.p/hw_display_virtio-gpu-udmabuf.c.o: warning: relocation against `vfio_device_list' in read-only section `.text'
  /usr/bin/ld: libsystem.a.p/hw_display_virtio-gpu-udmabuf.c.o: in function `vfio_device_lookup':
  /home/alex/lsrc/qemu.git/builds/all/../../hw/display/virtio-gpu-udmabuf.c:45:(.text+0x313): undefined reference to `vfio_device_list'
  /usr/bin/ld: libsystem.a.p/hw_display_virtio-gpu-udmabuf.c.o: in function `vfio_create_dmabuf':
  /home/alex/lsrc/qemu.git/builds/all/../../hw/display/virtio-gpu-udmabuf.c:34:(.text+0x361): undefined reference to `vfio_device_create_dmabuf'
  /usr/bin/ld: warning: creating DT_TEXTREL in a PIE


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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [RFC 4/6] virtio-gpu: Don't rely on res->blob to identify blob resources

[Akihiko Odaki]

On 2025/09/16 3:06, Kasireddy, Vivek wrote:
>> Subject: Re: [RFC 4/6] virtio-gpu: Don't rely on res->blob to identify blob
>> resources
>>
>> On 2025/09/15 15:33, Kasireddy, Vivek wrote:
>>> Hi Akihiko,
>>>
>>>> Subject: Re: [RFC 4/6] virtio-gpu: Don't rely on res->blob to identify blob
>>>> resources
>>>>
>>>> On 2025/09/13 11:48, Kasireddy, Vivek wrote:
>>>>> Hi Akihiko,
>>>>>
>>>>>> Subject: Re: [RFC 4/6] virtio-gpu: Don't rely on res->blob to identify
>> blob
>>>>>> resources
>>>>>>
>>>>>> On 2025/09/03 7:42, Vivek Kasireddy wrote:
>>>>>>> The res->blob pointer is only valid for blobs that have their
>>>>>>> backing storage in memfd. Therefore, we cannot use it to determine
>>>>>>> if a resource is a blob or not. Instead, we could use res->blob_size
>>>>>>> to make this determination as it is non-zero for blob resources
>>>>>>> regardless of where their backing storage is located.
>>>>>>
>>>>>> I guess this change needs to be applied before "[RFC 3/6]
>>>>>> virtio-gpu-udmabuf: Create dmabuf for blobs associated with VFIO
>>>>>> devices"; without this patch, the "create dmabuf" patch will probably
>>>>>> create an invalid blob.
>>>>> Ok, makes sense. I'll move it earlier in the patch series.
>>>>>
>>>>>>
>>>>>>>
>>>>>>> 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/virtio-gpu.c | 19 +++++++++++--------
>>>>>>>      1 file changed, 11 insertions(+), 8 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
>>>>>>> index 0a1a625b0e..2f9133c3b6 100644
>>>>>>> --- a/hw/display/virtio-gpu.c
>>>>>>> +++ b/hw/display/virtio-gpu.c
>>>>>>> @@ -57,7 +57,7 @@ void virtio_gpu_update_cursor_data(VirtIOGPU
>> *g,
>>>>>>>          }
>>>>>>>
>>>>>>>          if (res->blob_size) {
>>>>>>> -        if (res->blob_size < (s->current_cursor->width *
>>>>>>> +        if (!res->blob || res->blob_size < (s->current_cursor->width *
>>>>>>
>>>>>> I doubt that rejecting a valid blob due to an implementation concern
>>>>>> (whether the backing storage is in memfd) is tolerated in the
>> specification.
>>>>> Are you suggesting that the whole if (res->blob_size < (s-
>>> current_cursor-
>>>>> width *...
>>>>> check needs to be removed? I think it is just a sanity check to ensure
>> that the
>>>> blob
>>>>> size is big enough for cursor.
>>>>
>>>> I referred to !res->blob, the new condition. It rejects a valid blob
>>>> that is backed by VFIO.
>>> The problem is that for blobs backed by VFIO, res->blob can be NULL but
>> this function
>>> (virtio_gpu_update_cursor_data) is relying on res->blob always being
>> valid. That's why
>>> the new condition !res->blob is needed here to check the validity of res-
>>> blob.
>>
>> I understand the host-side implementation difficulty to support this
>> operation for VFIO, but the guest may not be prepared for the failure of
>> the operation so we shouldn't simply reject it.
> I think the worst case scenario here is Guest VM thinks its cursor is being
> drawn using the image it provided but nothing gets drawn. I guess we need
> to separately figure out if there are any alternate solutions to address this
> issue (such as rendering the cursor on the Host side).
> 
>>
>> By the way, perhaps it may be possible to provide res->blob for VFIO.
>> Since "[RFC 3/6] virtio-gpu-udmabuf: Create dmabuf for blobs associated
>> with VFIO devices" checks memory_region_is_ram_device(), the VFIO
>> backend providing the blob should support mmap().
> The problem is that for dmabuf implementations in the kernel, providing
> mmap() support is optional. And, the current vfio-pci implementation (that
> provides dmabuf feature) does not support it as there was some pushback.

Can you provide a reference of a relevant discussion if any?

> So, relying on res->blob always being valid is not going to work regardless.
It is still possible to mmap() using the standard VFIO device API even 
if we cannot mmap() via DMA-BUF.

Regards,
Akihiko Odaki

RE: [RFC 4/6] virtio-gpu: Don't rely on res->blob to identify blob resources

[Kasireddy, Vivek]

Hi Akihiko,

> >>>>>> On 2025/09/03 7:42, Vivek Kasireddy wrote:
> >>>>>>> The res->blob pointer is only valid for blobs that have their
> >>>>>>> backing storage in memfd. Therefore, we cannot use it to
> determine
> >>>>>>> if a resource is a blob or not. Instead, we could use res->blob_size
> >>>>>>> to make this determination as it is non-zero for blob resources
> >>>>>>> regardless of where their backing storage is located.
> >>>>>>
> >>>>>> I guess this change needs to be applied before "[RFC 3/6]
> >>>>>> virtio-gpu-udmabuf: Create dmabuf for blobs associated with VFIO
> >>>>>> devices"; without this patch, the "create dmabuf" patch will
> probably
> >>>>>> create an invalid blob.
> >>>>> Ok, makes sense. I'll move it earlier in the patch series.
> >>>>>
> >>>>>>
> >>>>>>>
> >>>>>>> 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/virtio-gpu.c | 19 +++++++++++--------
> >>>>>>>      1 file changed, 11 insertions(+), 8 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> >>>>>>> index 0a1a625b0e..2f9133c3b6 100644
> >>>>>>> --- a/hw/display/virtio-gpu.c
> >>>>>>> +++ b/hw/display/virtio-gpu.c
> >>>>>>> @@ -57,7 +57,7 @@ void
> virtio_gpu_update_cursor_data(VirtIOGPU
> >> *g,
> >>>>>>>          }
> >>>>>>>
> >>>>>>>          if (res->blob_size) {
> >>>>>>> -        if (res->blob_size < (s->current_cursor->width *
> >>>>>>> +        if (!res->blob || res->blob_size < (s->current_cursor->width *
> >>>>>>
> >>>>>> I doubt that rejecting a valid blob due to an implementation
> concern
> >>>>>> (whether the backing storage is in memfd) is tolerated in the
> >> specification.
> >>>>> Are you suggesting that the whole if (res->blob_size < (s-
> >>> current_cursor-
> >>>>> width *...
> >>>>> check needs to be removed? I think it is just a sanity check to ensure
> >> that the
> >>>> blob
> >>>>> size is big enough for cursor.
> >>>>
> >>>> I referred to !res->blob, the new condition. It rejects a valid blob
> >>>> that is backed by VFIO.
> >>> The problem is that for blobs backed by VFIO, res->blob can be NULL
> but
> >> this function
> >>> (virtio_gpu_update_cursor_data) is relying on res->blob always being
> >> valid. That's why
> >>> the new condition !res->blob is needed here to check the validity of res-
> >>> blob.
> >>
> >> I understand the host-side implementation difficulty to support this
> >> operation for VFIO, but the guest may not be prepared for the failure of
> >> the operation so we shouldn't simply reject it.
> > I think the worst case scenario here is Guest VM thinks its cursor is being
> > drawn using the image it provided but nothing gets drawn. I guess we
> need
> > to separately figure out if there are any alternate solutions to address this
> > issue (such as rendering the cursor on the Host side).
> >
> >>
> >> By the way, perhaps it may be possible to provide res->blob for VFIO.
> >> Since "[RFC 3/6] virtio-gpu-udmabuf: Create dmabuf for blobs associated
> >> with VFIO devices" checks memory_region_is_ram_device(), the VFIO
> >> backend providing the blob should support mmap().
> > The problem is that for dmabuf implementations in the kernel, providing
> > mmap() support is optional. And, the current vfio-pci implementation
> (that
> > provides dmabuf feature) does not support it as there was some
> pushback.
> 
> Can you provide a reference of a relevant discussion if any?
I meant to say that it was deemed undesirable to add mmap support to
vfio-pci dmabuf implementation considering the Confidential computing
and other use-cases. Here are some references:
https://lore.kernel.org/dri-devel/20240501125309.GB941030@nvidia.com/
https://lore.kernel.org/dri-devel/Zjs2bVVxBHEGUhF_@phenom.ffwll.local/
https://lore.kernel.org/dri-devel/20250107142719.179636-5-yilun.xu@linux.intel.com/

> 
> > So, relying on res->blob always being valid is not going to work regardless.
> It is still possible to mmap() using the standard VFIO device API even
> if we cannot mmap() via DMA-BUF.
Ah, right. I had not considered this idea and it seems viable. I'll try to implement
it for the next version of this series.

Thanks,
Vivek

> 
> Regards,
> Akihiko Odaki