wakeup-from-suspend and system_wakeup changes

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 <[email protected]>

Signed-off-by Daniel Henrique Barboza <[email protected]>

Cc'ing a few more people.

Quoted 43 lines · 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.

Quoted 7 lines

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 <[email protected]>

Signed-off-by Daniel Henrique Barboza <[email protected]>

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.

Quoted 8 lines

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?)

Quoted 8 lines · Eduardo Habkost

Quoted 8 lines

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.

Quoted 8 lines

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.

Quoted 42 lines · Daniel Henrique Barboza

Quoted 8 lines · Eduardo Habkost

Quoted 8 lines

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.

Quoted 8 lines

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.

Quoted 3 lines

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

Quoted 7 lines

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?

Quoted 1 line · Eduardo Habkost

On Mon, May 21, 2018 at 04:46:36PM -0300, Daniel Henrique Barboza wrote:

Quoted 11 lines

Quoted 7 lines

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.

Quoted 5 lines

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?

Quoted 5 lines

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

Quoted 2 lines · Markus Armbruster

Quoted 1 line · Eduardo Habkost

On Mon, May 21, 2018 at 04:46:36PM -0300, Daniel Henrique Barboza wrote:

[...]

Quoted 26 lines

Quoted 10 lines

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 7 lines

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 6 lines

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?

Quoted 6 lines

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.

Quoted 5 lines

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?

Quoted 2 lines · Markus Armbruster

Quoted 1 line · Eduardo Habkost

On Mon, May 21, 2018 at 04:46:36PM -0300, Daniel Henrique Barboza wrote:

[...]

Quoted 34 lines

Quoted 11 lines

Quoted 7 lines

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.

Quoted 5 lines

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

Quoted 2 lines

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

I think we should.

Quoted 9 lines

Quoted 4 lines

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.

Quoted 5 lines

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?

Quoted 4 lines

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.

Quoted 5 lines

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?

Quoted 38 lines · Eduardo Habkost

Quoted 2 lines · Markus Armbruster

Quoted 1 line · Eduardo Habkost

On Mon, May 21, 2018 at 04:46:36PM -0300, Daniel Henrique Barboza wrote:

[...]

Quoted 34 lines

Quoted 11 lines

Quoted 7 lines

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.

Quoted 5 lines

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

Quoted 4 lines

Quoted 2 lines

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?

Quoted 4 lines

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 :)

Quoted 9 lines

Quoted 6 lines

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.

Quoted 43 lines · Markus Armbruster

Quoted 41 lines · Eduardo Habkost

Quoted 2 lines · Markus Armbruster

Quoted 1 line · Eduardo Habkost

On Mon, May 21, 2018 at 04:46:36PM -0300, Daniel Henrique Barboza wrote:

[...]

Quoted 36 lines

Quoted 11 lines

Quoted 7 lines

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.

Quoted 5 lines

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?

Quoted 3 lines

Quoted 1 line

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.

Quoted 37 lines

Quoted 9 lines

Quoted 9 lines

Quoted 4 lines

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.

Quoted 8 lines

Quoted 5 lines

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.

Quoted 17 lines

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?

Quoted 4 lines

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.

Quoted 5 lines

Quoted 3 lines

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.

Quoted 1 line · Eduardo Habkost

On Wed, May 23, 2018 at 05:53:34PM +0200, Markus Armbruster wrote:

Quoted 16 lines

Quoted 10 lines

Quoted 8 lines

Quoted 4 lines

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.

Quoted 4 lines

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

Quoted 2 lines · Markus Armbruster

Quoted 1 line · Eduardo Habkost

On Wed, May 23, 2018 at 05:53:34PM +0200, Markus Armbruster wrote:

[...]

Quoted 25 lines

Quoted 16 lines

Quoted 10 lines

Quoted 8 lines

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 8 lines

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

Quoted 10 lines

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

Quoted 5 lines

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.

Quoted 2 lines

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 Habkost <[email protected]> writes:

Quoted 4 lines

[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?

Quoted 1 line · Markus Armbruster

Eduardo Habkost <[email protected]> writes:

[...]

Quoted 4 lines

Quoted 4 lines

[1] Doing a:

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

[...]

Quoted 2 lines

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.

[...]

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?

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 <[email protected]>

Signed-off-by Daniel Henrique Barboza <[email protected]>

---
 hmp.c          |  4 +++-
 qapi/misc.json |  3 ++-
 qmp.c          | 10 ++++++++++
 3 files changed, 15 insertions(+), 2 deletions(-)

hmp.c +0 -0

index ef93f4878b..d177d14cc9 100644

hmp.c +2 -0

+    qmp_system_wakeup(&err);
+    hmp_handle_error(mon, &err);
 }
 
 void hmp_nmi(Monitor *mon, const QDict *qdict)

qapi/misc.json +2 -1

index efba0449a6..73bfc3e436 100644
@@ -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
 #

qmp.c +10 -0

index 25fdc9a5b2..5366e963ad 100644
@@ -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);
 }

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.

Quoted 8 lines · 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.

Quoted 6 lines

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

Quoted 23 lines

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)

Quoted 6 lines

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.

Quoted 13 lines

Reported-by Balamuruhan S <[email protected]>

Signed-off-by Daniel Henrique Barboza <[email protected]>

---
  hmp.c          |  4 +++-
  qapi/misc.json |  3 ++-
  qmp.c          | 10 ++++++++++
  3 files changed, 15 insertions(+), 2 deletions(-)

hmp.c +0 -0

index ef93f4878b..d177d14cc9 100644

hmp.c void hmp_cont(Monitor *mon, const QDict *qdict) · line 1159

Quoted 5 lines

hmp.c +1 -1

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

Quoted 39 lines

hmp.c +2 -0

+    qmp_system_wakeup(&err);
+    hmp_handle_error(mon, &err);
  }
 
  void hmp_nmi(Monitor *mon, const QDict *qdict)

qapi/misc.json +2 -1

index efba0449a6..73bfc3e436 100644
@@ -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
  #

qmp.c +10 -0

index 25fdc9a5b2..5366e963ad 100644
@@ -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);
  }

Quoted 5 lines · 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.

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.