| Message ID | 1447097859-586-1-git-send-email-serge.fdrv@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 9 November 2015 at 19:37, Sergey Fedorov <serge.fdrv@gmail.com> wrote: > Though I don't clearly understand how singlestepping is done here, I just do > what Peter suggested in his commnets for v1 and send this patch for review. I'm > going to get into this while the patch is in review process... So the way the 32-bit code works for singlestep is complicated because of the need to handle the conditional instructions, which means you get a lot more cases like "this is a conditional SWI" that need to be handled. A quick summary of some of the possible cases: * unconditional normal instruction: -- need to write the PC and condexec bits back to the CPU state -- then take a singlestep insn (either the architectural one or the EXCP_DEBUG one depending on which sort of step we are doing) * unconditional exception-generating instruction -- for architectural step of SWI/HVC/SMC we need to advance the singlestep state machine so that they behave correctly -- generate the relevant exception and then no point writing the code to take EXCP_DEBUG &c because we won't get to it * conditional instruction (including cond. branches): -- earlier code has already written back the PC for the "condition passed" case -- write out the code which takes the singlestep exception for the "condition passed" case -- then do gen_set_label(dc->condlabel) -- then the code to take the single step exception after executing for the "condition failed" case In particular in this bit: if (dc->condjmp || !dc->is_jmp) { gen_set_pc_im(dc, dc->pc); dc->condjmp = 0; } the cases when we need to update the PC are (a) for the condition-failed codepath of a conditional insn (the condition-passed codepath will already have written PC) (b) for a non-conditional insn that hasn't already written PC The A64 equivalent is much simpler because the only cases we need to handle are: * exception already generated (no point writing anything) * jumps (PC already written, just write code to take the step exception) * everything else (write PC then take step exception) I'll review the patch after lunch. thanks -- PMM
On 9 November 2015 at 19:37, 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 > > This patch fixes a bug in AArch32 breakpoint handling: when > check_breakpoints helper does not generate an exception, ending the TB > early with DISAS_UPDATE couldn't update PC in CPU state and execution > hangs. > > Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com> Applied to target-arm.next, thanks. -- PMM
On 10.11.2015 15:15, Peter Maydell wrote: > So the way the 32-bit code works for singlestep is complicated > because of the need to handle the conditional instructions, > which means you get a lot more cases like "this is a conditional > SWI" that need to be handled. A quick summary of some of the > possible cases: > > * unconditional normal instruction: > -- need to write the PC and condexec bits back to the CPU state > -- then take a singlestep insn (either the architectural one > or the EXCP_DEBUG one depending on which sort of step we are doing) > * unconditional exception-generating instruction > -- for architectural step of SWI/HVC/SMC we need to advance the > singlestep state machine so that they behave correctly > -- generate the relevant exception and then no point writing the > code to take EXCP_DEBUG &c because we won't get to it > * conditional instruction (including cond. branches): > -- earlier code has already written back the PC for the > "condition passed" case > -- write out the code which takes the singlestep exception for > the "condition passed" case > -- then do gen_set_label(dc->condlabel) > -- then the code to take the single step exception after > executing for the "condition failed" case > > In particular in this bit: > if (dc->condjmp || !dc->is_jmp) { > gen_set_pc_im(dc, dc->pc); > dc->condjmp = 0; Hi Peter, Thank you a lot for your explanation! It was really helpful for understanding the code :) One thing I wasn't sure of was whether this "dc->condjmp = 0" means that "condition failed" codepath below will also generate an exception whereas it shouldn't? Getting into the way the condexec bits handled I see that gen_set_condexec() should be called before gen_helper_check_breakpoints(), and probably also before gen_helper_access_check_cp_reg() because these helpers can raise an exception. I'm going to prepare patches for that soon. Best regards, Sergey > } > the cases when we need to update the PC are > (a) for the condition-failed codepath of a conditional insn > (the condition-passed codepath will already have written PC) > (b) for a non-conditional insn that hasn't already written PC > > The A64 equivalent is much simpler because the only cases we > need to handle are: > * exception already generated (no point writing anything) > * jumps (PC already written, just write code to take the step exception) > * everything else (write PC then take step exception)
On 13 November 2015 at 21:13, Sergey Fedorov <serge.fdrv@gmail.com> wrote: > On 10.11.2015 15:15, Peter Maydell wrote: >> So the way the 32-bit code works for singlestep is complicated >> because of the need to handle the conditional instructions, >> which means you get a lot more cases like "this is a conditional >> SWI" that need to be handled. A quick summary of some of the >> possible cases: >> >> * unconditional normal instruction: >> -- need to write the PC and condexec bits back to the CPU state >> -- then take a singlestep insn (either the architectural one >> or the EXCP_DEBUG one depending on which sort of step we are doing) >> * unconditional exception-generating instruction >> -- for architectural step of SWI/HVC/SMC we need to advance the >> singlestep state machine so that they behave correctly >> -- generate the relevant exception and then no point writing the >> code to take EXCP_DEBUG &c because we won't get to it >> * conditional instruction (including cond. branches): >> -- earlier code has already written back the PC for the >> "condition passed" case >> -- write out the code which takes the singlestep exception for >> the "condition passed" case >> -- then do gen_set_label(dc->condlabel) >> -- then the code to take the single step exception after >> executing for the "condition failed" case >> >> In particular in this bit: >> if (dc->condjmp || !dc->is_jmp) { >> gen_set_pc_im(dc, dc->pc); >> dc->condjmp = 0; > > Hi Peter, > > Thank you a lot for your explanation! It was really helpful for > understanding the code :) One thing I wasn't sure of was whether this > "dc->condjmp = 0" means that "condition failed" codepath below will also > generate an exception whereas it shouldn't? You want a singlestep exception for both conditional-insn failed and conditional-insn passed (either way we've executed the instruction and should return control to the debugger). (The code is I think more confusing than it needs to be and also somewhat repetitive in the way we have the same code for "handle a trap insn if condjmp" and "handle a trap insn if not condjmp".) > Getting into the way the condexec bits handled I see that > gen_set_condexec() should be called before > gen_helper_check_breakpoints(), and probably also before > gen_helper_access_check_cp_reg() because these helpers can raise an > exception. I'm going to prepare patches for that soon. Yes, I think this is right in both cases. (I'm kind of surprised that we haven't noticed the invalid condexec for conditional Thumb mode system register accesses, but I guess mostly guests don't try to access registers that are going to trap.) thanks -- PMM
On 14.11.2015 22:45, Peter Maydell wrote: > On 13 November 2015 at 21:13, Sergey Fedorov <serge.fdrv@gmail.com> wrote: >> On 10.11.2015 15:15, Peter Maydell wrote: >>> So the way the 32-bit code works for singlestep is complicated >>> because of the need to handle the conditional instructions, >>> which means you get a lot more cases like "this is a conditional >>> SWI" that need to be handled. A quick summary of some of the >>> possible cases: >>> >>> * unconditional normal instruction: >>> -- need to write the PC and condexec bits back to the CPU state >>> -- then take a singlestep insn (either the architectural one >>> or the EXCP_DEBUG one depending on which sort of step we are doing) >>> * unconditional exception-generating instruction >>> -- for architectural step of SWI/HVC/SMC we need to advance the >>> singlestep state machine so that they behave correctly >>> -- generate the relevant exception and then no point writing the >>> code to take EXCP_DEBUG &c because we won't get to it >>> * conditional instruction (including cond. branches): >>> -- earlier code has already written back the PC for the >>> "condition passed" case >>> -- write out the code which takes the singlestep exception for >>> the "condition passed" case >>> -- then do gen_set_label(dc->condlabel) >>> -- then the code to take the single step exception after >>> executing for the "condition failed" case >>> >>> In particular in this bit: >>> if (dc->condjmp || !dc->is_jmp) { >>> gen_set_pc_im(dc, dc->pc); >>> dc->condjmp = 0; >> Hi Peter, >> >> Thank you a lot for your explanation! It was really helpful for >> understanding the code :) One thing I wasn't sure of was whether this >> "dc->condjmp = 0" means that "condition failed" codepath below will also >> generate an exception whereas it shouldn't? > You want a singlestep exception for both conditional-insn > failed and conditional-insn passed (either way we've executed > the instruction and should return control to the debugger). To be clear, I mean SWI/HVC/SMC exceptions could be generated when singlestepping through a conditional-insn failed codepath. > > (The code is I think more confusing than it needs to be > and also somewhat repetitive in the way we have the same code for > "handle a trap insn if condjmp" and "handle a trap insn if > not condjmp".) I think I could make this code more clear and concise as well as put some comments to describe it :) Best regards, Sergey > >> Getting into the way the condexec bits handled I see that >> gen_set_condexec() should be called before >> gen_helper_check_breakpoints(), and probably also before >> gen_helper_access_check_cp_reg() because these helpers can raise an >> exception. I'm going to prepare patches for that soon. > Yes, I think this is right in both cases. (I'm kind of > surprised that we haven't noticed the invalid condexec > for conditional Thumb mode system register accesses, but > I guess mostly guests don't try to access registers that > are going to trap.)
On 15 November 2015 at 20:30, Sergey Fedorov <serge.fdrv@gmail.com> wrote: > On 14.11.2015 22:45, Peter Maydell wrote: >> On 13 November 2015 at 21:13, Sergey Fedorov <serge.fdrv@gmail.com> wrote: >>> Thank you a lot for your explanation! It was really helpful for >>> understanding the code :) One thing I wasn't sure of was whether this >>> "dc->condjmp = 0" means that "condition failed" codepath below will also >>> generate an exception whereas it shouldn't? >> You want a singlestep exception for both conditional-insn >> failed and conditional-insn passed (either way we've executed >> the instruction and should return control to the debugger). > > To be clear, I mean SWI/HVC/SMC exceptions could be generated when > singlestepping through a conditional-insn failed codepath. When I read through the code earlier I thought that wasn't possible, but rereading it I see what you mean. I wonder if we introduced that accidentally adding some of the extra features along the way or if it's always been wrong like that... thanks -- PMM
diff --git a/target-arm/translate.c b/target-arm/translate.c index ff262a2..a56f7fe 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; @@ -11355,7 +11355,7 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb) /* We always get here via a jump, so know we are not in a conditional execution block. */ gen_exception_internal(EXCP_KERNEL_TRAP); - dc->is_jmp = DISAS_UPDATE; + dc->is_jmp = DISAS_EXC; break; } #else @@ -11363,7 +11363,7 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb) /* We always get here via a jump, so know we are not in a conditional execution block. */ gen_exception_internal(EXCP_EXCEPTION_EXIT); - dc->is_jmp = DISAS_UPDATE; + dc->is_jmp = DISAS_EXC; break; } #endif @@ -11497,7 +11497,8 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb) } gen_set_label(dc->condlabel); } - if (dc->condjmp || !dc->is_jmp) { + if (dc->condjmp || dc->is_jmp == DISAS_NEXT || + dc->is_jmp == DISAS_UPDATE) { gen_set_pc_im(dc, dc->pc); dc->condjmp = 0; } @@ -11533,9 +11534,11 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb) case DISAS_NEXT: 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: + default: /* indicate that the hash table must be used to find the next TB */ tcg_gen_exit_tb(0); break;
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 This patch fixes a bug in AArch32 breakpoint handling: when check_breakpoints helper does not generate an exception, ending the TB early with DISAS_UPDATE couldn't update PC in CPU state and execution hangs. Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com> --- Though I don't clearly understand how singlestepping is done here, I just do what Peter suggested in his commnets for v1 and send this patch for review. I'm going to get into this while the patch is in review process... Notes: Changes in v2: * 'default' label moved to right place * DISAS_EXC used after gen_exception_internal() * DISAS_UPDATE handling fixed for singlestepping * Breakpoint handling bug to fix by this patch mentioned in commit message target-arm/translate.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-)