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

[Markus Armbruster]

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.

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.

>> > +#
>> > +# 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).

>> > +#
>> > +# 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.

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.

I unde

>>                                       [...]  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.