qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] target-arm: Clean up DISAS_UPDATE usage in AArc


From: Peter Maydell
Subject: Re: [Qemu-devel] [PATCH] target-arm: Clean up DISAS_UPDATE usage in AArch32 translation code
Date: Fri, 6 Nov 2015 12:46:29 +0000

On 2 November 2015 at 18:16, Sergey Fedorov <address@hidden> wrote:
> AArch32 translation code does not distinguish between DISAS_UPDATE and
> DISAS_JUMP. Thus, we cannot use any of them without first updating PC in
> CPU state. Furthermore, it is too complicated to update PC in CPU state
> before PC gets updated in disas context. So it is hardly possible to
> correctly end TB early if is is not likely to be executed before calling
> disas_*_insn(), e.g. just after calling breakpoint check helper.
>
> Modify DISAS_UPDATE and DISAS_JUMP usage in AArch32 translation and
> apply to them the same semantic as AArch64 translation does:
>  - DISAS_UPDATE: update PC in CPU state when finishing translation
>  - DISAS_JUMP:   preserve current PC value in CPU state when finishing
>                  translation
>
> Signed-off-by: Sergey Fedorov <address@hidden>
> ---
>  target-arm/translate.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 9f1d740..ec740fd 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -870,7 +870,7 @@ static inline void gen_bx_im(DisasContext *s, uint32_t 
> addr)
>  {
>      TCGv_i32 tmp;
>
> -    s->is_jmp = DISAS_UPDATE;
> +    s->is_jmp = DISAS_JUMP;
>      if (s->thumb != (addr & 1)) {
>          tmp = tcg_temp_new_i32();
>          tcg_gen_movi_i32(tmp, addr & 1);
> @@ -883,7 +883,7 @@ static inline void gen_bx_im(DisasContext *s, uint32_t 
> addr)
>  /* Set PC and Thumb state from var.  var is marked as dead.  */
>  static inline void gen_bx(DisasContext *s, TCGv_i32 var)
>  {
> -    s->is_jmp = DISAS_UPDATE;
> +    s->is_jmp = DISAS_JUMP;
>      tcg_gen_andi_i32(cpu_R[15], var, ~1);
>      tcg_gen_andi_i32(var, var, 1);
>      store_cpu_field(var, thumb);
> @@ -1062,7 +1062,7 @@ static void gen_exception_insn(DisasContext *s, int 
> offset, int excp,
>  static inline void gen_lookup_tb(DisasContext *s)
>  {
>      tcg_gen_movi_i32(cpu_R[15], s->pc & ~1);
> -    s->is_jmp = DISAS_UPDATE;
> +    s->is_jmp = DISAS_JUMP;
>  }
>
>  static inline void gen_add_data_offset(DisasContext *s, unsigned int insn,
> @@ -4096,7 +4096,7 @@ static void gen_exception_return(DisasContext *s, 
> TCGv_i32 pc)
>      tmp = load_cpu_field(spsr);
>      gen_set_cpsr(tmp, CPSR_ERET_MASK);
>      tcg_temp_free_i32(tmp);
> -    s->is_jmp = DISAS_UPDATE;
> +    s->is_jmp = DISAS_JUMP;
>  }
>
>  /* Generate a v6 exception return.  Marks both values as dead.  */
> @@ -4105,7 +4105,7 @@ static void gen_rfe(DisasContext *s, TCGv_i32 pc, 
> TCGv_i32 cpsr)
>      gen_set_cpsr(cpsr, CPSR_ERET_MASK);
>      tcg_temp_free_i32(cpsr);
>      store_reg(s, 15, pc);
> -    s->is_jmp = DISAS_UPDATE;
> +    s->is_jmp = DISAS_JUMP;
>  }
>
>  static void gen_nop_hint(DisasContext *s, int val)
> @@ -9035,7 +9035,7 @@ static void disas_arm_insn(DisasContext *s, unsigned 
> int insn)
>                      tmp = load_cpu_field(spsr);
>                      gen_set_cpsr(tmp, CPSR_ERET_MASK);
>                      tcg_temp_free_i32(tmp);
> -                    s->is_jmp = DISAS_UPDATE;
> +                    s->is_jmp = DISAS_JUMP;
>                  }
>              }
>              break;
> @@ -11488,8 +11488,10 @@ void gen_intermediate_code(CPUARMState *env, 
> TranslationBlock *tb)
>              gen_goto_tb(dc, 1, dc->pc);
>              break;
>          default:
> -        case DISAS_JUMP:
>          case DISAS_UPDATE:
> +            gen_set_pc_im(dc, dc->pc);
> +            /* fall through */
> +        case DISAS_JUMP:

This is also changing the behaviour for the "default" label, which
I think is unintentional. In particular DISAS_EXC doesn't appear
in this switch and so we're relying on the default being "do nothing".
(I think it's just an oversight that DISAS_EXC isn't an explicit
case in this switch, as it is for target-a64.c.)

This patch leaves three instances of DISAS_UPDATE. For two
of those [userspace kernel page and the M profile exception-return
magic addresses], we've just done a gen_exception_internal(), so
updating the PC here is pointless. Those should be DISAS_EXC
instead I think. (The effect of your patch on these is just
generating unreachable code, so no user visible problems.)

The third one is in the breakpoint handling, which I guess
is the case we're aiming to fix?

I think if we want to use DISAS_UPDATE then we also need
to handle it inside the
 if (unlikely(cs->singlestep_enabled || dc->ss_active)) {
branch. Specifically, this check:
   if (dc->condjmp || !dc->is_jmp)
is trying to say "if this was a conditional jump, or it
has not already set the PC", and so now needs to read
   if (dc->condjmp || dc->is_jmp == DISAS_NEXT ||
       dc->is_jmp == DISAS_UPDATE)

Also, I've just noticed that we don't do a gen_set_pc_im()
*before* calling gen_helper_check_breakpoints(). That means
that if the possible breakpoint is not at the start of a TB
then the helper function will not see the correct env->pc.

>              /* indicate that the hash table must be used to find the next TB 
> */
>              tcg_gen_exit_tb(0);
>              break;
> --
> 1.9.1
>

thanks
-- PMM



reply via email to

[Prev in Thread] Current Thread [Next in Thread]