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

[Christian Borntraeger]

On 02.08.19 13:40, Igor Mammedov wrote:
> On Fri, 2 Aug 2019 12:27:50 +0200
> Christian Borntraeger <address@hidden> wrote:
> 
>> On 02.08.19 12:25, David Hildenbrand wrote:
>>> 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 <address@hidden>
>>>> ---
>>>> 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. :)  
>>
>> Agreed. As long as forward migration (e.g. 4.2->4.3 will work) and we 
>> document
>> that migration will fails for 2.12->.....4.1 for guests >=8TB we  can 
>> simplify
>> things.
> 
> I'll drop 3/4 and compat code in this patch.
> 
> Christian,
> 
> could you test this series on big enough host to ensure that KVM memslot 
> splitting
> part works as expected (i.e. it's possible to start >8TB guest)?

Can you send a v3 with all changes?