On Thu, Jun 06, 2019 at 10:30:00AM +0200, Auger Eric wrote:
> Hi Drew,
> On 5/12/19 10:36 AM, Andrew Jones wrote:
> > Allow the cpu type 'max' sve-max-vq property to work with kvm
> > too. If the property is not specified then the maximum kvm
> > supports is used. If it is specified we check that kvm supports
> > that exact length or error out if it doesn't.
> >
> > Signed-off-by: Andrew Jones <drjo...@redhat.com>
> > ---
> > target/arm/cpu.h | 4 +++
> > target/arm/cpu64.c | 7 ++--
> > target/arm/kvm64.c | 80 ++++++++++++++++++++++++++++++++++++++++++++--
> > 3 files changed, 86 insertions(+), 5 deletions(-)
> >
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index 733b840a7127..8292d547e8f9 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -3122,6 +3122,10 @@ static inline uint64_t arm_sctlr(CPUARMState *env,
> > int el)
> > }
> > }
> >
> > +static inline int arm_cpu_fls64(uint64_t v)
> > +{
> > + return !v ? 0 : 64 - clz64(v);
> > +}
> >
> > /* Return true if the processor is in big-endian mode. */
> > static inline bool arm_cpu_data_is_big_endian(CPUARMState *env)
> > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> > index 6c19ef6837d5..3756e7e2a3e5 100644
> > --- a/target/arm/cpu64.c
> > +++ b/target/arm/cpu64.c
> > @@ -292,7 +292,7 @@ static void aarch64_max_initfn(Object *obj)
> >
> > if (kvm_enabled()) {
> > kvm_arm_set_cpu_features_from_host(cpu);
> > - cpu->sve_max_vq = ARM_MAX_VQ;
> > + cpu->sve_max_vq = -1; /* set in kvm_arch_init_vcpu() */
> > } else {
> > uint64_t t;
> > uint32_t u;
> > @@ -374,9 +374,10 @@ static void aarch64_max_initfn(Object *obj)
> > #endif
> >
> > cpu->sve_max_vq = ARM_MAX_VQ;
> > - object_property_add(obj, "sve-max-vq", "uint32",
> > cpu_max_get_sve_vq,
> > - cpu_max_set_sve_vq, NULL, NULL, &error_fatal);
> > }
> > +
> > + object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_vq,
> > + cpu_max_set_sve_vq, NULL, NULL, &error_fatal);> }
> >
> > struct ARMCPUInfo {
> > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> > index c2d92df75353..0c666e405357 100644
> > --- a/target/arm/kvm64.c
> > +++ b/target/arm/kvm64.c
> > @@ -446,6 +446,59 @@ void kvm_arm_pmu_set_irq(CPUState *cs, int irq)
> > }
> > }
> >
> > +static int kvm_arm_get_sve_vls(CPUState *cs, uint64_t sve_vls[])
> > +{
> > + struct kvm_one_reg reg = {
> > + .id = KVM_REG_ARM64_SVE_VLS,
> > + .addr = (uint64_t)&sve_vls[0],
> > + };
> > + int i, ret;
> > +
> > + ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®);
> > + if (ret) {
> > + return ret;
> > + }
> > +
> > + ret = 0;
> is it mandated?
Yes, this can be removed, as we know ret=0 here.
> > + for (i = KVM_ARM64_SVE_VLS_WORDS - 1; i >= 0; --i) {
> > + if (sve_vls[i]) {
> > + ret = arm_cpu_fls64(sve_vls[i]) + i * 64;
> > + break;
> > + }
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int kvm_arm_set_sve_vls(CPUState *cs, uint64_t sve_vls[], int
> > max_vq)
> > +{
> > + struct kvm_one_reg reg = {
> > + .id = KVM_REG_ARM64_SVE_VLS,
> > + .addr = (uint64_t)&sve_vls[0],
> > + };
> > + int i;
> > +
> > + for (i = KVM_ARM64_SVE_VLS_WORDS - 1; i >= 0; --i) {
> > + if (sve_vls[i]) {
> > + int vq = arm_cpu_fls64(sve_vls[i]) + i * 64;
> nit: add a line
> > + while (vq > max_vq) {
> > + sve_vls[i] &= ~BIT_MASK(vq - 1);
> Isn't BIT_MASK for 32b. MAKE_64BIT_MASK?
We should just need a 'UL', not a 'ULL'. I think I'll change all
my BIT_MASK() usage to just BIT() for v2 though, even though
there doesn't appear to be a difference for the cases I've used
it.
> > + vq = arm_cpu_fls64(sve_vls[i]) + i * 64;
> > + }
> > + if (vq < max_vq) {
> I don't really get this check: having vq less than max_vq does not seems
> weird. Do you absolutely want vq=max_vq?
In this context 'vq' is the max vq KVM supports and max_vq is the maximum
the user requested. So if the user wants a maximum greater than what is
possible, then that's an error.
> > + error_report("sve-max-vq=%d is not a valid length",
> > max_vq);
> > + error_printf("next lowest is %d\n", vq);
> why mixing error_report/printf?
This is how we're supposed to do two line error messages, afaik.
> > + return -EINVAL;
> > + }
> > + if (vq == max_vq) {
> > + break;
> > + }
> > + }
> > + }
> > +
> > + return kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®);
> > +}
> > +
> > static inline void set_feature(uint64_t *features, int feature)
> > {
> > *features |= 1ULL << feature;
> > @@ -605,7 +658,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;
> > }
> >
> > @@ -631,7 +684,12 @@ int kvm_arch_init_vcpu(CPUState *cs)
> > }
> > if (cpu->sve_max_vq) {
> > if (!kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_SVE)) {
> > - cpu->sve_max_vq = 0;
> > + if (cpu->sve_max_vq == -1) {> + cpu->sve_max_vq
> > = 0;
> > + } else {
> > + error_report("This KVM host does not support SVE");
> > + return -EINVAL;
> > + }
> > } else {
> > cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SVE;
> > }
> > @@ -644,6 +702,24 @@ int kvm_arch_init_vcpu(CPUState *cs)
> > }
> >
> > if (cpu->sve_max_vq) {
> > + uint64_t sve_vls[KVM_ARM64_SVE_VLS_WORDS];
> line
> > + ret = kvm_arm_get_sve_vls(cs, sve_vls);
> > + if (ret < 0) {
> > + return ret;
> > + }
> > + if (cpu->sve_max_vq == -1) {> + cpu->sve_max_vq = ret;
> > + } else if (cpu->sve_max_vq > ret) {
> > + error_report("This KVM host does not support SVE vectors "
> I would rephrase the error mesg with something like:
> This KVM host supports SVE vectors of max VQ=%d whereas requested VQ is %d
Yup, giving a better hint as to what the host does support is a good idea.
> > + "of length %d quadwords (%d bytes)",
> > + cpu->sve_max_vq, cpu->sve_max_vq * 16);
> > + return -EINVAL;
> > + } else {
> > + ret = kvm_arm_set_sve_vls(cs, sve_vls, cpu->sve_max_vq);
> > + if (ret < 0) {
> > + return ret;
> > + }
> > + }
> > ret = kvm_arm_vcpu_finalize(cs, KVM_ARM_VCPU_SVE);
> > if (ret) {
> > return ret;
> >
> Thanks
>
> Eric
Thanks,
drew
