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

[Peter Maydell]

On Mon, 2 Jan 2023 at 17:52, Cédric Le Goater <clg@kaod.org> 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>

> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 2fa022f62b..27af57ea9a 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1258,6 +1258,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);
>
> @@ -1384,8 +1387,11 @@ void arm_cpu_post_init(Object *obj)
>          ? cpu_isar_feature(aa64_fp_simd, cpu)
>          : cpu_isar_feature(aa32_vfp, cpu)) {
>          cpu->has_vfp = true;
> +        cpu->has_vfp_d32 = true;

We shouldn't default to true if the CPU default is D16 already.
You should be able to use cpu_isar_feature(aa32_simd_r32, cpu)
to check this.

The setup of the property should probably be in its own if(),
rather than tucked into the has_vfp if(), so

   if (cpu->has_vfp && cpu_isar_feature(aa32_simd_r32, cpu)) {
       cpu->has_vfp_d32 = true;
       add property;
   }

This would mean that if the CPU is D16-only then the user can't
make it D32, which I think is OK. You could write it to
allow that if you wanted I guess, but then you'd need to
special-case M-profile, which doesn't permit D32 at all.

>          if (!kvm_enabled()) {
>              qdev_property_add_static(DEVICE(obj), &arm_cpu_has_vfp_property);
> +            qdev_property_add_static(DEVICE(obj),
> +                                     &arm_cpu_has_vfp_d32_property);
>          }
>      }
>
> @@ -1650,6 +1656,14 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>          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;

There should be a check so the user can't both disable D32 and enable Neon
(Neon always has 32 dregs).

Armv8A doesn't permit D16, so we shouldn't allow that combination either
(but note that v8R, support for which just landed, *does* permit it).

thanks
-- PMM