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

[Christian Borntraeger]

On 01.04.20 10:58, David Hildenbrand wrote:
> On 01.04.20 10:50, Christian Borntraeger wrote:
>> Older QEMU versions did fixup the ram size to match what can be reported
>> via sclp. We need to mimic this behaviour for machine types 4.2 and
>> older to not fail on inbound migration for memory sizes that do not fit.
>> Old machines with proper aligned memory sizes are not affected.
>>
>> Alignment table:
>>  VM size (<=) | Alignment
>> --------------------------
>>       1020M   |     1M
>>       2040M   |     2M
>>       4080M   |     4M
>>       8160M   |     8M
>>      16320M   |    16M
>>      32640M   |    32M
>>      65280M   |    64M
>>     130560M   |   128M
>>     261120M   |   256M
>>     522240M   |   512M
>>    1044480M   |     1G
>>    2088960M   |     2G
>>    4177920M   |     4G
>>    8355840M   |     8G
>>
>> Suggested action is to replace unaligned -m value with a suitable
>> aligned one or to use a machine version >= 5.0 as future versions might
>> remove the compatibility handling.
>>
>> For machine types >= 5.0 we can simply use an increment size of 1M and
>> use the full range of increment number which allows for all possible
>> memory sizes. The old limitation of having a maximum of 1020 increments
>> was added for standby memory, which we no longer support. With that we
>> can now support even weird memory sizes like 10001234 MB.
>>
> 
> You should probably add while the maxram_size changes are done.

Yes, I need to mention the maxram changes.

> 
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -27,6 +27,7 @@
>>  #include "qemu/ctype.h"
>>  #include "qemu/error-report.h"
>>  #include "qemu/option.h"
>> +#include "qemu/qemu-print.h"
>>  #include "s390-pci-bus.h"
>>  #include "sysemu/reset.h"
>>  #include "hw/s390x/storage-keys.h"
>> @@ -579,6 +580,23 @@ static void s390_nmi(NMIState *n, int cpu_index, Error 
>> **errp)
>>      s390_cpu_restart(S390_CPU(cs));
>>  }
>>  
>> +static ram_addr_t s390_fixup_ram_size(ram_addr_t sz)
>> +{
>> +    /* same logic as in sclp.c */
>> +    int increment_size = 20;
> 
> missing empty line

ack.

 
>> +    while ((sz >> increment_size) > MAX_STORAGE_INCREMENTS) {
>> +        increment_size++;
>> +    }
>> +    if (sz != sz >> increment_size << increment_size) {
> 
> I'd just cache the calculation so ...

something like

    while ((sz >> increment_size) > MAX_STORAGE_INCREMENTS) {
        increment_size++;
    }
    newsz = sz >> increment_size << increment_size;
    
    if (sz != newsz) {
        qemu_printf("Ram size %" PRIu64 "MB was fixed up to %" PRIu64
                    "MB to match machine restrictions. Consider updating "
                    "the guest definition.\n",
                    sz / MiB, newsz / MiB);
    }
    return newsz;

?

> 
>> +        qemu_printf("Ram size %" PRIu64 "MB was fixed up to %" PRIu64
>> +                    "MB to match machine restrictions. Consider updating "
>> +                    "the guest definition.\n",
>> +                    sz / 1048576,
>> +                    (sz >> increment_size << increment_size) / 1048576);
> 
> .. this looks less ugly.
> 
> Please use MiB instead of 1048576. (see my original patch)

ack.
 

>> +    }
>> +    return sz >> increment_size << increment_size;
>> +}
>> +
>>  static void ccw_machine_class_init(ObjectClass *oc, void *data)
>>  {
>>      MachineClass *mc = MACHINE_CLASS(oc);
>> @@ -808,6 +826,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->fixup_ram_size = s390_fixup_ram_size;
>>      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..d843c8fea2 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -346,7 +346,7 @@ static void sclp_realize(DeviceState *dev, Error **errp)
>>       */
>>      qdev_set_parent_bus(DEVICE(sclp->event_facility), sysbus_get_default());
>>  
>> -    ret = s390_set_memory_limit(machine->maxram_size, &hw_limit);
>> +    ret = s390_set_memory_limit(machine->ram_size, &hw_limit);
> 
> I mentioned in my patch why I left this as is. But we can change that
> with memory hotplug support.

Ok, will drop this.

> 
> 
>>      if (ret == -E2BIG) {
>>          error_setg(&err, "host supports a maximum of %" PRIu64 " GB",
>>                     hw_limit / GiB);
>> @@ -361,27 +361,20 @@ out:
>>  static void sclp_memory_init(SCLPDevice *sclp)
>>  {
>>      MachineState *machine = MACHINE(qdev_get_machine());
>> +    MachineClass *machine_class = MACHINE_GET_CLASS(qdev_get_machine());
>>      ram_addr_t initial_mem = machine->ram_size;
>>      int increment_size = 20;
>>  
>>      /* The storage increment size is a multiple of 1M and is a power of 2.
>> -     * The number of storage increments must be MAX_STORAGE_INCREMENTS or 
>> fewer.
>> +     * For some machine types, the number of storage increments must be
>> +     * MAX_STORAGE_INCREMENTS or fewer.
>>       * The variable 'increment_size' is an exponent of 2 that can be
>>       * used to calculate the size (in bytes) of an increment. */
>> -    while ((initial_mem >> increment_size) > MAX_STORAGE_INCREMENTS) {
>> +    while (machine_class->fixup_ram_size != NULL &&
>> +           (initial_mem >> increment_size) > MAX_STORAGE_INCREMENTS) {
>>          increment_size++;
>>      }
>>      sclp->increment_size = increment_size;
> 
> IIRC one could define ram size in KB. Not sure if it is worth checking
> against that.
> [...]

If this is not aligned to 1MB, that would already fail when registering the 
memslot, I think.


> 
>>  static void sclp_init(Object *obj)
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index 236d239c19..0532143327 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -152,6 +152,12 @@ typedef struct {
>>   *    It also will be used as a way to optin into "-m" option support.
>>   *    If it's not set by board, '-m' will be ignored and generic code will
>>   *    not create default RAM MemoryRegion.
>> + * @fixup_ram_size:
>> + *    amends user provided ram size (with -m option) using machine
> 
> s/amends/Amends/

ack
 
>> + *    specific algorithm. to be used by old machine types for compat
> 
> s/to/To/


ack

>> + *    purposes only.
>> + *    Applies only to default memory backend, i.e. explicit memory backend
> 
> "i.e.,"

ack