[Qemu-devel] [PATCH 0/9] hw/display: make display updates thread safe, part 1

[Qemu-devel] [PATCH 0/9] hw/display: make display updates thread safe, part 1

[Gerd Hoffmann]

  Hi,

Ok, 2.10 is open, lets start tackling the display update race
conditions.  This series adds the helper functions used to receive
a dirty bitmap snapshot, which is used by the display adapters then.
Also a bunch of display adapters are converted to use those helpers.

Changes from previous version:  Fixed one vga patch.  Left out are the
sparc adapters for now (Mark Cave-Ayland is busy cleaning them up and
fixing them).  Also left out the patch dropping the temporary
workaround, that obviously has to wait until all display adapters are
switched over.

please review,
  Gerd

Gerd Hoffmann (9):
  bitmap: add bitmap_copy_and_clear_atomic
  memory: add support getting and using a dirty bitmap copy.
  vga: add vga_scanline_invalidated helper
  vga: make display updates thread safe.
  framebuffer: make display updates thread safe
  exynos: make display updates thread safe
  g364fb: make display updates thread safe
  vmsvga: fix vmsvga_update_display
  sm501: make display updates thread safe

 include/exec/memory.h        | 47 +++++++++++++++++++++++++++
 include/exec/ram_addr.h      |  7 +++++
 include/qemu/bitmap.h        |  2 ++
 include/qemu/typedefs.h      |  1 +
 exec.c                       | 75 ++++++++++++++++++++++++++++++++++++++++++++
 hw/display/exynos4210_fimd.c | 11 ++++---
 hw/display/framebuffer.c     | 11 +++----
 hw/display/g364fb.c          | 28 +++--------------
 hw/display/sm501.c           | 23 ++++----------
 hw/display/vga.c             | 50 ++++++++++++++++-------------
 hw/display/vmware_vga.c      | 21 ++-----------
 memory.c                     | 17 ++++++++++
 util/bitmap.c                | 11 +++++++
 13 files changed, 213 insertions(+), 91 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH 1/9] bitmap: add bitmap_copy_and_clear_atomic

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/qemu/bitmap.h |  2 ++
 util/bitmap.c         | 11 +++++++++++
 2 files changed, 13 insertions(+)

diff --git a/include/qemu/bitmap.h b/include/qemu/bitmap.h
index 63ea2d0b1e..c318da12d7 100644
--- a/include/qemu/bitmap.h
+++ b/include/qemu/bitmap.h
@@ -220,6 +220,8 @@ void bitmap_set(unsigned long *map, long i, long len);
 void bitmap_set_atomic(unsigned long *map, long i, long len);
 void bitmap_clear(unsigned long *map, long start, long nr);
 bool bitmap_test_and_clear_atomic(unsigned long *map, long start, long nr);
+void bitmap_copy_and_clear_atomic(unsigned long *dst, unsigned long *src,
+                                  long nr);
 unsigned long bitmap_find_next_zero_area(unsigned long *map,
                                          unsigned long size,
                                          unsigned long start,
diff --git a/util/bitmap.c b/util/bitmap.c
index c1a84ca5e3..efced9a7d8 100644
--- a/util/bitmap.c
+++ b/util/bitmap.c
@@ -287,6 +287,17 @@ bool bitmap_test_and_clear_atomic(unsigned long *map, long start, long nr)
     return dirty != 0;
 }
 
+void bitmap_copy_and_clear_atomic(unsigned long *dst, unsigned long *src,
+                                  long nr)
+{
+    while (nr > 0) {
+        *dst = atomic_xchg(src, 0);
+        dst++;
+        src++;
+        nr -= BITS_PER_LONG;
+    }
+}
+
 #define ALIGN_MASK(x,mask)      (((x)+(mask))&~(mask))
 
 /**
-- 
2.9.3

[Qemu-devel] [PATCH 2/9] memory: add support getting and using a dirty bitmap copy.

[Gerd Hoffmann]

This patch adds support for getting and using a local copy of the dirty
bitmap.

memory_region_snapshot_and_clear_dirty() will create a snapshot of the
dirty bitmap for the specified range, clear the dirty bitmap and return
the copy.  The returned bitmap can be a bit larger than requested, the
range is expanded so the code can copy unsigned longs from the bitmap
and avoid atomic bit update operations.

memory_region_snapshot_get_dirty() will return the dirty status of
pages, pretty much like memory_region_get_dirty(), but using the copy
returned by memory_region_copy_and_clear_dirty().

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/exec/memory.h   | 47 +++++++++++++++++++++++++++++++
 include/exec/ram_addr.h |  7 +++++
 include/qemu/typedefs.h |  1 +
 exec.c                  | 75 +++++++++++++++++++++++++++++++++++++++++++++++++
 memory.c                | 17 +++++++++++
 5 files changed, 147 insertions(+)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index f20b191793..1e15e79d00 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -871,6 +871,53 @@ void memory_region_set_dirty(MemoryRegion *mr, hwaddr addr,
  */
 bool memory_region_test_and_clear_dirty(MemoryRegion *mr, hwaddr addr,
                                         hwaddr size, unsigned client);
+
+/**
+ * memory_region_snapshot_and_clear_dirty: Get a snapshot of the dirty
+ *                                         bitmap and clear it.
+ *
+ * Creates a snapshot of the dirty bitmap, clears the dirty bitmap and
+ * returns the snapshot.  The snapshot can then be used to query dirty
+ * status, using memory_region_snapshot_get_dirty.  Unlike
+ * memory_region_test_and_clear_dirty this allows to query the same
+ * page multiple times, which is especially useful for display updates
+ * where the scanlines often are not page aligned.
+ *
+ * The dirty bitmap region which gets copyed into the snapshot (and
+ * cleared afterwards) can be larger than requested.  The boundaries
+ * are rounded up/down so complete bitmap longs (covering 64 pages on
+ * 64bit hosts) can be copied over into the bitmap snapshot.  Which
+ * isn't a problem for display updates as the extra pages are outside
+ * the visible area, and in case the visible area changes a full
+ * display redraw is due anyway.  Should other use cases for this
+ * function emerge we might have to revisit this implementation
+ * detail.
+ *
+ * Use g_free to release DirtyBitmapSnapshot.
+ *
+ * @mr: the memory region being queried.
+ * @addr: the address (relative to the start of the region) being queried.
+ * @size: the size of the range being queried.
+ * @client: the user of the logging information; typically %DIRTY_MEMORY_VGA.
+ */
+DirtyBitmapSnapshot *memory_region_snapshot_and_clear_dirty(MemoryRegion *mr,
+                                                            hwaddr addr,
+                                                            hwaddr size,
+                                                            unsigned client);
+
+/**
+ * memory_region_snapshot_get_dirty: Check whether a range of bytes is dirty
+ *                                   in the specified dirty bitmap snapshot.
+ *
+ * @mr: the memory region being queried.
+ * @snap: the dirty bitmap snapshot
+ * @addr: the address (relative to the start of the region) being queried.
+ * @size: the size of the range being queried.
+ */
+bool memory_region_snapshot_get_dirty(MemoryRegion *mr,
+                                      DirtyBitmapSnapshot *snap,
+                                      hwaddr addr, hwaddr size);
+
 /**
  * memory_region_sync_dirty_bitmap: Synchronize a region's dirty bitmap with
  *                                  any external TLBs (e.g. kvm)
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index b05dc84ab9..2b63d7f59e 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -343,6 +343,13 @@ bool cpu_physical_memory_test_and_clear_dirty(ram_addr_t start,
                                               ram_addr_t length,
                                               unsigned client);
 
+DirtyBitmapSnapshot *cpu_physical_memory_snapshot_and_clear_dirty
+    (ram_addr_t start, ram_addr_t length, unsigned client);
+
+bool cpu_physical_memory_snapshot_get_dirty(DirtyBitmapSnapshot *snap,
+                                            ram_addr_t start,
+                                            ram_addr_t length);
+
 static inline void cpu_physical_memory_clear_dirty_range(ram_addr_t start,
                                                          ram_addr_t length)
 {
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index e95f28cfec..f08d327aec 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -23,6 +23,7 @@ typedef struct CPUAddressSpace CPUAddressSpace;
 typedef struct CPUState CPUState;
 typedef struct DeviceListener DeviceListener;
 typedef struct DeviceState DeviceState;
+typedef struct DirtyBitmapSnapshot DirtyBitmapSnapshot;
 typedef struct DisplayChangeListener DisplayChangeListener;
 typedef struct DisplayState DisplayState;
 typedef struct DisplaySurface DisplaySurface;
diff --git a/exec.c b/exec.c
index c97ef4a8da..a8894d5ba2 100644
--- a/exec.c
+++ b/exec.c
@@ -223,6 +223,12 @@ struct CPUAddressSpace {
     MemoryListener tcg_as_listener;
 };
 
+struct DirtyBitmapSnapshot {
+    ram_addr_t start;
+    ram_addr_t end;
+    unsigned long dirty[];
+};
+
 #endif
 
 #if !defined(CONFIG_USER_ONLY)
@@ -1061,6 +1067,75 @@ bool cpu_physical_memory_test_and_clear_dirty(ram_addr_t start,
     return dirty;
 }
 
+DirtyBitmapSnapshot *cpu_physical_memory_snapshot_and_clear_dirty
+     (ram_addr_t start, ram_addr_t length, unsigned client)
+{
+    DirtyMemoryBlocks *blocks;
+    unsigned long align = 1UL << (TARGET_PAGE_BITS + BITS_PER_LEVEL);
+    ram_addr_t first = QEMU_ALIGN_DOWN(start, align);
+    ram_addr_t last  = QEMU_ALIGN_UP(start + length, align);
+    DirtyBitmapSnapshot *snap;
+    unsigned long page, end, dest;
+
+    snap = g_malloc0(sizeof(*snap) +
+                     ((last - first) >> (TARGET_PAGE_BITS + 3)));
+    snap->start = first;
+    snap->end   = last;
+
+    page = first >> TARGET_PAGE_BITS;
+    end  = last  >> TARGET_PAGE_BITS;
+    dest = 0;
+
+    rcu_read_lock();
+
+    blocks = atomic_rcu_read(&ram_list.dirty_memory[client]);
+
+    while (page < end) {
+        unsigned long idx = page / DIRTY_MEMORY_BLOCK_SIZE;
+        unsigned long offset = page % DIRTY_MEMORY_BLOCK_SIZE;
+        unsigned long num = MIN(end - page, DIRTY_MEMORY_BLOCK_SIZE - offset);
+
+        assert(QEMU_IS_ALIGNED(offset, (1 << BITS_PER_LEVEL)));
+        assert(QEMU_IS_ALIGNED(num,    (1 << BITS_PER_LEVEL)));
+        offset >>= BITS_PER_LEVEL;
+
+        bitmap_copy_and_clear_atomic(snap->dirty + dest,
+                                     blocks->blocks[idx] + offset,
+                                     num);
+        page += num;
+        dest += num >> BITS_PER_LEVEL;
+    }
+
+    rcu_read_unlock();
+
+    if (tcg_enabled()) {
+        tlb_reset_dirty_range_all(start, length);
+    }
+
+    return snap;
+}
+
+bool cpu_physical_memory_snapshot_get_dirty(DirtyBitmapSnapshot *snap,
+                                            ram_addr_t start,
+                                            ram_addr_t length)
+{
+    unsigned long page, end;
+
+    assert(start >= snap->start);
+    assert(start + length <= snap->end);
+
+    end = TARGET_PAGE_ALIGN(start + length - snap->start) >> TARGET_PAGE_BITS;
+    page = (start - snap->start) >> TARGET_PAGE_BITS;
+
+    while (page < end) {
+        if (test_bit(page, snap->dirty)) {
+            return true;
+        }
+        page++;
+    }
+    return false;
+}
+
 /* Called from RCU critical section */
 hwaddr memory_region_section_get_iotlb(CPUState *cpu,
                                        MemoryRegionSection *section,
diff --git a/memory.c b/memory.c
index 4c95aaf39c..8a0648551f 100644
--- a/memory.c
+++ b/memory.c
@@ -1716,6 +1716,23 @@ bool memory_region_test_and_clear_dirty(MemoryRegion *mr, hwaddr addr,
                 memory_region_get_ram_addr(mr) + addr, size, client);
 }
 
+DirtyBitmapSnapshot *memory_region_snapshot_and_clear_dirty(MemoryRegion *mr,
+                                                            hwaddr addr,
+                                                            hwaddr size,
+                                                            unsigned client)
+{
+    assert(mr->ram_block);
+    return cpu_physical_memory_snapshot_and_clear_dirty(
+                memory_region_get_ram_addr(mr) + addr, size, client);
+}
+
+bool memory_region_snapshot_get_dirty(MemoryRegion *mr, DirtyBitmapSnapshot *snap,
+                                      hwaddr addr, hwaddr size)
+{
+    assert(mr->ram_block);
+    return cpu_physical_memory_snapshot_get_dirty(snap,
+                memory_region_get_ram_addr(mr) + addr, size);
+}
 
 void memory_region_sync_dirty_bitmap(MemoryRegion *mr)
 {
-- 
2.9.3

[Qemu-devel] [PATCH 3/9] vga: add vga_scanline_invalidated helper

[Gerd Hoffmann]

Add vga_scanline_invalidated helper to check whenever a scanline was
invalidated.  Add a sanity check to fix OOB read access for display
heights larger than 2048.

Only cirrus uses this, for hardware cursor rendering, so having this
work properly for the first 2048 scanlines only shouldn't be a problem
as the cirrus can't handle large resolutions anyway.  Also changing the
invalidated_y_table size would break live migration.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/vga.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index 69c3e1d674..3991b88aac 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -1434,6 +1434,14 @@ void vga_invalidate_scanlines(VGACommonState *s, int y1, int y2)
     }
 }
 
+static bool vga_scanline_invalidated(VGACommonState *s, int y)
+{
+    if (y >= VGA_MAX_HEIGHT) {
+        return false;
+    }
+    return s->invalidated_y_table[y >> 5] & (1 << (y & 0x1f));
+}
+
 void vga_sync_dirty_bitmap(VGACommonState *s)
 {
     memory_region_sync_dirty_bitmap(&s->vram);
@@ -1638,8 +1646,8 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
         page1 = addr + bwidth - 1;
         update |= memory_region_get_dirty(&s->vram, page0, page1 - page0,
                                           DIRTY_MEMORY_VGA);
-        /* explicit invalidation for the hardware cursor */
-        update |= (s->invalidated_y_table[y >> 5] >> (y & 0x1f)) & 1;
+        /* explicit invalidation for the hardware cursor (cirrus only) */
+        update |= vga_scanline_invalidated(s, y);
         if (update) {
             if (y_start < 0)
                 y_start = y;
@@ -1686,7 +1694,7 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
                                   page_max - page_min,
                                   DIRTY_MEMORY_VGA);
     }
-    memset(s->invalidated_y_table, 0, ((height + 31) >> 5) * 4);
+    memset(s->invalidated_y_table, 0, sizeof(s->invalidated_y_table));
 }
 
 static void vga_draw_blank(VGACommonState *s, int full_update)
-- 
2.9.3

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

[Gerd Hoffmann]

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);
+        }
         /* 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

[Qemu-devel] [PATCH 5/9] framebuffer: make display updates thread safe

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/framebuffer.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/hw/display/framebuffer.c b/hw/display/framebuffer.c
index 25aa46c8c7..d7310d25f2 100644
--- a/hw/display/framebuffer.c
+++ b/hw/display/framebuffer.c
@@ -67,7 +67,7 @@ void framebuffer_update_display(
     int *first_row, /* Input and output.  */
     int *last_row /* Output only */)
 {
-    hwaddr src_len;
+    DirtyBitmapSnapshot *snap;
     uint8_t *dest;
     uint8_t *src;
     int first, last = 0;
@@ -78,7 +78,6 @@ void framebuffer_update_display(
 
     i = *first_row;
     *first_row = -1;
-    src_len = (hwaddr)src_width * rows;
 
     mem = mem_section->mr;
     if (!mem) {
@@ -102,9 +101,10 @@ void framebuffer_update_display(
     src += i * src_width;
     dest += i * dest_row_pitch;
 
+    snap = memory_region_snapshot_and_clear_dirty(mem, addr, src_width * rows,
+                                                  DIRTY_MEMORY_VGA);
     for (; i < rows; i++) {
-        dirty = memory_region_get_dirty(mem, addr, src_width,
-                                             DIRTY_MEMORY_VGA);
+        dirty = memory_region_snapshot_get_dirty(mem, snap, addr, src_width);
         if (dirty || invalidate) {
             fn(opaque, dest, src, cols, dest_col_pitch);
             if (first == -1)
@@ -115,11 +115,10 @@ void framebuffer_update_display(
         src += src_width;
         dest += dest_row_pitch;
     }
+    g_free(snap);
     if (first < 0) {
         return;
     }
-    memory_region_reset_dirty(mem, mem_section->offset_within_region, src_len,
-                              DIRTY_MEMORY_VGA);
     *first_row = first;
     *last_row = last;
 }
-- 
2.9.3

[Qemu-devel] [PATCH 6/9] exynos: make display updates thread safe

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/exynos4210_fimd.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/display/exynos4210_fimd.c b/hw/display/exynos4210_fimd.c
index e5be713406..fd0b2bec65 100644
--- a/hw/display/exynos4210_fimd.c
+++ b/hw/display/exynos4210_fimd.c
@@ -1263,6 +1263,7 @@ static void exynos4210_fimd_update(void *opaque)
     Exynos4210fimdState *s = (Exynos4210fimdState *)opaque;
     DisplaySurface *surface;
     Exynos4210fimdWindow *w;
+    DirtyBitmapSnapshot *snap;
     int i, line;
     hwaddr fb_line_addr, inc_size;
     int scrn_height;
@@ -1291,10 +1292,12 @@ static void exynos4210_fimd_update(void *opaque)
             memory_region_sync_dirty_bitmap(w->mem_section.mr);
             host_fb_addr = w->host_fb_addr;
             fb_line_addr = w->mem_section.offset_within_region;
+            snap = memory_region_snapshot_and_clear_dirty(w->mem_section.mr,
+                    fb_line_addr, inc_size * scrn_height, DIRTY_MEMORY_VGA);
 
             for (line = 0; line < scrn_height; line++) {
-                is_dirty = memory_region_get_dirty(w->mem_section.mr,
-                            fb_line_addr, scrn_width, DIRTY_MEMORY_VGA);
+                is_dirty = memory_region_snapshot_get_dirty(w->mem_section.mr,
+                            snap, fb_line_addr, scrn_width);
 
                 if (s->invalidate || is_dirty) {
                     if (first_line == -1) {
@@ -1309,9 +1312,7 @@ static void exynos4210_fimd_update(void *opaque)
                 fb_line_addr += inc_size;
                 is_dirty = false;
             }
-            memory_region_reset_dirty(w->mem_section.mr,
-                w->mem_section.offset_within_region,
-                w->fb_len, DIRTY_MEMORY_VGA);
+            g_free(snap);
             blend = true;
         }
     }
-- 
2.9.3

[Qemu-devel] [PATCH 7/9] g364fb: make display updates thread safe

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/g364fb.c | 28 +++++-----------------------
 1 file changed, 5 insertions(+), 23 deletions(-)

diff --git a/hw/display/g364fb.c b/hw/display/g364fb.c
index 8cdc205dd9..86557d14a9 100644
--- a/hw/display/g364fb.c
+++ b/hw/display/g364fb.c
@@ -64,17 +64,8 @@ typedef struct G364State {
 
 static inline int check_dirty(G364State *s, ram_addr_t page)
 {
-    return memory_region_get_dirty(&s->mem_vram, page, G364_PAGE_SIZE,
-                                   DIRTY_MEMORY_VGA);
-}
-
-static inline void reset_dirty(G364State *s,
-                               ram_addr_t page_min, ram_addr_t page_max)
-{
-    memory_region_reset_dirty(&s->mem_vram,
-                              page_min,
-                              page_max + G364_PAGE_SIZE - page_min - 1,
-                              DIRTY_MEMORY_VGA);
+    return memory_region_test_and_clear_dirty(&s->mem_vram, page, G364_PAGE_SIZE,
+                                              DIRTY_MEMORY_VGA);
 }
 
 static void g364fb_draw_graphic8(G364State *s)
@@ -83,7 +74,7 @@ static void g364fb_draw_graphic8(G364State *s)
     int i, w;
     uint8_t *vram;
     uint8_t *data_display, *dd;
-    ram_addr_t page, page_min, page_max;
+    ram_addr_t page;
     int x, y;
     int xmin, xmax;
     int ymin, ymax;
@@ -114,8 +105,6 @@ static void g364fb_draw_graphic8(G364State *s)
     }
 
     page = 0;
-    page_min = (ram_addr_t)-1;
-    page_max = 0;
 
     x = y = 0;
     xmin = s->width;
@@ -137,9 +126,6 @@ static void g364fb_draw_graphic8(G364State *s)
         if (check_dirty(s, page)) {
             if (y < ymin)
                 ymin = ymax = y;
-            if (page_min == (ram_addr_t)-1)
-                page_min = page;
-            page_max = page;
             if (x < xmin)
                 xmin = x;
             for (i = 0; i < G364_PAGE_SIZE; i++) {
@@ -196,10 +182,7 @@ static void g364fb_draw_graphic8(G364State *s)
                 ymax = y;
         } else {
             int dy;
-            if (page_min != (ram_addr_t)-1) {
-                reset_dirty(s, page_min, page_max);
-                page_min = (ram_addr_t)-1;
-                page_max = 0;
+            if (xmax || ymax) {
                 dpy_gfx_update(s->con, xmin, ymin,
                                xmax - xmin + 1, ymax - ymin + 1);
                 xmin = s->width;
@@ -219,9 +202,8 @@ static void g364fb_draw_graphic8(G364State *s)
     }
 
 done:
-    if (page_min != (ram_addr_t)-1) {
+    if (xmax || ymax) {
         dpy_gfx_update(s->con, xmin, ymin, xmax - xmin + 1, ymax - ymin + 1);
-        reset_dirty(s, page_min, page_max);
     }
 }
 
-- 
2.9.3

[Qemu-devel] [PATCH 8/9] vmsvga: fix vmsvga_update_display

[Gerd Hoffmann]

Fix standard vga mode check:  Both s->config and s->enabled must be set
to enable vmware command fifo processing.

Drop dirty tracking code from the fifo rendering code path, it isn't
used anyway because vmsvga turns off dirty tracking when leaving
standard vga mode.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/vmware_vga.c | 21 +++------------------
 1 file changed, 3 insertions(+), 18 deletions(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index 6599cf078d..ec5f27d67e 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -1118,9 +1118,9 @@ static void vmsvga_update_display(void *opaque)
 {
     struct vmsvga_state_s *s = opaque;
     DisplaySurface *surface;
-    bool dirty = false;
 
-    if (!s->enable) {
+    if (!s->enable || !s->config) {
+        /* in standard vga mode */
         s->vga.hw_ops->gfx_update(&s->vga);
         return;
     }
@@ -1131,26 +1131,11 @@ static void vmsvga_update_display(void *opaque)
     vmsvga_fifo_run(s);
     vmsvga_update_rect_flush(s);
 
-    /*
-     * Is it more efficient to look at vram VGA-dirty bits or wait
-     * for the driver to issue SVGA_CMD_UPDATE?
-     */
-    if (memory_region_is_logging(&s->vga.vram, DIRTY_MEMORY_VGA)) {
-        vga_sync_dirty_bitmap(&s->vga);
-        dirty = memory_region_get_dirty(&s->vga.vram, 0,
-            surface_stride(surface) * surface_height(surface),
-            DIRTY_MEMORY_VGA);
-    }
-    if (s->invalidated || dirty) {
+    if (s->invalidated) {
         s->invalidated = 0;
         dpy_gfx_update(s->vga.con, 0, 0,
                    surface_width(surface), surface_height(surface));
     }
-    if (dirty) {
-        memory_region_reset_dirty(&s->vga.vram, 0,
-            surface_stride(surface) * surface_height(surface),
-            DIRTY_MEMORY_VGA);
-    }
 }
 
 static void vmsvga_reset(DeviceState *dev)
-- 
2.9.3

[Qemu-devel] [PATCH 9/9] sm501: make display updates thread safe

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/sm501.c | 23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 040a0b93f2..1987a537c0 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -1259,6 +1259,7 @@ static inline int get_depth_index(DisplaySurface *surface)
 static void sm501_draw_crt(SM501State * s)
 {
     DisplaySurface *surface = qemu_console_surface(s->con);
+    DirtyBitmapSnapshot *snap;
     int y;
     int width = (s->dc_crt_h_total & 0x00000FFF) + 1;
     int height = (s->dc_crt_v_total & 0x00000FFF) + 1;
@@ -1274,8 +1275,6 @@ static void sm501_draw_crt(SM501State * s)
     draw_hwc_line_func * draw_hwc_line = NULL;
     int full_update = 0;
     int y_start = -1;
-    ram_addr_t page_min = ~0l;
-    ram_addr_t page_max = 0l;
     ram_addr_t offset = 0;
 
     /* choose draw_line function */
@@ -1326,15 +1325,15 @@ static void sm501_draw_crt(SM501State * s)
 
     /* draw each line according to conditions */
     memory_region_sync_dirty_bitmap(&s->local_mem_region);
+    snap = memory_region_snapshot_and_clear_dirty(&s->local_mem_region,
+              offset, width * height * src_bpp, DIRTY_MEMORY_VGA);
     for (y = 0; y < height; y++) {
 	int update_hwc = draw_hwc_line ? within_hwc_y_range(s, y, 1) : 0;
 	int update = full_update || update_hwc;
-        ram_addr_t page0 = offset;
-        ram_addr_t page1 = offset + width * src_bpp - 1;
 
 	/* check dirty flags for each line */
-        update = memory_region_get_dirty(&s->local_mem_region, page0,
-                                         page1 - page0, DIRTY_MEMORY_VGA);
+        update = memory_region_snapshot_get_dirty(&s->local_mem_region, snap,
+                                                  offset, width * src_bpp);
 
 	/* draw line and change status */
 	if (update) {
@@ -1351,10 +1350,6 @@ static void sm501_draw_crt(SM501State * s)
 
 	    if (y_start < 0)
 		y_start = y;
-	    if (page0 < page_min)
-		page_min = page0;
-	    if (page1 > page_max)
-		page_max = page1;
 	} else {
 	    if (y_start >= 0) {
 		/* flush to display */
@@ -1366,17 +1361,11 @@ static void sm501_draw_crt(SM501State * s)
 	src += width * src_bpp;
 	offset += width * src_bpp;
     }
+    g_free(snap);
 
     /* complete flush to display */
     if (y_start >= 0)
         dpy_gfx_update(s->con, 0, y_start, width, y - y_start);
-
-    /* clear dirty flags */
-    if (page_min != ~0l) {
-	memory_region_reset_dirty(&s->local_mem_region,
-                                  page_min, page_max + TARGET_PAGE_SIZE,
-                                  DIRTY_MEMORY_VGA);
-    }
 }
 
 static void sm501_update_display(void *opaque)
-- 
2.9.3

Re: [Qemu-devel] [PATCH 9/9] sm501: make display updates thread safe

[BALATON Zoltan]

On Fri, 21 Apr 2017, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> hw/display/sm501.c | 23 ++++++-----------------
> 1 file changed, 6 insertions(+), 17 deletions(-)

Hold on with this please too. I have a series pending that changes this.

Regards,
BALATON Zoltan

Re: [Qemu-devel] [PATCH 0/9] hw/display: make display updates thread safe, part 1

[Mark Cave-Ayland]

On 21/04/17 10:16, Gerd Hoffmann wrote:

>   Hi,
> 
> Ok, 2.10 is open, lets start tackling the display update race
> conditions.  This series adds the helper functions used to receive
> a dirty bitmap snapshot, which is used by the display adapters then.
> Also a bunch of display adapters are converted to use those helpers.
> 
> Changes from previous version:  Fixed one vga patch.  Left out are the
> sparc adapters for now (Mark Cave-Ayland is busy cleaning them up and
> fixing them).  Also left out the patch dropping the temporary
> workaround, that obviously has to wait until all display adapters are
> switched over.

FWIW I'm going to send a pull request for the SPARC adapter cleanups
later today as I'll be AFK for the next week and don't want to hold you up.

I've also rebased my TCX/CG3 "make display updates thread-safe" patches
onto my v2 cleanup patchset and pushed to result to
https://github.com/mcayland/qemu/tree/vga-fixes-sparc so that in my
absence you can cherry-pick the cg3/tcx conversion patches into your
series while I'm away.


ATB,

Mark.

Re: [Qemu-devel] [PATCH 9/9] sm501: make display updates thread safe

[Peter Maydell]

On 21 April 2017 at 11:42, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> On Fri, 21 Apr 2017, Gerd Hoffmann wrote:
>>
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> ---
>> hw/display/sm501.c | 23 ++++++-----------------
>> 1 file changed, 6 insertions(+), 17 deletions(-)
>
>
> Hold on with this please too. I have a series pending that changes this.

Gerd: I've now applied Zoltan's patchset to git master, so you'll
want to rebase your changes on it.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 9/9] sm501: make display updates thread safe

[Gerd Hoffmann]

On Mo, 2017-04-24 at 13:09 +0100, Peter Maydell wrote:
> On 21 April 2017 at 11:42, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> > On Fri, 21 Apr 2017, Gerd Hoffmann wrote:
> >>
> >> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> >> ---
> >> hw/display/sm501.c | 23 ++++++-----------------
> >> 1 file changed, 6 insertions(+), 17 deletions(-)
> >
> >
> > Hold on with this please too. I have a series pending that changes this.
> 
> Gerd: I've now applied Zoltan's patchset to git master, so you'll
> want to rebase your changes on it.

Pull request for the first batch is just out of the door, without sm501
+ tcx + cg3 to avoid conflicts.  They will follow with the next batch,
which then should also able to revert the temporary workaround.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 2/9] memory: add support getting and using a dirty bitmap copy.

[Kevin Wolf]

Am 21.04.2017 um 11:16 hat Gerd Hoffmann geschrieben:
> +bool cpu_physical_memory_snapshot_get_dirty(DirtyBitmapSnapshot *snap,
> +                                            ram_addr_t start,
> +                                            ram_addr_t length)
> +{
> +    unsigned long page, end;
> +
> +    assert(start >= snap->start);
> +    assert(start + length <= snap->end);

Not sure if this has been reported somewhere else, but I got an
assertion failure here while booting a guest:

$ ~/source/qemu/x86_64-softmmu/qemu-system-x86_64 -enable-kvm -m 2G -drive file=Windows-10-20170427.0-x86_64.qcow2,snapshot=on -usbdevice tablet -vga qxl
qemu-system-x86_64: /home/kwolf/source/qemu/exec.c:1125: cpu_physical_memory_snapshot_get_dirty: Zusicherung >>start + length <= snap->end<< nicht erf?llt.
Abgebrochen (Speicherabzug geschrieben)

Unfortunately, I didn't have gdb attached or core dumps enabled, and it
doesn't seem to reproduce easily, so I don't have anything that could
help debugging it, but I thought I'd just let you know anyway.

Kevin

Re: [Qemu-devel] [PATCH 2/9] memory: add support getting and using a dirty bitmap copy.

[Gerd Hoffmann]

On Do, 2017-04-27 at 16:00 +0200, Kevin Wolf wrote:
> Am 21.04.2017 um 11:16 hat Gerd Hoffmann geschrieben:
> > +bool cpu_physical_memory_snapshot_get_dirty(DirtyBitmapSnapshot *snap,
> > +                                            ram_addr_t start,
> > +                                            ram_addr_t length)
> > +{
> > +    unsigned long page, end;
> > +
> > +    assert(start >= snap->start);
> > +    assert(start + length <= snap->end);
> 
> Not sure if this has been reported somewhere else, but I got an
> assertion failure here while booting a guest:
> 
> $ ~/source/qemu/x86_64-softmmu/qemu-system-x86_64 -enable-kvm -m 2G -drive file=Windows-10-20170427.0-x86_64.qcow2,snapshot=on -usbdevice tablet -vga qxl
> qemu-system-x86_64: /home/kwolf/source/qemu/exec.c:1125: cpu_physical_memory_snapshot_get_dirty: Zusicherung >>start + length <= snap->end<< nicht erf?llt.
> Abgebrochen (Speicherabzug geschrieben)
> 
> Unfortunately, I didn't have gdb attached or core dumps enabled, and it
> doesn't seem to reproduce easily, so I don't have anything that could
> help debugging it, but I thought I'd just let you know anyway.

Saw this a few times too, didn't have the time yet to dig deeper,
appears to happen due to a display update when the guest is half-way
through a mode switch and the vga registers are in inconsistent state.

Reproducer: boot fedora live iso, when isolinux switches back to text
mode it can trigger (one out of ten boots, loaded host seems to make it
more likely)

The easy way out is to just return false instead of asserting.

I want check how exactly this happens though, to make sure this isn't a
exploitable race (unlikely IMHO, but still worth checking ...).  Also
I'd prefer to fix vga and keep the assert()s, they are a good sanity
check.

cheers,
  Gerd

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

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

[Gerd Hoffmann]

  Hi,

> #4 cpu_physical_memory_snapshot_get_dirty (snap=0x555556a3b3d0,
> start=2148532224, length=511)

https://patchwork.ozlabs.org/patch/760013/

cheers,
  Gerd

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

[Ladi Prosek]

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