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

[Marc-André Lureau]

Hi

On Mon, Aug 6, 2018 at 11:16 PM, Michael S. Tsirkin <address@hidden> wrote:
> 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.

If Gerd doesn't chime in before RC4, I suggest to take that version.
He might want to update it or take a different approach post 3.0.

>
>> @@ -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>

thanks



-- 
Marc-André Lureau