From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58006) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wzmvz-0000te-Gl for qemu-devel@nongnu.org; Wed, 25 Jun 2014 09:08:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wzmvs-0000S0-03 for qemu-devel@nongnu.org; Wed, 25 Jun 2014 09:08:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:14984) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wzmvr-0000Q8-KJ for qemu-devel@nongnu.org; Wed, 25 Jun 2014 09:08:11 -0400 Date: Wed, 25 Jun 2014 15:08:03 +0200 From: Igor Mammedov Message-ID: <20140625150803.287c400f@nial.usersys.redhat.com> In-Reply-To: <20140625115447.GC13690@redhat.com> References: <1403696543-2458-1-git-send-email-imammedo@redhat.com> <1403696543-2458-4-git-send-email-imammedo@redhat.com> <20140625115447.GC13690@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/4] machine: convert ram_size, maxram_size, ram_slots to properties List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: stefano.stabellini@eu.citrix.com, marcel.a@redhat.com, agraf@suse.de, qemu-devel@nongnu.org, pbonzini@redhat.com, afaerber@suse.de On Wed, 25 Jun 2014 14:54:47 +0300 "Michael S. Tsirkin" 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 > > --- > > 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); +} 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 and it won't help much in this case. > > > > 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 >