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

[Markus Armbruster <armbru@redhat.com>]

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.