diff mbox

[v13,4/8] add a new runstate: RUN_STATE_GUEST_PANICKED

Message ID ab47cb1ef5a962848b2390d5b602b36aca1f149a.1362051582.git.hutao@cn.fujitsu.com
State New
Headers show

Commit Message

Hu Tao Feb. 28, 2013, 12:13 p.m. UTC 
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>
---
 migration.c      |  1 +
 qapi-schema.json |  6 +++++-
 qmp.c            |  3 ++-
 vl.c             | 11 ++++++++++-
 4 files changed, 18 insertions(+), 3 deletions(-)

Comments

Paolo Bonzini March 4, 2013, 9:40 a.m. UTC | #1 
Il 28/02/2013 13:13, Hu Tao ha scritto:
> 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>
> ---
>  migration.c      |  1 +
>  qapi-schema.json |  6 +++++-
>  qmp.c            |  3 ++-
>  vl.c             | 11 ++++++++++-
>  4 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/migration.c b/migration.c
> index c29830e..fa17b82 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -698,6 +698,7 @@ static void *buffered_file_thread(void *opaque)
>                  int64_t start_time, end_time;
>  
>                  DPRINTF("done iterating\n");
> +                save_run_state();
>                  start_time = qemu_get_clock_ms(rt_clock);
>                  qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
>                  if (old_vm_running) {
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 28b070f..8f1d138 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -174,11 +174,15 @@
>  # @suspended: guest is suspended (ACPI S3)
>  #
>  # @watchdog: the watchdog action is configured to pause and has been triggered
> +#
> +# @guest-panicked: the panicked action is configured to pause and has been
> +# triggered.
>  ##
>  { '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
> diff --git a/qmp.c b/qmp.c
> index 5f1bed1..f5027f6 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -150,7 +150,8 @@ void qmp_cont(Error **errp)
>      Error *local_err = NULL;
>  
>      if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
> -               runstate_check(RUN_STATE_SHUTDOWN)) {
> +            runstate_check(RUN_STATE_SHUTDOWN) ||
> +            runstate_check(RUN_STATE_GUEST_PANICKED)) {
>          error_set(errp, QERR_RESET_REQUIRED);
>          return;
>      } else if (runstate_check(RUN_STATE_SUSPENDED)) {
> diff --git a/vl.c b/vl.c
> index 3d08e1a..51d4922 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -536,6 +536,7 @@ static const RunStateTransition runstate_transitions_def[] = {
>  
>      { RUN_STATE_INMIGRATE, RUN_STATE_RUNNING },
>      { RUN_STATE_INMIGRATE, RUN_STATE_PAUSED },
> +    { RUN_STATE_INMIGRATE, RUN_STATE_GUEST_PANICKED },

Is this a consequence of the first patch?

>      { RUN_STATE_INTERNAL_ERROR, RUN_STATE_RUNNING },
>      { RUN_STATE_INTERNAL_ERROR, RUN_STATE_FINISH_MIGRATE },
> @@ -549,6 +550,7 @@ static const RunStateTransition runstate_transitions_def[] = {
>      { RUN_STATE_POSTMIGRATE, RUN_STATE_RUNNING },
>      { RUN_STATE_POSTMIGRATE, RUN_STATE_PAUSED },
>      { RUN_STATE_POSTMIGRATE, RUN_STATE_FINISH_MIGRATE },
> +    { RUN_STATE_POSTMIGRATE, RUN_STATE_GUEST_PANICKED },

Impossible.  GUEST_PANICKED requires an instruction to be executed in
the guest, so it should first go to RUNNING.

>      { RUN_STATE_PRELAUNCH, RUN_STATE_RUNNING },
>      { RUN_STATE_PRELAUNCH, RUN_STATE_FINISH_MIGRATE },
> @@ -559,6 +561,7 @@ static const RunStateTransition runstate_transitions_def[] = {
>  
>      { RUN_STATE_RESTORE_VM, RUN_STATE_RUNNING },
>      { RUN_STATE_RESTORE_VM, RUN_STATE_PAUSED },
> +    { RUN_STATE_RESTORE_VM, RUN_STATE_GUEST_PANICKED },

Is it also for the first patch?

>      { RUN_STATE_RUNNING, RUN_STATE_DEBUG },
>      { RUN_STATE_RUNNING, RUN_STATE_INTERNAL_ERROR },
> @@ -569,6 +572,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 },

This one is obviously ok.

>      { RUN_STATE_SAVE_VM, RUN_STATE_RUNNING },
>  
> @@ -583,6 +587,10 @@ 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_RUNNING },
> +    { RUN_STATE_GUEST_PANICKED, RUN_STATE_PAUSED },
> +    { RUN_STATE_GUEST_PANICKED, RUN_STATE_FINISH_MIGRATE },

Like SHUTDOWN, it should go first to PAUSED and then to RUNNING.  A
GUEST_PANICKED -> RUNNING transition is not possible.  You're seeing it
because you lack the addition of GUEST_PANICKED here:

        if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
            runstate_check(RUN_STATE_SHUTDOWN)) {
            runstate_set(RUN_STATE_PAUSED);
        }

I think you should first move the INTERNAL_ERROR || SHUTDOWN checks to a
separate function, so that you can then add GUEST_PANICKED.

Paolo

>      { RUN_STATE_MAX, RUN_STATE_MAX },
>  };
>  
> @@ -2001,7 +2009,8 @@ static bool main_loop_should_exit(void)
>          qemu_system_reset(VMRESET_REPORT);
>          resume_all_vcpus();
>          if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
> -            runstate_check(RUN_STATE_SHUTDOWN)) {
> +            runstate_check(RUN_STATE_SHUTDOWN) ||
> +            runstate_check(RUN_STATE_GUEST_PANICKED)) {
>              bdrv_iterate(iostatus_bdrv_it, NULL);
>              vm_start();
>          }
>
Hu Tao March 5, 2013, 3:17 a.m. UTC | #2 
On Mon, Mar 04, 2013 at 10:40:15AM +0100, Paolo Bonzini wrote:
> Il 28/02/2013 13:13, Hu Tao ha scritto:
> > 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>
> > ---
> >  migration.c      |  1 +
> >  qapi-schema.json |  6 +++++-
> >  qmp.c            |  3 ++-
> >  vl.c             | 11 ++++++++++-
> >  4 files changed, 18 insertions(+), 3 deletions(-)
> > 
> > diff --git a/migration.c b/migration.c
> > index c29830e..fa17b82 100644
> > --- a/migration.c
> > +++ b/migration.c
> > @@ -698,6 +698,7 @@ static void *buffered_file_thread(void *opaque)
> >                  int64_t start_time, end_time;
> >  
> >                  DPRINTF("done iterating\n");
> > +                save_run_state();
> >                  start_time = qemu_get_clock_ms(rt_clock);
> >                  qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
> >                  if (old_vm_running) {
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 28b070f..8f1d138 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -174,11 +174,15 @@
> >  # @suspended: guest is suspended (ACPI S3)
> >  #
> >  # @watchdog: the watchdog action is configured to pause and has been triggered
> > +#
> > +# @guest-panicked: the panicked action is configured to pause and has been
> > +# triggered.
> >  ##
> >  { '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
> > diff --git a/qmp.c b/qmp.c
> > index 5f1bed1..f5027f6 100644
> > --- a/qmp.c
> > +++ b/qmp.c
> > @@ -150,7 +150,8 @@ void qmp_cont(Error **errp)
> >      Error *local_err = NULL;
> >  
> >      if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
> > -               runstate_check(RUN_STATE_SHUTDOWN)) {
> > +            runstate_check(RUN_STATE_SHUTDOWN) ||
> > +            runstate_check(RUN_STATE_GUEST_PANICKED)) {
> >          error_set(errp, QERR_RESET_REQUIRED);
> >          return;
> >      } else if (runstate_check(RUN_STATE_SUSPENDED)) {
> > diff --git a/vl.c b/vl.c
> > index 3d08e1a..51d4922 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -536,6 +536,7 @@ static const RunStateTransition runstate_transitions_def[] = {
> >  
> >      { RUN_STATE_INMIGRATE, RUN_STATE_RUNNING },
> >      { RUN_STATE_INMIGRATE, RUN_STATE_PAUSED },
> > +    { RUN_STATE_INMIGRATE, RUN_STATE_GUEST_PANICKED },
> 
> Is this a consequence of the first patch?

Yes.

> 
> >      { RUN_STATE_INTERNAL_ERROR, RUN_STATE_RUNNING },
> >      { RUN_STATE_INTERNAL_ERROR, RUN_STATE_FINISH_MIGRATE },
> > @@ -549,6 +550,7 @@ static const RunStateTransition runstate_transitions_def[] = {
> >      { RUN_STATE_POSTMIGRATE, RUN_STATE_RUNNING },
> >      { RUN_STATE_POSTMIGRATE, RUN_STATE_PAUSED },
> >      { RUN_STATE_POSTMIGRATE, RUN_STATE_FINISH_MIGRATE },
> > +    { RUN_STATE_POSTMIGRATE, RUN_STATE_GUEST_PANICKED },
> 
> Impossible.  GUEST_PANICKED requires an instruction to be executed in
> the guest, so it should first go to RUNNING.
> 
> >      { RUN_STATE_PRELAUNCH, RUN_STATE_RUNNING },
> >      { RUN_STATE_PRELAUNCH, RUN_STATE_FINISH_MIGRATE },
> > @@ -559,6 +561,7 @@ static const RunStateTransition runstate_transitions_def[] = {
> >  
> >      { RUN_STATE_RESTORE_VM, RUN_STATE_RUNNING },
> >      { RUN_STATE_RESTORE_VM, RUN_STATE_PAUSED },
> > +    { RUN_STATE_RESTORE_VM, RUN_STATE_GUEST_PANICKED },
> 
> Is it also for the first patch?

Yes.

> 
> >      { RUN_STATE_RUNNING, RUN_STATE_DEBUG },
> >      { RUN_STATE_RUNNING, RUN_STATE_INTERNAL_ERROR },
> > @@ -569,6 +572,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 },
> 
> This one is obviously ok.
> 
> >      { RUN_STATE_SAVE_VM, RUN_STATE_RUNNING },
> >  
> > @@ -583,6 +587,10 @@ 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_RUNNING },
> > +    { RUN_STATE_GUEST_PANICKED, RUN_STATE_PAUSED },
> > +    { RUN_STATE_GUEST_PANICKED, RUN_STATE_FINISH_MIGRATE },
> 
> Like SHUTDOWN, it should go first to PAUSED and then to RUNNING.  A
> GUEST_PANICKED -> RUNNING transition is not possible.  You're seeing it
> because you lack the addition of GUEST_PANICKED here:
> 
>         if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
>             runstate_check(RUN_STATE_SHUTDOWN)) {
>             runstate_set(RUN_STATE_PAUSED);
>         }
> 
> I think you should first move the INTERNAL_ERROR || SHUTDOWN checks to a
> separate function, so that you can then add GUEST_PANICKED.

Will 

         if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
             runstate_check(RUN_STATE_SHUTDOWN) ||
             runstate_check(RUN_STATE_GUEST_PANICKED)) {
             runstate_set(RUN_STATE_PAUSED);
         }

be OK? Or I must be misunderstanding you.

> 
> Paolo
> 
> >      { RUN_STATE_MAX, RUN_STATE_MAX },
> >  };
> >  
> > @@ -2001,7 +2009,8 @@ static bool main_loop_should_exit(void)
> >          qemu_system_reset(VMRESET_REPORT);
> >          resume_all_vcpus();
> >          if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
> > -            runstate_check(RUN_STATE_SHUTDOWN)) {
> > +            runstate_check(RUN_STATE_SHUTDOWN) ||
> > +            runstate_check(RUN_STATE_GUEST_PANICKED)) {
> >              bdrv_iterate(iostatus_bdrv_it, NULL);
> >              vm_start();
> >          }
> >
Paolo Bonzini March 5, 2013, 8:26 a.m. UTC | #3 
Il 05/03/2013 04:17, Hu Tao ha scritto:
> Will 
> 
>          if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
>              runstate_check(RUN_STATE_SHUTDOWN) ||
>              runstate_check(RUN_STATE_GUEST_PANICKED)) {
>              runstate_set(RUN_STATE_PAUSED);
>          }
> 
> be OK? Or I must be misunderstanding you.
> 

Please move

       return (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
               runstate_check(RUN_STATE_SHUTDOWN) ||
               runstate_check(RUN_STATE_GUEST_PANICKED));

to a separate function (runstate_needs_reset for example), so that you
can reuse it in the two or three places that need it.

Paolo
Hu Tao March 6, 2013, 9:03 a.m. UTC | #4 
On Tue, Mar 05, 2013 at 09:26:18AM +0100, Paolo Bonzini wrote:
> Il 05/03/2013 04:17, Hu Tao ha scritto:
> > Will 
> > 
> >          if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
> >              runstate_check(RUN_STATE_SHUTDOWN) ||
> >              runstate_check(RUN_STATE_GUEST_PANICKED)) {
> >              runstate_set(RUN_STATE_PAUSED);
> >          }
> > 
> > be OK? Or I must be misunderstanding you.
> > 
> 
> Please move
> 
>        return (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
>                runstate_check(RUN_STATE_SHUTDOWN) ||
>                runstate_check(RUN_STATE_GUEST_PANICKED));
> 
> to a separate function (runstate_needs_reset for example), so that you
> can reuse it in the two or three places that need it.

See it now. Thanks!
diff mbox

Patch

diff --git a/migration.c b/migration.c
index c29830e..fa17b82 100644
--- a/migration.c
+++ b/migration.c
@@ -698,6 +698,7 @@  static void *buffered_file_thread(void *opaque)
                 int64_t start_time, end_time;
 
                 DPRINTF("done iterating\n");
+                save_run_state();
                 start_time = qemu_get_clock_ms(rt_clock);
                 qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
                 if (old_vm_running) {
diff --git a/qapi-schema.json b/qapi-schema.json
index 28b070f..8f1d138 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -174,11 +174,15 @@ 
 # @suspended: guest is suspended (ACPI S3)
 #
 # @watchdog: the watchdog action is configured to pause and has been triggered
+#
+# @guest-panicked: the panicked action is configured to pause and has been
+# triggered.
 ##
 { '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
diff --git a/qmp.c b/qmp.c
index 5f1bed1..f5027f6 100644
--- a/qmp.c
+++ b/qmp.c
@@ -150,7 +150,8 @@  void qmp_cont(Error **errp)
     Error *local_err = NULL;
 
     if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
-               runstate_check(RUN_STATE_SHUTDOWN)) {
+            runstate_check(RUN_STATE_SHUTDOWN) ||
+            runstate_check(RUN_STATE_GUEST_PANICKED)) {
         error_set(errp, QERR_RESET_REQUIRED);
         return;
     } else if (runstate_check(RUN_STATE_SUSPENDED)) {
diff --git a/vl.c b/vl.c
index 3d08e1a..51d4922 100644
--- a/vl.c
+++ b/vl.c
@@ -536,6 +536,7 @@  static const RunStateTransition runstate_transitions_def[] = {
 
     { RUN_STATE_INMIGRATE, RUN_STATE_RUNNING },
     { RUN_STATE_INMIGRATE, RUN_STATE_PAUSED },
+    { RUN_STATE_INMIGRATE, RUN_STATE_GUEST_PANICKED },
 
     { RUN_STATE_INTERNAL_ERROR, RUN_STATE_RUNNING },
     { RUN_STATE_INTERNAL_ERROR, RUN_STATE_FINISH_MIGRATE },
@@ -549,6 +550,7 @@  static const RunStateTransition runstate_transitions_def[] = {
     { RUN_STATE_POSTMIGRATE, RUN_STATE_RUNNING },
     { RUN_STATE_POSTMIGRATE, RUN_STATE_PAUSED },
     { RUN_STATE_POSTMIGRATE, RUN_STATE_FINISH_MIGRATE },
+    { RUN_STATE_POSTMIGRATE, RUN_STATE_GUEST_PANICKED },
 
     { RUN_STATE_PRELAUNCH, RUN_STATE_RUNNING },
     { RUN_STATE_PRELAUNCH, RUN_STATE_FINISH_MIGRATE },
@@ -559,6 +561,7 @@  static const RunStateTransition runstate_transitions_def[] = {
 
     { RUN_STATE_RESTORE_VM, RUN_STATE_RUNNING },
     { RUN_STATE_RESTORE_VM, RUN_STATE_PAUSED },
+    { RUN_STATE_RESTORE_VM, RUN_STATE_GUEST_PANICKED },
 
     { RUN_STATE_RUNNING, RUN_STATE_DEBUG },
     { RUN_STATE_RUNNING, RUN_STATE_INTERNAL_ERROR },
@@ -569,6 +572,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 },
 
@@ -583,6 +587,10 @@  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_RUNNING },
+    { RUN_STATE_GUEST_PANICKED, RUN_STATE_PAUSED },
+    { RUN_STATE_GUEST_PANICKED, RUN_STATE_FINISH_MIGRATE },
+
     { RUN_STATE_MAX, RUN_STATE_MAX },
 };
 
@@ -2001,7 +2009,8 @@  static bool main_loop_should_exit(void)
         qemu_system_reset(VMRESET_REPORT);
         resume_all_vcpus();
         if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
-            runstate_check(RUN_STATE_SHUTDOWN)) {
+            runstate_check(RUN_STATE_SHUTDOWN) ||
+            runstate_check(RUN_STATE_GUEST_PANICKED)) {
             bdrv_iterate(iostatus_bdrv_it, NULL);
             vm_start();
         }