On 12/1/25 14:57, Akihiko Odaki wrote: > On 2025/11/30 13:09, 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 <[email protected]> >> Signed-off-by: Dmitry Osipenko <[email protected]> >> --- >> hw/display/virtio-gpu-gl.c | 6 ++- >> hw/display/virtio-gpu-virgl.c | 87 ++++++++++++++++++++++++++-------- >> include/hw/virtio/virtio-gpu.h | 5 +- >> 3 files changed, 75 insertions(+), 23 deletions(-) >> >> diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c >> index b640900fc6f1..bf3fd75e9e6b 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)) { >> + 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 6a2aac0b6e5c..60d8fbf0445c 100644 >> --- a/hw/display/virtio-gpu-virgl.c >> +++ b/hw/display/virtio-gpu-virgl.c >> @@ -304,14 +304,46 @@ static void >> virgl_cmd_create_resource_3d(VirtIOGPU *g, >> virgl_renderer_resource_create(&args, NULL, 0); >> } >> +static int >> +virtio_gpu_virgl_resource_unref(VirtIOGPU *g, >> + struct virtio_gpu_virgl_resource *res, >> + bool *cmd_suspended) >> +{ >> + struct iovec *res_iovs = NULL; >> + int num_iovs = 0; >> +#if VIRGL_VERSION_MAJOR >= 1 >> + int ret; >> + >> + ret = virtio_gpu_virgl_unmap_resource_blob(g, res, cmd_suspended); >> + if (ret) { >> + return ret; >> + } >> + 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); >> @@ -324,27 +356,21 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g, >> return; >> } >> -#if VIRGL_VERSION_MAJOR >= 1 >> - if (virtio_gpu_virgl_unmap_resource_blob(g, res, cmd_suspended)) { >> - cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC; >> - return; >> - } >> - if (*cmd_suspended) { >> - return; >> - } >> -#endif >> + virtio_gpu_virgl_resource_unref(g, res, cmd_suspended); >> +} >> - virgl_renderer_resource_detach_iov(unref.resource_id, >> - &res_iovs, >> - &num_iovs); >> - if (res_iovs != NULL && num_iovs != 0) { >> - virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs); >> - } >> - virgl_renderer_resource_unref(unref.resource_id); >> +void virtio_gpu_virgl_resource_destroy(VirtIOGPU *g, >> + struct >> virtio_gpu_simple_resource *base, >> + Error **errp) >> +{ >> + struct virtio_gpu_virgl_resource *res; >> + bool suspended = false; >> - QTAILQ_REMOVE(&g->reslist, &res->base, next); >> + res = container_of(base, struct virtio_gpu_virgl_resource, base); >> - g_free(res); >> + if (virtio_gpu_virgl_resource_unref(g, res, &suspended)) { >> + error_setg(errp, "failed to destroy virgl resource"); >> + } >> } >> static void virgl_cmd_context_create(VirtIOGPU *g, >> @@ -1273,11 +1299,30 @@ void virtio_gpu_virgl_reset_scanout(VirtIOGPU *g) >> } >> } >> -void virtio_gpu_virgl_reset(VirtIOGPU *g) >> +int virtio_gpu_virgl_reset(VirtIOGPU *g) >> { >> + struct virtio_gpu_simple_resource *res, *tmp; >> + >> + /* >> + * Virglrender doesn't support context restoring. VirtIO-GPU >> + * state shall not be reset at runtime. Virgl blob resource >> + * unmapping can be deferred on unref, ensure that destruction >> + * is completed. >> + */ >> + QTAILQ_FOREACH_SAFE(res, &g->reslist, next, tmp) { >> + virtio_gpu_virgl_resource_destroy(g, res, NULL); >> + } >> + >> + if (!QTAILQ_EMPTY(&g->reslist)) { >> + error_report("%s: failed to reset virgl resources", __func__); >> + return -EBUSY; > > I think you missed my reply for an old comment (which was too late and > sent for v3 despite you have already sent v4) so please check it out: > https://lore.kernel.org/qemu-devel/d88cc89b- > [email protected]/
Totally missed that reply from you, thanks for pointing at it! Inlining it here: > No, I meant that virtio_gpu_virgl_resource_unref() may set > *cmd_suspended true. If that happens, QTAILQ_EMPTY(&g->reslist) will be > false, but it is fine so no error log should be emitted. Alright, the virtio_gpu_virgl_resource_destroy() itself should print out error message when it fails. Will remove the additional error_report(). > It is confusing what "runtime" refers to. This code doesn't care about > S3 so such a comment shouldn't be necessary. Will remove the part of comment talking about the runtime. Thanks a lot for the review. -- Best regards, Dmitry
