Re: [PATCH 15/25] target/arm: Allow users to set the number of VFP registers

[Cédric Le Goater]

On 1/19/23 13:34, Cédric Le Goater wrote:
> Cortex A7 CPUs with an FPU implementing VFPv4 without NEON support
> have 16 64-bit FPU registers and not 32 registers. Let users set the
> number of VFP registers with a CPU property.
>
> The primary use case of this property is for the Cortex A7 of the
> Aspeed AST2600 SoC.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   target/arm/cpu.h        |  2 ++
>   hw/arm/aspeed_ast2600.c |  2 ++
>   target/arm/cpu.c        | 31 +++++++++++++++++++++++++++++++
>   3 files changed, 35 insertions(+)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index bf2bce046d..5f2fefef4e 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -901,6 +901,8 @@ struct ArchCPU {
>       bool has_pmu;
>       /* CPU has VFP */
>       bool has_vfp;
> +    /* CPU has 32 VFP registers */
> +    bool has_vfp_d32;
>       /* CPU has Neon */
>       bool has_neon;
>       /* CPU has M-profile DSP extension */
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index cd75465c2b..37f43b4165 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -309,6 +309,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, 
> Error **errp)
>                                   &error_abort);
>           object_property_set_bool(OBJECT(&s->cpu[i]), "neon", false,
>                                   &error_abort);
> +        object_property_set_bool(OBJECT(&s->cpu[i]), "vfp-d32", false,
> +                                &error_abort);
>           object_property_set_link(OBJECT(&s->cpu[i]), "memory",
>                                    OBJECT(s->memory), &error_abort);
>   
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 5f63316dbf..ad90de75fb 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1269,6 +1269,9 @@ static Property arm_cpu_cfgend_property =
>   static Property arm_cpu_has_vfp_property =
>               DEFINE_PROP_BOOL("vfp", ARMCPU, has_vfp, true);
>   
> +static Property arm_cpu_has_vfp_d32_property =
> +            DEFINE_PROP_BOOL("vfp-d32", ARMCPU, has_vfp_d32, true);
> +
>   static Property arm_cpu_has_neon_property =
>               DEFINE_PROP_BOOL("neon", ARMCPU, has_neon, true);
>   
> @@ -1400,6 +1403,14 @@ void arm_cpu_post_init(Object *obj)
>           }
>       }
>   
> +    if (cpu->has_vfp && cpu_isar_feature(aa32_simd_r32, cpu)) {
> +        cpu->has_vfp_d32 = true;
> +        if (!kvm_enabled()) {
> +            qdev_property_add_static(DEVICE(obj),
> +                                     &arm_cpu_has_vfp_d32_property);
> +        }
> +    }
> +
>       if (arm_feature(&cpu->env, ARM_FEATURE_NEON)) {
>           cpu->has_neon = true;
>           if (!kvm_enabled()) {
> @@ -1661,6 +1672,26 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>           return;
>       }
>   
> +    if (!cpu->has_vfp_d32 &&
> +        arm_feature(env, ARM_FEATURE_V8) &&
> +        !arm_feature(env, ARM_FEATURE_M)) {
> +        error_setg(errp, "ARMv8A CPUs must have VFP32");
> +        return;
> +    }

The specs say that the permitted values of the SIMDReg bits [3:0]
in Armv8-A are 0b0000 and 0b0010. In Armv8-M, the possible values
of the field are b0001. All other values are reserved.

This should be better :
        
        if (arm_feature(env, ARM_FEATURE_V8)) {
            if (arm_feature(env, ARM_FEATURE_M)) {
                if (cpu->has_vfp_d32) {
                    error_setg(errp, "ARMv8-M CPUs do not support VFP-D32");
                    return;
                }
            } else if (!cpu->has_vfp_d32) {
                error_setg(errp, "ARMv8-A CPUs must have VFP-D32");
                return;
            }
        }

?

Thanks,

C.

> +    if (cpu->has_vfp_d32 != cpu->has_neon) {
> +        error_setg(errp, "ARM CPUs must have both VFP32 and Neon or neither");
> +        return;
> +    }
> +
> +   if (!cpu->has_vfp_d32) {
> +        uint32_t u;
> +
> +        u = cpu->isar.mvfr0;
> +        u = FIELD_DP32(u, MVFR0, SIMDREG, 1); /* 16 registers */
> +        cpu->isar.mvfr0 = u;
> +    }
> +
>       if (!cpu->has_vfp) {
>           uint64_t t;
>           uint32_t u;