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

[Andrew Jones]

On Mon, May 13, 2019 at 06:12:38PM +0200, Markus Armbruster wrote:
> Andrew Jones <address@hidden> 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 <address@hidden>
> > ---
> >  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.

> 
> > +#
> > +# @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 :)

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

> >  ##
> >  # @CpuModelExpansionInfo:
> >  #
> > diff --git a/target/arm/monitor.c b/target/arm/monitor.c
> > index 41b32b94b258..8b2afa255c92 100644
> > --- a/target/arm/monitor.c
> > +++ b/target/arm/monitor.c
> > @@ -24,6 +24,7 @@
> >  #include "hw/boards.h"
> >  #include "kvm_arm.h"
> >  #include "qapi/qapi-commands-target.h"
> > +#include "monitor/hmp-target.h"
> 
> Uh, hmp-target.h when the patch is supposedly about QMP only...
> 
> >  
> >  static GICCapability *gic_cap_new(int version)
> >  {
> > @@ -82,3 +83,64 @@ GICCapabilityList *qmp_query_gic_capabilities(Error 
> > **errp)
> >  
> >      return head;
> >  }
> > +
> > +static SVEVectorLengths *qmp_sve_vls_get(void)
> > +{
> > +    CPUArchState *env = mon_get_cpu_env();
> 
> Aha, you need it for mon_get_cpu_env().
> 
> mon_get_cpu_env() returns the current monitor's current CPU.  This is an
> HMP thing, QMP commands should never access it.
> 
> Looks like you use it to find one of the CPUs, so you can access its
> ->sve_max_vq.
> 
> "One of the CPUs" smells odd: what if they aren't all the same?  Perhaps
> that can't happen.  I don't know, you tell me :)
> 
> If any CPU will do, what about simply using first_cpu?

first_cpu will work. We currently only allow the same vector length set
for all cpus. I'll change it and drop the HMP things.

> 
> > +    ARMCPU *cpu = arm_env_get_cpu(env);
> > +    SVEVectorLengths *vls = g_new(SVEVectorLengths, 1);
> > +    intList **v = &vls->vls;
> > +    int i;
> > +
> > +    if (cpu->sve_max_vq == 0) {
> > +        *v = g_new0(intList, 1); /* one vl of 0 means none supported */
> > +        return vls;
> > +    }
> > +
> > +    for (i = 1; i <= cpu->sve_max_vq; ++i) {
> > +        *v = g_new0(intList, 1);
> > +        (*v)->value = i;
> > +        v = &(*v)->next;
> > +    }
> 
> What this loop does is not immediately obvious.  I think you could use a
> function comment.

OK

> 
> > +
> > +    return vls;
> > +}
> > +
> > +static SVEVectorLengths *qmp_sve_vls_dup_and_truncate(SVEVectorLengths 
> > *vls)
> > +{
> > +    SVEVectorLengths *trunc_vls;
> > +    intList **v, *p = vls->vls;
> > +
> > +    if (!p->next) {
> > +        return NULL;
> > +    }
> > +
> > +    trunc_vls = g_new(SVEVectorLengths, 1);
> > +    v = &trunc_vls->vls;
> > +
> > +    for (; p->next; p = p->next) {
> > +        *v = g_new0(intList, 1);
> > +        (*v)->value = p->value;
> > +        v = &(*v)->next;
> > +    }
> > +
> > +    return trunc_vls;
> > +}
> 
> More so.

More OK :)

> 
> > +
> > +SVEVectorLengthsList *qmp_query_sve_vector_lengths(Error **errp)
> > +{
> > +    SVEVectorLengthsList *vls_list = g_new0(SVEVectorLengthsList, 1);
> > +    SVEVectorLengths *vls = qmp_sve_vls_get();
> > +
> > +    while (vls) {
> > +        vls_list->value = vls;
> > +        vls = qmp_sve_vls_dup_and_truncate(vls);
> > +        if (vls) {
> > +            SVEVectorLengthsList *next = vls_list;
> > +            vls_list = g_new0(SVEVectorLengthsList, 1);
> > +            vls_list->next = next;
> > +        }
> > +    }
> > +
> > +    return vls_list;
> > +}
> > diff --git a/tests/qmp-cmd-test.c b/tests/qmp-cmd-test.c
> > index 9f5228cd9951..3d714dbc6a4a 100644
> > --- a/tests/qmp-cmd-test.c
> > +++ b/tests/qmp-cmd-test.c
> > @@ -90,6 +90,7 @@ static bool query_is_blacklisted(const char *cmd)
> >          /* Success depends on target arch: */
> >          "query-cpu-definitions",  /* arm, i386, ppc, s390x */
> >          "query-gic-capabilities", /* arm */
> > +        "query-sve-vector-lengths", /* arm */
> >          /* Success depends on target-specific build configuration: */
> >          "query-pci",              /* CONFIG_PCI */
> >          /* Success depends on launching SEV guest */

Thanks,
drew