Re: [Qemu-devel] [PATCH 03/12] ARM: Prepare translation for AArch64 code

[Peter Maydell]

On 6 March 2013 10:01, Alexander Graf <address@hidden> wrote:
> @@ -699,25 +740,31 @@ static inline void cpu_clone_regs(CPUARMState *env, 
> target_ulong newsp)
>  static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
>                                          target_ulong *cs_base, int *flags)
>  {
> -    int privmode;
> -    *pc = env->regs[15];
> -    *cs_base = 0;
> -    *flags = (env->thumb << ARM_TBFLAG_THUMB_SHIFT)
> -        | (env->vfp.vec_len << ARM_TBFLAG_VECLEN_SHIFT)
> -        | (env->vfp.vec_stride << ARM_TBFLAG_VECSTRIDE_SHIFT)
> -        | (env->condexec_bits << ARM_TBFLAG_CONDEXEC_SHIFT)
> -        | (env->bswap_code << ARM_TBFLAG_BSWAP_CODE_SHIFT);
> -    if (arm_feature(env, ARM_FEATURE_M)) {
> -        privmode = !((env->v7m.exception == 0) && (env->v7m.control & 1));
> +    if (is_a64(env)) {
> +        *pc = env->pc;
> +        *flags = 0;
>      } else {

This isn't going to work for system emulation mode. We need a bit in the
tb->flags which is "CPU is in AArch64 state". I think most of the existing
TB flag bits will end up zero when in AArch64, but for simplicity I
think we should just define that the flag word layout is the same in
both cases.

> -        privmode = (env->uncached_cpsr & CPSR_M) != ARM_CPU_MODE_USR;
> -    }
> -    if (privmode) {
> -        *flags |= ARM_TBFLAG_PRIV_MASK;
> -    }
> -    if (env->vfp.xregs[ARM_VFP_FPEXC] & (1 << 30)) {
> -        *flags |= ARM_TBFLAG_VFPEN_MASK;
> +        int privmode;
> +        *pc = env->regs[15];
> +        *flags = (env->thumb << ARM_TBFLAG_THUMB_SHIFT)
> +            | (env->vfp.vec_len << ARM_TBFLAG_VECLEN_SHIFT)
> +            | (env->vfp.vec_stride << ARM_TBFLAG_VECSTRIDE_SHIFT)
> +            | (env->condexec_bits << ARM_TBFLAG_CONDEXEC_SHIFT)
> +            | (env->bswap_code << ARM_TBFLAG_BSWAP_CODE_SHIFT);
> +        if (arm_feature(env, ARM_FEATURE_M)) {
> +            privmode = !((env->v7m.exception == 0) && (env->v7m.control & 
> 1));
> +        } else {
> +            privmode = (env->uncached_cpsr & CPSR_M) != ARM_CPU_MODE_USR;
> +        }
> +        if (privmode) {
> +            *flags |= ARM_TBFLAG_PRIV_MASK;
> +        }
> +        if (env->vfp.xregs[ARM_VFP_FPEXC] & (1 << 30)) {
> +            *flags |= ARM_TBFLAG_VFPEN_MASK;
> +        }
>      }
> +
> +    *cs_base = 0;
>  }
>
>  static inline bool cpu_has_work(CPUState *cpu)
> @@ -732,11 +779,15 @@ static inline bool cpu_has_work(CPUState *cpu)
>
>  static inline void cpu_pc_from_tb(CPUARMState *env, TranslationBlock *tb)
>  {
> -    env->regs[15] = tb->pc;
> +    if (is_a64(env)) {
> +        env->pc = tb->pc;
> +    } else {
> +        env->regs[15] = tb->pc;
> +    }

This should be based on the "is AArch64" tb->flags bit rather than the env
field I guess. (Though I think it is not going to be possible to get here
with tb->flags and is_a64(env) giving different answers.)

>  }
>
>  /* Load an instruction and return it in the standard little-endian order */
> -static inline uint32_t arm_ldl_code(CPUARMState *env, uint32_t addr,
> +static inline uint32_t arm_ldl_code(CPUARMState *env, target_ulong addr,
>                                      bool do_swap)
>  {
>      uint32_t insn = cpu_ldl_code(env, addr);
> @@ -747,7 +798,7 @@ static inline uint32_t arm_ldl_code(CPUARMState *env, 
> uint32_t addr,
>  }
>
>  /* Ditto, for a halfword (Thumb) instruction */
> -static inline uint16_t arm_lduw_code(CPUARMState *env, uint32_t addr,
> +static inline uint16_t arm_lduw_code(CPUARMState *env, target_ulong addr,
>                                       bool do_swap)
>  {
>      uint16_t insn = cpu_lduw_code(env, addr);
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index f8838f3..de04a0c 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -9749,7 +9749,7 @@ static inline void 
> gen_intermediate_code_internal(CPUARMState *env,
>      uint16_t *gen_opc_end;
>      int j, lj;
>      target_ulong pc_start;
> -    uint32_t next_page_start;
> +    target_ulong next_page_start;
>      int num_insns;
>      int max_insns;
>
> @@ -9833,24 +9833,26 @@ static inline void 
> gen_intermediate_code_internal(CPUARMState *env,
>          store_cpu_field(tmp, condexec_bits);
>        }
>      do {
> +        if (!is_a64(env)) {

Since we can only flip between AArch64 and AArch32 at exception boundaries
I think that the right place to do the 'if is_a64() { translate_a64_stuff() }'
check is outside the 'for each insn in the block' loop, not inside it.
At that point this code path is only executed for AArch32 and you don't need
this check.

>  #ifdef CONFIG_USER_ONLY
> -        /* Intercept jump to the magic kernel page.  */
> -        if (dc->pc >= 0xffff0000) {
> -            /* We always get here via a jump, so know we are not in a
> -               conditional execution block.  */
> -            gen_exception(EXCP_KERNEL_TRAP);
> -            dc->is_jmp = DISAS_UPDATE;
> -            break;
> -        }
> +            /* Intercept jump to the magic kernel page.  */
> +            if (dc->pc >= 0xffff0000) {
> +                /* We always get here via a jump, so know we are not in a
> +                   conditional execution block.  */
> +                gen_exception(EXCP_KERNEL_TRAP);
> +                dc->is_jmp = DISAS_UPDATE;
> +                break;
> +            }
>  #else
> -        if (dc->pc >= 0xfffffff0 && IS_M(env)) {
> -            /* We always get here via a jump, so know we are not in a
> -               conditional execution block.  */
> -            gen_exception(EXCP_EXCEPTION_EXIT);
> -            dc->is_jmp = DISAS_UPDATE;
> -            break;
> -        }
> +            if (dc->pc >= 0xfffffff0 && IS_M(env)) {
> +                /* We always get here via a jump, so know we are not in a
> +                   conditional execution block.  */
> +                gen_exception(EXCP_EXCEPTION_EXIT);
> +                dc->is_jmp = DISAS_UPDATE;
> +                break;
> +            }
>  #endif
> +        }
>
>          if (unlikely(!QTAILQ_EMPTY(&env->breakpoints))) {
>              QTAILQ_FOREACH(bp, &env->breakpoints, entry) {
> @@ -9904,7 +9906,7 @@ static inline void 
> gen_intermediate_code_internal(CPUARMState *env,
>          }
>
>          if (tcg_check_temp_count()) {
> -            fprintf(stderr, "TCG temporary leak before %08x\n", dc->pc);
> +            fprintf(stderr, "TCG temporary leak before "TARGET_FMT_lx"\n", 
> dc->pc);
>          }
>
>          /* Translation stops when a conditional branch is encountered.
> @@ -10074,6 +10076,10 @@ void cpu_dump_state(CPUARMState *env, FILE *f, 
> fprintf_function cpu_fprintf,
>
>  void restore_state_to_opc(CPUARMState *env, TranslationBlock *tb, int pc_pos)
>  {
> -    env->regs[15] = tcg_ctx.gen_opc_pc[pc_pos];
> +    if (is_a64(env)) {
> +        env->pc = tcg_ctx.gen_opc_pc[pc_pos];
> +    } else {
> +        env->regs[15] = tcg_ctx.gen_opc_pc[pc_pos];
> +    }
>      env->condexec_bits = gen_opc_condexec_bits[pc_pos];
>  }
> --
> 1.6.0.2
>

-- PMM