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

[Auger Eric <eric.auger@redhat.com>]

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 <drjones@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, &reg);
> +    if (ret) {
> +        return ret;
> +    }
> +
> +    ret = 0;
is it mandated?
> +    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?
> +                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?
> +                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?
> +                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
> +                         "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