| Message ID | c123808b760f4d8d743c4fd4a04e730005ff673e.1706279540.git.manos.pitsidianakis@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix resource freeing bugs in virtio-gpu-rutabaga | expand |
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 {
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 {
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
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 { >
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
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 {
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(-)