Paolo, there's a question for you further down, marked with your name.
Roman Bolshakov <r.bolsha...@yadro.com> writes:
> On Tue, Nov 17, 2020 at 09:51:58AM +0100, Markus Armbruster wrote:
>> Eduardo Habkost <ehabk...@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.bolsha...@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.
