[PULL,13/27] machine: add mem compound property

Make -m syntactic sugar for a compound property "-machine
mem.{size,max-size,slots}".  The new property does not have
the magic conversion to megabytes of unsuffixed arguments,
and also does not understand that "0" means the default size
(you have to leave it out to get the default).  This means
that we need to convert the QemuOpts by hand to a QDict.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Message-Id: <20220414165300.555321-4-pbonzini@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/core/machine.c |  80 ++++++++++++++++++++++++++++++
 qapi/machine.json |  18 +++++++
 softmmu/vl.c      | 123 +++++++++++++++-------------------------------
 3 files changed, 138 insertions(+), 83 deletions(-)

Paolo Bonzini <pbonzini@redhat.com> writes:

> Make -m syntactic sugar for a compound property "-machine
> mem.{size,max-size,slots}".  The new property does not have
> the magic conversion to megabytes of unsuffixed arguments,
> and also does not understand that "0" means the default size
> (you have to leave it out to get the default).  This means
> that we need to convert the QemuOpts by hand to a QDict.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Message-Id: <20220414165300.555321-4-pbonzini@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/core/machine.c |  80 ++++++++++++++++++++++++++++++
>  qapi/machine.json |  18 +++++++
>  softmmu/vl.c      | 123 +++++++++++++++-------------------------------
>  3 files changed, 138 insertions(+), 83 deletions(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 8cea94537d..46b8d0effa 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -523,6 +523,78 @@ static void machine_set_hmat(Object *obj, bool value, Error **errp)
>      ms->numa_state->hmat_enabled = value;
>  }
>  
> +static void machine_get_mem(Object *obj, Visitor *v, const char *name,
> +                            void *opaque, Error **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +    MemorySizeConfiguration mem = {
> +        .has_size = true,
> +        .size = ms->ram_size,
> +        .has_max_size = !!ms->ram_slots,
> +        .max_size = ms->maxram_size,
> +        .has_slots = !!ms->ram_slots,
> +        .slots = ms->ram_slots,
> +    };
> +    MemorySizeConfiguration *p_mem = &mem;
> +
> +    visit_type_MemorySizeConfiguration(v, name, &p_mem, &error_abort);
> +}
> +
> +static void machine_set_mem(Object *obj, Visitor *v, const char *name,
> +                            void *opaque, Error **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +    MachineClass *mc = MACHINE_GET_CLASS(obj);
> +    MemorySizeConfiguration *mem;
> +
> +    ERRP_GUARD();
> +
> +    if (!visit_type_MemorySizeConfiguration(v, name, &mem, errp)) {
> +        return;
> +    }
> +
> +    if (!mem->has_size) {
> +        mem->has_size = true;
> +        mem->size = mc->default_ram_size;
> +    }
> +    mem->size = QEMU_ALIGN_UP(mem->size, 8192);
> +    if (mc->fixup_ram_size) {
> +        mem->size = mc->fixup_ram_size(mem->size);
> +    }
> +    if ((ram_addr_t)mem->size != mem->size) {
> +        error_setg(errp, "ram size too large");
> +        goto out_free;
> +    }
> +
> +    if (mem->has_max_size) {
> +        if (mem->max_size < mem->size) {
> +            error_setg(errp, "invalid value of maxmem: "
> +                       "maximum memory size (0x%" PRIx64 ") must be at least "
> +                       "the initial memory size (0x%" PRIx64 ")",
> +                       mem->max_size, mem->size);
> +            goto out_free;
> +        }
> +        if (mem->has_slots && mem->slots && mem->max_size == mem->size) {
> +            error_setg(errp, "invalid value of maxmem: "
> +                       "memory slots were specified but maximum memory size "
> +                       "(0x%" PRIx64 ") is equal to the initial memory size "
> +                       "(0x%" PRIx64 ")", mem->max_size, mem->size);
> +            goto out_free;
> +        }
> +        ms->maxram_size = mem->max_size;
> +    } else {
> +        if (mem->has_slots) {
> +            error_setg(errp, "slots specified but no max-size");
> +            goto out_free;
> +        }
> +        ms->maxram_size = mem->size;
> +    }
> +    ms->ram_size = mem->size;
> +    ms->ram_slots = mem->has_slots ? mem->slots : 0;
> +out_free:
> +    qapi_free_MemorySizeConfiguration(mem);
> +}
> +
>  static char *machine_get_nvdimm_persistence(Object *obj, Error **errp)
>  {
>      MachineState *ms = MACHINE(obj);
> @@ -953,6 +1025,12 @@ static void machine_class_init(ObjectClass *oc, void *data)
>      object_class_property_set_description(oc, "memory-backend",
>                                            "Set RAM backend"
>                                            "Valid value is ID of hostmem based backend");
> +
> +    object_class_property_add(oc, "memory", "MemorySizeConfiguration",
> +        machine_get_mem, machine_set_mem,
> +        NULL, NULL);
> +    object_class_property_set_description(oc, "memory",
> +        "Memory size configuration");
>  }
>  
>  static void machine_class_base_init(ObjectClass *oc, void *data)
> @@ -983,6 +1061,8 @@ static void machine_initfn(Object *obj)
>      ms->mem_merge = true;
>      ms->enable_graphics = true;
>      ms->kernel_cmdline = g_strdup("");
> +    ms->ram_size = mc->default_ram_size;
> +    ms->maxram_size = mc->default_ram_size;
>  
>      if (mc->nvdimm_supported) {
>          Object *obj = OBJECT(ms);
> diff --git a/qapi/machine.json b/qapi/machine.json
> index e3dcf5a119..92480d4044 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1614,3 +1614,21 @@
>  ##
>  { 'enum': 'SmbiosEntryPointType',
>    'data': [ '32', '64' ] }
> +
> +##
> +# @MemorySizeConfiguration:
> +#
> +# Schema for memory size configuration.
> +#
> +# @size: memory size in bytes
> +#
> +# @max-size: maximum hotpluggable memory size in bytes
> +#
> +# @slots: number of available memory slots for hotplug
> +#
> +# Since: 7.1
> +##
> +{ 'struct': 'MemorySizeConfiguration', 'data': {
> +     '*size': 'size',
> +     '*max-size': 'size',
> +     '*slots': 'uint64' } }
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 13ae31e92f..65a665e0bc 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -159,11 +159,10 @@ static const char *mem_path;
>  static const char *incoming;
>  static const char *loadvm;
>  static const char *accelerators;
> +static bool have_custom_ram_size;
>  static QDict *machine_opts_dict;
>  static QTAILQ_HEAD(, ObjectOption) object_opts = QTAILQ_HEAD_INITIALIZER(object_opts);
>  static QTAILQ_HEAD(, DeviceOption) device_opts = QTAILQ_HEAD_INITIALIZER(device_opts);
> -static ram_addr_t maxram_size;
> -static uint64_t ram_slots;
>  static int display_remote;
>  static int snapshot;
>  static bool preconfig_requested;
> @@ -171,7 +170,6 @@ static QemuPluginList plugin_list = QTAILQ_HEAD_INITIALIZER(plugin_list);
>  static BlockdevOptionsQueue bdo_queue = QSIMPLEQ_HEAD_INITIALIZER(bdo_queue);
>  static bool nographic = false;
>  static int mem_prealloc; /* force preallocation of physical target memory */
> -static ram_addr_t ram_size;
>  static const char *vga_model = NULL;
>  static DisplayOptions dpy;
>  static int num_serial_hds;
> @@ -1736,6 +1734,7 @@ static void keyval_dashify(QDict *qdict, Error **errp)
>  static void qemu_apply_legacy_machine_options(QDict *qdict)
>  {
>      const char *value;
> +    QObject *prop;
>  
>      keyval_dashify(qdict, &error_fatal);
>  
> @@ -1768,6 +1767,13 @@ static void qemu_apply_legacy_machine_options(QDict *qdict)
>                                     false);
>          qdict_del(qdict, "kernel-irqchip");
>      }
> +
> +    prop = qdict_get(qdict, "memory");
> +    if (prop) {
> +        have_custom_ram_size =
> +            qobject_type(prop) == QTYPE_QDICT &&
> +            qdict_haskey(qobject_to(QDict, prop), "size");
> +    }
>  }
>  
>  static void object_option_foreach_add(bool (*type_opt_predicate)(const char *))
> @@ -1885,9 +1891,6 @@ static bool object_create_early(const char *type)
>  static void qemu_apply_machine_options(QDict *qdict)
>  {
>      object_set_properties_from_keyval(OBJECT(current_machine), qdict, false, &error_fatal);
> -    current_machine->ram_size = ram_size;
> -    current_machine->maxram_size = maxram_size;
> -    current_machine->ram_slots = ram_slots;
>  
>      if (semihosting_enabled() && !semihosting_get_argc()) {
>          /* fall back to the -kernel/-append */
> @@ -1998,12 +2001,6 @@ static void qemu_create_late_backends(void)
>      qemu_semihosting_console_init();
>  }
>  
> -static bool have_custom_ram_size(void)
> -{
> -    QemuOpts *opts = qemu_find_opts_singleton("memory");
> -    return !!qemu_opt_get_size(opts, "size", 0);
> -}
> -
>  static void qemu_resolve_machine_memdev(void)
>  {
>      if (current_machine->ram_memdev_id) {
> @@ -2018,7 +2015,7 @@ static void qemu_resolve_machine_memdev(void)
>              exit(EXIT_FAILURE);
>          }
>          backend_size = object_property_get_uint(backend, "size",  &error_abort);
> -        if (have_custom_ram_size() && backend_size != ram_size) {
> +        if (have_custom_ram_size && backend_size != current_machine->ram_size) {
>                  error_report("Size specified by -m option must match size of "
>                               "explicitly specified 'memory-backend' property");
>                  exit(EXIT_FAILURE);
> @@ -2028,95 +2025,58 @@ static void qemu_resolve_machine_memdev(void)
>                           "'-machine memory-backend'");
>              exit(EXIT_FAILURE);
>          }
> -        ram_size = backend_size;
> +        current_machine->ram_size = backend_size;
>      }
>  
>      if (!xen_enabled()) {
>          /* On 32-bit hosts, QEMU is limited by virtual address space */
> -        if (ram_size > (2047 << 20) && HOST_LONG_BITS == 32) {
> +        if (current_machine->ram_size > (2047 << 20) && HOST_LONG_BITS == 32) {
>              error_report("at most 2047 MB RAM can be simulated");
>              exit(1);
>          }
>      }
>  }
>  
> -static void set_memory_options(MachineClass *mc)
> +static void parse_memory_options(const char *arg)
>  {
> -    uint64_t sz;
> +    QemuOpts *opts;
> +    QDict *dict, *prop;
>      const char *mem_str;
> -    const ram_addr_t default_ram_size = mc->default_ram_size;
> -    QemuOpts *opts = qemu_find_opts_singleton("memory");
> -    Location loc;
>  
> -    loc_push_none(&loc);
> -    qemu_opts_loc_restore(opts);
> +    opts = qemu_opts_parse_noisily(qemu_find_opts("memory"), arg, true);
> +    if (!opts) {
> +        exit(EXIT_FAILURE);
> +    }
>  
> -    sz = 0;
> -    mem_str = qemu_opt_get(opts, "size");
> -    if (mem_str) {
> +    prop = qdict_new();
> +
> +    if (qemu_opt_get_size(opts, "size", 0) != 0) {
> +        mem_str = qemu_opt_get(opts, "size");
>          if (!*mem_str) {
>              error_report("missing 'size' option value");
>              exit(EXIT_FAILURE);
>          }
>  
> -        sz = qemu_opt_get_size(opts, "size", ram_size);
> -
>          /* Fix up legacy suffix-less format */
>          if (g_ascii_isdigit(mem_str[strlen(mem_str) - 1])) {
> -            uint64_t overflow_check = sz;
> -
> -            sz *= MiB;
> -            if (sz / MiB != overflow_check) {
> -                error_report("too large 'size' option value");
> -                exit(EXIT_FAILURE);
> -            }
> +            g_autofree char *mib_str = g_strdup_printf("%sM", mem_str);
> +            qdict_put_str(prop, "size", mib_str);
> +        } else {
> +            qdict_put_str(prop, "size", mem_str);
>          }
>      }
>  
> -    /* backward compatibility behaviour for case "-m 0" */
> -    if (sz == 0) {
> -        sz = default_ram_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");
> -        exit(EXIT_FAILURE);
> -    }
> -
> -    maxram_size = ram_size;
> -
>      if (qemu_opt_get(opts, "maxmem")) {
> -        uint64_t slots;
> -
> -        sz = qemu_opt_get_size(opts, "maxmem", 0);
> -        slots = qemu_opt_get_number(opts, "slots", 0);
> -        if (sz < ram_size) {
> -            error_report("invalid value of -m option maxmem: "
> -                         "maximum memory size (0x%" PRIx64 ") must be at least "
> -                         "the initial memory size (0x" RAM_ADDR_FMT ")",
> -                         sz, ram_size);
> -            exit(EXIT_FAILURE);
> -        } else if (slots && sz == ram_size) {
> -            error_report("invalid value of -m option maxmem: "
> -                         "memory slots were specified but maximum memory size "
> -                         "(0x%" PRIx64 ") is equal to the initial memory size "
> -                         "(0x" RAM_ADDR_FMT ")", sz, ram_size);
> -            exit(EXIT_FAILURE);
> -        }
> -
> -        maxram_size = sz;
> -        ram_slots = slots;
> -    } else if (qemu_opt_get(opts, "slots")) {
> -        error_report("invalid -m option value: missing 'maxmem' option");
> -        exit(EXIT_FAILURE);
> +        qdict_put_str(prop, "max-size", qemu_opt_get(opts, "maxmem"));
> +    }
> +    if (qemu_opt_get(opts, "slots")) {
> +        qdict_put_str(prop, "slots", qemu_opt_get(opts, "slots"));
>      }
>  
> -    loc_pop(&loc);
> +    dict = qdict_new();
> +    qdict_put(dict, "memory", prop);
> +    keyval_merge(machine_opts_dict, dict, &error_fatal);
> +    qobject_unref(dict);
>  }
>  
>  static void qemu_create_machine(QDict *qdict)
> @@ -2124,8 +2084,6 @@ static void qemu_create_machine(QDict *qdict)
>      MachineClass *machine_class = select_machine(qdict, &error_fatal);
>      object_set_machine_compat_props(machine_class->compat_props);
>  
> -    set_memory_options(machine_class);
> -
>      current_machine = MACHINE(object_new_with_class(OBJECT_CLASS(machine_class)));
>      object_property_add_child(object_get_root(), "machine",
>                                OBJECT(current_machine));
> @@ -2185,7 +2143,8 @@ static bool is_qemuopts_group(const char *group)
>      if (g_str_equal(group, "object") ||
>          g_str_equal(group, "machine") ||
>          g_str_equal(group, "smp-opts") ||
> -        g_str_equal(group, "boot-opts")) {
> +        g_str_equal(group, "boot-opts") ||
> +        g_str_equal(group, "memory")) {
>          return false;
>      }
>      return true;
> @@ -2209,6 +2168,8 @@ static void qemu_record_config_group(const char *group, QDict *dict,
>          machine_merge_property("smp", dict, &error_fatal);
>      } else if (g_str_equal(group, "boot-opts")) {
>          machine_merge_property("boot", dict, &error_fatal);
> +    } else if (g_str_equal(group, "memory")) {
> +        machine_merge_property("memory", dict, &error_fatal);
>      } else {
>          abort();
>      }
> @@ -3009,11 +2970,7 @@ void qemu_init(int argc, char **argv, char **envp)
>                  exit(0);
>                  break;
>              case QEMU_OPTION_m:
> -                opts = qemu_opts_parse_noisily(qemu_find_opts("memory"),
> -                                               optarg, true);
> -                if (!opts) {
> -                    exit(EXIT_FAILURE);
> -                }
> +                parse_memory_options(optarg);
>                  break;
>  #ifdef CONFIG_TPM
>              case QEMU_OPTION_tpmdev:

This appears to change the meaning of

    [memory]
      size = "1024"

in a -readconfig file from 1024MiB to 8KiB (1024 Bytes rounded up to
8KiB silently).

Aside: the failure mode is nasty: "KVM internal error. Suberror: 1".
Known issue.  Or rather known again issue (to me); I thought I had
broken KVM somehow.

Markus Armbruster <armbru@redhat.com> writes:

> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> Make -m syntactic sugar for a compound property "-machine
>> mem.{size,max-size,slots}".  The new property does not have
>> the magic conversion to megabytes of unsuffixed arguments,
>> and also does not understand that "0" means the default size
>> (you have to leave it out to get the default).  This means
>> that we need to convert the QemuOpts by hand to a QDict.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> Message-Id: <20220414165300.555321-4-pbonzini@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

[...]

> This appears to change the meaning of
>
>     [memory]
>       size = "1024"
>
> in a -readconfig file from 1024MiB to 8KiB (1024 Bytes rounded up to
> 8KiB silently).

No reply so far.

If we can't fix this, we better mention it in the release notes.

Can we fix it?

> Aside: the failure mode is nasty: "KVM internal error. Suberror: 1".
> Known issue.  Or rather known again issue (to me); I thought I had
> broken KVM somehow.

On 8/5/22 11:30, Markus Armbruster wrote:
>> This appears to change the meaning of
>>
>>      [memory]
>>        size = "1024"
>>
>> in a -readconfig file from 1024MiB to 8KiB (1024 Bytes rounded up to
>> 8KiB silently).
> No reply so far.
> 
> If we can't fix this, we better mention it in the release notes.
> 
> Can we fix it?
> 

Ugh, I hadn't seen it.  Let me take a look.

Paolo

On Fri, Aug 05, 2022 at 11:30:12AM +0200, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > Paolo Bonzini <pbonzini@redhat.com> writes:
> >
> >> Make -m syntactic sugar for a compound property "-machine
> >> mem.{size,max-size,slots}".  The new property does not have
> >> the magic conversion to megabytes of unsuffixed arguments,
> >> and also does not understand that "0" means the default size
> >> (you have to leave it out to get the default).  This means
> >> that we need to convert the QemuOpts by hand to a QDict.
> >>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> Message-Id: <20220414165300.555321-4-pbonzini@redhat.com>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> [...]
> 
> > This appears to change the meaning of
> >
> >     [memory]
> >       size = "1024"
> >
> > in a -readconfig file from 1024MiB to 8KiB (1024 Bytes rounded up to
> > 8KiB silently).
> 
> No reply so far.
> 
> If we can't fix this, we better mention it in the release notes.

That is such a massive change in semantics of -readconfig that if we
can't fix it we might as well just go all out and fully drop all notion
of backcompat for -readconfig.

With regards,
Daniel