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

[Luiz Capitulino]

On Tue, 05 Jul 2011 13:58:34 -0500
Anthony Liguori <address@hidden> wrote:

> On 07/05/2011 01:51 PM, Luiz Capitulino wrote:
> > On Tue, 05 Jul 2011 13:33:07 -0500
> > Anthony Liguori<address@hidden>  wrote:
> >
> >> On 07/05/2011 01:17 PM, Luiz Capitulino wrote:
> >>> 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:
> >>
> >> Which states are terminal, and what's the proper procedure to recover
> >> from non-terminal states?  Is it cont or system_reset followed by cont?
> >
> > Can I add that to the next patch (which also adds the QMP doc) or do you
> > prefer it in this patch?
> 
> It can be a separate patch, I was more curious about whether you had 
> thought through this for each of the states so that the states we were 
> introducing were well defined.

I haven't to be honest. Adding it as documentation will be a good
exercise though. Will do that for v2, but if you have comments about
specific states, please, let me know now.

> 
> For instance, it's not clear to me what the semantics of 
> 'load-state-error' are and how it's different from prelaunch.
> 
> I'm quite surprised that 'load-state-error' wouldn't be a terminal error.

Me too. I thought that starting (but not running) the VM after a failed
load_vmstate() was a feature to allow debugging. I've never thought that it
were possible to do 'cont' in this case (only badness can happen when doing
this, no?).

There are two solutions:

 1. Just drop load-state-error

 2. Doesn't allow a VM that failed a load_vmstate() call to be put to run,
    unless another call to load_vmstate() succeeds

> 
> Regards,
> 
> Anthony Liguori
> 
> >
> >>
> >>>
> >>>       - debug: guest is running under gdb
> >>>       - inmigrate: guest is paused waiting for an incoming migration
> >>>       - postmigrate: guest is paused following a successful migration
> >>>       - internal-error: Fatal internal error that prevents further guest
> >>>                   execution
> >>>       - load-state-error: guest is paused following a failed 'loadvm'
> >>>       - 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
> >>>       - paused: guest has been paused via the 'stop' command
> >>>       - prelaunch: QEMU was started with -S and guest has not started
> >>>       - running: guest is actively running
> >>>       - shutdown: guest is shut down (and -no-shutdown is in use)
> >>
> >> Regards,
> >>
> >> Anthony Liguori
> >>
> >>>
> >>> 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:
> >>> @@ -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;
> >>> +
> >>> +void vm_status_set(VMStatus s);
> >>> +const char *vm_status_get_name(void);
> >>> +
> >>>    /* SLIRP */
> >>>    void do_info_slirp(Monitor *mon);
> >>>
> >>> diff --git a/vl.c b/vl.c
> >>> index fcd7395..fb7208f 100644
> >>> --- a/vl.c
> >>> +++ b/vl.c
> >>> @@ -309,6 +309,37 @@ static int default_driver_check(QemuOpts *opts, void 
> >>> *opaque)
> >>>    }
> >>>
> >>>    /***********************************************************/
> >>> +/* VMStatus */
> >>> +
> >>> +static const char *const vm_status_name[VMST_MAX] = {
> >>> +    [VMST_DEBUG] = "debug",
> >>> +    [VMST_INMIGRATE] = "inmigrate",
> >>> +    [VMST_POSTMIGRATE] = "postmigrate",
> >>> +    [VMST_INTERROR] = "internal-error",
> >>> +    [VMST_IOERROR] = "io-error",
> >>> +    [VMST_LOADERROR] = "load-state-error",
> >>> +    [VMST_PAUSED] = "paused",
> >>> +    [VMST_PRELAUNCH] = "prelaunch",
> >>> +    [VMST_RUNNING] = "running",
> >>> +    [VMST_SHUTDOWN] = "shutdown",
> >>> +    [VMST_WATCHDOG] = "watchdog-error",
> >>> +};
> >>> +
> >>> +static VMStatus qemu_vm_status = VMST_NOSTATUS;
> >>> +
> >>> +void vm_status_set(VMStatus s)
> >>> +{
> >>> +    assert(s>   VMST_NOSTATUS&&   s<   VMST_MAX);
> >>> +    qemu_vm_status = s;
> >>> +}
> >>> +
> >>> +const char *vm_status_get_name(void)
> >>> +{
> >>> +    assert(qemu_vm_status>   VMST_NOSTATUS&&   qemu_vm_status<   
> >>> VMST_MAX);
> >>> +    return vm_status_name[qemu_vm_status];
> >>> +}
> >>> +
> >>> +/***********************************************************/
> >>>    /* real time host monotonic timer */
> >>>
> >>>    /***********************************************************/
> >>> @@ -1150,6 +1181,7 @@ void vm_start(void)
> >>>            vm_state_notify(1, 0);
> >>>            resume_all_vcpus();
> >>>            monitor_protocol_event(QEVENT_RESUME, NULL);
> >>> +        vm_status_set(VMST_RUNNING);
> >>>        }
> >>>    }
> >>>
> >>> @@ -1392,12 +1424,14 @@ static void main_loop(void)
> >>>
> >>>            if (qemu_debug_requested()) {
> >>>                vm_stop(VMSTOP_DEBUG);
> >>> +            vm_status_set(VMST_DEBUG);
> >>>            }
> >>>            if (qemu_shutdown_requested()) {
> >>>                qemu_kill_report();
> >>>                monitor_protocol_event(QEVENT_SHUTDOWN, NULL);
> >>>                if (no_shutdown) {
> >>>                    vm_stop(VMSTOP_SHUTDOWN);
> >>> +                vm_status_set(VMST_SHUTDOWN);
> >>>                    no_shutdown = 0;
> >>>                } else
> >>>                    break;
> >>> @@ -2476,6 +2510,7 @@ int main(int argc, char **argv, char **envp)
> >>>                    break;
> >>>                case QEMU_OPTION_S:
> >>>                    autostart = 0;
> >>> +                vm_status_set(VMST_PRELAUNCH);
> >>>                    break;
> >>>               case QEMU_OPTION_k:
> >>>                   keyboard_layout = optarg;
> >>> @@ -3307,11 +3342,13 @@ int main(int argc, char **argv, char **envp)
> >>>        qemu_system_reset(VMRESET_SILENT);
> >>>        if (loadvm) {
> >>>            if (load_vmstate(loadvm)<   0) {
> >>> +            vm_status_set(VMST_LOADERROR);
> >>>                autostart = 0;
> >>>            }
> >>>        }
> >>>
> >>>        if (incoming) {
> >>> +        vm_status_set(VMST_INMIGRATE);
> >>>            int ret = qemu_start_incoming_migration(incoming);
> >>>            if (ret<   0) {
> >>>                fprintf(stderr, "Migration failed. Exit code %s(%d), 
> >>> exiting.\n",
> >>
> >
>