[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 3/4] machine: convert ram_size, maxram_size, ram
|
From: |
Michael S. Tsirkin |
|
Subject: |
Re: [Qemu-devel] [PATCH 3/4] machine: convert ram_size, maxram_size, ram_slots to properties |
|
Date: |
Wed, 25 Jun 2014 18:37:09 +0300 |
On Wed, Jun 25, 2014 at 03:08:03PM +0200, Igor Mammedov wrote:
> On Wed, 25 Jun 2014 14:54:47 +0300
> "Michael S. Tsirkin" <address@hidden> wrote:
>
> > 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?
> machine_set_ram_slots() could be reduced to:
>
> +static void machine_set_ram_slots(Object *obj, Visitor *v,
> + void *opaque, const char *name,
> + Error **errp)
> +{
> + MachineState *ms = MACHINE(obj);
> +
> + visit_type_int(v, &value, name, errp);
> +}
>
Still too much for a single property, sorry.
> maxram_size/ram_size setters could be reduced ~11 LOC moving common
> parts to a helper.
>
> But generic property validator is probably out of scope for this
> series
We have time to do it right, we are past development freeze so this is
2.2 material.
In fact, FYI I'm not queueing 2.2 patches anyway, it would help me
if you tagged them as RFC explicitly.
> and it won't help much in this case.
Done right, it should help.
Basically we have a ton of integer properties
where the only tricky thing is that
value is validated before it is saved.
So what I want is a variant of DEFINE_PROP
which invokes a callback on the value
before saving it, and checks the return code.
Along the lines of:
DEFINE_PROP_UINT32_VALIDATE("foo", type, field, default, validate),
> >
> >
> > > 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
> >
- Re: [Qemu-devel] [PATCH 2/4] vl.c: use single local_err throughout main(), (continued)
[Qemu-devel] [PATCH 1/4] switch RAM_ADDR_FMT format specifier to print decimal, Igor Mammedov, 2014/06/25
Re: [Qemu-devel] [PATCH 1/4] switch RAM_ADDR_FMT format specifier to print decimal, Peter Maydell, 2014/06/25
[Qemu-devel] [PATCH 3/4] machine: convert ram_size, maxram_size, ram_slots to properties, Igor Mammedov, 2014/06/25
Re: [Qemu-devel] [PATCH 3/4] machine: convert ram_size, maxram_size, ram_slots to properties, Kirill Batuzov, 2014/06/25
[Qemu-devel] [PATCH 4/4] test -m option parameters, Igor Mammedov, 2014/06/25