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 <imamm...@redhat.com> >>> --- >>> 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 <da...@redhat.com> >> >> 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