From 20bcf73fa80c3477b6aaf5f39f18f031ff55de92 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Wed, 1 Jan 2014 21:56:57 +0000 Subject: [PATCH 1/8] vmstate: Make VMSTATE_STRUCT_POINTER take type, not ptr-to-type 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 Signed-off-by: Juan Quintela --- 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 02b7016a04..25ec549e71 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 a33edf4b0c..8ebacbbda0 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 be193baba1..fbd16a03e6 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) {\ From f9ee9f9ac28d6964772c08d5d428b713d58a3aca Mon Sep 17 00:00:00 2001 From: Alexey Kardashevskiy Date: Thu, 30 Jan 2014 23:03:50 +1100 Subject: [PATCH 2/8] exec: fix ram_list dirty map optimization 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 Signed-off-by: Juan Quintela --- 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 481a447417..2edfa96c6d 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); From f6c6483b259a2395ee44cfa966f622e0f2dbe2ae Mon Sep 17 00:00:00 2001 From: Orit Wasserman Date: Thu, 30 Jan 2014 20:08:33 +0200 Subject: [PATCH 3/8] Set xbzrle buffers to NULL after freeing them to avoid double free errors Signed-off-by: Orit Wasserman Reviewed-by: Juan Quintela Reviewed-by: Eric Blake Signed-off-by: Juan Quintela --- arch_init.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/arch_init.c b/arch_init.c index 77912e7a7d..66f5e82263 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; } } From c91e681a558fc21073ffc491b5a022d5f340fa0b Mon Sep 17 00:00:00 2001 From: Orit Wasserman Date: Thu, 30 Jan 2014 20:08:34 +0200 Subject: [PATCH 4/8] Add check for cache size smaller than page size Signed-off-by: Orit Wasserman Reviewed-by: Juan Quintela Signed-off-by: Juan Quintela --- 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 66f5e82263..8edeabee4c 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 7235c23ffe..84587e9e5a 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) From 905f26f2221e139ac0e7317ddac158c50f5cf876 Mon Sep 17 00:00:00 2001 From: "Gonglei (Arei)" Date: Thu, 30 Jan 2014 20:08:35 +0200 Subject: [PATCH 5/8] migration:fix free XBZRLE decoded_buf wrong 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 Reviewed-by: Peter Maydell Reviewed-by: Orit Wasserman Signed-off-by: GongLei Signed-off-by: Juan Quintela --- 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 8edeabee4c..5eff80b2a8 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 bfa3951a61..3e1e6c72bf 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 84587e9e5a..46a7305395 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); From a5615b14a66e86f620e90c8f4b3537c28bb328d4 Mon Sep 17 00:00:00 2001 From: Orit Wasserman Date: Thu, 30 Jan 2014 20:08:36 +0200 Subject: [PATCH 6/8] XBZRLE cache size should not be larger than guest memory size Signed-off-by: Orit Wasserman Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Juan Quintela --- migration.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/migration.c b/migration.c index 46a7305395..25add6f9e2 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", From a17b2fd3580d1da96e806c8b58e61255e8c57577 Mon Sep 17 00:00:00 2001 From: Orit Wasserman Date: Thu, 30 Jan 2014 20:08:37 +0200 Subject: [PATCH 7/8] Don't abort on out of memory when creating page cache Signed-off-by: Orit Wasserman Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Juan Quintela --- 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 5eff80b2a8..1fa5f1fdd4 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 a05db643cc..62a53f8c89 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; From 89db9987c07977bdb78d5d4b41d65e7acb9a5a2c Mon Sep 17 00:00:00 2001 From: Orit Wasserman Date: Thu, 30 Jan 2014 20:08:38 +0200 Subject: [PATCH 8/8] Don't abort on memory allocation error It is better to fail migration in case of failure to allocate new cache item Signed-off-by: Orit Wasserman Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Juan Quintela --- 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 1fa5f1fdd4..80574a090c 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 87894fea9f..d156f0d398 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 62a53f8c89..3ef6ee7ad2 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)