Re: [PATCH 09/13] target/arm/cpu: get endianness from cpu state

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 4/29/25 5:26 AM, Anton Johansson wrote:
> On 29/04/25, Pierrick Bouvier wrote:
>> Remove TARGET_BIG_ENDIAN dependency.
>>
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>   target/arm/cpu.c | 22 +++++++++++-----------
>>   1 file changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>> index e7a15ade8b4..85e886944f6 100644
>> --- a/target/arm/cpu.c
>> +++ b/target/arm/cpu.c
>> @@ -67,6 +67,15 @@ static void arm_cpu_set_pc(CPUState *cs, vaddr value)
>>       }
>>   }
>>   
>> +static bool arm_cpu_is_big_endian(CPUState *cs)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(cs);
>> +    CPUARMState *env = &cpu->env;
>> +
>> +    cpu_synchronize_state(cs);
>> +    return arm_cpu_data_is_big_endian(env);
>> +}
>> +
>>   static vaddr arm_cpu_get_pc(CPUState *cs)
>>   {
>>       ARMCPU *cpu = ARM_CPU(cs);
>> @@ -1130,15 +1139,6 @@ static void arm_cpu_kvm_set_irq(void *opaque, int irq, int level)
>>   #endif
>>   }
>>   
>> -static bool arm_cpu_virtio_is_big_endian(CPUState *cs)
>> -{
>> -    ARMCPU *cpu = ARM_CPU(cs);
>> -    CPUARMState *env = &cpu->env;
>> -
>> -    cpu_synchronize_state(cs);
>> -    return arm_cpu_data_is_big_endian(env);
>> -}
>> -
>>   #ifdef CONFIG_TCG
>>   bool arm_cpu_exec_halt(CPUState *cs)
>>   {
>> @@ -1203,7 +1203,7 @@ static void arm_disas_set_info(CPUState *cpu, disassemble_info *info)
>>   
>>       info->endian = BFD_ENDIAN_LITTLE;
>>       if (bswap_code(sctlr_b)) {
>> -        info->endian = TARGET_BIG_ENDIAN ? BFD_ENDIAN_LITTLE : BFD_ENDIAN_BIG;
>> +        info->endian = arm_cpu_is_big_endian(cpu) ? BFD_ENDIAN_LITTLE : BFD_ENDIAN_BIG;
>>       }
> 
> I'm not the most familiar with arm but as far as I can tell these are
> not equivalent. My understanding is that arm_cpu_is_big_endian() models
> data endianness, and TARGET_BIG_ENDIAN instruction endianness.
> 
> Also, for user mode where this branch is relevant, bswap_code() still
> depends on TARGET_BIG_ENDIAN anyway and the above branch would reduce to
> (on arm32)
> 
>    if (TARGET_BIG_ENDIAN ^ sctlr_b) {
>        info->endian = sctlr_b ? BFD_ENDIAN_LITTLE : BFD_ENDIAN_BIG;
>    }
> 
> giving the opposite result to the original code.
> 

Ooops, that's a good point, I missed it was calling 
arm_cpu_data_is_big_endian under the hoods.

I'll stick to target_big_endian().

Thanks,
Pierrick