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

[Roman Bolshakov]

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