Re: [Qemu-devel] [PATCH 2/3] runstate: introduce suspended state

Re: [Qemu-devel] [PATCH 2/3] runstate: introduce suspended state

[Gerd Hoffmann]

> 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

Re: [Qemu-devel] [PATCH 2/3] runstate: introduce suspended state

[Luiz Capitulino]

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.

Re: [Qemu-devel] [PATCH 2/3] runstate: introduce suspended state

[Gerd Hoffmann]

  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

Re: [Qemu-devel] [PATCH 2/3] runstate: introduce suspended state

[Luiz Capitulino]

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?

Re: [Qemu-devel] [PATCH 2/3] runstate: introduce suspended state

[Gerd Hoffmann]

  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

Re: [Qemu-devel] [PATCH 2/3] runstate: introduce suspended state

[Luiz Capitulino]

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.

Re: [Qemu-devel] [PATCH 2/3] runstate: introduce suspended state

[Gerd Hoffmann]

[-- Attachment #1: Type: text/plain, Size: 652 bytes --]

  Hi,

> 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.

Oh, I wasn't aware that a stopped vm behaves the same way.  So resuming
on migration is sort-of bug compatible then.  Patch attached.  Will you
add it to your series or should I submit it separately?

cheers,
  Gerd

[-- Attachment #2: 0001-wakeup-on-migration.patch --]
[-- Type: text/plain, Size: 930 bytes --]

>From 566ec2adacfa570e9d08067e242a6b7224c46c8e Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Wed, 7 Mar 2012 08:00:26 +0100
Subject: [PATCH] wakeup on migration

Wakeup the guest when the live part of the migation is finished.
This avoids being in suspended state on migration, so we don't
have to save the is_suspended bit.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 migration.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/migration.c b/migration.c
index 94f7839..f9e968e 100644
--- a/migration.c
+++ b/migration.c
@@ -252,6 +252,7 @@ static void migrate_fd_put_ready(void *opaque)
         int old_vm_running = runstate_is_running();
 
         DPRINTF("done iterating\n");
+        qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
         vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
 
         if (qemu_savevm_state_complete(s->file) < 0) {
-- 
1.7.1

Re: [Qemu-devel] [PATCH 2/3] runstate: introduce suspended state

[Luiz Capitulino]

On Fri, 04 May 2012 15:54:26 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> > 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.
> 
> Oh, I wasn't aware that a stopped vm behaves the same way.  So resuming
> on migration is sort-of bug compatible then.  Patch attached.  Will you
> add it to your series or should I submit it separately?

I can add it to my series, thanks Gerd!