[v14,1/4] add a new runstate: RUN_STATE_GUEST_PANICKED

The guest will be in this state when it is panicked.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 include/sysemu/sysemu.h |  1 +
 qapi-schema.json        |  5 ++++-
 qmp.c                   |  3 +--
 vl.c                    | 13 +++++++++++--
 4 files changed, 17 insertions(+), 5 deletions(-)

Hu Tao <hutao@cn.fujitsu.com> writes:

> The guest will be in this state when it is panicked.
>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  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);
>          }
>      }

Il 20/03/2013 09:58, Markus Armbruster ha scritto:
> Let's examine the other transitions to RUN_STATE_PAUSED, and whether
> they can now come from RUN_STATE_GUEST_PANICKED:
> 
> * process_incoming_migration_co()
> 
>   No, because we're in RUN_STATE_INMIGRATE here, aren't we?  Juan?

Yes.

> * 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?

RUN_STATE_FINISH_MIGRATE is reached with vm_stop_force_state, so every
state can go there.  Next question: why doesn't the switch to
RUN_STATE_SAVE_VM use vm_stop_force_state?

Next question: almost all states go to RUN_STATE_FINISH_MIGRATE, the
same would hold for RUN_STATE_SAVE_VM if it started using
vm_stop_force_state.  There are few exceptions, and I'm not even sure
all of them are correct (why can't RUN_STATE_DEBUG go to
RUN_STATE_FINISH_MIGRATE?).

Should vm_stop_force_state override the runstate check (either directly,
or by interposing a transition to RUN_STATE_PAUSED? The few outliers can
be handled with manually-placed assertions.

Paolo

Markus Armbruster <armbru@redhat.com> writes:

> Hu Tao <hutao@cn.fujitsu.com> writes:
>
>> The guest will be in this state when it is panicked.
>>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
>> ---
>>  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?

Nevermind this one.  Like for qmp_stop() below, the actual state
transition is protected by runstate_is_running().

> * 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.
[...]