Re: [Qemu-devel] [PATCH v2 4/8] linux-user: arm: handle CPSR.E correctly in strex emulation

[Peter Maydell]

On 30 May 2014 07:52, Paolo Bonzini <address@hidden> wrote:
> Il 29/05/2014 22:21, Peter Maydell ha scritto:
>> This looks bogus. bswap_code doesn't have anything to do
>> with whether data should be byteswapped. Consider the
>> ARMv5 big-endian code, which qemu-armeb also supports:
>> there both code and data are big-endian, and bswap_code
>> is false. bswap_code should only be consulted for iside
>> accesses.
>
> This is correct.  It is not very intuitive, but a XOR does exactly what we
> want.
>
> For v3 I'll wrap it into an inline function like this:
>
> /* get_user and put_user respectivaly return and expect data according
>  * to TARGET_WORDS_BIGENDIAN, but ldrex/strex emulation needs to take
>  * into account CPSR.E too.  It turns out that a XOR gives exactly the
>  * required result:
>  *
>  *             TARGET_WORDS_BIGENDIAN  bswap_code  CPSR.E    need swap?
>  *   LE/LE               no                0        0          no
>  *   LE/BE               no                0        1          yes
>  *   BE8/LE              yes               1        0          yes
>  *   BE8/BE              yes               1        1          no
>  *   BE32/BE             yes               0        0          no
>  */
> static inline bool swap_get_put_user(CPUARMState *env)
> {
>     return env->bswap_code ^ !!(env->uncached_cpsr & CPSR_E);
> }
>
> The reason is that bswap_code is about more than just code endianness, it
> affects get_user as well.  It more generally means "is the endianness given
> by TARGET_WORDS_BIGENDIAN the opposite of what we get at CPSR.E=0?", and it
> affects the setting of CPSR.E=1 in the signal handlers, as you pointed out
> in the review of patch 2.  I'll prepend a patch that renames bswap_code to
> be8_user, since in the end BE8 binaries on qemu-armeb are the only case
> where it is true.

Um. This feels like we're wrongly overloading this flag for
more than one thing. "Is the user-mode binary BE8?" is
definitely not a property of the CPU, so it shouldn't be
a CPU state flag. (Conversely, "is the iside endianness the
opposite way round to TARGET_WORDS_BIGENDIAN" is a CPU
property of sorts.) It seems to me that we probably want
to fix this by more correctly modelling the actual CPU
state involved here, by having user-mode either set or
not set SCTLR.B [set only if BE32 binary], and the data
and insn fetches honour both that and CPSR.E appropriately.)

thanks
-- PMM