[Qemu-devel] [PATCH v2 00/29] Dirty bitmap atomic access and optimizations

[Qemu-devel] [PATCH v2 00/29] Dirty bitmap atomic access and optimizations

[Paolo Bonzini]

QEMU is currently accessing the dirty bitmaps very liberally,
which is understandable since the accesses are cheap.  This is
however not good for squeezing maximum performance out of dataplane,
and is also not good if the accesses become more expensive---as is
the case when they use atomic primitives.

This patch series does the following optimizations and cleanups:

1) it lets KVM code treat migration as "just another dirty bitmap
client" instead of needing the special global_log_start/stop callbacks.
These remain in use in Xen and vhost.  This removes code and avoids
bugs such as the one fixed in commit 4cc856f (kvm-all: Sync dirty-bitmap
from kvm before kvm destroy the corresponding dirty_bitmap, 2015-04-02).

2) it avoids modifications to unused dirty bitmaps: code if TCG
is disabled, migration if no migration is in progress, VGA for
regions other than VRAM.

and on top of this makes dirty bitmap access atomic.  I'm not including
the patch to make the migration thread synchronize the bitmap outside
the big QEMU lock (thus removing the last source of jitter during the
RAM copy phase of migration) but it is also enabled by these patches.

Patches 1-4 are cleanups to DIRTY_MEMORY_VGA users.

Patches 5-12 are the first cleanup (KVM treats migration as just
another client).  Patches 13-14 are a simple optimization that is enabled
by these patches.

Patches 15-18 are bonus cleanups to translate-all.c's dirty memory
tracking for TCG.

Patches 19-22 are the second cleanup (avoid modifications to unused
dirty bitmaps).

Patches 23-28 are Stefan's patches for atomic access to the dirty
bitmap, which has no performance impact in the common case thanks to
the previous work.

Patch 29 is an unrelated strengthening of assertions, that mst spotted
while reviewing v1.

v1->v2: completed work on removing global_start/global_stop from KVM
	listener

        extra spelunking of TCG history so that the exec.c code
	makes more sense

        extra splitting of patches (Stefan)

        keep memory_region_is_logging and memory_region_get_dirty_log_mask
	APIs separate (mst)

Paolo Bonzini (23):
  memory: the only dirty memory flag for users is DIRTY_MEMORY_VGA
  g364fb: remove pointless call to memory_region_set_coalescing
  display: enable DIRTY_MEMORY_VGA tracking explicitly
  display: add memory_region_sync_dirty_bitmap calls
  memory: differentiate memory_region_is_logging and
    memory_region_get_dirty_log_mask
  memory: prepare for multiple bits in the dirty log mask
  framebuffer: check memory_region_is_logging
  ui/console: remove dpy_gfx_update_dirty
  memory: track DIRTY_MEMORY_CODE in mr->dirty_log_mask
  kvm: accept non-mapped memory in kvm_dirty_pages_log_change
  memory: include DIRTY_MEMORY_MIGRATION in the dirty log mask
  kvm: remove special handling of DIRTY_MEMORY_MIGRATION in the dirty
    log mask
  ram_addr: tweaks to xen_modified_memory
  exec: use memory_region_get_dirty_log_mask to optimize dirty tracking
  exec: move functions to translate-all.h
  translate-all: remove unnecessary argument to tb_invalidate_phys_range
  cputlb: remove useless arguments to tlb_unprotect_code_phys, rename
  translate-all: make less of tb_invalidate_phys_page_range depend on
    is_cpu_write_access
  exec: pass client mask to cpu_physical_memory_set_dirty_range
  exec: invert return value of cpu_physical_memory_get_clean, rename
  exec: only check relevant bitmaps for cleanliness
  memory: do not touch code dirty bitmap unless TCG is enabled
  memory: strengthen assertions on mr->terminates

Stefan Hajnoczi (6):
  bitmap: add atomic set functions
  bitmap: add atomic test and clear
  memory: use atomic ops for setting dirty memory bits
  migration: move dirty bitmap sync to ram_addr.h
  memory: replace cpu_physical_memory_reset_dirty() with test-and-clear
  memory: make cpu_physical_memory_sync_dirty_bitmap() fully atomic

 arch_init.c                  |  46 +--------------
 cputlb.c                     |   7 +--
 exec.c                       |  99 +++++++++++++++----------------
 hw/display/cg3.c             |   2 +
 hw/display/exynos4210_fimd.c |  20 ++++---
 hw/display/framebuffer.c     |   4 ++
 hw/display/g364fb.c          |   3 +-
 hw/display/sm501.c           |   2 +
 hw/display/tcx.c             |   2 +
 hw/display/vmware_vga.c      |   2 +-
 hw/virtio/dataplane/vring.c  |   2 +-
 hw/virtio/vhost.c            |   9 ++-
 include/exec/cputlb.h        |   3 +-
 include/exec/exec-all.h      |   6 +-
 include/exec/memory.h        |  25 ++++++--
 include/exec/ram_addr.h      | 138 ++++++++++++++++++++++++++++---------------
 include/qemu/bitmap.h        |   4 ++
 include/qemu/bitops.h        |  14 +++++
 include/ui/console.h         |   4 --
 kvm-all.c                    |  77 ++++++------------------
 linux-user/mmap.c            |   7 ++-
 memory.c                     |  76 ++++++++++++++++--------
 translate-all.c              |  20 +++----
 translate-all.h              |   7 +++
 ui/console.c                 |  61 -------------------
 user-exec.c                  |   1 +
 util/bitmap.c                |  81 +++++++++++++++++++++++++
 xen-hvm.c                    |  22 ++++---
 28 files changed, 401 insertions(+), 343 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 01/29] memory: the only dirty memory flag for users is DIRTY_MEMORY_VGA

[Paolo Bonzini]

DIRTY_MEMORY_MIGRATION is triggered by memory_global_dirty_log_start
and memory_global_dirty_log_stop, so it cannot be used with
memory_region_set_log.

Specify this in the documentation and assert it.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/exec/memory.h | 3 +--
 memory.c              | 1 +
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index a2ea587..081f7d6 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -625,8 +625,7 @@ void memory_region_ram_resize(MemoryRegion *mr, ram_addr_t newsize,
  *
  * @mr: the memory region being updated.
  * @log: whether dirty logging is to be enabled or disabled.
- * @client: the user of the logging information; %DIRTY_MEMORY_MIGRATION or
- *          %DIRTY_MEMORY_VGA.
+ * @client: the user of the logging information; %DIRTY_MEMORY_VGA only.
  */
 void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client);
 
diff --git a/memory.c b/memory.c
index a11e9bf..e688f5e 100644
--- a/memory.c
+++ b/memory.c
@@ -1354,6 +1354,7 @@ void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
 {
     uint8_t mask = 1 << client;
 
+    assert(client == DIRTY_MEMORY_VGA);
     memory_region_transaction_begin();
     mr->dirty_log_mask = (mr->dirty_log_mask & ~mask) | (log * mask);
     memory_region_update_pending |= mr->enabled;
-- 
1.8.3.1

[Qemu-devel] [PATCH 02/29] g364fb: remove pointless call to memory_region_set_coalescing

[Paolo Bonzini]

Coalescing work on MMIO, not RAM, thus this call has no effect.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/display/g364fb.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/display/g364fb.c b/hw/display/g364fb.c
index 46f7b41..6543f2f 100644
--- a/hw/display/g364fb.c
+++ b/hw/display/g364fb.c
@@ -489,7 +489,6 @@ static void g364fb_init(DeviceState *dev, G364State *s)
     memory_region_init_ram_ptr(&s->mem_vram, NULL, "vram",
                                s->vram_size, s->vram);
     vmstate_register_ram(&s->mem_vram, dev);
-    memory_region_set_coalescing(&s->mem_vram);
 }
 
 #define TYPE_G364 "sysbus-g364"
-- 
1.8.3.1

[Qemu-devel] [PATCH 03/29] display: enable DIRTY_MEMORY_VGA tracking explicitly

[Paolo Bonzini]

This will be required soon by the memory core.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/display/cg3.c             |  1 +
 hw/display/exynos4210_fimd.c | 20 +++++++++++++-------
 hw/display/g364fb.c          |  1 +
 hw/display/sm501.c           |  1 +
 hw/display/tcx.c             |  1 +
 5 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/hw/display/cg3.c b/hw/display/cg3.c
index 1e6ff2b..cbcf518 100644
--- a/hw/display/cg3.c
+++ b/hw/display/cg3.c
@@ -309,6 +309,7 @@ static void cg3_realizefn(DeviceState *dev, Error **errp)
 
     memory_region_init_ram(&s->vram_mem, NULL, "cg3.vram", s->vram_size,
                            &error_abort);
+    memory_region_set_log(&s->vram_mem, true, DIRTY_MEMORY_VGA);
     vmstate_register_ram_global(&s->vram_mem);
     sysbus_init_mmio(sbd, &s->vram_mem);
 
diff --git a/hw/display/exynos4210_fimd.c b/hw/display/exynos4210_fimd.c
index 45c62af..72b3a1d 100644
--- a/hw/display/exynos4210_fimd.c
+++ b/hw/display/exynos4210_fimd.c
@@ -1109,6 +1109,12 @@ static inline int fimd_get_buffer_id(Exynos4210fimdWindow *w)
     }
 }
 
+static void exynos4210_fimd_invalidate(void *opaque)
+{
+    Exynos4210fimdState *s = (Exynos4210fimdState *)opaque;
+    s->invalidate = true;
+}
+
 /* Updates specified window's MemorySection based on values of WINCON,
  * VIDOSDA, VIDOSDB, VIDWADDx and SHADOWCON registers */
 static void fimd_update_memory_section(Exynos4210fimdState *s, unsigned win)
@@ -1136,7 +1142,11 @@ static void fimd_update_memory_section(Exynos4210fimdState *s, unsigned win)
     /* TODO: add .exit and unref the region there.  Not needed yet since sysbus
      * does not support hot-unplug.
      */
-    memory_region_unref(w->mem_section.mr);
+    if (w->mem_section.mr) {
+        memory_region_set_log(w->mem_section.mr, false, DIRTY_MEMORY_VGA);
+        memory_region_unref(w->mem_section.mr);
+    }
+
     w->mem_section = memory_region_find(sysbus_address_space(sbd),
                                         fb_start_addr, w->fb_len);
     assert(w->mem_section.mr);
@@ -1162,6 +1172,8 @@ static void fimd_update_memory_section(Exynos4210fimdState *s, unsigned win)
         cpu_physical_memory_unmap(w->host_fb_addr, fb_mapped_len, 0, 0);
         goto error_return;
     }
+    memory_region_set_log(w->mem_section.mr, true, DIRTY_MEMORY_VGA);
+    exynos4210_fimd_invalidate(s);
     return;
 
 error_return:
@@ -1224,12 +1236,6 @@ static void exynos4210_fimd_update_irq(Exynos4210fimdState *s)
     }
 }
 
-static void exynos4210_fimd_invalidate(void *opaque)
-{
-    Exynos4210fimdState *s = (Exynos4210fimdState *)opaque;
-    s->invalidate = true;
-}
-
 static void exynos4210_update_resolution(Exynos4210fimdState *s)
 {
     DisplaySurface *surface = qemu_console_surface(s->console);
diff --git a/hw/display/g364fb.c b/hw/display/g364fb.c
index 6543f2f..be62dd6 100644
--- a/hw/display/g364fb.c
+++ b/hw/display/g364fb.c
@@ -489,6 +489,7 @@ static void g364fb_init(DeviceState *dev, G364State *s)
     memory_region_init_ram_ptr(&s->mem_vram, NULL, "vram",
                                s->vram_size, s->vram);
     vmstate_register_ram(&s->mem_vram, dev);
+    memory_region_set_log(&s->mem_vram, true, DIRTY_MEMORY_VGA);
 }
 
 #define TYPE_G364 "sysbus-g364"
diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index c72154b..43f8538 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -1412,6 +1412,7 @@ void sm501_init(MemoryRegion *address_space_mem, uint32_t base,
     memory_region_init_ram(&s->local_mem_region, NULL, "sm501.local",
                            local_mem_bytes, &error_abort);
     vmstate_register_ram_global(&s->local_mem_region);
+    memory_region_set_log(&s->local_mem_region, true, DIRTY_MEMORY_VGA);
     s->local_mem = memory_region_get_ram_ptr(&s->local_mem_region);
     memory_region_add_subregion(address_space_mem, base, &s->local_mem_region);
 
diff --git a/hw/display/tcx.c b/hw/display/tcx.c
index a9f9f66..58faa96 100644
--- a/hw/display/tcx.c
+++ b/hw/display/tcx.c
@@ -1006,6 +1006,7 @@ static void tcx_realizefn(DeviceState *dev, Error **errp)
     memory_region_init_ram(&s->vram_mem, OBJECT(s), "tcx.vram",
                            s->vram_size * (1 + 4 + 4), &error_abort);
     vmstate_register_ram_global(&s->vram_mem);
+    memory_region_set_log(&s->vram_mem, true, DIRTY_MEMORY_VGA);
     vram_base = memory_region_get_ram_ptr(&s->vram_mem);
 
     /* 10/ROM : FCode ROM */
-- 
1.8.3.1

[Qemu-devel] [PATCH 04/29] display: add memory_region_sync_dirty_bitmap calls

[Paolo Bonzini]

These are strictly speaking only needed for KVM and Xen, but it's still
nice to be consistent.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/display/cg3.c    | 1 +
 hw/display/g364fb.c | 1 +
 hw/display/sm501.c  | 1 +
 hw/display/tcx.c    | 1 +
 4 files changed, 4 insertions(+)

diff --git a/hw/display/cg3.c b/hw/display/cg3.c
index cbcf518..b94e5e0 100644
--- a/hw/display/cg3.c
+++ b/hw/display/cg3.c
@@ -106,6 +106,7 @@ static void cg3_update_display(void *opaque)
     pix = memory_region_get_ram_ptr(&s->vram_mem);
     data = (uint32_t *)surface_data(surface);
 
+    memory_region_sync_dirty_bitmap(&s->vram_mem);
     for (y = 0; y < height; y++) {
         int update = s->full_update;
 
diff --git a/hw/display/g364fb.c b/hw/display/g364fb.c
index be62dd6..52a9733 100644
--- a/hw/display/g364fb.c
+++ b/hw/display/g364fb.c
@@ -260,6 +260,7 @@ static void g364fb_update_display(void *opaque)
         qemu_console_resize(s->con, s->width, s->height);
     }
 
+    memory_region_sync_dirty_bitmap(&s->mem_vram);
     if (s->ctla & CTLA_FORCE_BLANK) {
         g364fb_draw_blank(s);
     } else if (s->depth == 8) {
diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 43f8538..15a5ba8 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -1322,6 +1322,7 @@ static void sm501_draw_crt(SM501State * s)
     }
 
     /* draw each line according to conditions */
+    memory_region_sync_dirty_bitmap(&s->local_mem_region);
     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;
diff --git a/hw/display/tcx.c b/hw/display/tcx.c
index 58faa96..2eb5aef 100644
--- a/hw/display/tcx.c
+++ b/hw/display/tcx.c
@@ -353,6 +353,7 @@ static void tcx_update_display(void *opaque)
         return;
     }
 
+    memory_region_sync_dirty_bitmap(&ts->vram_mem);
     for (y = 0; y < ts->height; page += TARGET_PAGE_SIZE) {
         if (memory_region_get_dirty(&ts->vram_mem, page, TARGET_PAGE_SIZE,
                                     DIRTY_MEMORY_VGA)) {
-- 
1.8.3.1

[Qemu-devel] [PATCH 05/29] memory: differentiate memory_region_is_logging and memory_region_get_dirty_log_mask

[Paolo Bonzini]

For now memory regions only track DIRTY_MEMORY_VGA individually, but
this will change soon.  To support this, split memory_region_is_logging
in two functions: one that returns a given bit from dirty_log_mask,
and one that returns the entire mask.  memory_region_is_logging gets an
extra parameter so that the compiler flags misuse.

While VGA-specific users (including the Xen listener!) will want to keep
checking that bit, KVM and vhost check for "any bit except migration"
(because migration is handled via the global start/stop listener
callbacks).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/display/vmware_vga.c     |  2 +-
 hw/virtio/dataplane/vring.c |  2 +-
 hw/virtio/vhost.c           |  3 ++-
 include/exec/memory.h       | 17 +++++++++++++++--
 kvm-all.c                   |  3 ++-
 memory.c                    |  7 ++++++-
 xen-hvm.c                   |  2 +-
 7 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index c17ddd1..7f397d3 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -1124,7 +1124,7 @@ static void vmsvga_update_display(void *opaque)
      * 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)) {
+    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),
diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
index 5c7b8c2..e378733 100644
--- a/hw/virtio/dataplane/vring.c
+++ b/hw/virtio/dataplane/vring.c
@@ -42,7 +42,7 @@ static void *vring_map(MemoryRegion **mr, hwaddr phys, hwaddr len,
     }
 
     /* Ignore regions with dirty logging, we cannot mark them dirty */
-    if (memory_region_is_logging(section.mr)) {
+    if (memory_region_get_dirty_log_mask(section.mr)) {
         goto out;
     }
 
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 54851b7..5f698a1 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -377,7 +377,8 @@ static void vhost_set_memory(MemoryListener *listener,
                                          memory_listener);
     hwaddr start_addr = section->offset_within_address_space;
     ram_addr_t size = int128_get64(section->size);
-    bool log_dirty = memory_region_is_logging(section->mr);
+    bool log_dirty =
+        memory_region_get_dirty_log_mask(section->mr) & ~(1 << DIRTY_MEMORY_MIGRATION);
     int s = offsetof(struct vhost_memory, regions) +
         (dev->mem->nregions + 1) * sizeof dev->mem->regions[0];
     void *ram;
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 081f7d6..229733e 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -569,11 +569,24 @@ const char *memory_region_name(const MemoryRegion *mr);
 /**
  * memory_region_is_logging: return whether a memory region is logging writes
  *
- * Returns %true if the memory region is logging writes
+ * Returns %true if the memory region is logging writes for the given client
+ * a bitmap of clients for which the memory region is logging writes.
  *
  * @mr: the memory region being queried
+ * @client: the client being queried
  */
-bool memory_region_is_logging(MemoryRegion *mr);
+bool memory_region_is_logging(MemoryRegion *mr, uint8_t client);
+
+/**
+ * memory_region_get_dirty_log_mask: return the clients for which a
+ * memory region is logging writes.
+ *
+ * Returns a bitmap of clients for which the memory region is logging writes.
+ * Right now this will be either 0 or (1 << DIRTY_MEMORY_VGA).
+ *
+ * @mr: the memory region being queried
+ */
+uint8_t memory_region_get_dirty_log_mask(MemoryRegion *mr);
 
 /**
  * memory_region_is_rom: check whether a memory region is ROM
diff --git a/kvm-all.c b/kvm-all.c
index dd44f8c..bd1f910 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -663,7 +663,8 @@ static void kvm_set_phys_mem(MemoryRegionSection *section, bool add)
     KVMSlot *mem, old;
     int err;
     MemoryRegion *mr = section->mr;
-    bool log_dirty = memory_region_is_logging(mr);
+    bool log_dirty =
+        memory_region_get_dirty_log_mask(mr) & ~(1 << DIRTY_MEMORY_MIGRATION);
     bool writeable = !mr->readonly && !mr->rom_device;
     bool readonly_flag = mr->readonly || memory_region_is_romd(mr);
     hwaddr start_addr = section->offset_within_address_space;
diff --git a/memory.c b/memory.c
index e688f5e..a982d99 100644
--- a/memory.c
+++ b/memory.c
@@ -1318,11 +1318,16 @@ bool memory_region_is_skip_dump(MemoryRegion *mr)
     return mr->skip_dump;
 }
 
-bool memory_region_is_logging(MemoryRegion *mr)
+uint8_t memory_region_get_dirty_log_mask(MemoryRegion *mr)
 {
     return mr->dirty_log_mask;
 }
 
+bool memory_region_is_logging(MemoryRegion *mr, uint8_t client)
+{
+    return memory_region_get_dirty_log_mask(mr) & (1 << client);
+}
+
 bool memory_region_is_rom(MemoryRegion *mr)
 {
     return mr->ram && mr->readonly;
diff --git a/xen-hvm.c b/xen-hvm.c
index 315864c..338ab29 100644
--- a/xen-hvm.c
+++ b/xen-hvm.c
@@ -488,7 +488,7 @@ static void xen_set_memory(struct MemoryListener *listener,
     XenIOState *state = container_of(listener, XenIOState, memory_listener);
     hwaddr start_addr = section->offset_within_address_space;
     ram_addr_t size = int128_get64(section->size);
-    bool log_dirty = memory_region_is_logging(section->mr);
+    bool log_dirty = memory_region_is_logging(section->mr, DIRTY_MEMORY_VGA);
     hvmmem_type_t mem_type;
 
     if (section->mr == &ram_memory) {
-- 
1.8.3.1

[Qemu-devel] [PATCH 06/29] memory: prepare for multiple bits in the dirty log mask

[Paolo Bonzini]

When the dirty log mask will also cover other bits than DIRTY_MEMORY_VGA,
some listeners may be interested in the overall zero/non-zero value of
the dirty log mask; others may be interested in the value of single bits.

For this reason, always call log_start/log_stop if bits have respectively
appeared or disappeared, and pass the old and new values of the dirty log
mask so that listeners can distinguish the kinds of change.

For example, KVM checks if dirty logging used to be completely disabled
(in log_start) or is now completely disabled (in log_stop).  On the
other hand, Xen has to check manually if DIRTY_MEMORY_VGA changed,
since that is the only bit it cares about.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/virtio/vhost.c     |  6 ++++--
 include/exec/memory.h |  6 ++++--
 kvm-all.c             | 14 ++++++++++++--
 memory.c              | 17 +++++++++++------
 xen-hvm.c             | 20 +++++++++++++-------
 5 files changed, 44 insertions(+), 19 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 5f698a1..6364b25 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -637,13 +637,15 @@ static void vhost_log_global_stop(MemoryListener *listener)
 }
 
 static void vhost_log_start(MemoryListener *listener,
-                            MemoryRegionSection *section)
+                            MemoryRegionSection *section,
+                            int old, int new)
 {
     /* FIXME: implement */
 }
 
 static void vhost_log_stop(MemoryListener *listener,
-                           MemoryRegionSection *section)
+                           MemoryRegionSection *section,
+                           int old, int new)
 {
     /* FIXME: implement */
 }
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 229733e..cfdf39a 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -184,8 +184,10 @@ struct MemoryListener {
     void (*region_add)(MemoryListener *listener, MemoryRegionSection *section);
     void (*region_del)(MemoryListener *listener, MemoryRegionSection *section);
     void (*region_nop)(MemoryListener *listener, MemoryRegionSection *section);
-    void (*log_start)(MemoryListener *listener, MemoryRegionSection *section);
-    void (*log_stop)(MemoryListener *listener, MemoryRegionSection *section);
+    void (*log_start)(MemoryListener *listener, MemoryRegionSection *section,
+                      int old, int new);
+    void (*log_stop)(MemoryListener *listener, MemoryRegionSection *section,
+                     int old, int new);
     void (*log_sync)(MemoryListener *listener, MemoryRegionSection *section);
     void (*log_global_start)(MemoryListener *listener);
     void (*log_global_stop)(MemoryListener *listener);
diff --git a/kvm-all.c b/kvm-all.c
index bd1f910..989fdd9 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -344,10 +344,15 @@ static int kvm_dirty_pages_log_change(hwaddr phys_addr,
 }
 
 static void kvm_log_start(MemoryListener *listener,
-                          MemoryRegionSection *section)
+                          MemoryRegionSection *section,
+                          int old, int new)
 {
     int r;
 
+    if (old != 0) {
+        return;
+    }
+
     r = kvm_dirty_pages_log_change(section->offset_within_address_space,
                                    int128_get64(section->size), true);
     if (r < 0) {
@@ -356,10 +361,15 @@ static void kvm_log_start(MemoryListener *listener,
 }
 
 static void kvm_log_stop(MemoryListener *listener,
-                          MemoryRegionSection *section)
+                          MemoryRegionSection *section,
+                          int old, int new)
 {
     int r;
 
+    if (new != 0) {
+        return;
+    }
+
     r = kvm_dirty_pages_log_change(section->offset_within_address_space,
                                    int128_get64(section->size), false);
     if (r < 0) {
diff --git a/memory.c b/memory.c
index a982d99..3d34f48 100644
--- a/memory.c
+++ b/memory.c
@@ -152,7 +152,7 @@ static bool memory_listener_match(MemoryListener *listener,
     } while (0)
 
 /* No need to ref/unref .mr, the FlatRange keeps it alive.  */
-#define MEMORY_LISTENER_UPDATE_REGION(fr, as, dir, callback)            \
+#define MEMORY_LISTENER_UPDATE_REGION(fr, as, dir, callback, _args...)  \
     MEMORY_LISTENER_CALL(callback, dir, (&(MemoryRegionSection) {       \
         .mr = (fr)->mr,                                                 \
         .address_space = (as),                                          \
@@ -160,7 +160,7 @@ static bool memory_listener_match(MemoryListener *listener,
         .size = (fr)->addr.size,                                        \
         .offset_within_address_space = int128_get64((fr)->addr.start),  \
         .readonly = (fr)->readonly,                                     \
-              }))
+              }), ##_args)
 
 struct CoalescedMemoryRange {
     AddrRange addr;
@@ -723,10 +723,15 @@ static void address_space_update_topology_pass(AddressSpace *as,
 
             if (adding) {
                 MEMORY_LISTENER_UPDATE_REGION(frnew, as, Forward, region_nop);
-                if (frold->dirty_log_mask && !frnew->dirty_log_mask) {
-                    MEMORY_LISTENER_UPDATE_REGION(frnew, as, Reverse, log_stop);
-                } else if (frnew->dirty_log_mask && !frold->dirty_log_mask) {
-                    MEMORY_LISTENER_UPDATE_REGION(frnew, as, Forward, log_start);
+                if (frnew->dirty_log_mask & ~frold->dirty_log_mask) {
+                    MEMORY_LISTENER_UPDATE_REGION(frnew, as, Forward, log_start,
+                                                  frold->dirty_log_mask,
+                                                  frnew->dirty_log_mask);
+                }
+                if (frold->dirty_log_mask & ~frnew->dirty_log_mask) {
+                    MEMORY_LISTENER_UPDATE_REGION(frnew, as, Reverse, log_stop,
+                                                  frold->dirty_log_mask,
+                                                  frnew->dirty_log_mask);
                 }
             }
 
diff --git a/xen-hvm.c b/xen-hvm.c
index 338ab29..cc55faa 100644
--- a/xen-hvm.c
+++ b/xen-hvm.c
@@ -646,21 +646,27 @@ static void xen_sync_dirty_bitmap(XenIOState *state,
 }
 
 static void xen_log_start(MemoryListener *listener,
-                          MemoryRegionSection *section)
+                          MemoryRegionSection *section,
+                          int old, int new)
 {
     XenIOState *state = container_of(listener, XenIOState, memory_listener);
 
-    xen_sync_dirty_bitmap(state, section->offset_within_address_space,
-                          int128_get64(section->size));
+    if (new & ~old & (1 << DIRTY_MIGRATION_VGA)) {
+        xen_sync_dirty_bitmap(state, section->offset_within_address_space,
+                              int128_get64(section->size));
+    }
 }
 
-static void xen_log_stop(MemoryListener *listener, MemoryRegionSection *section)
+static void xen_log_stop(MemoryListener *listener, MemoryRegionSection *section,
+                         int old, int new)
 {
     XenIOState *state = container_of(listener, XenIOState, memory_listener);
 
-    state->log_for_dirtybit = NULL;
-    /* Disable dirty bit tracking */
-    xc_hvm_track_dirty_vram(xen_xc, xen_domid, 0, 0, NULL);
+    if (old & ~new & (1 << DIRTY_MIGRATION_VGA)) {
+        state->log_for_dirtybit = NULL;
+        /* Disable dirty bit tracking */
+        xc_hvm_track_dirty_vram(xen_xc, xen_domid, 0, 0, NULL);
+    }
 }
 
 static void xen_log_sync(MemoryListener *listener, MemoryRegionSection *section)
-- 
1.8.3.1

[Qemu-devel] [PATCH 07/29] framebuffer: check memory_region_is_logging

[Paolo Bonzini]

framebuffer.c expects DIRTY_MEMORY_VGA logging to be always on, but that
will not be the case soon.  Because framebuffer.c computes the memory
region on the fly for every update (with memory_region_find), it cannot
enable/disable logging by itself.

Instead, always treat updates as invalidations if dirty logging is
not enabled, assuming that the board will enable logging on the
RAM region that includes the framebuffer.

To simplify the code, replace memory_region_get_dirty with
memory_region_test_and_clear_dirty.  Then memory_region_reset_dirty
is only needed in the invalidate case.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/display/framebuffer.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/display/framebuffer.c b/hw/display/framebuffer.c
index 4546e42..2cabced 100644
--- a/hw/display/framebuffer.c
+++ b/hw/display/framebuffer.c
@@ -63,6 +63,10 @@ void framebuffer_update_display(
     assert(mem_section.offset_within_address_space == base);
 
     memory_region_sync_dirty_bitmap(mem);
+    if (!memory_region_is_logging(mem, DIRTY_MEMORY_VGA)) {
+        invalidate = true;
+    }
+
     src_base = cpu_physical_memory_map(base, &src_len, 0);
     /* If we can't map the framebuffer then bail.  We could try harder,
        but it's not really worth it as dirty flag tracking will probably
-- 
1.8.3.1

[Qemu-devel] [PATCH 08/29] ui/console: remove dpy_gfx_update_dirty

[Paolo Bonzini]

dpy_gfx_update_dirty expects DIRTY_MEMORY_VGA logging to be always on,
but that will not be the case soon.  Because it computes the memory
region on the fly for every update (with memory_region_find), it cannot
enable/disable logging by itself.

We could always treat updates as invalidations if dirty logging is
not enabled, assuming that the board will enable logging on the
RAM region that includes the framebuffer.

However, the function is unused, so just drop it.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/ui/console.h |  4 ----
 ui/console.c         | 61 ----------------------------------------------------
 2 files changed, 65 deletions(-)

diff --git a/include/ui/console.h b/include/ui/console.h
index 2f5b9f0..02c9fdf 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -231,10 +231,6 @@ void dpy_text_resize(QemuConsole *con, int w, int h);
 void dpy_mouse_set(QemuConsole *con, int x, int y, int on);
 void dpy_cursor_define(QemuConsole *con, QEMUCursor *cursor);
 bool dpy_cursor_define_supported(QemuConsole *con);
-void dpy_gfx_update_dirty(QemuConsole *con,
-                          MemoryRegion *address_space,
-                          uint64_t base,
-                          bool invalidate);
 bool dpy_gfx_check_format(QemuConsole *con,
                           pixman_format_code_t format);
 
diff --git a/ui/console.c b/ui/console.c
index b15ca87..3eb5e91 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1599,67 +1599,6 @@ bool dpy_cursor_define_supported(QemuConsole *con)
     return false;
 }
 
-/*
- * Call dpy_gfx_update for all dirity scanlines.  Works for
- * DisplaySurfaces backed by guest memory (i.e. the ones created
- * using qemu_create_displaysurface_guestmem).
- */
-void dpy_gfx_update_dirty(QemuConsole *con,
-                          MemoryRegion *address_space,
-                          hwaddr base,
-                          bool invalidate)
-{
-    DisplaySurface *ds = qemu_console_surface(con);
-    int width = surface_stride(ds);
-    int height = surface_height(ds);
-    hwaddr size = width * height;
-    MemoryRegionSection mem_section;
-    MemoryRegion *mem;
-    ram_addr_t addr;
-    int first, last, i;
-    bool dirty;
-
-    mem_section = memory_region_find(address_space, base, size);
-    mem = mem_section.mr;
-    if (int128_get64(mem_section.size) != size ||
-        !memory_region_is_ram(mem_section.mr)) {
-        goto out;
-    }
-    assert(mem);
-
-    memory_region_sync_dirty_bitmap(mem);
-    addr = mem_section.offset_within_region;
-
-    first = -1;
-    last = -1;
-    for (i = 0; i < height; i++, addr += width) {
-        dirty = invalidate ||
-            memory_region_get_dirty(mem, addr, width, DIRTY_MEMORY_VGA);
-        if (dirty) {
-            if (first == -1) {
-                first = i;
-            }
-            last = i;
-        }
-        if (first != -1 && !dirty) {
-            assert(last != -1 && last >= first);
-            dpy_gfx_update(con, 0, first, surface_width(ds),
-                           last - first + 1);
-            first = -1;
-        }
-    }
-    if (first != -1) {
-        assert(last != -1 && last >= first);
-        dpy_gfx_update(con, 0, first, surface_width(ds),
-                       last - first + 1);
-    }
-
-    memory_region_reset_dirty(mem, mem_section.offset_within_region, size,
-                              DIRTY_MEMORY_VGA);
-out:
-    memory_region_unref(mem);
-}
-
 /***********************************************************/
 /* register display */
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 09/29] memory: track DIRTY_MEMORY_CODE in mr->dirty_log_mask

[Paolo Bonzini]

DIRTY_MEMORY_CODE is only needed for TCG.  By adding it directly to
mr->dirty_log_mask, we avoid testing for TCG everywhere a region is
checked for the enabled/disabled state of dirty logging.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/exec/memory.h | 1 -
 memory.c              | 4 ++++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index cfdf39a..b4584e5 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -584,7 +584,6 @@ bool memory_region_is_logging(MemoryRegion *mr, uint8_t client);
  * memory region is logging writes.
  *
  * Returns a bitmap of clients for which the memory region is logging writes.
- * Right now this will be either 0 or (1 << DIRTY_MEMORY_VGA).
  *
  * @mr: the memory region being queried
  */
diff --git a/memory.c b/memory.c
index 3d34f48..1966347 100644
--- a/memory.c
+++ b/memory.c
@@ -1141,6 +1141,7 @@ void memory_region_init_ram(MemoryRegion *mr,
     mr->terminates = true;
     mr->destructor = memory_region_destructor_ram;
     mr->ram_addr = qemu_ram_alloc(size, mr, errp);
+    mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
 }
 
 void memory_region_init_resizeable_ram(MemoryRegion *mr,
@@ -1158,6 +1159,7 @@ void memory_region_init_resizeable_ram(MemoryRegion *mr,
     mr->terminates = true;
     mr->destructor = memory_region_destructor_ram;
     mr->ram_addr = qemu_ram_alloc_resizeable(size, max_size, resized, mr, errp);
+    mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
 }
 
 #ifdef __linux__
@@ -1174,6 +1176,7 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
     mr->terminates = true;
     mr->destructor = memory_region_destructor_ram;
     mr->ram_addr = qemu_ram_alloc_from_file(size, mr, share, path, errp);
+    mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
 }
 #endif
 
@@ -1187,6 +1190,7 @@ void memory_region_init_ram_ptr(MemoryRegion *mr,
     mr->ram = true;
     mr->terminates = true;
     mr->destructor = memory_region_destructor_ram_from_ptr;
+    mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
 
     /* qemu_ram_alloc_from_ptr cannot fail with ptr != NULL.  */
     assert(ptr != NULL);
-- 
1.8.3.1

[Qemu-devel] [PATCH 10/29] kvm: accept non-mapped memory in kvm_dirty_pages_log_change

[Paolo Bonzini]

It is okay if memory is not mapped into the guest but has dirty logging
enabled.  When this happens, KVM will not do anything and only accesses
from the host will be logged.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 kvm-all.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 989fdd9..616bf04 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -335,12 +335,10 @@ static int kvm_dirty_pages_log_change(hwaddr phys_addr,
     KVMSlot *mem = kvm_lookup_matching_slot(s, phys_addr, phys_addr + size);
 
     if (mem == NULL)  {
-        fprintf(stderr, "BUG: %s: invalid parameters " TARGET_FMT_plx "-"
-                TARGET_FMT_plx "\n", __func__, phys_addr,
-                (hwaddr)(phys_addr + size - 1));
-        return -EINVAL;
+        return 0;
+    } else {
+        return kvm_slot_dirty_pages_log_change(mem, log_dirty);
     }
-    return kvm_slot_dirty_pages_log_change(mem, log_dirty);
 }
 
 static void kvm_log_start(MemoryListener *listener,
-- 
1.8.3.1

[Qemu-devel] [PATCH 11/29] memory: include DIRTY_MEMORY_MIGRATION in the dirty log mask

[Paolo Bonzini]

The separate handling of DIRTY_MEMORY_MIGRATION, which does not
call log_start/log_stop callbacks when it changes in a region's
dirty logging mask, has caused several bugs.

One recent example is commit 4cc856f (kvm-all: Sync dirty-bitmap from
kvm before kvm destroy the corresponding dirty_bitmap, 2015-04-02).
Another performance problem is that KVM keeps tracking dirty pages
after a failed live migration, which causes bad performance due to
disallowing huge page mapping.

This patch removes the root cause of the problem by reporting
DIRTY_MEMORY_MIGRATION changes via log_start and log_stop.
Note that we now have to rebuild the FlatView when global dirty
logging is enabled or disabled; this ensures that log_start and
log_stop callbacks are invoked.

This will also be used to make the setting of bitmaps conditional.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 memory.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/memory.c b/memory.c
index 1966347..174cd15 100644
--- a/memory.c
+++ b/memory.c
@@ -537,7 +537,7 @@ static void render_memory_region(FlatView *view,
     remain = clip.size;
 
     fr.mr = mr;
-    fr.dirty_log_mask = mr->dirty_log_mask;
+    fr.dirty_log_mask = memory_region_get_dirty_log_mask(mr);
     fr.romd_mode = mr->romd_mode;
     fr.readonly = readonly;
 
@@ -1329,7 +1329,11 @@ bool memory_region_is_skip_dump(MemoryRegion *mr)
 
 uint8_t memory_region_get_dirty_log_mask(MemoryRegion *mr)
 {
-    return mr->dirty_log_mask;
+    uint8_t mask = mr->dirty_log_mask;
+    if (global_dirty_log) {
+        mask |= (1 << DIRTY_MEMORY_MIGRATION);
+    }
+    return mask;
 }
 
 bool memory_region_is_logging(MemoryRegion *mr, uint8_t client)
@@ -1892,10 +1896,20 @@ void memory_global_dirty_log_start(void)
 {
     global_dirty_log = true;
     MEMORY_LISTENER_CALL_GLOBAL(log_global_start, Forward);
+
+    /* Refresh DIRTY_LOG_MIGRATION bit.  */
+    memory_region_transaction_begin();
+    memory_region_update_pending = true;
+    memory_region_transaction_commit();
 }
 
 void memory_global_dirty_log_stop(void)
 {
+    /* Refresh DIRTY_LOG_MIGRATION bit.  */
+    memory_region_transaction_begin();
+    memory_region_update_pending = true;
+    memory_region_transaction_commit();
+
     global_dirty_log = false;
     MEMORY_LISTENER_CALL_GLOBAL(log_global_stop, Reverse);
 }
-- 
1.8.3.1

[Qemu-devel] [PATCH 12/29] kvm: remove special handling of DIRTY_MEMORY_MIGRATION in the dirty log mask

[Paolo Bonzini]

One recent example is commit 4cc856f (kvm-all: Sync dirty-bitmap from
kvm before kvm destroy the corresponding dirty_bitmap, 2015-04-02).
Another performance problem is that KVM keeps tracking dirty pages
after a failed live migration, which causes bad performance due to
disallowing huge page mapping.

Thanks to the previous patch, KVM can now stop hooking into
log_global_start/stop.  This simplifies the KVM code noticeably.

Reported-by: Wanpeng Li <wanpeng.li@linux.intel.com>
Reported-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 kvm-all.c | 56 ++------------------------------------------------------
 1 file changed, 2 insertions(+), 54 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 616bf04..c43a4a1 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -83,7 +83,6 @@ struct KVMState
     struct kvm_coalesced_mmio_ring *coalesced_mmio_ring;
     bool coalesced_flush_in_progress;
     int broken_set_mem_region;
-    int migration_log;
     int vcpu_events;
     int robust_singlestep;
     int debugregs;
@@ -234,9 +233,6 @@ static int kvm_set_user_memory_region(KVMState *s, KVMSlot *slot)
     mem.guest_phys_addr = slot->start_addr;
     mem.userspace_addr = (unsigned long)slot->ram;
     mem.flags = slot->flags;
-    if (s->migration_log) {
-        mem.flags |= KVM_MEM_LOG_DIRTY_PAGES;
-    }
 
     if (slot->memory_size && mem.flags & KVM_MEM_READONLY) {
         /* Set the slot size to 0 before setting the slot to the desired
@@ -317,10 +313,6 @@ static int kvm_slot_dirty_pages_log_change(KVMSlot *mem, bool log_dirty)
     mem->flags = flags;
 
     /* If nothing changed effectively, no need to issue ioctl */
-    if (s->migration_log) {
-        flags |= KVM_MEM_LOG_DIRTY_PAGES;
-    }
-
     if (flags == old_flags) {
         return 0;
     }
@@ -375,31 +367,6 @@ static void kvm_log_stop(MemoryListener *listener,
     }
 }
 
-static int kvm_set_migration_log(bool enable)
-{
-    KVMState *s = kvm_state;
-    KVMSlot *mem;
-    int i, err;
-
-    s->migration_log = enable;
-
-    for (i = 0; i < s->nr_slots; i++) {
-        mem = &s->slots[i];
-
-        if (!mem->memory_size) {
-            continue;
-        }
-        if (!!(mem->flags & KVM_MEM_LOG_DIRTY_PAGES) == enable) {
-            continue;
-        }
-        err = kvm_set_user_memory_region(s, mem);
-        if (err) {
-            return err;
-        }
-    }
-    return 0;
-}
-
 /* get kvm's dirty pages bitmap and update qemu's */
 static int kvm_get_dirty_pages_log_range(MemoryRegionSection *section,
                                          unsigned long *bitmap)
@@ -671,8 +638,7 @@ static void kvm_set_phys_mem(MemoryRegionSection *section, bool add)
     KVMSlot *mem, old;
     int err;
     MemoryRegion *mr = section->mr;
-    bool log_dirty =
-        memory_region_get_dirty_log_mask(mr) & ~(1 << DIRTY_MEMORY_MIGRATION);
+    bool log_dirty = memory_region_get_dirty_log_mask(mr) != 0;
     bool writeable = !mr->readonly && !mr->rom_device;
     bool readonly_flag = mr->readonly || memory_region_is_romd(mr);
     hwaddr start_addr = section->offset_within_address_space;
@@ -724,7 +690,7 @@ static void kvm_set_phys_mem(MemoryRegionSection *section, bool add)
 
         old = *mem;
 
-        if ((mem->flags & KVM_MEM_LOG_DIRTY_PAGES) || s->migration_log) {
+        if (mem->flags & KVM_MEM_LOG_DIRTY_PAGES) {
             kvm_physical_sync_dirty_bitmap(section);
         }
 
@@ -853,22 +819,6 @@ static void kvm_log_sync(MemoryListener *listener,
     }
 }
 
-static void kvm_log_global_start(struct MemoryListener *listener)
-{
-    int r;
-
-    r = kvm_set_migration_log(1);
-    assert(r >= 0);
-}
-
-static void kvm_log_global_stop(struct MemoryListener *listener)
-{
-    int r;
-
-    r = kvm_set_migration_log(0);
-    assert(r >= 0);
-}
-
 static void kvm_mem_ioeventfd_add(MemoryListener *listener,
                                   MemoryRegionSection *section,
                                   bool match_data, uint64_t data,
@@ -944,8 +894,6 @@ static MemoryListener kvm_memory_listener = {
     .log_start = kvm_log_start,
     .log_stop = kvm_log_stop,
     .log_sync = kvm_log_sync,
-    .log_global_start = kvm_log_global_start,
-    .log_global_stop = kvm_log_global_stop,
     .eventfd_add = kvm_mem_ioeventfd_add,
     .eventfd_del = kvm_mem_ioeventfd_del,
     .coalesced_mmio_add = kvm_coalesce_mmio_region,
-- 
1.8.3.1

[Qemu-devel] [PATCH 13/29] ram_addr: tweaks to xen_modified_memory

[Paolo Bonzini]

Invoke xen_modified_memory from cpu_physical_memory_set_dirty_range_nocode;
it is akin to DIRTY_MEMORY_MIGRATION, so set it together with that bitmap.
The remaining call from invalidate_and_set_dirty's "else" branch will go
away soon.

Second, fix the second argument to the function in the
cpu_physical_memory_set_dirty_lebitmap call site.  That function is only used
by KVM, but it is better to be clean anyway.

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 exec.c                  | 3 ++-
 include/exec/ram_addr.h | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/exec.c b/exec.c
index 4717928..083d65d 100644
--- a/exec.c
+++ b/exec.c
@@ -2260,8 +2260,9 @@ static void invalidate_and_set_dirty(hwaddr addr,
     if (cpu_physical_memory_range_includes_clean(addr, length)) {
         tb_invalidate_phys_range(addr, addr + length, 0);
         cpu_physical_memory_set_dirty_range_nocode(addr, length);
+    } else {
+        xen_modified_memory(addr, length);
     }
-    xen_modified_memory(addr, length);
 }
 
 static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index ff558a4..7f6e928 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -112,6 +112,7 @@ static inline void cpu_physical_memory_set_dirty_range_nocode(ram_addr_t start,
     page = start >> TARGET_PAGE_BITS;
     bitmap_set(ram_list.dirty_memory[DIRTY_MEMORY_MIGRATION], page, end - page);
     bitmap_set(ram_list.dirty_memory[DIRTY_MEMORY_VGA], page, end - page);
+    xen_modified_memory(start, length);
 }
 
 static inline void cpu_physical_memory_set_dirty_range(ram_addr_t start,
@@ -155,7 +156,7 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
                 ram_list.dirty_memory[DIRTY_MEMORY_CODE][page + k] |= temp;
             }
         }
-        xen_modified_memory(start, pages);
+        xen_modified_memory(start, pages << TARGET_PAGE_BITS);
     } else {
         /*
          * bitmap-traveling is faster than memory-traveling (for addr...)
-- 
1.8.3.1

[Qemu-devel] [PATCH 14/29] exec: use memory_region_get_dirty_log_mask to optimize dirty tracking

[Paolo Bonzini]

The memory API can now return the exact set of bitmaps that have to
be tracked.  Use it instead of the in_migration variable.

In the next patches, we will also use it to set only DIRTY_MEMORY_VGA
or DIRTY_MEMORY_MIGRATION if necessary.  This can make a difference
for dataplane, especially after the dirty bitmap is changed to use
more expensive atomic operations.

Of some interest is the change to stl_phys_notdirty.  When migration
was introduced, stl_phys_notdirty was changed to effectively behave
as stl_phys during migration.  In fact, if one looks at the function as it
was in the beginning (commit 8df1cd0, physical memory access functions,
2005-01-28), at the time the dirty bitmap was the equivalent of
DIRTY_MEMORY_CODE nowadays; hence, the function simply should not touch
the dirty code bits.  This patch changes it to do the intended thing.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 exec.c | 59 +++++++++++++++++++----------------------------------------
 1 file changed, 19 insertions(+), 40 deletions(-)

diff --git a/exec.c b/exec.c
index 083d65d..6e83161 100644
--- a/exec.c
+++ b/exec.c
@@ -59,8 +59,6 @@
 //#define DEBUG_SUBPAGE
 
 #if !defined(CONFIG_USER_ONLY)
-static bool in_migration;
-
 /* ram_list is read under rcu_read_lock()/rcu_read_unlock().  Writes
  * are protected by the ramlist lock.
  */
@@ -871,11 +869,6 @@ void cpu_physical_memory_reset_dirty(ram_addr_t start, ram_addr_t length,
     }
 }
 
-static void cpu_physical_memory_set_dirty_tracking(bool enable)
-{
-    in_migration = enable;
-}
-
 /* Called from RCU critical section */
 hwaddr memory_region_section_get_iotlb(CPUState *cpu,
                                        MemoryRegionSection *section,
@@ -2140,22 +2133,6 @@ static void tcg_commit(MemoryListener *listener)
     }
 }
 
-static void core_log_global_start(MemoryListener *listener)
-{
-    cpu_physical_memory_set_dirty_tracking(true);
-}
-
-static void core_log_global_stop(MemoryListener *listener)
-{
-    cpu_physical_memory_set_dirty_tracking(false);
-}
-
-static MemoryListener core_memory_listener = {
-    .log_global_start = core_log_global_start,
-    .log_global_stop = core_log_global_stop,
-    .priority = 1,
-};
-
 void address_space_init_dispatch(AddressSpace *as)
 {
     as->dispatch = NULL;
@@ -2195,8 +2172,6 @@ static void memory_map_init(void)
     memory_region_init_io(system_io, NULL, &unassigned_io_ops, NULL, "io",
                           65536);
     address_space_init(&address_space_io, system_io, "I/O");
-
-    memory_listener_register(&core_memory_listener, &address_space_memory);
 }
 
 MemoryRegion *get_system_memory(void)
@@ -2254,12 +2229,18 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
 
 #else
 
-static void invalidate_and_set_dirty(hwaddr addr,
+static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr,
                                      hwaddr length)
 {
     if (cpu_physical_memory_range_includes_clean(addr, length)) {
-        tb_invalidate_phys_range(addr, addr + length, 0);
-        cpu_physical_memory_set_dirty_range_nocode(addr, length);
+        uint8_t dirty_log_mask = memory_region_get_dirty_log_mask(mr);
+        if (dirty_log_mask & (1 << DIRTY_MEMORY_CODE)) {
+            tb_invalidate_phys_range(addr, addr + length, 0);
+            dirty_log_mask &= ~(1 << DIRTY_MEMORY_CODE);
+        }
+        if (dirty_log_mask) {
+            cpu_physical_memory_set_dirty_range_nocode(addr, length);
+        }
     } else {
         xen_modified_memory(addr, length);
     }
@@ -2342,7 +2323,7 @@ bool address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
                 /* RAM case */
                 ptr = qemu_get_ram_ptr(addr1);
                 memcpy(ptr, buf, l);
-                invalidate_and_set_dirty(addr1, l);
+                invalidate_and_set_dirty(mr, addr1, l);
             }
         } else {
             if (!memory_access_is_direct(mr, is_write)) {
@@ -2431,7 +2412,7 @@ static inline void cpu_physical_memory_write_rom_internal(AddressSpace *as,
             switch (type) {
             case WRITE_DATA:
                 memcpy(ptr, buf, l);
-                invalidate_and_set_dirty(addr1, l);
+                invalidate_and_set_dirty(mr, addr1, l);
                 break;
             case FLUSH_CACHE:
                 flush_icache_range((uintptr_t)ptr, (uintptr_t)ptr + l);
@@ -2647,7 +2628,7 @@ void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
         mr = qemu_ram_addr_from_host(buffer, &addr1);
         assert(mr != NULL);
         if (is_write) {
-            invalidate_and_set_dirty(addr1, access_len);
+            invalidate_and_set_dirty(mr, addr1, access_len);
         }
         if (xen_enabled()) {
             xen_invalidate_map_cache_entry(buffer);
@@ -2871,6 +2852,7 @@ void stl_phys_notdirty(AddressSpace *as, hwaddr addr, uint32_t val)
     MemoryRegion *mr;
     hwaddr l = 4;
     hwaddr addr1;
+    uint8_t dirty_log_mask;
 
     mr = address_space_translate(as, addr, &addr1, &l,
                                  true);
@@ -2881,13 +2863,10 @@ void stl_phys_notdirty(AddressSpace *as, hwaddr addr, uint32_t val)
         ptr = qemu_get_ram_ptr(addr1);
         stl_p(ptr, val);
 
-        if (unlikely(in_migration)) {
-            if (cpu_physical_memory_is_clean(addr1)) {
-                /* invalidate code */
-                tb_invalidate_phys_page_range(addr1, addr1 + 4, 0);
-                /* set dirty bit */
-                cpu_physical_memory_set_dirty_range_nocode(addr1, 4);
-            }
+        dirty_log_mask = memory_region_get_dirty_log_mask(mr);
+        dirty_log_mask &= ~(1 << DIRTY_MEMORY_CODE);
+        if (dirty_log_mask) {
+            cpu_physical_memory_set_dirty_range_nocode(addr1, 4);
         }
     }
 }
@@ -2930,7 +2909,7 @@ static inline void stl_phys_internal(AddressSpace *as,
             stl_p(ptr, val);
             break;
         }
-        invalidate_and_set_dirty(addr1, 4);
+        invalidate_and_set_dirty(mr, addr1, 4);
     }
 }
 
@@ -2993,7 +2972,7 @@ static inline void stw_phys_internal(AddressSpace *as,
             stw_p(ptr, val);
             break;
         }
-        invalidate_and_set_dirty(addr1, 2);
+        invalidate_and_set_dirty(mr, addr1, 2);
     }
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 15/29] exec: move functions to translate-all.h

[Paolo Bonzini]

Remove them from the sundry exec-all.h header, since they are only used by
the TCG runtime in exec.c and user-exec.c.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/exec/exec-all.h | 6 +-----
 linux-user/mmap.c       | 1 +
 translate-all.h         | 8 ++++++++
 user-exec.c             | 1 +
 4 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 8eb0db3..e8f5fd8 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -90,11 +90,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
                               int cflags);
 void cpu_exec_init(CPUArchState *env);
 void QEMU_NORETURN cpu_loop_exit(CPUState *cpu);
-int page_unprotect(target_ulong address, uintptr_t pc, void *puc);
-void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
-                                   int is_cpu_write_access);
-void tb_invalidate_phys_range(tb_page_addr_t start, tb_page_addr_t end,
-                              int is_cpu_write_access);
+
 #if !defined(CONFIG_USER_ONLY)
 bool qemu_in_vcpu_thread(void);
 void cpu_reload_memory_map(CPUState *cpu);
diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index a249f0c..959ff4d 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -30,6 +30,7 @@
 
 #include "qemu.h"
 #include "qemu-common.h"
+#include "translate-all.h"
 
 //#define DEBUG_MMAP
 
diff --git a/translate-all.h b/translate-all.h
index 02832b2..9d115a6 100644
--- a/translate-all.h
+++ b/translate-all.h
@@ -21,7 +21,15 @@
 
 /* translate-all.c */
 void tb_invalidate_phys_page_fast(tb_page_addr_t start, int len);
+void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
+                                   int is_cpu_write_access);
+void tb_invalidate_phys_range(tb_page_addr_t start, tb_page_addr_t end,
+                              int is_cpu_write_access);
 void cpu_unlink_tb(CPUState *cpu);
 void tb_check_watchpoint(CPUState *cpu);
 
+#ifdef CONFIG_USER_ONLY
+int page_unprotect(target_ulong address, uintptr_t pc, void *puc);
+#endif
+
 #endif /* TRANSLATE_ALL_H */
diff --git a/user-exec.c b/user-exec.c
index 8f57e8a..ed9a07f 100644
--- a/user-exec.c
+++ b/user-exec.c
@@ -22,6 +22,7 @@
 #include "tcg.h"
 #include "qemu/bitops.h"
 #include "exec/cpu_ldst.h"
+#include "translate-all.h"
 
 #undef EAX
 #undef ECX
-- 
1.8.3.1

[Qemu-devel] [PATCH 16/29] translate-all: remove unnecessary argument to tb_invalidate_phys_range

[Paolo Bonzini]

The is_cpu_write_access argument is always 0, remove it.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 exec.c            | 2 +-
 linux-user/mmap.c | 6 +++---
 translate-all.c   | 5 ++---
 translate-all.h   | 3 +--
 4 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/exec.c b/exec.c
index 6e83161..1c75dd1 100644
--- a/exec.c
+++ b/exec.c
@@ -2235,7 +2235,7 @@ static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr,
     if (cpu_physical_memory_range_includes_clean(addr, length)) {
         uint8_t dirty_log_mask = memory_region_get_dirty_log_mask(mr);
         if (dirty_log_mask & (1 << DIRTY_MEMORY_CODE)) {
-            tb_invalidate_phys_range(addr, addr + length, 0);
+            tb_invalidate_phys_range(addr, addr + length);
             dirty_log_mask &= ~(1 << DIRTY_MEMORY_CODE);
         }
         if (dirty_log_mask) {
diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index 959ff4d..78e1b2d 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -575,7 +575,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
     page_dump(stdout);
     printf("\n");
 #endif
-    tb_invalidate_phys_range(start, start + len, 0);
+    tb_invalidate_phys_range(start, start + len);
     mmap_unlock();
     return start;
 fail:
@@ -680,7 +680,7 @@ int target_munmap(abi_ulong start, abi_ulong len)
 
     if (ret == 0) {
         page_set_flags(start, start + len, 0);
-        tb_invalidate_phys_range(start, start + len, 0);
+        tb_invalidate_phys_range(start, start + len);
     }
     mmap_unlock();
     return ret;
@@ -759,7 +759,7 @@ abi_long target_mremap(abi_ulong old_addr, abi_ulong old_size,
         page_set_flags(old_addr, old_addr + old_size, 0);
         page_set_flags(new_addr, new_addr + new_size, prot | PAGE_VALID);
     }
-    tb_invalidate_phys_range(new_addr, new_addr + new_size, 0);
+    tb_invalidate_phys_range(new_addr, new_addr + new_size);
     mmap_unlock();
     return new_addr;
 }
diff --git a/translate-all.c b/translate-all.c
index 65a76c5..bfa8202 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -1042,11 +1042,10 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
  * access: the virtual CPU will exit the current TB if code is modified inside
  * this TB.
  */
-void tb_invalidate_phys_range(tb_page_addr_t start, tb_page_addr_t end,
-                              int is_cpu_write_access)
+void tb_invalidate_phys_range(tb_page_addr_t start, tb_page_addr_t end)
 {
     while (start < end) {
-        tb_invalidate_phys_page_range(start, end, is_cpu_write_access);
+        tb_invalidate_phys_page_range(start, end, 0);
         start &= TARGET_PAGE_MASK;
         start += TARGET_PAGE_SIZE;
     }
diff --git a/translate-all.h b/translate-all.h
index 9d115a6..ef9166c 100644
--- a/translate-all.h
+++ b/translate-all.h
@@ -23,8 +23,7 @@
 void tb_invalidate_phys_page_fast(tb_page_addr_t start, int len);
 void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
                                    int is_cpu_write_access);
-void tb_invalidate_phys_range(tb_page_addr_t start, tb_page_addr_t end,
-                              int is_cpu_write_access);
+void tb_invalidate_phys_range(tb_page_addr_t start, tb_page_addr_t end);
 void cpu_unlink_tb(CPUState *cpu);
 void tb_check_watchpoint(CPUState *cpu);
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 17/29] cputlb: remove useless arguments to tlb_unprotect_code_phys, rename

[Paolo Bonzini]

These days modification of the TLB is done in notdirty_mem_write,
so the virtual address and env pointer as unnecessary.

The new name of the function, tlb_unprotect_code, is consistent with
tlb_protect_code.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cputlb.c              | 3 +--
 include/exec/cputlb.h | 3 +--
 translate-all.c       | 2 +-
 3 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/cputlb.c b/cputlb.c
index 38f2151..a86e76f 100644
--- a/cputlb.c
+++ b/cputlb.c
@@ -131,8 +131,7 @@ void tlb_protect_code(ram_addr_t ram_addr)
 
 /* update the TLB so that writes in physical page 'phys_addr' are no longer
    tested for self modifying code */
-void tlb_unprotect_code_phys(CPUState *cpu, ram_addr_t ram_addr,
-                             target_ulong vaddr)
+void tlb_unprotect_code(ram_addr_t ram_addr)
 {
     cpu_physical_memory_set_dirty_flag(ram_addr, DIRTY_MEMORY_CODE);
 }
diff --git a/include/exec/cputlb.h b/include/exec/cputlb.h
index e0da9d7..360815e 100644
--- a/include/exec/cputlb.h
+++ b/include/exec/cputlb.h
@@ -22,8 +22,7 @@
 #if !defined(CONFIG_USER_ONLY)
 /* cputlb.c */
 void tlb_protect_code(ram_addr_t ram_addr);
-void tlb_unprotect_code_phys(CPUState *cpu, ram_addr_t ram_addr,
-                             target_ulong vaddr);
+void tlb_unprotect_code(ram_addr_t ram_addr);
 void tlb_reset_dirty_range(CPUTLBEntry *tlb_entry, uintptr_t start,
                            uintptr_t length);
 void cpu_tlb_reset_dirty_all(ram_addr_t start1, ram_addr_t length);
diff --git a/translate-all.c b/translate-all.c
index bfa8202..0283edc 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -1158,7 +1158,7 @@ void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
     if (!p->first_tb) {
         invalidate_page_bitmap(p);
         if (is_cpu_write_access) {
-            tlb_unprotect_code_phys(cpu, start, cpu->mem_io_vaddr);
+            tlb_unprotect_code(start);
         }
     }
 #endif
-- 
1.8.3.1

[Qemu-devel] [PATCH 18/29] translate-all: make less of tb_invalidate_phys_page_range depend on is_cpu_write_access

[Paolo Bonzini]

is_cpu_write_access is only set if tb_invalidate_phys_page_range is called
from tb_invalidate_phys_page_fast, and hence from notdirty_mem_write.
However:

- the code bitmap can be built directly in tb_invalidate_phys_page_fast
  (unconditionally, since is_cpu_write_access would always be passed as 1);

- the virtual address is not needed to mark the page as "not containing
  code" (dirty code bitmap = 1), so we can also remove that use of
  is_cpu_write_access.  For calls of tb_invalidate_phys_page_range
  that do not come from notdirty_mem_write, the next call to
  notdirty_mem_write will notice that the page does not contain code
  anymore, and will fix up the TLB entry.

The parameter needs to remain in order to guard accesses to cpu->mem_io_pc.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 translate-all.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/translate-all.c b/translate-all.c
index 0283edc..855b329 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -1082,12 +1082,6 @@ void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
     if (!p) {
         return;
     }
-    if (!p->code_bitmap &&
-        ++p->code_write_count >= SMC_BITMAP_USE_THRESHOLD &&
-        is_cpu_write_access) {
-        /* build code bitmap */
-        build_page_bitmap(p);
-    }
 #if defined(TARGET_HAS_PRECISE_SMC)
     if (cpu != NULL) {
         env = cpu->env_ptr;
@@ -1157,9 +1151,7 @@ void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
     /* if no code remaining, no need to continue to use slow writes */
     if (!p->first_tb) {
         invalidate_page_bitmap(p);
-        if (is_cpu_write_access) {
-            tlb_unprotect_code(start);
-        }
+        tlb_unprotect_code(start);
     }
 #endif
 #ifdef TARGET_HAS_PRECISE_SMC
@@ -1192,6 +1184,11 @@ void tb_invalidate_phys_page_fast(tb_page_addr_t start, int len)
     if (!p) {
         return;
     }
+    if (!p->code_bitmap &&
+        ++p->code_write_count >= SMC_BITMAP_USE_THRESHOLD) {
+        /* build code bitmap */
+        build_page_bitmap(p);
+    }
     if (p->code_bitmap) {
         unsigned int nr;
         unsigned long b;
-- 
1.8.3.1

[Qemu-devel] [PATCH 19/29] exec: pass client mask to cpu_physical_memory_set_dirty_range

[Paolo Bonzini]

This cuts in half the cost of bitmap operations (which will become more
expensive when made atomic) during migration on non-VRAM regions.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 exec.c                  | 20 +++++++++++---------
 include/exec/ram_addr.h | 33 ++++++++++++++++-----------------
 memory.c                |  3 ++-
 3 files changed, 29 insertions(+), 27 deletions(-)

diff --git a/exec.c b/exec.c
index 1c75dd1..cd8665a 100644
--- a/exec.c
+++ b/exec.c
@@ -1356,7 +1356,8 @@ int qemu_ram_resize(ram_addr_t base, ram_addr_t newsize, Error **errp)
 
     cpu_physical_memory_clear_dirty_range(block->offset, block->used_length);
     block->used_length = newsize;
-    cpu_physical_memory_set_dirty_range(block->offset, block->used_length);
+    cpu_physical_memory_set_dirty_range(block->offset, block->used_length,
+                                        DIRTY_CLIENTS_ALL);
     memory_region_set_size(block->mr, newsize);
     if (block->resized) {
         block->resized(block->idstr, newsize, block->host);
@@ -1430,7 +1431,8 @@ static ram_addr_t ram_block_add(RAMBlock *new_block, Error **errp)
        }
     }
     cpu_physical_memory_set_dirty_range(new_block->offset,
-                                        new_block->used_length);
+                                        new_block->used_length,
+                                        DIRTY_CLIENTS_ALL);
 
     if (new_block->host) {
         qemu_ram_setup_dump(new_block->host, new_block->max_length);
@@ -1818,7 +1820,11 @@ static void notdirty_mem_write(void *opaque, hwaddr ram_addr,
     default:
         abort();
     }
-    cpu_physical_memory_set_dirty_range_nocode(ram_addr, size);
+    /* Set both VGA and migration bits for simplicity and to remove
+     * the notdirty callback faster.
+     */
+    cpu_physical_memory_set_dirty_range(ram_addr, size,
+                                        DIRTY_CLIENTS_NOCODE);
     /* we remove the notdirty callback only if the code has been
        flushed */
     if (!cpu_physical_memory_is_clean(ram_addr)) {
@@ -2238,9 +2244,7 @@ static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr,
             tb_invalidate_phys_range(addr, addr + length);
             dirty_log_mask &= ~(1 << DIRTY_MEMORY_CODE);
         }
-        if (dirty_log_mask) {
-            cpu_physical_memory_set_dirty_range_nocode(addr, length);
-        }
+        cpu_physical_memory_set_dirty_range(addr, length, dirty_log_mask);
     } else {
         xen_modified_memory(addr, length);
     }
@@ -2865,9 +2869,7 @@ void stl_phys_notdirty(AddressSpace *as, hwaddr addr, uint32_t val)
 
         dirty_log_mask = memory_region_get_dirty_log_mask(mr);
         dirty_log_mask &= ~(1 << DIRTY_MEMORY_CODE);
-        if (dirty_log_mask) {
-            cpu_physical_memory_set_dirty_range_nocode(addr1, 4);
-        }
+        cpu_physical_memory_set_dirty_range(addr1, 4, dirty_log_mask);
     }
 }
 
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index 7f6e928..29b89d2 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -41,6 +41,9 @@ void qemu_ram_free_from_ptr(ram_addr_t addr);
 
 int qemu_ram_resize(ram_addr_t base, ram_addr_t newsize, Error **errp);
 
+#define DIRTY_CLIENTS_ALL      ((1 << DIRTY_MEMORY_NUM) - 1)
+#define DIRTY_CLIENTS_NOCODE   (DIRTY_CLIENTS_ALL & ~(1 << DIRTY_MEMORY_CODE))
+
 static inline bool cpu_physical_memory_get_dirty(ram_addr_t start,
                                                  ram_addr_t length,
                                                  unsigned client)
@@ -103,28 +106,23 @@ static inline void cpu_physical_memory_set_dirty_flag(ram_addr_t addr,
     set_bit(addr >> TARGET_PAGE_BITS, ram_list.dirty_memory[client]);
 }
 
-static inline void cpu_physical_memory_set_dirty_range_nocode(ram_addr_t start,
-                                                              ram_addr_t length)
-{
-    unsigned long end, page;
-
-    end = TARGET_PAGE_ALIGN(start + length) >> TARGET_PAGE_BITS;
-    page = start >> TARGET_PAGE_BITS;
-    bitmap_set(ram_list.dirty_memory[DIRTY_MEMORY_MIGRATION], page, end - page);
-    bitmap_set(ram_list.dirty_memory[DIRTY_MEMORY_VGA], page, end - page);
-    xen_modified_memory(start, length);
-}
-
 static inline void cpu_physical_memory_set_dirty_range(ram_addr_t start,
-                                                       ram_addr_t length)
+                                                       ram_addr_t length,
+                                                       uint8_t mask)
 {
     unsigned long end, page;
 
     end = TARGET_PAGE_ALIGN(start + length) >> TARGET_PAGE_BITS;
     page = start >> TARGET_PAGE_BITS;
-    bitmap_set(ram_list.dirty_memory[DIRTY_MEMORY_MIGRATION], page, end - page);
-    bitmap_set(ram_list.dirty_memory[DIRTY_MEMORY_VGA], page, end - page);
-    bitmap_set(ram_list.dirty_memory[DIRTY_MEMORY_CODE], page, end - page);
+    if (likely(mask & (1 << DIRTY_MEMORY_MIGRATION))) {
+        bitmap_set(ram_list.dirty_memory[DIRTY_MEMORY_MIGRATION], page, end - page);
+    }
+    if (unlikely(mask & (1 << DIRTY_MEMORY_VGA))) {
+        bitmap_set(ram_list.dirty_memory[DIRTY_MEMORY_VGA], page, end - page);
+    }
+    if (unlikely(mask & (1 << DIRTY_MEMORY_CODE))) {
+        bitmap_set(ram_list.dirty_memory[DIRTY_MEMORY_CODE], page, end - page);
+    }
     xen_modified_memory(start, length);
 }
 
@@ -172,7 +170,8 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
                     addr = page_number * TARGET_PAGE_SIZE;
                     ram_addr = start + addr;
                     cpu_physical_memory_set_dirty_range(ram_addr,
-                                       TARGET_PAGE_SIZE * hpratio);
+                                       TARGET_PAGE_SIZE * hpratio,
+                                       DIRTY_CLIENTS_ALL);
                 } while (c != 0);
             }
         }
diff --git a/memory.c b/memory.c
index 174cd15..7422790 100644
--- a/memory.c
+++ b/memory.c
@@ -1390,7 +1390,8 @@ void memory_region_set_dirty(MemoryRegion *mr, hwaddr addr,
                              hwaddr size)
 {
     assert(mr->terminates);
-    cpu_physical_memory_set_dirty_range(mr->ram_addr + addr, size);
+    cpu_physical_memory_set_dirty_range(mr->ram_addr + addr, size,
+                                        memory_region_get_dirty_log_mask(mr));
 }
 
 bool memory_region_test_and_clear_dirty(MemoryRegion *mr, hwaddr addr,
-- 
1.8.3.1

[Qemu-devel] [PATCH 20/29] exec: invert return value of cpu_physical_memory_get_clean, rename

[Paolo Bonzini]

While it is obvious that cpu_physical_memory_get_dirty returns true even if
a single page is dirty, the same is not true for cpu_physical_memory_get_clean;
one would expect that it returns true only if all the pages are clean, but
it actually looks for even one clean page.  (By contrast, the caller of that
function, cpu_physical_memory_range_includes_clean, has a good name).

To clarify, rename the function to cpu_physical_memory_all_dirty and return
true if _all_ the pages are dirty.  This is the opposite of the previous
meaning, because "all are 1" is the same as "not (any is 0)", so we have to
modify cpu_physical_memory_range_includes_clean as well.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/exec/ram_addr.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index 29b89d2..c800b58 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -59,7 +59,7 @@ static inline bool cpu_physical_memory_get_dirty(ram_addr_t start,
     return next < end;
 }
 
-static inline bool cpu_physical_memory_get_clean(ram_addr_t start,
+static inline bool cpu_physical_memory_all_dirty(ram_addr_t start,
                                                  ram_addr_t length,
                                                  unsigned client)
 {
@@ -71,7 +71,7 @@ static inline bool cpu_physical_memory_get_clean(ram_addr_t start,
     page = start >> TARGET_PAGE_BITS;
     next = find_next_zero_bit(ram_list.dirty_memory[client], end, page);
 
-    return next < end;
+    return next >= end;
 }
 
 static inline bool cpu_physical_memory_get_dirty_flag(ram_addr_t addr,
@@ -92,10 +92,10 @@ static inline bool cpu_physical_memory_is_clean(ram_addr_t addr)
 static inline bool cpu_physical_memory_range_includes_clean(ram_addr_t start,
                                                             ram_addr_t length)
 {
-    bool vga = cpu_physical_memory_get_clean(start, length, DIRTY_MEMORY_VGA);
-    bool code = cpu_physical_memory_get_clean(start, length, DIRTY_MEMORY_CODE);
+    bool vga = !cpu_physical_memory_all_dirty(start, length, DIRTY_MEMORY_VGA);
+    bool code = !cpu_physical_memory_all_dirty(start, length, DIRTY_MEMORY_CODE);
     bool migration =
-        cpu_physical_memory_get_clean(start, length, DIRTY_MEMORY_MIGRATION);
+        !cpu_physical_memory_all_dirty(start, length, DIRTY_MEMORY_MIGRATION);
     return vga || code || migration;
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 21/29] exec: only check relevant bitmaps for cleanliness

[Paolo Bonzini]

Most of the time, not all bitmaps have to be marked as dirty;
do not do anything if the interesting ones are already dirty.
Previously, any clean bitmap would have cause all the bitmaps to be
marked dirty.

In fact, unless running TCG most of the time bitmap operations need
not be done at all, because memory_region_is_logging returns zero.
In this case, skip the call to cpu_physical_memory_range_includes_clean
altogether as well.

With this patch, cpu_physical_memory_set_dirty_range is called
unconditionally, so there need not be anymore a separate call to
xen_modified_memory.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 exec.c                  | 22 +++++++++++++---------
 include/exec/ram_addr.h | 25 ++++++++++++++++++-------
 2 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/exec.c b/exec.c
index cd8665a..b7aebbf 100644
--- a/exec.c
+++ b/exec.c
@@ -2238,16 +2238,20 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
 static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr,
                                      hwaddr length)
 {
-    if (cpu_physical_memory_range_includes_clean(addr, length)) {
-        uint8_t dirty_log_mask = memory_region_get_dirty_log_mask(mr);
-        if (dirty_log_mask & (1 << DIRTY_MEMORY_CODE)) {
-            tb_invalidate_phys_range(addr, addr + length);
-            dirty_log_mask &= ~(1 << DIRTY_MEMORY_CODE);
-        }
-        cpu_physical_memory_set_dirty_range(addr, length, dirty_log_mask);
-    } else {
-        xen_modified_memory(addr, length);
+    uint8_t dirty_log_mask = memory_region_get_dirty_log_mask(mr);
+    /* No early return if dirty_log_mask is or becomes 0, because
+     * cpu_physical_memory_set_dirty_range will still call
+     * xen_modified_memory.
+     */
+    if (dirty_log_mask) {
+        dirty_log_mask =
+            cpu_physical_memory_range_includes_clean(addr, length, dirty_log_mask);
+    }
+    if (dirty_log_mask & (1 << DIRTY_MEMORY_CODE)) {
+        tb_invalidate_phys_range(addr, addr + length);
+        dirty_log_mask &= ~(1 << DIRTY_MEMORY_CODE);
     }
+    cpu_physical_memory_set_dirty_range(addr, length, dirty_log_mask);
 }
 
 static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index c800b58..1928b80 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -89,14 +89,25 @@ static inline bool cpu_physical_memory_is_clean(ram_addr_t addr)
     return !(vga && code && migration);
 }
 
-static inline bool cpu_physical_memory_range_includes_clean(ram_addr_t start,
-                                                            ram_addr_t length)
+static inline uint8_t cpu_physical_memory_range_includes_clean(ram_addr_t start,
+                                                               ram_addr_t length,
+                                                               uint8_t mask)
 {
-    bool vga = !cpu_physical_memory_all_dirty(start, length, DIRTY_MEMORY_VGA);
-    bool code = !cpu_physical_memory_all_dirty(start, length, DIRTY_MEMORY_CODE);
-    bool migration =
-        !cpu_physical_memory_all_dirty(start, length, DIRTY_MEMORY_MIGRATION);
-    return vga || code || migration;
+    uint8_t ret = 0;
+
+    if (mask & (1 << DIRTY_MEMORY_VGA) &&
+        !cpu_physical_memory_all_dirty(start, length, DIRTY_MEMORY_VGA)) {
+        ret |= (1 << DIRTY_MEMORY_VGA);
+    }
+    if (mask & (1 << DIRTY_MEMORY_CODE) &&
+        !cpu_physical_memory_all_dirty(start, length, DIRTY_MEMORY_CODE)) {
+        ret |= (1 << DIRTY_MEMORY_CODE);
+    }
+    if (mask & (1 << DIRTY_MEMORY_MIGRATION) &&
+        !cpu_physical_memory_all_dirty(start, length, DIRTY_MEMORY_MIGRATION)) {
+        ret |= (1 << DIRTY_MEMORY_MIGRATION);
+    }
+    return ret;
 }
 
 static inline void cpu_physical_memory_set_dirty_flag(ram_addr_t addr,
-- 
1.8.3.1

[Qemu-devel] [PATCH 22/29] memory: do not touch code dirty bitmap unless TCG is enabled

[Paolo Bonzini]

cpu_physical_memory_set_dirty_lebitmap unconditionally syncs the
DIRTY_MEMORY_CODE bitmap.  This however is unused unless TCG is
enabled.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/exec/ram_addr.h | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index 1928b80..40b4b47 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -162,11 +162,14 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
 
                 ram_list.dirty_memory[DIRTY_MEMORY_MIGRATION][page + k] |= temp;
                 ram_list.dirty_memory[DIRTY_MEMORY_VGA][page + k] |= temp;
-                ram_list.dirty_memory[DIRTY_MEMORY_CODE][page + k] |= temp;
+                if (tcg_enabled()) {
+                    ram_list.dirty_memory[DIRTY_MEMORY_CODE][page + k] |= temp;
+                }
             }
         }
         xen_modified_memory(start, pages << TARGET_PAGE_BITS);
     } else {
+        uint8_t clients = tcg_enabled() ? DIRTY_CLIENTS_ALL : DIRTY_CLIENTS_NOCODE;
         /*
          * bitmap-traveling is faster than memory-traveling (for addr...)
          * especially when most of the memory is not dirty.
@@ -181,8 +184,7 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
                     addr = page_number * TARGET_PAGE_SIZE;
                     ram_addr = start + addr;
                     cpu_physical_memory_set_dirty_range(ram_addr,
-                                       TARGET_PAGE_SIZE * hpratio,
-                                       DIRTY_CLIENTS_ALL);
+                                       TARGET_PAGE_SIZE * hpratio, clients);
                 } while (c != 0);
             }
         }
-- 
1.8.3.1

[Qemu-devel] [PATCH 23/29] bitmap: add atomic set functions

[Paolo Bonzini]

From: Stefan Hajnoczi <stefanha@redhat.com>

Use atomic_or() for atomic bitmaps where several threads may set bits at
the same time.  This avoids the race condition between threads loading
an element, bitwise ORing, and then storing the element.

When setting all bits in a word we can avoid atomic ops and instead just
use an smp_mb() at the end.

Most bitmap users don't need atomicity so introduce new functions.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <1417519399-3166-2-git-send-email-stefanha@redhat.com>
[Avoid barrier in the single word case, use full barrier instead of write.
 - Paolo]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qemu/bitmap.h |  2 ++
 include/qemu/bitops.h | 14 ++++++++++++++
 util/bitmap.c         | 37 +++++++++++++++++++++++++++++++++++++
 3 files changed, 53 insertions(+)

diff --git a/include/qemu/bitmap.h b/include/qemu/bitmap.h
index f0273c9..3e0a4f3 100644
--- a/include/qemu/bitmap.h
+++ b/include/qemu/bitmap.h
@@ -39,6 +39,7 @@
  * bitmap_empty(src, nbits)			Are all bits zero in *src?
  * bitmap_full(src, nbits)			Are all bits set in *src?
  * bitmap_set(dst, pos, nbits)			Set specified bit area
+ * bitmap_set_atomic(dst, pos, nbits)   Set specified bit area with atomic ops
  * bitmap_clear(dst, pos, nbits)		Clear specified bit area
  * bitmap_find_next_zero_area(buf, len, pos, n, mask)	Find bit free area
  */
@@ -226,6 +227,7 @@ static inline int bitmap_intersects(const unsigned long *src1,
 }
 
 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);
 unsigned long bitmap_find_next_zero_area(unsigned long *map,
                                          unsigned long size,
diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
index 90ca8df..95f98fb 100644
--- a/include/qemu/bitops.h
+++ b/include/qemu/bitops.h
@@ -16,6 +16,7 @@
 #include <assert.h>
 
 #include "host-utils.h"
+#include "atomic.h"
 
 #define BITS_PER_BYTE           CHAR_BIT
 #define BITS_PER_LONG           (sizeof (unsigned long) * BITS_PER_BYTE)
@@ -39,6 +40,19 @@ static inline void set_bit(long nr, unsigned long *addr)
 }
 
 /**
+ * set_bit_atomic - Set a bit in memory atomically
+ * @nr: the bit to set
+ * @addr: the address to start counting from
+ */
+static inline void set_bit_atomic(long nr, unsigned long *addr)
+{
+    unsigned long mask = BIT_MASK(nr);
+    unsigned long *p = addr + BIT_WORD(nr);
+
+    atomic_or(p, mask);
+}
+
+/**
  * clear_bit - Clears a bit in memory
  * @nr: Bit to clear
  * @addr: Address to start counting from
diff --git a/util/bitmap.c b/util/bitmap.c
index 9c6bb52..6838d49 100644
--- a/util/bitmap.c
+++ b/util/bitmap.c
@@ -11,6 +11,7 @@
 
 #include "qemu/bitops.h"
 #include "qemu/bitmap.h"
+#include "qemu/atomic.h"
 
 /*
  * bitmaps provide an array of bits, implemented using an an
@@ -177,6 +178,42 @@ void bitmap_set(unsigned long *map, long start, long nr)
     }
 }
 
+void bitmap_set_atomic(unsigned long *map, long start, long nr)
+{
+    unsigned long *p = map + BIT_WORD(start);
+    const long size = start + nr;
+    int bits_to_set = BITS_PER_LONG - (start % BITS_PER_LONG);
+    unsigned long mask_to_set = BITMAP_FIRST_WORD_MASK(start);
+
+    /* First word */
+    if (nr - bits_to_set > 0) {
+        atomic_or(p, mask_to_set);
+        nr -= bits_to_set;
+        bits_to_set = BITS_PER_LONG;
+        mask_to_set = ~0UL;
+        p++;
+    }
+
+    /* Full words */
+    while (nr - bits_to_set >= 0) {
+        *p = ~0UL;
+        nr -= bits_to_set;
+        mask_to_set = ~0UL;
+        p++;
+    }
+
+    /* Last word */
+    if (nr) {
+        mask_to_set &= BITMAP_LAST_WORD_MASK(size);
+        atomic_or(p, mask_to_set);
+    } else {
+        /* If we avoided the full barrier in atomic_or(), issue a
+         * barrier to account for the assignments in the while loop.
+         */
+        smp_mb();
+    }
+}
+
 void bitmap_clear(unsigned long *map, long start, long nr)
 {
     unsigned long *p = map + BIT_WORD(start);
-- 
1.8.3.1

[Qemu-devel] [PATCH 24/29] bitmap: add atomic test and clear

[Paolo Bonzini]

From: Stefan Hajnoczi <stefanha@redhat.com>

The new bitmap_test_and_clear_atomic() function clears a range and
returns whether or not the bits were set.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <1417519399-3166-3-git-send-email-stefanha@redhat.com>
[Test before xchg; then a full barrier is needed at the end just like
 in the previous patch.  The barrier can be avoided if we did at least
 one xchg.  - Paolo]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qemu/bitmap.h |  2 ++
 util/bitmap.c         | 44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/include/qemu/bitmap.h b/include/qemu/bitmap.h
index 3e0a4f3..86dd9cd 100644
--- a/include/qemu/bitmap.h
+++ b/include/qemu/bitmap.h
@@ -41,6 +41,7 @@
  * bitmap_set(dst, pos, nbits)			Set specified bit area
  * bitmap_set_atomic(dst, pos, nbits)   Set specified bit area with atomic ops
  * bitmap_clear(dst, pos, nbits)		Clear specified bit area
+ * bitmap_test_and_clear_atomic(dst, pos, nbits)    Test and clear area
  * bitmap_find_next_zero_area(buf, len, pos, n, mask)	Find bit free area
  */
 
@@ -229,6 +230,7 @@ static inline int bitmap_intersects(const unsigned long *src1,
 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);
 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 6838d49..d3e088d 100644
--- a/util/bitmap.c
+++ b/util/bitmap.c
@@ -234,6 +234,50 @@ void bitmap_clear(unsigned long *map, long start, long nr)
     }
 }
 
+bool bitmap_test_and_clear_atomic(unsigned long *map, long start, long nr)
+{
+    unsigned long *p = map + BIT_WORD(start);
+    const long size = start + nr;
+    int bits_to_clear = BITS_PER_LONG - (start % BITS_PER_LONG);
+    unsigned long mask_to_clear = BITMAP_FIRST_WORD_MASK(start);
+    unsigned long dirty = 0;
+    unsigned long old_bits;
+
+    /* First word */
+    if (nr - bits_to_clear > 0) {
+        old_bits = atomic_fetch_and(p, ~mask_to_clear);
+        dirty |= old_bits & mask_to_clear;
+        nr -= bits_to_clear;
+        bits_to_clear = BITS_PER_LONG;
+        mask_to_clear = ~0UL;
+        p++;
+    }
+
+    /* Full words */
+    while (nr - bits_to_clear >= 0) {
+        if (*p) {
+            old_bits = atomic_xchg(p, 0);
+            dirty |= old_bits;
+        }
+        nr -= bits_to_clear;
+        mask_to_clear = ~0UL;
+        p++;
+    }
+
+    /* Last word */
+    if (nr) {
+        mask_to_clear &= BITMAP_LAST_WORD_MASK(size);
+        old_bits = atomic_fetch_and(p, ~mask_to_clear);
+        dirty |= old_bits & mask_to_clear;
+    } else {
+        if (!dirty) {
+            smp_mb();
+        }
+    }
+
+    return dirty != 0;
+}
+
 #define ALIGN_MASK(x,mask)      (((x)+(mask))&~(mask))
 
 /**
-- 
1.8.3.1

[Qemu-devel] [PATCH 25/29] memory: use atomic ops for setting dirty memory bits

[Paolo Bonzini]

From: Stefan Hajnoczi <stefanha@redhat.com>

Use set_bit_atomic() and bitmap_set_atomic() so that multiple threads
can dirty memory without race conditions.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <1417519399-3166-4-git-send-email-stefanha@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/exec/ram_addr.h | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index 40b4b47..bb69a28 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -114,7 +114,7 @@ static inline void cpu_physical_memory_set_dirty_flag(ram_addr_t addr,
                                                       unsigned client)
 {
     assert(client < DIRTY_MEMORY_NUM);
-    set_bit(addr >> TARGET_PAGE_BITS, ram_list.dirty_memory[client]);
+    set_bit_atomic(addr >> TARGET_PAGE_BITS, ram_list.dirty_memory[client]);
 }
 
 static inline void cpu_physical_memory_set_dirty_range(ram_addr_t start,
@@ -122,17 +122,18 @@ static inline void cpu_physical_memory_set_dirty_range(ram_addr_t start,
                                                        uint8_t mask)
 {
     unsigned long end, page;
+    unsigned long **d = ram_list.dirty_memory;
 
     end = TARGET_PAGE_ALIGN(start + length) >> TARGET_PAGE_BITS;
     page = start >> TARGET_PAGE_BITS;
     if (likely(mask & (1 << DIRTY_MEMORY_MIGRATION))) {
-        bitmap_set(ram_list.dirty_memory[DIRTY_MEMORY_MIGRATION], page, end - page);
+        bitmap_set_atomic(d[DIRTY_MEMORY_MIGRATION], page, end - page);
     }
     if (unlikely(mask & (1 << DIRTY_MEMORY_VGA))) {
-        bitmap_set(ram_list.dirty_memory[DIRTY_MEMORY_VGA], page, end - page);
+        bitmap_set_atomic(d[DIRTY_MEMORY_VGA], page, end - page);
     }
     if (unlikely(mask & (1 << DIRTY_MEMORY_CODE))) {
-        bitmap_set(ram_list.dirty_memory[DIRTY_MEMORY_CODE], page, end - page);
+        bitmap_set_atomic(d[DIRTY_MEMORY_CODE], page, end - page);
     }
     xen_modified_memory(start, length);
 }
@@ -159,11 +160,12 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
         for (k = 0; k < nr; k++) {
             if (bitmap[k]) {
                 unsigned long temp = leul_to_cpu(bitmap[k]);
+                unsigned long **d = ram_list.dirty_memory;
 
-                ram_list.dirty_memory[DIRTY_MEMORY_MIGRATION][page + k] |= temp;
-                ram_list.dirty_memory[DIRTY_MEMORY_VGA][page + k] |= temp;
+                atomic_or(&d[DIRTY_MEMORY_MIGRATION][page + k], temp);
+                atomic_or(&d[DIRTY_MEMORY_VGA][page + k], temp);
                 if (tcg_enabled()) {
-                    ram_list.dirty_memory[DIRTY_MEMORY_CODE][page + k] |= temp;
+                    atomic_or(&d[DIRTY_MEMORY_CODE][page + k], temp);
                 }
             }
         }
-- 
1.8.3.1

[Qemu-devel] [PATCH 26/29] migration: move dirty bitmap sync to ram_addr.h

[Paolo Bonzini]

From: Stefan Hajnoczi <stefanha@redhat.com>

The dirty memory bitmap is managed by ram_addr.h and copied to
migration_bitmap[] periodically during live migration.

Move the code to sync the bitmap to ram_addr.h where related code lives.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <1417519399-3166-5-git-send-email-stefanha@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch_init.c             | 46 ++--------------------------------------------
 include/exec/ram_addr.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 44 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 4c8fcee..ec86173 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -467,52 +467,10 @@ ram_addr_t migration_bitmap_find_and_reset_dirty(MemoryRegion *mr,
     return (next - base) << TARGET_PAGE_BITS;
 }
 
-static inline bool migration_bitmap_set_dirty(ram_addr_t addr)
-{
-    bool ret;
-    int nr = addr >> TARGET_PAGE_BITS;
-
-    ret = test_and_set_bit(nr, migration_bitmap);
-
-    if (!ret) {
-        migration_dirty_pages++;
-    }
-    return ret;
-}
-
 static void migration_bitmap_sync_range(ram_addr_t start, ram_addr_t length)
 {
-    ram_addr_t addr;
-    unsigned long page = BIT_WORD(start >> TARGET_PAGE_BITS);
-
-    /* start address is aligned at the start of a word? */
-    if (((page * BITS_PER_LONG) << TARGET_PAGE_BITS) == start) {
-        int k;
-        int nr = BITS_TO_LONGS(length >> TARGET_PAGE_BITS);
-        unsigned long *src = ram_list.dirty_memory[DIRTY_MEMORY_MIGRATION];
-
-        for (k = page; k < page + nr; k++) {
-            if (src[k]) {
-                unsigned long new_dirty;
-                new_dirty = ~migration_bitmap[k];
-                migration_bitmap[k] |= src[k];
-                new_dirty &= src[k];
-                migration_dirty_pages += ctpopl(new_dirty);
-                src[k] = 0;
-            }
-        }
-    } else {
-        for (addr = 0; addr < length; addr += TARGET_PAGE_SIZE) {
-            if (cpu_physical_memory_get_dirty(start + addr,
-                                              TARGET_PAGE_SIZE,
-                                              DIRTY_MEMORY_MIGRATION)) {
-                cpu_physical_memory_reset_dirty(start + addr,
-                                                TARGET_PAGE_SIZE,
-                                                DIRTY_MEMORY_MIGRATION);
-                migration_bitmap_set_dirty(start + addr);
-            }
-        }
-    }
+    migration_dirty_pages +=
+        cpu_physical_memory_sync_dirty_bitmap(migration_bitmap, start, length);
 }
 
 
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index bb69a28..9bd7827 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -218,5 +218,49 @@ static inline void cpu_physical_memory_clear_dirty_range(ram_addr_t start,
 void cpu_physical_memory_reset_dirty(ram_addr_t start, ram_addr_t length,
                                      unsigned client);
 
+static inline
+uint64_t cpu_physical_memory_sync_dirty_bitmap(unsigned long *dest,
+                                               ram_addr_t start,
+                                               ram_addr_t length)
+{
+    ram_addr_t addr;
+    unsigned long page = BIT_WORD(start >> TARGET_PAGE_BITS);
+    uint64_t num_dirty = 0;
+
+    /* start address is aligned at the start of a word? */
+    if (((page * BITS_PER_LONG) << TARGET_PAGE_BITS) == start) {
+        int k;
+        int nr = BITS_TO_LONGS(length >> TARGET_PAGE_BITS);
+        unsigned long *src = ram_list.dirty_memory[DIRTY_MEMORY_MIGRATION];
+
+        for (k = page; k < page + nr; k++) {
+            if (src[k]) {
+                unsigned long new_dirty;
+                new_dirty = ~dest[k];
+                dest[k] |= src[k];
+                new_dirty &= src[k];
+                num_dirty += ctpopl(new_dirty);
+                src[k] = 0;
+            }
+        }
+    } else {
+        for (addr = 0; addr < length; addr += TARGET_PAGE_SIZE) {
+            if (cpu_physical_memory_get_dirty(start + addr,
+                                              TARGET_PAGE_SIZE,
+                                              DIRTY_MEMORY_MIGRATION)) {
+                cpu_physical_memory_reset_dirty(start + addr,
+                                                TARGET_PAGE_SIZE,
+                                                DIRTY_MEMORY_MIGRATION);
+                long k = (start + addr) >> TARGET_PAGE_BITS;
+                if (!test_and_set_bit(k, dest)) {
+                    num_dirty++;
+                }
+            }
+        }
+    }
+
+    return num_dirty;
+}
+
 #endif
 #endif
-- 
1.8.3.1

[Qemu-devel] [PATCH 27/29] memory: replace cpu_physical_memory_reset_dirty() with test-and-clear

[Paolo Bonzini]

From: Stefan Hajnoczi <stefanha@redhat.com>

The cpu_physical_memory_reset_dirty() function is sometimes used
together with cpu_physical_memory_get_dirty().  This is not atomic since
two separate accesses to the dirty memory bitmap are made.

Turn cpu_physical_memory_reset_dirty() and
cpu_physical_memory_clear_dirty_range_type() into the atomic
cpu_physical_memory_test_and_clear_dirty().

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <1417519399-3166-6-git-send-email-stefanha@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cputlb.c                |  4 ++--
 exec.c                  | 23 +++++++++++++++++------
 include/exec/ram_addr.h | 33 ++++++++++-----------------------
 memory.c                | 11 ++++-------
 4 files changed, 33 insertions(+), 38 deletions(-)

diff --git a/cputlb.c b/cputlb.c
index a86e76f..75b2dcf 100644
--- a/cputlb.c
+++ b/cputlb.c
@@ -125,8 +125,8 @@ void tlb_flush_page(CPUState *cpu, target_ulong addr)
    can be detected */
 void tlb_protect_code(ram_addr_t ram_addr)
 {
-    cpu_physical_memory_reset_dirty(ram_addr, TARGET_PAGE_SIZE,
-                                    DIRTY_MEMORY_CODE);
+    cpu_physical_memory_test_and_clear_dirty(ram_addr, TARGET_PAGE_SIZE,
+                                             DIRTY_MEMORY_CODE);
 }
 
 /* update the TLB so that writes in physical page 'phys_addr' are no longer
diff --git a/exec.c b/exec.c
index b7aebbf..daed5a7 100644
--- a/exec.c
+++ b/exec.c
@@ -857,16 +857,27 @@ static void tlb_reset_dirty_range_all(ram_addr_t start, ram_addr_t length)
 }
 
 /* Note: start and end must be within the same ram block.  */
-void cpu_physical_memory_reset_dirty(ram_addr_t start, ram_addr_t length,
-                                     unsigned client)
+bool cpu_physical_memory_test_and_clear_dirty(ram_addr_t start,
+                                              ram_addr_t length,
+                                              unsigned client)
 {
-    if (length == 0)
-        return;
-    cpu_physical_memory_clear_dirty_range_type(start, length, client);
+    unsigned long end, page;
+    bool dirty;
+
+    if (length == 0) {
+        return false;
+    }
 
-    if (tcg_enabled()) {
+    end = TARGET_PAGE_ALIGN(start + length) >> TARGET_PAGE_BITS;
+    page = start >> TARGET_PAGE_BITS;
+    dirty = bitmap_test_and_clear_atomic(ram_list.dirty_memory[client],
+                                         page, end - page);
+
+    if (dirty && tcg_enabled()) {
         tlb_reset_dirty_range_all(start, length);
     }
+
+    return dirty;
 }
 
 /* Called from RCU critical section */
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index 9bd7827..ea77187 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -194,30 +194,19 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
 }
 #endif /* not _WIN32 */
 
-static inline void cpu_physical_memory_clear_dirty_range_type(ram_addr_t start,
-                                                              ram_addr_t length,
-                                                              unsigned client)
-{
-    unsigned long end, page;
-
-    assert(client < DIRTY_MEMORY_NUM);
-    end = TARGET_PAGE_ALIGN(start + length) >> TARGET_PAGE_BITS;
-    page = start >> TARGET_PAGE_BITS;
-    bitmap_clear(ram_list.dirty_memory[client], page, end - page);
-}
+bool cpu_physical_memory_test_and_clear_dirty(ram_addr_t start,
+                                              ram_addr_t length,
+                                              unsigned client);
 
 static inline void cpu_physical_memory_clear_dirty_range(ram_addr_t start,
                                                          ram_addr_t length)
 {
-    cpu_physical_memory_clear_dirty_range_type(start, length, DIRTY_MEMORY_MIGRATION);
-    cpu_physical_memory_clear_dirty_range_type(start, length, DIRTY_MEMORY_VGA);
-    cpu_physical_memory_clear_dirty_range_type(start, length, DIRTY_MEMORY_CODE);
+    cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_MIGRATION);
+    cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_VGA);
+    cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_CODE);
 }
 
 
-void cpu_physical_memory_reset_dirty(ram_addr_t start, ram_addr_t length,
-                                     unsigned client);
-
 static inline
 uint64_t cpu_physical_memory_sync_dirty_bitmap(unsigned long *dest,
                                                ram_addr_t start,
@@ -245,12 +234,10 @@ uint64_t cpu_physical_memory_sync_dirty_bitmap(unsigned long *dest,
         }
     } else {
         for (addr = 0; addr < length; addr += TARGET_PAGE_SIZE) {
-            if (cpu_physical_memory_get_dirty(start + addr,
-                                              TARGET_PAGE_SIZE,
-                                              DIRTY_MEMORY_MIGRATION)) {
-                cpu_physical_memory_reset_dirty(start + addr,
-                                                TARGET_PAGE_SIZE,
-                                                DIRTY_MEMORY_MIGRATION);
+            if (cpu_physical_memory_test_and_clear_dirty(
+                        start + addr,
+                        TARGET_PAGE_SIZE,
+                        DIRTY_MEMORY_MIGRATION)) {
                 long k = (start + addr) >> TARGET_PAGE_BITS;
                 if (!test_and_set_bit(k, dest)) {
                     num_dirty++;
diff --git a/memory.c b/memory.c
index 7422790..bb86b4b 100644
--- a/memory.c
+++ b/memory.c
@@ -1397,13 +1397,9 @@ void memory_region_set_dirty(MemoryRegion *mr, hwaddr addr,
 bool memory_region_test_and_clear_dirty(MemoryRegion *mr, hwaddr addr,
                                         hwaddr size, unsigned client)
 {
-    bool ret;
     assert(mr->terminates);
-    ret = cpu_physical_memory_get_dirty(mr->ram_addr + addr, size, client);
-    if (ret) {
-        cpu_physical_memory_reset_dirty(mr->ram_addr + addr, size, client);
-    }
-    return ret;
+    return cpu_physical_memory_test_and_clear_dirty(mr->ram_addr + addr,
+                                                    size, client);
 }
 
 
@@ -1447,7 +1443,8 @@ void memory_region_reset_dirty(MemoryRegion *mr, hwaddr addr,
                                hwaddr size, unsigned client)
 {
     assert(mr->terminates);
-    cpu_physical_memory_reset_dirty(mr->ram_addr + addr, size, client);
+    cpu_physical_memory_test_and_clear_dirty(mr->ram_addr + addr, size,
+                                             client);
 }
 
 int memory_region_get_fd(MemoryRegion *mr)
-- 
1.8.3.1

[Qemu-devel] [PATCH 28/29] memory: make cpu_physical_memory_sync_dirty_bitmap() fully atomic

[Paolo Bonzini]

From: Stefan Hajnoczi <stefanha@redhat.com>

The fast path of cpu_physical_memory_sync_dirty_bitmap() directly
manipulates the dirty bitmap.  Use atomic_xchg() to make the
test-and-clear atomic.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <1417519399-3166-7-git-send-email-stefanha@redhat.com>
[Only do xchg on nonzero words. - Paolo]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/exec/ram_addr.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index ea77187..0baba41 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -224,12 +224,12 @@ uint64_t cpu_physical_memory_sync_dirty_bitmap(unsigned long *dest,
 
         for (k = page; k < page + nr; k++) {
             if (src[k]) {
+                unsigned long bits = atomic_xchg(&src[k], 0);
                 unsigned long new_dirty;
                 new_dirty = ~dest[k];
-                dest[k] |= src[k];
-                new_dirty &= src[k];
+                dest[k] |= bits;
+                new_dirty &= bits;
                 num_dirty += ctpopl(new_dirty);
-                src[k] = 0;
             }
         }
     } else {
-- 
1.8.3.1

[Qemu-devel] [PATCH 29/29] memory: strengthen assertions on mr->terminates

[Paolo Bonzini]

mr->terminates alone doesn't guarantee that we are looking at a RAM region.
mr->ram_addr also has to be checked, in order to distinguish RAM and I/O
regions.

IOMMU regions were not setting mr->ram_addr to a bogus value, do it now
so that the assertions would fire for IOMMU regions as well.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 memory.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/memory.c b/memory.c
index bb86b4b..82d9df6 100644
--- a/memory.c
+++ b/memory.c
@@ -1242,6 +1242,7 @@ void memory_region_init_iommu(MemoryRegion *mr,
     memory_region_init(mr, owner, name, size);
     mr->iommu_ops = ops,
     mr->terminates = true;  /* then re-forwards */
+    mr->ram_addr = ~(ram_addr_t)0;
     notifier_list_init(&mr->iommu_notify);
 }
 
@@ -1382,14 +1383,14 @@ void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
 bool memory_region_get_dirty(MemoryRegion *mr, hwaddr addr,
                              hwaddr size, unsigned client)
 {
-    assert(mr->terminates);
+    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
     return cpu_physical_memory_get_dirty(mr->ram_addr + addr, size, client);
 }
 
 void memory_region_set_dirty(MemoryRegion *mr, hwaddr addr,
                              hwaddr size)
 {
-    assert(mr->terminates);
+    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
     cpu_physical_memory_set_dirty_range(mr->ram_addr + addr, size,
                                         memory_region_get_dirty_log_mask(mr));
 }
@@ -1397,7 +1398,7 @@ void memory_region_set_dirty(MemoryRegion *mr, hwaddr addr,
 bool memory_region_test_and_clear_dirty(MemoryRegion *mr, hwaddr addr,
                                         hwaddr size, unsigned client)
 {
-    assert(mr->terminates);
+    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
     return cpu_physical_memory_test_and_clear_dirty(mr->ram_addr + addr,
                                                     size, client);
 }
@@ -1442,7 +1443,7 @@ void memory_region_rom_device_set_romd(MemoryRegion *mr, bool romd_mode)
 void memory_region_reset_dirty(MemoryRegion *mr, hwaddr addr,
                                hwaddr size, unsigned client)
 {
-    assert(mr->terminates);
+    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
     cpu_physical_memory_test_and_clear_dirty(mr->ram_addr + addr, size,
                                              client);
 }
@@ -1453,7 +1454,7 @@ int memory_region_get_fd(MemoryRegion *mr)
         return memory_region_get_fd(mr->alias);
     }
 
-    assert(mr->terminates);
+    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
 
     return qemu_get_ram_fd(mr->ram_addr & TARGET_PAGE_MASK);
 }
@@ -1464,14 +1465,14 @@ void *memory_region_get_ram_ptr(MemoryRegion *mr)
         return memory_region_get_ram_ptr(mr->alias) + mr->alias_offset;
     }
 
-    assert(mr->terminates);
+    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
 
     return qemu_get_ram_ptr(mr->ram_addr & TARGET_PAGE_MASK);
 }
 
 void memory_region_ram_resize(MemoryRegion *mr, ram_addr_t newsize, Error **errp)
 {
-    assert(mr->terminates);
+    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
 
     qemu_ram_resize(mr->ram_addr, newsize, errp);
 }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 00/29] Dirty bitmap atomic access and optimizations

[Paolo Bonzini]

On 27/04/2015 18:28, Paolo Bonzini wrote:
> QEMU is currently accessing the dirty bitmaps very liberally,
> which is understandable since the accesses are cheap.  This is
> however not good for squeezing maximum performance out of dataplane,
> and is also not good if the accesses become more expensive---as is
> the case when they use atomic primitives.
> 
> This patch series does the following optimizations and cleanups:
> 
> 1) it lets KVM code treat migration as "just another dirty bitmap
> client" instead of needing the special global_log_start/stop callbacks.
> These remain in use in Xen and vhost.  This removes code and avoids
> bugs such as the one fixed in commit 4cc856f (kvm-all: Sync dirty-bitmap
> from kvm before kvm destroy the corresponding dirty_bitmap, 2015-04-02).
> 
> 2) it avoids modifications to unused dirty bitmaps: code if TCG
> is disabled, migration if no migration is in progress, VGA for
> regions other than VRAM.
> 
> and on top of this makes dirty bitmap access atomic.  I'm not including
> the patch to make the migration thread synchronize the bitmap outside
> the big QEMU lock (thus removing the last source of jitter during the
> RAM copy phase of migration) but it is also enabled by these patches.
> 
> Patches 1-4 are cleanups to DIRTY_MEMORY_VGA users.
> 
> Patches 5-12 are the first cleanup (KVM treats migration as just
> another client).  Patches 13-14 are a simple optimization that is enabled
> by these patches.
> 
> Patches 15-18 are bonus cleanups to translate-all.c's dirty memory
> tracking for TCG.
> 
> Patches 19-22 are the second cleanup (avoid modifications to unused
> dirty bitmaps).
> 
> Patches 23-28 are Stefan's patches for atomic access to the dirty
> bitmap, which has no performance impact in the common case thanks to
> the previous work.
> 
> Patch 29 is an unrelated strengthening of assertions, that mst spotted
> while reviewing v1.
> 
> v1->v2: completed work on removing global_start/global_stop from KVM
> 	listener
> 
>         extra spelunking of TCG history so that the exec.c code
> 	makes more sense
> 
>         extra splitting of patches (Stefan)
> 
>         keep memory_region_is_logging and memory_region_get_dirty_log_mask
> 	APIs separate (mst)
> 
> Paolo Bonzini (23):
>   memory: the only dirty memory flag for users is DIRTY_MEMORY_VGA
>   g364fb: remove pointless call to memory_region_set_coalescing
>   display: enable DIRTY_MEMORY_VGA tracking explicitly
>   display: add memory_region_sync_dirty_bitmap calls
>   memory: differentiate memory_region_is_logging and
>     memory_region_get_dirty_log_mask
>   memory: prepare for multiple bits in the dirty log mask
>   framebuffer: check memory_region_is_logging
>   ui/console: remove dpy_gfx_update_dirty
>   memory: track DIRTY_MEMORY_CODE in mr->dirty_log_mask
>   kvm: accept non-mapped memory in kvm_dirty_pages_log_change
>   memory: include DIRTY_MEMORY_MIGRATION in the dirty log mask
>   kvm: remove special handling of DIRTY_MEMORY_MIGRATION in the dirty
>     log mask
>   ram_addr: tweaks to xen_modified_memory
>   exec: use memory_region_get_dirty_log_mask to optimize dirty tracking
>   exec: move functions to translate-all.h
>   translate-all: remove unnecessary argument to tb_invalidate_phys_range
>   cputlb: remove useless arguments to tlb_unprotect_code_phys, rename
>   translate-all: make less of tb_invalidate_phys_page_range depend on
>     is_cpu_write_access
>   exec: pass client mask to cpu_physical_memory_set_dirty_range
>   exec: invert return value of cpu_physical_memory_get_clean, rename
>   exec: only check relevant bitmaps for cleanliness
>   memory: do not touch code dirty bitmap unless TCG is enabled
>   memory: strengthen assertions on mr->terminates
> 
> Stefan Hajnoczi (6):
>   bitmap: add atomic set functions
>   bitmap: add atomic test and clear
>   memory: use atomic ops for setting dirty memory bits
>   migration: move dirty bitmap sync to ram_addr.h
>   memory: replace cpu_physical_memory_reset_dirty() with test-and-clear
>   memory: make cpu_physical_memory_sync_dirty_bitmap() fully atomic
> 
>  arch_init.c                  |  46 +--------------
>  cputlb.c                     |   7 +--
>  exec.c                       |  99 +++++++++++++++----------------
>  hw/display/cg3.c             |   2 +
>  hw/display/exynos4210_fimd.c |  20 ++++---
>  hw/display/framebuffer.c     |   4 ++
>  hw/display/g364fb.c          |   3 +-
>  hw/display/sm501.c           |   2 +
>  hw/display/tcx.c             |   2 +
>  hw/display/vmware_vga.c      |   2 +-
>  hw/virtio/dataplane/vring.c  |   2 +-
>  hw/virtio/vhost.c            |   9 ++-
>  include/exec/cputlb.h        |   3 +-
>  include/exec/exec-all.h      |   6 +-
>  include/exec/memory.h        |  25 ++++++--
>  include/exec/ram_addr.h      | 138 ++++++++++++++++++++++++++++---------------
>  include/qemu/bitmap.h        |   4 ++
>  include/qemu/bitops.h        |  14 +++++
>  include/ui/console.h         |   4 --
>  kvm-all.c                    |  77 ++++++------------------
>  linux-user/mmap.c            |   7 ++-
>  memory.c                     |  76 ++++++++++++++++--------
>  translate-all.c              |  20 +++----
>  translate-all.h              |   7 +++
>  ui/console.c                 |  61 -------------------
>  user-exec.c                  |   1 +
>  util/bitmap.c                |  81 +++++++++++++++++++++++++
>  xen-hvm.c                    |  22 ++++---
>  28 files changed, 401 insertions(+), 343 deletions(-)
> 

Ping?

Paolo

Re: [Qemu-devel] [PATCH 01/29] memory: the only dirty memory flag for users is DIRTY_MEMORY_VGA

[Fam Zheng]

On Mon, 04/27 18:28, Paolo Bonzini wrote:
> DIRTY_MEMORY_MIGRATION is triggered by memory_global_dirty_log_start
> and memory_global_dirty_log_stop, so it cannot be used with
> memory_region_set_log.
> 
> Specify this in the documentation and assert it.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

> ---
>  include/exec/memory.h | 3 +--
>  memory.c              | 1 +
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index a2ea587..081f7d6 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -625,8 +625,7 @@ void memory_region_ram_resize(MemoryRegion *mr, ram_addr_t newsize,
>   *
>   * @mr: the memory region being updated.
>   * @log: whether dirty logging is to be enabled or disabled.
> - * @client: the user of the logging information; %DIRTY_MEMORY_MIGRATION or
> - *          %DIRTY_MEMORY_VGA.
> + * @client: the user of the logging information; %DIRTY_MEMORY_VGA only.
>   */
>  void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client);
>  
> diff --git a/memory.c b/memory.c
> index a11e9bf..e688f5e 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1354,6 +1354,7 @@ void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
>  {
>      uint8_t mask = 1 << client;
>  
> +    assert(client == DIRTY_MEMORY_VGA);
>      memory_region_transaction_begin();
>      mr->dirty_log_mask = (mr->dirty_log_mask & ~mask) | (log * mask);
>      memory_region_update_pending |= mr->enabled;
> -- 
> 1.8.3.1
> 
> 

Re: [Qemu-devel] [PATCH 03/29] display: enable DIRTY_MEMORY_VGA tracking explicitly

[Fam Zheng]

On Mon, 04/27 18:28, Paolo Bonzini wrote:
> This will be required soon by the memory core.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/display/cg3.c             |  1 +
>  hw/display/exynos4210_fimd.c | 20 +++++++++++++-------
>  hw/display/g364fb.c          |  1 +
>  hw/display/sm501.c           |  1 +
>  hw/display/tcx.c             |  1 +
>  5 files changed, 17 insertions(+), 7 deletions(-)

"git grep memory_region_init_ram hw/display/" also mentions qxl.vram, vga.vram,
tc6393xb.vram and vga.vram. Why don't these need it?

Fam

> 
> diff --git a/hw/display/cg3.c b/hw/display/cg3.c
> index 1e6ff2b..cbcf518 100644
> --- a/hw/display/cg3.c
> +++ b/hw/display/cg3.c
> @@ -309,6 +309,7 @@ static void cg3_realizefn(DeviceState *dev, Error **errp)
>  
>      memory_region_init_ram(&s->vram_mem, NULL, "cg3.vram", s->vram_size,
>                             &error_abort);
> +    memory_region_set_log(&s->vram_mem, true, DIRTY_MEMORY_VGA);
>      vmstate_register_ram_global(&s->vram_mem);
>      sysbus_init_mmio(sbd, &s->vram_mem);
>  
> diff --git a/hw/display/exynos4210_fimd.c b/hw/display/exynos4210_fimd.c
> index 45c62af..72b3a1d 100644
> --- a/hw/display/exynos4210_fimd.c
> +++ b/hw/display/exynos4210_fimd.c
> @@ -1109,6 +1109,12 @@ static inline int fimd_get_buffer_id(Exynos4210fimdWindow *w)
>      }
>  }
>  
> +static void exynos4210_fimd_invalidate(void *opaque)
> +{
> +    Exynos4210fimdState *s = (Exynos4210fimdState *)opaque;
> +    s->invalidate = true;
> +}
> +
>  /* Updates specified window's MemorySection based on values of WINCON,
>   * VIDOSDA, VIDOSDB, VIDWADDx and SHADOWCON registers */
>  static void fimd_update_memory_section(Exynos4210fimdState *s, unsigned win)
> @@ -1136,7 +1142,11 @@ static void fimd_update_memory_section(Exynos4210fimdState *s, unsigned win)
>      /* TODO: add .exit and unref the region there.  Not needed yet since sysbus
>       * does not support hot-unplug.
>       */
> -    memory_region_unref(w->mem_section.mr);
> +    if (w->mem_section.mr) {
> +        memory_region_set_log(w->mem_section.mr, false, DIRTY_MEMORY_VGA);
> +        memory_region_unref(w->mem_section.mr);
> +    }
> +
>      w->mem_section = memory_region_find(sysbus_address_space(sbd),
>                                          fb_start_addr, w->fb_len);
>      assert(w->mem_section.mr);
> @@ -1162,6 +1172,8 @@ static void fimd_update_memory_section(Exynos4210fimdState *s, unsigned win)
>          cpu_physical_memory_unmap(w->host_fb_addr, fb_mapped_len, 0, 0);
>          goto error_return;
>      }
> +    memory_region_set_log(w->mem_section.mr, true, DIRTY_MEMORY_VGA);
> +    exynos4210_fimd_invalidate(s);
>      return;
>  
>  error_return:
> @@ -1224,12 +1236,6 @@ static void exynos4210_fimd_update_irq(Exynos4210fimdState *s)
>      }
>  }
>  
> -static void exynos4210_fimd_invalidate(void *opaque)
> -{
> -    Exynos4210fimdState *s = (Exynos4210fimdState *)opaque;
> -    s->invalidate = true;
> -}
> -
>  static void exynos4210_update_resolution(Exynos4210fimdState *s)
>  {
>      DisplaySurface *surface = qemu_console_surface(s->console);
> diff --git a/hw/display/g364fb.c b/hw/display/g364fb.c
> index 6543f2f..be62dd6 100644
> --- a/hw/display/g364fb.c
> +++ b/hw/display/g364fb.c
> @@ -489,6 +489,7 @@ static void g364fb_init(DeviceState *dev, G364State *s)
>      memory_region_init_ram_ptr(&s->mem_vram, NULL, "vram",
>                                 s->vram_size, s->vram);
>      vmstate_register_ram(&s->mem_vram, dev);
> +    memory_region_set_log(&s->mem_vram, true, DIRTY_MEMORY_VGA);
>  }
>  
>  #define TYPE_G364 "sysbus-g364"
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index c72154b..43f8538 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -1412,6 +1412,7 @@ void sm501_init(MemoryRegion *address_space_mem, uint32_t base,
>      memory_region_init_ram(&s->local_mem_region, NULL, "sm501.local",
>                             local_mem_bytes, &error_abort);
>      vmstate_register_ram_global(&s->local_mem_region);
> +    memory_region_set_log(&s->local_mem_region, true, DIRTY_MEMORY_VGA);
>      s->local_mem = memory_region_get_ram_ptr(&s->local_mem_region);
>      memory_region_add_subregion(address_space_mem, base, &s->local_mem_region);
>  
> diff --git a/hw/display/tcx.c b/hw/display/tcx.c
> index a9f9f66..58faa96 100644
> --- a/hw/display/tcx.c
> +++ b/hw/display/tcx.c
> @@ -1006,6 +1006,7 @@ static void tcx_realizefn(DeviceState *dev, Error **errp)
>      memory_region_init_ram(&s->vram_mem, OBJECT(s), "tcx.vram",
>                             s->vram_size * (1 + 4 + 4), &error_abort);
>      vmstate_register_ram_global(&s->vram_mem);
> +    memory_region_set_log(&s->vram_mem, true, DIRTY_MEMORY_VGA);
>      vram_base = memory_region_get_ram_ptr(&s->vram_mem);
>  
>      /* 10/ROM : FCode ROM */
> -- 
> 1.8.3.1
> 
> 

Re: [Qemu-devel] [PATCH 04/29] display: add memory_region_sync_dirty_bitmap calls

[Fam Zheng]

On Mon, 04/27 18:28, Paolo Bonzini wrote:
> diff --git a/hw/display/tcx.c b/hw/display/tcx.c
> index 58faa96..2eb5aef 100644
> --- a/hw/display/tcx.c
> +++ b/hw/display/tcx.c
> @@ -353,6 +353,7 @@ static void tcx_update_display(void *opaque)
>          return;
>      }
>  
> +    memory_region_sync_dirty_bitmap(&ts->vram_mem);
>      for (y = 0; y < ts->height; page += TARGET_PAGE_SIZE) {
>          if (memory_region_get_dirty(&ts->vram_mem, page, TARGET_PAGE_SIZE,
>                                      DIRTY_MEMORY_VGA)) {

Why not change tcx24_update_display as well?

Fam

Re: [Qemu-devel] [PATCH 05/29] memory: differentiate memory_region_is_logging and memory_region_get_dirty_log_mask

[Fam Zheng]

On Mon, 04/27 18:28, Paolo Bonzini wrote:
> For now memory regions only track DIRTY_MEMORY_VGA individually, but
> this will change soon.  To support this, split memory_region_is_logging
> in two functions: one that returns a given bit from dirty_log_mask,
> and one that returns the entire mask.  memory_region_is_logging gets an
> extra parameter so that the compiler flags misuse.
> 
> While VGA-specific users (including the Xen listener!) will want to keep
> checking that bit, KVM and vhost check for "any bit except migration"
> (because migration is handled via the global start/stop listener
> callbacks).
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/display/vmware_vga.c     |  2 +-
>  hw/virtio/dataplane/vring.c |  2 +-
>  hw/virtio/vhost.c           |  3 ++-
>  include/exec/memory.h       | 17 +++++++++++++++--
>  kvm-all.c                   |  3 ++-
>  memory.c                    |  7 ++++++-
>  xen-hvm.c                   |  2 +-
>  7 files changed, 28 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
> index c17ddd1..7f397d3 100644
> --- a/hw/display/vmware_vga.c
> +++ b/hw/display/vmware_vga.c
> @@ -1124,7 +1124,7 @@ static void vmsvga_update_display(void *opaque)
>       * 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)) {
> +    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),
> diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
> index 5c7b8c2..e378733 100644
> --- a/hw/virtio/dataplane/vring.c
> +++ b/hw/virtio/dataplane/vring.c
> @@ -42,7 +42,7 @@ static void *vring_map(MemoryRegion **mr, hwaddr phys, hwaddr len,
>      }
>  
>      /* Ignore regions with dirty logging, we cannot mark them dirty */
> -    if (memory_region_is_logging(section.mr)) {
> +    if (memory_region_get_dirty_log_mask(section.mr)) {
>          goto out;
>      }
>  
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 54851b7..5f698a1 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -377,7 +377,8 @@ static void vhost_set_memory(MemoryListener *listener,
>                                           memory_listener);
>      hwaddr start_addr = section->offset_within_address_space;
>      ram_addr_t size = int128_get64(section->size);
> -    bool log_dirty = memory_region_is_logging(section->mr);
> +    bool log_dirty =
> +        memory_region_get_dirty_log_mask(section->mr) & ~(1 << DIRTY_MEMORY_MIGRATION);

Too long a line.

>      int s = offsetof(struct vhost_memory, regions) +
>          (dev->mem->nregions + 1) * sizeof dev->mem->regions[0];
>      void *ram;
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 081f7d6..229733e 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -569,11 +569,24 @@ const char *memory_region_name(const MemoryRegion *mr);
>  /**
>   * memory_region_is_logging: return whether a memory region is logging writes
>   *
> - * Returns %true if the memory region is logging writes
> + * Returns %true if the memory region is logging writes for the given client
> + * a bitmap of clients for which the memory region is logging writes.

I can't parse this sentence, rephrase a bit?

>   *
>   * @mr: the memory region being queried
> + * @client: the client being queried
>   */
> -bool memory_region_is_logging(MemoryRegion *mr);
> +bool memory_region_is_logging(MemoryRegion *mr, uint8_t client);
> +
> +/**
> + * memory_region_get_dirty_log_mask: return the clients for which a

s/which a/ which the/ ?

> + * memory region is logging writes.
> + *
> + * Returns a bitmap of clients for which the memory region is logging writes.

Why repeat?

All questions are trivial enough that once you fix them or if they don't need
fixing:

Reviewed-by: Fam Zheng <famz@redhat.com>

> + * Right now this will be either 0 or (1 << DIRTY_MEMORY_VGA).
> + *
> + * @mr: the memory region being queried
> + */
> +uint8_t memory_region_get_dirty_log_mask(MemoryRegion *mr);
>  
>  /**
>   * memory_region_is_rom: check whether a memory region is ROM
> diff --git a/kvm-all.c b/kvm-all.c
> index dd44f8c..bd1f910 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -663,7 +663,8 @@ static void kvm_set_phys_mem(MemoryRegionSection *section, bool add)
>      KVMSlot *mem, old;
>      int err;
>      MemoryRegion *mr = section->mr;
> -    bool log_dirty = memory_region_is_logging(mr);
> +    bool log_dirty =
> +        memory_region_get_dirty_log_mask(mr) & ~(1 << DIRTY_MEMORY_MIGRATION);
>      bool writeable = !mr->readonly && !mr->rom_device;
>      bool readonly_flag = mr->readonly || memory_region_is_romd(mr);
>      hwaddr start_addr = section->offset_within_address_space;
> diff --git a/memory.c b/memory.c
> index e688f5e..a982d99 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1318,11 +1318,16 @@ bool memory_region_is_skip_dump(MemoryRegion *mr)
>      return mr->skip_dump;
>  }
>  
> -bool memory_region_is_logging(MemoryRegion *mr)
> +uint8_t memory_region_get_dirty_log_mask(MemoryRegion *mr)
>  {
>      return mr->dirty_log_mask;
>  }
>  
> +bool memory_region_is_logging(MemoryRegion *mr, uint8_t client)
> +{
> +    return memory_region_get_dirty_log_mask(mr) & (1 << client);
> +}
> +
>  bool memory_region_is_rom(MemoryRegion *mr)
>  {
>      return mr->ram && mr->readonly;
> diff --git a/xen-hvm.c b/xen-hvm.c
> index 315864c..338ab29 100644
> --- a/xen-hvm.c
> +++ b/xen-hvm.c
> @@ -488,7 +488,7 @@ static void xen_set_memory(struct MemoryListener *listener,
>      XenIOState *state = container_of(listener, XenIOState, memory_listener);
>      hwaddr start_addr = section->offset_within_address_space;
>      ram_addr_t size = int128_get64(section->size);
> -    bool log_dirty = memory_region_is_logging(section->mr);
> +    bool log_dirty = memory_region_is_logging(section->mr, DIRTY_MEMORY_VGA);
>      hvmmem_type_t mem_type;
>  
>      if (section->mr == &ram_memory) {
> -- 
> 1.8.3.1
> 
> 

Re: [Qemu-devel] [PATCH 06/29] memory: prepare for multiple bits in the dirty log mask

[Fam Zheng]

On Mon, 04/27 18:28, Paolo Bonzini wrote:
> When the dirty log mask will also cover other bits than DIRTY_MEMORY_VGA,
> some listeners may be interested in the overall zero/non-zero value of
> the dirty log mask; others may be interested in the value of single bits.
> 
> For this reason, always call log_start/log_stop if bits have respectively
> appeared or disappeared, and pass the old and new values of the dirty log
> mask so that listeners can distinguish the kinds of change.
> 
> For example, KVM checks if dirty logging used to be completely disabled
> (in log_start) or is now completely disabled (in log_stop).  On the
> other hand, Xen has to check manually if DIRTY_MEMORY_VGA changed,
> since that is the only bit it cares about.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH 07/29] framebuffer: check memory_region_is_logging

[Fam Zheng]

On Mon, 04/27 18:28, Paolo Bonzini wrote:
> framebuffer.c expects DIRTY_MEMORY_VGA logging to be always on, but that
> will not be the case soon.  Because framebuffer.c computes the memory
> region on the fly for every update (with memory_region_find), it cannot
> enable/disable logging by itself.
> 
> Instead, always treat updates as invalidations if dirty logging is
> not enabled, assuming that the board will enable logging on the
> RAM region that includes the framebuffer.
> 
> To simplify the code, replace memory_region_get_dirty with
> memory_region_test_and_clear_dirty.  Then memory_region_reset_dirty
> is only needed in the invalidate case.

The commit message seems out-of-date, but I think it's better to keep the v1
hunks which skip memory_region_get_dirty if invalidate == true, otherwise use
memory_region_test_and_clear_dirty.

Fam

Re: [Qemu-devel] [PATCH 08/29] ui/console: remove dpy_gfx_update_dirty

[Fam Zheng]

On Mon, 04/27 18:28, Paolo Bonzini wrote:
> dpy_gfx_update_dirty expects DIRTY_MEMORY_VGA logging to be always on,
> but that will not be the case soon.  Because it computes the memory
> region on the fly for every update (with memory_region_find), it cannot
> enable/disable logging by itself.
> 
> We could always treat updates as invalidations if dirty logging is
> not enabled, assuming that the board will enable logging on the
> RAM region that includes the framebuffer.
> 
> However, the function is unused, so just drop it.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH 09/29] memory: track DIRTY_MEMORY_CODE in mr->dirty_log_mask

[Fam Zheng]

On Mon, 04/27 18:28, Paolo Bonzini wrote:
> DIRTY_MEMORY_CODE is only needed for TCG.  By adding it directly to
> mr->dirty_log_mask, we avoid testing for TCG everywhere a region is
> checked for the enabled/disabled state of dirty logging.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH 10/29] kvm: accept non-mapped memory in kvm_dirty_pages_log_change

[Fam Zheng]

On Mon, 04/27 18:28, Paolo Bonzini wrote:
> It is okay if memory is not mapped into the guest but has dirty logging
> enabled.  When this happens, KVM will not do anything and only accesses
> from the host will be logged.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Could you explain where this change is needed?

Fam

> ---
>  kvm-all.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index 989fdd9..616bf04 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -335,12 +335,10 @@ static int kvm_dirty_pages_log_change(hwaddr phys_addr,
>      KVMSlot *mem = kvm_lookup_matching_slot(s, phys_addr, phys_addr + size);
>  
>      if (mem == NULL)  {
> -        fprintf(stderr, "BUG: %s: invalid parameters " TARGET_FMT_plx "-"
> -                TARGET_FMT_plx "\n", __func__, phys_addr,
> -                (hwaddr)(phys_addr + size - 1));
> -        return -EINVAL;
> +        return 0;
> +    } else {
> +        return kvm_slot_dirty_pages_log_change(mem, log_dirty);
>      }
> -    return kvm_slot_dirty_pages_log_change(mem, log_dirty);
>  }
>  
>  static void kvm_log_start(MemoryListener *listener,
> -- 
> 1.8.3.1
> 
> 
> 

Re: [Qemu-devel] [PATCH 03/29] display: enable DIRTY_MEMORY_VGA tracking explicitly

[Paolo Bonzini]

On 26/05/2015 09:13, Fam Zheng wrote:
>> >  hw/display/cg3.c             |  1 +
>> >  hw/display/exynos4210_fimd.c | 20 +++++++++++++-------
>> >  hw/display/g364fb.c          |  1 +
>> >  hw/display/sm501.c           |  1 +
>> >  hw/display/tcx.c             |  1 +
>> >  5 files changed, 17 insertions(+), 7 deletions(-)
> "git grep memory_region_init_ram hw/display/" also mentions qxl.vram, vga.vram,
> tc6393xb.vram and vga.vram. Why don't these need it?

tc6393xb.c doesn't use dirty memory, and vga sets it in hw/display/vga.c.

Paolo

Re: [Qemu-devel] [PATCH 04/29] display: add memory_region_sync_dirty_bitmap calls

[Paolo Bonzini]

On 26/05/2015 09:36, Fam Zheng wrote:
> On Mon, 04/27 18:28, Paolo Bonzini wrote:
>> diff --git a/hw/display/tcx.c b/hw/display/tcx.c
>> index 58faa96..2eb5aef 100644
>> --- a/hw/display/tcx.c
>> +++ b/hw/display/tcx.c
>> @@ -353,6 +353,7 @@ static void tcx_update_display(void *opaque)
>>          return;
>>      }
>>  
>> +    memory_region_sync_dirty_bitmap(&ts->vram_mem);
>>      for (y = 0; y < ts->height; page += TARGET_PAGE_SIZE) {
>>          if (memory_region_get_dirty(&ts->vram_mem, page, TARGET_PAGE_SIZE,
>>                                      DIRTY_MEMORY_VGA)) {
> 
> Why not change tcx24_update_display as well?

You're right.

Paolo

Re: [Qemu-devel] [PATCH 10/29] kvm: accept non-mapped memory in kvm_dirty_pages_log_change

[Paolo Bonzini]

On 26/05/2015 10:10, Fam Zheng wrote:
> > It is okay if memory is not mapped into the guest but has dirty logging
> > enabled.  When this happens, KVM will not do anything and only accesses
> > from the host will be logged.
> > 
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Could you explain where this change is needed?

I received a report of this error, obtained via iofuzz.

Paolo

> Fam
> 
>> > ---
>> >  kvm-all.c | 8 +++-----
>> >  1 file changed, 3 insertions(+), 5 deletions(-)
>> > 
>> > diff --git a/kvm-all.c b/kvm-all.c
>> > index 989fdd9..616bf04 100644
>> > --- a/kvm-all.c
>> > +++ b/kvm-all.c
>> > @@ -335,12 +335,10 @@ static int kvm_dirty_pages_log_change(hwaddr phys_addr,
>> >      KVMSlot *mem = kvm_lookup_matching_slot(s, phys_addr, phys_addr + size);
>> >  
>> >      if (mem == NULL)  {
>> > -        fprintf(stderr, "BUG: %s: invalid parameters " TARGET_FMT_plx "-"
>> > -                TARGET_FMT_plx "\n", __func__, phys_addr,
>> > -                (hwaddr)(phys_addr + size - 1));
>> > -        return -EINVAL;
>> > +        return 0;
>> > +    } else {
>> > +        return kvm_slot_dirty_pages_log_change(mem, log_dirty);
>> >      }
>> > -    return kvm_slot_dirty_pages_log_change(mem, log_dirty);
>> >  }

Re: [Qemu-devel] [PATCH 10/29] kvm: accept non-mapped memory in kvm_dirty_pages_log_change

[Fam Zheng]

On Tue, 05/26 10:16, Paolo Bonzini wrote:
> 
> 
> On 26/05/2015 10:10, Fam Zheng wrote:
> > > It is okay if memory is not mapped into the guest but has dirty logging
> > > enabled.  When this happens, KVM will not do anything and only accesses
> > > from the host will be logged.
> > > 
> > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > 
> > Could you explain where this change is needed?
> 
> I received a report of this error, obtained via iofuzz.

Good to know. (maybe squash into the commit message?)

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH 11/29] memory: include DIRTY_MEMORY_MIGRATION in the dirty log mask

[Fam Zheng]

On Mon, 04/27 18:28, Paolo Bonzini wrote:
> The separate handling of DIRTY_MEMORY_MIGRATION, which does not
> call log_start/log_stop callbacks when it changes in a region's
> dirty logging mask, has caused several bugs.
> 
> One recent example is commit 4cc856f (kvm-all: Sync dirty-bitmap from
> kvm before kvm destroy the corresponding dirty_bitmap, 2015-04-02).
> Another performance problem is that KVM keeps tracking dirty pages
> after a failed live migration, which causes bad performance due to
> disallowing huge page mapping.
> 
> This patch removes the root cause of the problem by reporting
> DIRTY_MEMORY_MIGRATION changes via log_start and log_stop.
> Note that we now have to rebuild the FlatView when global dirty
> logging is enabled or disabled; this ensures that log_start and
> log_stop callbacks are invoked.
> 
> This will also be used to make the setting of bitmaps conditional.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  memory.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/memory.c b/memory.c
> index 1966347..174cd15 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -537,7 +537,7 @@ static void render_memory_region(FlatView *view,
>      remain = clip.size;
>  
>      fr.mr = mr;
> -    fr.dirty_log_mask = mr->dirty_log_mask;
> +    fr.dirty_log_mask = memory_region_get_dirty_log_mask(mr);
>      fr.romd_mode = mr->romd_mode;
>      fr.readonly = readonly;
>  
> @@ -1329,7 +1329,11 @@ bool memory_region_is_skip_dump(MemoryRegion *mr)
>  
>  uint8_t memory_region_get_dirty_log_mask(MemoryRegion *mr)
>  {
> -    return mr->dirty_log_mask;
> +    uint8_t mask = mr->dirty_log_mask;
> +    if (global_dirty_log) {
> +        mask |= (1 << DIRTY_MEMORY_MIGRATION);

This is ugly, but I don't know how to do differently. :(

> +    }
> +    return mask;
>  }
>  
>  bool memory_region_is_logging(MemoryRegion *mr, uint8_t client)
> @@ -1892,10 +1896,20 @@ void memory_global_dirty_log_start(void)
>  {
>      global_dirty_log = true;
>      MEMORY_LISTENER_CALL_GLOBAL(log_global_start, Forward);
> +
> +    /* Refresh DIRTY_LOG_MIGRATION bit.  */
> +    memory_region_transaction_begin();
> +    memory_region_update_pending = true;
> +    memory_region_transaction_commit();
>  }
>  
>  void memory_global_dirty_log_stop(void)
>  {
> +    /* Refresh DIRTY_LOG_MIGRATION bit.  */
> +    memory_region_transaction_begin();
> +    memory_region_update_pending = true;
> +    memory_region_transaction_commit();
> +
>      global_dirty_log = false;

Shouldn't this go before memory_region_transaction_begin?

Fam

Re: [Qemu-devel] [PATCH 12/29] kvm: remove special handling of DIRTY_MEMORY_MIGRATION in the dirty log mask

[Fam Zheng]

On Mon, 04/27 18:28, Paolo Bonzini wrote:
> One recent example is commit 4cc856f (kvm-all: Sync dirty-bitmap from
> kvm before kvm destroy the corresponding dirty_bitmap, 2015-04-02).
> Another performance problem is that KVM keeps tracking dirty pages
> after a failed live migration, which causes bad performance due to
> disallowing huge page mapping.
> 
> Thanks to the previous patch, KVM can now stop hooking into
> log_global_start/stop.  This simplifies the KVM code noticeably.
> 
> Reported-by: Wanpeng Li <wanpeng.li@linux.intel.com>
> Reported-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH 11/29] memory: include DIRTY_MEMORY_MIGRATION in the dirty log mask

[Paolo Bonzini]

On 26/05/2015 10:40, Fam Zheng wrote:
> > @@ -1329,7 +1329,11 @@ bool memory_region_is_skip_dump(MemoryRegion *mr)
> >  
> >  uint8_t memory_region_get_dirty_log_mask(MemoryRegion *mr)
> >  {
> > -    return mr->dirty_log_mask;
> > +    uint8_t mask = mr->dirty_log_mask;
> > +    if (global_dirty_log) {
> > +        mask |= (1 << DIRTY_MEMORY_MIGRATION);
> 
> This is ugly, but I don't know how to do differently. :(

Why do you think it's ugly?  As long as the log_start/log_stop callbacks
are handled properly, I think it's okay.

I find the "Refresh DIRTY_LOG_MIGRATION bit." part uglier. :)

Paolo

Re: [Qemu-devel] [PATCH 11/29] memory: include DIRTY_MEMORY_MIGRATION in the dirty log mask

[Paolo Bonzini]

On 26/05/2015 10:40, Fam Zheng wrote:
>> >  void memory_global_dirty_log_stop(void)
>> >  {
>> > +    /* Refresh DIRTY_LOG_MIGRATION bit.  */
>> > +    memory_region_transaction_begin();
>> > +    memory_region_update_pending = true;
>> > +    memory_region_transaction_commit();
>> > +
>> >      global_dirty_log = false;
> Shouldn't this go before memory_region_transaction_begin?

Yes.

Paolo

Re: [Qemu-devel] [PATCH 11/29] memory: include DIRTY_MEMORY_MIGRATION in the dirty log mask

[Fam Zheng]

On Tue, 05/26 11:07, Paolo Bonzini wrote:
> 
> 
> On 26/05/2015 10:40, Fam Zheng wrote:
> > > @@ -1329,7 +1329,11 @@ bool memory_region_is_skip_dump(MemoryRegion *mr)
> > >  
> > >  uint8_t memory_region_get_dirty_log_mask(MemoryRegion *mr)
> > >  {
> > > -    return mr->dirty_log_mask;
> > > +    uint8_t mask = mr->dirty_log_mask;
> > > +    if (global_dirty_log) {
> > > +        mask |= (1 << DIRTY_MEMORY_MIGRATION);
> > 
> > This is ugly, but I don't know how to do differently. :(
> 
> Why do you think it's ugly?
> As long as the log_start/log_stop callbacks
> are handled properly, I think it's okay.

Ugly in the per object function relying on a global to propogate the new value.

Fam

Re: [Qemu-devel] [PATCH 11/29] memory: include DIRTY_MEMORY_MIGRATION in the dirty log mask

[Paolo Bonzini]

On 26/05/2015 11:22, Fam Zheng wrote:
>>>> > > > @@ -1329,7 +1329,11 @@ bool memory_region_is_skip_dump(MemoryRegion *mr)
>>>> > > >  
>>>> > > >  uint8_t memory_region_get_dirty_log_mask(MemoryRegion *mr)
>>>> > > >  {
>>>> > > > -    return mr->dirty_log_mask;
>>>> > > > +    uint8_t mask = mr->dirty_log_mask;
>>>> > > > +    if (global_dirty_log) {
>>>> > > > +        mask |= (1 << DIRTY_MEMORY_MIGRATION);
>>> > > 
>>> > > This is ugly, but I don't know how to do differently. :(
>> > 
>> > Why do you think it's ugly?
>> > As long as the log_start/log_stop callbacks
>> > are handled properly, I think it's okay.
> Ugly in the per object function relying on a global to propogate the new value.

Yes, but that's the point of the patch. :)  It lets the listeners track
updates to the local state of the MemoryRegion instead of having to
track the global state.  So it's somewhat ugly, but it is less ugly than
tracking exactly the same global state in exec.c (core_log_global_start)
and kvm-all.c (kvm_log_global_start).

Does this make sense?

Paolo

Re: [Qemu-devel] [PATCH 11/29] memory: include DIRTY_MEMORY_MIGRATION in the dirty log mask

[Fam Zheng]

On Tue, 05/26 11:26, Paolo Bonzini wrote:
> 
> 
> On 26/05/2015 11:22, Fam Zheng wrote:
> >>>> > > > @@ -1329,7 +1329,11 @@ bool memory_region_is_skip_dump(MemoryRegion *mr)
> >>>> > > >  
> >>>> > > >  uint8_t memory_region_get_dirty_log_mask(MemoryRegion *mr)
> >>>> > > >  {
> >>>> > > > -    return mr->dirty_log_mask;
> >>>> > > > +    uint8_t mask = mr->dirty_log_mask;
> >>>> > > > +    if (global_dirty_log) {
> >>>> > > > +        mask |= (1 << DIRTY_MEMORY_MIGRATION);
> >>> > > 
> >>> > > This is ugly, but I don't know how to do differently. :(
> >> > 
> >> > Why do you think it's ugly?
> >> > As long as the log_start/log_stop callbacks
> >> > are handled properly, I think it's okay.
> > Ugly in the per object function relying on a global to propogate the new value.
> 
> Yes, but that's the point of the patch. :)  It lets the listeners track
> updates to the local state of the MemoryRegion instead of having to
> track the global state.  So it's somewhat ugly, but it is less ugly than
> tracking exactly the same global state in exec.c (core_log_global_start)
> and kvm-all.c (kvm_log_global_start).

Truely it's less ugly :)

> 
> Does this make sense?

Yes. Thanks.

Fam

Re: [Qemu-devel] [PATCH 14/29] exec: use memory_region_get_dirty_log_mask to optimize dirty tracking

[Fam Zheng]

On Mon, 04/27 18:28, Paolo Bonzini wrote:
> The memory API can now return the exact set of bitmaps that have to
> be tracked.  Use it instead of the in_migration variable.
> 
> In the next patches, we will also use it to set only DIRTY_MEMORY_VGA
> or DIRTY_MEMORY_MIGRATION if necessary.  This can make a difference
> for dataplane, especially after the dirty bitmap is changed to use
> more expensive atomic operations.
> 
> Of some interest is the change to stl_phys_notdirty.  When migration
> was introduced, stl_phys_notdirty was changed to effectively behave
> as stl_phys during migration.  In fact, if one looks at the function as it
> was in the beginning (commit 8df1cd0, physical memory access functions,
> 2005-01-28), at the time the dirty bitmap was the equivalent of
> DIRTY_MEMORY_CODE nowadays; hence, the function simply should not touch
> the dirty code bits.  This patch changes it to do the intended thing.

There are three changes in this patch:

 1) Removal of core_memory_listener;
 2) Test of dirty log mask bits in invalidate_and_set_dirty;
 3) Test of dirty log mask bits in stl_phys_notdirty.

1) and 3) are connected by in_migration, so they belong to the same patch. But
I'm not sure about 2). Is it required by 1) and 3), or it's changed because it
also touches the condition of tb_invalidate_phys_range?

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  exec.c | 59 +++++++++++++++++++----------------------------------------
>  1 file changed, 19 insertions(+), 40 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 083d65d..6e83161 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -59,8 +59,6 @@
>  //#define DEBUG_SUBPAGE
>  
>  #if !defined(CONFIG_USER_ONLY)
> -static bool in_migration;
> -
>  /* ram_list is read under rcu_read_lock()/rcu_read_unlock().  Writes
>   * are protected by the ramlist lock.
>   */
> @@ -871,11 +869,6 @@ void cpu_physical_memory_reset_dirty(ram_addr_t start, ram_addr_t length,
>      }
>  }
>  
> -static void cpu_physical_memory_set_dirty_tracking(bool enable)
> -{
> -    in_migration = enable;
> -}
> -
>  /* Called from RCU critical section */
>  hwaddr memory_region_section_get_iotlb(CPUState *cpu,
>                                         MemoryRegionSection *section,
> @@ -2140,22 +2133,6 @@ static void tcg_commit(MemoryListener *listener)
>      }
>  }
>  
> -static void core_log_global_start(MemoryListener *listener)
> -{
> -    cpu_physical_memory_set_dirty_tracking(true);
> -}
> -
> -static void core_log_global_stop(MemoryListener *listener)
> -{
> -    cpu_physical_memory_set_dirty_tracking(false);
> -}
> -
> -static MemoryListener core_memory_listener = {
> -    .log_global_start = core_log_global_start,
> -    .log_global_stop = core_log_global_stop,
> -    .priority = 1,
> -};
> -
>  void address_space_init_dispatch(AddressSpace *as)
>  {
>      as->dispatch = NULL;
> @@ -2195,8 +2172,6 @@ static void memory_map_init(void)
>      memory_region_init_io(system_io, NULL, &unassigned_io_ops, NULL, "io",
>                            65536);
>      address_space_init(&address_space_io, system_io, "I/O");
> -
> -    memory_listener_register(&core_memory_listener, &address_space_memory);
>  }
>  
>  MemoryRegion *get_system_memory(void)
> @@ -2254,12 +2229,18 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
>  
>  #else
>  
> -static void invalidate_and_set_dirty(hwaddr addr,
> +static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr,
>                                       hwaddr length)
>  {
>      if (cpu_physical_memory_range_includes_clean(addr, length)) {
> -        tb_invalidate_phys_range(addr, addr + length, 0);
> -        cpu_physical_memory_set_dirty_range_nocode(addr, length);
> +        uint8_t dirty_log_mask = memory_region_get_dirty_log_mask(mr);
> +        if (dirty_log_mask & (1 << DIRTY_MEMORY_CODE)) {
> +            tb_invalidate_phys_range(addr, addr + length, 0);
> +            dirty_log_mask &= ~(1 << DIRTY_MEMORY_CODE);
> +        }
> +        if (dirty_log_mask) {
> +            cpu_physical_memory_set_dirty_range_nocode(addr, length);
> +        }
>      } else {
>          xen_modified_memory(addr, length);
>      }
> @@ -2342,7 +2323,7 @@ bool address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
>                  /* RAM case */
>                  ptr = qemu_get_ram_ptr(addr1);
>                  memcpy(ptr, buf, l);
> -                invalidate_and_set_dirty(addr1, l);
> +                invalidate_and_set_dirty(mr, addr1, l);
>              }
>          } else {
>              if (!memory_access_is_direct(mr, is_write)) {
> @@ -2431,7 +2412,7 @@ static inline void cpu_physical_memory_write_rom_internal(AddressSpace *as,
>              switch (type) {
>              case WRITE_DATA:
>                  memcpy(ptr, buf, l);
> -                invalidate_and_set_dirty(addr1, l);
> +                invalidate_and_set_dirty(mr, addr1, l);
>                  break;
>              case FLUSH_CACHE:
>                  flush_icache_range((uintptr_t)ptr, (uintptr_t)ptr + l);
> @@ -2647,7 +2628,7 @@ void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
>          mr = qemu_ram_addr_from_host(buffer, &addr1);
>          assert(mr != NULL);
>          if (is_write) {
> -            invalidate_and_set_dirty(addr1, access_len);
> +            invalidate_and_set_dirty(mr, addr1, access_len);
>          }
>          if (xen_enabled()) {
>              xen_invalidate_map_cache_entry(buffer);
> @@ -2871,6 +2852,7 @@ void stl_phys_notdirty(AddressSpace *as, hwaddr addr, uint32_t val)
>      MemoryRegion *mr;
>      hwaddr l = 4;
>      hwaddr addr1;
> +    uint8_t dirty_log_mask;
>  
>      mr = address_space_translate(as, addr, &addr1, &l,
>                                   true);
> @@ -2881,13 +2863,10 @@ void stl_phys_notdirty(AddressSpace *as, hwaddr addr, uint32_t val)
>          ptr = qemu_get_ram_ptr(addr1);
>          stl_p(ptr, val);
>  
> -        if (unlikely(in_migration)) {
> -            if (cpu_physical_memory_is_clean(addr1)) {
> -                /* invalidate code */
> -                tb_invalidate_phys_page_range(addr1, addr1 + 4, 0);
> -                /* set dirty bit */
> -                cpu_physical_memory_set_dirty_range_nocode(addr1, 4);
> -            }
> +        dirty_log_mask = memory_region_get_dirty_log_mask(mr);
> +        dirty_log_mask &= ~(1 << DIRTY_MEMORY_CODE);
> +        if (dirty_log_mask) {
> +            cpu_physical_memory_set_dirty_range_nocode(addr1, 4);

Looks OK.

A side question, it seems cpu_physical_memory_is_clean returns true if *any* of
three bitmaps is clean:

    static inline bool cpu_physical_memory_is_clean(ram_addr_t addr)
    {
        bool vga = cpu_physical_memory_get_dirty_flag(addr, DIRTY_MEMORY_VGA);
        bool code = cpu_physical_memory_get_dirty_flag(addr, DIRTY_MEMORY_CODE);
        bool migration =
            cpu_physical_memory_get_dirty_flag(addr, DIRTY_MEMORY_MIGRATION);
->      return !(vga && code && migration);
    }

It's counter-intuitive. Why is that?

Fam

>          }
>      }
>  }
> @@ -2930,7 +2909,7 @@ static inline void stl_phys_internal(AddressSpace *as,
>              stl_p(ptr, val);
>              break;
>          }
> -        invalidate_and_set_dirty(addr1, 4);
> +        invalidate_and_set_dirty(mr, addr1, 4);
>      }
>  }
>  
> @@ -2993,7 +2972,7 @@ static inline void stw_phys_internal(AddressSpace *as,
>              stw_p(ptr, val);
>              break;
>          }
> -        invalidate_and_set_dirty(addr1, 2);
> +        invalidate_and_set_dirty(mr, addr1, 2);
>      }
>  }
>  
> -- 
> 1.8.3.1
> 
> 

Re: [Qemu-devel] [PATCH 15/29] exec: move functions to translate-all.h

[Fam Zheng]

On Mon, 04/27 18:28, Paolo Bonzini wrote:
> Remove them from the sundry exec-all.h header, since they are only used by
> the TCG runtime in exec.c and user-exec.c.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH 16/29] translate-all: remove unnecessary argument to tb_invalidate_phys_range

[Fam Zheng]

On Mon, 04/27 18:28, Paolo Bonzini wrote:
> The is_cpu_write_access argument is always 0, remove it.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH 17/29] cputlb: remove useless arguments to tlb_unprotect_code_phys, rename

[Fam Zheng]

On Mon, 04/27 18:28, Paolo Bonzini wrote:
> These days modification of the TLB is done in notdirty_mem_write,
> so the virtual address and env pointer as unnecessary.
> 
> The new name of the function, tlb_unprotect_code, is consistent with
> tlb_protect_code.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH 14/29] exec: use memory_region_get_dirty_log_mask to optimize dirty tracking

[Paolo Bonzini]

On 26/05/2015 12:42, Fam Zheng wrote:
> On Mon, 04/27 18:28, Paolo Bonzini wrote:
>> The memory API can now return the exact set of bitmaps that have to
>> be tracked.  Use it instead of the in_migration variable.
>>
>> In the next patches, we will also use it to set only DIRTY_MEMORY_VGA
>> or DIRTY_MEMORY_MIGRATION if necessary.  This can make a difference
>> for dataplane, especially after the dirty bitmap is changed to use
>> more expensive atomic operations.
>>
>> Of some interest is the change to stl_phys_notdirty.  When migration
>> was introduced, stl_phys_notdirty was changed to effectively behave
>> as stl_phys during migration.  In fact, if one looks at the function as it
>> was in the beginning (commit 8df1cd0, physical memory access functions,
>> 2005-01-28), at the time the dirty bitmap was the equivalent of
>> DIRTY_MEMORY_CODE nowadays; hence, the function simply should not touch
>> the dirty code bits.  This patch changes it to do the intended thing.
> 
> There are three changes in this patch:
> 
>  1) Removal of core_memory_listener;
>  2) Test of dirty log mask bits in invalidate_and_set_dirty;
>  3) Test of dirty log mask bits in stl_phys_notdirty.
> 
> 1) and 3) are connected by in_migration, so they belong to the same patch. But
> I'm not sure about 2). Is it required by 1) and 3), or it's changed because it
> also touches the condition of tb_invalidate_phys_range?

The idea was really to put together (2) and (3), which are connected by
memory_region_get_dirty_log_mask and
cpu_physical_memory_set_dirty_range_nocode.  The difference is that (2)
calls tb_invalidate_phys_range, while (3) does not (because it is
"_notdirty").

(1) is just dead code removal.

> Looks OK.
> 
> A side question, it seems cpu_physical_memory_is_clean returns true if *any* of
> three bitmaps is clean:
> 
>     static inline bool cpu_physical_memory_is_clean(ram_addr_t addr)
>     {
>         bool vga = cpu_physical_memory_get_dirty_flag(addr, DIRTY_MEMORY_VGA);
>         bool code = cpu_physical_memory_get_dirty_flag(addr, DIRTY_MEMORY_CODE);
>         bool migration =
>             cpu_physical_memory_get_dirty_flag(addr, DIRTY_MEMORY_MIGRATION);
> ->      return !(vga && code && migration);
>     }
> 
> It's counter-intuitive. Why is that?

If any bit is clean, writes need trapping in order to set those bits.

Remember that the code in address_space_stl_notdirty didn't really make
sense before this patch, so do not read much into it.  At the end of the
series, cpu_physical_memory_is_clean is only used from
notdirty_mem_write and tlb_set_page_with_attrs, i.e. only from TCG.
That is more understandable.  Perhaps we can rename it to
cpu_physical_memory_needs_notdirty.

Paolo

> 
> Fam
> 
>>          }
>>      }
>>  }
>> @@ -2930,7 +2909,7 @@ static inline void stl_phys_internal(AddressSpace *as,
>>              stl_p(ptr, val);
>>              break;
>>          }
>> -        invalidate_and_set_dirty(addr1, 4);
>> +        invalidate_and_set_dirty(mr, addr1, 4);
>>      }
>>  }
>>  
>> @@ -2993,7 +2972,7 @@ static inline void stw_phys_internal(AddressSpace *as,
>>              stw_p(ptr, val);
>>              break;
>>          }
>> -        invalidate_and_set_dirty(addr1, 2);
>> +        invalidate_and_set_dirty(mr, addr1, 2);
>>      }
>>  }
>>  
>> -- 
>> 1.8.3.1
>>
>>

Re: [Qemu-devel] [PATCH 19/29] exec: pass client mask to cpu_physical_memory_set_dirty_range

[Fam Zheng]

On Mon, 04/27 18:28, Paolo Bonzini wrote:
> This cuts in half the cost of bitmap operations (which will become more
> expensive when made atomic) during migration on non-VRAM regions.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  exec.c                  | 20 +++++++++++---------
>  include/exec/ram_addr.h | 33 ++++++++++++++++-----------------
>  memory.c                |  3 ++-
>  3 files changed, 29 insertions(+), 27 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 1c75dd1..cd8665a 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1356,7 +1356,8 @@ int qemu_ram_resize(ram_addr_t base, ram_addr_t newsize, Error **errp)
>  
>      cpu_physical_memory_clear_dirty_range(block->offset, block->used_length);
>      block->used_length = newsize;
> -    cpu_physical_memory_set_dirty_range(block->offset, block->used_length);
> +    cpu_physical_memory_set_dirty_range(block->offset, block->used_length,
> +                                        DIRTY_CLIENTS_ALL);
>      memory_region_set_size(block->mr, newsize);
>      if (block->resized) {
>          block->resized(block->idstr, newsize, block->host);
> @@ -1430,7 +1431,8 @@ static ram_addr_t ram_block_add(RAMBlock *new_block, Error **errp)
>         }
>      }
>      cpu_physical_memory_set_dirty_range(new_block->offset,
> -                                        new_block->used_length);
> +                                        new_block->used_length,
> +                                        DIRTY_CLIENTS_ALL);
>  
>      if (new_block->host) {
>          qemu_ram_setup_dump(new_block->host, new_block->max_length);
> @@ -1818,7 +1820,11 @@ static void notdirty_mem_write(void *opaque, hwaddr ram_addr,
>      default:
>          abort();
>      }
> -    cpu_physical_memory_set_dirty_range_nocode(ram_addr, size);
> +    /* Set both VGA and migration bits for simplicity and to remove
> +     * the notdirty callback faster.
> +     */
> +    cpu_physical_memory_set_dirty_range(ram_addr, size,
> +                                        DIRTY_CLIENTS_NOCODE);
>      /* we remove the notdirty callback only if the code has been
>         flushed */
>      if (!cpu_physical_memory_is_clean(ram_addr)) {
> @@ -2238,9 +2244,7 @@ static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr,
>              tb_invalidate_phys_range(addr, addr + length);
>              dirty_log_mask &= ~(1 << DIRTY_MEMORY_CODE);
>          }
> -        if (dirty_log_mask) {
> -            cpu_physical_memory_set_dirty_range_nocode(addr, length);
> -        }
> +        cpu_physical_memory_set_dirty_range(addr, length, dirty_log_mask);
>      } else {
>          xen_modified_memory(addr, length);
>      }
> @@ -2865,9 +2869,7 @@ void stl_phys_notdirty(AddressSpace *as, hwaddr addr, uint32_t val)
>  
>          dirty_log_mask = memory_region_get_dirty_log_mask(mr);
>          dirty_log_mask &= ~(1 << DIRTY_MEMORY_CODE);
> -        if (dirty_log_mask) {
> -            cpu_physical_memory_set_dirty_range_nocode(addr1, 4);
> -        }
> +        cpu_physical_memory_set_dirty_range(addr1, 4, dirty_log_mask);
>      }
>  }
>  
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index 7f6e928..29b89d2 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -41,6 +41,9 @@ void qemu_ram_free_from_ptr(ram_addr_t addr);
>  
>  int qemu_ram_resize(ram_addr_t base, ram_addr_t newsize, Error **errp);
>  
> +#define DIRTY_CLIENTS_ALL      ((1 << DIRTY_MEMORY_NUM) - 1)
> +#define DIRTY_CLIENTS_NOCODE   (DIRTY_CLIENTS_ALL & ~(1 << DIRTY_MEMORY_CODE))
> +
>  static inline bool cpu_physical_memory_get_dirty(ram_addr_t start,
>                                                   ram_addr_t length,
>                                                   unsigned client)
> @@ -103,28 +106,23 @@ static inline void cpu_physical_memory_set_dirty_flag(ram_addr_t addr,
>      set_bit(addr >> TARGET_PAGE_BITS, ram_list.dirty_memory[client]);
>  }
>  
> -static inline void cpu_physical_memory_set_dirty_range_nocode(ram_addr_t start,
> -                                                              ram_addr_t length)
> -{
> -    unsigned long end, page;
> -
> -    end = TARGET_PAGE_ALIGN(start + length) >> TARGET_PAGE_BITS;
> -    page = start >> TARGET_PAGE_BITS;
> -    bitmap_set(ram_list.dirty_memory[DIRTY_MEMORY_MIGRATION], page, end - page);
> -    bitmap_set(ram_list.dirty_memory[DIRTY_MEMORY_VGA], page, end - page);
> -    xen_modified_memory(start, length);
> -}
> -
>  static inline void cpu_physical_memory_set_dirty_range(ram_addr_t start,
> -                                                       ram_addr_t length)
> +                                                       ram_addr_t length,
> +                                                       uint8_t mask)
>  {
>      unsigned long end, page;
>  
>      end = TARGET_PAGE_ALIGN(start + length) >> TARGET_PAGE_BITS;
>      page = start >> TARGET_PAGE_BITS;
> -    bitmap_set(ram_list.dirty_memory[DIRTY_MEMORY_MIGRATION], page, end - page);
> -    bitmap_set(ram_list.dirty_memory[DIRTY_MEMORY_VGA], page, end - page);
> -    bitmap_set(ram_list.dirty_memory[DIRTY_MEMORY_CODE], page, end - page);
> +    if (likely(mask & (1 << DIRTY_MEMORY_MIGRATION))) {
> +        bitmap_set(ram_list.dirty_memory[DIRTY_MEMORY_MIGRATION], page, end - page);

Wrap the line? Other than that:

Reviewed-by: Fam Zheng <famz@redhat.com>

> +    }
> +    if (unlikely(mask & (1 << DIRTY_MEMORY_VGA))) {
> +        bitmap_set(ram_list.dirty_memory[DIRTY_MEMORY_VGA], page, end - page);
> +    }
> +    if (unlikely(mask & (1 << DIRTY_MEMORY_CODE))) {
> +        bitmap_set(ram_list.dirty_memory[DIRTY_MEMORY_CODE], page, end - page);
> +    }
>      xen_modified_memory(start, length);
>  }
>  
> @@ -172,7 +170,8 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>                      addr = page_number * TARGET_PAGE_SIZE;
>                      ram_addr = start + addr;
>                      cpu_physical_memory_set_dirty_range(ram_addr,
> -                                       TARGET_PAGE_SIZE * hpratio);
> +                                       TARGET_PAGE_SIZE * hpratio,
> +                                       DIRTY_CLIENTS_ALL);
>                  } while (c != 0);
>              }
>          }
> diff --git a/memory.c b/memory.c
> index 174cd15..7422790 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1390,7 +1390,8 @@ void memory_region_set_dirty(MemoryRegion *mr, hwaddr addr,
>                               hwaddr size)
>  {
>      assert(mr->terminates);
> -    cpu_physical_memory_set_dirty_range(mr->ram_addr + addr, size);
> +    cpu_physical_memory_set_dirty_range(mr->ram_addr + addr, size,
> +                                        memory_region_get_dirty_log_mask(mr));
>  }
>  
>  bool memory_region_test_and_clear_dirty(MemoryRegion *mr, hwaddr addr,
> -- 
> 1.8.3.1
> 
> 
> 

Re: [Qemu-devel] [PATCH 14/29] exec: use memory_region_get_dirty_log_mask to optimize dirty tracking

[Fam Zheng]

On Tue, 05/26 12:58, Paolo Bonzini wrote:
> 
> 
> On 26/05/2015 12:42, Fam Zheng wrote:
> > On Mon, 04/27 18:28, Paolo Bonzini wrote:
> >> The memory API can now return the exact set of bitmaps that have to
> >> be tracked.  Use it instead of the in_migration variable.
> >>
> >> In the next patches, we will also use it to set only DIRTY_MEMORY_VGA
> >> or DIRTY_MEMORY_MIGRATION if necessary.  This can make a difference
> >> for dataplane, especially after the dirty bitmap is changed to use
> >> more expensive atomic operations.
> >>
> >> Of some interest is the change to stl_phys_notdirty.  When migration
> >> was introduced, stl_phys_notdirty was changed to effectively behave
> >> as stl_phys during migration.  In fact, if one looks at the function as it
> >> was in the beginning (commit 8df1cd0, physical memory access functions,
> >> 2005-01-28), at the time the dirty bitmap was the equivalent of
> >> DIRTY_MEMORY_CODE nowadays; hence, the function simply should not touch
> >> the dirty code bits.  This patch changes it to do the intended thing.
> > 
> > There are three changes in this patch:
> > 
> >  1) Removal of core_memory_listener;
> >  2) Test of dirty log mask bits in invalidate_and_set_dirty;
> >  3) Test of dirty log mask bits in stl_phys_notdirty.
> > 
> > 1) and 3) are connected by in_migration, so they belong to the same patch. But
> > I'm not sure about 2). Is it required by 1) and 3), or it's changed because it
> > also touches the condition of tb_invalidate_phys_range?
> 
> The idea was really to put together (2) and (3), which are connected by
> memory_region_get_dirty_log_mask and
> cpu_physical_memory_set_dirty_range_nocode.  The difference is that (2)
> calls tb_invalidate_phys_range, while (3) does not (because it is
> "_notdirty").
> 
> (1) is just dead code removal.
> 
> > Looks OK.
> > 
> > A side question, it seems cpu_physical_memory_is_clean returns true if *any* of
> > three bitmaps is clean:
> > 
> >     static inline bool cpu_physical_memory_is_clean(ram_addr_t addr)
> >     {
> >         bool vga = cpu_physical_memory_get_dirty_flag(addr, DIRTY_MEMORY_VGA);
> >         bool code = cpu_physical_memory_get_dirty_flag(addr, DIRTY_MEMORY_CODE);
> >         bool migration =
> >             cpu_physical_memory_get_dirty_flag(addr, DIRTY_MEMORY_MIGRATION);
> > ->      return !(vga && code && migration);
> >     }
> > 
> > It's counter-intuitive. Why is that?
> 
> If any bit is clean, writes need trapping in order to set those bits.

Yes.

> 
> Remember that the code in address_space_stl_notdirty didn't really make
> sense before this patch, so do not read much into it.  At the end of the
> series, cpu_physical_memory_is_clean is only used from
> notdirty_mem_write and tlb_set_page_with_attrs, i.e. only from TCG.
> That is more understandable.  Perhaps we can rename it to
> cpu_physical_memory_needs_notdirty.

Thanks, that complements my reading!

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH 07/29] framebuffer: check memory_region_is_logging

[Paolo Bonzini]

On 26/05/2015 10:02, Fam Zheng wrote:
> On Mon, 04/27 18:28, Paolo Bonzini wrote:
>> framebuffer.c expects DIRTY_MEMORY_VGA logging to be always on, but that
>> will not be the case soon.  Because framebuffer.c computes the memory
>> region on the fly for every update (with memory_region_find), it cannot
>> enable/disable logging by itself.
>>
>> Instead, always treat updates as invalidations if dirty logging is
>> not enabled, assuming that the board will enable logging on the
>> RAM region that includes the framebuffer.
>>
>> To simplify the code, replace memory_region_get_dirty with
>> memory_region_test_and_clear_dirty.  Then memory_region_reset_dirty
>> is only needed in the invalidate case.
> 
> The commit message seems out-of-date, but I think it's better to keep the v1
> hunks which skip memory_region_get_dirty if invalidate == true, otherwise use
> memory_region_test_and_clear_dirty.

Unfortunately that is wrong.  See the first paragraph of the commit
message: "because framebuffer.c computes the memory region on the fly
for every update (with memory_region_find), it cannot enable/disable
logging by itself".

Only the last paragraph of the commit message is out-of-date, and I will
remove it.

Paolo

Re: [Qemu-devel] [PATCH 03/29] display: enable DIRTY_MEMORY_VGA tracking explicitly

[Paolo Bonzini]

Peter, Mark, Aurelien, can you review and ack this patch?

Thanks,

Paolo

On 27/04/2015 18:28, Paolo Bonzini wrote:
> This will be required soon by the memory core.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/display/cg3.c             |  1 +
>  hw/display/exynos4210_fimd.c | 20 +++++++++++++-------
>  hw/display/g364fb.c          |  1 +
>  hw/display/sm501.c           |  1 +
>  hw/display/tcx.c             |  1 +
>  5 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/display/cg3.c b/hw/display/cg3.c
> index 1e6ff2b..cbcf518 100644
> --- a/hw/display/cg3.c
> +++ b/hw/display/cg3.c
> @@ -309,6 +309,7 @@ static void cg3_realizefn(DeviceState *dev, Error **errp)
>  
>      memory_region_init_ram(&s->vram_mem, NULL, "cg3.vram", s->vram_size,
>                             &error_abort);
> +    memory_region_set_log(&s->vram_mem, true, DIRTY_MEMORY_VGA);
>      vmstate_register_ram_global(&s->vram_mem);
>      sysbus_init_mmio(sbd, &s->vram_mem);
>  
> diff --git a/hw/display/exynos4210_fimd.c b/hw/display/exynos4210_fimd.c
> index 45c62af..72b3a1d 100644
> --- a/hw/display/exynos4210_fimd.c
> +++ b/hw/display/exynos4210_fimd.c
> @@ -1109,6 +1109,12 @@ static inline int fimd_get_buffer_id(Exynos4210fimdWindow *w)
>      }
>  }
>  
> +static void exynos4210_fimd_invalidate(void *opaque)
> +{
> +    Exynos4210fimdState *s = (Exynos4210fimdState *)opaque;
> +    s->invalidate = true;
> +}
> +
>  /* Updates specified window's MemorySection based on values of WINCON,
>   * VIDOSDA, VIDOSDB, VIDWADDx and SHADOWCON registers */
>  static void fimd_update_memory_section(Exynos4210fimdState *s, unsigned win)
> @@ -1136,7 +1142,11 @@ static void fimd_update_memory_section(Exynos4210fimdState *s, unsigned win)
>      /* TODO: add .exit and unref the region there.  Not needed yet since sysbus
>       * does not support hot-unplug.
>       */
> -    memory_region_unref(w->mem_section.mr);
> +    if (w->mem_section.mr) {
> +        memory_region_set_log(w->mem_section.mr, false, DIRTY_MEMORY_VGA);
> +        memory_region_unref(w->mem_section.mr);
> +    }
> +
>      w->mem_section = memory_region_find(sysbus_address_space(sbd),
>                                          fb_start_addr, w->fb_len);
>      assert(w->mem_section.mr);
> @@ -1162,6 +1172,8 @@ static void fimd_update_memory_section(Exynos4210fimdState *s, unsigned win)
>          cpu_physical_memory_unmap(w->host_fb_addr, fb_mapped_len, 0, 0);
>          goto error_return;
>      }
> +    memory_region_set_log(w->mem_section.mr, true, DIRTY_MEMORY_VGA);
> +    exynos4210_fimd_invalidate(s);
>      return;
>  
>  error_return:
> @@ -1224,12 +1236,6 @@ static void exynos4210_fimd_update_irq(Exynos4210fimdState *s)
>      }
>  }
>  
> -static void exynos4210_fimd_invalidate(void *opaque)
> -{
> -    Exynos4210fimdState *s = (Exynos4210fimdState *)opaque;
> -    s->invalidate = true;
> -}
> -
>  static void exynos4210_update_resolution(Exynos4210fimdState *s)
>  {
>      DisplaySurface *surface = qemu_console_surface(s->console);
> diff --git a/hw/display/g364fb.c b/hw/display/g364fb.c
> index 6543f2f..be62dd6 100644
> --- a/hw/display/g364fb.c
> +++ b/hw/display/g364fb.c
> @@ -489,6 +489,7 @@ static void g364fb_init(DeviceState *dev, G364State *s)
>      memory_region_init_ram_ptr(&s->mem_vram, NULL, "vram",
>                                 s->vram_size, s->vram);
>      vmstate_register_ram(&s->mem_vram, dev);
> +    memory_region_set_log(&s->mem_vram, true, DIRTY_MEMORY_VGA);
>  }
>  
>  #define TYPE_G364 "sysbus-g364"
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index c72154b..43f8538 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -1412,6 +1412,7 @@ void sm501_init(MemoryRegion *address_space_mem, uint32_t base,
>      memory_region_init_ram(&s->local_mem_region, NULL, "sm501.local",
>                             local_mem_bytes, &error_abort);
>      vmstate_register_ram_global(&s->local_mem_region);
> +    memory_region_set_log(&s->local_mem_region, true, DIRTY_MEMORY_VGA);
>      s->local_mem = memory_region_get_ram_ptr(&s->local_mem_region);
>      memory_region_add_subregion(address_space_mem, base, &s->local_mem_region);
>  
> diff --git a/hw/display/tcx.c b/hw/display/tcx.c
> index a9f9f66..58faa96 100644
> --- a/hw/display/tcx.c
> +++ b/hw/display/tcx.c
> @@ -1006,6 +1006,7 @@ static void tcx_realizefn(DeviceState *dev, Error **errp)
>      memory_region_init_ram(&s->vram_mem, OBJECT(s), "tcx.vram",
>                             s->vram_size * (1 + 4 + 4), &error_abort);
>      vmstate_register_ram_global(&s->vram_mem);
> +    memory_region_set_log(&s->vram_mem, true, DIRTY_MEMORY_VGA);
>      vram_base = memory_region_get_ram_ptr(&s->vram_mem);
>  
>      /* 10/ROM : FCode ROM */
> 

Re: [Qemu-devel] [PATCH 20/29] exec: invert return value of cpu_physical_memory_get_clean, rename

[Fam Zheng]

On Mon, 04/27 18:28, Paolo Bonzini wrote:
> While it is obvious that cpu_physical_memory_get_dirty returns true even if
> a single page is dirty, the same is not true for cpu_physical_memory_get_clean;
> one would expect that it returns true only if all the pages are clean, but
> it actually looks for even one clean page.  (By contrast, the caller of that
> function, cpu_physical_memory_range_includes_clean, has a good name).
> 
> To clarify, rename the function to cpu_physical_memory_all_dirty and return
> true if _all_ the pages are dirty.  This is the opposite of the previous
> meaning, because "all are 1" is the same as "not (any is 0)", so we have to
> modify cpu_physical_memory_range_includes_clean as well.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

Then maybe we can rename cpu_physical_memory_is_clean to
cpu_physical_memory_has_clean, or as you suggested,
cpu_physical_memory_needs_notdirty (but wait, what is 'notdirty'? :), for a
similar reason.

Fam

Re: [Qemu-devel] [PATCH 19/29] exec: pass client mask to cpu_physical_memory_set_dirty_range

[Paolo Bonzini]

On 26/05/2015 13:08, Fam Zheng wrote:
>> > +    if (likely(mask & (1 << DIRTY_MEMORY_MIGRATION))) {
>> > +        bitmap_set(ram_list.dirty_memory[DIRTY_MEMORY_MIGRATION], page, end - page);
> Wrap the line? Other than that:
> 
> Reviewed-by: Fam Zheng <famz@redhat.com>

Yes, the line gets shorter at the end of the series.

Paolo

Re: [Qemu-devel] [PATCH 21/29] exec: only check relevant bitmaps for cleanliness

[Fam Zheng]

On Mon, 04/27 18:28, Paolo Bonzini wrote:
> Most of the time, not all bitmaps have to be marked as dirty;
> do not do anything if the interesting ones are already dirty.
> Previously, any clean bitmap would have cause all the bitmaps to be
> marked dirty.
> 
> In fact, unless running TCG most of the time bitmap operations need
> not be done at all, because memory_region_is_logging returns zero.
> In this case, skip the call to cpu_physical_memory_range_includes_clean
> altogether as well.
> 
> With this patch, cpu_physical_memory_set_dirty_range is called
> unconditionally, so there need not be anymore a separate call to
> xen_modified_memory.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH 22/29] memory: do not touch code dirty bitmap unless TCG is enabled

[Fam Zheng]

On Mon, 04/27 18:28, Paolo Bonzini wrote:
> cpu_physical_memory_set_dirty_lebitmap unconditionally syncs the
> DIRTY_MEMORY_CODE bitmap.  This however is unused unless TCG is
> enabled.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH 05/29] memory: differentiate memory_region_is_logging and memory_region_get_dirty_log_mask

[Paolo Bonzini]

On 26/05/2015 09:46, Fam Zheng wrote:
>> >   *
>> > - * Returns %true if the memory region is logging writes
>> > + * Returns %true if the memory region is logging writes for the given client
>> > + * a bitmap of clients for which the memory region is logging writes.
> I can't parse this sentence, rephrase a bit?

The second line is stail

>> >   *
>> >   * @mr: the memory region being queried
>> > + * @client: the client being queried
>> >   */
>> > -bool memory_region_is_logging(MemoryRegion *mr);
>> > +bool memory_region_is_logging(MemoryRegion *mr, uint8_t client);
>> > +
>> > +/**
>> > + * memory_region_get_dirty_log_mask: return the clients for which a
> s/which a/ which the/ ?

"a" is consistent with "memory_region_name: get a memory region's name",
for example.

>> > + * memory region is logging writes.
>> > + *
>> > + * Returns a bitmap of clients for which the memory region is logging writes.
> Why repeat?

Just a consequence of how the doc comments are written.  For some
functions the summary is the same as the description of the return
value.  I'll change it to just "Returns a bitmap of clients, which right
now will be either 0 or (1 << DIRTY_MEMORY_VGA)".

Paolo

> All questions are trivial enough that once you fix them or if they don't need
> fixing:
> 
> Reviewed-by: Fam Zheng <famz@redhat.com>
> 

Re: [Qemu-devel] [PATCH 03/29] display: enable DIRTY_MEMORY_VGA tracking explicitly

[Peter Maydell]

On 26 May 2015 at 12:24, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Peter, Mark, Aurelien, can you review and ack this patch?

Could you provide some explanation/documentation of when a
display device needs to set DIRTY_MEMORY_VGA (and when it
doesn't)? If we get it wrong is there any way to make qemu
assert or otherwise catch the error?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 23/29] bitmap: add atomic set functions

[Fam Zheng]

On Mon, 04/27 18:28, Paolo Bonzini wrote:
> From: Stefan Hajnoczi <stefanha@redhat.com>
> 
> Use atomic_or() for atomic bitmaps where several threads may set bits at
> the same time.  This avoids the race condition between threads loading
> an element, bitwise ORing, and then storing the element.
> 
> When setting all bits in a word we can avoid atomic ops and instead just
> use an smp_mb() at the end.
> 
> Most bitmap users don't need atomicity so introduce new functions.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Message-Id: <1417519399-3166-2-git-send-email-stefanha@redhat.com>
> [Avoid barrier in the single word case, use full barrier instead of write.
>  - Paolo]
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/qemu/bitmap.h |  2 ++
>  include/qemu/bitops.h | 14 ++++++++++++++
>  util/bitmap.c         | 37 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 53 insertions(+)
> 
> diff --git a/include/qemu/bitmap.h b/include/qemu/bitmap.h
> index f0273c9..3e0a4f3 100644
> --- a/include/qemu/bitmap.h
> +++ b/include/qemu/bitmap.h
> @@ -39,6 +39,7 @@
>   * bitmap_empty(src, nbits)			Are all bits zero in *src?
>   * bitmap_full(src, nbits)			Are all bits set in *src?
>   * bitmap_set(dst, pos, nbits)			Set specified bit area
> + * bitmap_set_atomic(dst, pos, nbits)   Set specified bit area with atomic ops
>   * bitmap_clear(dst, pos, nbits)		Clear specified bit area
>   * bitmap_find_next_zero_area(buf, len, pos, n, mask)	Find bit free area
>   */
> @@ -226,6 +227,7 @@ static inline int bitmap_intersects(const unsigned long *src1,
>  }
>  
>  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);
>  unsigned long bitmap_find_next_zero_area(unsigned long *map,
>                                           unsigned long size,
> diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
> index 90ca8df..95f98fb 100644
> --- a/include/qemu/bitops.h
> +++ b/include/qemu/bitops.h
> @@ -16,6 +16,7 @@
>  #include <assert.h>
>  
>  #include "host-utils.h"
> +#include "atomic.h"
>  
>  #define BITS_PER_BYTE           CHAR_BIT
>  #define BITS_PER_LONG           (sizeof (unsigned long) * BITS_PER_BYTE)
> @@ -39,6 +40,19 @@ static inline void set_bit(long nr, unsigned long *addr)
>  }
>  
>  /**
> + * set_bit_atomic - Set a bit in memory atomically
> + * @nr: the bit to set
> + * @addr: the address to start counting from
> + */
> +static inline void set_bit_atomic(long nr, unsigned long *addr)
> +{
> +    unsigned long mask = BIT_MASK(nr);
> +    unsigned long *p = addr + BIT_WORD(nr);
> +
> +    atomic_or(p, mask);
> +}
> +
> +/**
>   * clear_bit - Clears a bit in memory
>   * @nr: Bit to clear
>   * @addr: Address to start counting from
> diff --git a/util/bitmap.c b/util/bitmap.c
> index 9c6bb52..6838d49 100644
> --- a/util/bitmap.c
> +++ b/util/bitmap.c
> @@ -11,6 +11,7 @@
>  
>  #include "qemu/bitops.h"
>  #include "qemu/bitmap.h"
> +#include "qemu/atomic.h"
>  
>  /*
>   * bitmaps provide an array of bits, implemented using an an
> @@ -177,6 +178,42 @@ void bitmap_set(unsigned long *map, long start, long nr)
>      }
>  }
>  
> +void bitmap_set_atomic(unsigned long *map, long start, long nr)
> +{
> +    unsigned long *p = map + BIT_WORD(start);
> +    const long size = start + nr;

s/size/end/ ?

Otherwise,

Reviewed-by: Fam Zheng <famz@redhat.com>

> +    int bits_to_set = BITS_PER_LONG - (start % BITS_PER_LONG);
> +    unsigned long mask_to_set = BITMAP_FIRST_WORD_MASK(start);
> +
> +    /* First word */
> +    if (nr - bits_to_set > 0) {
> +        atomic_or(p, mask_to_set);
> +        nr -= bits_to_set;
> +        bits_to_set = BITS_PER_LONG;
> +        mask_to_set = ~0UL;
> +        p++;
> +    }
> +
> +    /* Full words */
> +    while (nr - bits_to_set >= 0) {
> +        *p = ~0UL;
> +        nr -= bits_to_set;
> +        mask_to_set = ~0UL;
> +        p++;
> +    }

For full words, will memset be faster by any chance?

> +
> +    /* Last word */
> +    if (nr) {
> +        mask_to_set &= BITMAP_LAST_WORD_MASK(size);
> +        atomic_or(p, mask_to_set);
> +    } else {
> +        /* If we avoided the full barrier in atomic_or(), issue a
> +         * barrier to account for the assignments in the while loop.
> +         */
> +        smp_mb();
> +    }
> +}
> +
>  void bitmap_clear(unsigned long *map, long start, long nr)
>  {
>      unsigned long *p = map + BIT_WORD(start);
> -- 
> 1.8.3.1
> 
> 

Re: [Qemu-devel] [PATCH 03/29] display: enable DIRTY_MEMORY_VGA tracking explicitly

[Paolo Bonzini]

On 26/05/2015 13:52, Peter Maydell wrote:
> On 26 May 2015 at 12:24, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> > Peter, Mark, Aurelien, can you review and ack this patch?
> Could you provide some explanation/documentation of when a
> display device needs to set DIRTY_MEMORY_VGA (and when it
> doesn't)?

It needs to set it if it uses memory_region_get/set/clear_dirty with
DIRTY_MEMORY_VGA as the last argument.

> If we get it wrong is there any way to make qemu
> assert or otherwise catch the error?

It may be possible to check against mr->dirty_log_mask in
memory_region_get/set/clear_dirty.  However, it is just as likely to
have some corner case that is correct but triggers the assertion.

I haven't thought much about it, because a simple grep for
DIRTY_MEMORY_VGA will catch the device models that need care.

Paolo

Re: [Qemu-devel] [PATCH 23/29] bitmap: add atomic set functions

[Paolo Bonzini]

On 26/05/2015 13:54, Fam Zheng wrote:
> > +void bitmap_set_atomic(unsigned long *map, long start, long nr)
> > +{
> > +    unsigned long *p = map + BIT_WORD(start);
> > +    const long size = start + nr;
> 
> s/size/end/ ?

That is consistent with bitmap_set, but we could consider changing it
there too.

Paolo

> Otherwise,
> 
> Reviewed-by: Fam Zheng <famz@redhat.com>
> 

Re: [Qemu-devel] [PATCH 03/29] display: enable DIRTY_MEMORY_VGA tracking explicitly

[Peter Maydell]

On 26 May 2015 at 12:56, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 26/05/2015 13:52, Peter Maydell wrote:
>> On 26 May 2015 at 12:24, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> > Peter, Mark, Aurelien, can you review and ack this patch?
>> Could you provide some explanation/documentation of when a
>> display device needs to set DIRTY_MEMORY_VGA (and when it
>> doesn't)?
>
> It needs to set it if it uses memory_region_get/set/clear_dirty with
> DIRTY_MEMORY_VGA as the last argument.

OK, and which devices need to do that?

-- PMM

Re: [Qemu-devel] [PATCH 03/29] display: enable DIRTY_MEMORY_VGA tracking explicitly

[Paolo Bonzini]

On 26/05/2015 14:08, Peter Maydell wrote:
>>> >> Could you provide some explanation/documentation of when a
>>> >> display device needs to set DIRTY_MEMORY_VGA (and when it
>>> >> doesn't)?
>> >
>> > It needs to set it if it uses memory_region_get/set/clear_dirty with
>> > DIRTY_MEMORY_VGA as the last argument.
> OK, and which devices need to do that?

Those that care to optimize their update_display callback.  It's
entirely opt in.

Paolo

Re: [Qemu-devel] [PATCH 03/29] display: enable DIRTY_MEMORY_VGA tracking explicitly

[Peter Maydell]

On 26 May 2015 at 13:10, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 26/05/2015 14:08, Peter Maydell wrote:
>>>> >> Could you provide some explanation/documentation of when a
>>>> >> display device needs to set DIRTY_MEMORY_VGA (and when it
>>>> >> doesn't)?
>>> >
>>> > It needs to set it if it uses memory_region_get/set/clear_dirty with
>>> > DIRTY_MEMORY_VGA as the last argument.
>> OK, and which devices need to do that?
>
> Those that care to optimize their update_display callback.  It's
> entirely opt in.

It also appears to be entirely undocumented :-)
The reason I'm asking is that the a lot of the bulk of QEMU is
device models, which are often written by people without a deep
understanding of QEMU's APIs and then reviewed by people who
may not be familiar with all the subsystem details either. So
APIs which are required to be used by devices are more in need
of documentation and assertion-sanity-checks on their use than
APIs which are only used by core code or even by tcg frontends.

For instance, up until this thread I'd assumed that DIRTY_MEMORY_VGA
was pretty much what the name implies, something that's only
of interest to the PC VGA card emulation. So it's very hard
for me to review something that's relating to an API that's
not one I know of and which isn't documented either.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 24/29] bitmap: add atomic test and clear

[Fam Zheng]

On Mon, 04/27 18:28, Paolo Bonzini wrote:
> From: Stefan Hajnoczi <stefanha@redhat.com>
> 
> The new bitmap_test_and_clear_atomic() function clears a range and
> returns whether or not the bits were set.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Message-Id: <1417519399-3166-3-git-send-email-stefanha@redhat.com>
> [Test before xchg; then a full barrier is needed at the end just like
>  in the previous patch.  The barrier can be avoided if we did at least
>  one xchg.  - Paolo]
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/qemu/bitmap.h |  2 ++
>  util/bitmap.c         | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/include/qemu/bitmap.h b/include/qemu/bitmap.h
> index 3e0a4f3..86dd9cd 100644
> --- a/include/qemu/bitmap.h
> +++ b/include/qemu/bitmap.h
> @@ -41,6 +41,7 @@
>   * bitmap_set(dst, pos, nbits)			Set specified bit area
>   * bitmap_set_atomic(dst, pos, nbits)   Set specified bit area with atomic ops
>   * bitmap_clear(dst, pos, nbits)		Clear specified bit area
> + * bitmap_test_and_clear_atomic(dst, pos, nbits)    Test and clear area
>   * bitmap_find_next_zero_area(buf, len, pos, n, mask)	Find bit free area
>   */
>  
> @@ -229,6 +230,7 @@ static inline int bitmap_intersects(const unsigned long *src1,
>  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);
>  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 6838d49..d3e088d 100644
> --- a/util/bitmap.c
> +++ b/util/bitmap.c
> @@ -234,6 +234,50 @@ void bitmap_clear(unsigned long *map, long start, long nr)
>      }
>  }
>  
> +bool bitmap_test_and_clear_atomic(unsigned long *map, long start, long nr)
> +{
> +    unsigned long *p = map + BIT_WORD(start);
> +    const long size = start + nr;
> +    int bits_to_clear = BITS_PER_LONG - (start % BITS_PER_LONG);
> +    unsigned long mask_to_clear = BITMAP_FIRST_WORD_MASK(start);
> +    unsigned long dirty = 0;
> +    unsigned long old_bits;
> +
> +    /* First word */
> +    if (nr - bits_to_clear > 0) {
> +        old_bits = atomic_fetch_and(p, ~mask_to_clear);
> +        dirty |= old_bits & mask_to_clear;
> +        nr -= bits_to_clear;
> +        bits_to_clear = BITS_PER_LONG;
> +        mask_to_clear = ~0UL;
> +        p++;
> +    }
> +

It's broken if the clear range is the last part of first word:

map = 0xFFFFFFFFFFFFFFFF, ...

bitmap_test_and_clear_atomic(map, 60, 4):

Expected result:

   0xFFFFFFFFFFFFFFFF, ...

Actual result:

   0x0

Fam

Re: [Qemu-devel] [PATCH 25/29] memory: use atomic ops for setting dirty memory bits

[Fam Zheng]

On Mon, 04/27 18:28, Paolo Bonzini wrote:
> From: Stefan Hajnoczi <stefanha@redhat.com>
> 
> Use set_bit_atomic() and bitmap_set_atomic() so that multiple threads
> can dirty memory without race conditions.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Message-Id: <1417519399-3166-4-git-send-email-stefanha@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH 24/29] bitmap: add atomic test and clear

[Fam Zheng]

On Tue, 05/26 20:27, Fam Zheng wrote:
> On Mon, 04/27 18:28, Paolo Bonzini wrote:
> > From: Stefan Hajnoczi <stefanha@redhat.com>
> > 
> > The new bitmap_test_and_clear_atomic() function clears a range and
> > returns whether or not the bits were set.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > Message-Id: <1417519399-3166-3-git-send-email-stefanha@redhat.com>
> > [Test before xchg; then a full barrier is needed at the end just like
> >  in the previous patch.  The barrier can be avoided if we did at least
> >  one xchg.  - Paolo]
> > 
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  include/qemu/bitmap.h |  2 ++
> >  util/bitmap.c         | 44 ++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 46 insertions(+)
> > 
> > diff --git a/include/qemu/bitmap.h b/include/qemu/bitmap.h
> > index 3e0a4f3..86dd9cd 100644
> > --- a/include/qemu/bitmap.h
> > +++ b/include/qemu/bitmap.h
> > @@ -41,6 +41,7 @@
> >   * bitmap_set(dst, pos, nbits)			Set specified bit area
> >   * bitmap_set_atomic(dst, pos, nbits)   Set specified bit area with atomic ops
> >   * bitmap_clear(dst, pos, nbits)		Clear specified bit area
> > + * bitmap_test_and_clear_atomic(dst, pos, nbits)    Test and clear area
> >   * bitmap_find_next_zero_area(buf, len, pos, n, mask)	Find bit free area
> >   */
> >  
> > @@ -229,6 +230,7 @@ static inline int bitmap_intersects(const unsigned long *src1,
> >  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);
> >  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 6838d49..d3e088d 100644
> > --- a/util/bitmap.c
> > +++ b/util/bitmap.c
> > @@ -234,6 +234,50 @@ void bitmap_clear(unsigned long *map, long start, long nr)
> >      }
> >  }
> >  
> > +bool bitmap_test_and_clear_atomic(unsigned long *map, long start, long nr)
> > +{
> > +    unsigned long *p = map + BIT_WORD(start);
> > +    const long size = start + nr;
> > +    int bits_to_clear = BITS_PER_LONG - (start % BITS_PER_LONG);
> > +    unsigned long mask_to_clear = BITMAP_FIRST_WORD_MASK(start);
> > +    unsigned long dirty = 0;
> > +    unsigned long old_bits;
> > +
> > +    /* First word */
> > +    if (nr - bits_to_clear > 0) {
> > +        old_bits = atomic_fetch_and(p, ~mask_to_clear);
> > +        dirty |= old_bits & mask_to_clear;
> > +        nr -= bits_to_clear;
> > +        bits_to_clear = BITS_PER_LONG;
> > +        mask_to_clear = ~0UL;
> > +        p++;
> > +    }
> > +
> 
> It's broken if the clear range is the last part of first word:
> 
> map =        0xFFFFFFFFFFFFFFFF, ...
> 
> bitmap_test_and_clear_atomic(map, 60, 4):
> 
> Expected result:
> 
>              0xFFFFFFFFFFFFFFFF, ...

Sorry, I meant 0xFFFFFFFFFFFFFFF (the highest 4 bits being cleared).

> 
> Actual result:
> 
>    0x0
> 
> Fam
> 

Re: [Qemu-devel] [PATCH 24/29] bitmap: add atomic test and clear

[Paolo Bonzini]

On 26/05/2015 14:27, Fam Zheng wrote:
> It's broken if the clear range is the last part of first word:
> 
> map = 0xFFFFFFFFFFFFFFFF, ...
> 
> bitmap_test_and_clear_atomic(map, 60, 4):
> 
> Expected result:
> 
>    0xFFFFFFFFFFFFFFFF, ...
> 
> Actual result:
> 
>    0x0

You're right, this:


    /* Full words */
    while (nr - bits_to_clear >= 0) {

must be surrounded by an "if (mask_to_clear == ~0UL)" or "if
(bits_to_clear == BITS_PER_LONG)."  Patch 23 has the same problem.

Paolo

Re: [Qemu-devel] [PATCH 26/29] migration: move dirty bitmap sync to ram_addr.h

[Fam Zheng]

On Mon, 04/27 18:28, Paolo Bonzini wrote:
> From: Stefan Hajnoczi <stefanha@redhat.com>
> 
> The dirty memory bitmap is managed by ram_addr.h and copied to
> migration_bitmap[] periodically during live migration.
> 
> Move the code to sync the bitmap to ram_addr.h where related code lives.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Message-Id: <1417519399-3166-5-git-send-email-stefanha@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch_init.c             | 46 ++--------------------------------------------
>  include/exec/ram_addr.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 46 insertions(+), 44 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 4c8fcee..ec86173 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -467,52 +467,10 @@ ram_addr_t migration_bitmap_find_and_reset_dirty(MemoryRegion *mr,
>      return (next - base) << TARGET_PAGE_BITS;
>  }
>  
> -static inline bool migration_bitmap_set_dirty(ram_addr_t addr)
> -{
> -    bool ret;
> -    int nr = addr >> TARGET_PAGE_BITS;
> -
> -    ret = test_and_set_bit(nr, migration_bitmap);
> -
> -    if (!ret) {
> -        migration_dirty_pages++;
> -    }
> -    return ret;
> -}
> -
>  static void migration_bitmap_sync_range(ram_addr_t start, ram_addr_t length)
>  {
> -    ram_addr_t addr;
> -    unsigned long page = BIT_WORD(start >> TARGET_PAGE_BITS);
> -
> -    /* start address is aligned at the start of a word? */
> -    if (((page * BITS_PER_LONG) << TARGET_PAGE_BITS) == start) {
> -        int k;
> -        int nr = BITS_TO_LONGS(length >> TARGET_PAGE_BITS);
> -        unsigned long *src = ram_list.dirty_memory[DIRTY_MEMORY_MIGRATION];
> -
> -        for (k = page; k < page + nr; k++) {
> -            if (src[k]) {
> -                unsigned long new_dirty;
> -                new_dirty = ~migration_bitmap[k];
> -                migration_bitmap[k] |= src[k];
> -                new_dirty &= src[k];
> -                migration_dirty_pages += ctpopl(new_dirty);
> -                src[k] = 0;
> -            }
> -        }
> -    } else {
> -        for (addr = 0; addr < length; addr += TARGET_PAGE_SIZE) {
> -            if (cpu_physical_memory_get_dirty(start + addr,
> -                                              TARGET_PAGE_SIZE,
> -                                              DIRTY_MEMORY_MIGRATION)) {
> -                cpu_physical_memory_reset_dirty(start + addr,
> -                                                TARGET_PAGE_SIZE,
> -                                                DIRTY_MEMORY_MIGRATION);
> -                migration_bitmap_set_dirty(start + addr);
> -            }
> -        }
> -    }
> +    migration_dirty_pages +=
> +        cpu_physical_memory_sync_dirty_bitmap(migration_bitmap, start, length);
>  }
>  
>  
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index bb69a28..9bd7827 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -218,5 +218,49 @@ static inline void cpu_physical_memory_clear_dirty_range(ram_addr_t start,
>  void cpu_physical_memory_reset_dirty(ram_addr_t start, ram_addr_t length,
>                                       unsigned client);
>  
> +static inline
> +uint64_t cpu_physical_memory_sync_dirty_bitmap(unsigned long *dest,
> +                                               ram_addr_t start,
> +                                               ram_addr_t length)
> +{
> +    ram_addr_t addr;
> +    unsigned long page = BIT_WORD(start >> TARGET_PAGE_BITS);
> +    uint64_t num_dirty = 0;
> +
> +    /* start address is aligned at the start of a word? */
> +    if (((page * BITS_PER_LONG) << TARGET_PAGE_BITS) == start) {
> +        int k;
> +        int nr = BITS_TO_LONGS(length >> TARGET_PAGE_BITS);
> +        unsigned long *src = ram_list.dirty_memory[DIRTY_MEMORY_MIGRATION];
> +
> +        for (k = page; k < page + nr; k++) {
> +            if (src[k]) {
> +                unsigned long new_dirty;
> +                new_dirty = ~dest[k];
> +                dest[k] |= src[k];
> +                new_dirty &= src[k];
> +                num_dirty += ctpopl(new_dirty);
> +                src[k] = 0;
> +            }
> +        }
> +    } else {
> +        for (addr = 0; addr < length; addr += TARGET_PAGE_SIZE) {
> +            if (cpu_physical_memory_get_dirty(start + addr,
> +                                              TARGET_PAGE_SIZE,
> +                                              DIRTY_MEMORY_MIGRATION)) {
> +                cpu_physical_memory_reset_dirty(start + addr,
> +                                                TARGET_PAGE_SIZE,
> +                                                DIRTY_MEMORY_MIGRATION);
> +                long k = (start + addr) >> TARGET_PAGE_BITS;

The variable declaration should come first in a code block.

Fam

> +                if (!test_and_set_bit(k, dest)) {
> +                    num_dirty++;
> +                }
> +            }
> +        }
> +    }
> +
> +    return num_dirty;
> +}
> +
>  #endif
>  #endif
> -- 
> 1.8.3.1
> 
> 

Re: [Qemu-devel] [PATCH 23/29] bitmap: add atomic set functions

[Fam Zheng]

On Tue, 05/26 19:54, Fam Zheng wrote:
> On Mon, 04/27 18:28, Paolo Bonzini wrote:
> > From: Stefan Hajnoczi <stefanha@redhat.com>
> > 
> > Use atomic_or() for atomic bitmaps where several threads may set bits at
> > the same time.  This avoids the race condition between threads loading
> > an element, bitwise ORing, and then storing the element.
> > 
> > When setting all bits in a word we can avoid atomic ops and instead just
> > use an smp_mb() at the end.
> > 
> > Most bitmap users don't need atomicity so introduce new functions.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > Message-Id: <1417519399-3166-2-git-send-email-stefanha@redhat.com>
> > [Avoid barrier in the single word case, use full barrier instead of write.
> >  - Paolo]
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  include/qemu/bitmap.h |  2 ++
> >  include/qemu/bitops.h | 14 ++++++++++++++
> >  util/bitmap.c         | 37 +++++++++++++++++++++++++++++++++++++
> >  3 files changed, 53 insertions(+)
> > 
> > diff --git a/include/qemu/bitmap.h b/include/qemu/bitmap.h
> > index f0273c9..3e0a4f3 100644
> > --- a/include/qemu/bitmap.h
> > +++ b/include/qemu/bitmap.h
> > @@ -39,6 +39,7 @@
> >   * bitmap_empty(src, nbits)			Are all bits zero in *src?
> >   * bitmap_full(src, nbits)			Are all bits set in *src?
> >   * bitmap_set(dst, pos, nbits)			Set specified bit area
> > + * bitmap_set_atomic(dst, pos, nbits)   Set specified bit area with atomic ops
> >   * bitmap_clear(dst, pos, nbits)		Clear specified bit area
> >   * bitmap_find_next_zero_area(buf, len, pos, n, mask)	Find bit free area
> >   */
> > @@ -226,6 +227,7 @@ static inline int bitmap_intersects(const unsigned long *src1,
> >  }
> >  
> >  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);
> >  unsigned long bitmap_find_next_zero_area(unsigned long *map,
> >                                           unsigned long size,
> > diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
> > index 90ca8df..95f98fb 100644
> > --- a/include/qemu/bitops.h
> > +++ b/include/qemu/bitops.h
> > @@ -16,6 +16,7 @@
> >  #include <assert.h>
> >  
> >  #include "host-utils.h"
> > +#include "atomic.h"
> >  
> >  #define BITS_PER_BYTE           CHAR_BIT
> >  #define BITS_PER_LONG           (sizeof (unsigned long) * BITS_PER_BYTE)
> > @@ -39,6 +40,19 @@ static inline void set_bit(long nr, unsigned long *addr)
> >  }
> >  
> >  /**
> > + * set_bit_atomic - Set a bit in memory atomically
> > + * @nr: the bit to set
> > + * @addr: the address to start counting from
> > + */
> > +static inline void set_bit_atomic(long nr, unsigned long *addr)
> > +{
> > +    unsigned long mask = BIT_MASK(nr);
> > +    unsigned long *p = addr + BIT_WORD(nr);
> > +
> > +    atomic_or(p, mask);
> > +}
> > +
> > +/**
> >   * clear_bit - Clears a bit in memory
> >   * @nr: Bit to clear
> >   * @addr: Address to start counting from
> > diff --git a/util/bitmap.c b/util/bitmap.c
> > index 9c6bb52..6838d49 100644
> > --- a/util/bitmap.c
> > +++ b/util/bitmap.c
> > @@ -11,6 +11,7 @@
> >  
> >  #include "qemu/bitops.h"
> >  #include "qemu/bitmap.h"
> > +#include "qemu/atomic.h"
> >  
> >  /*
> >   * bitmaps provide an array of bits, implemented using an an
> > @@ -177,6 +178,42 @@ void bitmap_set(unsigned long *map, long start, long nr)
> >      }
> >  }
> >  
> > +void bitmap_set_atomic(unsigned long *map, long start, long nr)
> > +{
> > +    unsigned long *p = map + BIT_WORD(start);
> > +    const long size = start + nr;
> 
> s/size/end/ ?
> 
> Otherwise,
> 
> Reviewed-by: Fam Zheng <famz@redhat.com>

NACK

It's broken the same way as bitmap_test_and_clear_atomic. See my reply to patch
24.

Fam

> 
> > +    int bits_to_set = BITS_PER_LONG - (start % BITS_PER_LONG);
> > +    unsigned long mask_to_set = BITMAP_FIRST_WORD_MASK(start);
> > +
> > +    /* First word */
> > +    if (nr - bits_to_set > 0) {
> > +        atomic_or(p, mask_to_set);
> > +        nr -= bits_to_set;
> > +        bits_to_set = BITS_PER_LONG;
> > +        mask_to_set = ~0UL;
> > +        p++;
> > +    }
> > +
> > +    /* Full words */
> > +    while (nr - bits_to_set >= 0) {
> > +        *p = ~0UL;
> > +        nr -= bits_to_set;
> > +        mask_to_set = ~0UL;
> > +        p++;
> > +    }
> 
> For full words, will memset be faster by any chance?
> 
> > +
> > +    /* Last word */
> > +    if (nr) {
> > +        mask_to_set &= BITMAP_LAST_WORD_MASK(size);
> > +        atomic_or(p, mask_to_set);
> > +    } else {
> > +        /* If we avoided the full barrier in atomic_or(), issue a
> > +         * barrier to account for the assignments in the while loop.
> > +         */
> > +        smp_mb();
> > +    }
> > +}
> > +
> >  void bitmap_clear(unsigned long *map, long start, long nr)
> >  {
> >      unsigned long *p = map + BIT_WORD(start);
> > -- 
> > 1.8.3.1
> > 
> > 
> 

Re: [Qemu-devel] [PATCH 27/29] memory: replace cpu_physical_memory_reset_dirty() with test-and-clear

[Fam Zheng]

On Mon, 04/27 18:28, Paolo Bonzini wrote:
> From: Stefan Hajnoczi <stefanha@redhat.com>
> 
> The cpu_physical_memory_reset_dirty() function is sometimes used
> together with cpu_physical_memory_get_dirty().  This is not atomic since
> two separate accesses to the dirty memory bitmap are made.
> 
> Turn cpu_physical_memory_reset_dirty() and
> cpu_physical_memory_clear_dirty_range_type() into the atomic
> cpu_physical_memory_test_and_clear_dirty().
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Message-Id: <1417519399-3166-6-git-send-email-stefanha@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  cputlb.c                |  4 ++--
>  exec.c                  | 23 +++++++++++++++++------
>  include/exec/ram_addr.h | 33 ++++++++++-----------------------
>  memory.c                | 11 ++++-------
>  4 files changed, 33 insertions(+), 38 deletions(-)
> 
> diff --git a/cputlb.c b/cputlb.c
> index a86e76f..75b2dcf 100644
> --- a/cputlb.c
> +++ b/cputlb.c
> @@ -125,8 +125,8 @@ void tlb_flush_page(CPUState *cpu, target_ulong addr)
>     can be detected */
>  void tlb_protect_code(ram_addr_t ram_addr)
>  {
> -    cpu_physical_memory_reset_dirty(ram_addr, TARGET_PAGE_SIZE,
> -                                    DIRTY_MEMORY_CODE);
> +    cpu_physical_memory_test_and_clear_dirty(ram_addr, TARGET_PAGE_SIZE,
> +                                             DIRTY_MEMORY_CODE);
>  }
>  
>  /* update the TLB so that writes in physical page 'phys_addr' are no longer
> diff --git a/exec.c b/exec.c
> index b7aebbf..daed5a7 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -857,16 +857,27 @@ static void tlb_reset_dirty_range_all(ram_addr_t start, ram_addr_t length)
>  }
>  
>  /* Note: start and end must be within the same ram block.  */
> -void cpu_physical_memory_reset_dirty(ram_addr_t start, ram_addr_t length,
> -                                     unsigned client)
> +bool cpu_physical_memory_test_and_clear_dirty(ram_addr_t start,
> +                                              ram_addr_t length,
> +                                              unsigned client)
>  {
> -    if (length == 0)
> -        return;
> -    cpu_physical_memory_clear_dirty_range_type(start, length, client);
> +    unsigned long end, page;
> +    bool dirty;
> +
> +    if (length == 0) {
> +        return false;
> +    }
>  
> -    if (tcg_enabled()) {
> +    end = TARGET_PAGE_ALIGN(start + length) >> TARGET_PAGE_BITS;
> +    page = start >> TARGET_PAGE_BITS;
> +    dirty = bitmap_test_and_clear_atomic(ram_list.dirty_memory[client],
> +                                         page, end - page);
> +
> +    if (dirty && tcg_enabled()) {
>          tlb_reset_dirty_range_all(start, length);
>      }
> +
> +    return dirty;
>  }
>  
>  /* Called from RCU critical section */
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index 9bd7827..ea77187 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -194,30 +194,19 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>  }
>  #endif /* not _WIN32 */
>  
> -static inline void cpu_physical_memory_clear_dirty_range_type(ram_addr_t start,
> -                                                              ram_addr_t length,
> -                                                              unsigned client)
> -{
> -    unsigned long end, page;
> -
> -    assert(client < DIRTY_MEMORY_NUM);
> -    end = TARGET_PAGE_ALIGN(start + length) >> TARGET_PAGE_BITS;
> -    page = start >> TARGET_PAGE_BITS;
> -    bitmap_clear(ram_list.dirty_memory[client], page, end - page);
> -}
> +bool cpu_physical_memory_test_and_clear_dirty(ram_addr_t start,
> +                                              ram_addr_t length,
> +                                              unsigned client);
>  
>  static inline void cpu_physical_memory_clear_dirty_range(ram_addr_t start,
>                                                           ram_addr_t length)
>  {
> -    cpu_physical_memory_clear_dirty_range_type(start, length, DIRTY_MEMORY_MIGRATION);
> -    cpu_physical_memory_clear_dirty_range_type(start, length, DIRTY_MEMORY_VGA);
> -    cpu_physical_memory_clear_dirty_range_type(start, length, DIRTY_MEMORY_CODE);
> +    cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_MIGRATION);

Is this line too long? Otherwise,

Reviewed-by: Fam Zheng <famz@redhat.com>

> +    cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_VGA);
> +    cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_CODE);
>  }
>  
>  
> -void cpu_physical_memory_reset_dirty(ram_addr_t start, ram_addr_t length,
> -                                     unsigned client);
> -
>  static inline
>  uint64_t cpu_physical_memory_sync_dirty_bitmap(unsigned long *dest,
>                                                 ram_addr_t start,
> @@ -245,12 +234,10 @@ uint64_t cpu_physical_memory_sync_dirty_bitmap(unsigned long *dest,
>          }
>      } else {
>          for (addr = 0; addr < length; addr += TARGET_PAGE_SIZE) {
> -            if (cpu_physical_memory_get_dirty(start + addr,
> -                                              TARGET_PAGE_SIZE,
> -                                              DIRTY_MEMORY_MIGRATION)) {
> -                cpu_physical_memory_reset_dirty(start + addr,
> -                                                TARGET_PAGE_SIZE,
> -                                                DIRTY_MEMORY_MIGRATION);
> +            if (cpu_physical_memory_test_and_clear_dirty(
> +                        start + addr,
> +                        TARGET_PAGE_SIZE,
> +                        DIRTY_MEMORY_MIGRATION)) {
>                  long k = (start + addr) >> TARGET_PAGE_BITS;
>                  if (!test_and_set_bit(k, dest)) {
>                      num_dirty++;
> diff --git a/memory.c b/memory.c
> index 7422790..bb86b4b 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1397,13 +1397,9 @@ void memory_region_set_dirty(MemoryRegion *mr, hwaddr addr,
>  bool memory_region_test_and_clear_dirty(MemoryRegion *mr, hwaddr addr,
>                                          hwaddr size, unsigned client)
>  {
> -    bool ret;
>      assert(mr->terminates);
> -    ret = cpu_physical_memory_get_dirty(mr->ram_addr + addr, size, client);
> -    if (ret) {
> -        cpu_physical_memory_reset_dirty(mr->ram_addr + addr, size, client);
> -    }
> -    return ret;
> +    return cpu_physical_memory_test_and_clear_dirty(mr->ram_addr + addr,
> +                                                    size, client);
>  }
>  
>  
> @@ -1447,7 +1443,8 @@ void memory_region_reset_dirty(MemoryRegion *mr, hwaddr addr,
>                                 hwaddr size, unsigned client)
>  {
>      assert(mr->terminates);
> -    cpu_physical_memory_reset_dirty(mr->ram_addr + addr, size, client);
> +    cpu_physical_memory_test_and_clear_dirty(mr->ram_addr + addr, size,
> +                                             client);
>  }
>  
>  int memory_region_get_fd(MemoryRegion *mr)
> -- 
> 1.8.3.1
> 
> 
> 

Re: [Qemu-devel] [PATCH 28/29] memory: make cpu_physical_memory_sync_dirty_bitmap() fully atomic

[Fam Zheng]

On Mon, 04/27 18:28, Paolo Bonzini wrote:
> From: Stefan Hajnoczi <stefanha@redhat.com>
> 
> The fast path of cpu_physical_memory_sync_dirty_bitmap() directly
> manipulates the dirty bitmap.  Use atomic_xchg() to make the
> test-and-clear atomic.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Message-Id: <1417519399-3166-7-git-send-email-stefanha@redhat.com>
> [Only do xchg on nonzero words. - Paolo]
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

> ---
>  include/exec/ram_addr.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index ea77187..0baba41 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -224,12 +224,12 @@ uint64_t cpu_physical_memory_sync_dirty_bitmap(unsigned long *dest,
>  
>          for (k = page; k < page + nr; k++) {
>              if (src[k]) {
> +                unsigned long bits = atomic_xchg(&src[k], 0);
>                  unsigned long new_dirty;
>                  new_dirty = ~dest[k];
> -                dest[k] |= src[k];
> -                new_dirty &= src[k];
> +                dest[k] |= bits;
> +                new_dirty &= bits;
>                  num_dirty += ctpopl(new_dirty);
> -                src[k] = 0;
>              }
>          }
>      } else {
> -- 
> 1.8.3.1
> 
> 

Re: [Qemu-devel] [PATCH 27/29] memory: replace cpu_physical_memory_reset_dirty() with test-and-clear

[Paolo Bonzini]

On 26/05/2015 14:42, Fam Zheng wrote:
>> >  {
>> > -    cpu_physical_memory_clear_dirty_range_type(start, length, DIRTY_MEMORY_MIGRATION);
>> > -    cpu_physical_memory_clear_dirty_range_type(start, length, DIRTY_MEMORY_VGA);
>> > -    cpu_physical_memory_clear_dirty_range_type(start, length, DIRTY_MEMORY_CODE);
>> > +    cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_MIGRATION);
> Is this line too long? Otherwise,

Yes, it's 84 characters long.  On the other hand, it is shorter than the
existing line...

Paolo

> Reviewed-by: Fam Zheng <famz@redhat.com>
> 
>> > +    cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_VGA);
>> > +    cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_CODE);
>> >  }
>> >  

Re: [Qemu-devel] [PATCH 29/29] memory: strengthen assertions on mr->terminates

[Fam Zheng]

On Mon, 04/27 18:28, Paolo Bonzini wrote:
> mr->terminates alone doesn't guarantee that we are looking at a RAM region.
> mr->ram_addr also has to be checked, in order to distinguish RAM and I/O
> regions.
> 
> IOMMU regions were not setting mr->ram_addr to a bogus value, do it now
> so that the assertions would fire for IOMMU regions as well.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

It would be nicer to introduce a memory_region_is_ram(MemoryRegion *mr), the
~(ram_addr_t) duplications are too many.

Fam

> ---
>  memory.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/memory.c b/memory.c
> index bb86b4b..82d9df6 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1242,6 +1242,7 @@ void memory_region_init_iommu(MemoryRegion *mr,
>      memory_region_init(mr, owner, name, size);
>      mr->iommu_ops = ops,
>      mr->terminates = true;  /* then re-forwards */
> +    mr->ram_addr = ~(ram_addr_t)0;
>      notifier_list_init(&mr->iommu_notify);
>  }
>  
> @@ -1382,14 +1383,14 @@ void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
>  bool memory_region_get_dirty(MemoryRegion *mr, hwaddr addr,
>                               hwaddr size, unsigned client)
>  {
> -    assert(mr->terminates);
> +    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
>      return cpu_physical_memory_get_dirty(mr->ram_addr + addr, size, client);
>  }
>  
>  void memory_region_set_dirty(MemoryRegion *mr, hwaddr addr,
>                               hwaddr size)
>  {
> -    assert(mr->terminates);
> +    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
>      cpu_physical_memory_set_dirty_range(mr->ram_addr + addr, size,
>                                          memory_region_get_dirty_log_mask(mr));
>  }
> @@ -1397,7 +1398,7 @@ void memory_region_set_dirty(MemoryRegion *mr, hwaddr addr,
>  bool memory_region_test_and_clear_dirty(MemoryRegion *mr, hwaddr addr,
>                                          hwaddr size, unsigned client)
>  {
> -    assert(mr->terminates);
> +    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
>      return cpu_physical_memory_test_and_clear_dirty(mr->ram_addr + addr,
>                                                      size, client);
>  }
> @@ -1442,7 +1443,7 @@ void memory_region_rom_device_set_romd(MemoryRegion *mr, bool romd_mode)
>  void memory_region_reset_dirty(MemoryRegion *mr, hwaddr addr,
>                                 hwaddr size, unsigned client)
>  {
> -    assert(mr->terminates);
> +    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
>      cpu_physical_memory_test_and_clear_dirty(mr->ram_addr + addr, size,
>                                               client);
>  }
> @@ -1453,7 +1454,7 @@ int memory_region_get_fd(MemoryRegion *mr)
>          return memory_region_get_fd(mr->alias);
>      }
>  
> -    assert(mr->terminates);
> +    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
>  
>      return qemu_get_ram_fd(mr->ram_addr & TARGET_PAGE_MASK);
>  }
> @@ -1464,14 +1465,14 @@ void *memory_region_get_ram_ptr(MemoryRegion *mr)
>          return memory_region_get_ram_ptr(mr->alias) + mr->alias_offset;
>      }
>  
> -    assert(mr->terminates);
> +    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
>  
>      return qemu_get_ram_ptr(mr->ram_addr & TARGET_PAGE_MASK);
>  }
>  
>  void memory_region_ram_resize(MemoryRegion *mr, ram_addr_t newsize, Error **errp)
>  {
> -    assert(mr->terminates);
> +    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
>  
>      qemu_ram_resize(mr->ram_addr, newsize, errp);
>  }
> -- 
> 1.8.3.1
> 

Re: [Qemu-devel] [PATCH 29/29] memory: strengthen assertions on mr->terminates

[Paolo Bonzini]

On 26/05/2015 14:49, Fam Zheng wrote:
> It would be nicer to introduce a memory_region_is_ram(MemoryRegion *mr), the
> ~(ram_addr_t) duplications are too many.

Is RAM_ADDR_INVALID okay too?

Paolo

> Fam
> 
>> ---
>>  memory.c | 15 ++++++++-------
>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/memory.c b/memory.c
>> index bb86b4b..82d9df6 100644
>> --- a/memory.c
>> +++ b/memory.c
>> @@ -1242,6 +1242,7 @@ void memory_region_init_iommu(MemoryRegion *mr,
>>      memory_region_init(mr, owner, name, size);
>>      mr->iommu_ops = ops,
>>      mr->terminates = true;  /* then re-forwards */
>> +    mr->ram_addr = ~(ram_addr_t)0;
>>      notifier_list_init(&mr->iommu_notify);
>>  }
>>  
>> @@ -1382,14 +1383,14 @@ void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
>>  bool memory_region_get_dirty(MemoryRegion *mr, hwaddr addr,
>>                               hwaddr size, unsigned client)
>>  {
>> -    assert(mr->terminates);
>> +    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
>>      return cpu_physical_memory_get_dirty(mr->ram_addr + addr, size, client);
>>  }
>>  
>>  void memory_region_set_dirty(MemoryRegion *mr, hwaddr addr,
>>                               hwaddr size)
>>  {
>> -    assert(mr->terminates);
>> +    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
>>      cpu_physical_memory_set_dirty_range(mr->ram_addr + addr, size,
>>                                          memory_region_get_dirty_log_mask(mr));
>>  }
>> @@ -1397,7 +1398,7 @@ void memory_region_set_dirty(MemoryRegion *mr, hwaddr addr,
>>  bool memory_region_test_and_clear_dirty(MemoryRegion *mr, hwaddr addr,
>>                                          hwaddr size, unsigned client)
>>  {
>> -    assert(mr->terminates);
>> +    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
>>      return cpu_physical_memory_test_and_clear_dirty(mr->ram_addr + addr,
>>                                                      size, client);
>>  }
>> @@ -1442,7 +1443,7 @@ void memory_region_rom_device_set_romd(MemoryRegion *mr, bool romd_mode)
>>  void memory_region_reset_dirty(MemoryRegion *mr, hwaddr addr,
>>                                 hwaddr size, unsigned client)
>>  {
>> -    assert(mr->terminates);
>> +    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
>>      cpu_physical_memory_test_and_clear_dirty(mr->ram_addr + addr, size,
>>                                               client);
>>  }
>> @@ -1453,7 +1454,7 @@ int memory_region_get_fd(MemoryRegion *mr)
>>          return memory_region_get_fd(mr->alias);
>>      }
>>  
>> -    assert(mr->terminates);
>> +    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
>>  
>>      return qemu_get_ram_fd(mr->ram_addr & TARGET_PAGE_MASK);
>>  }
>> @@ -1464,14 +1465,14 @@ void *memory_region_get_ram_ptr(MemoryRegion *mr)
>>          return memory_region_get_ram_ptr(mr->alias) + mr->alias_offset;
>>      }
>>  
>> -    assert(mr->terminates);
>> +    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
>>  
>>      return qemu_get_ram_ptr(mr->ram_addr & TARGET_PAGE_MASK);
>>  }
>>  
>>  void memory_region_ram_resize(MemoryRegion *mr, ram_addr_t newsize, Error **errp)
>>  {
>> -    assert(mr->terminates);
>> +    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
>>  
>>      qemu_ram_resize(mr->ram_addr, newsize, errp);
>>  }
>> -- 
>> 1.8.3.1
>>

Re: [Qemu-devel] [PATCH 13/29] ram_addr: tweaks to xen_modified_memory

[Fam Zheng]

On Mon, 04/27 18:28, Paolo Bonzini wrote:
> Invoke xen_modified_memory from cpu_physical_memory_set_dirty_range_nocode;
> it is akin to DIRTY_MEMORY_MIGRATION, so set it together with that bitmap.
> The remaining call from invalidate_and_set_dirty's "else" branch will go
> away soon.
> 
> Second, fix the second argument to the function in the
> cpu_physical_memory_set_dirty_lebitmap call site.  That function is only used
> by KVM, but it is better to be clean anyway.
> 
> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

> ---
>  exec.c                  | 3 ++-
>  include/exec/ram_addr.h | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 4717928..083d65d 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2260,8 +2260,9 @@ static void invalidate_and_set_dirty(hwaddr addr,
>      if (cpu_physical_memory_range_includes_clean(addr, length)) {
>          tb_invalidate_phys_range(addr, addr + length, 0);
>          cpu_physical_memory_set_dirty_range_nocode(addr, length);
> +    } else {
> +        xen_modified_memory(addr, length);
>      }
> -    xen_modified_memory(addr, length);
>  }
>  
>  static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index ff558a4..7f6e928 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -112,6 +112,7 @@ static inline void cpu_physical_memory_set_dirty_range_nocode(ram_addr_t start,
>      page = start >> TARGET_PAGE_BITS;
>      bitmap_set(ram_list.dirty_memory[DIRTY_MEMORY_MIGRATION], page, end - page);
>      bitmap_set(ram_list.dirty_memory[DIRTY_MEMORY_VGA], page, end - page);
> +    xen_modified_memory(start, length);
>  }
>  
>  static inline void cpu_physical_memory_set_dirty_range(ram_addr_t start,
> @@ -155,7 +156,7 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>                  ram_list.dirty_memory[DIRTY_MEMORY_CODE][page + k] |= temp;
>              }
>          }
> -        xen_modified_memory(start, pages);
> +        xen_modified_memory(start, pages << TARGET_PAGE_BITS);
>      } else {
>          /*
>           * bitmap-traveling is faster than memory-traveling (for addr...)
> -- 
> 1.8.3.1
> 
> 

Re: [Qemu-devel] [PATCH 02/29] g364fb: remove pointless call to memory_region_set_coalescing

[Fam Zheng]

On Mon, 04/27 18:28, Paolo Bonzini wrote:
> Coalescing work on MMIO, not RAM, thus this call has no effect.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

> ---
>  hw/display/g364fb.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/display/g364fb.c b/hw/display/g364fb.c
> index 46f7b41..6543f2f 100644
> --- a/hw/display/g364fb.c
> +++ b/hw/display/g364fb.c
> @@ -489,7 +489,6 @@ static void g364fb_init(DeviceState *dev, G364State *s)
>      memory_region_init_ram_ptr(&s->mem_vram, NULL, "vram",
>                                 s->vram_size, s->vram);
>      vmstate_register_ram(&s->mem_vram, dev);
> -    memory_region_set_coalescing(&s->mem_vram);
>  }
>  
>  #define TYPE_G364 "sysbus-g364"
> -- 
> 1.8.3.1
> 
> 

Re: [Qemu-devel] [PATCH 29/29] memory: strengthen assertions on mr->terminates

[Fam Zheng]

On Tue, 05/26 14:52, Paolo Bonzini wrote:
> 
> 
> On 26/05/2015 14:49, Fam Zheng wrote:
> > It would be nicer to introduce a memory_region_is_ram(MemoryRegion *mr), the
> > ~(ram_addr_t) duplications are too many.
> 
> Is RAM_ADDR_INVALID okay too?

I think so. If you replace every ~(ram_addr_t)0 with RAM_ADDR_INVALID, you can
add my

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v2 00/29] Dirty bitmap atomic access and optimizations

[Fam Zheng]

On Mon, 04/27 18:28, Paolo Bonzini wrote:
> Patches 1-4 are cleanups to DIRTY_MEMORY_VGA users.
> 
> Patches 5-12 are the first cleanup (KVM treats migration as just
> another client).  Patches 13-14 are a simple optimization that is enabled
> by these patches.
> 
> Patches 15-18 are bonus cleanups to translate-all.c's dirty memory
> tracking for TCG.
> 
> Patches 19-22 are the second cleanup (avoid modifications to unused
> dirty bitmaps).
> 
> Patches 23-28 are Stefan's patches for atomic access to the dirty
> bitmap, which has no performance impact in the common case thanks to
> the previous work.
> 
> Patch 29 is an unrelated strengthening of assertions, that mst spotted
> while reviewing v1.

I've looked at everything except patch 18, which is really beyond my
understanding. Since it's running late for me now, I'll probably try patch 3
again tomorrow.

Thanks,

Fam

Re: [Qemu-devel] [PATCH 03/29] display: enable DIRTY_MEMORY_VGA tracking explicitly

[Mark Cave-Ayland]

On 26/05/15 12:56, Paolo Bonzini wrote:

> On 26/05/2015 13:52, Peter Maydell wrote:
>> On 26 May 2015 at 12:24, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>> Peter, Mark, Aurelien, can you review and ack this patch?
>> Could you provide some explanation/documentation of when a
>> display device needs to set DIRTY_MEMORY_VGA (and when it
>> doesn't)?
> 
> It needs to set it if it uses memory_region_get/set/clear_dirty with
> DIRTY_MEMORY_VGA as the last argument.
> 
>> If we get it wrong is there any way to make qemu
>> assert or otherwise catch the error?
> 
> It may be possible to check against mr->dirty_log_mask in
> memory_region_get/set/clear_dirty.  However, it is just as likely to
> have some corner case that is correct but triggers the assertion.
> 
> I haven't thought much about it, because a simple grep for
> DIRTY_MEMORY_VGA will catch the device models that need care.

Hi Paolo,

Sorry for a taking a while to get around to this. I've just tested the
v3 patchset on qemu-system-sparc for both CG3 and TCX and I don't see
any display issues (or at least if this were broken I'd expect to see
missing video updates/strange video artifacts).

So while I can't comment on the specifics, it looks reasonable based
upon the patchset and doesn't break anything so:

Acked-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.