[Qemu-devel] [PATCH v7 0/3] wakeup-from-suspend and system_wakeup changes

[Qemu-devel] [PATCH v7 0/3] wakeup-from-suspend and system_wakeup changes

[Daniel Henrique Barboza]

This is the follow-up version of the series:

[PATCH v6 0/2] qmp: 'wakeup-suspend-support' in query-target

With an extra patch that was standalone:

[PATCH v3 1/1] qmp.c: system_wakeup: adding RUN_STATE_SUSPENDED
check before proceeding

Changes from v6 version:
- patch 1:
    * renamed the method to qemu_wakeup_suspend_support;
    * added an extra wing in a comment;
    * changed the documentation of system_wakeup to mention the
new flag 'wakeup-suspend-support'.

- patch 2:
    * shortened the documentation of both guest-suspend-ram and
guest-suspend-hybrid.

- patch 3:
    * added a verification for wakeup-from-suspend in
qmp_system_wakeup with an exclusive error message if the guest
does not have wake-up support;
    * updated system_wakeup documentation to mention that now
we're not silently failing anymore.


Daniel Henrique Barboza (3):
  qmp: adding 'wakeup-suspend-support' in query-target
  qga: update guest-suspend-ram and guest-suspend-hybrid descriptions
  qmp.c: system_wakeup: runstate and wake-up support check

 arch_init.c             |  1 +
 hmp.c                   |  4 +++-
 include/sysemu/sysemu.h |  1 +
 qapi/misc.json          |  9 +++++++--
 qga/qapi-schema.json    | 12 ++++++------
 qmp.c                   | 10 ++++++++++
 vl.c                    | 22 ++++++++++++++++++++++
 7 files changed, 50 insertions(+), 9 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target

[Daniel Henrique Barboza]

When issuing the qmp/hmp 'system_wakeup' command, what happens in a
nutshell is:

- qmp_system_wakeup_request set runstate to RUNNING, sets a wakeup_reason
and notify the event
- in the main_loop, all vcpus are paused, a system reset is issued, all
subscribers of wakeup_notifiers receives a notification, vcpus are then
resumed and the wake up QAPI event is fired

Note that this procedure alone doesn't ensure that the guest will awake
from SUSPENDED state - the subscribers of the wake up event must take
action to resume the guest, otherwise the guest will simply reboot.

At this moment there are only two subscribers of the wake up event: one
in hw/acpi/core.c and another one in hw/i386/xen/xen-hvm.c. This means
that system_wakeup does not work as intended with other architectures.

However, only the presence of 'system_wakeup' is required for QGA to
support 'guest-suspend-ram' and 'guest-suspend-hybrid' at this moment.
This means that the user/management will expect to suspend the guest using
one of those suspend commands and then resume execution using system_wakeup,
regardless of the support offered in system_wakeup in the first place.

This patch adds a new flag called 'wakeup-suspend-support' in TargetInfo
that allows the caller to query if the guest supports wake up from
suspend via system_wakeup. It goes over the subscribers of the wake up
event and, if it's empty, it assumes that the guest does not support
wake up from suspend (and thus, pm-suspend itself).

This is the expected output of query-target when running a x86 guest:

{"execute" : "query-target"}
{"return": {"arch": "x86_64", "wakeup-suspend-support": true}}

This is the output when running a pseries guest:

{"execute" : "query-target"}
{"return": {"arch": "ppc64", "wakeup-suspend-support": false}}

Given that the TargetInfo structure is read-only, adding a new flag to
it is backwards compatible. There is no need to deprecate the old
TargetInfo format.

With this extra tool, management can avoid situations where a guest
that does not have proper suspend/wake capabilities ends up in
inconsistent state (e.g.
https://github.com/open-power-host-os/qemu/issues/31).

Reported-by: Balamuruhan S <bala24@linux.vnet.ibm.com>
Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
---
 arch_init.c             |  1 +
 include/sysemu/sysemu.h |  1 +
 qapi/misc.json          |  8 ++++++--
 vl.c                    | 22 ++++++++++++++++++++++
 4 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 9597218ced..fa5407e2de 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -115,6 +115,7 @@ TargetInfo *qmp_query_target(Error **errp)
 
     info->arch = qapi_enum_parse(&SysEmuTarget_lookup, TARGET_NAME, -1,
                                  &error_abort);
+    info->wakeup_suspend_support = qemu_wakeup_suspend_support();
 
     return info;
 }
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 544ab77a2b..b987e61d76 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -79,6 +79,7 @@ void qemu_system_debug_request(void);
 void qemu_system_vmstop_request(RunState reason);
 void qemu_system_vmstop_request_prepare(void);
 bool qemu_vmstop_requested(RunState *r);
+bool qemu_wakeup_suspend_support(void);
 ShutdownCause qemu_shutdown_requested_get(void);
 ShutdownCause qemu_reset_requested_get(void);
 void qemu_system_killed(int signal, pid_t pid);
diff --git a/qapi/misc.json b/qapi/misc.json
index f5988cc0b5..efba0449a6 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -1245,7 +1245,9 @@
 ##
 # @system_wakeup:
 #
-# Wakeup guest from suspend.  Does nothing in case the guest isn't suspended.
+# If the guest has wake-up from suspend support enabled
+# (wakeup-suspend-support flag from query-target), wakeup guest from
+# suspend. Does nothing otherwise.
 #
 # Since:  1.1
 #
@@ -2484,11 +2486,13 @@
 # Information describing the QEMU target.
 #
 # @arch: the target architecture
+# @wakeup-suspend-support: true if the target supports wake up from
+#                          suspend (since 2.13)
 #
 # Since: 1.2.0
 ##
 { 'struct': 'TargetInfo',
-  'data': { 'arch': 'SysEmuTarget' } }
+  'data': { 'arch': 'SysEmuTarget', 'wakeup-suspend-support': 'bool' } }
 
 ##
 # @query-target:
diff --git a/vl.c b/vl.c
index 3b39bbd7a8..b8fe41860a 100644
--- a/vl.c
+++ b/vl.c
@@ -1832,11 +1832,33 @@ void qemu_system_wakeup_enable(WakeupReason reason, bool enabled)
     }
 }
 
+/*
+ * The existence of a wake-up notifier is being checked in the function
+ * qemu_wakeup_suspend_support and it's used in the logic of the
+ * wakeup-suspend-support flag of QMP 'query-target' command. The idea
+ * of this flag is to indicate whether the guest supports wake-up from
+ * suspend (via system_wakeup QMP/HMP call for example), warning the user
+ * that the guest can't handle both wake-up from suspend and the suspend
+ * itself via QGA guest-suspend-ram and guest-suspend-hybrid (if it
+ * can't wake up, it can't be suspended safely).
+ *
+ * An assumption is made by the wakeup-suspend-support flag that only the
+ * guests that can go to RUN_STATE_SUSPENDED and wake up properly would
+ * be interested in this wakeup_notifier. Adding a wakeup_notifier for
+ * any other reason will break the logic of the wakeup-suspend-support
+ * flag and can lead to user/management confusion about the suspend/wake-up
+ * support of the guest.
+ */
 void qemu_register_wakeup_notifier(Notifier *notifier)
 {
     notifier_list_add(&wakeup_notifiers, notifier);
 }
 
+bool qemu_wakeup_suspend_support(void)
+{
+    return !QLIST_EMPTY(&wakeup_notifiers.notifiers);
+}
+
 void qemu_system_killed(int signal, pid_t pid)
 {
     shutdown_signal = signal;
-- 
2.14.3

Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target

[Markus Armbruster]

Cc'ing a few more people.

Daniel Henrique Barboza <danielhb@linux.ibm.com> writes:

> When issuing the qmp/hmp 'system_wakeup' command, what happens in a
> nutshell is:
>
> - qmp_system_wakeup_request set runstate to RUNNING, sets a wakeup_reason
> and notify the event
> - in the main_loop, all vcpus are paused, a system reset is issued, all
> subscribers of wakeup_notifiers receives a notification, vcpus are then
> resumed and the wake up QAPI event is fired
>
> Note that this procedure alone doesn't ensure that the guest will awake
> from SUSPENDED state - the subscribers of the wake up event must take
> action to resume the guest, otherwise the guest will simply reboot.
>
> At this moment there are only two subscribers of the wake up event: one
> in hw/acpi/core.c and another one in hw/i386/xen/xen-hvm.c. This means
> that system_wakeup does not work as intended with other architectures.
>
> However, only the presence of 'system_wakeup' is required for QGA to
> support 'guest-suspend-ram' and 'guest-suspend-hybrid' at this moment.
> This means that the user/management will expect to suspend the guest using
> one of those suspend commands and then resume execution using system_wakeup,
> regardless of the support offered in system_wakeup in the first place.
>
> This patch adds a new flag called 'wakeup-suspend-support' in TargetInfo
> that allows the caller to query if the guest supports wake up from
> suspend via system_wakeup. It goes over the subscribers of the wake up
> event and, if it's empty, it assumes that the guest does not support
> wake up from suspend (and thus, pm-suspend itself).
>
> This is the expected output of query-target when running a x86 guest:
>
> {"execute" : "query-target"}
> {"return": {"arch": "x86_64", "wakeup-suspend-support": true}}
>
> This is the output when running a pseries guest:
>
> {"execute" : "query-target"}
> {"return": {"arch": "ppc64", "wakeup-suspend-support": false}}
>
> Given that the TargetInfo structure is read-only, adding a new flag to
> it is backwards compatible. There is no need to deprecate the old
> TargetInfo format.
>
> With this extra tool, management can avoid situations where a guest
> that does not have proper suspend/wake capabilities ends up in
> inconsistent state (e.g.
> https://github.com/open-power-host-os/qemu/issues/31).
>
> Reported-by: Balamuruhan S <bala24@linux.vnet.ibm.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>

Is query-target is the right place to carry this flag?  v7 is rather
late for this kind of question; my sincere apologies.

The flag is true after qemu_register_wakeup_notifier().  Callers so far:

* piix4_pm_realize() via acpi_pm1_cnt_init()

  This is the realize method of device PIIX4_PM.  It's an optional
  onboard device (suppressed by -no-acpi) of machine types
  pc-i440fx-VERSION, pc-VERSION, malta.

* pc_q35_init() via ich9_lpc_pm_init(), ich9_pm_init(),
  acpi_pm1_cnt_init()

  This is the initialization method of machine types pc-q35-VERSION.
  Note that -no-acpi is not honored.

* vt82c686b_pm_realize() via acpi_pm1_cnt_init()

  This is the realize method of device VT82C686B_PM.  It's an onboard
  device of machine type fulong2e.  Again, -no-acpi is not honored.
 
* xen_hvm_init()

  This one gets called with -accel xen.  I suspect the actual callback
  xen_wakeup_notifier() doesn't actually make wakeup work, unlike the
  acpi_notify_wakeup() callback registered by the other callers.

  Issue#1: this calls into question your assumption that the existence
  of a wake-up notifier implies wake-up works.  It still holds if
  --accel xen is only accepted together with other configuration that
  triggers registration of acpi_notify_wakeup().  Is it?  Stefano,
  Anthony?

Issue#2: the flag isn't a property of the target.  Due to -no-acpi, it's
not even a property of the machine type.  If it was, query-machines
would be the natural owner of the flag.

Perhaps query-machines is still the proper owner.  The value of
wakeup-suspend-support would have to depend on -no-acpi for the machine
types that honor it.  Not ideal; I'd prefer MachineInfo to be static.
Tolerable?  I guess that's also a libvirt question.

Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target

[Eduardo Habkost]

On Fri, May 18, 2018 at 10:48:31AM +0200, Markus Armbruster wrote:
> Cc'ing a few more people.
> 
> Daniel Henrique Barboza <danielhb@linux.ibm.com> writes:
> 
> > When issuing the qmp/hmp 'system_wakeup' command, what happens in a
> > nutshell is:
> >
> > - qmp_system_wakeup_request set runstate to RUNNING, sets a wakeup_reason
> > and notify the event
> > - in the main_loop, all vcpus are paused, a system reset is issued, all
> > subscribers of wakeup_notifiers receives a notification, vcpus are then
> > resumed and the wake up QAPI event is fired
> >
> > Note that this procedure alone doesn't ensure that the guest will awake
> > from SUSPENDED state - the subscribers of the wake up event must take
> > action to resume the guest, otherwise the guest will simply reboot.
> >
> > At this moment there are only two subscribers of the wake up event: one
> > in hw/acpi/core.c and another one in hw/i386/xen/xen-hvm.c. This means
> > that system_wakeup does not work as intended with other architectures.
> >
> > However, only the presence of 'system_wakeup' is required for QGA to
> > support 'guest-suspend-ram' and 'guest-suspend-hybrid' at this moment.
> > This means that the user/management will expect to suspend the guest using
> > one of those suspend commands and then resume execution using system_wakeup,
> > regardless of the support offered in system_wakeup in the first place.
> >
> > This patch adds a new flag called 'wakeup-suspend-support' in TargetInfo
> > that allows the caller to query if the guest supports wake up from
> > suspend via system_wakeup. It goes over the subscribers of the wake up
> > event and, if it's empty, it assumes that the guest does not support
> > wake up from suspend (and thus, pm-suspend itself).
> >
> > This is the expected output of query-target when running a x86 guest:
> >
> > {"execute" : "query-target"}
> > {"return": {"arch": "x86_64", "wakeup-suspend-support": true}}
> >
> > This is the output when running a pseries guest:
> >
> > {"execute" : "query-target"}
> > {"return": {"arch": "ppc64", "wakeup-suspend-support": false}}
> >
> > Given that the TargetInfo structure is read-only, adding a new flag to
> > it is backwards compatible. There is no need to deprecate the old
> > TargetInfo format.
> >
> > With this extra tool, management can avoid situations where a guest
> > that does not have proper suspend/wake capabilities ends up in
> > inconsistent state (e.g.
> > https://github.com/open-power-host-os/qemu/issues/31).
> >
> > Reported-by: Balamuruhan S <bala24@linux.vnet.ibm.com>
> > Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
> 
> Is query-target is the right place to carry this flag?  v7 is rather
> late for this kind of question; my sincere apologies.
[...]
> 
> Issue#2: the flag isn't a property of the target.  Due to -no-acpi, it's
> not even a property of the machine type.  If it was, query-machines
> would be the natural owner of the flag.
> 
> Perhaps query-machines is still the proper owner.  The value of
> wakeup-suspend-support would have to depend on -no-acpi for the machine
> types that honor it.  Not ideal; I'd prefer MachineInfo to be static.
> Tolerable?  I guess that's also a libvirt question.

It depends when libvirt is going to query it.  Is it OK to only
query it after the VM is already up and running?  If it is, then
we can simply expose it as a read-only property of the machine
object.

Or, if we don't want to rely on qom-get as a stable API, we can
add a new query command (query-machine? query-power-management?)

-- 
Eduardo

Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target

[Daniel Henrique Barboza]

On 05/21/2018 03:14 PM, Eduardo Habkost wrote:
>> Issue#2: the flag isn't a property of the target.  Due to -no-acpi, it's
>> not even a property of the machine type.  If it was, query-machines
>> would be the natural owner of the flag.
>>
>> Perhaps query-machines is still the proper owner.  The value of
>> wakeup-suspend-support would have to depend on -no-acpi for the machine
>> types that honor it.  Not ideal; I'd prefer MachineInfo to be static.
>> Tolerable?  I guess that's also a libvirt question.
> It depends when libvirt is going to query it.  Is it OK to only
> query it after the VM is already up and running?  If it is, then
> we can simply expose it as a read-only property of the machine
> object.
>
> Or, if we don't want to rely on qom-get as a stable API, we can
> add a new query command (query-machine? query-power-management?)
>
In the first version this logic was included in a new query command called
"query-wakeup-from-suspend-support":

https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg00889.html

In that review it was suggested that this logic could be a flag in 
either query-target
or query-machines API. Before sending the v2 I sent the following comment:

"After investigating, I think that it's simpler to hook the wakeup 
support info into
TargetInfo than MachineInfo, given that the detection I'm using for this 
new property
is based on the current runtime state. Hooking into MachineInfo would 
require to
change the MachineClass to add a new property, then setting it up for 
the machines
that have the wakeup support (only x86 so far). Definitely doable, but 
if we don't
have any favorites between MachineInfo and TargetInfo I'd rather pick 
the simpler
route.

So, if no one objects, I'll rework this series by putting the logic 
inside query-target
instead of a new API."

Since no objection was made back then, this logic was put into 
query-target starting
in v2. Still, I don't have any favorites though: query-target looks ok, 
query-machine
looks ok and a new API looks ok too. It's all about what makes (more) 
sense in the
management level, I think.


danielhb

Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target

[Eduardo Habkost]

On Mon, May 21, 2018 at 04:46:36PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 05/21/2018 03:14 PM, Eduardo Habkost wrote:
> > > Issue#2: the flag isn't a property of the target.  Due to -no-acpi, it's
> > > not even a property of the machine type.  If it was, query-machines
> > > would be the natural owner of the flag.
> > > 
> > > Perhaps query-machines is still the proper owner.  The value of
> > > wakeup-suspend-support would have to depend on -no-acpi for the machine
> > > types that honor it.  Not ideal; I'd prefer MachineInfo to be static.
> > > Tolerable?  I guess that's also a libvirt question.
> > It depends when libvirt is going to query it.  Is it OK to only
> > query it after the VM is already up and running?  If it is, then
> > we can simply expose it as a read-only property of the machine
> > object.
> > 
> > Or, if we don't want to rely on qom-get as a stable API, we can
> > add a new query command (query-machine? query-power-management?)
> > 
> In the first version this logic was included in a new query command called
> "query-wakeup-from-suspend-support":
> 
> https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg00889.html
> 
> In that review it was suggested that this logic could be a flag in either
> query-target
> or query-machines API. Before sending the v2 I sent the following comment:
> 
> "After investigating, I think that it's simpler to hook the wakeup support
> info into
> TargetInfo than MachineInfo, given that the detection I'm using for this new
> property
> is based on the current runtime state. Hooking into MachineInfo would
> require to
> change the MachineClass to add a new property, then setting it up for the
> machines
> that have the wakeup support (only x86 so far). Definitely doable, but if we
> don't
> have any favorites between MachineInfo and TargetInfo I'd rather pick the
> simpler
> route.
> 
> So, if no one objects, I'll rework this series by putting the logic inside
> query-target
> instead of a new API."

Apologies for not noticing this series months ago.  :(


> 
> Since no objection was made back then, this logic was put into query-target
> starting
> in v2. Still, I don't have any favorites though: query-target looks ok,
> query-machine
> looks ok and a new API looks ok too. It's all about what makes (more) sense
> in the
> management level, I think.

I understand the original objection from Eric: having to add a
new command for every runtime flag we want to expose to the user
looks wrong to me.

However, extending query-machines and query-target looks wrong
too, however.  query-target looks wrong because this not a
property of the target.  query-machines is wrong because this is
not a static property of the machine-type, but of the running
machine instance.

Can we have a new query command that could be an obvious
container for simple machine capabilities that are not static?  A
name like "query-machine" would be generic enough for that, I
guess.

Markus, Eric, what do you think?

-- 
Eduardo

Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Mon, May 21, 2018 at 04:46:36PM -0300, Daniel Henrique Barboza wrote:
>> 
>> 
>> On 05/21/2018 03:14 PM, Eduardo Habkost wrote:
>> > > Issue#2: the flag isn't a property of the target.  Due to -no-acpi, it's
>> > > not even a property of the machine type.  If it was, query-machines
>> > > would be the natural owner of the flag.
>> > > 
>> > > Perhaps query-machines is still the proper owner.  The value of
>> > > wakeup-suspend-support would have to depend on -no-acpi for the machine
>> > > types that honor it.  Not ideal; I'd prefer MachineInfo to be static.
>> > > Tolerable?  I guess that's also a libvirt question.
>> > It depends when libvirt is going to query it.  Is it OK to only
>> > query it after the VM is already up and running?  If it is, then
>> > we can simply expose it as a read-only property of the machine
>> > object.
>> > 
>> > Or, if we don't want to rely on qom-get as a stable API, we can
>> > add a new query command (query-machine? query-power-management?)
>> > 
>> In the first version this logic was included in a new query command called
>> "query-wakeup-from-suspend-support":
>> 
>> https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg00889.html
>> 
>> In that review it was suggested that this logic could be a flag in either
>> query-target
>> or query-machines API. Before sending the v2 I sent the following comment:
>> 
>> "After investigating, I think that it's simpler to hook the wakeup support
>> info into
>> TargetInfo than MachineInfo, given that the detection I'm using for this new
>> property
>> is based on the current runtime state. Hooking into MachineInfo would
>> require to
>> change the MachineClass to add a new property, then setting it up for the
>> machines
>> that have the wakeup support (only x86 so far). Definitely doable, but if we
>> don't
>> have any favorites between MachineInfo and TargetInfo I'd rather pick the
>> simpler
>> route.
>> 
>> So, if no one objects, I'll rework this series by putting the logic inside
>> query-target
>> instead of a new API."
>
> Apologies for not noticing this series months ago.  :(

Seconded.  Daniel, this (minor) mess is absolutely not your fault.

>> Since no objection was made back then, this logic was put into query-target
>> starting
>> in v2. Still, I don't have any favorites though: query-target looks ok,
>> query-machine
>> looks ok and a new API looks ok too. It's all about what makes (more) sense
>> in the
>> management level, I think.
>
> I understand the original objection from Eric: having to add a
> new command for every runtime flag we want to expose to the user
> looks wrong to me.

Agreed.

> However, extending query-machines and query-target looks wrong
> too, however.  query-target looks wrong because this not a
> property of the target.  query-machines is wrong because this is
> not a static property of the machine-type, but of the running
> machine instance.

Of the two, query-machines looks less wrong.

Arguably, -no-acpi should not exist.  It's an ad hoc flag that sneakily
splits a few machine types into two variants, with and without ACPI.
It's silently ignored for other machine types, even APCI-capable ones.

If the machine type variants with and without ACPI were separate types,
wakeup-suspend-support would be a static property of the machine type.

However, "separate types" probably doesn't scale: I'm afraid we'd end up
with an undesirable number of machine types.  Avoiding that is exactly
why we have machine types with configurable options.  I suspect that's
how ACPI should be configured (if at all).

So, should we make -no-acpi sugar for a machine type parameter?  And
then deprecate -no-acpi for good measure?

> Can we have a new query command that could be an obvious
> container for simple machine capabilities that are not static?  A
> name like "query-machine" would be generic enough for that, I
> guess.

Having command names differ only in a single letter is awkward, but
let's focus on things other than naming now, and use
query-current-machine like a working title.

query-machines is wrong because wakeup-suspend-support isn't static for
some machine types.

query-current-machine is also kind of wrong because
wakeup-suspend-support *is* static for most machine types.

Worse, a machine type property that is static for all machine types now
could conceivably become dynamic when we add a machine type
configuration knob.

Would a way to tie the property to the configuration knob help?
Something like wakeup-suspend-support taking values true (supported),
false (not supported), and "acpi" (supported if machine type
configuration knob "acpi" is switched on).

> Markus, Eric, what do you think?

Haven't made up my mind, yet :)

Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target

[Eduardo Habkost]

On Wed, May 23, 2018 at 11:17:55AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> > On Mon, May 21, 2018 at 04:46:36PM -0300, Daniel Henrique Barboza wrote:
[...]
> >> Since no objection was made back then, this logic was put into query-target
> >> starting
> >> in v2. Still, I don't have any favorites though: query-target looks ok,
> >> query-machine
> >> looks ok and a new API looks ok too. It's all about what makes (more) sense
> >> in the
> >> management level, I think.
> >
> > I understand the original objection from Eric: having to add a
> > new command for every runtime flag we want to expose to the user
> > looks wrong to me.
> 
> Agreed.
> 
> > However, extending query-machines and query-target looks wrong
> > too, however.  query-target looks wrong because this not a
> > property of the target.  query-machines is wrong because this is
> > not a static property of the machine-type, but of the running
> > machine instance.
> 
> Of the two, query-machines looks less wrong.
> 
> Arguably, -no-acpi should not exist.  It's an ad hoc flag that sneakily
> splits a few machine types into two variants, with and without ACPI.
> It's silently ignored for other machine types, even APCI-capable ones.
> 
> If the machine type variants with and without ACPI were separate types,
> wakeup-suspend-support would be a static property of the machine type.
> 
> However, "separate types" probably doesn't scale: I'm afraid we'd end up
> with an undesirable number of machine types.  Avoiding that is exactly
> why we have machine types with configurable options.  I suspect that's
> how ACPI should be configured (if at all).
> 
> So, should we make -no-acpi sugar for a machine type parameter?  And
> then deprecate -no-acpi for good measure?

I think we should.


> 
> > Can we have a new query command that could be an obvious
> > container for simple machine capabilities that are not static?  A
> > name like "query-machine" would be generic enough for that, I
> > guess.
> 
> Having command names differ only in a single letter is awkward, but
> let's focus on things other than naming now, and use
> query-current-machine like a working title.
> 
> query-machines is wrong because wakeup-suspend-support isn't static for
> some machine types.
> 
> query-current-machine is also kind of wrong because
> wakeup-suspend-support *is* static for most machine types.
> 

The most appropriate solution depends a lot on how/when
management software needs to query this.

If they only need to query it at runtime for a running VM,
there's no reason for us to go of our way and add complexity just
to make it look like static data in query-machines.

On the other hand, if they really need to query it before
configuring/starting a VM, it won't be useful at all to make it
available only at runtime.

Daniel, when/how exactly software would need to query the new
flag?


> Worse, a machine type property that is static for all machine types now
> could conceivably become dynamic when we add a machine type
> configuration knob.
> 

This isn't the first time a machine capability that seems static
actually depends on other configuration arguments.  We will
probably need to address this eventually.


> Would a way to tie the property to the configuration knob help?
> Something like wakeup-suspend-support taking values true (supported),
> false (not supported), and "acpi" (supported if machine type
> configuration knob "acpi" is switched on).
> 

I would prefer a more generic mechanism.  Maybe make
'query-machines' accept a 'machine-options' argument?

-- 
Eduardo

Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target

[Daniel Henrique Barboza]

Hi,

On 05/23/2018 09:27 AM, Eduardo Habkost wrote:
> On Wed, May 23, 2018 at 11:17:55AM +0200, Markus Armbruster wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>>> On Mon, May 21, 2018 at 04:46:36PM -0300, Daniel Henrique Barboza wrote:
> [...]
>>>> Since no objection was made back then, this logic was put into query-target
>>>> starting
>>>> in v2. Still, I don't have any favorites though: query-target looks ok,
>>>> query-machine
>>>> looks ok and a new API looks ok too. It's all about what makes (more) sense
>>>> in the
>>>> management level, I think.
>>> I understand the original objection from Eric: having to add a
>>> new command for every runtime flag we want to expose to the user
>>> looks wrong to me.
>> Agreed.
>>
>>> However, extending query-machines and query-target looks wrong
>>> too, however.  query-target looks wrong because this not a
>>> property of the target.  query-machines is wrong because this is
>>> not a static property of the machine-type, but of the running
>>> machine instance.
>> Of the two, query-machines looks less wrong.
>>
>> Arguably, -no-acpi should not exist.  It's an ad hoc flag that sneakily
>> splits a few machine types into two variants, with and without ACPI.
>> It's silently ignored for other machine types, even APCI-capable ones.
>>
>> If the machine type variants with and without ACPI were separate types,
>> wakeup-suspend-support would be a static property of the machine type.
>>
>> However, "separate types" probably doesn't scale: I'm afraid we'd end up
>> with an undesirable number of machine types.  Avoiding that is exactly
>> why we have machine types with configurable options.  I suspect that's
>> how ACPI should be configured (if at all).
>>
>> So, should we make -no-acpi sugar for a machine type parameter?  And
>> then deprecate -no-acpi for good measure?
> I think we should.
>
>
>>> Can we have a new query command that could be an obvious
>>> container for simple machine capabilities that are not static?  A
>>> name like "query-machine" would be generic enough for that, I
>>> guess.
>> Having command names differ only in a single letter is awkward, but
>> let's focus on things other than naming now, and use
>> query-current-machine like a working title.
>>
>> query-machines is wrong because wakeup-suspend-support isn't static for
>> some machine types.
>>
>> query-current-machine is also kind of wrong because
>> wakeup-suspend-support *is* static for most machine types.
>>
> The most appropriate solution depends a lot on how/when
> management software needs to query this.
>
> If they only need to query it at runtime for a running VM,
> there's no reason for us to go of our way and add complexity just
> to make it look like static data in query-machines.
>
> On the other hand, if they really need to query it before
> configuring/starting a VM, it won't be useful at all to make it
> available only at runtime.
>
> Daniel, when/how exactly software would need to query the new
> flag?

The original idea of this series was to provide a way to inform management
when not to execute a pm-suspend* command. This is a command from the
guest agent, thus it's only available when the guest is already running. 
As far
as I know there is no way to suspend the VM  without using the guest agent.

Thus, unless management wants to store this state to avoid querying it 
multiple
times during the VM lifetime (I remember from Libvirt code that it 
stores some
sort of capabilities of the domain in an internal state, although I 
can't recall if
this info would be eligible for that), there is no need to query this 
until the VM
is booted.




>
>
>> Worse, a machine type property that is static for all machine types now
>> could conceivably become dynamic when we add a machine type
>> configuration knob.
>>
> This isn't the first time a machine capability that seems static
> actually depends on other configuration arguments.  We will
> probably need to address this eventually.
>
>
>> Would a way to tie the property to the configuration knob help?
>> Something like wakeup-suspend-support taking values true (supported),
>> false (not supported), and "acpi" (supported if machine type
>> configuration knob "acpi" is switched on).
>>
> I would prefer a more generic mechanism.  Maybe make
> 'query-machines' accept a 'machine-options' argument?
>

Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Wed, May 23, 2018 at 11:17:55AM +0200, Markus Armbruster wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>> > On Mon, May 21, 2018 at 04:46:36PM -0300, Daniel Henrique Barboza wrote:
> [...]
>> >> Since no objection was made back then, this logic was put into query-target
>> >> starting
>> >> in v2. Still, I don't have any favorites though: query-target looks ok,
>> >> query-machine
>> >> looks ok and a new API looks ok too. It's all about what makes (more) sense
>> >> in the
>> >> management level, I think.
>> >
>> > I understand the original objection from Eric: having to add a
>> > new command for every runtime flag we want to expose to the user
>> > looks wrong to me.
>> 
>> Agreed.
>> 
>> > However, extending query-machines and query-target looks wrong
>> > too, however.  query-target looks wrong because this not a
>> > property of the target.  query-machines is wrong because this is
>> > not a static property of the machine-type, but of the running
>> > machine instance.
>> 
>> Of the two, query-machines looks less wrong.
>> 
>> Arguably, -no-acpi should not exist.  It's an ad hoc flag that sneakily
>> splits a few machine types into two variants, with and without ACPI.
>> It's silently ignored for other machine types, even APCI-capable ones.
>> 
>> If the machine type variants with and without ACPI were separate types,
>> wakeup-suspend-support would be a static property of the machine type.
>> 
>> However, "separate types" probably doesn't scale: I'm afraid we'd end up
>> with an undesirable number of machine types.  Avoiding that is exactly
>> why we have machine types with configurable options.  I suspect that's
>> how ACPI should be configured (if at all).
>> 
>> So, should we make -no-acpi sugar for a machine type parameter?  And
>> then deprecate -no-acpi for good measure?
>
> I think we should.

Would you like to take care of it?

>> > Can we have a new query command that could be an obvious
>> > container for simple machine capabilities that are not static?  A
>> > name like "query-machine" would be generic enough for that, I
>> > guess.
>> 
>> Having command names differ only in a single letter is awkward, but
>> let's focus on things other than naming now, and use
>> query-current-machine like a working title.
>> 
>> query-machines is wrong because wakeup-suspend-support isn't static for
>> some machine types.
>> 
>> query-current-machine is also kind of wrong because
>> wakeup-suspend-support *is* static for most machine types.
>
> The most appropriate solution depends a lot on how/when
> management software needs to query this.

Right.

> If they only need to query it at runtime for a running VM,
> there's no reason for us to go of our way and add complexity just
> to make it look like static data in query-machines.
>
> On the other hand, if they really need to query it before
> configuring/starting a VM, it won't be useful at all to make it
> available only at runtime.
>
> Daniel, when/how exactly software would need to query the new
> flag?
>
>
>> Worse, a machine type property that is static for all machine types now
>> could conceivably become dynamic when we add a machine type
>> configuration knob.
>> 
>
> This isn't the first time a machine capability that seems static
> actually depends on other configuration arguments.  We will
> probably need to address this eventually.

Then the best time to address it is now, provided we can :)

>> Would a way to tie the property to the configuration knob help?
>> Something like wakeup-suspend-support taking values true (supported),
>> false (not supported), and "acpi" (supported if machine type
>> configuration knob "acpi" is switched on).
>> 
>
> I would prefer a more generic mechanism.  Maybe make
> 'query-machines' accept a 'machine-options' argument?

This can support arbitrary configuration dependencies, unlike my
proposal.  However, I fear combinatorial explosion would make querying
anything but "default configuration" and "current configuration"
impractical, and "default configuration" would be basically useless, as
you can't predict how arguments will affect the value query-machines.

Whether this is an issue depends on how management software wants to use
query-machines.

Whether the ability to support arbitrary configuration dependencies is a
useful feature or an invitation to stupid stunts is another open
question :)

Here's a synthesis of the two proposals: have query-machines spell out
which of its results are determinate, and which configuration bits need
to be supplied to resolve the indeterminate ones.  For machine type
"pc-q35-*", wakeup-suspend-support would always yield true, but for
"pc-i440fx-*" it would return true when passed an acpi: true argument,
false when passed an acpi: false argument, and an encoding of
"indeterminate, you need to pass an acpi argument to learn more" when
passed no acpi argument.

I'm not saying this synthesis makes sense, I'm just exploring the design
space.

We need input from libvirt guys.

Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target

[Eduardo Habkost]

On Wed, May 23, 2018 at 05:53:34PM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Wed, May 23, 2018 at 11:17:55AM +0200, Markus Armbruster wrote:
> >> Eduardo Habkost <ehabkost@redhat.com> writes:
> >> > On Mon, May 21, 2018 at 04:46:36PM -0300, Daniel Henrique Barboza wrote:
> > [...]
> >> >> Since no objection was made back then, this logic was put into query-target
> >> >> starting
> >> >> in v2. Still, I don't have any favorites though: query-target looks ok,
> >> >> query-machine
> >> >> looks ok and a new API looks ok too. It's all about what makes (more) sense
> >> >> in the
> >> >> management level, I think.
> >> >
> >> > I understand the original objection from Eric: having to add a
> >> > new command for every runtime flag we want to expose to the user
> >> > looks wrong to me.
> >> 
> >> Agreed.
> >> 
> >> > However, extending query-machines and query-target looks wrong
> >> > too, however.  query-target looks wrong because this not a
> >> > property of the target.  query-machines is wrong because this is
> >> > not a static property of the machine-type, but of the running
> >> > machine instance.
> >> 
> >> Of the two, query-machines looks less wrong.
> >> 
> >> Arguably, -no-acpi should not exist.  It's an ad hoc flag that sneakily
> >> splits a few machine types into two variants, with and without ACPI.
> >> It's silently ignored for other machine types, even APCI-capable ones.
> >> 
> >> If the machine type variants with and without ACPI were separate types,
> >> wakeup-suspend-support would be a static property of the machine type.
> >> 
> >> However, "separate types" probably doesn't scale: I'm afraid we'd end up
> >> with an undesirable number of machine types.  Avoiding that is exactly
> >> why we have machine types with configurable options.  I suspect that's
> >> how ACPI should be configured (if at all).
> >> 
> >> So, should we make -no-acpi sugar for a machine type parameter?  And
> >> then deprecate -no-acpi for good measure?
> >
> > I think we should.
> 
> Would you like to take care of it?

Adding to my TODO-list, I hope I will be able to do it before the
next release.

> 
[...]
> >
> > This isn't the first time a machine capability that seems static
> > actually depends on other configuration arguments.  We will
> > probably need to address this eventually.
> 
> Then the best time to address it is now, provided we can :)

I'm not sure this is the best time.  If libvirt only needs the
runtime value and don't need any info at query-machines time, I
think support for this on query-machines will be left unused and
they will only use the query-current-machine value.

Just giving libvirt the runtime data it wants
(query-current-machine) seems way better than requiring libvirt
to interpret a set of rules and independently calculate something
QEMU already knows.

> 
> >> Would a way to tie the property to the configuration knob help?
> >> Something like wakeup-suspend-support taking values true (supported),
> >> false (not supported), and "acpi" (supported if machine type
> >> configuration knob "acpi" is switched on).
> >> 
> >
> > I would prefer a more generic mechanism.  Maybe make
> > 'query-machines' accept a 'machine-options' argument?
> 
> This can support arbitrary configuration dependencies, unlike my
> proposal.  However, I fear combinatorial explosion would make querying
> anything but "default configuration" and "current configuration"
> impractical, and "default configuration" would be basically useless, as
> you can't predict how arguments will affect the value query-machines.
> 
> Whether this is an issue depends on how management software wants to use
> query-machines.
> 
> Whether the ability to support arbitrary configuration dependencies is a
> useful feature or an invitation to stupid stunts is another open
> question :)
> 
> Here's a synthesis of the two proposals: have query-machines spell out
> which of its results are determinate, and which configuration bits need
> to be supplied to resolve the indeterminate ones.  For machine type
> "pc-q35-*", wakeup-suspend-support would always yield true, but for
> "pc-i440fx-*" it would return true when passed an acpi: true argument,
> false when passed an acpi: false argument, and an encoding of
> "indeterminate, you need to pass an acpi argument to learn more" when
> passed no acpi argument.

I like this proposal for other query-machines fields (like bus
information), but I think doing this for wakeup-suspend-support
is overkill, based on Daniel's description of its intended usage.


> 
> I'm not saying this synthesis makes sense, I'm just exploring the design
> space.
> 
> We need input from libvirt guys.

I have the impression we need real use cases so they can evaluate
the proposal.  wakeup-suspend-support doesn't seem like a use
case that really needs support on query-machines (because we
can simply provide the data at runtime).

-- 
Eduardo

Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Wed, May 23, 2018 at 05:53:34PM +0200, Markus Armbruster wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>> 
>> > On Wed, May 23, 2018 at 11:17:55AM +0200, Markus Armbruster wrote:
>> >> Eduardo Habkost <ehabkost@redhat.com> writes:
>> >> > On Mon, May 21, 2018 at 04:46:36PM -0300, Daniel Henrique Barboza wrote:
>> > [...]
>> >> >> Since no objection was made back then, this logic was put into query-target
>> >> >> starting
>> >> >> in v2. Still, I don't have any favorites though: query-target looks ok,
>> >> >> query-machine
>> >> >> looks ok and a new API looks ok too. It's all about what makes (more) sense
>> >> >> in the
>> >> >> management level, I think.
>> >> >
>> >> > I understand the original objection from Eric: having to add a
>> >> > new command for every runtime flag we want to expose to the user
>> >> > looks wrong to me.
>> >> 
>> >> Agreed.
>> >> 
>> >> > However, extending query-machines and query-target looks wrong
>> >> > too, however.  query-target looks wrong because this not a
>> >> > property of the target.  query-machines is wrong because this is
>> >> > not a static property of the machine-type, but of the running
>> >> > machine instance.
>> >> 
>> >> Of the two, query-machines looks less wrong.
>> >> 
>> >> Arguably, -no-acpi should not exist.  It's an ad hoc flag that sneakily
>> >> splits a few machine types into two variants, with and without ACPI.
>> >> It's silently ignored for other machine types, even APCI-capable ones.
>> >> 
>> >> If the machine type variants with and without ACPI were separate types,
>> >> wakeup-suspend-support would be a static property of the machine type.
>> >> 
>> >> However, "separate types" probably doesn't scale: I'm afraid we'd end up
>> >> with an undesirable number of machine types.  Avoiding that is exactly
>> >> why we have machine types with configurable options.  I suspect that's
>> >> how ACPI should be configured (if at all).
>> >> 
>> >> So, should we make -no-acpi sugar for a machine type parameter?  And
>> >> then deprecate -no-acpi for good measure?
>> >
>> > I think we should.
>> 
>> Would you like to take care of it?
>
> Adding to my TODO-list, I hope I will be able to do it before the
> next release.

Thanks!

>> >> > Can we have a new query command that could be an obvious
>> >> > container for simple machine capabilities that are not static?  A
>> >> > name like "query-machine" would be generic enough for that, I
>> >> > guess.
>> >> 
>> >> Having command names differ only in a single letter is awkward, but
>> >> let's focus on things other than naming now, and use
>> >> query-current-machine like a working title.
>> >> 
>> >> query-machines is wrong because wakeup-suspend-support isn't static for
>> >> some machine types.
>> >> 
>> >> query-current-machine is also kind of wrong because
>> >> wakeup-suspend-support *is* static for most machine types.
>> >
>> > The most appropriate solution depends a lot on how/when
>> > management software needs to query this.
>> 
>> Right.
>> 
>> > If they only need to query it at runtime for a running VM,
>> > there's no reason for us to go of our way and add complexity just
>> > to make it look like static data in query-machines.
>> >
>> > On the other hand, if they really need to query it before
>> > configuring/starting a VM, it won't be useful at all to make it
>> > available only at runtime.
>> >
>> > Daniel, when/how exactly software would need to query the new
>> > flag?
>> >
>> >
>> >> Worse, a machine type property that is static for all machine types now
>> >> could conceivably become dynamic when we add a machine type
>> >> configuration knob.
>> >> 
>> >
>> > This isn't the first time a machine capability that seems static
>> > actually depends on other configuration arguments.  We will
>> > probably need to address this eventually.
>> 
>> Then the best time to address it is now, provided we can :)
>
> I'm not sure this is the best time.  If libvirt only needs the
> runtime value and don't need any info at query-machines time, I
> think support for this on query-machines will be left unused and
> they will only use the query-current-machine value.
>
> Just giving libvirt the runtime data it wants
> (query-current-machine) seems way better than requiring libvirt
> to interpret a set of rules and independently calculate something
> QEMU already knows.

I wouldn't mind adding a query-current-machine to report dynamic machine
capabilities if that helps QMP clients.  query-machines could continue
to report static machine capabilities then.

However, we do need a plan on how to distribute machine capabilities
between query-machines and query-current-machine, in particular how to
handle changing staticness.

wakeup-suspend-support is static for most machine types, but dynamic for
some.  Where should it go?

It needs to go into query-current-machine when its dynamic with the
current machine.  It may go there just to keep things regular even if
its static with the current machine.

Does it go into query-machines, too?  If not, clients lose the ability
to examine all machines efficiently.  Even if this isn't an issue for
wakeup-suspend-support: are we sure this can't be an issue for any
future capabilities?

If it goes into query-machines, what should its value be for the machine
types where it's dynamic?  Should it be absent, perhaps, letting clients
know they have to consult query-current-machine to find the value?

What if a capability previously thought static becomes dynamic?  We can
add it to query-current-machine just fine, but removing it from
query-machines would be a compatibility break.  Making it optional,
too.  Should all new members of MachineInfo be optional, just in case?

These are design questions we need to ponder *now*.  Picking a solution
that satisfies current needs while ignoring future needs has bitten us
in the posterior time and again.  We're not going to successfully
predict *all* future needs, but not even trying should be easy to beat.

That's what I meant by "the best time to address it is now".

[...]

Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target

[Eduardo Habkost]

On Fri, May 25, 2018 at 08:30:59AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> > On Wed, May 23, 2018 at 05:53:34PM +0200, Markus Armbruster wrote:
[...]
> >> >> Worse, a machine type property that is static for all machine types now
> >> >> could conceivably become dynamic when we add a machine type
> >> >> configuration knob.
> >> >> 
> >> >
> >> > This isn't the first time a machine capability that seems static
> >> > actually depends on other configuration arguments.  We will
> >> > probably need to address this eventually.
> >> 
> >> Then the best time to address it is now, provided we can :)
> >
> > I'm not sure this is the best time.  If libvirt only needs the
> > runtime value and don't need any info at query-machines time, I
> > think support for this on query-machines will be left unused and
> > they will only use the query-current-machine value.
> >
> > Just giving libvirt the runtime data it wants
> > (query-current-machine) seems way better than requiring libvirt
> > to interpret a set of rules and independently calculate something
> > QEMU already knows.
> 
> I wouldn't mind adding a query-current-machine to report dynamic machine
> capabilities if that helps QMP clients.  query-machines could continue
> to report static machine capabilities then.
> 
> However, we do need a plan on how to distribute machine capabilities
> between query-machines and query-current-machine, in particular how to
> handle changing staticness.

Handling dynamic data that becomes static is easy: we can keep it
on both.

> 
> wakeup-suspend-support is static for most machine types, but dynamic for
> some.  Where should it go?

I think it obviously should go on query-current-machine.  Maybe
it can be added to query-machines in the future if it's deemed
useful.

> 
> It needs to go into query-current-machine when its dynamic with the
> current machine.  It may go there just to keep things regular even if
> its static with the current machine.
> 
> Does it go into query-machines, too?  If not, clients lose the ability
> to examine all machines efficiently.  Even if this isn't an issue for
> wakeup-suspend-support: are we sure this can't be an issue for any
> future capabilities?

I don't think this will be an issue for wakup-suspend-support,
but this is already an issue for existing capabilities.  See
below[1].


> 
> If it goes into query-machines, what should its value be for the machine
> types where it's dynamic?  Should it be absent, perhaps, letting clients
> know they have to consult query-current-machine to find the value?
> 
> What if a capability previously thought static becomes dynamic?  We can
> add it to query-current-machine just fine, but removing it from
> query-machines would be a compatibility break.  Making it optional,
> too.  Should all new members of MachineInfo be optional, just in case?
> 

The above are questions we must ponder if we are considering
extending query-machines.  We have been avoiding them for a few
years, by simply not extending query-machines very often and
letting libvirt hardcode machine-type info.  :(


> These are design questions we need to ponder *now*.  Picking a solution
> that satisfies current needs while ignoring future needs has bitten us
> in the posterior time and again.  We're not going to successfully
> predict *all* future needs, but not even trying should be easy to beat.
> 
> That's what I meant by "the best time to address it is now".
> 

I agree.  I just think there are countless other use cases that
require extending query-machines today.  I'd prefer to use one of
them as a starting point for this design exercise, instead of
wakeup-suspend-support.


[1] Doing a:
  $ git grep 'STR.*machine, "'
on libvirt source is enough to find some code demonstrating where
query-machines is already lacking today:

src/libxl/libxl_capabilities.c:583:    if (STREQ(machine, "xenpv"))
src/libxl/libxl_capabilities.c:729:    if (STREQ(domCaps->machine, "xenfv"))
src/libxl/libxl_driver.c:6379:        if (STRNEQ(machine, "xenpv") && STRNEQ(machine, "xenfv")) {
src/qemu/qemu_capabilities.c:2435:            STREQ(def->os.machine, "ppce500"))
src/qemu/qemu_capabilities.c:2439:            STREQ(def->os.machine, "prep"))
src/qemu/qemu_capabilities.c:2443:            STREQ(def->os.machine, "bamboo"))
src/qemu/qemu_capabilities.c:2446:        if (STREQ(def->os.machine, "mpc8544ds"))
src/qemu/qemu_capabilities.c:5376:        STREQ(def->os.machine, "isapc");
src/qemu/qemu_command.c:3829:    if (STRPREFIX(def->os.machine, "s390-virtio") &&
src/qemu/qemu_domain.c:2798:        if (STREQ(def->os.machine, "isapc")) {
src/qemu/qemu_domain.c:5009:        if (STREQ(def->os.machine, "versatilepb"))
src/qemu/qemu_domain.c:8222:    return (STRPREFIX(machine, "pc-q35") ||
src/qemu/qemu_domain.c:8223:            STREQ(machine, "q35"));
src/qemu/qemu_domain.c:8237:    return (STREQ(machine, "pc") ||
src/qemu/qemu_domain.c:8238:            STRPREFIX(machine, "pc-0.") ||
src/qemu/qemu_domain.c:8239:            STRPREFIX(machine, "pc-1.") ||
src/qemu/qemu_domain.c:8240:            STRPREFIX(machine, "pc-i440") ||
src/qemu/qemu_domain.c:8241:            STRPREFIX(machine, "rhel"));
src/qemu/qemu_domain.c:8285:    const char *p = STRSKIP(machine, "pc-q35-");
src/qemu/qemu_domain.c:8310:    return STRPREFIX(machine, "s390-ccw");
src/qemu/qemu_domain.c:8329:    if (STRNEQ(machine, "virt") &&
src/qemu/qemu_domain.c:8330:        !STRPREFIX(machine, "virt-"))
src/qemu/qemu_domain.c:8351:    if (STRNEQ(machine, "pseries") &&
src/qemu/qemu_domain.c:8352:        !STRPREFIX(machine, "pseries-"))
src/qemu/qemu_domain.c:8564:        STREQ(machine, "malta") ||
src/qemu/qemu_domain.c:8565:        STREQ(machine, "sun4u") ||
src/qemu/qemu_domain.c:8566:        STREQ(machine, "g3beige");
src/qemu/qemu_domain_address.c:467:    if (!(STRPREFIX(def->os.machine, "vexpress-") ||
src/qemu/qemu_domain_address.c:2145:    if (STREQ(def->os.machine, "versatilepb"))
src/util/virarch.c:170:    } else if (STREQ(ut.machine, "amd64")) {

-- 
Eduardo

Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Fri, May 25, 2018 at 08:30:59AM +0200, Markus Armbruster wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>> > On Wed, May 23, 2018 at 05:53:34PM +0200, Markus Armbruster wrote:
> [...]
>> >> >> Worse, a machine type property that is static for all machine types now
>> >> >> could conceivably become dynamic when we add a machine type
>> >> >> configuration knob.
>> >> >> 
>> >> >
>> >> > This isn't the first time a machine capability that seems static
>> >> > actually depends on other configuration arguments.  We will
>> >> > probably need to address this eventually.
>> >> 
>> >> Then the best time to address it is now, provided we can :)
>> >
>> > I'm not sure this is the best time.  If libvirt only needs the
>> > runtime value and don't need any info at query-machines time, I
>> > think support for this on query-machines will be left unused and
>> > they will only use the query-current-machine value.
>> >
>> > Just giving libvirt the runtime data it wants
>> > (query-current-machine) seems way better than requiring libvirt
>> > to interpret a set of rules and independently calculate something
>> > QEMU already knows.
>> 
>> I wouldn't mind adding a query-current-machine to report dynamic machine
>> capabilities if that helps QMP clients.  query-machines could continue
>> to report static machine capabilities then.
>> 
>> However, we do need a plan on how to distribute machine capabilities
>> between query-machines and query-current-machine, in particular how to
>> handle changing staticness.
>
> Handling dynamic data that becomes static is easy: we can keep it
> on both.

The duplication is less than elegant, but backward-compatibility
generally is.

>> wakeup-suspend-support is static for most machine types, but dynamic for
>> some.  Where should it go?
>
> I think it obviously should go on query-current-machine.  Maybe
> it can be added to query-machines in the future if it's deemed
> useful.
>
>> It needs to go into query-current-machine when its dynamic with the
>> current machine.  It may go there just to keep things regular even if
>> its static with the current machine.
>> 
>> Does it go into query-machines, too?  If not, clients lose the ability
>> to examine all machines efficiently.  Even if this isn't an issue for
>> wakeup-suspend-support: are we sure this can't be an issue for any
>> future capabilities?
>
> I don't think this will be an issue for wakup-suspend-support,
> but this is already an issue for existing capabilities.  See
> below[1].
>
>
>> 
>> If it goes into query-machines, what should its value be for the machine
>> types where it's dynamic?  Should it be absent, perhaps, letting clients
>> know they have to consult query-current-machine to find the value?
>> 
>> What if a capability previously thought static becomes dynamic?  We can
>> add it to query-current-machine just fine, but removing it from
>> query-machines would be a compatibility break.  Making it optional,
>> too.  Should all new members of MachineInfo be optional, just in case?
>> 
>
> The above are questions we must ponder if we are considering
> extending query-machines.  We have been avoiding them for a few
> years, by simply not extending query-machines very often and
> letting libvirt hardcode machine-type info.  :(
>
>
>> These are design questions we need to ponder *now*.  Picking a solution
>> that satisfies current needs while ignoring future needs has bitten us
>> in the posterior time and again.  We're not going to successfully
>> predict *all* future needs, but not even trying should be easy to beat.
>> 
>> That's what I meant by "the best time to address it is now".
>> 
>
> I agree.  I just think there are countless other use cases that
> require extending query-machines today.  I'd prefer to use one of
> them as a starting point for this design exercise, instead of
> wakeup-suspend-support.

Analyzing all the use cases we know makes sense.

> [1] Doing a:
>   $ git grep 'STR.*machine, "'
> on libvirt source is enough to find some code demonstrating where
> query-machines is already lacking today:
>
> src/libxl/libxl_capabilities.c:583:    if (STREQ(machine, "xenpv"))
> src/libxl/libxl_capabilities.c:729:    if (STREQ(domCaps->machine, "xenfv"))
> src/libxl/libxl_driver.c:6379:        if (STRNEQ(machine, "xenpv") && STRNEQ(machine, "xenfv")) {
> src/qemu/qemu_capabilities.c:2435:            STREQ(def->os.machine, "ppce500"))
> src/qemu/qemu_capabilities.c:2439:            STREQ(def->os.machine, "prep"))
> src/qemu/qemu_capabilities.c:2443:            STREQ(def->os.machine, "bamboo"))
> src/qemu/qemu_capabilities.c:2446:        if (STREQ(def->os.machine, "mpc8544ds"))
> src/qemu/qemu_capabilities.c:5376:        STREQ(def->os.machine, "isapc");
> src/qemu/qemu_command.c:3829:    if (STRPREFIX(def->os.machine, "s390-virtio") &&
> src/qemu/qemu_domain.c:2798:        if (STREQ(def->os.machine, "isapc")) {
> src/qemu/qemu_domain.c:5009:        if (STREQ(def->os.machine, "versatilepb"))
> src/qemu/qemu_domain.c:8222:    return (STRPREFIX(machine, "pc-q35") ||
> src/qemu/qemu_domain.c:8223:            STREQ(machine, "q35"));
> src/qemu/qemu_domain.c:8237:    return (STREQ(machine, "pc") ||
> src/qemu/qemu_domain.c:8238:            STRPREFIX(machine, "pc-0.") ||
> src/qemu/qemu_domain.c:8239:            STRPREFIX(machine, "pc-1.") ||
> src/qemu/qemu_domain.c:8240:            STRPREFIX(machine, "pc-i440") ||
> src/qemu/qemu_domain.c:8241:            STRPREFIX(machine, "rhel"));
> src/qemu/qemu_domain.c:8285:    const char *p = STRSKIP(machine, "pc-q35-");
> src/qemu/qemu_domain.c:8310:    return STRPREFIX(machine, "s390-ccw");
> src/qemu/qemu_domain.c:8329:    if (STRNEQ(machine, "virt") &&
> src/qemu/qemu_domain.c:8330:        !STRPREFIX(machine, "virt-"))
> src/qemu/qemu_domain.c:8351:    if (STRNEQ(machine, "pseries") &&
> src/qemu/qemu_domain.c:8352:        !STRPREFIX(machine, "pseries-"))
> src/qemu/qemu_domain.c:8564:        STREQ(machine, "malta") ||
> src/qemu/qemu_domain.c:8565:        STREQ(machine, "sun4u") ||
> src/qemu/qemu_domain.c:8566:        STREQ(machine, "g3beige");
> src/qemu/qemu_domain_address.c:467:    if (!(STRPREFIX(def->os.machine, "vexpress-") ||
> src/qemu/qemu_domain_address.c:2145:    if (STREQ(def->os.machine, "versatilepb"))
> src/util/virarch.c:170:    } else if (STREQ(ut.machine, "amd64")) {

How can we get from this grep to a list of static or dynamic machine
type capabilties?

Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target

[Eduardo Habkost]

On Mon, May 28, 2018 at 09:23:54AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
[...]
> > [1] Doing a:
> >   $ git grep 'STR.*machine, "'
> > on libvirt source is enough to find some code demonstrating where
> > query-machines is already lacking today:
[...]
> How can we get from this grep to a list of static or dynamic machine
> type capabilties?

Let's look at the code:


$ git grep -W 'STR.*machine, "'
src/libxl/libxl_capabilities.c=libxlMakeDomainOSCaps(const char *machine,
src/libxl/libxl_capabilities.c-                      virDomainCapsOSPtr os,
src/libxl/libxl_capabilities.c-                      virFirmwarePtr *firmwares,
src/libxl/libxl_capabilities.c-                      size_t nfirmwares)
src/libxl/libxl_capabilities.c-{
src/libxl/libxl_capabilities.c-    virDomainCapsLoaderPtr capsLoader = &os->loader;
src/libxl/libxl_capabilities.c-    size_t i;
src/libxl/libxl_capabilities.c-
src/libxl/libxl_capabilities.c-    os->supported = true;
src/libxl/libxl_capabilities.c-
src/libxl/libxl_capabilities.c:    if (STREQ(machine, "xenpv"))
src/libxl/libxl_capabilities.c-        return 0;

I don't understand why this one is here, but we can find out what
we could add to query-machines to make this unnecessary.


[...]
--
src/libxl/libxl_capabilities.c=libxlMakeDomainCapabilities(virDomainCapsPtr domCaps,
src/libxl/libxl_capabilities.c-                            virFirmwarePtr *firmwares,
src/libxl/libxl_capabilities.c-                            size_t nfirmwares)
src/libxl/libxl_capabilities.c-{
src/libxl/libxl_capabilities.c-    virDomainCapsOSPtr os = &domCaps->os;
src/libxl/libxl_capabilities.c-    virDomainCapsDeviceDiskPtr disk = &domCaps->disk;
src/libxl/libxl_capabilities.c-    virDomainCapsDeviceGraphicsPtr graphics = &domCaps->graphics;
src/libxl/libxl_capabilities.c-    virDomainCapsDeviceVideoPtr video = &domCaps->video;
src/libxl/libxl_capabilities.c-    virDomainCapsDeviceHostdevPtr hostdev = &domCaps->hostdev;
src/libxl/libxl_capabilities.c-
src/libxl/libxl_capabilities.c:    if (STREQ(domCaps->machine, "xenfv"))
src/libxl/libxl_capabilities.c-        domCaps->maxvcpus = HVM_MAX_VCPUS;
src/libxl/libxl_capabilities.c-    else
src/libxl/libxl_capabilities.c-        domCaps->maxvcpus = PV_MAX_VCPUS;

Looks like libvirt isn't using MachineInfo::cpu-max.  libvirt
bug, or workaround for QEMU limitation?


[...]
--
src/libxl/libxl_driver.c=libxlConnectGetDomainCapabilities(virConnectPtr conn,
src/libxl/libxl_driver.c-                                  const char *emulatorbin,
src/libxl/libxl_driver.c-                                  const char *arch_str,
src/libxl/libxl_driver.c-                                  const char *machine,
src/libxl/libxl_driver.c-                                  const char *virttype_str,
src/libxl/libxl_driver.c-                                  unsigned int flags)
src/libxl/libxl_driver.c-{
[...]
src/libxl/libxl_driver.c-    if (machine) {
src/libxl/libxl_driver.c:        if (STRNEQ(machine, "xenpv") && STRNEQ(machine, "xenfv")) {
src/libxl/libxl_driver.c-            virReportError(VIR_ERR_INVALID_ARG, "%s",
src/libxl/libxl_driver.c-                           _("Xen only supports 'xenpv' and 'xenfv' machines"));


Not sure if this should be encoded in QEMU.  accel=xen works with
other PC machines, doesn't it?


[...]
--
src/qemu/qemu_capabilities.c=bool virQEMUCapsHasPCIMultiBus(virQEMUCapsPtr qemuCaps,
src/qemu/qemu_capabilities.c-                               const virDomainDef *def)
src/qemu/qemu_capabilities.c-{
src/qemu/qemu_capabilities.c-    /* x86_64 and i686 support PCI-multibus on all machine types
src/qemu/qemu_capabilities.c-     * since forever */
src/qemu/qemu_capabilities.c-    if (ARCH_IS_X86(def->os.arch))
src/qemu/qemu_capabilities.c-        return true;
src/qemu/qemu_capabilities.c-
src/qemu/qemu_capabilities.c-    if (def->os.arch == VIR_ARCH_PPC ||
src/qemu/qemu_capabilities.c-        ARCH_IS_PPC64(def->os.arch)) {
src/qemu/qemu_capabilities.c-        /*
src/qemu/qemu_capabilities.c-         * Usage of pci.0 naming:
src/qemu/qemu_capabilities.c-         *
src/qemu/qemu_capabilities.c-         *    ref405ep: no pci
src/qemu/qemu_capabilities.c-         *       taihu: no pci
src/qemu/qemu_capabilities.c-         *      bamboo: 1.1.0
src/qemu/qemu_capabilities.c-         *       mac99: 2.0.0
src/qemu/qemu_capabilities.c-         *     g3beige: 2.0.0
src/qemu/qemu_capabilities.c-         *        prep: 1.4.0
src/qemu/qemu_capabilities.c-         *     pseries: 2.0.0
src/qemu/qemu_capabilities.c-         *   mpc8544ds: forever
src/qemu/qemu_capabilities.c-         * virtex-m507: no pci
src/qemu/qemu_capabilities.c-         *     ppce500: 1.6.0
src/qemu/qemu_capabilities.c-         */
src/qemu/qemu_capabilities.c-
src/qemu/qemu_capabilities.c-        /* We do not store the qemu version in domain status XML.
src/qemu/qemu_capabilities.c-         * Hope the user is using a QEMU new enough to use 'pci.0',
src/qemu/qemu_capabilities.c-         * otherwise the results of this function will be wrong
src/qemu/qemu_capabilities.c-         * for domains already running at the time of daemon
src/qemu/qemu_capabilities.c-         * restart */
src/qemu/qemu_capabilities.c-        if (qemuCaps->version == 0)
src/qemu/qemu_capabilities.c-            return true;
src/qemu/qemu_capabilities.c-
src/qemu/qemu_capabilities.c-        if (qemuCaps->version >= 2000000)
src/qemu/qemu_capabilities.c-            return true;
src/qemu/qemu_capabilities.c-
src/qemu/qemu_capabilities.c-        if (qemuCaps->version >= 1006000 &&
src/qemu/qemu_capabilities.c:            STREQ(def->os.machine, "ppce500"))
src/qemu/qemu_capabilities.c-            return true;
src/qemu/qemu_capabilities.c-
src/qemu/qemu_capabilities.c-        if (qemuCaps->version >= 1004000 &&
src/qemu/qemu_capabilities.c:            STREQ(def->os.machine, "prep"))
src/qemu/qemu_capabilities.c-            return true;
src/qemu/qemu_capabilities.c-
src/qemu/qemu_capabilities.c-        if (qemuCaps->version >= 1001000 &&
src/qemu/qemu_capabilities.c:            STREQ(def->os.machine, "bamboo"))
src/qemu/qemu_capabilities.c-            return true;
src/qemu/qemu_capabilities.c-
src/qemu/qemu_capabilities.c:        if (STREQ(def->os.machine, "mpc8544ds"))
src/qemu/qemu_capabilities.c-            return true;

This is due to the lack of bus information on query-machines.


[...]
src/qemu/qemu_capabilities.c-
src/qemu/qemu_capabilities.c-        return false;
src/qemu/qemu_capabilities.c-    }
src/qemu/qemu_capabilities.c-
src/qemu/qemu_capabilities.c-    /* If 'virt' supports PCI, it supports multibus.
src/qemu/qemu_capabilities.c-     * No extra conditions here for simplicity.
src/qemu/qemu_capabilities.c-     */
src/qemu/qemu_capabilities.c-    if (qemuDomainIsVirt(def))
src/qemu/qemu_capabilities.c-        return true;
src/qemu/qemu_capabilities.c-
src/qemu/qemu_capabilities.c-    return false;
src/qemu/qemu_capabilities.c-}
--
src/qemu/qemu_capabilities.c=virQEMUCapsSupportsVmport(virQEMUCapsPtr qemuCaps,
src/qemu/qemu_capabilities.c-                          const virDomainDef *def)
src/qemu/qemu_capabilities.c-{
src/qemu/qemu_capabilities.c-    if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_VMPORT_OPT))
src/qemu/qemu_capabilities.c-        return false;
src/qemu/qemu_capabilities.c-
src/qemu/qemu_capabilities.c-    return qemuDomainIsI440FX(def) ||
src/qemu/qemu_capabilities.c-        qemuDomainIsQ35(def) ||
src/qemu/qemu_capabilities.c:        STREQ(def->os.machine, "isapc");
src/qemu/qemu_capabilities.c-}

Lack of bus information on query-machines?


--
src/qemu/qemu_command.c=qemuBuildMemballoonCommandLine(virCommandPtr cmd,
src/qemu/qemu_command.c-                               const virDomainDef *def,
src/qemu/qemu_command.c-                               virQEMUCapsPtr qemuCaps)
src/qemu/qemu_command.c-{
src/qemu/qemu_command.c-    virBuffer buf = VIR_BUFFER_INITIALIZER;
src/qemu/qemu_command.c-
src/qemu/qemu_command.c:    if (STRPREFIX(def->os.machine, "s390-virtio") &&
src/qemu/qemu_command.c-        virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390) && def->memballoon)
src/qemu/qemu_command.c-        def->memballoon->model = VIR_DOMAIN_MEMBALLOON_MODEL_NONE;


Lack of bus information on query-machines?


[...]
--
src/qemu/qemu_domain.c=qemuDomainDefAddDefaultDevices(virDomainDefPtr def,
src/qemu/qemu_domain.c-                               virQEMUCapsPtr qemuCaps)
src/qemu/qemu_domain.c-{
[...]
src/qemu/qemu_domain.c:        if (STREQ(def->os.machine, "isapc")) {
src/qemu/qemu_domain.c-            addDefaultUSB = false;


Lack of bus/defaults information on query-machines?

This whole function is a collection of cases where
bus/device/defaults information is missing on query-machines:

src/qemu/qemu_domain.c-            break;
src/qemu/qemu_domain.c-        }
src/qemu/qemu_domain.c-        if (qemuDomainIsQ35(def)) {
src/qemu/qemu_domain.c-            addPCIeRoot = true;
src/qemu/qemu_domain.c-            addImplicitSATA = true;
[...]
src/qemu/qemu_domain.c-        if (qemuDomainIsI440FX(def))
src/qemu/qemu_domain.c-            addPCIRoot = true;
[...]
src/qemu/qemu_domain.c-        break;
src/qemu/qemu_domain.c-
src/qemu/qemu_domain.c-    case VIR_ARCH_ARMV7L:
src/qemu/qemu_domain.c-    case VIR_ARCH_AARCH64:
src/qemu/qemu_domain.c-        addDefaultUSB = false;
src/qemu/qemu_domain.c-        addDefaultMemballoon = false;
src/qemu/qemu_domain.c-        if (qemuDomainIsVirt(def))
src/qemu/qemu_domain.c-            addPCIeRoot = virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_GPEX);
src/qemu/qemu_domain.c-        break;
src/qemu/qemu_domain.c-
src/qemu/qemu_domain.c-    case VIR_ARCH_PPC64:
src/qemu/qemu_domain.c-    case VIR_ARCH_PPC64LE:
src/qemu/qemu_domain.c-        addPCIRoot = true;
src/qemu/qemu_domain.c-        addDefaultUSBKBD = true;
src/qemu/qemu_domain.c-        addDefaultUSBMouse = true;
src/qemu/qemu_domain.c-        /* For pSeries guests, the firmware provides the same
src/qemu/qemu_domain.c-         * functionality as the pvpanic device, so automatically
src/qemu/qemu_domain.c-         * add the definition if not already present */
src/qemu/qemu_domain.c-        if (qemuDomainIsPSeries(def))
src/qemu/qemu_domain.c-            addPanicDevice = true;
src/qemu/qemu_domain.c-        break;
[...]
--
src/qemu/qemu_domain.c=qemuDomainDefaultNetModel(const virDomainDef *def,
src/qemu/qemu_domain.c-                          virQEMUCapsPtr qemuCaps)
src/qemu/qemu_domain.c-{
src/qemu/qemu_domain.c-    if (ARCH_IS_S390(def->os.arch))
src/qemu/qemu_domain.c-        return "virtio";
src/qemu/qemu_domain.c-
src/qemu/qemu_domain.c-    if (def->os.arch == VIR_ARCH_ARMV7L ||
src/qemu/qemu_domain.c-        def->os.arch == VIR_ARCH_AARCH64) {
src/qemu/qemu_domain.c:        if (STREQ(def->os.machine, "versatilepb"))
src/qemu/qemu_domain.c-            return "smc91c111";
src/qemu/qemu_domain.c-
src/qemu/qemu_domain.c-        if (qemuDomainIsVirt(def))
src/qemu/qemu_domain.c-            return "virtio";
src/qemu/qemu_domain.c-
src/qemu/qemu_domain.c-        /* Incomplete. vexpress (and a few others) use this, but not all
src/qemu/qemu_domain.c-         * arm boards */
src/qemu/qemu_domain.c-        return "lan9118";


Lack of bus/defaults information on query-machines?

[...]
--
src/qemu/qemu_domain.c=qemuDomainMachineIsQ35(const char *machine)
src/qemu/qemu_domain.c-{
src/qemu/qemu_domain.c:    return (STRPREFIX(machine, "pc-q35") ||
src/qemu/qemu_domain.c:            STREQ(machine, "q35"));
src/qemu/qemu_domain.c-}

Need to grep for callers of this function.

--
src/qemu/qemu_domain.c=qemuDomainMachineIsI440FX(const char *machine)
src/qemu/qemu_domain.c-{
src/qemu/qemu_domain.c:    return (STREQ(machine, "pc") ||
src/qemu/qemu_domain.c:            STRPREFIX(machine, "pc-0.") ||
src/qemu/qemu_domain.c:            STRPREFIX(machine, "pc-1.") ||
src/qemu/qemu_domain.c:            STRPREFIX(machine, "pc-i440") ||
src/qemu/qemu_domain.c:            STRPREFIX(machine, "rhel"));
src/qemu/qemu_domain.c-}

Need to grep for callers of this function.

---
src/qemu/qemu_domain.c=qemuDomainMachineNeedsFDC(const char *machine)
src/qemu/qemu_domain.c-{
src/qemu/qemu_domain.c:    const char *p = STRSKIP(machine, "pc-q35-");
src/qemu/qemu_domain.c-
src/qemu/qemu_domain.c-    if (p) {
src/qemu/qemu_domain.c-        if (STRPREFIX(p, "1.") ||
src/qemu/qemu_domain.c-            STRPREFIX(p, "2.0") ||
src/qemu/qemu_domain.c-            STRPREFIX(p, "2.1") ||
src/qemu/qemu_domain.c-            STRPREFIX(p, "2.2") ||
src/qemu/qemu_domain.c-            STRPREFIX(p, "2.3"))
src/qemu/qemu_domain.c-            return false;
src/qemu/qemu_domain.c-        return true;
src/qemu/qemu_domain.c-    }
src/qemu/qemu_domain.c-    return false;
src/qemu/qemu_domain.c-}

Lack of bus/defaults information on query-machines.


--
src/qemu/qemu_domain.c=qemuDomainMachineIsS390CCW(const char *machine)
src/qemu/qemu_domain.c-{
src/qemu/qemu_domain.c:    return STRPREFIX(machine, "s390-ccw");
src/qemu/qemu_domain.c-}

Need to grep for callers of this function.


--
src/qemu/qemu_domain.c=qemuDomainMachineIsVirt(const char *machine,
src/qemu/qemu_domain.c-                        const virArch arch)
src/qemu/qemu_domain.c-{
src/qemu/qemu_domain.c-    if (arch != VIR_ARCH_ARMV7L &&
src/qemu/qemu_domain.c-        arch != VIR_ARCH_AARCH64)
src/qemu/qemu_domain.c-        return false;
src/qemu/qemu_domain.c-
src/qemu/qemu_domain.c:    if (STRNEQ(machine, "virt") &&
src/qemu/qemu_domain.c:        !STRPREFIX(machine, "virt-"))
src/qemu/qemu_domain.c-        return false;
src/qemu/qemu_domain.c-
src/qemu/qemu_domain.c-    return true;
src/qemu/qemu_domain.c-}

Need to grep for callers of this function.

--
src/qemu/qemu_domain.c=qemuDomainMachineIsPSeries(const char *machine,
src/qemu/qemu_domain.c-                           const virArch arch)
src/qemu/qemu_domain.c-{
src/qemu/qemu_domain.c-    if (!ARCH_IS_PPC64(arch))
src/qemu/qemu_domain.c-        return false;
src/qemu/qemu_domain.c-
src/qemu/qemu_domain.c:    if (STRNEQ(machine, "pseries") &&
src/qemu/qemu_domain.c:        !STRPREFIX(machine, "pseries-"))
src/qemu/qemu_domain.c-        return false;
src/qemu/qemu_domain.c-
src/qemu/qemu_domain.c-    return true;
src/qemu/qemu_domain.c-}

Need to grep for callers of this function.


--
src/qemu/qemu_domain.c=qemuDomainMachineHasBuiltinIDE(const char *machine)
src/qemu/qemu_domain.c-{
src/qemu/qemu_domain.c-    return qemuDomainMachineIsI440FX(machine) ||
src/qemu/qemu_domain.c:        STREQ(machine, "malta") ||
src/qemu/qemu_domain.c:        STREQ(machine, "sun4u") ||
src/qemu/qemu_domain.c:        STREQ(machine, "g3beige");
src/qemu/qemu_domain.c-}

Lack of bus/defaults information on query-machines.


[...]
--
src/qemu/qemu_domain_address.c=qemuDomainAssignARMVirtioMMIOAddresses(virDomainDefPtr def,
src/qemu/qemu_domain_address.c-                                       virQEMUCapsPtr qemuCaps)
src/qemu/qemu_domain_address.c-{
src/qemu/qemu_domain_address.c-    if (def->os.arch != VIR_ARCH_ARMV7L &&
src/qemu/qemu_domain_address.c-        def->os.arch != VIR_ARCH_AARCH64)
src/qemu/qemu_domain_address.c-        return;
src/qemu/qemu_domain_address.c-
src/qemu/qemu_domain_address.c:    if (!(STRPREFIX(def->os.machine, "vexpress-") ||
src/qemu/qemu_domain_address.c-          qemuDomainIsVirt(def)))
src/qemu/qemu_domain_address.c-        return;

Lack of bus/device/defaults information on query-machines?

[...]
--
src/qemu/qemu_domain_address.c=qemuDomainSupportsPCI(virDomainDefPtr def,
src/qemu/qemu_domain_address.c-                      virQEMUCapsPtr qemuCaps)
src/qemu/qemu_domain_address.c-{
src/qemu/qemu_domain_address.c-    if ((def->os.arch != VIR_ARCH_ARMV7L) && (def->os.arch != VIR_ARCH_AARCH64))
src/qemu/qemu_domain_address.c-        return true;
src/qemu/qemu_domain_address.c-
src/qemu/qemu_domain_address.c:    if (STREQ(def->os.machine, "versatilepb"))
src/qemu/qemu_domain_address.c-        return true;
src/qemu/qemu_domain_address.c-
src/qemu/qemu_domain_address.c-    if (qemuDomainIsVirt(def) &&
src/qemu/qemu_domain_address.c-        virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_GPEX))
src/qemu/qemu_domain_address.c-        return true;
src/qemu/qemu_domain_address.c-

Lack of bus information on query-machines.

[...]

-- 
Eduardo

Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target

[Daniel Henrique Barboza]

Hi,

Sorry for the delay. I'll summarize what I've understood from the discussion
so far:

- query-target is the wrong place for this flag. query-machines is 
(less) wrong
because it is not a static property of the machine object

- a new "query-current-machine" can be created to host these dynamic
properties that belongs to the current instance of the VM

- there are machines in which the suspend support may vary with a
"-no-acpi" option that would disable both the suspend and wake-up
support. In this case, I see no problem into counting this flag into
the logic (assuming it is possible, of course) and setting it as "false"
if there is -no-acpi present (or even making the API returning "yes",
"no" or "acpi" like Markus suggested) somewhere.


Based on the last email from Eduardo, apparently there is a handful
of other machine properties that can be hosted in either this new
query-current-machine API or query-machines. I believe that this is
more of a long term goal, but this new query-current-machine API
would be a good kick-off and we should go for it.

Is this a fair understanding? Did I miss something?


Thanks,


Daniel



On 05/29/2018 11:55 AM, Eduardo Habkost wrote:
> On Mon, May 28, 2018 at 09:23:54AM +0200, Markus Armbruster wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
> [...]
>>> [1] Doing a:
>>>    $ git grep 'STR.*machine, "'
>>> on libvirt source is enough to find some code demonstrating where
>>> query-machines is already lacking today:
> [...]
>> How can we get from this grep to a list of static or dynamic machine
>> type capabilties?
> Let's look at the code:
>
>
> $ git grep -W 'STR.*machine, "'
> src/libxl/libxl_capabilities.c=libxlMakeDomainOSCaps(const char *machine,
> src/libxl/libxl_capabilities.c-                      virDomainCapsOSPtr os,
> src/libxl/libxl_capabilities.c-                      virFirmwarePtr *firmwares,
> src/libxl/libxl_capabilities.c-                      size_t nfirmwares)
> src/libxl/libxl_capabilities.c-{
> src/libxl/libxl_capabilities.c-    virDomainCapsLoaderPtr capsLoader = &os->loader;
> src/libxl/libxl_capabilities.c-    size_t i;
> src/libxl/libxl_capabilities.c-
> src/libxl/libxl_capabilities.c-    os->supported = true;
> src/libxl/libxl_capabilities.c-
> src/libxl/libxl_capabilities.c:    if (STREQ(machine, "xenpv"))
> src/libxl/libxl_capabilities.c-        return 0;
>
> I don't understand why this one is here, but we can find out what
> we could add to query-machines to make this unnecessary.
>
>
> [...]
> --
> src/libxl/libxl_capabilities.c=libxlMakeDomainCapabilities(virDomainCapsPtr domCaps,
> src/libxl/libxl_capabilities.c-                            virFirmwarePtr *firmwares,
> src/libxl/libxl_capabilities.c-                            size_t nfirmwares)
> src/libxl/libxl_capabilities.c-{
> src/libxl/libxl_capabilities.c-    virDomainCapsOSPtr os = &domCaps->os;
> src/libxl/libxl_capabilities.c-    virDomainCapsDeviceDiskPtr disk = &domCaps->disk;
> src/libxl/libxl_capabilities.c-    virDomainCapsDeviceGraphicsPtr graphics = &domCaps->graphics;
> src/libxl/libxl_capabilities.c-    virDomainCapsDeviceVideoPtr video = &domCaps->video;
> src/libxl/libxl_capabilities.c-    virDomainCapsDeviceHostdevPtr hostdev = &domCaps->hostdev;
> src/libxl/libxl_capabilities.c-
> src/libxl/libxl_capabilities.c:    if (STREQ(domCaps->machine, "xenfv"))
> src/libxl/libxl_capabilities.c-        domCaps->maxvcpus = HVM_MAX_VCPUS;
> src/libxl/libxl_capabilities.c-    else
> src/libxl/libxl_capabilities.c-        domCaps->maxvcpus = PV_MAX_VCPUS;
>
> Looks like libvirt isn't using MachineInfo::cpu-max.  libvirt
> bug, or workaround for QEMU limitation?
>
>
> [...]
> --
> src/libxl/libxl_driver.c=libxlConnectGetDomainCapabilities(virConnectPtr conn,
> src/libxl/libxl_driver.c-                                  const char *emulatorbin,
> src/libxl/libxl_driver.c-                                  const char *arch_str,
> src/libxl/libxl_driver.c-                                  const char *machine,
> src/libxl/libxl_driver.c-                                  const char *virttype_str,
> src/libxl/libxl_driver.c-                                  unsigned int flags)
> src/libxl/libxl_driver.c-{
> [...]
> src/libxl/libxl_driver.c-    if (machine) {
> src/libxl/libxl_driver.c:        if (STRNEQ(machine, "xenpv") && STRNEQ(machine, "xenfv")) {
> src/libxl/libxl_driver.c-            virReportError(VIR_ERR_INVALID_ARG, "%s",
> src/libxl/libxl_driver.c-                           _("Xen only supports 'xenpv' and 'xenfv' machines"));
>
>
> Not sure if this should be encoded in QEMU.  accel=xen works with
> other PC machines, doesn't it?
>
>
> [...]
> --
> src/qemu/qemu_capabilities.c=bool virQEMUCapsHasPCIMultiBus(virQEMUCapsPtr qemuCaps,
> src/qemu/qemu_capabilities.c-                               const virDomainDef *def)
> src/qemu/qemu_capabilities.c-{
> src/qemu/qemu_capabilities.c-    /* x86_64 and i686 support PCI-multibus on all machine types
> src/qemu/qemu_capabilities.c-     * since forever */
> src/qemu/qemu_capabilities.c-    if (ARCH_IS_X86(def->os.arch))
> src/qemu/qemu_capabilities.c-        return true;
> src/qemu/qemu_capabilities.c-
> src/qemu/qemu_capabilities.c-    if (def->os.arch == VIR_ARCH_PPC ||
> src/qemu/qemu_capabilities.c-        ARCH_IS_PPC64(def->os.arch)) {
> src/qemu/qemu_capabilities.c-        /*
> src/qemu/qemu_capabilities.c-         * Usage of pci.0 naming:
> src/qemu/qemu_capabilities.c-         *
> src/qemu/qemu_capabilities.c-         *    ref405ep: no pci
> src/qemu/qemu_capabilities.c-         *       taihu: no pci
> src/qemu/qemu_capabilities.c-         *      bamboo: 1.1.0
> src/qemu/qemu_capabilities.c-         *       mac99: 2.0.0
> src/qemu/qemu_capabilities.c-         *     g3beige: 2.0.0
> src/qemu/qemu_capabilities.c-         *        prep: 1.4.0
> src/qemu/qemu_capabilities.c-         *     pseries: 2.0.0
> src/qemu/qemu_capabilities.c-         *   mpc8544ds: forever
> src/qemu/qemu_capabilities.c-         * virtex-m507: no pci
> src/qemu/qemu_capabilities.c-         *     ppce500: 1.6.0
> src/qemu/qemu_capabilities.c-         */
> src/qemu/qemu_capabilities.c-
> src/qemu/qemu_capabilities.c-        /* We do not store the qemu version in domain status XML.
> src/qemu/qemu_capabilities.c-         * Hope the user is using a QEMU new enough to use 'pci.0',
> src/qemu/qemu_capabilities.c-         * otherwise the results of this function will be wrong
> src/qemu/qemu_capabilities.c-         * for domains already running at the time of daemon
> src/qemu/qemu_capabilities.c-         * restart */
> src/qemu/qemu_capabilities.c-        if (qemuCaps->version == 0)
> src/qemu/qemu_capabilities.c-            return true;
> src/qemu/qemu_capabilities.c-
> src/qemu/qemu_capabilities.c-        if (qemuCaps->version >= 2000000)
> src/qemu/qemu_capabilities.c-            return true;
> src/qemu/qemu_capabilities.c-
> src/qemu/qemu_capabilities.c-        if (qemuCaps->version >= 1006000 &&
> src/qemu/qemu_capabilities.c:            STREQ(def->os.machine, "ppce500"))
> src/qemu/qemu_capabilities.c-            return true;
> src/qemu/qemu_capabilities.c-
> src/qemu/qemu_capabilities.c-        if (qemuCaps->version >= 1004000 &&
> src/qemu/qemu_capabilities.c:            STREQ(def->os.machine, "prep"))
> src/qemu/qemu_capabilities.c-            return true;
> src/qemu/qemu_capabilities.c-
> src/qemu/qemu_capabilities.c-        if (qemuCaps->version >= 1001000 &&
> src/qemu/qemu_capabilities.c:            STREQ(def->os.machine, "bamboo"))
> src/qemu/qemu_capabilities.c-            return true;
> src/qemu/qemu_capabilities.c-
> src/qemu/qemu_capabilities.c:        if (STREQ(def->os.machine, "mpc8544ds"))
> src/qemu/qemu_capabilities.c-            return true;
>
> This is due to the lack of bus information on query-machines.
>
>
> [...]
> src/qemu/qemu_capabilities.c-
> src/qemu/qemu_capabilities.c-        return false;
> src/qemu/qemu_capabilities.c-    }
> src/qemu/qemu_capabilities.c-
> src/qemu/qemu_capabilities.c-    /* If 'virt' supports PCI, it supports multibus.
> src/qemu/qemu_capabilities.c-     * No extra conditions here for simplicity.
> src/qemu/qemu_capabilities.c-     */
> src/qemu/qemu_capabilities.c-    if (qemuDomainIsVirt(def))
> src/qemu/qemu_capabilities.c-        return true;
> src/qemu/qemu_capabilities.c-
> src/qemu/qemu_capabilities.c-    return false;
> src/qemu/qemu_capabilities.c-}
> --
> src/qemu/qemu_capabilities.c=virQEMUCapsSupportsVmport(virQEMUCapsPtr qemuCaps,
> src/qemu/qemu_capabilities.c-                          const virDomainDef *def)
> src/qemu/qemu_capabilities.c-{
> src/qemu/qemu_capabilities.c-    if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_VMPORT_OPT))
> src/qemu/qemu_capabilities.c-        return false;
> src/qemu/qemu_capabilities.c-
> src/qemu/qemu_capabilities.c-    return qemuDomainIsI440FX(def) ||
> src/qemu/qemu_capabilities.c-        qemuDomainIsQ35(def) ||
> src/qemu/qemu_capabilities.c:        STREQ(def->os.machine, "isapc");
> src/qemu/qemu_capabilities.c-}
>
> Lack of bus information on query-machines?
>
>
> --
> src/qemu/qemu_command.c=qemuBuildMemballoonCommandLine(virCommandPtr cmd,
> src/qemu/qemu_command.c-                               const virDomainDef *def,
> src/qemu/qemu_command.c-                               virQEMUCapsPtr qemuCaps)
> src/qemu/qemu_command.c-{
> src/qemu/qemu_command.c-    virBuffer buf = VIR_BUFFER_INITIALIZER;
> src/qemu/qemu_command.c-
> src/qemu/qemu_command.c:    if (STRPREFIX(def->os.machine, "s390-virtio") &&
> src/qemu/qemu_command.c-        virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390) && def->memballoon)
> src/qemu/qemu_command.c-        def->memballoon->model = VIR_DOMAIN_MEMBALLOON_MODEL_NONE;
>
>
> Lack of bus information on query-machines?
>
>
> [...]
> --
> src/qemu/qemu_domain.c=qemuDomainDefAddDefaultDevices(virDomainDefPtr def,
> src/qemu/qemu_domain.c-                               virQEMUCapsPtr qemuCaps)
> src/qemu/qemu_domain.c-{
> [...]
> src/qemu/qemu_domain.c:        if (STREQ(def->os.machine, "isapc")) {
> src/qemu/qemu_domain.c-            addDefaultUSB = false;
>
>
> Lack of bus/defaults information on query-machines?
>
> This whole function is a collection of cases where
> bus/device/defaults information is missing on query-machines:
>
> src/qemu/qemu_domain.c-            break;
> src/qemu/qemu_domain.c-        }
> src/qemu/qemu_domain.c-        if (qemuDomainIsQ35(def)) {
> src/qemu/qemu_domain.c-            addPCIeRoot = true;
> src/qemu/qemu_domain.c-            addImplicitSATA = true;
> [...]
> src/qemu/qemu_domain.c-        if (qemuDomainIsI440FX(def))
> src/qemu/qemu_domain.c-            addPCIRoot = true;
> [...]
> src/qemu/qemu_domain.c-        break;
> src/qemu/qemu_domain.c-
> src/qemu/qemu_domain.c-    case VIR_ARCH_ARMV7L:
> src/qemu/qemu_domain.c-    case VIR_ARCH_AARCH64:
> src/qemu/qemu_domain.c-        addDefaultUSB = false;
> src/qemu/qemu_domain.c-        addDefaultMemballoon = false;
> src/qemu/qemu_domain.c-        if (qemuDomainIsVirt(def))
> src/qemu/qemu_domain.c-            addPCIeRoot = virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_GPEX);
> src/qemu/qemu_domain.c-        break;
> src/qemu/qemu_domain.c-
> src/qemu/qemu_domain.c-    case VIR_ARCH_PPC64:
> src/qemu/qemu_domain.c-    case VIR_ARCH_PPC64LE:
> src/qemu/qemu_domain.c-        addPCIRoot = true;
> src/qemu/qemu_domain.c-        addDefaultUSBKBD = true;
> src/qemu/qemu_domain.c-        addDefaultUSBMouse = true;
> src/qemu/qemu_domain.c-        /* For pSeries guests, the firmware provides the same
> src/qemu/qemu_domain.c-         * functionality as the pvpanic device, so automatically
> src/qemu/qemu_domain.c-         * add the definition if not already present */
> src/qemu/qemu_domain.c-        if (qemuDomainIsPSeries(def))
> src/qemu/qemu_domain.c-            addPanicDevice = true;
> src/qemu/qemu_domain.c-        break;
> [...]
> --
> src/qemu/qemu_domain.c=qemuDomainDefaultNetModel(const virDomainDef *def,
> src/qemu/qemu_domain.c-                          virQEMUCapsPtr qemuCaps)
> src/qemu/qemu_domain.c-{
> src/qemu/qemu_domain.c-    if (ARCH_IS_S390(def->os.arch))
> src/qemu/qemu_domain.c-        return "virtio";
> src/qemu/qemu_domain.c-
> src/qemu/qemu_domain.c-    if (def->os.arch == VIR_ARCH_ARMV7L ||
> src/qemu/qemu_domain.c-        def->os.arch == VIR_ARCH_AARCH64) {
> src/qemu/qemu_domain.c:        if (STREQ(def->os.machine, "versatilepb"))
> src/qemu/qemu_domain.c-            return "smc91c111";
> src/qemu/qemu_domain.c-
> src/qemu/qemu_domain.c-        if (qemuDomainIsVirt(def))
> src/qemu/qemu_domain.c-            return "virtio";
> src/qemu/qemu_domain.c-
> src/qemu/qemu_domain.c-        /* Incomplete. vexpress (and a few others) use this, but not all
> src/qemu/qemu_domain.c-         * arm boards */
> src/qemu/qemu_domain.c-        return "lan9118";
>
>
> Lack of bus/defaults information on query-machines?
>
> [...]
> --
> src/qemu/qemu_domain.c=qemuDomainMachineIsQ35(const char *machine)
> src/qemu/qemu_domain.c-{
> src/qemu/qemu_domain.c:    return (STRPREFIX(machine, "pc-q35") ||
> src/qemu/qemu_domain.c:            STREQ(machine, "q35"));
> src/qemu/qemu_domain.c-}
>
> Need to grep for callers of this function.
>
> --
> src/qemu/qemu_domain.c=qemuDomainMachineIsI440FX(const char *machine)
> src/qemu/qemu_domain.c-{
> src/qemu/qemu_domain.c:    return (STREQ(machine, "pc") ||
> src/qemu/qemu_domain.c:            STRPREFIX(machine, "pc-0.") ||
> src/qemu/qemu_domain.c:            STRPREFIX(machine, "pc-1.") ||
> src/qemu/qemu_domain.c:            STRPREFIX(machine, "pc-i440") ||
> src/qemu/qemu_domain.c:            STRPREFIX(machine, "rhel"));
> src/qemu/qemu_domain.c-}
>
> Need to grep for callers of this function.
>
> ---
> src/qemu/qemu_domain.c=qemuDomainMachineNeedsFDC(const char *machine)
> src/qemu/qemu_domain.c-{
> src/qemu/qemu_domain.c:    const char *p = STRSKIP(machine, "pc-q35-");
> src/qemu/qemu_domain.c-
> src/qemu/qemu_domain.c-    if (p) {
> src/qemu/qemu_domain.c-        if (STRPREFIX(p, "1.") ||
> src/qemu/qemu_domain.c-            STRPREFIX(p, "2.0") ||
> src/qemu/qemu_domain.c-            STRPREFIX(p, "2.1") ||
> src/qemu/qemu_domain.c-            STRPREFIX(p, "2.2") ||
> src/qemu/qemu_domain.c-            STRPREFIX(p, "2.3"))
> src/qemu/qemu_domain.c-            return false;
> src/qemu/qemu_domain.c-        return true;
> src/qemu/qemu_domain.c-    }
> src/qemu/qemu_domain.c-    return false;
> src/qemu/qemu_domain.c-}
>
> Lack of bus/defaults information on query-machines.
>
>
> --
> src/qemu/qemu_domain.c=qemuDomainMachineIsS390CCW(const char *machine)
> src/qemu/qemu_domain.c-{
> src/qemu/qemu_domain.c:    return STRPREFIX(machine, "s390-ccw");
> src/qemu/qemu_domain.c-}
>
> Need to grep for callers of this function.
>
>
> --
> src/qemu/qemu_domain.c=qemuDomainMachineIsVirt(const char *machine,
> src/qemu/qemu_domain.c-                        const virArch arch)
> src/qemu/qemu_domain.c-{
> src/qemu/qemu_domain.c-    if (arch != VIR_ARCH_ARMV7L &&
> src/qemu/qemu_domain.c-        arch != VIR_ARCH_AARCH64)
> src/qemu/qemu_domain.c-        return false;
> src/qemu/qemu_domain.c-
> src/qemu/qemu_domain.c:    if (STRNEQ(machine, "virt") &&
> src/qemu/qemu_domain.c:        !STRPREFIX(machine, "virt-"))
> src/qemu/qemu_domain.c-        return false;
> src/qemu/qemu_domain.c-
> src/qemu/qemu_domain.c-    return true;
> src/qemu/qemu_domain.c-}
>
> Need to grep for callers of this function.
>
> --
> src/qemu/qemu_domain.c=qemuDomainMachineIsPSeries(const char *machine,
> src/qemu/qemu_domain.c-                           const virArch arch)
> src/qemu/qemu_domain.c-{
> src/qemu/qemu_domain.c-    if (!ARCH_IS_PPC64(arch))
> src/qemu/qemu_domain.c-        return false;
> src/qemu/qemu_domain.c-
> src/qemu/qemu_domain.c:    if (STRNEQ(machine, "pseries") &&
> src/qemu/qemu_domain.c:        !STRPREFIX(machine, "pseries-"))
> src/qemu/qemu_domain.c-        return false;
> src/qemu/qemu_domain.c-
> src/qemu/qemu_domain.c-    return true;
> src/qemu/qemu_domain.c-}
>
> Need to grep for callers of this function.
>
>
> --
> src/qemu/qemu_domain.c=qemuDomainMachineHasBuiltinIDE(const char *machine)
> src/qemu/qemu_domain.c-{
> src/qemu/qemu_domain.c-    return qemuDomainMachineIsI440FX(machine) ||
> src/qemu/qemu_domain.c:        STREQ(machine, "malta") ||
> src/qemu/qemu_domain.c:        STREQ(machine, "sun4u") ||
> src/qemu/qemu_domain.c:        STREQ(machine, "g3beige");
> src/qemu/qemu_domain.c-}
>
> Lack of bus/defaults information on query-machines.
>
>
> [...]
> --
> src/qemu/qemu_domain_address.c=qemuDomainAssignARMVirtioMMIOAddresses(virDomainDefPtr def,
> src/qemu/qemu_domain_address.c-                                       virQEMUCapsPtr qemuCaps)
> src/qemu/qemu_domain_address.c-{
> src/qemu/qemu_domain_address.c-    if (def->os.arch != VIR_ARCH_ARMV7L &&
> src/qemu/qemu_domain_address.c-        def->os.arch != VIR_ARCH_AARCH64)
> src/qemu/qemu_domain_address.c-        return;
> src/qemu/qemu_domain_address.c-
> src/qemu/qemu_domain_address.c:    if (!(STRPREFIX(def->os.machine, "vexpress-") ||
> src/qemu/qemu_domain_address.c-          qemuDomainIsVirt(def)))
> src/qemu/qemu_domain_address.c-        return;
>
> Lack of bus/device/defaults information on query-machines?
>
> [...]
> --
> src/qemu/qemu_domain_address.c=qemuDomainSupportsPCI(virDomainDefPtr def,
> src/qemu/qemu_domain_address.c-                      virQEMUCapsPtr qemuCaps)
> src/qemu/qemu_domain_address.c-{
> src/qemu/qemu_domain_address.c-    if ((def->os.arch != VIR_ARCH_ARMV7L) && (def->os.arch != VIR_ARCH_AARCH64))
> src/qemu/qemu_domain_address.c-        return true;
> src/qemu/qemu_domain_address.c-
> src/qemu/qemu_domain_address.c:    if (STREQ(def->os.machine, "versatilepb"))
> src/qemu/qemu_domain_address.c-        return true;
> src/qemu/qemu_domain_address.c-
> src/qemu/qemu_domain_address.c-    if (qemuDomainIsVirt(def) &&
> src/qemu/qemu_domain_address.c-        virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_GPEX))
> src/qemu/qemu_domain_address.c-        return true;
> src/qemu/qemu_domain_address.c-
>
> Lack of bus information on query-machines.
>
> [...]
>

Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target

[Markus Armbruster]

Daniel Henrique Barboza <danielhb@linux.ibm.com> writes:

> Hi,
>
> Sorry for the delay. I'll summarize what I've understood from the discussion
> so far:
>
> - query-target is the wrong place for this flag. query-machines is
> (less) wrong
> because it is not a static property of the machine object
>
> - a new "query-current-machine" can be created to host these dynamic
> properties that belongs to the current instance of the VM
>
> - there are machines in which the suspend support may vary with a
> "-no-acpi" option that would disable both the suspend and wake-up
> support. In this case, I see no problem into counting this flag into
> the logic (assuming it is possible, of course) and setting it as "false"
> if there is -no-acpi present (or even making the API returning "yes",
> "no" or "acpi" like Markus suggested) somewhere.
>
>
> Based on the last email from Eduardo, apparently there is a handful
> of other machine properties that can be hosted in either this new
> query-current-machine API or query-machines. I believe that this is
> more of a long term goal, but this new query-current-machine API
> would be a good kick-off and we should go for it.
>
> Is this a fair understanding? Did I miss something?

Sounds fair to me.

Adding query-current-machine on the evidence of just one flag would be
questionable, but Eduardo expects there to be more, so it's okay.

Whether a property is static or dynamic can change over time, which
makes the choice of query-machines vs. query-current-machine
non-trivial.  We better write down how we plan to handle mispredictions,
i.e. what to do when a property we put into query-machines turns out to
be dynamic, or a property we put into query-current-machine turns out to
be static, or we'd like query-machines to cover a property that is
static except for a few machines.

Eduardo, anything to add?

[Qemu-devel] [PATCH v7 2/3] qga: update guest-suspend-ram and guest-suspend-hybrid descriptions

[Daniel Henrique Barboza]

This patch updates the descriptions of 'guest-suspend-ram' and
'guest-suspend-hybrid' to mention that both commands relies now
on the proper support for wake up from suspend, retrieved by the
'wakeup-suspend-support' attribute of the 'query-target' QMP
command.

Reported-by: Balamuruhan S <bala24@linux.vnet.ibm.com>
Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/qapi-schema.json | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 17884c7c70..bc46c2d0f0 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -565,9 +565,9 @@
 # For the best results it's strongly recommended to have the pm-utils
 # package installed in the guest.
 #
-# IMPORTANT: guest-suspend-ram requires QEMU to support the 'system_wakeup'
-# command.  Thus, it's *required* to query QEMU for the presence of the
-# 'system_wakeup' command before issuing guest-suspend-ram.
+# IMPORTANT: guest-suspend-ram requires working wakeup support in
+# QEMU. You *must* check QMP command query-target returns
+# wakeup-suspend-support: true before issuing this command.
 #
 # This command does NOT return a response on success. There are two options
 # to check for success:
@@ -592,9 +592,9 @@
 #
 # This command requires the pm-utils package to be installed in the guest.
 #
-# IMPORTANT: guest-suspend-hybrid requires QEMU to support the 'system_wakeup'
-# command.  Thus, it's *required* to query QEMU for the presence of the
-# 'system_wakeup' command before issuing guest-suspend-hybrid.
+# IMPORTANT: guest-suspend-hybrid requires working wakeup support in
+# QEMU. You *must* check QMP command query-target returns
+# wakeup-suspend-support: true before issuing this command.
 #
 # This command does NOT return a response on success. There are two options
 # to check for success:
-- 
2.14.3

[Qemu-devel] [PATCH v7 3/3] qmp.c: system_wakeup: runstate and wake-up support check

[Daniel Henrique Barboza]

The qmp/hmp command 'system_wakeup' is simply a direct call to
'qemu_system_wakeup_request' from vl.c. This function verifies if
runstate is SUSPENDED and if the wake up reason is valid before
proceeding. However, no error or warning is thrown if any of those
pre-requirements isn't met. There is no way for the caller to
differentiate between a successful wakeup or an error state caused
when trying to wake up a guest that wasn't suspended.

This means that system_wakeup is silently failing, which can be
considered a bug. Adding error handling isn't an API break in this
case - applications that didn't check the result will remain broken,
the ones that check it will have a chance to deal with it.

Adding to that, recent changes in query-target added a new flag called
wakeup-suspend-support that indicates if the guest has the capability
of waking up from suspended state. Although such guest will never
reach SUSPENDED state and erroring it out in this scenario would
suffice, it is more informative for the user to differentiate between a
failure because the guest isn't suspended versus a failure because the
guest does not have support for wake up at all.

All this considered, this patch changes qmp_system_wakeup to:

- check if the guest has the capability to wake-up from suspend. If
not, error out informing about the lack of support;

- make the runstate verification before proceeding to call
qemu_system_wakeup_request, firing up an error message if the user tries
to wake up a machine that isn't in SUSPENDED state.

After this patch, this is the output of system_wakeup in a guest that
does not have wake-up from suspend support (ppc64):

(qemu) system_wakeup
wake-up from suspend is not supported by this guest
(qemu)

And this is the output of system_wakeup in a x86 guest that has the
support but isn't suspended:

(qemu) system_wakeup
Unable to wake up: guest is not in suspended state
(qemu)

Reported-by: Balamuruhan S <bala24@linux.vnet.ibm.com>
Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
---
 hmp.c          |  4 +++-
 qapi/misc.json |  3 ++-
 qmp.c          | 10 ++++++++++
 3 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/hmp.c b/hmp.c
index ef93f4878b..d177d14cc9 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1156,7 +1156,9 @@ void hmp_cont(Monitor *mon, const QDict *qdict)
 
 void hmp_system_wakeup(Monitor *mon, const QDict *qdict)
 {
-    qmp_system_wakeup(NULL);
+    Error *err = NULL;
+    qmp_system_wakeup(&err);
+    hmp_handle_error(mon, &err);
 }
 
 void hmp_nmi(Monitor *mon, const QDict *qdict)
diff --git a/qapi/misc.json b/qapi/misc.json
index efba0449a6..73bfc3e436 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -1247,7 +1247,8 @@
 #
 # If the guest has wake-up from suspend support enabled
 # (wakeup-suspend-support flag from query-target), wakeup guest from
-# suspend. Does nothing otherwise.
+# suspend if the guest is in SUSPENDED state. Returns an error
+# otherwise.
 #
 # Since:  1.1
 #
diff --git a/qmp.c b/qmp.c
index 25fdc9a5b2..5366e963ad 100644
--- a/qmp.c
+++ b/qmp.c
@@ -205,6 +205,16 @@ void qmp_cont(Error **errp)
 
 void qmp_system_wakeup(Error **errp)
 {
+    if (!qemu_wakeup_suspend_support()) {
+        error_setg(errp,
+                   "wake-up from suspend is not supported by this guest");
+        return;
+    }
+    if (!runstate_check(RUN_STATE_SUSPENDED)) {
+        error_setg(errp,
+                   "Unable to wake up: guest is not in suspended state");
+        return;
+    }
     qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
 }
 
-- 
2.14.3

Re: [Qemu-devel] [PATCH v7 3/3] qmp.c: system_wakeup: runstate and wake-up support check

[Markus Armbruster]

This affects both QMP and HMP.  Let's rephrase the title to

    qmp hmp: Make system_wakeup check wake-up support and run state

Can do that in my tree.

Daniel Henrique Barboza <danielhb@linux.ibm.com> writes:

> The qmp/hmp command 'system_wakeup' is simply a direct call to
> 'qemu_system_wakeup_request' from vl.c. This function verifies if
> runstate is SUSPENDED and if the wake up reason is valid before
> proceeding. However, no error or warning is thrown if any of those
> pre-requirements isn't met. There is no way for the caller to
> differentiate between a successful wakeup or an error state caused
> when trying to wake up a guest that wasn't suspended.
>
> This means that system_wakeup is silently failing, which can be
> considered a bug. Adding error handling isn't an API break in this
> case - applications that didn't check the result will remain broken,
> the ones that check it will have a chance to deal with it.
>
> Adding to that, recent changes in query-target added a new flag called

"... the commit before previous extended query-target with a new flag..."

> wakeup-suspend-support that indicates if the guest has the capability
> of waking up from suspended state. Although such guest will never
> reach SUSPENDED state and erroring it out in this scenario would
> suffice, it is more informative for the user to differentiate between a
> failure because the guest isn't suspended versus a failure because the
> guest does not have support for wake up at all.
>
> All this considered, this patch changes qmp_system_wakeup to:
>
> - check if the guest has the capability to wake-up from suspend. If
> not, error out informing about the lack of support;
>
> - make the runstate verification before proceeding to call
> qemu_system_wakeup_request, firing up an error message if the user tries
> to wake up a machine that isn't in SUSPENDED state.
>
> After this patch, this is the output of system_wakeup in a guest that
> does not have wake-up from suspend support (ppc64):
>
> (qemu) system_wakeup
> wake-up from suspend is not supported by this guest
> (qemu)
>
> And this is the output of system_wakeup in a x86 guest that has the
> support but isn't suspended:
>
> (qemu) system_wakeup
> Unable to wake up: guest is not in suspended state
> (qemu)

The examples are a nice touch.

> Reported-by: Balamuruhan S <bala24@linux.vnet.ibm.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
> ---
>  hmp.c          |  4 +++-
>  qapi/misc.json |  3 ++-
>  qmp.c          | 10 ++++++++++
>  3 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/hmp.c b/hmp.c
> index ef93f4878b..d177d14cc9 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1156,7 +1156,9 @@ void hmp_cont(Monitor *mon, const QDict *qdict)
>  
>  void hmp_system_wakeup(Monitor *mon, const QDict *qdict)
>  {
> -    qmp_system_wakeup(NULL);
> +    Error *err = NULL;

Blank line between declaration and statement, please.  Happy to touch
this up in my tree.

> +    qmp_system_wakeup(&err);
> +    hmp_handle_error(mon, &err);
>  }
>  
>  void hmp_nmi(Monitor *mon, const QDict *qdict)
> diff --git a/qapi/misc.json b/qapi/misc.json
> index efba0449a6..73bfc3e436 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -1247,7 +1247,8 @@
>  #
>  # If the guest has wake-up from suspend support enabled
>  # (wakeup-suspend-support flag from query-target), wakeup guest from
> -# suspend. Does nothing otherwise.
> +# suspend if the guest is in SUSPENDED state. Returns an error
> +# otherwise.
>  #
>  # Since:  1.1
>  #
> diff --git a/qmp.c b/qmp.c
> index 25fdc9a5b2..5366e963ad 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -205,6 +205,16 @@ void qmp_cont(Error **errp)
>  
>  void qmp_system_wakeup(Error **errp)
>  {
> +    if (!qemu_wakeup_suspend_support()) {
> +        error_setg(errp,
> +                   "wake-up from suspend is not supported by this guest");
> +        return;
> +    }
> +    if (!runstate_check(RUN_STATE_SUSPENDED)) {
> +        error_setg(errp,
> +                   "Unable to wake up: guest is not in suspended state");
> +        return;
> +    }
>      qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
>  }

Re: [Qemu-devel] [PATCH v7 3/3] qmp.c: system_wakeup: runstate and wake-up support check

[Daniel Henrique Barboza]

On 05/18/2018 05:48 AM, Markus Armbruster wrote:
> This affects both QMP and HMP.  Let's rephrase the title to
>
>      qmp hmp: Make system_wakeup check wake-up support and run state
>
> Can do that in my tree.

I can rename the commit name, and handle the other suggestions you
made below, in the respin I'll end up making due to the changes
about to happen in patch 1. No problem.

I've cited 'query-target' all over the patch series too. All these 
references
will need to be changed with the new API of the flag (query-machines seems
to be the favorite so far). It's easier to just resend everything 
up-to-date.


Thanks,


Daniel

>
> Daniel Henrique Barboza <danielhb@linux.ibm.com> writes:
>
>> The qmp/hmp command 'system_wakeup' is simply a direct call to
>> 'qemu_system_wakeup_request' from vl.c. This function verifies if
>> runstate is SUSPENDED and if the wake up reason is valid before
>> proceeding. However, no error or warning is thrown if any of those
>> pre-requirements isn't met. There is no way for the caller to
>> differentiate between a successful wakeup or an error state caused
>> when trying to wake up a guest that wasn't suspended.
>>
>> This means that system_wakeup is silently failing, which can be
>> considered a bug. Adding error handling isn't an API break in this
>> case - applications that didn't check the result will remain broken,
>> the ones that check it will have a chance to deal with it.
>>
>> Adding to that, recent changes in query-target added a new flag called
> "... the commit before previous extended query-target with a new flag..."
>
>> wakeup-suspend-support that indicates if the guest has the capability
>> of waking up from suspended state. Although such guest will never
>> reach SUSPENDED state and erroring it out in this scenario would
>> suffice, it is more informative for the user to differentiate between a
>> failure because the guest isn't suspended versus a failure because the
>> guest does not have support for wake up at all.
>>
>> All this considered, this patch changes qmp_system_wakeup to:
>>
>> - check if the guest has the capability to wake-up from suspend. If
>> not, error out informing about the lack of support;
>>
>> - make the runstate verification before proceeding to call
>> qemu_system_wakeup_request, firing up an error message if the user tries
>> to wake up a machine that isn't in SUSPENDED state.
>>
>> After this patch, this is the output of system_wakeup in a guest that
>> does not have wake-up from suspend support (ppc64):
>>
>> (qemu) system_wakeup
>> wake-up from suspend is not supported by this guest
>> (qemu)
>>
>> And this is the output of system_wakeup in a x86 guest that has the
>> support but isn't suspended:
>>
>> (qemu) system_wakeup
>> Unable to wake up: guest is not in suspended state
>> (qemu)
> The examples are a nice touch.
>
>> Reported-by: Balamuruhan S <bala24@linux.vnet.ibm.com>
>> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
>> ---
>>   hmp.c          |  4 +++-
>>   qapi/misc.json |  3 ++-
>>   qmp.c          | 10 ++++++++++
>>   3 files changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/hmp.c b/hmp.c
>> index ef93f4878b..d177d14cc9 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -1156,7 +1156,9 @@ void hmp_cont(Monitor *mon, const QDict *qdict)
>>   
>>   void hmp_system_wakeup(Monitor *mon, const QDict *qdict)
>>   {
>> -    qmp_system_wakeup(NULL);
>> +    Error *err = NULL;
> Blank line between declaration and statement, please.  Happy to touch
> this up in my tree.
>
>> +    qmp_system_wakeup(&err);
>> +    hmp_handle_error(mon, &err);
>>   }
>>   
>>   void hmp_nmi(Monitor *mon, const QDict *qdict)
>> diff --git a/qapi/misc.json b/qapi/misc.json
>> index efba0449a6..73bfc3e436 100644
>> --- a/qapi/misc.json
>> +++ b/qapi/misc.json
>> @@ -1247,7 +1247,8 @@
>>   #
>>   # If the guest has wake-up from suspend support enabled
>>   # (wakeup-suspend-support flag from query-target), wakeup guest from
>> -# suspend. Does nothing otherwise.
>> +# suspend if the guest is in SUSPENDED state. Returns an error
>> +# otherwise.
>>   #
>>   # Since:  1.1
>>   #
>> diff --git a/qmp.c b/qmp.c
>> index 25fdc9a5b2..5366e963ad 100644
>> --- a/qmp.c
>> +++ b/qmp.c
>> @@ -205,6 +205,16 @@ void qmp_cont(Error **errp)
>>   
>>   void qmp_system_wakeup(Error **errp)
>>   {
>> +    if (!qemu_wakeup_suspend_support()) {
>> +        error_setg(errp,
>> +                   "wake-up from suspend is not supported by this guest");
>> +        return;
>> +    }
>> +    if (!runstate_check(RUN_STATE_SUSPENDED)) {
>> +        error_setg(errp,
>> +                   "Unable to wake up: guest is not in suspended state");
>> +        return;
>> +    }
>>       qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
>>   }

Re: [Qemu-devel] [PATCH v7 3/3] qmp.c: system_wakeup: runstate and wake-up support check

[Markus Armbruster]

Daniel Henrique Barboza <danielhb@linux.ibm.com> writes:

> On 05/18/2018 05:48 AM, Markus Armbruster wrote:
>> This affects both QMP and HMP.  Let's rephrase the title to
>>
>>      qmp hmp: Make system_wakeup check wake-up support and run state
>>
>> Can do that in my tree.
>
> I can rename the commit name, and handle the other suggestions you
> made below, in the respin I'll end up making due to the changes
> about to happen in patch 1. No problem.
>
> I've cited 'query-target' all over the patch series too. All these
> references
> will need to be changed with the new API of the flag (query-machines seems
> to be the favorite so far). It's easier to just resend everything
> up-to-date.

Makes my life easier :)

However, I'd recommend to give people a bit more time to chime in on the
proper home of wakeup-suspend-support before you respin.