Patchwork [04/15] Improve vm_stop reason declarations

login
register
mail settings
Submitter Jan Kiszka
Date Feb. 7, 2011, 11:19 a.m.
Message ID <35fe72889a43ad4465c298a70069e9a6fc1eab66.1297077506.git.jan.kiszka@siemens.com>
Download mbox | patch
Permalink /patch/82206/
State New
Headers show

Comments

Jan Kiszka - Feb. 7, 2011, 11:19 a.m.
Define and use dedicated constants for vm_stop reasons, they actually
have nothing to do with the EXCP_* defines used so far. At this chance,
specify more detailed reasons so that VM state change handlers can
evaluate them.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 cpus.c          |    4 ++--
 gdbstub.c       |   19 ++++++++++---------
 hw/ide/core.c   |    2 +-
 hw/scsi-disk.c  |    2 +-
 hw/virtio-blk.c |    2 +-
 hw/watchdog.c   |    2 +-
 kvm-all.c       |    2 +-
 migration.c     |    2 +-
 monitor.c       |    4 ++--
 savevm.c        |    4 ++--
 sysemu.h        |    8 ++++++++
 vl.c            |    2 +-
 12 files changed, 31 insertions(+), 22 deletions(-)
Marcelo Tosatti - Feb. 8, 2011, 6:59 p.m.
On Mon, Feb 07, 2011 at 12:19:15PM +0100, Jan Kiszka wrote:
> index d6556c9..3397566 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -2194,14 +2194,14 @@ static void gdb_vm_state_change(void *opaque, int running, int reason)
>      const char *type;
>      int ret;
>  
> -    if (running || (reason != EXCP_DEBUG && reason != EXCP_INTERRUPT) ||
> -        s->state == RS_INACTIVE || s->state == RS_SYSCALL)
> +    if (running || (reason != VMSTOP_DEBUG && reason != VMSTOP_INTERRUPT) ||
> +        s->state == RS_INACTIVE || s->state == RS_SYSCALL) {
>          return;

What about VMSTOP_USER ?

VMSTOP_INTERRUPT -> "the VM is stopped by an interrupt".

VMSTOP_USER -> "the VM is stopped by the user".

> diff --git a/migration.c b/migration.c
> index 3612572..20ea113 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -378,7 +378,7 @@ void migrate_fd_put_ready(void *opaque)
>          int old_vm_running = vm_running;
>  
>          DPRINTF("done iterating\n");
> -        vm_stop(0);
> +        vm_stop(VMSTOP_RWSTATE);

VMSTOP_RWSTATE is cryptic. What about VMSTOP_SAVEVM, MIGRATE, etc.

Nice!
Jan Kiszka - Feb. 9, 2011, 8:07 a.m.
On 2011-02-08 19:59, Marcelo Tosatti wrote:
> On Mon, Feb 07, 2011 at 12:19:15PM +0100, Jan Kiszka wrote:
>> index d6556c9..3397566 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -2194,14 +2194,14 @@ static void gdb_vm_state_change(void *opaque, int running, int reason)
>>      const char *type;
>>      int ret;
>>  
>> -    if (running || (reason != EXCP_DEBUG && reason != EXCP_INTERRUPT) ||
>> -        s->state == RS_INACTIVE || s->state == RS_SYSCALL)
>> +    if (running || (reason != VMSTOP_DEBUG && reason != VMSTOP_INTERRUPT) ||
>> +        s->state == RS_INACTIVE || s->state == RS_SYSCALL) {
>>          return;
> 
> What about VMSTOP_USER ?
> 
> VMSTOP_INTERRUPT -> "the VM is stopped by an interrupt".
> 
> VMSTOP_USER -> "the VM is stopped by the user".

Makes a lot of sense, will change.

> 
>> diff --git a/migration.c b/migration.c
>> index 3612572..20ea113 100644
>> --- a/migration.c
>> +++ b/migration.c
>> @@ -378,7 +378,7 @@ void migrate_fd_put_ready(void *opaque)
>>          int old_vm_running = vm_running;
>>  
>>          DPRINTF("done iterating\n");
>> -        vm_stop(0);
>> +        vm_stop(VMSTOP_RWSTATE);
> 
> VMSTOP_RWSTATE is cryptic. What about VMSTOP_SAVEVM, MIGRATE, etc.

Both alternatives are not completely accurate as we also stop of vmload.
What about VMSTOP_SYNC_STATE or UPDATE_STATE?

Jan
Marcelo Tosatti - Feb. 9, 2011, 2:17 p.m.
On Wed, Feb 09, 2011 at 09:07:01AM +0100, Jan Kiszka wrote:
> On 2011-02-08 19:59, Marcelo Tosatti wrote:
> > On Mon, Feb 07, 2011 at 12:19:15PM +0100, Jan Kiszka wrote:
> >> index d6556c9..3397566 100644
> >> --- a/gdbstub.c
> >> +++ b/gdbstub.c
> >> @@ -2194,14 +2194,14 @@ static void gdb_vm_state_change(void *opaque, int running, int reason)
> >>      const char *type;
> >>      int ret;
> >>  
> >> -    if (running || (reason != EXCP_DEBUG && reason != EXCP_INTERRUPT) ||
> >> -        s->state == RS_INACTIVE || s->state == RS_SYSCALL)
> >> +    if (running || (reason != VMSTOP_DEBUG && reason != VMSTOP_INTERRUPT) ||
> >> +        s->state == RS_INACTIVE || s->state == RS_SYSCALL) {
> >>          return;
> > 
> > What about VMSTOP_USER ?
> > 
> > VMSTOP_INTERRUPT -> "the VM is stopped by an interrupt".
> > 
> > VMSTOP_USER -> "the VM is stopped by the user".
> 
> Makes a lot of sense, will change.
> 
> > 
> >> diff --git a/migration.c b/migration.c
> >> index 3612572..20ea113 100644
> >> --- a/migration.c
> >> +++ b/migration.c
> >> @@ -378,7 +378,7 @@ void migrate_fd_put_ready(void *opaque)
> >>          int old_vm_running = vm_running;
> >>  
> >>          DPRINTF("done iterating\n");
> >> -        vm_stop(0);
> >> +        vm_stop(VMSTOP_RWSTATE);
> > 
> > VMSTOP_RWSTATE is cryptic. What about VMSTOP_SAVEVM, MIGRATE, etc.
> 
> Both alternatives are not completely accurate as we also stop of vmload.
> What about VMSTOP_SYNC_STATE or UPDATE_STATE?

IMO it would be better to state the specific reason why the vm stopped,
not some generic action. VMSTOP_SAVEVM, VMSTOP_VMLOAD and
VMSTOP_MIGRATE.
Jan Kiszka - Feb. 9, 2011, 2:51 p.m.
On 2011-02-09 15:17, Marcelo Tosatti wrote:
> On Wed, Feb 09, 2011 at 09:07:01AM +0100, Jan Kiszka wrote:
>> On 2011-02-08 19:59, Marcelo Tosatti wrote:
>>> On Mon, Feb 07, 2011 at 12:19:15PM +0100, Jan Kiszka wrote:
>>>> index d6556c9..3397566 100644
>>>> --- a/gdbstub.c
>>>> +++ b/gdbstub.c
>>>> @@ -2194,14 +2194,14 @@ static void gdb_vm_state_change(void *opaque, int running, int reason)
>>>>      const char *type;
>>>>      int ret;
>>>>  
>>>> -    if (running || (reason != EXCP_DEBUG && reason != EXCP_INTERRUPT) ||
>>>> -        s->state == RS_INACTIVE || s->state == RS_SYSCALL)
>>>> +    if (running || (reason != VMSTOP_DEBUG && reason != VMSTOP_INTERRUPT) ||
>>>> +        s->state == RS_INACTIVE || s->state == RS_SYSCALL) {
>>>>          return;
>>>
>>> What about VMSTOP_USER ?
>>>
>>> VMSTOP_INTERRUPT -> "the VM is stopped by an interrupt".
>>>
>>> VMSTOP_USER -> "the VM is stopped by the user".
>>
>> Makes a lot of sense, will change.
>>
>>>
>>>> diff --git a/migration.c b/migration.c
>>>> index 3612572..20ea113 100644
>>>> --- a/migration.c
>>>> +++ b/migration.c
>>>> @@ -378,7 +378,7 @@ void migrate_fd_put_ready(void *opaque)
>>>>          int old_vm_running = vm_running;
>>>>  
>>>>          DPRINTF("done iterating\n");
>>>> -        vm_stop(0);
>>>> +        vm_stop(VMSTOP_RWSTATE);
>>>
>>> VMSTOP_RWSTATE is cryptic. What about VMSTOP_SAVEVM, MIGRATE, etc.
>>
>> Both alternatives are not completely accurate as we also stop of vmload.
>> What about VMSTOP_SYNC_STATE or UPDATE_STATE?
> 
> IMO it would be better to state the specific reason why the vm stopped,
> not some generic action. VMSTOP_SAVEVM, VMSTOP_VMLOAD and
> VMSTOP_MIGRATE.
> 

Ah, OK. Can be done.

Jan

Patch

diff --git a/cpus.c b/cpus.c
index e93d8b9..e08ec03 100644
--- a/cpus.c
+++ b/cpus.c
@@ -168,8 +168,8 @@  static bool all_cpus_idle(void)
 static void cpu_debug_handler(CPUState *env)
 {
     gdb_set_stop_cpu(env);
-    debug_requested = EXCP_DEBUG;
-    vm_stop(EXCP_DEBUG);
+    debug_requested = VMSTOP_DEBUG;
+    vm_stop(VMSTOP_DEBUG);
 }
 
 #ifdef CONFIG_LINUX
diff --git a/gdbstub.c b/gdbstub.c
index d6556c9..3397566 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2194,14 +2194,14 @@  static void gdb_vm_state_change(void *opaque, int running, int reason)
     const char *type;
     int ret;
 
-    if (running || (reason != EXCP_DEBUG && reason != EXCP_INTERRUPT) ||
-        s->state == RS_INACTIVE || s->state == RS_SYSCALL)
+    if (running || (reason != VMSTOP_DEBUG && reason != VMSTOP_INTERRUPT) ||
+        s->state == RS_INACTIVE || s->state == RS_SYSCALL) {
         return;
-
+    }
     /* disable single step if it was enable */
     cpu_single_step(env, 0);
 
-    if (reason == EXCP_DEBUG) {
+    if (reason == VMSTOP_DEBUG) {
         if (env->watchpoint_hit) {
             switch (env->watchpoint_hit->flags & BP_MEM_ACCESS) {
             case BP_MEM_READ:
@@ -2252,7 +2252,7 @@  void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...)
     gdb_current_syscall_cb = cb;
     s->state = RS_SYSCALL;
 #ifndef CONFIG_USER_ONLY
-    vm_stop(EXCP_DEBUG);
+    vm_stop(VMSTOP_DEBUG);
 #endif
     s->state = RS_IDLE;
     va_start(va, fmt);
@@ -2326,7 +2326,7 @@  static void gdb_read_byte(GDBState *s, int ch)
     if (vm_running) {
         /* when the CPU is running, we cannot do anything except stop
            it when receiving a char */
-        vm_stop(EXCP_INTERRUPT);
+        vm_stop(VMSTOP_INTERRUPT);
     } else
 #endif
     {
@@ -2588,7 +2588,7 @@  static void gdb_chr_event(void *opaque, int event)
 {
     switch (event) {
     case CHR_EVENT_OPENED:
-        vm_stop(EXCP_INTERRUPT);
+        vm_stop(VMSTOP_INTERRUPT);
         gdb_has_xml = 0;
         break;
     default:
@@ -2628,8 +2628,9 @@  static int gdb_monitor_write(CharDriverState *chr, const uint8_t *buf, int len)
 #ifndef _WIN32
 static void gdb_sigterm_handler(int signal)
 {
-    if (vm_running)
-        vm_stop(EXCP_INTERRUPT);
+    if (vm_running) {
+        vm_stop(VMSTOP_INTERRUPT);
+    }
 }
 #endif
 
diff --git a/hw/ide/core.c b/hw/ide/core.c
index dd63664..9c91a49 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -465,7 +465,7 @@  static int ide_handle_rw_error(IDEState *s, int error, int op)
         s->bus->dma->ops->set_unit(s->bus->dma, s->unit);
         s->bus->dma->ops->add_status(s->bus->dma, op);
         bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
-        vm_stop(0);
+        vm_stop(VMSTOP_DISKFULL);
     } 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 488eedd..b05e654 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -239,7 +239,7 @@  static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
         r->status |= SCSI_REQ_STATUS_RETRY | type;
 
         bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
-        vm_stop(0);
+        vm_stop(VMSTOP_DISKFULL);
     } else {
         if (type == SCSI_REQ_STATUS_RETRY_READ) {
             r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag, 0);
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index ffac5a4..b14fb99 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -78,7 +78,7 @@  static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
         req->next = s->rq;
         s->rq = req;
         bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
-        vm_stop(0);
+        vm_stop(VMSTOP_DISKFULL);
     } 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 e9dd56e..1c900a1 100644
--- a/hw/watchdog.c
+++ b/hw/watchdog.c
@@ -132,7 +132,7 @@  void watchdog_perform_action(void)
 
     case WDT_PAUSE:             /* same as 'stop' command in monitor */
         watchdog_mon_event("pause");
-        vm_stop(0);
+        vm_stop(VMSTOP_WATCHDOG);
         break;
 
     case WDT_DEBUG:
diff --git a/kvm-all.c b/kvm-all.c
index 42dfed8..19cf188 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -994,7 +994,7 @@  int kvm_cpu_exec(CPUState *env)
 
     if (ret < 0) {
         cpu_dump_state(env, stderr, fprintf, CPU_DUMP_CODE);
-        vm_stop(0);
+        vm_stop(VMSTOP_PANIC);
         env->exit_request = 1;
     }
     if (env->exit_request) {
diff --git a/migration.c b/migration.c
index 3612572..20ea113 100644
--- a/migration.c
+++ b/migration.c
@@ -378,7 +378,7 @@  void migrate_fd_put_ready(void *opaque)
         int old_vm_running = vm_running;
 
         DPRINTF("done iterating\n");
-        vm_stop(0);
+        vm_stop(VMSTOP_RWSTATE);
 
         if ((qemu_savevm_state_complete(s->mon, s->file)) < 0) {
             if (old_vm_running) {
diff --git a/monitor.c b/monitor.c
index 7fc311d..1df3ad5 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1255,7 +1255,7 @@  static void do_singlestep(Monitor *mon, const QDict *qdict)
  */
 static int do_stop(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
-    vm_stop(EXCP_INTERRUPT);
+    vm_stop(VMSTOP_INTERRUPT);
     return 0;
 }
 
@@ -2783,7 +2783,7 @@  static void do_loadvm(Monitor *mon, const QDict *qdict)
     int saved_vm_running  = vm_running;
     const char *name = qdict_get_str(qdict, "name");
 
-    vm_stop(0);
+    vm_stop(VMSTOP_RWSTATE);
 
     if (load_vmstate(name) == 0 && saved_vm_running) {
         vm_start();
diff --git a/savevm.c b/savevm.c
index 4453217..b378d3d 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1575,7 +1575,7 @@  static int qemu_savevm_state(Monitor *mon, QEMUFile *f)
     int ret;
 
     saved_vm_running = vm_running;
-    vm_stop(0);
+    vm_stop(VMSTOP_RWSTATE);
 
     if (qemu_savevm_state_blocked(mon)) {
         ret = -EINVAL;
@@ -1896,7 +1896,7 @@  void do_savevm(Monitor *mon, const QDict *qdict)
     }
 
     saved_vm_running = vm_running;
-    vm_stop(0);
+    vm_stop(VMSTOP_RWSTATE);
 
     memset(sn, 0, sizeof(*sn));
 
diff --git a/sysemu.h b/sysemu.h
index 23ae17e..b23e722 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -37,6 +37,14 @@  VMChangeStateEntry *qemu_add_vm_change_state_handler(VMChangeStateHandler *cb,
                                                      void *opaque);
 void qemu_del_vm_change_state_handler(VMChangeStateEntry *e);
 
+#define VMSTOP_INTERRUPT 0
+#define VMSTOP_DEBUG     1
+#define VMSTOP_SHUTDOWN  2
+#define VMSTOP_DISKFULL  3
+#define VMSTOP_WATCHDOG  4
+#define VMSTOP_PANIC     5
+#define VMSTOP_RWSTATE   6
+
 void vm_start(void);
 void vm_stop(int reason);
 
diff --git a/vl.c b/vl.c
index 837be97..9440f98 100644
--- a/vl.c
+++ b/vl.c
@@ -1433,7 +1433,7 @@  static void main_loop(void)
         if (qemu_shutdown_requested()) {
             monitor_protocol_event(QEVENT_SHUTDOWN, NULL);
             if (no_shutdown) {
-                vm_stop(0);
+                vm_stop(VMSTOP_SHUTDOWN);
                 no_shutdown = 0;
             } else
                 break;