Re: [Qemu-devel] [PATCH] virtio-gpu: fix crashes upon warm reboot with vga mode

[Michael S. Tsirkin]

On Fri, Aug 03, 2018 at 05:32:35PM +0200, Marc-André Lureau wrote:
> With vga=775 on the Linux command line a first boot of the VM running
> Linux works fine. After a warm reboot it crashes during Linux boot.
> 
> Before that, valgrind points out bad memory write to console
> surface. The VGA code is not aware that virtio-gpu got a message
> surface scanout when the display is disabled. Let's reset VGA graphic
> mode when it is the case, so that a new display surface is created
> when doing further VGA operations.
> 
> https://bugs.launchpad.net/qemu/+bug/1784900/
> 
> Reported-by: Stefan Berger <address@hidden>
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  include/hw/virtio/virtio-gpu.h |  1 +
>  hw/display/virtio-gpu.c        |  5 +++++
>  hw/display/virtio-vga.c        | 11 +++++++++++
>  3 files changed, 17 insertions(+)
> 
> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> index 9780f755ef..d0321672f4 100644
> --- a/include/hw/virtio/virtio-gpu.h
> +++ b/include/hw/virtio/virtio-gpu.h
> @@ -125,6 +125,7 @@ typedef struct VirtIOGPU {
>          uint32_t bytes_3d;
>      } stats;
>  
> +    void (*disable_scanout)(struct VirtIOGPU *g, int scanout_id);
>      Error *migration_blocker;
>  } VirtIOGPU;
>  
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index ec366f4c35..3ddd29c0de 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -421,6 +421,11 @@ static void virtio_gpu_disable_scanout(VirtIOGPU *g, int 
> scanout_id)
>                                           scanout->height ?: 480,
>                                           "Guest disabled display.");
>      }
> +
> +    if (g->disable_scanout) {
> +        g->disable_scanout(g, scanout_id);
> +    }
> +
>      dpy_gfx_replace_surface(scanout->con, ds);
>      scanout->resource_id = 0;
>      scanout->ds = NULL;
> diff --git a/hw/display/virtio-vga.c b/hw/display/virtio-vga.c
> index 2b36f2899a..672b7f9ce2 100644
> --- a/hw/display/virtio-vga.c
> +++ b/hw/display/virtio-vga.c
> @@ -75,6 +75,16 @@ static void virtio_vga_gl_block(void *opaque, bool block)
>      }
>  }
>  
> +static void virtio_vga_disable_scanout(VirtIOGPU *g, int scanout_id)
> +{
> +    VirtIOVGA *vvga = container_of(g, VirtIOVGA, vdev);
> +
> +    if (scanout_id == 0) {
> +        /* reset surface if needed */
> +        vvga->vga.graphic_mode = -1;
> +    }
> +}
> +
>  static const GraphicHwOps virtio_vga_ops = {
>      .invalidate = virtio_vga_invalidate_display,
>      .gfx_update = virtio_vga_update_display,

Would it make sense to add
vga_disable_scanout() to hw/display/vga_int.h and
poke at graphic_mode there?

I'll leave the decision to you.

> @@ -156,6 +166,7 @@ static void virtio_vga_realize(VirtIOPCIProxy *vpci_dev, 
> Error **errp)
>                                   vvga->vga_mrs, true);
>  
>      vga->con = g->scanout[0].con;
> +    g->disable_scanout = virtio_vga_disable_scanout;
>      graphic_console_set_hwops(vga->con, &virtio_vga_ops, vvga);
>  
>      for (i = 0; i < g->conf.max_outputs; i++) {

While I really know very little about vga, it seems
like that's the standard way to force full update so


Reviewed-by: Michael S. Tsirkin <address@hidden>

-- 
MST