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 <[email protected]>
---
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 {
