[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 2/4] qemu: Add support for S3/S4 state configuration



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]