| Message ID | 20201116131011.26607-2-r.bolshakov@yadro.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add HMP/QMP commands to query accelerator | expand |
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 > +# > +# 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. > +# > +# 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. 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.
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 > > +# > > +# 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. > Sure, please let me know which one is better. > > +# > > +# 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. > Thanks for catching that! I'll add documentation for the field. > Better would be requiring 'name' to be one of an enum type. I haven't found any enums available, that's why I used accel_find that looks up accel from string in QOM. > 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. > I considered that, but wasn't sure if it's right or wrong. I'd prefer it over the first option with enums. Likely, we can do that by iterating all concerete accelerators: object_class_get_list(TYPE_ACCEL, false); name parameter can be then dropped and query-accel would be renamed to query-accels. The approach has a drawback - there's no way to return accelerators that aren't compiled, i.e. kvm on macOS or hvf on Linux. I don't know if it's an issue or not. query-accels would only return all available accelerators registered via QOM and one of them would be enabled. I think I'd try to use query-accel in libvirt before proceeding with query-accels. If it'll be apparent that query-accels is superior, then'd go with it. Thanks, Roman
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 > > +# > > +# 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. > > > +# > > +# 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. 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? > [...] 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?
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.
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
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.
On 18/11/20 09:36, Markus Armbruster wrote: > > 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. That's because step 3 is a user error, unlike (in the usual case) the others: 1. You get it from "-accel whpx" if whpx is not available; this is not a user error if there is a fallback. You also get it from misspellings such as "-accel kvmm", which is presumably a user error, but it's hard to distinguish the two especially if a future version of QEMU ends up adding a "kvmm" accelerator 3. You get it from "-accel tcg,misspeled-property=off". This is a user error. You also get it from "-accel tcg,absent-property=on" and "-accel tcg,invalid-value=42". This may be a property that the user wants to set but was only added in a future version of QEMU. Either way, it's quite likely that the user does _not_ want a fallback here. 4. Here the accelerator exists but the machine does not support it. The choice is to fallback to the next accelerato. This means there is no way for the user to distinguish "the accelerator doesn't exist" from "the accelerator exists but is not supported". This was done for no particular reason other than to keep the code simple. > Failure in step 1 is "accelerator not compiled in". Predictable with > qom-list-types. > > Failure in step 3 doesn't look predictable. It's more or less predictable with qom-list-properties, though of course not the "invalid value for the property" case. > 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? It should be added, I agree, perhaps as /machine/accel. Paolo
Am 18.11.2020 um 09:36 hat Markus Armbruster geschrieben: > >> >> [...] 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. This doesn't feel right. If I understand right, the proposal is to get a result like this: [ { 'name': 'kvm', 'enabled': true }, { 'name': 'tcg', 'enabled': false }, { 'name': 'xen', 'enabled': false } ] The condition that only one field can be enabled would only be in the documentation instead of being enforced. Instead, consider a response like this: { 'available': [ 'kvm', 'tcg', 'xen' ], 'active': 'kvm' } This is not only shorter, but also makes sure that only one accelerator can be reported as active. Kevin
On Wed, Nov 18, 2020 at 12:28:45PM +0100, Kevin Wolf wrote: > Am 18.11.2020 um 09:36 hat Markus Armbruster geschrieben: > > >> >> [...] 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. > > This doesn't feel right. > > If I understand right, the proposal is to get a result like this: > > [ { 'name': 'kvm', 'enabled': true }, > { 'name': 'tcg', 'enabled': false }, > { 'name': 'xen', 'enabled': false } ] > > The condition that only one field can be enabled would only be in the > documentation instead of being enforced. > > Instead, consider a response like this: > > { 'available': [ 'kvm', 'tcg', 'xen' ], > 'active': 'kvm' } > > This is not only shorter, but also makes sure that only one accelerator > can be reported as active. I guess this can be extended with a union to report extra props for the accelerator, discriminated on the 'active' field eg { 'available': [ 'kvm', 'tcg', 'xen' ], 'active': 'kvm', 'data': { "allow-nested": true, } } Regards, Daniel
Paolo Bonzini <pbonzini@redhat.com> writes: > On 18/11/20 09:36, Markus Armbruster wrote: >> >> 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. > > That's because step 3 is a user error, unlike (in the usual case) the > others: > > 1. You get it from "-accel whpx" if whpx is not available; this is not a > user error if there is a fallback. You also get it from misspellings > such as "-accel kvmm", which is presumably a user error, but it's hard > to distinguish the two especially if a future version of QEMU ends up > adding a "kvmm" accelerator > > 3. You get it from "-accel tcg,misspeled-property=off". This is a user > error. You also get it from "-accel tcg,absent-property=on" and "-accel > tcg,invalid-value=42". This may be a property that the user wants to > set but was only added in a future version of QEMU. Either way, it's > quite likely that the user does _not_ want a fallback here. > > 4. Here the accelerator exists but the machine does not support it. The > choice is to fallback to the next accelerato. > > This means there is no way for the user to distinguish "the accelerator > doesn't exist" from "the accelerator exists but is not supported". This > was done for no particular reason other than to keep the code simple. The resulting error reporting is perhaps not as clear as it could be. We report several kinds of errors: (a) Accelerator misconfiguration, immediately fatal (b) Accelerator doesn't work, not fatal (we fall back to the next one) (c) "no accelerator found", fatal (d) "falling back to", not fatal (we carry on) Reporting (b) as error is questionable, because it need not be an actual error. We report (d) when an accelerator works after other(s) didn't. This is not actually an error. Example: $ qemu-system-x86_64 -accel xen -S -accel nonexistent -accel kvm xencall: error: Could not obtain handle on privileged command interface: No such file or directory xen be core: xen be core: can't open xen interface can't open xen interface So far, this is just libxen* and accel/xen/xen-all.c being loquacious. qemu-system-x86_64: -accel xen: failed to initialize xen: Operation not permitted qemu-system-x86_64: -accel nonexistent: invalid accelerator nonexistent qemu-system-x86_64: falling back to KVM These look like errors, but aren't; things are working exactly as intended, and QEMU runs. If we want to be chatty about it, we should make them info, not error. Note that a nonsensical accelerator is treated just like an accelerator that exists, but happens to be unavailable. Trap for less than perfect typists. Same with /dev/kvm made inaccessible: $ qemu-system-x86_64 -accel xen -S -accel nonexistent -accel kvm [Xen chatter...] qemu-system-x86_64: -accel xen: failed to initialize xen: Operation not permitted qemu-system-x86_64: -accel nonexistent: invalid accelerator nonexistent Could not access KVM kernel module: Permission denied qemu-system-x86_64: -accel kvm: failed to initialize kvm: Permission denied Here, we do have a fatal error. We report it as four errors. Tolerable. If we turn the maybe-not-really-errors into info to make the first example less confusing, we need to report a "no accelerator works" error at the end. >> Failure in step 1 is "accelerator not compiled in". Predictable with >> qom-list-types. >> >> Failure in step 3 doesn't look predictable. > > It's more or less predictable with qom-list-properties, though of course > not the "invalid value for the property" case. > >> 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? > > It should be added, I agree, perhaps as /machine/accel. Makes sense. Thanks!
On 18/11/20 14:08, Markus Armbruster wrote: > These look like errors, but aren't; things are working exactly as > intended, and QEMU runs. If we want to be chatty about it, we should > make them info, not error. If there were an info_report, I would have sent a patch already. :) In general, these are probably not the only cases where error_report is used as a fancy fprintf(stderr), rather than to report actual errors. Paolo > Same with /dev/kvm made inaccessible: > > $ qemu-system-x86_64 -accel xen -S -accel nonexistent -accel kvm > [Xen chatter...] > qemu-system-x86_64: -accel xen: failed to initialize xen: Operation not permitted > qemu-system-x86_64: -accel nonexistent: invalid accelerator nonexistent > Could not access KVM kernel module: Permission denied > qemu-system-x86_64: -accel kvm: failed to initialize kvm: Permission denied > > Here, we do have a fatal error. We report it as four errors. > Tolerable. > > If we turn the maybe-not-really-errors into info to make the first > example less confusing, we need to report a "no accelerator works" error > at the end.
Daniel P. Berrangé <berrange@redhat.com> writes: > On Wed, Nov 18, 2020 at 12:28:45PM +0100, Kevin Wolf wrote: >> Am 18.11.2020 um 09:36 hat Markus Armbruster geschrieben: >> > >> >> [...] 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. >> >> This doesn't feel right. >> >> If I understand right, the proposal is to get a result like this: >> >> [ { 'name': 'kvm', 'enabled': true }, >> { 'name': 'tcg', 'enabled': false }, >> { 'name': 'xen', 'enabled': false } ] >> >> The condition that only one field can be enabled would only be in the >> documentation instead of being enforced. >> >> Instead, consider a response like this: >> >> { 'available': [ 'kvm', 'tcg', 'xen' ], >> 'active': 'kvm' } >> >> This is not only shorter, but also makes sure that only one accelerator >> can be reported as active. Better. Documentation only, not enforced: no duplicate array elements. We got that elsewhere already (arrays used as sets, basically). If we want to provide for reporting additional information on available accelerators, tweak it to {"available": [{"name": "kvm"}, {"name": "tcg"}, {"name": "xen"}], "active": "kvm"} If accelerator-specific extra information is needed, the array elements can compatibly evolve into flat unions. Another way to skin this cat: {"available": {"kvm": { extra properties... }, "tcg": ..., "xen": ...}, "active": "kvm"} No need for unions then. "No dupes" is enforced. We could inline "available": {"kvm": { extra properties... }, "tcg": ..., "xen": ..., "active": "kvm"} Future accelerators can't be named "active" then. > I guess this can be extended with a union to report extra props for the > accelerator, discriminated on the 'active' field eg > > { 'available': [ 'kvm', 'tcg', 'xen' ], > 'active': 'kvm', > 'data': { > "allow-nested": true, > } > } Correct.
On Wed, Nov 18, 2020 at 02:08:21PM +0100, Markus Armbruster wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > > On 18/11/20 09:36, Markus Armbruster wrote: > >> 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? > > > > It should be added, I agree, perhaps as /machine/accel. > > Makes sense. > I also tried to find it there when traversed QOM :) So, I'll then add it instead of ad-hoc queries in v2? That'd allow us to use standard QOM queries to determine all built-in accelerators and query actually enabled one. Thanks, Roman
Paolo Bonzini <pbonzini@redhat.com> writes: > On 18/11/20 14:08, Markus Armbruster wrote: >> These look like errors, but aren't; things are working exactly as >> intended, and QEMU runs. If we want to be chatty about it, we should >> make them info, not error. > > If there were an info_report, I would have sent a patch already. :) Commit 97f40301f1 "error: Functions to report warnings and informational messages", 2017-07-13 :) > In general, these are probably not the only cases where error_report > is used as a fancy fprintf(stderr), rather than to report actual > errors. True!
On 18/11/20 15:45, Markus Armbruster wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> On 18/11/20 14:08, Markus Armbruster wrote: >>> These look like errors, but aren't; things are working exactly as >>> intended, and QEMU runs. If we want to be chatty about it, we should >>> make them info, not error. >> >> If there were an info_report, I would have sent a patch already. :) > > Commit 97f40301f1 "error: Functions to report warnings and informational > messages", 2017-07-13 :) Doh, I just learnt about info_report. It never occurred to me until now that without a warning or info marker it would be an error. I can see though why you didn't add "error" automatically for REPORT_TYPE_ERROR, while leaving REPORT_TYPE_INFO unadorned. Between the incorrectly-marked errors and probably some "error: error: " it would be awful. Paolo >> In general, these are probably not the only cases where error_report >> is used as a fancy fprintf(stderr), rather than to report actual >> errors. > > True! >
On Wed, Nov 18, 2020 at 02:53:26PM +0100, Markus Armbruster wrote: [...] > Another way to skin this cat: > > {"available": {"kvm": { extra properties... }, > "tcg": ..., > "xen": ...}, > "active": "kvm"} How would this structure be represented in the QAPI schema? In other words, how do I say "Dict[str, AccelInfo]" in QAPIese? > > No need for unions then. "No dupes" is enforced. > > We could inline "available": > > {"kvm": { extra properties... }, > "tcg": ..., > "xen": ..., > "active": "kvm"} > > Future accelerators can't be named "active" then. > > > I guess this can be extended with a union to report extra props for the > > accelerator, discriminated on the 'active' field eg > > > > { 'available': [ 'kvm', 'tcg', 'xen' ], > > 'active': 'kvm', > > 'data': { > > "allow-nested": true, > > } > > } > > Correct.
On 11/18/20 9:45 AM, Eduardo Habkost wrote: > On Wed, Nov 18, 2020 at 02:53:26PM +0100, Markus Armbruster wrote: > [...] >> Another way to skin this cat: >> >> {"available": {"kvm": { extra properties... }, >> "tcg": ..., >> "xen": ...}, >> "active": "kvm"} > > How would this structure be represented in the QAPI schema? > > In other words, how do I say "Dict[str, AccelInfo]" in QAPIese? {'type':'AvailAccel', 'data': { '*kvm': 'KvmExtraProps', '*tcg': 'TcgExtraProps', '*xen': 'XenExtraProps', '*hax': 'HaxExtraProps' } } {'command':'query-accel', 'returns': { 'available': 'AvailAccel', 'active': 'strOrEnum' } } where adding a new accelerator then adds a new optional member to AvailAccel as well as possibly a new enum member if 'active' is driving by an enum instead of 'str'.
On Wed, Nov 18, 2020 at 09:56:28AM -0600, Eric Blake wrote: > On 11/18/20 9:45 AM, Eduardo Habkost wrote: > > On Wed, Nov 18, 2020 at 02:53:26PM +0100, Markus Armbruster wrote: > > [...] > >> Another way to skin this cat: > >> > >> {"available": {"kvm": { extra properties... }, > >> "tcg": ..., > >> "xen": ...}, > >> "active": "kvm"} > > > > How would this structure be represented in the QAPI schema? > > > > In other words, how do I say "Dict[str, AccelInfo]" in QAPIese? > > {'type':'AvailAccel', 'data': { > '*kvm': 'KvmExtraProps', > '*tcg': 'TcgExtraProps', > '*xen': 'XenExtraProps', > '*hax': 'HaxExtraProps' } } > {'command':'query-accel', 'returns': { > 'available': 'AvailAccel', 'active': 'strOrEnum' } } > > where adding a new accelerator then adds a new optional member to > AvailAccel as well as possibly a new enum member if 'active' is driving > by an enum instead of 'str'. Is it possible to represent this if we don't enumerate all possible dictionary keys in advance? (I'm not suggesting we should/shouldn't do that, just wondering if it's possible)
Eduardo Habkost <ehabkost@redhat.com> writes: > On Wed, Nov 18, 2020 at 09:56:28AM -0600, Eric Blake wrote: >> On 11/18/20 9:45 AM, Eduardo Habkost wrote: >> > On Wed, Nov 18, 2020 at 02:53:26PM +0100, Markus Armbruster wrote: >> > [...] >> >> Another way to skin this cat: >> >> >> >> {"available": {"kvm": { extra properties... }, >> >> "tcg": ..., >> >> "xen": ...}, >> >> "active": "kvm"} >> > >> > How would this structure be represented in the QAPI schema? >> > >> > In other words, how do I say "Dict[str, AccelInfo]" in QAPIese? >> >> {'type':'AvailAccel', 'data': { >> '*kvm': 'KvmExtraProps', >> '*tcg': 'TcgExtraProps', >> '*xen': 'XenExtraProps', >> '*hax': 'HaxExtraProps' } } >> {'command':'query-accel', 'returns': { >> 'available': 'AvailAccel', 'active': 'strOrEnum' } } >> >> where adding a new accelerator then adds a new optional member to >> AvailAccel as well as possibly a new enum member if 'active' is driving >> by an enum instead of 'str'. > > Is it possible to represent this if we don't enumerate all > possible dictionary keys in advance? (I'm not suggesting we > should/shouldn't do that, just wondering if it's possible) Mostly no. The definition of a complex type (struct or union) specifies all members. There is no way to say "and whatever else may be there". We actually have such types anyway. Consider command device_add: it takes arguments 'driver', 'bus', 'str', and properties. Its arguments type is "struct of driver, bus, str, and whatever else may be there". Since the schema language can't express this, we cheat: { 'command': 'device_add', 'data': {'driver': 'str', '*bus': 'str', '*id': 'str'}, 'gen': false } # so we can get the additional arguments With 'gen': false, 'data' is at best a statement of intent. In this case, it's correct, just incomplete[*]. Introspection takes 'data' at face value. It's exactly as accurate as 'data' is. We could extend the schema language so we can say { 'command': 'device_add', 'data': {'driver': 'str', '*bus': 'str', '*id': 'str', '**props': 'dict'} where 'props' receives any remaining arguments. Fairly common language feature, e.g. &rest in Lisp, ** in Python, ... Removed the need for 'gen': false, and enables more accurate introspection. Type 'dict' doesn't exist, yet. I think it could. We got 'any' already. [*] There have been uses of 'gen': false where 'data' was actually wrong. For an example, see commit b8a98326d5 "qapi-schema: Fix up misleading specification of netdev_add".
On 11/16/20 2:10 PM, 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(+) ... > +## > +# @query-accel: > +# > +# Returns information about an accelerator > +# > +# Returns: @KvmInfo > +# > +# Since: 6.0.0 > +# > +# Example: > +# > +# -> { "execute": "query-accel", "arguments": { "name": "kvm" } } > +# <- { "return": { "enabled": true, "present": true } } FWIW you can use 'qom-list-types' for that: { "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" } ] } Which is what I use for integration tests: https://www.mail-archive.com/qemu-devel@nongnu.org/msg675079.html https://www.mail-archive.com/qemu-devel@nongnu.org/msg675085.html
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c index a08143b323..0454394e76 100644 --- a/monitor/qmp-cmds.c +++ b/monitor/qmp-cmds.c @@ -62,6 +62,21 @@ KvmInfo *qmp_query_kvm(Error **errp) return info; } +KvmInfo *qmp_query_accel(const char *name, Error **errp) +{ + KvmInfo *info = g_malloc0(sizeof(*info)); + + AccelClass *ac = accel_find(name); + + if (ac) { + info->enabled = *ac->allowed; + info->present = true; + } + + return info; +} + + UuidInfo *qmp_query_uuid(Error **errp) { UuidInfo *info = g_malloc0(sizeof(*info)); diff --git a/qapi/machine.json b/qapi/machine.json index 7c9a263778..11f364fab4 100644 --- a/qapi/machine.json +++ b/qapi/machine.json @@ -591,6 +591,25 @@ ## { 'command': 'query-kvm', 'returns': 'KvmInfo' } +## +# @query-accel: +# +# Returns information about an accelerator +# +# Returns: @KvmInfo +# +# Since: 6.0.0 +# +# Example: +# +# -> { "execute": "query-accel", "arguments": { "name": "kvm" } } +# <- { "return": { "enabled": true, "present": true } } +# +## +{ 'command': 'query-accel', + 'data': { 'name': 'str' }, + 'returns': 'KvmInfo' } + ## # @NumaOptionsType: #
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(+)