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