On Mon, Apr 01, 2019 at 18:18:24 -0300, 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_WAKEUP_SUSPEND_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 ++++ 2 files changed, 9 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 56228e7a36..318198cbdd 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", );

@@ -978,6 +982,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 06c7606e2f..633f2690a3 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_WAKEUP_SUSPEND_SUPPORT, /* query-current-machine API flag */

This should include 'PM' in the name. Libvirt's suspend is a different operation which does not need acpi. Also the comment string is bad. You should mention that it's kind of dynamic. Additionally you didn't run tests there are at least 3 sets of test files which show query-current-machine but this patch does not modify the expected output files, thus it breaks the test suite.

QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; -- 2.20.1

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, Apr 01, 2019 at 18:18:25 -0300, Daniel Henrique Barboza wrote:

QEMU_CAPS_WAKEUP_SUSPEND_SUPPORT is a bool that represents the value of the wakeup-suspend-support flag of the new query-current-machine QMP API, available in QEMU 4.0.

The flag is set in qemuProcessLaunch, using a new function called qemuProcessSetupWakeupSuspendSupport. This function checks first if the QEMU process has support for the new API, which is reflected by the QEMU_CAPS_QUERY_CURRENT_MACHINE cap. If positive, query it and set QEMU_CAPS_WAKEUP_SUSPEND_SUPPORT with the current value of wakeup-suspend-support. --- src/qemu/qemu_monitor.c | 8 ++++++++ src/qemu/qemu_monitor.h | 3 +++ src/qemu/qemu_monitor_json.c | 35 +++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 4 ++++ src/qemu/qemu_process.c | 38 ++++++++++++++++++++++++++++++++++++ 5 files changed, 88 insertions(+)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index babcbde878..fc6dc2197a 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4472,3 +4472,11 @@ qemuMonitorGetPRManagerInfo(qemuMonitorPtr mon, virHashFree(info); return ret; } + +int +qemuMonitorGetWakeupSuspendSupport(qemuMonitorPtr mon, bool *enabled) +{ + QEMU_CHECK_MONITOR(mon); + + return qemuMonitorJSONGetWakeupSuspendSupport(mon, enabled); +} diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 086195ff98..62074c1acb 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1220,4 +1220,7 @@ struct _qemuMonitorPRManagerInfo { int qemuMonitorGetPRManagerInfo(qemuMonitorPtr mon, virHashTablePtr *retinfo);

+int qemuMonitorGetWakeupSuspendSupport(qemuMonitorPtr mon, + bool *enabled); + #endif /* LIBVIRT_QEMU_MONITOR_H */ diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 743a88b914..16d34fb217 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -8452,3 +8452,38 @@ qemuMonitorJSONGetPRManagerInfo(qemuMonitorPtr mon, return ret;

} + +int +qemuMonitorJSONGetWakeupSuspendSupport(qemuMonitorPtr mon, bool *enabled)

This should be a generic function which returns all responses from query-current-machine. Having a single-use one will make us refactor this later. The code in qemu_caps.c should then decide which ones to consider to use as caps. You can inspire yourself in the way we detect e.g. command.s

+{ + int ret = -1; + virJSONValuePtr cmd, data; + 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; + + data = virJSONValueObjectGetObject(reply, "return"); + + if (virJSONValueObjectGetBoolean(data, "wakeup-suspend-support", + enabled)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing wakeup-suspend-support in " + "query-current-machine response")); + goto cleanup; + } + + ret = 0; + + 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..ede828d622 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -576,4 +576,8 @@ int qemuMonitorJSONGetPRManagerInfo(qemuMonitorPtr mon, virHashTablePtr info) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

+int qemuMonitorJSONGetWakeupSuspendSupport(qemuMonitorPtr mon, + bool *enabled) + 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 dc7317b723..71984714de 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6449,6 +6449,41 @@ qemuProcessSetupDiskThrottlingBlockdev(virQEMUDriverPtr driver, return ret; }

+/** + * qemuProcessSetupWakeupSuspendSupport: + * + * Sets up QEMU_CAPS_WAKEUP_SUSPEND_SUPPORT in case QEMU has support + * for the query-current-machine QMP API. This can be verified by + * checking for the QEMU_CAPS_QUERY_CURRENT_MACHINE cap. + */ +static int +qemuProcessSetupWakeupSuspendSupport(virQEMUDriverPtr driver, + virDomainObjPtr vm, + qemuDomainAsyncJob asyncJob)

Again please make this more generic so that the next person does not have to refactor it.

+{ + qemuDomainObjPrivatePtr priv = vm->privateData; + bool enabled; + int ret = -1; + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_CURRENT_MACHINE)) + return 0; + + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) + return -1; + + if (qemuMonitorGetWakeupSuspendSupport(qemuDomainGetMonitor(vm), &enabled) < 0) + goto cleanup; + + if (enabled) + virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_WAKEUP_SUSPEND_SUPPORT);

You don't have the lock on @vm until you exit the monitor so @priv should not be modified.

+ + ret = 0; + + cleanup: + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1; + return ret; +}

/** * qemuProcessLaunch: @@ -6777,6 +6812,9 @@ qemuProcessLaunch(virConnectPtr conn, qemuProcessAutoDestroyAdd(driver, vm, conn) < 0) goto cleanup;

+ if (qemuProcessSetupWakeupSuspendSupport(driver, vm, asyncJob) < 0) + goto cleanup; + ret = 0;

cleanup: -- 2.20.1

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, Apr 01, 2019 at 18:18:26 -0300, 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_WAKEUP_SUSPEND_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> ---

I am not sure if Libvirt uses Launchpad for bug tracking like QEMU does. If it's not the case, I believe the 'Fixes:' line up above can be ignored/deleted.

src/qemu/qemu_driver.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 62d8d977c5..9f1f170dfc 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19209,6 +19209,8 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom, virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; qemuAgentPtr agent; + qemuDomainObjPrivatePtr priv; + bool hasQueryMachineAPI = false; int ret = -1;

virCheckFlags(0, -1); @@ -19231,6 +19233,28 @@ 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. + */ + hasQueryMachineAPI = virQEMUCapsGet(priv->qemuCaps, + QEMU_CAPS_QUERY_CURRENT_MACHINE);

I don't think you need the extra variable, just put it in the single condition.

+ if (hasQueryMachineAPI && + !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_WAKEUP_SUSPEND_SUPPORT)) { + + virReportError(VIR_ERR_NO_SUPPORT, "%s",

You want to use VIR_ERR_OPERATION_UNSUPPORTED. VIR_ERR_NO_SUPPORT is reserved for hypervisor drivers notifying that the API call is not supported

+ _("Domain does not have suspend support")); + goto cleanup; + } + if (virDomainPMSuspendForDurationEnsureACL(dom->conn, vm->def) < 0) goto cleanup;

-- 2.20.1

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list