Re: [PATCH v2 03/10] target/arm: Set disassemble_info::endian value in disas_set_info()

[Richard Henderson <richard.henderson@linaro.org>]

On 2/10/25 14:59, Philippe Mathieu-Daudé wrote:
> On 10/2/25 23:10, Richard Henderson wrote:
>> On 2/10/25 13:29, Philippe Mathieu-Daudé wrote:
>>> Have the CPUClass::disas_set_info() callback set the
>>> disassemble_info::endian field.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   target/arm/cpu.c | 10 +++-------
>>>   1 file changed, 3 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>>> index 94f1c55622b..68b3a9d3ab0 100644
>>> --- a/target/arm/cpu.c
>>> +++ b/target/arm/cpu.c
>>> @@ -1188,7 +1188,7 @@ static void arm_disas_set_info(CPUState *cpu, disassemble_info 
>>> *info)
>>>   {
>>>       ARMCPU *ac = ARM_CPU(cpu);
>>>       CPUARMState *env = &ac->env;
>>> -    bool sctlr_b;
>>> +    bool sctlr_b = arm_sctlr_b(env);
>>>       if (is_a64(env)) {
>>>           info->cap_arch = CS_ARCH_ARM64;
>>> @@ -1215,13 +1215,9 @@ static void arm_disas_set_info(CPUState *cpu, disassemble_info 
>>> *info)
>>>           info->cap_mode = cap_mode;
>>>       }
>>> -    sctlr_b = arm_sctlr_b(env);
>>> +    info->endian = BFD_ENDIAN_LITTLE;
>>>       if (bswap_code(sctlr_b)) {
>>> -#if TARGET_BIG_ENDIAN
>>> -        info->endian = BFD_ENDIAN_LITTLE;
>>> -#else
>>> -        info->endian = BFD_ENDIAN_BIG;
>>> -#endif
>>> +        info->endian = TARGET_BIG_ENDIAN ? BFD_ENDIAN_LITTLE : BFD_ENDIAN_BIG;
>>>       }
>>>       info->flags &= ~INSN_ARM_BE32;
>>>   #ifndef CONFIG_USER_ONLY
>>
>> This is a faithful adjustment to the existing code, so,
>>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>
>> However:
>>
>> (1) aarch64 code is always little-endian,
>> (2) sctlr_b is always false from armv7 (and thus always false for aarch64)
>> (3) I think the BE32 logic is wrong -- CONFIG_USER_ONLY is irrelevant.
>>      See linux-user/arm/cpu_loop.c, target_cpu_copy_regs.
> 
> What about v7-R [*]? I don't see SCTLR_IE defined as 1<<31 for AArch32,
> only:

BE32 was a really old arm thingy, and I it was removed in armv7 (see arm_sctlr_b).
With BE8 (armv6+), instructions are always little-endian, only data accesses change.

> For the Cortex-r5* we have, SCTLR_IE is always 0 in reset_sctlr.
> 
> Is it OK to consider v7-R implemented as little-endian in QEMU?

Yes.


r~