[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target |
Date: |
Wed, 23 May 2018 11:17:55 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux) |
Eduardo Habkost <address@hidden> writes:
> On Mon, May 21, 2018 at 04:46:36PM -0300, Daniel Henrique Barboza wrote:
>>
>>
>> On 05/21/2018 03:14 PM, Eduardo Habkost wrote:
>> > > Issue#2: the flag isn't a property of the target. Due to -no-acpi, it's
>> > > not even a property of the machine type. If it was, query-machines
>> > > would be the natural owner of the flag.
>> > >
>> > > Perhaps query-machines is still the proper owner. The value of
>> > > wakeup-suspend-support would have to depend on -no-acpi for the machine
>> > > types that honor it. Not ideal; I'd prefer MachineInfo to be static.
>> > > Tolerable? I guess that's also a libvirt question.
>> > It depends when libvirt is going to query it. Is it OK to only
>> > query it after the VM is already up and running? If it is, then
>> > we can simply expose it as a read-only property of the machine
>> > object.
>> >
>> > Or, if we don't want to rely on qom-get as a stable API, we can
>> > add a new query command (query-machine? query-power-management?)
>> >
>> In the first version this logic was included in a new query command called
>> "query-wakeup-from-suspend-support":
>>
>> https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg00889.html
>>
>> In that review it was suggested that this logic could be a flag in either
>> query-target
>> or query-machines API. Before sending the v2 I sent the following comment:
>>
>> "After investigating, I think that it's simpler to hook the wakeup support
>> info into
>> TargetInfo than MachineInfo, given that the detection I'm using for this new
>> property
>> is based on the current runtime state. Hooking into MachineInfo would
>> require to
>> change the MachineClass to add a new property, then setting it up for the
>> machines
>> that have the wakeup support (only x86 so far). Definitely doable, but if we
>> don't
>> have any favorites between MachineInfo and TargetInfo I'd rather pick the
>> simpler
>> route.
>>
>> So, if no one objects, I'll rework this series by putting the logic inside
>> query-target
>> instead of a new API."
>
> Apologies for not noticing this series months ago. :(
Seconded. Daniel, this (minor) mess is absolutely not your fault.
>> Since no objection was made back then, this logic was put into query-target
>> starting
>> in v2. Still, I don't have any favorites though: query-target looks ok,
>> query-machine
>> looks ok and a new API looks ok too. It's all about what makes (more) sense
>> in the
>> management level, I think.
>
> I understand the original objection from Eric: having to add a
> new command for every runtime flag we want to expose to the user
> looks wrong to me.
Agreed.
> However, extending query-machines and query-target looks wrong
> too, however. query-target looks wrong because this not a
> property of the target. query-machines is wrong because this is
> not a static property of the machine-type, but of the running
> machine instance.
Of the two, query-machines looks less wrong.
Arguably, -no-acpi should not exist. It's an ad hoc flag that sneakily
splits a few machine types into two variants, with and without ACPI.
It's silently ignored for other machine types, even APCI-capable ones.
If the machine type variants with and without ACPI were separate types,
wakeup-suspend-support would be a static property of the machine type.
However, "separate types" probably doesn't scale: I'm afraid we'd end up
with an undesirable number of machine types. Avoiding that is exactly
why we have machine types with configurable options. I suspect that's
how ACPI should be configured (if at all).
So, should we make -no-acpi sugar for a machine type parameter? And
then deprecate -no-acpi for good measure?
> Can we have a new query command that could be an obvious
> container for simple machine capabilities that are not static? A
> name like "query-machine" would be generic enough for that, I
> guess.
Having command names differ only in a single letter is awkward, but
let's focus on things other than naming now, and use
query-current-machine like a working title.
query-machines is wrong because wakeup-suspend-support isn't static for
some machine types.
query-current-machine is also kind of wrong because
wakeup-suspend-support *is* static for most machine types.
Worse, a machine type property that is static for all machine types now
could conceivably become dynamic when we add a machine type
configuration knob.
Would a way to tie the property to the configuration knob help?
Something like wakeup-suspend-support taking values true (supported),
false (not supported), and "acpi" (supported if machine type
configuration knob "acpi" is switched on).
> Markus, Eric, what do you think?
Haven't made up my mind, yet :)
- [Qemu-devel] [PATCH v7 0/3] wakeup-from-suspend and system_wakeup changes, Daniel Henrique Barboza, 2018/05/17
- [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target, Daniel Henrique Barboza, 2018/05/17
- Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target, Markus Armbruster, 2018/05/18
- Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target, Eduardo Habkost, 2018/05/21
- Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target, Daniel Henrique Barboza, 2018/05/21
- Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target, Eduardo Habkost, 2018/05/21
- Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target,
Markus Armbruster <=
- Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target, Eduardo Habkost, 2018/05/23
- Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target, Daniel Henrique Barboza, 2018/05/23
- Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target, Markus Armbruster, 2018/05/23
- Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target, Eduardo Habkost, 2018/05/24
- Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target, Markus Armbruster, 2018/05/25
- Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target, Eduardo Habkost, 2018/05/25
- Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target, Markus Armbruster, 2018/05/28
- Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target, Eduardo Habkost, 2018/05/29
[Qemu-devel] [PATCH v7 2/3] qga: update guest-suspend-ram and guest-suspend-hybrid descriptions, Daniel Henrique Barboza, 2018/05/17
[Qemu-devel] [PATCH v7 3/3] qmp.c: system_wakeup: runstate and wake-up support check, Daniel Henrique Barboza, 2018/05/17