Re: [Qemu-devel] [PATCH 4/9] vga: make display updates thread safe.

[Ladi Prosek]

Hi Gerd,

On Fri, Apr 21, 2017 at 11:16 AM, Gerd Hoffmann <address@hidden> wrote:
> The vga code clears the dirty bits *after* reading the framebuffer
> memory.  So if the guest framebuffer updates hits the race window
> between vga reading the framebuffer and vga clearing the dirty bits
> vga will miss that update
>
> Fix it by using the new memory_region_copy_and_clear_dirty()
> memory_region_copy_get_dirty() functions.  That way we clear the
> dirty bitmap before reading the framebuffer.  Any guest display
> updates happening in parallel will be properly tracked in the
> dirty bitmap then and the next display refresh will pick them up.
>
> Problem triggers with mttcg only.  Before mttcg was merged tcg
> never ran in parallel to vga emulation.  Using kvm will hide the
> problem too, due to qemu operating on a userspace copy of the
> kernel's dirty bitmap.
>
> Signed-off-by: Gerd Hoffmann <address@hidden>
> ---
>  hw/display/vga.c | 36 +++++++++++++++++-------------------
>  1 file changed, 17 insertions(+), 19 deletions(-)
>
> diff --git a/hw/display/vga.c b/hw/display/vga.c
> index 3991b88aac..b2516c8d21 100644
> --- a/hw/display/vga.c
> +++ b/hw/display/vga.c
> @@ -1465,7 +1465,8 @@ static void vga_draw_graphic(VGACommonState *s, int 
> full_update)
>      DisplaySurface *surface = qemu_console_surface(s->con);
>      int y1, y, update, linesize, y_start, double_scan, mask, depth;
>      int width, height, shift_control, line_offset, bwidth, bits;
> -    ram_addr_t page0, page1, page_min, page_max;
> +    ram_addr_t page0, page1;
> +    DirtyBitmapSnapshot *snap = NULL;
>      int disp_width, multi_scan, multi_run;
>      uint8_t *d;
>      uint32_t v, addr1, addr;
> @@ -1480,9 +1481,6 @@ static void vga_draw_graphic(VGACommonState *s, int 
> full_update)
>
>      full_update |= update_basic_params(s);
>
> -    if (!full_update)
> -        vga_sync_dirty_bitmap(s);
> -
>      s->get_resolution(s, &width, &height);
>      disp_width = width;
>
> @@ -1625,11 +1623,17 @@ static void vga_draw_graphic(VGACommonState *s, int 
> full_update)
>      addr1 = (s->start_addr * 4);
>      bwidth = (width * bits + 7) / 8;
>      y_start = -1;
> -    page_min = -1;
> -    page_max = 0;
>      d = surface_data(surface);
>      linesize = surface_stride(surface);
>      y1 = 0;
> +
> +    if (!full_update) {
> +        vga_sync_dirty_bitmap(s);
> +        snap = memory_region_snapshot_and_clear_dirty(&s->vram, addr1,
> +                                                      bwidth * height,
> +                                                      DIRTY_MEMORY_VGA);
> +    }
> +
>      for(y = 0; y < height; y++) {
>          addr = addr1;
>          if (!(s->cr[VGA_CRTC_MODE] & 1)) {
> @@ -1644,17 +1648,17 @@ static void vga_draw_graphic(VGACommonState *s, int 
> full_update)
>          update = full_update;
>          page0 = addr;
>          page1 = addr + bwidth - 1;
> -        update |= memory_region_get_dirty(&s->vram, page0, page1 - page0,
> -                                          DIRTY_MEMORY_VGA);
> +        if (full_update) {
> +            update = 1;
> +        } else {
> +            update = memory_region_snapshot_get_dirty(&s->vram, snap,
> +                                                      page0, page1 - page0);
> +        }

This seems to have broken Windows. I am getting a sporadic assert on
boot at the following callstack:

#4 cpu_physical_memory_snapshot_get_dirty (snap=0x555556a3b3d0,
start=2148532224, length=511)
#5 memory_region_snapshot_get_dirty (mr=0x555558007ad0,
snap=0x555556a3b3d0, addr=393216, size=511)
#6 vga_draw_graphic (s=0x555558007ac0, full_update=0)
#7 vga_update_display (opaque=0x555558007ac0)
#8 graphic_hw_update (con=0x55555820f6e0)
#9 qemu_spice_display_refresh (ssd=0x555558007700)
#10 display_refresh (dcl=0x555558007708)
#11 dpy_refresh (s=0x55555820f670)
#12 gui_update (opaque=0x55555820f670)
#13 timerlist_run_timers (timer_list=0x555556836630)
#14 qemu_clock_run_timers (type=QEMU_CLOCK_REALTIME)
#15 qemu_clock_run_all_timers ()
#16 main_loop_wait (nonblocking=0)
#17 main_loop ()
#18 main (argc=83, argv=0x7fffffffdac8, envp=0x7fffffffdd68)

(gdb)
#4  0x000055555576984d in cpu_physical_memory_snapshot_get_dirty
(snap=0x555556a3b3d0, start=2148532224, length=511) at
/home/lprosek/qemu/exec.c:1129
1129        assert(start + length <= snap->end);
(gdb) p *snap
$1 = {start = 2148007936, end = 2148532224, dirty = 0x555556a3b3e0}
(gdb) p start
$2 = 2148532224
(gdb) p length
$3 = 511

'snap->end' is equal to 'start', so:

qemu-system-x86_64: /home/lprosek/qemu/exec.c:1129:
cpu_physical_memory_snapshot_get_dirty: Assertion `start + length <=
snap->end' failed.

Is this enough information to figure out what's going on or would you
like me to take a closer look?

Thanks!
Ladi

>          /* explicit invalidation for the hardware cursor (cirrus only) */
>          update |= vga_scanline_invalidated(s, y);
>          if (update) {
>              if (y_start < 0)
>                  y_start = y;
> -            if (page0 < page_min)
> -                page_min = page0;
> -            if (page1 > page_max)
> -                page_max = page1;
>              if (!(is_buffer_shared(surface))) {
>                  vga_draw_line(s, d, s->vram_ptr + addr, width);
>                  if (s->cursor_draw_line)
> @@ -1687,13 +1691,7 @@ static void vga_draw_graphic(VGACommonState *s, int 
> full_update)
>          dpy_gfx_update(s->con, 0, y_start,
>                         disp_width, y - y_start);
>      }
> -    /* reset modified pages */
> -    if (page_max >= page_min) {
> -        memory_region_reset_dirty(&s->vram,
> -                                  page_min,
> -                                  page_max - page_min,
> -                                  DIRTY_MEMORY_VGA);
> -    }
> +    g_free(snap);
>      memset(s->invalidated_y_table, 0, sizeof(s->invalidated_y_table));
>  }
>
> --
> 2.9.3
>
>