Re: [RFC PATCH v5 3/4] virtio-gpu: Destroy virgl resources on virtio-gpu reset

[Dmitry Osipenko <dmitry.osipenko@collabora.com>]

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 <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  | 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-
> d796-4bc4-8c90-07da3b88cc7f@rsg.ci.i.u-tokyo.ac.jp/

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