* [Qemu-devel] [PATCH v7 0/3] wakeup-from-suspend and system_wakeup changes @ 2018-05-17 19:23 Daniel Henrique Barboza 2018-05-17 19:23 ` [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target Daniel Henrique Barboza ` (2 more replies) 0 siblings, 3 replies; 22+ messages in thread From: Daniel Henrique Barboza @ 2018-05-17 19:23 UTC (permalink / raw) To: qemu-devel; +Cc: armbru, mdroth, eblake, dgilbert, 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target 2018-05-17 19:23 [Qemu-devel] [PATCH v7 0/3] wakeup-from-suspend and system_wakeup changes Daniel Henrique Barboza @ 2018-05-17 19:23 ` Daniel Henrique Barboza 2018-05-18 8:48 ` Markus Armbruster 2018-05-17 19:23 ` [Qemu-devel] [PATCH v7 2/3] qga: update guest-suspend-ram and guest-suspend-hybrid descriptions Daniel Henrique Barboza 2018-05-17 19:23 ` [Qemu-devel] [PATCH v7 3/3] qmp.c: system_wakeup: runstate and wake-up support check Daniel Henrique Barboza 2 siblings, 1 reply; 22+ messages in thread From: Daniel Henrique Barboza @ 2018-05-17 19:23 UTC (permalink / raw) To: qemu-devel; +Cc: armbru, mdroth, eblake, dgilbert, 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target 2018-05-17 19:23 ` [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target Daniel Henrique Barboza @ 2018-05-18 8:48 ` Markus Armbruster 2018-05-21 18:14 ` Eduardo Habkost 0 siblings, 1 reply; 22+ messages in thread From: Markus Armbruster @ 2018-05-18 8:48 UTC (permalink / raw) To: Daniel Henrique Barboza Cc: qemu-devel, dgilbert, mdroth, Eduardo Habkost, Stefano Stabellini, Anthony Perard, libvir-list, Michael S. Tsirkin, Igor Mammedov 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target 2018-05-18 8:48 ` Markus Armbruster @ 2018-05-21 18:14 ` Eduardo Habkost 2018-05-21 19:46 ` Daniel Henrique Barboza 0 siblings, 1 reply; 22+ messages in thread From: Eduardo Habkost @ 2018-05-21 18:14 UTC (permalink / raw) To: Markus Armbruster Cc: Daniel Henrique Barboza, qemu-devel, dgilbert, mdroth, Stefano Stabellini, Anthony Perard, libvir-list, Michael S. Tsirkin, Igor Mammedov 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target 2018-05-21 18:14 ` Eduardo Habkost @ 2018-05-21 19:46 ` Daniel Henrique Barboza 2018-05-21 20:26 ` Eduardo Habkost 0 siblings, 1 reply; 22+ messages in thread From: Daniel Henrique Barboza @ 2018-05-21 19:46 UTC (permalink / raw) To: Eduardo Habkost, Markus Armbruster Cc: Stefano Stabellini, Michael S. Tsirkin, libvir-list, mdroth, qemu-devel, Anthony Perard, Igor Mammedov, dgilbert 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target 2018-05-21 19:46 ` Daniel Henrique Barboza @ 2018-05-21 20:26 ` Eduardo Habkost 2018-05-23 9:17 ` Markus Armbruster 0 siblings, 1 reply; 22+ messages in thread From: Eduardo Habkost @ 2018-05-21 20:26 UTC (permalink / raw) To: Daniel Henrique Barboza Cc: Markus Armbruster, Stefano Stabellini, Michael S. Tsirkin, libvir-list, mdroth, qemu-devel, Anthony Perard, Igor Mammedov, dgilbert, Eric Blake 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target 2018-05-21 20:26 ` Eduardo Habkost @ 2018-05-23 9:17 ` Markus Armbruster 2018-05-23 12:27 ` Eduardo Habkost 0 siblings, 1 reply; 22+ messages in thread From: Markus Armbruster @ 2018-05-23 9:17 UTC (permalink / raw) To: Eduardo Habkost Cc: Daniel Henrique Barboza, Stefano Stabellini, Michael S. Tsirkin, libvir-list, mdroth, qemu-devel, Anthony Perard, Igor Mammedov, dgilbert 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 :) ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target 2018-05-23 9:17 ` Markus Armbruster @ 2018-05-23 12:27 ` Eduardo Habkost 2018-05-23 14:11 ` Daniel Henrique Barboza 2018-05-23 15:53 ` Markus Armbruster 0 siblings, 2 replies; 22+ messages in thread From: Eduardo Habkost @ 2018-05-23 12:27 UTC (permalink / raw) To: Markus Armbruster Cc: Daniel Henrique Barboza, Stefano Stabellini, Michael S. Tsirkin, libvir-list, mdroth, qemu-devel, Anthony Perard, Igor Mammedov, dgilbert 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target 2018-05-23 12:27 ` Eduardo Habkost @ 2018-05-23 14:11 ` Daniel Henrique Barboza 2018-05-23 15:53 ` Markus Armbruster 1 sibling, 0 replies; 22+ messages in thread From: Daniel Henrique Barboza @ 2018-05-23 14:11 UTC (permalink / raw) To: Eduardo Habkost, Markus Armbruster Cc: Stefano Stabellini, Michael S. Tsirkin, libvir-list, qemu-devel, mdroth, Anthony Perard, Igor Mammedov, dgilbert 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? > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target 2018-05-23 12:27 ` Eduardo Habkost 2018-05-23 14:11 ` Daniel Henrique Barboza @ 2018-05-23 15:53 ` Markus Armbruster 2018-05-24 18:57 ` Eduardo Habkost 1 sibling, 1 reply; 22+ messages in thread From: Markus Armbruster @ 2018-05-23 15:53 UTC (permalink / raw) To: Eduardo Habkost Cc: Stefano Stabellini, Michael S. Tsirkin, libvir-list, Daniel Henrique Barboza, qemu-devel, mdroth, Anthony Perard, Igor Mammedov, dgilbert 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target 2018-05-23 15:53 ` Markus Armbruster @ 2018-05-24 18:57 ` Eduardo Habkost 2018-05-25 6:30 ` Markus Armbruster 0 siblings, 1 reply; 22+ messages in thread From: Eduardo Habkost @ 2018-05-24 18:57 UTC (permalink / raw) To: Markus Armbruster Cc: Stefano Stabellini, Michael S. Tsirkin, libvir-list, Daniel Henrique Barboza, qemu-devel, mdroth, Anthony Perard, Igor Mammedov, dgilbert 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target 2018-05-24 18:57 ` Eduardo Habkost @ 2018-05-25 6:30 ` Markus Armbruster 2018-05-25 20:30 ` Eduardo Habkost 0 siblings, 1 reply; 22+ messages in thread From: Markus Armbruster @ 2018-05-25 6:30 UTC (permalink / raw) To: Eduardo Habkost Cc: Stefano Stabellini, Michael S. Tsirkin, libvir-list, Daniel Henrique Barboza, mdroth, qemu-devel, Anthony Perard, Igor Mammedov, dgilbert 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". [...] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target 2018-05-25 6:30 ` Markus Armbruster @ 2018-05-25 20:30 ` Eduardo Habkost 2018-05-28 7:23 ` Markus Armbruster 0 siblings, 1 reply; 22+ messages in thread From: Eduardo Habkost @ 2018-05-25 20:30 UTC (permalink / raw) To: Markus Armbruster Cc: Stefano Stabellini, Michael S. Tsirkin, libvir-list, Daniel Henrique Barboza, mdroth, qemu-devel, Anthony Perard, Igor Mammedov, dgilbert 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target 2018-05-25 20:30 ` Eduardo Habkost @ 2018-05-28 7:23 ` Markus Armbruster 2018-05-29 14:55 ` Eduardo Habkost 0 siblings, 1 reply; 22+ messages in thread From: Markus Armbruster @ 2018-05-28 7:23 UTC (permalink / raw) To: Eduardo Habkost Cc: Stefano Stabellini, Michael S. Tsirkin, libvir-list, Daniel Henrique Barboza, qemu-devel, mdroth, Anthony Perard, Igor Mammedov, dgilbert 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? ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target 2018-05-28 7:23 ` Markus Armbruster @ 2018-05-29 14:55 ` Eduardo Habkost 2018-06-19 20:29 ` Daniel Henrique Barboza 0 siblings, 1 reply; 22+ messages in thread From: Eduardo Habkost @ 2018-05-29 14:55 UTC (permalink / raw) To: Markus Armbruster Cc: Stefano Stabellini, Michael S. Tsirkin, libvir-list, Daniel Henrique Barboza, qemu-devel, mdroth, Anthony Perard, Igor Mammedov, dgilbert 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target 2018-05-29 14:55 ` Eduardo Habkost @ 2018-06-19 20:29 ` Daniel Henrique Barboza 2018-06-20 7:09 ` Markus Armbruster 0 siblings, 1 reply; 22+ messages in thread From: Daniel Henrique Barboza @ 2018-06-19 20:29 UTC (permalink / raw) To: Eduardo Habkost, Markus Armbruster Cc: Stefano Stabellini, Michael S. Tsirkin, libvir-list, mdroth, qemu-devel, Anthony Perard, Igor Mammedov, dgilbert, danielhb413 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. > > [...] > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target 2018-06-19 20:29 ` Daniel Henrique Barboza @ 2018-06-20 7:09 ` Markus Armbruster 0 siblings, 0 replies; 22+ messages in thread From: Markus Armbruster @ 2018-06-20 7:09 UTC (permalink / raw) To: Daniel Henrique Barboza Cc: Eduardo Habkost, Markus Armbruster, Stefano Stabellini, Michael S. Tsirkin, libvir-list, danielhb413, qemu-devel, mdroth, Anthony Perard, Igor Mammedov, dgilbert 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? ^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH v7 2/3] qga: update guest-suspend-ram and guest-suspend-hybrid descriptions 2018-05-17 19:23 [Qemu-devel] [PATCH v7 0/3] wakeup-from-suspend and system_wakeup changes Daniel Henrique Barboza 2018-05-17 19:23 ` [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target Daniel Henrique Barboza @ 2018-05-17 19:23 ` Daniel Henrique Barboza 2018-05-17 19:23 ` [Qemu-devel] [PATCH v7 3/3] qmp.c: system_wakeup: runstate and wake-up support check Daniel Henrique Barboza 2 siblings, 0 replies; 22+ messages in thread From: Daniel Henrique Barboza @ 2018-05-17 19:23 UTC (permalink / raw) To: qemu-devel; +Cc: armbru, mdroth, eblake, dgilbert, 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH v7 3/3] qmp.c: system_wakeup: runstate and wake-up support check 2018-05-17 19:23 [Qemu-devel] [PATCH v7 0/3] wakeup-from-suspend and system_wakeup changes Daniel Henrique Barboza 2018-05-17 19:23 ` [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target Daniel Henrique Barboza 2018-05-17 19:23 ` [Qemu-devel] [PATCH v7 2/3] qga: update guest-suspend-ram and guest-suspend-hybrid descriptions Daniel Henrique Barboza @ 2018-05-17 19:23 ` Daniel Henrique Barboza 2018-05-18 8:48 ` Markus Armbruster 2 siblings, 1 reply; 22+ messages in thread From: Daniel Henrique Barboza @ 2018-05-17 19:23 UTC (permalink / raw) To: qemu-devel; +Cc: armbru, mdroth, eblake, dgilbert, 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v7 3/3] qmp.c: system_wakeup: runstate and wake-up support check 2018-05-17 19:23 ` [Qemu-devel] [PATCH v7 3/3] qmp.c: system_wakeup: runstate and wake-up support check Daniel Henrique Barboza @ 2018-05-18 8:48 ` Markus Armbruster 2018-05-18 12:52 ` Daniel Henrique Barboza 0 siblings, 1 reply; 22+ messages in thread From: Markus Armbruster @ 2018-05-18 8:48 UTC (permalink / raw) To: Daniel Henrique Barboza; +Cc: qemu-devel, dgilbert, mdroth 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); > } ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v7 3/3] qmp.c: system_wakeup: runstate and wake-up support check 2018-05-18 8:48 ` Markus Armbruster @ 2018-05-18 12:52 ` Daniel Henrique Barboza 2018-05-18 15:00 ` Markus Armbruster 0 siblings, 1 reply; 22+ messages in thread From: Daniel Henrique Barboza @ 2018-05-18 12:52 UTC (permalink / raw) To: Markus Armbruster; +Cc: qemu-devel, dgilbert, mdroth 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); >> } ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v7 3/3] qmp.c: system_wakeup: runstate and wake-up support check 2018-05-18 12:52 ` Daniel Henrique Barboza @ 2018-05-18 15:00 ` Markus Armbruster 0 siblings, 0 replies; 22+ messages in thread From: Markus Armbruster @ 2018-05-18 15:00 UTC (permalink / raw) To: Daniel Henrique Barboza; +Cc: mdroth, qemu-devel, dgilbert 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2018-06-20 7:09 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-17 19:23 [Qemu-devel] [PATCH v7 0/3] wakeup-from-suspend and system_wakeup changes Daniel Henrique Barboza 2018-05-17 19:23 ` [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target Daniel Henrique Barboza 2018-05-18 8:48 ` Markus Armbruster 2018-05-21 18:14 ` Eduardo Habkost 2018-05-21 19:46 ` Daniel Henrique Barboza 2018-05-21 20:26 ` Eduardo Habkost 2018-05-23 9:17 ` Markus Armbruster 2018-05-23 12:27 ` Eduardo Habkost 2018-05-23 14:11 ` Daniel Henrique Barboza 2018-05-23 15:53 ` Markus Armbruster 2018-05-24 18:57 ` Eduardo Habkost 2018-05-25 6:30 ` Markus Armbruster 2018-05-25 20:30 ` Eduardo Habkost 2018-05-28 7:23 ` Markus Armbruster 2018-05-29 14:55 ` Eduardo Habkost 2018-06-19 20:29 ` Daniel Henrique Barboza 2018-06-20 7:09 ` Markus Armbruster 2018-05-17 19:23 ` [Qemu-devel] [PATCH v7 2/3] qga: update guest-suspend-ram and guest-suspend-hybrid descriptions Daniel Henrique Barboza 2018-05-17 19:23 ` [Qemu-devel] [PATCH v7 3/3] qmp.c: system_wakeup: runstate and wake-up support check Daniel Henrique Barboza 2018-05-18 8:48 ` Markus Armbruster 2018-05-18 12:52 ` Daniel Henrique Barboza 2018-05-18 15:00 ` Markus Armbruster
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.