[libvirt] [PATCH 0/4] qemu: Support disable_s3/s4 for -M q35
q35/ICH9 uses a different qemu option for disabling s3/s4 support. Probe for it and wire it up. Cole Robinson (4): qemu: capabilities: s/Pixx/Piix/g qemu: caps: Rename CAPS_DISABLE_S[34] to CAPS_PIIX_DISABLE_S[34] qemu: caps: check for q35/ICH9 disable S3/S4 qemu: command: wire up usage of q35/ich9 disable s3/s4 src/qemu/qemu_capabilities.c | 19 +++-- src/qemu/qemu_capabilities.h | 6 +- src/qemu/qemu_command.c | 32 ++++++-- tests/qemucapabilitiesdata/caps_1.2.2-1.replies | 23 ++++-- tests/qemucapabilitiesdata/caps_1.3.1-1.replies | 22 ++++-- tests/qemucapabilitiesdata/caps_1.4.2-1.replies | 23 ++++-- tests/qemucapabilitiesdata/caps_1.5.3-1.replies | 22 ++++-- tests/qemucapabilitiesdata/caps_1.6.0-1.replies | 22 ++++-- tests/qemucapabilitiesdata/caps_1.6.50-1.replies | 22 ++++-- tests/qemucapabilitiesdata/caps_2.1.1-1.replies | 22 ++++-- tests/qemucapabilitiesdata/caps_2.4.0-1.caps | 2 + tests/qemucapabilitiesdata/caps_2.4.0-1.replies | 92 ++++++++++++++++++++-- tests/qemucapabilitiesdata/caps_2.5.0-1.caps | 2 + tests/qemucapabilitiesdata/caps_2.5.0-1.replies | 92 ++++++++++++++++++++-- tests/qemucapabilitiesdata/caps_2.6.0-1.caps | 2 + tests/qemucapabilitiesdata/caps_2.6.0-1.replies | 92 ++++++++++++++++++++-- .../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 | 17 +++- 21 files changed, 507 insertions(+), 87 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 -- 2.5.0
The chipset is called PIIX; the functions are misnamed --- src/qemu/qemu_capabilities.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 7b30441..80dd936 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -1596,7 +1596,7 @@ static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsIDEDrive[] = { { "wwn", QEMU_CAPS_IDE_DRIVE_WWN }, }; -static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsPixx4PM[] = { +static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsPiix4PM[] = { { "disable_s3", QEMU_CAPS_DISABLE_S3 }, { "disable_s4", QEMU_CAPS_DISABLE_S4 }, }; @@ -1679,8 +1679,8 @@ static struct virQEMUCapsObjectTypeProps virQEMUCapsObjectProps[] = { ARRAY_CARDINALITY(virQEMUCapsObjectPropsSCSIDisk) }, { "ide-drive", virQEMUCapsObjectPropsIDEDrive, ARRAY_CARDINALITY(virQEMUCapsObjectPropsIDEDrive) }, - { "PIIX4_PM", virQEMUCapsObjectPropsPixx4PM, - ARRAY_CARDINALITY(virQEMUCapsObjectPropsPixx4PM) }, + { "PIIX4_PM", virQEMUCapsObjectPropsPiix4PM, + ARRAY_CARDINALITY(virQEMUCapsObjectPropsPiix4PM) }, { "usb-redir", virQEMUCapsObjectPropsUSBRedir, ARRAY_CARDINALITY(virQEMUCapsObjectPropsUSBRedir) }, { "usb-host", virQEMUCapsObjectPropsUSBHost, -- 2.5.0
These settings are specific to PIIX, so clarify it --- src/qemu/qemu_capabilities.c | 4 ++-- src/qemu/qemu_capabilities.h | 4 ++-- src/qemu/qemu_command.c | 4 ++-- tests/qemuxml2argvtest.c | 8 ++++---- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 80dd936..475febc 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -1597,8 +1597,8 @@ static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsIDEDrive[] = { }; static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsPiix4PM[] = { - { "disable_s3", QEMU_CAPS_DISABLE_S3 }, - { "disable_s4", QEMU_CAPS_DISABLE_S4 }, + { "disable_s3", QEMU_CAPS_PIIX_DISABLE_S3 }, + { "disable_s4", QEMU_CAPS_PIIX_DISABLE_S4 }, }; static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsUSBRedir[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index c148f2d..99879d8 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -190,10 +190,10 @@ typedef enum { QEMU_CAPS_SCSI_LSI, /* -device lsi */ QEMU_CAPS_VIRTIO_SCSI, /* -device virtio-scsi-* */ QEMU_CAPS_BLOCKIO, /* -device ...logical_block_size & co */ - QEMU_CAPS_DISABLE_S3, /* S3 BIOS Advertisement on/off */ + QEMU_CAPS_PIIX_DISABLE_S3, /* -M pc S3 BIOS Advertisement on/off */ /* 105 */ - QEMU_CAPS_DISABLE_S4, /* S4 BIOS Advertisement on/off */ + QEMU_CAPS_PIIX_DISABLE_S4, /* -M pc S4 BIOS Advertisement on/off */ QEMU_CAPS_USB_REDIR_FILTER, /* usb-redir.filter */ QEMU_CAPS_IDE_DRIVE_WWN, /* Is ide-drive.wwn available? */ QEMU_CAPS_SCSI_DISK_WWN, /* Is scsi-disk.wwn available? */ diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 1f05935..0ee3247 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9806,7 +9806,7 @@ qemuBuildCommandLine(virConnectPtr conn, } if (def->pm.s3) { - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DISABLE_S3)) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX_DISABLE_S3)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("setting ACPI S3 not supported")); goto error; @@ -9817,7 +9817,7 @@ qemuBuildCommandLine(virConnectPtr conn, } if (def->pm.s4) { - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DISABLE_S4)) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX_DISABLE_S4)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("setting ACPI S4 not supported")); goto error; diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index b023522..b75d453 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -702,7 +702,7 @@ mymain(void) DO_TEST("hugepages", QEMU_CAPS_MEM_PATH); DO_TEST("hugepages-numa", QEMU_CAPS_RTC, QEMU_CAPS_NO_KVM_PIT, - QEMU_CAPS_DISABLE_S3, QEMU_CAPS_DISABLE_S4, + QEMU_CAPS_PIIX_DISABLE_S3, QEMU_CAPS_PIIX_DISABLE_S4, QEMU_CAPS_DEVICE, QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_ICH9_USB_EHCI1, QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_SPICE, QEMU_CAPS_CHARDEV_SPICEVMC, @@ -970,9 +970,9 @@ mymain(void) DO_TEST("input-usbmouse", NONE); DO_TEST("input-usbtablet", NONE); DO_TEST("misc-acpi", NONE); - DO_TEST("misc-disable-s3", QEMU_CAPS_DISABLE_S3); - DO_TEST("misc-disable-suspends", QEMU_CAPS_DISABLE_S3, QEMU_CAPS_DISABLE_S4); - DO_TEST("misc-enable-s4", QEMU_CAPS_DISABLE_S4); + DO_TEST("misc-disable-s3", QEMU_CAPS_PIIX_DISABLE_S3); + DO_TEST("misc-disable-suspends", QEMU_CAPS_PIIX_DISABLE_S3, QEMU_CAPS_PIIX_DISABLE_S4); + DO_TEST("misc-enable-s4", QEMU_CAPS_PIIX_DISABLE_S4); DO_TEST_FAILURE("misc-enable-s4", NONE); DO_TEST("misc-no-reboot", NONE); DO_TEST("misc-uuid", NONE); -- 2.5.0
Update test data to match --- src/qemu/qemu_capabilities.c | 9 +++ src/qemu/qemu_capabilities.h | 2 + tests/qemucapabilitiesdata/caps_1.2.2-1.replies | 23 ++++-- tests/qemucapabilitiesdata/caps_1.3.1-1.replies | 22 ++++-- tests/qemucapabilitiesdata/caps_1.4.2-1.replies | 23 ++++-- tests/qemucapabilitiesdata/caps_1.5.3-1.replies | 22 ++++-- tests/qemucapabilitiesdata/caps_1.6.0-1.replies | 22 ++++-- tests/qemucapabilitiesdata/caps_1.6.50-1.replies | 22 ++++-- tests/qemucapabilitiesdata/caps_2.1.1-1.replies | 22 ++++-- tests/qemucapabilitiesdata/caps_2.4.0-1.caps | 2 + tests/qemucapabilitiesdata/caps_2.4.0-1.replies | 92 ++++++++++++++++++++++-- tests/qemucapabilitiesdata/caps_2.5.0-1.caps | 2 + tests/qemucapabilitiesdata/caps_2.5.0-1.replies | 92 ++++++++++++++++++++++-- tests/qemucapabilitiesdata/caps_2.6.0-1.caps | 2 + tests/qemucapabilitiesdata/caps_2.6.0-1.replies | 92 ++++++++++++++++++++++-- 15 files changed, 379 insertions(+), 70 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 475febc..92f42dc 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -309,6 +309,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "virtio-tablet", /* 205 */ "virtio-input-host", "chardev-file-append", + "ich9-disable-s3", + "ich9-disable-s4", ); @@ -1650,6 +1652,11 @@ static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioGpu[] = { { "virgl", QEMU_CAPS_DEVICE_VIRTIO_GPU_VIRGL }, }; +static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsICH9[] = { + { "disable_s3", QEMU_CAPS_ICH9_DISABLE_S3 }, + { "disable_s4", QEMU_CAPS_ICH9_DISABLE_S4 }, +}; + struct virQEMUCapsObjectTypeProps { const char *type; struct virQEMUCapsStringFlags *props; @@ -1705,6 +1712,8 @@ static struct virQEMUCapsObjectTypeProps virQEMUCapsObjectProps[] = { ARRAY_CARDINALITY(virQEMUCapsObjectPropsQxlVga) }, { "virtio-gpu-pci", virQEMUCapsObjectPropsVirtioGpu, ARRAY_CARDINALITY(virQEMUCapsObjectPropsVirtioGpu) }, + { "ICH9-LPC", virQEMUCapsObjectPropsICH9, + ARRAY_CARDINALITY(virQEMUCapsObjectPropsICH9) }, }; diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 99879d8..336031d 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -336,6 +336,8 @@ typedef enum { QEMU_CAPS_VIRTIO_TABLET, /* -device virtio-tablet-{device,pci} */ QEMU_CAPS_VIRTIO_INPUT_HOST, /* -device virtio-input-host-{device,pci} */ QEMU_CAPS_CHARDEV_FILE_APPEND, /* -chardev file,append=on|off */ + QEMU_CAPS_ICH9_DISABLE_S3, /* -M q35 S3 BIOS Advertisement on/off */ + QEMU_CAPS_ICH9_DISABLE_S4, /* -M q35 S4 BIOS Advertisement on/off */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_1.2.2-1.replies b/tests/qemucapabilitiesdata/caps_1.2.2-1.replies index e7de77b..e6cf089 100644 --- a/tests/qemucapabilitiesdata/caps_1.2.2-1.replies +++ b/tests/qemucapabilitiesdata/caps_1.2.2-1.replies @@ -1606,6 +1606,15 @@ } { + "id": "libvirt-32", + "error": { + "class": "DeviceNotFound", + "desc": "Device 'ICH9-LPC' not found" + } +} + + +{ "return": [ { "name": "xenpv" @@ -1649,7 +1658,7 @@ "name": "none" } ], - "id": "libvirt-32" + "id": "libvirt-33" } { @@ -1721,7 +1730,7 @@ "name": "Opteron_G4" } ], - "id": "libvirt-33" + "id": "libvirt-34" } { @@ -1729,11 +1738,11 @@ "enabled": false, "present": true }, - "id": "libvirt-34" + "id": "libvirt-35" } { - "id": "libvirt-35", + "id": "libvirt-36", "error": { "class": "CommandNotFound", "desc": "The command query-tpm-models has not been found" @@ -1741,7 +1750,7 @@ } { - "id": "libvirt-36", + "id": "libvirt-37", "error": { "class": "CommandNotFound", "desc": "The command query-tpm-types has not been found" @@ -1749,7 +1758,7 @@ } { - "id": "libvirt-37", + "id": "libvirt-38", "error": { "class": "CommandNotFound", "desc": "The command query-command-line-options has not been found" @@ -1763,5 +1772,5 @@ "state": false } ], - "id": "libvirt-38" + "id": "libvirt-39" } diff --git a/tests/qemucapabilitiesdata/caps_1.3.1-1.replies b/tests/qemucapabilitiesdata/caps_1.3.1-1.replies index bf9cbad..cd48ead 100644 --- a/tests/qemucapabilitiesdata/caps_1.3.1-1.replies +++ b/tests/qemucapabilitiesdata/caps_1.3.1-1.replies @@ -1785,6 +1785,14 @@ } { + "id": "libvirt-33", + "error": { + "class": "DeviceNotFound", + "desc": "Device 'ICH9-LPC' not found" + } +} + +{ "return": [ { "name": "xenpv" @@ -1835,7 +1843,7 @@ "name": "none" } ], - "id": "libvirt-33" + "id": "libvirt-34" } { @@ -1913,7 +1921,7 @@ "name": "Opteron_G5" } ], - "id": "libvirt-34" + "id": "libvirt-35" } { @@ -1921,11 +1929,11 @@ "enabled": false, "present": true }, - "id": "libvirt-35" + "id": "libvirt-36" } { - "id": "libvirt-36", + "id": "libvirt-37", "error": { "class": "CommandNotFound", "desc": "The command query-tpm-models has not been found" @@ -1933,7 +1941,7 @@ } { - "id": "libvirt-37", + "id": "libvirt-38", "error": { "class": "CommandNotFound", "desc": "The command query-tpm-types has not been found" @@ -1941,7 +1949,7 @@ } { - "id": "libvirt-38", + "id": "libvirt-39", "error": { "class": "CommandNotFound", "desc": "The command query-command-line-options has not been found" @@ -1955,5 +1963,5 @@ "state": false } ], - "id": "libvirt-39" + "id": "libvirt-40" } diff --git a/tests/qemucapabilitiesdata/caps_1.4.2-1.replies b/tests/qemucapabilitiesdata/caps_1.4.2-1.replies index bd7980f..5e8bdb4 100644 --- a/tests/qemucapabilitiesdata/caps_1.4.2-1.replies +++ b/tests/qemucapabilitiesdata/caps_1.4.2-1.replies @@ -1832,6 +1832,15 @@ } { + "id": "libvirt-33", + "error": { + "class": "DeviceNotFound", + "desc": "Device 'ICH9-LPC' not found" + } +} + + +{ "return": [ { "name": "xenpv" @@ -1885,7 +1894,7 @@ "name": "none" } ], - "id": "libvirt-33" + "id": "libvirt-34" } { @@ -1963,7 +1972,7 @@ "name": "qemu64" } ], - "id": "libvirt-34" + "id": "libvirt-35" } { @@ -1971,11 +1980,11 @@ "enabled": false, "present": true }, - "id": "libvirt-35" + "id": "libvirt-36" } { - "id": "libvirt-36", + "id": "libvirt-37", "error": { "class": "CommandNotFound", "desc": "The command query-tpm-models has not been found" @@ -1983,7 +1992,7 @@ } { - "id": "libvirt-37", + "id": "libvirt-38", "error": { "class": "CommandNotFound", "desc": "The command query-tpm-types has not been found" @@ -1991,7 +2000,7 @@ } { - "id": "libvirt-38", + "id": "libvirt-39", "error": { "class": "CommandNotFound", "desc": "The command query-command-line-options has not been found" @@ -2005,5 +2014,5 @@ "state": false } ], - "id": "libvirt-39" + "id": "libvirt-40" } diff --git a/tests/qemucapabilitiesdata/caps_1.5.3-1.replies b/tests/qemucapabilitiesdata/caps_1.5.3-1.replies index abdba6c..1f4081e 100644 --- a/tests/qemucapabilitiesdata/caps_1.5.3-1.replies +++ b/tests/qemucapabilitiesdata/caps_1.5.3-1.replies @@ -1906,6 +1906,14 @@ } { + "id": "libvirt-33", + "error": { + "class": "DeviceNotFound", + "desc": "Device 'ICH9-LPC' not found" + } +} + +{ "return": [ { "name": "pc-q35-1.4", @@ -1975,7 +1983,7 @@ "cpu-max": 1 } ], - "id": "libvirt-33" + "id": "libvirt-34" } { @@ -2053,7 +2061,7 @@ "name": "qemu64" } ], - "id": "libvirt-34" + "id": "libvirt-35" } { @@ -2061,19 +2069,19 @@ "enabled": false, "present": true }, - "id": "libvirt-35" + "id": "libvirt-36" } { "return": [ ], - "id": "libvirt-36" + "id": "libvirt-37" } { "return": [ ], - "id": "libvirt-37" + "id": "libvirt-38" } { @@ -2749,7 +2757,7 @@ "option": "drive" } ], - "id": "libvirt-38" + "id": "libvirt-39" } { @@ -2759,5 +2767,5 @@ "state": false } ], - "id": "libvirt-39" + "id": "libvirt-40" } diff --git a/tests/qemucapabilitiesdata/caps_1.6.0-1.replies b/tests/qemucapabilitiesdata/caps_1.6.0-1.replies index 26a0e9d..63dcde6 100644 --- a/tests/qemucapabilitiesdata/caps_1.6.0-1.replies +++ b/tests/qemucapabilitiesdata/caps_1.6.0-1.replies @@ -1968,6 +1968,14 @@ } { + "id": "libvirt-33", + "error": { + "class": "DeviceNotFound", + "desc": "Device 'ICH9-LPC' not found" + } +} + +{ "return": [ { "name": "xenpv", @@ -2053,7 +2061,7 @@ "cpu-max": 1 } ], - "id": "libvirt-33" + "id": "libvirt-34" } { @@ -2131,7 +2139,7 @@ "name": "qemu64" } ], - "id": "libvirt-34" + "id": "libvirt-35" } { @@ -2139,19 +2147,19 @@ "enabled": false, "present": true }, - "id": "libvirt-35" + "id": "libvirt-36" } { "return": [ ], - "id": "libvirt-36" + "id": "libvirt-37" } { "return": [ ], - "id": "libvirt-37" + "id": "libvirt-38" } { @@ -2729,7 +2737,7 @@ "option": "drive" } ], - "id": "libvirt-38" + "id": "libvirt-39" } { @@ -2751,5 +2759,5 @@ "state": false } ], - "id": "libvirt-39" + "id": "libvirt-40" } diff --git a/tests/qemucapabilitiesdata/caps_1.6.50-1.replies b/tests/qemucapabilitiesdata/caps_1.6.50-1.replies index 5c493b7..869b5ed 100644 --- a/tests/qemucapabilitiesdata/caps_1.6.50-1.replies +++ b/tests/qemucapabilitiesdata/caps_1.6.50-1.replies @@ -1932,6 +1932,14 @@ } { + "id": "libvirt-33", + "error": { + "class": "DeviceNotFound", + "desc": "Device 'ICH9-LPC' not found" + } +} + +{ "return": [ { "name": "xenpv", @@ -2025,7 +2033,7 @@ "cpu-max": 1 } ], - "id": "libvirt-33" + "id": "libvirt-34" } { @@ -2103,7 +2111,7 @@ "name": "qemu64" } ], - "id": "libvirt-34" + "id": "libvirt-35" } { @@ -2111,19 +2119,19 @@ "enabled": false, "present": true }, - "id": "libvirt-35" + "id": "libvirt-36" } { "return": [ ], - "id": "libvirt-36" + "id": "libvirt-37" } { "return": [ ], - "id": "libvirt-37" + "id": "libvirt-38" } { @@ -2711,7 +2719,7 @@ "option": "drive" } ], - "id": "libvirt-38" + "id": "libvirt-39" } { @@ -2733,5 +2741,5 @@ "state": false } ], - "id": "libvirt-39" + "id": "libvirt-40" } diff --git a/tests/qemucapabilitiesdata/caps_2.1.1-1.replies b/tests/qemucapabilitiesdata/caps_2.1.1-1.replies index ba85b28..86047c1 100644 --- a/tests/qemucapabilitiesdata/caps_2.1.1-1.replies +++ b/tests/qemucapabilitiesdata/caps_2.1.1-1.replies @@ -2378,6 +2378,14 @@ } { + "id": "libvirt-33", + "error": { + "class": "DeviceNotFound", + "desc": "Device 'ICH9-LPC' not found" + } +} + +{ "return": [ { "name": "pc-1.3", @@ -2487,7 +2495,7 @@ "cpu-max": 255 } ], - "id": "libvirt-33" + "id": "libvirt-34" } { @@ -2568,7 +2576,7 @@ "name": "qemu64" } ], - "id": "libvirt-34" + "id": "libvirt-35" } { @@ -2576,21 +2584,21 @@ "enabled": false, "present": true }, - "id": "libvirt-35" + "id": "libvirt-36" } { "return": [ "tpm-tis" ], - "id": "libvirt-36" + "id": "libvirt-37" } { "return": [ "passthrough" ], - "id": "libvirt-37" + "id": "libvirt-38" } { @@ -3450,7 +3458,7 @@ "option": "drive" } ], - "id": "libvirt-38" + "id": "libvirt-39" } { @@ -3472,5 +3480,5 @@ "capability": "zero-blocks" } ], - "id": "libvirt-39" + "id": "libvirt-40" } diff --git a/tests/qemucapabilitiesdata/caps_2.4.0-1.caps b/tests/qemucapabilitiesdata/caps_2.4.0-1.caps index d67a48d..ffc09c6 100644 --- a/tests/qemucapabilitiesdata/caps_2.4.0-1.caps +++ b/tests/qemucapabilitiesdata/caps_2.4.0-1.caps @@ -167,4 +167,6 @@ <flag name='virtio-mouse'/> <flag name='virtio-tablet'/> <flag name='virtio-input-host'/> + <flag name='ich9-disable-s3'/> + <flag name='ich9-disable-s4'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_2.4.0-1.replies b/tests/qemucapabilitiesdata/caps_2.4.0-1.replies index 8f50128..8d25759 100644 --- a/tests/qemucapabilitiesdata/caps_2.4.0-1.replies +++ b/tests/qemucapabilitiesdata/caps_2.4.0-1.replies @@ -2759,6 +2759,84 @@ } { + "return": [ + { + "type": "bool", + "name": "memory-hotplug-support" + }, + { + "type": "uint32", + "name": "rombar" + }, + { + "type": "uint32", + "name": "sci_int" + }, + { + "type": "uint32", + "name": "gpe0_blk_len" + }, + { + "type": "uint32", + "name": "pm_io_base" + }, + { + "type": "bool", + "name": "noreboot" + }, + { + "type": "bool", + "name": "multifunction", + "description": "on/off" + }, + { + "type": "uint8", + "name": "disable_s4" + }, + { + "type": "uint8", + "name": "acpi_disable_cmd" + }, + { + "type": "str", + "name": "romfile" + }, + { + "type": "uint8", + "name": "disable_s3" + }, + { + "type": "uint8", + "name": "s4_val" + }, + { + "type": "uint8", + "name": "acpi_enable_cmd" + }, + { + "type": "bool", + "name": "command_serr_enable", + "description": "on/off" + }, + { + "type": "int32", + "name": "addr", + "description": "Slot and optional function number, example: 06.0 or 06" + }, + { + "type": "bool", + "name": "enable_tco" + }, + { + "type": "uint32", + "name": "gpe0_blk" + } + ], + "id": "libvirt-33" +} + + +{ "return": [ { "name": "pc-i440fx-2.4", @@ -2884,7 +2962,7 @@ "cpu-max": 255 } ], - "id": "libvirt-33" + "id": "libvirt-34" } { @@ -2974,7 +3052,7 @@ "name": "qemu64" } ], - "id": "libvirt-34" + "id": "libvirt-35" } { @@ -2982,21 +3060,21 @@ "enabled": false, "present": true }, - "id": "libvirt-35" + "id": "libvirt-36" } { "return": [ "tpm-tis" ], - "id": "libvirt-36" + "id": "libvirt-37" } { "return": [ "passthrough" ], - "id": "libvirt-37" + "id": "libvirt-38" } { @@ -4000,7 +4078,7 @@ "option": "drive" } ], - "id": "libvirt-38" + "id": "libvirt-39" } { @@ -4030,5 +4108,5 @@ "capability": "events" } ], - "id": "libvirt-39" + "id": "libvirt-40" } diff --git a/tests/qemucapabilitiesdata/caps_2.5.0-1.caps b/tests/qemucapabilitiesdata/caps_2.5.0-1.caps index f4f3673..2a4f5c0 100644 --- a/tests/qemucapabilitiesdata/caps_2.5.0-1.caps +++ b/tests/qemucapabilitiesdata/caps_2.5.0-1.caps @@ -168,4 +168,6 @@ <flag name='virtio-mouse'/> <flag name='virtio-tablet'/> <flag name='virtio-input-host'/> + <flag name='ich9-disable-s3'/> + <flag name='ich9-disable-s4'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_2.5.0-1.replies b/tests/qemucapabilitiesdata/caps_2.5.0-1.replies index d90a74b..fc3f699 100644 --- a/tests/qemucapabilitiesdata/caps_2.5.0-1.replies +++ b/tests/qemucapabilitiesdata/caps_2.5.0-1.replies @@ -2764,6 +2764,84 @@ } { + "return": [ + { + "type": "bool", + "name": "memory-hotplug-support" + }, + { + "type": "uint32", + "name": "rombar" + }, + { + "type": "uint32", + "name": "sci_int" + }, + { + "type": "uint32", + "name": "gpe0_blk_len" + }, + { + "type": "uint32", + "name": "pm_io_base" + }, + { + "type": "bool", + "name": "noreboot" + }, + { + "type": "bool", + "name": "multifunction", + "description": "on/off" + }, + { + "type": "uint8", + "name": "disable_s4" + }, + { + "type": "uint8", + "name": "acpi_disable_cmd" + }, + { + "type": "str", + "name": "romfile" + }, + { + "type": "uint8", + "name": "disable_s3" + }, + { + "type": "uint8", + "name": "s4_val" + }, + { + "type": "uint8", + "name": "acpi_enable_cmd" + }, + { + "type": "bool", + "name": "command_serr_enable", + "description": "on/off" + }, + { + "type": "int32", + "name": "addr", + "description": "Slot and optional function number, example: 06.0 or 06" + }, + { + "type": "bool", + "name": "enable_tco" + }, + { + "type": "uint32", + "name": "gpe0_blk" + } + ], + "id": "libvirt-33" +} + + +{ "return": [ { "name": "pc-i440fx-2.4", @@ -2889,7 +2967,7 @@ "cpu-max": 255 } ], - "id": "libvirt-33" + "id": "libvirt-34" } { @@ -2979,7 +3057,7 @@ "name": "qemu64" } ], - "id": "libvirt-34" + "id": "libvirt-35" } { @@ -2987,21 +3065,21 @@ "enabled": false, "present": true }, - "id": "libvirt-35" + "id": "libvirt-36" } { "return": [ "tpm-tis" ], - "id": "libvirt-36" + "id": "libvirt-37" } { "return": [ "passthrough" ], - "id": "libvirt-37" + "id": "libvirt-38" } { @@ -4005,7 +4083,7 @@ "option": "drive" } ], - "id": "libvirt-38" + "id": "libvirt-39" } { @@ -4035,5 +4113,5 @@ "capability": "events" } ], - "id": "libvirt-39" + "id": "libvirt-40" } diff --git a/tests/qemucapabilitiesdata/caps_2.6.0-1.caps b/tests/qemucapabilitiesdata/caps_2.6.0-1.caps index e296efc..944208e 100644 --- a/tests/qemucapabilitiesdata/caps_2.6.0-1.caps +++ b/tests/qemucapabilitiesdata/caps_2.6.0-1.caps @@ -170,4 +170,6 @@ <flag name='virtio-tablet'/> <flag name='virtio-input-host'/> <flag name='chardev-file-append'/> + <flag name='ich9-disable-s3'/> + <flag name='ich9-disable-s4'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_2.6.0-1.replies b/tests/qemucapabilitiesdata/caps_2.6.0-1.replies index 90a1d61..304f918 100644 --- a/tests/qemucapabilitiesdata/caps_2.6.0-1.replies +++ b/tests/qemucapabilitiesdata/caps_2.6.0-1.replies @@ -2764,6 +2764,84 @@ } { + "return": [ + { + "type": "bool", + "name": "memory-hotplug-support" + }, + { + "type": "uint32", + "name": "rombar" + }, + { + "type": "uint32", + "name": "sci_int" + }, + { + "type": "uint32", + "name": "gpe0_blk_len" + }, + { + "type": "uint32", + "name": "pm_io_base" + }, + { + "type": "bool", + "name": "noreboot" + }, + { + "type": "bool", + "name": "multifunction", + "description": "on/off" + }, + { + "type": "uint8", + "name": "disable_s4" + }, + { + "type": "uint8", + "name": "acpi_disable_cmd" + }, + { + "type": "str", + "name": "romfile" + }, + { + "type": "uint8", + "name": "disable_s3" + }, + { + "type": "uint8", + "name": "s4_val" + }, + { + "type": "uint8", + "name": "acpi_enable_cmd" + }, + { + "type": "bool", + "name": "command_serr_enable", + "description": "on/off" + }, + { + "type": "int32", + "name": "addr", + "description": "Slot and optional function number, example: 06.0 or 06" + }, + { + "type": "bool", + "name": "enable_tco" + }, + { + "type": "uint32", + "name": "gpe0_blk" + } + ], + "id": "libvirt-33" +} + + +{ "return": [ { "name": "pc-i440fx-2.4", @@ -2889,7 +2967,7 @@ "cpu-max": 255 } ], - "id": "libvirt-33" + "id": "libvirt-34" } { @@ -2979,7 +3057,7 @@ "name": "qemu64" } ], - "id": "libvirt-34" + "id": "libvirt-35" } { @@ -2987,21 +3065,21 @@ "enabled": false, "present": true }, - "id": "libvirt-35" + "id": "libvirt-36" } { "return": [ "tpm-tis" ], - "id": "libvirt-36" + "id": "libvirt-37" } { "return": [ "passthrough" ], - "id": "libvirt-37" + "id": "libvirt-38" } { @@ -4009,7 +4087,7 @@ "option": "drive" } ], - "id": "libvirt-38" + "id": "libvirt-39" } { @@ -4039,5 +4117,5 @@ "capability": "events" } ], - "id": "libvirt-39" + "id": "libvirt-40" } -- 2.5.0
On 01/09/2016 04:32 PM, Cole Robinson wrote:
Update test data to match --- src/qemu/qemu_capabilities.c | 9 +++ src/qemu/qemu_capabilities.h | 2 + tests/qemucapabilitiesdata/caps_1.2.2-1.replies | 23 ++++-- tests/qemucapabilitiesdata/caps_1.3.1-1.replies | 22 ++++-- tests/qemucapabilitiesdata/caps_1.4.2-1.replies | 23 ++++-- tests/qemucapabilitiesdata/caps_1.5.3-1.replies | 22 ++++-- tests/qemucapabilitiesdata/caps_1.6.0-1.replies | 22 ++++-- tests/qemucapabilitiesdata/caps_1.6.50-1.replies | 22 ++++-- tests/qemucapabilitiesdata/caps_2.1.1-1.replies | 22 ++++-- tests/qemucapabilitiesdata/caps_2.4.0-1.caps | 2 + tests/qemucapabilitiesdata/caps_2.4.0-1.replies | 92 ++++++++++++++++++++++-- tests/qemucapabilitiesdata/caps_2.5.0-1.caps | 2 + tests/qemucapabilitiesdata/caps_2.5.0-1.replies | 92 ++++++++++++++++++++++-- tests/qemucapabilitiesdata/caps_2.6.0-1.caps | 2 + tests/qemucapabilitiesdata/caps_2.6.0-1.replies | 92 ++++++++++++++++++++++-- 15 files changed, 379 insertions(+), 70 deletions(-)
I forgot to add a comment here... what is the recommended way to regenerate this data? I did it by hand which was not fun - Cole
On Sat, Jan 09, 2016 at 04:35:45PM -0500, Cole Robinson wrote:
On 01/09/2016 04:32 PM, Cole Robinson wrote:
Update test data to match --- src/qemu/qemu_capabilities.c | 9 +++ src/qemu/qemu_capabilities.h | 2 + tests/qemucapabilitiesdata/caps_1.2.2-1.replies | 23 ++++-- tests/qemucapabilitiesdata/caps_1.3.1-1.replies | 22 ++++-- tests/qemucapabilitiesdata/caps_1.4.2-1.replies | 23 ++++-- tests/qemucapabilitiesdata/caps_1.5.3-1.replies | 22 ++++-- tests/qemucapabilitiesdata/caps_1.6.0-1.replies | 22 ++++-- tests/qemucapabilitiesdata/caps_1.6.50-1.replies | 22 ++++-- tests/qemucapabilitiesdata/caps_2.1.1-1.replies | 22 ++++-- tests/qemucapabilitiesdata/caps_2.4.0-1.caps | 2 + tests/qemucapabilitiesdata/caps_2.4.0-1.replies | 92 ++++++++++++++++++++++-- tests/qemucapabilitiesdata/caps_2.5.0-1.caps | 2 + tests/qemucapabilitiesdata/caps_2.5.0-1.replies | 92 ++++++++++++++++++++++-- tests/qemucapabilitiesdata/caps_2.6.0-1.caps | 2 + tests/qemucapabilitiesdata/caps_2.6.0-1.replies | 92 ++++++++++++++++++++++-- 15 files changed, 379 insertions(+), 70 deletions(-)
I forgot to add a comment here... what is the recommended way to regenerate this data? I did it by hand which was not fun
Taking it from debug logs would be the only not-very-annoying way to do that. Also when adding one call by hand, I don't think the IDs need to match in the tests.
- Cole
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
If the q35 specific disable s3/s4 setting isn't disabled, fallback to 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) --- 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; + + if (qemuDomainMachineIsQ35(def) && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_DISABLE_S4)) { + pm_object = "ICH9-LPC"; + } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX_DISABLE_S4)) { + pm_object = "PIIX4_PM"; + } else { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("setting ACPI S4 not supported")); goto error; } + virCommandAddArg(cmd, "-global"); - virCommandAddArgFormat(cmd, "PIIX4_PM.disable_s4=%d", - def->pm.s4 == VIR_TRISTATE_BOOL_NO); + virCommandAddArgFormat(cmd, "%s.disable_s4=%d", + pm_object, def->pm.s4 == VIR_TRISTATE_BOOL_NO); } /* diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35-pm-disable-fallback.args b/tests/qemuxml2argvdata/qemuxml2argv-q35-pm-disable-fallback.args new file mode 100644 index 0000000..bab2b68 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35-pm-disable-fallback.args @@ -0,0 +1,23 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-kvm \ +-name q35 \ +-S \ +-M pc-q35-2.5 \ +-m 1024 \ +-smp 1 \ +-uuid 56f5055c-1b8d-490c-844a-ad646a1caaaa \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/test-monitor,server,nowait \ +-no-acpi \ +-global PIIX4_PM.disable_s3=1 \ +-global PIIX4_PM.disable_s4=1 \ +-boot n \ +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \ +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x1 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35-pm-disable-fallback.xml b/tests/qemuxml2argvdata/qemuxml2argv-q35-pm-disable-fallback.xml new file mode 100644 index 0000000..95177ae --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35-pm-disable-fallback.xml @@ -0,0 +1,18 @@ +<domain type='qemu'> + <name>q35</name> + <uuid>56f5055c-1b8d-490c-844a-ad646a1caaaa</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-q35-2.5'>hvm</type> + <boot dev='network'/> + </os> + <pm> + <suspend-to-mem enabled='no'/> + <suspend-to-disk enabled='no'/> + </pm> + <devices> + <emulator>/usr/bin/qemu-kvm</emulator> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35-pm-disable.args b/tests/qemuxml2argvdata/qemuxml2argv-q35-pm-disable.args new file mode 100644 index 0000000..dbf1119 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35-pm-disable.args @@ -0,0 +1,23 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-kvm \ +-name q35 \ +-S \ +-M pc-q35-2.5 \ +-m 1024 \ +-smp 1 \ +-uuid 56f5055c-1b8d-490c-844a-ad646a1caaaa \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/test-monitor,server,nowait \ +-no-acpi \ +-global ICH9-LPC.disable_s3=1 \ +-global ICH9-LPC.disable_s4=1 \ +-boot n \ +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \ +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x1 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35-pm-disable.xml b/tests/qemuxml2argvdata/qemuxml2argv-q35-pm-disable.xml new file mode 100644 index 0000000..95177ae --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35-pm-disable.xml @@ -0,0 +1,18 @@ +<domain type='qemu'> + <name>q35</name> + <uuid>56f5055c-1b8d-490c-844a-ad646a1caaaa</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-q35-2.5'>hvm</type> + <boot dev='network'/> + </os> + <pm> + <suspend-to-mem enabled='no'/> + <suspend-to-disk enabled='no'/> + </pm> + <devices> + <emulator>/usr/bin/qemu-kvm</emulator> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index b75d453..f361f2a 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1506,6 +1506,15 @@ mymain(void) QEMU_CAPS_ICH9_AHCI, QEMU_CAPS_DEVICE_VIDEO_PRIMARY, QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL); + DO_TEST("q35-pm-disable", + QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, QEMU_CAPS_ICH9_AHCI, + QEMU_CAPS_PIIX_DISABLE_S3, QEMU_CAPS_PIIX_DISABLE_S4, + QEMU_CAPS_ICH9_DISABLE_S3, QEMU_CAPS_ICH9_DISABLE_S4); + DO_TEST("q35-pm-disable-fallback", + QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, QEMU_CAPS_ICH9_AHCI, + QEMU_CAPS_PIIX_DISABLE_S3, QEMU_CAPS_PIIX_DISABLE_S4); DO_TEST("pcie-root-port", QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, -- 2.5.0
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.
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! - Cole
On Sun, Jan 10, 2016 at 03:27:12PM -0500, Cole Robinson wrote:
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!
Of course I meant then checking as well. Anyway, what you pushed is exactly what I wanted to suggest as a second option. Thanks, Martin
On Sat, Jan 09, 2016 at 04:32:19PM -0500, Cole Robinson wrote:
q35/ICH9 uses a different qemu option for disabling s3/s4 support. Probe for it and wire it up.
Cole Robinson (4): qemu: capabilities: s/Pixx/Piix/g qemu: caps: Rename CAPS_DISABLE_S[34] to CAPS_PIIX_DISABLE_S[34] qemu: caps: check for q35/ICH9 disable S3/S4 qemu: command: wire up usage of q35/ich9 disable s3/s4
ACK series with the stuff in 4/4 cleaned up.
participants (2)
-
Cole Robinson -
Martin Kletzander