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

[Markus Armbruster <armbru@redhat.com>]

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