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:

> 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/.

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

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

   # 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)' }
> +
>  ##
>  # @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?

> +    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.

> +
> +    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.

> +
> +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 */