On 28.02.20 17:49, Shameerali Kolothum Thodi wrote: > > >> -----Original Message----- >> From: David Hildenbrand [mailto:da...@redhat.com] >> Sent: 13 February 2020 17:09 >> To: Shameerali Kolothum Thodi <shameerali.kolothum.th...@huawei.com>; >> Igor Mammedov <imamm...@redhat.com> >> Cc: peter.mayd...@linaro.org; xiaoguangrong.e...@gmail.com; >> m...@redhat.com; shannon.zha...@gmail.com; qemu-devel@nongnu.org; >> xuwei (O) <xuw...@huawei.com>; Linuxarm <linux...@huawei.com>; >> eric.au...@redhat.com; qemu-...@nongnu.org; ler...@redhat.com; >> dgilb...@redhat.com; Juan Jose Quintela Carreira <quint...@redhat.com> >> Subject: Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback > > [...] > >>>> Thanks for that. I had a go with the below patch and it indeed fixes the >>>> issue >>>> of callback not being called on resize. But the migration fails with the >>>> below >>>> error, >>>> >>>> For x86 >>>> --------- >>>> qemu-system-x86_64: Unknown combination of migration flags: 0x14 >>>> qemu-system-x86_64: error while loading state for instance 0x0 of device >> 'ram' >>>> qemu-system-x86_64: load of migration failed: Invalid argument >>>> >>>> For arm64 >>>> -------------- >>>> qemu-system-aarch64: Received an unexpected compressed page >>>> qemu-system-aarch64: error while loading state for instance 0x0 of device >> 'ram' >>>> qemu-system-aarch64: load of migration failed: Invalid argument >>>> >>>> I haven’t debugged this further but looks like there is a corruption >> happening. >>>> Please let me know if you have any clue. >>> >>> The issue is >>> >>> qemu_put_be64(f, ram_bytes_total_common(true) | >> RAM_SAVE_FLAG_MEM_SIZE) >>> >>> The total ram size we store must be page aligned, otherwise it will be >>> detected as flags. Hm ... maybe we can round it up ... >>> >> >> I'm afraid we can't otherwise we will run into issues in >> ram_load_precopy(). Hm ... > > Sorry, took a while to get back on this. Yes, round up indeed breaks in > ram_load_precopy() . I had the below on top of your patch and that > seems to do the job (sanity tested on arm/virt). > > Please take a look and let me know if you see any issues with this approach. > > Thanks, > Shameer > > diff --git a/migration/ram.c b/migration/ram.c > index 2acc4b85ca..7447f0cefa 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -1782,7 +1782,7 @@ static uint64_t ram_bytes_total_migration(void) > RCU_READ_LOCK_GUARD(); > > RAMBLOCK_FOREACH_MIGRATABLE(block) { > - total += ramblock_ram_bytes_migration(block); > + total += block->used_length; > } > return total; > } > @@ -3479,7 +3479,7 @@ static int ram_load_precopy(QEMUFile *f) > ret = -EINVAL; > } > > - total_ram_bytes -= length; > + total_ram_bytes -= block->used_length; > } > break; > > >
What you mean is the following: commit 702f4325086c3a8d6083787f8bc8503f7523bac8 (HEAD -> master) Author: David Hildenbrand <da...@redhat.com> Date: Wed Feb 12 19:16:34 2020 +0100 tmp Signed-off-by: David Hildenbrand <da...@redhat.com> diff --git a/exec.c b/exec.c index 67e520d18e..cec643b914 100644 --- a/exec.c +++ b/exec.c @@ -2125,11 +2125,21 @@ static int memory_try_enable_merging(void *addr, size_t len) */ int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp) { + const ram_addr_t unaligned_size = newsize; + assert(block); newsize = HOST_PAGE_ALIGN(newsize); if (block->used_length == newsize) { + /* + * We don't have to resize the ram block (which only knows aligned + * sizes), however, we have to notify if the unaligned size changed. + */ + if (block->resized && unaligned_size != memory_region_size(block->mr)) { + block->resized(block->idstr, unaligned_size, block->host); + memory_region_set_size(block->mr, unaligned_size); + } return 0; } @@ -2153,9 +2163,9 @@ int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp) block->used_length = newsize; cpu_physical_memory_set_dirty_range(block->offset, block->used_length, DIRTY_CLIENTS_ALL); - memory_region_set_size(block->mr, newsize); + memory_region_set_size(block->mr, unaligned_size); if (block->resized) { - block->resized(block->idstr, newsize, block->host); + block->resized(block->idstr, unaligned_size, block->host); } return 0; } diff --git a/migration/ram.c b/migration/ram.c index d2208b5534..249d3edede 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -3412,7 +3412,15 @@ static int ram_save_setup(QEMUFile *f, void *opaque) RAMBLOCK_FOREACH_MIGRATABLE(block) { qemu_put_byte(f, strlen(block->idstr)); qemu_put_buffer(f, (uint8_t *)block->idstr, strlen(block->idstr)); - qemu_put_be64(f, block->used_length); + /* + * When resizing on the target, we need the unaligned size, + * otherwise we lose the unaligned sise during migration. + * + * Note: The sum of all ram blocks will differ to + * ram_bytes_total_common(true) stored above. + */ + qemu_put_be64(f, ramblock_ram_bytes_migration(block)); + if (migrate_postcopy_ram() && block->page_size != qemu_host_page_size) { qemu_put_be64(f, block->page_size); @@ -4429,7 +4437,7 @@ static int ram_load_precopy(QEMUFile *f) ret = -EINVAL; } - total_ram_bytes -= length; + total_ram_bytes -= block->used_length; } break; Please note that this will *for sure* break migration between QEMU versions. So I don't think this will work. We should instead think about 1. Migrating the actual size of the 3 memory regions separately and setting them via memory_region_ram_resize() when loading the vmstate. This will trigger another FW cfg fixup and should be fine (with the same qemu_ram_resize() above). 2. Introduce a new RAM_SAVE_FLAG_MEM_SIZE_2, that e.g., stores the number of ramblocks, not the total amount of memory of the ram blocks. But it's hacky, because we migrate something for RAM blocks, that is not a RAM block concept (sub-block sizes). I think you should look into 1. Shouldn't be too hard I think. -- Thanks, David / dhildenb