Re: [Qemu-devel] [PATCH 1/4] target/arm: Allow VFP and Neon to be disabled via a CPU property

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 5/17/19 7:40 PM, Peter Maydell wrote:
> Allow VFP and neon to be disabled via a CPU property. As with
> the "pmu" property, we only allow these features to be removed
> from CPUs which have it by default, not added to CPUs which
> don't have it.
> 
> The primary motivation here is to be able to optionally
> create Cortex-M33 CPUs with no FPU, but we provide switches
> for both VFP and Neon because the two interact:
>  * AArch64 can't have one without the other
>  * Some ID register fields only change if both are disabled
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/cpu.h |   4 ++
>  target/arm/cpu.c | 150 +++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 148 insertions(+), 6 deletions(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 733b840a712..778fb293e7c 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -797,6 +797,10 @@ struct ARMCPU {
>      bool has_el3;
>      /* CPU has PMU (Performance Monitor Unit) */
>      bool has_pmu;
> +    /* CPU has VFP */
> +    bool has_vfp;
> +    /* CPU has Neon */
> +    bool has_neon;
>  
>      /* CPU has memory protection unit */
>      bool has_mpu;
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 8eee1d8c59a..406fd360a2a 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -763,6 +763,12 @@ static Property arm_cpu_cfgend_property =
>  static Property arm_cpu_has_pmu_property =
>              DEFINE_PROP_BOOL("pmu", ARMCPU, has_pmu, true);
>  
> +static Property arm_cpu_has_vfp_property =
> +            DEFINE_PROP_BOOL("vfp", ARMCPU, has_vfp, true);
> +
> +static Property arm_cpu_has_neon_property =
> +            DEFINE_PROP_BOOL("neon", ARMCPU, has_neon, true);
> +
>  static Property arm_cpu_has_mpu_property =
>              DEFINE_PROP_BOOL("has-mpu", ARMCPU, has_mpu, true);
>  
> @@ -803,6 +809,13 @@ void arm_cpu_post_init(Object *obj)
>      if (arm_feature(&cpu->env, ARM_FEATURE_M)) {
>          set_feature(&cpu->env, ARM_FEATURE_PMSA);
>      }
> +    /* Similarly for the VFP feature bits */
> +    if (arm_feature(&cpu->env, ARM_FEATURE_VFP4)) {
> +        set_feature(&cpu->env, ARM_FEATURE_VFP3);
> +    }
> +    if (arm_feature(&cpu->env, ARM_FEATURE_VFP3)) {
> +        set_feature(&cpu->env, ARM_FEATURE_VFP);
> +    }
>  
>      if (arm_feature(&cpu->env, ARM_FEATURE_CBAR) ||
>          arm_feature(&cpu->env, ARM_FEATURE_CBAR_RO)) {
> @@ -847,6 +860,27 @@ void arm_cpu_post_init(Object *obj)
>                                   &error_abort);
>      }
>  
> +    /*
> +     * 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
> +     * ID/feature registers, so the guest always sees what the host has.
> +     */

I wondered about that last week when refreshing Samuel TCG/KVM split,
because the VFP code pulls a lot of unnecessary code on KVM (in
particular the softfloat lib).

> +    if (arm_feature(&cpu->env, ARM_FEATURE_VFP)) {
> +        cpu->has_vfp = true;
> +        if (!kvm_enabled()) {
> +            qdev_property_add_static(DEVICE(obj), &arm_cpu_has_vfp_property,
> +                                     &error_abort);
> +        }
> +    }
> +
> +    if (arm_feature(&cpu->env, ARM_FEATURE_NEON)) {
> +        cpu->has_neon = true;
> +        if (!kvm_enabled()) {
> +            qdev_property_add_static(DEVICE(obj), &arm_cpu_has_neon_property,
> +                                     &error_abort);
> +        }
> +    }
> +
>      if (arm_feature(&cpu->env, ARM_FEATURE_PMSA)) {
>          qdev_property_add_static(DEVICE(obj), &arm_cpu_has_mpu_property,
>                                   &error_abort);
> @@ -956,6 +990,116 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> +    if (arm_feature(env, ARM_FEATURE_AARCH64) &&
> +        cpu->has_vfp != cpu->has_neon) {
> +        /*
> +         * This is an architectural requirement for AArch64; AArch32 is
> +         * more flexible and permits VFP-no-Neon and Neon-no-VFP.
> +         */
> +        error_setg(errp,
> +                   "AArch64 CPUs must have both VFP and Neon or neither");
> +        return;
> +    }
> +
> +    if (!cpu->has_vfp) {
> +        uint64_t t;
> +        uint32_t u;
> +
> +        unset_feature(env, ARM_FEATURE_VFP);
> +        unset_feature(env, ARM_FEATURE_VFP3);
> +        unset_feature(env, ARM_FEATURE_VFP4);
> +
> +        t = cpu->isar.id_aa64isar1;
> +        t = FIELD_DP64(t, ID_AA64ISAR1, JSCVT, 0);
> +        cpu->isar.id_aa64isar1 = t;
> +
> +        t = cpu->isar.id_aa64pfr0;
> +        t = FIELD_DP64(t, ID_AA64PFR0, FP, 0xf);
> +        cpu->isar.id_aa64pfr0 = t;
> +
> +        u = cpu->isar.id_isar6;
> +        u = FIELD_DP32(u, ID_ISAR6, JSCVT, 0);
> +        cpu->isar.id_isar6 = u;
> +
> +        u = cpu->isar.mvfr0;
> +        u = FIELD_DP32(u, MVFR0, FPSP, 0);
> +        u = FIELD_DP32(u, MVFR0, FPDP, 0);
> +        u = FIELD_DP32(u, MVFR0, FPTRAP, 0);
> +        u = FIELD_DP32(u, MVFR0, FPDIVIDE, 0);
> +        u = FIELD_DP32(u, MVFR0, FPSQRT, 0);
> +        u = FIELD_DP32(u, MVFR0, FPSHVEC, 0);
> +        u = FIELD_DP32(u, MVFR0, FPROUND, 0);
> +        cpu->isar.mvfr0 = u;
> +
> +        u = cpu->isar.mvfr1;
> +        u = FIELD_DP32(u, MVFR1, FPFTZ, 0);
> +        u = FIELD_DP32(u, MVFR1, FPDNAN, 0);
> +        u = FIELD_DP32(u, MVFR1, FPHP, 0);
> +        cpu->isar.mvfr1 = u;
> +
> +        u = cpu->isar.mvfr2;
> +        u = FIELD_DP32(u, MVFR2, FPMISC, 0);
> +        cpu->isar.mvfr2 = u;
> +    }
> +
> +    if (!cpu->has_neon) {
> +        uint64_t t;
> +        uint32_t u;
> +
> +        unset_feature(env, ARM_FEATURE_NEON);
> +
> +        t = cpu->isar.id_aa64isar0;
> +        t = FIELD_DP64(t, ID_AA64ISAR0, DP, 0);
> +        cpu->isar.id_aa64isar0 = t;
> +
> +        t = cpu->isar.id_aa64isar1;
> +        t = FIELD_DP64(t, ID_AA64ISAR1, FCMA, 0);
> +        cpu->isar.id_aa64isar1 = t;
> +
> +        t = cpu->isar.id_aa64pfr0;
> +        t = FIELD_DP64(t, ID_AA64PFR0, ADVSIMD, 0xf);
> +        cpu->isar.id_aa64pfr0 = t;
> +
> +        u = cpu->isar.id_isar5;
> +        u = FIELD_DP32(u, ID_ISAR5, RDM, 0);
> +        u = FIELD_DP32(u, ID_ISAR5, VCMA, 0);
> +        cpu->isar.id_isar5 = u;
> +
> +        u = cpu->isar.id_isar6;
> +        u = FIELD_DP32(u, ID_ISAR6, DP, 0);
> +        u = FIELD_DP32(u, ID_ISAR6, FHM, 0);
> +        cpu->isar.id_isar6 = u;
> +
> +        u = cpu->isar.mvfr1;
> +        u = FIELD_DP32(u, MVFR1, SIMDLS, 0);
> +        u = FIELD_DP32(u, MVFR1, SIMDINT, 0);
> +        u = FIELD_DP32(u, MVFR1, SIMDSP, 0);
> +        u = FIELD_DP32(u, MVFR1, SIMDHP, 0);
> +        u = FIELD_DP32(u, MVFR1, SIMDFMAC, 0);
> +        cpu->isar.mvfr1 = u;
> +
> +        u = cpu->isar.mvfr2;
> +        u = FIELD_DP32(u, MVFR2, SIMDMISC, 0);
> +        cpu->isar.mvfr2 = u;
> +    }
> +
> +    if (!cpu->has_neon && !cpu->has_vfp) {
> +        uint64_t t;
> +        uint32_t u;
> +
> +        t = cpu->isar.id_aa64isar0;
> +        t = FIELD_DP64(t, ID_AA64ISAR0, FHM, 0);
> +        cpu->isar.id_aa64isar0 = t;
> +
> +        t = cpu->isar.id_aa64isar1;
> +        t = FIELD_DP64(t, ID_AA64ISAR1, FRINTTS, 0);
> +        cpu->isar.id_aa64isar1 = t;
> +
> +        u = cpu->isar.mvfr0;
> +        u = FIELD_DP32(u, MVFR0, SIMDREG, 0);
> +        cpu->isar.mvfr0 = u;
> +    }
> +
>      /* Some features automatically imply others: */
>      if (arm_feature(env, ARM_FEATURE_V8)) {
>          if (arm_feature(env, ARM_FEATURE_M)) {
> @@ -1016,12 +1160,6 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>      if (arm_feature(env, ARM_FEATURE_V5)) {
>          set_feature(env, ARM_FEATURE_V4T);
>      }
> -    if (arm_feature(env, ARM_FEATURE_VFP4)) {
> -        set_feature(env, ARM_FEATURE_VFP3);
> -    }
> -    if (arm_feature(env, ARM_FEATURE_VFP3)) {
> -        set_feature(env, ARM_FEATURE_VFP);
> -    }
>      if (arm_feature(env, ARM_FEATURE_LPAE)) {
>          set_feature(env, ARM_FEATURE_V7MP);
>          set_feature(env, ARM_FEATURE_PXN);
> 

I really like the *_feature() API, it is very clean and powerful.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>