[PATCH 1/2] hw/display: report an error if virgl initialization failed

[PATCH 1/2] hw/display: report an error if virgl initialization failed

[marcandre.lureau]

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

Currently, virgl initialization error are silently ignored.

This is likely going to crash later on, as the device isn't fully
initialized then.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/display/virtio-gpu-virgl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 092c6dc380..46b56f94d9 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -605,6 +605,7 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
 
     ret = virgl_renderer_init(g, 0, &virtio_gpu_3d_cbs);
     if (ret != 0) {
+        error_report("virgl could not be initialized: %d", ret);
         return ret;
     }
 
-- 
2.29.0

[PATCH 2/2] hw/display: fail early when multiple virgl devices are requested

[marcandre.lureau]

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

This avoids failing to initialize virgl and crashing later on, and clear
the user expectations.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/display/virtio-gpu-gl.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
index d971b48080..c973d4824b 100644
--- a/hw/display/virtio-gpu-gl.c
+++ b/hw/display/virtio-gpu-gl.c
@@ -25,6 +25,8 @@
 
 #include <virglrenderer.h>
 
+static int virgl_count = 0;
+
 static void virtio_gpu_gl_update_cursor_data(VirtIOGPU *g,
                                              struct virtio_gpu_scanout *s,
                                              uint32_t resource_id)
@@ -113,6 +115,11 @@ static void virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
     return;
 #endif
 
+    if (virgl_count++ > 0) {
+        error_setg(errp, "multiple virgl devices aren't supported yet");
+        return;
+    }
+
     if (!display_opengl) {
         error_setg(errp, "opengl is not available");
         return;
@@ -124,6 +131,10 @@ static void virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
 
     virtio_gpu_device_realize(qdev, errp);
 }
+static void virtio_gpu_gl_device_unrealize(DeviceState *dev)
+{
+    virgl_count--;
+}
 
 static Property virtio_gpu_gl_properties[] = {
     DEFINE_PROP_BIT("stats", VirtIOGPU, parent_obj.conf.flags,
@@ -144,6 +155,7 @@ static void virtio_gpu_gl_class_init(ObjectClass *klass, void *data)
     vgc->update_cursor_data = virtio_gpu_gl_update_cursor_data;
 
     vdc->realize = virtio_gpu_gl_device_realize;
+    vdc->unrealize = virtio_gpu_gl_device_unrealize;
     vdc->reset = virtio_gpu_gl_reset;
     device_class_set_props(dc, virtio_gpu_gl_properties);
 }
-- 
2.29.0

Re: [PATCH 2/2] hw/display: fail early when multiple virgl devices are requested

[Mark Cave-Ayland]

On 01/07/2021 08:18, marcandre.lureau@redhat.com wrote:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> This avoids failing to initialize virgl and crashing later on, and clear
> the user expectations.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   hw/display/virtio-gpu-gl.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
> index d971b48080..c973d4824b 100644
> --- a/hw/display/virtio-gpu-gl.c
> +++ b/hw/display/virtio-gpu-gl.c
> @@ -25,6 +25,8 @@
>   
>   #include <virglrenderer.h>
>   
> +static int virgl_count = 0;
> +
>   static void virtio_gpu_gl_update_cursor_data(VirtIOGPU *g,
>                                                struct virtio_gpu_scanout *s,
>                                                uint32_t resource_id)
> @@ -113,6 +115,11 @@ static void virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
>       return;
>   #endif
>   
> +    if (virgl_count++ > 0) {
> +        error_setg(errp, "multiple virgl devices aren't supported yet");
> +        return;
> +    }
> +
>       if (!display_opengl) {
>           error_setg(errp, "opengl is not available");
>           return;
> @@ -124,6 +131,10 @@ static void virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
>   
>       virtio_gpu_device_realize(qdev, errp);
>   }
> +static void virtio_gpu_gl_device_unrealize(DeviceState *dev)
> +{
> +    virgl_count--;
> +}
>   
>   static Property virtio_gpu_gl_properties[] = {
>       DEFINE_PROP_BIT("stats", VirtIOGPU, parent_obj.conf.flags,
> @@ -144,6 +155,7 @@ static void virtio_gpu_gl_class_init(ObjectClass *klass, void *data)
>       vgc->update_cursor_data = virtio_gpu_gl_update_cursor_data;
>   
>       vdc->realize = virtio_gpu_gl_device_realize;
> +    vdc->unrealize = virtio_gpu_gl_device_unrealize;
>       vdc->reset = virtio_gpu_gl_reset;
>       device_class_set_props(dc, virtio_gpu_gl_properties);
>   }

FWIW I think the best way to prevent instantiation of multiple devices is to use the 
QOM API to detect if more than one instance of a class exists within the QOM tree.

Have a look at fw_cfg_find() and its usage from fw_cfg_common_realize() in 
hw/nvram/fw_cfg.c for an example of this.


ATB,

Mark.

Re: [PATCH 2/2] hw/display: fail early when multiple virgl devices are requested

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 2739 bytes --]

Hi Mark

On Sat, Jul 3, 2021 at 9:15 AM Mark Cave-Ayland <
mark.cave-ayland@ilande.co.uk> wrote:

> On 01/07/2021 08:18, marcandre.lureau@redhat.com wrote:
>
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > This avoids failing to initialize virgl and crashing later on, and clear
> > the user expectations.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >   hw/display/virtio-gpu-gl.c | 12 ++++++++++++
> >   1 file changed, 12 insertions(+)
> >
> > diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
> > index d971b48080..c973d4824b 100644
> > --- a/hw/display/virtio-gpu-gl.c
> > +++ b/hw/display/virtio-gpu-gl.c
> > @@ -25,6 +25,8 @@
> >
> >   #include <virglrenderer.h>
> >
> > +static int virgl_count = 0;
> > +
> >   static void virtio_gpu_gl_update_cursor_data(VirtIOGPU *g,
> >                                                struct virtio_gpu_scanout
> *s,
> >                                                uint32_t resource_id)
> > @@ -113,6 +115,11 @@ static void
> virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
> >       return;
> >   #endif
> >
> > +    if (virgl_count++ > 0) {
> > +        error_setg(errp, "multiple virgl devices aren't supported yet");
> > +        return;
> > +    }
> > +
> >       if (!display_opengl) {
> >           error_setg(errp, "opengl is not available");
> >           return;
> > @@ -124,6 +131,10 @@ static void
> virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
> >
> >       virtio_gpu_device_realize(qdev, errp);
> >   }
> > +static void virtio_gpu_gl_device_unrealize(DeviceState *dev)
> > +{
> > +    virgl_count--;
> > +}
> >
> >   static Property virtio_gpu_gl_properties[] = {
> >       DEFINE_PROP_BIT("stats", VirtIOGPU, parent_obj.conf.flags,
> > @@ -144,6 +155,7 @@ static void virtio_gpu_gl_class_init(ObjectClass
> *klass, void *data)
> >       vgc->update_cursor_data = virtio_gpu_gl_update_cursor_data;
> >
> >       vdc->realize = virtio_gpu_gl_device_realize;
> > +    vdc->unrealize = virtio_gpu_gl_device_unrealize;
> >       vdc->reset = virtio_gpu_gl_reset;
> >       device_class_set_props(dc, virtio_gpu_gl_properties);
> >   }
>
> FWIW I think the best way to prevent instantiation of multiple devices is
> to use the
> QOM API to detect if more than one instance of a class exists within the
> QOM tree.
>
> Have a look at fw_cfg_find() and its usage from fw_cfg_common_realize() in
> hw/nvram/fw_cfg.c for an example of this.
>

Good idea, I forgot about that trick. Sent "[PATCH v2] hw/display: fail
early when multiple virgl devices are requested".

thanks

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 3900 bytes --]