[PATCHv4 0/5] QEMU: Enabling live-migrate on HVM on qemu-xen device model in 4.2 - qemu patchset

[PATCHv4 0/5] QEMU: Enabling live-migrate on HVM on qemu-xen device model in 4.2 - qemu patchset

[Alex Bligh]

This patch series consists of 2	parts:
* 11 patches to	libxl
* 5 patches to QEMU

The 11 patches to libxl are unchanged since version 3 of the patch,
but have Acked-By lines reintroduced.

The 5 patches to QEMU are unchanged since version 2 of the patch.

These patches enable live-migrate on HVM using the upstream qemu-xen
device model under Xen 4.2. Currently this is unimplemented. In	the
main they are backports of patches in xen-unstable, thought the
QEMU side in particular needed some fiddling.

I would	suggest these patches should be included in 4.2.2.

You can find these on Github at:
 https://github.com/abligh/qemu-upstream-4.2-testing-livemigrate

Or on the master branch of this repository:
 https://github.com/abligh/qemu-upstream-4.2-testing-livemigrate.git

Alex Bligh (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           |   49 +++++++++++++++++++++----------------------------
 hw/xen.h         |    1 +
 memory.c         |    4 ++++
 qapi-schema.json |   13 +++++++++++++
 qmp-commands.hx  |   24 ++++++++++++++++++++++++
 xen-all.c        |   47 +++++++++++++++++++++++++++++++++++++++++++++--
 xen-stub.c       |    9 +++++++++
 7 files changed, 117 insertions(+), 30 deletions(-)

-- 
1.7.4.1

[PATCHv4 1/5] QMP, Introduce xen-set-global-dirty-log command.

[Alex Bligh]

This command is used during a migration of a guest under Xen. It calls
cpu_physical_memory_set_dirty_tracking.

Backport of 39f42439d0629d3921629dc4b38e68df8f2f7b83

Signed-off-by: Alex Bligh <alex@alex.org.uk>
---
 qapi-schema.json |   13 +++++++++++++
 qmp-commands.hx  |   24 ++++++++++++++++++++++++
 xen-all.c        |    6 ++++++
 xen-stub.c       |    5 +++++
 4 files changed, 48 insertions(+), 0 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index a669e98..bb0d7c5 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -905,3 +905,16 @@
 # Since: 1.1
 ##
 { '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.3
+##
+{ 'command': 'xen-set-global-dirty-log', 'data': { 'enable': 'bool' } }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index bf1df49..0de68df 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -472,6 +472,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",
         .params     = "[-d] [-b] [-i] uri",
diff --git a/xen-all.c b/xen-all.c
index 3256509..6b4e511 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -12,6 +12,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"
@@ -524,6 +525,11 @@ static CPUPhysMemoryClient xen_cpu_phys_memory_client = {
     .log_stop = xen_log_stop,
 };
 
+void qmp_xen_set_global_dirty_log(bool enable, Error **errp)
+{
+    cpu_physical_memory_set_dirty_tracking(!!enable);
+}
+
 /* VCPU Operations, MMIO, IO ring ... */
 
 static void xen_reset_vcpu(void *opaque)
diff --git a/xen-stub.c b/xen-stub.c
index efe2ab5..25317ec 100644
--- a/xen-stub.c
+++ b/xen-stub.c
@@ -8,6 +8,7 @@
 
 #include "qemu-common.h"
 #include "hw/xen.h"
+#include "qmp-commands.h"
 
 void xenstore_store_pv_console_info(int i, CharDriverState *chr)
 {
@@ -43,3 +44,7 @@ int xen_init(void)
 {
     return -ENOSYS;
 }
+
+void qmp_xen_set_global_dirty_log(bool enable, Error **errp)
+{
+}
-- 
1.7.4.1

[PATCHv4 2/5] xen: Introduce xen_modified_memory.

[Alex Bligh]

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.

Backport of 910b38e4dc4c37683c8b821e75a7f4cf095e4b21

Signed-off-by: Alex Bligh <alex@alex.org.uk>
---
 hw/xen.h   |    1 +
 xen-all.c  |   21 +++++++++++++++++++++
 xen-stub.c |    4 ++++
 3 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/hw/xen.h b/hw/xen.h
index 2162111..359a275 100644
--- a/hw/xen.h
+++ b/hw/xen.h
@@ -45,6 +45,7 @@ void xenstore_store_pv_console_info(int i, struct CharDriverState *chr);
 
 #if defined(NEED_CPU_H) && !defined(CONFIG_USER_ONLY)
 void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size);
+void xen_modified_memory(ram_addr_t start, ram_addr_t length);
 #endif
 
 #if defined(CONFIG_XEN) && CONFIG_XEN_CTRL_INTERFACE_VERSION < 400
diff --git a/xen-all.c b/xen-all.c
index 6b4e511..121289d 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -1135,3 +1135,24 @@ void destroy_hvm_domain(bool reboot)
         xc_interface_close(xc_handle);
     }
 }
+
+void xen_modified_memory(ram_addr_t start, ram_addr_t length)
+{
+    if (unlikely(cpu_physical_memory_get_dirty_tracking())) {
+        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 25317ec..7b54477 100644
--- a/xen-stub.c
+++ b/xen-stub.c
@@ -48,3 +48,7 @@ int xen_init(void)
 void qmp_xen_set_global_dirty_log(bool enable, Error **errp)
 {
 }
+
+void xen_modified_memory(ram_addr_t start, ram_addr_t length)
+{
+}
-- 
1.7.4.1

[PATCHv4 3/5] exec: Introduce helper to set dirty flags.

[Alex Bligh]

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

Backport of 51d7a9eb2b64e787c90bea1027308087eac22065

Signed-off-by: Alex Bligh <alex@alex.org.uk>
---
 exec.c |   45 +++++++++++++++++----------------------------
 1 files changed, 17 insertions(+), 28 deletions(-)

diff --git a/exec.c b/exec.c
index 6c206ff..511777b 100644
--- a/exec.c
+++ b/exec.c
@@ -3951,6 +3951,18 @@ int cpu_memory_rw_debug(CPUState *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)
 {
@@ -4003,13 +4015,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 {
@@ -4081,6 +4087,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);
+	    invalidate_and_set_dirty(addr1, l);
             qemu_put_ram_ptr(ptr);
         }
         len -= l;
@@ -4211,13 +4218,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;
             }
@@ -4561,13 +4562,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);
     }
 }
 
@@ -4639,13 +4634,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);
     }
 }
 
-- 
1.7.4.1

[PATCHv4 4/5] exec, memory: Call to xen_modified_memory.

[Alex Bligh]

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

Backport of e226939de5814527a21396903b08c3d0ed989558

Signed-off-by: Alex Bligh <alex@alex.org.uk>
---
 exec.c   |    4 ++++
 memory.c |    4 ++++
 2 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/exec.c b/exec.c
index 511777b..401f9bc 100644
--- a/exec.c
+++ b/exec.c
@@ -2988,6 +2988,9 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name,
     memset(ram_list.phys_dirty + (new_block->offset >> TARGET_PAGE_BITS),
            0xff, size >> TARGET_PAGE_BITS);
 
+    if (xen_enabled())
+        xen_modified_memory(new_block->offset, size);
+
     if (kvm_enabled())
         kvm_setup_guest_memory(new_block->host, size);
 
@@ -3961,6 +3964,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 7c20a07..6e0c596 100644
--- a/memory.c
+++ b/memory.c
@@ -16,6 +16,7 @@
 #include "ioport.h"
 #include "bitops.h"
 #include "kvm.h"
+#include "hw/xen.h"
 #include <assert.h>
 
 unsigned memory_region_transaction_depth = 0;
@@ -1065,6 +1066,9 @@ bool memory_region_get_dirty(MemoryRegion *mr, target_phys_addr_t addr,
 void memory_region_set_dirty(MemoryRegion *mr, target_phys_addr_t addr)
 {
     assert(mr->terminates);
+    if (xen_enabled())
+        xen_modified_memory((mr->ram_addr + addr) & TARGET_PAGE_MASK,
+                            TARGET_PAGE_SIZE);
     return cpu_physical_memory_set_dirty(mr->ram_addr + addr);
 }
 
-- 
1.7.4.1

[PATCHv4 5/5] xen: Set the vram dirty when an error occur.

[Alex Bligh]

If the call to xc_hvm_track_dirty_vram() fails, then we set dirtybit on all the
video ram. This case happens during migration.

Backport of 8aba7dc02d5660df7e7d8651304b3079908358be

Signed-off-by: Alex Bligh <alex@alex.org.uk>
---
 xen-all.c |   20 ++++++++++++++++++--
 1 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/xen-all.c b/xen-all.c
index 121289d..dbd759c 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -470,7 +470,21 @@ static int xen_sync_dirty_bitmap(XenIOState *state,
     rc = xc_hvm_track_dirty_vram(xen_xc, xen_domid,
                                  start_addr >> TARGET_PAGE_BITS, npages,
                                  bitmap);
-    if (rc) {
+    if (rc < 0) {
+        if (rc != -ENODATA) {
+            ram_addr_t addr, end;
+
+            xen_modified_memory(start_addr, size);
+            
+            end = TARGET_PAGE_ALIGN(start_addr + size);
+            for (addr = start_addr & TARGET_PAGE_MASK; addr < end; addr += TARGET_PAGE_SIZE) {
+                cpu_physical_memory_set_dirty(addr);
+            }
+
+            DPRINTF("xen: track_dirty_vram failed (0x" TARGET_FMT_plx
+                    ", 0x" TARGET_FMT_plx "): %s\n",
+                    start_addr, start_addr + size, strerror(-rc));
+        }
         return rc;
     }
 
@@ -479,7 +493,9 @@ static int xen_sync_dirty_bitmap(XenIOState *state,
         while (map != 0) {
             j = ffsl(map) - 1;
             map &= ~(1ul << j);
-            cpu_physical_memory_set_dirty(vram_offset + (i * width + j) * TARGET_PAGE_SIZE);
+            target_phys_addr_t todirty = vram_offset + (i * width + j) * TARGET_PAGE_SIZE;
+            xen_modified_memory(todirty, TARGET_PAGE_SIZE);
+            cpu_physical_memory_set_dirty(todirty);
         };
     }
 
-- 
1.7.4.1

Re: [PATCHv4 3/5] exec: Introduce helper to set dirty flags.

[Stefano Stabellini]

On Tue, 19 Feb 2013, Alex Bligh wrote:
> This new helper/hook is used in the next patch to add an extra call in a single
> place.
> 
> Backport of 51d7a9eb2b64e787c90bea1027308087eac22065
> 
> Signed-off-by: Alex Bligh <alex@alex.org.uk>
> ---
>  exec.c |   45 +++++++++++++++++----------------------------
>  1 files changed, 17 insertions(+), 28 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 6c206ff..511777b 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -3951,6 +3951,18 @@ int cpu_memory_rw_debug(CPUState *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)
>  {
> @@ -4003,13 +4015,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 {
> @@ -4081,6 +4087,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);
> +	    invalidate_and_set_dirty(addr1, l);
>              qemu_put_ram_ptr(ptr);
>          }
>          len -= l;

In QEMU the code style is spaces for indentation.
Moreover the right way of doing the backport here would be firstly to
backport 0b57e287, then 51d7a9eb2b64e787c90bea1027308087eac22065 should
just apply.

Re: [PATCHv4 4/5] exec, memory: Call to xen_modified_memory.

[Stefano Stabellini]

On Tue, 19 Feb 2013, Alex Bligh wrote:
> This patch add some calls to xen_modified_memory to notify Xen about dirtybits
> during migration.
> 
> Backport of e226939de5814527a21396903b08c3d0ed989558
> 
> Signed-off-by: Alex Bligh <alex@alex.org.uk>
>
>  exec.c   |    4 ++++
>  memory.c |    4 ++++
>  2 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 511777b..401f9bc 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2988,6 +2988,9 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name,
>      memset(ram_list.phys_dirty + (new_block->offset >> TARGET_PAGE_BITS),
>             0xff, size >> TARGET_PAGE_BITS);
>  
> +    if (xen_enabled())
> +        xen_modified_memory(new_block->offset, size);
> +

e226939de5814527a21396903b08c3d0ed989558 adds two calls to
xen_modified_memory, one in cpu_physical_memory_set_dirty_range, the
other in invalidate_and_set_dirty.

Where does this one come from?

If something is missing you need to backport that patch too, rather than
trying to adapt this patch.
In this case I believe you need to backport
1720aeee72888f80b974c33b6eb39922a0bea992.


>      if (kvm_enabled())
>          kvm_setup_guest_memory(new_block->host, size);
>  
> @@ -3961,6 +3964,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 7c20a07..6e0c596 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -16,6 +16,7 @@
>  #include "ioport.h"
>  #include "bitops.h"
>  #include "kvm.h"
> +#include "hw/xen.h"
>  #include <assert.h>
>  
>  unsigned memory_region_transaction_depth = 0;
> @@ -1065,6 +1066,9 @@ bool memory_region_get_dirty(MemoryRegion *mr, target_phys_addr_t addr,
>  void memory_region_set_dirty(MemoryRegion *mr, target_phys_addr_t addr)
>  {
>      assert(mr->terminates);
> +    if (xen_enabled())
> +        xen_modified_memory((mr->ram_addr + addr) & TARGET_PAGE_MASK,
> +                            TARGET_PAGE_SIZE);
>      return cpu_physical_memory_set_dirty(mr->ram_addr + addr);
>  }
>  
> -- 
> 1.7.4.1
> 

Re: [PATCHv4 5/5] xen: Set the vram dirty when an error occur.

[Stefano Stabellini]

On Tue, 19 Feb 2013, Alex Bligh 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.
> 
> Backport of 8aba7dc02d5660df7e7d8651304b3079908358be
> 
> Signed-off-by: Alex Bligh <alex@alex.org.uk>
> ---
>  xen-all.c |   20 ++++++++++++++++++--
>  1 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/xen-all.c b/xen-all.c
> index 121289d..dbd759c 100644
> --- a/xen-all.c
> +++ b/xen-all.c
> @@ -470,7 +470,21 @@ static int xen_sync_dirty_bitmap(XenIOState *state,
>      rc = xc_hvm_track_dirty_vram(xen_xc, xen_domid,
>                                   start_addr >> TARGET_PAGE_BITS, npages,
>                                   bitmap);
> -    if (rc) {
> +    if (rc < 0) {
> +        if (rc != -ENODATA) {
> +            ram_addr_t addr, end;
> +
> +            xen_modified_memory(start_addr, size);
> +            
> +            end = TARGET_PAGE_ALIGN(start_addr + size);
> +            for (addr = start_addr & TARGET_PAGE_MASK; addr < end; addr += TARGET_PAGE_SIZE) {
> +                cpu_physical_memory_set_dirty(addr);
> +            }
> +
> +            DPRINTF("xen: track_dirty_vram failed (0x" TARGET_FMT_plx
> +                    ", 0x" TARGET_FMT_plx "): %s\n",
> +                    start_addr, start_addr + size, strerror(-rc));
> +        }
>          return rc;
>      }

8aba7dc02d5660df7e7d8651304b3079908358be only adds a simple call to
xen_modified_memory if rc != ENODATA.
Where does the rest of the code you are adding comes from?


> @@ -479,7 +493,9 @@ static int xen_sync_dirty_bitmap(XenIOState *state,
>          while (map != 0) {
>              j = ffsl(map) - 1;
>              map &= ~(1ul << j);
> -            cpu_physical_memory_set_dirty(vram_offset + (i * width + j) * TARGET_PAGE_SIZE);
> +            target_phys_addr_t todirty = vram_offset + (i * width + j) * TARGET_PAGE_SIZE;
> +            xen_modified_memory(todirty, TARGET_PAGE_SIZE);
> +            cpu_physical_memory_set_dirty(todirty);
>          };
>      }

where does this chuck come from?
Wouldn't it make more sense to add a call to xen_modified_memory from
cpu_physical_memory_set_dirty?

Re: [PATCHv4 5/5] xen: Set the vram dirty when an error occur.

[Alex Bligh]

Stefano,

--On 19 February 2013 18:05:46 +0000 Stefano Stabellini 
<stefano.stabellini@eu.citrix.com> wrote:

> On Tue, 19 Feb 2013, Alex Bligh 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.

This was the patch I had most difficulty with, frankly because I had
little idea what it was doing. Education welcome!

>>
>> Backport of 8aba7dc02d5660df7e7d8651304b3079908358be
>>
>> Signed-off-by: Alex Bligh <alex@alex.org.uk>
>> ---
>>  xen-all.c |   20 ++++++++++++++++++--
>>  1 files changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen-all.c b/xen-all.c
>> index 121289d..dbd759c 100644
>> --- a/xen-all.c
>> +++ b/xen-all.c
>> @@ -470,7 +470,21 @@ static int xen_sync_dirty_bitmap(XenIOState *state,
>>      rc = xc_hvm_track_dirty_vram(xen_xc, xen_domid,
>>                                   start_addr >> TARGET_PAGE_BITS, npages,
>>                                   bitmap);
>> -    if (rc) {
>> +    if (rc < 0) {
>> +        if (rc != -ENODATA) {
>> +            ram_addr_t addr, end;
>> +
>> +            xen_modified_memory(start_addr, size);
>> +
>> +            end = TARGET_PAGE_ALIGN(start_addr + size);
>> +            for (addr = start_addr & TARGET_PAGE_MASK; addr < end; addr
>> += TARGET_PAGE_SIZE) { +
>> cpu_physical_memory_set_dirty(addr);
>> +            }
>> +
>> +            DPRINTF("xen: track_dirty_vram failed (0x" TARGET_FMT_plx
>> +                    ", 0x" TARGET_FMT_plx "): %s\n",
>> +                    start_addr, start_addr + size, strerror(-rc));
>> +        }
>>          return rc;
>>      }
>
> 8aba7dc02d5660df7e7d8651304b3079908358be only adds a simple call to
> xen_modified_memory if rc != ENODATA.
> Where does the rest of the code you are adding comes from?

Applying the patch unchanged is not easy as there are a pile of
conflicting items. I was taking a conservative approach partly
as I didn't fully understand what types of addresses could be
safely used where. My approach was to make this function as similar
as possible to xen 4.3 immediately after the patch, which is here:

http://git.qemu.org/?p=qemu.git;a=blob;f=xen-all.c;h=e6308be23adb7198c144883eb886fb6edb5fe09f;hb=8aba7dc02d5660df7e7d8651304b3079908358be

There were some oddnesses there, such as:

 508     if (rc < 0) {
 509         if (rc != -ENODATA) {
 510             memory_region_set_dirty(framebuffer, 0, size);
 511             DPRINTF("xen: track_dirty_vram failed (0x" TARGET_FMT_plx
 512                     ", 0x" TARGET_FMT_plx "): %s\n",
 513                     start_addr, start_addr + size, strerror(-rc));
 514         }
 515         return;
 516     }

What is the point of the outer check on rc?

I think you are asking why I didn't just call memory_region_set_dirty.
memory_region_set_dirty in 4.2 (as opposed to 4.3) takes:
 void memory_region_set_dirty(MemoryRegion *mr, target_phys_addr_t addr);
and the 3 parameter version is unavailable, so what I did was (I hope)
the equivalent of what the 3 parameter version would have done, going
through each page.

I could have modified memory_region_set_dirty to cope with multiple pages
but that seemed like a more intrusive change.

>> @@ -479,7 +493,9 @@ static int xen_sync_dirty_bitmap(XenIOState *state,
>>          while (map != 0) {
>>              j = ffsl(map) - 1;
>>              map &= ~(1ul << j);
>> -            cpu_physical_memory_set_dirty(vram_offset + (i * width + j)
>> * TARGET_PAGE_SIZE); +            target_phys_addr_t todirty =
>> vram_offset + (i * width + j) * TARGET_PAGE_SIZE; +
>> xen_modified_memory(todirty, TARGET_PAGE_SIZE);
>> +            cpu_physical_memory_set_dirty(todirty);
>>          };
>>      }
>
> where does this chuck come from?
> Wouldn't it make more sense to add a call to xen_modified_memory from
> cpu_physical_memory_set_dirty?

Again, I was trying to emulate what the 3 parameter
memory_region_set_dirty() does. I believe cpu_physical_memory_set_dirty has
other callers, and was unsure whether adding a call to xen_modified_memory
would be safe in there.

-- 
Alex Bligh

Re: [PATCHv4 3/5] exec: Introduce helper to set dirty flags.

[Alex Bligh]

Stefano,

--On 19 February 2013 17:42:50 +0000 Stefano Stabellini 
<stefano.stabellini@eu.citrix.com> wrote:

> In QEMU the code style is spaces for indentation.

OK no idea how that happened - sorry. I'm using qemu's published emacs
c mode (or thought I was).

> Moreover the right way of doing the backport here would be firstly to
> backport 0b57e287, then 51d7a9eb2b64e787c90bea1027308087eac22065 should
> just apply.

I just backported the 5 patches you pointed me at, and any code changes
they required.

So the change I posted brings in invalidate_and_set_dirty, pretty much
per the original patch. Is the issue here I combined it with the other
patch you mentioned? Guidance welcome.

-- 
Alex Bligh

Re: [PATCHv4 4/5] exec, memory: Call to xen_modified_memory.

[Alex Bligh]

--On 19 February 2013 18:03:04 +0000 Stefano Stabellini 
<stefano.stabellini@eu.citrix.com> wrote:

> e226939de5814527a21396903b08c3d0ed989558 adds two calls to
> xen_modified_memory, one in cpu_physical_memory_set_dirty_range, the
> other in invalidate_and_set_dirty.
>
> Where does this one come from?
>
> If something is missing you need to backport that patch too, rather than
> trying to adapt this patch.
> In this case I believe you need to backport
> 1720aeee72888f80b974c33b6eb39922a0bea992.

Again, I was trying to make the fewest changes possible, being aware I
have little understanding of the code. Some assistance would be appreciated
here particularly as the substitute patch you mention would appear to have
have wider implications.

-- 
Alex Bligh

Re: [PATCHv4 3/5] exec: Introduce helper to set dirty flags.

[Stefano Stabellini]

On Tue, 19 Feb 2013, Alex Bligh wrote:
> Stefano,
> 
> --On 19 February 2013 17:42:50 +0000 Stefano Stabellini 
> <stefano.stabellini@eu.citrix.com> wrote:
> 
> > In QEMU the code style is spaces for indentation.
> 
> OK no idea how that happened - sorry. I'm using qemu's published emacs
> c mode (or thought I was).
> 
> > Moreover the right way of doing the backport here would be firstly to
> > backport 0b57e287, then 51d7a9eb2b64e787c90bea1027308087eac22065 should
> > just apply.
> 
> I just backported the 5 patches you pointed me at, and any code changes
> they required.
> 
> So the change I posted brings in invalidate_and_set_dirty, pretty much
> per the original patch. Is the issue here I combined it with the other
> patch you mentioned? Guidance welcome.

In general is preferable to backport a patch as-is.

If the patch in question depends on another patch, which is small and
can also be easily backported, like in this case, the best course of
action is to backport them both as separate patches.

Re: [PATCHv4 5/5] xen: Set the vram dirty when an error occur.

[Stefano Stabellini]

On Tue, 19 Feb 2013, Alex Bligh wrote:
> Stefano,
> 
> --On 19 February 2013 18:05:46 +0000 Stefano Stabellini 
> <stefano.stabellini@eu.citrix.com> wrote:
> 
> > On Tue, 19 Feb 2013, Alex Bligh 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.
> 
> This was the patch I had most difficulty with, frankly because I had
> little idea what it was doing. Education welcome!
> 
> >>
> >> Backport of 8aba7dc02d5660df7e7d8651304b3079908358be
> >>
> >> Signed-off-by: Alex Bligh <alex@alex.org.uk>
> >> ---
> >>  xen-all.c |   20 ++++++++++++++++++--
> >>  1 files changed, 18 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/xen-all.c b/xen-all.c
> >> index 121289d..dbd759c 100644
> >> --- a/xen-all.c
> >> +++ b/xen-all.c
> >> @@ -470,7 +470,21 @@ static int xen_sync_dirty_bitmap(XenIOState *state,
> >>      rc = xc_hvm_track_dirty_vram(xen_xc, xen_domid,
> >>                                   start_addr >> TARGET_PAGE_BITS, npages,
> >>                                   bitmap);
> >> -    if (rc) {
> >> +    if (rc < 0) {
> >> +        if (rc != -ENODATA) {
> >> +            ram_addr_t addr, end;
> >> +
> >> +            xen_modified_memory(start_addr, size);
> >> +
> >> +            end = TARGET_PAGE_ALIGN(start_addr + size);
> >> +            for (addr = start_addr & TARGET_PAGE_MASK; addr < end; addr
> >> += TARGET_PAGE_SIZE) { +
> >> cpu_physical_memory_set_dirty(addr);
> >> +            }
> >> +
> >> +            DPRINTF("xen: track_dirty_vram failed (0x" TARGET_FMT_plx
> >> +                    ", 0x" TARGET_FMT_plx "): %s\n",
> >> +                    start_addr, start_addr + size, strerror(-rc));
> >> +        }
> >>          return rc;
> >>      }
> >
> > 8aba7dc02d5660df7e7d8651304b3079908358be only adds a simple call to
> > xen_modified_memory if rc != ENODATA.
> > Where does the rest of the code you are adding comes from?
> 
> Applying the patch unchanged is not easy as there are a pile of
> conflicting items. I was taking a conservative approach partly
> as I didn't fully understand what types of addresses could be
> safely used where. My approach was to make this function as similar
> as possible to xen 4.3 immediately after the patch, which is here:
> 
> http://git.qemu.org/?p=qemu.git;a=blob;f=xen-all.c;h=e6308be23adb7198c144883eb886fb6edb5fe09f;hb=8aba7dc02d5660df7e7d8651304b3079908358be
> 
> There were some oddnesses there, such as:
> 
>  508     if (rc < 0) {
>  509         if (rc != -ENODATA) {
>  510             memory_region_set_dirty(framebuffer, 0, size);
>  511             DPRINTF("xen: track_dirty_vram failed (0x" TARGET_FMT_plx
>  512                     ", 0x" TARGET_FMT_plx "): %s\n",
>  513                     start_addr, start_addr + size, strerror(-rc));
>  514         }
>  515         return;
>  516     }
> 
> What is the point of the outer check on rc?

Yeah, the code could be more readable. The point would be only to act on
errors.


> I think you are asking why I didn't just call memory_region_set_dirty.
> memory_region_set_dirty in 4.2 (as opposed to 4.3) takes:
>  void memory_region_set_dirty(MemoryRegion *mr, target_phys_addr_t addr);
> and the 3 parameter version is unavailable, so what I did was (I hope)
> the equivalent of what the 3 parameter version would have done, going
> through each page.
> 
> I could have modified memory_region_set_dirty to cope with multiple pages
> but that seemed like a more intrusive change.

I think that the best thing to do in this case is just to add a call to
xen_modified_memory if (rc != -ENODATA).


> >> @@ -479,7 +493,9 @@ static int xen_sync_dirty_bitmap(XenIOState *state,
> >>          while (map != 0) {
> >>              j = ffsl(map) - 1;
> >>              map &= ~(1ul << j);
> >> -            cpu_physical_memory_set_dirty(vram_offset + (i * width + j)
> >> * TARGET_PAGE_SIZE); +            target_phys_addr_t todirty =
> >> vram_offset + (i * width + j) * TARGET_PAGE_SIZE; +
> >> xen_modified_memory(todirty, TARGET_PAGE_SIZE);
> >> +            cpu_physical_memory_set_dirty(todirty);
> >>          };
> >>      }
> >
> > where does this chuck come from?
> > Wouldn't it make more sense to add a call to xen_modified_memory from
> > cpu_physical_memory_set_dirty?
> 
> Again, I was trying to emulate what the 3 parameter
> memory_region_set_dirty() does. I believe cpu_physical_memory_set_dirty has
> other callers, and was unsure whether adding a call to xen_modified_memory
> would be safe in there.
 
Considering that this change wasn't part of the original patch, it needs
to go into a different patch. Probably patch 4/5 would be a better fit,
given that this change is needed because
cpu_physical_memory_set_dirty_range doesn't exist.

Finally I think that adding a call to xen_modified_memory from
cpu_physical_memory_set_dirty would simplify things. The size would be
PAGE_SIZE.

Re: [PATCHv4 4/5] exec, memory: Call to xen_modified_memory.

[Stefano Stabellini]

On Tue, 19 Feb 2013, Alex Bligh wrote:
> --On 19 February 2013 18:03:04 +0000 Stefano Stabellini 
> <stefano.stabellini@eu.citrix.com> wrote:
> 
> > e226939de5814527a21396903b08c3d0ed989558 adds two calls to
> > xen_modified_memory, one in cpu_physical_memory_set_dirty_range, the
> > other in invalidate_and_set_dirty.
> >
> > Where does this one come from?
> >
> > If something is missing you need to backport that patch too, rather than
> > trying to adapt this patch.
> > In this case I believe you need to backport
> > 1720aeee72888f80b974c33b6eb39922a0bea992.
> 
> Again, I was trying to make the fewest changes possible, being aware I
> have little understanding of the code. Some assistance would be appreciated
> here particularly as the substitute patch you mention would appear to have
> have wider implications.

OK. Keep the patch as you wrote it and add a comment in the description
saying why you needed to make the additional changes that are not part
of e226939de5814527a21396903b08c3d0ed989558.

Also add a call to xen_modified_memory from
cpu_physical_memory_set_dirty, if I am not mistaken that should solve
the problem with the dirty vram.

Re: [PATCHv4 4/5] exec, memory: Call to xen_modified_memory.

[Alex Bligh]

Stefano,

--On 20 February 2013 12:30:45 +0000 Stefano Stabellini 
<stefano.stabellini@eu.citrix.com> wrote:

> Also add a call to xen_modified_memory from
> cpu_physical_memory_set_dirty, if I am not mistaken that should solve
> the problem with the dirty vram.

I think I now recall why I did not do this.

Firstly, cpu_physical_memory_set_dirty is inline, and I'd have to check
(presumably) xen_enabled() then call a non-inline function. This seemed
a bit much to satisfy only the needs of the VRAM check as (as far as I
could tell - I may well be wrong) xen_modifed_memory was being called
appropriately by all the other callers anyway.

Secondly, if I do need to modify this one, it seemed odd that I'd need
not need to modify cpu_physical_set_dirty_flags.

It seemed cleaner just to fix up the VRAM stuff.

What do you think? I'll send another patch series with everything else
in.

-- 
Alex Bligh