[Qemu-devel] [PULL v2 0/8] migration queue

[Qemu-devel] [PULL v2 0/8] migration queue

[Juan Quintela]

Hi

After sending the 1st version of this pull, I noticed that I
integrated the wrong version of Orit patches.  Resubmitting with the
correct one.

As a bonus, it includes the reviews-by from David O:-)

Sorry for the inconveniences, Juan.

[v1]

This includes:
- Peter changes to make VMSTATE_STRUCT_POINTER more consistent
- Fix migration with hpratio (ppc on ppc64 basically)
- Orit/Arei cleanups/fixes to xbzrle


The following changes since commit 8cfc114a2f293c40077d1bdb7500b29db359ca22:

  linux-user: Fix trampoline code for CRIS (2014-02-03 14:04:00 +0000)

are available in the git repository at:

  git://github.com/juanquintela/qemu.git tags/migration/20140204-1

for you to fetch changes up to 89db9987c07977bdb78d5d4b41d65e7acb9a5a2c:

  Don't abort on memory allocation error (2014-02-04 16:50:37 +0100)

----------------------------------------------------------------
migration/next for 20140204

----------------------------------------------------------------
Alexey Kardashevskiy (1):
      exec: fix ram_list dirty map optimization

Gonglei (Arei) (1):
      migration:fix free XBZRLE decoded_buf wrong

Orit Wasserman (5):
      Set xbzrle buffers to NULL after freeing them to avoid double free errors
      Add check for cache size smaller than page size
      XBZRLE cache size should not be larger than guest memory size
      Don't abort on out of memory when creating page cache
      Don't abort on memory allocation error

Peter Maydell (1):
      vmstate: Make VMSTATE_STRUCT_POINTER take type, not ptr-to-type

 arch_init.c                    | 49 +++++++++++++++++++++++++++++++-----------
 hw/arm/pxa2xx.c                |  2 +-
 include/exec/ram_addr.h        |  3 ++-
 include/hw/ptimer.h            | 10 ++-------
 include/migration/migration.h  |  1 +
 include/migration/page_cache.h |  4 +++-
 include/migration/vmstate.h    |  8 +++----
 migration.c                    | 18 +++++++++++++++-
 page_cache.c                   | 34 +++++++++++++++++++++--------
 9 files changed, 92 insertions(+), 37 deletions(-)

[Qemu-devel] [PATCH 1/8] vmstate: Make VMSTATE_STRUCT_POINTER take type, not ptr-to-type

[Juan Quintela]

From: Peter Maydell <peter.maydell@linaro.org>

The VMSTATE_STRUCT_POINTER macros are a bit odd in that they
must be passed an argument "FooType *" rather than just taking
the FooType. They're only used in one place, so it's easy to
tidy this up. This also lets us use the macro to replace the
hand-rolled VMSTATE_PTIMER.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/arm/pxa2xx.c             |  2 +-
 include/hw/ptimer.h         | 10 ++--------
 include/migration/vmstate.h |  8 ++++----
 3 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
index 02b7016..25ec549 100644
--- a/hw/arm/pxa2xx.c
+++ b/hw/arm/pxa2xx.c
@@ -1448,7 +1448,7 @@ static const VMStateDescription vmstate_pxa2xx_i2c = {
         VMSTATE_UINT8(ibmr, PXA2xxI2CState),
         VMSTATE_UINT8(data, PXA2xxI2CState),
         VMSTATE_STRUCT_POINTER(slave, PXA2xxI2CState,
-                               vmstate_pxa2xx_i2c_slave, PXA2xxI2CSlaveState *),
+                               vmstate_pxa2xx_i2c_slave, PXA2xxI2CSlaveState),
         VMSTATE_END_OF_LIST()
     }
 };
diff --git a/include/hw/ptimer.h b/include/hw/ptimer.h
index a33edf4..8ebacbb 100644
--- a/include/hw/ptimer.h
+++ b/include/hw/ptimer.h
@@ -27,14 +27,8 @@ void ptimer_stop(ptimer_state *s);

 extern const VMStateDescription vmstate_ptimer;

-#define VMSTATE_PTIMER(_field, _state) {                             \
-    .name       = (stringify(_field)),                               \
-    .version_id = (1),                                               \
-    .vmsd       = &vmstate_ptimer,                                   \
-    .size       = sizeof(ptimer_state *),                            \
-    .flags      = VMS_STRUCT|VMS_POINTER,                            \
-    .offset     = vmstate_offset_pointer(_state, _field, ptimer_state), \
-}
+#define VMSTATE_PTIMER(_field, _state) \
+    VMSTATE_STRUCT_POINTER_V(_field, _state, 1, vmstate_ptimer, ptimer_state)

 #define VMSTATE_PTIMER_ARRAY(_f, _s, _n)                                \
     VMSTATE_ARRAY_OF_POINTER_TO_STRUCT(_f, _s, _n, 0,                   \
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index be193ba..fbd16a0 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -314,9 +314,9 @@ extern const VMStateInfo vmstate_info_bitmap;
     .name         = (stringify(_field)),                             \
     .version_id   = (_version),                                        \
     .vmsd         = &(_vmsd),                                        \
-    .size         = sizeof(_type),                                   \
+    .size         = sizeof(_type *),                                 \
     .flags        = VMS_STRUCT|VMS_POINTER,                          \
-    .offset       = vmstate_offset_value(_state, _field, _type),     \
+    .offset       = vmstate_offset_pointer(_state, _field, _type),   \
 }

 #define VMSTATE_STRUCT_POINTER_TEST_V(_field, _state, _test, _version, _vmsd, _type) { \
@@ -324,9 +324,9 @@ extern const VMStateInfo vmstate_info_bitmap;
     .version_id   = (_version),                                        \
     .field_exists = (_test),                                         \
     .vmsd         = &(_vmsd),                                        \
-    .size         = sizeof(_type),                                   \
+    .size         = sizeof(_type *),                                 \
     .flags        = VMS_STRUCT|VMS_POINTER,                          \
-    .offset       = vmstate_offset_value(_state, _field, _type),     \
+    .offset       = vmstate_offset_pointer(_state, _field, _type),   \
 }

 #define VMSTATE_ARRAY_OF_POINTER(_field, _state, _num, _version, _info, _type) {\
-- 
1.8.5.3

[Qemu-devel] [PATCH 2/8] exec: fix ram_list dirty map optimization

[Juan Quintela]

From: Alexey Kardashevskiy <aik@ozlabs.ru>

The ae2810c4bb3b383176e8e1b33931b16c01483aab patch introduced
optimization for ram_list.dirty_memory update. However it can only
work correctly if hpratio is 1 as the @bitmap parameter stores 1 bits
per system page size (may vary, 4K or 64K on PPC64) and
ram_list.dirty_memory stores 1 bit per TARGET_PAGE_SIZE
(which is hardcoded to 4K).

This fixes hpratio!=1 case to fall back to the slow path.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 include/exec/ram_addr.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index 481a447..2edfa96 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -93,7 +93,8 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
     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) {
+    if ((((page * BITS_PER_LONG) << TARGET_PAGE_BITS) == start) &&
+        (hpratio == 1)) {
         long k;
         long nr = BITS_TO_LONGS(pages);

-- 
1.8.5.3

[Qemu-devel] [PATCH 3/8] Set xbzrle buffers to NULL after freeing them to avoid double free errors

[Juan Quintela]

From: Orit Wasserman <owasserm@redhat.com>

Signed-off-by: Orit Wasserman <owasserm@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch_init.c b/arch_init.c
index 77912e7..66f5e82 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -617,6 +617,9 @@ static void migration_end(void)
         g_free(XBZRLE.current_buf);
         g_free(XBZRLE.decoded_buf);
         XBZRLE.cache = NULL;
+        XBZRLE.encoded_buf = NULL;
+        XBZRLE.current_buf = NULL;
+        XBZRLE.decoded_buf = NULL;
     }
 }

-- 
1.8.5.3

[Qemu-devel] [PATCH 4/8] Add check for cache size smaller than page size

[Juan Quintela]

From: Orit Wasserman <owasserm@redhat.com>

Signed-off-by: Orit Wasserman <owasserm@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c |  4 ++++
 migration.c | 10 +++++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/arch_init.c b/arch_init.c
index 66f5e82..8edeabe 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -178,6 +178,10 @@ static struct {

 int64_t xbzrle_cache_resize(int64_t new_size)
 {
+    if (new_size < TARGET_PAGE_SIZE) {
+        return -1;
+    }
+
     if (XBZRLE.cache != NULL) {
         return cache_resize(XBZRLE.cache, new_size / TARGET_PAGE_SIZE) *
             TARGET_PAGE_SIZE;
diff --git a/migration.c b/migration.c
index 7235c23..84587e9 100644
--- a/migration.c
+++ b/migration.c
@@ -469,6 +469,7 @@ void qmp_migrate_cancel(Error **errp)
 void qmp_migrate_set_cache_size(int64_t value, Error **errp)
 {
     MigrationState *s = migrate_get_current();
+    int64_t new_size;

     /* Check for truncation */
     if (value != (size_t)value) {
@@ -477,7 +478,14 @@ void qmp_migrate_set_cache_size(int64_t value, Error **errp)
         return;
     }

-    s->xbzrle_cache_size = xbzrle_cache_resize(value);
+    new_size = xbzrle_cache_resize(value);
+    if (new_size < 0) {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
+                  "is smaller than page size");
+        return;
+    }
+
+    s->xbzrle_cache_size = new_size;
 }

 int64_t qmp_query_migrate_cache_size(Error **errp)
-- 
1.8.5.3

[Qemu-devel] [PATCH 5/8] migration:fix free XBZRLE decoded_buf wrong

[Juan Quintela]

From: "Gonglei (Arei)" <arei.gonglei@huawei.com>

When qemu do live migration with xbzrle, qemu malloc decoded_buf
at destination end but free it at source end. It will crash qemu
by double free error in some scenarios. Splitting the XBZRLE structure
for clear logic distinguishing src/dst side.

Signed-off-by: ChenLiang <chenliang88@huawei.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Orit Wasserman <owasserm@redhat.com>
Signed-off-by: GongLei <arei.gonglei@huawei.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c                   | 22 ++++++++++++----------
 include/migration/migration.h |  1 +
 migration.c                   |  1 +
 3 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 8edeabe..5eff80b 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -164,17 +164,15 @@ static struct {
     uint8_t *encoded_buf;
     /* buffer for storing page content */
     uint8_t *current_buf;
-    /* buffer used for XBZRLE decoding */
-    uint8_t *decoded_buf;
     /* Cache for XBZRLE */
     PageCache *cache;
 } XBZRLE = {
     .encoded_buf = NULL,
     .current_buf = NULL,
-    .decoded_buf = NULL,
     .cache = NULL,
 };
-
+/* buffer used for XBZRLE decoding */
+static uint8_t *xbzrle_decoded_buf;

 int64_t xbzrle_cache_resize(int64_t new_size)
 {
@@ -606,6 +604,12 @@ uint64_t ram_bytes_total(void)
     return total;
 }

+void free_xbzrle_decoded_buf(void)
+{
+    g_free(xbzrle_decoded_buf);
+    xbzrle_decoded_buf = NULL;
+}
+
 static void migration_end(void)
 {
     if (migration_bitmap) {
@@ -619,11 +623,9 @@ static void migration_end(void)
         g_free(XBZRLE.cache);
         g_free(XBZRLE.encoded_buf);
         g_free(XBZRLE.current_buf);
-        g_free(XBZRLE.decoded_buf);
         XBZRLE.cache = NULL;
         XBZRLE.encoded_buf = NULL;
         XBZRLE.current_buf = NULL;
-        XBZRLE.decoded_buf = NULL;
     }
 }

@@ -814,8 +816,8 @@ static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host)
     unsigned int xh_len;
     int xh_flags;

-    if (!XBZRLE.decoded_buf) {
-        XBZRLE.decoded_buf = g_malloc(TARGET_PAGE_SIZE);
+    if (!xbzrle_decoded_buf) {
+        xbzrle_decoded_buf = g_malloc(TARGET_PAGE_SIZE);
     }

     /* extract RLE header */
@@ -832,10 +834,10 @@ static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host)
         return -1;
     }
     /* load data and decode */
-    qemu_get_buffer(f, XBZRLE.decoded_buf, xh_len);
+    qemu_get_buffer(f, xbzrle_decoded_buf, xh_len);

     /* decode RLE */
-    ret = xbzrle_decode_buffer(XBZRLE.decoded_buf, xh_len, host,
+    ret = xbzrle_decode_buffer(xbzrle_decoded_buf, xh_len, host,
                                TARGET_PAGE_SIZE);
     if (ret == -1) {
         fprintf(stderr, "Failed to load XBZRLE page - decode error!\n");
diff --git a/include/migration/migration.h b/include/migration/migration.h
index bfa3951..3e1e6c7 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -109,6 +109,7 @@ MigrationState *migrate_get_current(void);
 uint64_t ram_bytes_remaining(void);
 uint64_t ram_bytes_transferred(void);
 uint64_t ram_bytes_total(void);
+void free_xbzrle_decoded_buf(void);

 void acct_update_position(QEMUFile *f, size_t size, bool zero);

diff --git a/migration.c b/migration.c
index 84587e9..46a7305 100644
--- a/migration.c
+++ b/migration.c
@@ -105,6 +105,7 @@ static void process_incoming_migration_co(void *opaque)

     ret = qemu_loadvm_state(f);
     qemu_fclose(f);
+    free_xbzrle_decoded_buf();
     if (ret < 0) {
         fprintf(stderr, "load of migration failed\n");
         exit(EXIT_FAILURE);
-- 
1.8.5.3

[Qemu-devel] [PATCH 6/8] XBZRLE cache size should not be larger than guest memory size

[Juan Quintela]

From: Orit Wasserman <owasserm@redhat.com>

Signed-off-by: Orit Wasserman <owasserm@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/migration.c b/migration.c
index 46a7305..25add6f 100644
--- a/migration.c
+++ b/migration.c
@@ -479,6 +479,13 @@ void qmp_migrate_set_cache_size(int64_t value, Error **errp)
         return;
     }

+    /* Cache should not be larger than guest ram size */
+    if (value > ram_bytes_total()) {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
+                  "exceeds guest ram size ");
+        return;
+    }
+
     new_size = xbzrle_cache_resize(value);
     if (new_size < 0) {
         error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
-- 
1.8.5.3

[Qemu-devel] [PATCH 7/8] Don't abort on out of memory when creating page cache

[Juan Quintela]

From: Orit Wasserman <owasserm@redhat.com>

Signed-off-by: Orit Wasserman <owasserm@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c  | 18 ++++++++++++++++--
 page_cache.c | 18 ++++++++++++++----
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 5eff80b..1fa5f1f 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -664,8 +664,22 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
             DPRINTF("Error creating cache\n");
             return -1;
         }
-        XBZRLE.encoded_buf = g_malloc0(TARGET_PAGE_SIZE);
-        XBZRLE.current_buf = g_malloc(TARGET_PAGE_SIZE);
+
+        /* We prefer not to abort if there is no memory */
+        XBZRLE.encoded_buf = g_try_malloc0(TARGET_PAGE_SIZE);
+        if (!XBZRLE.encoded_buf) {
+            DPRINTF("Error allocating encoded_buf\n");
+            return -1;
+        }
+
+        XBZRLE.current_buf = g_try_malloc(TARGET_PAGE_SIZE);
+        if (!XBZRLE.current_buf) {
+            DPRINTF("Error allocating current_buf\n");
+            g_free(XBZRLE.encoded_buf);
+            XBZRLE.encoded_buf = NULL;
+            return -1;
+        }
+
         acct_clear();
     }

diff --git a/page_cache.c b/page_cache.c
index a05db64..62a53f8 100644
--- a/page_cache.c
+++ b/page_cache.c
@@ -60,8 +60,12 @@ PageCache *cache_init(int64_t num_pages, unsigned int page_size)
         return NULL;
     }

-    cache = g_malloc(sizeof(*cache));
-
+    /* We prefer not to abort if there is no memory */
+    cache = g_try_malloc(sizeof(*cache));
+    if (!cache) {
+        DPRINTF("Failed to allocate cache\n");
+        return NULL;
+    }
     /* round down to the nearest power of 2 */
     if (!is_power_of_2(num_pages)) {
         num_pages = pow2floor(num_pages);
@@ -74,8 +78,14 @@ PageCache *cache_init(int64_t num_pages, unsigned int page_size)

     DPRINTF("Setting cache buckets to %" PRId64 "\n", cache->max_num_items);

-    cache->page_cache = g_malloc((cache->max_num_items) *
-                                 sizeof(*cache->page_cache));
+    /* We prefer not to abort if there is no memory */
+    cache->page_cache = g_try_malloc((cache->max_num_items) *
+                                     sizeof(*cache->page_cache));
+    if (!cache->page_cache) {
+        DPRINTF("Failed to allocate cache->page_cache\n");
+        g_free(cache);
+        return NULL;
+    }

     for (i = 0; i < cache->max_num_items; i++) {
         cache->page_cache[i].it_data = NULL;
-- 
1.8.5.3

[Qemu-devel] [PATCH 8/8] Don't abort on memory allocation error

[Juan Quintela]

From: Orit Wasserman <owasserm@redhat.com>

It is better to fail migration in case of failure to
allocate new cache item

Signed-off-by: Orit Wasserman <owasserm@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c                    |  4 +++-
 include/migration/page_cache.h |  4 +++-
 page_cache.c                   | 16 +++++++++++-----
 3 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 1fa5f1f..80574a0 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -284,7 +284,9 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t *current_data,

     if (!cache_is_cached(XBZRLE.cache, current_addr)) {
         if (!last_stage) {
-            cache_insert(XBZRLE.cache, current_addr, current_data);
+            if (cache_insert(XBZRLE.cache, current_addr, current_data) == -1) {
+                return -1;
+            }
         }
         acct_info.xbzrle_cache_miss++;
         return -1;
diff --git a/include/migration/page_cache.h b/include/migration/page_cache.h
index 87894fe..d156f0d 100644
--- a/include/migration/page_cache.h
+++ b/include/migration/page_cache.h
@@ -60,11 +60,13 @@ uint8_t *get_cached_data(const PageCache *cache, uint64_t addr);
  * cache_insert: insert the page into the cache. the page cache
  * will dup the data on insert. the previous value will be overwritten
  *
+ * Returns -1 on error
+ *
  * @cache pointer to the PageCache struct
  * @addr: page address
  * @pdata: pointer to the page
  */
-void cache_insert(PageCache *cache, uint64_t addr, uint8_t *pdata);
+int cache_insert(PageCache *cache, uint64_t addr, uint8_t *pdata);

 /**
  * cache_resize: resize the page cache. In case of size reduction the extra
diff --git a/page_cache.c b/page_cache.c
index 62a53f8..3ef6ee7 100644
--- a/page_cache.c
+++ b/page_cache.c
@@ -150,7 +150,7 @@ uint8_t *get_cached_data(const PageCache *cache, uint64_t addr)
     return cache_get_by_addr(cache, addr)->it_data;
 }

-void cache_insert(PageCache *cache, uint64_t addr, uint8_t *pdata)
+int cache_insert(PageCache *cache, uint64_t addr, uint8_t *pdata)
 {

     CacheItem *it = NULL;
@@ -161,16 +161,22 @@ void cache_insert(PageCache *cache, uint64_t addr, uint8_t *pdata)
     /* actual update of entry */
     it = cache_get_by_addr(cache, addr);

-    /* free old cached data if any */
-    g_free(it->it_data);
-
+    /* allocate page */
     if (!it->it_data) {
+        it->it_data = g_try_malloc(cache->page_size);
+        if (!it->it_data) {
+            DPRINTF("Error allocating page\n");
+            return -1;
+        }
         cache->num_items++;
     }

-    it->it_data = g_memdup(pdata, cache->page_size);
+    memcpy(it->it_data, pdata, cache->page_size);
+
     it->it_age = ++cache->max_item_age;
     it->it_addr = addr;
+
+    return 0;
 }

 int64_t cache_resize(PageCache *cache, int64_t new_num_pages)
-- 
1.8.5.3

Re: [Qemu-devel] [PULL v2 0/8] migration queue

[Peter Maydell]

On 4 February 2014 15:55, Juan Quintela <quintela@redhat.com> wrote:
> Hi
>
> After sending the 1st version of this pull, I noticed that I
> integrated the wrong version of Orit patches.  Resubmitting with the
> correct one.
>
> As a bonus, it includes the reviews-by from David O:-)
>
> Sorry for the inconveniences, Juan.
>
> [v1]
>
> This includes:
> - Peter changes to make VMSTATE_STRUCT_POINTER more consistent
> - Fix migration with hpratio (ppc on ppc64 basically)
> - Orit/Arei cleanups/fixes to xbzrle
>
>
> The following changes since commit 8cfc114a2f293c40077d1bdb7500b29db359ca22:
>
>   linux-user: Fix trampoline code for CRIS (2014-02-03 14:04:00 +0000)
>
> are available in the git repository at:
>
>   git://github.com/juanquintela/qemu.git tags/migration/20140204-1

Thanks, applied.

-- PMM