diff mbox series

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

Message ID 20200401085014.100125-1-borntraeger@de.ibm.com
State New
Headers show
Series [v2] vl/s390: fixup ram sizes for compat machines | expand

Commit Message

Christian Borntraeger April 1, 2020, 8:50 a.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 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.

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   | 19 +++++++++++++++++++
 hw/s390x/sclp.c              | 19 ++++++-------------
 include/hw/boards.h          |  7 +++++++
 softmmu/vl.c                 |  3 +++
 6 files changed, 38 insertions(+), 16 deletions(-)

Comments

David Hildenbrand April 1, 2020, 8:58 a.m. UTC | #1
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.,"
Cornelia Huck April 1, 2020, 10:13 a.m. UTC | #2
On Wed,  1 Apr 2020 04:50:14 -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

"to replace any unaligned -m value" ?

> aligned one or to use a machine version >= 5.0 as future versions might
> remove the compatibility handling.

I'm confused by the second part of the sentence. Warning about possible
future removal of the compat stuff is fine, but I don't understand the
suggestion to use a machine type >= 5.0. If I create a new machine that
does not need be migrated to an old QEMU, using the latest machine type
always seems like the best idea, right? And for a migration target it's
not like we can choose the version freely anyway.

> 
> 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.
> 
> 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   | 19 +++++++++++++++++++
>  hw/s390x/sclp.c              | 19 ++++++-------------
>  include/hw/boards.h          |  7 +++++++
>  softmmu/vl.c                 |  3 +++
>  6 files changed, 38 insertions(+), 16 deletions(-)
> 

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

"Applies only to the default memory backend, i.e., an explicitly
specified memory backend will not be affected."

?

>   */
>  struct MachineClass {
>      /*< private >*/
Christian Borntraeger April 1, 2020, 11:01 a.m. UTC | #3
On 01.04.20 12:13, Cornelia Huck wrote:
> On Wed,  1 Apr 2020 04:50:14 -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
> 
> "to replace any unaligned -m value" ?
> 
>> aligned one or to use a machine version >= 5.0 as future versions might
>> remove the compatibility handling.
> 
> I'm confused by the second part of the sentence. Warning about possible
> future removal of the compat stuff is fine, but I don't understand the
> suggestion to use a machine type >= 5.0. If I create a new machine that
> does not need be migrated to an old QEMU, using the latest machine type
> always seems like the best idea, right? And for a migration target it's
> not like we can choose the version freely anyway.


My point was that - when you redefine your guest, which is disruptive anyway
you could also change the machine version to 5.0 and keep the strange memory
size.
Cornelia Huck April 1, 2020, 11:12 a.m. UTC | #4
On Wed, 1 Apr 2020 13:01:43 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 01.04.20 12:13, Cornelia Huck wrote:
> > On Wed,  1 Apr 2020 04:50:14 -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  
> > 
> > "to replace any unaligned -m value" ?
> >   
> >> aligned one or to use a machine version >= 5.0 as future versions might
> >> remove the compatibility handling.  
> > 
> > I'm confused by the second part of the sentence. Warning about possible
> > future removal of the compat stuff is fine, but I don't understand the
> > suggestion to use a machine type >= 5.0. If I create a new machine that
> > does not need be migrated to an old QEMU, using the latest machine type
> > always seems like the best idea, right? And for a migration target it's
> > not like we can choose the version freely anyway.  
> 
> 
> My point was that - when you redefine your guest, which is disruptive anyway
> you could also change the machine version to 5.0 and keep the strange memory
> size.

Ah, ok. That depends however on whether you still need compatibility,
so it might not be advisable. What about:

"...or to switch to a machine version >= 5.0 if migration to older
machine types is not needed; future versions might remove the
compatibility handling."

?
Christian Borntraeger April 1, 2020, 11:55 a.m. UTC | #5
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
David Hildenbrand April 1, 2020, 12:04 p.m. UTC | #6
> 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

Maybe warn_report()

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

might be able to squeeze that into the previous line.

>     }
>     return newsz;
> 
> ?

Much better.

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

True, not sure how cryptic the error will be :)


LGTM
Cornelia Huck April 1, 2020, 12:07 p.m. UTC | #7
On Wed, 1 Apr 2020 14:04:17 +0200
David Hildenbrand <david@redhat.com> wrote:

> > 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  
> 
> Maybe warn_report()

The _report() functions prescribe using just a single sentence without
trailing period, though. The only real difference is whether the
message goes to stderr or stdout in absence of a monitor.

> 
> >                     "MB to match machine restrictions. Consider updating "
> >                     "the guest definition.\n",
> >                     sz / MiB, newsz / MiB);  
> 
> might be able to squeeze that into the previous line.
> 
> >     }
> >     return newsz;
> > 
> > ?  
> 
> Much better.

Seconded :)
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..3e8c34a3e1 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,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;
+    while ((sz >> increment_size) > MAX_STORAGE_INCREMENTS) {
+        increment_size++;
+    }
+    if (sz != sz >> increment_size << increment_size) {
+        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);
+    }
+    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);
     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;
-
-    /* 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..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
+ *    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");