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 <drjo...@redhat.com> > > --- > > 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 >