[libvirt] [PATCH v3 0/4] Don't pm suspend guest if it's unable to wake up
This is v3 of: https://www.redhat.com/archives/libvir-list/2019-April/msg00690.html I've taken Daniel's patches and reworked them. Diff to v2: - Patches 1-2/3 are split into three patches - Capability is checked for when querying other capabilities - Fixed a small security issue in 3/3 Daniel Henrique Barboza (2): qemu_capabilities: Add QEMU_CAPS_QUERY_CURRENT_MACHINE qemuDomainPMSuspendForDuration: check for QEMU_CAPS_PM_WAKEUP_SUPPORT Michal Prívozník (2): qemu_monitor: Introduce handler for 'query-current-machine' command qemu_capabilities: Introduce QEMU_CAPS_PM_WAKEUP_SUPPORT src/qemu/qemu_capabilities.c | 26 ++++++++++ src/qemu/qemu_capabilities.h | 4 ++ src/qemu/qemu_driver.c | 21 ++++++++ src/qemu/qemu_monitor.c | 10 ++++ src/qemu/qemu_monitor.h | 9 ++++ src/qemu/qemu_monitor_json.c | 50 +++++++++++++++++++ src/qemu/qemu_monitor_json.h | 5 ++ .../caps_4.0.0.riscv32.replies | 11 ++++ .../caps_4.0.0.riscv32.xml | 2 + .../caps_4.0.0.riscv64.replies | 11 ++++ .../caps_4.0.0.riscv64.xml | 2 + .../caps_4.0.0.x86_64.replies | 11 ++++ .../caps_4.0.0.x86_64.xml | 2 + 13 files changed, 164 insertions(+) -- 2.21.0
From: Daniel Henrique Barboza <danielhb413@gmail.com> QEMU commit 46ea94ca9cf ("qmp: query-current-machine with wakeup-suspend-support") added a new QMP command called 'query-current-machine' that retrieves guest parameters that can vary in the same machine model (e.g. ACPI support for x86 VMs depends on the '--no-acpi' option). Currently, this API has a single flag, 'wakeup-suspend-support', that indicates whether the guest has the capability of waking up from suspended state. Introduce a libvirt capability that reflects whether qemu has the monitor command. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml | 1 + tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml | 1 + tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml | 1 + 5 files changed, 6 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index a2de8630cd..5b3a2d0c33 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -524,6 +524,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "nvdimm.unarmed", "scsi-disk.device_id", "virtio-pci-non-transitional", + "query-current-machine", ); @@ -969,6 +970,7 @@ struct virQEMUCapsStringFlags virQEMUCapsCommands[] = { { "query-cpus-fast", QEMU_CAPS_QUERY_CPUS_FAST }, { "qom-list-properties", QEMU_CAPS_QOM_LIST_PROPERTIES }, { "blockdev-del", QEMU_CAPS_BLOCKDEV_DEL }, + { "query-current-machine", QEMU_CAPS_QUERY_CURRENT_MACHINE }, }; struct virQEMUCapsStringFlags virQEMUCapsMigration[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 9abeed9014..d3f90e82a8 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -507,6 +507,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_DEVICE_NVDIMM_UNARMED, /* -device nvdimm,unarmed= */ QEMU_CAPS_SCSI_DISK_DEVICE_ID, /* 'device_id' property of scsi disk */ QEMU_CAPS_VIRTIO_PCI_TRANSITIONAL, /* virtio *-pci-{non-}transitional devices */ + QEMU_CAPS_QUERY_CURRENT_MACHINE, /* query-current-machine command */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml b/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml index a8b5eb61c5..9d20be128a 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml @@ -163,6 +163,7 @@ <flag name='memory-backend-file.pmem'/> <flag name='scsi-disk.device_id'/> <flag name='virtio-pci-non-transitional'/> + <flag name='query-current-machine'/> <version>3001091</version> <kvmVersion>0</kvmVersion> <microcodeVersion>0</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml b/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml index 311740f34f..a1a59e14cb 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml @@ -163,6 +163,7 @@ <flag name='memory-backend-file.pmem'/> <flag name='scsi-disk.device_id'/> <flag name='virtio-pci-non-transitional'/> + <flag name='query-current-machine'/> <version>3001091</version> <kvmVersion>0</kvmVersion> <microcodeVersion>0</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml index 4585ff9e07..899c0cf7c8 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml @@ -201,6 +201,7 @@ <flag name='nvdimm.unarmed'/> <flag name='scsi-disk.device_id'/> <flag name='virtio-pci-non-transitional'/> + <flag name='query-current-machine'/> <version>3001050</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100758</microcodeVersion> -- 2.21.0
So far, this command returns a structure with only one member: 'wakeup-suspend-support'. But that's okay. It's what we are after anyway. Based-on-work-of: Daniel Henrique Barboza <danielhb413@gmail.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_monitor.c | 10 ++++++++ src/qemu/qemu_monitor.h | 9 +++++++ src/qemu/qemu_monitor_json.c | 50 ++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 5 ++++ 4 files changed, 74 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index babcbde878..e1fcbac13f 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4472,3 +4472,13 @@ qemuMonitorGetPRManagerInfo(qemuMonitorPtr mon, virHashFree(info); return ret; } + + +int +qemuMonitorGetCurrentMachineInfo(qemuMonitorPtr mon, + qemuMonitorCurrentMachineInfoPtr info) +{ + QEMU_CHECK_MONITOR(mon); + + return qemuMonitorJSONGetCurrentMachineInfo(mon, info); +} diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index caf62af5e2..9242d37407 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1221,4 +1221,13 @@ struct _qemuMonitorPRManagerInfo { int qemuMonitorGetPRManagerInfo(qemuMonitorPtr mon, virHashTablePtr *retinfo); +typedef struct _qemuMonitorCurrentMachineInfo qemuMonitorCurrentMachineInfo; +typedef qemuMonitorCurrentMachineInfo *qemuMonitorCurrentMachineInfoPtr; +struct _qemuMonitorCurrentMachineInfo { + bool wakeupSuspendSupport; +}; + +int qemuMonitorGetCurrentMachineInfo(qemuMonitorPtr mon, + qemuMonitorCurrentMachineInfoPtr info); + #endif /* LIBVIRT_QEMU_MONITOR_H */ diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index c0564fdc2b..5ef62dbbcc 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -8450,3 +8450,53 @@ qemuMonitorJSONGetPRManagerInfo(qemuMonitorPtr mon, return ret; } + + +static int +qemuMonitorJSONExtractCurrentMachineInfo(virJSONValuePtr reply, + qemuMonitorCurrentMachineInfoPtr info) +{ + virJSONValuePtr data; + + data = virJSONValueObjectGetObject(reply, "return"); + if (!data) + goto malformed; + + if (virJSONValueObjectGetBoolean(data, "wakeup-suspend-support", + &info->wakeupSuspendSupport) < 0) + goto malformed; + + return 0; + + malformed: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed qemu-current-machine reply")); + return -1; +} + + +int +qemuMonitorJSONGetCurrentMachineInfo(qemuMonitorPtr mon, + qemuMonitorCurrentMachineInfoPtr info) +{ + int ret = -1; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + + if (!(cmd = qemuMonitorJSONMakeCommand("query-current-machine", + NULL))) + return -1; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + goto cleanup; + + if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0) + goto cleanup; + + ret = qemuMonitorJSONExtractCurrentMachineInfo(reply, info); + + cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index c10513da15..746b7072ca 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -576,4 +576,9 @@ int qemuMonitorJSONGetPRManagerInfo(qemuMonitorPtr mon, virHashTablePtr info) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int +qemuMonitorJSONGetCurrentMachineInfo(qemuMonitorPtr mon, + qemuMonitorCurrentMachineInfoPtr info) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + #endif /* LIBVIRT_QEMU_MONITOR_JSON_H */ -- 2.21.0
This capability tells whether qemu is capable of waking up the guest from PM suspend. Based-on-work-of: Daniel Henrique Barboza <danielhb413@gmail.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_capabilities.c | 24 +++++++++++++++++++ src/qemu/qemu_capabilities.h | 3 +++ .../caps_4.0.0.riscv32.replies | 11 +++++++++ .../caps_4.0.0.riscv32.xml | 1 + .../caps_4.0.0.riscv64.replies | 11 +++++++++ .../caps_4.0.0.riscv64.xml | 1 + .../caps_4.0.0.x86_64.replies | 11 +++++++++ .../caps_4.0.0.x86_64.xml | 1 + 8 files changed, 63 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 5b3a2d0c33..389986d924 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -525,6 +525,9 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "scsi-disk.device_id", "virtio-pci-non-transitional", "query-current-machine", + + /* 330 */ + "wakeup-suspend-support", ); @@ -2769,6 +2772,25 @@ virQEMUCapsProbeQMPSEVCapabilities(virQEMUCapsPtr qemuCaps, } +static int +virQEMUCapsProbeQMPCurrentMachine(virQEMUCapsPtr qemuCaps, + qemuMonitorPtr mon) +{ + qemuMonitorCurrentMachineInfo info = { 0 }; + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CURRENT_MACHINE)) + return 0; + + if (qemuMonitorGetCurrentMachineInfo(mon, &info) < 0) + return -1; + + if (info.wakeupSuspendSupport) + virQEMUCapsSet(qemuCaps, QEMU_CAPS_PM_WAKEUP_SUPPORT); + + return 0; +} + + bool virQEMUCapsCPUFilterFeatures(const char *name, void *opaque) @@ -4373,6 +4395,8 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, return -1; if (virQEMUCapsProbeQMPSEVCapabilities(qemuCaps, mon) < 0) return -1; + if (virQEMUCapsProbeQMPCurrentMachine(qemuCaps, mon) < 0) + return -1; virQEMUCapsInitProcessCaps(qemuCaps); diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index d3f90e82a8..721092b91b 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -509,6 +509,9 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_VIRTIO_PCI_TRANSITIONAL, /* virtio *-pci-{non-}transitional devices */ QEMU_CAPS_QUERY_CURRENT_MACHINE, /* query-current-machine command */ + /* 330 */ + QEMU_CAPS_PM_WAKEUP_SUPPORT, /* domain has wake-up from suspend support */ + QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.replies b/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.replies index c7dac44289..a4d5786301 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.replies +++ b/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.replies @@ -17756,3 +17756,14 @@ ], "id": "libvirt-39" } + +{ + "execute": "query-current-machine", + "id": "libvirt-40" +} + +{ + "return": { + "wakeup-suspend-support": true + } +} diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml b/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml index 9d20be128a..c82b0d5e74 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml @@ -164,6 +164,7 @@ <flag name='scsi-disk.device_id'/> <flag name='virtio-pci-non-transitional'/> <flag name='query-current-machine'/> + <flag name='wakeup-suspend-support'/> <version>3001091</version> <kvmVersion>0</kvmVersion> <microcodeVersion>0</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.replies b/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.replies index 6fda8ad2d2..f2d209cd91 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.replies +++ b/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.replies @@ -17756,3 +17756,14 @@ ], "id": "libvirt-39" } + +{ + "execute": "query-current-machine", + "id": "libvirt-40" +} + +{ + "return": { + "wakeup-suspend-support": true + } +} diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml b/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml index a1a59e14cb..bf7f95e3b1 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml @@ -164,6 +164,7 @@ <flag name='scsi-disk.device_id'/> <flag name='virtio-pci-non-transitional'/> <flag name='query-current-machine'/> + <flag name='wakeup-suspend-support'/> <version>3001091</version> <kvmVersion>0</kvmVersion> <microcodeVersion>0</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.replies b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.replies index aa9ee38c80..47ed75d359 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.replies +++ b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.replies @@ -21052,6 +21052,17 @@ } } +{ + "execute": "query-current-machine", + "id": "libvirt-50" +} + +{ + "return": { + "wakeup-suspend-support": true + } +} + { "execute": "qmp_capabilities", "id": "libvirt-1" diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml index 899c0cf7c8..df1b0da731 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml @@ -202,6 +202,7 @@ <flag name='scsi-disk.device_id'/> <flag name='virtio-pci-non-transitional'/> <flag name='query-current-machine'/> + <flag name='wakeup-suspend-support'/> <version>3001050</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100758</microcodeVersion> -- 2.21.0
On Thu, Apr 11, 2019 at 10:25:11 +0200, Michal Privoznik wrote:
This capability tells whether qemu is capable of waking up the guest from PM suspend.
Based-on-work-of: Daniel Henrique Barboza <danielhb413@gmail.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_capabilities.c | 24 +++++++++++++++++++ src/qemu/qemu_capabilities.h | 3 +++ .../caps_4.0.0.riscv32.replies | 11 +++++++++ .../caps_4.0.0.riscv32.xml | 1 + .../caps_4.0.0.riscv64.replies | 11 +++++++++ .../caps_4.0.0.riscv64.xml | 1 + .../caps_4.0.0.x86_64.replies | 11 +++++++++ .../caps_4.0.0.x86_64.xml | 1 + 8 files changed, 63 insertions(+)
[...]
bool virQEMUCapsCPUFilterFeatures(const char *name, void *opaque) @@ -4373,6 +4395,8 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, return -1; if (virQEMUCapsProbeQMPSEVCapabilities(qemuCaps, mon) < 0) return -1; + if (virQEMUCapsProbeQMPCurrentMachine(qemuCaps, mon) < 0) + return -1;
This seems wrong by definition. The function is supposed to query current machine, but the capability lookup code uses 'none' machine type. IIUC the support for wakeup in some cases depends on the presence of ACPI in the guest and thus really can't be cached this way.
diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.replies b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.replies index aa9ee38c80..47ed75d359 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.replies +++ b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.replies @@ -21052,6 +21052,17 @@ } }
+{ + "execute": "query-current-machine", + "id": "libvirt-50" +} + +{ + "return": { + "wakeup-suspend-support": true + }
Did you gather this from an actual qemu run? I get the following when using 'none' machine: $ ./x86_64-softmmu/qemu-system-x86_64 -qmp stdio -machine none {"QMP": {"version": {"qemu": {"micro": 92, "minor": 1, "major": 3}, "package": "v4.0.0-rc2-24-gbcdb5721dd"}, "capabilities": ["oob"]}} VNC server running on ::1:5900 {"execute":"qmp_capabilities"} {"return": {}} {"execute":"query-current-machine"} {"return": {"wakeup-suspend-support": false}} Same result is with this patch applied.
On 4/11/19 10:39 AM, Peter Krempa wrote:
On Thu, Apr 11, 2019 at 10:25:11 +0200, Michal Privoznik wrote:
This capability tells whether qemu is capable of waking up the guest from PM suspend.
Based-on-work-of: Daniel Henrique Barboza <danielhb413@gmail.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_capabilities.c | 24 +++++++++++++++++++ src/qemu/qemu_capabilities.h | 3 +++ .../caps_4.0.0.riscv32.replies | 11 +++++++++ .../caps_4.0.0.riscv32.xml | 1 + .../caps_4.0.0.riscv64.replies | 11 +++++++++ .../caps_4.0.0.riscv64.xml | 1 + .../caps_4.0.0.x86_64.replies | 11 +++++++++ .../caps_4.0.0.x86_64.xml | 1 + 8 files changed, 63 insertions(+)
[...]
bool virQEMUCapsCPUFilterFeatures(const char *name, void *opaque) @@ -4373,6 +4395,8 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, return -1; if (virQEMUCapsProbeQMPSEVCapabilities(qemuCaps, mon) < 0) return -1; + if (virQEMUCapsProbeQMPCurrentMachine(qemuCaps, mon) < 0) + return -1;
This seems wrong by definition. The function is supposed to query current machine, but the capability lookup code uses 'none' machine type. IIUC the support for wakeup in some cases depends on the presence of ACPI in the guest and thus really can't be cached this way.
Yep, I was just about to write this but then got sidetracked. So I see two options here: 1) Query if guest is capable of PM suspend at runtime. However, this has to be done in a clever way. It would need to be done not only in qemuProcessLaunch() (or some subordinate functions), it would need to be done in qemuProcessReconnect() too (to cover case when user is upgrading libvirt). For this, qemuMonitorConnect() looks like the best place - it's called from both locations and it also already contains some 'runtime capabilities' querying. Except not really - the migration capabilities queried there are stored in @priv->migrationCaps rather than priv->qemuCaps. Problem with storing runtime capabilities in priv->qemuCaps is that they would get formatted into status XML. And this opens whole different can of worms. For instance, this feature relies on 'query-commands' to see if 'query-current-machine' command is available. Well, we can't re-run 'query-commands' in runtime because that might change the set of qemuCaps used when constructing cmd line. Way around this is to have 'runtime only' capabilities, that would not be stored in status XML. At some point we will need those, but writing that infrastructure is not easy and a bit too much to ask for this small fix. Especially, when we have another option: 2) Issue 'query-current-machine' from qemuDomainPMSuspendForDuration(). In every run. This is a bit suboptimal, but how frequently do we expect this API to be called? Caching qemu capabilities is there to speed up decission process which makes sense if we'd be making the decission 1000 times a second (e.g. building cmd line). With this approach the slowdown would be negligible. Of course, we would need to run the command in 'silent' mode, i.e. not report any error if qemu doesn't know it. Michal
On 4/11/19 6:58 AM, Michal Privoznik wrote:
On 4/11/19 10:39 AM, Peter Krempa wrote:
On Thu, Apr 11, 2019 at 10:25:11 +0200, Michal Privoznik wrote:
This capability tells whether qemu is capable of waking up the guest from PM suspend.
Based-on-work-of: Daniel Henrique Barboza <danielhb413@gmail.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_capabilities.c | 24 +++++++++++++++++++ src/qemu/qemu_capabilities.h | 3 +++ .../caps_4.0.0.riscv32.replies | 11 +++++++++ .../caps_4.0.0.riscv32.xml | 1 + .../caps_4.0.0.riscv64.replies | 11 +++++++++ .../caps_4.0.0.riscv64.xml | 1 + .../caps_4.0.0.x86_64.replies | 11 +++++++++ .../caps_4.0.0.x86_64.xml | 1 + 8 files changed, 63 insertions(+)
[...]
bool virQEMUCapsCPUFilterFeatures(const char *name, void *opaque) @@ -4373,6 +4395,8 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, return -1; if (virQEMUCapsProbeQMPSEVCapabilities(qemuCaps, mon) < 0) return -1; + if (virQEMUCapsProbeQMPCurrentMachine(qemuCaps, mon) < 0) + return -1;
This seems wrong by definition. The function is supposed to query current machine, but the capability lookup code uses 'none' machine type. IIUC the support for wakeup in some cases depends on the presence of ACPI in the guest and thus really can't be cached this way.
Yep, I was just about to write this but then got sidetracked.
So I see two options here: 1) Query if guest is capable of PM suspend at runtime. However, this has to be done in a clever way. It would need to be done not only in qemuProcessLaunch() (or some subordinate functions), it would need to be done in qemuProcessReconnect() too (to cover case when user is upgrading libvirt). For this, qemuMonitorConnect() looks like the best place - it's called from both locations and it also already contains some 'runtime capabilities' querying. Except not really - the migration capabilities queried there are stored in @priv->migrationCaps rather than priv->qemuCaps. Problem with storing runtime capabilities in priv->qemuCaps is that they would get formatted into status XML. And this opens whole different can of worms. For instance, this feature relies on 'query-commands' to see if 'query-current-machine' command is available. Well, we can't re-run 'query-commands' in runtime because that might change the set of qemuCaps used when constructing cmd line. Way around this is to have 'runtime only' capabilities, that would not be stored in status XML. At some point we will need those, but writing that infrastructure is not easy and a bit too much to ask for this small fix. Especially, when we have another option:
2) Issue 'query-current-machine' from qemuDomainPMSuspendForDuration(). In every run. This is a bit suboptimal, but how frequently do we expect this API to be called? Caching qemu capabilities is there to speed up decission process which makes sense if we'd be making the decission 1000 times a second (e.g. building cmd line). With this approach the slowdown would be negligible. Of course, we would need to run the command in 'silent' mode, i.e. not report any error if qemu doesn't know it.
I think the root issue in this series is that I made a mistake by calling wake-up support a capability. In Libvirt terms, it really doesn't feel like it - it can't be polled inside qemu_caps because runtime parameters such as '--no-acpi' can change its value, but at the same time it feels weird to populate qemuCaps in qemuProcess time. All that said, I think a good solution would be (2). dompmsuspend isn't a performance sensitive command, thus the extra time to execute the API can be ignored. Also, we can still check for QEMU_CAPS_QUERY_CURRENT_MACHINE inside qemuDomainPMSuspendForDuration to avoid firing up an error in a QEMU version that doesn't know the API. Thanks, DHB
Michal
On 4/11/19 12:12 PM, Daniel Henrique Barboza wrote:
<snip/>
All that said, I think a good solution would be (2). dompmsuspend isn't a performance sensitive command, thus the extra time to execute the API can be ignored. Also, we can still check for QEMU_CAPS_QUERY_CURRENT_MACHINE inside qemuDomainPMSuspendForDuration to avoid firing up an error in a QEMU version that doesn't know the API.
That's the thing. We can't. The capability doesn't exist yet. So every domain that is currently running thinks that qemu doesn't have the capability. And when updating libvirt to say next release only freshly started domains might/will have the capability. For all already running domains libvirt thinks the capability is not there. Only freshly started domains will get the capability. IOW, upgrading libvirt won't help you unless you restart your domains (might not want to do that). What I'm suggesting is to just execute the command, do not call qemuMonitorJSONCheckReply() and try to fetch info from the reply anyway. For instance, this is how qemuMonitorJSONGetKVMState() works. Michal
On Thu, Apr 11, 2019 at 13:37:49 +0200, Michal Privoznik wrote:
On 4/11/19 12:12 PM, Daniel Henrique Barboza wrote:
<snip/>
All that said, I think a good solution would be (2). dompmsuspend isn't a performance sensitive command, thus the extra time to execute the API can be ignored. Also, we can still check for QEMU_CAPS_QUERY_CURRENT_MACHINE inside qemuDomainPMSuspendForDuration to avoid firing up an error in a QEMU version that doesn't know the API.
That's the thing. We can't. The capability doesn't exist yet. So every domain that is currently running thinks that qemu doesn't have the capability. And when updating libvirt to say next release only freshly started domains might/will have the capability. For all already running domains libvirt thinks the capability is not there. Only freshly started domains will get the capability. IOW, upgrading libvirt won't help you unless you restart your domains (might not want to do that).
If you just go ahead with the PM suspend in case the capability bit for the presence of query-current-machine is not present you just will keep the existing behaviour. I think that is perfectly tolerable. Error should be reported only if query-current-machine is supported and it returns false for wakeup-suspend-support. (Obviously also if our caps indicate that query-current-machine is supported but returns an error).
On 4/11/19 8:43 AM, Peter Krempa wrote:
On 4/11/19 12:12 PM, Daniel Henrique Barboza wrote:
<snip/>
All that said, I think a good solution would be (2). dompmsuspend isn't a performance sensitive command, thus the extra time to execute the API can be ignored. Also, we can still check for QEMU_CAPS_QUERY_CURRENT_MACHINE inside qemuDomainPMSuspendForDuration to avoid firing up an error in a QEMU version that doesn't know the API. That's the thing. We can't. The capability doesn't exist yet. So every domain that is currently running thinks that qemu doesn't have the capability. And when updating libvirt to say next release only freshly started domains might/will have the capability. For all already running domains libvirt thinks the capability is not there. Only freshly started domains will get the capability. IOW, upgrading libvirt won't help you unless you restart your domains (might not want to do that). If you just go ahead with the PM suspend in case the capability bit for
On Thu, Apr 11, 2019 at 13:37:49 +0200, Michal Privoznik wrote: the presence of query-current-machine is not present you just will keep the existing behaviour. I think that is perfectly tolerable.
Error should be reported only if query-current-machine is supported and it returns false for wakeup-suspend-support. (Obviously also if our caps indicate that query-current-machine is supported but returns an error).
I agree with this approach. Michal, do you agree agree with this? If so, let me know if you plan to re-spin this series again. Otherwise I can do it. Thanks, DHB
On 4/17/19 8:27 PM, Daniel Henrique Barboza wrote:
On 4/11/19 8:43 AM, Peter Krempa wrote:
On 4/11/19 12:12 PM, Daniel Henrique Barboza wrote:
<snip/>
All that said, I think a good solution would be (2). dompmsuspend isn't a performance sensitive command, thus the extra time to execute the API can be ignored. Also, we can still check for QEMU_CAPS_QUERY_CURRENT_MACHINE inside qemuDomainPMSuspendForDuration to avoid firing up an error in a QEMU version that doesn't know the API. That's the thing. We can't. The capability doesn't exist yet. So every domain that is currently running thinks that qemu doesn't have the capability. And when updating libvirt to say next release only freshly started domains might/will have the capability. For all already running domains libvirt thinks the capability is not there. Only freshly started domains will get the capability. IOW, upgrading libvirt won't help you unless you restart your domains (might not want to do that). If you just go ahead with the PM suspend in case the capability bit for
On Thu, Apr 11, 2019 at 13:37:49 +0200, Michal Privoznik wrote: the presence of query-current-machine is not present you just will keep the existing behaviour. I think that is perfectly tolerable.
Error should be reported only if query-current-machine is supported and it returns false for wakeup-suspend-support. (Obviously also if our caps indicate that query-current-machine is supported but returns an error).
I agree with this approach.
Michal, do you agree agree with this? If so, let me know if you plan to re-spin this series again. Otherwise I can do it.
My only concern is that we don't recreate capabilities for running domains. That means that domains which are already running won't benefit from this feature. On the other hand, I guess this is almost a corner case and thus probably not worth spending too much time on it. So I say go for it. I don't plan to post patches anytime soon (burried under tonns of other stuff) so if you want to post the patches, please do. Michal
On Thu, Apr 11, 2019 at 11:58:04 +0200, Michal Privoznik wrote:
On 4/11/19 10:39 AM, Peter Krempa wrote:
On Thu, Apr 11, 2019 at 10:25:11 +0200, Michal Privoznik wrote:
This capability tells whether qemu is capable of waking up the guest from PM suspend.
Based-on-work-of: Daniel Henrique Barboza <danielhb413@gmail.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_capabilities.c | 24 +++++++++++++++++++ src/qemu/qemu_capabilities.h | 3 +++ .../caps_4.0.0.riscv32.replies | 11 +++++++++ .../caps_4.0.0.riscv32.xml | 1 + .../caps_4.0.0.riscv64.replies | 11 +++++++++ .../caps_4.0.0.riscv64.xml | 1 + .../caps_4.0.0.x86_64.replies | 11 +++++++++ .../caps_4.0.0.x86_64.xml | 1 + 8 files changed, 63 insertions(+)
[...]
bool virQEMUCapsCPUFilterFeatures(const char *name, void *opaque) @@ -4373,6 +4395,8 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, return -1; if (virQEMUCapsProbeQMPSEVCapabilities(qemuCaps, mon) < 0) return -1; + if (virQEMUCapsProbeQMPCurrentMachine(qemuCaps, mon) < 0) + return -1;
This seems wrong by definition. The function is supposed to query current machine, but the capability lookup code uses 'none' machine type. IIUC the support for wakeup in some cases depends on the presence of ACPI in the guest and thus really can't be cached this way.
Yep, I was just about to write this but then got sidetracked.
So I see two options here: 1) Query if guest is capable of PM suspend at runtime. However, this has to be done in a clever way. It would need to be done not only in qemuProcessLaunch() (or some subordinate functions), it would need to be done in qemuProcessReconnect() too (to cover case when user is upgrading
If you are upgrading the capabilities parsed from the status XML won't have the bit representing the presence of the query command. This means that new libvirt would still attempt the suspend ignoring whether it is supported or not. I don't think it makes sense to re-detect presence of stuff which was done prior to upgrading.
libvirt). For this, qemuMonitorConnect() looks like the best place - it's called from both locations and it also already contains some 'runtime capabilities' querying. Except not really - the migration capabilities
I was considering whether it's a good idea to keep runtime only capabilities in the qemuCaps array. I can find both arguments for it and against it. The convenient part is that it's automatically stored in the status XML.
queried there are stored in @priv->migrationCaps rather than priv->qemuCaps. Problem with storing runtime capabilities in priv->qemuCaps is that they would get formatted into status XML. And this opens whole different can of
It is strongly desired to store it in the status XML. That way you don't have to re-query.
worms. For instance, this feature relies on 'query-commands' to see if 'query-current-machine' command is available. Well, we can't re-run 'query-commands' in runtime because that might change the set of qemuCaps used when constructing cmd line.
Why bother? If qemu was started prior to the support being added I don't see a compelling reason to requery everything. In most cases it would be also wrong to do so.
Way around this is to have 'runtime only' capabilities, that would not be stored in status XML. At some point we will need those, but writing that infrastructure is not easy and a bit too much to ask for this small fix. Especially, when we have another option:
2) Issue 'query-current-machine' from qemuDomainPMSuspendForDuration(). In every run. This is a bit suboptimal, but how frequently do we expect this
I don't think it's suboptimal. Normally users don't use PM suspend for the VM so you can argue that this saves one qemu call in all the cases where PM suspend is never used.
API to be called? Caching qemu capabilities is there to speed up decission process which makes sense if we'd be making the decission 1000 times a second (e.g. building cmd line). With this approach the slowdown would be negligible. Of course, we would need to run the command in 'silent' mode, i.e. not report any error if qemu doesn't know it.
Or you can still use the capability bit whether the query command is available.
On 4/11/19 12:19 PM, Peter Krempa wrote:
On Thu, Apr 11, 2019 at 11:58:04 +0200, Michal Privoznik wrote:
On 4/11/19 10:39 AM, Peter Krempa wrote:
On Thu, Apr 11, 2019 at 10:25:11 +0200, Michal Privoznik wrote:
This capability tells whether qemu is capable of waking up the guest from PM suspend.
Based-on-work-of: Daniel Henrique Barboza <danielhb413@gmail.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_capabilities.c | 24 +++++++++++++++++++ src/qemu/qemu_capabilities.h | 3 +++ .../caps_4.0.0.riscv32.replies | 11 +++++++++ .../caps_4.0.0.riscv32.xml | 1 + .../caps_4.0.0.riscv64.replies | 11 +++++++++ .../caps_4.0.0.riscv64.xml | 1 + .../caps_4.0.0.x86_64.replies | 11 +++++++++ .../caps_4.0.0.x86_64.xml | 1 + 8 files changed, 63 insertions(+)
[...]
bool virQEMUCapsCPUFilterFeatures(const char *name, void *opaque) @@ -4373,6 +4395,8 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, return -1; if (virQEMUCapsProbeQMPSEVCapabilities(qemuCaps, mon) < 0) return -1; + if (virQEMUCapsProbeQMPCurrentMachine(qemuCaps, mon) < 0) + return -1;
This seems wrong by definition. The function is supposed to query current machine, but the capability lookup code uses 'none' machine type. IIUC the support for wakeup in some cases depends on the presence of ACPI in the guest and thus really can't be cached this way.
Yep, I was just about to write this but then got sidetracked.
So I see two options here: 1) Query if guest is capable of PM suspend at runtime. However, this has to be done in a clever way. It would need to be done not only in qemuProcessLaunch() (or some subordinate functions), it would need to be done in qemuProcessReconnect() too (to cover case when user is upgrading
If you are upgrading the capabilities parsed from the status XML won't have the bit representing the presence of the query command. This means that new libvirt would still attempt the suspend ignoring whether it is supported or not. I don't think it makes sense to re-detect presence of stuff which was done prior to upgrading.
Well, then mere upgrade of libvirt won't fix this bug for you. You'd have to restart the domain to make this bug go away.
libvirt). For this, qemuMonitorConnect() looks like the best place - it's called from both locations and it also already contains some 'runtime capabilities' querying. Except not really - the migration capabilities
I was considering whether it's a good idea to keep runtime only capabilities in the qemuCaps array. I can find both arguments for it and against it. The convenient part is that it's automatically stored in the status XML.
queried there are stored in @priv->migrationCaps rather than priv->qemuCaps. Problem with storing runtime capabilities in priv->qemuCaps is that they would get formatted into status XML. And this opens whole different can of
It is strongly desired to store it in the status XML. That way you don't have to re-query.
Well, what if you upgrade from ver1 to ver2 which introduces a new runtime capability? How could it be used if we would not requery caps on reconnect? Note, I don't mean full capabilities querying which is expensive. I'm talking about runtime capabilities. For instance this case. Suppose we have a capability (as proposed here) called PM_WAKEUP_SUPPORT. If we would requery caps at reconnect then we wouldn't fix this bug. On the other hand, for this specific bug - if you'd pmsuspend a domain that is incapable of wakeup, the only thing left for you to do is destroy + start combo which will fetch you the capability. But that's not the point.
worms. For instance, this feature relies on 'query-commands' to see if 'query-current-machine' command is available. Well, we can't re-run 'query-commands' in runtime because that might change the set of qemuCaps used when constructing cmd line.
Why bother? If qemu was started prior to the support being added I don't see a compelling reason to requery everything. In most cases it would be also wrong to do so.
Way around this is to have 'runtime only' capabilities, that would not be stored in status XML. At some point we will need those, but writing that infrastructure is not easy and a bit too much to ask for this small fix. Especially, when we have another option:
2) Issue 'query-current-machine' from qemuDomainPMSuspendForDuration(). In every run. This is a bit suboptimal, but how frequently do we expect this
I don't think it's suboptimal. Normally users don't use PM suspend for the VM so you can argue that this saves one qemu call in all the cases where PM suspend is never used.
API to be called? Caching qemu capabilities is there to speed up decission process which makes sense if we'd be making the decission 1000 times a second (e.g. building cmd line). With this approach the slowdown would be negligible. Of course, we would need to run the command in 'silent' mode, i.e. not report any error if qemu doesn't know it.
Or you can still use the capability bit whether the query command is available.
Nope, you can't. Let's just issue the command in qemuDomainPMSuspendForDuration(), unconditionally, ignore/silent any errors and leave runtime caps for another day. Michal
On Thu, Apr 11, 2019 at 13:49:03 +0200, Michal Privoznik wrote:
On 4/11/19 12:19 PM, Peter Krempa wrote:
On Thu, Apr 11, 2019 at 11:58:04 +0200, Michal Privoznik wrote:
On 4/11/19 10:39 AM, Peter Krempa wrote:
On Thu, Apr 11, 2019 at 10:25:11 +0200, Michal Privoznik wrote:
[...]
If you are upgrading the capabilities parsed from the status XML won't have the bit representing the presence of the query command. This means that new libvirt would still attempt the suspend ignoring whether it is supported or not. I don't think it makes sense to re-detect presence of stuff which was done prior to upgrading.
Well, then mere upgrade of libvirt won't fix this bug for you. You'd have to restart the domain to make this bug go away.
That is true. On the other hand a "mere" upgrade of libvirt will help in this case only if you start a VM with qemu-4.0.0 or newer (unreleased). With libvirt 5.2 or older and then upgrade to a new libvirtd. Older qemus can't be fixed as they don't support the command and starting with newer libvirt would not have the problem even if we cache the presence of query-current machine. Said that I find it perfectly acceptable to call query-current-machine unconditionally in qemuDomainPMSuspendForDuration, I just don't see a problem in caching presence of query-current-machine.
From: Daniel Henrique Barboza <danielhb413@gmail.com> If the current QEMU guest can't wake up from suspend properly, avoid suspending the guest at all. This is done by checking the QEMU_CAPS_PM_WAKEUP_SUPPORT cap. The absence of the cap indicates that we're dealing with a QEMU version older than 4.0 (which implements the required QMP API). In this case, proceed as usual with the suspend logic since we can't assume whether the guest has support or not. Fixes: https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1759509 Reported-by: Balamuruhan S <bala24@linux.vnet.ibm.com> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index fe2c586274..c4bd53677d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19151,6 +19151,7 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom, unsigned int flags) { virQEMUDriverPtr driver = dom->conn->privateData; + qemuDomainObjPrivatePtr priv; virDomainObjPtr vm; qemuAgentPtr agent; int ret = -1; @@ -19178,6 +19179,26 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom, if (virDomainPMSuspendForDurationEnsureACL(dom->conn, vm->def) < 0) goto cleanup; + priv = vm->privateData; + + /* + * We can't check just for QEMU_CAPS_WAKEUP_SUSPEND_SUPPORT because, + * in case this cap is disabled, it is not possible to tell if the guest + * does not have wake-up from suspend support or if the current QEMU + * instance does not have the API. + * + * The case we want to handle here is when QEMU has the API and + * QEMU_CAPS_WAKEUP_SUSPEND_SUPPORT cap is disabled. Otherwise, do + * not interfere with the suspend process. + */ + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_CURRENT_MACHINE) && + !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_PM_WAKEUP_SUPPORT)) { + + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("Domain does not have suspend support")); + goto cleanup; + } + if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_MODIFY) < 0) goto cleanup; -- 2.21.0
participants (3)
-
Daniel Henrique Barboza -
Michal Privoznik -
Peter Krempa