| Message ID | 1312384643-581-3-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 08/03/2011 06:17 PM, Luiz Capitulino wrote: > @@ -9,6 +9,20 @@ > #include "notify.h" > > /* vl.c */ > + > +typedef enum { > + QSTATE_DEBUG, /* qemu is running under gdb */ > + QSTATE_INTERROR, /* paused due to an internal error */ > + QSTATE_IOERROR, /* paused due to an I/O error */ > + QSTATE_PAUSED, /* paused by the user (ie. the 'stop' command) */ > + QSTATE_PREMIGRATE, /* paused preparing to finish migrate */ > + QSTATE_RESTVM, /* paused restoring the VM state */ > + QSTATE_RUNNING, /* qemu is running */ > + QSTATE_SAVEVM, /* paused saving VM state */ > + QSTATE_SHUTDOWN, /* guest shut down and -no-shutdown is in use */ > + QSTATE_WATCHDOG /* watchdog fired and qemu is configured to pause */ > +} QemuState; > + > extern const char *bios_name; > Why "QemuState"? In general, "qemu" can be inferred from the fact that we're in qemu.git. Suggest "RunState". Second, these states can coexist. A user may pause the VM simultaneously with the watchdog firing or entering premigrate state. In fact, with multiple monitors, each monitor can pause and resume the vm independently. So I think we should keep a reference count instead of just a start/stop state. Perhaps vm_stop(QemuState s) { ++stopcount[s]; } vm_is_stopped() { for (s in states) if (stopcount[s]) return true; return false; }
On 2011-08-04 11:55, Avi Kivity wrote: > On 08/03/2011 06:17 PM, Luiz Capitulino wrote: >> @@ -9,6 +9,20 @@ >> #include "notify.h" >> >> /* vl.c */ >> + >> +typedef enum { >> + QSTATE_DEBUG, /* qemu is running under gdb */ >> + QSTATE_INTERROR, /* paused due to an internal error */ >> + QSTATE_IOERROR, /* paused due to an I/O error */ >> + QSTATE_PAUSED, /* paused by the user (ie. the 'stop' >> command) */ >> + QSTATE_PREMIGRATE, /* paused preparing to finish migrate */ >> + QSTATE_RESTVM, /* paused restoring the VM state */ >> + QSTATE_RUNNING, /* qemu is running */ >> + QSTATE_SAVEVM, /* paused saving VM state */ >> + QSTATE_SHUTDOWN, /* guest shut down and -no-shutdown is in >> use */ >> + QSTATE_WATCHDOG /* watchdog fired and qemu is configured to >> pause */ >> +} QemuState; >> + >> extern const char *bios_name; >> > > Why "QemuState"? In general, "qemu" can be inferred from the fact that > we're in qemu.git. Suggest "RunState". > > Second, these states can coexist. A user may pause the VM > simultaneously with the watchdog firing or entering premigrate state. > In fact, with multiple monitors, each monitor can pause and resume the > vm independently. > > So I think we should keep a reference count instead of just a start/stop > state. Perhaps > > vm_stop(QemuState s) > { > ++stopcount[s]; > } > > vm_is_stopped() > { > for (s in states) > if (stopcount[s]) > return true; > return false; > } I don't think this makes sense nor is user-friendly. If one command channel suspends the machine, others have the chance to subscribe for that event. Maintaining a suspension counter would mean you also need a channel to query its value. IMHO, there is also no use for defining stopped orthogonally to premigrate and other states that imply that the machine is stopped. Basically they mean "stopped for/because of X". We just need to avoid that you can enter plain stopped state from them by issuing the corresponding monitor command. The other way around might be possible, though, if there are race windows. Jan
On 2011-08-04 12:17, Jan Kiszka wrote: > On 2011-08-04 11:55, Avi Kivity wrote: >> On 08/03/2011 06:17 PM, Luiz Capitulino wrote: >>> @@ -9,6 +9,20 @@ >>> #include "notify.h" >>> >>> /* vl.c */ >>> + >>> +typedef enum { >>> + QSTATE_DEBUG, /* qemu is running under gdb */ >>> + QSTATE_INTERROR, /* paused due to an internal error */ >>> + QSTATE_IOERROR, /* paused due to an I/O error */ >>> + QSTATE_PAUSED, /* paused by the user (ie. the 'stop' >>> command) */ >>> + QSTATE_PREMIGRATE, /* paused preparing to finish migrate */ >>> + QSTATE_RESTVM, /* paused restoring the VM state */ >>> + QSTATE_RUNNING, /* qemu is running */ >>> + QSTATE_SAVEVM, /* paused saving VM state */ >>> + QSTATE_SHUTDOWN, /* guest shut down and -no-shutdown is in >>> use */ >>> + QSTATE_WATCHDOG /* watchdog fired and qemu is configured to >>> pause */ >>> +} QemuState; >>> + >>> extern const char *bios_name; >>> >> >> Why "QemuState"? In general, "qemu" can be inferred from the fact that >> we're in qemu.git. Suggest "RunState". >> >> Second, these states can coexist. A user may pause the VM >> simultaneously with the watchdog firing or entering premigrate state. >> In fact, with multiple monitors, each monitor can pause and resume the >> vm independently. >> >> So I think we should keep a reference count instead of just a start/stop >> state. Perhaps >> >> vm_stop(QemuState s) >> { >> ++stopcount[s]; >> } >> >> vm_is_stopped() >> { >> for (s in states) >> if (stopcount[s]) >> return true; >> return false; >> } > > I don't think this makes sense nor is user-friendly. If one command > channel suspends the machine, others have the chance to subscribe for > that event. Maintaining a suspension counter would mean you also need a > channel to query its value. > > IMHO, there is also no use for defining stopped orthogonally to > premigrate and other states that imply that the machine is stopped. > Basically they mean "stopped for/because of X". We just need to avoid > that you can enter plain stopped state from them by issuing the > corresponding monitor command. The other way around might be possible, > though, if there are race windows. The makes me wonder if qemu_state_set shouldn't validate if the state transition is legal (simple switch/case). That way, we would have a central point and could also avoid potential races or logical bugs where a newly set state is accidentally overwritten due to unexpected execution order. Jan
On Thu, 04 Aug 2011 12:17:55 +0200 Jan Kiszka <jan.kiszka@siemens.com> wrote: > On 2011-08-04 11:55, Avi Kivity wrote: > > On 08/03/2011 06:17 PM, Luiz Capitulino wrote: > >> @@ -9,6 +9,20 @@ > >> #include "notify.h" > >> > >> /* vl.c */ > >> + > >> +typedef enum { > >> + QSTATE_DEBUG, /* qemu is running under gdb */ > >> + QSTATE_INTERROR, /* paused due to an internal error */ > >> + QSTATE_IOERROR, /* paused due to an I/O error */ > >> + QSTATE_PAUSED, /* paused by the user (ie. the 'stop' > >> command) */ > >> + QSTATE_PREMIGRATE, /* paused preparing to finish migrate */ > >> + QSTATE_RESTVM, /* paused restoring the VM state */ > >> + QSTATE_RUNNING, /* qemu is running */ > >> + QSTATE_SAVEVM, /* paused saving VM state */ > >> + QSTATE_SHUTDOWN, /* guest shut down and -no-shutdown is in > >> use */ > >> + QSTATE_WATCHDOG /* watchdog fired and qemu is configured to > >> pause */ > >> +} QemuState; > >> + > >> extern const char *bios_name; > >> > > > > Why "QemuState"? In general, "qemu" can be inferred from the fact that > > we're in qemu.git. Suggest "RunState". Having a RUNSTATE_PAUSED seems a bit strange when reading it for the first time. But I chose QemuState because I didn't have other options, so I'm fine with RunState. > > Second, these states can coexist. A user may pause the VM > > simultaneously with the watchdog firing or entering premigrate state. > > In fact, with multiple monitors, each monitor can pause and resume the > > vm independently. > > > > So I think we should keep a reference count instead of just a start/stop > > state. Perhaps > > > > vm_stop(QemuState s) > > { > > ++stopcount[s]; > > } > > > > vm_is_stopped() > > { > > for (s in states) > > if (stopcount[s]) > > return true; > > return false; > > } > > I don't think this makes sense nor is user-friendly. If one command > channel suspends the machine, others have the chance to subscribe for > that event. Maintaining a suspension counter would mean you also need a > channel to query its value. Agreed. Can be done incrementally if this turns out to be needed. > > IMHO, there is also no use for defining stopped orthogonally to > premigrate and other states that imply that the machine is stopped. > Basically they mean "stopped for/because of X". We just need to avoid > that you can enter plain stopped state from them by issuing the > corresponding monitor command. The other way around might be possible, > though, if there are race windows. > > Jan > >
On 08/04/2011 01:17 PM, Jan Kiszka wrote: > > > > Why "QemuState"? In general, "qemu" can be inferred from the fact that > > we're in qemu.git. Suggest "RunState". > > > > Second, these states can coexist. A user may pause the VM > > simultaneously with the watchdog firing or entering premigrate state. > > In fact, with multiple monitors, each monitor can pause and resume the > > vm independently. > > > > So I think we should keep a reference count instead of just a start/stop > > state. Perhaps > > > > vm_stop(QemuState s) > > { > > ++stopcount[s]; > > } > > > > vm_is_stopped() > > { > > for (s in states) > > if (stopcount[s]) > > return true; > > return false; > > } > > I don't think this makes sense nor is user-friendly. If one command > channel suspends the machine, others have the chance to subscribe for > that event. It's inherently racy. > Maintaining a suspension counter would mean you also need a > channel to query its value. Why? > IMHO, there is also no use for defining stopped orthogonally to > premigrate and other states that imply that the machine is stopped. > Basically they mean "stopped for/because of X". We just need to avoid > that you can enter plain stopped state from them by issuing the > corresponding monitor command. The other way around might be possible, > though, if there are race windows. > I'm worried about the following race: stop (qemu stopped for internal reason) stop comment processed resume The (qemu stopped for internal reason) part is lost.
On Mon, 08 Aug 2011 14:22:14 +0300 Avi Kivity <avi@redhat.com> wrote: > On 08/04/2011 01:17 PM, Jan Kiszka wrote: > > > > > > Why "QemuState"? In general, "qemu" can be inferred from the fact that > > > we're in qemu.git. Suggest "RunState". > > > > > > Second, these states can coexist. A user may pause the VM > > > simultaneously with the watchdog firing or entering premigrate state. > > > In fact, with multiple monitors, each monitor can pause and resume the > > > vm independently. > > > > > > So I think we should keep a reference count instead of just a start/stop > > > state. Perhaps > > > > > > vm_stop(QemuState s) > > > { > > > ++stopcount[s]; > > > } > > > > > > vm_is_stopped() > > > { > > > for (s in states) > > > if (stopcount[s]) > > > return true; > > > return false; > > > } > > > > I don't think this makes sense nor is user-friendly. If one command > > channel suspends the machine, others have the chance to subscribe for > > that event. > > It's inherently racy. > > > Maintaining a suspension counter would mean you also need a > > channel to query its value. > > Why? > > > IMHO, there is also no use for defining stopped orthogonally to > > premigrate and other states that imply that the machine is stopped. > > Basically they mean "stopped for/because of X". We just need to avoid > > that you can enter plain stopped state from them by issuing the > > corresponding monitor command. The other way around might be possible, > > though, if there are race windows. > > > > I'm worried about the following race: > > stop > (qemu stopped for internal reason) > stop comment processed > > resume > > The (qemu stopped for internal reason) part is lost. If the "stop" you're referring to happens through vm_stop(), then no, it won't be lost because do_vm_stop() doesn't allow qemu to be stopped twice.
On 08/08/2011 04:25 PM, Luiz Capitulino wrote: > > > > I'm worried about the following race: > > > > stop > > (qemu stopped for internal reason) > > stop comment processed > > > > resume > > > > The (qemu stopped for internal reason) part is lost. > > If the "stop" you're referring to happens through vm_stop(), then no, > it won't be lost because do_vm_stop() doesn't allow qemu to be stopped > twice. What happens then? The user sees an error?
On Mon, 08 Aug 2011 16:27:05 +0300 Avi Kivity <avi@redhat.com> wrote: > On 08/08/2011 04:25 PM, Luiz Capitulino wrote: > > > > > > I'm worried about the following race: > > > > > > stop > > > (qemu stopped for internal reason) > > > stop comment processed > > > > > > resume > > > > > > The (qemu stopped for internal reason) part is lost. > > > > If the "stop" you're referring to happens through vm_stop(), then no, > > it won't be lost because do_vm_stop() doesn't allow qemu to be stopped > > twice. > > What happens then? The user sees an error? It's ignored.
On 08/08/2011 04:28 PM, Luiz Capitulino wrote: > On Mon, 08 Aug 2011 16:27:05 +0300 > Avi Kivity<avi@redhat.com> wrote: > > > On 08/08/2011 04:25 PM, Luiz Capitulino wrote: > > > > > > > > I'm worried about the following race: > > > > > > > > stop > > > > (qemu stopped for internal reason) > > > > stop comment processed > > > > > > > > resume > > > > > > > > The (qemu stopped for internal reason) part is lost. > > > > > > If the "stop" you're referring to happens through vm_stop(), then no, > > > it won't be lost because do_vm_stop() doesn't allow qemu to be stopped > > > twice. > > > > What happens then? The user sees an error? > > It's ignored. Well, then, the user won't know something happened and will happily resume the guest, like I outlined above. When you ignore something in the first set, something breaks in the third.
On Mon, 08 Aug 2011 16:40:17 +0300 Avi Kivity <avi@redhat.com> wrote: > On 08/08/2011 04:28 PM, Luiz Capitulino wrote: > > On Mon, 08 Aug 2011 16:27:05 +0300 > > Avi Kivity<avi@redhat.com> wrote: > > > > > On 08/08/2011 04:25 PM, Luiz Capitulino wrote: > > > > > > > > > > I'm worried about the following race: > > > > > > > > > > stop > > > > > (qemu stopped for internal reason) > > > > > stop comment processed > > > > > > > > > > resume > > > > > > > > > > The (qemu stopped for internal reason) part is lost. > > > > > > > > If the "stop" you're referring to happens through vm_stop(), then no, > > > > it won't be lost because do_vm_stop() doesn't allow qemu to be stopped > > > > twice. > > > > > > What happens then? The user sees an error? > > > > It's ignored. > > Well, then, the user won't know something happened and will happily > resume the guest, like I outlined above. I think it makes sense to return an error in the monitor if the user tries to stop qemu when it's already stopped. Not sure if it will do what you think it should do, but we should always tell the user when we're unable to carry his/her orders. But it does make sense to me to not allow stopping twice. First because it doesn't make sense to stop something which is not moving and second because what else can stop the vm if it's already stopped? Maybe vm_stop() should return an error, but I think this goes beyond this series. > > When you ignore something in the first set, something breaks in the third. >
On 08/08/2011 04:47 PM, Luiz Capitulino wrote: > > > > Well, then, the user won't know something happened and will happily > > resume the guest, like I outlined above. > > I think it makes sense to return an error in the monitor if the user > tries to stop qemu when it's already stopped. Not sure if it will do what you > think it should do, but we should always tell the user when we're unable to > carry his/her orders. > > But it does make sense to me to not allow stopping twice. First because it > doesn't make sense to stop something which is not moving and second because > what else can stop the vm if it's already stopped? > > Maybe vm_stop() should return an error, but I think this goes beyond this > series. > This is why I suggested a reference count. In this case, we can always stop the guest "twice", because we don't lost information when we resume.
On Mon, 08 Aug 2011 16:54:16 +0300 Avi Kivity <avi@redhat.com> wrote: > On 08/08/2011 04:47 PM, Luiz Capitulino wrote: > > > > > > Well, then, the user won't know something happened and will happily > > > resume the guest, like I outlined above. > > > > I think it makes sense to return an error in the monitor if the user > > tries to stop qemu when it's already stopped. Not sure if it will do what you > > think it should do, but we should always tell the user when we're unable to > > carry his/her orders. > > > > But it does make sense to me to not allow stopping twice. First because it > > doesn't make sense to stop something which is not moving and second because > > what else can stop the vm if it's already stopped? > > > > Maybe vm_stop() should return an error, but I think this goes beyond this > > series. > > > > This is why I suggested a reference count. In this case, we can always > stop the guest "twice", because we don't lost information when we resume. I could give it a try in the near future, as I really think it's independent from this series, but I still don't understand what can stop an already stopped VM besides the user. This is a real question, is it really possible? If only the user can do that, then the refcount is overkill as just returning an error will do it.
On 08/08/2011 05:06 PM, Luiz Capitulino wrote: > > > > This is why I suggested a reference count. In this case, we can always > > stop the guest "twice", because we don't lost information when we resume. > > I could give it a try in the near future, as I really think it's independent > from this series, but I still don't understand what can stop an already stopped > VM besides the user. This is a real question, is it really possible? > > If only the user can do that, then the refcount is overkill as just returning > an error will do it. Most of the reasons in QemuState are asynchronous and can happen at any time while the guest is running. Because they're asynchronous, they can trigger before a monitor stop is issued but caught after it is processed. We could possibly synchronize during user stop, but that lets us capture only the first non-user reason. And even if we return an error, that only pushes the refcounting to the user; you probably don't want to return a "vm is already stopped" error to the user, he'll just ask "why are you telling me that".
On Mon, 08 Aug 2011 17:27:24 +0300 Avi Kivity <avi@redhat.com> wrote: > On 08/08/2011 05:06 PM, Luiz Capitulino wrote: > > > > > > This is why I suggested a reference count. In this case, we can always > > > stop the guest "twice", because we don't lost information when we resume. > > > > I could give it a try in the near future, as I really think it's independent > > from this series, but I still don't understand what can stop an already stopped > > VM besides the user. This is a real question, is it really possible? > > > > If only the user can do that, then the refcount is overkill as just returning > > an error will do it. > > Most of the reasons in QemuState are asynchronous and can happen at any > time while the guest is running. Because they're asynchronous, they can > trigger before a monitor stop is issued but caught after it is processed. > > We could possibly synchronize during user stop, but that lets us capture > only the first non-user reason. Isn't it good enough? Another question: do you think that this problem is so relevant that we should include the fix in the first merge? > > And even if we return an error, that only pushes the refcounting to the > user; you probably don't want to return a "vm is already stopped" error > to the user, he'll just ask "why are you telling me that". >
diff --git a/audio/audio.c b/audio/audio.c index 50d2b64..254cffc 100644 --- a/audio/audio.c +++ b/audio/audio.c @@ -1743,7 +1743,7 @@ static int audio_driver_init (AudioState *s, struct audio_driver *drv) } static void audio_vm_change_state_handler (void *opaque, int running, - int reason) + QemuState state) { AudioState *s = opaque; HWVoiceOut *hwo = NULL; diff --git a/cpus.c b/cpus.c index 6bf4e3f..ebbb8b9 100644 --- a/cpus.c +++ b/cpus.c @@ -118,13 +118,13 @@ int cpu_is_stopped(CPUState *env) return !vm_running || env->stopped; } -static void do_vm_stop(int reason) +static void do_vm_stop(QemuState state) { if (vm_running) { cpu_disable_ticks(); vm_running = 0; pause_all_vcpus(); - vm_state_notify(0, reason); + vm_state_notify(0, state); qemu_aio_flush(); bdrv_flush_all(); monitor_protocol_event(QEVENT_STOP, NULL); @@ -628,9 +628,9 @@ void cpu_stop_current(void) { } -void vm_stop(int reason) +void vm_stop(QemuState state) { - do_vm_stop(reason); + do_vm_stop(state); } #else /* CONFIG_IOTHREAD */ @@ -1022,7 +1022,7 @@ void cpu_stop_current(void) } } -void vm_stop(int reason) +void vm_stop(QemuState state) { if (!qemu_thread_is_self(&io_thread)) { qemu_system_vmstop_request(reason); @@ -1033,7 +1033,7 @@ void vm_stop(int reason) cpu_stop_current(); return; } - do_vm_stop(reason); + do_vm_stop(state); } #endif diff --git a/gdbstub.c b/gdbstub.c index 27b0cfa..ae57d6a 100644 --- a/gdbstub.c +++ b/gdbstub.c @@ -2267,7 +2267,7 @@ void gdb_set_stop_cpu(CPUState *env) } #ifndef CONFIG_USER_ONLY -static void gdb_vm_state_change(void *opaque, int running, int reason) +static void gdb_vm_state_change(void *opaque, int running, QemuState state) { GDBState *s = gdbserver_state; CPUState *env = s->c_cpu; @@ -2278,8 +2278,8 @@ static void gdb_vm_state_change(void *opaque, int running, int reason) if (running || s->state == RS_INACTIVE || s->state == RS_SYSCALL) { return; } - switch (reason) { - case VMSTOP_DEBUG: + switch (state) { + case QSTATE_DEBUG: if (env->watchpoint_hit) { switch (env->watchpoint_hit->flags & BP_MEM_ACCESS) { case BP_MEM_READ: @@ -2302,25 +2302,25 @@ static void gdb_vm_state_change(void *opaque, int running, int reason) tb_flush(env); ret = GDB_SIGNAL_TRAP; break; - case VMSTOP_USER: + case QSTATE_PAUSED: ret = GDB_SIGNAL_INT; break; - case VMSTOP_SHUTDOWN: + case QSTATE_SHUTDOWN: ret = GDB_SIGNAL_QUIT; break; - case VMSTOP_DISKFULL: + case QSTATE_IOERROR: ret = GDB_SIGNAL_IO; break; - case VMSTOP_WATCHDOG: + case QSTATE_WATCHDOG: ret = GDB_SIGNAL_ALRM; break; - case VMSTOP_PANIC: + case QSTATE_INTERROR: ret = GDB_SIGNAL_ABRT; break; - case VMSTOP_SAVEVM: - case VMSTOP_LOADVM: + case QSTATE_SAVEVM: + case QSTATE_RESTVM: return; - case VMSTOP_MIGRATE: + case QSTATE_PREMIGRATE: ret = GDB_SIGNAL_XCPU; break; default: @@ -2357,7 +2357,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(VMSTOP_DEBUG); + vm_stop(QSTATE_DEBUG); #endif s->state = RS_IDLE; va_start(va, fmt); @@ -2431,7 +2431,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(VMSTOP_USER); + vm_stop(QSTATE_PAUSED); } else #endif { @@ -2693,7 +2693,7 @@ static void gdb_chr_event(void *opaque, int event) { switch (event) { case CHR_EVENT_OPENED: - vm_stop(VMSTOP_USER); + vm_stop(QSTATE_PAUSED); gdb_has_xml = 0; break; default: @@ -2734,7 +2734,7 @@ static int gdb_monitor_write(CharDriverState *chr, const uint8_t *buf, int len) static void gdb_sigterm_handler(int signal) { if (vm_running) { - vm_stop(VMSTOP_USER); + vm_stop(QSTATE_PAUSED); } } #endif diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 1f008a3..ec9a727 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -1102,7 +1102,7 @@ static void ahci_irq_set(void *opaque, int n, int level) { } -static void ahci_dma_restart_cb(void *opaque, int running, int reason) +static void ahci_dma_restart_cb(void *opaque, int running, QemuState state) { } diff --git a/hw/ide/core.c b/hw/ide/core.c index d145b19..8c5a52b 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -523,7 +523,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->error_status = op; bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read); - vm_stop(VMSTOP_DISKFULL); + vm_stop(QSTATE_IOERROR); } else { if (op & BM_STATUS_DMA_RETRY) { dma_buf_commit(s, 0); @@ -1815,7 +1815,7 @@ static int ide_nop_int(IDEDMA *dma, int x) return 0; } -static void ide_nop_restart(void *opaque, int x, int y) +static void ide_nop_restart(void *opaque, int x, QemuState y) { } diff --git a/hw/ide/internal.h b/hw/ide/internal.h index 02e805f..fb8e3e5 100644 --- a/hw/ide/internal.h +++ b/hw/ide/internal.h @@ -10,6 +10,7 @@ #include "block_int.h" #include "iorange.h" #include "dma.h" +#include "sysemu.h" /* debug IDE devices */ //#define DEBUG_IDE @@ -379,7 +380,7 @@ typedef void EndTransferFunc(IDEState *); typedef void DMAStartFunc(IDEDMA *, IDEState *, BlockDriverCompletionFunc *); typedef int DMAFunc(IDEDMA *); typedef int DMAIntFunc(IDEDMA *, int); -typedef void DMARestartFunc(void *, int, int); +typedef void DMARestartFunc(void *, int, QemuState); struct unreported_events { bool eject_request; diff --git a/hw/ide/pci.c b/hw/ide/pci.c index 9f3050a..427829e 100644 --- a/hw/ide/pci.c +++ b/hw/ide/pci.c @@ -223,7 +223,7 @@ static void bmdma_restart_bh(void *opaque) } } -static void bmdma_restart_cb(void *opaque, int running, int reason) +static void bmdma_restart_cb(void *opaque, int running, QemuState state) { IDEDMA *dma = opaque; BMDMAState *bm = DO_UPCAST(BMDMAState, dma, dma); diff --git a/hw/kvmclock.c b/hw/kvmclock.c index 692ad18..de5a655 100644 --- a/hw/kvmclock.c +++ b/hw/kvmclock.c @@ -59,7 +59,8 @@ static int kvmclock_post_load(void *opaque, int version_id) return kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data); } -static void kvmclock_vm_state_change(void *opaque, int running, int reason) +static void kvmclock_vm_state_change(void *opaque, int running, + QemuState state) { KVMClockState *s = opaque; diff --git a/hw/qxl.c b/hw/qxl.c index a6fb7f0..98217a6 100644 --- a/hw/qxl.c +++ b/hw/qxl.c @@ -1180,7 +1180,8 @@ static void qxl_hw_text_update(void *opaque, console_ch_t *chardata) } } -static void qxl_vm_change_state_handler(void *opaque, int running, int reason) +static void qxl_vm_change_state_handler(void *opaque, int running, + QemuState state) { PCIQXLDevice *qxl = opaque; qemu_spice_vm_change_state_handler(&qxl->ssd, running, reason); diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c index f42a5d1..643781c 100644 --- a/hw/scsi-disk.c +++ b/hw/scsi-disk.c @@ -215,7 +215,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(VMSTOP_DISKFULL); + vm_stop(QSTATE_IOERROR); } else { if (type == SCSI_REQ_STATUS_RETRY_READ) { scsi_req_data(&r->req, 0); @@ -333,7 +333,7 @@ static void scsi_dma_restart_bh(void *opaque) } } -static void scsi_dma_restart_cb(void *opaque, int running, int reason) +static void scsi_dma_restart_cb(void *opaque, int running, QemuState state) { SCSIDiskState *s = opaque; diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c index 6471ac8..48b6aa7 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(VMSTOP_DISKFULL); + vm_stop(QSTATE_IOERROR); } else { virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR); bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, is_read); @@ -423,7 +423,8 @@ static void virtio_blk_dma_restart_bh(void *opaque) virtio_submit_multiwrite(s->bs, &mrb); } -static void virtio_blk_dma_restart_cb(void *opaque, int running, int reason) +static void virtio_blk_dma_restart_cb(void *opaque, int running, + QemuState state) { VirtIOBlock *s = opaque; diff --git a/hw/virtio.c b/hw/virtio.c index a8f4940..7293db0 100644 --- a/hw/virtio.c +++ b/hw/virtio.c @@ -836,7 +836,7 @@ void virtio_cleanup(VirtIODevice *vdev) qemu_free(vdev->vq); } -static void virtio_vmstate_change(void *opaque, int running, int reason) +static void virtio_vmstate_change(void *opaque, int running, QemuState state) { VirtIODevice *vdev = opaque; bool backend_run = running && (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK); diff --git a/hw/watchdog.c b/hw/watchdog.c index 1c900a1..a3c2d9b 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(VMSTOP_WATCHDOG); + vm_stop(QSTATE_WATCHDOG); break; case WDT_DEBUG: diff --git a/kvm-all.c b/kvm-all.c index cbc2532..4da5fff 100644 --- a/kvm-all.c +++ b/kvm-all.c @@ -1014,7 +1014,7 @@ int kvm_cpu_exec(CPUState *env) if (ret < 0) { cpu_dump_state(env, stderr, fprintf, CPU_DUMP_CODE); - vm_stop(VMSTOP_PANIC); + vm_stop(QSTATE_INTERROR); } env->exit_request = 0; diff --git a/migration.c b/migration.c index 2a15b98..9724ce0 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(VMSTOP_MIGRATE); + vm_stop(QSTATE_PREMIGRATE); if ((qemu_savevm_state_complete(s->mon, s->file)) < 0) { if (old_vm_running) { diff --git a/monitor.c b/monitor.c index 1b8ba2c..6ce0391 100644 --- a/monitor.c +++ b/monitor.c @@ -1291,7 +1291,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_stop(QSTATE_PAUSED); return 0; } @@ -2814,7 +2814,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(VMSTOP_LOADVM); + vm_stop(QSTATE_RESTVM); if (load_vmstate(name) == 0 && saved_vm_running) { vm_start(); diff --git a/qemu-timer.c b/qemu-timer.c index 30e8f12..5dbaaa3 100644 --- a/qemu-timer.c +++ b/qemu-timer.c @@ -1134,7 +1134,8 @@ static void win32_rearm_timer(struct qemu_alarm_timer *t) #endif /* _WIN32 */ -static void alarm_timer_on_change_state_rearm(void *opaque, int running, int reason) +static void alarm_timer_on_change_state_rearm(void *opaque, int running, + QemuState state) { if (running) qemu_rearm_alarm_timer((struct qemu_alarm_timer *) opaque); diff --git a/savevm.c b/savevm.c index 7801aa7..d8db493 100644 --- a/savevm.c +++ b/savevm.c @@ -1603,7 +1603,7 @@ static int qemu_savevm_state(Monitor *mon, QEMUFile *f) int ret; saved_vm_running = vm_running; - vm_stop(VMSTOP_SAVEVM); + vm_stop(QSTATE_SAVEVM); if (qemu_savevm_state_blocked(mon)) { ret = -EINVAL; @@ -1932,7 +1932,7 @@ void do_savevm(Monitor *mon, const QDict *qdict) } saved_vm_running = vm_running; - vm_stop(VMSTOP_SAVEVM); + vm_stop(QSTATE_SAVEVM); memset(sn, 0, sizeof(*sn)); diff --git a/sysemu.h b/sysemu.h index 61ad4fb..2c3ea3d 100644 --- a/sysemu.h +++ b/sysemu.h @@ -9,6 +9,20 @@ #include "notify.h" /* vl.c */ + +typedef enum { + QSTATE_DEBUG, /* qemu is running under gdb */ + QSTATE_INTERROR, /* paused due to an internal error */ + QSTATE_IOERROR, /* paused due to an I/O error */ + QSTATE_PAUSED, /* paused by the user (ie. the 'stop' command) */ + QSTATE_PREMIGRATE, /* paused preparing to finish migrate */ + QSTATE_RESTVM, /* paused restoring the VM state */ + QSTATE_RUNNING, /* qemu is running */ + QSTATE_SAVEVM, /* paused saving VM state */ + QSTATE_SHUTDOWN, /* guest shut down and -no-shutdown is in use */ + QSTATE_WATCHDOG /* watchdog fired and qemu is configured to pause */ +} QemuState; + extern const char *bios_name; extern int vm_running; @@ -18,28 +32,18 @@ int qemu_uuid_parse(const char *str, uint8_t *uuid); #define UUID_FMT "%02hhx%02hhx%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx" typedef struct vm_change_state_entry VMChangeStateEntry; -typedef void VMChangeStateHandler(void *opaque, int running, int reason); +typedef void VMChangeStateHandler(void *opaque, int running, QemuState state); VMChangeStateEntry *qemu_add_vm_change_state_handler(VMChangeStateHandler *cb, void *opaque); void qemu_del_vm_change_state_handler(VMChangeStateEntry *e); -void vm_state_notify(int running, int reason); - -#define VMSTOP_USER 0 -#define VMSTOP_DEBUG 1 -#define VMSTOP_SHUTDOWN 2 -#define VMSTOP_DISKFULL 3 -#define VMSTOP_WATCHDOG 4 -#define VMSTOP_PANIC 5 -#define VMSTOP_SAVEVM 6 -#define VMSTOP_LOADVM 7 -#define VMSTOP_MIGRATE 8 +void vm_state_notify(int running, QemuState state); #define VMRESET_SILENT false #define VMRESET_REPORT true void vm_start(void); -void vm_stop(int reason); +void vm_stop(QemuState state); void qemu_system_reset_request(void); void qemu_system_shutdown_request(void); diff --git a/target-i386/kvm.c b/target-i386/kvm.c index 10fb2c4..c88cd14 100644 --- a/target-i386/kvm.c +++ b/target-i386/kvm.c @@ -334,7 +334,7 @@ static int kvm_inject_mce_oldstyle(CPUState *env) return 0; } -static void cpu_update_state(void *opaque, int running, int reason) +static void cpu_update_state(void *opaque, int running, QemuState state) { CPUState *env = opaque; diff --git a/ui/spice-display.c b/ui/spice-display.c index feeee73..02baeaf 100644 --- a/ui/spice-display.c +++ b/ui/spice-display.c @@ -191,7 +191,8 @@ void qemu_spice_destroy_host_primary(SimpleSpiceDisplay *ssd) ssd->worker->destroy_primary_surface(ssd->worker, 0); } -void qemu_spice_vm_change_state_handler(void *opaque, int running, int reason) +void qemu_spice_vm_change_state_handler(void *opaque, int running, + QemuState state) { SimpleSpiceDisplay *ssd = opaque; diff --git a/vl.c b/vl.c index 426cea7..faa7c5f 100644 --- a/vl.c +++ b/vl.c @@ -1142,14 +1142,14 @@ void qemu_del_vm_change_state_handler(VMChangeStateEntry *e) qemu_free (e); } -void vm_state_notify(int running, int reason) +void vm_state_notify(int running, QemuState state) { VMChangeStateEntry *e; - trace_vm_state_notify(running, reason); + trace_vm_state_notify(running, state); for (e = vm_change_state_head.lh_first; e; e = e->entries.le_next) { - e->cb(e->opaque, running, reason); + e->cb(e->opaque, running, state); } } @@ -1158,7 +1158,7 @@ void vm_start(void) if (!vm_running) { cpu_enable_ticks(); vm_running = 1; - vm_state_notify(1, 0); + vm_state_notify(1, QSTATE_RUNNING); resume_all_vcpus(); monitor_protocol_event(QEVENT_RESUME, NULL); } @@ -1402,13 +1402,13 @@ static void main_loop(void) #endif if (qemu_debug_requested()) { - vm_stop(VMSTOP_DEBUG); + vm_stop(QSTATE_DEBUG); } if (qemu_shutdown_requested()) { qemu_kill_report(); monitor_protocol_event(QEVENT_SHUTDOWN, NULL); if (no_shutdown) { - vm_stop(VMSTOP_SHUTDOWN); + vm_stop(QSTATE_SHUTDOWN); } else break; } diff --git a/xen-all.c b/xen-all.c index 9eaeac1..5d2bc4c 100644 --- a/xen-all.c +++ b/xen-all.c @@ -846,7 +846,8 @@ static void xen_main_loop_prepare(XenIOState *state) /* Initialise Xen */ -static void xen_change_state_handler(void *opaque, int running, int reason) +static void xen_change_state_handler(void *opaque, int running, + QemuState state) { if (running) { /* record state running */ @@ -854,7 +855,8 @@ static void xen_change_state_handler(void *opaque, int running, int reason) } } -static void xen_hvm_change_state_handler(void *opaque, int running, int reason) +static void xen_hvm_change_state_handler(void *opaque, int running, + QemuState state) { XenIOState *state = opaque; if (running) {
Today, when notifying a VM state change with vm_state_notify(), we pass a VMSTOP macro as the 'reason' argument. This is not ideal, because the VMSTOP macros tells why qemu stopped and not exactly what the current VM state is. One example to demonstrate this problem is that vm_start() calls vm_state_notify() with reason=0, which turns out to be VMSTOP_USER. This commit fixes that by replacing the VMSTOP macros with a proper QemuState type. Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> --- audio/audio.c | 2 +- cpus.c | 12 ++++++------ gdbstub.c | 30 +++++++++++++++--------------- hw/ide/ahci.c | 2 +- hw/ide/core.c | 4 ++-- hw/ide/internal.h | 3 ++- hw/ide/pci.c | 2 +- hw/kvmclock.c | 3 ++- hw/qxl.c | 3 ++- hw/scsi-disk.c | 4 ++-- hw/virtio-blk.c | 5 +++-- hw/virtio.c | 2 +- hw/watchdog.c | 2 +- kvm-all.c | 2 +- migration.c | 2 +- monitor.c | 4 ++-- qemu-timer.c | 3 ++- savevm.c | 4 ++-- sysemu.h | 30 +++++++++++++++++------------- target-i386/kvm.c | 2 +- ui/spice-display.c | 3 ++- vl.c | 12 ++++++------ xen-all.c | 6 ++++-- 23 files changed, 77 insertions(+), 65 deletions(-)