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

[Cédric Le Goater]

On 6/7/23 13:06, Joel Stanley wrote:
> On Wed, 7 Jun 2023 at 04:40, 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>
>
> You saw a crash with a buildroot image without this change, as I recall?

yes, when compiled with VFPv4d32 support, user space crashes on real HW.

Thanks,

C.

> The logic is a bit hard to follow but it is good to see a fix.
>
> Reviewed-by: Joel Stanley <joel@jms.id.au>
>
> > ---
> >   target/arm/cpu.h        |  2 ++
> >   hw/arm/aspeed_ast2600.c |  2 ++
> >   target/arm/cpu.c        | 32 ++++++++++++++++++++++++++++++++
> >   3 files changed, 36 insertions(+)
> >
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index d469a2637b32..79f1a96ddf39 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -916,6 +916,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 1bf12461481c..a8b3a8065a11 100644
> > --- a/hw/arm/aspeed_ast2600.c
> > +++ b/hw/arm/aspeed_ast2600.c
> > @@ -316,6 +316,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 5182ed0c9113..74fe6ae78192 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -1275,6 +1275,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);
> >
> > @@ -1406,6 +1409,22 @@ 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()) {
> > +            /*
> > +             * The permitted values of the SIMDReg bits [3:0] on
> > +             * Armv8-A are either 0b0000 and 0b0010. On such CPUs,
> > +             * make sure that has_vfp_d32 can not be set to false.
> > +             */
> > +            if (!(arm_feature(&cpu->env, ARM_FEATURE_V8) &&
> > +                  !arm_feature(&cpu->env, ARM_FEATURE_M))) {
> > +                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()) {
> > @@ -1672,6 +1691,19 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
> > **errp)
> >           return;
> >       }
> >
> > +    if (cpu->has_vfp_d32 != cpu->has_neon) {
> > +        error_setg(errp, "ARM CPUs must have both VFP-D32 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;
> > --
> > 2.40.1