Re: [Qemu-devel] [PATCH v1 5/5] s390: do not call memory_region_allocate_system_memory() multiple times

[Christian Borntraeger]

This crashes

a simple -kernel -initrd example on s390x.

#0  0x000003ff94e3e47c in __GI_raise (address@hidden) at 
../sysdeps/unix/sysv/linux/raise.c:50
#1  0x000003ff94e23d18 in __GI_abort () at abort.c:79
#2  0x000003ff94e365e6 in __assert_fail_base
    (fmt=0x3ff94f60ca6 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", 
address@hidden "new_block", address@hidden "/home/cborntra/REPOS/qemu/exec.c", 
address@hidden, address@hidden <__PRETTY_FUNCTION__.34656> 
"qemu_ram_set_idstr") at assert.c:92
#3  0x000003ff94e36664 in __GI___assert_fail
    (address@hidden "new_block", address@hidden 
"/home/cborntra/REPOS/qemu/exec.c", address@hidden, address@hidden 
<__PRETTY_FUNCTION__.34656> "qemu_ram_set_idstr") at assert.c:101
#4  0x000000000102e062 in qemu_ram_set_idstr (address@hidden, name=<optimized 
out>, address@hidden) at /home/cborntra/REPOS/qemu/exec.c:2041
#5  0x00000000011f5b0a in vmstate_register_ram (mr=0x2cd2dd0, 
address@hidden<error reading variable: value has been optimized out>, 
address@hidden) at /home/cborntra/REPOS/qemu/migration/savevm.c:2828
#6  0x00000000011f5b5a in vmstate_register_ram_global (mr=<error reading 
variable: value has been optimized out>) at 
/home/cborntra/REPOS/qemu/migration/savevm.c:2841
#7  0x000000000110d2ce in s390_memory_init (mem_size=<optimized out>) at 
/home/cborntra/REPOS/qemu/hw/s390x/s390-virtio-ccw.c:186
#8  0x000000000110d2ce in ccw_init (machine=0x2a96770) at 
/home/cborntra/REPOS/qemu/hw/s390x/s390-virtio-ccw.c:266
#9  0x00000000011b342c in machine_run_board_init (machine=0x2a96770) at 
/home/cborntra/REPOS/qemu/hw/core/machine.c:1030
#10 0x0000000001026fee in main (argc=<optimized out>, argv=<optimized out>, 
envp=<optimized out>) at /home/cborntra/REPOS/qemu/vl.c:4479



On 15.04.19 15:27, Igor Mammedov wrote:
> s390 was trying to solve limited memslot size issue by abusing
> memory_region_allocate_system_memory(), which breaks API contract
> where the function might be called only once.
> 
> s390 should have used memory aliases to fragment inital memory into
> smaller chunks to satisfy KVM's memslot limitation. But its a bit
> late now, since allocated pieces are transfered in migration stream
> separately, so it's not possible to just replace broken layout with
> correct one. Previous patch made MemoryRegion alases migratable and
> this patch switches to use them to split big initial RAM chunk into
> smaller pieces up to KVM_SLOT_MAX_BYTES each and registers aliases
> for migration.
> 
> Signed-off-by: Igor Mammedov <address@hidden>
> ---
> A don't have access to a suitable system to test it, so I've simulated
> it with smaller chunks on x84 host. Ping-pong migration between old
> and new QEMU worked fine.  KVM part should be fine as memslots
> using mapped MemoryRegions (in this case it would be aliases) as
> far as I know but is someone could test it on big enough host it
> would be nice.
> ---
>  hw/s390x/s390-virtio-ccw.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index d11069b..12ca3a9 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -161,20 +161,30 @@ static void virtio_ccw_register_hcalls(void)
>  static void s390_memory_init(ram_addr_t mem_size)
>  {
>      MemoryRegion *sysmem = get_system_memory();
> +    MemoryRegion *ram = g_new(MemoryRegion, 1);
>      ram_addr_t chunk, offset = 0;
>      unsigned int number = 0;
>      gchar *name;
>  
>      /* allocate RAM for core */
> +    memory_region_allocate_system_memory(ram, NULL, "s390.whole.ram", 
> mem_size);
> +    /*
> +     * memory_region_allocate_system_memory() registers allocated RAM for
> +     * migration, however for compat reasons the RAM should be passed over
> +     * as RAMBlocks of the size upto KVM_SLOT_MAX_BYTES. So unregister just
> +     * allocated RAM so it won't be migrated directly. Aliases will take
> +     * of segmenting RAM into legacy chunks.
> +     */
> +    vmstate_unregister_ram(ram, NULL);
>      name = g_strdup_printf("s390.ram");
>      while (mem_size) {
> -        MemoryRegion *ram = g_new(MemoryRegion, 1);
> -        uint64_t size = mem_size;
> +        MemoryRegion *alias = g_new(MemoryRegion, 1);
>  
>          /* KVM does not allow memslots >= 8 TB */
> -        chunk = MIN(size, KVM_SLOT_MAX_BYTES);
> -        memory_region_allocate_system_memory(ram, NULL, name, chunk);
> -        memory_region_add_subregion(sysmem, offset, ram);
> +        chunk = MIN(mem_size, KVM_SLOT_MAX_BYTES);
> +        memory_region_init_alias(alias, NULL, name, ram, offset, chunk);
> +        vmstate_register_ram_global(alias);
> +        memory_region_add_subregion(sysmem, offset, alias);
>          mem_size -= chunk;
>          offset += chunk;
>          g_free(name);
>