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

[Paolo Bonzini]

Il 29/05/2014 22:21, Peter Maydell ha scritto:
> On 29 May 2014 20:46, Paolo Bonzini <address@hidden> wrote:
> > Now that CPSR.E is set correctly, prepare for when setend will be able
> > to change it; bswap data in and out of strex manually by comparing
> > bswap_code to CPSR.E (we do not have the luxury of using TCGMemOps).
> >
> > Signed-off-by: Paolo Bonzini <address@hidden>
> > ---
> >  linux-user/main.c | 50 +++++++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 43 insertions(+), 7 deletions(-)
> >
> > diff --git a/linux-user/main.c b/linux-user/main.c
> > index 8eb910a..b129a2b 100644
> > --- a/linux-user/main.c
> > +++ b/linux-user/main.c
> > @@ -449,6 +449,38 @@ void cpu_loop(CPUX86State *env)
> >          __r;                                            \
> >      })
> >
> > +#define get_user_data_u32(x, gaddr, env)                \
> > +    ({ abi_long __r = get_user_u32((x), (gaddr));       \
> > +        if (!__r && (env)->bswap_code != !!((env)->uncached_cpsr & CPSR_E)) { \
> > +            (x) = bswap32(x);                           \
> > +        }                                               \
> > +        __r;                                            \
> > +    })
>
> 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.

Instead, BE32 is handled incorrectly for TCG-generated accesses.  I'll 
fix it for v3.

Paolo