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

[Martin Kletzander]

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
-------------- 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/20160111/aa6470a1/attachment-0001.sig>