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

[RFC PATCH v1 0/1] 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: 20251109164711.686873-1-dmitry.osipenko@collabora.com

Dmitry Osipenko (1):
  virtio-gpu: Support mapping hostmem blobs with map_fixed

 hw/display/virtio-gpu-gl.c     | 37 +++++++++++++++++
 hw/display/virtio-gpu-virgl.c  | 72 +++++++++++++++++++++++++++++++++-
 include/hw/virtio/virtio-gpu.h |  3 ++
 3 files changed, 110 insertions(+), 2 deletions(-)

-- 
2.51.1

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

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

diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
index b640900fc6f1..e1481291948a 100644
--- a/hw/display/virtio-gpu-gl.c
+++ b/hw/display/virtio-gpu-gl.c
@@ -122,6 +122,9 @@ static void virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
 {
     ERRP_GUARD();
     VirtIOGPU *g = VIRTIO_GPU(qdev);
+    VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
+    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
+    void *map;
 
 #if HOST_BIG_ENDIAN
     error_setg(errp, "virgl is not supported on bigendian platforms");
@@ -152,6 +155,31 @@ static void virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
 #endif
 
     virtio_gpu_device_realize(qdev, errp);
+
+    /*
+     * Check whether virtio_gpu_device_realize() failed.
+     */
+    if (!g->ctrl_bh) {
+        return;
+    }
+
+    if (virtio_gpu_hostmem_enabled(b->conf)) {
+        map = mmap(NULL, b->conf.hostmem, PROT_NONE,
+                   MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+        if (map == MAP_FAILED) {
+            error_setg(errp,
+                       "virgl hostmem region could not be initialized: %s",
+                       strerror(errno));
+            return;
+        }
+
+        gl->hostmem_mmap = map;
+        gl->hostmem_mr = g_new0(MemoryRegion, 1);
+        memory_region_init_ram_ptr(gl->hostmem_mr, NULL, "hostmem-anon",
+                                   b->conf.hostmem, gl->hostmem_mmap);
+        memory_region_add_subregion(&b->hostmem, 0, gl->hostmem_mr);
+        memory_region_set_enabled(gl->hostmem_mr, true);
+    }
 }
 
 static const Property virtio_gpu_gl_properties[] = {
@@ -167,6 +195,7 @@ static void virtio_gpu_gl_device_unrealize(DeviceState *qdev)
 {
     VirtIOGPU *g = VIRTIO_GPU(qdev);
     VirtIOGPUGL *gl = VIRTIO_GPU_GL(qdev);
+    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
 
     if (gl->renderer_state >= RS_INITED) {
 #if VIRGL_VERSION_MAJOR >= 1
@@ -187,6 +216,14 @@ static void virtio_gpu_gl_device_unrealize(DeviceState *qdev)
     gl->renderer_state = RS_START;
 
     g_array_unref(g->capset_ids);
+
+    if (gl->hostmem_mr) {
+        memory_region_set_enabled(gl->hostmem_mr, false);
+        memory_region_del_subregion(&b->hostmem, gl->hostmem_mr);
+    }
+    if (gl->hostmem_mmap) {
+        munmap(gl->hostmem_mmap, b->conf.hostmem);
+    }
 }
 
 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 5c9a9ee84392..d4188cf1d6b2 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -21,6 +21,7 @@
 #include "hw/virtio/virtio-gpu-pixman.h"
 
 #include "ui/egl-helpers.h"
+#include <stdio.h>
 
 #include <virglrenderer.h>
 
@@ -44,6 +45,7 @@
 struct virtio_gpu_virgl_resource {
     struct virtio_gpu_simple_resource base;
     MemoryRegion *mr;
+    void *map_fixed;
 };
 
 static struct virtio_gpu_virgl_resource *
@@ -116,6 +118,7 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
 {
     struct virtio_gpu_virgl_hostmem_region *vmr;
     VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
+    VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
     MemoryRegion *mr;
     uint64_t size;
     void *data;
@@ -126,6 +129,40 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
         return -EOPNOTSUPP;
     }
 
+#if VIRGL_CHECK_VERSION(1, 2, 1) && !defined(_WIN32)
+    /*
+     * virgl_renderer_resource_map_fixed() allows to create multiple
+     * mappings of the same resource, while virgl_renderer_resource_map()
+     * not.
+     */
+    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;
+    }
+
+    /*
+     * -EOPNOTSUPP is returned if MAP_FIXED unsupported by this resource,
+     * mapping falls back to a blob subregion method in that case.
+     */
+    ret = virgl_renderer_resource_map_fixed(res->base.resource_id,
+                                            gl->hostmem_mmap + offset);
+    if (ret && ret != -EOPNOTSUPP) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: failed to map(fixed) virgl resource: %s\n",
+                      __func__, strerror(-ret));
+        return ret;
+    }
+
+    if (ret == 0) {
+        fprintf(stderr, "virgl_renderer_resource_map_fixed\n");
+
+        res->map_fixed = gl->hostmem_mmap + offset;
+        return 0;
+    }
+#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",
@@ -138,7 +175,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(gl->hostmem_mr, offset, mr);
     memory_region_set_enabled(mr, true);
 
     /*
@@ -163,9 +200,27 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
 {
     struct virtio_gpu_virgl_hostmem_region *vmr;
     VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
+    VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
     MemoryRegion *mr = res->mr;
     int ret;
 
+#if VIRGL_CHECK_VERSION(1, 2, 1) && !defined(_WIN32)
+    if (res->map_fixed) {
+        res->map_fixed = mmap(res->map_fixed, res->base.blob_size, PROT_NONE,
+                              MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED,
+                              -1, 0);
+        if (res->map_fixed == MAP_FAILED) {
+            ret = -errno;
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "%s: failed to unmap(fixed) virgl resource: %s\n",
+                          __func__, strerror(-ret));
+            return ret;
+        }
+
+        res->map_fixed = NULL;
+    }
+#endif
+
     if (!mr) {
         return 0;
     }
@@ -202,7 +257,7 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
 
         /* 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);
+        memory_region_del_subregion(gl->hostmem_mr, mr);
         object_unparent(OBJECT(mr));
     }
 
@@ -1270,9 +1325,22 @@ void virtio_gpu_virgl_reset_scanout(VirtIOGPU *g)
 
 void virtio_gpu_virgl_reset(VirtIOGPU *g)
 {
+#if VIRGL_CHECK_VERSION(1, 2, 1) && !defined(_WIN32)
+    VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
+    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
+#endif
+
     virgl_renderer_reset();
 
     virtio_gpu_virgl_reset_async_fences(g);
+
+#if VIRGL_CHECK_VERSION(1, 2, 1) && !defined(_WIN32)
+    if (gl->hostmem_mmap &&
+        mmap(gl->hostmem_mmap, b->conf.hostmem, PROT_NONE,
+             MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0) == MAP_FAILED) {
+        error_report("failed to reset virgl hostmem: %s", strerror(errno));
+    }
+#endif
 }
 
 int virtio_gpu_virgl_init(VirtIOGPU *g)
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 172f5ffce3ed..e1122edd6eeb 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_mr;
+    void *hostmem_mmap;
 };
 
 struct VhostUserGPU {
-- 
2.51.1

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

[Akihiko Odaki]

On 2025/11/13 8:31, 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.
> 
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---
>   hw/display/virtio-gpu-gl.c     | 37 +++++++++++++++++
>   hw/display/virtio-gpu-virgl.c  | 72 +++++++++++++++++++++++++++++++++-
>   include/hw/virtio/virtio-gpu.h |  3 ++
>   3 files changed, 110 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
> index b640900fc6f1..e1481291948a 100644
> --- a/hw/display/virtio-gpu-gl.c
> +++ b/hw/display/virtio-gpu-gl.c
> @@ -122,6 +122,9 @@ static void virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
>   {
>       ERRP_GUARD();
>       VirtIOGPU *g = VIRTIO_GPU(qdev);
> +    VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
> +    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);

Nitpick: this order is slightly odd as VirtIOGPUBase is the base class 
of VirtIOGPU and VirtIOGPUGL. So let's do:

VirtIOGPUBase *b = VIRTIO_GPU_BASE(qdev); // super class of b
VirtIOGPU *g = VIRTIO_GPU(qdev); // base class of gl
VirtIOGPUGL *gl = VIRTIO_GPU_GL(qdev);

Arguments are unified to qdev for consistency with other functions.

> +    void *map;
>   
>   #if HOST_BIG_ENDIAN
>       error_setg(errp, "virgl is not supported on bigendian platforms");
> @@ -152,6 +155,31 @@ static void virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
>   #endif
>   
>       virtio_gpu_device_realize(qdev, errp);
> +
> +    /*
> +     * Check whether virtio_gpu_device_realize() failed.
> +     */
> +    if (!g->ctrl_bh) {

Instead, do:
if (*errp) {
     return;
}

With this change it is clear that it checks whether 
virtio_gpu_device_realize() failed so the comment will be unnecessary.

> +        return;
> +    }
> +
> +    if (virtio_gpu_hostmem_enabled(b->conf)) {
> +        map = mmap(NULL, b->conf.hostmem, PROT_NONE,
> +                   MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);

I think you need to check CONFIG_WIN32.

> +        if (map == MAP_FAILED) {
> +            error_setg(errp,
> +                       "virgl hostmem region could not be initialized: %s",
> +                       strerror(errno));
> +            return;
> +        }
> +
> +        gl->hostmem_mmap = map;
> +        gl->hostmem_mr = g_new0(MemoryRegion, 1);

Having "b->hostmem" and "gl->hostmem_mr" is rather confusing.

> +        memory_region_init_ram_ptr(gl->hostmem_mr, NULL, "hostmem-anon",

"anon" is misleading because virgl_renderer_resource_map_fixed() will 
map non-anonymous pages.

Looking for an alternative word, I found docs/devel/memory.rst refers 
this kind of memory region as "background".

> +                                   b->conf.hostmem, gl->hostmem_mmap);
> +        memory_region_add_subregion(&b->hostmem, 0, gl->hostmem_mr);
> +        memory_region_set_enabled(gl->hostmem_mr, true);

Calling memory_region_set_enabled() is unnecessary. I failed to point 
out this for commit 7c092f17ccee ("virtio-gpu: Handle resource blob 
commands")...

> +    }
>   }
>   
>   static const Property virtio_gpu_gl_properties[] = {
> @@ -167,6 +195,7 @@ static void virtio_gpu_gl_device_unrealize(DeviceState *qdev)
>   {
>       VirtIOGPU *g = VIRTIO_GPU(qdev);
>       VirtIOGPUGL *gl = VIRTIO_GPU_GL(qdev);
> +    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
>   
>       if (gl->renderer_state >= RS_INITED) {
>   #if VIRGL_VERSION_MAJOR >= 1
> @@ -187,6 +216,14 @@ static void virtio_gpu_gl_device_unrealize(DeviceState *qdev)
>       gl->renderer_state = RS_START;
>   
>       g_array_unref(g->capset_ids);
> +
> +    if (gl->hostmem_mr) {
> +        memory_region_set_enabled(gl->hostmem_mr, false);

This memory_region_set_enabled() is unnecessary either.

> +        memory_region_del_subregion(&b->hostmem, gl->hostmem_mr);
> +    }
> +    if (gl->hostmem_mmap) {
> +        munmap(gl->hostmem_mmap, b->conf.hostmem);
> +    }

Please remove munmap(). It is not guaranteed that the memory region will 
be finalized immediately with memory_region_del_subregion(), so there 
can be a remaining reference to gl->hostmem_mmap.

virtio-gpu is not hotpluggable so you don't have to worry about memory 
leaks.

>   }
>   
>   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 5c9a9ee84392..d4188cf1d6b2 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -21,6 +21,7 @@
>   #include "hw/virtio/virtio-gpu-pixman.h"
>   
>   #include "ui/egl-helpers.h"
> +#include <stdio.h>
>   
>   #include <virglrenderer.h>
>   
> @@ -44,6 +45,7 @@
>   struct virtio_gpu_virgl_resource {
>       struct virtio_gpu_simple_resource base;
>       MemoryRegion *mr;
> +    void *map_fixed;
>   };
>   
>   static struct virtio_gpu_virgl_resource *
> @@ -116,6 +118,7 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
>   {
>       struct virtio_gpu_virgl_hostmem_region *vmr;
>       VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
> +    VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
>       MemoryRegion *mr;
>       uint64_t size;
>       void *data;
> @@ -126,6 +129,40 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
>           return -EOPNOTSUPP;
>       }
>   
> +#if VIRGL_CHECK_VERSION(1, 2, 1) && !defined(_WIN32)

Check CONFIG_WIN32 instead. scripts/checkpatch.pl has the following code:
# check of hardware specific defines
# we have e.g. CONFIG_LINUX and CONFIG_WIN32 for common cases
# where they might be necessary.
		if ($line =~ m@^.\s*\#\s*if.*\b__@) {
			WARN("architecture specific defines should be avoided\n" .  $herecurr);
		}

This combination of VIRGL_CHECK_VERSION(1, 2, 1) && 
!defined(CONFIG_WIN32) is rather frequent so perhaps defining a macro 
for this may be useful.

> +    /*
> +     * virgl_renderer_resource_map_fixed() allows to create multiple
> +     * mappings of the same resource, while virgl_renderer_resource_map()
> +     * not.
> +     */
> +    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;
> +    }
> +
> +    /*
> +     * -EOPNOTSUPP is returned if MAP_FIXED unsupported by this resource,
> +     * mapping falls back to a blob subregion method in that case.
> +     */
> +    ret = virgl_renderer_resource_map_fixed(res->base.resource_id,
> +                                            gl->hostmem_mmap + offset);
> +    if (ret && ret != -EOPNOTSUPP) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: failed to map(fixed) virgl resource: %s\n",
> +                      __func__, strerror(-ret));
> +        return ret;
> +    }
> +
> +    if (ret == 0) {
> +        fprintf(stderr, "virgl_renderer_resource_map_fixed\n");

I guess you forgot removing a debug log.

> +
> +        res->map_fixed = gl->hostmem_mmap + offset;
> +        return 0;
> +    }

I would restructure these statements as follows:

ret = virgl_renderer_resource_map_fixed(...);
switch (ret) {
case 0:
     ...
     return 0;

case -EOPNOTSUPP:
     /*
      * -MAP_FIXED is unsupported by this resource.
      * Mapping falls back to a blob subregion method in that case.
      */
     break;

default:
     ...
     return ret;
}

...so that you don't need to check ret before checking
ret != -EOPNOTSUPP and what case the comment on the fallback applies.

> +#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",
> @@ -138,7 +175,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(gl->hostmem_mr, offset, mr);

docs/devel/memory.rst says having a subregion in a RAM reion is not 
preferred. Instead, memory_region_add_subregion_overlap() should be used 
for hostmem_mr to make it "background".

>       memory_region_set_enabled(mr, true);
>   
>       /*
> @@ -163,9 +200,27 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
>   {
>       struct virtio_gpu_virgl_hostmem_region *vmr;
>       VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
> +    VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
>       MemoryRegion *mr = res->mr;
>       int ret;
>   
> +#if VIRGL_CHECK_VERSION(1, 2, 1) && !defined(_WIN32)
> +    if (res->map_fixed) {
> +        res->map_fixed = mmap(res->map_fixed, res->base.blob_size, PROT_NONE,
> +                              MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED,
> +                              -1, 0);
> +        if (res->map_fixed == MAP_FAILED) {
> +            ret = -errno;
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "%s: failed to unmap(fixed) virgl resource: %s\n",
> +                          __func__, strerror(-ret));
> +            return ret;
> +        }
> +
> +        res->map_fixed = NULL;
> +    }
> +#endif
> +
>       if (!mr) {
>           return 0;
>       }
> @@ -202,7 +257,7 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
>   
>           /* 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);
> +        memory_region_del_subregion(gl->hostmem_mr, mr);
>           object_unparent(OBJECT(mr));
>       }
>   
> @@ -1270,9 +1325,22 @@ void virtio_gpu_virgl_reset_scanout(VirtIOGPU *g)
>   
>   void virtio_gpu_virgl_reset(VirtIOGPU *g)
>   {
> +#if VIRGL_CHECK_VERSION(1, 2, 1) && !defined(_WIN32)
> +    VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
> +    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
> +#endif
> +
>       virgl_renderer_reset();
>   
>       virtio_gpu_virgl_reset_async_fences(g);
> +
> +#if VIRGL_CHECK_VERSION(1, 2, 1) && !defined(_WIN32)
> +    if (gl->hostmem_mmap &&
> +        mmap(gl->hostmem_mmap, b->conf.hostmem, PROT_NONE,
> +             MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0) == MAP_FAILED) {
> +        error_report("failed to reset virgl hostmem: %s", strerror(errno));
> +    }
> +#endif

I think this is better to be done before virgl_renderer_reset() to avoid 
having dangling pages after virgl_renderer_reset() and before this 
mmap() call.

Regards,
Akihiko Odaki

>   }
>   
>   int virtio_gpu_virgl_init(VirtIOGPU *g)
> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> index 172f5ffce3ed..e1122edd6eeb 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_mr;
> +    void *hostmem_mmap;
>   };
>   
>   struct VhostUserGPU {

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

[Akihiko Odaki]

On 2025/11/13 11:52, Akihiko Odaki wrote:
> On 2025/11/13 8:31, 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.
>>
>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>> ---
>>   hw/display/virtio-gpu-gl.c     | 37 +++++++++++++++++
>>   hw/display/virtio-gpu-virgl.c  | 72 +++++++++++++++++++++++++++++++++-
>>   include/hw/virtio/virtio-gpu.h |  3 ++
>>   3 files changed, 110 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
>> index b640900fc6f1..e1481291948a 100644
>> --- a/hw/display/virtio-gpu-gl.c
>> +++ b/hw/display/virtio-gpu-gl.c
>> @@ -122,6 +122,9 @@ static void 
>> virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
>>   {
>>       ERRP_GUARD();
>>       VirtIOGPU *g = VIRTIO_GPU(qdev);
>> +    VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
>> +    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
> 
> Nitpick: this order is slightly odd as VirtIOGPUBase is the base class 
> of VirtIOGPU and VirtIOGPUGL. So let's do:
> 
> VirtIOGPUBase *b = VIRTIO_GPU_BASE(qdev); // super class of b
> VirtIOGPU *g = VIRTIO_GPU(qdev); // base class of gl
> VirtIOGPUGL *gl = VIRTIO_GPU_GL(qdev);
> 
> Arguments are unified to qdev for consistency with other functions.
> 
>> +    void *map;
>>   #if HOST_BIG_ENDIAN
>>       error_setg(errp, "virgl is not supported on bigendian platforms");
>> @@ -152,6 +155,31 @@ static void 
>> virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
>>   #endif
>>       virtio_gpu_device_realize(qdev, errp);
>> +
>> +    /*
>> +     * Check whether virtio_gpu_device_realize() failed.
>> +     */
>> +    if (!g->ctrl_bh) {
> 
> Instead, do:
> if (*errp) {
>      return;
> }
> 
> With this change it is clear that it checks whether 
> virtio_gpu_device_realize() failed so the comment will be unnecessary.
> 
>> +        return;
>> +    }
>> +
>> +    if (virtio_gpu_hostmem_enabled(b->conf)) {
>> +        map = mmap(NULL, b->conf.hostmem, PROT_NONE,

I'm concerned that mapping with PROT_NONE may allow the guest crash QEMU 
by accessing the hostmem region without blobs, especially with TCG (not 
sure about KVM).

Perhaps PROT_READ | PROT_WRITE may be a safe choice. It is ugly and lets 
the guest read and write garbage to the region without blobs, but at 
least avoids crashes.

>> +                   MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> 
> I think you need to check CONFIG_WIN32.
> 
>> +        if (map == MAP_FAILED) {
>> +            error_setg(errp,
>> +                       "virgl hostmem region could not be 
>> initialized: %s",
>> +                       strerror(errno));
>> +            return;
>> +        }
>> +
>> +        gl->hostmem_mmap = map;
>> +        gl->hostmem_mr = g_new0(MemoryRegion, 1);
> 
> Having "b->hostmem" and "gl->hostmem_mr" is rather confusing.
> 
>> +        memory_region_init_ram_ptr(gl->hostmem_mr, NULL, "hostmem-anon",
> 
> "anon" is misleading because virgl_renderer_resource_map_fixed() will 
> map non-anonymous pages.
> 
> Looking for an alternative word, I found docs/devel/memory.rst refers 
> this kind of memory region as "background".
> 
>> +                                   b->conf.hostmem, gl->hostmem_mmap);
>> +        memory_region_add_subregion(&b->hostmem, 0, gl->hostmem_mr);
>> +        memory_region_set_enabled(gl->hostmem_mr, true);
> 
> Calling memory_region_set_enabled() is unnecessary. I failed to point 
> out this for commit 7c092f17ccee ("virtio-gpu: Handle resource blob 
> commands")...
> 
>> +    }
>>   }
>>   static const Property virtio_gpu_gl_properties[] = {
>> @@ -167,6 +195,7 @@ static void 
>> virtio_gpu_gl_device_unrealize(DeviceState *qdev)
>>   {
>>       VirtIOGPU *g = VIRTIO_GPU(qdev);
>>       VirtIOGPUGL *gl = VIRTIO_GPU_GL(qdev);
>> +    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
>>       if (gl->renderer_state >= RS_INITED) {
>>   #if VIRGL_VERSION_MAJOR >= 1
>> @@ -187,6 +216,14 @@ static void 
>> virtio_gpu_gl_device_unrealize(DeviceState *qdev)
>>       gl->renderer_state = RS_START;
>>       g_array_unref(g->capset_ids);
>> +
>> +    if (gl->hostmem_mr) {
>> +        memory_region_set_enabled(gl->hostmem_mr, false);
> 
> This memory_region_set_enabled() is unnecessary either.
> 
>> +        memory_region_del_subregion(&b->hostmem, gl->hostmem_mr);
>> +    }
>> +    if (gl->hostmem_mmap) {
>> +        munmap(gl->hostmem_mmap, b->conf.hostmem);
>> +    }
> 
> Please remove munmap(). It is not guaranteed that the memory region will 
> be finalized immediately with memory_region_del_subregion(), so there 
> can be a remaining reference to gl->hostmem_mmap.
> 
> virtio-gpu is not hotpluggable so you don't have to worry about memory 
> leaks.
> 
>>   }
>>   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 5c9a9ee84392..d4188cf1d6b2 100644
>> --- a/hw/display/virtio-gpu-virgl.c
>> +++ b/hw/display/virtio-gpu-virgl.c
>> @@ -21,6 +21,7 @@
>>   #include "hw/virtio/virtio-gpu-pixman.h"
>>   #include "ui/egl-helpers.h"
>> +#include <stdio.h>
>>   #include <virglrenderer.h>
>> @@ -44,6 +45,7 @@
>>   struct virtio_gpu_virgl_resource {
>>       struct virtio_gpu_simple_resource base;
>>       MemoryRegion *mr;
>> +    void *map_fixed;
>>   };
>>   static struct virtio_gpu_virgl_resource *
>> @@ -116,6 +118,7 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
>>   {
>>       struct virtio_gpu_virgl_hostmem_region *vmr;
>>       VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
>> +    VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
>>       MemoryRegion *mr;
>>       uint64_t size;
>>       void *data;
>> @@ -126,6 +129,40 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
>>           return -EOPNOTSUPP;
>>       }
>> +#if VIRGL_CHECK_VERSION(1, 2, 1) && !defined(_WIN32)
> 
> Check CONFIG_WIN32 instead. scripts/checkpatch.pl has the following code:
> # check of hardware specific defines
> # we have e.g. CONFIG_LINUX and CONFIG_WIN32 for common cases
> # where they might be necessary.
>          if ($line =~ m@^.\s*\#\s*if.*\b__@) {
>              WARN("architecture specific defines should be avoided\n" .  
> $herecurr);
>          }
> 
> This combination of VIRGL_CHECK_VERSION(1, 2, 1) && ! 
> defined(CONFIG_WIN32) is rather frequent so perhaps defining a macro for 
> this may be useful.
> 
>> +    /*
>> +     * virgl_renderer_resource_map_fixed() allows to create multiple
>> +     * mappings of the same resource, while 
>> virgl_renderer_resource_map()
>> +     * not.
>> +     */
>> +    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;
>> +    }
>> +
>> +    /*
>> +     * -EOPNOTSUPP is returned if MAP_FIXED unsupported by this 
>> resource,
>> +     * mapping falls back to a blob subregion method in that case.
>> +     */
>> +    ret = virgl_renderer_resource_map_fixed(res->base.resource_id,
>> +                                            gl->hostmem_mmap + offset);
>> +    if (ret && ret != -EOPNOTSUPP) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "%s: failed to map(fixed) virgl resource: %s\n",
>> +                      __func__, strerror(-ret));
>> +        return ret;
>> +    }
>> +
>> +    if (ret == 0) {
>> +        fprintf(stderr, "virgl_renderer_resource_map_fixed\n");
> 
> I guess you forgot removing a debug log.
> 
>> +
>> +        res->map_fixed = gl->hostmem_mmap + offset;
>> +        return 0;
>> +    }
> 
> I would restructure these statements as follows:
> 
> ret = virgl_renderer_resource_map_fixed(...);
> switch (ret) {
> case 0:
>      ...
>      return 0;
> 
> case -EOPNOTSUPP:
>      /*
>       * -MAP_FIXED is unsupported by this resource.
>       * Mapping falls back to a blob subregion method in that case.
>       */
>      break;
> 
> default:
>      ...
>      return ret;
> }
> 
> ...so that you don't need to check ret before checking
> ret != -EOPNOTSUPP and what case the comment on the fallback applies.
> 
>> +#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",
>> @@ -138,7 +175,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(gl->hostmem_mr, offset, mr);
> 
> docs/devel/memory.rst says having a subregion in a RAM reion is not 
> preferred. Instead, memory_region_add_subregion_overlap() should be used 
> for hostmem_mr to make it "background".
> 
>>       memory_region_set_enabled(mr, true);
>>       /*
>> @@ -163,9 +200,27 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
>>   {
>>       struct virtio_gpu_virgl_hostmem_region *vmr;
>>       VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
>> +    VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
>>       MemoryRegion *mr = res->mr;
>>       int ret;
>> +#if VIRGL_CHECK_VERSION(1, 2, 1) && !defined(_WIN32)
>> +    if (res->map_fixed) {
>> +        res->map_fixed = mmap(res->map_fixed, res->base.blob_size, 
>> PROT_NONE,
>> +                              MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED,
>> +                              -1, 0);
>> +        if (res->map_fixed == MAP_FAILED) {
>> +            ret = -errno;
>> +            qemu_log_mask(LOG_GUEST_ERROR,
>> +                          "%s: failed to unmap(fixed) virgl resource: 
>> %s\n",
>> +                          __func__, strerror(-ret));
>> +            return ret;
>> +        }
>> +
>> +        res->map_fixed = NULL;
>> +    }
>> +#endif
>> +
>>       if (!mr) {
>>           return 0;
>>       }
>> @@ -202,7 +257,7 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
>>           /* 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);
>> +        memory_region_del_subregion(gl->hostmem_mr, mr);
>>           object_unparent(OBJECT(mr));
>>       }
>> @@ -1270,9 +1325,22 @@ void virtio_gpu_virgl_reset_scanout(VirtIOGPU *g)
>>   void virtio_gpu_virgl_reset(VirtIOGPU *g)
>>   {
>> +#if VIRGL_CHECK_VERSION(1, 2, 1) && !defined(_WIN32)
>> +    VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
>> +    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
>> +#endif
>> +
>>       virgl_renderer_reset();
>>       virtio_gpu_virgl_reset_async_fences(g);
>> +
>> +#if VIRGL_CHECK_VERSION(1, 2, 1) && !defined(_WIN32)
>> +    if (gl->hostmem_mmap &&
>> +        mmap(gl->hostmem_mmap, b->conf.hostmem, PROT_NONE,
>> +             MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0) == 
>> MAP_FAILED) {
>> +        error_report("failed to reset virgl hostmem: %s", 
>> strerror(errno));
>> +    }
>> +#endif
> 
> I think this is better to be done before virgl_renderer_reset() to avoid 
> having dangling pages after virgl_renderer_reset() and before this 
> mmap() call.
> 
> Regards,
> Akihiko Odaki
> 
>>   }
>>   int virtio_gpu_virgl_init(VirtIOGPU *g)
>> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/ 
>> virtio-gpu.h
>> index 172f5ffce3ed..e1122edd6eeb 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_mr;
>> +    void *hostmem_mmap;
>>   };
>>   struct VhostUserGPU {
> 

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

[Dmitry Osipenko]

Hello Akihiko,

On 11/13/25 09:51, Akihiko Odaki wrote:
> On 2025/11/13 11:52, Akihiko Odaki wrote:
>> On 2025/11/13 8:31, 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.
>>>
>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>>> ---
>>>   hw/display/virtio-gpu-gl.c     | 37 +++++++++++++++++
>>>   hw/display/virtio-gpu-virgl.c  | 72 +++++++++++++++++++++++++++++++++-
>>>   include/hw/virtio/virtio-gpu.h |  3 ++
>>>   3 files changed, 110 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
>>> index b640900fc6f1..e1481291948a 100644
>>> --- a/hw/display/virtio-gpu-gl.c
>>> +++ b/hw/display/virtio-gpu-gl.c
>>> @@ -122,6 +122,9 @@ static void
>>> virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
>>>   {
>>>       ERRP_GUARD();
>>>       VirtIOGPU *g = VIRTIO_GPU(qdev);
>>> +    VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
>>> +    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
>>
>> Nitpick: this order is slightly odd as VirtIOGPUBase is the base class
>> of VirtIOGPU and VirtIOGPUGL. So let's do:
>>
>> VirtIOGPUBase *b = VIRTIO_GPU_BASE(qdev); // super class of b
>> VirtIOGPU *g = VIRTIO_GPU(qdev); // base class of gl
>> VirtIOGPUGL *gl = VIRTIO_GPU_GL(qdev);
>>
>> Arguments are unified to qdev for consistency with other functions.
>>
>>> +    void *map;
>>>   #if HOST_BIG_ENDIAN
>>>       error_setg(errp, "virgl is not supported on bigendian platforms");
>>> @@ -152,6 +155,31 @@ static void
>>> virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
>>>   #endif
>>>       virtio_gpu_device_realize(qdev, errp);
>>> +
>>> +    /*
>>> +     * Check whether virtio_gpu_device_realize() failed.
>>> +     */
>>> +    if (!g->ctrl_bh) {
>>
>> Instead, do:
>> if (*errp) {
>>      return;
>> }
>>
>> With this change it is clear that it checks whether
>> virtio_gpu_device_realize() failed so the comment will be unnecessary.
>>
>>> +        return;
>>> +    }
>>> +
>>> +    if (virtio_gpu_hostmem_enabled(b->conf)) {
>>> +        map = mmap(NULL, b->conf.hostmem, PROT_NONE,
> 
> I'm concerned that mapping with PROT_NONE may allow the guest crash QEMU
> by accessing the hostmem region without blobs, especially with TCG (not
> sure about KVM).
> 
> Perhaps PROT_READ | PROT_WRITE may be a safe choice. It is ugly and lets
> the guest read and write garbage to the region without blobs, but at
> least avoids crashes.

Thanks a lot for a quick and thorough review, very appreciate. Will test
how KVM behaves when accessing PROT_NONE and address TCG + rest of the
comments.

-- 
Best regards,
Dmitry

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

[Dmitry Osipenko]

On 11/13/25 15:16, Dmitry Osipenko wrote:
> Hello Akihiko,
> 
> On 11/13/25 09:51, Akihiko Odaki wrote:
>> On 2025/11/13 11:52, Akihiko Odaki wrote:
>>> On 2025/11/13 8:31, 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.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>>>> ---
>>>>   hw/display/virtio-gpu-gl.c     | 37 +++++++++++++++++
>>>>   hw/display/virtio-gpu-virgl.c  | 72 +++++++++++++++++++++++++++++++++-
>>>>   include/hw/virtio/virtio-gpu.h |  3 ++
>>>>   3 files changed, 110 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
>>>> index b640900fc6f1..e1481291948a 100644
>>>> --- a/hw/display/virtio-gpu-gl.c
>>>> +++ b/hw/display/virtio-gpu-gl.c
>>>> @@ -122,6 +122,9 @@ static void
>>>> virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
>>>>   {
>>>>       ERRP_GUARD();
>>>>       VirtIOGPU *g = VIRTIO_GPU(qdev);
>>>> +    VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
>>>> +    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
>>>
>>> Nitpick: this order is slightly odd as VirtIOGPUBase is the base class
>>> of VirtIOGPU and VirtIOGPUGL. So let's do:
>>>
>>> VirtIOGPUBase *b = VIRTIO_GPU_BASE(qdev); // super class of b
>>> VirtIOGPU *g = VIRTIO_GPU(qdev); // base class of gl
>>> VirtIOGPUGL *gl = VIRTIO_GPU_GL(qdev);
>>>
>>> Arguments are unified to qdev for consistency with other functions.
>>>
>>>> +    void *map;
>>>>   #if HOST_BIG_ENDIAN
>>>>       error_setg(errp, "virgl is not supported on bigendian platforms");
>>>> @@ -152,6 +155,31 @@ static void
>>>> virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
>>>>   #endif
>>>>       virtio_gpu_device_realize(qdev, errp);
>>>> +
>>>> +    /*
>>>> +     * Check whether virtio_gpu_device_realize() failed.
>>>> +     */
>>>> +    if (!g->ctrl_bh) {
>>>
>>> Instead, do:
>>> if (*errp) {
>>>      return;
>>> }
>>>
>>> With this change it is clear that it checks whether
>>> virtio_gpu_device_realize() failed so the comment will be unnecessary.
>>>
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    if (virtio_gpu_hostmem_enabled(b->conf)) {
>>>> +        map = mmap(NULL, b->conf.hostmem, PROT_NONE,
>>
>> I'm concerned that mapping with PROT_NONE may allow the guest crash QEMU
>> by accessing the hostmem region without blobs, especially with TCG (not
>> sure about KVM).
>>
>> Perhaps PROT_READ | PROT_WRITE may be a safe choice. It is ugly and lets
>> the guest read and write garbage to the region without blobs, but at
>> least avoids crashes.
> 
> Thanks a lot for a quick and thorough review, very appreciate. Will test
> how KVM behaves when accessing PROT_NONE and address TCG + rest of the
> comments.

KVM faults and terminates QEMU. Both KVM and TCG shouldn't use
PROT_NONE, good catch.

-- 
Best regards,
Dmitry