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) > 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);