Re: [PATCH/RFC] vl/s390: fixup ram sizes for compat machines

[Christian Borntraeger]

On 31.03.20 14:13, David Hildenbrand wrote:
> On 31.03.20 14:02, Christian Borntraeger wrote:
>> compat machines did fixup the ram size to match what can be reported via
>> sclp. We need to mimic those for machines 4.2 and older to not fail on
>> inbound migration.
>>
>> Fixes: 3a12fc61af5c ("390x/s390-virtio-ccw: use memdev for RAM")
>> Reported-by: Lukáš Doktor <address@hidden>
>> Cc: Igor Mammedov <address@hidden>
>> Cc: Dr. David Alan Gilbert <address@hidden>
>> Signed-off-by: David Hildenbrand <address@hidden>
>> Signed-off-by: Christian Borntraeger <address@hidden>
>> ---
>>  hw/s390x/s390-virtio-ccw.c | 12 ++++++++++++
>>  hw/s390x/sclp.c            | 10 ----------
>>  include/hw/boards.h        |  1 +
>>  softmmu/vl.c               |  3 +++
>>  4 files changed, 16 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 3cf19c99f3..748c08b157 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -579,6 +579,17 @@ static void s390_nmi(NMIState *n, int cpu_index, Error 
>> **errp)
>>      s390_cpu_restart(S390_CPU(cs));
>>  }
>>  
>> +#define MAX_STORAGE_INCREMENTS                  1020
>> +static ram_addr_t s390_align_ram(ram_addr_t sz)
>> +{
>> +    /* same logic as in sclp.c */
>> +    int increment_size = 20;
>> +    while ((sz >> increment_size) > MAX_STORAGE_INCREMENTS) {
>> +        increment_size++;
>> +    }
>> +    return sz >> increment_size << increment_size;
>> +}
>> +
>>  static void ccw_machine_class_init(ObjectClass *oc, void *data)
>>  {
>>      MachineClass *mc = MACHINE_CLASS(oc);
>> @@ -808,6 +819,7 @@ static void 
>> ccw_machine_4_2_instance_options(MachineState *machine)
>>  static void ccw_machine_4_2_class_options(MachineClass *mc)
>>  {
>>      ccw_machine_5_0_class_options(mc);
>> +    mc->machine_align_ram = s390_align_ram;
>>      compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
>>  }
>>  DEFINE_CCW_MACHINE(4_2, "4.2", false);
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index d8ae207731..0a6ff2be82 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -372,16 +372,6 @@ static void sclp_memory_init(SCLPDevice *sclp)
>>          increment_size++;
>>      }
>>      sclp->increment_size = increment_size;
>> -
>> -    /* The core memory area needs to be aligned with the increment size.
>> -     * In effect, this can cause the user-specified memory size to be 
>> rounded
>> -     * down to align with the nearest increment boundary. */
>> -    initial_mem = initial_mem >> increment_size << increment_size;
>> -
>> -    machine->ram_size = initial_mem;
>> -    machine->maxram_size = initial_mem;
>> -    /* let's propagate the changed ram size into the global variable. */
>> -    ram_size = initial_mem;
>>  }
>>  
>>  static void sclp_init(Object *obj)
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index 236d239c19..e3574f4b5f 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -218,6 +218,7 @@ struct MachineClass {
>>                                                           unsigned 
>> cpu_index);
>>      const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine);
>>      int64_t (*get_default_cpu_node_id)(const MachineState *ms, int idx);
>> +    ram_addr_t (*machine_align_ram)(ram_addr_t size);
>>  };
>>  
>>  /**
>> diff --git a/softmmu/vl.c b/softmmu/vl.c
>> index 1d33a28340..12b5758d12 100644
>> --- a/softmmu/vl.c
>> +++ b/softmmu/vl.c
>> @@ -2601,6 +2601,9 @@ static bool set_memory_options(uint64_t *ram_slots, 
>> ram_addr_t *maxram_size,
>>      }
>>  
>>      sz = QEMU_ALIGN_UP(sz, 8192);
>> +    if (mc->machine_align_ram) {
>> +        sz = mc->machine_align_ram(sz);
>> +    }
>>      ram_size = sz;
>>      if (ram_size != sz) {
>>          error_report("ram size too large");
>>
> 
> 1. You're missing the maxram changes from my patch.

Yes, will fixup the remaining things. 
> 
> 2. Shouldn't we error out in case ram_size is not aligned in sclp.c (new
> machines)?

I think its perfectly fine to have slightly larger ram than what sclp reports. 
Maybe a future
sclp extension will improve this? (In fact since we no longer have sclp memory 
hotplug we COULD
use more than 1020 increments)