Re: [libvirt] [PATCH 4/4] qemu: command: wire up usage of q35/ich9 disable s3/s4

On 01/10/2016 04:54 AM, Martin Kletzander wrote: > 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"? > Yeah, I meant supported. Fixed >> 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. > Yeah I'm still a little scared of rejecting configs that plausibly exist in the wild, when the end result is the same either (PM wasn't disabled). We can add it later though >> --- >> 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"; That doesn't work as is with my patch since it doesn't incorporate checking qemuCaps. I made a similarish change though and pushed. Thanks for the review!