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

[Haibo Xu]

On Tue, 15 Sep 2020 at 14:11, Andrew Jones <drjones@redhat.com> wrote:
>
> 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
>

Yes,  It should be in the kvm_enabled() part.
Will fix it in the next version.

Thanks,
Haibo

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