[Qemu-devel] [PATCH V3 0/5] Xen, introducing dirty log for migration.

[Qemu-devel] [PATCH V3 0/5] Xen, introducing dirty log for migration.

[Anthony PERARD]

Hi,

This patch set will fix live migration under Xen. For this I introduce a new
QMP command to switch global-dirty log and few calls (in exec.c and memory.c)
to xen set_dirty function.

Change since v2:
  - renamed set_dirty_helper to invalidate_and_set_dirty.
  - in the last patch, set vram as dirty if the xen call fails, instead of only
    during migration.

Change v1-v2:
  - New patch to set dirty if not, in exec.c
    => only one place to add the xen call in exec.c


Thanks for your reviews,



Anthony PERARD (5):
  QMP, Introduce xen-set-global-dirty-log command.
  xen: Introduce xen_modified_memory.
  exec: Introduce helper to set dirty flags.
  exec, memory: Call to xen_modified_memory.
  xen: Set the vram dirty when an error occur.

 exec.c           | 53 ++++++++++++++++++-----------------------------------
 hw/xen.h         |  1 +
 memory.c         |  2 ++
 qapi-schema.json | 13 +++++++++++++
 qmp-commands.hx  | 24 ++++++++++++++++++++++++
 xen-all.c        | 39 ++++++++++++++++++++++++++++++++++++++-
 xen-stub.c       |  9 +++++++++
 7 files changed, 105 insertions(+), 36 deletions(-)

-- 
Anthony PERARD

[Qemu-devel] [PATCH V3 1/5] QMP, Introduce xen-set-global-dirty-log command.

[Anthony PERARD]

This command is used during a migration of a guest under Xen. It calls
memory_global_dirty_log_start or memory_global_dirty_log_stop according to the
argument pass to the command.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qapi-schema.json | 13 +++++++++++++
 qmp-commands.hx  | 24 ++++++++++++++++++++++++
 xen-all.c        | 15 +++++++++++++++
 xen-stub.c       |  5 +++++
 4 files changed, 57 insertions(+)

diff --git a/qapi-schema.json b/qapi-schema.json
index 14e4419..696f45a 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1956,6 +1956,19 @@
 { 'command': 'xen-save-devices-state', 'data': {'filename': 'str'} }
 
 ##
+# @xen-set-global-dirty-log
+#
+# Enable or disable the global dirty log mode.
+#
+# @enable: true to enable, false to disable.
+#
+# Returns: nothing
+#
+# Since: 1.2
+##
+{ 'command': 'xen-set-global-dirty-log', 'data': { 'enable': 'bool' } }
+
+##
 # @device_del:
 #
 # Remove a device from a guest
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 6e21ddb..662b7cf 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -493,6 +493,30 @@ Example:
 EQMP
 
     {
+        .name       = "xen-set-global-dirty-log",
+        .args_type  = "enable:b",
+        .mhandler.cmd_new = qmp_marshal_input_xen_set_global_dirty_log,
+    },
+
+SQMP
+xen-set-global-dirty-log
+-------
+
+Enable or disable the global dirty log mode.
+
+Arguments:
+
+- "enable": Enable it or disable it.
+
+Example:
+
+-> { "execute": "xen-set-global-dirty-log",
+     "arguments": { "enable": true } }
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "migrate",
         .args_type  = "detach:-d,blk:-b,inc:-i,uri:s",
         .mhandler.cmd_new = qmp_marshal_input_migrate,
diff --git a/xen-all.c b/xen-all.c
index f76b051..f75ae9f 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -14,6 +14,7 @@
 #include "hw/pc.h"
 #include "hw/xen_common.h"
 #include "hw/xen_backend.h"
+#include "qmp-commands.h"
 
 #include "range.h"
 #include "xen-mapcache.h"
@@ -36,6 +37,7 @@
 
 static MemoryRegion ram_memory, ram_640k, ram_lo, ram_hi;
 static MemoryRegion *framebuffer;
+static bool xen_in_migration;
 
 /* Compatibility with older version */
 #if __XEN_LATEST_INTERFACE_VERSION__ < 0x0003020a
@@ -552,10 +554,14 @@ static void xen_log_sync(MemoryListener *listener, MemoryRegionSection *section)
 
 static void xen_log_global_start(MemoryListener *listener)
 {
+    if (xen_enabled()) {
+        xen_in_migration = true;
+    }
 }
 
 static void xen_log_global_stop(MemoryListener *listener)
 {
+    xen_in_migration = false;
 }
 
 static void xen_eventfd_add(MemoryListener *listener,
@@ -588,6 +594,15 @@ static MemoryListener xen_memory_listener = {
     .priority = 10,
 };
 
+void qmp_xen_set_global_dirty_log(bool enable, Error **errp)
+{
+    if (enable) {
+        memory_global_dirty_log_start();
+    } else {
+        memory_global_dirty_log_stop();
+    }
+}
+
 /* VCPU Operations, MMIO, IO ring ... */
 
 static void xen_reset_vcpu(void *opaque)
diff --git a/xen-stub.c b/xen-stub.c
index 8ff2b79..5e66ba8 100644
--- a/xen-stub.c
+++ b/xen-stub.c
@@ -11,6 +11,7 @@
 #include "qemu-common.h"
 #include "hw/xen.h"
 #include "memory.h"
+#include "qmp-commands.h"
 
 void xenstore_store_pv_console_info(int i, CharDriverState *chr)
 {
@@ -54,3 +55,7 @@ int xen_init(void)
 void xen_register_framebuffer(MemoryRegion *mr)
 {
 }
+
+void qmp_xen_set_global_dirty_log(bool enable, Error **errp)
+{
+}
-- 
Anthony PERARD

[Qemu-devel] [PATCH V3 2/5] xen: Introduce xen_modified_memory.

[Anthony PERARD]

This function is to be used during live migration. Every write access to the
guest memory should call this funcion so the Xen tools knows which pages are
dirty.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 hw/xen.h   |  1 +
 xen-all.c  | 21 +++++++++++++++++++++
 xen-stub.c |  4 ++++
 3 files changed, 26 insertions(+)

diff --git a/hw/xen.h b/hw/xen.h
index e5926b7..d14e92d 100644
--- a/hw/xen.h
+++ b/hw/xen.h
@@ -48,6 +48,7 @@ void xenstore_store_pv_console_info(int i, struct CharDriverState *chr);
 struct MemoryRegion;
 void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size,
                    struct MemoryRegion *mr);
+void xen_modified_memory(ram_addr_t start, ram_addr_t length);
 #endif
 
 struct MemoryRegion;
diff --git a/xen-all.c b/xen-all.c
index f75ae9f..6250593 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -1228,3 +1228,24 @@ void xen_shutdown_fatal_error(const char *fmt, ...)
     /* destroy the domain */
     qemu_system_shutdown_request();
 }
+
+void xen_modified_memory(ram_addr_t start, ram_addr_t length)
+{
+    if (unlikely(xen_in_migration)) {
+        int rc;
+        ram_addr_t start_pfn, nb_pages;
+
+        if (length == 0) {
+            length = TARGET_PAGE_SIZE;
+        }
+        start_pfn = start >> TARGET_PAGE_BITS;
+        nb_pages = ((start + length + TARGET_PAGE_SIZE - 1) >> TARGET_PAGE_BITS)
+            - start_pfn;
+        rc = xc_hvm_modified_memory(xen_xc, xen_domid, start_pfn, nb_pages);
+        if (rc) {
+            fprintf(stderr, "%s failed for "RAM_ADDR_FMT" ("RAM_ADDR_FMT
+                    "): %i, %s\n", __func__,
+                    start, nb_pages, rc, strerror(-rc));
+        }
+    }
+}
diff --git a/xen-stub.c b/xen-stub.c
index 5e66ba8..9214392 100644
--- a/xen-stub.c
+++ b/xen-stub.c
@@ -59,3 +59,7 @@ void xen_register_framebuffer(MemoryRegion *mr)
 void qmp_xen_set_global_dirty_log(bool enable, Error **errp)
 {
 }
+
+void xen_modified_memory(ram_addr_t start, ram_addr_t length)
+{
+}
-- 
Anthony PERARD

Re: [Qemu-devel] [PATCH V3 2/5] xen: Introduce xen_modified_memory.

[Stefano Stabellini]

On Thu, 20 Sep 2012, Anthony PERARD wrote:
> This function is to be used during live migration. Every write access to the
> guest memory should call this funcion so the Xen tools knows which pages are
> dirty.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  hw/xen.h   |  1 +
>  xen-all.c  | 21 +++++++++++++++++++++
>  xen-stub.c |  4 ++++
>  3 files changed, 26 insertions(+)
> 
> diff --git a/hw/xen.h b/hw/xen.h
> index e5926b7..d14e92d 100644
> --- a/hw/xen.h
> +++ b/hw/xen.h
> @@ -48,6 +48,7 @@ void xenstore_store_pv_console_info(int i, struct CharDriverState *chr);
>  struct MemoryRegion;
>  void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size,
>                     struct MemoryRegion *mr);
> +void xen_modified_memory(ram_addr_t start, ram_addr_t length);
>  #endif
>  
>  struct MemoryRegion;
> diff --git a/xen-all.c b/xen-all.c
> index f75ae9f..6250593 100644
> --- a/xen-all.c
> +++ b/xen-all.c
> @@ -1228,3 +1228,24 @@ void xen_shutdown_fatal_error(const char *fmt, ...)
>      /* destroy the domain */
>      qemu_system_shutdown_request();
>  }
> +
> +void xen_modified_memory(ram_addr_t start, ram_addr_t length)
> +{
> +    if (unlikely(xen_in_migration)) {
> +        int rc;
> +        ram_addr_t start_pfn, nb_pages;
> +
> +        if (length == 0) {
> +            length = TARGET_PAGE_SIZE;
> +        }
> +        start_pfn = start >> TARGET_PAGE_BITS;
> +        nb_pages = ((start + length + TARGET_PAGE_SIZE - 1) >> TARGET_PAGE_BITS)
> +            - start_pfn;
> +        rc = xc_hvm_modified_memory(xen_xc, xen_domid, start_pfn, nb_pages);
> +        if (rc) {
> +            fprintf(stderr, "%s failed for "RAM_ADDR_FMT" ("RAM_ADDR_FMT
> +                    "): %i, %s\n", __func__,
> +                    start, nb_pages, rc, strerror(-rc));

a would prefer a more sane way of splitting this sentence in lines

Re: [Qemu-devel] [PATCH V3 2/5] xen: Introduce xen_modified_memory.

[Anthony PERARD]

On Thu, Sep 20, 2012 at 1:29 PM, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
>
>> +void xen_modified_memory(ram_addr_t start, ram_addr_t length)
>> +{
>> +    if (unlikely(xen_in_migration)) {
>> +        int rc;
>> +        ram_addr_t start_pfn, nb_pages;
>> +
>> +        if (length == 0) {
>> +            length = TARGET_PAGE_SIZE;
>> +        }
>> +        start_pfn = start >> TARGET_PAGE_BITS;
>> +        nb_pages = ((start + length + TARGET_PAGE_SIZE - 1) >> TARGET_PAGE_BITS)
>> +            - start_pfn;
>> +        rc = xc_hvm_modified_memory(xen_xc, xen_domid, start_pfn, nb_pages);
>> +        if (rc) {
>> +            fprintf(stderr, "%s failed for "RAM_ADDR_FMT" ("RAM_ADDR_FMT
>> +                    "): %i, %s\n", __func__,
>> +                    start, nb_pages, rc, strerror(-rc));
>
> a would prefer a more sane way of splitting this sentence in lines

Will do.

-- 
Anthony PERARD

[Qemu-devel] [PATCH V3 3/5] exec: Introduce helper to set dirty flags.

[Anthony PERARD]

This new helper/hook is used in the next patch to add an extra call in a single
place.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 exec.c | 52 +++++++++++++++++-----------------------------------
 1 file changed, 17 insertions(+), 35 deletions(-)

diff --git a/exec.c b/exec.c
index f22e9e6..209ac1c 100644
--- a/exec.c
+++ b/exec.c
@@ -3419,6 +3419,18 @@ int cpu_memory_rw_debug(CPUArchState *env, target_ulong addr,
 }
 
 #else
+
+static void invalidate_and_set_dirty(target_phys_addr_t addr,
+                                     target_phys_addr_t length)
+{
+    if (!cpu_physical_memory_is_dirty(addr)) {
+        /* invalidate code */
+        tb_invalidate_phys_page_range(addr, addr + length, 0);
+        /* set dirty bit */
+        cpu_physical_memory_set_dirty_flags(addr, (0xff & ~CODE_DIRTY_FLAG));
+    }
+}
+
 void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
                             int len, int is_write)
 {
@@ -3464,13 +3476,7 @@ void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
                 /* RAM case */
                 ptr = qemu_get_ram_ptr(addr1);
                 memcpy(ptr, buf, l);
-                if (!cpu_physical_memory_is_dirty(addr1)) {
-                    /* invalidate code */
-                    tb_invalidate_phys_page_range(addr1, addr1 + l, 0);
-                    /* set dirty bit */
-                    cpu_physical_memory_set_dirty_flags(
-                        addr1, (0xff & ~CODE_DIRTY_FLAG));
-                }
+                invalidate_and_set_dirty(addr1, l);
                 qemu_put_ram_ptr(ptr);
             }
         } else {
@@ -3536,13 +3542,7 @@ void cpu_physical_memory_write_rom(target_phys_addr_t addr,
             /* ROM/RAM case */
             ptr = qemu_get_ram_ptr(addr1);
             memcpy(ptr, buf, l);
-            if (!cpu_physical_memory_is_dirty(addr1)) {
-                /* invalidate code */
-                tb_invalidate_phys_page_range(addr1, addr1 + l, 0);
-                /* set dirty bit */
-                cpu_physical_memory_set_dirty_flags(
-                    addr1, (0xff & ~CODE_DIRTY_FLAG));
-            }
+            invalidate_and_set_dirty(addr1, l);
             qemu_put_ram_ptr(ptr);
         }
         len -= l;
@@ -3668,13 +3668,7 @@ void cpu_physical_memory_unmap(void *buffer, target_phys_addr_t len,
                 l = TARGET_PAGE_SIZE;
                 if (l > access_len)
                     l = access_len;
-                if (!cpu_physical_memory_is_dirty(addr1)) {
-                    /* invalidate code */
-                    tb_invalidate_phys_page_range(addr1, addr1 + l, 0);
-                    /* set dirty bit */
-                    cpu_physical_memory_set_dirty_flags(
-                        addr1, (0xff & ~CODE_DIRTY_FLAG));
-                }
+                invalidate_and_set_dirty(addr1, l);
                 addr1 += l;
                 access_len -= l;
             }
@@ -3980,13 +3974,7 @@ static inline void stl_phys_internal(target_phys_addr_t addr, uint32_t val,
             stl_p(ptr, val);
             break;
         }
-        if (!cpu_physical_memory_is_dirty(addr1)) {
-            /* invalidate code */
-            tb_invalidate_phys_page_range(addr1, addr1 + 4, 0);
-            /* set dirty bit */
-            cpu_physical_memory_set_dirty_flags(addr1,
-                (0xff & ~CODE_DIRTY_FLAG));
-        }
+        invalidate_and_set_dirty(addr1, 4);
     }
 }
 
@@ -4053,13 +4041,7 @@ static inline void stw_phys_internal(target_phys_addr_t addr, uint32_t val,
             stw_p(ptr, val);
             break;
         }
-        if (!cpu_physical_memory_is_dirty(addr1)) {
-            /* invalidate code */
-            tb_invalidate_phys_page_range(addr1, addr1 + 2, 0);
-            /* set dirty bit */
-            cpu_physical_memory_set_dirty_flags(addr1,
-                (0xff & ~CODE_DIRTY_FLAG));
-        }
+        invalidate_and_set_dirty(addr1, 2);
     }
 }
 
-- 
Anthony PERARD

Re: [Qemu-devel] [PATCH V3 3/5] exec: Introduce helper to set dirty flags.

[Avi Kivity]

On 09/20/2012 02:12 PM, Anthony PERARD wrote:
> This new helper/hook is used in the next patch to add an extra call in a single
> place.
> 

Reviewed-by: Avi Kivity <avi@redhat.com>


-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] [PATCH V3 4/5] exec, memory: Call to xen_modified_memory.

[Anthony PERARD]

This patch add some calls to xen_modified_memory to notify Xen about dirtybits
during migration.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 exec.c   | 1 +
 memory.c | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/exec.c b/exec.c
index 209ac1c..844a73c 100644
--- a/exec.c
+++ b/exec.c
@@ -3429,6 +3429,7 @@ static void invalidate_and_set_dirty(target_phys_addr_t addr,
         /* set dirty bit */
         cpu_physical_memory_set_dirty_flags(addr, (0xff & ~CODE_DIRTY_FLAG));
     }
+    xen_modified_memory(addr, length);
 }
 
 void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
diff --git a/memory.c b/memory.c
index 4f3ade0..015c544 100644
--- a/memory.c
+++ b/memory.c
@@ -19,6 +19,7 @@
 #include "bitops.h"
 #include "kvm.h"
 #include <assert.h>
+#include "hw/xen.h"
 
 #define WANT_EXEC_OBSOLETE
 #include "exec-obsolete.h"
@@ -1077,6 +1078,7 @@ void memory_region_set_dirty(MemoryRegion *mr, target_phys_addr_t addr,
                              target_phys_addr_t size)
 {
     assert(mr->terminates);
+    xen_modified_memory(mr->ram_addr + addr, size);
     return cpu_physical_memory_set_dirty_range(mr->ram_addr + addr, size, -1);
 }
 
-- 
Anthony PERARD

[Qemu-devel] [PATCH V3 5/5] xen: Set the vram dirty when an error occur.

[Anthony PERARD]

If the call to xc_hvm_track_dirty_vram() fails, then we set dirtybit on all the
video ram. This case happens during migration, but we don't need a special case
for live migration with patch.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 xen-all.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen-all.c b/xen-all.c
index 6250593..ab28261 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -507,7 +507,8 @@ static void xen_sync_dirty_bitmap(XenIOState *state,
                                  bitmap);
     if (rc < 0) {
         if (rc != -ENODATA) {
-            fprintf(stderr, "xen: track_dirty_vram failed (0x" TARGET_FMT_plx
+            memory_region_set_dirty(framebuffer, 0, size);
+            DPRINTF("xen: track_dirty_vram failed (0x" TARGET_FMT_plx
                     ", 0x" TARGET_FMT_plx "): %s\n",
                     start_addr, start_addr + size, strerror(-rc));
         }
-- 
Anthony PERARD

Re: [Qemu-devel] [PATCH V3 5/5] xen: Set the vram dirty when an error occur.

[Stefano Stabellini]

On Thu, 20 Sep 2012, Anthony PERARD wrote:
> If the call to xc_hvm_track_dirty_vram() fails, then we set dirtybit on all the
> video ram. This case happens during migration, but we don't need a special case
> for live migration with patch.

Just as a reference, what is this special case for live migration that
you are talking about? A pointer to the qemu-xen-traditional code would
be welcomed here.



> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  xen-all.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/xen-all.c b/xen-all.c
> index 6250593..ab28261 100644
> --- a/xen-all.c
> +++ b/xen-all.c
> @@ -507,7 +507,8 @@ static void xen_sync_dirty_bitmap(XenIOState *state,
>                                   bitmap);
>      if (rc < 0) {
>          if (rc != -ENODATA) {
> -            fprintf(stderr, "xen: track_dirty_vram failed (0x" TARGET_FMT_plx
> +            memory_region_set_dirty(framebuffer, 0, size);
> +            DPRINTF("xen: track_dirty_vram failed (0x" TARGET_FMT_plx
>                      ", 0x" TARGET_FMT_plx "): %s\n",
>                      start_addr, start_addr + size, strerror(-rc));
>          }
> -- 
> Anthony PERARD
> 

Re: [Qemu-devel] [PATCH V3 5/5] xen: Set the vram dirty when an error occur.

[Anthony PERARD]

On Thu, Sep 20, 2012 at 1:31 PM, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> On Thu, 20 Sep 2012, Anthony PERARD wrote:
>> If the call to xc_hvm_track_dirty_vram() fails, then we set dirtybit on all the
>> video ram. This case happens during migration, but we don't need a special case
>> for live migration with patch.
>
> Just as a reference, what is this special case for live migration that
> you are talking about? A pointer to the qemu-xen-traditional code would
> be welcomed here.

It was in the previous iteration of this patches series, qemu-trad
does not have a special case for migration, but a fail of the xc_ call
result in the vram to be marked as dirty.

-- 
Anthony PERARD