[RFC PATCH v9 0/5] Support mapping virtio-gpu virgl hostmem blobs using MAP_FIXED API

[RFC PATCH v9 0/5] Support mapping virtio-gpu virgl hostmem blobs using MAP_FIXED API

[Dmitry Osipenko]

Virglrender got a new API that allows mapping host blobs at a given
memory address using MAP_FIXED mmap flag [1]. Usage of this new API brings
major performance and stability improvement for venus and drm native contexts,
see commit message of the RFC patch for details.

Sending early to collect review feeback and have patch prepared by the
time new version of libvirglrenderer will be released with the stabilized
API.

[1] https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/1374

Based-on: 20251116125641.2255794-1-dmitry.osipenko@collabora.com

Changelog:

v9: - Added patch converting finish_unmapping to mapping_state that was
      suggested by Akihiko Odaki to address MR unmapping problem that may
      arise on resetting virtio-gpu.

    - Changed virtio_gpu_virgl_reset() to return true/false.
      Suggested by Akihiko Odaki.

    - Made virtio_gpu_virgl_reset() and virtio_gpu_virgl_init() static.

v8: - Added virtio_gpu_virgl_update_render_state() to virgl_resume_cmdq_bh(),
      making sure that GPU is reset once cmd execution is resumed after
      suspencion. Suggested by Akihiko Odaki.

v7: - Changed virtio_gpu_virgl_reset() to keep virtio-gpu in reset state
      when reset fails and renamed cmd_suspended -> suspended argument
      of resource_unref(), as was suggested by Akihiko Odaki.

v6: - Updated comment for virtio_gpu_virgl_reset(), removing part
      telling that reset at "runtime" is unexpected and removing
      excessive error message about a failed reset. Requested by
      Akihiko Odaki.

v5: - Switched to use error_setg_errno() for qemu_ram_mmap() error
      handling, as was suggested Akihiko Odaki.

    - Added r-b from Alex Bennée to the first patch.

    - Moved hostmem mapping offset validation to upper function as
      was suggested by Alex Bennée.

    - Dropped all patches and changes that made funcs to return -1
      since it was rejected by Alex Bennée. Refactoring can be done
      later on in a separate patchset.

    - Extended clarifying comment of virtio_gpu_virgl_reset().

v4: - Addressed v3 review comments from Akihiko Odaki.

    - Dropped patch making resource_unmap() error reported as a host
      failure instead of guest and added patch improving resource_map_blob()
      error reporting.

    - Re-added CONFIG_WIN32 checks.

    - Added clarifying comment to virtio_gpu_virgl_reset() RE unsupported
      context restoring.

v3: - Addressed v2 review comments from Akihiko Odaki.

    - Droped check for CONFIG_WIN32. My current understanding that
      MAP_FIXED is supported by Cygwin.

    - Added new patches resetting virgl resources, validating hostmem
      offset and improving error-handlings.

    - Added r-b from Akihiko Odaki to the frist patch and t-b from
      Yiwei Zhang to the map_fixed patch.

v2: - Addressed v1 review comments from Akihiko Odaki

    - Added patch that removes unnecessary memory_region_set_enabled(),
      suggested by Akihiko Odaki

Dmitry Osipenko (5):
  virtio-gpu: Remove superfluous memory_region_set_enabled()
  virtio-gpu: Validate hostmem mapping offset
  virtio-gpu: Replace finish_unmapping with mapping_state
  virtio-gpu: Destroy virgl resources on virtio-gpu reset
  virtio-gpu: Support mapping hostmem blobs with map_fixed

 hw/display/trace-events        |   2 +-
 hw/display/virtio-gpu-gl.c     |  58 ++++++---
 hw/display/virtio-gpu-virgl.c  | 216 ++++++++++++++++++++++++++++-----
 include/hw/virtio/virtio-gpu.h |   9 +-
 4 files changed, 235 insertions(+), 50 deletions(-)

-- 
2.52.0

[RFC PATCH v9 1/5] virtio-gpu: Remove superfluous memory_region_set_enabled()

[Dmitry Osipenko]

There is no need to explicitly enable/disable memory region when it's
added or deleted respectively. Remove superfluous set_enabled() calls
for consistency.

Suggested-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
Reviewed-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 hw/display/virtio-gpu-virgl.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 80f71c0b66aa..a6860f63b563 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -139,7 +139,6 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
     mr = &vmr->mr;
     memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size, data);
     memory_region_add_subregion(&b->hostmem, offset, mr);
-    memory_region_set_enabled(mr, true);
 
     /*
      * MR could outlive the resource if MR's reference is held outside of
@@ -201,7 +200,6 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
         b->renderer_blocked++;
 
         /* memory region owns self res->mr object and frees it by itself */
-        memory_region_set_enabled(mr, false);
         memory_region_del_subregion(&b->hostmem, mr);
         object_unparent(OBJECT(mr));
     }
-- 
2.52.0

[RFC PATCH v9 2/5] virtio-gpu: Validate hostmem mapping offset

[Dmitry Osipenko]

Check hostmem mapping boundaries originated from guest.

Suggested-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 hw/display/virtio-gpu-virgl.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index a6860f63b563..6a2aac0b6e5c 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -767,6 +767,7 @@ static void virgl_cmd_resource_map_blob(VirtIOGPU *g,
     struct virtio_gpu_resource_map_blob mblob;
     struct virtio_gpu_virgl_resource *res;
     struct virtio_gpu_resp_map_info resp;
+    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
     int ret;
 
     VIRTIO_GPU_FILL_CMD(mblob);
@@ -780,6 +781,15 @@ static void virgl_cmd_resource_map_blob(VirtIOGPU *g,
         return;
     }
 
+    if (mblob.offset + res->base.blob_size > b->conf.hostmem ||
+        mblob.offset + res->base.blob_size < mblob.offset) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: failed to map virgl resource: invalid offset\n",
+                      __func__);
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
+        return;
+    }
+
     ret = virtio_gpu_virgl_map_resource_blob(g, res, mblob.offset);
     if (ret) {
         cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
-- 
2.52.0

[RFC PATCH v9 3/5] virtio-gpu: Replace finish_unmapping with mapping_state

[Dmitry Osipenko]

Allow virtio_gpu_virgl_unmap_resource_blob() to be invoked while async
unmapping is in progress. Do it in preparation to improvement of virtio-gpu
resetting that will require this change.

Suggested-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 hw/display/trace-events       |  2 +-
 hw/display/virtio-gpu-virgl.c | 28 +++++++++++++++++++++++-----
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/hw/display/trace-events b/hw/display/trace-events
index e323a82cff24..4bfc457fbac1 100644
--- a/hw/display/trace-events
+++ b/hw/display/trace-events
@@ -39,7 +39,7 @@ virtio_gpu_cmd_res_create_2d(uint32_t res, uint32_t fmt, uint32_t w, uint32_t h)
 virtio_gpu_cmd_res_create_3d(uint32_t res, uint32_t fmt, uint32_t w, uint32_t h, uint32_t d) "res 0x%x, fmt 0x%x, w %d, h %d, d %d"
 virtio_gpu_cmd_res_create_blob(uint32_t res, uint64_t size) "res 0x%x, size %" PRId64
 virtio_gpu_cmd_res_map_blob(uint32_t res, void *vmr, void *mr) "res 0x%x, vmr %p, mr %p"
-virtio_gpu_cmd_res_unmap_blob(uint32_t res, void *mr, bool finish_unmapping) "res 0x%x, mr %p, finish_unmapping %d"
+virtio_gpu_cmd_res_unmap_blob(uint32_t res, void *mr, int mapping_state) "res 0x%x, mr %p, mapping_state %d"
 virtio_gpu_cmd_res_unref(uint32_t res) "res 0x%x"
 virtio_gpu_cmd_res_back_attach(uint32_t res) "res 0x%x"
 virtio_gpu_cmd_res_back_detach(uint32_t res) "res 0x%x"
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 6a2aac0b6e5c..342e93728df0 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -68,10 +68,16 @@ virgl_get_egl_display(G_GNUC_UNUSED void *cookie)
 #endif
 
 #if VIRGL_VERSION_MAJOR >= 1
+enum virtio_gpu_virgl_hostmem_region_mapping_state {
+    VIRTIO_GPU_MR_MAPPED,
+    VIRTIO_GPU_MR_UNMAP_STARTED,
+    VIRTIO_GPU_MR_UNMAP_COMPLETED,
+};
+
 struct virtio_gpu_virgl_hostmem_region {
     MemoryRegion mr;
     struct VirtIOGPU *g;
-    bool finish_unmapping;
+    enum virtio_gpu_virgl_hostmem_region_mapping_state mapping_state;
 };
 
 static struct virtio_gpu_virgl_hostmem_region *
@@ -95,7 +101,7 @@ static void virtio_gpu_virgl_hostmem_region_free(void *obj)
     VirtIOGPUGL *gl;
 
     vmr = to_hostmem_region(mr);
-    vmr->finish_unmapping = true;
+    vmr->mapping_state = VIRTIO_GPU_MR_UNMAP_COMPLETED;
 
     b = VIRTIO_GPU_BASE(vmr->g);
     b->renderer_blocked--;
@@ -135,6 +141,7 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
 
     vmr = g_new0(struct virtio_gpu_virgl_hostmem_region, 1);
     vmr->g = g;
+    vmr->mapping_state = VIRTIO_GPU_MR_MAPPED;
 
     mr = &vmr->mr;
     memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size, data);
@@ -171,7 +178,8 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
 
     vmr = to_hostmem_region(res->mr);
 
-    trace_virtio_gpu_cmd_res_unmap_blob(res->base.resource_id, mr, vmr->finish_unmapping);
+    trace_virtio_gpu_cmd_res_unmap_blob(res->base.resource_id, mr,
+                                        vmr->mapping_state);
 
     /*
      * Perform async unmapping in 3 steps:
@@ -182,7 +190,8 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
      *    asynchronously by virtio_gpu_virgl_hostmem_region_free().
      * 3. Finish the unmapping with final virgl_renderer_resource_unmap().
      */
-    if (vmr->finish_unmapping) {
+    switch (vmr->mapping_state) {
+    case VIRTIO_GPU_MR_UNMAP_COMPLETED:
         res->mr = NULL;
         g_free(vmr);
 
@@ -193,15 +202,24 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
                           __func__, strerror(-ret));
             return ret;
         }
-    } else {
+        break;
+
+    case VIRTIO_GPU_MR_MAPPED:
         *cmd_suspended = true;
 
         /* render will be unblocked once MR is freed */
         b->renderer_blocked++;
 
+        vmr->mapping_state = VIRTIO_GPU_MR_UNMAP_STARTED;
+
         /* memory region owns self res->mr object and frees it by itself */
         memory_region_del_subregion(&b->hostmem, mr);
         object_unparent(OBJECT(mr));
+        break;
+
+    case VIRTIO_GPU_MR_UNMAP_STARTED:
+        *cmd_suspended = true;
+        break;
     }
 
     return 0;
-- 
2.52.0

[RFC PATCH v9 4/5] virtio-gpu: Destroy virgl resources on virtio-gpu reset

[Dmitry Osipenko]

Properly destroy virgl resources on virtio-gpu reset to not leak resources
on a hot reboot of a VM.

Suggested-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 hw/display/virtio-gpu-gl.c     |  18 +----
 hw/display/virtio-gpu-virgl.c  | 117 ++++++++++++++++++++++++++-------
 include/hw/virtio/virtio-gpu.h |   6 +-
 3 files changed, 101 insertions(+), 40 deletions(-)

diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
index b941e9a4b789..8b71dd6fc26f 100644
--- a/hw/display/virtio-gpu-gl.c
+++ b/hw/display/virtio-gpu-gl.c
@@ -63,29 +63,14 @@ static void virtio_gpu_gl_flushed(VirtIOGPUBase *b)
 static void virtio_gpu_gl_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtIOGPU *g = VIRTIO_GPU(vdev);
-    VirtIOGPUGL *gl = VIRTIO_GPU_GL(vdev);
     struct virtio_gpu_ctrl_command *cmd;
 
     if (!virtio_queue_ready(vq)) {
         return;
     }
 
-    switch (gl->renderer_state) {
-    case RS_RESET:
-        virtio_gpu_virgl_reset(g);
-        /* fallthrough */
-    case RS_START:
-        if (virtio_gpu_virgl_init(g)) {
-            gl->renderer_state = RS_INIT_FAILED;
-            return;
-        }
-
-        gl->renderer_state = RS_INITED;
-        break;
-    case RS_INIT_FAILED:
+    if (!virtio_gpu_virgl_update_render_state(g)) {
         return;
-    case RS_INITED:
-        break;
     }
 
     cmd = virtqueue_pop(vq, sizeof(struct virtio_gpu_ctrl_command));
@@ -201,6 +186,7 @@ static void virtio_gpu_gl_class_init(ObjectClass *klass, const void *data)
     vgc->process_cmd = virtio_gpu_virgl_process_cmd;
     vgc->update_cursor_data = virtio_gpu_gl_update_cursor_data;
 
+    vgc->resource_destroy = virtio_gpu_virgl_resource_destroy;
     vdc->realize = virtio_gpu_gl_device_realize;
     vdc->unrealize = virtio_gpu_gl_device_unrealize;
     vdc->reset = virtio_gpu_gl_reset;
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 342e93728df0..15a98336969b 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -90,6 +90,10 @@ static void virtio_gpu_virgl_resume_cmdq_bh(void *opaque)
 {
     VirtIOGPU *g = opaque;
 
+    if (!virtio_gpu_virgl_update_render_state(g)) {
+        return;
+    }
+
     virtio_gpu_process_cmdq(g);
 }
 
@@ -322,14 +326,46 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
     virgl_renderer_resource_create(&args, NULL, 0);
 }
 
+static int
+virtio_gpu_virgl_resource_unref(VirtIOGPU *g,
+                                struct virtio_gpu_virgl_resource *res,
+                                bool *suspended)
+{
+    struct iovec *res_iovs = NULL;
+    int num_iovs = 0;
+#if VIRGL_VERSION_MAJOR >= 1
+    int ret;
+
+    ret = virtio_gpu_virgl_unmap_resource_blob(g, res, suspended);
+    if (ret) {
+        return ret;
+    }
+    if (*suspended) {
+        return 0;
+    }
+#endif
+
+    virgl_renderer_resource_detach_iov(res->base.resource_id,
+                                       &res_iovs,
+                                       &num_iovs);
+    if (res_iovs != NULL && num_iovs != 0) {
+        virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs);
+    }
+    virgl_renderer_resource_unref(res->base.resource_id);
+
+    QTAILQ_REMOVE(&g->reslist, &res->base, next);
+
+    g_free(res);
+
+    return 0;
+}
+
 static void virgl_cmd_resource_unref(VirtIOGPU *g,
                                      struct virtio_gpu_ctrl_command *cmd,
                                      bool *cmd_suspended)
 {
     struct virtio_gpu_resource_unref unref;
     struct virtio_gpu_virgl_resource *res;
-    struct iovec *res_iovs = NULL;
-    int num_iovs = 0;
 
     VIRTIO_GPU_FILL_CMD(unref);
     trace_virtio_gpu_cmd_res_unref(unref.resource_id);
@@ -342,27 +378,21 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
         return;
     }
 
-#if VIRGL_VERSION_MAJOR >= 1
-    if (virtio_gpu_virgl_unmap_resource_blob(g, res, cmd_suspended)) {
-        cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
-        return;
-    }
-    if (*cmd_suspended) {
-        return;
-    }
-#endif
+    virtio_gpu_virgl_resource_unref(g, res, cmd_suspended);
+}
 
-    virgl_renderer_resource_detach_iov(unref.resource_id,
-                                       &res_iovs,
-                                       &num_iovs);
-    if (res_iovs != NULL && num_iovs != 0) {
-        virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs);
-    }
-    virgl_renderer_resource_unref(unref.resource_id);
+void virtio_gpu_virgl_resource_destroy(VirtIOGPU *g,
+                                       struct virtio_gpu_simple_resource *base,
+                                       Error **errp)
+{
+    struct virtio_gpu_virgl_resource *res;
+    bool suspended = false;
 
-    QTAILQ_REMOVE(&g->reslist, &res->base, next);
+    res = container_of(base, struct virtio_gpu_virgl_resource, base);
 
-    g_free(res);
+    if (virtio_gpu_virgl_resource_unref(g, res, &suspended)) {
+        error_setg(errp, "failed to destroy virgl resource");
+    }
 }
 
 static void virgl_cmd_context_create(VirtIOGPU *g,
@@ -1291,14 +1321,30 @@ void virtio_gpu_virgl_reset_scanout(VirtIOGPU *g)
     }
 }
 
-void virtio_gpu_virgl_reset(VirtIOGPU *g)
+static bool virtio_gpu_virgl_reset(VirtIOGPU *g)
 {
+    struct virtio_gpu_simple_resource *res, *tmp;
+
+    /*
+     * Virgl blob resource unmapping can be suspended and
+     * deferred on unref, ensure that destruction is completed.
+     */
+    QTAILQ_FOREACH_SAFE(res, &g->reslist, next, tmp) {
+        virtio_gpu_virgl_resource_destroy(g, res, NULL);
+    }
+
+    if (!QTAILQ_EMPTY(&g->reslist)) {
+        return false;
+    }
+
     virgl_renderer_reset();
 
     virtio_gpu_virgl_reset_async_fences(g);
+
+    return true;
 }
 
-int virtio_gpu_virgl_init(VirtIOGPU *g)
+static int virtio_gpu_virgl_init(VirtIOGPU *g)
 {
     int ret;
     uint32_t flags = 0;
@@ -1376,6 +1422,33 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
     return 0;
 }
 
+bool virtio_gpu_virgl_update_render_state(VirtIOGPU *g)
+{
+    VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
+
+    switch (gl->renderer_state) {
+    case RS_RESET:
+        if (!virtio_gpu_virgl_reset(g)) {
+            return false;
+        }
+        /* fallthrough */
+    case RS_START:
+        if (virtio_gpu_virgl_init(g)) {
+            gl->renderer_state = RS_INIT_FAILED;
+            return false;
+        }
+
+        gl->renderer_state = RS_INITED;
+        break;
+    case RS_INIT_FAILED:
+        return false;
+    case RS_INITED:
+        break;
+    }
+
+    return true;
+}
+
 static void virtio_gpu_virgl_add_capset(GArray *capset_ids, uint32_t capset_id)
 {
     g_array_append_val(capset_ids, capset_id);
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 718332284305..1f509d0d5beb 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -389,9 +389,11 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
                                   struct virtio_gpu_ctrl_command *cmd);
 void virtio_gpu_virgl_fence_poll(VirtIOGPU *g);
 void virtio_gpu_virgl_reset_scanout(VirtIOGPU *g);
-void virtio_gpu_virgl_reset(VirtIOGPU *g);
-int virtio_gpu_virgl_init(VirtIOGPU *g);
 GArray *virtio_gpu_virgl_get_capsets(VirtIOGPU *g);
 void virtio_gpu_virgl_reset_async_fences(VirtIOGPU *g);
+void virtio_gpu_virgl_resource_destroy(VirtIOGPU *g,
+                                       struct virtio_gpu_simple_resource *res,
+                                       Error **errp);
+bool virtio_gpu_virgl_update_render_state(VirtIOGPU *g);
 
 #endif
-- 
2.52.0

[RFC PATCH v9 5/5] virtio-gpu: Support mapping hostmem blobs with map_fixed

[Dmitry Osipenko]

Support mapping virgl blobs to a fixed location of a hostmem memory
region using new virglrenderer MAP_FIXED API.

This new feature closes multiple problems for virtio-gpu on QEMU:

- Having dedicated memory region for each mapped blob works notoriously
slow due to QEMU's memory region software design built around RCU that
isn't optimized for frequent removal of the regions

- KVM isn't optimized for a frequent slot changes too

- QEMU/KVM has a limit for a total number of created memory regions,
crashing QEMU when limit is reached

This patch makes virtio-gpu-gl to pre-create a single anonymous memory
region covering whole hostmem area to which blobs will be mapped using
the MAP_FIXED API.

Not all virgl resources will support mapping at a fixed memory address. For
them, we will continue to create individual nested memory sub-regions. In
particular, vrend resources may not have MAP_FIXED capability.

Venus and DRM native contexts will largely benefit from the MAP_FIXED
feature in terms of performance and stability improvement.

Tested-by: Yiwei Zhang <zzyiwei@gmail.com>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 hw/display/virtio-gpu-gl.c     | 40 ++++++++++++++++++++++-
 hw/display/virtio-gpu-virgl.c  | 59 +++++++++++++++++++++++++++++++++-
 include/hw/virtio/virtio-gpu.h |  3 ++
 3 files changed, 100 insertions(+), 2 deletions(-)

diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
index 8b71dd6fc26f..add6af73e980 100644
--- a/hw/display/virtio-gpu-gl.c
+++ b/hw/display/virtio-gpu-gl.c
@@ -13,6 +13,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu/iov.h"
+#include "qemu/mmap-alloc.h"
 #include "qemu/module.h"
 #include "qemu/error-report.h"
 #include "qapi/error.h"
@@ -106,7 +107,12 @@ static void virtio_gpu_gl_reset(VirtIODevice *vdev)
 static void virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
 {
     ERRP_GUARD();
-    VirtIOGPU *g = VIRTIO_GPU(qdev);
+    VirtIOGPUBase *b = VIRTIO_GPU_BASE(qdev);
+    VirtIOGPU *g = VIRTIO_GPU(b);
+#if !defined(CONFIG_WIN32)
+    VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
+    void *map;
+#endif
 
 #if HOST_BIG_ENDIAN
     error_setg(errp, "virgl is not supported on bigendian platforms");
@@ -137,6 +143,27 @@ static void virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
 #endif
 
     virtio_gpu_device_realize(qdev, errp);
+    if (*errp) {
+        return;
+    }
+
+#if !defined(CONFIG_WIN32)
+    if (virtio_gpu_hostmem_enabled(b->conf)) {
+        map = qemu_ram_mmap(-1, b->conf.hostmem, qemu_real_host_page_size(),
+                            0, 0);
+        if (map == MAP_FAILED) {
+            error_setg_errno(errp, errno,
+                             "virgl hostmem region could not be initialized");
+            return;
+        }
+
+        gl->hostmem_mmap = map;
+        memory_region_init_ram_ptr(&gl->hostmem_background, NULL,
+                                   "hostmem-background", b->conf.hostmem,
+                                   gl->hostmem_mmap);
+        memory_region_add_subregion(&b->hostmem, 0, &gl->hostmem_background);
+    }
+#endif
 }
 
 static const Property virtio_gpu_gl_properties[] = {
@@ -172,6 +199,17 @@ static void virtio_gpu_gl_device_unrealize(DeviceState *qdev)
     gl->renderer_state = RS_START;
 
     g_array_unref(g->capset_ids);
+
+    /*
+     * It is not guaranteed that the memory region will be finalized
+     * immediately with memory_region_del_subregion(), there can be
+     * a remaining reference to gl->hostmem_mmap. VirtIO-GPU is not
+     * hotpluggable, hence no need to worry about the leaked mapping.
+     *
+     * The memory_region_del_subregion(gl->hostmem_background) is unnecessary
+     * because b->hostmem  and gl->hostmem_background belong to the same
+     * device and will be gone at the same time.
+     */
 }
 
 static void virtio_gpu_gl_class_init(ObjectClass *klass, const void *data)
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 15a98336969b..4d0def7c1dca 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -41,9 +41,13 @@
      VIRGL_VERSION_MICRO >= (micro))
 #endif
 
+#define VIRGL_HAS_MAP_FIXED \
+    (VIRGL_CHECK_VERSION(1, 2, 1) && !IS_ENABLED(CONFIG_WIN32))
+
 struct virtio_gpu_virgl_resource {
     struct virtio_gpu_simple_resource base;
     MemoryRegion *mr;
+    void *map_fixed;
 };
 
 static struct virtio_gpu_virgl_resource *
@@ -126,6 +130,9 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
 {
     struct virtio_gpu_virgl_hostmem_region *vmr;
     VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
+#if VIRGL_HAS_MAP_FIXED
+    VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
+#endif
     MemoryRegion *mr;
     uint64_t size;
     void *data;
@@ -136,6 +143,41 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
         return -EOPNOTSUPP;
     }
 
+#if VIRGL_HAS_MAP_FIXED
+    /*
+     * virgl_renderer_resource_map_fixed() allows to create multiple
+     * mappings of the same resource, while virgl_renderer_resource_map()
+     * not. Don't allow mapping same resource twice.
+     */
+    if (res->map_fixed || res->mr) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: failed to map(fixed) virgl resource: already mapped\n",
+                      __func__);
+        return -EBUSY;
+    }
+
+    ret = virgl_renderer_resource_map_fixed(res->base.resource_id,
+                                            gl->hostmem_mmap + offset);
+    switch (ret) {
+    case 0:
+        res->map_fixed = gl->hostmem_mmap + offset;
+        return 0;
+
+    case -EOPNOTSUPP:
+        /*
+         * MAP_FIXED is unsupported by this resource.
+         * Mapping falls back to a blob subregion method in that case.
+         */
+        break;
+
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: failed to map(fixed) virgl resource: %s\n",
+                      __func__, strerror(-ret));
+        return ret;
+    }
+#endif
+
     ret = virgl_renderer_resource_map(res->base.resource_id, &data, &size);
     if (ret) {
         qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to map virgl resource: %s\n",
@@ -149,7 +191,7 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
 
     mr = &vmr->mr;
     memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size, data);
-    memory_region_add_subregion(&b->hostmem, offset, mr);
+    memory_region_add_subregion_overlap(&b->hostmem, offset, mr, 1);
 
     /*
      * MR could outlive the resource if MR's reference is held outside of
@@ -176,6 +218,21 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
     MemoryRegion *mr = res->mr;
     int ret;
 
+#if VIRGL_HAS_MAP_FIXED
+    if (res->map_fixed) {
+        if (mmap(res->map_fixed, res->base.blob_size, PROT_READ | PROT_WRITE,
+                 MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED,
+                 -1, 0) == MAP_FAILED) {
+            ret = -errno;
+            error_report("%s: failed to unmap(fixed) virgl resource: %s",
+                          __func__, strerror(-ret));
+            return ret;
+        }
+
+        res->map_fixed = NULL;
+    }
+#endif
+
     if (!mr) {
         return 0;
     }
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 1f509d0d5beb..a48b50d4c825 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -263,6 +263,9 @@ struct VirtIOGPUGL {
 
     QEMUBH *async_fence_bh;
     QSLIST_HEAD(, virtio_gpu_virgl_context_fence) async_fenceq;
+
+    MemoryRegion hostmem_background;
+    void *hostmem_mmap;
 };
 
 struct VhostUserGPU {
-- 
2.52.0

Re: [RFC PATCH v9 4/5] virtio-gpu: Destroy virgl resources on virtio-gpu reset

[Akihiko Odaki]

On 2026/01/13 7:52, Dmitry Osipenko wrote:
> Properly destroy virgl resources on virtio-gpu reset to not leak resources
> on a hot reboot of a VM.
> 
> Suggested-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---
>   hw/display/virtio-gpu-gl.c     |  18 +----
>   hw/display/virtio-gpu-virgl.c  | 117 ++++++++++++++++++++++++++-------
>   include/hw/virtio/virtio-gpu.h |   6 +-
>   3 files changed, 101 insertions(+), 40 deletions(-)
> 
> diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
> index b941e9a4b789..8b71dd6fc26f 100644
> --- a/hw/display/virtio-gpu-gl.c
> +++ b/hw/display/virtio-gpu-gl.c
> @@ -63,29 +63,14 @@ static void virtio_gpu_gl_flushed(VirtIOGPUBase *b)
>   static void virtio_gpu_gl_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
>   {
>       VirtIOGPU *g = VIRTIO_GPU(vdev);
> -    VirtIOGPUGL *gl = VIRTIO_GPU_GL(vdev);
>       struct virtio_gpu_ctrl_command *cmd;
>   
>       if (!virtio_queue_ready(vq)) {
>           return;
>       }
>   
> -    switch (gl->renderer_state) {
> -    case RS_RESET:
> -        virtio_gpu_virgl_reset(g);
> -        /* fallthrough */
> -    case RS_START:
> -        if (virtio_gpu_virgl_init(g)) {
> -            gl->renderer_state = RS_INIT_FAILED;
> -            return;
> -        }
> -
> -        gl->renderer_state = RS_INITED;
> -        break;
> -    case RS_INIT_FAILED:
> +    if (!virtio_gpu_virgl_update_render_state(g)) {
>           return;
> -    case RS_INITED:
> -        break;
>       }
>   
>       cmd = virtqueue_pop(vq, sizeof(struct virtio_gpu_ctrl_command));
> @@ -201,6 +186,7 @@ static void virtio_gpu_gl_class_init(ObjectClass *klass, const void *data)
>       vgc->process_cmd = virtio_gpu_virgl_process_cmd;
>       vgc->update_cursor_data = virtio_gpu_gl_update_cursor_data;
>   
> +    vgc->resource_destroy = virtio_gpu_virgl_resource_destroy;
>       vdc->realize = virtio_gpu_gl_device_realize;
>       vdc->unrealize = virtio_gpu_gl_device_unrealize;
>       vdc->reset = virtio_gpu_gl_reset;
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index 342e93728df0..15a98336969b 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -90,6 +90,10 @@ static void virtio_gpu_virgl_resume_cmdq_bh(void *opaque)
>   {
>       VirtIOGPU *g = opaque;
>   
> +    if (!virtio_gpu_virgl_update_render_state(g)) {
> +        return;
> +    }
> +
>       virtio_gpu_process_cmdq(g);
>   }
>   
> @@ -322,14 +326,46 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
>       virgl_renderer_resource_create(&args, NULL, 0);
>   }
>   
> +static int
> +virtio_gpu_virgl_resource_unref(VirtIOGPU *g,
> +                                struct virtio_gpu_virgl_resource *res,
> +                                bool *suspended)
> +{
> +    struct iovec *res_iovs = NULL;
> +    int num_iovs = 0;
> +#if VIRGL_VERSION_MAJOR >= 1
> +    int ret;
> +
> +    ret = virtio_gpu_virgl_unmap_resource_blob(g, res, suspended);
> +    if (ret) {
> +        return ret;
> +    }
> +    if (*suspended) {
> +        return 0;
> +    }
> +#endif
> +
> +    virgl_renderer_resource_detach_iov(res->base.resource_id,
> +                                       &res_iovs,
> +                                       &num_iovs);
> +    if (res_iovs != NULL && num_iovs != 0) {
> +        virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs);
> +    }
> +    virgl_renderer_resource_unref(res->base.resource_id);
> +
> +    QTAILQ_REMOVE(&g->reslist, &res->base, next);
> +
> +    g_free(res);
> +
> +    return 0;
> +}
> +
>   static void virgl_cmd_resource_unref(VirtIOGPU *g,
>                                        struct virtio_gpu_ctrl_command *cmd,
>                                        bool *cmd_suspended)
>   {
>       struct virtio_gpu_resource_unref unref;
>       struct virtio_gpu_virgl_resource *res;
> -    struct iovec *res_iovs = NULL;
> -    int num_iovs = 0;
>   
>       VIRTIO_GPU_FILL_CMD(unref);
>       trace_virtio_gpu_cmd_res_unref(unref.resource_id);
> @@ -342,27 +378,21 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
>           return;
>       }
>   
> -#if VIRGL_VERSION_MAJOR >= 1
> -    if (virtio_gpu_virgl_unmap_resource_blob(g, res, cmd_suspended)) {
> -        cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
> -        return;
> -    }
> -    if (*cmd_suspended) {
> -        return;
> -    }
> -#endif
> +    virtio_gpu_virgl_resource_unref(g, res, cmd_suspended);
> +}
>   
> -    virgl_renderer_resource_detach_iov(unref.resource_id,
> -                                       &res_iovs,
> -                                       &num_iovs);
> -    if (res_iovs != NULL && num_iovs != 0) {
> -        virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs);
> -    }
> -    virgl_renderer_resource_unref(unref.resource_id);
> +void virtio_gpu_virgl_resource_destroy(VirtIOGPU *g,
> +                                       struct virtio_gpu_simple_resource *base,
> +                                       Error **errp)

An error reporting rule described in include/qapi/error.h requires 
functions that use Error to return a value indicating success or failure.

For this particular case, I think it can simply return what 
virtio_gpu_virgl_resource_unref() returns, without having errp.

> +{
> +    struct virtio_gpu_virgl_resource *res;
> +    bool suspended = false;
>   
> -    QTAILQ_REMOVE(&g->reslist, &res->base, next);
> +    res = container_of(base, struct virtio_gpu_virgl_resource, base);
>   
> -    g_free(res);
> +    if (virtio_gpu_virgl_resource_unref(g, res, &suspended)) {
> +        error_setg(errp, "failed to destroy virgl resource");
> +    }
>   }
>   
>   static void virgl_cmd_context_create(VirtIOGPU *g,
> @@ -1291,14 +1321,30 @@ void virtio_gpu_virgl_reset_scanout(VirtIOGPU *g)
>       }
>   }
>   
> -void virtio_gpu_virgl_reset(VirtIOGPU *g)
> +static bool virtio_gpu_virgl_reset(VirtIOGPU *g)
>   {
> +    struct virtio_gpu_simple_resource *res, *tmp;
> +
> +    /*
> +     * Virgl blob resource unmapping can be suspended and
> +     * deferred on unref, ensure that destruction is completed.
> +     */
> +    QTAILQ_FOREACH_SAFE(res, &g->reslist, next, tmp) {
> +        virtio_gpu_virgl_resource_destroy(g, res, NULL);
> +    }
> +
> +    if (!QTAILQ_EMPTY(&g->reslist)) {
> +        return false;
> +    }
> +
>       virgl_renderer_reset();
>   
>       virtio_gpu_virgl_reset_async_fences(g);
> +
> +    return true;
>   }
>   
> -int virtio_gpu_virgl_init(VirtIOGPU *g)
> +static int virtio_gpu_virgl_init(VirtIOGPU *g)
>   {
>       int ret;
>       uint32_t flags = 0;
> @@ -1376,6 +1422,33 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
>       return 0;
>   }
>   
> +bool virtio_gpu_virgl_update_render_state(VirtIOGPU *g)
> +{
> +    VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
> +
> +    switch (gl->renderer_state) {
> +    case RS_RESET:
> +        if (!virtio_gpu_virgl_reset(g)) {
> +            return false;
> +        }
> +        /* fallthrough */
> +    case RS_START:
> +        if (virtio_gpu_virgl_init(g)) {
> +            gl->renderer_state = RS_INIT_FAILED;
> +            return false;
> +        }
> +
> +        gl->renderer_state = RS_INITED;
> +        break;
> +    case RS_INIT_FAILED:
> +        return false;
> +    case RS_INITED:
> +        break;
> +    }
> +
> +    return true;
> +}
> +
>   static void virtio_gpu_virgl_add_capset(GArray *capset_ids, uint32_t capset_id)
>   {
>       g_array_append_val(capset_ids, capset_id);
> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> index 718332284305..1f509d0d5beb 100644
> --- a/include/hw/virtio/virtio-gpu.h
> +++ b/include/hw/virtio/virtio-gpu.h
> @@ -389,9 +389,11 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>                                     struct virtio_gpu_ctrl_command *cmd);
>   void virtio_gpu_virgl_fence_poll(VirtIOGPU *g);
>   void virtio_gpu_virgl_reset_scanout(VirtIOGPU *g);
> -void virtio_gpu_virgl_reset(VirtIOGPU *g);
> -int virtio_gpu_virgl_init(VirtIOGPU *g);
>   GArray *virtio_gpu_virgl_get_capsets(VirtIOGPU *g);
>   void virtio_gpu_virgl_reset_async_fences(VirtIOGPU *g);
> +void virtio_gpu_virgl_resource_destroy(VirtIOGPU *g,
> +                                       struct virtio_gpu_simple_resource *res,
> +                                       Error **errp);
> +bool virtio_gpu_virgl_update_render_state(VirtIOGPU *g);
>   
>   #endif

Re: [RFC PATCH v9 3/5] virtio-gpu: Replace finish_unmapping with mapping_state

[Akihiko Odaki]

On 2026/01/13 7:52, Dmitry Osipenko wrote:
> Allow virtio_gpu_virgl_unmap_resource_blob() to be invoked while async
> unmapping is in progress. Do it in preparation to improvement of virtio-gpu
> resetting that will require this change.
> 
> Suggested-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---
>   hw/display/trace-events       |  2 +-
>   hw/display/virtio-gpu-virgl.c | 28 +++++++++++++++++++++++-----
>   2 files changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/display/trace-events b/hw/display/trace-events
> index e323a82cff24..4bfc457fbac1 100644
> --- a/hw/display/trace-events
> +++ b/hw/display/trace-events
> @@ -39,7 +39,7 @@ virtio_gpu_cmd_res_create_2d(uint32_t res, uint32_t fmt, uint32_t w, uint32_t h)
>   virtio_gpu_cmd_res_create_3d(uint32_t res, uint32_t fmt, uint32_t w, uint32_t h, uint32_t d) "res 0x%x, fmt 0x%x, w %d, h %d, d %d"
>   virtio_gpu_cmd_res_create_blob(uint32_t res, uint64_t size) "res 0x%x, size %" PRId64
>   virtio_gpu_cmd_res_map_blob(uint32_t res, void *vmr, void *mr) "res 0x%x, vmr %p, mr %p"
> -virtio_gpu_cmd_res_unmap_blob(uint32_t res, void *mr, bool finish_unmapping) "res 0x%x, mr %p, finish_unmapping %d"
> +virtio_gpu_cmd_res_unmap_blob(uint32_t res, void *mr, int mapping_state) "res 0x%x, mr %p, mapping_state %d"
>   virtio_gpu_cmd_res_unref(uint32_t res) "res 0x%x"
>   virtio_gpu_cmd_res_back_attach(uint32_t res) "res 0x%x"
>   virtio_gpu_cmd_res_back_detach(uint32_t res) "res 0x%x"
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index 6a2aac0b6e5c..342e93728df0 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -68,10 +68,16 @@ virgl_get_egl_display(G_GNUC_UNUSED void *cookie)
>   #endif
>   
>   #if VIRGL_VERSION_MAJOR >= 1
> +enum virtio_gpu_virgl_hostmem_region_mapping_state {
> +    VIRTIO_GPU_MR_MAPPED,
> +    VIRTIO_GPU_MR_UNMAP_STARTED,
> +    VIRTIO_GPU_MR_UNMAP_COMPLETED,
> +};
> +
>   struct virtio_gpu_virgl_hostmem_region {
>       MemoryRegion mr;
>       struct VirtIOGPU *g;
> -    bool finish_unmapping;
> +    enum virtio_gpu_virgl_hostmem_region_mapping_state mapping_state;
>   };
>   
>   static struct virtio_gpu_virgl_hostmem_region *
> @@ -95,7 +101,7 @@ static void virtio_gpu_virgl_hostmem_region_free(void *obj)
>       VirtIOGPUGL *gl;
>   
>       vmr = to_hostmem_region(mr);
> -    vmr->finish_unmapping = true;
> +    vmr->mapping_state = VIRTIO_GPU_MR_UNMAP_COMPLETED;
>   
>       b = VIRTIO_GPU_BASE(vmr->g);
>       b->renderer_blocked--;
> @@ -135,6 +141,7 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
>   
>       vmr = g_new0(struct virtio_gpu_virgl_hostmem_region, 1);
>       vmr->g = g;
> +    vmr->mapping_state = VIRTIO_GPU_MR_MAPPED;
>   
>       mr = &vmr->mr;
>       memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size, data);
> @@ -171,7 +178,8 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
>   
>       vmr = to_hostmem_region(res->mr);
>   
> -    trace_virtio_gpu_cmd_res_unmap_blob(res->base.resource_id, mr, vmr->finish_unmapping);
> +    trace_virtio_gpu_cmd_res_unmap_blob(res->base.resource_id, mr,
> +                                        vmr->mapping_state);
>   
>       /*
>        * Perform async unmapping in 3 steps:
> @@ -182,7 +190,8 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
>        *    asynchronously by virtio_gpu_virgl_hostmem_region_free().
>        * 3. Finish the unmapping with final virgl_renderer_resource_unmap().
>        */
> -    if (vmr->finish_unmapping) {
> +    switch (vmr->mapping_state) {
> +    case VIRTIO_GPU_MR_UNMAP_COMPLETED:
>           res->mr = NULL;
>           g_free(vmr);
>   
> @@ -193,15 +202,24 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
>                             __func__, strerror(-ret));
>               return ret;
>           }
> -    } else {
> +        break;
> +
> +    case VIRTIO_GPU_MR_MAPPED:
>           *cmd_suspended = true;
>   
>           /* render will be unblocked once MR is freed */
>           b->renderer_blocked++;
>   
> +        vmr->mapping_state = VIRTIO_GPU_MR_UNMAP_STARTED;
> +
>           /* memory region owns self res->mr object and frees it by itself */
>           memory_region_del_subregion(&b->hostmem, mr);
>           object_unparent(OBJECT(mr));
> +        break;

I suggest:

- Put vmr->mapping_state = VIRTIO_GPU_MR_UNMAP_STARTED; here
- Remove *cmd_suspended = true for VIRTIO_GPU_MR_MAPPED.
- Let it fall through.

This way, it is clear that we need to execute *cmd_suspended = true 
because the state is now VIRTIO_GPU_MR_UNMAP_STARTED, and we can save on 
line by not having a duplicate *cmd_suspended = true.

> +
> +    case VIRTIO_GPU_MR_UNMAP_STARTED:
> +        *cmd_suspended = true;
> +        break;
>       }
>   
>       return 0;

Re: [RFC PATCH v9 4/5] virtio-gpu: Destroy virgl resources on virtio-gpu reset

[Dmitry Osipenko]

On 1/13/26 07:47, Akihiko Odaki wrote:
>> +void virtio_gpu_virgl_resource_destroy(VirtIOGPU *g,
>> +                                       struct
>> virtio_gpu_simple_resource *base,
>> +                                       Error **errp)
> 
> An error reporting rule described in include/qapi/error.h requires
> functions that use Error to return a value indicating success or failure.
> 
> For this particular case, I think it can simply return what
> virtio_gpu_virgl_resource_unref() returns, without having errp.

The error reporting arg is added here because resource_destroy()
callback of VirtIOGPUClass requires it:

```
vgc->resource_destroy = virtio_gpu_virgl_resource_destroy;
```

The include/qapi/error.h says "Whenever practical, also return a value
that indicates success / failure". Hence, the returned value is optional.

I don't quite see how errp can be avoided.

Perhaps you're meaning to add virtio_gpu_gl_resource_destroy() that will
be local to virtio-gpu-gl.c. Will change this in v10, otherwise please
clarify more your suggestion.

Thanks for the review.

-- 
Best regards,
Dmitry

Re: [RFC PATCH v9 4/5] virtio-gpu: Destroy virgl resources on virtio-gpu reset

[Akihiko Odaki]

On 2026/01/16 22:08, Dmitry Osipenko wrote:
> On 1/13/26 07:47, Akihiko Odaki wrote:
>>> +void virtio_gpu_virgl_resource_destroy(VirtIOGPU *g,
>>> +                                       struct
>>> virtio_gpu_simple_resource *base,
>>> +                                       Error **errp)
>>
>> An error reporting rule described in include/qapi/error.h requires
>> functions that use Error to return a value indicating success or failure.
>>
>> For this particular case, I think it can simply return what
>> virtio_gpu_virgl_resource_unref() returns, without having errp.
> 
> The error reporting arg is added here because resource_destroy()
> callback of VirtIOGPUClass requires it:
> 
> ```
> vgc->resource_destroy = virtio_gpu_virgl_resource_destroy;
> ```

I missed that line. That is my fault.

> 
> The include/qapi/error.h says "Whenever practical, also return a value
> that indicates success / failure". Hence, the returned value is optional.
> 
> I don't quite see how errp can be avoided.
> 
> Perhaps you're meaning to add virtio_gpu_gl_resource_destroy() that will
> be local to virtio-gpu-gl.c. Will change this in v10, otherwise please
> clarify more your suggestion.

Let's keep this way. The error handling infrastructure intentionally 
allows to specify NULL when you don't care the error, so we don't need 
another function to dismiss the error.

Re: [RFC PATCH v9 3/5] virtio-gpu: Replace finish_unmapping with mapping_state

[Dmitry Osipenko]

On 1/13/26 07:51, Akihiko Odaki wrote:
> On 2026/01/13 7:52, Dmitry Osipenko wrote:
>> Allow virtio_gpu_virgl_unmap_resource_blob() to be invoked while async
>> unmapping is in progress. Do it in preparation to improvement of
>> virtio-gpu
>> resetting that will require this change.
>>
>> Suggested-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>> ---
>>   hw/display/trace-events       |  2 +-
>>   hw/display/virtio-gpu-virgl.c | 28 +++++++++++++++++++++++-----
>>   2 files changed, 24 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/display/trace-events b/hw/display/trace-events
>> index e323a82cff24..4bfc457fbac1 100644
>> --- a/hw/display/trace-events
>> +++ b/hw/display/trace-events
>> @@ -39,7 +39,7 @@ virtio_gpu_cmd_res_create_2d(uint32_t res, uint32_t
>> fmt, uint32_t w, uint32_t h)
>>   virtio_gpu_cmd_res_create_3d(uint32_t res, uint32_t fmt, uint32_t w,
>> uint32_t h, uint32_t d) "res 0x%x, fmt 0x%x, w %d, h %d, d %d"
>>   virtio_gpu_cmd_res_create_blob(uint32_t res, uint64_t size) "res
>> 0x%x, size %" PRId64
>>   virtio_gpu_cmd_res_map_blob(uint32_t res, void *vmr, void *mr) "res
>> 0x%x, vmr %p, mr %p"
>> -virtio_gpu_cmd_res_unmap_blob(uint32_t res, void *mr, bool
>> finish_unmapping) "res 0x%x, mr %p, finish_unmapping %d"
>> +virtio_gpu_cmd_res_unmap_blob(uint32_t res, void *mr, int
>> mapping_state) "res 0x%x, mr %p, mapping_state %d"
>>   virtio_gpu_cmd_res_unref(uint32_t res) "res 0x%x"
>>   virtio_gpu_cmd_res_back_attach(uint32_t res) "res 0x%x"
>>   virtio_gpu_cmd_res_back_detach(uint32_t res) "res 0x%x"
>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-
>> virgl.c
>> index 6a2aac0b6e5c..342e93728df0 100644
>> --- a/hw/display/virtio-gpu-virgl.c
>> +++ b/hw/display/virtio-gpu-virgl.c
>> @@ -68,10 +68,16 @@ virgl_get_egl_display(G_GNUC_UNUSED void *cookie)
>>   #endif
>>     #if VIRGL_VERSION_MAJOR >= 1
>> +enum virtio_gpu_virgl_hostmem_region_mapping_state {
>> +    VIRTIO_GPU_MR_MAPPED,
>> +    VIRTIO_GPU_MR_UNMAP_STARTED,
>> +    VIRTIO_GPU_MR_UNMAP_COMPLETED,
>> +};
>> +
>>   struct virtio_gpu_virgl_hostmem_region {
>>       MemoryRegion mr;
>>       struct VirtIOGPU *g;
>> -    bool finish_unmapping;
>> +    enum virtio_gpu_virgl_hostmem_region_mapping_state mapping_state;
>>   };
>>     static struct virtio_gpu_virgl_hostmem_region *
>> @@ -95,7 +101,7 @@ static void
>> virtio_gpu_virgl_hostmem_region_free(void *obj)
>>       VirtIOGPUGL *gl;
>>         vmr = to_hostmem_region(mr);
>> -    vmr->finish_unmapping = true;
>> +    vmr->mapping_state = VIRTIO_GPU_MR_UNMAP_COMPLETED;
>>         b = VIRTIO_GPU_BASE(vmr->g);
>>       b->renderer_blocked--;
>> @@ -135,6 +141,7 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
>>         vmr = g_new0(struct virtio_gpu_virgl_hostmem_region, 1);
>>       vmr->g = g;
>> +    vmr->mapping_state = VIRTIO_GPU_MR_MAPPED;
>>         mr = &vmr->mr;
>>       memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size, data);
>> @@ -171,7 +178,8 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
>>         vmr = to_hostmem_region(res->mr);
>>   -    trace_virtio_gpu_cmd_res_unmap_blob(res->base.resource_id, mr,
>> vmr->finish_unmapping);
>> +    trace_virtio_gpu_cmd_res_unmap_blob(res->base.resource_id, mr,
>> +                                        vmr->mapping_state);
>>         /*
>>        * Perform async unmapping in 3 steps:
>> @@ -182,7 +190,8 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
>>        *    asynchronously by virtio_gpu_virgl_hostmem_region_free().
>>        * 3. Finish the unmapping with final
>> virgl_renderer_resource_unmap().
>>        */
>> -    if (vmr->finish_unmapping) {
>> +    switch (vmr->mapping_state) {
>> +    case VIRTIO_GPU_MR_UNMAP_COMPLETED:
>>           res->mr = NULL;
>>           g_free(vmr);
>>   @@ -193,15 +202,24 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU
>> *g,
>>                             __func__, strerror(-ret));
>>               return ret;
>>           }
>> -    } else {
>> +        break;
>> +
>> +    case VIRTIO_GPU_MR_MAPPED:
>>           *cmd_suspended = true;
>>             /* render will be unblocked once MR is freed */
>>           b->renderer_blocked++;
>>   +        vmr->mapping_state = VIRTIO_GPU_MR_UNMAP_STARTED;
>> +
>>           /* memory region owns self res->mr object and frees it by
>> itself */
>>           memory_region_del_subregion(&b->hostmem, mr);
>>           object_unparent(OBJECT(mr));
>> +        break;
> 
> I suggest:
> 
> - Put vmr->mapping_state = VIRTIO_GPU_MR_UNMAP_STARTED; here
> - Remove *cmd_suspended = true for VIRTIO_GPU_MR_MAPPED.
> - Let it fall through.
> 
> This way, it is clear that we need to execute *cmd_suspended = true
> because the state is now VIRTIO_GPU_MR_UNMAP_STARTED, and we can save on
> line by not having a duplicate *cmd_suspended = true.

The `mapping_state = VIRTIO_GPU_MR_UNMAP_STARTED` shall be set before
memory_region_del_subregion() is invoked because technically refcounting
logic may change in future and MR may become freed instantly. Will only
add the fall-through if no objections, otherwise please comment on v10.

Best regards,
Dmitry

Re: [RFC PATCH v9 3/5] virtio-gpu: Replace finish_unmapping with mapping_state

[Akihiko Odaki]

On 2026/01/19 1:28, Dmitry Osipenko wrote:
> On 1/13/26 07:51, Akihiko Odaki wrote:
>> On 2026/01/13 7:52, Dmitry Osipenko wrote:
>>> Allow virtio_gpu_virgl_unmap_resource_blob() to be invoked while async
>>> unmapping is in progress. Do it in preparation to improvement of
>>> virtio-gpu
>>> resetting that will require this change.
>>>
>>> Suggested-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>>> ---
>>>    hw/display/trace-events       |  2 +-
>>>    hw/display/virtio-gpu-virgl.c | 28 +++++++++++++++++++++++-----
>>>    2 files changed, 24 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/hw/display/trace-events b/hw/display/trace-events
>>> index e323a82cff24..4bfc457fbac1 100644
>>> --- a/hw/display/trace-events
>>> +++ b/hw/display/trace-events
>>> @@ -39,7 +39,7 @@ virtio_gpu_cmd_res_create_2d(uint32_t res, uint32_t
>>> fmt, uint32_t w, uint32_t h)
>>>    virtio_gpu_cmd_res_create_3d(uint32_t res, uint32_t fmt, uint32_t w,
>>> uint32_t h, uint32_t d) "res 0x%x, fmt 0x%x, w %d, h %d, d %d"
>>>    virtio_gpu_cmd_res_create_blob(uint32_t res, uint64_t size) "res
>>> 0x%x, size %" PRId64
>>>    virtio_gpu_cmd_res_map_blob(uint32_t res, void *vmr, void *mr) "res
>>> 0x%x, vmr %p, mr %p"
>>> -virtio_gpu_cmd_res_unmap_blob(uint32_t res, void *mr, bool
>>> finish_unmapping) "res 0x%x, mr %p, finish_unmapping %d"
>>> +virtio_gpu_cmd_res_unmap_blob(uint32_t res, void *mr, int
>>> mapping_state) "res 0x%x, mr %p, mapping_state %d"
>>>    virtio_gpu_cmd_res_unref(uint32_t res) "res 0x%x"
>>>    virtio_gpu_cmd_res_back_attach(uint32_t res) "res 0x%x"
>>>    virtio_gpu_cmd_res_back_detach(uint32_t res) "res 0x%x"
>>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-
>>> virgl.c
>>> index 6a2aac0b6e5c..342e93728df0 100644
>>> --- a/hw/display/virtio-gpu-virgl.c
>>> +++ b/hw/display/virtio-gpu-virgl.c
>>> @@ -68,10 +68,16 @@ virgl_get_egl_display(G_GNUC_UNUSED void *cookie)
>>>    #endif
>>>      #if VIRGL_VERSION_MAJOR >= 1
>>> +enum virtio_gpu_virgl_hostmem_region_mapping_state {
>>> +    VIRTIO_GPU_MR_MAPPED,
>>> +    VIRTIO_GPU_MR_UNMAP_STARTED,
>>> +    VIRTIO_GPU_MR_UNMAP_COMPLETED,
>>> +};
>>> +
>>>    struct virtio_gpu_virgl_hostmem_region {
>>>        MemoryRegion mr;
>>>        struct VirtIOGPU *g;
>>> -    bool finish_unmapping;
>>> +    enum virtio_gpu_virgl_hostmem_region_mapping_state mapping_state;
>>>    };
>>>      static struct virtio_gpu_virgl_hostmem_region *
>>> @@ -95,7 +101,7 @@ static void
>>> virtio_gpu_virgl_hostmem_region_free(void *obj)
>>>        VirtIOGPUGL *gl;
>>>          vmr = to_hostmem_region(mr);
>>> -    vmr->finish_unmapping = true;
>>> +    vmr->mapping_state = VIRTIO_GPU_MR_UNMAP_COMPLETED;
>>>          b = VIRTIO_GPU_BASE(vmr->g);
>>>        b->renderer_blocked--;
>>> @@ -135,6 +141,7 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
>>>          vmr = g_new0(struct virtio_gpu_virgl_hostmem_region, 1);
>>>        vmr->g = g;
>>> +    vmr->mapping_state = VIRTIO_GPU_MR_MAPPED;
>>>          mr = &vmr->mr;
>>>        memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size, data);
>>> @@ -171,7 +178,8 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
>>>          vmr = to_hostmem_region(res->mr);
>>>    -    trace_virtio_gpu_cmd_res_unmap_blob(res->base.resource_id, mr,
>>> vmr->finish_unmapping);
>>> +    trace_virtio_gpu_cmd_res_unmap_blob(res->base.resource_id, mr,
>>> +                                        vmr->mapping_state);
>>>          /*
>>>         * Perform async unmapping in 3 steps:
>>> @@ -182,7 +190,8 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
>>>         *    asynchronously by virtio_gpu_virgl_hostmem_region_free().
>>>         * 3. Finish the unmapping with final
>>> virgl_renderer_resource_unmap().
>>>         */
>>> -    if (vmr->finish_unmapping) {
>>> +    switch (vmr->mapping_state) {
>>> +    case VIRTIO_GPU_MR_UNMAP_COMPLETED:
>>>            res->mr = NULL;
>>>            g_free(vmr);
>>>    @@ -193,15 +202,24 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU
>>> *g,
>>>                              __func__, strerror(-ret));
>>>                return ret;
>>>            }
>>> -    } else {
>>> +        break;
>>> +
>>> +    case VIRTIO_GPU_MR_MAPPED:
>>>            *cmd_suspended = true;
>>>              /* render will be unblocked once MR is freed */
>>>            b->renderer_blocked++;
>>>    +        vmr->mapping_state = VIRTIO_GPU_MR_UNMAP_STARTED;
>>> +
>>>            /* memory region owns self res->mr object and frees it by
>>> itself */
>>>            memory_region_del_subregion(&b->hostmem, mr);
>>>            object_unparent(OBJECT(mr));
>>> +        break;
>>
>> I suggest:
>>
>> - Put vmr->mapping_state = VIRTIO_GPU_MR_UNMAP_STARTED; here
>> - Remove *cmd_suspended = true for VIRTIO_GPU_MR_MAPPED.
>> - Let it fall through.
>>
>> This way, it is clear that we need to execute *cmd_suspended = true
>> because the state is now VIRTIO_GPU_MR_UNMAP_STARTED, and we can save on
>> line by not having a duplicate *cmd_suspended = true.
> 
> The `mapping_state = VIRTIO_GPU_MR_UNMAP_STARTED` shall be set before
> memory_region_del_subregion() is invoked because technically refcounting
> logic may change in future and MR may become freed instantly. Will only
> add the fall-through if no objections, otherwise please comment on v10.

That makes sense.

Strictly speaking, even if the refcounting change happens, 
qemu_bh_schedule(gl->cmdq_resume_bh) will delay the next invocation of 
this function in virtio_gpu_virgl_hostmem_region_free(). But even 
virtio_gpu_virgl_hostmem_region_free() can be changed in the future so I 
no longer believe moving `mapping_state = VIRTIO_GPU_MR_UNMAP_STARTED` 
is absolutely good. Please choose a design option you prefer.

Regards,
Akihiko Odaki

Re: [RFC PATCH v9 3/5] virtio-gpu: Replace finish_unmapping with mapping_state

[Dmitry Osipenko]

On 1/19/26 08:54, Akihiko Odaki wrote:
> On 2026/01/19 1:28, Dmitry Osipenko wrote:
>> On 1/13/26 07:51, Akihiko Odaki wrote:
>>> On 2026/01/13 7:52, Dmitry Osipenko wrote:
>>>> Allow virtio_gpu_virgl_unmap_resource_blob() to be invoked while async
>>>> unmapping is in progress. Do it in preparation to improvement of
>>>> virtio-gpu
>>>> resetting that will require this change.
>>>>
>>>> Suggested-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
>>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>>>> ---
>>>>    hw/display/trace-events       |  2 +-
>>>>    hw/display/virtio-gpu-virgl.c | 28 +++++++++++++++++++++++-----
>>>>    2 files changed, 24 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/hw/display/trace-events b/hw/display/trace-events
>>>> index e323a82cff24..4bfc457fbac1 100644
>>>> --- a/hw/display/trace-events
>>>> +++ b/hw/display/trace-events
>>>> @@ -39,7 +39,7 @@ virtio_gpu_cmd_res_create_2d(uint32_t res, uint32_t
>>>> fmt, uint32_t w, uint32_t h)
>>>>    virtio_gpu_cmd_res_create_3d(uint32_t res, uint32_t fmt, uint32_t w,
>>>> uint32_t h, uint32_t d) "res 0x%x, fmt 0x%x, w %d, h %d, d %d"
>>>>    virtio_gpu_cmd_res_create_blob(uint32_t res, uint64_t size) "res
>>>> 0x%x, size %" PRId64
>>>>    virtio_gpu_cmd_res_map_blob(uint32_t res, void *vmr, void *mr) "res
>>>> 0x%x, vmr %p, mr %p"
>>>> -virtio_gpu_cmd_res_unmap_blob(uint32_t res, void *mr, bool
>>>> finish_unmapping) "res 0x%x, mr %p, finish_unmapping %d"
>>>> +virtio_gpu_cmd_res_unmap_blob(uint32_t res, void *mr, int
>>>> mapping_state) "res 0x%x, mr %p, mapping_state %d"
>>>>    virtio_gpu_cmd_res_unref(uint32_t res) "res 0x%x"
>>>>    virtio_gpu_cmd_res_back_attach(uint32_t res) "res 0x%x"
>>>>    virtio_gpu_cmd_res_back_detach(uint32_t res) "res 0x%x"
>>>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-
>>>> virgl.c
>>>> index 6a2aac0b6e5c..342e93728df0 100644
>>>> --- a/hw/display/virtio-gpu-virgl.c
>>>> +++ b/hw/display/virtio-gpu-virgl.c
>>>> @@ -68,10 +68,16 @@ virgl_get_egl_display(G_GNUC_UNUSED void *cookie)
>>>>    #endif
>>>>      #if VIRGL_VERSION_MAJOR >= 1
>>>> +enum virtio_gpu_virgl_hostmem_region_mapping_state {
>>>> +    VIRTIO_GPU_MR_MAPPED,
>>>> +    VIRTIO_GPU_MR_UNMAP_STARTED,
>>>> +    VIRTIO_GPU_MR_UNMAP_COMPLETED,
>>>> +};
>>>> +
>>>>    struct virtio_gpu_virgl_hostmem_region {
>>>>        MemoryRegion mr;
>>>>        struct VirtIOGPU *g;
>>>> -    bool finish_unmapping;
>>>> +    enum virtio_gpu_virgl_hostmem_region_mapping_state mapping_state;
>>>>    };
>>>>      static struct virtio_gpu_virgl_hostmem_region *
>>>> @@ -95,7 +101,7 @@ static void
>>>> virtio_gpu_virgl_hostmem_region_free(void *obj)
>>>>        VirtIOGPUGL *gl;
>>>>          vmr = to_hostmem_region(mr);
>>>> -    vmr->finish_unmapping = true;
>>>> +    vmr->mapping_state = VIRTIO_GPU_MR_UNMAP_COMPLETED;
>>>>          b = VIRTIO_GPU_BASE(vmr->g);
>>>>        b->renderer_blocked--;
>>>> @@ -135,6 +141,7 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
>>>>          vmr = g_new0(struct virtio_gpu_virgl_hostmem_region, 1);
>>>>        vmr->g = g;
>>>> +    vmr->mapping_state = VIRTIO_GPU_MR_MAPPED;
>>>>          mr = &vmr->mr;
>>>>        memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size, data);
>>>> @@ -171,7 +178,8 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
>>>>          vmr = to_hostmem_region(res->mr);
>>>>    -    trace_virtio_gpu_cmd_res_unmap_blob(res->base.resource_id, mr,
>>>> vmr->finish_unmapping);
>>>> +    trace_virtio_gpu_cmd_res_unmap_blob(res->base.resource_id, mr,
>>>> +                                        vmr->mapping_state);
>>>>          /*
>>>>         * Perform async unmapping in 3 steps:
>>>> @@ -182,7 +190,8 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
>>>>         *    asynchronously by virtio_gpu_virgl_hostmem_region_free().
>>>>         * 3. Finish the unmapping with final
>>>> virgl_renderer_resource_unmap().
>>>>         */
>>>> -    if (vmr->finish_unmapping) {
>>>> +    switch (vmr->mapping_state) {
>>>> +    case VIRTIO_GPU_MR_UNMAP_COMPLETED:
>>>>            res->mr = NULL;
>>>>            g_free(vmr);
>>>>    @@ -193,15 +202,24 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU
>>>> *g,
>>>>                              __func__, strerror(-ret));
>>>>                return ret;
>>>>            }
>>>> -    } else {
>>>> +        break;
>>>> +
>>>> +    case VIRTIO_GPU_MR_MAPPED:
>>>>            *cmd_suspended = true;
>>>>              /* render will be unblocked once MR is freed */
>>>>            b->renderer_blocked++;
>>>>    +        vmr->mapping_state = VIRTIO_GPU_MR_UNMAP_STARTED;
>>>> +
>>>>            /* memory region owns self res->mr object and frees it by
>>>> itself */
>>>>            memory_region_del_subregion(&b->hostmem, mr);
>>>>            object_unparent(OBJECT(mr));
>>>> +        break;
>>>
>>> I suggest:
>>>
>>> - Put vmr->mapping_state = VIRTIO_GPU_MR_UNMAP_STARTED; here
>>> - Remove *cmd_suspended = true for VIRTIO_GPU_MR_MAPPED.
>>> - Let it fall through.
>>>
>>> This way, it is clear that we need to execute *cmd_suspended = true
>>> because the state is now VIRTIO_GPU_MR_UNMAP_STARTED, and we can save on
>>> line by not having a duplicate *cmd_suspended = true.
>>
>> The `mapping_state = VIRTIO_GPU_MR_UNMAP_STARTED` shall be set before
>> memory_region_del_subregion() is invoked because technically refcounting
>> logic may change in future and MR may become freed instantly. Will only
>> add the fall-through if no objections, otherwise please comment on v10.
> 
> That makes sense.
> 
> Strictly speaking, even if the refcounting change happens,
> qemu_bh_schedule(gl->cmdq_resume_bh) will delay the next invocation of
> this function in virtio_gpu_virgl_hostmem_region_free(). But even
> virtio_gpu_virgl_hostmem_region_free() can be changed in the future so I
> no longer believe moving `mapping_state = VIRTIO_GPU_MR_UNMAP_STARTED`
> is absolutely good. Please choose a design option you prefer.

virtio_gpu_virgl_hostmem_region_free() changes state to UNMAP_COMPLETED,
hence the UNMAP_STARTED need to be set before region_free() invoked.
Will keep the v10 variant then, thanks.

-- 
Best regards,
Dmitry