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

[Haibo Xu]

On Fri, 7 Aug 2020 at 16:28, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> 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.
>

I'm not sure whether it's proper to follow the 'pmu' setting here
which has it on  by default.
To be honest, I also prefer to turn it off by default(Please refer to
the comments in the cover letter).

> > +        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)
> >
>