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

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

On 8/10/20 5:03 AM, Haibo Xu wrote:
> 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.

You are right, we don't know whether if the feature is announced
to the guest, the guest will use the SPE registers, so similarly
to PMU have it default ON if available.

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