Re: [PATCH v13 03/13] virtio-gpu: Handle virtio_gpu_virgl_init() failure

["Marc-André Lureau" <marcandre.lureau@gmail.com>]

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

Hi

On Mon, May 27, 2024 at 7:03 AM Dmitry Osipenko <
dmitry.osipenko@collabora.com> wrote:

> virtio_gpu_virgl_init() may fail, leading to a further Qemu crash
> because Qemu assumes it never fails. Check virtio_gpu_virgl_init()
> return code and don't execute virtio commands on error. Failed
> virtio_gpu_virgl_init() will result in a timed out virtio commands
> for a guest OS.
>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---
>  hw/display/virtio-gpu-gl.c     | 29 +++++++++++++++++++++--------
>  include/hw/virtio/virtio-gpu.h | 11 +++++++++--
>  2 files changed, 30 insertions(+), 10 deletions(-)
>
> diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
> index e06be60dfbfc..38a2b1bd3916 100644
> --- a/hw/display/virtio-gpu-gl.c
> +++ b/hw/display/virtio-gpu-gl.c
> @@ -29,9 +29,14 @@ static void virtio_gpu_gl_update_cursor_data(VirtIOGPU
> *g,
>                                               struct virtio_gpu_scanout *s,
>                                               uint32_t resource_id)
>  {
> +    VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
>      uint32_t width, height;
>      uint32_t pixels, *data;
>
> +    if (gl->renderer_state != RS_INITED) {
> +        return;
> +    }
> +
>      data = virgl_renderer_get_cursor_data(resource_id, &width, &height);
>      if (!data) {
>          return;
> @@ -65,13 +70,21 @@ static void virtio_gpu_gl_handle_ctrl(VirtIODevice
> *vdev, VirtQueue *vq)
>          return;
>      }
>
> -    if (!gl->renderer_inited) {
> -        virtio_gpu_virgl_init(g);
> -        gl->renderer_inited = true;
> -    }
> -    if (gl->renderer_reset) {
> -        gl->renderer_reset = false;
> +    switch (gl->renderer_state) {
> +    case RS_RESET:
>          virtio_gpu_virgl_reset(g);
> +        /* fallthrough */
> +    case RS_START:
> +        if (virtio_gpu_virgl_init(g)) {
> +            gl->renderer_state = RS_INIT_FAILED;
> +        } else {
> +            gl->renderer_state = RS_INITED;
> +        }
> +        break;
> +    case RS_INIT_FAILED:
> +        return;
> +    case RS_INITED:
> +        break;
>      }
>
>
This still lets it go through the cmd processing after setting
gl->renderer_state = RS_INIT_FAILED, the first time.


>      cmd = virtqueue_pop(vq, sizeof(struct virtio_gpu_ctrl_command));
> @@ -98,9 +111,9 @@ static void virtio_gpu_gl_reset(VirtIODevice *vdev)
>       * GL functions must be called with the associated GL context in main
>       * thread, and when the renderer is unblocked.
>       */
> -    if (gl->renderer_inited && !gl->renderer_reset) {
> +    if (gl->renderer_state == RS_INITED) {
>          virtio_gpu_virgl_reset_scanout(g);
> -        gl->renderer_reset = true;
> +        gl->renderer_state = RS_RESET;
>      }
>  }
>
> diff --git a/include/hw/virtio/virtio-gpu.h
> b/include/hw/virtio/virtio-gpu.h
> index 7ff989a45a5c..6e71d799e5da 100644
> --- a/include/hw/virtio/virtio-gpu.h
> +++ b/include/hw/virtio/virtio-gpu.h
> @@ -224,11 +224,18 @@ struct VirtIOGPUClass {
>                               Error **errp);
>  };
>
> +/* VirtIOGPUGL renderer states */
> +typedef enum {
> +    RS_START,       /* starting state */
> +    RS_INIT_FAILED, /* failed initialisation */
> +    RS_INITED,      /* initialised and working */
> +    RS_RESET,       /* inited and reset pending, moves to start after
> reset */
> +} RenderState;
> +
>  struct VirtIOGPUGL {
>      struct VirtIOGPU parent_obj;
>
> -    bool renderer_inited;
> -    bool renderer_reset;
> +    RenderState renderer_state;
>
>      QEMUTimer *fence_poll;
>      QEMUTimer *print_stats;
> --
> 2.44.0
>
>

-- 
Marc-André Lureau

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