[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/8] Introduce the VMStatus type
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH 1/8] Introduce the VMStatus type |
Date: |
Tue, 12 Jul 2011 18:16:26 +0200 |
User-agent: |
Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.17) Gecko/20110428 Fedora/3.1.10-1.fc15 Thunderbird/3.1.10 |
Am 12.07.2011 18:03, schrieb 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.
Maybe we can have vm_stop() take two arguments at least, so that you
can't forget updating the status when you call vm_stop()? Or are there
cases of vm_stop() that shouldn't change the status?
Kevin
- [Qemu-devel] [PATCH 1/8] Introduce the VMStatus type, (continued)
- Re: [Qemu-devel] [PATCH 1/8] Introduce the VMStatus type, Markus Armbruster, 2011/07/12
- Re: [Qemu-devel] [PATCH 1/8] Introduce the VMStatus type, Luiz Capitulino, 2011/07/12
- Re: [Qemu-devel] [PATCH 1/8] Introduce the VMStatus type, Kevin Wolf, 2011/07/12
- Re: [Qemu-devel] [PATCH 1/8] Introduce the VMStatus type, Luiz Capitulino, 2011/07/12
- Re: [Qemu-devel] [PATCH 1/8] Introduce the VMStatus type, Luiz Capitulino, 2011/07/12
- Re: [Qemu-devel] [PATCH 1/8] Introduce the VMStatus type,
Kevin Wolf <=
- Re: [Qemu-devel] [PATCH 1/8] Introduce the VMStatus type, Luiz Capitulino, 2011/07/12
[Qemu-devel] [PATCH 2/8] QMP: query-status: Introduce 'status' key, Luiz Capitulino, 2011/07/05
[Qemu-devel] [PATCH 3/8] block: Support to keep track of I/O status, Luiz Capitulino, 2011/07/05
Re: [Qemu-devel] [PATCH 3/8] block: Support to keep track of I/O status, Kevin Wolf, 2011/07/12
[Qemu-devel] [PATCH 6/8] scsi: Support I/O status, Luiz Capitulino, 2011/07/05