Igor Mammedov <imamm...@redhat.com> writes: > On Thu, 21 Nov 2013 11:12:43 +0100 > Markus Armbruster <arm...@redhat.com> wrote: > >> Igor Mammedov <imamm...@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 <imamm...@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!