[Qemu-devel] [RfC PATCH 0/4] make display updates thread safe.

[Qemu-devel] [RfC PATCH 0/4] make display updates thread safe.

[Gerd Hoffmann]

  Hi,

First attempt on making display updates thread-save for real.  Most
interesting patches at this point are #2 (adds helper functions to
create and use a dirty bitmap copy) and #3 (updates vga code to use
them).

Patch #1 fixes a bug I've noticed while wading through the vga code,
and #4 removes the temporary workaround for testing purposes.  Which
will of course break some or all non-vga display adapters as those
are not ported over yet.

git branch is available here:
  git://git.kraxel.org/qemu work/vga-fixes

Gerd Hoffmann (4):
  vga: add vga_scanline_invalidated helper
  memory: add support getting and using a dirty bitmap copy.
  vga: make display updates thread safe.
  [testing] console: remove do_safe_dpy_refresh

 exec.c                  | 79 +++++++++++++++++++++++++++++++++++++++++++++++++
 hw/display/vga.c        | 50 +++++++++++++++++--------------
 include/exec/memory.h   | 13 ++++++++
 include/exec/ram_addr.h |  8 +++++
 include/qemu/typedefs.h |  1 +
 memory.c                | 15 ++++++++++
 ui/console.c            | 25 +---------------
 7 files changed, 145 insertions(+), 46 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [RfC PATCH 1/4] 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 69c3e1d..f320946 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)
-- 
1.8.3.1

[Qemu-devel] [RfC PATCH 2/4] 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_copy_and_clear_dirty() will create a copy 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_copy_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>
---
 exec.c                  | 79 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/exec/memory.h   | 13 ++++++++
 include/exec/ram_addr.h |  8 +++++
 include/qemu/typedefs.h |  1 +
 memory.c                | 15 ++++++++++
 5 files changed, 116 insertions(+)

diff --git a/exec.c b/exec.c
index e57a8a2..57c5bc9 100644
--- a/exec.c
+++ b/exec.c
@@ -223,6 +223,12 @@ struct CPUAddressSpace {
     MemoryListener tcg_as_listener;
 };
 
+struct DirtyCopy {
+    ram_addr_t start;
+    ram_addr_t end;
+    unsigned long dirty[];
+};
+
 #endif
 
 #if !defined(CONFIG_USER_ONLY)
@@ -1061,6 +1067,79 @@ bool cpu_physical_memory_test_and_clear_dirty(ram_addr_t start,
     return dirty;
 }
 
+DirtyCopy *cpu_physical_memory_copy_and_clear_dirty(ram_addr_t start,
+                                                    ram_addr_t length,
+                                                    unsigned client)
+{
+    DirtyMemoryBlocks *blocks;
+    unsigned long align = 1 << (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);
+    DirtyCopy *copy;
+    unsigned long page, end, dest;
+
+    copy = g_malloc0(sizeof(*copy) +
+                     ((last - first) >> (TARGET_PAGE_BITS + 3)));
+    copy->start = first;
+    copy->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(copy->dirty + dest,
+                    blocks->blocks[idx] + (offset >> BITS_PER_LEVEL),
+                    num);
+        bitmap_zero(blocks->blocks[idx] + (offset >> BITS_PER_LEVEL),
+                    num);
+
+        page += num;
+        dest += num >> BITS_PER_LEVEL;
+    }
+
+    rcu_read_unlock();
+
+    if (tcg_enabled()) {
+        tlb_reset_dirty_range_all(start, length);
+    }
+
+    return copy;
+}
+
+bool cpu_physical_memory_copy_get_dirty(DirtyCopy *copy,
+                                        ram_addr_t start,
+                                        ram_addr_t length)
+{
+    unsigned long page, end;
+
+    assert(start > copy->start);
+    assert(start + length < copy->end);
+
+    end = TARGET_PAGE_ALIGN(start + length - copy->start) >> TARGET_PAGE_BITS;
+    page = (start - copy->start) >> TARGET_PAGE_BITS;
+
+    while (page < end) {
+        if (test_bit(page, copy->dirty)) {
+            return true;
+        }
+        page++;
+    }
+    return false;
+}
+
 /* Called from RCU critical section */
 hwaddr memory_region_section_get_iotlb(CPUState *cpu,
                                        MemoryRegionSection *section,
diff --git a/include/exec/memory.h b/include/exec/memory.h
index e39256a..f89cfa7 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -871,6 +871,19 @@ 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_copy_and_clear_dirty: TODO
+ */
+DirtyCopy *memory_region_copy_and_clear_dirty(MemoryRegion *mr, hwaddr addr,
+                                              hwaddr size, unsigned client);
+
+/**
+ * memory_region_copy_get_dirty: TODO
+ */
+bool memory_region_copy_get_dirty(MemoryRegion *mr, DirtyCopy *copy,
+                                  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 b05dc84..3886f0a 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -343,6 +343,14 @@ bool cpu_physical_memory_test_and_clear_dirty(ram_addr_t start,
                                               ram_addr_t length,
                                               unsigned client);
 
+DirtyCopy *cpu_physical_memory_copy_and_clear_dirty(ram_addr_t start,
+                                                    ram_addr_t length,
+                                                    unsigned client);
+
+bool cpu_physical_memory_copy_get_dirty(DirtyCopy *copy,
+                                        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 e95f28c..5344aee 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 DirtyCopy DirtyCopy;
 typedef struct DisplayChangeListener DisplayChangeListener;
 typedef struct DisplayState DisplayState;
 typedef struct DisplaySurface DisplaySurface;
diff --git a/memory.c b/memory.c
index 4c95aaf..7d75ca6 100644
--- a/memory.c
+++ b/memory.c
@@ -1716,6 +1716,21 @@ bool memory_region_test_and_clear_dirty(MemoryRegion *mr, hwaddr addr,
                 memory_region_get_ram_addr(mr) + addr, size, client);
 }
 
+DirtyCopy *memory_region_copy_and_clear_dirty(MemoryRegion *mr, hwaddr addr,
+                                              hwaddr size, unsigned client)
+{
+    assert(mr->ram_block);
+    return cpu_physical_memory_copy_and_clear_dirty(
+                memory_region_get_ram_addr(mr) + addr, size, client);
+}
+
+bool memory_region_copy_get_dirty(MemoryRegion *mr, DirtyCopy *copy,
+                                  hwaddr addr, hwaddr size)
+{
+    assert(mr->ram_block);
+    return cpu_physical_memory_copy_get_dirty(copy,
+                memory_region_get_ram_addr(mr) + addr, size);
+}
 
 void memory_region_sync_dirty_bitmap(MemoryRegion *mr)
 {
-- 
1.8.3.1

[Qemu-devel] [RfC PATCH 3/4] 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 f320946..7daccb8 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;
+    DirtyCopy *copy = 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);
+        copy = memory_region_copy_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_copy_get_dirty(&s->vram, copy,
+                                                  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(copy);
     memset(s->invalidated_y_table, 0, sizeof(s->invalidated_y_table));
 }
 
-- 
1.8.3.1

[Qemu-devel] [RfC PATCH 4/4] [testing] console: remove do_safe_dpy_refresh

[Gerd Hoffmann]

Drop the temporary workaround for the broken display updates.

TODO: before actually merging this we have to fix all the non-vga
      display adapters too ...

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/console.c | 25 +------------------------
 1 file changed, 1 insertion(+), 24 deletions(-)

diff --git a/ui/console.c b/ui/console.c
index 419b098..4c70d8b 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1575,36 +1575,13 @@ bool dpy_gfx_check_format(QemuConsole *con,
     return true;
 }
 
-/*
- * Safe DPY refresh for TCG guests. We use the exclusive mechanism to
- * ensure the TCG vCPUs are quiescent so we can avoid races between
- * dirty page tracking for direct frame-buffer access by the guest.
- *
- * This is a temporary stopgap until we've fixed the dirty tracking
- * races in display adapters.
- */
-static void do_safe_dpy_refresh(DisplayChangeListener *dcl)
-{
-    qemu_mutex_unlock_iothread();
-    start_exclusive();
-    qemu_mutex_lock_iothread();
-    dcl->ops->dpy_refresh(dcl);
-    qemu_mutex_unlock_iothread();
-    end_exclusive();
-    qemu_mutex_lock_iothread();
-}
-
 static void dpy_refresh(DisplayState *s)
 {
     DisplayChangeListener *dcl;
 
     QLIST_FOREACH(dcl, &s->listeners, next) {
         if (dcl->ops->dpy_refresh) {
-            if (tcg_enabled()) {
-                do_safe_dpy_refresh(dcl);
-            } else {
-                dcl->ops->dpy_refresh(dcl);
-            }
+            dcl->ops->dpy_refresh(dcl);
         }
     }
 }
-- 
1.8.3.1

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

[Mark Cave-Ayland]

On 30/03/17 07:55, Gerd Hoffmann wrote:

>   Hi,
> 
> First attempt on making display updates thread-save for real.  Most
> interesting patches at this point are #2 (adds helper functions to
> create and use a dirty bitmap copy) and #3 (updates vga code to use
> them).
> 
> Patch #1 fixes a bug I've noticed while wading through the vga code,
> and #4 removes the temporary workaround for testing purposes.  Which
> will of course break some or all non-vga display adapters as those
> are not ported over yet.
> 
> git branch is available here:
>   git://git.kraxel.org/qemu work/vga-fixes
> 
> Gerd Hoffmann (4):
>   vga: add vga_scanline_invalidated helper
>   memory: add support getting and using a dirty bitmap copy.
>   vga: make display updates thread safe.
>   [testing] console: remove do_safe_dpy_refresh
> 
>  exec.c                  | 79 +++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/display/vga.c        | 50 +++++++++++++++++--------------
>  include/exec/memory.h   | 13 ++++++++
>  include/exec/ram_addr.h |  8 +++++
>  include/qemu/typedefs.h |  1 +
>  memory.c                | 15 ++++++++++
>  ui/console.c            | 25 +---------------
>  7 files changed, 145 insertions(+), 46 deletions(-)

Excellent! I can help out with converting and/or testing the SPARC
devices (cg3/tcx) if required.

Out of interest, from your work do you have a rough estimate as to how
this affects guest performance? For example benchmarks with Jan's
original commit to reduce the locking vs. Alex's current workaround vs.
with this patchset applied?


ATB,

Mark.

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

[Gerd Hoffmann]

  Hi,

> Excellent! I can help out with converting and/or testing the SPARC
> devices (cg3/tcx) if required.

Sure, test results and patches are very welcome.

I've touched dirty bitmap code for the first time, so I've sent out this
rfc for early feedback at the approach taken before going out converting
more display drivers.  Seems nobody found patch #2 horrible so far, I
take that as good sign that we can go on with this draft API.

> Out of interest, from your work do you have a rough estimate as to how
> this affects guest performance? For example benchmarks with Jan's
> original commit to reduce the locking vs. Alex's current workaround vs.
> with this patchset applied?

I expect this improves performance with mttcg because the locking goes
away, which allows more concurrency, probably more visible with more
guest cores.  Didn't benchmark it though.

cheers,
  Gerd

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

[Paolo Bonzini]

On 30/03/2017 08:55, Gerd Hoffmann wrote:
> +        bitmap_copy(copy->dirty + dest,
> +                    blocks->blocks[idx] + (offset >> BITS_PER_LEVEL),
> +                    num);
> +        bitmap_zero(blocks->blocks[idx] + (offset >> BITS_PER_LEVEL),
> +                    num);
> +

This needs to access the bitmap atomically, so you'll need a new function

  bool bitmap_copy_and_clear_atomic(unsigned long *dest,
				    unsigned long *src, long nr)

Paolo

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

[Paolo Bonzini]

On 30/03/2017 08:55, Gerd Hoffmann wrote:
>  
> +struct DirtyCopy {
> +    ram_addr_t start;
> +    ram_addr_t end;
> +    unsigned long dirty[];
> +};
> +
>  #endif

Maybe DirtyBitmapSnapshot (and s/copy/snapshot/ in the rest of the API too)?

Paolo

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

[Mark Cave-Ayland]

On 30/03/17 14:41, Gerd Hoffmann wrote:

>   Hi,
> 
>> Excellent! I can help out with converting and/or testing the SPARC
>> devices (cg3/tcx) if required.
> 
> Sure, test results and patches are very welcome.

I've spent a bit of time over the weekend attempting to convert the
SPARC CG3/TCX framebuffers over to use your new code and pushed the
result to my github branch here:
https://github.com/mcayland/qemu/commits/vga-fixes-sparc.

Feel free to steal/test/borrow as required. The two main things I've
discovered to date are:

1) Problems with asserts() in cpu_physical_memory_snapshot_get_dirty()

Using your current git branch, I get an assert() using VGA under
qemu-system-ppc on startup unless I change the first assert to
"assert(start >= snap->start)".

I'm also not convinced by the second assert "assert(start + length <
snap->end)" either. In the example CG3/TCX conversions I ended up having
to subtract 1 from the size of the region passed to
memory_region_snapshot_get_dirty() in order to avoid it firing when
accessing the last scanline which seems wrong. I didn't explicitly add a
patch to change this to "<=" in my git patchset just in case it broke
further assumptions in the bitmap code relating to the end address.


2) Redraw issues with CG3/TCX after conversion

Despite the conversion from your patches looking reasonably
straightforward, I still see problems with areas of the screen not
updating after conversion. I can easily reproduce these locally as follows:

./qemu-system-sparc -vga cg3 -prom-env 'auto-boot?=false'
./qemu-system-sparc -vga tcx -g 1024x768x8 -prom-env 'auto-boot?=false'
./qemu-system-sparc -vga tcx -g 1024x768x24 -prom-env 'auto-boot?=false'

You can trigger extra screen updates by typing "show-devs" into OpenBIOS
which will cause a lot of screen activity as it writes the device tree
out to the framebuffer. I'm wondering if maybe this is related to the
difference in sizes between DIRTY_MEMORY_BLOCK_SIZE and TARGET_PAGE_BITS
for qemu-system-sparc?

Finally on a slightly unrelated note, the TCX driver still has old code
for 8 and 16 bpp surfaces in QEMU. Am I right in understanding that all
surfaces in QEMU are now 32-bit and the 8/16 bpp code can be removed?


ATB,

Mark.

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

[Gerd Hoffmann]

  Hi,

> I've spent a bit of time over the weekend attempting to convert the
> SPARC CG3/TCX framebuffers over to use your new code and pushed the
> result to my github branch here:
> https://github.com/mcayland/qemu/commits/vga-fixes-sparc.

work/vga-fixes branch updated & rebased to latest master, with your
patches added.

> Using your current git branch, I get an assert() using VGA under
> qemu-system-ppc on startup unless I change the first assert to
> "assert(start >= snap->start)".
> 
> I'm also not convinced by the second assert "assert(start + length <
> snap->end)" either. In the example CG3/TCX conversions I ended up having
> to subtract 1 from the size of the region passed to
> memory_region_snapshot_get_dirty() in order to avoid it firing when
> accessing the last scanline which seems wrong. I didn't explicitly add a
> patch to change this to "<=" in my git patchset just in case it broke
> further assumptions in the bitmap code relating to the end address.

Both asserts should use inclusive compare.  Added a patch fixing the
other assert too.

> 2) Redraw issues with CG3/TCX after conversion
> 
> Despite the conversion from your patches looking reasonably
> straightforward, I still see problems with areas of the screen not
> updating after conversion. I can easily reproduce these locally as follows:
> 
> ./qemu-system-sparc -vga cg3 -prom-env 'auto-boot?=false'
> ./qemu-system-sparc -vga tcx -g 1024x768x8 -prom-env 'auto-boot?=false'
> ./qemu-system-sparc -vga tcx -g 1024x768x24 -prom-env 'auto-boot?=false'

Hmm, I have a blank screen only (yellow with cg3, black with tcx).
No openfirmware prompt showing up ...

> Finally on a slightly unrelated note, the TCX driver still has old code
> for 8 and 16 bpp surfaces in QEMU. Am I right in understanding that all
> surfaces in QEMU are now 32-bit and the 8/16 bpp code can be removed?

Yes, that is correct.  tcx uses qemu_resize_console() so surfaces will
always be 32bpp.

cheers,
  Gerd

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

[Mark Cave-Ayland]

On 03/04/17 09:44, Gerd Hoffmann wrote:

>   Hi,
> 
>> I've spent a bit of time over the weekend attempting to convert the
>> SPARC CG3/TCX framebuffers over to use your new code and pushed the
>> result to my github branch here:
>> https://github.com/mcayland/qemu/commits/vga-fixes-sparc.
> 
> work/vga-fixes branch updated & rebased to latest master, with your
> patches added.

I've just rebased on your branch with some extra fixes which should be
squashed into the earlier commits and pushed to github again - see the
commit messages for more information.

>> 2) Redraw issues with CG3/TCX after conversion
>>
>> Despite the conversion from your patches looking reasonably
>> straightforward, I still see problems with areas of the screen not
>> updating after conversion. I can easily reproduce these locally as follows:
>>
>> ./qemu-system-sparc -vga cg3 -prom-env 'auto-boot?=false'
>> ./qemu-system-sparc -vga tcx -g 1024x768x8 -prom-env 'auto-boot?=false'
>> ./qemu-system-sparc -vga tcx -g 1024x768x24 -prom-env 'auto-boot?=false'
> 
> Hmm, I have a blank screen only (yellow with cg3, black with tcx).
> No openfirmware prompt showing up ...

Yeah it's slightly baffling. The easiest one to look at is cg3 since
it's a simple 8-bit palette dumb framebuffer with 256 colors, i.e. the
differences between 8d3af4a~1 and 8d3af4a ("cg3: make display updates
thread safe") in your updated branch. If nothing seems wrong with the
conversion (once my latest fixes have been squashed in) then I presume
something odd is happening in the dirty bitmap code...


ATB,

Mark.

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

[Paolo Bonzini]

On 30/03/2017 15:41, Gerd Hoffmann wrote:
>   Hi,
> 
>> Excellent! I can help out with converting and/or testing the SPARC
>> devices (cg3/tcx) if required.
> 
> Sure, test results and patches are very welcome.
> 
> I've touched dirty bitmap code for the first time, so I've sent out this
> rfc for early feedback at the approach taken before going out converting
> more display drivers.  Seems nobody found patch #2 horrible so far, I
> take that as good sign that we can go on with this draft API.
> 
>> Out of interest, from your work do you have a rough estimate as to how
>> this affects guest performance? For example benchmarks with Jan's
>> original commit to reduce the locking vs. Alex's current workaround vs.
>> with this patchset applied?
> 
> I expect this improves performance with mttcg because the locking goes
> away, which allows more concurrency, probably more visible with more
> guest cores.  Didn't benchmark it though.

I checked the branch, is bitmap_copy_and_clear_atomic correct when you
have partial updates?  Maybe you need to handle partial updates of the
first and last word?  Also, the first and last bit should optionally be
left untouched, for example:

first y: 20
last y: 99
stride: 320
bytes read: 6400..31999
snapshotted pages: 4096..32767 (copying all of word 0 would do)
cleared dirty bits: 8192..28671 (only bits 1..14 must be zeroed)

Thanks,

Paolo

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

[Gerd Hoffmann]

  Hi,

> I checked the branch, is bitmap_copy_and_clear_atomic correct when you
> have partial updates?  Maybe you need to handle partial updates of the
> first and last word?

Should not be a problem.  We might clear some more bits, but these are
outsize the visible area so they should cause visible corruption (and if
the visible area changes the display code needs to do a full refresh
anyway).

cheers,
  Gerd

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

[Mark Cave-Ayland]

On 03/04/17 13:03, Gerd Hoffmann wrote:

>   Hi,
> 
>> I checked the branch, is bitmap_copy_and_clear_atomic correct when you
>> have partial updates?  Maybe you need to handle partial updates of the
>> first and last word?
> 
> Should not be a problem.  We might clear some more bits, but these are
> outsize the visible area so they should cause visible corruption (and if
> the visible area changes the display code needs to do a full refresh
> anyway).

Right. Also is there a reason that
cpu_physical_memory_snapshot_and_clear_dirty() uses BITS_PER_LEVEL when
calculating the alignment used for the first/last addresses? I would
have thought you can remove the align variable and use:

    ram_addr_t first = QEMU_ALIGN_DOWN(start, TARGET_PAGE_SIZE);
    ram_addr_t last  = QEMU_ALIGN_UP(start + length, TARGET_PAGE_SIZE);

Otherwise you end up expanding the range of your last address
considerably beyond the next page alignment. And also in the main page
loop why is BITS_PER_LEVEL used? I see that several of the internal
bitmap routines appear to use BITS_PER_LONG for compressing into the
bitmap which might be more appropriate?


ATB,

Mark.

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

[Gerd Hoffmann]

On Mo, 2017-04-03 at 13:24 +0100, Mark Cave-Ayland wrote:
> On 03/04/17 13:03, Gerd Hoffmann wrote:
> 
> >   Hi,
> > 
> >> I checked the branch, is bitmap_copy_and_clear_atomic correct when you
> >> have partial updates?  Maybe you need to handle partial updates of the
> >> first and last word?
> > 
> > Should not be a problem.  We might clear some more bits, but these are
> > outsize the visible area so they should cause visible corruption (and if
> > the visible area changes the display code needs to do a full refresh
> > anyway).
> 
> Right. Also is there a reason that
> cpu_physical_memory_snapshot_and_clear_dirty() uses BITS_PER_LEVEL when
> calculating the alignment used for the first/last addresses?

The code copy doesn't operate on bits but on bitmap longs, so I can just
copy the whole long instead of fiddeling with bits.  So I round down to
a bitmap long start.  On 64bit hosts that are units of 64 pages.

Actually querying the copied bitmap operates on page granularity of
course.

> Otherwise you end up expanding the range of your last address
> considerably beyond the next page alignment.

Yes, it's larger, but as it expands to the start of the long word I
expect the penalty is almost zero (same cache line) and being able to
just copy the whole long instead of dealing with individual bits should
be a win.

> And also in the main page
> loop why is BITS_PER_LEVEL used? I see that several of the internal
> bitmap routines appear to use BITS_PER_LONG for compressing into the
> bitmap which might be more appropriate?

shift by BITS_PER_LEVEL is the same as division by BITS_PER_LONG

cheers,
  Gerd

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

[Mark Cave-Ayland]

On 03/04/17 13:42, Gerd Hoffmann wrote:

> On Mo, 2017-04-03 at 13:24 +0100, Mark Cave-Ayland wrote:
>> On 03/04/17 13:03, Gerd Hoffmann wrote:
>>
>>>   Hi,
>>>
>>>> I checked the branch, is bitmap_copy_and_clear_atomic correct when you
>>>> have partial updates?  Maybe you need to handle partial updates of the
>>>> first and last word?
>>>
>>> Should not be a problem.  We might clear some more bits, but these are
>>> outsize the visible area so they should cause visible corruption (and if
>>> the visible area changes the display code needs to do a full refresh
>>> anyway).
>>
>> Right. Also is there a reason that
>> cpu_physical_memory_snapshot_and_clear_dirty() uses BITS_PER_LEVEL when
>> calculating the alignment used for the first/last addresses?
> 
> The code copy doesn't operate on bits but on bitmap longs, so I can just
> copy the whole long instead of fiddeling with bits.  So I round down to
> a bitmap long start.  On 64bit hosts that are units of 64 pages.
> 
> Actually querying the copied bitmap operates on page granularity of
> course.
> 
>> Otherwise you end up expanding the range of your last address
>> considerably beyond the next page alignment.
> 
> Yes, it's larger, but as it expands to the start of the long word I
> expect the penalty is almost zero (same cache line) and being able to
> just copy the whole long instead of dealing with individual bits should
> be a win.
> 
>> And also in the main page
>> loop why is BITS_PER_LEVEL used? I see that several of the internal
>> bitmap routines appear to use BITS_PER_LONG for compressing into the
>> bitmap which might be more appropriate?
> 
> shift by BITS_PER_LEVEL is the same as division by BITS_PER_LONG

Right hopefully I understand this now. The following diff appears to fix
CG3/TCX for me:

diff --git a/exec.c b/exec.c
index 000af18..66bdcf4 100644
--- a/exec.c
+++ b/exec.c
@@ -1071,7 +1071,7 @@ DirtyBitmapSnapshot
*cpu_physical_memory_snapshot_and_clear_dirty
      (ram_addr_t start, ram_addr_t length, unsigned client)
 {
     DirtyMemoryBlocks *blocks;
-    unsigned long align = 1 << (TARGET_PAGE_BITS + BITS_PER_LEVEL);
+    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;
@@ -1097,7 +1097,6 @@ DirtyBitmapSnapshot
*cpu_physical_memory_snapshot_and_clear_dirty

         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 >>
BITS_PER_LEVEL),


There were 2 issues here: without the UL suffix on align I was getting
incorrect first/last addresses since the high bits of align weren't
being cleared, and then offset appeared to be shifted twice.

Does this look reasonable to you?


ATB,

Mark.

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

[Paolo Bonzini]

On 03/04/2017 14:03, Gerd Hoffmann wrote:
> We might clear some more bits, but these are
> outsize the visible area so they should cause visible corruption (and if
> the visible area changes the display code needs to do a full refresh
> anyway).

True, though this makes the snapshot abstraction a little more
specialized.  Please add a FIXME comment with the explanation, at least.

Paolo

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

[Gerd Hoffmann]

  Hi,

> -    unsigned long align = 1 << (TARGET_PAGE_BITS + BITS_PER_LEVEL);
> +    unsigned long align = 1UL << (TARGET_PAGE_BITS + BITS_PER_LEVEL);

> There were 2 issues here: without the UL suffix on align I was getting
> incorrect first/last addresses since the high bits of align weren't
> being cleared,

Ah, thanks, I'll add that.

> and then offset appeared to be shifted twice.

Yep, noticed that too meanwhile, fixed in the branch pushed half an hour
ago.  I've dropped the other shift though ;)

cheers,
  Gerd

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

[Gerd Hoffmann]

On Mo, 2017-04-03 at 19:02 +0200, Paolo Bonzini wrote:
> 
> On 03/04/2017 14:03, Gerd Hoffmann wrote:
> > We might clear some more bits, but these are
> > outsize the visible area so they should cause visible corruption (and if
> > the visible area changes the display code needs to do a full refresh
> > anyway).
> 
> True, though this makes the snapshot abstraction a little more
> specialized.  Please add a FIXME comment with the explanation, at least.

Sure, I'll add a note to the (to be written) docs.

cheers,
  Gerd

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

[Mark Cave-Ayland]

On 04/04/17 07:12, Gerd Hoffmann wrote:

>   Hi,
> 
>> -    unsigned long align = 1 << (TARGET_PAGE_BITS + BITS_PER_LEVEL);
>> +    unsigned long align = 1UL << (TARGET_PAGE_BITS + BITS_PER_LEVEL);
> 
>> There were 2 issues here: without the UL suffix on align I was getting
>> incorrect first/last addresses since the high bits of align weren't
>> being cleared,
> 
> Ah, thanks, I'll add that.
> 
>> and then offset appeared to be shifted twice.
> 
> Yep, noticed that too meanwhile, fixed in the branch pushed half an hour
> ago.  I've dropped the other shift though ;)

Confirmed!

The only minor nit I've noticed is that commit 322aef7 "cg3: fix up size
parameter for memory_region_get_dirty()" isn't quite right now that the
asserts() in cpu_physical_memory_snapshot_get_dirty() have now been
fixed - the size should now be "width" rather than "width - 1".

Other than that, I've just given it a quick spin across my SPARC CG3/TCX
and PPC VGA images and it looks good here :)


ATB,

Mark.