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

2025-12-01 Thread Dmitry Osipenko
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 
>> Signed-off-by: Dmitry Osipenko 
>> ---
>>   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)
>> +in

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

2025-12-01 Thread Akihiko Odaki

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 
Signed-off-by: Dmitry Osipenko 
---
  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

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

2025-11-29 Thread Dmitry Osipenko
Properly destroy virgl resources on virtio-gpu reset to not leak resources
on a hot reboot of a VM.

Suggested-by: Akihiko Odaki 
Signed-off-by: Dmitry Osipenko 
---
 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)) {
+e