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 <imamm...@redhat.com>
> ---
> 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. :)
--
Thanks,
David / dhildenb
