[PATCH v1 0/3] Fix resource freeing bugs in virtio-gpu-rutabaga

[PATCH v1 0/3] Fix resource freeing bugs in virtio-gpu-rutabaga

[Manos Pitsidianakis]

While testing the rutabaga gpu device, we noticed that if the device is 
reset, it stops working and complains about missing resource ids. A 
quick investigation discovered that the generic VirtIOGPU implementation 
frees all resources, but for Rutabaga, they are tied with rutabaga 
objects that need to be destroyed as well.

This series adds a resource_destroy class method that the Rutabaga 
device can override and do its own bookkeeping. 

Manos Pitsidianakis (3):
  hw/display/virtio-gpu.c: use reset_bh class method
  virtio-gpu.c: add resource_destroy class method
  virtio-gpu-rutabaga.c: override resource_destroy method

 hw/display/virtio-gpu-rutabaga.c | 31 ++++++++++++++++++++-----------
 hw/display/virtio-gpu.c          | 21 +++++++++++++++++----
 include/hw/virtio/virtio-gpu.h   |  2 ++
 3 files changed, 39 insertions(+), 15 deletions(-)


base-commit: e029fe22caad9b75c7ab69bd4e84853c11fb71e0
-- 
γαῖα πυρί μιχθήτω

[PATCH v1 1/3] hw/display/virtio-gpu.c: use reset_bh class method

[Manos Pitsidianakis]

While the VirtioGPU type has a reset_bh field to specify a reset
callback, it's never used. virtio_gpu_reset() calls the general
virtio_gpu_reset_bh() function for all devices that inherit from
VirtioGPU.

While no devices override reset_bh at the moment, a device reset might
require special logic for implementations in the future.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 hw/display/virtio-gpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index f8a675eb30..2b73ae585b 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1515,7 +1515,7 @@ void virtio_gpu_reset(VirtIODevice *vdev)
             qemu_cond_wait_bql(&g->reset_cond);
         }
     } else {
-        virtio_gpu_reset_bh(g);
+        aio_bh_call(g->reset_bh);
     }
 
     while (!QTAILQ_EMPTY(&g->cmdq)) {
-- 
γαῖα πυρί μιχθήτω

[PATCH v1 2/3] virtio-gpu.c: add resource_destroy class method

[Manos Pitsidianakis]

When destroying/unrefing resources, devices such as virtio-gpu-rutabaga
need to do their own bookkeeping (free rutabaga resources that are
associated with the virtio_gpu_simple_resource).

This commit adds a class method so that virtio-gpu-rutabaga can override
it in the next commit.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 hw/display/virtio-gpu.c        | 19 ++++++++++++++++---
 include/hw/virtio/virtio-gpu.h |  2 ++
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 2b73ae585b..96420ba74f 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -401,8 +401,9 @@ static void virtio_gpu_disable_scanout(VirtIOGPU *g, int scanout_id)
     scanout->height = 0;
 }
 
-static void virtio_gpu_resource_destroy(VirtIOGPU *g,
-                                        struct virtio_gpu_simple_resource *res)
+static int32_t
+virtio_gpu_resource_destroy(VirtIOGPU *g,
+                            struct virtio_gpu_simple_resource *res)
 {
     int i;
 
@@ -419,6 +420,8 @@ static void virtio_gpu_resource_destroy(VirtIOGPU *g,
     QTAILQ_REMOVE(&g->reslist, res, next);
     g->hostmem -= res->hostmem;
     g_free(res);
+
+    return 0;
 }
 
 static void virtio_gpu_resource_unref(VirtIOGPU *g,
@@ -1488,11 +1491,20 @@ static void virtio_gpu_device_unrealize(DeviceState *qdev)
 static void virtio_gpu_reset_bh(void *opaque)
 {
     VirtIOGPU *g = VIRTIO_GPU(opaque);
+    VirtIOGPUClass *vgc = VIRTIO_GPU_GET_CLASS(g);
     struct virtio_gpu_simple_resource *res, *tmp;
+    int32_t result, resource_id;
     int i = 0;
 
     QTAILQ_FOREACH_SAFE(res, &g->reslist, next, tmp) {
-        virtio_gpu_resource_destroy(g, res);
+        resource_id = res->resource_id;
+        result = vgc->resource_destroy(g, res);
+        if (result) {
+            error_report("%s: %s resource_destroy"
+                         "for resource_id = %d failed with return value = %d;",
+                         __func__, object_get_typename(OBJECT(g)), resource_id,
+                         result);
+        }
     }
 
     for (i = 0; i < g->parent_obj.conf.max_outputs; i++) {
@@ -1632,6 +1644,7 @@ static void virtio_gpu_class_init(ObjectClass *klass, void *data)
     vgc->handle_ctrl = virtio_gpu_handle_ctrl;
     vgc->process_cmd = virtio_gpu_simple_process_cmd;
     vgc->update_cursor_data = virtio_gpu_update_cursor_data;
+    vgc->resource_destroy = virtio_gpu_resource_destroy;
     vgbc->gl_flushed = virtio_gpu_handle_gl_flushed;
 
     vdc->realize = virtio_gpu_device_realize;
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 584ba2ed73..5683354236 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -219,6 +219,8 @@ struct VirtIOGPUClass {
     void (*update_cursor_data)(VirtIOGPU *g,
                                struct virtio_gpu_scanout *s,
                                uint32_t resource_id);
+    int32_t (*resource_destroy)(VirtIOGPU *g,
+                                struct virtio_gpu_simple_resource *res);
 };
 
 struct VirtIOGPUGL {
-- 
γαῖα πυρί μιχθήτω

[PATCH v1 3/3] virtio-gpu-rutabaga.c: override resource_destroy method

[Manos Pitsidianakis]

When the Rutabaga GPU device frees resources, it calls
rutabaga_resource_unref for that resource_id. However, when the generic
VirtIOGPU functions destroys resources, it only removes the
virtio_gpu_simple_resource from the device's VirtIOGPU->reslist list.
The rutabaga resource associated with that resource_id is then leaked.

This commit overrides the resource_destroy class method introduced in
the previous commit to fix this.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 hw/display/virtio-gpu-rutabaga.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/hw/display/virtio-gpu-rutabaga.c b/hw/display/virtio-gpu-rutabaga.c
index 9e67f9bd51..66b5a115ac 100644
--- a/hw/display/virtio-gpu-rutabaga.c
+++ b/hw/display/virtio-gpu-rutabaga.c
@@ -147,6 +147,24 @@ rutabaga_cmd_create_resource_3d(VirtIOGPU *g,
     QTAILQ_INSERT_HEAD(&g->reslist, res, next);
 }
 
+static int32_t
+virtio_gpu_rutabaga_resource_unref(VirtIOGPU *g,
+                                   struct virtio_gpu_simple_resource *res)
+{
+    int32_t result;
+    VirtIOGPURutabaga *vr = VIRTIO_GPU_RUTABAGA(g);
+
+    result = rutabaga_resource_unref(vr->rutabaga, res->resource_id);
+
+    if (res->image) {
+        pixman_image_unref(res->image);
+    }
+
+    QTAILQ_REMOVE(&g->reslist, res, next);
+    g_free(res);
+    return result;
+}
+
 static void
 rutabaga_cmd_resource_unref(VirtIOGPU *g,
                             struct virtio_gpu_ctrl_command *cmd)
@@ -155,8 +173,6 @@ rutabaga_cmd_resource_unref(VirtIOGPU *g,
     struct virtio_gpu_simple_resource *res;
     struct virtio_gpu_resource_unref unref;
 
-    VirtIOGPURutabaga *vr = VIRTIO_GPU_RUTABAGA(g);
-
     VIRTIO_GPU_FILL_CMD(unref);
 
     trace_virtio_gpu_cmd_res_unref(unref.resource_id);
@@ -164,15 +180,8 @@ rutabaga_cmd_resource_unref(VirtIOGPU *g,
     res = virtio_gpu_find_resource(g, unref.resource_id);
     CHECK(res, cmd);
 
-    result = rutabaga_resource_unref(vr->rutabaga, unref.resource_id);
+    result = virtio_gpu_rutabaga_resource_unref(g, res);
     CHECK(!result, cmd);
-
-    if (res->image) {
-        pixman_image_unref(res->image);
-    }
-
-    QTAILQ_REMOVE(&g->reslist, res, next);
-    g_free(res);
 }
 
 static void
@@ -1099,7 +1108,7 @@ static void virtio_gpu_rutabaga_class_init(ObjectClass *klass, void *data)
     vgc->handle_ctrl = virtio_gpu_rutabaga_handle_ctrl;
     vgc->process_cmd = virtio_gpu_rutabaga_process_cmd;
     vgc->update_cursor_data = virtio_gpu_rutabaga_update_cursor;
-
+    vgc->resource_destroy = virtio_gpu_rutabaga_resource_unref;
     vdc->realize = virtio_gpu_rutabaga_realize;
     device_class_set_props(dc, virtio_gpu_rutabaga_properties);
 }
-- 
γαῖα πυρί μιχθήτω

Re: [PATCH v1 2/3] virtio-gpu.c: add resource_destroy class method

[Philippe Mathieu-Daudé]

Hi Manos,

On 26/1/24 15:41, Manos Pitsidianakis wrote:
> When destroying/unrefing resources, devices such as virtio-gpu-rutabaga
> need to do their own bookkeeping (free rutabaga resources that are
> associated with the virtio_gpu_simple_resource).
> 
> This commit adds a class method so that virtio-gpu-rutabaga can override
> it in the next commit.
> 
> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> ---
>   hw/display/virtio-gpu.c        | 19 ++++++++++++++++---
>   include/hw/virtio/virtio-gpu.h |  2 ++
>   2 files changed, 18 insertions(+), 3 deletions(-)


>   static void virtio_gpu_resource_unref(VirtIOGPU *g,
> @@ -1488,11 +1491,20 @@ static void virtio_gpu_device_unrealize(DeviceState *qdev)
>   static void virtio_gpu_reset_bh(void *opaque)
>   {
>       VirtIOGPU *g = VIRTIO_GPU(opaque);
> +    VirtIOGPUClass *vgc = VIRTIO_GPU_GET_CLASS(g);
>       struct virtio_gpu_simple_resource *res, *tmp;
> +    int32_t result, resource_id;
>       int i = 0;
>   
>       QTAILQ_FOREACH_SAFE(res, &g->reslist, next, tmp) {
> -        virtio_gpu_resource_destroy(g, res);
> +        resource_id = res->resource_id;
> +        result = vgc->resource_destroy(g, res);
> +        if (result) {
> +            error_report("%s: %s resource_destroy"
> +                         "for resource_id = %d failed with return value = %d;",

'%d' is for 'int', for 'int32_t' you need 'PRId32'.

But why return that type instead of 'int'?

> +                         __func__, object_get_typename(OBJECT(g)), resource_id,
> +                         result);
> +        }
>       }


> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> index 584ba2ed73..5683354236 100644
> --- a/include/hw/virtio/virtio-gpu.h
> +++ b/include/hw/virtio/virtio-gpu.h
> @@ -219,6 +219,8 @@ struct VirtIOGPUClass {
>       void (*update_cursor_data)(VirtIOGPU *g,
>                                  struct virtio_gpu_scanout *s,
>                                  uint32_t resource_id);
> +    int32_t (*resource_destroy)(VirtIOGPU *g,
> +                                struct virtio_gpu_simple_resource *res);
>   };
>   
>   struct VirtIOGPUGL {

Re: [PATCH v1 2/3] virtio-gpu.c: add resource_destroy class method

[Alex Bennée]

Manos Pitsidianakis <manos.pitsidianakis@linaro.org> writes:

> When destroying/unrefing resources, devices such as virtio-gpu-rutabaga
> need to do their own bookkeeping (free rutabaga resources that are
> associated with the virtio_gpu_simple_resource).
>
> This commit adds a class method so that virtio-gpu-rutabaga can override
> it in the next commit.
>
> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> ---
>  hw/display/virtio-gpu.c        | 19 ++++++++++++++++---
>  include/hw/virtio/virtio-gpu.h |  2 ++
>  2 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 2b73ae585b..96420ba74f 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -401,8 +401,9 @@ static void virtio_gpu_disable_scanout(VirtIOGPU *g, int scanout_id)
>      scanout->height = 0;
>  }
>  
> -static void virtio_gpu_resource_destroy(VirtIOGPU *g,
> -                                        struct virtio_gpu_simple_resource *res)
> +static int32_t
> +virtio_gpu_resource_destroy(VirtIOGPU *g,
> +                            struct virtio_gpu_simple_resource *res)
>  {
>      int i;
>  
> @@ -419,6 +420,8 @@ static void virtio_gpu_resource_destroy(VirtIOGPU *g,
>      QTAILQ_REMOVE(&g->reslist, res, next);
>      g->hostmem -= res->hostmem;
>      g_free(res);
> +
> +    return 0;
>  }
>  
>  static void virtio_gpu_resource_unref(VirtIOGPU *g,
> @@ -1488,11 +1491,20 @@ static void virtio_gpu_device_unrealize(DeviceState *qdev)
>  static void virtio_gpu_reset_bh(void *opaque)
>  {
>      VirtIOGPU *g = VIRTIO_GPU(opaque);
> +    VirtIOGPUClass *vgc = VIRTIO_GPU_GET_CLASS(g);
>      struct virtio_gpu_simple_resource *res, *tmp;
> +    int32_t result, resource_id;
>      int i = 0;
>  
>      QTAILQ_FOREACH_SAFE(res, &g->reslist, next, tmp) {
> -        virtio_gpu_resource_destroy(g, res);
> +        resource_id = res->resource_id;
> +        result = vgc->resource_destroy(g, res);
> +        if (result) {
> +            error_report("%s: %s resource_destroy"
> +                         "for resource_id = %d failed with return value = %d;",
> +                         __func__, object_get_typename(OBJECT(g)), resource_id,
> +                         result);
> +        }
>      }
>  
>      for (i = 0; i < g->parent_obj.conf.max_outputs; i++) {
> @@ -1632,6 +1644,7 @@ static void virtio_gpu_class_init(ObjectClass *klass, void *data)
>      vgc->handle_ctrl = virtio_gpu_handle_ctrl;
>      vgc->process_cmd = virtio_gpu_simple_process_cmd;
>      vgc->update_cursor_data = virtio_gpu_update_cursor_data;
> +    vgc->resource_destroy = virtio_gpu_resource_destroy;
>      vgbc->gl_flushed = virtio_gpu_handle_gl_flushed;
>  
>      vdc->realize = virtio_gpu_device_realize;
> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> index 584ba2ed73..5683354236 100644
> --- a/include/hw/virtio/virtio-gpu.h
> +++ b/include/hw/virtio/virtio-gpu.h
> @@ -219,6 +219,8 @@ struct VirtIOGPUClass {
>      void (*update_cursor_data)(VirtIOGPU *g,
>                                 struct virtio_gpu_scanout *s,
>                                 uint32_t resource_id);
> +    int32_t (*resource_destroy)(VirtIOGPU *g,
> +                                struct virtio_gpu_simple_resource
>      *res);

What range of errors to you expect to have here? Otherwise you might as
well return a bool for success/fail.

>  };
>  
>  struct VirtIOGPUGL {

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v1 2/3] virtio-gpu.c: add resource_destroy class method

[Manos Pitsidianakis]

On Fri, 26 Jan 2024 at 20:09, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Manos Pitsidianakis <manos.pitsidianakis@linaro.org> writes:
>
> > When destroying/unrefing resources, devices such as virtio-gpu-rutabaga
> > need to do their own bookkeeping (free rutabaga resources that are
> > associated with the virtio_gpu_simple_resource).
> >
> > This commit adds a class method so that virtio-gpu-rutabaga can override
> > it in the next commit.
> >
> > Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> > ---
> >  hw/display/virtio-gpu.c        | 19 ++++++++++++++++---
> >  include/hw/virtio/virtio-gpu.h |  2 ++
> >  2 files changed, 18 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> > index 2b73ae585b..96420ba74f 100644
> > --- a/hw/display/virtio-gpu.c
> > +++ b/hw/display/virtio-gpu.c
> > @@ -401,8 +401,9 @@ static void virtio_gpu_disable_scanout(VirtIOGPU *g, int scanout_id)
> >      scanout->height = 0;
> >  }
> >
> > -static void virtio_gpu_resource_destroy(VirtIOGPU *g,
> > -                                        struct virtio_gpu_simple_resource *res)
> > +static int32_t
> > +virtio_gpu_resource_destroy(VirtIOGPU *g,
> > +                            struct virtio_gpu_simple_resource *res)
> >  {
> >      int i;
> >
> > @@ -419,6 +420,8 @@ static void virtio_gpu_resource_destroy(VirtIOGPU *g,
> >      QTAILQ_REMOVE(&g->reslist, res, next);
> >      g->hostmem -= res->hostmem;
> >      g_free(res);
> > +
> > +    return 0;
> >  }
> >
> >  static void virtio_gpu_resource_unref(VirtIOGPU *g,
> > @@ -1488,11 +1491,20 @@ static void virtio_gpu_device_unrealize(DeviceState *qdev)
> >  static void virtio_gpu_reset_bh(void *opaque)
> >  {
> >      VirtIOGPU *g = VIRTIO_GPU(opaque);
> > +    VirtIOGPUClass *vgc = VIRTIO_GPU_GET_CLASS(g);
> >      struct virtio_gpu_simple_resource *res, *tmp;
> > +    int32_t result, resource_id;
> >      int i = 0;
> >
> >      QTAILQ_FOREACH_SAFE(res, &g->reslist, next, tmp) {
> > -        virtio_gpu_resource_destroy(g, res);
> > +        resource_id = res->resource_id;
> > +        result = vgc->resource_destroy(g, res);
> > +        if (result) {
> > +            error_report("%s: %s resource_destroy"
> > +                         "for resource_id = %d failed with return value = %d;",
> > +                         __func__, object_get_typename(OBJECT(g)), resource_id,
> > +                         result);
> > +        }
> >      }
> >
> >      for (i = 0; i < g->parent_obj.conf.max_outputs; i++) {
> > @@ -1632,6 +1644,7 @@ static void virtio_gpu_class_init(ObjectClass *klass, void *data)
> >      vgc->handle_ctrl = virtio_gpu_handle_ctrl;
> >      vgc->process_cmd = virtio_gpu_simple_process_cmd;
> >      vgc->update_cursor_data = virtio_gpu_update_cursor_data;
> > +    vgc->resource_destroy = virtio_gpu_resource_destroy;
> >      vgbc->gl_flushed = virtio_gpu_handle_gl_flushed;
> >
> >      vdc->realize = virtio_gpu_device_realize;
> > diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> > index 584ba2ed73..5683354236 100644
> > --- a/include/hw/virtio/virtio-gpu.h
> > +++ b/include/hw/virtio/virtio-gpu.h
> > @@ -219,6 +219,8 @@ struct VirtIOGPUClass {
> >      void (*update_cursor_data)(VirtIOGPU *g,
> >                                 struct virtio_gpu_scanout *s,
> >                                 uint32_t resource_id);
> > +    int32_t (*resource_destroy)(VirtIOGPU *g,
> > +                                struct virtio_gpu_simple_resource
> >      *res);
>
> What range of errors to you expect to have here? Otherwise you might as
> well return a bool for success/fail.


Rutabaga can return EINVAL or ESRCH.



> >  };
> >
> >  struct VirtIOGPUGL {
>
> --
> Alex Bennée
> Virtualisation Tech Lead @ Linaro

Re: [PATCH v1 2/3] virtio-gpu.c: add resource_destroy class method

[Manos Pitsidianakis]

On Fri, 26 Jan 2024 at 17:22, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Hi Manos,
>
> On 26/1/24 15:41, Manos Pitsidianakis wrote:
> > When destroying/unrefing resources, devices such as virtio-gpu-rutabaga
> > need to do their own bookkeeping (free rutabaga resources that are
> > associated with the virtio_gpu_simple_resource).
> >
> > This commit adds a class method so that virtio-gpu-rutabaga can override
> > it in the next commit.
> >
> > Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> > ---
> >   hw/display/virtio-gpu.c        | 19 ++++++++++++++++---
> >   include/hw/virtio/virtio-gpu.h |  2 ++
> >   2 files changed, 18 insertions(+), 3 deletions(-)
>
>
> >   static void virtio_gpu_resource_unref(VirtIOGPU *g,
> > @@ -1488,11 +1491,20 @@ static void virtio_gpu_device_unrealize(DeviceState *qdev)
> >   static void virtio_gpu_reset_bh(void *opaque)
> >   {
> >       VirtIOGPU *g = VIRTIO_GPU(opaque);
> > +    VirtIOGPUClass *vgc = VIRTIO_GPU_GET_CLASS(g);
> >       struct virtio_gpu_simple_resource *res, *tmp;
> > +    int32_t result, resource_id;
> >       int i = 0;
> >
> >       QTAILQ_FOREACH_SAFE(res, &g->reslist, next, tmp) {
> > -        virtio_gpu_resource_destroy(g, res);
> > +        resource_id = res->resource_id;
> > +        result = vgc->resource_destroy(g, res);
> > +        if (result) {
> > +            error_report("%s: %s resource_destroy"
> > +                         "for resource_id = %d failed with return value = %d;",
>
> '%d' is for 'int', for 'int32_t' you need 'PRId32'.

Thanks,

> But why return that type instead of 'int'?

Because devices might use FFI, and other languages use fixed size
integers. Since rutabaga is the only one doing this right now, I used
their integer width.

> > +                         __func__, object_get_typename(OBJECT(g)), resource_id,
> > +                         result);
> > +        }
> >       }
>
>
> > diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> > index 584ba2ed73..5683354236 100644
> > --- a/include/hw/virtio/virtio-gpu.h
> > +++ b/include/hw/virtio/virtio-gpu.h
> > @@ -219,6 +219,8 @@ struct VirtIOGPUClass {
> >       void (*update_cursor_data)(VirtIOGPU *g,
> >                                  struct virtio_gpu_scanout *s,
> >                                  uint32_t resource_id);
> > +    int32_t (*resource_destroy)(VirtIOGPU *g,
> > +                                struct virtio_gpu_simple_resource *res);
> >   };
> >
> >   struct VirtIOGPUGL {
>

Re: [PATCH v1 1/3] hw/display/virtio-gpu.c: use reset_bh class method

[Marc-André Lureau]

On Fri, Jan 26, 2024 at 7:43 PM Manos Pitsidianakis
<manos.pitsidianakis@linaro.org> wrote:
>
> While the VirtioGPU type has a reset_bh field to specify a reset
> callback, it's never used. virtio_gpu_reset() calls the general
> virtio_gpu_reset_bh() function for all devices that inherit from
> VirtioGPU.
>
> While no devices override reset_bh at the moment, a device reset might
> require special logic for implementations in the future.
>
> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  hw/display/virtio-gpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index f8a675eb30..2b73ae585b 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -1515,7 +1515,7 @@ void virtio_gpu_reset(VirtIODevice *vdev)
>              qemu_cond_wait_bql(&g->reset_cond);
>          }
>      } else {
> -        virtio_gpu_reset_bh(g);
> +        aio_bh_call(g->reset_bh);
>      }
>
>      while (!QTAILQ_EMPTY(&g->cmdq)) {
> --
> γαῖα πυρί μιχθήτω
>
>


-- 
Marc-André Lureau

Re: [PATCH v1 2/3] virtio-gpu.c: add resource_destroy class method

[Marc-André Lureau]

Hi

On Fri, Jan 26, 2024 at 10:20 PM Manos Pitsidianakis
<manos.pitsidianakis@linaro.org> wrote:
>
> On Fri, 26 Jan 2024 at 17:22, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >
> > Hi Manos,
> >
> > On 26/1/24 15:41, Manos Pitsidianakis wrote:
> > > When destroying/unrefing resources, devices such as virtio-gpu-rutabaga
> > > need to do their own bookkeeping (free rutabaga resources that are
> > > associated with the virtio_gpu_simple_resource).
> > >
> > > This commit adds a class method so that virtio-gpu-rutabaga can override
> > > it in the next commit.
> > >
> > > Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> > > ---
> > >   hw/display/virtio-gpu.c        | 19 ++++++++++++++++---
> > >   include/hw/virtio/virtio-gpu.h |  2 ++
> > >   2 files changed, 18 insertions(+), 3 deletions(-)
> >
> >
> > >   static void virtio_gpu_resource_unref(VirtIOGPU *g,
> > > @@ -1488,11 +1491,20 @@ static void virtio_gpu_device_unrealize(DeviceState *qdev)
> > >   static void virtio_gpu_reset_bh(void *opaque)
> > >   {
> > >       VirtIOGPU *g = VIRTIO_GPU(opaque);
> > > +    VirtIOGPUClass *vgc = VIRTIO_GPU_GET_CLASS(g);
> > >       struct virtio_gpu_simple_resource *res, *tmp;
> > > +    int32_t result, resource_id;
> > >       int i = 0;
> > >
> > >       QTAILQ_FOREACH_SAFE(res, &g->reslist, next, tmp) {
> > > -        virtio_gpu_resource_destroy(g, res);
> > > +        resource_id = res->resource_id;
> > > +        result = vgc->resource_destroy(g, res);
> > > +        if (result) {
> > > +            error_report("%s: %s resource_destroy"
> > > +                         "for resource_id = %d failed with return value = %d;",
> >
> > '%d' is for 'int', for 'int32_t' you need 'PRId32'.
>
> Thanks,
>
> > But why return that type instead of 'int'?
>
> Because devices might use FFI, and other languages use fixed size
> integers. Since rutabaga is the only one doing this right now, I used
> their integer width.
>

Imo introducing an API in QEMU, you should follow the conventions and
use bool/Error. Implementation should adapt with the rutabaga API.

> > > +                         __func__, object_get_typename(OBJECT(g)), resource_id,
> > > +                         result);
> > > +        }
> > >       }
> >
> >
> > > diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> > > index 584ba2ed73..5683354236 100644
> > > --- a/include/hw/virtio/virtio-gpu.h
> > > +++ b/include/hw/virtio/virtio-gpu.h
> > > @@ -219,6 +219,8 @@ struct VirtIOGPUClass {
> > >       void (*update_cursor_data)(VirtIOGPU *g,
> > >                                  struct virtio_gpu_scanout *s,
> > >                                  uint32_t resource_id);
> > > +    int32_t (*resource_destroy)(VirtIOGPU *g,
> > > +                                struct virtio_gpu_simple_resource *res);
> > >   };
> > >
> > >   struct VirtIOGPUGL {
> >
>

thanks

-- 
Marc-André Lureau