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

2015-11-02 Thread 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 
---
 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

2015-11-02 Thread Peter Maydell 
On 2 November 2015 at 18:16, Sergey Fedorov  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

2015-11-03 Thread Sergey Fedorov 
On 02.11.2015 21:29, Peter Maydell wrote:
> On 2 November 2015 at 18:16, Sergey Fedorov  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

2015-11-06 Thread Peter Maydell 
On 2 November 2015 at 18:16, Sergey Fedorov  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 
> ---
>  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.

>  /* indic

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

2015-11-09 Thread Sergey Fedorov 
On 06.11.2015 15:46, Peter Maydell wrote:
> On 2 November 2015 at 18:16, Sergey Fedorov  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 
>> ---
>>  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