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

[Peter Maydell <peter.maydell@linaro.org>]

On 21 June 2014 13:58, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Set it on startup, in signal handler frames and in new threads.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  linux-user/arm/target_cpu.h | 2 ++
>  linux-user/main.c           | 3 ++-
>  linux-user/signal.c         | 2 ++
>  target-arm/cpu.h            | 3 +++
>  4 files changed, 9 insertions(+), 1 deletions(-)
>
> diff --git a/linux-user/arm/target_cpu.h b/linux-user/arm/target_cpu.h
> index 39d65b6..d0411c7 100644
> --- a/linux-user/arm/target_cpu.h
> +++ b/linux-user/arm/target_cpu.h
> @@ -25,6 +25,8 @@ static inline void cpu_clone_regs(CPUARMState *env, target_ulong newsp)
>          env->regs[13] = newsp;
>      }
>      env->regs[0] = 0;
> +    env->uncached_cpsr &= ~CPSR_E;
> +    env->uncached_cpsr |= env->signal_cpsr_e;

Where does this come from? I can't see anything in the
kernel's handling of clone that changes CPSR.E...
http://lxr.free-electrons.com/source/arch/arm/kernel/process.c#L346

(There is code for handling CPSR_E in the kernel's start_thread()
macro but that is actually only called for starting new
processes, AFAICT.)

>  }
>
>  static inline void cpu_set_tls(CPUARMState *env, target_ulong newtls)
> diff --git a/linux-user/main.c b/linux-user/main.c
> index dbaa42a..795a407 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -4195,7 +4195,8 @@ int main(int argc, char **argv, char **envp)
>          /* Enable BE8.  */
>          if (EF_ARM_EABI_VERSION(info->elf_flags) >= EF_ARM_EABI_VER4
>              && (info->elf_flags & EF_ARM_BE8)) {
> -            /* nothing for now, CPSR.E not emulated yet */
> +            env->uncached_cpsr |= CPSR_E;
> +            env->signal_cpsr_e = CPSR_E;
>          } else {
>              if (arm_feature(env, ARM_FEATURE_V7)) {
>                  fprintf(stderr, "BE32 binaries only supported until ARMv6\n");
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 624c34d..87ddabd 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -1609,6 +1609,8 @@ setup_return(CPUARMState *env, struct target_sigaction *ka,
>          cpsr &= ~CPSR_T;
>      }
>
> +    cpsr |= env->signal_cpsr_e;

This won't clear CPSR.E if it happened to be set;
you should probably make the earlier 'cpsr &= ~CPSR_IT;'
be 'cpsr &= ~(CPSR_IT | CPSR_E);'

> +
>      if (ka->sa_flags & TARGET_SA_RESTORER) {
>          retcode = ka->sa_restorer;
>      } else {
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 4a9d2a8..cb5be84 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -316,6 +316,9 @@ typedef struct CPUARMState {
>  #if defined(CONFIG_USER_ONLY)
>      /* For usermode syscall translation.  */
>      int eabi;
> +
> +    /* CPSR.E value for new threads and signal handlers.  */

I think "new threads and" should be deleted here, see above.

> +    uint32_t signal_cpsr_e;
>  #endif
>
>      CPU_COMMON
> --
> 1.9.3

thanks
-- PMM