| Message ID | 1335559216-13849-3-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
> diff --git a/input.c b/input.c > index 6b5c2c3..47e6900 100644 > --- a/input.c > +++ b/input.c > @@ -130,7 +130,7 @@ void qemu_remove_led_event_handler(QEMUPutLEDEntry *entry) > > void kbd_put_keycode(int keycode) > { > - if (!runstate_is_running()) { > + if (!runstate_is_running() && !runstate_check(RUN_STATE_SUSPENDED)) { > return; > } > if (qemu_put_kbd_event) { IIRC there is a simliar check for the mouse ... Does it make sense to add a runstate_is_running_or_suspended() function? Overall the series looks good to me. cheers, Gerd
On Wed, 02 May 2012 09:08:55 +0200 Gerd Hoffmann <kraxel@redhat.com> wrote: > > diff --git a/input.c b/input.c > > index 6b5c2c3..47e6900 100644 > > --- a/input.c > > +++ b/input.c > > @@ -130,7 +130,7 @@ void qemu_remove_led_event_handler(QEMUPutLEDEntry *entry) > > > > void kbd_put_keycode(int keycode) > > { > > - if (!runstate_is_running()) { > > + if (!runstate_is_running() && !runstate_check(RUN_STATE_SUSPENDED)) { > > return; > > } > > if (qemu_put_kbd_event) { > > IIRC there is a simliar check for the mouse ... Will add. > Does it make sense to add a runstate_is_running_or_suspended() function? I think that the question we have to answer is: apart from the keyboard and mouse, is there any device that wants to run while qemu is suspended? If this is true only for the keyboard and mouse, then having the above check is fine. Now, if this is the case for several devices then we might need a different solution, as this patch will brake them.
Hi, > I think that the question we have to answer is: apart from the keyboard > and mouse, is there any device that wants to run while qemu is suspended? pretty much anything which may wake up the guest. The nics for example for wake-on-lan. I'm not sure whenever they care about the runstate at all though. Mouse and keyboard ignore events when the guest is stopped so if you type into a vnc client for a stopped guest the events wouldn't get queued up and cause unwanted effects when unpausing the guest. When suspended we want forward the events though so wakeup-by-keyboard works. Not sure whenever we have simliar logic elsewhere (like stop queuing network packets when the guest doesn't run). cheers, Gerd
On Thu, 03 May 2012 09:56:52 +0200 Gerd Hoffmann <kraxel@redhat.com> wrote: > Hi, > > > I think that the question we have to answer is: apart from the keyboard > > and mouse, is there any device that wants to run while qemu is suspended? > > pretty much anything which may wake up the guest. The nics for example > for wake-on-lan. I'm not sure whenever they care about the runstate at > all though. > > Mouse and keyboard ignore events when the guest is stopped so if you > type into a vnc client for a stopped guest the events wouldn't get > queued up and cause unwanted effects when unpausing the guest. When > suspended we want forward the events though so wakeup-by-keyboard works. > > Not sure whenever we have simliar logic elsewhere (like stop queuing > network packets when the guest doesn't run). I've quickly checked this and it seems we don't. However, I've ran into a different issue today: migrating while suspended doesn't work. The target VM seems to be locked into S3, it just doesn't resume. Haven't investigated yet, but this is expected to work, right?
Hi, > However, I've ran into a different issue today: migrating while suspended > doesn't work. The target VM seems to be locked into S3, it just doesn't resume. > > Haven't investigated yet, but this is expected to work, right? Well, that one is still on the todo list. There is the temporary stopgap to just resume the machine before migration so we don't have to migrate the is_suspended bit (not merged). I plan to fix that properly, it needs some discussion to figure a sane way as we don't have a vmstate section for global state like this where we could attach a subsection to. That didn't happen yet though ... cheers, Gerd
On Fri, 04 May 2012 10:50:08 +0200 Gerd Hoffmann <kraxel@redhat.com> wrote: > Hi, > > > However, I've ran into a different issue today: migrating while suspended > > doesn't work. The target VM seems to be locked into S3, it just doesn't resume. > > > > Haven't investigated yet, but this is expected to work, right? > > Well, that one is still on the todo list. There is the temporary > stopgap to just resume the machine before migration so we don't have to > migrate the is_suspended bit (not merged). Ok. > I plan to fix that properly, it needs some discussion to figure a sane > way as we don't have a vmstate section for global state like this where > we could attach a subsection to. That didn't happen yet though ... There's a quite simple and stupid way to do it. When we migrate an stopped VM, it's automatically resumed on the target. We could do the same for a suspended VM: we could automatically resume it before migrating, as you said above. That's not the behavior I'd expect, I mean I would expect a stopped VM to remain stopped after it's migrated. But it has always been this way and I'm afraid we can't change this.
diff --git a/input.c b/input.c index 6b5c2c3..47e6900 100644 --- a/input.c +++ b/input.c @@ -130,7 +130,7 @@ void qemu_remove_led_event_handler(QEMUPutLEDEntry *entry) void kbd_put_keycode(int keycode) { - if (!runstate_is_running()) { + if (!runstate_is_running() && !runstate_check(RUN_STATE_SUSPENDED)) { return; } if (qemu_put_kbd_event) { diff --git a/qapi-schema.json b/qapi-schema.json index 0166ec2..4dbcb26 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -116,12 +116,14 @@ # # @shutdown: guest is shut down (and -no-shutdown is in use) # +# @suspended: guest is suspended (ACPI S3) +# # @watchdog: the watchdog action is configured to pause and has been triggered ## { 'enum': 'RunState', 'data': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused', 'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm', - 'running', 'save-vm', 'shutdown', 'watchdog' ] } + 'running', 'save-vm', 'shutdown', 'suspended', 'watchdog' ] } ## # @StatusInfo: diff --git a/qmp.c b/qmp.c index a182b51..fee9fb2 100644 --- a/qmp.c +++ b/qmp.c @@ -151,6 +151,8 @@ void qmp_cont(Error **errp) runstate_check(RUN_STATE_SHUTDOWN)) { error_set(errp, QERR_RESET_REQUIRED); return; + } else if (runstate_check(RUN_STATE_SUSPENDED)) { + return; } bdrv_iterate(iostatus_bdrv_it, NULL); diff --git a/vl.c b/vl.c index ae91a8a..2d828c8 100644 --- a/vl.c +++ b/vl.c @@ -366,6 +366,10 @@ static const RunStateTransition runstate_transitions_def[] = { { RUN_STATE_SHUTDOWN, RUN_STATE_PAUSED }, { RUN_STATE_SHUTDOWN, RUN_STATE_FINISH_MIGRATE }, + { RUN_STATE_RUNNING, RUN_STATE_SUSPENDED }, + { RUN_STATE_DEBUG, RUN_STATE_SUSPENDED }, + { RUN_STATE_SUSPENDED, RUN_STATE_RUNNING }, + { RUN_STATE_WATCHDOG, RUN_STATE_RUNNING }, { RUN_STATE_WATCHDOG, RUN_STATE_FINISH_MIGRATE }, @@ -1420,6 +1424,7 @@ static void qemu_system_suspend(void) { pause_all_vcpus(); notifier_list_notify(&suspend_notifiers, NULL); + runstate_set(RUN_STATE_SUSPENDED); monitor_protocol_event(QEVENT_SUSPEND, NULL); is_suspended = true; } @@ -1447,6 +1452,7 @@ void qemu_system_wakeup_request(WakeupReason reason) if (!(wakeup_reason_mask & (1 << reason))) { return; } + runstate_set(RUN_STATE_RUNNING); monitor_protocol_event(QEVENT_WAKEUP, NULL); notifier_list_notify(&wakeup_notifiers, &reason); reset_requested = 1;
QEMU enters in this state when the guest suspends to ram (S3). This is important so that HMP users and QMP clients can know that the guest is suspended. QMP clients also have an event for this, but events are not reliable and are limited (ie. a client can connect to QEMU after the event has been emitted). Having a different state for S3 brings a new issue, though. Currently, when the guest suspends QEMU is kept in RUN_STATE_RUNNING state. This has two implications: 1. We report to the HMP user or QMP client that the guest is 'running', which is not true 2. QEMU doesn't know the guest entered S3, this means that some parts of QEMU (eg. the input hardware) keeps working normally during S3 With this patch, item 1 is fixed, but item 2 becomes an issue because now QEMU is not 'running' anymore and this can confuse the drivers that are expected to keep working during suspend. There are two solutions to this problem: 1. Define that RUN_STATE_SUSPENDED is actually a "running" state. This means that runstate_is_running() will return true when the guest is in S3 2. Keep RUN_STATE_SUSPENDED as a paused state and hard-code checks for RUN_STATE_SUSPENDED for the parts of QEMU that want to run normally when the guest is in S3 This patch does 2 because it fits better in RunState's design. Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> --- input.c | 2 +- qapi-schema.json | 4 +++- qmp.c | 2 ++ vl.c | 6 ++++++ 4 files changed, 12 insertions(+), 2 deletions(-)