1. Patchew
  2. QEMU
  3. [PATCH v1 0/3] Fix resource freeing bugs in virtio-gpu-rutabaga
  4. View patch

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

Manos Pitsidianakis posted 3 patches 10 months ago
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Gerd Hoffmann <kraxel@redhat.com>
There is a newer version of this series
[PATCH v1 2/3] virtio-gpu.c: add resource_destroy class method
Posted by Manos Pitsidianakis 10 months ago 
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 {
-- 
γαῖα πυρί μιχθήτω
Re: [PATCH v1 2/3] virtio-gpu.c: add resource_destroy class method
Posted by Alex Bennée 10 months ago 
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
Posted by Manos Pitsidianakis 10 months ago 
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
Posted by Philippe Mathieu-Daudé 10 months ago 
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
Posted by Manos Pitsidianakis 10 months ago 
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 2/3] virtio-gpu.c: add resource_destroy class method
Posted by Marc-André Lureau 10 months ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.