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

[Christian Borntraeger]

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