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

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 <pbonzini@redhat.com> 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

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

[Paolo Bonzini]

Il 02/06/2014 18:17, Peter Maydell ha scritto:
> 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.)

So basically it's s/bswap_code/!sctlr_b/.  Thanks for the suggestion!

Paolo

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

[Peter Maydell]

On 3 June 2014 10:23, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 02/06/2014 18:17, Peter Maydell ha scritto:
>
>> 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.)
>
>
> So basically it's s/bswap_code/!sctlr_b/.  Thanks for the suggestion!

Actually I was thinking again about this last night, and
user-mode is a bit odd, since it's the only place we
ever have TARGET_WORDS_BIGENDIAN set.

System emulation: TARGET_WORDS_BIGENDIAN indicates the
"bus endianness", always LE for ARM.
User emulation: TARGET_WORDS_BIGENDIAN indicates the
"kernel API endianness", so may be BE or LE.

In system emulation it's clear how we should implement
things: TARGET_WORDS_BIGENDIAN is never set, data
accesses honour CPSR.E by doing MO_BE or MO_LE accesses,
instruction accesses are always MO_LE, and SCTLR.B
is implemented by XORing the address with 3 (for
byte accesses) or 1 (for halfword accesses).
[SCTLR.B is word-invariant big-endian, hence the need
to use XORs. In practice since there's not much interest
in system emulation of BE32 we can leave it unimplemented.]

In user emulation, things are more complicated for BE32,
because we're sort of emulating the word-invariant
bigendian using byte-invariant big-endian (this is
safe because there's no way for a userspace program
to get at anything that would let it tell the
difference). So we can't just say "set SCTLR.B
and handle as if SCTLR.B is set in the way system
emulation would", because the behaviour has to
be different.

So in summary I'm not sure of the right approach
any more...

thanks
-- PMM

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

[Paolo Bonzini]

Il 03/06/2014 11:54, Peter Maydell ha scritto:
> In user emulation, things are more complicated for BE32,
> because we're sort of emulating the word-invariant
> bigendian using byte-invariant big-endian (this is
> safe because there's no way for a userspace program
> to get at anything that would let it tell the
> difference). So we can't just say "set SCTLR.B
> and handle as if SCTLR.B is set in the way system
> emulation would", because the behaviour has to
> be different.
> 
> So in summary I'm not sure of the right approach
> any more...

I think overall sctlr_b makes for more accurate and overall
clearer code.

Here are the functions I'm using to map between the various properties:

+static inline bool bswap_code(bool sctlr_b)
+{
+#ifdef CONFIG_USER_ONLY
+    /* Mixed-endian modes are BE8 (SCTLR.B = 0, TARGET_WORDS_BIGENDIAN = 1)
+     * and LE8 (SCTLR.B = 1, TARGET_WORDS_BIGENDIAN = 0).
+     */
+    return
+#ifdef TARGET_WORDS_BIGENDIAN
+        1 ^
+#endif
+        sctlr_b;
+#else
+    /* We do not implement BE32 mode for system-mode emulation, but
+     * anyway it would always do little-endian accesses with
+     * TARGET_WORDS_BIGENDIAN = 0.
+     */
+    return 0;
+#endif
+}
+
+#ifdef CONFIG_USER_ONLY
+static inline bool arm_cpu_bswap_data(CPUARMState *env)
+{
+    return
+#ifdef TARGET_WORDS_BIGENDIAN
+       1 ^
+#endif
+       !!(env->cp15.c1_sys & SCTLR_B) ^
+       !!(env->uncached_cpsr & CPSR_E);
+}
+#endif
+
+static inline bool arm_tbflag_is_data_be(unsigned tbflags)
+{
+    return
+#ifdef CONFIG_USER_ONLY
+        ARM_TBFLAG_SCTLR_B(tbflags) ^
+#endif
+        ARM_TBFLAG_CPSR_E(tbflags);
+}
+

I think this is reasonably close to what you would have for SCTLR.B
emulation, only the XORing of addresses is missing.

bswap_code is used in much fewer places than the current env->bswap_code,
basically only in the definitions of arm_ld*_code and get_user_code_*.
Everywhere else the code is accessing SCTLR.B, which is "real"
architectural state.  The confusing manner of handling it in user-mode
emulation is wrapped by the above three inline functions.

Paolo

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

[Paolo Bonzini]

Il 03/06/2014 11:54, Peter Maydell ha scritto:
> In system emulation it's clear how we should implement
> things: TARGET_WORDS_BIGENDIAN is never set, data
> accesses honour CPSR.E by doing MO_BE or MO_LE accesses,
> instruction accesses are always MO_LE, and SCTLR.B
> is implemented by XORing the address with 3 (for
> byte accesses) or 1 (for halfword accesses).

I think it's 2 for halfword accesses. :)

What about 64-bit accesses?  Does the processor swap the two words of 
the result?

Paolo

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

[Peter Maydell]

On 4 June 2014 08:48, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 03/06/2014 11:54, Peter Maydell ha scritto:
>>
>> In system emulation it's clear how we should implement
>>
>> things: TARGET_WORDS_BIGENDIAN is never set, data
>> accesses honour CPSR.E by doing MO_BE or MO_LE accesses,
>> instruction accesses are always MO_LE, and SCTLR.B
>> is implemented by XORing the address with 3 (for
>> byte accesses) or 1 (for halfword accesses).

> I think it's 2 for halfword accesses. :)

Doh :-)

> What about 64-bit accesses?  Does the processor swap
> the two words of the result?

Yes. (There's a helpful table describing this in
section O.3.2 (in Appendix O) of the v7 ARM ARM.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 0/8] target-arm/linux-user-arm: implement setend, CPSR.E, SCTLR.EE

[Stefan Weil]

Am 29.05.2014 um 21:46 schrieb Paolo Bonzini:
> This is v2 of the series to implement CPSR.E.  I've now included support
> for strex emulation and SCTLR.EE.  I've now also tested system emulation
> better using an Arch Linux image.
>
> Using the Raspberry Pi optimized memcmp library and associated harness,
> QEMU 2.0 fails with "Illegal instruction", while these patches work.
>
> Paolo
>
> Paolo Bonzini (8):
>    linux-user: arm: fix coding style for some linux-user signal functions
>    linux-user: arm: set CPSR.E correctly for BE8 mode
>    linux-user: arm: pass env to get_user_code_*
>    linux-user: arm: handle CPSR.E correctly in strex emulation
>    target-arm: implement SCTLR.EE
>    target-arm: pass DisasContext to gen_aa32_ld*/st*
>    target-arm: introduce be8 tbflag
>    target-arm: implement setend
>
>   linux-user/main.c      |  71 +++++++++---
>   linux-user/signal.c    | 122 +++++++++++---------
>   target-arm/cpu.h       |   7 ++
>   target-arm/helper.c    |  38 +++++-
>   target-arm/helper.h    |   1 +
>   target-arm/op_helper.c |   5 +
>   target-arm/translate.c | 307 +++++++++++++++++++++++++------------------------
>   target-arm/translate.h |   1 +
>   8 files changed, 322 insertions(+), 230 deletions(-)

I'd appreciate if this series could be applied to git master, but it 
looks like it needs to be rebased and fixed for the last few patches.

Regards
Stefan

Re: [Qemu-devel] [PATCH v2 0/8] target-arm/linux-user-arm: implement setend, CPSR.E, SCTLR.EE

[Paolo Bonzini]

On 31/10/2014 21:52, Stefan Weil wrote:
> I'd appreciate if this series could be applied to git master, but it
> looks like it needs to be rebased and fixed for the last few patches.

Peter, I'm rebasing this and I'm unsure how SCTLR.EE should behave wrt
EL2.  When setting CPSR_E from SCTLR.EE, should I use
A32_BANKED_CURRENT_REG_GET or env->sctlr[MAX(arm_current_el(env), 1])?
And similarly, for the page-table walks does EL2 use the EL3 (sctlr_s)
or EL2 (hsctlr) value?

Thanks,

Paolo

Re: [Qemu-devel] [PATCH v2 0/8] target-arm/linux-user-arm: implement setend, CPSR.E, SCTLR.EE

[Peter Maydell]

On 5 February 2015 at 17:33, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Peter, I'm rebasing this and I'm unsure how SCTLR.EE should behave wrt
> EL2.  When setting CPSR_E from SCTLR.EE, should I use
> A32_BANKED_CURRENT_REG_GET or env->sctlr[MAX(arm_current_el(env), 1])?

This is for exception entry, yes? In the 32-bit exception entry
code path (arm_cpu_do_interrupt()) you want A32_BANKED_CURRENT_REG_GET
(and make sure you do it after we've switched mode/EL, not before).
In the 64-bit exception entry path there's no equivalent thing to
do because there's no CPSR.E in AArch64 (endianness is always
determined by the currently operative SCTLR.EE or SCTLR.E0E bit);
but your patchset isn't trying to address AArch64 in any case.

> And similarly, for the page-table walks does EL2 use the EL3 (sctlr_s)
> or EL2 (hsctlr) value?

For pagetable walks see the patchseries I just committed to master.
Table D4-1 in the v8 ARM ARM defines the semantics required, which
in QEMU terms is to check regime_sctlr(env, mmu_idx) & SCTLR_EE.

thanks
-- PMM