Re: [qemu-s390x] [PATCH for-4.2 v3 2/2] s390: do not call memory_region_allocate_system_memory() multiple times

[David Hildenbrand]

On 02.08.19 15:41, Christian Borntraeger wrote:
> 
> 
> On 02.08.19 15:36, David Hildenbrand wrote:
>> On 02.08.19 15:32, 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.
>>>
>>> Beside an invalid use of API, the approach also introduced migration
>>> issue, since RAM chunks for each KVM_SLOT_MAX_BYTES are transferred in
>>> migration stream as separate RAMBlocks.
>>>
>>> After discussion [1], it was agreed to break migration from older
>>> QEMU for guest with RAM >8Tb (as it was relatively new (since 2.12)
>>> and considered to be not actually used downstream).
>>> Migration should keep working for guests with less than 8TB and for
>>> more than 8TB with QEMU 4.2 and newer binary.
>>> In case user tries to migrate more than 8TB guest, between incompatible
>>> QEMU versions, migration should fail gracefully due to non-exiting
>>> RAMBlock ID or RAMBlock size mismatch.
>>>
>>> Taking in account above and that now KVM code is able to split too
>>> big MemorySection into several memslots, stop abusing
>>>   memory_region_allocate_system_memory()
>>> and use only one memory region for RAM.
>>>
>>> 1) [PATCH RFC v2 4/4] s390: do not call  
>>> memory_region_allocate_system_memory() multiple times
>>>
>>> Signed-off-by: Igor Mammedov <address@hidden>
>>> ---
>>> v3:
>>>   - drop migration compat code
>>>
>>> PS:
>>> I don't have access to a suitable system to test it.
>>> ---
>>>  hw/s390x/s390-virtio-ccw.c | 21 +++------------------
>>>  1 file changed, 3 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>> index 0c03ffb7c7..e30058df38 100644
>>> --- a/hw/s390x/s390-virtio-ccw.c
>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>> @@ -154,27 +154,12 @@ static void virtio_ccw_register_hcalls(void)
>>>  static void s390_memory_init(ram_addr_t mem_size)
>>>  {
>>>      MemoryRegion *sysmem = get_system_memory();
>>> -    ram_addr_t chunk, offset = 0;
>>> -    unsigned int number = 0;
>>> +    MemoryRegion *ram = g_new(MemoryRegion, 1);
>>>      Error *local_err = NULL;
>>> -    gchar *name;
>>>  
>>>      /* 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);
>>> -    }
>>> -    g_free(name);
>>> +    memory_region_allocate_system_memory(ram, NULL, "s390.ram", mem_size);
>>> +    memory_region_add_subregion(sysmem, 0, ram);
>>>  
>>>      /*
>>>       * Configure the maximum page size. As no memory devices were created
>>>
>>
>> Reviewed-by: David Hildenbrand <address@hidden>
>>
>> We have to remember putting it into the next release notes. (or do we
>> even want to get this into v4.1 ?)
> 
> This is too invasive for 4.1
> 

Makes sense

-- 

Thanks,

David / dhildenb