Re: [Qemu-devel] [PATCH v3 04/11] linux-user: arm: set CPSR.E correctly for BE8 mode

[Peter Crosthwaite]

On Tue, Jun 23, 2015 at 11:54 AM, Peter Maydell
<address@hidden> wrote:
> On 23 June 2015 at 19:43, Peter Crosthwaite
> <address@hidden> wrote:
>> On Tue, Jun 23, 2015 at 1:04 AM, Peter Maydell <address@hidden> wrote:
>>> The Linux userland ABI says:
>>>  (1) the ELF file defines whether an executable is BE8 or not
>>>  (2) this setting affects:
>>>     (a) whether we start at the process entry point in BE or LE
>>>     (b) whether we run signal handlers in BE or LE
>>>     (c) whether newly cloned threads start in BE or LE
>>>
>>> signal_cpsr_e is how this patch implements that -- we set it
>>> based on the ELF file flags, then set CPSR.E based on it:
>>>  * in main, for the initial thread
>>>  * in cpu_clone_regs, for subsequent threads
>
> Aside: this is a bug in the patch which I noted first time
> round with code review -- new threads don't get CPSR.E reset
> like this, they inherit the CPSR.E of the thread they're
> cloned from.
>
>>>  * in signal.c, for signal handlers
>
> This is what the flag is really for.
>
>>> For AArch64 BE we will need something similar. I don't know if
>>> there's somewhere more appropriate to store this "what's the
>>> ELF file endianness" state, but we do need to keep it somewhere...
>>>
>>
>> So my current thinking is the new state captured in TB flags,
>> disas-context and this thing is just a bool for endianess. No sense of
>> CPSR.E or SCTLR.xx in the newly added state across the series. The TB
>> flag is then based on SCTLR.EE, SCTLR.E0E or CPSR.E depending on
>> processor mode. We already have arm_cpu_is_big_endian() to calculate
>> this.
>
> I'm confused. arm_cpu_is_big_endian() tells you whether the CPU
> is *currently* big-endian or not. That doesn't help you with
> answering the question "I'm about to run a signal handler; what
> should I set the CPSR.E bit to?" in linux-user mode. That's
> what signal_cpsr_e does.
>

arm_cpu_is_bigendian is the consumer of this information. We still
need some state for signal_cpsr_e, just the question is what state
does that set. If we reuse arm_cpu_is_big_endian, then signal_cpsr_e
(or its rename) needs to drive CPSR.E as well as SCTLR.E0E.

>> That means that this logic would change signal_cpsr_e to a generic
>> endianess bool that will set both SCTLR_EL1.E0E and CPSR.E at all the
>> points Paolo is patching. SCTLR.EEs shouldn't need patching as
>> usermode shouldn't be affected (maybe add an assert in
>> arm_cpu_big_endian for usermode).
>
> I'm not entirely sure what you're suggesting here, but
> a "generic endianness bool" sounds more confusing than something
> that's specific about exactly what it's trying to control.

Agree, what I am trying to get away from though is using "CPSR" or any
other regs in the naming scheme.

> "endianness for data accesses",

This can work.

 "endianness for code accesses",
> "BE8 vs BE32",

This is probably more complete, as could be an enum for LE, BE8 and BE32.

Regards,
Peter

 "setting of TARGET_WORDS_BIGENDIAN", "endianness
> to use for signal handlers", "exception endianness" and so on
> are all different concepts which can't necessarily be collapsed
> into a single "endianness bool".
>
> thanks
> -- PMM
>