| Message ID | 20201027182144.3315885-22-pbonzini@redhat.com |
|---|---|
| State | Accepted |
| Commit | b24986e7845594f4ce394403d2b5c15e89ce04f8 |
| Headers | show |
| Series | cleanup qemu_init and make sense of command line processing | expand |
On Tue, 27 Oct 2020 14:21:36 -0400 Paolo Bonzini <pbonzini@redhat.com> wrote: > This is a bit nasty: the machine is storing a string and later > resolving it. We probably want to remove the memdev property > and instead make this a memory-set command. "-M memdev" can be > handled as a legacy option that is special cased by > machine_set_property. I'd treat this description as topic starter and drop it from this patch in v2. with that, Reviewed-by: Igor Mammedov <imammedo@redhat.com> how memory-set would help or be any better than memdev? memdev should be a link property, but due to RAM allocation being dependent on used accel we can't create actual backend until accelerator is initialized (i.e. after machine options parsed, which forced me to make memdev a string that refers to a backend created later). If we can make RAM allocation independent from used accelerator (if I recall right it has TCG dependency) and if we split -m CLI handling and default ram_memdev_id (which is implicit CLI), then we can make -M memdev a link and move RAM backends to qemu_create_early_backends() time. Which in context of creating machine via QMP would imply that link should be set explicitly via QMP after backend is created. Flow could look like this: CLI part: -m / defaults - preps and 'if not NUMA' executes QMP object_add memory-backend-foo,size=X,id=(ram_memdev_id) in case -M memory-backend is not set explicitly, or fetch id of explicitly provided backend (which would be created by qemu_create_early_backends) QMP part: object_add machine_foo,id=my_machine set (link) my_machine.memory-backend=(ram_memdev_id) that way we do no need to create a separate memory-set command, we can handle it as a normal property, where all compat stuff is kept in CLI part. For CLI part handling there are 2 caveats: * Xen doesn't use memdev at all, it allocates memory region directly. Not sure what we should do in this case (may be we can create a separate xen-ram backend for it and remove 'mr == &ram_memory' in xen_ram_alloc() hack) * legacy S390 machine types (<5.0) fixup ram_size before creating backend, if user provided value is not correct 5c30ef937f5 (i.e. not aligned properly). I guess we are stuck with it, given it's version-ed machine type. The rest of the code was fixed to error-out in the case board doesn't like -m value. Or we can treat it as user error (which should be corrected by user) and deprecate/remove fixup. > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > softmmu/vl.c | 70 +++++++++++++++++++++++++++------------------------- > 1 file changed, 37 insertions(+), 33 deletions(-) > > diff --git a/softmmu/vl.c b/softmmu/vl.c > index 9a3c92387e..1485aba8be 100644 > --- a/softmmu/vl.c > +++ b/softmmu/vl.c > @@ -2834,6 +2834,42 @@ static bool have_custom_ram_size(void) > return !!qemu_opt_get(opts, "size"); > } > > +static void qemu_resolve_machine_memdev(void) > +{ > + if (current_machine->ram_memdev_id) { > + Object *backend; > + ram_addr_t backend_size; > + > + backend = object_resolve_path_type(current_machine->ram_memdev_id, > + TYPE_MEMORY_BACKEND, NULL); > + if (!backend) { > + error_report("Memory backend '%s' not found", > + current_machine->ram_memdev_id); > + exit(EXIT_FAILURE); > + } > + backend_size = object_property_get_uint(backend, "size", &error_abort); > + if (have_custom_ram_size() && backend_size != ram_size) { > + error_report("Size specified by -m option must match size of " > + "explicitly specified 'memory-backend' property"); > + exit(EXIT_FAILURE); > + } > + if (mem_path) { > + error_report("'-mem-path' can't be used together with" > + "'-machine memory-backend'"); > + exit(EXIT_FAILURE); > + } > + ram_size = backend_size; > + } > + > + if (!xen_enabled()) { > + /* On 32-bit hosts, QEMU is limited by virtual address space */ > + if (ram_size > (2047 << 20) && HOST_LONG_BITS == 32) { > + error_report("at most 2047 MB RAM can be simulated"); > + exit(1); > + } > + } > +} > + > static void set_memory_options(MachineClass *mc) > { > uint64_t sz; > @@ -4476,39 +4512,7 @@ void qemu_init(int argc, char **argv, char **envp) > current_machine->cpu_type = parse_cpu_option(cpu_option); > } > > - if (current_machine->ram_memdev_id) { > - Object *backend; > - ram_addr_t backend_size; > - > - backend = object_resolve_path_type(current_machine->ram_memdev_id, > - TYPE_MEMORY_BACKEND, NULL); > - if (!backend) { > - error_report("Memory backend '%s' not found", > - current_machine->ram_memdev_id); > - exit(EXIT_FAILURE); > - } > - backend_size = object_property_get_uint(backend, "size", &error_abort); > - if (have_custom_ram_size() && backend_size != ram_size) { > - error_report("Size specified by -m option must match size of " > - "explicitly specified 'memory-backend' property"); > - exit(EXIT_FAILURE); > - } > - if (mem_path) { > - error_report("'-mem-path' can't be used together with" > - "'-machine memory-backend'"); > - exit(EXIT_FAILURE); > - } > - ram_size = backend_size; > - } > - > - if (!xen_enabled()) { > - /* On 32-bit hosts, QEMU is limited by virtual address space */ > - if (ram_size > (2047 << 20) && HOST_LONG_BITS == 32) { > - error_report("at most 2047 MB RAM can be simulated"); > - exit(1); > - } > - } > - > + qemu_resolve_machine_memdev(); > parse_numa_opts(current_machine); > > /* do monitor/qmp handling at preconfig state if requested */
diff --git a/softmmu/vl.c b/softmmu/vl.c index 9a3c92387e..1485aba8be 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -2834,6 +2834,42 @@ static bool have_custom_ram_size(void) return !!qemu_opt_get(opts, "size"); } +static void qemu_resolve_machine_memdev(void) +{ + if (current_machine->ram_memdev_id) { + Object *backend; + ram_addr_t backend_size; + + backend = object_resolve_path_type(current_machine->ram_memdev_id, + TYPE_MEMORY_BACKEND, NULL); + if (!backend) { + error_report("Memory backend '%s' not found", + current_machine->ram_memdev_id); + exit(EXIT_FAILURE); + } + backend_size = object_property_get_uint(backend, "size", &error_abort); + if (have_custom_ram_size() && backend_size != ram_size) { + error_report("Size specified by -m option must match size of " + "explicitly specified 'memory-backend' property"); + exit(EXIT_FAILURE); + } + if (mem_path) { + error_report("'-mem-path' can't be used together with" + "'-machine memory-backend'"); + exit(EXIT_FAILURE); + } + ram_size = backend_size; + } + + if (!xen_enabled()) { + /* On 32-bit hosts, QEMU is limited by virtual address space */ + if (ram_size > (2047 << 20) && HOST_LONG_BITS == 32) { + error_report("at most 2047 MB RAM can be simulated"); + exit(1); + } + } +} + static void set_memory_options(MachineClass *mc) { uint64_t sz; @@ -4476,39 +4512,7 @@ void qemu_init(int argc, char **argv, char **envp) current_machine->cpu_type = parse_cpu_option(cpu_option); } - if (current_machine->ram_memdev_id) { - Object *backend; - ram_addr_t backend_size; - - backend = object_resolve_path_type(current_machine->ram_memdev_id, - TYPE_MEMORY_BACKEND, NULL); - if (!backend) { - error_report("Memory backend '%s' not found", - current_machine->ram_memdev_id); - exit(EXIT_FAILURE); - } - backend_size = object_property_get_uint(backend, "size", &error_abort); - if (have_custom_ram_size() && backend_size != ram_size) { - error_report("Size specified by -m option must match size of " - "explicitly specified 'memory-backend' property"); - exit(EXIT_FAILURE); - } - if (mem_path) { - error_report("'-mem-path' can't be used together with" - "'-machine memory-backend'"); - exit(EXIT_FAILURE); - } - ram_size = backend_size; - } - - if (!xen_enabled()) { - /* On 32-bit hosts, QEMU is limited by virtual address space */ - if (ram_size > (2047 << 20) && HOST_LONG_BITS == 32) { - error_report("at most 2047 MB RAM can be simulated"); - exit(1); - } - } - + qemu_resolve_machine_memdev(); parse_numa_opts(current_machine); /* do monitor/qmp handling at preconfig state if requested */
This is a bit nasty: the machine is storing a string and later resolving it. We probably want to remove the memdev property and instead make this a memory-set command. "-M memdev" can be handled as a legacy option that is special cased by machine_set_property. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- softmmu/vl.c | 70 +++++++++++++++++++++++++++------------------------- 1 file changed, 37 insertions(+), 33 deletions(-)