Re: [Qemu-devel] [PATCH 1/8] Introduce the VMStatus type

[Luiz Capitulino]

On Tue, 12 Jul 2011 12:12:31 -0300
Luiz Capitulino <address@hidden> wrote:

> On Tue, 12 Jul 2011 16:51:03 +0200
> Kevin Wolf <address@hidden> wrote:
> 
> > Am 12.07.2011 16:25, schrieb Luiz Capitulino:
> > > On Tue, 12 Jul 2011 09:28:05 +0200
> > > Markus Armbruster <address@hidden> wrote:
> > > 
> > >> Luiz Capitulino <address@hidden> writes:
> > >>
> > >>> We need to track the VM status so that QMP can report it to clients.
> > >>>
> > >>> This commit adds the VMStatus type and related functions. The
> > >>> vm_status_set() function is used to keep track of the current
> > >>> VM status.
> > >>>
> > >>> The current statuses are:
> > >>
> > >> Nitpicking about names, bear with me.
> > >>
> > >>>     - debug: guest is running under gdb
> > >>>     - inmigrate: guest is paused waiting for an incoming migration
> > >>
> > >> incoming-migration?
> > >>
> > >>>     - postmigrate: guest is paused following a successful migration
> > >>
> > >> post-migrate?
> > >>
> > >>>     - internal-error: Fatal internal error that prevents further guest
> > >>>                 execution
> > >>>     - load-state-error: guest is paused following a failed 'loadvm'
> > >>
> > >> Less than obvious.  If you like concrete, name it loadvm-failed.  If you
> > >> like abstract, name it restore-vm-failed.
> > > 
> > > Ok for your suggestions above.
> > > 
> > >>
> > >>>     - io-error: the last IOP has failed and the device is configured
> > >>>                 to pause on I/O errors
> > >>>     - watchdog-error: the watchdog action is configured to pause and
> > >>>                       has been triggered
> > >>
> > >> Sounds like the watchdog suffered an error.  watchdog-fired?
> > > 
> > > Maybe watchdog-paused.
> > > 
> > >>
> > >>>     - paused: guest has been paused via the 'stop' command
> > >>
> > >> stop-command?
> > > 
> > > I prefer 'paused', it communicates better the state we're in.
> > > 
> > >>
> > >>>     - prelaunch: QEMU was started with -S and guest has not started
> > >>
> > >> unstarted?
> > > 
> > > Looks the same to me.
> > > 
> > >>
> > >>>     - running: guest is actively running
> > >>>     - shutdown: guest is shut down (and -no-shutdown is in use)
> > >>>
> > >>> Signed-off-by: Luiz Capitulino <address@hidden>
> > >>> ---
> > >>>  gdbstub.c       |    4 ++++
> > >>>  hw/ide/core.c   |    1 +
> > >>>  hw/scsi-disk.c  |    1 +
> > >>>  hw/virtio-blk.c |    1 +
> > >>>  hw/watchdog.c   |    1 +
> > >>>  kvm-all.c       |    1 +
> > >>>  migration.c     |    3 +++
> > >>>  monitor.c       |    5 ++++-
> > >>>  sysemu.h        |   19 +++++++++++++++++++
> > >>>  vl.c            |   37 +++++++++++++++++++++++++++++++++++++
> > >>>  10 files changed, 72 insertions(+), 1 deletions(-)
> > >>>
> > >>> diff --git a/gdbstub.c b/gdbstub.c
> > >>> index c085a5a..61b700a 100644
> > >>> --- a/gdbstub.c
> > >>> +++ b/gdbstub.c
> > >>> @@ -2358,6 +2358,7 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, 
> > >>> const char *fmt, ...)
> > >>>      s->state = RS_SYSCALL;
> > >>>  #ifndef CONFIG_USER_ONLY
> > >>>      vm_stop(VMSTOP_DEBUG);
> > >>> +    vm_status_set(VMST_DEBUG);
> > >>>  #endif
> > >>>      s->state = RS_IDLE;
> > >>>      va_start(va, fmt);
> > >>> @@ -2432,6 +2433,7 @@ static void gdb_read_byte(GDBState *s, int ch)
> > >>>          /* when the CPU is running, we cannot do anything except stop
> > >>>             it when receiving a char */
> > >>>          vm_stop(VMSTOP_USER);
> > >>> +        vm_status_set(VMST_DEBUG);
> > >>>      } else
> > >>>  #endif
> > >>>      {
> > >>> @@ -2694,6 +2696,7 @@ static void gdb_chr_event(void *opaque, int event)
> > >>>      switch (event) {
> > >>>      case CHR_EVENT_OPENED:
> > >>>          vm_stop(VMSTOP_USER);
> > >>> +        vm_status_set(VMST_DEBUG);
> > >>>          gdb_has_xml = 0;
> > >>>          break;
> > >>>      default:
> > >>
> > >> Previous hunk has VMST_DEBUG with VMST_DEBUG.  Odd.
> > >>
> > >>> @@ -2735,6 +2738,7 @@ static void gdb_sigterm_handler(int signal)
> > >>>  {
> > >>>      if (vm_running) {
> > >>>          vm_stop(VMSTOP_USER);
> > >>> +        vm_status_set(VMST_DEBUG);
> > >>>      }
> > >>>  }
> > >>>  #endif
> > >>> diff --git a/hw/ide/core.c b/hw/ide/core.c
> > >>> index ca17a43..bf9df41 100644
> > >>> --- a/hw/ide/core.c
> > >>> +++ b/hw/ide/core.c
> > >>> @@ -523,6 +523,7 @@ static int ide_handle_rw_error(IDEState *s, int 
> > >>> error, int op)
> > >>>          s->bus->error_status = op;
> > >>>          bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
> > >>>          vm_stop(VMSTOP_DISKFULL);
> > >>> +        vm_status_set(VMST_IOERROR);
> > >>>      } else {
> > >>>          if (op & BM_STATUS_DMA_RETRY) {
> > >>>              dma_buf_commit(s, 0);
> > >>> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> > >>> index a8c7372..66037fd 100644
> > >>> --- a/hw/scsi-disk.c
> > >>> +++ b/hw/scsi-disk.c
> > >>> @@ -216,6 +216,7 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int 
> > >>> error, int type)
> > >>>  
> > >>>          bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
> > >>>          vm_stop(VMSTOP_DISKFULL);
> > >>> +        vm_status_set(VMST_IOERROR);
> > >>>      } else {
> > >>>          if (type == SCSI_REQ_STATUS_RETRY_READ) {
> > >>>              scsi_req_data(&r->req, 0);
> > >>> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> > >>> index 91e0394..bf70200 100644
> > >>> --- a/hw/virtio-blk.c
> > >>> +++ b/hw/virtio-blk.c
> > >>> @@ -79,6 +79,7 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq 
> > >>> *req, int error,
> > >>>          s->rq = req;
> > >>>          bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
> > >>>          vm_stop(VMSTOP_DISKFULL);
> > >>> +        vm_status_set(VMST_IOERROR);
> > >>>      } else {
> > >>>          virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
> > >>>          bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
> > >>> diff --git a/hw/watchdog.c b/hw/watchdog.c
> > >>> index 1c900a1..d130cbb 100644
> > >>> --- a/hw/watchdog.c
> > >>> +++ b/hw/watchdog.c
> > >>> @@ -133,6 +133,7 @@ void watchdog_perform_action(void)
> > >>>      case WDT_PAUSE:             /* same as 'stop' command in monitor */
> > >>>          watchdog_mon_event("pause");
> > >>>          vm_stop(VMSTOP_WATCHDOG);
> > >>> +        vm_status_set(VMST_WATCHDOG);
> > >>>          break;
> > >>>  
> > >>>      case WDT_DEBUG:
> > >>> diff --git a/kvm-all.c b/kvm-all.c
> > >>> index cbc2532..aee9e0a 100644
> > >>> --- a/kvm-all.c
> > >>> +++ b/kvm-all.c
> > >>> @@ -1015,6 +1015,7 @@ int kvm_cpu_exec(CPUState *env)
> > >>>      if (ret < 0) {
> > >>>          cpu_dump_state(env, stderr, fprintf, CPU_DUMP_CODE);
> > >>>          vm_stop(VMSTOP_PANIC);
> > >>> +        vm_status_set(VMST_INTERROR);
> > >>>      }
> > >>>  
> > >>>      env->exit_request = 0;
> > >>> diff --git a/migration.c b/migration.c
> > >>> index af3a1f2..674792f 100644
> > >>> --- a/migration.c
> > >>> +++ b/migration.c
> > >>> @@ -394,6 +394,9 @@ void migrate_fd_put_ready(void *opaque)
> > >>>              }
> > >>>              state = MIG_STATE_ERROR;
> > >>>          }
> > >>> +        if (state == MIG_STATE_COMPLETED) {
> > >>> +            vm_status_set(VMST_POSTMIGRATE);
> > >>> +        }
> > >>>          s->state = state;
> > >>>          notifier_list_notify(&migration_state_notifiers);
> > >>>      }
> > >>> diff --git a/monitor.c b/monitor.c
> > >>> index 67ceb46..1cb3191 100644
> > >>> --- a/monitor.c
> > >>> +++ b/monitor.c
> > >>> @@ -1258,6 +1258,7 @@ static void do_singlestep(Monitor *mon, const 
> > >>> QDict *qdict)
> > >>>  static int do_stop(Monitor *mon, const QDict *qdict, QObject 
> > >>> **ret_data)
> > >>>  {
> > >>>      vm_stop(VMSTOP_USER);
> > >>> +    vm_status_set(VMST_PAUSED);
> > >>>      return 0;
> > >>>  }
> > >>>  
> > >>> @@ -2782,7 +2783,9 @@ static void do_loadvm(Monitor *mon, const QDict 
> > >>> *qdict)
> > >>>  
> > >>>      vm_stop(VMSTOP_LOADVM);
> > >>>  
> > >>> -    if (load_vmstate(name) == 0 && saved_vm_running) {
> > >>> +    if (load_vmstate(name) < 0) {
> > >>> +        vm_status_set(VMST_LOADERROR);
> > >>> +    } else if (saved_vm_running) {
> > >>>          vm_start();
> > >>>      }
> > >>>  }
> > >>> diff --git a/sysemu.h b/sysemu.h
> > >>> index d3013f5..7308ac5 100644
> > >>> --- a/sysemu.h
> > >>> +++ b/sysemu.h
> > >>> @@ -77,6 +77,25 @@ int qemu_savevm_state_complete(Monitor *mon, 
> > >>> QEMUFile *f);
> > >>>  void qemu_savevm_state_cancel(Monitor *mon, QEMUFile *f);
> > >>>  int qemu_loadvm_state(QEMUFile *f);
> > >>>  
> > >>> +typedef enum {
> > >>> +    VMST_NOSTATUS,
> > >>> +    VMST_DEBUG,
> > >>> +    VMST_INMIGRATE,
> > >>> +    VMST_POSTMIGRATE,
> > >>> +    VMST_INTERROR,
> > >>> +    VMST_IOERROR,
> > >>> +    VMST_LOADERROR,
> > >>> +    VMST_PAUSED,
> > >>> +    VMST_PRELAUNCH,
> > >>> +    VMST_RUNNING,
> > >>> +    VMST_SHUTDOWN,
> > >>> +    VMST_WATCHDOG,
> > >>> +    VMST_MAX,
> > >>> +} VMStatus;
> > >>
> > >> How are these related to the VMSTOP_*?
> > >>
> > >> Why do we need a separate enumeration?
> > 
> > Luiz, what about this part? For me, this was the most important
> > question. We already have VMSTOP_*, and every caller of vm_stop() should
> > change the status (most of the vm_status_set() calls come immediately
> > after a vm_stop() call), so it would appear logical that vm_stop(),
> > which already gets a reason, sets the status.
> > 
> > Probably we would need a few additional reasons for vm_stop(), but
> > keeping two separate status values for almost the same thing looks
> > suspicious.
> 
> Well, that's how I was doing it but I had a conversation with Anthony
> and he was against using vm_stop() for this, because (IIRC) they are
> different things.

Let me elaborate the way I see it. The VMSTOP_* macros are stop reasons.
They say why the VM stopped, not what the VM status is. For example, "running"
is a valid vm state, but it doesn't make sense as a "stop reason".

It's possible to change vm_stop() (and vm_start()) to set the state instead,
but it's a more profound surgery than it may seem at first. For example, we
have things like vm stop notifiers, which would have to be changed to get the
vm status instead of a stop reason.

I started doing it that way and have to admit that having the vm state as
a different thing made the implementation simpler.