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

[David Hildenbrand]

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.

> +++ 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

> +    while ((sz >> increment_size) > MAX_STORAGE_INCREMENTS) {
> +        increment_size++;
> +    }
> +    if (sz != sz >> increment_size << increment_size) {

I'd just cache the calculation so ...

> +        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)

> +    }
> +    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.


>      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.
[...]

>  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/

> + *    specific algorithm. to be used by old machine types for compat

s/to/To/

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

"i.e.,"


-- 
Thanks,

David / dhildenb