diff mbox series

[v3,1/1] vl/s390x: fixup ram sizes for compat machines

Message ID 20200401123754.109602-1-borntraeger@de.ibm.com (mailing list archive)
State New, archived
Headers show
Series [v3,1/1] vl/s390x: fixup ram sizes for compat machines | expand

Commit Message

Christian Borntraeger April 1, 2020, 12:37 p.m. UTC
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 if a change to a newer machine type is possible, use a
machine version >= 5.0.

A 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.

As we no longer fixup maxram_size as well, make other users use ram_size
instead. Keep using maxram_size when setting the maximum ram size in KVM,
as that will come in handy in the future when supporting memory hotplug
(in contrast, storage keys and storage attributes for hotplugged memory
will have to be migrated per RAM block in the future).

Fixes: 3a12fc61af5c ("390x/s390-virtio-ccw: use memdev for RAM")
Reported-by: Lukáš Doktor <ldoktor@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/s390x/s390-skeys.c        |  2 +-
 hw/s390x/s390-stattrib-kvm.c |  4 ++--
 hw/s390x/s390-virtio-ccw.c   | 21 +++++++++++++++++++++
 hw/s390x/sclp.c              | 17 +++++------------
 include/hw/boards.h          |  7 +++++++
 softmmu/vl.c                 |  3 +++
 6 files changed, 39 insertions(+), 15 deletions(-)

Comments

Christian Borntraeger April 1, 2020, 12:46 p.m. UTC | #1
On 01.04.20 14:37, Christian Borntraeger wrote:
> +    if (sz != newsz) {
> +        qemu_printf("Ram size %" PRIu64 "MB was fixed up to %" PRIu64
> +                    "MB to match machine restrictions. Consider updating "
> +                    "the guest definition.i\n", sz / MiB, newsz / MiB);

                                             ^ spurious i.
David Hildenbrand April 1, 2020, 1:14 p.m. UTC | #2
On 01.04.20 14:37, 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 if a change to a newer machine type is possible, use a
> machine version >= 5.0.
> 
> A 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.
> 
> As we no longer fixup maxram_size as well, make other users use ram_size
> instead. Keep using maxram_size when setting the maximum ram size in KVM,
> as that will come in handy in the future when supporting memory hotplug
> (in contrast, storage keys and storage attributes for hotplugged memory
> will have to be migrated per RAM block in the future).
> 
> Fixes: 3a12fc61af5c ("390x/s390-virtio-ccw: use memdev for RAM")
> Reported-by: Lukáš Doktor <ldoktor@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  hw/s390x/s390-skeys.c        |  2 +-
>  hw/s390x/s390-stattrib-kvm.c |  4 ++--
>  hw/s390x/s390-virtio-ccw.c   | 21 +++++++++++++++++++++
>  hw/s390x/sclp.c              | 17 +++++------------
>  include/hw/boards.h          |  7 +++++++
>  softmmu/vl.c                 |  3 +++
>  6 files changed, 39 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
> index 5da6e5292f..a9a4ae7b39 100644
> --- a/hw/s390x/s390-skeys.c
> +++ b/hw/s390x/s390-skeys.c
> @@ -176,7 +176,7 @@ static void qemu_s390_skeys_init(Object *obj)
>      QEMUS390SKeysState *skeys = QEMU_S390_SKEYS(obj);
>      MachineState *machine = MACHINE(qdev_get_machine());
>  
> -    skeys->key_count = machine->maxram_size / TARGET_PAGE_SIZE;
> +    skeys->key_count = machine->ram_size / TARGET_PAGE_SIZE;
>      skeys->keydata = g_malloc0(skeys->key_count);
>  }
>  
> diff --git a/hw/s390x/s390-stattrib-kvm.c b/hw/s390x/s390-stattrib-kvm.c
> index c7e1f35524..f89d8d9d16 100644
> --- a/hw/s390x/s390-stattrib-kvm.c
> +++ b/hw/s390x/s390-stattrib-kvm.c
> @@ -85,7 +85,7 @@ static int kvm_s390_stattrib_set_stattr(S390StAttribState *sa,
>  {
>      KVMS390StAttribState *sas = KVM_S390_STATTRIB(sa);
>      MachineState *machine = MACHINE(qdev_get_machine());
> -    unsigned long max = machine->maxram_size / TARGET_PAGE_SIZE;
> +    unsigned long max = machine->ram_size / TARGET_PAGE_SIZE;
>  
>      if (start_gfn + count > max) {
>          error_report("Out of memory bounds when setting storage attributes");
> @@ -104,7 +104,7 @@ static void kvm_s390_stattrib_synchronize(S390StAttribState *sa)
>  {
>      KVMS390StAttribState *sas = KVM_S390_STATTRIB(sa);
>      MachineState *machine = MACHINE(qdev_get_machine());
> -    unsigned long max = machine->maxram_size / TARGET_PAGE_SIZE;
> +    unsigned long max = machine->ram_size / TARGET_PAGE_SIZE;
>      /* We do not need to reach the maximum buffer size allowed */
>      unsigned long cx, len = KVM_S390_SKEYS_MAX / 2;
>      int r;
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 3cf19c99f3..61a8a0e693 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ 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,25 @@ 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;
> +    ram_addr_t newsz;
> +
> +    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.i\n", sz / MiB, newsz / MiB);

Not sure if news/zs will be printed correctly in case ram_addr_t !=
uint64_t.

Thanks!

Reviewed-by: David Hildenbrand <david@redhat.com>
Igor Mammedov April 1, 2020, 4:34 p.m. UTC | #3
On Wed,  1 Apr 2020 08:37:54 -0400
Christian Borntraeger <borntraeger@de.ibm.com> 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 if a change to a newer machine type is possible, use a
> machine version >= 5.0.
> 
> A 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.
> 
> As we no longer fixup maxram_size as well, make other users use ram_size
> instead. Keep using maxram_size when setting the maximum ram size in KVM,
> as that will come in handy in the future when supporting memory hotplug
> (in contrast, storage keys and storage attributes for hotplugged memory
> will have to be migrated per RAM block in the future).
> 
> Fixes: 3a12fc61af5c ("390x/s390-virtio-ccw: use memdev for RAM")
> Reported-by: Lukáš Doktor <ldoktor@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>

Acked-by: Igor Mammedov <imammedo@redhat.com>

minor nit below if you have to respin

> ---
>  hw/s390x/s390-skeys.c        |  2 +-
>  hw/s390x/s390-stattrib-kvm.c |  4 ++--
>  hw/s390x/s390-virtio-ccw.c   | 21 +++++++++++++++++++++
>  hw/s390x/sclp.c              | 17 +++++------------
>  include/hw/boards.h          |  7 +++++++
>  softmmu/vl.c                 |  3 +++
>  6 files changed, 39 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
> index 5da6e5292f..a9a4ae7b39 100644
> --- a/hw/s390x/s390-skeys.c
> +++ b/hw/s390x/s390-skeys.c
> @@ -176,7 +176,7 @@ static void qemu_s390_skeys_init(Object *obj)
>      QEMUS390SKeysState *skeys = QEMU_S390_SKEYS(obj);
>      MachineState *machine = MACHINE(qdev_get_machine());
>  
> -    skeys->key_count = machine->maxram_size / TARGET_PAGE_SIZE;
> +    skeys->key_count = machine->ram_size / TARGET_PAGE_SIZE;
>      skeys->keydata = g_malloc0(skeys->key_count);
>  }
>  
> diff --git a/hw/s390x/s390-stattrib-kvm.c b/hw/s390x/s390-stattrib-kvm.c
> index c7e1f35524..f89d8d9d16 100644
> --- a/hw/s390x/s390-stattrib-kvm.c
> +++ b/hw/s390x/s390-stattrib-kvm.c
> @@ -85,7 +85,7 @@ static int kvm_s390_stattrib_set_stattr(S390StAttribState *sa,
>  {
>      KVMS390StAttribState *sas = KVM_S390_STATTRIB(sa);
>      MachineState *machine = MACHINE(qdev_get_machine());
> -    unsigned long max = machine->maxram_size / TARGET_PAGE_SIZE;
> +    unsigned long max = machine->ram_size / TARGET_PAGE_SIZE;
>  
>      if (start_gfn + count > max) {
>          error_report("Out of memory bounds when setting storage attributes");
> @@ -104,7 +104,7 @@ static void kvm_s390_stattrib_synchronize(S390StAttribState *sa)
>  {
>      KVMS390StAttribState *sas = KVM_S390_STATTRIB(sa);
>      MachineState *machine = MACHINE(qdev_get_machine());
> -    unsigned long max = machine->maxram_size / TARGET_PAGE_SIZE;
> +    unsigned long max = machine->ram_size / TARGET_PAGE_SIZE;
>      /* We do not need to reach the maximum buffer size allowed */
>      unsigned long cx, len = KVM_S390_SKEYS_MAX / 2;
>      int r;
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 3cf19c99f3..61a8a0e693 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ 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,25 @@ 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;
> +    ram_addr_t newsz;
> +
> +    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
                                                   ^^^^^^^^

for unaware  user it could be confusing as it could be read as 'value was increased'
s/fixed up/amended/ might be better

> +                    "MB to match machine restrictions. Consider updating "
> +                    "the guest definition.i\n", sz / MiB, newsz / MiB);

also it might be better to use size_to_str() to format numbers

> +    }
> +    return newsz;
> +}
> +
>  static void ccw_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> @@ -808,6 +828,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..ede056b3ef 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -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;
> -
> -    /* 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..fd4d62b501 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
> + *    specific algorithm. To be used by old machine types for compat
> + *    purposes only.
> + *    Applies only to default memory backend, i.e., explicit memory backend
> + *    wasn't used.
>   */
>  struct MachineClass {
>      /*< private >*/
> @@ -218,6 +224,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 (*fixup_ram_size)(ram_addr_t size);
>  };
>  
>  /**
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 1d33a28340..09f8a1b0a7 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->fixup_ram_size) {
> +        sz = mc->fixup_ram_size(sz);
> +    }
>      ram_size = sz;
>      if (ram_size != sz) {
>          error_report("ram size too large");
Cornelia Huck April 2, 2020, 9:22 a.m. UTC | #4
On Wed, 1 Apr 2020 15:14:12 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 01.04.20 14:37, Christian Borntraeger wrote:

> > +static ram_addr_t s390_fixup_ram_size(ram_addr_t sz)
> > +{
> > +    /* same logic as in sclp.c */
> > +    int increment_size = 20;
> > +    ram_addr_t newsz;
> > +
> > +    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.i\n", sz / MiB, newsz / MiB);  
> 
> Not sure if news/zs will be printed correctly in case ram_addr_t !=
> uint64_t.

RAM_ADDR_FMT might be the right thing to use here?
Christian Borntraeger April 2, 2020, 9:25 a.m. UTC | #5
On 02.04.20 11:22, Cornelia Huck wrote:
> On Wed, 1 Apr 2020 15:14:12 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 01.04.20 14:37, Christian Borntraeger wrote:
> 
>>> +static ram_addr_t s390_fixup_ram_size(ram_addr_t sz)
>>> +{
>>> +    /* same logic as in sclp.c */
>>> +    int increment_size = 20;
>>> +    ram_addr_t newsz;
>>> +
>>> +    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.i\n", sz / MiB, newsz / MiB);  
>>
>> Not sure if news/zs will be printed correctly in case ram_addr_t !=
>> uint64_t.
> 
> RAM_ADDR_FMT might be the right thing to use here?

I tried that but it returns a hex value in bytes. This is unusable. Thats why I divided
by MiB.  We could simply do an u64 cast?
Cornelia Huck April 2, 2020, 9:27 a.m. UTC | #6
On Wed, 1 Apr 2020 18:34:56 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Wed,  1 Apr 2020 08:37:54 -0400
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> > +static ram_addr_t s390_fixup_ram_size(ram_addr_t sz)
> > +{
> > +    /* same logic as in sclp.c */
> > +    int increment_size = 20;
> > +    ram_addr_t newsz;
> > +
> > +    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  
>                                                    ^^^^^^^^
> 
> for unaware  user it could be confusing as it could be read as 'value was increased'
> s/fixed up/amended/ might be better

"rounded", perhaps?

> 
> > +                    "MB to match machine restrictions. Consider updating "
> > +                    "the guest definition.i\n", sz / MiB, newsz / MiB);  
> 
> also it might be better to use size_to_str() to format numbers

The text explicitly talks about 'MB'... not sure if it would be
confusing if the user specified MB and ended up with GB or so in this
message.

> 
> > +    }
> > +    return newsz;
> > +}
> > +

(If we can agree upon message and format, I'll happily fix that up when
applying.)
Christian Borntraeger April 2, 2020, 9:39 a.m. UTC | #7
On 02.04.20 11:27, Cornelia Huck wrote:
> On Wed, 1 Apr 2020 18:34:56 +0200
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
>> On Wed,  1 Apr 2020 08:37:54 -0400
>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>>> +static ram_addr_t s390_fixup_ram_size(ram_addr_t sz)
>>> +{
>>> +    /* same logic as in sclp.c */
>>> +    int increment_size = 20;
>>> +    ram_addr_t newsz;
>>> +
>>> +    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  
>>                                                    ^^^^^^^^
>>
>> for unaware  user it could be confusing as it could be read as 'value was increased'
>> s/fixed up/amended/ might be better
> 
> "rounded", perhaps?
> 
>>
>>> +                    "MB to match machine restrictions. Consider updating "
>>> +                    "the guest definition.i\n", sz / MiB, newsz / MiB);  
>>
>> also it might be better to use size_to_str() to format numbers
> 
> The text explicitly talks about 'MB'... not sure if it would be
> confusing if the user specified MB and ended up with GB or so in this
> message.
> 
>>
>>> +    }
>>> +    return newsz;
>>> +}
>>> +
> 
> (If we can agree upon message and format, I'll happily fix that up when
> applying.)

Is the the only thing that blocks this? I would rather try to get this fixed before rc2.
Cornelia Huck April 2, 2020, 9:43 a.m. UTC | #8
On Thu, 2 Apr 2020 11:39:11 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 02.04.20 11:27, Cornelia Huck wrote:
> > On Wed, 1 Apr 2020 18:34:56 +0200
> > Igor Mammedov <imammedo@redhat.com> wrote:
> >   
> >> On Wed,  1 Apr 2020 08:37:54 -0400
> >> Christian Borntraeger <borntraeger@de.ibm.com> wrote:  
> >   
> >>> +static ram_addr_t s390_fixup_ram_size(ram_addr_t sz)
> >>> +{
> >>> +    /* same logic as in sclp.c */
> >>> +    int increment_size = 20;
> >>> +    ram_addr_t newsz;
> >>> +
> >>> +    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    
> >>                                                    ^^^^^^^^
> >>
> >> for unaware  user it could be confusing as it could be read as 'value was increased'
> >> s/fixed up/amended/ might be better  
> > 
> > "rounded", perhaps?
> >   
> >>  
> >>> +                    "MB to match machine restrictions. Consider updating "
> >>> +                    "the guest definition.i\n", sz / MiB, newsz / MiB);    
> >>
> >> also it might be better to use size_to_str() to format numbers  
> > 
> > The text explicitly talks about 'MB'... not sure if it would be
> > confusing if the user specified MB and ended up with GB or so in this
> > message.
> >   
> >>  
> >>> +    }
> >>> +    return newsz;
> >>> +}
> >>> +  
> > 
> > (If we can agree upon message and format, I'll happily fix that up when
> > applying.)  
> 
> Is the the only thing that blocks this? I would rather try to get this fixed before rc2.

Yes. I plan to send a pull request as soon as I have applied this.
Cornelia Huck April 2, 2020, 10:27 a.m. UTC | #9
On Thu, 2 Apr 2020 11:25:34 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 02.04.20 11:22, Cornelia Huck wrote:
> > On Wed, 1 Apr 2020 15:14:12 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> On 01.04.20 14:37, Christian Borntraeger wrote:  
> >   
> >>> +static ram_addr_t s390_fixup_ram_size(ram_addr_t sz)
> >>> +{
> >>> +    /* same logic as in sclp.c */
> >>> +    int increment_size = 20;
> >>> +    ram_addr_t newsz;
> >>> +
> >>> +    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.i\n", sz / MiB, newsz / MiB);    
> >>
> >> Not sure if news/zs will be printed correctly in case ram_addr_t !=
> >> uint64_t.  
> > 
> > RAM_ADDR_FMT might be the right thing to use here?  
> 
> I tried that but it returns a hex value in bytes. This is unusable. Thats why I divided
> by MiB. 

Nod.

> We could simply do an u64 cast?

Not sure if we might end up with "cast to integer of different length"
values on some platforms (that I unfortunately don't have around to
test). I hate that stuff.
Christian Borntraeger April 2, 2020, 10:32 a.m. UTC | #10
On 02.04.20 12:27, Cornelia Huck wrote:

>> We could simply do an u64 cast?
> 
> Not sure if we might end up with "cast to integer of different length"
> values on some platforms (that I unfortunately don't have around to
> test). I hate that stuff.

That kind of message should NEVER happen. the whole purpose of integer casts
is to change the size and length. This message only happens when you cast
from pointer to integer.
Cornelia Huck April 2, 2020, 11:25 a.m. UTC | #11
On Thu, 2 Apr 2020 11:39:11 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 02.04.20 11:27, Cornelia Huck wrote:
> > On Wed, 1 Apr 2020 18:34:56 +0200
> > Igor Mammedov <imammedo@redhat.com> wrote:
> >   
> >> On Wed,  1 Apr 2020 08:37:54 -0400
> >> Christian Borntraeger <borntraeger@de.ibm.com> wrote:  
> >   
> >>> +static ram_addr_t s390_fixup_ram_size(ram_addr_t sz)
> >>> +{
> >>> +    /* same logic as in sclp.c */
> >>> +    int increment_size = 20;
> >>> +    ram_addr_t newsz;
> >>> +
> >>> +    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    
> >>                                                    ^^^^^^^^
> >>
> >> for unaware  user it could be confusing as it could be read as 'value was increased'
> >> s/fixed up/amended/ might be better  
> > 
> > "rounded", perhaps?
> >   
> >>  
> >>> +                    "MB to match machine restrictions. Consider updating "
> >>> +                    "the guest definition.i\n", sz / MiB, newsz / MiB);    
> >>
> >> also it might be better to use size_to_str() to format numbers  
> > 
> > The text explicitly talks about 'MB'... not sure if it would be
> > confusing if the user specified MB and ended up with GB or so in this
> > message.
> >   
> >>  
> >>> +    }
> >>> +    return newsz;
> >>> +}
> >>> +  
> > 
> > (If we can agree upon message and format, I'll happily fix that up when
> > applying.)  
> 
> Is the the only thing that blocks this? I would rather try to get this fixed before rc2.
> 

I now have

    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", (uint64_t) (sz / MiB),           
                    (uint64_t) (newsz / MiB));                                  
    }

Opinions?
Christian Borntraeger April 2, 2020, 11:32 a.m. UTC | #12
On 02.04.20 13:25, Cornelia Huck wrote:

>>
>> Is the the only thing that blocks this? I would rather try to get this fixed before rc2.
>>
> 
> I now have
> 
>     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", (uint64_t) (sz / MiB),           
>                     (uint64_t) (newsz / MiB));                                  
>     }
> 
> Opinions?

Looks good to me.
Igor Mammedov April 2, 2020, 11:39 a.m. UTC | #13
On Thu, 2 Apr 2020 11:27:35 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Wed, 1 Apr 2020 18:34:56 +0200
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > On Wed,  1 Apr 2020 08:37:54 -0400
> > Christian Borntraeger <borntraeger@de.ibm.com> wrote:  
> 
> > > +static ram_addr_t s390_fixup_ram_size(ram_addr_t sz)
> > > +{
> > > +    /* same logic as in sclp.c */
> > > +    int increment_size = 20;
> > > +    ram_addr_t newsz;
> > > +
> > > +    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    
> >                                                    ^^^^^^^^
> > 
> > for unaware  user it could be confusing as it could be read as 'value was increased'
> > s/fixed up/amended/ might be better  
> 
> "rounded", perhaps?
> 
> >   
> > > +                    "MB to match machine restrictions. Consider updating "
> > > +                    "the guest definition.i\n", sz / MiB, newsz / MiB);    
> > 
> > also it might be better to use size_to_str() to format numbers  
> 
> The text explicitly talks about 'MB'... not sure if it would be
> confusing if the user specified MB and ended up with GB or so in this
> message.

MB can be dropped, since it still might not match what user specified with -m
it could be specified in b/kb/mb/gb over there

so I'd drop MB and print value size_to_str() returns
(it will add appropriate suffix if I'm not mistaken)


> >   
> > > +    }
> > > +    return newsz;
> > > +}
> > > +  
> 
> (If we can agree upon message and format, I'll happily fix that up when
> applying.)
> 
>
Christian Borntraeger April 2, 2020, 11:42 a.m. UTC | #14
On 02.04.20 13:39, Igor Mammedov wrote:
[...]
>>>   
>>>> +                    "MB to match machine restrictions. Consider updating "
>>>> +                    "the guest definition.i\n", sz / MiB, newsz / MiB);    
>>>
>>> also it might be better to use size_to_str() to format numbers  
>>
>> The text explicitly talks about 'MB'... not sure if it would be
>> confusing if the user specified MB and ended up with GB or so in this
>> message.
> 
> MB can be dropped, since it still might not match what user specified with -m
> it could be specified in b/kb/mb/gb over there
> 
> so I'd drop MB and print value size_to_str() returns
> (it will add appropriate suffix if I'm not mistaken)
> 

The return value of size_to_str must be freed. Arent we going overboard for such
a message?
Igor Mammedov April 2, 2020, 12:05 p.m. UTC | #15
On Thu, 2 Apr 2020 13:42:22 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 02.04.20 13:39, Igor Mammedov wrote:
> [...]
> >>>     
> >>>> +                    "MB to match machine restrictions. Consider updating "
> >>>> +                    "the guest definition.i\n", sz / MiB, newsz / MiB);      
> >>>
> >>> also it might be better to use size_to_str() to format numbers    
> >>
> >> The text explicitly talks about 'MB'... not sure if it would be
> >> confusing if the user specified MB and ended up with GB or so in this
> >> message.  
> > 
> > MB can be dropped, since it still might not match what user specified with -m
> > it could be specified in b/kb/mb/gb over there
> > 
> > so I'd drop MB and print value size_to_str() returns
> > (it will add appropriate suffix if I'm not mistaken)
> >   
> 
> The return value of size_to_str must be freed. Arent we going overboard for such
> a message?

yep, one can use g_autofree for it.
on upside one doesn't have to bother with picking proper format string
which is far from trivial in case type mutates depending on host.

>
Christian Borntraeger April 2, 2020, 12:09 p.m. UTC | #16
On 02.04.20 14:05, Igor Mammedov wrote:
> On Thu, 2 Apr 2020 13:42:22 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On 02.04.20 13:39, Igor Mammedov wrote:
>> [...]
>>>>>     
>>>>>> +                    "MB to match machine restrictions. Consider updating "
>>>>>> +                    "the guest definition.i\n", sz / MiB, newsz / MiB);      
>>>>>
>>>>> also it might be better to use size_to_str() to format numbers    
>>>>
>>>> The text explicitly talks about 'MB'... not sure if it would be
>>>> confusing if the user specified MB and ended up with GB or so in this
>>>> message.  
>>>
>>> MB can be dropped, since it still might not match what user specified with -m
>>> it could be specified in b/kb/mb/gb over there
>>>
>>> so I'd drop MB and print value size_to_str() returns
>>> (it will add appropriate suffix if I'm not mistaken)

Another thing: size_to_str is also do rounding (whenever the integer part is >1000).
Doesnt this result in potential messages where both numbers are the same?
Christian Borntraeger April 2, 2020, 12:35 p.m. UTC | #17
On 02.04.20 14:09, Christian Borntraeger wrote:
> 
> 
> On 02.04.20 14:05, Igor Mammedov wrote:
>> On Thu, 2 Apr 2020 13:42:22 +0200
>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>
>>> On 02.04.20 13:39, Igor Mammedov wrote:
>>> [...]
>>>>>>     
>>>>>>> +                    "MB to match machine restrictions. Consider updating "
>>>>>>> +                    "the guest definition.i\n", sz / MiB, newsz / MiB);      
>>>>>>
>>>>>> also it might be better to use size_to_str() to format numbers    
>>>>>
>>>>> The text explicitly talks about 'MB'... not sure if it would be
>>>>> confusing if the user specified MB and ended up with GB or so in this
>>>>> message.  
>>>>
>>>> MB can be dropped, since it still might not match what user specified with -m
>>>> it could be specified in b/kb/mb/gb over there
>>>>
>>>> so I'd drop MB and print value size_to_str() returns
>>>> (it will add appropriate suffix if I'm not mistaken)
> 
> Another thing: size_to_str is also do rounding (whenever the integer part is >1000).
> Doesnt this result in potential messages where both numbers are the same?

For example

10241263616-> 9.54 GiB
10241262592-> 9.54 GiB

The only guaranteed way to actually see a difference is to use MB.
Cornelia Huck April 2, 2020, 2:18 p.m. UTC | #18
On Thu, 2 Apr 2020 14:35:21 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 02.04.20 14:09, Christian Borntraeger wrote:
> > 
> > 
> > On 02.04.20 14:05, Igor Mammedov wrote:  
> >> On Thu, 2 Apr 2020 13:42:22 +0200
> >> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >>  
> >>> On 02.04.20 13:39, Igor Mammedov wrote:
> >>> [...]  
> >>>>>>       
> >>>>>>> +                    "MB to match machine restrictions. Consider updating "
> >>>>>>> +                    "the guest definition.i\n", sz / MiB, newsz / MiB);        
> >>>>>>
> >>>>>> also it might be better to use size_to_str() to format numbers      
> >>>>>
> >>>>> The text explicitly talks about 'MB'... not sure if it would be
> >>>>> confusing if the user specified MB and ended up with GB or so in this
> >>>>> message.    
> >>>>
> >>>> MB can be dropped, since it still might not match what user specified with -m
> >>>> it could be specified in b/kb/mb/gb over there
> >>>>
> >>>> so I'd drop MB and print value size_to_str() returns
> >>>> (it will add appropriate suffix if I'm not mistaken)  
> > 
> > Another thing: size_to_str is also do rounding (whenever the integer part is >1000).
> > Doesnt this result in potential messages where both numbers are the same?  
> 
> For example
> 
> 10241263616-> 9.54 GiB
> 10241262592-> 9.54 GiB
> 
> The only guaranteed way to actually see a difference is to use MB.
> 

Ok, so it seems the way to go is to use the uint64_t cast, as
size_to_str() is unfortunately not doing what we need.
Igor Mammedov April 2, 2020, 3:01 p.m. UTC | #19
On Thu, 2 Apr 2020 14:35:21 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 02.04.20 14:09, Christian Borntraeger wrote:
> > 
> > 
> > On 02.04.20 14:05, Igor Mammedov wrote:  
> >> On Thu, 2 Apr 2020 13:42:22 +0200
> >> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >>  
> >>> On 02.04.20 13:39, Igor Mammedov wrote:
> >>> [...]  
> >>>>>>       
> >>>>>>> +                    "MB to match machine restrictions. Consider updating "
> >>>>>>> +                    "the guest definition.i\n", sz / MiB, newsz / MiB);        
> >>>>>>
> >>>>>> also it might be better to use size_to_str() to format numbers      
> >>>>>
> >>>>> The text explicitly talks about 'MB'... not sure if it would be
> >>>>> confusing if the user specified MB and ended up with GB or so in this
> >>>>> message.    
> >>>>
> >>>> MB can be dropped, since it still might not match what user specified with -m
> >>>> it could be specified in b/kb/mb/gb over there
> >>>>
> >>>> so I'd drop MB and print value size_to_str() returns
> >>>> (it will add appropriate suffix if I'm not mistaken)  
> > 
> > Another thing: size_to_str is also do rounding (whenever the integer part is >1000).
> > Doesnt this result in potential messages where both numbers are the same?  
> 
> For example
> 
> 10241263616-> 9.54 GiB
> 10241262592-> 9.54 GiB

doesn't seem to be working as one would expect (and it's used in number of places now)
CCing original author of it

> The only guaranteed way to actually see a difference is to use MB.
> 
>
Cornelia Huck April 2, 2020, 3:16 p.m. UTC | #20
On Wed,  1 Apr 2020 08:37:54 -0400
Christian Borntraeger <borntraeger@de.ibm.com> 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 if a change to a newer machine type is possible, use a
> machine version >= 5.0.
> 
> A future versions might remove the compatibility handling.

s/versions/version/ (fixed it up)

> 
> 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.
> 
> As we no longer fixup maxram_size as well, make other users use ram_size
> instead. Keep using maxram_size when setting the maximum ram size in KVM,
> as that will come in handy in the future when supporting memory hotplug
> (in contrast, storage keys and storage attributes for hotplugged memory
> will have to be migrated per RAM block in the future).
> 
> Fixes: 3a12fc61af5c ("390x/s390-virtio-ccw: use memdev for RAM")
> Reported-by: Lukáš Doktor <ldoktor@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  hw/s390x/s390-skeys.c        |  2 +-
>  hw/s390x/s390-stattrib-kvm.c |  4 ++--
>  hw/s390x/s390-virtio-ccw.c   | 21 +++++++++++++++++++++
>  hw/s390x/sclp.c              | 17 +++++------------
>  include/hw/boards.h          |  7 +++++++
>  softmmu/vl.c                 |  3 +++
>  6 files changed, 39 insertions(+), 15 deletions(-)

Thanks, applied to s390-fixes (with the fixup message fixed up.)

[I plan to send a pull request for s390-fixes tomorrow, so please let
me know if there are any further concerns.]
Peter Xu April 2, 2020, 5:48 p.m. UTC | #21
On Thu, Apr 02, 2020 at 05:01:23PM +0200, Igor Mammedov wrote:
> On Thu, 2 Apr 2020 14:35:21 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
> > On 02.04.20 14:09, Christian Borntraeger wrote:
> > > 
> > > 
> > > On 02.04.20 14:05, Igor Mammedov wrote:  
> > >> On Thu, 2 Apr 2020 13:42:22 +0200
> > >> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> > >>  
> > >>> On 02.04.20 13:39, Igor Mammedov wrote:
> > >>> [...]  
> > >>>>>>       
> > >>>>>>> +                    "MB to match machine restrictions. Consider updating "
> > >>>>>>> +                    "the guest definition.i\n", sz / MiB, newsz / MiB);        
> > >>>>>>
> > >>>>>> also it might be better to use size_to_str() to format numbers      
> > >>>>>
> > >>>>> The text explicitly talks about 'MB'... not sure if it would be
> > >>>>> confusing if the user specified MB and ended up with GB or so in this
> > >>>>> message.    
> > >>>>
> > >>>> MB can be dropped, since it still might not match what user specified with -m
> > >>>> it could be specified in b/kb/mb/gb over there
> > >>>>
> > >>>> so I'd drop MB and print value size_to_str() returns
> > >>>> (it will add appropriate suffix if I'm not mistaken)  
> > > 
> > > Another thing: size_to_str is also do rounding (whenever the integer part is >1000).
> > > Doesnt this result in potential messages where both numbers are the same?  
> > 
> > For example
> > 
> > 10241263616-> 9.54 GiB
> > 10241262592-> 9.54 GiB
> 
> doesn't seem to be working as one would expect (and it's used in number of places now)
> CCing original author of it

That looks sane to me.  Gib is IEC binary unit as explained by the
comment of the function, and the function prints with %0.3g so that we
keep three digits.  IIUC that's ideal when we want to display
something like disk image sizes, but for sure it won't satisfy any
formatting of digits.  Maybe add a new helper?

Thanks,

> 
> > The only guaranteed way to actually see a difference is to use MB.
> > 
> > 
>
diff mbox series

Patch

diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index 5da6e5292f..a9a4ae7b39 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -176,7 +176,7 @@  static void qemu_s390_skeys_init(Object *obj)
     QEMUS390SKeysState *skeys = QEMU_S390_SKEYS(obj);
     MachineState *machine = MACHINE(qdev_get_machine());
 
-    skeys->key_count = machine->maxram_size / TARGET_PAGE_SIZE;
+    skeys->key_count = machine->ram_size / TARGET_PAGE_SIZE;
     skeys->keydata = g_malloc0(skeys->key_count);
 }
 
diff --git a/hw/s390x/s390-stattrib-kvm.c b/hw/s390x/s390-stattrib-kvm.c
index c7e1f35524..f89d8d9d16 100644
--- a/hw/s390x/s390-stattrib-kvm.c
+++ b/hw/s390x/s390-stattrib-kvm.c
@@ -85,7 +85,7 @@  static int kvm_s390_stattrib_set_stattr(S390StAttribState *sa,
 {
     KVMS390StAttribState *sas = KVM_S390_STATTRIB(sa);
     MachineState *machine = MACHINE(qdev_get_machine());
-    unsigned long max = machine->maxram_size / TARGET_PAGE_SIZE;
+    unsigned long max = machine->ram_size / TARGET_PAGE_SIZE;
 
     if (start_gfn + count > max) {
         error_report("Out of memory bounds when setting storage attributes");
@@ -104,7 +104,7 @@  static void kvm_s390_stattrib_synchronize(S390StAttribState *sa)
 {
     KVMS390StAttribState *sas = KVM_S390_STATTRIB(sa);
     MachineState *machine = MACHINE(qdev_get_machine());
-    unsigned long max = machine->maxram_size / TARGET_PAGE_SIZE;
+    unsigned long max = machine->ram_size / TARGET_PAGE_SIZE;
     /* We do not need to reach the maximum buffer size allowed */
     unsigned long cx, len = KVM_S390_SKEYS_MAX / 2;
     int r;
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 3cf19c99f3..61a8a0e693 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ 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,25 @@  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;
+    ram_addr_t newsz;
+
+    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.i\n", sz / MiB, newsz / MiB);
+    }
+    return newsz;
+}
+
 static void ccw_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -808,6 +828,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..ede056b3ef 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -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;
-
-    /* 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..fd4d62b501 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
+ *    specific algorithm. To be used by old machine types for compat
+ *    purposes only.
+ *    Applies only to default memory backend, i.e., explicit memory backend
+ *    wasn't used.
  */
 struct MachineClass {
     /*< private >*/
@@ -218,6 +224,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 (*fixup_ram_size)(ram_addr_t size);
 };
 
 /**
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 1d33a28340..09f8a1b0a7 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->fixup_ram_size) {
+        sz = mc->fixup_ram_size(sz);
+    }
     ram_size = sz;
     if (ram_size != sz) {
         error_report("ram size too large");