| Message ID | 20170421091632.30900-5-kraxel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Gerd, On Fri, Apr 21, 2017 at 11:16 AM, Gerd Hoffmann <kraxel@redhat.com> 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 <kraxel@redhat.com> > --- > 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 > >
Hi, > #4 cpu_physical_memory_snapshot_get_dirty (snap=0x555556a3b3d0, > start=2148532224, length=511) https://patchwork.ozlabs.org/patch/760013/ cheers, Gerd
On Tue, May 9, 2017 at 4:02 PM, Gerd Hoffmann <kraxel@redhat.com> wrote: > Hi, > >> #4 cpu_physical_memory_snapshot_get_dirty (snap=0x555556a3b3d0, >> start=2148532224, length=511) > > https://patchwork.ozlabs.org/patch/760013/ Awesome, thanks! > cheers, > Gerd
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); + } /* 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)); }
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 <kraxel@redhat.com> --- hw/display/vga.c | 36 +++++++++++++++++------------------- 1 file changed, 17 insertions(+), 19 deletions(-)