On 09.08.2012 10:26, 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 5472267..b8160b6 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -172,6 +172,8 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST,
> "bridge", /* 100 */
> "lsi",
> "virtio-scsi-pci",
> + "disable-s3",
> + "disable-s4",
>
> );
>
> @@ -1403,6 +1405,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);
> @@ -1499,6 +1502,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 d606890..e49424a 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -138,6 +138,8 @@ enum qemuCapsFlags {
> QEMU_CAPS_NETDEV_BRIDGE = 100, /* bridge helper support */
> QEMU_CAPS_SCSI_LSI = 101, /* -device lsi */
> QEMU_CAPS_VIRTIO_SCSI_PCI = 102, /* -device virtio-scsi-pci */
> + QEMU_CAPS_DISABLE_S3 = 103, /* S3 BIOS Advertisement on/off */
> + QEMU_CAPS_DISABLE_S4 = 104, /* 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 9383530..34ee00e 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -4761,6 +4761,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)
Redundant if. After the first condition def->pm.s3 can be either _ON or
_OFF. Nothing else.
> + 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)
ditto
> + 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
> @@ -8279,6 +8321,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,
it's not an internal error really.
s/VIR_ERR_INTERNAL_ERROR/VIR_ERR_CONFIG_UNSUPPORTED/
I know we misuse VIR_INTERNAL_ERROR in much places, but that should be
fixed too.
> + _("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,
s/VIR_ERR_INTERNAL_ERROR/VIR_ERR_CONFIG_UNSUPPORTED/
btw: what about optimizing this a bit? My intent is to join this and
previous virReportError() into one:
val += strlen("PIIX4_PM.disable_s3=");
if (STREQ(val, "0"))
def->pm.s3 = VIR_DOMAIN_PM_STATE_ON;
else if (STREQ(val, "1"))
def->pm.s3 = VIR_DOMAIN_PM_STATE_OFF;
else
virReportError();
This apply for s4 as well.
> + _("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 dee1268..13d5917 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -13097,6 +13097,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"));
s/signals are/state is/
> + 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"));
ditto
> + goto cleanup;
> + }
> + }
> +
> if (priv->agentError) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("QEMU guest agent is not available due to an error"));
>
Otherwise looking good.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list