Re: [PATCH v3 03/12] target/arm/cpu: spe: Add an option to turn on/off vSPE support

[Andrew Jones]

On Tue, Sep 15, 2020 at 03:11:39AM +0000, Haibo Xu wrote:
> Adds a spe=[on/off] option to enable/disable vSPE support in
> guest vCPU.
> 
> Reviewed-by: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
> ---
>  target/arm/cpu.c   |  6 ++++++
>  target/arm/cpu.h   | 13 ++++++++++++
>  target/arm/cpu64.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 71 insertions(+)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index c179e0752d..f211958eaa 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1310,6 +1310,12 @@ void arm_cpu_finalize_features(ARMCPU *cpu, Error 
> **errp)
>              error_propagate(errp, local_err);
>              return;
>          }
> +
> +        arm_cpu_spe_finalize(cpu, &local_err);
> +        if (local_err != NULL) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
>      }
>  }
>  
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index a1c7d8ebae..baf2bbcee8 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -24,6 +24,7 @@
>  #include "hw/registerfields.h"
>  #include "cpu-qom.h"
>  #include "exec/cpu-defs.h"
> +#include "qapi/qapi-types-common.h"
>  
>  /* ARM processors have a weak memory model */
>  #define TCG_GUEST_DEFAULT_MO      (0)
> @@ -196,9 +197,11 @@ typedef struct {
>  #ifdef TARGET_AARCH64
>  # define ARM_MAX_VQ    16
>  void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp);
> +void arm_cpu_spe_finalize(ARMCPU *cpu, Error **errp);
>  #else
>  # define ARM_MAX_VQ    1
>  static inline void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp) { }
> +static inline void arm_cpu_spe_finalize(ARMCPU *cpu, Error **errp) { }
>  #endif
>  
>  typedef struct ARMVectorReg {
> @@ -829,6 +832,8 @@ struct ARMCPU {
>      bool has_el3;
>      /* CPU has PMU (Performance Monitor Unit) */
>      bool has_pmu;
> +    /* CPU has SPE (Statistical Profiling Extension) */
> +    OnOffAuto has_spe;
>      /* CPU has VFP */
>      bool has_vfp;
>      /* CPU has Neon */
> @@ -3869,6 +3874,14 @@ static inline bool isar_feature_aa64_pmu_8_4(const 
> ARMISARegisters *id)
>          FIELD_EX64(id->id_aa64dfr0, ID_AA64DFR0, PMUVER) != 0xf;
>  }
>  
> +/*
> + * Currently we don't differentiate between the ARMv8.2-SPE and ARMv8.3-SPE.
> + */
> +static inline bool isar_feature_aa64_spe(const ARMISARegisters *id)
> +{
> +    return FIELD_EX64(id->id_aa64dfr0, ID_AA64DFR0, PMSVER) != 0;
> +}
> +
>  static inline bool isar_feature_aa64_rcpc_8_3(const ARMISARegisters *id)
>  {
>      return FIELD_EX64(id->id_aa64isar1, ID_AA64ISAR1, LRCPC) != 0;
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 3c2b3d9599..4997c4a3c0 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -572,6 +572,55 @@ void aarch64_add_sve_properties(Object *obj)
>      }
>  }
>  
> +void arm_cpu_spe_finalize(ARMCPU *cpu, Error **errp)
> +{
> +    uint64_t t;
> +    uint32_t value = 0;
> +
> +    if (cpu->has_spe == ON_OFF_AUTO_AUTO) {
> +        if (kvm_enabled() && kvm_arm_spe_supported()) {
> +            cpu->has_spe = ON_OFF_AUTO_ON;
> +        } else {
> +            cpu->has_spe = ON_OFF_AUTO_OFF;
> +        }
> +    } else if (cpu->has_spe == ON_OFF_AUTO_ON) {
> +        if (!kvm_enabled() || !kvm_arm_spe_supported()) {
> +            error_setg(errp, "'spe' cannot be enabled on this host");
> +            return;
> +        }
> +    }
> +
> +    /*
> +     * According to the ARM ARM, the ID_AA64DFR0[PMSVER] currently
> +     * support 3 values:
> +     *
> +     * 0b0000: SPE not implemented
> +     * 0b0001: ARMv8.2-SPE implemented
> +     * 0b0010: ARMv8.3-SPE implemented
> +     *
> +     * But the kernel KVM API didn't expose all these 3 values, and
> +     * we can only get whether the SPE feature is supported or not.
> +     * So here we just set the PMSVER to 1 if this feature was supported.
> +     */
> +    if (cpu->has_spe == ON_OFF_AUTO_ON) {
> +        value = 1;
> +    }
> +
> +    t = cpu->isar.id_aa64dfr0;
> +    t = FIELD_DP64(t, ID_AA64DFR0, PMSVER, value);
> +    cpu->isar.id_aa64dfr0 = t;
> +}
> +
> +static bool arm_spe_get(Object *obj, Error **errp)
> +{
> +    return ARM_CPU(obj)->has_spe != ON_OFF_AUTO_OFF;
> +}
> +
> +static void arm_spe_set(Object *obj, bool value, Error **errp)
> +{
> +    ARM_CPU(obj)->has_spe = value ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
> +}
> +
>  /* -cpu max: if KVM is enabled, like -cpu host (best possible with this 
> host);
>   * otherwise, a CPU with as many features enabled as our emulation supports.
>   * The version of '-cpu max' for qemu-system-arm is defined in cpu.c;
> @@ -721,6 +770,9 @@ static void aarch64_max_initfn(Object *obj)
>      aarch64_add_sve_properties(obj);
>      object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_max_vq,
>                          cpu_max_set_sve_max_vq, NULL, NULL);
> +
> +    cpu->has_spe = ON_OFF_AUTO_AUTO;
> +    object_property_add_bool(obj, "spe", arm_spe_get, arm_spe_set);
>  }

The cover-letter says we're only providing the 'spe' property for KVM
enabled guests, but we're adding it here in the common part of the
max init function. It should be added in the kvm_enabled() part only.

Thanks,
drew

>  
>  static const ARMCPUInfo aarch64_cpus[] = {
> -- 
> 2.17.1
> 
>