[Qemu-devel] [PATCH] target-arm: Clean up DISAS_UPDATE usage in AArch32 translation code

[Qemu-devel] [PATCH] target-arm: Clean up DISAS_UPDATE usage in AArch32 translation code

[Sergey Fedorov]

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 <serge.fdrv@gmail.com>
---
 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:
             /* indicate that the hash table must be used to find the next TB */
             tcg_gen_exit_tb(0);
             break;
-- 
1.9.1

Re: [Qemu-devel] [PATCH] target-arm: Clean up DISAS_UPDATE usage in AArch32 translation code

[Peter Maydell]

On 2 November 2015 at 18:16, Sergey Fedorov <serge.fdrv@gmail.com> 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

Is this fixing the breakpoint related bug? If so the commit message
should say so. Otherwise it just looks like cleanup...

(I'll review the patch tomorrow.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-arm: Clean up DISAS_UPDATE usage in AArch32 translation code

[Sergey Fedorov]

On 02.11.2015 21:29, Peter Maydell wrote:
> On 2 November 2015 at 18:16, Sergey Fedorov <serge.fdrv@gmail.com> 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
> Is this fixing the breakpoint related bug? If so the commit message
> should say so. Otherwise it just looks like cleanup...
>
> (I'll review the patch tomorrow.)

Yes it's fixing a bug in breakpoint handling. I'll update the commit
message.

Best,
Sergey

Re: [Qemu-devel] [PATCH] target-arm: Clean up DISAS_UPDATE usage in AArch32 translation code

[Peter Maydell]

On 2 November 2015 at 18:16, Sergey Fedorov <serge.fdrv@gmail.com> 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 <serge.fdrv@gmail.com>
> ---
>  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

Re: [Qemu-devel] [PATCH] target-arm: Clean up DISAS_UPDATE usage in AArch32 translation code

[Sergey Fedorov]

On 06.11.2015 15:46, Peter Maydell wrote:
> On 2 November 2015 at 18:16, Sergey Fedorov <serge.fdrv@gmail.com> 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 <serge.fdrv@gmail.com>
>> ---
>>  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.)

Thanks for the catch, I'll fix this.

> 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.)

This one, too.

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

Yes, that is what I'm going to fix. I'll mention this in a commit message.

> 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)

This issue is a subtle one. Looks like I'm going to have to spend some
time getting into this. Actually, I just don't understand this code for
now :) Would it be OK, if I just do what you suggest and send v2 as soon
as possible and get into this later on?

> 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.

I'm going to prepare a separate patch to fix this.

Thanks,
Sergey

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