Re: [Qemu-devel] [PATCH v14 1/4] add a new runstate: RUN_STATE_GUEST_PANICKED

[Markus Armbruster]

Hu Tao <address@hidden> writes:

> The guest will be in this state when it is panicked.
>
> Signed-off-by: Wen Congyang <address@hidden>
> Signed-off-by: Hu Tao <address@hidden>
> ---
>  include/sysemu/sysemu.h |  1 +
>  qapi-schema.json        |  5 ++++-
>  qmp.c                   |  3 +--
>  vl.c                    | 13 +++++++++++--
>  4 files changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index b19ec95..0412a8a 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -22,6 +22,7 @@ int qemu_uuid_parse(const char *str, uint8_t *uuid);
>  bool runstate_check(RunState state);
>  void runstate_set(RunState new_state);
>  int runstate_is_running(void);
> +bool runstate_needs_reset(void);
>  typedef struct vm_change_state_entry VMChangeStateEntry;
>  typedef void VMChangeStateHandler(void *opaque, int running, RunState state);
>  
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 28b070f..003cbf2 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -174,11 +174,14 @@
>  # @suspended: guest is suspended (ACPI S3)
>  #
>  # @watchdog: the watchdog action is configured to pause and has been 
> triggered
> +#
> +# @guest-panicked: guest has been panicked as a result of guest OS panic
>  ##
>  { 'enum': 'RunState',
>    'data': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
>              'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm',
> -            'running', 'save-vm', 'shutdown', 'suspended', 'watchdog' ] }
> +            'running', 'save-vm', 'shutdown', 'suspended', 'watchdog',
> +            'guest-panicked' ] }
>  
>  ##
>  # @SnapshotInfo

RUN_STATE_GUEST_PANICKED is similar to RUN_STATE_INTERNAL_ERROR and
RUN_STATE_SHUTDOWN: the only way for the guest to continue is reset.
Correct?

> diff --git a/qmp.c b/qmp.c
> index 55b056b..a1ebb5d 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -149,8 +149,7 @@ void qmp_cont(Error **errp)
>  {
>      Error *local_err = NULL;
>  
> -    if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
> -               runstate_check(RUN_STATE_SHUTDOWN)) {
> +    if (runstate_needs_reset()) {
>          error_set(errp, QERR_RESET_REQUIRED);
>          return;
>      } else if (runstate_check(RUN_STATE_SUSPENDED)) {
> diff --git a/vl.c b/vl.c
> index c03edf1..926822b 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -566,6 +566,7 @@ static const RunStateTransition 
> runstate_transitions_def[] = {
>      { RUN_STATE_RUNNING, RUN_STATE_SAVE_VM },
>      { RUN_STATE_RUNNING, RUN_STATE_SHUTDOWN },
>      { RUN_STATE_RUNNING, RUN_STATE_WATCHDOG },
> +    { RUN_STATE_RUNNING, RUN_STATE_GUEST_PANICKED },
>  
>      { RUN_STATE_SAVE_VM, RUN_STATE_RUNNING },
>  

This permits runstate_set(RUN_STATE_GUEST_PANICKED) in
RUN_STATE_RUNNING.  Used in PATCH 3/4.  Good.

> @@ -580,6 +581,8 @@ static const RunStateTransition 
> runstate_transitions_def[] = {
>      { RUN_STATE_WATCHDOG, RUN_STATE_RUNNING },
>      { RUN_STATE_WATCHDOG, RUN_STATE_FINISH_MIGRATE },
>  
> +    { RUN_STATE_GUEST_PANICKED, RUN_STATE_PAUSED },
> +
>      { RUN_STATE_MAX, RUN_STATE_MAX },
>  };

This permits runstate_set(RUN_STATE_PAUSED) in RUN_STATE_GUEST_PANICKED.
An example for such a transition is in the last patch hunk: main loop
resetting the guest.

Let's examine the other transitions to RUN_STATE_PAUSED, and whether
they can now come from RUN_STATE_GUEST_PANICKED:

* gdb_read_byte()

  No, because vm_stop() is protected by runstate_is_running() here.

* gdb_chr_event() case CHR_EVENT_OPENED

  Can this happen in RUN_STATE_GUEST_PANICKED?  Jan?

  What about RUN_STATE_INTERNAL_ERROR and RUN_STATE_SHUTDOWN?
  
* gdb_sigterm_handler()

  No, because vm_stop() is protected by runstate_is_running() here.

  Aside: despite its name, this function handles SIGINT.  Ugh.

* process_incoming_migration_co()

  No, because we're in RUN_STATE_INMIGRATE here, aren't we?  Juan?

* qmp_stop()

  No, because vm_stop() calls do_vm_stop() to do the actual state
  transition, which protects it by runstate_is_running().

  We can ignore the conditional, it merely punts the vm_stop() to the
  main loop.

Next question: RUN_STATE_INTERNAL_ERROR and RUN_STATE_SHUTDOWN may go to
RUN_STATE_FINISH_MIGRATE, but RUN_STATE_GUEST_PANICKED may not.  Why?

>  
> @@ -621,6 +624,13 @@ int runstate_is_running(void)
>      return runstate_check(RUN_STATE_RUNNING);
>  }
>  
> +bool runstate_needs_reset(void)
> +{
> +    return runstate_check(RUN_STATE_INTERNAL_ERROR) ||
> +        runstate_check(RUN_STATE_SHUTDOWN) ||
> +        runstate_check(RUN_STATE_GUEST_PANICKED);
> +}
> +
>  StatusInfo *qmp_query_status(Error **errp)
>  {
>      StatusInfo *info = g_malloc0(sizeof(*info));
> @@ -1966,8 +1976,7 @@ static bool main_loop_should_exit(void)
>          cpu_synchronize_all_states();
>          qemu_system_reset(VMRESET_REPORT);
>          resume_all_vcpus();
> -        if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
> -            runstate_check(RUN_STATE_SHUTDOWN)) {
> +        if (runstate_needs_reset()) {
>              runstate_set(RUN_STATE_PAUSED);
>          }
>      }