Re: [PATCH 1/2] target/arm: Disable VFPv4-D32 when NEON is not available

["Cédric Le Goater" <clg@kaod.org>]

On 9/29/22 01:00, Joel Stanley wrote:
> On Wed, 28 Sept 2022 at 16:47, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> As the Cortex A7 MPCore Technical reference says :
>>
>>    "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."
>>
>> Modify the mvfr0 register value of the cortex A7 to advertise only 16
>> registers when NEON is not available, and not 32 registers.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> 
> 
> 
>> ---
>>   target/arm/cpu.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>> index 7ec3281da9aa..01dc74c32add 100644
>> --- a/target/arm/cpu.c
>> +++ b/target/arm/cpu.c
>> @@ -1684,6 +1684,10 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>           cpu->isar.id_isar6 = u;
>>
>>           if (!arm_feature(env, ARM_FEATURE_M)) {
> 
> Can you explain why the test is put behind the !ARM_FEATURE_M check?

Do you mean the setting of MVFR0 ?

because it was close to the code clearing the SIMD bits (NEON)
of MVFR1 and it seemed more in sync with the specs :

    "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."

(That said, M processors don't have NEON, so this part of the code
should never be reached )

It could be done outside of this test also because SIMDREG = 1 is
a valid value for M processors and the code path  :

     if (!cpu->has_neon && !cpu->has_vfp) {

will set MVFR0 to 0 later on if needed.


M55 seems to be a special case though :

     cpu->isar.mvfr1 = 0x12100211

these are the FPU and MVE bits.

C.

> 
> Reviewed-by: Joel Stanley <joel@jms.id.au>
> 
>> +            u = cpu->isar.mvfr0;
>> +            u = FIELD_DP32(u, MVFR0, SIMDREG, 1); /* 16 registers */
>> +            cpu->isar.mvfr0 = u;
>> +
>>               u = cpu->isar.mvfr1;
>>               u = FIELD_DP32(u, MVFR1, SIMDLS, 0);
>>               u = FIELD_DP32(u, MVFR1, SIMDINT, 0);
>> --
>> 2.37.3
>>