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

[David Hildenbrand]

On 31.03.20 14:16, Christian Borntraeger wrote:
> 
> 
> 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)

I'd say if the guest cannot detect it *right now*, we should error out
(because that's not what the user asked for).

Anyhow, whatever we do, we should add it to the changelog.

-- 
Thanks,

David / dhildenb