[Qemu-devel] [PATCH v2 0/4]: runstate: introduce 'suspended' state

[Qemu-devel] [PATCH v2 0/4]: runstate: introduce 'suspended' state

[Luiz Capitulino]

This is for 1.1.

The first patch is a fix from Gerd, it's included in this series for our
convenience. The patch introducing the new RunState is actually patch 03/04.

V2

- include a related fix from Gerd [Gerd]
- check for RUN_STATE_SUSPENDED in the mouse driver [Gerd]
- small changelog changes [Me]

 input.c          |    4 ++--
 migration.c      |    1 +
 qapi-schema.json |    8 +++++---
 qmp.c            |    2 ++
 vl.c             |   14 +++++++++-----
 5 files changed, 19 insertions(+), 10 deletions(-)

[Qemu-devel] [PATCH 1/4] wakeup on migration

[Luiz Capitulino]

From: Gerd Hoffmann <kraxel@redhat.com>

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>
Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 migration.c |    1 +
 1 file changed, 1 insertion(+)

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

[Qemu-devel] [PATCH 2/4] qapi-schema.json: fix RunState enums alphabetical order

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qapi-schema.json |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 9193fb9..0166ec2 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -92,6 +92,8 @@
 #
 # @debug: QEMU is running on a debugger
 #
+# @finish-migrate: guest is paused to finish the migration process
+#
 # @inmigrate: guest is paused waiting for an incoming migration
 #
 # @internal-error: An internal error that prevents further guest execution
@@ -106,8 +108,6 @@
 #
 # @prelaunch: QEMU was started with -S and guest has not started
 #
-# @finish-migrate: guest is paused to finish the migration process
-#
 # @restore-vm: guest is paused to restore VM state
 #
 # @running: guest is actively running
-- 
1.7.9.2.384.g4a92a

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

[Luiz Capitulino]

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 also has 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. Every
device that doesn't run when the VM is stopped but wants to run
when the VM is suspended has to check for RUN_STATE_SUSPENDED
explicitly. This is the case for the keyboard and mouse devices,
for example.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 input.c          |    4 ++--
 qapi-schema.json |    4 +++-
 qmp.c            |    2 ++
 vl.c             |    7 +++++++
 4 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/input.c b/input.c
index 6b5c2c3..6968b31 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) {
@@ -154,7 +154,7 @@ void kbd_mouse_event(int dx, int dy, int dz, int buttons_state)
     void *mouse_event_opaque;
     int width, height;
 
-    if (!runstate_is_running()) {
+    if (!runstate_is_running() && !runstate_check(RUN_STATE_SUSPENDED)) {
         return;
     }
     if (QTAILQ_EMPTY(&mouse_handlers)) {
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..a7afc79 100644
--- a/vl.c
+++ b/vl.c
@@ -366,6 +366,11 @@ static const RunStateTransition runstate_transitions_def[] = {
     { RUN_STATE_SHUTDOWN, RUN_STATE_PAUSED },
     { RUN_STATE_SHUTDOWN, RUN_STATE_FINISH_MIGRATE },
 
+    { RUN_STATE_DEBUG, RUN_STATE_SUSPENDED },
+    { RUN_STATE_RUNNING, RUN_STATE_SUSPENDED },
+    { RUN_STATE_SUSPENDED, RUN_STATE_RUNNING },
+    { RUN_STATE_SUSPENDED, RUN_STATE_FINISH_MIGRATE },
+
     { RUN_STATE_WATCHDOG, RUN_STATE_RUNNING },
     { RUN_STATE_WATCHDOG, RUN_STATE_FINISH_MIGRATE },
 
@@ -1420,6 +1425,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 +1453,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;
-- 
1.7.9.2.384.g4a92a

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

[Paolo Bonzini]

Il 04/05/2012 17:02, Luiz Capitulino ha scritto:
> 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' ] }
>  

This breaks QAPI ABI.

Not really a breaker for this series, but it shows how we are not yet
ready to keep a stable ABI (as opposed to API), and thus any
restrictions on adding optional arguments to commands are premature.
(And IMO wrong, there are plenty of ways to have versioned symbols in C
without breaking the ABI---not talking about ELF symbol versioning).

Paolo

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

[Luiz Capitulino]

On Fri, 04 May 2012 18:39:06 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 04/05/2012 17:02, Luiz Capitulino ha scritto:
> > 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' ] }
> >  
> 
> This breaks QAPI ABI.
> 
> Not really a breaker for this series, but it shows how we are not yet
> ready to keep a stable ABI (as opposed to API), and thus any

Having to add a new enum every time a new value is needed is going to be fun.

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

[Eric Blake]

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

On 05/04/2012 10:50 AM, Luiz Capitulino wrote:
> On Fri, 04 May 2012 18:39:06 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> Il 04/05/2012 17:02, Luiz Capitulino ha scritto:
>>> 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' ] }
>>>  
>>
>> This breaks QAPI ABI.
>>
>> Not really a breaker for this series, but it shows how we are not yet
>> ready to keep a stable ABI (as opposed to API), and thus any
> 
> Having to add a new enum every time a new value is needed is going to be fun.

I think Paolo's point was that new values should be added at the end of
the list.  Your patch, as written, changes 'watchdog' from 13th to 14th;
what you should have done is left 'watchdog' at 13th and made
'suspended' be 14th.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

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

[Luiz Capitulino]

On Fri, 04 May 2012 11:07:03 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 05/04/2012 10:50 AM, Luiz Capitulino wrote:
> > On Fri, 04 May 2012 18:39:06 +0200
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > 
> >> Il 04/05/2012 17:02, Luiz Capitulino ha scritto:
> >>> 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' ] }
> >>>  
> >>
> >> This breaks QAPI ABI.
> >>
> >> Not really a breaker for this series, but it shows how we are not yet
> >> ready to keep a stable ABI (as opposed to API), and thus any
> > 
> > Having to add a new enum every time a new value is needed is going to be fun.
> 
> I think Paolo's point was that new values should be added at the end of
> the list.  Your patch, as written, changes 'watchdog' from 13th to 14th;
> what you should have done is left 'watchdog' at 13th and made
> 'suspended' be 14th.

We don't have a stable QAPI ABI today, and if I'm not missing the point
here he's advocating against it.

I don't think this series need any changes in that regard.

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

[Paolo Bonzini]

Il 04/05/2012 19:13, Luiz Capitulino ha scritto:
>>>> > >> This breaks QAPI ABI.
>>>> > >>
>>>> > >> Not really a breaker for this series, but it shows how we are not yet
>>>> > >> ready to keep a stable ABI (as opposed to API), and thus any
>>> > > 
>>> > > Having to add a new enum every time a new value is needed is going to be fun.
>> > 
>> > I think Paolo's point was that new values should be added at the end of
>> > the list.  Your patch, as written, changes 'watchdog' from 13th to 14th;
>> > what you should have done is left 'watchdog' at 13th and made
>> > 'suspended' be 14th.
> 
> We don't have a stable QAPI ABI today, and if I'm not missing the point
> here he's advocating against it.

Yes, but Eric's solution would be fine.

> I don't think this series need any changes in that regard.

I agree.

Paolo

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

[Luiz Capitulino]

On Sat, 05 May 2012 09:55:35 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 04/05/2012 19:13, Luiz Capitulino ha scritto:
> >>>> > >> This breaks QAPI ABI.
> >>>> > >>
> >>>> > >> Not really a breaker for this series, but it shows how we are not yet
> >>>> > >> ready to keep a stable ABI (as opposed to API), and thus any
> >>> > > 
> >>> > > Having to add a new enum every time a new value is needed is going to be fun.
> >> > 
> >> > I think Paolo's point was that new values should be added at the end of
> >> > the list.  Your patch, as written, changes 'watchdog' from 13th to 14th;
> >> > what you should have done is left 'watchdog' at 13th and made
> >> > 'suspended' be 14th.
> > 
> > We don't have a stable QAPI ABI today, and if I'm not missing the point
> > here he's advocating against it.
> 
> Yes, but Eric's solution would be fine.

I'm afraid not, we generate a _MAX enum for bound checking. Yet another
argument in favor of your first call.

[Qemu-devel] [PATCH 4/4] vl: prop is_suspended variable

[Luiz Capitulino]

Check for the RUN_STATE_SUSPENDED state instead.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 vl.c |    7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/vl.c b/vl.c
index a7afc79..5e0080b 100644
--- a/vl.c
+++ b/vl.c
@@ -1293,7 +1293,6 @@ static pid_t shutdown_pid;
 static int powerdown_requested;
 static int debug_requested;
 static int suspend_requested;
-static bool is_suspended;
 static NotifierList suspend_notifiers =
     NOTIFIER_LIST_INITIALIZER(suspend_notifiers);
 static NotifierList wakeup_notifiers =
@@ -1427,12 +1426,11 @@ static void qemu_system_suspend(void)
     notifier_list_notify(&suspend_notifiers, NULL);
     runstate_set(RUN_STATE_SUSPENDED);
     monitor_protocol_event(QEVENT_SUSPEND, NULL);
-    is_suspended = true;
 }
 
 void qemu_system_suspend_request(void)
 {
-    if (is_suspended) {
+    if (runstate_check(RUN_STATE_SUSPENDED)) {
         return;
     }
     suspend_requested = 1;
@@ -1447,7 +1445,7 @@ void qemu_register_suspend_notifier(Notifier *notifier)
 
 void qemu_system_wakeup_request(WakeupReason reason)
 {
-    if (!is_suspended) {
+    if (!runstate_check(RUN_STATE_SUSPENDED)) {
         return;
     }
     if (!(wakeup_reason_mask & (1 << reason))) {
@@ -1458,7 +1456,6 @@ void qemu_system_wakeup_request(WakeupReason reason)
     notifier_list_notify(&wakeup_notifiers, &reason);
     reset_requested = 1;
     qemu_notify_event();
-    is_suspended = false;
 }
 
 void qemu_system_wakeup_enable(WakeupReason reason, bool enabled)
-- 
1.7.9.2.384.g4a92a

Re: [Qemu-devel] [PATCH 4/4] vl: prop is_suspended variable

[Markus Armbruster]

"prop is_suspended variable": do you mean "drop"?

Re: [Qemu-devel] [PATCH 4/4] vl: prop is_suspended variable

[Luiz Capitulino]

On Mon, 07 May 2012 09:02:29 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> "prop is_suspended variable": do you mean "drop"?

Hm, yes, will respin. Thanks!

[Qemu-devel] [PATCH v3 0/4]: runstate: introduce 'suspended' state

[Luiz Capitulino]

The first patch is a fix from Gerd, it's included in this series for our
convenience. The patch introducing the new RunState is actually patch 03/04.

This is for 1.1.

V3

- small log typo fix [Markus]

 input.c          |    4 ++--
 migration.c      |    1 +
 qapi-schema.json |    8 +++++---
 qmp.c            |    2 ++
 vl.c             |   14 +++++++++-----
 5 files changed, 19 insertions(+), 10 deletions(-)

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

[Luiz Capitulino]

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 also has 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. Every
device that doesn't run when the VM is stopped but wants to run
when the VM is suspended has to check for RUN_STATE_SUSPENDED
explicitly. This is the case for the keyboard and mouse devices,
for example.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 input.c          |    4 ++--
 qapi-schema.json |    4 +++-
 qmp.c            |    2 ++
 vl.c             |    7 +++++++
 4 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/input.c b/input.c
index 6b5c2c3..6968b31 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) {
@@ -154,7 +154,7 @@ void kbd_mouse_event(int dx, int dy, int dz, int buttons_state)
     void *mouse_event_opaque;
     int width, height;
 
-    if (!runstate_is_running()) {
+    if (!runstate_is_running() && !runstate_check(RUN_STATE_SUSPENDED)) {
         return;
     }
     if (QTAILQ_EMPTY(&mouse_handlers)) {
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..a7afc79 100644
--- a/vl.c
+++ b/vl.c
@@ -366,6 +366,11 @@ static const RunStateTransition runstate_transitions_def[] = {
     { RUN_STATE_SHUTDOWN, RUN_STATE_PAUSED },
     { RUN_STATE_SHUTDOWN, RUN_STATE_FINISH_MIGRATE },
 
+    { RUN_STATE_DEBUG, RUN_STATE_SUSPENDED },
+    { RUN_STATE_RUNNING, RUN_STATE_SUSPENDED },
+    { RUN_STATE_SUSPENDED, RUN_STATE_RUNNING },
+    { RUN_STATE_SUSPENDED, RUN_STATE_FINISH_MIGRATE },
+
     { RUN_STATE_WATCHDOG, RUN_STATE_RUNNING },
     { RUN_STATE_WATCHDOG, RUN_STATE_FINISH_MIGRATE },
 
@@ -1420,6 +1425,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 +1453,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;
-- 
1.7.9.2.384.g4a92a