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
