| Message ID | 20201123141435.2726558-14-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | cleanup qemu_init and make sense of command line processing | expand |
On Mon, 23 Nov 2020 09:14:12 -0500 Paolo Bonzini <pbonzini@redhat.com> wrote: > Move more sane parts of the huge qemu_init function out of it. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > softmmu/vl.c | 12 +++++------- > 1 file changed, 5 insertions(+), 7 deletions(-) > > diff --git a/softmmu/vl.c b/softmmu/vl.c > index ab08a0290c..5d68cf828c 100644 > --- a/softmmu/vl.c > +++ b/softmmu/vl.c > @@ -3070,6 +3070,11 @@ static void qemu_init_board(void) > { > MachineClass *machine_class = MACHINE_GET_CLASS(current_machine); > > + if (semihosting_enabled() && !semihosting_get_argc() && current_machine->kernel_filename) { > + /* fall back to the -kernel/-append */ > + semihosting_arg_fallback(current_machine->kernel_filename, current_machine->kernel_cmdline); > + } it doesn't seem to depend on anything that warrants calling it this late. > if (machine_class->default_ram_id && current_machine->ram_size && > numa_uses_legacy_mem() && !current_machine->ram_memdev_id) { > create_default_memdev(current_machine, mem_path); > @@ -4385,13 +4390,6 @@ void qemu_init(int argc, char **argv, char **envp) > boot_order = machine_class->default_boot_order; > } > > - if (semihosting_enabled() && !semihosting_get_argc()) { > - const char *kernel_filename = qemu_opt_get(machine_opts, "kernel"); > - const char *kernel_cmdline = qemu_opt_get(machine_opts, "append"); > - /* fall back to the -kernel/-append */ > - semihosting_arg_fallback(kernel_filename, kernel_cmdline); > - } Can we move this hunk as is to somewhere around qemu_maybe_daemonize() time? > if (net_init_clients(&err) < 0) { > error_report_err(err); > exit(1);
On 26/11/20 18:10, Igor Mammedov wrote: > On Mon, 23 Nov 2020 09:14:12 -0500 > Paolo Bonzini <pbonzini@redhat.com> wrote: > >> Move more sane parts of the huge qemu_init function out of it. >> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> softmmu/vl.c | 12 +++++------- >> 1 file changed, 5 insertions(+), 7 deletions(-) >> >> diff --git a/softmmu/vl.c b/softmmu/vl.c >> index ab08a0290c..5d68cf828c 100644 >> --- a/softmmu/vl.c >> +++ b/softmmu/vl.c >> @@ -3070,6 +3070,11 @@ static void qemu_init_board(void) >> { >> MachineClass *machine_class = MACHINE_GET_CLASS(current_machine); >> >> + if (semihosting_enabled() && !semihosting_get_argc() && current_machine->kernel_filename) { >> + /* fall back to the -kernel/-append */ >> + semihosting_arg_fallback(current_machine->kernel_filename, current_machine->kernel_cmdline); >> + } > > it doesn't seem to depend on anything that warrants calling it this late. Yes, calling it around machine initialization time is also a possibility. I just wanted to get rid of it in code that I'm actually looking at. :) Paolo > >> if (machine_class->default_ram_id && current_machine->ram_size && >> numa_uses_legacy_mem() && !current_machine->ram_memdev_id) { >> create_default_memdev(current_machine, mem_path); >> @@ -4385,13 +4390,6 @@ void qemu_init(int argc, char **argv, char **envp) >> boot_order = machine_class->default_boot_order; >> } >> >> - if (semihosting_enabled() && !semihosting_get_argc()) { >> - const char *kernel_filename = qemu_opt_get(machine_opts, "kernel"); >> - const char *kernel_cmdline = qemu_opt_get(machine_opts, "append"); >> - /* fall back to the -kernel/-append */ >> - semihosting_arg_fallback(kernel_filename, kernel_cmdline); >> - } > > Can we move this hunk as is to somewhere around qemu_maybe_daemonize() time? > > >> if (net_init_clients(&err) < 0) { >> error_report_err(err); >> exit(1); >
On Fri, 27 Nov 2020 06:03:43 +0100 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 26/11/20 18:10, Igor Mammedov wrote: > > On Mon, 23 Nov 2020 09:14:12 -0500 > > Paolo Bonzini <pbonzini@redhat.com> wrote: > > > >> Move more sane parts of the huge qemu_init function out of it. > >> > >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > >> --- > >> softmmu/vl.c | 12 +++++------- > >> 1 file changed, 5 insertions(+), 7 deletions(-) > >> > >> diff --git a/softmmu/vl.c b/softmmu/vl.c > >> index ab08a0290c..5d68cf828c 100644 > >> --- a/softmmu/vl.c > >> +++ b/softmmu/vl.c > >> @@ -3070,6 +3070,11 @@ static void qemu_init_board(void) > >> { > >> MachineClass *machine_class = MACHINE_GET_CLASS(current_machine); > >> > >> + if (semihosting_enabled() && !semihosting_get_argc() && current_machine->kernel_filename) { > >> + /* fall back to the -kernel/-append */ > >> + semihosting_arg_fallback(current_machine->kernel_filename, current_machine->kernel_cmdline); > >> + } > > > > it doesn't seem to depend on anything that warrants calling it this late. > > Yes, calling it around machine initialization time is also a > possibility. I just wanted to get rid of it in code that I'm actually > looking at. :) I'd prefer it being moved close to CLI parsing, in a place where other _early call go. We probably want qemu_init_board() being clear of not really needed clutter. > > Paolo > > > > >> if (machine_class->default_ram_id && current_machine->ram_size && > >> numa_uses_legacy_mem() && !current_machine->ram_memdev_id) { > >> create_default_memdev(current_machine, mem_path); > >> @@ -4385,13 +4390,6 @@ void qemu_init(int argc, char **argv, char **envp) > >> boot_order = machine_class->default_boot_order; > >> } > >> > >> - if (semihosting_enabled() && !semihosting_get_argc()) { > >> - const char *kernel_filename = qemu_opt_get(machine_opts, "kernel"); > >> - const char *kernel_cmdline = qemu_opt_get(machine_opts, "append"); > >> - /* fall back to the -kernel/-append */ > >> - semihosting_arg_fallback(kernel_filename, kernel_cmdline); > >> - } > > > > Can we move this hunk as is to somewhere around qemu_maybe_daemonize() time? > > > > > >> if (net_init_clients(&err) < 0) { > >> error_report_err(err); > >> exit(1); > > >
On 27/11/20 11:31, Igor Mammedov wrote: >> Yes, calling it around machine initialization time is also a >> possibility. I just wanted to get rid of it in code that I'm actually >> looking at.:) > I'd prefer it being moved close to CLI parsing, > in a place where other _early call go. > > We probably want qemu_init_board() being clear of not really needed clutter. > Fair enough, I'd put -semihosting-config in the same bucket as -m/-boot/-smp (machine configuration that isn't in -M) so I'll move it together with them. Paolo
On Fri, 27 Nov 2020 12:22:51 +0100 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 27/11/20 11:31, Igor Mammedov wrote: > >> Yes, calling it around machine initialization time is also a > >> possibility. I just wanted to get rid of it in code that I'm actually > >> looking at.:) > > I'd prefer it being moved close to CLI parsing, > > in a place where other _early call go. > > > > We probably want qemu_init_board() being clear of not really needed clutter. > > > > Fair enough, I'd put -semihosting-config in the same bucket as > -m/-boot/-smp (machine configuration that isn't in -M) so I'll move it > together with them. it might be machine code, but I didn't see anything that depends on machine in there that's why I've suggested to move it right after we parse CLI options (i.e. as early as possible) to keep the rest of code cleaner > > Paolo >
On 27/11/20 13:12, Igor Mammedov wrote: >> Fair enough, I'd put -semihosting-config in the same bucket as >> -m/-boot/-smp (machine configuration that isn't in -M) so I'll move it >> together with them. > it might be machine code, but I didn't see anything that depends > on machine in there that's why I've suggested to move it right after > we parse CLI options (i.e. as early as possible) to keep the rest of > code cleaner Yes, if I just remove this patch the fallback will end up in qemu_apply_machine_options. That seems to be the right place. Paolo
diff --git a/softmmu/vl.c b/softmmu/vl.c index ab08a0290c..5d68cf828c 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -3070,6 +3070,11 @@ static void qemu_init_board(void) { MachineClass *machine_class = MACHINE_GET_CLASS(current_machine); + if (semihosting_enabled() && !semihosting_get_argc() && current_machine->kernel_filename) { + /* fall back to the -kernel/-append */ + semihosting_arg_fallback(current_machine->kernel_filename, current_machine->kernel_cmdline); + } + if (machine_class->default_ram_id && current_machine->ram_size && numa_uses_legacy_mem() && !current_machine->ram_memdev_id) { create_default_memdev(current_machine, mem_path); @@ -4385,13 +4390,6 @@ void qemu_init(int argc, char **argv, char **envp) boot_order = machine_class->default_boot_order; } - if (semihosting_enabled() && !semihosting_get_argc()) { - const char *kernel_filename = qemu_opt_get(machine_opts, "kernel"); - const char *kernel_cmdline = qemu_opt_get(machine_opts, "append"); - /* fall back to the -kernel/-append */ - semihosting_arg_fallback(kernel_filename, kernel_cmdline); - } - if (net_init_clients(&err) < 0) { error_report_err(err); exit(1);
Move more sane parts of the huge qemu_init function out of it. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- softmmu/vl.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)