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

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

[Dmitry Osipenko]

Virglrender got a new unstable 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:

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 (7):
  virtio-gpu: Remove superfluous memory_region_set_enabled()
  virtio-gpu: Validate hostmem mapping offset
  virtio-gpu: Improve virgl_cmd_resource_map_blob() error handling
  virtio-gpu: Make virtio_gpu_virgl_unmap_resource_blob() return -1 on
    error
  virtio-gpu: Destroy virgl resources on virtio-gpu reset
  virtio-gpu: Make virtio_gpu_virgl_init() return -1 on error
  virtio-gpu: Support mapping hostmem blobs with map_fixed

 hw/display/virtio-gpu-gl.c     |  49 +++++++++-
 hw/display/virtio-gpu-virgl.c  | 172 +++++++++++++++++++++++++++------
 include/hw/virtio/virtio-gpu.h |   8 +-
 3 files changed, 195 insertions(+), 34 deletions(-)

-- 
2.51.1

[RFC PATCH v4 1/7] 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>
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.51.1

[RFC PATCH v4 2/7] 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 | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index a6860f63b563..2224f59cf5d7 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -126,6 +126,14 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
         return -EOPNOTSUPP;
     }
 
+    if (offset + res->base.blob_size > b->conf.hostmem ||
+        offset + res->base.blob_size < offset) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: failed to map virgl resource: invalid offset\n",
+                      __func__);
+        return -EINVAL;
+    }
+
     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",
-- 
2.51.1

[RFC PATCH v4 3/7] virtio-gpu: Improve virgl_cmd_resource_map_blob() error handling

[Dmitry Osipenko]

Make virgl_cmd_resource_map_blob() return -1 for errors originated from
external virglrenderer library and respond to guest with most appropriate
error message.

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

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 2224f59cf5d7..edcdad0af232 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -138,7 +138,7 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
     if (ret) {
         qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to map virgl resource: %s\n",
                       __func__, strerror(-ret));
-        return ret;
+        return -1;
     }
 
     vmr = g_new0(struct virtio_gpu_virgl_hostmem_region, 1);
@@ -789,7 +789,16 @@ static void virgl_cmd_resource_map_blob(VirtIOGPU *g,
     }
 
     ret = virtio_gpu_virgl_map_resource_blob(g, res, mblob.offset);
-    if (ret) {
+
+    switch (ret) {
+    case 0:
+        break;
+
+    case -EINVAL:
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
+        return;
+
+    default:
         cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
         return;
     }
-- 
2.51.1

[RFC PATCH v4 4/7] virtio-gpu: Make virtio_gpu_virgl_unmap_resource_blob() return -1 on error

[Dmitry Osipenko]

Make virtio_gpu_virgl_unmap_resource_blob() return -1 for more consistency
of error propagation style in the code, adhering to QEMU's devel/style
recommendations and preparing code for further code changes utilizing this
function.

Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 hw/display/virtio-gpu-virgl.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index edcdad0af232..a7b14a312c83 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -199,7 +199,7 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
             qemu_log_mask(LOG_GUEST_ERROR,
                           "%s: failed to unmap virgl resource: %s\n",
                           __func__, strerror(-ret));
-            return ret;
+            return -1;
         }
     } else {
         *cmd_suspended = true;
@@ -333,7 +333,7 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
     }
 
 #if VIRGL_VERSION_MAJOR >= 1
-    if (virtio_gpu_virgl_unmap_resource_blob(g, res, cmd_suspended)) {
+    if (virtio_gpu_virgl_unmap_resource_blob(g, res, cmd_suspended) < 0) {
         cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
         return;
     }
@@ -829,7 +829,7 @@ static void virgl_cmd_resource_unmap_blob(VirtIOGPU *g,
     }
 
     ret = virtio_gpu_virgl_unmap_resource_blob(g, res, cmd_suspended);
-    if (ret) {
+    if (ret < 0) {
         cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
         return;
     }
-- 
2.51.1

[RFC PATCH v4 5/7] 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     |  6 ++-
 hw/display/virtio-gpu-virgl.c  | 83 +++++++++++++++++++++++++---------
 include/hw/virtio/virtio-gpu.h |  5 +-
 3 files changed, 71 insertions(+), 23 deletions(-)

diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
index b640900fc6f1..bca7d489c1e3 100644
--- a/hw/display/virtio-gpu-gl.c
+++ b/hw/display/virtio-gpu-gl.c
@@ -72,7 +72,10 @@ static void virtio_gpu_gl_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
 
     switch (gl->renderer_state) {
     case RS_RESET:
-        virtio_gpu_virgl_reset(g);
+        if (virtio_gpu_virgl_reset(g) < 0) {
+            gl->renderer_state = RS_INIT_FAILED;
+            return;
+        }
         /* fallthrough */
     case RS_START:
         if (virtio_gpu_virgl_init(g)) {
@@ -201,6 +204,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 a7b14a312c83..91951c3ffb0a 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -312,14 +312,44 @@ 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 *cmd_suspended)
+{
+    struct iovec *res_iovs = NULL;
+    int num_iovs = 0;
+
+#if VIRGL_VERSION_MAJOR >= 1
+    if (virtio_gpu_virgl_unmap_resource_blob(g, res, cmd_suspended) < 0) {
+        return -1;
+    }
+    if (*cmd_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);
@@ -332,27 +362,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) < 0) {
-        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) < 0) {
+        error_setg(errp, "failed to destroy virgl resource");
+    }
 }
 
 static void virgl_cmd_context_create(VirtIOGPU *g,
@@ -1280,11 +1304,28 @@ void virtio_gpu_virgl_reset_scanout(VirtIOGPU *g)
     }
 }
 
-void virtio_gpu_virgl_reset(VirtIOGPU *g)
+int virtio_gpu_virgl_reset(VirtIOGPU *g)
 {
+    struct virtio_gpu_simple_resource *res, *tmp;
+
+    /*
+     * Virglrender doesn't support context restoring. VirtIO-GPU
+     * state shall not be reset at runtime.
+     */
+    QTAILQ_FOREACH_SAFE(res, &g->reslist, next, tmp) {
+        virtio_gpu_virgl_resource_destroy(g, res, NULL);
+    }
+
+    if (!QTAILQ_EMPTY(&g->reslist)) {
+        error_report("%s: failed to reset virgl resources", __func__);
+        return -1;
+    }
+
     virgl_renderer_reset();
 
     virtio_gpu_virgl_reset_async_fences(g);
+
+    return 0;
 }
 
 int virtio_gpu_virgl_init(VirtIOGPU *g)
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 718332284305..9e1473d1bb66 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -389,9 +389,12 @@ 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_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);
 
 #endif
-- 
2.51.1

[RFC PATCH v4 6/7] virtio-gpu: Make virtio_gpu_virgl_init() return -1 on error

[Dmitry Osipenko]

Make virtio_gpu_virgl_init() return -1 on error to make it consistent
with virtio_gpu_virgl_reset() in regards to error handling codding style,
adhering to QEMU's devel/style recommendations.

Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 hw/display/virtio-gpu-gl.c    | 2 +-
 hw/display/virtio-gpu-virgl.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
index bca7d489c1e3..d65da4863923 100644
--- a/hw/display/virtio-gpu-gl.c
+++ b/hw/display/virtio-gpu-gl.c
@@ -78,7 +78,7 @@ static void virtio_gpu_gl_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
         }
         /* fallthrough */
     case RS_START:
-        if (virtio_gpu_virgl_init(g)) {
+        if (virtio_gpu_virgl_init(g) < 0) {
             gl->renderer_state = RS_INIT_FAILED;
             return;
         }
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 91951c3ffb0a..9b36b378c2fd 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -1371,7 +1371,7 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
              * Async-fence was bugged in virglrenderer versions <= 1.1.1.
              */
             error_report("drm requires egl display and virglrenderer >= 1.2.0");
-            return -EINVAL;
+            return -1;
         }
     }
 #endif
@@ -1379,7 +1379,7 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
     ret = virgl_renderer_init(g, flags, &virtio_gpu_3d_cbs);
     if (ret != 0) {
         error_report("virgl could not be initialized: %d", ret);
-        return ret;
+        return -1;
     }
 
     gl->fence_poll = timer_new_ms(QEMU_CLOCK_VIRTUAL,
-- 
2.51.1

[RFC PATCH v4 7/7] 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     | 41 +++++++++++++++++++++++-
 hw/display/virtio-gpu-virgl.c  | 58 +++++++++++++++++++++++++++++++++-
 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 d65da4863923..8a7a33946085 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"
@@ -124,7 +125,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");
@@ -155,6 +161,28 @@ 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(errp,
+                       "virgl hostmem region could not be initialized: %s",
+                       strerror(errno));
+            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[] = {
@@ -190,6 +218,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 9b36b378c2fd..987980e8a49d 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 *
@@ -116,6 +120,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;
@@ -134,6 +141,41 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
         return -EINVAL;
     }
 
+#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 -1;
+    }
+#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",
@@ -146,7 +188,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
@@ -173,6 +215,20 @@ 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) {
+            error_report("%s: failed to unmap(fixed) virgl resource: %s",
+                          __func__, strerror(errno));
+            return -1;
+        }
+
+        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 9e1473d1bb66..420c6e2a2515 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.51.1

Re: [RFC PATCH v4 7/7] virtio-gpu: Support mapping hostmem blobs with map_fixed

[Akihiko Odaki]

On 2025/11/25 11:35, Dmitry Osipenko wrote:
> 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     | 41 +++++++++++++++++++++++-
>   hw/display/virtio-gpu-virgl.c  | 58 +++++++++++++++++++++++++++++++++-
>   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 d65da4863923..8a7a33946085 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"
> @@ -124,7 +125,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");
> @@ -155,6 +161,28 @@ 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(errp,
> +                       "virgl hostmem region could not be initialized: %s",
> +                       strerror(errno));

error_setg_errno() will make this a bit simpler.

> +            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[] = {
> @@ -190,6 +218,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 9b36b378c2fd..987980e8a49d 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 *
> @@ -116,6 +120,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;
> @@ -134,6 +141,41 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
>           return -EINVAL;
>       }
>   
> +#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 -1;
> +    }
> +#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",
> @@ -146,7 +188,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
> @@ -173,6 +215,20 @@ 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) {
> +            error_report("%s: failed to unmap(fixed) virgl resource: %s",
> +                          __func__, strerror(errno));
> +            return -1;
> +        }
> +
> +        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 9e1473d1bb66..420c6e2a2515 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 {

Re: [RFC PATCH v4 1/7] virtio-gpu: Remove superfluous memory_region_set_enabled()

[Alex Bennée]

Dmitry Osipenko <dmitry.osipenko@collabora.com> writes:

> 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>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [RFC PATCH v4 2/7] virtio-gpu: Validate hostmem mapping offset

[Alex Bennée]

Dmitry Osipenko <dmitry.osipenko@collabora.com> writes:

> 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 | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index a6860f63b563..2224f59cf5d7 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -126,6 +126,14 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
>          return -EOPNOTSUPP;
>      }
>  
> +    if (offset + res->base.blob_size > b->conf.hostmem ||
> +        offset + res->base.blob_size < offset) {

This second check seems weird. offset + blob_size could only every be
smaller than offset if blob_size was negative. I feel we should have
caught that earlier if it can happen.

Are we trying to catch an overflow here?

> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: failed to map virgl resource: invalid offset\n",
> +                      __func__);
> +        return -EINVAL;
> +    }
> +
>      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",

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [RFC PATCH v4 3/7] virtio-gpu: Improve virgl_cmd_resource_map_blob() error handling

[Alex Bennée]

Dmitry Osipenko <dmitry.osipenko@collabora.com> writes:

> Make virgl_cmd_resource_map_blob() return -1 for errors originated from
> external virglrenderer library and respond to guest with most appropriate
> error message.
>
> Suggested-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> # guest err msg
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---
>  hw/display/virtio-gpu-virgl.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index 2224f59cf5d7..edcdad0af232 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -138,7 +138,7 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
>      if (ret) {
>          qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to map virgl resource: %s\n",
>                        __func__, strerror(-ret));
> -        return ret;
> +        return -1;

If we are using errno's lets use the defines rather than -1, should this
be -EPERM?

>      }
>  
>      vmr = g_new0(struct virtio_gpu_virgl_hostmem_region, 1);
> @@ -789,7 +789,16 @@ static void virgl_cmd_resource_map_blob(VirtIOGPU *g,
>      }
>  
>      ret = virtio_gpu_virgl_map_resource_blob(g, res, mblob.offset);
> -    if (ret) {
> +
> +    switch (ret) {
> +    case 0:
> +        break;
> +
> +    case -EINVAL:
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
> +        return;

which isn't what can come here. I see EOPNOTSUPP not being handled either.

> +
> +    default:
>          cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>          return;
>      }

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [RFC PATCH v4 4/7] virtio-gpu: Make virtio_gpu_virgl_unmap_resource_blob() return -1 on error

[Alex Bennée]

Dmitry Osipenko <dmitry.osipenko@collabora.com> writes:

> Make virtio_gpu_virgl_unmap_resource_blob() return -1 for more consistency
> of error propagation style in the code, adhering to QEMU's devel/style
> recommendations and preparing code for further code changes utilizing this
> function.

I'm not so sure of that. If the function is a pass/fail then we tend to
prefer using bools in newer code. If you need richer internal error
reporting then start using your errno defines. If this is user visible
(i.e. during configuration) then we can make more of Error* and friends.

>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---
>  hw/display/virtio-gpu-virgl.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index edcdad0af232..a7b14a312c83 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -199,7 +199,7 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
>              qemu_log_mask(LOG_GUEST_ERROR,
>                            "%s: failed to unmap virgl resource: %s\n",
>                            __func__, strerror(-ret));
> -            return ret;
> +            return -1;
>          }
>      } else {
>          *cmd_suspended = true;
> @@ -333,7 +333,7 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
>      }
>  
>  #if VIRGL_VERSION_MAJOR >= 1
> -    if (virtio_gpu_virgl_unmap_resource_blob(g, res, cmd_suspended)) {
> +    if (virtio_gpu_virgl_unmap_resource_blob(g, res, cmd_suspended) < 0) {
>          cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>          return;
>      }
> @@ -829,7 +829,7 @@ static void virgl_cmd_resource_unmap_blob(VirtIOGPU *g,
>      }
>  
>      ret = virtio_gpu_virgl_unmap_resource_blob(g, res, cmd_suspended);
> -    if (ret) {
> +    if (ret < 0) {
>          cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>          return;
>      }

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [RFC PATCH v4 6/7] virtio-gpu: Make virtio_gpu_virgl_init() return -1 on error

[Alex Bennée]

Dmitry Osipenko <dmitry.osipenko@collabora.com> writes:

> Make virtio_gpu_virgl_init() return -1 on error to make it consistent
> with virtio_gpu_virgl_reset() in regards to error handling codding style,
> adhering to QEMU's devel/style recommendations.

See previous comment on other -1 returns.

>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---
>  hw/display/virtio-gpu-gl.c    | 2 +-
>  hw/display/virtio-gpu-virgl.c | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
> index bca7d489c1e3..d65da4863923 100644
> --- a/hw/display/virtio-gpu-gl.c
> +++ b/hw/display/virtio-gpu-gl.c
> @@ -78,7 +78,7 @@ static void virtio_gpu_gl_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
>          }
>          /* fallthrough */
>      case RS_START:
> -        if (virtio_gpu_virgl_init(g)) {
> +        if (virtio_gpu_virgl_init(g) < 0) {
>              gl->renderer_state = RS_INIT_FAILED;
>              return;
>          }
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index 91951c3ffb0a..9b36b378c2fd 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -1371,7 +1371,7 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
>               * Async-fence was bugged in virglrenderer versions <= 1.1.1.
>               */
>              error_report("drm requires egl display and virglrenderer >= 1.2.0");
> -            return -EINVAL;
> +            return -1;
>          }
>      }
>  #endif
> @@ -1379,7 +1379,7 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
>      ret = virgl_renderer_init(g, flags, &virtio_gpu_3d_cbs);
>      if (ret != 0) {
>          error_report("virgl could not be initialized: %d", ret);
> -        return ret;
> +        return -1;
>      }
>  
>      gl->fence_poll = timer_new_ms(QEMU_CLOCK_VIRTUAL,

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [RFC PATCH v4 2/7] virtio-gpu: Validate hostmem mapping offset

[Dmitry Osipenko]

On 11/25/25 14:54, Alex Bennée wrote:
> Dmitry Osipenko <dmitry.osipenko@collabora.com> writes:
> 
>> 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 | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
>> index a6860f63b563..2224f59cf5d7 100644
>> --- a/hw/display/virtio-gpu-virgl.c
>> +++ b/hw/display/virtio-gpu-virgl.c
>> @@ -126,6 +126,14 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
>>          return -EOPNOTSUPP;
>>      }
>>  
>> +    if (offset + res->base.blob_size > b->conf.hostmem ||
>> +        offset + res->base.blob_size < offset) {
> 
> This second check seems weird. offset + blob_size could only every be
> smaller than offset if blob_size was negative. I feel we should have
> caught that earlier if it can happen.
> 
> Are we trying to catch an overflow here?

The second check catches integer overflow for huge mblob.offset that is
u64 coming from guest. This wasn't caught before, we missed validation
of the offset value.

-- 
Best regards,
Dmitry

Re: [RFC PATCH v4 3/7] virtio-gpu: Improve virgl_cmd_resource_map_blob() error handling

[Dmitry Osipenko]

On 11/25/25 15:00, Alex Bennée wrote:
>>      if (ret) {
>>          qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to map virgl resource: %s\n",
>>                        __func__, strerror(-ret));
>> -        return ret;
>> +        return -1;
> If we are using errno's lets use the defines rather than -1, should this
> be -EPERM?
> 
>>      }
>>  
>>      vmr = g_new0(struct virtio_gpu_virgl_hostmem_region, 1);
>> @@ -789,7 +789,16 @@ static void virgl_cmd_resource_map_blob(VirtIOGPU *g,
>>      }
>>  
>>      ret = virtio_gpu_virgl_map_resource_blob(g, res, mblob.offset);
>> -    if (ret) {
>> +
>> +    switch (ret) {
>> +    case 0:
>> +        break;
>> +
>> +    case -EINVAL:
>> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
>> +        return;
> which isn't what can come here. I see EOPNOTSUPP not being handled either.

When error comes from external virglrenderer code, we don't know exact
reason it fails. Using -1 allows clear distinguish of external vs
internal to QEMU errors.

EOPNOTSUPP and everything else that doesn't have a directly matching
guest code falls into the default error handling returning
VIRTIO_GPU_RESP_ERR_UNSPEC to guest.

-- 
Best regards,
Dmitry

Re: [RFC PATCH v4 4/7] virtio-gpu: Make virtio_gpu_virgl_unmap_resource_blob() return -1 on error

[Dmitry Osipenko]

On 11/25/25 15:09, Alex Bennée wrote:
> Dmitry Osipenko <dmitry.osipenko@collabora.com> writes:
> 
>> Make virtio_gpu_virgl_unmap_resource_blob() return -1 for more consistency
>> of error propagation style in the code, adhering to QEMU's devel/style
>> recommendations and preparing code for further code changes utilizing this
>> function.
> I'm not so sure of that. If the function is a pass/fail then we tend to
> prefer using bools in newer code. If you need richer internal error
> reporting then start using your errno defines. If this is user visible
> (i.e. during configuration) then we can make more of Error* and friends.

The current code is a mix of all variants. Will be good to stick with one.

Akihiko, what are your thoughts on the best option for the errors
handling? Would you also prefer bools?

-- 
Best regards,
Dmitry

Re: [RFC PATCH v4 4/7] virtio-gpu: Make virtio_gpu_virgl_unmap_resource_blob() return -1 on error

[Akihiko Odaki]

On 2025/11/26 0:49, Dmitry Osipenko wrote:
> On 11/25/25 15:09, Alex Bennée wrote:
>> Dmitry Osipenko <dmitry.osipenko@collabora.com> writes:
>>
>>> Make virtio_gpu_virgl_unmap_resource_blob() return -1 for more consistency
>>> of error propagation style in the code, adhering to QEMU's devel/style
>>> recommendations and preparing code for further code changes utilizing this
>>> function.
>> I'm not so sure of that. If the function is a pass/fail then we tend to
>> prefer using bools in newer code. If you need richer internal error
>> reporting then start using your errno defines. If this is user visible
>> (i.e. during configuration) then we can make more of Error* and friends.
> 
> The current code is a mix of all variants. Will be good to stick with one.
> 
> Akihiko, what are your thoughts on the best option for the errors
> handling? Would you also prefer bools?

Sorry, I missed this.

I'm for bools. It allows compilers to enforce having either of two 
values, whose semantics is defined in include/qapi/error.h "true on 
success / false on failure"; it says functions should also have "Error 
**errp" but I think the semantics of bool can be applied for those 
without the parameter.

If you are still not sure or there are disagreements, I think it is 
better to ask Markus Armbruster, who maintains the error reporting 
facility shared for the whole codebase.

Regards,
Akihiko Odaki

Re: [RFC PATCH v4 4/7] virtio-gpu: Make virtio_gpu_virgl_unmap_resource_blob() return -1 on error

[Dmitry Osipenko]

On 12/5/25 07:08, Akihiko Odaki wrote:
> On 2025/11/26 0:49, Dmitry Osipenko wrote:
>> On 11/25/25 15:09, Alex Bennée wrote:
>>> Dmitry Osipenko <dmitry.osipenko@collabora.com> writes:
>>>
>>>> Make virtio_gpu_virgl_unmap_resource_blob() return -1 for more
>>>> consistency
>>>> of error propagation style in the code, adhering to QEMU's devel/style
>>>> recommendations and preparing code for further code changes
>>>> utilizing this
>>>> function.
>>> I'm not so sure of that. If the function is a pass/fail then we tend to
>>> prefer using bools in newer code. If you need richer internal error
>>> reporting then start using your errno defines. If this is user visible
>>> (i.e. during configuration) then we can make more of Error* and friends.
>>
>> The current code is a mix of all variants. Will be good to stick with
>> one.
>>
>> Akihiko, what are your thoughts on the best option for the errors
>> handling? Would you also prefer bools?
> 
> Sorry, I missed this.
> 
> I'm for bools. It allows compilers to enforce having either of two
> values, whose semantics is defined in include/qapi/error.h "true on
> success / false on failure"; it says functions should also have "Error
> **errp" but I think the semantics of bool can be applied for those
> without the parameter.
> 
> If you are still not sure or there are disagreements, I think it is
> better to ask Markus Armbruster, who maintains the error reporting
> facility shared for the whole codebase.

Thanks for the reply. The bools are fine with me. I removed patches
changing the error handling in a next version of this patchset as they
are a bit controversial and not essential for this series.

-- 
Best regards,
Dmitry