| Message ID | 149865776960.17063.4875279139522061160.stgit@frigg.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/28/2017 06:49 AM, Lluís Vilanova wrote: > + /* We want to stop the TB if the next insn starts in a new page, > + * or if it spans between this page and the next. This means that > + * if we're looking at the last halfword in the page we need to > + * see if it's a 16-bit Thumb insn (which will fit in this TB) > + * or a 32-bit Thumb insn (which won't). > + * This is to avoid generating a silly TB with a single 16-bit insn > + * in it at the end of this page (which would execute correctly > + * but isn't very efficient). > + */ > + return DISAS_PAGE_CROSS; Any reason to introduce a new name as opposed to TOO_MANY? As far as I can tell they're the same.... > + if (dc->ss_active && !dc->pstate_ss) { > + /* Singlestep state is Active-pending. > + * If we're in this state at the start of a TB then either > + * a) we just took an exception to an EL which is being debugged > + * and this is the first insn in the exception handler > + * b) debug exceptions were masked and we just unmasked them > + * without changing EL (eg by clearing PSTATE.D) > + * In either case we're going to take a swstep exception in the > + * "did not step an insn" case, and so the syndrome ISV and EX > + * bits should be zero. > + */ > + assert(dc->base.num_insns == 1); > + gen_exception(EXCP_UDEF, syn_swstep(dc->ss_same_el, 0, 0), > + default_exception_el(dc)); > + dc->base.is_jmp = DISAS_SKIP; This is surely DISAS_EXC -- see gen_step_complete_exception. Why introduce a new name? r~
Richard Henderson writes: > On 06/28/2017 06:49 AM, Lluís Vilanova wrote: >> + /* We want to stop the TB if the next insn starts in a new page, >> + * or if it spans between this page and the next. This means that >> + * if we're looking at the last halfword in the page we need to >> + * see if it's a 16-bit Thumb insn (which will fit in this TB) >> + * or a 32-bit Thumb insn (which won't). >> + * This is to avoid generating a silly TB with a single 16-bit insn >> + * in it at the end of this page (which would execute correctly >> + * but isn't very efficient). >> + */ >> + return DISAS_PAGE_CROSS; > Any reason to introduce a new name as opposed to TOO_MANY? As far as I can tell > they're the same.... Yes, DISAS_SS and DISAS_PAGE_CROSS turned out to be remnants of previous series. >> + if (dc->ss_active && !dc->pstate_ss) { >> + /* Singlestep state is Active-pending. >> + * If we're in this state at the start of a TB then either >> + * a) we just took an exception to an EL which is being debugged >> + * and this is the first insn in the exception handler >> + * b) debug exceptions were masked and we just unmasked them >> + * without changing EL (eg by clearing PSTATE.D) >> + * In either case we're going to take a swstep exception in the >> + * "did not step an insn" case, and so the syndrome ISV and EX >> + * bits should be zero. >> + */ >> + assert(dc->base.num_insns == 1); >> + gen_exception(EXCP_UDEF, syn_swstep(dc->ss_same_el, 0, 0), >> + default_exception_el(dc)); >> + dc->base.is_jmp = DISAS_SKIP; > This is surely DISAS_EXC -- see gen_step_complete_exception. > Why introduce a new name? The original code goes straight to done_generating here, and that's the purpose of DISAS_SKIP (skip the code executed between the end of the loop and the done_generating label). Thanks, Lluis
On 07/07/2017 01:13 AM, Lluís Vilanova wrote: >>> + if (dc->ss_active && !dc->pstate_ss) { >>> + /* Singlestep state is Active-pending. >>> + * If we're in this state at the start of a TB then either >>> + * a) we just took an exception to an EL which is being debugged >>> + * and this is the first insn in the exception handler >>> + * b) debug exceptions were masked and we just unmasked them >>> + * without changing EL (eg by clearing PSTATE.D) >>> + * In either case we're going to take a swstep exception in the >>> + * "did not step an insn" case, and so the syndrome ISV and EX >>> + * bits should be zero. >>> + */ >>> + assert(dc->base.num_insns == 1); >>> + gen_exception(EXCP_UDEF, syn_swstep(dc->ss_same_el, 0, 0), >>> + default_exception_el(dc)); >>> + dc->base.is_jmp = DISAS_SKIP; > >> This is surely DISAS_EXC -- see gen_step_complete_exception. >> Why introduce a new name? > > The original code goes straight to done_generating here, and that's the purpose > of DISAS_SKIP (skip the code executed between the end of the loop and the > done_generating label). That is the purpose of DISAS_EXC too. We've called a noreturn helper to raise an exception and all following code is unreached. If there *was* any code being emitted afterward, that is arguably a bug. r~
Richard Henderson writes: > On 07/07/2017 01:13 AM, Lluís Vilanova wrote: >>>> + if (dc->ss_active && !dc->pstate_ss) { >>>> + /* Singlestep state is Active-pending. >>>> + * If we're in this state at the start of a TB then either >>>> + * a) we just took an exception to an EL which is being debugged >>>> + * and this is the first insn in the exception handler >>>> + * b) debug exceptions were masked and we just unmasked them >>>> + * without changing EL (eg by clearing PSTATE.D) >>>> + * In either case we're going to take a swstep exception in the >>>> + * "did not step an insn" case, and so the syndrome ISV and EX >>>> + * bits should be zero. >>>> + */ >>>> + assert(dc->base.num_insns == 1); >>>> + gen_exception(EXCP_UDEF, syn_swstep(dc->ss_same_el, 0, 0), >>>> + default_exception_el(dc)); >>>> + dc->base.is_jmp = DISAS_SKIP; >> >>> This is surely DISAS_EXC -- see gen_step_complete_exception. >>> Why introduce a new name? >> >> The original code goes straight to done_generating here, and that's the purpose >> of DISAS_SKIP (skip the code executed between the end of the loop and the >> done_generating label). > That is the purpose of DISAS_EXC too. We've called a noreturn helper to raise > an exception and all following code is unreached. If there *was* any code being > emitted afterward, that is arguably a bug. There was no code being generated after this specific case, but I haven't checked if DISAS_EXC is set in any other place that is not immediately followed by a "goto done_generating". Does this mean DISAS_EXC should be on the generic code and do a "goto done_generating" whenever it is found? And if so, what are the correct places to check for this? After ops->insn_start, ops->translate_insn, ops->tb_stop? Thanks, Lluis
On 7 July 2017 at 16:26, Richard Henderson <rth@twiddle.net> wrote: > That is the purpose of DISAS_EXC too. We've called a noreturn helper to > raise an exception and all following code is unreached. If there *was* any > code being emitted afterward, that is arguably a bug. One exception to that is a conditionally executed exception generating exception -- there will in that case be a following label for the condfail case to branch to and the code for the condfail path. The distinction in the case that this code fragment is touching is that the cases handled in current master via 'goto done_generating' and in Lluis' patch as DISAS_SKIP are the "this insn is going to generate an exception without even thinking about conditional exception" (ie breakpoints, singlestep); DISAS_EXC is for "the instruction itself generates an exception, so don't bother with emitting too much unreachable code to update the PC etc, but we still need to handle the usual end-of-insn condfail path". We do a few things in the DISAS_EXC codepath (like calling gen_set_condexec()) which strictly speaking are pointless but which it didn't seem worth trying to avoid just to avoid generating a few extra bytes in the generated code in a not-terribly-likely case. thanks -- PMM
On 07/07/2017 07:18 AM, Lluís Vilanova wrote: > There was no code being generated after this specific case, but I haven't > checked if DISAS_EXC is set in any other place that is not immediately followed > by a "goto done_generating". Typically we haven't actually done a goto, but simply exit the loop and emit nothing within the final cleanup (tb_stop?). > Does this mean DISAS_EXC should be on the generic code and do a "goto > done_generating" whenever it is found? And if so, what are the correct places to > check for this? After ops->insn_start, ops->translate_insn, ops->tb_stop? Yes, this should be handled generically, since all targets need it. That said, I would prefer a better name like DISAS_NORETURN, which does not imply that an actual exception has been raised, but explicitly says that all following code is dead. r~
On 07/07/2017 07:33 AM, Peter Maydell wrote: > On 7 July 2017 at 16:26, Richard Henderson <rth@twiddle.net> wrote: >> That is the purpose of DISAS_EXC too. We've called a noreturn helper to >> raise an exception and all following code is unreached. If there *was* any >> code being emitted afterward, that is arguably a bug. > > One exception to that is a conditionally executed > exception generating exception -- there will in that > case be a following label for the condfail case to branch > to and the code for the condfail path. > > The distinction in the case that this code fragment is touching > is that the cases handled in current master via 'goto > done_generating' and in Lluis' patch as > DISAS_SKIP are the "this insn is going to generate an > exception without even thinking about conditional > exception" (ie breakpoints, singlestep); DISAS_EXC > is for "the instruction itself generates an exception, > so don't bother with emitting too much unreachable > code to update the PC etc, but we still need to handle > the usual end-of-insn condfail path". Ok. LLuis, this implies that the DISAS_NORETURN that I talked about elsewhere should be the thing handled generically, but that target/arm still needs a target-specific define for DISAS_EXC so that the conditional execution handler can make the distinction. > We do a few things in the DISAS_EXC codepath > (like calling gen_set_condexec()) which strictly speaking > are pointless but which it didn't seem worth trying to > avoid just to avoid generating a few extra bytes in the > generated code in a not-terribly-likely case. Yeah. We'd probably be better off just adding dead-code removal to TCG. Something that used to be difficult but would now be trivial to do. r~
Richard Henderson writes: > On 07/07/2017 07:18 AM, Lluís Vilanova wrote: >> There was no code being generated after this specific case, but I haven't >> checked if DISAS_EXC is set in any other place that is not immediately followed >> by a "goto done_generating". > Typically we haven't actually done a goto, but simply exit the loop and emit > nothing within the final cleanup (tb_stop?). The case handled by DISAS_SKIP ignores tb_stop() (the target code can simply return when DISAS_EXC is found instead of DISAS_SKIP) *and* gen_io_end(); this last one is never omitted when DISAS_EXC is found now, and theoretically DISAS_EXC can be set by any target-specific hook. Thus my question if the generic call to gen_io_end() should check for DISAS_EXC too (I have no idea if it would be an error to call it with DISAS_EXC set, or whether it makes sense to for a target to set it so that gen_io_start() is called but gen_io_end() is then skipped by a DISAS_EXC set in ops->translate_insn()). >> Does this mean DISAS_EXC should be on the generic code and do a "goto >> done_generating" whenever it is found? And if so, what are the correct places to >> check for this? After ops->insn_start, ops->translate_insn, ops->tb_stop? > Yes, this should be handled generically, since all targets need it. > That said, I would prefer a better name like DISAS_NORETURN, which does not > imply that an actual exception has been raised, but explicitly says that all > following code is dead. I can use that name. And in fact, generalizing DISAS_NORETURN will allow dropping the enum result of breakpoint_check(), and instead simply return a bool (whether a breakpoint did hit). Targets can then set DISAS_NORETURN and return true instead of returning BC_HIT_TB (simply returning true is equivalent to the previous BC_HIT_INSN). Cheers, Lluis
On 07/10/2017 03:47 AM, Lluís Vilanova wrote: > Richard Henderson writes: > >> On 07/07/2017 07:18 AM, Lluís Vilanova wrote: >>> There was no code being generated after this specific case, but I haven't >>> checked if DISAS_EXC is set in any other place that is not immediately followed >>> by a "goto done_generating". > >> Typically we haven't actually done a goto, but simply exit the loop and emit >> nothing within the final cleanup (tb_stop?). > > The case handled by DISAS_SKIP ignores tb_stop() (the target code can simply > return when DISAS_EXC is found instead of DISAS_SKIP) *and* gen_io_end(); this > last one is never omitted when DISAS_EXC is found now, and theoretically > DISAS_EXC can be set by any target-specific hook. Thus my question if the > generic call to gen_io_end() should check for DISAS_EXC too (I have no idea if > it would be an error to call it with DISAS_EXC set, or whether it makes sense to > for a target to set it so that gen_io_start() is called but gen_io_end() is then > skipped by a DISAS_EXC set in ops->translate_insn()). It is not an error to call gen_io_start when gen_io_end isn't called (or isn't reached). There are many ways that can happen. The reason that arm does the goto after the gen_exception for single-stepping was simply convenience. Nothing would have gone wrong if it had used dc->is_jmp = DISAS_EXC; break; instead. r~
diff --git a/target/arm/translate.c b/target/arm/translate.c index 790eaa2164..7ab09a7e5f 100644 --- a/target/arm/translate.c +++ b/target/arm/translate.c @@ -11841,6 +11841,9 @@ static void arm_trblock_init_disas_context(DisasContextBase *dcbase, dc->pstate_ss = ARM_TBFLAG_PSTATE_SS(dc->base.tb->flags); dc->is_ldex = false; dc->ss_same_el = false; /* Can't be true since EL_d must be AArch64 */ + + dc->next_page_start = + (dc->base.pc_first & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE; } static void arm_trblock_init_globals(DisasContextBase *dcbase, CPUState *cpu) @@ -11944,14 +11947,82 @@ static BreakpointCheckType arm_trblock_breakpoint_check( } } +static target_ulong arm_trblock_translate_insn(DisasContextBase *dcbase, + CPUState *cpu) +{ + DisasContext *dc = container_of(dcbase, DisasContext, base); + CPUARMState *env = cpu->env_ptr; + + if (dc->ss_active && !dc->pstate_ss) { + /* Singlestep state is Active-pending. + * If we're in this state at the start of a TB then either + * a) we just took an exception to an EL which is being debugged + * and this is the first insn in the exception handler + * b) debug exceptions were masked and we just unmasked them + * without changing EL (eg by clearing PSTATE.D) + * In either case we're going to take a swstep exception in the + * "did not step an insn" case, and so the syndrome ISV and EX + * bits should be zero. + */ + assert(dc->base.num_insns == 1); + gen_exception(EXCP_UDEF, syn_swstep(dc->ss_same_el, 0, 0), + default_exception_el(dc)); + dc->base.is_jmp = DISAS_SKIP; + return dc->pc; + } + + if (dc->thumb) { + disas_thumb_insn(env, dc); + if (dc->condexec_mask) { + dc->condexec_cond = (dc->condexec_cond & 0xe) + | ((dc->condexec_mask >> 4) & 1); + dc->condexec_mask = (dc->condexec_mask << 1) & 0x1f; + if (dc->condexec_mask == 0) { + dc->condexec_cond = 0; + } + } + } else { + unsigned int insn = arm_ldl_code(env, dc->pc, dc->sctlr_b); + dc->pc += 4; + disas_arm_insn(dc, insn); + } + + if (dc->condjmp && !dc->base.is_jmp) { + gen_set_label(dc->condlabel); + dc->condjmp = 0; + } + + + /* Translation stops when a conditional branch is encountered. + * Otherwise the subsequent code could get translated several times. + * Also stop translation when a page boundary is reached. This + * ensures prefetch aborts occur at the right place. */ + + if (is_singlestepping(dc)) { + dc->base.is_jmp = DISAS_SS; + } else if ((dc->pc >= dc->next_page_start) || + ((dc->pc >= dc->next_page_start - 3) && + insn_crosses_page(env, dc))) { + /* We want to stop the TB if the next insn starts in a new page, + * or if it spans between this page and the next. This means that + * if we're looking at the last halfword in the page we need to + * see if it's a 16-bit Thumb insn (which will fit in this TB) + * or a 32-bit Thumb insn (which won't). + * This is to avoid generating a silly TB with a single 16-bit insn + * in it at the end of this page (which would execute correctly + * but isn't very efficient). + */ + return DISAS_PAGE_CROSS; + } + + return dc->pc; +} + /* generate intermediate code for basic block 'tb'. */ void gen_intermediate_code(CPUState *cpu, TranslationBlock *tb) { - CPUARMState *env = cpu->env_ptr; DisasContext dc1, *dc = &dc1; - target_ulong next_page_start; int max_insns; - bool end_of_page; /* generate intermediate code */ @@ -11973,7 +12044,6 @@ void gen_intermediate_code(CPUState *cpu, TranslationBlock *tb) arm_trblock_init_globals(&dc->base, cpu); - next_page_start = (dc->base.pc_first & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE; max_insns = tb->cflags & CF_COUNT_MASK; if (max_insns == 0) { max_insns = CF_COUNT_MASK; @@ -12024,72 +12094,20 @@ void gen_intermediate_code(CPUState *cpu, TranslationBlock *tb) gen_io_start(); } - if (dc->ss_active && !dc->pstate_ss) { - /* Singlestep state is Active-pending. - * If we're in this state at the start of a TB then either - * a) we just took an exception to an EL which is being debugged - * and this is the first insn in the exception handler - * b) debug exceptions were masked and we just unmasked them - * without changing EL (eg by clearing PSTATE.D) - * In either case we're going to take a swstep exception in the - * "did not step an insn" case, and so the syndrome ISV and EX - * bits should be zero. - */ - assert(dc->base.num_insns == 1); - gen_exception(EXCP_UDEF, syn_swstep(dc->ss_same_el, 0, 0), - default_exception_el(dc)); - goto done_generating; - } - - if (dc->thumb) { - disas_thumb_insn(env, dc); - if (dc->condexec_mask) { - dc->condexec_cond = (dc->condexec_cond & 0xe) - | ((dc->condexec_mask >> 4) & 1); - dc->condexec_mask = (dc->condexec_mask << 1) & 0x1f; - if (dc->condexec_mask == 0) { - dc->condexec_cond = 0; - } - } - } else { - unsigned int insn = arm_ldl_code(env, dc->pc, dc->sctlr_b); - dc->pc += 4; - disas_arm_insn(dc, insn); - } - - if (dc->condjmp && !dc->base.is_jmp) { - gen_set_label(dc->condlabel); - dc->condjmp = 0; - } + dc->base.pc_next = arm_trblock_translate_insn(&dc->base, cpu); if (tcg_check_temp_count()) { fprintf(stderr, "TCG temporary leak before "TARGET_FMT_lx"\n", dc->pc); } - /* Translation stops when a conditional branch is encountered. - * Otherwise the subsequent code could get translated several times. - * Also stop translation when a page boundary is reached. This - * ensures prefetch aborts occur at the right place. */ - - /* We want to stop the TB if the next insn starts in a new page, - * or if it spans between this page and the next. This means that - * if we're looking at the last halfword in the page we need to - * see if it's a 16-bit Thumb insn (which will fit in this TB) - * or a 32-bit Thumb insn (which won't). - * This is to avoid generating a silly TB with a single 16-bit insn - * in it at the end of this page (which would execute correctly - * but isn't very efficient). - */ - end_of_page = (dc->pc >= next_page_start) || - ((dc->pc >= next_page_start - 3) && insn_crosses_page(env, dc)); - - } while (!dc->base.is_jmp && !tcg_op_buf_full() && - !is_singlestepping(dc) && - !singlestep && - !end_of_page && - dc->base.num_insns < max_insns); + if (!dc->base.is_jmp && (tcg_op_buf_full() || singlestep || + dc->base.num_insns >= max_insns)) { + dc->base.is_jmp = DISAS_TOO_MANY; + } + } while (!dc->base.is_jmp); + if (dc->base.is_jmp != DISAS_SKIP) { if (tb->cflags & CF_LAST_IO) { if (dc->condjmp) { /* FIXME: This can theoretically happen with self-modifying @@ -12127,6 +12145,7 @@ void gen_intermediate_code(CPUState *cpu, TranslationBlock *tb) gen_exception(EXCP_SMC, syn_aa32_smc(), 3); break; case DISAS_NEXT: + case DISAS_TOO_MANY: case DISAS_UPDATE: gen_set_pc_im(dc, dc->pc); /* fall through */ @@ -12145,6 +12164,7 @@ void gen_intermediate_code(CPUState *cpu, TranslationBlock *tb) */ switch(dc->base.is_jmp) { case DISAS_NEXT: + case DISAS_TOO_MANY: gen_goto_tb(dc, 1, dc->pc); break; case DISAS_UPDATE: @@ -12198,6 +12218,7 @@ void gen_intermediate_code(CPUState *cpu, TranslationBlock *tb) gen_goto_tb(dc, 1, dc->pc); } } + } done_generating: gen_tb_end(tb, dc->base.num_insns); diff --git a/target/arm/translate.h b/target/arm/translate.h index 6fe40a344a..f830775540 100644 --- a/target/arm/translate.h +++ b/target/arm/translate.h @@ -9,6 +9,7 @@ typedef struct DisasContext { DisasContextBase base; target_ulong pc; + target_ulong next_page_start; uint32_t insn; /* Nonzero if this instruction has been conditionally skipped. */ int condjmp; @@ -148,6 +149,9 @@ static void disas_set_insn_syndrome(DisasContext *s, uint32_t syn) * as opposed to attempting to use lookup_and_goto_ptr. */ #define DISAS_EXIT DISAS_TARGET_11 +#define DISAS_SS DISAS_TARGET_12 +#define DISAS_PAGE_CROSS DISAS_TARGET_13 +#define DISAS_SKIP DISAS_TARGET_14 #ifdef TARGET_AARCH64 void a64_translate_init(void);
Incrementally paves the way towards using the generic instruction translation loop. Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu> --- target/arm/translate.c | 147 +++++++++++++++++++++++++++--------------------- target/arm/translate.h | 4 + 2 files changed, 88 insertions(+), 63 deletions(-)