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