4:16 p.m.
changes from v3:
- patch 3 removed. We no longer consider PM_WAKEUP_SUPPORT a
cap. It is confusing to have a 'runtime cap' that can't be resolved
inside qemu_capabilities.c. The suspend operation isn't time
critical per se, and a call to QMP each time 'virsh dompmsuspend'
is executed is not dreadful to the user experience. More information
and discussion can be found at [1].
- due to the change above, patch 4 now executes the
query-current-machine QMP call (if the running QEMU instance supports
it) during qemuDomainPMSuspendForDuration to check if the guest
has wakeup support.
- previous patch link:
[1] https://www.redhat.com/archives/libvir-list/2019-April/msg00767.html
Daniel Henrique Barboza (2):
qemu_capabilities: Add QEMU_CAPS_QUERY_CURRENT_MACHINE
qemuDomainPMSuspendForDuration: check for wake-up support
Michal Prívozník (1):
qemu_monitor: Introduce handler for 'query-current-machine' command
src/qemu/qemu_capabilities.c | 2 +
src/qemu/qemu_capabilities.h | 1 +
src/qemu/qemu_driver.c | 54 +++++++++++++++++++
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.xml | 1 +
.../caps_4.0.0.riscv64.xml | 1 +
.../caps_4.0.0.x86_64.xml | 1 +
10 files changed, 134 insertions(+)
--
2.20.1
4:16 p.m.
New subject: [libvirt] [PATCH v4 1/3] qemu_capabilities: Add QEMU_CAPS_QUERY_CURRENT_MACHINE
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(a)gmail.com>
Signed-off-by: Michal Privoznik <mprivozn(a)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 f8ea66b577..a0b2ca73fb 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -524,6 +524,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
"scsi-disk.device_id",
"virtio-pci-non-transitional",
"overcommit",
+ "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 23ecef8c63..67c8e80462 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -506,6 +506,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check
*/
QEMU_CAPS_SCSI_DISK_DEVICE_ID, /* 'device_id' property of scsi disk */
QEMU_CAPS_VIRTIO_PCI_TRANSITIONAL, /* virtio *-pci-{non-}transitional devices */
QEMU_CAPS_OVERCOMMIT, /* -overcommit */
+ 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 d6fc996528..2b6aa7b371 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='scsi-disk.device_id'/>
<flag name='virtio-pci-non-transitional'/>
<flag name='overcommit'/>
+ <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 fcc34b0ae6..8c0965a634 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='scsi-disk.device_id'/>
<flag name='virtio-pci-non-transitional'/>
<flag name='overcommit'/>
+ <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 bc8e35e226..f4be7017fe 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='scsi-disk.device_id'/>
<flag name='virtio-pci-non-transitional'/>
<flag name='overcommit'/>
+ <flag name='query-current-machine'/>
<version>3001050</version>
<kvmVersion>0</kvmVersion>
<microcodeVersion>43100758</microcodeVersion>
--
2.20.1
4:16 p.m.
New subject: [libvirt] [PATCH v4 2/3] qemu_monitor: Introduce handler for 'query-current-machine' command
From: Michal Privoznik <mprivozn(a)redhat.com>
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(a)gmail.com>
Signed-off-by: Michal Privoznik <mprivozn(a)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 e7d063a5a8..908967f46c 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -8459,3 +8459,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.20.1
4:16 p.m.
New subject: [libvirt] [PATCH v4 3/3] qemuDomainPMSuspendForDuration: check for wake-up support
If the current QEMU guest can't wake up from suspend properly,
and we are able to determine that, avoid suspending the guest
at all. To be able to determine this support, QEMU needs to
implement the 'query-current-machine' QMP call. This is reflected
by the QEMU_CAPS_QUERY_CURRENT_MACHINE cap.
If the cap is enabled, a new function qemuDomainProbeQMPCurrentMachine
is called. This is wrapper for qemuMonitorGetCurrentMachineInfo,
where the 'wakeup-suspend-support' flag is retrieved from
'query-current-machine'. If wakeupSuspendSupport is true,
proceed with the regular flow of qemuDomainPMSuspendForDuration.
The absence of QEMU_CAPS_QUERY_CURRENT_MACHINE 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 of qemuDomainPMSuspendForDuration, 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(a)linux.vnet.ibm.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/qemu/qemu_driver.c | 54 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 54 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 31d8647eee..d584f6ae66 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -19145,6 +19145,35 @@ qemuDomainGetCPUStats(virDomainPtr domain,
return ret;
}
+static int
+qemuDomainProbeQMPCurrentMachine(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ bool *enabled)
+{
+ qemuMonitorCurrentMachineInfo info = { 0 };
+ qemuDomainObjPrivatePtr priv;
+ int ret = -1;
+
+ *enabled = false;
+ priv = vm->privateData;
+
+ qemuDomainObjEnterMonitor(driver, vm);
+
+ if (qemuMonitorGetCurrentMachineInfo(priv->mon, &info) < 0)
+ goto cleanup;
+
+ ret = 0;
+
+ if (info.wakeupSuspendSupport)
+ *enabled = true;
+
+ cleanup:
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
+
+ return ret;
+}
+
static int
qemuDomainPMSuspendForDuration(virDomainPtr dom,
unsigned int target,
@@ -19152,6 +19181,7 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom,
unsigned int flags)
{
virQEMUDriverPtr driver = dom->conn->privateData;
+ qemuDomainObjPrivatePtr priv;
virDomainObjPtr vm;
qemuAgentPtr agent;
int ret = -1;
@@ -19185,6 +19215,30 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom,
if (virDomainObjCheckActive(vm) < 0)
goto endjob;
+ /*
+ * The case we want to handle here is when QEMU has the API (i.e.
+ * QEMU_CAPS_QUERY_CURRENT_MACHINE is set). Otherwise, do not interfere
+ * with the suspend process. This means that existing running domains,
+ * that don't know about this cap, will keep their old behavior of
+ * suspending 'in the dark'.
+ */
+ priv = vm->privateData;
+ if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_CURRENT_MACHINE)) {
+ bool enabled;
+
+ if (qemuDomainProbeQMPCurrentMachine(driver, vm, &enabled) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Error executing query-current-machine call"));
+ goto endjob;
+ }
+
+ if (!enabled) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("Domain does not have suspend support"));
+ goto endjob;
+ }
+ }
+
if (vm->def->pm.s3 || vm->def->pm.s4) {
if (vm->def->pm.s3 == VIR_TRISTATE_BOOL_NO &&
(target == VIR_NODE_SUSPEND_TARGET_MEM ||
--
2.20.1
4:46 a.m.
New subject: [libvirt] [PATCH v4 3/3] qemuDomainPMSuspendForDuration: check for wake-up support
On 4/24/19 11:16 PM, Daniel Henrique Barboza wrote:
If the current QEMU guest can't wake up from suspend properly,
and we are able to determine that, avoid suspending the guest
at all. To be able to determine this support, QEMU needs to
implement the 'query-current-machine' QMP call. This is reflected
by the QEMU_CAPS_QUERY_CURRENT_MACHINE cap.
If the cap is enabled, a new function qemuDomainProbeQMPCurrentMachine
is called. This is wrapper for qemuMonitorGetCurrentMachineInfo,
where the 'wakeup-suspend-support' flag is retrieved from
'query-current-machine'. If wakeupSuspendSupport is true,
proceed with the regular flow of qemuDomainPMSuspendForDuration.
The absence of QEMU_CAPS_QUERY_CURRENT_MACHINE 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 of qemuDomainPMSuspendForDuration, 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(a)linux.vnet.ibm.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/qemu/qemu_driver.c | 54 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 54 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 31d8647eee..d584f6ae66 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -19145,6 +19145,35 @@ qemuDomainGetCPUStats(virDomainPtr domain,
return ret;
}
+static int
+qemuDomainProbeQMPCurrentMachine(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ bool *enabled)
+{
+ qemuMonitorCurrentMachineInfo info = { 0 };
+ qemuDomainObjPrivatePtr priv;
+ int ret = -1;
+
+ *enabled = false;
+ priv = vm->privateData;
+
+ qemuDomainObjEnterMonitor(driver, vm);
+
+ if (qemuMonitorGetCurrentMachineInfo(priv->mon, &info) < 0)
+ goto cleanup;
+
+ ret = 0;
+
+ if (info.wakeupSuspendSupport)
+ *enabled = true;
+
+ cleanup:
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
+
+ return ret;
This can be simplified.
+}
+
static int
qemuDomainPMSuspendForDuration(virDomainPtr dom,
unsigned int target,
@@ -19152,6 +19181,7 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom,
unsigned int flags)
{
virQEMUDriverPtr driver = dom->conn->privateData;
+ qemuDomainObjPrivatePtr priv;
virDomainObjPtr vm;
qemuAgentPtr agent;
int ret = -1;
@@ -19185,6 +19215,30 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom,
if (virDomainObjCheckActive(vm) < 0)
goto endjob;
+ /*
+ * The case we want to handle here is when QEMU has the API (i.e.
+ * QEMU_CAPS_QUERY_CURRENT_MACHINE is set). Otherwise, do not interfere
+ * with the suspend process. This means that existing running domains,
+ * that don't know about this cap, will keep their old behavior of
+ * suspending 'in the dark'.
+ */
+ priv = vm->privateData;
+ if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_CURRENT_MACHINE)) {
+ bool enabled;
s/enabled/wakeupSupported/ to make it a bit more obvious what it is
we're querying for here.
+
+ if (qemuDomainProbeQMPCurrentMachine(driver, vm, &enabled) < 0) {
This is not sufficient. A few lines above (not visible in the patch
context) we grab agent job but this enters the monitor then. This is
dangerous because it means we're talking on monitor without locking it
and thus might interfere with some other thread. Either we grab the
correct job depending whether we are going to talk on monitor or not, or
we can just grab both jobs even if qemu doesn't have the capability.
The cleaner solution would be the former.
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Error executing query-current-machine call"));
No need to overwrite an error here. I mean,
qemuDomainProbeQMPCurrentMachine() will report error on failure (not
explicitly, but the functions it's calling do report error.
+ goto endjob;
+ }
+
+ if (!enabled) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("Domain does not have suspend support"));
+ goto endjob;
+ }
+ }
+
if (vm->def->pm.s3 || vm->def->pm.s4) {
if (vm->def->pm.s3 == VIR_TRISTATE_BOOL_NO &&
(target == VIR_NODE_SUSPEND_TARGET_MEM ||
I'll fix the issues before pushing.
Michal
6:31 a.m.
New subject: [libvirt] [PATCH v4 3/3] qemuDomainPMSuspendForDuration: check for wake-up support
On 4/25/19 6:46 AM, Michal Privoznik wrote:
On 4/24/19 11:16 PM, Daniel Henrique Barboza wrote:
> If the current QEMU guest can't wake up from suspend properly,
> and we are able to determine that, avoid suspending the guest
> at all. To be able to determine this support, QEMU needs to
> implement the 'query-current-machine' QMP call. This is reflected
> by the QEMU_CAPS_QUERY_CURRENT_MACHINE cap.
>
> If the cap is enabled, a new function qemuDomainProbeQMPCurrentMachine
> is called. This is wrapper for qemuMonitorGetCurrentMachineInfo,
> where the 'wakeup-suspend-support' flag is retrieved from
> 'query-current-machine'. If wakeupSuspendSupport is true,
> proceed with the regular flow of qemuDomainPMSuspendForDuration.
>
> The absence of QEMU_CAPS_QUERY_CURRENT_MACHINE 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 of qemuDomainPMSuspendForDuration, 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(a)linux.vnet.ibm.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
> ---
> src/qemu/qemu_driver.c | 54 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 54 insertions(+)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 31d8647eee..d584f6ae66 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -19145,6 +19145,35 @@ qemuDomainGetCPUStats(virDomainPtr domain,
> return ret;
> }
> +static int
> +qemuDomainProbeQMPCurrentMachine(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + bool *enabled)
> +{
> + qemuMonitorCurrentMachineInfo info = { 0 };
> + qemuDomainObjPrivatePtr priv;
> + int ret = -1;
> +
> + *enabled = false;
> + priv = vm->privateData;
> +
> + qemuDomainObjEnterMonitor(driver, vm);
> +
> + if (qemuMonitorGetCurrentMachineInfo(priv->mon, &info) < 0)
> + goto cleanup;
> +
> + ret = 0;
> +
> + if (info.wakeupSuspendSupport)
> + *enabled = true;
> +
> + cleanup:
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + ret = -1;
> +
> + return ret;
This can be simplified.
> +}
> +
> static int
> qemuDomainPMSuspendForDuration(virDomainPtr dom,
> unsigned int target,
> @@ -19152,6 +19181,7 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom,
> unsigned int flags)
> {
> virQEMUDriverPtr driver = dom->conn->privateData;
> + qemuDomainObjPrivatePtr priv;
> virDomainObjPtr vm;
> qemuAgentPtr agent;
> int ret = -1;
> @@ -19185,6 +19215,30 @@ qemuDomainPMSuspendForDuration(virDomainPtr
> dom,
> if (virDomainObjCheckActive(vm) < 0)
> goto endjob;
> + /*
> + * The case we want to handle here is when QEMU has the API (i.e.
> + * QEMU_CAPS_QUERY_CURRENT_MACHINE is set). Otherwise, do not
> interfere
> + * with the suspend process. This means that existing running
> domains,
> + * that don't know about this cap, will keep their old behavior of
> + * suspending 'in the dark'.
> + */
> + priv = vm->privateData;
> + if (virQEMUCapsGet(priv->qemuCaps,
> QEMU_CAPS_QUERY_CURRENT_MACHINE)) {
> + bool enabled;
s/enabled/wakeupSupported/ to make it a bit more obvious what it is
we're querying for here.
> +
> + if (qemuDomainProbeQMPCurrentMachine(driver, vm, &enabled) <
> 0) {
This is not sufficient. A few lines above (not visible in the patch
context) we grab agent job but this enters the monitor then. This is
dangerous because it means we're talking on monitor without locking it
and thus might interfere with some other thread. Either we grab the
correct job depending whether we are going to talk on monitor or not,
or we can just grab both jobs even if qemu doesn't have the capability.
Thanks for the explanation and fixing before pushing. I was under the
impression
that the now extinct line:
"if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_MODIFY) < 0)"
Was enough to hold the lock context for all the VM resources, not only
the QEMU
agent. Mostly because I saw the usage of:
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
Right below, in qemuDomainPMWakeup(), that access the monitor in the same
function.
But now that I'm putting it all together I noticed that the functions
are all different .... so qemuDomainObjBeginJob() locks the monitor,
qemuDomainObjBeginAgentJob() locks the agent, and
qemuDomainObjBeginJobWithAgent() can lock both, depending if
job isn't QEMU_JOB_NONE. Is this somewhat close to the actual usage?
Thanks,
DHB
The cleaner solution would be the former.
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Error executing query-current-machine
> call"));
No need to overwrite an error here. I mean,
qemuDomainProbeQMPCurrentMachine() will report error on failure (not
explicitly, but the functions it's calling do report error.
> + goto endjob;
> + }
> +
> + if (!enabled) {
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> + _("Domain does not have suspend support"));
> + goto endjob;
> + }
> + }
> +
> if (vm->def->pm.s3 || vm->def->pm.s4) {
> if (vm->def->pm.s3 == VIR_TRISTATE_BOOL_NO &&
> (target == VIR_NODE_SUSPEND_TARGET_MEM ||
>
I'll fix the issues before pushing.
Michal
8:26 a.m.
New subject: [libvirt] [PATCH v4 3/3] qemuDomainPMSuspendForDuration: check for wake-up support
On 4/25/19 1:31 PM, Daniel Henrique Barboza wrote:
Thanks for the explanation and fixing before pushing. I was under the
impression
that the now extinct line:
"if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_MODIFY) < 0)"
Was enough to hold the lock context for all the VM resources, not only
the QEMU
agent. Mostly because I saw the usage of:
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
Right below, in qemuDomainPMWakeup(), that access the monitor in the same
function.
But now that I'm putting it all together I noticed that the functions
are all different .... so qemuDomainObjBeginJob() locks the monitor,
qemuDomainObjBeginAgentJob() locks the agent, and
qemuDomainObjBeginJobWithAgent() can lock both, depending if
job isn't QEMU_JOB_NONE. Is this somewhat close to the actual usage?
Exactly. Previously we had only one job condition where all threads had
to serialize. I mean one condition per VM. So even if you had two APIs:
one wanted to talk on monitor and the other wanted to talk to the guest
agent they would serialize. This is obviously suboptimal. So I've split
the condition into two. The downside is that we have to be more careful
which job we must take (and there's no job promotion as in 'I already
have say monitor job and now I realized I need an agent job too').
Michal
4:46 a.m.
On 4/24/19 11:16 PM, Daniel Henrique Barboza wrote:
changes from v3:
- patch 3 removed. We no longer consider PM_WAKEUP_SUPPORT a
cap. It is confusing to have a 'runtime cap' that can't be resolved
inside qemu_capabilities.c. The suspend operation isn't time
critical per se, and a call to QMP each time 'virsh dompmsuspend'
is executed is not dreadful to the user experience. More information
and discussion can be found at [1].
- due to the change above, patch 4 now executes the
query-current-machine QMP call (if the running QEMU instance supports
it) during qemuDomainPMSuspendForDuration to check if the guest
has wakeup support.
- previous patch link:
[1] https://www.redhat.com/archives/libvir-list/2019-April/msg00767.html
Daniel Henrique Barboza (2):
qemu_capabilities: Add QEMU_CAPS_QUERY_CURRENT_MACHINE
qemuDomainPMSuspendForDuration: check for wake-up support
Michal Prívozník (1):
qemu_monitor: Introduce handler for 'query-current-machine' command
src/qemu/qemu_capabilities.c | 2 +
src/qemu/qemu_capabilities.h | 1 +
src/qemu/qemu_driver.c | 54 +++++++++++++++++++
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.xml | 1 +
.../caps_4.0.0.riscv64.xml | 1 +
.../caps_4.0.0.x86_64.xml | 1 +
10 files changed, 134 insertions(+)
Fixed, ACKed and pushed.
Michal
2041
days inactive
2042
days old
7 comments
2 participants
participants (2)
-
Daniel Henrique Barboza -
Michal Privoznik