[Qemu-devel] [PATCH] target-arm: Clean up DISAS_UPDATE usage in AArch32 translation code
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
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
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
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
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