| Message ID | 1385001528-12003-5-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
Igor Mammedov wrote: > Along with conversion extend -m option to support following parameters > ... > + if (!opts) { > exit(1); > } > - sz = QEMU_ALIGN_UP((uint64_t)value, 8192); > + > + /* fixup legacy sugffix-less format */ > s/sugffix/suffix > + end = qemu_opt_get(opts, "mem"); > + if (g_ascii_isdigit(end[strlen(end) - 1])) { > + s = g_strconcat(end, "M", NULL); > + qemu_opt_set(opts, "mem", s); > + g_free(s); > + } > + > + sz = QEMU_ALIGN_UP(qemu_opt_get_size(opts, "mem", ram_size), > + 8192); > + /* compatibility behaviour for case "-m 0" */ > + if (sz == 0) { > + sz = DEFAULT_RAM_SIZE * 1024 * 1024; > + } > + > ram_size = sz; > if (ram_size != sz) { > fprintf(stderr, "qemu: ram size too large\n"); > exit(1); > } > + /* store aligned value for future use */ > + qemu_opt_set_number(opts, "mem", ram_size); > + > + sz = qemu_opt_get_size(opts, "maxmem", ram_size); > + if (sz< ram_size) { > + qemu_opt_set_number(opts, "maxmem", ram_size); > + } > break; > } > #ifdef CONFIG_TPM > @@ -4029,11 +4083,6 @@ int main(int argc, char **argv, char **envp) > exit(1); > } > > - /* init the memory */ > - if (ram_size == 0) { > - ram_size = DEFAULT_RAM_SIZE * 1024 * 1024; > - } > - > if (qemu_opts_foreach(qemu_find_opts("device"), device_help_func, NULL, 0) > != 0) { > exit(0); >
Igor Mammedov <imammedo@redhat.com> writes: > Along with conversion extend -m option to support following parameters: Please split this into two patches: first conversion to QemuOpts, then extension. > "mem" - startup memory amount > "slots" - total number of hotplug memory slots > "maxmem" - maximum possible memory > > "slots" and "maxmem" should go in pair and "maxmem" should be greater > than "mem" for memory hotplug to be usable. > > v2: > make sure maxmem is not less than ram size > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > qemu-options.hx | 9 +++++- > vl.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++--------- > 2 files changed, 68 insertions(+), 14 deletions(-) > > diff --git a/qemu-options.hx b/qemu-options.hx > index 8b94264..fe4559b 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -210,8 +210,13 @@ use is discouraged as it may be removed from future versions. > ETEXI > > DEF("m", HAS_ARG, QEMU_OPTION_m, > - "-m megs set virtual RAM size to megs MB [default=" > - stringify(DEFAULT_RAM_SIZE) "]\n", QEMU_ARCH_ALL) > + "-m [mem=]megs[,slots=n,maxmem=size]\n" > + " set virtual RAM size to megs MB [default=" > + stringify(DEFAULT_RAM_SIZE) "]\n" > + " mem=start-up memory amount\n" > + " slots=maximum number of hotplug slots\n" > + " maxmem=maximum total amount of memory\n", > + QEMU_ARCH_ALL) Help text is confusing, because it discusses megs twice. Fits right in, as output of -help is generally confusing (to put it politely). What about something like this: -m [mem=]megs[,slots=n,maxmem=size] configure guest RAM mem: initial amount of guest memory (default: XXX) slots: number of hotplug slots (default: none) maxmem: maximum amount of guest memory (default: mem) > STEXI > @item -m @var{megs} > @findex -m > diff --git a/vl.c b/vl.c > index f28674f..5974f0f 100644 > --- a/vl.c > +++ b/vl.c > @@ -529,6 +529,28 @@ static QemuOptsList qemu_msg_opts = { > }, > }; > > +static QemuOptsList qemu_mem_opts = { > + .name = "memory-opts", > + .implied_opt_name = "mem", > + .head = QTAILQ_HEAD_INITIALIZER(qemu_mem_opts.head), > + .merge_lists = true, Yes, because multiple -m need to accumulate. > + .desc = { > + { > + .name = "mem", > + .type = QEMU_OPT_SIZE, > + }, > + { > + .name = "slots", > + .type = QEMU_OPT_NUMBER, > + }, > + { > + .name = "maxmem", > + .type = QEMU_OPT_SIZE, > + }, > + { /* end of list */ } > + }, > +}; > + > /** > * Get machine options > * > @@ -2816,6 +2838,14 @@ static int object_create(QemuOpts *opts, void *opaque) > return 0; > } > > +static void qemu_init_default_mem_opts(uint64_t size) > +{ > + QemuOpts *opts = qemu_opts_create_nofail(&qemu_mem_opts); > + qemu_opt_set_number(opts, "mem", size); > + qemu_opt_set_number(opts, "maxmem", size); > + qemu_opt_set_number(opts, "slots", 0); > +} > + We usually don't put defaults in QemuOpts. Instead, the code querying QemuOpts detects and handles absence of the parameter. Can be as simple as qemu_opt_get_size(opts, "mem", DEFAULT_RAM_SIZE * 1024 * 1024). See also below. > int main(int argc, char **argv, char **envp) > { > int i; > @@ -2887,6 +2917,7 @@ int main(int argc, char **argv, char **envp) > qemu_add_opts(&qemu_tpmdev_opts); > qemu_add_opts(&qemu_realtime_opts); > qemu_add_opts(&qemu_msg_opts); > + qemu_add_opts(&qemu_mem_opts); > > runstate_init(); > > @@ -2901,7 +2932,8 @@ int main(int argc, char **argv, char **envp) > module_call_init(MODULE_INIT_MACHINE); > machine = find_default_machine(); > cpu_model = NULL; > - ram_size = 0; > + ram_size = DEFAULT_RAM_SIZE * 1024 * 1024; > + qemu_init_default_mem_opts(ram_size); > snapshot = 0; > cyls = heads = secs = 0; > translation = BIOS_ATA_TRANSLATION_AUTO; > @@ -3178,21 +3210,43 @@ int main(int argc, char **argv, char **envp) > exit(0); > break; > case QEMU_OPTION_m: { > - int64_t value; > uint64_t sz; > - char *end; > + const char *end; > + char *s; > > - value = strtosz(optarg, &end); > - if (value < 0 || *end) { > - fprintf(stderr, "qemu: invalid ram size: %s\n", optarg); > + opts = qemu_opts_parse(qemu_find_opts("memory-opts"), > + optarg, 1); > + if (!opts) { > exit(1); > } > - sz = QEMU_ALIGN_UP((uint64_t)value, 8192); > + > + /* fixup legacy sugffix-less format */ /* fix up legacy suffix-less format */ The problem here is that OPT_SIZE treats values without a size suffix as bytes, but we need to default to MiB for backward compatibility. > + end = qemu_opt_get(opts, "mem"); > + if (g_ascii_isdigit(end[strlen(end) - 1])) { > + s = g_strconcat(end, "M", NULL); > + qemu_opt_set(opts, "mem", s); > + g_free(s); > + } Ugly. Why is the variable called 'end'? qemu_opt_set() appends to the list of options. The un-fixed-up option remains in the list. qemu_opt_unset() could fix that, but it asserts opts_accepts_any() for unclear reasons. git-blame points to Kevin. Have you considered qemu_opt_set_number()? If this "need a default suffix" pattern exists elsewhere, we should consider extending QemuOptDesc to cover it. > + > + sz = QEMU_ALIGN_UP(qemu_opt_get_size(opts, "mem", ram_size), > + 8192); > + /* compatibility behaviour for case "-m 0" */ > + if (sz == 0) { > + sz = DEFAULT_RAM_SIZE * 1024 * 1024; > + } > + Yes, this is needed. Our command line is bonkers. > ram_size = sz; > if (ram_size != sz) { > fprintf(stderr, "qemu: ram size too large\n"); > exit(1); > } > + /* store aligned value for future use */ > + qemu_opt_set_number(opts, "mem", ram_size); Here, you use qemu_opt_set_number(). Again, this appends to the list, and leaves the non-aligned value in. > + > + sz = qemu_opt_get_size(opts, "maxmem", ram_size); > + if (sz < ram_size) { > + qemu_opt_set_number(opts, "maxmem", ram_size); > + } > break; > } Looks like you want to fix up something like "-m 1024", so that maxmem stays equal to mem. I'm afraid you also "fix" user errors like "-m mem=1024M,maxmem=512M". If you refrain from putting defaults into opts, you can distinguish the cases "user didn't specify maxmem, so assume mem", and "user specified maxmem, so check it's >= mem". > #ifdef CONFIG_TPM > @@ -4029,11 +4083,6 @@ int main(int argc, char **argv, char **envp) > exit(1); > } > > - /* init the memory */ > - if (ram_size == 0) { > - ram_size = DEFAULT_RAM_SIZE * 1024 * 1024; > - } > - > if (qemu_opts_foreach(qemu_find_opts("device"), device_help_func, NULL, 0) > != 0) { > exit(0);
On Thu, 21 Nov 2013 14:01:43 +0800 Li Guang <lig.fnst@cn.fujitsu.com> wrote: > Igor Mammedov wrote: > > Along with conversion extend -m option to support following parameters > > > ... > > + if (!opts) { > > exit(1); > > } > > - sz = QEMU_ALIGN_UP((uint64_t)value, 8192); > > + > > + /* fixup legacy sugffix-less format */ > > > s/sugffix/suffix > Thaks, fixed in memhpv2-wip-on-pci tree
Il 21/11/2013 03:38, Igor Mammedov ha scritto: > Along with conversion extend -m option to support following parameters: > "mem" - startup memory amount > "slots" - total number of hotplug memory slots > "maxmem" - maximum possible memory > > "slots" and "maxmem" should go in pair and "maxmem" should be greater > than "mem" for memory hotplug to be usable. Strictly speaking, slots and maxmem should be added later, not in this patch. Not a blocker, though. Paolo > v2: > make sure maxmem is not less than ram size > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > qemu-options.hx | 9 +++++- > vl.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++--------- > 2 files changed, 68 insertions(+), 14 deletions(-) > > diff --git a/qemu-options.hx b/qemu-options.hx > index 8b94264..fe4559b 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -210,8 +210,13 @@ use is discouraged as it may be removed from future versions. > ETEXI > > DEF("m", HAS_ARG, QEMU_OPTION_m, > - "-m megs set virtual RAM size to megs MB [default=" > - stringify(DEFAULT_RAM_SIZE) "]\n", QEMU_ARCH_ALL) > + "-m [mem=]megs[,slots=n,maxmem=size]\n" > + " set virtual RAM size to megs MB [default=" > + stringify(DEFAULT_RAM_SIZE) "]\n" > + " mem=start-up memory amount\n" > + " slots=maximum number of hotplug slots\n" > + " maxmem=maximum total amount of memory\n", > + QEMU_ARCH_ALL) > STEXI > @item -m @var{megs} > @findex -m > diff --git a/vl.c b/vl.c > index f28674f..5974f0f 100644 > --- a/vl.c > +++ b/vl.c > @@ -529,6 +529,28 @@ static QemuOptsList qemu_msg_opts = { > }, > }; > > +static QemuOptsList qemu_mem_opts = { > + .name = "memory-opts", > + .implied_opt_name = "mem", > + .head = QTAILQ_HEAD_INITIALIZER(qemu_mem_opts.head), > + .merge_lists = true, > + .desc = { > + { > + .name = "mem", > + .type = QEMU_OPT_SIZE, > + }, > + { > + .name = "slots", > + .type = QEMU_OPT_NUMBER, > + }, > + { > + .name = "maxmem", > + .type = QEMU_OPT_SIZE, > + }, > + { /* end of list */ } > + }, > +}; > + > /** > * Get machine options > * > @@ -2816,6 +2838,14 @@ static int object_create(QemuOpts *opts, void *opaque) > return 0; > } > > +static void qemu_init_default_mem_opts(uint64_t size) > +{ > + QemuOpts *opts = qemu_opts_create_nofail(&qemu_mem_opts); > + qemu_opt_set_number(opts, "mem", size); > + qemu_opt_set_number(opts, "maxmem", size); > + qemu_opt_set_number(opts, "slots", 0); > +} > + > int main(int argc, char **argv, char **envp) > { > int i; > @@ -2887,6 +2917,7 @@ int main(int argc, char **argv, char **envp) > qemu_add_opts(&qemu_tpmdev_opts); > qemu_add_opts(&qemu_realtime_opts); > qemu_add_opts(&qemu_msg_opts); > + qemu_add_opts(&qemu_mem_opts); > > runstate_init(); > > @@ -2901,7 +2932,8 @@ int main(int argc, char **argv, char **envp) > module_call_init(MODULE_INIT_MACHINE); > machine = find_default_machine(); > cpu_model = NULL; > - ram_size = 0; > + ram_size = DEFAULT_RAM_SIZE * 1024 * 1024; > + qemu_init_default_mem_opts(ram_size); > snapshot = 0; > cyls = heads = secs = 0; > translation = BIOS_ATA_TRANSLATION_AUTO; > @@ -3178,21 +3210,43 @@ int main(int argc, char **argv, char **envp) > exit(0); > break; > case QEMU_OPTION_m: { > - int64_t value; > uint64_t sz; > - char *end; > + const char *end; > + char *s; > > - value = strtosz(optarg, &end); > - if (value < 0 || *end) { > - fprintf(stderr, "qemu: invalid ram size: %s\n", optarg); > + opts = qemu_opts_parse(qemu_find_opts("memory-opts"), > + optarg, 1); > + if (!opts) { > exit(1); > } > - sz = QEMU_ALIGN_UP((uint64_t)value, 8192); > + > + /* fixup legacy sugffix-less format */ > + end = qemu_opt_get(opts, "mem"); > + if (g_ascii_isdigit(end[strlen(end) - 1])) { > + s = g_strconcat(end, "M", NULL); > + qemu_opt_set(opts, "mem", s); > + g_free(s); > + } > + > + sz = QEMU_ALIGN_UP(qemu_opt_get_size(opts, "mem", ram_size), > + 8192); > + /* compatibility behaviour for case "-m 0" */ > + if (sz == 0) { > + sz = DEFAULT_RAM_SIZE * 1024 * 1024; > + } > + > ram_size = sz; > if (ram_size != sz) { > fprintf(stderr, "qemu: ram size too large\n"); > exit(1); > } > + /* store aligned value for future use */ > + qemu_opt_set_number(opts, "mem", ram_size); > + > + sz = qemu_opt_get_size(opts, "maxmem", ram_size); > + if (sz < ram_size) { > + qemu_opt_set_number(opts, "maxmem", ram_size); > + } > break; > } > #ifdef CONFIG_TPM > @@ -4029,11 +4083,6 @@ int main(int argc, char **argv, char **envp) > exit(1); > } > > - /* init the memory */ > - if (ram_size == 0) { > - ram_size = DEFAULT_RAM_SIZE * 1024 * 1024; > - } > - > if (qemu_opts_foreach(qemu_find_opts("device"), device_help_func, NULL, 0) > != 0) { > exit(0); >
On Thu, 21 Nov 2013 11:12:43 +0100 Markus Armbruster <armbru@redhat.com> wrote: > Igor Mammedov <imammedo@redhat.com> writes: > > > Along with conversion extend -m option to support following parameters: > > Please split this into two patches: first conversion to QemuOpts, then > extension. > > > "mem" - startup memory amount > > "slots" - total number of hotplug memory slots > > "maxmem" - maximum possible memory > > > > "slots" and "maxmem" should go in pair and "maxmem" should be greater > > than "mem" for memory hotplug to be usable. > > > > v2: > > make sure maxmem is not less than ram size > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > qemu-options.hx | 9 +++++- > > vl.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++--------- > > 2 files changed, 68 insertions(+), 14 deletions(-) > > > > diff --git a/qemu-options.hx b/qemu-options.hx > > index 8b94264..fe4559b 100644 > > --- a/qemu-options.hx > > +++ b/qemu-options.hx > > @@ -210,8 +210,13 @@ use is discouraged as it may be removed from future versions. > > ETEXI > > > > DEF("m", HAS_ARG, QEMU_OPTION_m, > > - "-m megs set virtual RAM size to megs MB [default=" > > - stringify(DEFAULT_RAM_SIZE) "]\n", QEMU_ARCH_ALL) > > + "-m [mem=]megs[,slots=n,maxmem=size]\n" > > + " set virtual RAM size to megs MB [default=" > > + stringify(DEFAULT_RAM_SIZE) "]\n" > > + " mem=start-up memory amount\n" > > + " slots=maximum number of hotplug slots\n" > > + " maxmem=maximum total amount of memory\n", > > + QEMU_ARCH_ALL) > > Help text is confusing, because it discusses megs twice. Fits right in, > as output of -help is generally confusing (to put it politely). > > What about something like this: > > -m [mem=]megs[,slots=n,maxmem=size] > configure guest RAM > mem: initial amount of guest memory (default: XXX) > slots: number of hotplug slots (default: none) > maxmem: maximum amount of guest memory (default: mem) Sure, I'll fix it. > > STEXI > > @item -m @var{megs} > > @findex -m > > diff --git a/vl.c b/vl.c > > index f28674f..5974f0f 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -529,6 +529,28 @@ static QemuOptsList qemu_msg_opts = { > > }, > > }; > > > > +static QemuOptsList qemu_mem_opts = { > > + .name = "memory-opts", > > + .implied_opt_name = "mem", > > + .head = QTAILQ_HEAD_INITIALIZER(qemu_mem_opts.head), > > + .merge_lists = true, > > Yes, because multiple -m need to accumulate. > > > + .desc = { > > + { > > + .name = "mem", > > + .type = QEMU_OPT_SIZE, > > + }, > > + { > > + .name = "slots", > > + .type = QEMU_OPT_NUMBER, > > + }, > > + { > > + .name = "maxmem", > > + .type = QEMU_OPT_SIZE, > > + }, > > + { /* end of list */ } > > + }, > > +}; > > + > > /** > > * Get machine options > > * > > @@ -2816,6 +2838,14 @@ static int object_create(QemuOpts *opts, void *opaque) > > return 0; > > } > > > > +static void qemu_init_default_mem_opts(uint64_t size) > > +{ > > + QemuOpts *opts = qemu_opts_create_nofail(&qemu_mem_opts); > > + qemu_opt_set_number(opts, "mem", size); > > + qemu_opt_set_number(opts, "maxmem", size); > > + qemu_opt_set_number(opts, "slots", 0); > > +} > > + > > We usually don't put defaults in QemuOpts. Instead, the code querying > QemuOpts detects and handles absence of the parameter. Can be as simple > as qemu_opt_get_size(opts, "mem", DEFAULT_RAM_SIZE * 1024 * 1024). It allows to make number of uses a bit simpler: for example v6: QemuOpts *opts = qemu_opts_find(qemu_find_opts("memory-opts"), NULL); if (!opts) { /* no -m x,... was passed to cmd line so no mem hotplug */ return; } mem_st->dev_count = qemu_opt_get_number(opts, "slots", 0); becomes" QemuOpts *opts = qemu_opts_find(qemu_find_opts("memory-opts"), NULL); state->dev_count = qemu_opt_get_number(opts, "slots", 0); and eliminates need to pepper code with DEFAULT_RAM_SIZE * 1024 * 1024 or like constants wherever qemu_opt_get_..() is called, that was main reason to set defaults. i.e. set defaults only once instead of spreading them in every place qemu_opt_get_..() is called. > > See also below. > > > int main(int argc, char **argv, char **envp) > > { > > int i; > > @@ -2887,6 +2917,7 @@ int main(int argc, char **argv, char **envp) > > qemu_add_opts(&qemu_tpmdev_opts); > > qemu_add_opts(&qemu_realtime_opts); > > qemu_add_opts(&qemu_msg_opts); > > + qemu_add_opts(&qemu_mem_opts); > > > > runstate_init(); > > > > @@ -2901,7 +2932,8 @@ int main(int argc, char **argv, char **envp) > > module_call_init(MODULE_INIT_MACHINE); > > machine = find_default_machine(); > > cpu_model = NULL; > > - ram_size = 0; > > + ram_size = DEFAULT_RAM_SIZE * 1024 * 1024; > > + qemu_init_default_mem_opts(ram_size); > > snapshot = 0; > > cyls = heads = secs = 0; > > translation = BIOS_ATA_TRANSLATION_AUTO; > > @@ -3178,21 +3210,43 @@ int main(int argc, char **argv, char **envp) > > exit(0); > > break; > > case QEMU_OPTION_m: { > > - int64_t value; > > uint64_t sz; > > - char *end; > > + const char *end; > > + char *s; > > > > - value = strtosz(optarg, &end); > > - if (value < 0 || *end) { > > - fprintf(stderr, "qemu: invalid ram size: %s\n", optarg); > > + opts = qemu_opts_parse(qemu_find_opts("memory-opts"), > > + optarg, 1); > > + if (!opts) { > > exit(1); > > } > > - sz = QEMU_ALIGN_UP((uint64_t)value, 8192); > > + > > + /* fixup legacy sugffix-less format */ > > /* fix up legacy suffix-less format */ > > The problem here is that OPT_SIZE treats values without a size suffix as > bytes, but we need to default to MiB for backward compatibility. > > > + end = qemu_opt_get(opts, "mem"); > > + if (g_ascii_isdigit(end[strlen(end) - 1])) { > > + s = g_strconcat(end, "M", NULL); > > + qemu_opt_set(opts, "mem", s); > > + g_free(s); > > + } > > Ugly. > > Why is the variable called 'end'? would be 'suffix' better? > > qemu_opt_set() appends to the list of options. The un-fixed-up option > remains in the list. qemu_opt_unset() could fix that, but it asserts > opts_accepts_any() for unclear reasons. git-blame points to Kevin. it would be cleaner to unset it but it works event without unsetting it, since qemu_opt_set...() adds to tail and qemu_opt_get...() finds options from tail to head. Nevertheless, Kevin do you recall reasons for assert in 0dd6c526: int qemu_opt_unset(QemuOpts *opts, const char *name) ... assert(opts_accepts_any(opts)); would it be ok if I remove it? > > Have you considered qemu_opt_set_number()? it was code left from v6 when I didn't know about it. Sorry, I'll use it instead. > > If this "need a default suffix" pattern exists elsewhere, we should > consider extending QemuOptDesc to cover it. I haven't seen/don't recall need for it anywhere else, but it would be cleanest solution. But it would introduce temptation for users to shrug off suffixes which is wrong in my opinion. -m 1024 is confusing unless you know that it's in Mb. it would be better not to introduce mechanism, that would allow to do such thing in favor of explicit suffixes. > > > + > > + sz = QEMU_ALIGN_UP(qemu_opt_get_size(opts, "mem", ram_size), > > + 8192); > > + /* compatibility behaviour for case "-m 0" */ > > + if (sz == 0) { > > + sz = DEFAULT_RAM_SIZE * 1024 * 1024; > > + } > > + > > Yes, this is needed. Our command line is bonkers. > > > ram_size = sz; > > if (ram_size != sz) { > > fprintf(stderr, "qemu: ram size too large\n"); > > exit(1); > > } > > + /* store aligned value for future use */ > > + qemu_opt_set_number(opts, "mem", ram_size); > > Here, you use qemu_opt_set_number(). > > Again, this appends to the list, and leaves the non-aligned value in. it's not an issue since the last appended opt is used in qemu_opt_get_size(). > > > + > > + sz = qemu_opt_get_size(opts, "maxmem", ram_size); > > + if (sz < ram_size) { > > + qemu_opt_set_number(opts, "maxmem", ram_size); > > + } > > break; > > } > > Looks like you want to fix up something like "-m 1024", so that maxmem > stays equal to mem. I'm afraid you also "fix" user errors like "-m > mem=1024M,maxmem=512M". Perhaps it would be better to bail out with error here. > > If you refrain from putting defaults into opts, you can distinguish the > cases "user didn't specify maxmem, so assume mem", and "user specified > maxmem, so check it's >= mem". So foar, there is no point in distinguishing above cases, since maxmem <= mem is invalid value and hotplug should be disabled. So setting default maxmem to mem or anything less effectively disables hotplug. > > > #ifdef CONFIG_TPM > > @@ -4029,11 +4083,6 @@ int main(int argc, char **argv, char **envp) > > exit(1); > > } > > > > - /* init the memory */ > > - if (ram_size == 0) { > > - ram_size = DEFAULT_RAM_SIZE * 1024 * 1024; > > - } > > - > > if (qemu_opts_foreach(qemu_find_opts("device"), device_help_func, NULL, 0) > > != 0) { > > exit(0); Thanks for review!
Igor Mammedov <imammedo@redhat.com> writes: > On Thu, 21 Nov 2013 11:12:43 +0100 > Markus Armbruster <armbru@redhat.com> wrote: > >> Igor Mammedov <imammedo@redhat.com> writes: >> >> > Along with conversion extend -m option to support following parameters: >> >> Please split this into two patches: first conversion to QemuOpts, then >> extension. >> >> > "mem" - startup memory amount >> > "slots" - total number of hotplug memory slots >> > "maxmem" - maximum possible memory >> > >> > "slots" and "maxmem" should go in pair and "maxmem" should be greater >> > than "mem" for memory hotplug to be usable. >> > >> > v2: >> > make sure maxmem is not less than ram size >> > >> > Signed-off-by: Igor Mammedov <imammedo@redhat.com> >> > --- >> > qemu-options.hx | 9 +++++- >> > vl.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++--------- >> > 2 files changed, 68 insertions(+), 14 deletions(-) >> > >> > diff --git a/qemu-options.hx b/qemu-options.hx >> > index 8b94264..fe4559b 100644 >> > --- a/qemu-options.hx >> > +++ b/qemu-options.hx >> > @@ -210,8 +210,13 @@ use is discouraged as it may be removed from future versions. >> > ETEXI >> > >> > DEF("m", HAS_ARG, QEMU_OPTION_m, >> > - "-m megs set virtual RAM size to megs MB [default=" >> > - stringify(DEFAULT_RAM_SIZE) "]\n", QEMU_ARCH_ALL) >> > + "-m [mem=]megs[,slots=n,maxmem=size]\n" >> > + " set virtual RAM size to megs MB [default=" >> > + stringify(DEFAULT_RAM_SIZE) "]\n" >> > + " mem=start-up memory amount\n" >> > + " slots=maximum number of hotplug slots\n" >> > + " maxmem=maximum total amount of memory\n", >> > + QEMU_ARCH_ALL) >> >> Help text is confusing, because it discusses megs twice. Fits right in, >> as output of -help is generally confusing (to put it politely). >> >> What about something like this: >> >> -m [mem=]megs[,slots=n,maxmem=size] >> configure guest RAM >> mem: initial amount of guest memory (default: XXX) >> slots: number of hotplug slots (default: none) >> maxmem: maximum amount of guest memory (default: mem) > Sure, I'll fix it. > >> > STEXI >> > @item -m @var{megs} >> > @findex -m >> > diff --git a/vl.c b/vl.c >> > index f28674f..5974f0f 100644 >> > --- a/vl.c >> > +++ b/vl.c >> > @@ -529,6 +529,28 @@ static QemuOptsList qemu_msg_opts = { >> > }, >> > }; >> > >> > +static QemuOptsList qemu_mem_opts = { >> > + .name = "memory-opts", >> > + .implied_opt_name = "mem", >> > + .head = QTAILQ_HEAD_INITIALIZER(qemu_mem_opts.head), >> > + .merge_lists = true, >> >> Yes, because multiple -m need to accumulate. >> >> > + .desc = { >> > + { >> > + .name = "mem", >> > + .type = QEMU_OPT_SIZE, >> > + }, >> > + { >> > + .name = "slots", >> > + .type = QEMU_OPT_NUMBER, >> > + }, >> > + { >> > + .name = "maxmem", >> > + .type = QEMU_OPT_SIZE, >> > + }, >> > + { /* end of list */ } >> > + }, >> > +}; >> > + >> > /** >> > * Get machine options >> > * >> > @@ -2816,6 +2838,14 @@ static int object_create(QemuOpts *opts, void *opaque) >> > return 0; >> > } >> > >> > +static void qemu_init_default_mem_opts(uint64_t size) >> > +{ >> > + QemuOpts *opts = qemu_opts_create_nofail(&qemu_mem_opts); >> > + qemu_opt_set_number(opts, "mem", size); >> > + qemu_opt_set_number(opts, "maxmem", size); >> > + qemu_opt_set_number(opts, "slots", 0); >> > +} >> > + >> >> We usually don't put defaults in QemuOpts. Instead, the code querying >> QemuOpts detects and handles absence of the parameter. Can be as simple >> as qemu_opt_get_size(opts, "mem", DEFAULT_RAM_SIZE * 1024 * 1024). > It allows to make number of uses a bit simpler: > > for example v6: > QemuOpts *opts = qemu_opts_find(qemu_find_opts("memory-opts"), NULL); > if (!opts) { /* no -m x,... was passed to cmd line so no mem hotplug */ > return; > } > mem_st->dev_count = qemu_opt_get_number(opts, "slots", 0); > > becomes" > QemuOpts *opts = qemu_opts_find(qemu_find_opts("memory-opts"), NULL); > state->dev_count = qemu_opt_get_number(opts, "slots", 0); > > and eliminates need to pepper code with DEFAULT_RAM_SIZE * 1024 * 1024 or > like constants wherever qemu_opt_get_..() is called, that was main reason > to set defaults. i.e. set defaults only once instead of spreading them in > every place qemu_opt_get_..() is called. Two separate issues here: 1. The "no qemu_mem_opts have been specified" case This is equivalent to "empty options". Therefore, the case can be eliminated by pre-creating empty options. No objection. The three existing merge_lists users don't do that. Perhaps they should. 2. How to provide default values Supplying defaults is left to the caller of qemu_opt_get_FOO() by design. Pre-creating option parameters deviates from that pattern. You justify it by saying it "eliminates need to pepper code with DEFAULT_RAM_SIZE * 1024 * 1024". How many occurrences? Drawback: you lose the ability to see whether the user gave a value. See below. >> See also below. >> >> > int main(int argc, char **argv, char **envp) >> > { >> > int i; >> > @@ -2887,6 +2917,7 @@ int main(int argc, char **argv, char **envp) >> > qemu_add_opts(&qemu_tpmdev_opts); >> > qemu_add_opts(&qemu_realtime_opts); >> > qemu_add_opts(&qemu_msg_opts); >> > + qemu_add_opts(&qemu_mem_opts); >> > >> > runstate_init(); >> > >> > @@ -2901,7 +2932,8 @@ int main(int argc, char **argv, char **envp) >> > module_call_init(MODULE_INIT_MACHINE); >> > machine = find_default_machine(); >> > cpu_model = NULL; >> > - ram_size = 0; >> > + ram_size = DEFAULT_RAM_SIZE * 1024 * 1024; >> > + qemu_init_default_mem_opts(ram_size); >> > snapshot = 0; >> > cyls = heads = secs = 0; >> > translation = BIOS_ATA_TRANSLATION_AUTO; >> > @@ -3178,21 +3210,43 @@ int main(int argc, char **argv, char **envp) >> > exit(0); >> > break; >> > case QEMU_OPTION_m: { >> > - int64_t value; >> > uint64_t sz; >> > - char *end; >> > + const char *end; >> > + char *s; >> > >> > - value = strtosz(optarg, &end); >> > - if (value < 0 || *end) { >> > - fprintf(stderr, "qemu: invalid ram size: %s\n", optarg); >> > + opts = qemu_opts_parse(qemu_find_opts("memory-opts"), >> > + optarg, 1); >> > + if (!opts) { >> > exit(1); >> > } >> > - sz = QEMU_ALIGN_UP((uint64_t)value, 8192); >> > + >> > + /* fixup legacy sugffix-less format */ >> >> /* fix up legacy suffix-less format */ >> >> The problem here is that OPT_SIZE treats values without a size suffix as >> bytes, but we need to default to MiB for backward compatibility. >> >> > + end = qemu_opt_get(opts, "mem"); >> > + if (g_ascii_isdigit(end[strlen(end) - 1])) { >> > + s = g_strconcat(end, "M", NULL); >> > + qemu_opt_set(opts, "mem", s); >> > + g_free(s); >> > + } >> >> Ugly. >> >> Why is the variable called 'end'? > would be 'suffix' better? end points to the whole value string, not the end of anything, and neither to a suffix of anything. >> qemu_opt_set() appends to the list of options. The un-fixed-up option >> remains in the list. qemu_opt_unset() could fix that, but it asserts >> opts_accepts_any() for unclear reasons. git-blame points to Kevin. > it would be cleaner to unset it but it works event without unsetting it, > since qemu_opt_set...() adds to tail and qemu_opt_get...() finds options > from tail to head. > > Nevertheless, Kevin do you recall reasons for assert in 0dd6c526: > > int qemu_opt_unset(QemuOpts *opts, const char *name) > ... > assert(opts_accepts_any(opts)); > > would it be ok if I remove it? > >> >> Have you considered qemu_opt_set_number()? > it was code left from v6 when I didn't know about it. Sorry, I'll > use it instead. > >> >> If this "need a default suffix" pattern exists elsewhere, we should >> consider extending QemuOptDesc to cover it. > I haven't seen/don't recall need for it anywhere else, but it would be > cleanest solution. But it would introduce temptation for users > to shrug off suffixes which is wrong in my opinion. -m 1024 is confusing > unless you know that it's in Mb. > > it would be better not to introduce mechanism, that would allow to do such > thing in favor of explicit suffixes. I'm afraid that horse as left the barn long ago: * -numa mem=VAL accepts an optional suffix, defaulting it to 'M'. * Likewise, HMP commands block_resize, block_stream, block_job_set_speed, migrate_set_cache_size, migrate_set_speed. But point taken. >> > + >> > + sz = QEMU_ALIGN_UP(qemu_opt_get_size(opts, "mem", ram_size), >> > + 8192); >> > + /* compatibility behaviour for case "-m 0" */ >> > + if (sz == 0) { >> > + sz = DEFAULT_RAM_SIZE * 1024 * 1024; >> > + } >> > + >> >> Yes, this is needed. Our command line is bonkers. >> >> > ram_size = sz; >> > if (ram_size != sz) { >> > fprintf(stderr, "qemu: ram size too large\n"); >> > exit(1); >> > } >> > + /* store aligned value for future use */ >> > + qemu_opt_set_number(opts, "mem", ram_size); >> >> Here, you use qemu_opt_set_number(). >> >> Again, this appends to the list, and leaves the non-aligned value in. > it's not an issue since the last appended opt is used in qemu_opt_get_size(). > >> >> > + >> > + sz = qemu_opt_get_size(opts, "maxmem", ram_size); >> > + if (sz < ram_size) { >> > + qemu_opt_set_number(opts, "maxmem", ram_size); >> > + } >> > break; >> > } >> >> Looks like you want to fix up something like "-m 1024", so that maxmem >> stays equal to mem. I'm afraid you also "fix" user errors like "-m >> mem=1024M,maxmem=512M". > Perhaps it would be better to bail out with error here. > >> >> If you refrain from putting defaults into opts, you can distinguish the >> cases "user didn't specify maxmem, so assume mem", and "user specified >> maxmem, so check it's >= mem". > So foar, there is no point in distinguishing above cases, > since maxmem <= mem is invalid value and hotplug should be disabled. > So setting default maxmem to mem or anything less effectively disables hotplug. Yes, setting maxmem < mem is invalid and should be rejected, but not setting maxmem at all should be accepted even when you set mem. Your patch like this pseudo-code: mem = DEFAULT_RAM_SIZE * 1024 * 1024 maxmem = mem if user specifies mem: mem = user's mem if user specifes max-mem: mem = user's max-mem if max-mem < mem what now? should error our if max-mem and mem were specified by the user shouldn't if user didn't specify max-mem! but can't say whether he did I'd do it this way: mem = unset maxmem = unset if user specifies mem: mem = user's mem if user specifes max-mem: mem = user's max-mem if mem != unset && max-mem != unset && max-mem < mem error I'd use QemuOpts for the user's command line, and no more. For anything beyond that, I'd use ordinary variables, such as ram_size. >> > #ifdef CONFIG_TPM >> > @@ -4029,11 +4083,6 @@ int main(int argc, char **argv, char **envp) >> > exit(1); >> > } >> > >> > - /* init the memory */ >> > - if (ram_size == 0) { >> > - ram_size = DEFAULT_RAM_SIZE * 1024 * 1024; >> > - } >> > - >> > if (qemu_opts_foreach(qemu_find_opts("device"), device_help_func, NULL, 0) >> > != 0) { >> > exit(0); > > Thanks for review!
On Tue, 26 Nov 2013 15:49:05 +0100 Markus Armbruster <armbru@redhat.com> wrote: > Igor Mammedov <imammedo@redhat.com> writes: > > > On Thu, 21 Nov 2013 11:12:43 +0100 > > Markus Armbruster <armbru@redhat.com> wrote: > > > >> Igor Mammedov <imammedo@redhat.com> writes: > >> [...] > Two separate issues here: > > 1. The "no qemu_mem_opts have been specified" case > > This is equivalent to "empty options". Therefore, the case can be > eliminated by pre-creating empty options. No objection. > > The three existing merge_lists users don't do that. Perhaps they > should. > > 2. How to provide default values > > Supplying defaults is left to the caller of qemu_opt_get_FOO() by > design. > > Pre-creating option parameters deviates from that pattern. You > justify it by saying it "eliminates need to pepper code with > DEFAULT_RAM_SIZE * 1024 * 1024". How many occurrences? beside of vl.c for: mem & maxmem 1 in hw/i386/pc.c slots - 6 in several files see below for continuation: > > Drawback: you lose the ability to see whether the user gave a value. > See below. > [...] > >> Ugly. > >> > >> Why is the variable called 'end'? > > would be 'suffix' better? > > end points to the whole value string, not the end of anything, and > neither to a suffix of anything. Any suggestions? [...] > >> If you refrain from putting defaults into opts, you can distinguish the > >> cases "user didn't specify maxmem, so assume mem", and "user specified > >> maxmem, so check it's >= mem". > > So foar, there is no point in distinguishing above cases, > > since maxmem <= mem is invalid value and hotplug should be disabled. > > So setting default maxmem to mem or anything less effectively disables hotplug. > > Yes, setting maxmem < mem is invalid and should be rejected, but not > setting maxmem at all should be accepted even when you set mem. > > Your patch like this pseudo-code: > > mem = DEFAULT_RAM_SIZE * 1024 * 1024 > maxmem = mem > > if user specifies mem: > mem = user's mem > if user specifes max-mem: > mem = user's max-mem > > if max-mem < mem > what now? > should error our if max-mem and mem were specified by the user > shouldn't if user didn't specify max-mem! > but can't say whether he did > > I'd do it this way: > > mem = unset > maxmem = unset > > if user specifies mem: > mem = user's mem > if user specifes max-mem: > mem = user's max-mem > > if mem != unset && max-mem != unset && max-mem < mem > error > > I'd use QemuOpts for the user's command line, and no more. For anything > beyond that, I'd use ordinary variables, such as ram_size. Ok, I'll revert to the old code where options users check for option availability, it's not that much code. As for using QemuOpts as global store for global variables: * using local variables would require changing of machine init or/and QEMUMachine and changing functions signature pass them down the stack to consumers. * adding "slots" readonly property to i440fx & q35 for consumption in ACPI hotplug code and building ACPI tables. It would be essentially another global lookup for i440fx & q35 object and pulling "slots" property, which is much longer way/complex way to get global value. That's a lot of boilerplate code for the same outcome. * about setting default for "mem" value: if default "mem" is not set and no -m is provided on CLI, we get case where ram_size = foo & "mem" unset And if I recall correctly there was an effort to provide interface for currently used QemuOpts to external users. So "mem" would get inconsistent with what QEMU uses. To sum up above said: * I'd like to continue using QemuOpts as global constant value store, it saves from adding a lot of boilerplate-code that would do the same. Doing "git grep qemu_get_machine_opts" gets us several precedents that already use it that way. * I believe that setting default in QemuOpts for "mem" is a good thing that leads to consistent meaning of "mem" with what QEMU actually uses.
On Mon, 25 Nov 2013 13:51:03 +0100 Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 21/11/2013 03:38, Igor Mammedov ha scritto: > > Along with conversion extend -m option to support following parameters: > > "mem" - startup memory amount > > "slots" - total number of hotplug memory slots > > "maxmem" - maximum possible memory > > > > "slots" and "maxmem" should go in pair and "maxmem" should be greater > > than "mem" for memory hotplug to be usable. > > Strictly speaking, slots and maxmem should be added later, not in this > patch. Not a blocker, though. I've split patch taking in account most of Markus' comments and added extra error checks for option values. Amended patches are posted as reply to this thread. > Paolo > [...]
Igor Mammedov <imammedo@redhat.com> writes: > On Tue, 26 Nov 2013 15:49:05 +0100 > Markus Armbruster <armbru@redhat.com> wrote: > >> Igor Mammedov <imammedo@redhat.com> writes: >> >> > On Thu, 21 Nov 2013 11:12:43 +0100 >> > Markus Armbruster <armbru@redhat.com> wrote: >> > >> >> Igor Mammedov <imammedo@redhat.com> writes: >> >> > [...] >> Two separate issues here: >> >> 1. The "no qemu_mem_opts have been specified" case >> >> This is equivalent to "empty options". Therefore, the case can be >> eliminated by pre-creating empty options. No objection. >> >> The three existing merge_lists users don't do that. Perhaps they >> should. >> >> 2. How to provide default values >> >> Supplying defaults is left to the caller of qemu_opt_get_FOO() by >> design. >> >> Pre-creating option parameters deviates from that pattern. You >> justify it by saying it "eliminates need to pepper code with >> DEFAULT_RAM_SIZE * 1024 * 1024". How many occurrences? > beside of vl.c for: > mem & maxmem 1 in hw/i386/pc.c > slots - 6 in several files Could the common code be factored out the old-fashioned way? Precedence: qemu_get_machine_opts() encapsulates some QemuOpts-related details, so its many users don't have to deal with them. > see below for continuation: > >> >> Drawback: you lose the ability to see whether the user gave a value. >> See below. >> > [...] >> >> Ugly. >> >> >> >> Why is the variable called 'end'? >> > would be 'suffix' better? >> >> end points to the whole value string, not the end of anything, and >> neither to a suffix of anything. > Any suggestions? What about val? > [...] >> >> If you refrain from putting defaults into opts, you can distinguish the >> >> cases "user didn't specify maxmem, so assume mem", and "user specified >> >> maxmem, so check it's >= mem". >> > So foar, there is no point in distinguishing above cases, >> > since maxmem <= mem is invalid value and hotplug should be disabled. >> > So setting default maxmem to mem or anything less effectively >> > disables hotplug. >> >> Yes, setting maxmem < mem is invalid and should be rejected, but not >> setting maxmem at all should be accepted even when you set mem. >> >> Your patch like this pseudo-code: >> >> mem = DEFAULT_RAM_SIZE * 1024 * 1024 >> maxmem = mem >> >> if user specifies mem: >> mem = user's mem >> if user specifes max-mem: >> mem = user's max-mem >> >> if max-mem < mem >> what now? >> should error our if max-mem and mem were specified by the user >> shouldn't if user didn't specify max-mem! >> but can't say whether he did >> >> I'd do it this way: >> >> mem = unset >> maxmem = unset >> >> if user specifies mem: >> mem = user's mem >> if user specifes max-mem: >> mem = user's max-mem >> >> if mem != unset && max-mem != unset && max-mem < mem >> error >> >> I'd use QemuOpts for the user's command line, and no more. For anything >> beyond that, I'd use ordinary variables, such as ram_size. > Ok, I'll revert to the old code where options users check for option > availability, it's not that much code. > > > As for using QemuOpts as global store for global variables: > > * using local variables would require changing of machine init or/and > QEMUMachine and changing functions signature pass them down the stack to > consumers. Extending QEMUMachineInitArgs should suffice. Once you're inside the board code, passing stuff around as C parameters is probably an improvement over passing around QemuOpts. > * adding "slots" readonly property to i440fx & q35 for consumption in > ACPI hotplug code and building ACPI tables. It would be essentially another > global lookup for i440fx & q35 object and pulling "slots" property, > which is much longer way/complex way to get global value. That's a lot of > boilerplate code for the same outcome. Can't say without seeing the code. > * about setting default for "mem" value: if default "mem" is not set and > no -m is provided on CLI, we get case where > ram_size = foo & "mem" unset > And if I recall correctly there was an effort to provide interface for > currently used QemuOpts to external users. So "mem" would get inconsistent > with what QEMU uses. QemuOpts do not record what QEMU uses. They record what the user asked for. > To sum up above said: > * I'd like to continue using QemuOpts as global constant value store, it > saves from adding a lot of boilerplate-code that would do the same. Keeping the user's configuration just in QemuOpts is fine. What I don't like is messing with it there. This includes storing defaults. Here's another reason: -writeconfig should write out exactly the user's configuration. If you mess with it, it may write out messed up configuration, depending on *when* you mess with it. > Doing > "git grep qemu_get_machine_opts" > gets us several precedents that already use it that way. Note that it does *not* store defaults in QemuOpts, it only creates empty opts. I'm not sure that was a good idea. > * I believe that setting default in QemuOpts for "mem" is a good thing that > leads to consistent meaning of "mem" with what QEMU actually uses. I'm not sure I got this argument.
On Wed, 27 Nov 2013 15:35:09 +0100 Markus Armbruster <armbru@redhat.com> wrote: > Igor Mammedov <imammedo@redhat.com> writes: > > > On Tue, 26 Nov 2013 15:49:05 +0100 > > Markus Armbruster <armbru@redhat.com> wrote: > > > >> Igor Mammedov <imammedo@redhat.com> writes: > >> > >> > On Thu, 21 Nov 2013 11:12:43 +0100 > >> > Markus Armbruster <armbru@redhat.com> wrote: > >> > > >> >> Igor Mammedov <imammedo@redhat.com> writes: > >> >> > > [...] > >> Two separate issues here: > >> > >> 1. The "no qemu_mem_opts have been specified" case > >> > >> This is equivalent to "empty options". Therefore, the case can be > >> eliminated by pre-creating empty options. No objection. > >> > >> The three existing merge_lists users don't do that. Perhaps they > >> should. > >> > >> 2. How to provide default values > >> > >> Supplying defaults is left to the caller of qemu_opt_get_FOO() by > >> design. > >> > >> Pre-creating option parameters deviates from that pattern. You > >> justify it by saying it "eliminates need to pepper code with > >> DEFAULT_RAM_SIZE * 1024 * 1024". How many occurrences? > > beside of vl.c for: > > mem & maxmem 1 in hw/i386/pc.c > > slots - 6 in several files > > Could the common code be factored out the old-fashioned way? replacing one one-liner with another might help a little but won't change a thing in general. It will be essentially the same. > > Precedence: qemu_get_machine_opts() encapsulates some QemuOpts-related > details, so its many users don't have to deal with them. > > > see below for continuation: > > > >> > >> Drawback: you lose the ability to see whether the user gave a value. > >> See below. > >> > > [...] > >> >> Ugly. > >> >> > >> >> Why is the variable called 'end'? > >> > would be 'suffix' better? > >> > >> end points to the whole value string, not the end of anything, and > >> neither to a suffix of anything. > > Any suggestions? > > What about val? I've replaced it with "mem_str" see "[PATCH 04/28] vl: convert -m to QemuOpts" > > > [...] > >> >> If you refrain from putting defaults into opts, you can distinguish the > >> >> cases "user didn't specify maxmem, so assume mem", and "user specified > >> >> maxmem, so check it's >= mem". > >> > So foar, there is no point in distinguishing above cases, > >> > since maxmem <= mem is invalid value and hotplug should be disabled. > >> > So setting default maxmem to mem or anything less effectively > >> > disables hotplug. > >> > >> Yes, setting maxmem < mem is invalid and should be rejected, but not > >> setting maxmem at all should be accepted even when you set mem. > >> > >> Your patch like this pseudo-code: > >> > >> mem = DEFAULT_RAM_SIZE * 1024 * 1024 > >> maxmem = mem > >> > >> if user specifies mem: > >> mem = user's mem > >> if user specifes max-mem: > >> mem = user's max-mem > >> > >> if max-mem < mem > >> what now? > >> should error our if max-mem and mem were specified by the user > >> shouldn't if user didn't specify max-mem! > >> but can't say whether he did > >> > >> I'd do it this way: > >> > >> mem = unset > >> maxmem = unset > >> > >> if user specifies mem: > >> mem = user's mem > >> if user specifes max-mem: > >> mem = user's max-mem > >> > >> if mem != unset && max-mem != unset && max-mem < mem > >> error > >> > >> I'd use QemuOpts for the user's command line, and no more. For anything > >> beyond that, I'd use ordinary variables, such as ram_size. > > Ok, I'll revert to the old code where options users check for option > > availability, it's not that much code. > > > > > > As for using QemuOpts as global store for global variables: > > > > * using local variables would require changing of machine init or/and > > QEMUMachine and changing functions signature pass them down the stack to > > consumers. > > Extending QEMUMachineInitArgs should suffice. Once you're inside the > board code, passing stuff around as C parameters is probably an > improvement over passing around QemuOpts. > > > * adding "slots" readonly property to i440fx & q35 for consumption in > > ACPI hotplug code and building ACPI tables. It would be essentially another > > global lookup for i440fx & q35 object and pulling "slots" property, > > which is much longer way/complex way to get global value. That's a lot of > > boilerplate code for the same outcome. > > Can't say without seeing the code. > > > * about setting default for "mem" value: if default "mem" is not set and > > no -m is provided on CLI, we get case where > > ram_size = foo & "mem" unset > > And if I recall correctly there was an effort to provide interface for > > currently used QemuOpts to external users. So "mem" would get inconsistent > > with what QEMU uses. > > QemuOpts do not record what QEMU uses. They record what the user asked > for. > > > To sum up above said: > > * I'd like to continue using QemuOpts as global constant value store, it > > saves from adding a lot of boilerplate-code that would do the same. > > Keeping the user's configuration just in QemuOpts is fine. What I don't > like is messing with it there. This includes storing defaults. > > Here's another reason: -writeconfig should write out exactly the user's > configuration. If you mess with it, it may write out messed up > configuration, depending on *when* you mess with it. > > > Doing > > "git grep qemu_get_machine_opts" > > gets us several precedents that already use it that way. > > Note that it does *not* store defaults in QemuOpts, it only creates > empty opts. I'm not sure that was a good idea. I've dropped completely defaults setting in QemuOpts please see: "[PATCH 04/28] vl: convert -m to QemuOpts" "[PATCH 05/28] vl.c: extend -m option to support options for memory hotplug" As for ">it only creates empty opts." I'm confused. qemu_opt_get(qemu_get_machine_opts(), "foo") pattern showed by grep is the same as I use to get "slots/maxmem": exec.c: if (!qemu_opt_get_bool(qemu_get_machine_opts(), "mem-merge", true)) { hw/arm/boot.c: info->dtb_filename = qemu_opt_get(qemu_get_machine_opts(), "dtb"); hw/ppc/spapr.c: const char *drivename = qemu_opt_get(qemu_get_machine_opts(), "nvram"); hw/ppc/virtex_ml507.c: dtb_filename = qemu_opt_get(qemu_get_machine_opts(), "dtb"); include/sysemu/sysemu.h:QemuOpts *qemu_get_machine_opts(void); kvm-all.c: if (!qemu_opt_get_bool(qemu_get_machine_opts(), "kernel_irqchip", true) || target-i386/kvm.c: shadow_mem = qemu_opt_get_size(qemu_get_machine_opts(), probably it is there because passing them as C parameters is more intrusive than just using user supplied values directly. > > * I believe that setting default in QemuOpts for "mem" is a good thing that > > leads to consistent meaning of "mem" with what QEMU actually uses. > > I'm not sure I got this argument. I can easily drop this hunk from "[PATCH 04/28] vl: convert -m to QemuOpts", I've posted tonight as reply to this thread, since ram_size is already passed to machine_init(), it's not worth arguing.
Igor Mammedov <imammedo@redhat.com> writes: > On Wed, 27 Nov 2013 15:35:09 +0100 > Markus Armbruster <armbru@redhat.com> wrote: > >> Igor Mammedov <imammedo@redhat.com> writes: >> >> > On Tue, 26 Nov 2013 15:49:05 +0100 >> > Markus Armbruster <armbru@redhat.com> wrote: >> > >> >> Igor Mammedov <imammedo@redhat.com> writes: >> >> >> >> > On Thu, 21 Nov 2013 11:12:43 +0100 >> >> > Markus Armbruster <armbru@redhat.com> wrote: >> >> > >> >> >> Igor Mammedov <imammedo@redhat.com> writes: >> >> >> >> > [...] >> >> Two separate issues here: >> >> >> >> 1. The "no qemu_mem_opts have been specified" case >> >> >> >> This is equivalent to "empty options". Therefore, the case can be >> >> eliminated by pre-creating empty options. No objection. >> >> >> >> The three existing merge_lists users don't do that. Perhaps they >> >> should. >> >> >> >> 2. How to provide default values >> >> >> >> Supplying defaults is left to the caller of qemu_opt_get_FOO() by >> >> design. >> >> >> >> Pre-creating option parameters deviates from that pattern. You >> >> justify it by saying it "eliminates need to pepper code with >> >> DEFAULT_RAM_SIZE * 1024 * 1024". How many occurrences? >> > beside of vl.c for: >> > mem & maxmem 1 in hw/i386/pc.c >> > slots - 6 in several files >> >> Could the common code be factored out the old-fashioned way? > replacing one one-liner with another might help a little but > won't change a thing in general. It will be essentially the same. Need to review your latest to have an opinion here :) >> Precedence: qemu_get_machine_opts() encapsulates some QemuOpts-related >> details, so its many users don't have to deal with them. >> >> > see below for continuation: >> > >> >> >> >> Drawback: you lose the ability to see whether the user gave a value. >> >> See below. >> >> >> > [...] >> >> >> Ugly. >> >> >> >> >> >> Why is the variable called 'end'? >> >> > would be 'suffix' better? >> >> >> >> end points to the whole value string, not the end of anything, and >> >> neither to a suffix of anything. >> > Any suggestions? >> >> What about val? > I've replaced it with "mem_str" see > "[PATCH 04/28] vl: convert -m to QemuOpts" Works for me. >> > [...] >> >> >> If you refrain from putting defaults into opts, you can distinguish the >> >> >> cases "user didn't specify maxmem, so assume mem", and "user specified >> >> >> maxmem, so check it's >= mem". >> >> > So foar, there is no point in distinguishing above cases, >> >> > since maxmem <= mem is invalid value and hotplug should be disabled. >> >> > So setting default maxmem to mem or anything less effectively >> >> > disables hotplug. >> >> >> >> Yes, setting maxmem < mem is invalid and should be rejected, but not >> >> setting maxmem at all should be accepted even when you set mem. >> >> >> >> Your patch like this pseudo-code: >> >> >> >> mem = DEFAULT_RAM_SIZE * 1024 * 1024 >> >> maxmem = mem >> >> >> >> if user specifies mem: >> >> mem = user's mem >> >> if user specifes max-mem: >> >> mem = user's max-mem >> >> >> >> if max-mem < mem >> >> what now? >> >> should error our if max-mem and mem were specified by the user >> >> shouldn't if user didn't specify max-mem! >> >> but can't say whether he did >> >> >> >> I'd do it this way: >> >> >> >> mem = unset >> >> maxmem = unset >> >> >> >> if user specifies mem: >> >> mem = user's mem >> >> if user specifes max-mem: >> >> mem = user's max-mem >> >> >> >> if mem != unset && max-mem != unset && max-mem < mem >> >> error >> >> >> >> I'd use QemuOpts for the user's command line, and no more. For anything >> >> beyond that, I'd use ordinary variables, such as ram_size. >> > Ok, I'll revert to the old code where options users check for option >> > availability, it's not that much code. >> > >> > >> > As for using QemuOpts as global store for global variables: >> > >> > * using local variables would require changing of machine init or/and >> > QEMUMachine and changing functions signature pass them down the stack to >> > consumers. >> >> Extending QEMUMachineInitArgs should suffice. Once you're inside the >> board code, passing stuff around as C parameters is probably an >> improvement over passing around QemuOpts. >> >> > * adding "slots" readonly property to i440fx & q35 for consumption in >> > ACPI hotplug code and building ACPI tables. It would be >> > essentially another >> > global lookup for i440fx & q35 object and pulling "slots" property, >> > which is much longer way/complex way to get global value. That's a lot of >> > boilerplate code for the same outcome. >> >> Can't say without seeing the code. >> >> > * about setting default for "mem" value: if default "mem" is not set and >> > no -m is provided on CLI, we get case where >> > ram_size = foo & "mem" unset >> > And if I recall correctly there was an effort to provide interface for >> > currently used QemuOpts to external users. So "mem" would get inconsistent >> > with what QEMU uses. >> >> QemuOpts do not record what QEMU uses. They record what the user asked >> for. >> >> > To sum up above said: >> > * I'd like to continue using QemuOpts as global constant value store, it >> > saves from adding a lot of boilerplate-code that would do the same. >> >> Keeping the user's configuration just in QemuOpts is fine. What I don't >> like is messing with it there. This includes storing defaults. >> >> Here's another reason: -writeconfig should write out exactly the user's >> configuration. If you mess with it, it may write out messed up >> configuration, depending on *when* you mess with it. >> >> > Doing >> > "git grep qemu_get_machine_opts" >> > gets us several precedents that already use it that way. >> >> Note that it does *not* store defaults in QemuOpts, it only creates >> empty opts. I'm not sure that was a good idea. > I've dropped completely defaults setting in QemuOpts please see: > "[PATCH 04/28] vl: convert -m to QemuOpts" > "[PATCH 05/28] vl.c: extend -m option to support options for memory hotplug" > > As for ">it only creates empty opts." I'm confused. > qemu_opt_get(qemu_get_machine_opts(), "foo") pattern showed by grep is > the same > as I use to get "slots/maxmem": > > exec.c: if (!qemu_opt_get_bool(qemu_get_machine_opts(), "mem-merge", true)) { > hw/arm/boot.c: info->dtb_filename = > qemu_opt_get(qemu_get_machine_opts(), "dtb"); > hw/ppc/spapr.c: const char *drivename = > qemu_opt_get(qemu_get_machine_opts(), "nvram"); > hw/ppc/virtex_ml507.c: dtb_filename = > qemu_opt_get(qemu_get_machine_opts(), "dtb"); > include/sysemu/sysemu.h:QemuOpts *qemu_get_machine_opts(void); > kvm-all.c: if (!qemu_opt_get_bool(qemu_get_machine_opts(), > "kernel_irqchip", true) || > target-i386/kvm.c: shadow_mem = qemu_opt_get_size(qemu_get_machine_opts(), > > probably it is there because passing them as C parameters is more > intrusive than > just using user supplied values directly. qemu_get_machine_opts() is precedent for factoring out common opts-querying code. It has one aspect that may have been a bad idea: it creates empty machine opts when the user didn't specify any option creating machine opts. Hope this unconfuses you. If not, I guess we should just move on to discussing your revised patch :) >> > * I believe that setting default in QemuOpts for "mem" is a good thing that >> > leads to consistent meaning of "mem" with what QEMU actually uses. >> >> I'm not sure I got this argument. > I can easily drop this hunk from "[PATCH 04/28] vl: convert -m to QemuOpts", > I've posted tonight as reply to this thread, > since ram_size is already passed to machine_init(), it's not worth arguing. Okay.
diff --git a/qemu-options.hx b/qemu-options.hx index 8b94264..fe4559b 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -210,8 +210,13 @@ use is discouraged as it may be removed from future versions. ETEXI DEF("m", HAS_ARG, QEMU_OPTION_m, - "-m megs set virtual RAM size to megs MB [default=" - stringify(DEFAULT_RAM_SIZE) "]\n", QEMU_ARCH_ALL) + "-m [mem=]megs[,slots=n,maxmem=size]\n" + " set virtual RAM size to megs MB [default=" + stringify(DEFAULT_RAM_SIZE) "]\n" + " mem=start-up memory amount\n" + " slots=maximum number of hotplug slots\n" + " maxmem=maximum total amount of memory\n", + QEMU_ARCH_ALL) STEXI @item -m @var{megs} @findex -m diff --git a/vl.c b/vl.c index f28674f..5974f0f 100644 --- a/vl.c +++ b/vl.c @@ -529,6 +529,28 @@ static QemuOptsList qemu_msg_opts = { }, }; +static QemuOptsList qemu_mem_opts = { + .name = "memory-opts", + .implied_opt_name = "mem", + .head = QTAILQ_HEAD_INITIALIZER(qemu_mem_opts.head), + .merge_lists = true, + .desc = { + { + .name = "mem", + .type = QEMU_OPT_SIZE, + }, + { + .name = "slots", + .type = QEMU_OPT_NUMBER, + }, + { + .name = "maxmem", + .type = QEMU_OPT_SIZE, + }, + { /* end of list */ } + }, +}; + /** * Get machine options * @@ -2816,6 +2838,14 @@ static int object_create(QemuOpts *opts, void *opaque) return 0; } +static void qemu_init_default_mem_opts(uint64_t size) +{ + QemuOpts *opts = qemu_opts_create_nofail(&qemu_mem_opts); + qemu_opt_set_number(opts, "mem", size); + qemu_opt_set_number(opts, "maxmem", size); + qemu_opt_set_number(opts, "slots", 0); +} + int main(int argc, char **argv, char **envp) { int i; @@ -2887,6 +2917,7 @@ int main(int argc, char **argv, char **envp) qemu_add_opts(&qemu_tpmdev_opts); qemu_add_opts(&qemu_realtime_opts); qemu_add_opts(&qemu_msg_opts); + qemu_add_opts(&qemu_mem_opts); runstate_init(); @@ -2901,7 +2932,8 @@ int main(int argc, char **argv, char **envp) module_call_init(MODULE_INIT_MACHINE); machine = find_default_machine(); cpu_model = NULL; - ram_size = 0; + ram_size = DEFAULT_RAM_SIZE * 1024 * 1024; + qemu_init_default_mem_opts(ram_size); snapshot = 0; cyls = heads = secs = 0; translation = BIOS_ATA_TRANSLATION_AUTO; @@ -3178,21 +3210,43 @@ int main(int argc, char **argv, char **envp) exit(0); break; case QEMU_OPTION_m: { - int64_t value; uint64_t sz; - char *end; + const char *end; + char *s; - value = strtosz(optarg, &end); - if (value < 0 || *end) { - fprintf(stderr, "qemu: invalid ram size: %s\n", optarg); + opts = qemu_opts_parse(qemu_find_opts("memory-opts"), + optarg, 1); + if (!opts) { exit(1); } - sz = QEMU_ALIGN_UP((uint64_t)value, 8192); + + /* fixup legacy sugffix-less format */ + end = qemu_opt_get(opts, "mem"); + if (g_ascii_isdigit(end[strlen(end) - 1])) { + s = g_strconcat(end, "M", NULL); + qemu_opt_set(opts, "mem", s); + g_free(s); + } + + sz = QEMU_ALIGN_UP(qemu_opt_get_size(opts, "mem", ram_size), + 8192); + /* compatibility behaviour for case "-m 0" */ + if (sz == 0) { + sz = DEFAULT_RAM_SIZE * 1024 * 1024; + } + ram_size = sz; if (ram_size != sz) { fprintf(stderr, "qemu: ram size too large\n"); exit(1); } + /* store aligned value for future use */ + qemu_opt_set_number(opts, "mem", ram_size); + + sz = qemu_opt_get_size(opts, "maxmem", ram_size); + if (sz < ram_size) { + qemu_opt_set_number(opts, "maxmem", ram_size); + } break; } #ifdef CONFIG_TPM @@ -4029,11 +4083,6 @@ int main(int argc, char **argv, char **envp) exit(1); } - /* init the memory */ - if (ram_size == 0) { - ram_size = DEFAULT_RAM_SIZE * 1024 * 1024; - } - if (qemu_opts_foreach(qemu_find_opts("device"), device_help_func, NULL, 0) != 0) { exit(0);
Along with conversion extend -m option to support following parameters: "mem" - startup memory amount "slots" - total number of hotplug memory slots "maxmem" - maximum possible memory "slots" and "maxmem" should go in pair and "maxmem" should be greater than "mem" for memory hotplug to be usable. v2: make sure maxmem is not less than ram size Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- qemu-options.hx | 9 +++++- vl.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++--------- 2 files changed, 68 insertions(+), 14 deletions(-)