Message ID | 20190512083624.8916-9-drjones@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | target/arm/kvm: enable SVE in guests | expand |
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 */
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. > > > +# > > +# @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
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: >> > # [...]
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 +# 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). +# +# @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 +# 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" 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(); + 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; + } + + 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; +} + +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 */
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(+)