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.
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list