Re: [Qemu-devel] [PATCH 07/13] target/arm/kvm: max cpu: Allow sve max vector length setting

[Andrew Jones]

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 <address@hidden>
> > ---
> >  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, &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, &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