Re: [Qemu-devel] [PATCH 3/4] machine: convert ram_size, maxram_size, ram_slots to properties

[Michael S. Tsirkin]

On Wed, Jun 25, 2014 at 01:42:22PM +0200, Igor Mammedov wrote:
> ... short term it will help writing unit tests accessing
> these values via QMP. And down the road it will allow to drop
> global variable ram_size.
> 
> Signed-off-by: Igor Mammedov <address@hidden>
> ---
>  hw/core/machine.c   |  181 
> +++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/boards.h |    5 ++
>  vl.c                |  123 +++++++++++++++-------------------
>  3 files changed, 240 insertions(+), 69 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index cbba679..f73b810 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -12,6 +12,10 @@
>  
>  #include "hw/boards.h"
>  #include "qapi/visitor.h"
> +#include "qemu/error-report.h"
> +#include "hw/xen/xen.h"
> +
> +static const ram_addr_t default_ram_size  = 128 * 1024 * 1024;
>  
>  static char *machine_get_accel(Object *obj, Error **errp)
>  {
> @@ -235,8 +239,118 @@ static void machine_set_firmware(Object *obj, const 
> char *value, Error **errp)
>      ms->firmware = g_strdup(value);
>  }
>  
> +static void machine_get_ram_size(Object *obj, Visitor *v,
> +                                 void *opaque, const char *name,
> +                                 Error **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +    int64_t value = ms->ram_size;
> +
> +    visit_type_int(v, &value, name, errp);
> +}
> +
> +static void machine_set_ram_size(Object *obj, Visitor *v,
> +                                 void *opaque, const char *name,
> +                                 Error **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +    Error *error = NULL;
> +    int64_t value;
> +
> +    visit_type_int(v, &value, name, &error);
> +    if (error) {
> +        goto out;
> +    }
> +
> +    value = QEMU_ALIGN_UP(value, 8192);
> +    ms->ram_size = value;
> +    if (ms->ram_size != value) {
> +        error_setg(&error, "ram size too large");
> +        goto out;
> +    }
> +
> +    if (!ms->ram_size) {
> +        error_setg(&error, "ram size can't be 0");
> +    }
> +out:
> +    if (error) {
> +        error_propagate(errp, error);
> +    }
> +}
> +
> +static void machine_get_maxram_size(Object *obj, Visitor *v,
> +                                    void *opaque, const char *name,
> +                                    Error **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +    int64_t value = ms->maxram_size;
> +
> +    visit_type_int(v, &value, name, errp);
> +}
> +
> +static void machine_set_maxram_size(Object *obj, Visitor *v,
> +                                    void *opaque, const char *name,
> +                                    Error **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +    Error *error = NULL;
> +    int64_t value;
> +
> +    visit_type_int(v, &value, name, &error);
> +    if (error) {
> +        goto out;
> +    }
> +
> +    ms->maxram_size = value;
> +    if (ms->maxram_size != value) {
> +        error_setg(&error, "maxmem is too large");
> +        goto out;
> +    }
> +
> +    if (!ms->maxram_size) {
> +        error_setg(&error, "maxmem can't be 0");
> +    }
> +out:
> +    if (error) {
> +        error_propagate(errp, error);
> +    }
> +}
> +
> +static void machine_get_ram_slots(Object *obj, Visitor *v,
> +                                  void *opaque, const char *name,
> +                                  Error **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +    int64_t value = ms->ram_slots;
> +
> +    visit_type_int(v, &value, name, errp);
> +}
> +
> +static void machine_set_ram_slots(Object *obj, Visitor *v,
> +                                  void *opaque, const char *name,
> +                                  Error **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +    Error *error = NULL;
> +    int64_t value;
> +
> +    visit_type_int(v, &value, name, &error);
> +    if (error) {
> +        goto out;
> +    }
> +
> +    ms->ram_slots = value;
> +
> +out:
> +    if (error) {
> +        error_propagate(errp, error);
> +    }
> +}
>

Really that's too much boiler plate code for
each value.  Can't we support a "validate" callback from
standard integer values, to be called after parsing?


>  static void machine_initfn(Object *obj)
>  {
> +    MachineState *ms = MACHINE(obj);
> +
>      object_property_add_str(obj, "accel",
>                              machine_get_accel, machine_set_accel, NULL);
>      object_property_add_bool(obj, "kernel_irqchip",
> @@ -274,6 +388,20 @@ static void machine_initfn(Object *obj)
>      object_property_add_bool(obj, "usb", machine_get_usb, machine_set_usb, 
> NULL);
>      object_property_add_str(obj, "firmware",
>                              machine_get_firmware, machine_set_firmware, 
> NULL);
> +
> +    ms->ram_size = default_ram_size;
> +    object_property_add(obj, MACHINE_MEMORY_SIZE_OPT, "int",
> +                        machine_get_ram_size,
> +                        machine_set_ram_size,
> +                        NULL, NULL, NULL);
> +    object_property_add(obj, MACHINE_MAXMEMORY_SIZE_OPT, "int",
> +                        machine_get_maxram_size,
> +                        machine_set_maxram_size,
> +                        NULL, NULL, NULL);
> +    object_property_add(obj, MACHINE_MEMORY_SLOTS_OPT, "int",
> +                        machine_get_ram_slots,
> +                        machine_set_ram_slots,
> +                        NULL, NULL, NULL);
>  }
>  
>  static void machine_finalize(Object *obj)
> @@ -290,11 +418,64 @@ static void machine_finalize(Object *obj)
>      g_free(ms->firmware);
>  }
>  
> +static void machine_pre_init(MachineState *ms, Error **errp)
> +{
> +    Error *error = NULL;
> +
> +    if ((ms->ram_size > ms->maxram_size) && ms->maxram_size) {
> +        error_setg(&error, "invalid \"maxmem\" value (%" RAM_ADDR_FMT
> +                   ") < initial memory (%" RAM_ADDR_FMT ")",
> +                   ms->maxram_size, ms->ram_size);
> +        goto out;
> +    }
> +
> +    if ((ms->ram_size < ms->maxram_size) && !ms->ram_slots) {
> +        error_setg(&error, "invalid \"maxmem\" value (%" RAM_ADDR_FMT ") "
> +                   "more than initial memory (%" PRIu64 ") but "
> +                   "no hotplug slots where specified",
> +                   ms->maxram_size, ms->ram_size);
> +        goto out;
> +    }
> +
> +    if ((ms->ram_size == ms->maxram_size) && ms->ram_slots) {

ugly () within conditions, not really needed.

> +        error_setg(&error, "invalid \"maxmem\" value (%" RAM_ADDR_FMT ") "
> +                   "it must be more than initial memory if hotplug slots > 
> 0",
> +                   ms->maxram_size);
> +        goto out;
> +    }
> +
> +    if (!ms->maxram_size && !ms->ram_slots) {
> +        ms->maxram_size = ms->ram_size;
> +    }
> +
> +    if (!xen_enabled()) {
> +        /* On 32-bit hosts, QEMU is limited by virtual address space */
> +        if (HOST_LONG_BITS == 32 && ((ms->ram_size > (2047 << 20)) ||
> +                                     (ms->maxram_size > (2047 << 20)))) {
> +            error_setg(&error, "at most 2047 MB RAM can be simulated\n");
> +            goto out;
> +        }
> +    }
> +
> +out:
> +    if (error) {
> +        error_propagate(errp, error);
> +    }
> +}
> +
> +static void machine_class_init(ObjectClass *oc, void *data)
> +{
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +
> +    mc->instance_pre_init = machine_pre_init;
> +}
> +
>  static const TypeInfo machine_info = {
>      .name = TYPE_MACHINE,
>      .parent = TYPE_OBJECT,
>      .abstract = true,
>      .class_size = sizeof(MachineClass),
> +    .class_init = machine_class_init,
>      .instance_size = sizeof(MachineState),
>      .instance_init = machine_initfn,
>      .instance_finalize = machine_finalize,
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 605a970..1b980c5 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -81,6 +81,7 @@ struct MachineClass {
>      const char *desc;
>  
>      void (*init)(MachineState *state);
> +    void (*instance_pre_init)(MachineState *state, Error **errp);
>      void (*reset)(void);
>      void (*hot_add_cpu)(const int64_t id, Error **errp);
>      int (*kvm_type)(const char *arg);
> @@ -134,4 +135,8 @@ struct MachineState {
>      const char *cpu_model;
>  };
>  
> +#define MACHINE_MEMORY_SIZE_OPT    "memory-size"
> +#define MACHINE_MEMORY_SLOTS_OPT   "memory-slots"
> +#define MACHINE_MAXMEMORY_SIZE_OPT "maxmem"
> +
>  #endif
> diff --git a/vl.c b/vl.c
> index 0a39c93..3ed3582 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -119,8 +119,6 @@ int main(int argc, char **argv)
>  #include "qom/object_interfaces.h"
>  #include "qapi-event.h"
>  
> -#define DEFAULT_RAM_SIZE 128
> -
>  #define MAX_VIRTIO_CONSOLES 1
>  #define MAX_SCLP_CONSOLES 1
>  
> @@ -2928,10 +2926,8 @@ int main(int argc, char **argv, char **envp)
>      const char *trace_events = NULL;
>      const char *trace_file = NULL;
>      Error *local_err = NULL;
> -    const ram_addr_t default_ram_size = (ram_addr_t)DEFAULT_RAM_SIZE *
> -                                        1024 * 1024;
> -    ram_addr_t maxram_size = default_ram_size;
> -    uint64_t ram_slots = 0;
> +    const char *maxram_size_str = NULL;
> +    const char *ram_slots_str = NULL;
>      FILE *vmstate_dump_file = NULL;
>  
>      atexit(qemu_run_exit_notifiers);
> @@ -2978,7 +2974,7 @@ int main(int argc, char **argv, char **envp)
>      module_call_init(MODULE_INIT_MACHINE);
>      machine_class = find_default_machine();
>      cpu_model = NULL;
> -    ram_size = default_ram_size;
> +    ram_size = 0;
>      snapshot = 0;
>      cyls = heads = secs = 0;
>      translation = BIOS_ATA_TRANSLATION_AUTO;
> @@ -3269,7 +3265,6 @@ int main(int argc, char **argv, char **envp)
>              case QEMU_OPTION_m: {
>                  uint64_t sz;
>                  const char *mem_str;
> -                const char *maxmem_str, *slots_str;
>  
>                  opts = qemu_opts_parse(qemu_find_opts("memory"),
>                                         optarg, 1);
> @@ -3287,7 +3282,7 @@ int main(int argc, char **argv, char **envp)
>                      exit(EXIT_FAILURE);
>                  }
>  
> -                sz = qemu_opt_get_size(opts, "size", ram_size);
> +                sz = qemu_opt_get_size(opts, "size", 0);
>  
>                  /* Fix up legacy suffix-less format */
>                  if (g_ascii_isdigit(mem_str[strlen(mem_str) - 1])) {
> @@ -3301,54 +3296,12 @@ int main(int argc, char **argv, char **envp)
>                  }
>  
>                  /* backward compatibility behaviour for case "-m 0" */
> -                if (sz == 0) {
> -                    sz = default_ram_size;
> -                }
> -
> -                sz = QEMU_ALIGN_UP(sz, 8192);
> -                ram_size = sz;
> -                if (ram_size != sz) {
> -                    error_report("ram size too large");
> -                    exit(EXIT_FAILURE);
> +                if (sz != 0) {
> +                    ram_size = sz;
>                  }
>  
> -                maxmem_str = qemu_opt_get(opts, "maxmem");
> -                slots_str = qemu_opt_get(opts, "slots");
> -                if (maxmem_str && slots_str) {
> -                    uint64_t slots;
> -
> -                    sz = qemu_opt_get_size(opts, "maxmem", 0);
> -                    if (sz < ram_size) {
> -                        fprintf(stderr, "qemu: invalid -m option value: 
> maxmem "
> -                                "(%" PRIu64 ") <= initial memory (%"
> -                                RAM_ADDR_FMT ")\n", sz, ram_size);
> -                        exit(EXIT_FAILURE);
> -                    }
> -
> -                    slots = qemu_opt_get_number(opts, "slots", 0);
> -                    if ((sz > ram_size) && !slots) {
> -                        fprintf(stderr, "qemu: invalid -m option value: 
> maxmem "
> -                                "(%" PRIu64 ") more than initial memory (%"
> -                                RAM_ADDR_FMT ") but no hotplug slots where "
> -                                "specified\n", sz, ram_size);
> -                        exit(EXIT_FAILURE);
> -                    }
> -
> -                    if ((sz <= ram_size) && slots) {
> -                        fprintf(stderr, "qemu: invalid -m option value:  %"
> -                                PRIu64 " hotplug slots where specified but "
> -                                "maxmem (%" PRIu64 ") <= initial memory (%"
> -                                RAM_ADDR_FMT ")\n", slots, sz, ram_size);
> -                        exit(EXIT_FAILURE);
> -                    }
> -                    maxram_size = sz;
> -                    ram_slots = slots;
> -                } else if ((!maxmem_str && slots_str) ||
> -                           (maxmem_str && !slots_str)) {
> -                    fprintf(stderr, "qemu: invalid -m option value: missing "
> -                            "'%s' option\n", slots_str ? "maxmem" : "slots");
> -                    exit(EXIT_FAILURE);
> -                }
> +                maxram_size_str = qemu_opt_get(opts, "maxmem");
> +                ram_slots_str = qemu_opt_get(opts, "slots");
>                  break;
>              }
>  #ifdef CONFIG_TPM
> @@ -4203,14 +4156,48 @@ int main(int argc, char **argv, char **envp)
>          exit(1);
>      }
>  
> -    /* store value for the future use */
> -    qemu_opt_set_number(qemu_find_opts_singleton("memory"), "size", 
> ram_size);
> -
>      if (qemu_opts_foreach(qemu_find_opts("device"), device_help_func, NULL, 
> 0)
>          != 0) {
>          exit(0);
>      }
>  
> +    if (ram_size) {
> +        object_property_set_int(OBJECT(current_machine), ram_size,
> +                                MACHINE_MEMORY_SIZE_OPT, &local_err);
> +        if (local_err) {
> +            error_report("%s", error_get_pretty(local_err));
> +            error_free(local_err);
> +            exit(EXIT_FAILURE);
> +        }
> +    }
> +
> +    if (maxram_size_str) {
> +        uint64_t sz = qemu_opt_get_size(qemu_find_opts_singleton("memory"),
> +                                        "maxmem", 0);
> +
> +        parse_option_size("maxmem", maxram_size_str, &sz, &local_err);
> +        if (local_err) {
> +            error_report("%s", error_get_pretty(local_err));
> +            error_free(local_err);
> +            exit(EXIT_FAILURE);
> +        }
> +        object_property_set_int(OBJECT(current_machine), sz,
> +                                MACHINE_MAXMEMORY_SIZE_OPT, &local_err);
> +        if (local_err) {
> +            error_report("%s", error_get_pretty(local_err));
> +            error_free(local_err);
> +            exit(EXIT_FAILURE);
> +        }
> +    }
> +
> +    if (ram_slots_str) {
> +        if (object_set_property(MACHINE_MEMORY_SLOTS_OPT, ram_slots_str,
> +                                current_machine)) {
> +            exit(EXIT_FAILURE);
> +        }
> +    }
> +
> +
>      machine_opts = qemu_get_machine_opts();
>      if (qemu_opt_foreach(machine_opts, object_set_property, current_machine,
>                           1) < 0) {
> @@ -4229,6 +4216,9 @@ int main(int argc, char **argv, char **envp)
>          }
>      }
>  
> +    ram_size = object_property_get_int(OBJECT(current_machine),
> +                                       MACHINE_MEMORY_SIZE_OPT,
> +                                       &error_abort);
>      machine_opts = qemu_get_machine_opts();
>      kernel_filename = qemu_opt_get(machine_opts, "kernel");
>      initrd_filename = qemu_opt_get(machine_opts, "initrd");
> @@ -4314,14 +4304,6 @@ int main(int argc, char **argv, char **envp)
>      if (foreach_device_config(DEV_BT, bt_parse))
>          exit(1);
>  
> -    if (!xen_enabled()) {
> -        /* On 32-bit hosts, QEMU is limited by virtual address space */
> -        if (ram_size > (2047 << 20) && HOST_LONG_BITS == 32) {
> -            fprintf(stderr, "qemu: at most 2047 MB RAM can be simulated\n");
> -            exit(1);
> -        }
> -    }
> -
>      blk_mig_init();
>      ram_mig_init();
>  
> @@ -4386,12 +4368,15 @@ int main(int argc, char **argv, char **envp)
>  
>      qdev_machine_init();
>  
> -    current_machine->ram_size = ram_size;
> -    current_machine->maxram_size = maxram_size;
> -    current_machine->ram_slots = ram_slots;
>      current_machine->boot_order = boot_order;
>      current_machine->cpu_model = cpu_model;
>  
> +    machine_class->instance_pre_init(current_machine, &local_err);
> +    if (local_err != NULL) {
> +        error_report("%s", error_get_pretty(local_err));
> +        error_free(local_err);
> +        exit(EXIT_FAILURE);
> +    }
>      machine_class->init(current_machine);
>  
>      audio_init();
> -- 
> 1.7.1