Re: [PATCH for-6.0 1/6] qapi: Add query-accel command

[Markus Armbruster]

Paolo, there's a question for you further down, marked with your name.

Roman Bolshakov <r.bolshakov@yadro.com> writes:

> On Tue, Nov 17, 2020 at 09:51:58AM +0100, Markus Armbruster wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>> 
>> > On Mon, Nov 16, 2020 at 10:20:04AM -0600, Eric Blake wrote:
>> >> On 11/16/20 7:10 AM, Roman Bolshakov wrote:
>> >> > There's a problem for management applications to determine if certain
>> >> > accelerators available. Generic QMP command should help with that.
>> >> > 
>> >> > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
>> >> > ---
>> >> >  monitor/qmp-cmds.c | 15 +++++++++++++++
>> >> >  qapi/machine.json  | 19 +++++++++++++++++++
>> >> >  2 files changed, 34 insertions(+)
>> >> > 
>> >> 
>> >> > +++ b/qapi/machine.json
>> >> > @@ -591,6 +591,25 @@
>> >> >  ##
>> >> >  { 'command': 'query-kvm', 'returns': 'KvmInfo' }
>> >> >  
>> >> > +##
>> >> > +# @query-accel:
>> >> > +#
>> >> > +# Returns information about an accelerator
>> >> > +#
>> >> > +# Returns: @KvmInfo
>> 
>> Is reusing KvmInfo here is good idea?  Recall:
>> 
>>     ##
>>     # @KvmInfo:
>>     #
>>     # Information about support for KVM acceleration
>>     #
>>     # @enabled: true if KVM acceleration is active
>>     #
>>     # @present: true if KVM acceleration is built into this executable
>>     #
>>     # Since: 0.14.0
>>     ##
>>     { 'struct': 'KvmInfo', 'data': {'enabled': 'bool', 'present': 'bool'} }
>> 
>> I figure @present will always be true with query-accel.  In other words,
>> it's useless noise.
>> 
>
> Hi Markus,
>
> Actually, no. Provided implementation returns present = true only if we
> can find the accel in QOM, i.e. on macOS we get present = false for kvm.
> And on Linux we get present = false for hvf, e.g.:
>
> C: {"execute": "query-accel", "arguments": {"name": "hvf"}}
> S: {"return": {"enabled": true, "present": true}}
> C: {"execute": "query-accel", "arguments": {"name": "kvm"}}
> S: {"return": {"enabled": false, "present": false}}
> C: {"execute": "query-accel", "arguments": {"name": "tcg"}}
> S: {"return": {"enabled": false, "present": true}}

Aha.  I expected query-accel to fail when the named accelerator does not
exist.  Has two advantages:

* Same behavior for "does not exist because it's not compiled in" and
  for "does not exist because it was added only to a future version of
  QEMU'".  Easier for QMP clients: they get to handle one kind of
  failure rather than two.

* If we later change 'name' from 'str' to an enum, behavior stays the
  same.

Compelling enough, don't you think?

>> If we return information on all accelerators, KvmInfo won't do, because
>> it lacks the accelerator name.
>> 
>> If we choose to reuse KvmInfo regardless, it needs to be renamed to
>> something like AccelInfo, and the doc comment generalized beyond KVM.
>> 
>
> I have renamed it to AccelInfo in another patch in the series :)

I noticed only after I sent my reply :)

>> >> > +#
>> >> > +# Since: 6.0.0
>> >> 
>> >> We're inconsistent on whether we have 'Since: x.y' or 'Since: x.y.z',
>> >> although I prefer the shorter form.  Maybe Markus has an opnion on that.
>> 
>> Yes: use the shorter form, unless .z != .0.
>> 
>> The shorter form is much more common:
>> 
>>     $ sed -En 's/.*since:? *([0-9][0-9.]*).*/\1/pi' qapi/*json | sed 
>> 's/[^.]//g' | sort | uniq -c
>>        1065 .
>>         129 ..
>> 
>> .z != 0 should be rare: the stable branch is for bug fixes, and bug
>> fixes rarely require schema changes.  It is: there is just one instance,
>> from commit ab9ba614556 (v3.0.0) and 0779afdc897 (v2.12.1).
>> 
>
> Thanks, I'll use 6.0 then.
>
>> >> > +#
>> >> > +# Example:
>> >> > +#
>> >> > +# -> { "execute": "query-accel", "arguments": { "name": "kvm" } }
>> >> > +# <- { "return": { "enabled": true, "present": true } }
>> >> > +#
>> >> > +##
>> >> > +{ 'command': 'query-accel',
>> >> > +  'data': { 'name': 'str' },
>> >> > +  'returns': 'KvmInfo' }
>> >> 
>> >> '@name' is undocumented and an open-coded string.  Better would be
>> >> requiring 'name' to be one of an enum type.  [...]
>> >
>> > This seem similar to CPU models, machine types, device types, and
>> > backend object types: the set of valid values is derived from the
>> > list of subtypes of a QOM type.  We don't duplicate lists of QOM
>> > types in the QAPI schema, today.
>> 
>> Yes.
>> 
>> > Do we want to duplicate the list of accelerators in the QAPI
>> > schema, or should we wait for a generic solution that works for
>> > any QOM type?
>> 
>> There are only a few accelerators, so duplicating them wouldn't be too
>> bad.  Still, we need a better reason than "because we can".
>> 
>> QAPI enum vs. 'str' doesn't affect the wire format: both use JSON
>> string.
>> 
>
> 'str' is quite flexible. If we remove an accel, provided QOM command
> won't complain. It'll just reply that the accel is not present :)
>
>> With a QAPI enum, the values available in this QEMU build are visible in
>> query-qmp-schema.
>> 
>> Without a QAPI enum, we need a separate, ad hoc query.  For QOM types,
>> we have qom-list-types.
>> 
>> If you're familiar with qom-list-types, you may want to skip to
>> "Conclusion:" now.
>> 
>> Ad hoc queries can take additional arguments.  qom-list-types does:
>> "implements" and "abstract" limit output.  Default is "all
>> non-abstract".
>> 
>> This provides a way to find accelerators: use "implements": "accel" to
>> return only concrete subtypes of "accel":
>> 
>>    ---> {"execute": "qom-list-types", "arguments": {"implements": "accel"}}
>>    <--- {"return": [{"name": "qtest-accel", "parent": "accel"}, {"name": 
>> "tcg-accel", "parent": "accel"}, {"name": "xen-accel", "parent": "accel"}, 
>> {"name": "kvm-accel", "parent": "accel"}, {"name": "accel", "parent": 
>> "object"}]}
>> 
>> Aside: the reply includes "accel", because it's not abstract.  Bug?
>> 
>> Same for CPU models ("implements": "cpu"), machine types ("implements":
>> "machine"), device types ("implements": "device").  Not for backend
>> object types, because they don't have a common base type.  Certain kinds
>> of backend object types do, e.g. character device backends are subtypes
>> of "chardev".
>> 
>> Ad hoc queries can provide additional information.  qom-list-types does:
>> the parent type.
>> 
>> This provides a second way to find subtypes, which might be more
>> efficient when you need to know the subtypes of T for many T:
>> 
>>    Get *all* QOM types in one go:
>> 
>>    ---> {"execute": "qom-list-types", "arguments": {"abstract": false}}
>>    <--- lots of output
>> 
>>    Use output member "parent" to construct the parent tree.  The
>>    sub-tree rooted at QOM type "accel" has the subtypes of "accel".
>> 
>>    Awkward: since output lacks an "abstract" member, we have to
>>    determine it indirectly, by getting just the concrete types:
>> 
>>    ---> {"execute": "qom-list-types", "arguments": {"abstract": true}}
>>    <--- slightly less output
>> 
>>    We can add "abstract" to the output if we care.
>> 
>>    Unlike the first way, this one works *only* for "is subtype of",
>>    *not* for "implements interface".
>> 
>>    We can add interface information to the output if we care.
>> 
>> Likewise, QOM properties are not in the QAPI schema, and we need ad hoc
>> queries instead of query-qmp-schema: qom-list-properties, qom-list,
>> device-list-properties.  Flaws include inadequate type information and
>> difficulties around dynamic properties.
>> 
>> Conclusion: as is, QOM is designed to defeat our general QAPI/QMP
>> introspection mechanism.  It provides its own instead.  Proper
>> integration of QOM and QAPI/QMP would be great.  Integrating small parts
>> in ways that do not get us closer to complete integration does not seem
>> worthwhile.
>> 
>> For some QOM types, we have additional ad hoc queries that provide more
>> information, e.g. query-machines, query-cpu-definitions, and now the
>> proposed query-accel.
>> 
>> query-accel is *only* necessary if its additional information is.
>> 
>
> Thanks for the profound answer!

You're welcome!

> I'm not an expert of QOM/QAPI. Please correct me if my understanding is
> wrong.
>
> 1. We can use QOM capabilities in QMP to get all accelerators:
>
> C: {"execute": "qom-list-types", "arguments": {"implements": "accel"}}
> S: {"return": [{"name": "qtest-accel", "parent": "accel"}, {"name": 
> "tcg-accel", "parent": "accel"}, {"name": "hax-accel", "parent": "accel"}, 
> {"name": "hvf-accel", "parent": "accel"}, {"name": "accel", "parent": 
> "object"}]}
>
> The response answers if an accelerator was compiled with current QEMU
> binary. All accelerators outside of the list aren't present.

Correct.

> 2. We can't figure out if an accel is actually enabled without relying
> on ad-hoc query-accel because there are no means for that in QOM.
> I might be wrong here but I couldn't find a way to list fields of an
> accel class using qom-list and qom-get.

I have to confess I find the code confusing.

The part that counts is do_configure_accelerator().  I works as follows:

1. Look up the chosen accelerator's QOM type (can fail)
2. Instantiate it (can't fail)
3. Set properties (can fail)
4. Connect the accelerator to the current machine (can fail)

Aside: step 3 uses &error_fatal, unlike the other failures.  Fishy.

Failure in step 1 is "accelerator not compiled in".  Predictable with
qom-list-types.

Failure in step 3 doesn't look predictable.

Failure in step 4 can be due to kernel lacking (a workable version of)
KVM, but the current machine gets a say, too.  Also doesn't look
predictable.

Aside: kvm_init() reports errors with fprintf(), tsk, tsk, tsk.

Existing query-kvm doesn't really predict failure, either.  'present' is
true if CONFIG_KVM.  Should be equivalent to "QOM type exists",
i.e. step 1.  I guess 'enabled' is true when the KVM accelerator is in
use.

While figuring this out, I noticed that the TYPE_ACCEL instance we
create doesn't get its parent set.  It's therefore not in the QOM
composition tree, and inaccessible with qom-get.  Paolo, is this as it
should be?

> I have no particular preference of query-accel vs wiring current accel
> to QOM to be able to find it unless the latter one takes x10 time to
> implement and rework everything. But I need some understanding of what
> would be preferred way for everyone :)

If all you want is figuring out which accelerators this particular QEMU
build has, use qom-list-types.

If you have a compelling use case for predicting which accelerators can
actually work, and nobody has clever ideas on how to do that with
existing commands, then an ad hoc query-accel seems the way to go.

[...]
>> >>                                       [...]  Even better would be
>> >> returning an array of KvmInfo with information on all supported
>> >> accelerators at once, rather than making the user call this command once
>> >> per name.
>> >
>> > Maybe.  It would save us the work of answering the question
>> > above, but is this (querying information for all accelerators at
>> > once) going to be a common use case?
>> 
>> I recommend to scratch the query-accel parameter, and return information
>> on all accelerators in this build of QEMU.  Simple, and consistent with
>> similar ad hoc queries.  If a client is interested in just one, fishing
>> it out of the list is easy enough.
>> 
>
> Works for me. I'll then leave KvmInfo as is and will introduce AccelInfo
> with two fields: name and enabled. enabled will be true only for
> currently active accelerator.

Please document that at most on result can have 'enabled': true.