* [RFC PATCH v3 0/7] Support mapping virtio-gpu virgl hostmem blobs using MAP_FIXED API
@ 2025-11-20 4:06 Dmitry Osipenko
2025-11-20 4:06 ` [RFC PATCH v3 1/7] virtio-gpu: Remove superfluous memory_region_set_enabled() Dmitry Osipenko
` (6 more replies)
0 siblings, 7 replies; 15+ messages in thread
From: Dmitry Osipenko @ 2025-11-20 4:06 UTC (permalink / raw)
To: Akihiko Odaki, Huang Rui, Marc-André Lureau,
Philippe Mathieu-Daudé, Gerd Hoffmann, Alex Bennée,
Pierre-Eric Pelloux-Prayer, Michael S . Tsirkin, Paolo Bonzini,
Yiwei Zhang, Sergio Lopez Pascual
Cc: Gert Wollny, qemu-devel, Gurchetan Singh, Alyssa Ross,
Roger Pau Monné, Alex Deucher, Stefano Stabellini,
Christian König, Xenia Ragiadakou, Honglei Huang,
Julia Zhang, Chen Jiqian, Rob Clark, Robert Beckett
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:
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: Make virtio_gpu_virgl_unmap_resource_blob() return -1 on
error
virtio-gpu: Blame host for virgl blob resource unmapping 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 | 51 ++++++++++-
hw/display/virtio-gpu-virgl.c | 159 ++++++++++++++++++++++++++-------
include/hw/virtio/virtio-gpu.h | 8 +-
3 files changed, 183 insertions(+), 35 deletions(-)
--
2.51.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* [RFC PATCH v3 1/7] virtio-gpu: Remove superfluous memory_region_set_enabled()
2025-11-20 4:06 [RFC PATCH v3 0/7] Support mapping virtio-gpu virgl hostmem blobs using MAP_FIXED API Dmitry Osipenko
@ 2025-11-20 4:06 ` Dmitry Osipenko
2025-11-20 4:06 ` [RFC PATCH v3 2/7] virtio-gpu: Validate hostmem mapping offset Dmitry Osipenko
` (5 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: Dmitry Osipenko @ 2025-11-20 4:06 UTC (permalink / raw)
To: Akihiko Odaki, Huang Rui, Marc-André Lureau,
Philippe Mathieu-Daudé, Gerd Hoffmann, Alex Bennée,
Pierre-Eric Pelloux-Prayer, Michael S . Tsirkin, Paolo Bonzini,
Yiwei Zhang, Sergio Lopez Pascual
Cc: Gert Wollny, qemu-devel, Gurchetan Singh, Alyssa Ross,
Roger Pau Monné, Alex Deucher, Stefano Stabellini,
Christian König, Xenia Ragiadakou, Honglei Huang,
Julia Zhang, Chen Jiqian, Rob Clark, Robert Beckett
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [RFC PATCH v3 2/7] virtio-gpu: Validate hostmem mapping offset
2025-11-20 4:06 [RFC PATCH v3 0/7] Support mapping virtio-gpu virgl hostmem blobs using MAP_FIXED API Dmitry Osipenko
2025-11-20 4:06 ` [RFC PATCH v3 1/7] virtio-gpu: Remove superfluous memory_region_set_enabled() Dmitry Osipenko
@ 2025-11-20 4:06 ` Dmitry Osipenko
2025-11-20 5:18 ` Akihiko Odaki
2025-11-20 4:06 ` [RFC PATCH v3 3/7] virtio-gpu: Make virtio_gpu_virgl_unmap_resource_blob() return -1 on error Dmitry Osipenko
` (4 subsequent siblings)
6 siblings, 1 reply; 15+ messages in thread
From: Dmitry Osipenko @ 2025-11-20 4:06 UTC (permalink / raw)
To: Akihiko Odaki, Huang Rui, Marc-André Lureau,
Philippe Mathieu-Daudé, Gerd Hoffmann, Alex Bennée,
Pierre-Eric Pelloux-Prayer, Michael S . Tsirkin, Paolo Bonzini,
Yiwei Zhang, Sergio Lopez Pascual
Cc: Gert Wollny, qemu-devel, Gurchetan Singh, Alyssa Ross,
Roger Pau Monné, Alex Deucher, Stefano Stabellini,
Christian König, Xenia Ragiadakou, Honglei Huang,
Julia Zhang, Chen Jiqian, Rob Clark, Robert Beckett
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..cf1da4b79626 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 -1;
+ }
+
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [RFC PATCH v3 3/7] virtio-gpu: Make virtio_gpu_virgl_unmap_resource_blob() return -1 on error
2025-11-20 4:06 [RFC PATCH v3 0/7] Support mapping virtio-gpu virgl hostmem blobs using MAP_FIXED API Dmitry Osipenko
2025-11-20 4:06 ` [RFC PATCH v3 1/7] virtio-gpu: Remove superfluous memory_region_set_enabled() Dmitry Osipenko
2025-11-20 4:06 ` [RFC PATCH v3 2/7] virtio-gpu: Validate hostmem mapping offset Dmitry Osipenko
@ 2025-11-20 4:06 ` Dmitry Osipenko
2025-11-20 4:06 ` [RFC PATCH v3 4/7] virtio-gpu: Blame host for virgl blob resource unmapping error Dmitry Osipenko
` (3 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: Dmitry Osipenko @ 2025-11-20 4:06 UTC (permalink / raw)
To: Akihiko Odaki, Huang Rui, Marc-André Lureau,
Philippe Mathieu-Daudé, Gerd Hoffmann, Alex Bennée,
Pierre-Eric Pelloux-Prayer, Michael S . Tsirkin, Paolo Bonzini,
Yiwei Zhang, Sergio Lopez Pascual
Cc: Gert Wollny, qemu-devel, Gurchetan Singh, Alyssa Ross,
Roger Pau Monné, Alex Deucher, Stefano Stabellini,
Christian König, Xenia Ragiadakou, Honglei Huang,
Julia Zhang, Chen Jiqian, Rob Clark, Robert Beckett
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 cf1da4b79626..bc22763cc931 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;
}
@@ -820,7 +820,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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [RFC PATCH v3 4/7] virtio-gpu: Blame host for virgl blob resource unmapping error
2025-11-20 4:06 [RFC PATCH v3 0/7] Support mapping virtio-gpu virgl hostmem blobs using MAP_FIXED API Dmitry Osipenko
` (2 preceding siblings ...)
2025-11-20 4:06 ` [RFC PATCH v3 3/7] virtio-gpu: Make virtio_gpu_virgl_unmap_resource_blob() return -1 on error Dmitry Osipenko
@ 2025-11-20 4:06 ` Dmitry Osipenko
2025-11-20 5:23 ` Akihiko Odaki
2025-11-20 4:06 ` [RFC PATCH v3 5/7] virtio-gpu: Destroy virgl resources on virtio-gpu reset Dmitry Osipenko
` (2 subsequent siblings)
6 siblings, 1 reply; 15+ messages in thread
From: Dmitry Osipenko @ 2025-11-20 4:06 UTC (permalink / raw)
To: Akihiko Odaki, Huang Rui, Marc-André Lureau,
Philippe Mathieu-Daudé, Gerd Hoffmann, Alex Bennée,
Pierre-Eric Pelloux-Prayer, Michael S . Tsirkin, Paolo Bonzini,
Yiwei Zhang, Sergio Lopez Pascual
Cc: Gert Wollny, qemu-devel, Gurchetan Singh, Alyssa Ross,
Roger Pau Monné, Alex Deucher, Stefano Stabellini,
Christian König, Xenia Ragiadakou, Honglei Huang,
Julia Zhang, Chen Jiqian, Rob Clark, Robert Beckett
Report virgl blob resource unmapping error as a host error since it's
host's fault.
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
hw/display/virtio-gpu-virgl.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index bc22763cc931..ae8de8ee47da 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -196,9 +196,8 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
ret = virgl_renderer_resource_unmap(res->base.resource_id);
if (ret) {
- qemu_log_mask(LOG_GUEST_ERROR,
- "%s: failed to unmap virgl resource: %s\n",
- __func__, strerror(-ret));
+ error_report("%s: failed to unmap virgl resource: %s",
+ __func__, strerror(-ret));
return -1;
}
} else {
--
2.51.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [RFC PATCH v3 5/7] virtio-gpu: Destroy virgl resources on virtio-gpu reset
2025-11-20 4:06 [RFC PATCH v3 0/7] Support mapping virtio-gpu virgl hostmem blobs using MAP_FIXED API Dmitry Osipenko
` (3 preceding siblings ...)
2025-11-20 4:06 ` [RFC PATCH v3 4/7] virtio-gpu: Blame host for virgl blob resource unmapping error Dmitry Osipenko
@ 2025-11-20 4:06 ` Dmitry Osipenko
2025-11-20 5:07 ` Akihiko Odaki
2025-11-20 4:06 ` [RFC PATCH v3 6/7] virtio-gpu: Make virtio_gpu_virgl_init() return -1 on error Dmitry Osipenko
2025-11-20 4:06 ` [RFC PATCH v3 7/7] virtio-gpu: Support mapping hostmem blobs with map_fixed Dmitry Osipenko
6 siblings, 1 reply; 15+ messages in thread
From: Dmitry Osipenko @ 2025-11-20 4:06 UTC (permalink / raw)
To: Akihiko Odaki, Huang Rui, Marc-André Lureau,
Philippe Mathieu-Daudé, Gerd Hoffmann, Alex Bennée,
Pierre-Eric Pelloux-Prayer, Michael S . Tsirkin, Paolo Bonzini,
Yiwei Zhang, Sergio Lopez Pascual
Cc: Gert Wollny, qemu-devel, Gurchetan Singh, Alyssa Ross,
Roger Pau Monné, Alex Deucher, Stefano Stabellini,
Christian König, Xenia Ragiadakou, Honglei Huang,
Julia Zhang, Chen Jiqian, Rob Clark, Robert Beckett
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 | 79 +++++++++++++++++++++++++---------
include/hw/virtio/virtio-gpu.h | 5 ++-
3 files changed, 67 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 ae8de8ee47da..24d329022da9 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -311,14 +311,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);
@@ -331,27 +361,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,
@@ -1270,11 +1294,24 @@ 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;
+
+ QTAILQ_FOREACH_SAFE(res, &g->reslist, next, tmp) {
+ virtio_gpu_virgl_resource_destroy(g, res, NULL);
+ }
+
+ if (!QTAILQ_EMPTY(&g->reslist)) {
+ error_report("failed to reset virgl resources");
+ 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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [RFC PATCH v3 6/7] virtio-gpu: Make virtio_gpu_virgl_init() return -1 on error
2025-11-20 4:06 [RFC PATCH v3 0/7] Support mapping virtio-gpu virgl hostmem blobs using MAP_FIXED API Dmitry Osipenko
` (4 preceding siblings ...)
2025-11-20 4:06 ` [RFC PATCH v3 5/7] virtio-gpu: Destroy virgl resources on virtio-gpu reset Dmitry Osipenko
@ 2025-11-20 4:06 ` Dmitry Osipenko
2025-11-20 4:06 ` [RFC PATCH v3 7/7] virtio-gpu: Support mapping hostmem blobs with map_fixed Dmitry Osipenko
6 siblings, 0 replies; 15+ messages in thread
From: Dmitry Osipenko @ 2025-11-20 4:06 UTC (permalink / raw)
To: Akihiko Odaki, Huang Rui, Marc-André Lureau,
Philippe Mathieu-Daudé, Gerd Hoffmann, Alex Bennée,
Pierre-Eric Pelloux-Prayer, Michael S . Tsirkin, Paolo Bonzini,
Yiwei Zhang, Sergio Lopez Pascual
Cc: Gert Wollny, qemu-devel, Gurchetan Singh, Alyssa Ross,
Roger Pau Monné, Alex Deucher, Stefano Stabellini,
Christian König, Xenia Ragiadakou, Honglei Huang,
Julia Zhang, Chen Jiqian, Rob Clark, Robert Beckett
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 24d329022da9..35201d629b9c 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -1357,7 +1357,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
@@ -1365,7 +1365,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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [RFC PATCH v3 7/7] virtio-gpu: Support mapping hostmem blobs with map_fixed
2025-11-20 4:06 [RFC PATCH v3 0/7] Support mapping virtio-gpu virgl hostmem blobs using MAP_FIXED API Dmitry Osipenko
` (5 preceding siblings ...)
2025-11-20 4:06 ` [RFC PATCH v3 6/7] virtio-gpu: Make virtio_gpu_virgl_init() return -1 on error Dmitry Osipenko
@ 2025-11-20 4:06 ` Dmitry Osipenko
2025-11-20 5:14 ` Akihiko Odaki
6 siblings, 1 reply; 15+ messages in thread
From: Dmitry Osipenko @ 2025-11-20 4:06 UTC (permalink / raw)
To: Akihiko Odaki, Huang Rui, Marc-André Lureau,
Philippe Mathieu-Daudé, Gerd Hoffmann, Alex Bennée,
Pierre-Eric Pelloux-Prayer, Michael S . Tsirkin, Paolo Bonzini,
Yiwei Zhang, Sergio Lopez Pascual
Cc: Gert Wollny, qemu-devel, Gurchetan Singh, Alyssa Ross,
Roger Pau Monné, Alex Deucher, Stefano Stabellini,
Christian König, Xenia Ragiadakou, Honglei Huang,
Julia Zhang, Chen Jiqian, Rob Clark, Robert Beckett
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 | 43 ++++++++++++++++++++++++-
hw/display/virtio-gpu-virgl.c | 57 +++++++++++++++++++++++++++++++++-
include/hw/virtio/virtio-gpu.h | 3 ++
3 files changed, 101 insertions(+), 2 deletions(-)
diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
index d65da4863923..097f3ce4cb71 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,30 @@ 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 (*errp) {
+ return;
+ }
+
+ 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);
+ }
}
static const Property virtio_gpu_gl_properties[] = {
@@ -190,6 +220,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 35201d629b9c..2bd0ed6227cf 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -41,9 +41,12 @@
VIRGL_VERSION_MICRO >= (micro))
#endif
+#define VIRGL_HAS_MAP_FIXED VIRGL_CHECK_VERSION(1, 2, 1)
+
struct virtio_gpu_virgl_resource {
struct virtio_gpu_simple_resource base;
MemoryRegion *mr;
+ void *map_fixed;
};
static struct virtio_gpu_virgl_resource *
@@ -116,6 +119,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 +140,41 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
return -1;
}
+#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 -1;
+ }
+
+ 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 +187,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 +214,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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [RFC PATCH v3 5/7] virtio-gpu: Destroy virgl resources on virtio-gpu reset
2025-11-20 4:06 ` [RFC PATCH v3 5/7] virtio-gpu: Destroy virgl resources on virtio-gpu reset Dmitry Osipenko
@ 2025-11-20 5:07 ` Akihiko Odaki
2025-11-20 14:54 ` Dmitry Osipenko
0 siblings, 1 reply; 15+ messages in thread
From: Akihiko Odaki @ 2025-11-20 5:07 UTC (permalink / raw)
To: Dmitry Osipenko, Huang Rui, Marc-André Lureau,
Philippe Mathieu-Daudé, Gerd Hoffmann, Alex Bennée,
Pierre-Eric Pelloux-Prayer, Michael S . Tsirkin, Paolo Bonzini,
Yiwei Zhang, Sergio Lopez Pascual
Cc: Gert Wollny, qemu-devel, Gurchetan Singh, Alyssa Ross,
Roger Pau Monné, Alex Deucher, Stefano Stabellini,
Christian König, Xenia Ragiadakou, Honglei Huang,
Julia Zhang, Chen Jiqian, Rob Clark, Robert Beckett
On 2025/11/20 13:06, Dmitry Osipenko wrote:
> Properly destroy virgl resources on virtio-gpu reset to not leak resources
> on a hot reboot of a VM.
>
> Suggested-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---
> hw/display/virtio-gpu-gl.c | 6 ++-
> hw/display/virtio-gpu-virgl.c | 79 +++++++++++++++++++++++++---------
> include/hw/virtio/virtio-gpu.h | 5 ++-
> 3 files changed, 67 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 ae8de8ee47da..24d329022da9 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -311,14 +311,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);
> @@ -331,27 +361,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,
> @@ -1270,11 +1294,24 @@ 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;
> +
> + QTAILQ_FOREACH_SAFE(res, &g->reslist, next, tmp) {
> + virtio_gpu_virgl_resource_destroy(g, res, NULL);
> + }
> +
> + if (!QTAILQ_EMPTY(&g->reslist)) {
> + error_report("failed to reset virgl resources");
> + return -1;
It shouldn't report an error if suspended.
> + }
> +
> 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH v3 7/7] virtio-gpu: Support mapping hostmem blobs with map_fixed
2025-11-20 4:06 ` [RFC PATCH v3 7/7] virtio-gpu: Support mapping hostmem blobs with map_fixed Dmitry Osipenko
@ 2025-11-20 5:14 ` Akihiko Odaki
2025-11-20 14:19 ` Dmitry Osipenko
0 siblings, 1 reply; 15+ messages in thread
From: Akihiko Odaki @ 2025-11-20 5:14 UTC (permalink / raw)
To: Dmitry Osipenko, Huang Rui, Marc-André Lureau,
Philippe Mathieu-Daudé, Gerd Hoffmann, Alex Bennée,
Pierre-Eric Pelloux-Prayer, Michael S . Tsirkin, Paolo Bonzini,
Yiwei Zhang, Sergio Lopez Pascual
Cc: Gert Wollny, qemu-devel, Gurchetan Singh, Alyssa Ross,
Roger Pau Monné, Alex Deucher, Stefano Stabellini,
Christian König, Xenia Ragiadakou, Honglei Huang,
Julia Zhang, Chen Jiqian, Rob Clark, Robert Beckett
On 2025/11/20 13:06, 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 | 43 ++++++++++++++++++++++++-
> hw/display/virtio-gpu-virgl.c | 57 +++++++++++++++++++++++++++++++++-
> include/hw/virtio/virtio-gpu.h | 3 ++
> 3 files changed, 101 insertions(+), 2 deletions(-)
>
> diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
> index d65da4863923..097f3ce4cb71 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,30 @@ 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.
> + */
It is obvious so this comment is unnecessary.
> + if (*errp) {
> + return;
> + }
> +
> + if (virtio_gpu_hostmem_enabled(b->conf)) {
> + map = qemu_ram_mmap(-1, b->conf.hostmem, qemu_real_host_page_size(),
> + 0, 0);
qemu_ram_mmap() shouldn't be called if !defined(CONFIG_WIN32).
> + 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);
> + }
> }
>
> static const Property virtio_gpu_gl_properties[] = {
> @@ -190,6 +220,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 35201d629b9c..2bd0ed6227cf 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -41,9 +41,12 @@
> VIRGL_VERSION_MICRO >= (micro))
> #endif
>
> +#define VIRGL_HAS_MAP_FIXED VIRGL_CHECK_VERSION(1, 2, 1)
> +
> struct virtio_gpu_virgl_resource {
> struct virtio_gpu_simple_resource base;
> MemoryRegion *mr;
> + void *map_fixed;
> };
>
> static struct virtio_gpu_virgl_resource *
> @@ -116,6 +119,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 +140,41 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
> return -1;
> }
>
> +#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 -1;
> + }
> +
> + 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 +187,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 +214,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 {
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH v3 2/7] virtio-gpu: Validate hostmem mapping offset
2025-11-20 4:06 ` [RFC PATCH v3 2/7] virtio-gpu: Validate hostmem mapping offset Dmitry Osipenko
@ 2025-11-20 5:18 ` Akihiko Odaki
0 siblings, 0 replies; 15+ messages in thread
From: Akihiko Odaki @ 2025-11-20 5:18 UTC (permalink / raw)
To: Dmitry Osipenko, Huang Rui, Marc-André Lureau,
Philippe Mathieu-Daudé, Gerd Hoffmann, Alex Bennée,
Pierre-Eric Pelloux-Prayer, Michael S . Tsirkin, Paolo Bonzini,
Yiwei Zhang, Sergio Lopez Pascual
Cc: Gert Wollny, qemu-devel, Gurchetan Singh, Alyssa Ross,
Roger Pau Monné, Alex Deucher, Stefano Stabellini,
Christian König, Xenia Ragiadakou, Honglei Huang,
Julia Zhang, Chen Jiqian, Rob Clark, Robert Beckett
On 2025/11/20 13:06, Dmitry Osipenko wrote:
> 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..cf1da4b79626 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 -1;
The last returned error is -EOPNOTSUPP but this returns -1, which does
not look right.
QEMU should also return VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER if this
check fails.
> + }
> +
> 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",
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH v3 4/7] virtio-gpu: Blame host for virgl blob resource unmapping error
2025-11-20 4:06 ` [RFC PATCH v3 4/7] virtio-gpu: Blame host for virgl blob resource unmapping error Dmitry Osipenko
@ 2025-11-20 5:23 ` Akihiko Odaki
0 siblings, 0 replies; 15+ messages in thread
From: Akihiko Odaki @ 2025-11-20 5:23 UTC (permalink / raw)
To: Dmitry Osipenko, Huang Rui, Marc-André Lureau,
Philippe Mathieu-Daudé, Gerd Hoffmann, Alex Bennée,
Pierre-Eric Pelloux-Prayer, Michael S . Tsirkin, Paolo Bonzini,
Yiwei Zhang, Sergio Lopez Pascual
Cc: Gert Wollny, qemu-devel, Gurchetan Singh, Alyssa Ross,
Roger Pau Monné, Alex Deucher, Stefano Stabellini,
Christian König, Xenia Ragiadakou, Honglei Huang,
Julia Zhang, Chen Jiqian, Rob Clark, Robert Beckett
On 2025/11/20 13:06, Dmitry Osipenko wrote:
> Report virgl blob resource unmapping error as a host error since it's
> host's fault.
>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---
> hw/display/virtio-gpu-virgl.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index bc22763cc931..ae8de8ee47da 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -196,9 +196,8 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
>
> ret = virgl_renderer_resource_unmap(res->base.resource_id);
> if (ret) {
> - qemu_log_mask(LOG_GUEST_ERROR,
> - "%s: failed to unmap virgl resource: %s\n",
> - __func__, strerror(-ret));
> + error_report("%s: failed to unmap virgl resource: %s",
> + __func__, strerror(-ret));
I'm not sure about this. Unlike plain mmap(), virglrenderer *may* return
an error because of the current state it has. Skimming the virglrenderer
code it apparently does not happen, but there is no guarantee.
I also checked crosvm but it didn't help; it don't distinguish guest and
host errors.
Regards,
Akihiko Odaki
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH v3 7/7] virtio-gpu: Support mapping hostmem blobs with map_fixed
2025-11-20 5:14 ` Akihiko Odaki
@ 2025-11-20 14:19 ` Dmitry Osipenko
0 siblings, 0 replies; 15+ messages in thread
From: Dmitry Osipenko @ 2025-11-20 14:19 UTC (permalink / raw)
To: Akihiko Odaki, Huang Rui, Marc-André Lureau,
Philippe Mathieu-Daudé, Gerd Hoffmann, Alex Bennée,
Pierre-Eric Pelloux-Prayer, Michael S . Tsirkin, Paolo Bonzini,
Yiwei Zhang, Sergio Lopez Pascual
Cc: Gert Wollny, qemu-devel, Gurchetan Singh, Alyssa Ross,
Roger Pau Monné, Alex Deucher, Stefano Stabellini,
Christian König, Xenia Ragiadakou, Honglei Huang,
Julia Zhang, Chen Jiqian, Rob Clark, Robert Beckett
On 11/20/25 08:14, Akihiko Odaki wrote:
>> + if (virtio_gpu_hostmem_enabled(b->conf)) {
>> + map = qemu_ram_mmap(-1, b->conf.hostmem,
>> qemu_real_host_page_size(),
>> + 0, 0);
>
> qemu_ram_mmap() shouldn't be called if !defined(CONFIG_WIN32).
Indeed, see now that mmap-alloc.c not compiled for Windows target.
Though, it appears to me that Cygwin provides a compatible mmap()
implementation that supports MAP_FIXED, thus the mapping likely should
work on Windows.
Will re-add !defined(CONFIG_WIN32) and address rest of the comments.
Thanks a lot for the review.
--
Best regards,
Dmitry
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH v3 5/7] virtio-gpu: Destroy virgl resources on virtio-gpu reset
2025-11-20 5:07 ` Akihiko Odaki
@ 2025-11-20 14:54 ` Dmitry Osipenko
2025-11-20 15:02 ` Dmitry Osipenko
0 siblings, 1 reply; 15+ messages in thread
From: Dmitry Osipenko @ 2025-11-20 14:54 UTC (permalink / raw)
To: Akihiko Odaki, Huang Rui, Marc-André Lureau,
Philippe Mathieu-Daudé, Gerd Hoffmann, Alex Bennée,
Pierre-Eric Pelloux-Prayer, Michael S . Tsirkin, Paolo Bonzini,
Yiwei Zhang, Sergio Lopez Pascual
Cc: Gert Wollny, qemu-devel, Gurchetan Singh, Alyssa Ross,
Roger Pau Monné, Alex Deucher, Stefano Stabellini,
Christian König, Xenia Ragiadakou, Honglei Huang,
Julia Zhang, Chen Jiqian, Rob Clark, Robert Beckett
On 11/20/25 08:07, Akihiko Odaki wrote:
>> +int virtio_gpu_virgl_reset(VirtIOGPU *g)
>> {
>> + struct virtio_gpu_simple_resource *res, *tmp;
>> +
>> + QTAILQ_FOREACH_SAFE(res, &g->reslist, next, tmp) {
>> + virtio_gpu_virgl_resource_destroy(g, res, NULL);
>> + }
>> +
>> + if (!QTAILQ_EMPTY(&g->reslist)) {
>> + error_report("failed to reset virgl resources");
>> + return -1;
>
> It shouldn't report an error if suspended.
Could you please expand on what you're meaning by "suspended"?
Suppose you're talking about guest's kernel suspension. There was a
discussion on [1] RE a need to use `x-pcie-pm-no-soft-reset=true` option
to avoid virtio-gpu resetting across S3 suspend-resume. This option
works with virtio-vga, but not with virtio-vga-gl device where VM hangs
on startup. So currently S3 doesn't work for virgl and needs further fixing.
[1]
https://lore.kernel.org/dri-devel/20250924194755.1265531-1-dongwon.kim@intel.com/
--
Best regards,
Dmitry
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH v3 5/7] virtio-gpu: Destroy virgl resources on virtio-gpu reset
2025-11-20 14:54 ` Dmitry Osipenko
@ 2025-11-20 15:02 ` Dmitry Osipenko
0 siblings, 0 replies; 15+ messages in thread
From: Dmitry Osipenko @ 2025-11-20 15:02 UTC (permalink / raw)
To: Akihiko Odaki, Huang Rui, Marc-André Lureau,
Philippe Mathieu-Daudé, Gerd Hoffmann, Alex Bennée,
Pierre-Eric Pelloux-Prayer, Michael S . Tsirkin, Paolo Bonzini,
Yiwei Zhang, Sergio Lopez Pascual
Cc: Gert Wollny, qemu-devel, Gurchetan Singh, Alyssa Ross,
Roger Pau Monné, Alex Deucher, Stefano Stabellini,
Christian König, Xenia Ragiadakou, Honglei Huang,
Julia Zhang, Chen Jiqian, Rob Clark, Robert Beckett
On 11/20/25 17:54, Dmitry Osipenko wrote:
> On 11/20/25 08:07, Akihiko Odaki wrote:
>>> +int virtio_gpu_virgl_reset(VirtIOGPU *g)
>>> {
>>> + struct virtio_gpu_simple_resource *res, *tmp;
>>> +
>>> + QTAILQ_FOREACH_SAFE(res, &g->reslist, next, tmp) {
>>> + virtio_gpu_virgl_resource_destroy(g, res, NULL);
>>> + }
>>> +
>>> + if (!QTAILQ_EMPTY(&g->reslist)) {
>>> + error_report("failed to reset virgl resources");
>>> + return -1;
>>
>> It shouldn't report an error if suspended.
>
> Could you please expand on what you're meaning by "suspended"?
>
> Suppose you're talking about guest's kernel suspension. There was a
> discussion on [1] RE a need to use `x-pcie-pm-no-soft-reset=true` option
> to avoid virtio-gpu resetting across S3 suspend-resume. This option
> works with virtio-vga, but not with virtio-vga-gl device where VM hangs
> on startup. So currently S3 doesn't work for virgl and needs further fixing.
>
> [1]
> https://lore.kernel.org/dri-devel/20250924194755.1265531-1-dongwon.kim@intel.com/
Correction: `x-pcie-pm-no-soft-reset=true` works with virgl without
enabled hostmem. It's hostmem that doesn't work with the additional PCI bus.
--
Best regards,
Dmitry
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2025-11-20 15:03 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-11-20 4:06 [RFC PATCH v3 0/7] Support mapping virtio-gpu virgl hostmem blobs using MAP_FIXED API Dmitry Osipenko
2025-11-20 4:06 ` [RFC PATCH v3 1/7] virtio-gpu: Remove superfluous memory_region_set_enabled() Dmitry Osipenko
2025-11-20 4:06 ` [RFC PATCH v3 2/7] virtio-gpu: Validate hostmem mapping offset Dmitry Osipenko
2025-11-20 5:18 ` Akihiko Odaki
2025-11-20 4:06 ` [RFC PATCH v3 3/7] virtio-gpu: Make virtio_gpu_virgl_unmap_resource_blob() return -1 on error Dmitry Osipenko
2025-11-20 4:06 ` [RFC PATCH v3 4/7] virtio-gpu: Blame host for virgl blob resource unmapping error Dmitry Osipenko
2025-11-20 5:23 ` Akihiko Odaki
2025-11-20 4:06 ` [RFC PATCH v3 5/7] virtio-gpu: Destroy virgl resources on virtio-gpu reset Dmitry Osipenko
2025-11-20 5:07 ` Akihiko Odaki
2025-11-20 14:54 ` Dmitry Osipenko
2025-11-20 15:02 ` Dmitry Osipenko
2025-11-20 4:06 ` [RFC PATCH v3 6/7] virtio-gpu: Make virtio_gpu_virgl_init() return -1 on error Dmitry Osipenko
2025-11-20 4:06 ` [RFC PATCH v3 7/7] virtio-gpu: Support mapping hostmem blobs with map_fixed Dmitry Osipenko
2025-11-20 5:14 ` Akihiko Odaki
2025-11-20 14:19 ` Dmitry Osipenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).