[libvirt] [PATCH v2 0/3] Per-guest S3/S4 configuration
There is a way to tell qemu how (not) to advertise S3/S4 ACPI sleep state capability to the guest OS. This series includes the capability to set this in the XML and also covers all the handling from qemu point of view including checking for the support, parameter parsing, command building, checking before sending the signals through guest agent and also tests. -- v2: - Modified the patch to reflect danpb's notes (according to qemu people the setting the disable_s[34] parameter to 0/1 ensures that the states will be enabled/disabled respectively) Martin Kletzander (3): Add per-guest S3/S4 state configuration qemu: Add support for S3/S4 state configuration tests: Add tests for qemu S3/S4 state configuration docs/schemas/domaincommon.rng | 33 ++++++ src/conf/domain_conf.c | 44 +++++++++ src/conf/domain_conf.h | 14 +++ src/libvirt_private.syms | 2 + src/qemu/qemu_capabilities.c | 7 ++ src/qemu/qemu_capabilities.h | 2 + src/qemu/qemu_command.c | 103 ++++++++++++++++++++ src/qemu/qemu_driver.c | 17 +++ tests/qemuargv2xmltest.c | 3 + .../qemuxml2argv-misc-disable-s3.args | 4 + .../qemuxml2argv-misc-disable-s3.xml | 27 +++++ .../qemuxml2argv-misc-disable-suspends.args | 4 + .../qemuxml2argv-misc-disable-suspends.xml | 27 +++++ .../qemuxml2argv-misc-enable-s4.args | 4 + .../qemuxml2argv-misc-enable-s4.xml | 27 +++++ tests/qemuxml2argvtest.c | 4 + tests/qemuxml2xmltest.c | 3 + 17 files changed, 325 insertions(+), 0 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-misc-disable-s3.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-misc-disable-s3.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-misc-disable-suspends.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-misc-disable-suspends.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-misc-enable-s4.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-misc-enable-s4.xml -- 1.7.8.6
There is a new <pm/> element implemented that can control what ACPI sleeping states will be advertised by BIOS and allowed to be switched to by libvirt. The default keeps defaults on hypervisor, otherwise forces chosen setting. --- docs/schemas/domaincommon.rng | 33 ++++++++++++++++++++++++++++++ src/conf/domain_conf.c | 44 +++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 14 +++++++++++++ src/libvirt_private.syms | 2 + 4 files changed, 93 insertions(+), 0 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index c85d763..657d723 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -53,6 +53,9 @@ <ref name="features"/> <ref name="termination"/> <optional> + <ref name="pm"/> + </optional> + <optional> <ref name="devices"/> </optional> <optional> @@ -2192,6 +2195,36 @@ </choice> </define> <!-- + Control ACPI sleep states (dis)allowed for the domain + For each of the states the following rules apply: + on: the state will be forcefully enabled + off: the state will be forcefully disabled + not specified: hypervisor will be left to decide its defaults + --> + <define name="pm"> + <element name="pm"> + <interleave> + <optional> + <attribute name="s3"> + <ref name="suspendChoices"/> + </attribute> + </optional> + <optional> + <attribute name="s4"> + <ref name="suspendChoices"/> + </attribute> + </optional> + </interleave> + <empty/> + </element> + </define> + <define name="suspendChoices"> + <choice> + <value>on</value> + <value>off</value> + </choice> + </define> + <!-- Specific setup for a qemu emulated character device. Note: this definition doesn't fully specify the constraints on this node. --> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 43b3f80..7a8279b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -124,6 +124,11 @@ VIR_ENUM_IMPL(virDomainLifecycleCrash, VIR_DOMAIN_LIFECYCLE_CRASH_LAST, "coredump-destroy", "coredump-restart") +VIR_ENUM_IMPL(virDomainPMState, VIR_DOMAIN_PM_STATE_LAST, + "default", + "on", + "off") + VIR_ENUM_IMPL(virDomainDevice, VIR_DOMAIN_DEVICE_LAST, "none", "disk", @@ -8381,6 +8386,19 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, virDomainLifecycleCrashTypeFromString) < 0) goto error; + if (virXPathNode("./pm", ctxt)) { + tmp = virXPathString("string(./pm/@s3)", ctxt); + if (tmp) { + def->pm.s3 = virDomainPMStateTypeFromString(tmp); + VIR_FREE(tmp); + } + tmp = virXPathString("string(./pm/@s4)", ctxt); + if (tmp) { + def->pm.s4 = virDomainPMStateTypeFromString(tmp); + VIR_FREE(tmp); + } + } + tmp = virXPathString("string(./clock/@offset)", ctxt); if (tmp) { if ((def->clock.offset = virDomainClockOffsetTypeFromString(tmp)) < 0) { @@ -13061,6 +13079,32 @@ virDomainDefFormatInternal(virDomainDefPtr def, virDomainLifecycleCrashTypeToString) < 0) goto cleanup; + if (def->pm.s3 || def->pm.s4) { + virBufferAddLit(buf, " <pm"); + + if (def->pm.s3) { + const char *tmp = virDomainPMStateTypeToString(def->pm.s3); + if (!tmp) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected PM state %d"), def->pm.s3); + goto cleanup; + } + virBufferAsprintf(buf, " s3='%s'", tmp); + } + + if (def->pm.s4) { + const char *tmp = virDomainPMStateTypeToString(def->pm.s4); + if (!tmp) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected PM state %d"), def->pm.s4); + goto cleanup; + } + virBufferAsprintf(buf, " s4='%s'", tmp); + } + + virBufferAddLit(buf, "/>\n"); + } + virBufferAddLit(buf, " <devices>\n"); virBufferEscapeString(buf, " <emulator>%s</emulator>\n", diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 9fdda78..3eddd5f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1372,6 +1372,14 @@ enum virDomainLifecycleCrashAction { VIR_DOMAIN_LIFECYCLE_CRASH_LAST }; +enum virDomainPMState { + VIR_DOMAIN_PM_STATE_DEFAULT = 0, + VIR_DOMAIN_PM_STATE_ON, + VIR_DOMAIN_PM_STATE_OFF, + + VIR_DOMAIN_PM_STATE_LAST, +}; + enum virDomainBIOSUseserial { VIR_DOMAIN_BIOS_USESERIAL_DEFAULT = 0, VIR_DOMAIN_BIOS_USESERIAL_YES, @@ -1618,6 +1626,11 @@ struct _virDomainDef { int onPoweroff; int onCrash; + struct { + int s3; + int s4; + } pm; + virDomainOSDef os; char *emulator; int features; @@ -2165,6 +2178,7 @@ VIR_ENUM_DECL(virDomainBoot) VIR_ENUM_DECL(virDomainFeature) VIR_ENUM_DECL(virDomainLifecycle) VIR_ENUM_DECL(virDomainLifecycleCrash) +VIR_ENUM_DECL(virDomainPMState) VIR_ENUM_DECL(virDomainDevice) VIR_ENUM_DECL(virDomainDeviceAddress) VIR_ENUM_DECL(virDomainDeviceAddressPciMulti) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index bb8849b..98bcb98 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -431,6 +431,8 @@ virDomainPausedReasonTypeFromString; virDomainPausedReasonTypeToString; virDomainPciRombarModeTypeFromString; virDomainPciRombarModeTypeToString; +virDomainPMStateTypeFromString; +virDomainPMStateTypeToString; virDomainRedirdevBusTypeFromString; virDomainRedirdevBusTypeToString; virDomainRemoveInactive; -- 1.7.8.6
On 08/02/2012 06:05 AM, Martin Kletzander wrote:
There is a new <pm/> element implemented that can control what ACPI sleeping states will be advertised by BIOS and allowed to be switched to by libvirt. The default keeps defaults on hypervisor, otherwise forces chosen setting.
You are proposing /domain/pm; but we also have /domain/os/bios, would this be better as a subelement /domain/os/bios/pm, since it is related to bios options?
--- docs/schemas/domaincommon.rng | 33 ++++++++++++++++++++++++++++++ src/conf/domain_conf.c | 44 +++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 14 +++++++++++++ src/libvirt_private.syms | 2 + 4 files changed, 93 insertions(+), 0 deletions(-)
@@ -2192,6 +2195,36 @@ </choice> </define> <!-- + Control ACPI sleep states (dis)allowed for the domain + For each of the states the following rules apply: + on: the state will be forcefully enabled + off: the state will be forcefully disabled + not specified: hypervisor will be left to decide its defaults + --> + <define name="pm"> + <element name="pm"> + <interleave> + <optional> + <attribute name="s3">
Is the name 's3' too x86-centric? Remember, for virNodeSuspendForDuration, we went with the names 'mem' (s3) and 'disk' (s4) to give a more descriptive naming for where we were suspending. So controlling whether s3 is advertised is really controlling whether 'suspend-to-memory' is advertised.
+++ b/src/conf/domain_conf.h @@ -1372,6 +1372,14 @@ enum virDomainLifecycleCrashAction { VIR_DOMAIN_LIFECYCLE_CRASH_LAST };
+enum virDomainPMState { + VIR_DOMAIN_PM_STATE_DEFAULT = 0, + VIR_DOMAIN_PM_STATE_ON, + VIR_DOMAIN_PM_STATE_OFF, + + VIR_DOMAIN_PM_STATE_LAST, +}; + enum virDomainBIOSUseserial { VIR_DOMAIN_BIOS_USESERIAL_DEFAULT = 0, VIR_DOMAIN_BIOS_USESERIAL_YES, @@ -1618,6 +1626,11 @@ struct _virDomainDef { int onPoweroff; int onCrash;
+ struct { + int s3; + int s4;
Add a comment that these values are type enum virDomainPMState. The code itself looks decent (and thanks for the test cases), but I'd like a second opinion on whether the choice of XML naming is the best; if we choose a different naming scheme, I think you should be able to adapt the rest of the patch pretty easily. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Thu, Aug 2, 2012 at 3:36 PM, Eric Blake <eblake@redhat.com> wrote:
On 08/02/2012 06:05 AM, Martin Kletzander wrote:
There is a new <pm/> element implemented that can control what ACPI sleeping states will be advertised by BIOS and allowed to be switched to by libvirt. The default keeps defaults on hypervisor, otherwise forces chosen setting.
You are proposing /domain/pm; but we also have /domain/os/bios, would this be better as a subelement /domain/os/bios/pm, since it is related to bios options?
I would say that /domain/os/bios/pm isn't the correct place because /domain/os relates only to OS booting while Power Management is outside of the scope of booting. In fact /domain/os/bios simply controls whether the BIOS output will be over VGA or over serial. Features specific to the system are typically kept as top level items, which matches /domain/pm. I would however say that using the terms ACPI, s3 and s4 isn't good design for the XML. You can leave that to documentation but /domain/pm/suspend-to-mem and /domain/pm/suspend-to-disk are better so that the same XML structure can be used for PPC. They have concepts similar to s3 and s4 (I only briefly Googled this) but they're named completely differently. The only under place I could see this would be /domain/features/acpi. If its there then s3 and s4 potentially make sense, but then we're left without a place for PPC to tie in. -- Doug Goldstein
On 08/03/2012 07:16 AM, Doug Goldstein wrote:
On Thu, Aug 2, 2012 at 3:36 PM, Eric Blake <eblake@redhat.com> wrote:
On 08/02/2012 06:05 AM, Martin Kletzander wrote:
There is a new <pm/> element implemented that can control what ACPI sleeping states will be advertised by BIOS and allowed to be switched to by libvirt. The default keeps defaults on hypervisor, otherwise forces chosen setting.
You are proposing /domain/pm; but we also have /domain/os/bios, would this be better as a subelement /domain/os/bios/pm, since it is related to bios options?
I would say that /domain/os/bios/pm isn't the correct place because /domain/os relates only to OS booting while Power Management is outside of the scope of booting. In fact /domain/os/bios simply controls whether the BIOS output will be over VGA or over serial. Features specific to the system are typically kept as top level items, which matches /domain/pm. I would however say that using the terms ACPI, s3 and s4 isn't good design for the XML. You can leave that to documentation but /domain/pm/suspend-to-mem and /domain/pm/suspend-to-disk are better so that the same XML structure can be used for PPC. They have concepts similar to s3 and s4 (I only briefly Googled this) but they're named completely differently. The only under place I could see this would be /domain/features/acpi. If its there then s3 and s4 potentially make sense, but then we're left without a place for PPC to tie in.
Thanks both of you for the tips. I also see I haven't added this to the documentation, which I have to fix in next version of the patch. I have no problem adapting the naming in XML to whatever will suit you. I was thinking to put this into /domain/sysinfo, but it looked like it doesn't fit there and I couldn't make it tristate in /domain/features, so that's why I've chosen this.
Il 03/08/2012 07:16, Doug Goldstein ha scritto:
You are proposing /domain/pm; but we also have /domain/os/bios, would this be better as a subelement /domain/os/bios/pm, since it is related to bios options? I would say that /domain/os/bios/pm isn't the correct place because /domain/os relates only to OS booting while Power Management is outside of the scope of booting. In fact /domain/os/bios simply controls whether the BIOS output will be over VGA or over serial. Features specific to the system are typically kept as top level items, which matches /domain/pm. I would however say that using the terms ACPI, s3 and s4 isn't good design for the XML. You can leave that to documentation but /domain/pm/suspend-to-mem and /domain/pm/suspend-to-disk are better so that the same XML structure can be used for PPC. They have concepts similar to s3 and s4 (I only briefly Googled this) but they're named completely differently. The only under place I could see this would be /domain/features/acpi. If its there then s3 and s4 potentially make sense, but then we're left without a place for PPC to tie in.
I would put it under /domain/features/pm. Paolo
On 08/03/2012 09:46 AM, Paolo Bonzini wrote:
Il 03/08/2012 07:16, Doug Goldstein ha scritto:
You are proposing /domain/pm; but we also have /domain/os/bios, would this be better as a subelement /domain/os/bios/pm, since it is related to bios options? I would say that /domain/os/bios/pm isn't the correct place because /domain/os relates only to OS booting while Power Management is outside of the scope of booting. In fact /domain/os/bios simply controls whether the BIOS output will be over VGA or over serial. Features specific to the system are typically kept as top level items, which matches /domain/pm. I would however say that using the terms ACPI, s3 and s4 isn't good design for the XML. You can leave that to documentation but /domain/pm/suspend-to-mem and /domain/pm/suspend-to-disk are better so that the same XML structure can be used for PPC. They have concepts similar to s3 and s4 (I only briefly Googled this) but they're named completely differently. The only under place I could see this would be /domain/features/acpi. If its there then s3 and s4 potentially make sense, but then we're left without a place for PPC to tie in.
I would put it under /domain/features/pm.
Paolo
In v1 I had this in /domain/features, but due to the way the features are parsed (they are just flags) I couldn't make it tristate as danpb suggested (and he was right), so I had to choose a different place, but otherwise it would fit there logically, yes. Martin
This patch adds support for running qemu guests with the required parameters to forcefully enable or disable BIOS advertising of S3 and S4 states. The support for this is added to capabilities and there is also a qemu command parameter parsing implemented. --- src/qemu/qemu_capabilities.c | 7 +++ src/qemu/qemu_capabilities.h | 2 + src/qemu/qemu_command.c | 103 ++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_driver.c | 17 +++++++ 4 files changed, 129 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 85c49a2..b4a662f 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -169,6 +169,8 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST, "virtio-s390", "balloon-event", + "disable-s3", /* 100 */ + "disable-s4", ); struct qemu_feature_flags { @@ -1396,6 +1398,7 @@ qemuCapsExtractDeviceStr(const char *qemu, "-device", "virtio-blk-pci,?", "-device", "virtio-net-pci,?", "-device", "scsi-disk,?", + "-device", "PIIX4_PM,?", NULL); /* qemu -help goes to stdout, but qemu -device ? goes to stderr. */ virCommandSetErrorBuffer(cmd, &output); @@ -1487,6 +1490,10 @@ qemuCapsParseDeviceStr(const char *str, virBitmapPtr flags) qemuCapsSet(flags, QEMU_CAPS_SCSI_CD); if (strstr(str, "ide-cd")) qemuCapsSet(flags, QEMU_CAPS_IDE_CD); + if (strstr(str, "PIIX4_PM.disable_s3=")) + qemuCapsSet(flags, QEMU_CAPS_DISABLE_S3); + if (strstr(str, "PIIX4_PM.disable_s4=")) + qemuCapsSet(flags, QEMU_CAPS_DISABLE_S4); return 0; } diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index e8251dc..a909f67 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -135,6 +135,8 @@ enum qemuCapsFlags { QEMU_CAPS_NEC_USB_XHCI = 97, /* -device nec-usb-xhci */ QEMU_CAPS_VIRTIO_S390 = 98, /* -device virtio-*-s390 */ QEMU_CAPS_BALLOON_EVENT = 99, /* Async event for balloon changes */ + QEMU_CAPS_DISABLE_S3 = 100, /* S3 BIOS Advertisement on/off */ + QEMU_CAPS_DISABLE_S4 = 101, /* S4 BIOS Advertisement on/off */ QEMU_CAPS_LAST, /* this must always be the last item */ }; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6ad65a6..a30c2f1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4692,6 +4692,48 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, "-no-acpi"); } + if (def->pm.s3) { + if (qemuCapsGet(qemuCaps, QEMU_CAPS_DISABLE_S3)) { + if (def->pm.s3 == VIR_DOMAIN_PM_STATE_ON) + virCommandAddArgList(cmd, + "-global", + "PIIX4_PM.disable_s3=0", + NULL); + + else if (def->pm.s3 == VIR_DOMAIN_PM_STATE_OFF) + virCommandAddArgList(cmd, + "-global", + "PIIX4_PM.disable_s3=1", + NULL); + + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", _("setting ACPI S3 not supported")); + goto error; + } + } + + if (def->pm.s4) { + if (qemuCapsGet(qemuCaps, QEMU_CAPS_DISABLE_S4)) { + if (def->pm.s4 == VIR_DOMAIN_PM_STATE_ON) + virCommandAddArgList(cmd, + "-global", + "PIIX4_PM.disable_s4=0", + NULL); + + else if (def->pm.s4 == VIR_DOMAIN_PM_STATE_OFF) + virCommandAddArgList(cmd, + "-global", + "PIIX4_PM.disable_s4=1", + NULL); + + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", _("setting ACPI S4 not supported")); + goto error; + } + } + if (!def->os.bootloader) { /* * We prefer using explicit bootindex=N parameters for predictable @@ -8199,6 +8241,67 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, *monConfig = chr; } + } else if (STREQ(arg, "-global") && + STRPREFIX(progargv[i + 1], "PIIX4_PM.disable_s3=")) { + /* We want to parse only the known "-global" parameters, + * so the ones that we don't know are still added to the + * namespace */ + WANT_VALUE(); + + val += strlen("PIIX4_PM.disable_s3="); + + if (strlen(val) != 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid value for disable_s3 parameter: " + "'%s'"), val); + goto error; + } + + switch (*val) { + case '0': + def->pm.s3 = VIR_DOMAIN_PM_STATE_ON; + break; + + case '1': + def->pm.s3 = VIR_DOMAIN_PM_STATE_OFF; + break; + + default: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid value for disable_s3 parameter: " + "'%s'"), val); + goto error; + } + + } else if (STREQ(arg, "-global") && + STRPREFIX(progargv[i + 1], "PIIX4_PM.disable_s4=")) { + WANT_VALUE(); + + val += strlen("PIIX4_PM.disable_s4="); + + if (strlen(val) != 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid value for disable_s4 parameter: " + "'%s'"), val); + goto error; + } + + switch (*val) { + case '0': + def->pm.s4 = VIR_DOMAIN_PM_STATE_ON; + break; + + case '1': + def->pm.s4 = VIR_DOMAIN_PM_STATE_OFF; + break; + + default: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid value for disable_s4 parameter: " + "'%s'"), val); + goto error; + } + } else if (STREQ(arg, "-S")) { /* ignore, always added by libvirt */ } else { diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b3f946c..24712ce 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13066,6 +13066,23 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom, goto cleanup; } + if (vm->def->pm.s3 || vm->def->pm.s4) { + if (!vm->def->pm.s3 == VIR_DOMAIN_PM_STATE_OFF && + (target == VIR_NODE_SUSPEND_TARGET_MEM || + target == VIR_NODE_SUSPEND_TARGET_HYBRID)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("S3 signals are disabled for this domain")); + goto cleanup; + } + + if (vm->def->pm.s4 == VIR_DOMAIN_PM_STATE_OFF && + target == VIR_NODE_SUSPEND_TARGET_DISK) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("S4 signals are disabled for this domain")); + goto cleanup; + } + } + if (priv->agentError) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("QEMU guest agent is not available due to an error")); -- 1.7.8.6
On 08/02/2012 06:05 AM, Martin Kletzander wrote:
This patch adds support for running qemu guests with the required parameters to forcefully enable or disable BIOS advertising of S3 and S4 states. The support for this is added to capabilities and there is also a qemu command parameter parsing implemented. --- src/qemu/qemu_capabilities.c | 7 +++ src/qemu/qemu_capabilities.h | 2 + src/qemu/qemu_command.c | 103 ++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_driver.c | 17 +++++++ 4 files changed, 129 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 85c49a2..b4a662f 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -169,6 +169,8 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST, "virtio-s390", "balloon-event",
+ "disable-s3", /* 100 */ + "disable-s4",
Do we need two capability bits here, or are we guaranteed that both command line options are always present or absent together and one bit sufficient? If we use just one bit, maybe naming it 'disable-pm' would be reasonable. Then again, the qemu folks might say that it really should be two independent bits, such as if it is easier to backport just one of the two controls to an older qemu. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08/02/2012 10:48 PM, Eric Blake wrote:
On 08/02/2012 06:05 AM, Martin Kletzander wrote:
This patch adds support for running qemu guests with the required parameters to forcefully enable or disable BIOS advertising of S3 and S4 states. The support for this is added to capabilities and there is also a qemu command parameter parsing implemented. --- src/qemu/qemu_capabilities.c | 7 +++ src/qemu/qemu_capabilities.h | 2 + src/qemu/qemu_command.c | 103 ++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_driver.c | 17 +++++++ 4 files changed, 129 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 85c49a2..b4a662f 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -169,6 +169,8 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST, "virtio-s390", "balloon-event",
+ "disable-s3", /* 100 */ + "disable-s4",
Do we need two capability bits here, or are we guaranteed that both command line options are always present or absent together and one bit sufficient? If we use just one bit, maybe naming it 'disable-pm' would be reasonable. Then again, the qemu folks might say that it really should be two independent bits, such as if it is easier to backport just one of the two controls to an older qemu.
It depends. We can make it as one bit and check only for one of those (or both). I don't think this will be dropped any time nor that someone has qemu old enough not to support this. The level to which these checks should be done is very hard to guess for me and can vary a lot. For example check that qemu will use PIIX4_PM device, because otherwise these values mean nothing. OTOH qemu uses this one by default even without specifying that, because there has to be some chipset and this is the only one that exists there (if I understood that correctly). Hard to say, but I'll gladly listen to more experienced colleague ;) Martin
Few tests were added which are checking whether the parsing of the xml and command-line arguments is working and compatible with each other. --- tests/qemuargv2xmltest.c | 3 ++ .../qemuxml2argv-misc-disable-s3.args | 4 +++ .../qemuxml2argv-misc-disable-s3.xml | 27 ++++++++++++++++++++ .../qemuxml2argv-misc-disable-suspends.args | 4 +++ .../qemuxml2argv-misc-disable-suspends.xml | 27 ++++++++++++++++++++ .../qemuxml2argv-misc-enable-s4.args | 4 +++ .../qemuxml2argv-misc-enable-s4.xml | 27 ++++++++++++++++++++ tests/qemuxml2argvtest.c | 4 +++ tests/qemuxml2xmltest.c | 3 ++ 9 files changed, 103 insertions(+), 0 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-misc-disable-s3.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-misc-disable-s3.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-misc-disable-suspends.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-misc-disable-suspends.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-misc-enable-s4.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-misc-enable-s4.xml diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c index 439218e..ad5f45b 100644 --- a/tests/qemuargv2xmltest.c +++ b/tests/qemuargv2xmltest.c @@ -205,6 +205,9 @@ mymain(void) /* Can't rountrip xenner arch */ /*DO_TEST("input-xen");*/ DO_TEST("misc-acpi"); + DO_TEST("misc-disable-s3"); + DO_TEST("misc-disable-suspends"); + DO_TEST("misc-enable-s4"); DO_TEST("misc-no-reboot"); DO_TEST("misc-uuid"); DO_TEST("net-user"); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-misc-disable-s3.args b/tests/qemuxml2argvdata/qemuxml2argv-misc-disable-s3.args new file mode 100644 index 0000000..dbee64d --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-misc-disable-s3.args @@ -0,0 +1,4 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S \ +-M pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait \ +-no-acpi -global PIIX4_PM.disable_s3=1 -boot c -hda /dev/HostVG/QEMUGuest1 \ +-net none -serial none -parallel none -usb diff --git a/tests/qemuxml2argvdata/qemuxml2argv-misc-disable-s3.xml b/tests/qemuxml2argvdata/qemuxml2argv-misc-disable-s3.xml new file mode 100644 index 0000000..7938245 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-misc-disable-s3.xml @@ -0,0 +1,27 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>8caaa98c-e7bf-5845-126a-1fc316bd1089</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <pm s3='off'/> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-misc-disable-suspends.args b/tests/qemuxml2argvdata/qemuxml2argv-misc-disable-suspends.args new file mode 100644 index 0000000..6f63d8b --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-misc-disable-suspends.args @@ -0,0 +1,4 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S \ +-M pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait \ +-no-acpi -global PIIX4_PM.disable_s3=1 -global PIIX4_PM.disable_s4=1 -boot c \ +-hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -usb diff --git a/tests/qemuxml2argvdata/qemuxml2argv-misc-disable-suspends.xml b/tests/qemuxml2argvdata/qemuxml2argv-misc-disable-suspends.xml new file mode 100644 index 0000000..0b28584 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-misc-disable-suspends.xml @@ -0,0 +1,27 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>8caaa98c-e7bf-5845-126a-1fc316bd1089</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <pm s3='off' s4='off'/> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-misc-enable-s4.args b/tests/qemuxml2argvdata/qemuxml2argv-misc-enable-s4.args new file mode 100644 index 0000000..b54e7b8 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-misc-enable-s4.args @@ -0,0 +1,4 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S \ +-M pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait \ +-no-acpi -global PIIX4_PM.disable_s4=0 -boot c -hda /dev/HostVG/QEMUGuest1 \ +-net none -serial none -parallel none -usb diff --git a/tests/qemuxml2argvdata/qemuxml2argv-misc-enable-s4.xml b/tests/qemuxml2argvdata/qemuxml2argv-misc-enable-s4.xml new file mode 100644 index 0000000..61b8299 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-misc-enable-s4.xml @@ -0,0 +1,27 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>8caaa98c-e7bf-5845-126a-1fc316bd1089</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <pm s4='on'/> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 39fcd9f..a3f52a6 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -561,6 +561,10 @@ mymain(void) DO_TEST("input-usbtablet", NONE); DO_TEST_ERROR("input-xen", QEMU_CAPS_DOMID); DO_TEST("misc-acpi", NONE); + DO_TEST("misc-disable-s3", QEMU_CAPS_DISABLE_S3); + DO_TEST("misc-disable-suspends", QEMU_CAPS_DISABLE_S3, QEMU_CAPS_DISABLE_S4); + DO_TEST("misc-enable-s4", QEMU_CAPS_DISABLE_S4); + DO_TEST_FAILURE("misc-enable-s4", NONE); DO_TEST("misc-no-reboot", NONE); DO_TEST("misc-uuid", QEMU_CAPS_NAME, QEMU_CAPS_UUID); DO_TEST("net-user", NONE); diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index dcdba4f..b60736f 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -170,6 +170,9 @@ mymain(void) DO_TEST("input-usbtablet"); DO_TEST("input-xen"); DO_TEST("misc-acpi"); + DO_TEST("misc-disable-s3"); + DO_TEST("misc-disable-suspends"); + DO_TEST("misc-enable-s4"); DO_TEST("misc-no-reboot"); DO_TEST("net-user"); DO_TEST("net-virtio"); -- 1.7.8.6
participants (4)
-
Doug Goldstein -
Eric Blake -
Martin Kletzander -
Paolo Bonzini