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

[Roman Bolshakov]

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}}

> 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 :)

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

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.

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 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 :)

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

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.

Thanks,
Roman