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

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