[libvirt] [PATCH v2 0/3] introducing QEMU_CAPS_QUERY_CURRENT_MACHINE and QEMU_CAPS_PM_WAKEUP_SUPPORT

v2: Fixed review problems found by Peter Krempa, most notably: - changed the cap name - made the code generic to be easier to add other caps from the query-current-machine API - changed unit tests accordingly Previous version can be found at: https://www.redhat.com/archives/libvir-list/2019-April/msg00104.html Daniel Henrique Barboza (3): adding QEMU_CAPS_QUERY_CURRENT_MACHINE and QEMU_CAPS_PM_WAKEUP_SUPPORT qemu_process: setting QEMU_CAPS_QUERY_CURRENT_MACHINE caps qemuDomainPMSuspendForDuration: check for QEMU_CAPS_PM_WAKEUP_SUPPORT src/qemu/qemu_capabilities.c | 5 ++ src/qemu/qemu_capabilities.h | 4 ++ src/qemu/qemu_driver.c | 21 ++++++++ src/qemu/qemu_monitor.c | 17 ++++++ src/qemu/qemu_monitor.h | 11 ++++ src/qemu/qemu_monitor_json.c | 53 +++++++++++++++++++ src/qemu/qemu_monitor_json.h | 5 ++ src/qemu/qemu_process.c | 36 +++++++++++++ .../caps_4.0.0.riscv32.xml | 1 + .../caps_4.0.0.riscv64.xml | 1 + .../caps_4.0.0.x86_64.xml | 1 + 11 files changed, 155 insertions(+) -- 2.20.1

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. The original intent of this new API is to avoid situations such as [1], where an user can execute "virsh dompmsuspend" in a guest that can't wake up due to lack of support, making the guest unusable. This is currently the case for any non-x86 arch guests and for some x86 guests that starts with --no-acpi (q35 machines implements suspend support even with the --no-acpi flag). This is the Libvirt side of this API that will be available in QEMU 4.0. QEMU_CAPS_QUERY_CURRENT_MACHINE is a new virQEMUCapsCommands that will indicate if the QEMU binary supports the 'query-current-machine' API. QEMU_CAPS_PM_WAKEUP_SUPPORT is a flag that reflects the 'wakeup-suspend-support' value for the current QEMU instance. In the next patches these two caps will be populated and used in qemu_driver.c, 'qemuDomainPMSuspendForDuration', to complete the fix for [1]. [1] https://github.com/open-power-host-os/qemu/issues/31 Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_capabilities.c | 5 +++++ src/qemu/qemu_capabilities.h | 4 ++++ 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, 12 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 71d4c01296..5c1b41aa3e 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -523,6 +523,10 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "nvdimm.unarmed", "scsi-disk.device_id", "virtio-pci-non-transitional", + "query-current-machine", + + /* 330 */ + "wakeup-suspend-support", ); @@ -968,6 +972,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 c6f6980684..85d0c653fa 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -506,6 +506,10 @@ 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 */ + + /* 330 */ + QEMU_CAPS_PM_WAKEUP_SUPPORT, /* dynamic capability that shows if the running 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.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.20.1

On 4/9/19 6:18 PM, Daniel Henrique Barboza wrote:
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.
The original intent of this new API is to avoid situations such as [1], where an user can execute "virsh dompmsuspend" in a guest that can't wake up due to lack of support, making the guest unusable. This is currently the case for any non-x86 arch guests and for some x86 guests that starts with --no-acpi (q35 machines implements suspend support even with the --no-acpi flag).
This is the Libvirt side of this API that will be available in QEMU 4.0. QEMU_CAPS_QUERY_CURRENT_MACHINE is a new virQEMUCapsCommands that will indicate if the QEMU binary supports the 'query-current-machine' API. QEMU_CAPS_PM_WAKEUP_SUPPORT is a flag that reflects the 'wakeup-suspend-support' value for the current QEMU instance. In the next patches these two caps will be populated and used in qemu_driver.c, 'qemuDomainPMSuspendForDuration', to complete the fix for [1].
[1] https://github.com/open-power-host-os/qemu/issues/31
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_capabilities.c | 5 +++++ src/qemu/qemu_capabilities.h | 4 ++++ 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, 12 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 71d4c01296..5c1b41aa3e 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -523,6 +523,10 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "nvdimm.unarmed", "scsi-disk.device_id", "virtio-pci-non-transitional", + "query-current-machine", + + /* 330 */ + "wakeup-suspend-support",
This is unused capability yet. I rather see it introduced with its usage. Michal

The new QMP API query-current-machine reports VM capabilities that can vary depending on the VM configuration, in contrast to most QEMU caps that can be set once during libvirtd init as exposed in qemu_capabilities.c. As such, to properly set the runtime capabilities exposed by this API, we must query them at VM launch time. To do that, a new function called qemuProcessSetCurrentMachineCaps is called in qemuProcessLaunch. A new struct called qemuMonitorCurrentMachineInfo was created to host the output of the API (which contains only the wakeup-suspend-support flag for now, but can be expanded in the future). This struct is populated by qemuMonitorGetCurrentMachineInfo, which does the heavy work - executes query-current-machine, parses the result and populates the qemuMonitorCurrentMachineInfo struct. qemuProcessSetCurrentMachineCaps can then read the structure and set the capabilities accordingly. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_monitor.c | 17 ++++++++++++ src/qemu/qemu_monitor.h | 11 ++++++++ src/qemu/qemu_monitor_json.c | 53 ++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 5 ++++ src/qemu/qemu_process.c | 36 ++++++++++++++++++++++++ 5 files changed, 122 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index babcbde878..d4222f0bcc 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4472,3 +4472,20 @@ qemuMonitorGetPRManagerInfo(qemuMonitorPtr mon, virHashFree(info); return ret; } + +int +qemuMonitorGetCurrentMachineInfo(qemuMonitorPtr mon, + qemuMonitorCurrentMachineInfoPtr info) +{ + int ret = -1; + + QEMU_CHECK_MONITOR(mon); + + if (qemuMonitorJSONGetCurrentMachineInfo(mon, info) < 0) + goto cleanup; + + ret = 0; + + cleanup: + return ret; +} diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 086195ff98..fcd5a022b5 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1220,4 +1220,15 @@ struct _qemuMonitorPRManagerInfo { int qemuMonitorGetPRManagerInfo(qemuMonitorPtr mon, virHashTablePtr *retinfo); +struct _qemuMonitorCurrentMachineInfo { + bool wakeupSuspendSupport; +}; + +typedef struct _qemuMonitorCurrentMachineInfo qemuMonitorCurrentMachineInfo; +typedef qemuMonitorCurrentMachineInfo *qemuMonitorCurrentMachineInfoPtr; + +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 8e6c3ccd63..c6a2a59e6b 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -8449,3 +8449,56 @@ qemuMonitorJSONGetPRManagerInfo(qemuMonitorPtr mon, return ret; } + +static int +qemuMonitorJSONExtractCurrentMachineInfo(virJSONValuePtr reply, + qemuMonitorCurrentMachineInfoPtr info) +{ + virJSONValuePtr data; + int ret = -1; + + data = virJSONValueObjectGetObject(reply, "return"); + if (!data) + goto malformed; + + if (virJSONValueObjectGetBoolean(data, "wakeup-suspend-support", + &info->wakeupSuspendSupport) < 0) { + goto malformed; + } + + ret = 0; + + out: + return ret; + + malformed: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed qemu-current-machine reply")); + goto out; +} + +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 */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 47d8ca2ff1..b60c1ecfbe 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6461,6 +6461,39 @@ qemuProcessSetupDiskThrottlingBlockdev(virQEMUDriverPtr driver, return ret; } +static int +qemuProcessSetCurrentMachineCaps(virQEMUDriverPtr driver, + virDomainObjPtr vm, + qemuDomainAsyncJob asyncJob) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + qemuMonitorCurrentMachineInfoPtr info; + int ret = -1; + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_CURRENT_MACHINE)) + return 0; + + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) + return -1; + + if (VIR_ALLOC(info) < 0) + return -1; + + ret = qemuMonitorGetCurrentMachineInfo(priv->mon, info); + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1; + + if (ret < 0) + goto cleanup; + + if (info->wakeupSuspendSupport) + virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_PM_WAKEUP_SUPPORT); + + cleanup: + VIR_FREE(info); + return ret; +} /** * qemuProcessLaunch: @@ -6778,6 +6811,9 @@ qemuProcessLaunch(virConnectPtr conn, if (qemuProcessSetupDiskThrottlingBlockdev(driver, vm, asyncJob) < 0) goto cleanup; + if (qemuProcessSetCurrentMachineCaps(driver, vm, asyncJob) < 0) + goto cleanup; + /* Since CPUs were not started yet, the balloon could not return the memory * to the host and thus cur_balloon needs to be updated so that GetXMLdesc * and friends return the correct size in case they can't grab the job */ -- 2.20.1

On 4/9/19 6:18 PM, Daniel Henrique Barboza wrote:
The new QMP API query-current-machine reports VM capabilities that can vary depending on the VM configuration, in contrast to most QEMU caps that can be set once during libvirtd init as exposed in qemu_capabilities.c.
As such, to properly set the runtime capabilities exposed by this API, we must query them at VM launch time. To do that, a new function called qemuProcessSetCurrentMachineCaps is called in qemuProcessLaunch. A new struct called qemuMonitorCurrentMachineInfo was created to host the output of the API (which contains only the wakeup-suspend-support flag for now, but can be expanded in the future). This struct is populated by qemuMonitorGetCurrentMachineInfo, which does the heavy work - executes query-current-machine, parses the result and populates the qemuMonitorCurrentMachineInfo struct.
qemuProcessSetCurrentMachineCaps can then read the structure and set the capabilities accordingly.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_monitor.c | 17 ++++++++++++ src/qemu/qemu_monitor.h | 11 ++++++++ src/qemu/qemu_monitor_json.c | 53 ++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 5 ++++ src/qemu/qemu_process.c | 36 ++++++++++++++++++++++++ 5 files changed, 122 insertions(+)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index babcbde878..d4222f0bcc 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4472,3 +4472,20 @@ qemuMonitorGetPRManagerInfo(qemuMonitorPtr mon, virHashFree(info); return ret; } + +int +qemuMonitorGetCurrentMachineInfo(qemuMonitorPtr mon, + qemuMonitorCurrentMachineInfoPtr info) +{ + int ret = -1; + + QEMU_CHECK_MONITOR(mon); + + if (qemuMonitorJSONGetCurrentMachineInfo(mon, info) < 0) + goto cleanup; + + ret = 0; + + cleanup: + return ret; +} diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 086195ff98..fcd5a022b5 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1220,4 +1220,15 @@ struct _qemuMonitorPRManagerInfo { int qemuMonitorGetPRManagerInfo(qemuMonitorPtr mon, virHashTablePtr *retinfo);
+struct _qemuMonitorCurrentMachineInfo { + bool wakeupSuspendSupport; +}; + +typedef struct _qemuMonitorCurrentMachineInfo qemuMonitorCurrentMachineInfo; +typedef qemuMonitorCurrentMachineInfo *qemuMonitorCurrentMachineInfoPtr; + +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 8e6c3ccd63..c6a2a59e6b 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -8449,3 +8449,56 @@ qemuMonitorJSONGetPRManagerInfo(qemuMonitorPtr mon, return ret;
} + +static int +qemuMonitorJSONExtractCurrentMachineInfo(virJSONValuePtr reply, + qemuMonitorCurrentMachineInfoPtr info) +{ + virJSONValuePtr data; + int ret = -1; + + data = virJSONValueObjectGetObject(reply, "return"); + if (!data) + goto malformed; + + if (virJSONValueObjectGetBoolean(data, "wakeup-suspend-support", + &info->wakeupSuspendSupport) < 0) { + goto malformed; + } + + ret = 0; + + out: + return ret; + + malformed: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed qemu-current-machine reply")); + goto out; +} + +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 */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 47d8ca2ff1..b60c1ecfbe 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6461,6 +6461,39 @@ qemuProcessSetupDiskThrottlingBlockdev(virQEMUDriverPtr driver, return ret; }
+static int +qemuProcessSetCurrentMachineCaps(virQEMUDriverPtr driver, + virDomainObjPtr vm, + qemuDomainAsyncJob asyncJob) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + qemuMonitorCurrentMachineInfoPtr info; + int ret = -1; + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_CURRENT_MACHINE)) + return 0; + + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) + return -1; + + if (VIR_ALLOC(info) < 0) + return -1; + + ret = qemuMonitorGetCurrentMachineInfo(priv->mon, info); + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1; + + if (ret < 0) + goto cleanup; + + if (info->wakeupSuspendSupport) + virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_PM_WAKEUP_SUPPORT); + + cleanup: + VIR_FREE(info); + return ret; +}
/** * qemuProcessLaunch: @@ -6778,6 +6811,9 @@ qemuProcessLaunch(virConnectPtr conn, if (qemuProcessSetupDiskThrottlingBlockdev(driver, vm, asyncJob) < 0) goto cleanup;
+ if (qemuProcessSetCurrentMachineCaps(driver, vm, asyncJob) < 0) + goto cleanup;
Oh, we don't query for qemu capabilities in ProcessLaunch. Maybe it doesn't matter right now because there is no decission to be made when building the cmd line, but nevertheless this must be done in virQEMUCapsInitQMPMonitor() or in one of the function it's calling. Michal

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. This is the output of dompmsuspend in a guest that does not have wake-up support declared in the query-current-machine: $ sudo ./run tools/virsh dompmsuspend ub1810-noACPI3 mem error: Domain ub1810-noACPI3 could not be suspended error: this function is not supported by the connection driver: Domain does not have suspend support 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> --- 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 7e5bbc3cc9..6ee1247c7b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19152,6 +19152,7 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom, virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; qemuAgentPtr agent; + qemuDomainObjPrivatePtr priv; int ret = -1; virCheckFlags(0, -1); @@ -19174,6 +19175,26 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom, if (!(vm = qemuDomObjFromDomain(dom))) 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 (virDomainPMSuspendForDurationEnsureACL(dom->conn, vm->def) < 0) goto cleanup; -- 2.20.1

On 4/9/19 6:18 PM, Daniel Henrique Barboza wrote:
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.
This is the output of dompmsuspend in a guest that does not have wake-up support declared in the query-current-machine:
$ sudo ./run tools/virsh dompmsuspend ub1810-noACPI3 mem error: Domain ub1810-noACPI3 could not be suspended error: this function is not supported by the connection driver: Domain does not have suspend support
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> --- 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 7e5bbc3cc9..6ee1247c7b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19152,6 +19152,7 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom, virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; qemuAgentPtr agent; + qemuDomainObjPrivatePtr priv; int ret = -1;
virCheckFlags(0, -1); @@ -19174,6 +19175,26 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom, if (!(vm = qemuDomObjFromDomain(dom))) 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 (virDomainPMSuspendForDurationEnsureACL(dom->conn, vm->def) < 0) goto cleanup;
See this EnsureACL() call? It has to be done before this caps check you're introducing. The reason is that if there is an ACL rule that prohibits access to a domain, then this would leak info on it. For instance, instead of "no such domain" or "no perms for this domain" a malicious user would see "domain does not have suspend support" so he/she would know the domain is there and that it doesn't have suspend support. Long story short, this check of yours needs to be placed after the ACL check. Michal

On 4/9/19 6:18 PM, Daniel Henrique Barboza wrote:
v2: Fixed review problems found by Peter Krempa, most notably: - changed the cap name - made the code generic to be easier to add other caps from the query-current-machine API - changed unit tests accordingly
Previous version can be found at: https://www.redhat.com/archives/libvir-list/2019-April/msg00104.html
Daniel Henrique Barboza (3): adding QEMU_CAPS_QUERY_CURRENT_MACHINE and QEMU_CAPS_PM_WAKEUP_SUPPORT qemu_process: setting QEMU_CAPS_QUERY_CURRENT_MACHINE caps qemuDomainPMSuspendForDuration: check for QEMU_CAPS_PM_WAKEUP_SUPPORT
src/qemu/qemu_capabilities.c | 5 ++ src/qemu/qemu_capabilities.h | 4 ++ src/qemu/qemu_driver.c | 21 ++++++++ src/qemu/qemu_monitor.c | 17 ++++++ src/qemu/qemu_monitor.h | 11 ++++ src/qemu/qemu_monitor_json.c | 53 +++++++++++++++++++ src/qemu/qemu_monitor_json.h | 5 ++ src/qemu/qemu_process.c | 36 +++++++++++++ .../caps_4.0.0.riscv32.xml | 1 + .../caps_4.0.0.riscv64.xml | 1 + .../caps_4.0.0.x86_64.xml | 1 + 11 files changed, 155 insertions(+)
I've applied these and reworked them slightly. I'm going to repost them as v3 and keep your authorship (if that's okay with you). Michal
participants (2)
-
Daniel Henrique Barboza
-
Michal Privoznik