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: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 ?)


-- 

Thanks,

David / dhildenb