[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 13/14] target/arm/cpu64: max cpu: Support sve
From: |
Andrew Jones |
Subject: |
Re: [Qemu-devel] [PATCH v2 13/14] target/arm/cpu64: max cpu: Support sve properties with KVM |
Date: |
Wed, 17 Jul 2019 10:41:57 +0200 |
User-agent: |
NeoMutt/20180716 |
On Fri, Jun 28, 2019 at 05:55:50PM +0200, Auger Eric wrote:
> Hi Drew,
>
> On 6/21/19 6:34 PM, Andrew Jones wrote:
> > Extend the SVE vq map initialization and validation with KVM's
> > supported vector lengths when KVM is enabled. In order to determine
> > and select supported lengths we add two new KVM functions for getting
> > and setting the KVM_REG_ARM64_SVE_VLS pseudo-register.
> >
> > Signed-off-by: Andrew Jones <address@hidden>
> > ---
> > target/arm/cpu.h | 3 +-
> > target/arm/cpu64.c | 171 +++++++++++++++++++++++++++------------
> > target/arm/kvm64.c | 117 +++++++++++++++++++++++++--
> > target/arm/kvm_arm.h | 19 +++++
> > target/arm/monitor.c | 2 +-
> > tests/arm-cpu-features.c | 86 +++++++++++++++++---
> > 6 files changed, 331 insertions(+), 67 deletions(-)
> >
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index cbb155cf72a5..8a1c6c66a462 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -926,7 +926,8 @@ struct ARMCPU {
> > * (bit-number + 1) * 16 bytes, i.e. each bit number + 1 is the vector
> > * length in quadwords. We need a map size twice the maximum
> > * quadword length though because we use two bits for each vector
> > - * length in order to track three states: uninitialized, off, and on.
> > + * length in order to track four states: uninitialized, uninitialized
> > + * but supported by KVM, off, and on.
> > */
> > DECLARE_BITMAP(sve_vq_map, ARM_MAX_VQ * 2);
> > };
> > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> > index 2e595ad53137..6e92aa54b9c8 100644
> > --- a/target/arm/cpu64.c
> > +++ b/target/arm/cpu64.c
> > @@ -261,10 +261,11 @@ static void aarch64_a72_initfn(Object *obj)
> > * While we eventually use cpu->sve_vq_map as a typical bitmap, where each
> > vq
> > * has only two states (off/on), until we've finalized the map at realize
> > time
> > * we use an extra bit, at the vq - 1 + ARM_MAX_VQ bit number, to also
> > allow
> > - * tracking of the uninitialized state. The arm_vq_state typedef and
> > following
> > - * functions allow us to more easily work with the bitmap. Also, while the
> > map
> > - * is still initializing, sve-max-vq has an additional three states,
> > bringing
> > - * the number of its states to five, which are the following:
> > + * tracking of the uninitialized state and the uninitialized but supported
> > by
> > + * KVM state. The arm_vq_state typedef and following functions allow us to
> > more
> > + * easily work with the bitmap. Also, while the map is still initializing,
> > + * sve-max-vq has an additional three states, bringing the number of its
> > states
> > + * to five, which are the following:
> > *
> > * sve-max-vq:
> > * 0: SVE is disabled. The default value for a vq in the map is 'OFF'.
> > @@ -296,6 +297,11 @@ typedef enum arm_vq_state {
> > ARM_VQ_OFF,
> > ARM_VQ_ON,
> > ARM_VQ_UNINITIALIZED,
> > + ARM_VQ_UNINITIALIZED_KVM_SUPPORTED
> > + /*
> > + * More states cannot be added without adding bits to sve_vq_map
> > + * and modifying its supporting functions.
> > + */
> > } arm_vq_state;
> >
> > static arm_vq_state arm_cpu_vq_map_get(ARMCPU *cpu, int vq)
> > @@ -324,6 +330,23 @@ static void arm_cpu_vq_map_init(ARMCPU *cpu)
> > {
> > bitmap_zero(cpu->sve_vq_map, ARM_MAX_VQ * 2);
> > bitmap_set(cpu->sve_vq_map, ARM_MAX_VQ, ARM_MAX_VQ);
> > +
> > + if (kvm_enabled()) {
> > + DECLARE_BITMAP(kvm_supported, ARM_MAX_VQ);
> > + uint32_t kvm_max_vq;
> > +
> > + bitmap_zero(kvm_supported, ARM_MAX_VQ);
> > +
> > + kvm_arm_sve_get_vls(CPU(cpu), kvm_supported, ARM_MAX_VQ,
> > &kvm_max_vq);
> > +
> > + if (kvm_max_vq > ARM_MAX_VQ) {
> > + warn_report("KVM supports vector lengths larger than "
> > + "QEMU can enable");
> > + }
> > +
> > + bitmap_or(cpu->sve_vq_map, cpu->sve_vq_map,
> > + kvm_supported, ARM_MAX_VQ);
> So you store the KVM supported state in the 1st half of the bitmap
> (ON/OFF)? That's confusing actually. And the second half of the bitmap
> still is used to encode whether the state is UNITIALIZED? As Richard
> suggested would be clearer with separate bitmaps including for the KVM
> supported state.
You can think of it as two states, which is indeed confusing, or you can
think of it as one state: 3, where the two bits that compose '3' come from
two different parts of the bitmap, rather than from contiguous bits.
That's admittedly a bit complex too, but that's why I introduced helper
functions and a typedef. Perhaps a comment here explaining that this
bitmap_or() creates ARM_VQ_UNINITIALIZED_KVM_SUPPORTED entries would help?
> > + }
> > }
> >
> > static bool arm_cpu_vq_map_is_finalized(ARMCPU *cpu)
> > @@ -371,12 +394,7 @@ void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp)
> > return;
> > }
> >
> > - /* sve-max-vq and sve<vl-bits> properties not yet implemented for KVM
> > */
> > - if (kvm_enabled()) {
> > - return;
> > - }
> > -
> > - if (cpu->sve_max_vq == ARM_SVE_INIT) {
> > + if (!kvm_enabled() && cpu->sve_max_vq == ARM_SVE_INIT) {
> > object_property_set_uint(OBJECT(cpu), ARM_MAX_VQ, "sve-max-vq",
> > &err);
> > if (err) {
> > error_propagate(errp, err);
> > @@ -431,6 +449,11 @@ static void cpu_max_set_sve_max_vq(Object *obj,
> > Visitor *v, const char *name,
> > return;
> > }
> >
> > + if (kvm_enabled() && !kvm_arm_sve_supported(CPU(cpu))) {
> > + error_setg(errp, "'sve' feature not supported by KVM on this
> > host");
> > + return;
> > + }
> > +
> > /*
> > * It gets complicated trying to support both sve-max-vq and
> > * sve<vl-bits> properties together, so we mostly don't. We
> > @@ -460,6 +483,12 @@ static void cpu_max_set_sve_max_vq(Object *obj,
> > Visitor *v, const char *name,
> > sprintf(name, "sve%d", vq * 128);
> > object_property_set_bool(obj, true, name, &err);
> > if (err) {
> > + if (kvm_enabled()) {
> > + error_append_hint(&err, "It is not possible to use "
> > + "sve-max-vq with this KVM host. Try "
> > + "using only sve<vl-bits> "
> > + "properties.\n");
> If I understand correctly, the problem is that setting sve-max-vq to a
> given value implies all <= required lengths need to be supported and at
> least one isn't. In that case the object_property_set_bool will produce
> a "cannot enable %s", with a hint "This KVM host does not support
> the vector length %d-bits". So here you may simply say
> sve-max-vq cannot be used as one of the requuired 128b length inferior
> or equal to sve-max-vq is not supported by the host.
Which is more or less what I thought I was doing here. Are you suggesting
I change the error text to something else? If so, can you please provide
a suggestion?
>
>
> > + }
> > error_propagate(errp, err);
> > return;
> > }
> > @@ -484,6 +513,12 @@ static void cpu_arm_get_sve_vq(Object *obj, Visitor
> > *v, const char *name,
> > value = true;
> > } else if (vq_state == ARM_VQ_OFF) {
> > value = false;
> > + } else if (kvm_enabled() && vq_state == ARM_VQ_UNINITIALIZED) {
> > + /*
> > + * When KVM is enabled, anything not supported by the host must
> > have
> > + * 'OFF' for the default.
> > + */
> > + value = false;
> > } else {
> > /*
> > * vq is uninitialized. We pick a default here based on the
> > @@ -539,6 +574,11 @@ static void cpu_arm_set_sve_vq(Object *obj, Visitor
> > *v, const char *name,
> > return;
> > }
> >
> > + if (value && kvm_enabled() && !kvm_arm_sve_supported(CPU(cpu))) {
> > + error_setg(errp, "'sve' feature not supported by KVM on this
> > host");
> > + return;
> > + }
> > +
> > if (arm_sve_have_max_vq(cpu)) {
> > max_vq = cpu->sve_max_vq;
> > } else {
> > @@ -569,6 +609,8 @@ static void cpu_arm_set_sve_vq(Object *obj, Visitor *v,
> > const char *name,
> > }
> > }
> >
> > + vq_state = arm_cpu_vq_map_get(cpu, vq);
> > +
> > if (arm_sve_have_max_vq(cpu) && value && vq > cpu->sve_max_vq) {
> > error_setg(errp, "cannot enable %s", name);
> > error_append_hint(errp, "vq=%d (%d bits) is larger than the "
> > @@ -580,19 +622,31 @@ static void cpu_arm_set_sve_vq(Object *obj, Visitor
> > *v, const char *name,
> > error_append_hint(errp, "The maximum vector length must be "
> > "enabled, sve-max-vq=%d (%d bits)\n",
> > cpu->sve_max_vq, cpu->sve_max_vq * 128);
> > - } else if (arm_sve_have_max_vq(cpu) && !value && vq < cpu->sve_max_vq
> > &&
> > - is_power_of_2(vq)) {
> > + } else if (!kvm_enabled() && arm_sve_have_max_vq(cpu) && !value &&
> > + vq < cpu->sve_max_vq && is_power_of_2(vq)) {
> > error_setg(errp, "cannot disable %s", name);
> > error_append_hint(errp, "vq=%d (%d bits) is required as it is a "
> > "power-of-2 length smaller than the maximum, "
> > "sve-max-vq=%d (%d bits)\n", vq, vq * 128,
> > cpu->sve_max_vq, cpu->sve_max_vq * 128);
> > - } else if (!value && vq < max_vq && is_power_of_2(vq)) {
> > + } else if (!kvm_enabled() && !value && vq < max_vq &&
> > is_power_of_2(vq)) {
> > error_setg(errp, "cannot disable %s", name);
> > error_append_hint(errp, "Vector length %d-bits is required as it "
> > "is a power-of-2 length smaller than another "
> > "enabled vector length. Disable all larger
> > vector "
> > "lengths first.\n", vq * 128);
> > + } else if (kvm_enabled() && value && vq_state == ARM_VQ_UNINITIALIZED)
> > {
> > + error_setg(errp, "cannot enable %s", name);
> > + error_append_hint(errp, "This KVM host does not support "
> > + "the vector length %d-bits.\n", vq * 128);
> > + } else if (kvm_enabled() && !value && vq < max_vq &&
> > + (vq_state == ARM_VQ_ON ||
> > + vq_state == ARM_VQ_UNINITIALIZED_KVM_SUPPORTED)) {
> > + error_setg(errp, "cannot disable %s", name);
> > + error_append_hint(errp, "Vector length %d-bits is a KVM supported "
> > + "length smaller than another enabled vector "
> > + "length. Disable all larger vector lengths "
> > + "first.\n", vq * 128);
> all those checks may be more readable if segment into kvm / !kvm, value
> / !value
I don't understand this suggestion. Can you provide examples of more
readable checks?
> > } else {
> > if (value) {
> > bool fail = false;
> > @@ -602,31 +656,53 @@ static void cpu_arm_set_sve_vq(Object *obj, Visitor
> > *v, const char *name,
> > * Enabling a vector length automatically enables all
> > * uninitialized power-of-2 lengths smaller than it, as
> > * per the architecture.
> > + *
> > + * For KVM we have to automatically enable all supported,
> > + * uninitialized lengths smaller than this length, even
> > + * when it's not a power-of-2.
> > */
> > for (s = 1; s < vq; ++s) {
> > - if (is_power_of_2(s)) {
> > - vq_state = arm_cpu_vq_map_get(cpu, s);
> > - if (vq_state == ARM_VQ_UNINITIALIZED) {
> > - arm_cpu_vq_map_set(cpu, s, ARM_VQ_ON);
> > - } else if (vq_state == ARM_VQ_OFF) {
> > - fail = true;
> > - break;
> > - }
> > + vq_state = arm_cpu_vq_map_get(cpu, s);
> > + if (!kvm_enabled() && is_power_of_2(s) &&
> > + vq_state == ARM_VQ_UNINITIALIZED) {
> > + arm_cpu_vq_map_set(cpu, s, ARM_VQ_ON);
> > + } else if (vq_state == ARM_VQ_UNINITIALIZED_KVM_SUPPORTED)
> > {
> > + assert(kvm_enabled());
> > + arm_cpu_vq_map_set(cpu, s, ARM_VQ_ON);
> > + } else if ((kvm_enabled() || is_power_of_2(s)) &&
> > + vq_state == ARM_VQ_OFF) {
> > + fail = true;
> > + break;
> > }
> > }
> >
> > - if (fail) {
> > + if (!kvm_enabled() && fail) {
> > error_setg(errp, "cannot enable %s", name);
> > error_append_hint(errp, "Vector length %d-bits is disabled
> > "
> > "and is a power-of-2 length smaller than
> > "
> > "%s. All power-of-2 vector lengths
> > smaller "
> > "than the maximum length are
> > required.\n",
> > s * 128, name);
> > +
> > + } else if (fail) {
> > + error_setg(errp, "cannot enable %s", name);
> > + error_append_hint(errp, "Vector length %d-bits is disabled
> > "
> > + "and the KVM host requires all supported
> > "
> > + "vector lengths smaller than %s to also
> > be "
> > + "enabled.\n", s * 128, name);
> > } else {
> > arm_cpu_vq_map_set(cpu, vq, ARM_VQ_ON);
> > }
> > } else {
> > - arm_cpu_vq_map_set(cpu, vq, ARM_VQ_OFF);
> > + /*
> > + * For KVM if the vq wasn't supported then we leave it in
> > + * the ARM_VQ_UNINITIALIZED state in order to keep that
> > + * unsupported information. It'll be set to OFF later when
> > + * we finalize the map.
> > + */
> > + if (!kvm_enabled() || vq_state != ARM_VQ_UNINITIALIZED) {
> > + arm_cpu_vq_map_set(cpu, vq, ARM_VQ_OFF);
> > + }
> > }
> > }
> > }
> > @@ -689,11 +765,6 @@ static void aarch64_max_initfn(Object *obj)
> >
> > if (kvm_enabled()) {
> > kvm_arm_set_cpu_features_from_host(cpu);
> > - /*
> > - * KVM doesn't yet support the sve-max-vq property, but
> > - * setting cpu->sve_max_vq is also used to turn SVE on.
> > - */
> > - cpu->sve_max_vq = ARM_SVE_INIT;
> > } else {
> > uint64_t t;
> > uint32_t u;
> > @@ -774,32 +845,32 @@ static void aarch64_max_initfn(Object *obj)
> > cpu->ctr = 0x80038003; /* 32 byte I and D cacheline size, VIPT
> > icache */
> > cpu->dcz_blocksize = 7; /* 512 bytes */
> > #endif
> > -
> > - /*
> > - * sve_max_vq is initially unspecified, but must be initialized to
> > a
> > - * non-zero value (ARM_SVE_INIT) to indicate that this cpu type has
> > - * SVE. It will be finalized in arm_cpu_realizefn().
> > - */
> > - cpu->sve_max_vq = ARM_SVE_INIT;
> > - object_property_add(obj, "sve-max-vq", "uint32",
> > cpu_max_get_sve_max_vq,
> > - cpu_max_set_sve_max_vq, NULL, NULL,
> > &error_fatal);
> > -
> > - /*
> > - * sve_vq_map uses a special state while setting properties, so
> > - * we initialize it here with its init function and finalize it
> > - * in arm_cpu_realizefn().
> > - */
> > - arm_cpu_vq_map_init(cpu);
> > - for (vq = 1; vq <= ARM_MAX_VQ; ++vq) {
> > - char name[8];
> > - sprintf(name, "sve%d", vq * 128);
> > - object_property_add(obj, name, "bool", cpu_arm_get_sve_vq,
> > - cpu_arm_set_sve_vq, NULL, NULL,
> > &error_fatal);
> > - }
> > }
> >
> > object_property_add(obj, "sve", "bool", cpu_arm_get_sve,
> > cpu_arm_set_sve, NULL, NULL, &error_fatal);
> > +
> > + /*
> > + * sve_max_vq is initially unspecified, but must be initialized to a
> > + * non-zero value (ARM_SVE_INIT) to indicate that this cpu type has
> > + * SVE. It will be finalized in arm_cpu_realizefn().
> > + */
> > + cpu->sve_max_vq = ARM_SVE_INIT;
> > + object_property_add(obj, "sve-max-vq", "uint32",
> > cpu_max_get_sve_max_vq,
> > + cpu_max_set_sve_max_vq, NULL, NULL, &error_fatal);
> > +
> > + /*
> > + * sve_vq_map uses a special state while setting properties, so
> > + * we initialize it here with its init function and finalize it
> > + * in arm_cpu_realizefn().
> > + */
> > + arm_cpu_vq_map_init(cpu);
> > + for (vq = 1; vq <= ARM_MAX_VQ; ++vq) {
> > + char name[8];
> > + sprintf(name, "sve%d", vq * 128);
> > + object_property_add(obj, name, "bool", cpu_arm_get_sve_vq,
> > + cpu_arm_set_sve_vq, NULL, NULL, &error_fatal);
> > + }
> > }
> >
> > struct ARMCPUInfo {
> > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> > index 2821135a4d0e..5b0707e1192b 100644
> > --- a/target/arm/kvm64.c
> > +++ b/target/arm/kvm64.c
> > @@ -617,6 +617,110 @@ bool kvm_arm_sve_supported(CPUState *cpu)
> > return ret > 0;
> > }
> >
> > +QEMU_BUILD_BUG_ON(KVM_ARM64_SVE_VQ_MIN != 1);
> > +
> > +void kvm_arm_sve_get_vls(CPUState *cs, unsigned long *map,
> > + uint32_t qemu_max_vq, uint32_t *kvm_max_vq)
> you could have kvm_max_vq as a returned value?
Actually I'll just move the warn_report() to kvm_arm_sve_get_vls() and
then completely drop the kvm_max_vq parameter/return for v3. That's a
cleanup that didn't cross my mind until after posting.
> > +{
> > + static uint64_t vls[KVM_ARM64_SVE_VLS_WORDS];
> > + static uint32_t host_max_vq = -1;
> > + uint32_t vq;
> > + int i, j;
> > +
> > + bitmap_clear(map, 0, qemu_max_vq);
> > + *kvm_max_vq = 0;
> > +
> > + /*
> > + * KVM ensures all host CPUs support the same set of vector lengths.
> > + * So we only need to create a scratch VCPU once and then cache the
> > + * results.
> > + */
> > + if (host_max_vq == -1) {
> always true?
If KVM changes, then we can change this too, but for now it's always true.
> > + int fdarray[3], ret = -1;
> > +
> > + if (!kvm_arm_create_scratch_host_vcpu(NULL, fdarray, NULL)) {
> > + error_report("failed to create scratch vcpu");
> > + abort();
> > + }
> > +
> > + if (ioctl(fdarray[0], KVM_CHECK_EXTENSION, KVM_CAP_ARM_SVE) > 0) {
> suggestion:
>
> has_sve = ioctl(fdarray[0], KVM_CHECK_EXTENSION, KVM_CAP_ARM_SVE);
> kvm_arm_destroy_scratch_host_vcpu(fdarray);
> if (!has_sve) {
> return;
> }
>
> /* host supports SVE */
> ../..
I prefer it the way it is, because then I get to tuck these two temporary
structures below into the "supports SVE" block. If I do it the way you
suggest, then I'll have to move them up under the last 'if', which doesn't
fit as nicely. I could add the /* Host supports SVE */ comment to the
block I have though, if you'd like.
> > + struct kvm_vcpu_init init = {
> > + .target = -1,
> > + .features[0] = (1 << KVM_ARM_VCPU_SVE),
> > + };
> > + struct kvm_one_reg reg = {
> > + .id = KVM_REG_ARM64_SVE_VLS,
> > + .addr = (uint64_t)&vls[0],
> > + };
> > +
> > + kvm_arm_destroy_scratch_host_vcpu(fdarray);
> > +
> > + if (!kvm_arm_create_scratch_host_vcpu(NULL, fdarray, &init)) {
> > + error_report("failed to create scratch vcpu");
> > + abort();
> > + }
> > +
> > + ret = ioctl(fdarray[2], KVM_GET_ONE_REG, ®);
> > + if (ret) {
> > + error_report("failed to get KVM_REG_ARM64_SVE_VLS: %s",
> > + strerror(errno));
> > + abort();
> > + }
> > + }
> > +
> > + kvm_arm_destroy_scratch_host_vcpu(fdarray);
> > +
> > + if (ret) {
> > + /* The host doesn't support SVE. */
> > + return;
> > + }
> > + }
> > +
> > + for (i = KVM_ARM64_SVE_VLS_WORDS - 1; i >= 0; --i) {
> > + if (!vls[i]) {
> > + continue;
> > + }
> > + if (host_max_vq == -1) {
> > + host_max_vq = 64 - clz64(vls[i]) + i * 64;
> > + }
> > + for (j = 1; j <= 64; ++j) {
> > + vq = j + i * 64;
> > + if (vq > qemu_max_vq) {
> > + break;
> > + }
> > + if (vls[i] & (1UL << (j - 1))) {
> > + set_bit(vq - 1, map);
> > + }
> > + }
> > + }
> > +
> > + *kvm_max_vq = host_max_vq;
> > +}
> > +
> > +static int kvm_arm_sve_set_vls(CPUState *cs)
> > +{
> > + uint64_t vls[KVM_ARM64_SVE_VLS_WORDS] = {0};
> > + struct kvm_one_reg reg = {
> > + .id = KVM_REG_ARM64_SVE_VLS,
> > + .addr = (uint64_t)&vls[0],
> > + };
> > + ARMCPU *cpu = ARM_CPU(cs);
> > + uint32_t vq;
> > + int i, j;
> > +
> > + assert(cpu->sve_max_vq <= KVM_ARM64_SVE_VQ_MAX);
> > +
> > + for (vq = 1; vq <= cpu->sve_max_vq; ++vq) {
> > + if (test_bit(vq - 1, cpu->sve_vq_map)) {
> > + i = (vq - 1) / 64;
> > + j = (vq - 1) % 64;
> > + vls[i] |= 1UL << j;
> > + }
> > + }
> > +
> > + return kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®);
> > +}
> > +
> > #define ARM_CPU_ID_MPIDR 3, 0, 0, 0, 5
> >
> > int kvm_arch_init_vcpu(CPUState *cs)
> > @@ -628,7 +732,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
> >
> > if (cpu->kvm_target == QEMU_KVM_ARM_TARGET_NONE ||
> > !object_dynamic_cast(OBJECT(cpu), TYPE_AARCH64_CPU)) {
> > - fprintf(stderr, "KVM is not supported for this guest CPU type\n");
> > + error_report("KVM is not supported for this guest CPU type");
> > return -EINVAL;
> > }
> >
> > @@ -653,11 +757,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
> > unset_feature(&env->features, ARM_FEATURE_PMU);
> > }
> > if (cpu->sve_max_vq) {
> > - if (!kvm_arm_sve_supported(cs)) {
> > - cpu->sve_max_vq = 0;
> > - } else {
> > - cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SVE;
> > - }
> > + assert(kvm_arm_sve_supported(cs));
> > + cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SVE;
> > }
> >
> > /* Do KVM_ARM_VCPU_INIT ioctl */
> > @@ -667,6 +768,10 @@ int kvm_arch_init_vcpu(CPUState *cs)
> > }
> >
> > if (cpu->sve_max_vq) {
> > + ret = kvm_arm_sve_set_vls(cs);
> > + if (ret) {
> > + return ret;
> > + }
> > ret = kvm_arm_vcpu_finalize(cs, KVM_ARM_VCPU_SVE);
> > if (ret) {
> > return ret;
> > diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
> > index 2367f8ab78ed..d5eb341e906d 100644
> > --- a/target/arm/kvm_arm.h
> > +++ b/target/arm/kvm_arm.h
> > @@ -212,6 +212,22 @@ typedef struct ARMHostCPUFeatures {
> > */
> > bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf);
> >
> > +/**
> > + * kvm_arm_sve_get_vls:
> kernel-doc comment?
afaict I've formatted this comment in the same style as the other
functions in this file.
> > + * @cs: CPUState
> > + * @map: bitmap to fill in
> > + * @qemu_max_vq: the maximum vector length QEMU supports in quadwords
> > + * (size of the bitmap to fill in)
> > + * @kvm_max_vq: the maximum vector length KVM supports in quadwords
> > + *
> > + * Get all the SVE vector lengths supported by the KVM host, setting
> > + * the bits corresponding to their length in quadwords minus one
> > + * (vq - 1) in @map up to @qemu_max_vq. Also assign @kvm_max_vq to the
> > + * maximum vector length the KVM host supports.
> > + */
> > +void kvm_arm_sve_get_vls(CPUState *cs, unsigned long *map,
> > + uint32_t qemu_max_vq, uint32_t *kvm_max_vq);
> > +
> > /**
> > * kvm_arm_set_cpu_features_from_host:
> > * @cpu: ARMCPU to set the features for
> > @@ -314,6 +330,9 @@ static inline int kvm_arm_vgic_probe(void)
> > static inline void kvm_arm_pmu_set_irq(CPUState *cs, int irq) {}
> > static inline void kvm_arm_pmu_init(CPUState *cs) {}
> >
> > +static inline void kvm_arm_sve_get_vls(CPUState *cs, unsigned long *map,
> > + uint32_t qemu_max_vq,
> > + uint32_t *kvm_max_vq) {}
> > #endif
> >
> > static inline const char *gic_class_name(void)
> > diff --git a/target/arm/monitor.c b/target/arm/monitor.c
> > index 1e213906fd8f..284818fb4a51 100644
> > --- a/target/arm/monitor.c
> > +++ b/target/arm/monitor.c
> > @@ -100,7 +100,7 @@ QEMU_BUILD_BUG_ON(ARM_MAX_VQ > 16);
> > *
> > * The sve<vl-bits> features need to be in reverse order in order to
> > * enable/disable the largest vector lengths first, ensuring all
> > - * power-of-2 vector lengths smaller can also be enabled/disabled.
> > + * smaller required vector lengths can also be enabled/disabled.
> > */
> > static const char *cpu_model_advertised_features[] = {
> > "aarch64", "pmu", "sve",
> > diff --git a/tests/arm-cpu-features.c b/tests/arm-cpu-features.c
> > index 67ad5f2b78d5..349bd0dca6d1 100644
> > --- a/tests/arm-cpu-features.c
> > +++ b/tests/arm-cpu-features.c
> > @@ -324,23 +324,35 @@ static void sve_tests_sve_max_vq_8(const void *data)
> > qtest_quit(qts);
> > }
> >
> > -static void sve_tests_sve_off(const void *data)
> > +static void sve_tests_off(QTestState *qts, const char *cpu_type)
> > {
> > - QTestState *qts;
> > -
> > - qts = qtest_init(MACHINE "-cpu max,sve=off");
> > -
> > /*
> > * SVE is off, so the map should be empty.
> > */
> > - assert_sve_vls(qts, "max", 0, NULL);
> > + assert_sve_vls(qts, cpu_type, 0, NULL);
> >
> > /*
> > * We can't turn anything on, but off is OK.
> > */
> > - assert_error(qts, "max", "cannot enable sve128", "{ 'sve128': true }");
> > - assert_sve_vls(qts, "max", 0, "{ 'sve128': false }");
> > + assert_error(qts, cpu_type, "cannot enable sve128", "{ 'sve128': true
> > }");
> > + assert_sve_vls(qts, cpu_type, 0, "{ 'sve128': false }");
> > +}
> >
> > +static void sve_tests_sve_off(const void *data)
> > +{
> > + QTestState *qts;
> > +
> > + qts = qtest_init(MACHINE "-cpu max,sve=off");
> > + sve_tests_off(qts, "max");
> > + qtest_quit(qts);
> > +}
> > +
> > +static void sve_tests_sve_off_kvm(const void *data)
> > +{
> > + QTestState *qts;
> > +
> > + qts = qtest_init(MACHINE "-accel kvm -cpu max,sve=off");
> > + sve_tests_off(qts, "max");
> > qtest_quit(qts);
> > }
> >
> > @@ -392,12 +404,66 @@ static void test_query_cpu_model_expansion_kvm(const
> > void *data)
> > assert_has_feature(qts, "host", "pmu");
> >
> > if (g_str_equal(qtest_get_arch(), "aarch64")) {
> > + bool kvm_supports_sve;
> > + uint32_t max_vq, vq;
> > + uint64_t vls;
> > + char name[8];
> > + QDict *resp;
> > + char *error;
> > +
> > assert_has_feature(qts, "host", "aarch64");
> > - assert_has_feature(qts, "max", "sve");
> >
> > assert_error(qts, "cortex-a15",
> > "The CPU definition 'cortex-a15' cannot "
> > "be used with KVM on this host", NULL);
> > +
> > + assert_has_feature(qts, "max", "sve");
> > + resp = do_query_no_props(qts, "max");
> > + g_assert(resp);
> > + kvm_supports_sve = qdict_get_bool(resp_get_props(resp), "sve");
> > + qobject_unref(resp);
> > +
> > + if (kvm_supports_sve) {
> > + resp = do_query_no_props(qts, "max");
> > + resp_get_sve_vls(resp, &vls, &max_vq);
> > + g_assert(max_vq != 0);
> > + qobject_unref(resp);
> > +
> > + /* Enabling a supported length is of course fine. */
> > + sprintf(name, "sve%d", max_vq * 128);
> > + assert_sve_vls(qts, "max", vls, "{ %s: true }", name);
> > +
> > + /* Also disabling the largest lengths is fine. */
> > + assert_sve_vls(qts, "max", (vls & ~BIT(max_vq - 1)),
> > + "{ %s: false }", name);
> > +
> > + for (vq = 1; vq <= max_vq; ++vq) {
> > + if (!(vls & BIT(vq - 1))) {
> > + /* vq is unsupported */
> > + break;
> > + }
> > + }
> > + if (vq <= SVE_MAX_VQ) {
> > + sprintf(name, "sve%d", vq * 128);
> > + error = g_strdup_printf("cannot enable %s", name);
> > + assert_error(qts, "max", error, "{ %s: true }", name);
> > + g_free(error);
> > + }
> > +
> > + if (max_vq > 1) {
> > + /* The next smaller, supported vq is required */
> > + vq = 64 - __builtin_clzll(vls & ~BIT(max_vq - 1));
> > + sprintf(name, "sve%d", vq * 128);
> > + error = g_strdup_printf("cannot disable %s", name);
> > + assert_error(qts, "max", error, "{ %s: false }", name);
> > + g_free(error);
> > + }
> > + } else {
> > + resp = do_query_no_props(qts, "max");
> > + resp_get_sve_vls(resp, &vls, &max_vq);
> > + g_assert(max_vq == 0);
> > + qobject_unref(resp);
> > + }
> > } else {
> > assert_error(qts, "host",
> > "'pmu' feature not supported by KVM on this host",
> > @@ -434,6 +500,8 @@ int main(int argc, char **argv)
> > if (kvm_available) {
> > qtest_add_data_func("/arm/kvm/query-cpu-model-expansion",
> > NULL, test_query_cpu_model_expansion_kvm);
> > + qtest_add_data_func("/arm/kvm/query-cpu-model-expansion/sve-off",
> > + NULL, sve_tests_sve_off_kvm);
> > }
> >
> > return g_test_run();
> >
>
> Thanks
>
> Eric
>
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-devel] [PATCH v2 13/14] target/arm/cpu64: max cpu: Support sve properties with KVM,
Andrew Jones <=