Re: [Qemu-devel] [PATCH RFC v2 4/4] s390: do not call memory_region_allocate_system_memory() multiple times

[David Hildenbrand <david@redhat.com>]

On 02.08.19 11:38, Igor Mammedov wrote:
> s390 was trying to solve limited KVM 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. To workaround issue, MemoryRegion alases are made
> migratable and this patch switches to use them to split big initial
> RAM chunk into smaller pieces (KVM_SLOT_MAX_BYTES max) and registers
> aliases for migration. That should keep migration compatible with
> previous QEMU versions.
> 
> New machine types (since 4.2) will use single memory region, which
> will get transimitted in migration stream as a whole RAMBlock.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> I 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.
> ---
>  include/hw/s390x/s390-virtio-ccw.h |  4 +++
>  hw/s390x/s390-virtio-ccw.c         | 56 +++++++++++++++++++++---------
>  2 files changed, 43 insertions(+), 17 deletions(-)
> 
> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
> index 00632f94b4..f9ed3737f8 100644
> --- a/include/hw/s390x/s390-virtio-ccw.h
> +++ b/include/hw/s390x/s390-virtio-ccw.h
> @@ -21,6 +21,9 @@
>  #define S390_MACHINE_CLASS(klass) \
>      OBJECT_CLASS_CHECK(S390CcwMachineClass, (klass), TYPE_S390_CCW_MACHINE)
>  
> +#define S390_MACHINE_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(S390CcwMachineClass, (obj), TYPE_S390_CCW_MACHINE)
> +
>  /*
>   * KVM does only support memory slots up to KVM_MEM_MAX_NR_PAGES pages
>   * as the dirty bitmap must be managed by bitops that take an int as
> @@ -50,6 +53,7 @@ typedef struct S390CcwMachineClass {
>      bool cpu_model_allowed;
>      bool css_migration_enabled;
>      bool hpage_1m_allowed;
> +    bool split_ram_layout;
>  } S390CcwMachineClass;
>  
>  /* runtime-instrumentation allowed by the machine */
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 073672f9cb..9160c1ed0a 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -151,28 +151,47 @@ static void virtio_ccw_register_hcalls(void)
>                                     virtio_ccw_hcall_early_printk);
>  }
>  
> -static void s390_memory_init(ram_addr_t mem_size)
> +static void s390_memory_init(MachineState *ms)
>  {
> +    S390CcwMachineClass *s390mc = S390_MACHINE_GET_CLASS(ms);
>      MemoryRegion *sysmem = get_system_memory();
> -    ram_addr_t chunk, offset = 0;
> +    MemoryRegion *ram = g_new(MemoryRegion, 1);
>      unsigned int number = 0;
>      Error *local_err = NULL;
> -    gchar *name;
> +    ram_addr_t mem_size = ms->ram_size;
> +    gchar *name = g_strdup_printf(s390mc->split_ram_layout ?
> +                                  "s390.whole.ram" : "s390.ram");
>  
>      /* allocate RAM for core */
> -    name = g_strdup_printf("s390.ram");
> -    while (mem_size) {
> -        MemoryRegion *ram = g_new(MemoryRegion, 1);
> -        uint64_t size = mem_size;
> -
> -        /* 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);
> -        mem_size -= chunk;
> -        offset += chunk;
> -        g_free(name);
> -        name = g_strdup_printf("s390.ram.%u", ++number);
> +    memory_region_allocate_system_memory(ram, NULL, name, mem_size);
> +
> +    /* migration compatible RAM handling for 4.1 and older machines */
> +    if (s390mc->split_ram_layout) {
> +       ram_addr_t chunk, offset = 0;
> +       /*
> +        * 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 care
> +        * of segmenting RAM into legacy chunks that migration compatible.
> +        */
> +       vmstate_unregister_ram(ram, NULL);
> +       name = g_strdup_printf("s390.ram");
> +       while (mem_size) {
> +           MemoryRegion *alias = g_new(MemoryRegion, 1);
> +
> +           /* KVM does not allow memslots >= 8 TB */
> +           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);
> +           name = g_strdup_printf("s390.ram.%u", ++number);
> +       }
> +    } else {
> +       memory_region_add_subregion(sysmem, 0, ram);
>      }
>      g_free(name);
>  
> @@ -257,7 +276,7 @@ static void ccw_init(MachineState *machine)
>  
>      s390_sclp_init();
>      /* init memory + setup max page size. Required for the CPU model */
> -    s390_memory_init(machine->ram_size);
> +    s390_memory_init(machine);
>  
>      /* init CPUs (incl. CPU model) early so s390_has_feature() works */
>      s390_init_cpus(machine);
> @@ -667,8 +686,11 @@ static void ccw_machine_4_1_instance_options(MachineState *machine)
>  
>  static void ccw_machine_4_1_class_options(MachineClass *mc)
>  {
> +    S390CcwMachineClass *s390mc = S390_MACHINE_CLASS(mc);
> +
>      ccw_machine_4_2_class_options(mc);
>      compat_props_add(mc->compat_props, hw_compat_4_1, hw_compat_4_1_len);
> +    s390mc->split_ram_layout = true;
>  }
>  DEFINE_CCW_MACHINE(4_1, "4.1", false);
>  
> 

As discussed, I am not sure if adding that compat code really is worth
it. :)

-- 

Thanks,

David / dhildenb