Re: [PATCH 3/7] target/arm/cpu: spe: Add an option to turn on/off vSPE support

["Philippe Mathieu-Daudé" <f4bug@amsat.org>]

Hi Haibo,

On 8/7/20 10:10 AM, Haibo Xu wrote:
> Adds a spe=[on/off] option to enable/disable vSPE support in
> guest vCPU. Note this option is only available for "-cpu host"
> with KVM mode, and default value is on. 
> 
> Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
> ---
>  target/arm/cpu.c | 28 ++++++++++++++++++++++++++++
>  target/arm/cpu.h |  3 +++ 
>  2 files changed, 31 insertions(+)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 111579554f..40768b4d19 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1122,6 +1122,29 @@ static void arm_set_pmu(Object *obj, bool value, Error **errp)
>      cpu->has_pmu = value;
>  }
>  
> +static bool arm_get_spe(Object *obj, Error **errp)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +
> +    return cpu->has_spe;
> +}
> +
> +static void arm_set_spe(Object *obj, bool value, Error **errp)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +
> +    if (value) {
> +        if (kvm_enabled() && !kvm_arm_spe_supported()) {
> +            error_setg(errp, "'spe' feature not supported by KVM on this host");
> +            return;
> +        }
> +        set_feature(&cpu->env, ARM_FEATURE_SPE);
> +    } else {
> +        unset_feature(&cpu->env, ARM_FEATURE_SPE);
> +    }
> +    cpu->has_spe = value;
> +}
> +
>  unsigned int gt_cntfrq_period_ns(ARMCPU *cpu)
>  {
>      /*
> @@ -1195,6 +1218,11 @@ void arm_cpu_post_init(Object *obj)
>          object_property_add_bool(obj, "pmu", arm_get_pmu, arm_set_pmu);
>      }
> 
> +    if (arm_feature(&cpu->env, ARM_FEATURE_SPE)) {
> +        cpu->has_spe = true;

Being a profiling feature, are you sure you want it to be ON by default?

I'd expect the opposite, either being turned on via command line at
startup or by a management layer at runtime, when someone is ready
to record the perf events and analyze them.

> +        object_property_add_bool(obj, "spe", arm_get_spe, arm_set_spe);
> +    }
> +
>      /*
>       * Allow user to turn off VFP and Neon support, but only for TCG --
>       * KVM does not currently allow us to lie to the guest about its
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 9e8ed423ea..fe0ac14386 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -822,6 +822,8 @@ struct ARMCPU {
>      bool has_el3;
>      /* CPU has PMU (Performance Monitor Unit) */
>      bool has_pmu;
> +    /* CPU has SPE (Statistical Profiling Extension) */
> +    bool has_spe;
>      /* CPU has VFP */
>      bool has_vfp;
>      /* CPU has Neon */
> @@ -1959,6 +1961,7 @@ enum arm_features {
>      ARM_FEATURE_VBAR, /* has cp15 VBAR */
>      ARM_FEATURE_M_SECURITY, /* M profile Security Extension */
>      ARM_FEATURE_M_MAIN, /* M profile Main Extension */
> +    ARM_FEATURE_SPE, /* has SPE support */
>  };
> 
>  static inline int arm_feature(CPUARMState *env, int feature)
>