Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target

[Eduardo Habkost <ehabkost@redhat.com>]

On Wed, May 23, 2018 at 05:53:34PM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Wed, May 23, 2018 at 11:17:55AM +0200, Markus Armbruster wrote:
> >> Eduardo Habkost <ehabkost@redhat.com> writes:
> >> > On Mon, May 21, 2018 at 04:46:36PM -0300, Daniel Henrique Barboza wrote:
> > [...]
> >> >> 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?
> >
> > I think we should.
> 
> Would you like to take care of it?

Adding to my TODO-list, I hope I will be able to do it before the
next release.

> 
[...]
> >
> > This isn't the first time a machine capability that seems static
> > actually depends on other configuration arguments.  We will
> > probably need to address this eventually.
> 
> Then the best time to address it is now, provided we can :)

I'm not sure this is the best time.  If libvirt only needs the
runtime value and don't need any info at query-machines time, I
think support for this on query-machines will be left unused and
they will only use the query-current-machine value.

Just giving libvirt the runtime data it wants
(query-current-machine) seems way better than requiring libvirt
to interpret a set of rules and independently calculate something
QEMU already knows.

> 
> >> 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).
> >> 
> >
> > I would prefer a more generic mechanism.  Maybe make
> > 'query-machines' accept a 'machine-options' argument?
> 
> This can support arbitrary configuration dependencies, unlike my
> proposal.  However, I fear combinatorial explosion would make querying
> anything but "default configuration" and "current configuration"
> impractical, and "default configuration" would be basically useless, as
> you can't predict how arguments will affect the value query-machines.
> 
> Whether this is an issue depends on how management software wants to use
> query-machines.
> 
> Whether the ability to support arbitrary configuration dependencies is a
> useful feature or an invitation to stupid stunts is another open
> question :)
> 
> Here's a synthesis of the two proposals: have query-machines spell out
> which of its results are determinate, and which configuration bits need
> to be supplied to resolve the indeterminate ones.  For machine type
> "pc-q35-*", wakeup-suspend-support would always yield true, but for
> "pc-i440fx-*" it would return true when passed an acpi: true argument,
> false when passed an acpi: false argument, and an encoding of
> "indeterminate, you need to pass an acpi argument to learn more" when
> passed no acpi argument.

I like this proposal for other query-machines fields (like bus
information), but I think doing this for wakeup-suspend-support
is overkill, based on Daniel's description of its intended usage.


> 
> I'm not saying this synthesis makes sense, I'm just exploring the design
> space.
> 
> We need input from libvirt guys.

I have the impression we need real use cases so they can evaluate
the proposal.  wakeup-suspend-support doesn't seem like a use
case that really needs support on query-machines (because we
can simply provide the data at runtime).

-- 
Eduardo