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

[Eduardo Habkost]

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


> +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();
> -- 
> 2.7.4
> 

-- 
Eduardo