Re: [RFC PATCH] ast2600: Fix CPU features

[Cédric Le Goater]

On 9/27/22 03:49, Joel Stanley wrote:
> On Mon, 26 Sept 2022 at 07:05, Cédric Le Goater <clg@kaod.org> wrote:
> > On 9/26/22 08:26, Cédric Le Goater wrote:
> > > Currently, the CPU features exposed to the AST2600 QEMU machines are :
> > >
> > >     half thumb fastmult vfp edsp neon vfpv3 tls vfpv4 idiva idivt
> > >     vfpd32 lpae evtstrm
> > >
> > > But, the features of the Cortex A7 CPU on the Aspeed AST2600 A3 SoC
> > > are :
> > >
> > >     half thumb fastmult vfp edsp vfpv3 vfpv3d16 tls vfpv4 idiva idivt
> > >     lpae evtstrm
> > >
> > > The vfpv3d16 feature bit is common to both vfpv3 and vfpv4, and for
> > > this SoC, QEMU should advertise a VFPv4 unit with 16 double-precision
> > > registers, and not 32 registers.
> > >
> > > Drop neon support and hack the default mvfr0 register value of the
> > > cortex A7 to advertise 16 registers.
> > >
> > > How can that be done cleanly ? Should we :
> > >
> > >    * introduce a new A7 CPU with its own _initfn routine ?
> > >    * introduce a new CPU property to set the number of "Advanced SIMD
> > >      and floating-point" registers in arm_cpu_realizefn() ?
> >
> > This is a note in the Cortex A7 MPCore Technical reference saying :
> >
> > "When FPU option is selected without NEON, the FPU is VFPv4-D16 and uses 16
> > double-precision registers. When the FPU is implemented with NEON, the FPU is
> > VFPv4-D32 and uses 32 double-precision registers. This register bank is shared
> > with NEON."
>
> The datasheet only has this to say:
>
> "1.2GHz dual-core ARM Cortex A7 (r0p5) 32-bit CPU with FPU"
>
> With no details about the FPU. The hardware is a golden reference though:
>
>   fpsid: 41023075
>   mvfr0: 10110221
>   mvfr1: 11000011
>
> $ bitfield mvfr0 0x10110221
> decoding as Media and VFP Feature Register 0
> 0x10110221 [269550113]
>        A_SIMD registers: 0x1 [16 x 64-bit registers]
>        Single precision: 0x2 [Supported, VFPv4 or VFPv3]
>        Double precision: 0x2 [Supported, VFPv4 or VFPv3]
> VFP exception trapping: 0x0 [Not supported]
>                  Divide: 0x1 [Hardware divide is supported]
>             Square Root: 0x1 [Hardware square root supported]
>           Short vectors: 0x0 [Not supported]
>      VFP Rounding Modes: 0x1 [All modes supported]
>
> $ bitfield mvfr1 0x11000011
> decoding as Media and VFP Feature Register 1
> 0x11000011 [285212689]
>                 FZ: 0x1
>         D_NaN mode: 0x1
> A_SIMD load/store: 0x0
>     A_SIMD integer: 0x0
>        A_SIMD SPFP: 0x0
>        A_SIMD HPFP: 0x0
>           VFP HPFP: 0x2
>        A_SIMD FMAC: 0x1
>
> As you say, no NEON  and 16 64-bit registers.
>
> > Could we deduce the number of registers from the availability of the NEON
> > feature, on A7 only ?
>
> We certainly should make the NEON property match the mvfr1 value.
> Linux tests for NEON with this:
>
>     (fmrx(MVFR1) & 0x000fff00) == 0x00011100)
>
> https://elixir.bootlin.com/linux/v5.19/source/arch/arm/vfp/vfpmodule.c#L812


ok. I will resend with 2 patches. An obvious first one removing NEON
from the AsT2600 SoC and a second decreasing the number of registers
to 16 when NEON is off.


Thanks,

C.

> Cheers,
>
> Joel
>
> > > This problem was raised by a buildroot rootfs compiled with vfpv4.
> > > Boot went fine under QEMU but on real HW, user space binaries had
> > > issues with output. Compiling buildroot with vfpv4d16 fixed it and
> > > I didn't dig further. Nevertheless, it would be nice to catch such
> > > issues with QEMU.
> > >
> > > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > > ---
> > >    hw/arm/aspeed_ast2600.c | 2 ++
> > >    target/arm/cpu_tcg.c    | 2 +-
> > >    2 files changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> > > index dcdc9bc54456..af987fd418ec 100644
> > > --- a/hw/arm/aspeed_ast2600.c
> > > +++ b/hw/arm/aspeed_ast2600.c
> > > @@ -330,6 +330,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, 
> > > Error **errp)
> > >
> > >            object_property_set_int(OBJECT(&s->cpu[i]), "cntfrq", 1125000000,
> > >                                    &error_abort);
> > > +        object_property_set_bool(OBJECT(&s->cpu[i]), "neon", false,
> > > +                                &error_abort);
> > >            object_property_set_link(OBJECT(&s->cpu[i]), "memory",
> > >                                     OBJECT(s->memory), &error_abort);
> > >
> > > diff --git a/target/arm/cpu_tcg.c b/target/arm/cpu_tcg.c
> > > index 98b5ba216041..b3f93783a061 100644
> > > --- a/target/arm/cpu_tcg.c
> > > +++ b/target/arm/cpu_tcg.c
> > > @@ -545,7 +545,7 @@ static void cortex_a7_initfn(Object *obj)
> > >        cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A7;
> > >        cpu->midr = 0x410fc075;
> > >        cpu->reset_fpsid = 0x41023075;
> > > -    cpu->isar.mvfr0 = 0x10110222;
> > > +    cpu->isar.mvfr0 = 0x10110221; /* SIMDREG == 0x1 -> 16 registers */
> > >        cpu->isar.mvfr1 = 0x11111111;
> > >        cpu->ctr = 0x84448003;
> > >        cpu->reset_sctlr = 0x00c50078;