Dmitry Osipenko <dmitry.osipe...@collabora.com> writes: > 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.osipe...@collabora.com> > --- > hw/display/virtio-gpu-gl.c | 15 +++++++++++++-- > hw/display/virtio-gpu-virgl.c | 3 +++ > include/hw/virtio/virtio-gpu.h | 1 + > 3 files changed, 17 insertions(+), 2 deletions(-) > > diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c > index 0c0a8d136954..b353c3193afa 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_inited) { > + return; > + } > + > data = virgl_renderer_get_cursor_data(resource_id, &width, &height); > if (!data) { > return; > @@ -60,13 +65,18 @@ static void virtio_gpu_gl_handle_ctrl(VirtIODevice *vdev, > VirtQueue *vq) > VirtIOGPU *g = VIRTIO_GPU(vdev); > VirtIOGPUGL *gl = VIRTIO_GPU_GL(vdev); > struct virtio_gpu_ctrl_command *cmd; > + int ret; > > - if (!virtio_queue_ready(vq)) { > + if (!virtio_queue_ready(vq) || gl->renderer_init_failed) { > return; > } > > if (!gl->renderer_inited) { > - virtio_gpu_virgl_init(g); > + ret = virtio_gpu_virgl_init(g); > + if (ret) { > + gl->renderer_init_failed = true; > + return; > + } > gl->renderer_inited = true; > } > if (gl->renderer_reset) { > @@ -101,6 +111,7 @@ static void virtio_gpu_gl_reset(VirtIODevice *vdev) > if (gl->renderer_inited && !gl->renderer_reset) { > virtio_gpu_virgl_reset_scanout(g); > gl->renderer_reset = true; > + gl->renderer_init_failed = false; > } > } > > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c > index 6ba4c24fda1d..bfbc6553e879 100644 > --- a/hw/display/virtio-gpu-virgl.c > +++ b/hw/display/virtio-gpu-virgl.c > @@ -668,6 +668,9 @@ int virtio_gpu_virgl_get_num_capsets(VirtIOGPU *g) > > void virtio_gpu_virgl_deinit(VirtIOGPU *g) > { > + if (virtio_gpu_stats_enabled(g->parent_obj.conf)) { > + timer_free(g->print_stats); > + } > timer_free(g->fence_poll); > virgl_renderer_cleanup(NULL); > } > diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h > index 8ece1ec2e98b..236daba24dd2 100644 > --- a/include/hw/virtio/virtio-gpu.h > +++ b/include/hw/virtio/virtio-gpu.h > @@ -230,6 +230,7 @@ struct VirtIOGPUGL { > > bool renderer_inited; > bool renderer_reset; > + bool renderer_init_failed;
Do we really want 3 bools to represent a bunch of mutually incompatible state? How about: typedef enum { /** starting state */ RS_START = 0, /** renderer initialised */ RS_INITED, /** renderer reset */ RS_RESET, /** failed initialisation */ RS_INIT_FAILED } RenderState; then for example you could flow virtio_gpu_gl_handle_ctrl() as: switch (gl->render_state) { case RS_START: { gl->render_state = virtio_gpu_virgl_init(g) ? RS_INIT_FAILED : RS_INITED; } case RS_RESET: { virtio_gpu_virgl_reset(g); gl->render_state = RS_INITED; break; } case RS_INIT_FAILED: /* nothing to do, return early and maybe log */ return; case RS_INITED: /* just continue */ break; } > }; > > struct VhostUserGPU { -- Alex Bennée Virtualisation Tech Lead @ Linaro