Re: [Qemu-devel] [PATCH 08/13] target/arm/monitor: Add query-sve-vector-lengths

[Markus Armbruster <armbru@redhat.com>]

Andrew Jones <drjones@redhat.com> writes:

> On Mon, May 13, 2019 at 06:12:38PM +0200, Markus Armbruster wrote:
>> Andrew Jones <drjones@redhat.com> writes:
>> 
>> > Provide a QMP interface to query the supported SVE vector lengths.
>> > A migratable guest will need to explicitly specify a valid set of
>> > lengths on the command line and that set can be obtained from the
>> > list returned with this QMP command.
>> >
>> > This patch only introduces the QMP command with the TCG implementation.
>> > The result may not yet be correct for KVM. Following patches ensure
>> > the KVM result is correct.
>> >
>> > Signed-off-by: Andrew Jones <drjones@redhat.com>
>> > ---
>> >  qapi/target.json     | 34 ++++++++++++++++++++++++
>> >  target/arm/monitor.c | 62 ++++++++++++++++++++++++++++++++++++++++++++
>> >  tests/qmp-cmd-test.c |  1 +
>> >  3 files changed, 97 insertions(+)
>> >
>> > diff --git a/qapi/target.json b/qapi/target.json
>> > index 1d4d54b6002e..ca1e85254780 100644
>> > --- a/qapi/target.json
>> > +++ b/qapi/target.json
>> > @@ -397,6 +397,40 @@
>> >  { 'command': 'query-gic-capabilities', 'returns': ['GICCapability'],
>> >    'if': 'defined(TARGET_ARM)' }
>> >  
>> > +##
>> > +# @SVEVectorLengths:
>> > +#
>> > +# The struct contains a list of integers where each integer is a valid
>> 
>> Suggest to s/The struct contains/Contains/.
>
> OK
>
>> 
>> > +# SVE vector length for a KVM guest on this host. The vector lengths
>> > +# are in quadword (128-bit) units, e.g. '4' means 512 bits (64 bytes).
>> 
>> Any particular reason for counting quad-words instead of bytes, or
>> perhaps bits?
>
> It can be considered just bits here, but when set in sve-vls-map those
> bits are chosen to mean quadwords as that allows us to get up to 8192-bit
> vectors with a single 64-bit word. Maybe I should write more of that here
> to clarify.

Please do.

In general, QMP prefers the plain units over arbitrary multiples,
e.g. Byte, not Mebibyte.  Sadly, many violations of this rule have crept
in.

More complete documentation should help us determine the unit we want
here.

>> > +#
>> > +# @vls:      list of vector lengths in quadwords.
>> > +#
>> > +# Since: 4.1
>> > +##
>> > +{ 'struct': 'SVEVectorLengths',
>> > +  'data': { 'vls': ['int'] },
>> > +  'if': 'defined(TARGET_ARM)' }
>> > +
>> > +##
>> > +# @query-sve-vector-lengths:
>> > +#
>> > +# This command is ARM-only. It will return a list of SVEVectorLengths
>> 
>> No other target-specific command documents its target-specificness like
>> this.  Suggest
>
> Well, it's pretty similar to query-gic-capabilities, which is what I used
> as a template, but I'm happy to change it to whatever you suggest :)

Here's the documentation we generate for query-gic-capabilities:

 -- Command: query-gic-capabilities

     This command is ARM-only.  It will return a list of GICCapability
     objects that describe its capability bits.

     Returns: a list of GICCapability objects.

     Since: 2.6

     Example:
          -> { "execute": "query-gic-capabilities" }
          <- { "return": [{ "version": 2, "emulated": true, "kernel": false },
                          { "version": 3, "emulated": false, "kernel": true } ] }

     If: 'defined(TARGET_ARM)'

The "If:" line is generated from the schema's condition.  It's not as
readable as I'd like it to be, but it's there, and it always matches the
code.

"This command is ARM-only" is redundant.  Escaped review.  A patch to
drop it would be welcome.

>>    # Query valid SVE vector length sets.
>> 
>> > +# objects. The list describes all valid SVE vector length sets.
>> > +#
>> > +# Returns: a list of SVEVectorLengths objects
>> > +#
>> > +# Since: 4.1
>> > +#
>> > +# -> { "execute": "query-sve-vector-lengths" }
>> > +# <- { "return": [ { "vls": [ 1 ] },
>> > +#                  { "vls": [ 1, 2 ] },
>> > +#                  { "vls": [ 1, 2, 4 ] } ] }
>> > +#
>> > +##
>> > +{ 'command': 'query-sve-vector-lengths', 'returns': ['SVEVectorLengths'],
>> > +  'if': 'defined(TARGET_ARM)' }
>> > +
>
> Yup, will do

Took me a few seconds to connect this to my "# Query valid SVE vector
length sets." line %-}

>> >  ##
>> >  # @CpuModelExpansionInfo:
>> >  #
[...]