Re: [Qemu-devel] [RFC 4/6] CLI: add -paused option

[David Gibson]

On Mon, Oct 16, 2017 at 02:59:16PM -0200, Eduardo Habkost wrote:
> On Mon, Oct 16, 2017 at 06:22:54PM +0200, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> >  include/sysemu/sysemu.h |  1 +
> >  qemu-options.hx         | 15 ++++++++++++++
> >  qmp.c                   |  5 +++++
> >  vl.c                    | 54 
> > ++++++++++++++++++++++++++++++++++++++++++++++++-
> >  4 files changed, 74 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> > index b213696..3feb94f 100644
> > --- a/include/sysemu/sysemu.h
> > +++ b/include/sysemu/sysemu.h
> > @@ -66,6 +66,7 @@ typedef enum WakeupReason {
> >      QEMU_WAKEUP_REASON_OTHER,
> >  } WakeupReason;
> >  
> > +void qemu_exit_preconfig_request(void);
> >  void qemu_system_reset_request(ShutdownCause reason);
> >  void qemu_system_suspend_request(void);
> >  void qemu_register_suspend_notifier(Notifier *notifier);
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 39225ae..bd44db8 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -3498,6 +3498,21 @@ STEXI
> >  Run the emulation in single step mode.
> >  ETEXI
> >  
> > +DEF("paused", HAS_ARG, QEMU_OPTION_paused, \
> > +    "-paused [state=]postconf|preconf\n"
> > +    "                postconf: pause QEMU after machine is initialized\n"
> > +    "                preconf: pause QEMU before machine is initialized\n",
> > +    QEMU_ARCH_ALL)
> 
> I would like to allow pausing before machine-type is selected, so
> management could run query-machines before choosing a
> machine-type.  Would that need a third "-pause" mode, or will we
> be able to change "preconf" to pause before select_machine() is
> called?
> 
> The same probably applies to other things initialized before
> machine_run_board_init() that could be configurable using QMP,
> including but not limited to:
> * Accelerator configuration
> * Registering global properties
> * RAM size
> * SMP/CPU configuration

Yeah.. having a bunch of different possible pause stages to select
doesn't sound great.  Could we avoid this by instead changing -S to
pause at the earliest possible spot, but having any monitor commands
that require a later stage automatically "fast forwarding" to the
right phase?

> 
> 
> > +STEXI
> > address@hidden -paused
> > address@hidden -paused
> > +if set enabled interactive configuration stages before machine emulation 
> > starts.
> > +'postconf' option value mimics -S option behaviour where machine is created
> > +but emulation isn't started. 'preconf' option value pauses QEMU before 
> > machine
> > +is created, which allows to query and configure properties affecting 
> > machine
> > +initialization. Use monitor/QMP command 'cont' to go to exit paused state.
> 
> What if "-S" is used at the same time"?  Will "cont" only
> initialize the machine and wait for another "cont" command to
> start the VCPUs, or will it unpause everything?
> 
> 
> > +ETEXI
> > +
> >  DEF("S", 0, QEMU_OPTION_S, \
> >      "-S              freeze CPU at startup (use 'c' to start execution)\n",
> >      QEMU_ARCH_ALL)
> > diff --git a/qmp.c b/qmp.c
> > index e8c3031..49e9a5c 100644
> > --- a/qmp.c
> > +++ b/qmp.c
> > @@ -167,6 +167,11 @@ void qmp_cont(Error **errp)
> >      BlockBackend *blk;
> >      Error *local_err = NULL;
> >  
> > +    if (runstate_check(RUN_STATE_PRELAUNCH)) {
> > +        qemu_exit_preconfig_request();
> > +        return;
> > +    }
> > +
> >      /* if there is a dump in background, we should wait until the dump
> >       * finished */
> >      if (dump_in_progress()) {
> > diff --git a/vl.c b/vl.c
> > index 3fed457..30631fd 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -555,6 +555,20 @@ static QemuOptsList qemu_fw_cfg_opts = {
> >      },
> >  };
> >  
> > +static QemuOptsList qemu_paused_opts = {
> > +    .name = "paused",
> > +    .implied_opt_name = "state",
> > +    .head = QTAILQ_HEAD_INITIALIZER(qemu_paused_opts.head),
> > +    .desc = {
> > +        {
> > +            .name = "state",
> > +            .type = QEMU_OPT_STRING,
> > +            .help = "Pause state of QEMU on startup",
> > +        },
> > +        { /* end of list */ }
> > +    },
> > +};
> > +
> >  /**
> >   * Get machine options
> >   *
> > @@ -1689,6 +1703,11 @@ static pid_t shutdown_pid;
> >  static int powerdown_requested;
> >  static int debug_requested;
> >  static int suspend_requested;
> > +static enum {
> > +    PRECONFIG_CONT = 0,
> > +    PRECONFIG_PAUSE,
> > +    PRECONFIG_SKIP,
> > +} preconfig_requested;
> >  static WakeupReason wakeup_reason;
> >  static NotifierList powerdown_notifiers =
> >      NOTIFIER_LIST_INITIALIZER(powerdown_notifiers);
> > @@ -1773,6 +1792,11 @@ static int qemu_debug_requested(void)
> >      return r;
> >  }
> >  
> > +void qemu_exit_preconfig_request(void)
> > +{
> > +    preconfig_requested = PRECONFIG_CONT;
> > +}
> > +
> >  /*
> >   * Reset the VM. Issue an event unless @reason is SHUTDOWN_CAUSE_NONE.
> >   */
> > @@ -1939,6 +1963,12 @@ static bool main_loop_should_exit(void)
> >      RunState r;
> >      ShutdownCause request;
> >  
> > +    if (runstate_check(RUN_STATE_PRELAUNCH)) {
> > +        if (preconfig_requested == PRECONFIG_CONT) {
> > +            preconfig_requested = PRECONFIG_SKIP;
> > +            return true;
> > +        }
> > +    }
> >      if (qemu_debug_requested()) {
> >          vm_stop(RUN_STATE_DEBUG);
> >      }
> > @@ -3177,6 +3207,7 @@ int main(int argc, char **argv, char **envp)
> >      qemu_add_opts(&qemu_icount_opts);
> >      qemu_add_opts(&qemu_semihosting_config_opts);
> >      qemu_add_opts(&qemu_fw_cfg_opts);
> > +    qemu_add_opts(&qemu_paused_opts);
> >      module_call_init(MODULE_INIT_OPTS);
> >  
> >      runstate_init();
> > @@ -3845,6 +3876,26 @@ int main(int argc, char **argv, char **envp)
> >                      exit(1);
> >                  }
> >                  break;
> > +            case QEMU_OPTION_paused:
> > +                {
> > +                    const char *value;
> > +
> > +                    opts = 
> > qemu_opts_parse_noisily(qemu_find_opts("paused"),
> > +                                                   optarg, true);
> > +                    if (opts == NULL) {
> > +                        exit(1);
> > +                    }
> > +                    value = qemu_opt_get(opts, "state");
> > +                    if (!strcmp(value, "postconf")) {
> > +                        autostart = 0;
> > +                    } else if (!strcmp(value, "preconf")) {
> > +                        preconfig_requested = PRECONFIG_PAUSE;
> > +                    } else {
> > +                        error_report("incomplete '-paused' option\n");
> > +                        exit(1);
> > +                    }
> > +                    break;
> > +                }
> >              case QEMU_OPTION_enable_kvm:
> >                  olist = qemu_find_opts("machine");
> >                  qemu_opts_parse_noisily(olist, "accel=kvm", false);
> > @@ -4731,7 +4782,6 @@ int main(int argc, char **argv, char **envp)
> >      current_machine->boot_order = boot_order;
> >      current_machine->cpu_model = cpu_model;
> >  
> > -
> >      /* parse features once if machine provides default cpu_type */
> >      if (machine_class->default_cpu_type) {
> >          current_machine->cpu_type = machine_class->default_cpu_type;
> > @@ -4741,6 +4791,8 @@ int main(int argc, char **argv, char **envp)
> >          }
> >      }
> >  
> > +    main_loop(); /* do monitor/qmp handling at preconfig state if 
> > requested */
> > +
> 
> I'm impressed by the simplicity of the implementation.  I though
> this would involve moving everything between this line and the
> next main_loop() call outside main(), so they would be called by
> qmp_cont().
> 
> Any expert on GLib's Event Loop sees any gotcha in this method?
> 
> I would like to do a careful review of main_loop_wait() and
> main_loop_should_exit(), to ensure those functions don't depend
> on anything that's initialized after this line.  Probably a few
> existing QMP commands can crash if machine is not initialized
> yet?
> 
> The rules and expectations on initialization ordering are very
> subtle, I suggest including test code for the new feature to
> ensure nothing crashes or breaks in the future.
> 
> 
> >      machine_run_board_init(current_machine);
> >  
> >      realtime_init();
> 

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature