| Message ID | 149865801156.17063.15618379976159104550.stgit@frigg.lan |
|---|---|
| State | New |
| Headers | show |
On 06/28/2017 06:53 AM, Lluís Vilanova wrote: > Incrementally paves the way towards using the generic instruction translation > loop. > > Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu> > --- > target/arm/translate-a64.c | 74 +++++++++++++++++++++++++++----------------- > 1 file changed, 46 insertions(+), 28 deletions(-) > > diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c > index 9c870f6d07..586a01a2de 100644 > --- a/target/arm/translate-a64.c > +++ b/target/arm/translate-a64.c > @@ -11244,6 +11244,9 @@ static void aarch64_trblock_init_disas_context(DisasContextBase *dcbase, > dc->is_ldex = false; > dc->ss_same_el = (arm_debug_target_el(env) == dc->current_el); > > + dc->next_page_start = > + (dc->base.pc_first & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE; I think a better solution for a fixed-length ISA is to adjust max_insns. Perhaps the init_disas_context hook should be able to modify it? And, while I'm thinking of it -- why is the init_globals hook separate? There's nothing in between the two hook calls, and the more modern target front ends won't need it. r~
Richard Henderson writes: > On 06/28/2017 06:53 AM, Lluís Vilanova wrote: >> Incrementally paves the way towards using the generic instruction translation >> loop. >> >> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu> >> --- >> target/arm/translate-a64.c | 74 +++++++++++++++++++++++++++----------------- >> 1 file changed, 46 insertions(+), 28 deletions(-) >> >> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c >> index 9c870f6d07..586a01a2de 100644 >> --- a/target/arm/translate-a64.c >> +++ b/target/arm/translate-a64.c >> @@ -11244,6 +11244,9 @@ static void aarch64_trblock_init_disas_context(DisasContextBase *dcbase, dc-> is_ldex = false; dc-> ss_same_el = (arm_debug_target_el(env) == dc->current_el); >> + dc->next_page_start = >> + (dc->base.pc_first & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE; > I think a better solution for a fixed-length ISA is to adjust max_insns. Perhaps > the init_disas_context hook should be able to modify it? ARM has the thumb instructions, so it really isn't a fixed-length ISA. > And, while I'm thinking of it -- why is the init_globals hook separate? There's > nothing in between the two hook calls, and the more modern target front ends > won't need it. You mean merging init_disas_context and init_globals? I wanted to keep semantically different code on separate hooks, but I can pull the init_globals code into init_disas_context (hoping that as targets get modernized, they won't initialize any global there). Thanks, Lluis
On 07/07/2017 01:18 AM, Lluís Vilanova wrote: >>> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c >>> index 9c870f6d07..586a01a2de 100644 >>> --- a/target/arm/translate-a64.c >>> +++ b/target/arm/translate-a64.c >>> @@ -11244,6 +11244,9 @@ static void aarch64_trblock_init_disas_context(DisasContextBase *dcbase, > dc-> is_ldex = false; > dc-> ss_same_el = (arm_debug_target_el(env) == dc->current_el); >>> + dc->next_page_start = >>> + (dc->base.pc_first & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE; > >> I think a better solution for a fixed-length ISA is to adjust max_insns. Perhaps >> the init_disas_context hook should be able to modify it? > > ARM has the thumb instructions, so it really isn't a fixed-length ISA. From the filename above we're talking about AArch64 here. Whcih is most definitely a fixed-width ISA. That said, even for AArch32 we know by the TB flags whether or not we're going to be generating arm or thumb code. I think that these hooks will allow arm and thumb mode to finally be split apart cleanly, instead of the tangle that they are now. I see arm's gen_intermediate_code eventually looking like const TranslatorOps *ops = &arm_translator_ops; if (ARM_TBFLAG_THUMB(tb->flags)) { ops = &thumb_translator_ops; } #ifdef TARGET_AARCH64 if (ARM_TBFLAG_AARCH64_STATE(tb->flags)) { ops = &aarch64_translator_ops; } #endif translator_loop(ops, &dc.base, cpu, tb); There would certainly be some amount of shared code, but it would also allow quite a few of the existing dc->thumb checks to be eliminated. >> And, while I'm thinking of it -- why is the init_globals hook separate? There's >> nothing in between the two hook calls, and the more modern target front ends >> won't need it. > > You mean merging init_disas_context and init_globals? I wanted to keep > semantically different code on separate hooks, but I can pull the init_globals > code into init_disas_context (hoping that as targets get modernized, they won't > initialize any global there). I suppose you can leave the two hooks separate for now. It doesn't hurt, and it's kind of a reminder of things that need cleaning up. I do wonder if we should provide a generic empty hook, so that a target that does not need a particular hook need not define an empty function. It could just put e.g. "translator_noop" into the structure. Ok, maybe a better name than that... r~
On Fri, Jul 07, 2017 at 05:46:19 -1000, Richard Henderson wrote: > I do wonder if we should provide a generic empty hook, so that a target that > does not need a particular hook need not define an empty function. It could > just put e.g. "translator_noop" into the structure. Ok, maybe a better name > than that... NULL would serve that purpose well. The subsequent "if (hook)" branch would be essentially free because it'd be trivially predicted. E.
Richard Henderson writes: > On 07/07/2017 01:18 AM, Lluís Vilanova wrote: >>>> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c >>>> index 9c870f6d07..586a01a2de 100644 >>>> --- a/target/arm/translate-a64.c >>>> +++ b/target/arm/translate-a64.c >>>> @@ -11244,6 +11244,9 @@ static void aarch64_trblock_init_disas_context(DisasContextBase *dcbase, dc-> is_ldex = false; dc-> ss_same_el = (arm_debug_target_el(env) == dc->current_el); >>>> + dc->next_page_start = >>>> + (dc->base.pc_first & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE; >> >>> I think a better solution for a fixed-length ISA is to adjust max_insns. Perhaps >>> the init_disas_context hook should be able to modify it? >> >> ARM has the thumb instructions, so it really isn't a fixed-length ISA. > From the filename above we're talking about AArch64 here. > Whcih is most definitely a fixed-width ISA. > That said, even for AArch32 we know by the TB flags whether or not we're going > to be generating arm or thumb code. I think that these hooks will allow arm and > thumb mode to finally be split apart cleanly, instead of the tangle that they > are now. > I see arm's gen_intermediate_code eventually looking like > const TranslatorOps *ops = &arm_translator_ops; > if (ARM_TBFLAG_THUMB(tb->flags)) { > ops = &thumb_translator_ops; > } > #ifdef TARGET_AARCH64 > if (ARM_TBFLAG_AARCH64_STATE(tb->flags)) { > ops = &aarch64_translator_ops; > } > #endif > translator_loop(ops, &dc.base, cpu, tb); > There would certainly be some amount of shared code, but it would also allow > quite a few of the existing dc->thumb checks to be eliminated. Does this really need to be addressed on this series? >>> And, while I'm thinking of it -- why is the init_globals hook separate? There's >>> nothing in between the two hook calls, and the more modern target front ends >>> won't need it. >> >> You mean merging init_disas_context and init_globals? I wanted to keep >> semantically different code on separate hooks, but I can pull the init_globals >> code into init_disas_context (hoping that as targets get modernized, they won't >> initialize any global there). > I suppose you can leave the two hooks separate for now. It doesn't hurt, and > it's kind of a reminder of things that need cleaning up. Well, I've sent a (too rushed) v12 that features the merge. Since I'll have to send a v13, I can split them again if you want. > I do wonder if we should provide a generic empty hook, so that a target that > does not need a particular hook need not define an empty function. It could > just put e.g. "translator_noop" into the structure. Ok, maybe a better name > than that... I'm not really sure it's worth the effort. The original version trated NULLs as a "skip this operation" (as Emilio suggests to revive), but after discussing the approach it was decided that defining an empty function was not too much effort, so that's what I went for. I can restore the NULL approach or add a default empty hook implementation (translator_ignored_op?) if there's a strong feeling to change it. Cheers, Lluis
Emilio G Cota writes: > On Fri, Jul 07, 2017 at 05:46:19 -1000, Richard Henderson wrote: >> I do wonder if we should provide a generic empty hook, so that a target that >> does not need a particular hook need not define an empty function. It could >> just put e.g. "translator_noop" into the structure. Ok, maybe a better name >> than that... > NULL would serve that purpose well. The subsequent "if (hook)" branch > would be essentially free because it'd be trivially predicted. Someone (Richard? can't remember exactly) already objected to using NULL. Cheers, Lluis
On 07/07/2017 07:32 AM, Lluís Vilanova wrote: >> That said, even for AArch32 we know by the TB flags whether or not we're going >> to be generating arm or thumb code. I think that these hooks will allow arm and >> thumb mode to finally be split apart cleanly, instead of the tangle that they >> are now. >> >> I see arm's gen_intermediate_code eventually looking like >> >> const TranslatorOps *ops = &arm_translator_ops; >> if (ARM_TBFLAG_THUMB(tb->flags)) { >> ops = &thumb_translator_ops; >> } >> #ifdef TARGET_AARCH64 >> if (ARM_TBFLAG_AARCH64_STATE(tb->flags)) { >> ops = &aarch64_translator_ops; >> } >> #endif >> translator_loop(ops, &dc.base, cpu, tb); > >> There would certainly be some amount of shared code, but it would also allow >> quite a few of the existing dc->thumb checks to be eliminated. > > Does this really need to be addressed on this series? No. It is quite a tangle and it'll take some time to rectify. >> I suppose you can leave the two hooks separate for now. It doesn't hurt, and >> it's kind of a reminder of things that need cleaning up. > > Well, I've sent a (too rushed) v12 that features the merge. Since I'll have to > send a v13, I can split them again if you want. Don't go to any extra trouble if no one else has any strong feelings. > I can restore the NULL approach or add a default empty hook implementation > (translator_ignored_op?) if there's a strong feeling to change it. Ok, we can work that out. r~
Richard Henderson writes: > On 07/07/2017 07:32 AM, Lluís Vilanova wrote: [...] >> I can restore the NULL approach or add a default empty hook implementation >> (translator_ignored_op?) if there's a strong feeling to change it. > Ok, we can work that out. I'm not sure what you mean. Leave as-is (targets must define all hooks), ignore NULLs as before, add a default empty implementation for each hook? Cheers, Lluis
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c index 9c870f6d07..586a01a2de 100644 --- a/target/arm/translate-a64.c +++ b/target/arm/translate-a64.c @@ -11244,6 +11244,9 @@ static void aarch64_trblock_init_disas_context(DisasContextBase *dcbase, dc->is_ldex = false; dc->ss_same_el = (arm_debug_target_el(env) == dc->current_el); + dc->next_page_start = + (dc->base.pc_first & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE; + init_tmp_a64_array(dc); } @@ -11278,12 +11281,45 @@ static BreakpointCheckType aarch64_trblock_breakpoint_check( } } +static target_ulong aarch64_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_EXC; + } else { + disas_a64_insn(env, dc); + } + + if (dc->ss_active) { + dc->base.is_jmp = DISAS_SS; + } else if (dc->pc >= dc->next_page_start) { + dc->base.is_jmp = DISAS_PAGE_CROSS; + } + + return dc->pc; +} + void gen_intermediate_code_a64(DisasContextBase *dcbase, CPUState *cs, TranslationBlock *tb) { - CPUARMState *env = cs->env_ptr; DisasContext *dc = container_of(dcbase, DisasContext, base); - target_ulong next_page_start; int max_insns; dc->base.tb = tb; @@ -11294,7 +11330,6 @@ void gen_intermediate_code_a64(DisasContextBase *dcbase, CPUState *cs, dc->base.singlestep_enabled = cs->singlestep_enabled; aarch64_trblock_init_disas_context(&dc->base, cs); - next_page_start = (dc->base.pc_first & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE; max_insns = dc->base.tb->cflags & CF_COUNT_MASK; if (max_insns == 0) { max_insns = CF_COUNT_MASK; @@ -11344,42 +11379,24 @@ void gen_intermediate_code_a64(DisasContextBase *dcbase, CPUState *cs, 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)); - dc->base.is_jmp = DISAS_EXC; - break; - } - - disas_a64_insn(env, dc); + dc->base.pc_next = aarch64_trblock_translate_insn(&dc->base, cs); if (tcg_check_temp_count()) { fprintf(stderr, "TCG temporary leak before "TARGET_FMT_lx"\n", dc->pc); } + if (!dc->base.is_jmp && (tcg_op_buf_full() || cs->singlestep_enabled || + singlestep || dc->base.num_insns >= max_insns)) { + dc->base.is_jmp = DISAS_TOO_MANY; + } + /* 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. */ - } while (!dc->base.is_jmp && !tcg_op_buf_full() && - !cs->singlestep_enabled && - !singlestep && - !dc->ss_active && - dc->pc < next_page_start && - dc->base.num_insns < max_insns); + } while (!dc->base.is_jmp); if (dc->base.tb->cflags & CF_LAST_IO) { gen_io_end(); @@ -11404,6 +11421,7 @@ void gen_intermediate_code_a64(DisasContextBase *dcbase, CPUState *cs, } else { switch (dc->base.is_jmp) { case DISAS_NEXT: + case DISAS_TOO_MANY: gen_goto_tb(dc, 1, dc->pc); break; default:
Incrementally paves the way towards using the generic instruction translation loop. Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu> --- target/arm/translate-a64.c | 74 +++++++++++++++++++++++++++----------------- 1 file changed, 46 insertions(+), 28 deletions(-)