[libvirt] [PATCH 4/4] qemu: command: wire up usage of q35/ich9 disable s3/s4
Martin Kletzander
mkletzan at redhat.com
Sun Jan 10 09:54:46 UTC 2016
On Sat, Jan 09, 2016 at 04:32:23PM -0500, Cole Robinson wrote:
>If the q35 specific disable s3/s4 setting isn't disabled, fallback to
this reads weirdly, maybe you meant "supported" instead of "disabled"?
>specifying the PIIX setting, which is the previous behavior. It doesn't
>have any effect, but qemu will just warn about it rather than error:
>
>qemu-system-x86_64: Warning: global PIIX4_PM.disable_s3=1 not used
>qemu-system-x86_64: Warning: global PIIX4_PM.disable_s4=1 not used
>
>Since it doesn't error, I don't think we should either, since there
>may be configs in the wild that already have q35 + disable_s3/4 (via
>virt-manager)
We can even skip specifying it at all, back when I implemented it there
were just no nono-pc x86 machines types =) Or we can error out when
starting as we do with other changes that would otherwise be
incompatible. If the error message is clear, I see no problem with it.
But that can be changed later on.
>---
> src/qemu/qemu_command.c | 32 ++++++++++++++++++----
> .../qemuxml2argv-q35-pm-disable-fallback.args | 23 ++++++++++++++++
> .../qemuxml2argv-q35-pm-disable-fallback.xml | 18 ++++++++++++
> .../qemuxml2argv-q35-pm-disable.args | 23 ++++++++++++++++
> .../qemuxml2argv-q35-pm-disable.xml | 18 ++++++++++++
> tests/qemuxml2argvtest.c | 9 ++++++
> 6 files changed, 117 insertions(+), 6 deletions(-)
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-pm-disable-fallback.args
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-pm-disable-fallback.xml
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-pm-disable.args
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-pm-disable.xml
>
>diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>index 0ee3247..dc7adcb 100644
>--- a/src/qemu/qemu_command.c
>+++ b/src/qemu/qemu_command.c
>@@ -9806,25 +9806,45 @@ qemuBuildCommandLine(virConnectPtr conn,
> }
>
> if (def->pm.s3) {
>- if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX_DISABLE_S3)) {
>+ const char *pm_object = NULL;
>+
>+ if (qemuDomainMachineIsQ35(def) &&
>+ virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_DISABLE_S3)) {
>+ pm_object = "ICH9-LPC";
>+ } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX_DISABLE_S3)) {
>+ /* We fall back to this even for q35, since it's what we did
>+ pre-q35-pm support. QEMU starts up fine (with a warning) if
>+ mixing PIIX PM and -M q35. Starting to reject things here
>+ could mean we refuse to start existing configs in the wild.*/
>+ pm_object = "PIIX4_PM";
>+ } else {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> "%s", _("setting ACPI S3 not supported"));
> goto error;
> }
>+
> virCommandAddArg(cmd, "-global");
>- virCommandAddArgFormat(cmd, "PIIX4_PM.disable_s3=%d",
>- def->pm.s3 == VIR_TRISTATE_BOOL_NO);
>+ virCommandAddArgFormat(cmd, "%s.disable_s3=%d",
>+ pm_object, def->pm.s3 == VIR_TRISTATE_BOOL_NO);
> }
>
> if (def->pm.s4) {
>- if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX_DISABLE_S4)) {
>+ const char *pm_object;
>+
In the previous block you initialize it, but here you don't. How about
putting it out of these blocks and just initialize it to:
pm_object = qemuDomainMachineIsQ35(def) ? "ICH9-LPC" : "PIIX4_PM";
Your version works as well, it's just inconsistent. Other than that, it
looks fine.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160110/16bce3ec/attachment-0001.sig>
More information about the libvir-list
mailing list