| Message ID | 149865462073.17063.17488435214703164893.stgit@frigg.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jun 28, 2017 at 15:57:00 +0300, 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/i386/translate.c | 72 +++++++++++++++++++++++++++++++---------------- > 1 file changed, 48 insertions(+), 24 deletions(-) > > diff --git a/target/i386/translate.c b/target/i386/translate.c > index 3eee348de7..da4b409d97 100644 > --- a/target/i386/translate.c > +++ b/target/i386/translate.c > @@ -4420,15 +4420,17 @@ static void gen_sse(CPUX86State *env, DisasContext *s, int b, > > /* convert one instruction. s->base.is_jmp is set if the translation must > be stopped. Return the next pc value */ > -static target_ulong disas_insn(CPUX86State *env, DisasContext *s, > - target_ulong pc_start) > +static target_ulong disas_insn(DisasContextBase *dcbase, CPUState *cpu) > { > + DisasContext *s = container_of(dcbase, DisasContext, base); > + CPUX86State *env = cpu->env_ptr; Minor nit: you can pass dc (*s) here directly, no need for container_of (snip) > +static target_ulong i386_trblock_translate_insn(DisasContextBase *dcbase, > + CPUState *cpu) > +{ > + DisasContext *dc = container_of(dcbase, DisasContext, base); > + target_ulong pc_next = disas_insn(&dc->base, cpu); > + > + if (dc->base.is_jmp) { > + return pc_next; > + } > + > + if (dc->tf || (dc->base.tb->flags & HF_INHIBIT_IRQ_MASK)) { > + /* if single step mode, we generate only one instruction and > + generate an exception */ > + /* if irq were inhibited with HF_INHIBIT_IRQ_MASK, we clear > + the flag and abort the translation to give the irqs a > + change to be happen */ I know you just moved lines around, but while at it, s/change to be/chance to/ Other than that, Reviewed-by: Emilio G. Cota <cota@braap.org> E.
On 06/28/2017 05:57 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/i386/translate.c | 72 +++++++++++++++++++++++++++++++---------------- > 1 file changed, 48 insertions(+), 24 deletions(-) Reviewed-by: Richard Henderson <rth@twiddle.net> r~
Emilio G Cota writes: > On Wed, Jun 28, 2017 at 15:57:00 +0300, 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/i386/translate.c | 72 +++++++++++++++++++++++++++++++---------------- >> 1 file changed, 48 insertions(+), 24 deletions(-) >> >> diff --git a/target/i386/translate.c b/target/i386/translate.c >> index 3eee348de7..da4b409d97 100644 >> --- a/target/i386/translate.c >> +++ b/target/i386/translate.c >> @@ -4420,15 +4420,17 @@ static void gen_sse(CPUX86State *env, DisasContext *s, int b, >> >> /* convert one instruction. s->base.is_jmp is set if the translation must >> be stopped. Return the next pc value */ >> -static target_ulong disas_insn(CPUX86State *env, DisasContext *s, >> - target_ulong pc_start) >> +static target_ulong disas_insn(DisasContextBase *dcbase, CPUState *cpu) >> { >> + DisasContext *s = container_of(dcbase, DisasContext, base); >> + CPUX86State *env = cpu->env_ptr; > Minor nit: you can pass dc (*s) here directly, no need for container_of [...] I prefer not to so that the code will work in the future (i.e., not assuming the location of base inside disascontext). Thanks, Lluis
On 07/06/2017 11:25 PM, Lluís Vilanova wrote: >>> /* convert one instruction. s->base.is_jmp is set if the translation must >>> be stopped. Return the next pc value */ >>> -static target_ulong disas_insn(CPUX86State *env, DisasContext *s, >>> - target_ulong pc_start) >>> +static target_ulong disas_insn(DisasContextBase *dcbase, CPUState *cpu) >>> { >>> + DisasContext *s = container_of(dcbase, DisasContext, base); >>> + CPUX86State *env = cpu->env_ptr; > >> Minor nit: you can pass dc (*s) here directly, no need for container_of > [...] > > I prefer not to so that the code will work in the future (i.e., not assuming the > location of base inside disascontext). There's clearly a misunderstanding here. Emilio is saying that disas_insn is not a hook and private to the front-end. Therefore the argument to the function should be "DisasContet *s" and not "DisasContextBase *dcbase". And I agree. Passing DisasContext should be preferred where possible. r~
Richard Henderson writes: > On 07/06/2017 11:25 PM, Lluís Vilanova wrote: >>>> /* convert one instruction. s->base.is_jmp is set if the translation must >>>> be stopped. Return the next pc value */ >>>> -static target_ulong disas_insn(CPUX86State *env, DisasContext *s, >>>> - target_ulong pc_start) >>>> +static target_ulong disas_insn(DisasContextBase *dcbase, CPUState *cpu) >>>> { >>>> + DisasContext *s = container_of(dcbase, DisasContext, base); >>>> + CPUX86State *env = cpu->env_ptr; >> >>> Minor nit: you can pass dc (*s) here directly, no need for container_of >> [...] >> >> I prefer not to so that the code will work in the future (i.e., not assuming the >> location of base inside disascontext). > There's clearly a misunderstanding here. > Emilio is saying that disas_insn is not a hook and private to the > front-end. Therefore the argument to the function should be "DisasContet *s" and > not "DisasContextBase *dcbase". > And I agree. Passing DisasContext should be preferred where possible. Woooops, my bad! :) Thanks, Lluis
diff --git a/target/i386/translate.c b/target/i386/translate.c index 3eee348de7..da4b409d97 100644 --- a/target/i386/translate.c +++ b/target/i386/translate.c @@ -4420,15 +4420,17 @@ static void gen_sse(CPUX86State *env, DisasContext *s, int b, /* convert one instruction. s->base.is_jmp is set if the translation must be stopped. Return the next pc value */ -static target_ulong disas_insn(CPUX86State *env, DisasContext *s, - target_ulong pc_start) +static target_ulong disas_insn(DisasContextBase *dcbase, CPUState *cpu) { + DisasContext *s = container_of(dcbase, DisasContext, base); + CPUX86State *env = cpu->env_ptr; int b, prefixes; int shift; TCGMemOp ot, aflag, dflag; int modrm, reg, rm, mod, op, opreg, val; target_ulong next_eip, tval; int rex_w, rex_r; + target_ulong pc_start = s->base.pc_next; s->pc_start = s->pc = pc_start; prefixes = 0; @@ -8478,10 +8480,51 @@ static BreakpointCheckType i386_trblock_breakpoint_check( } } +static target_ulong i386_trblock_translate_insn(DisasContextBase *dcbase, + CPUState *cpu) +{ + DisasContext *dc = container_of(dcbase, DisasContext, base); + target_ulong pc_next = disas_insn(&dc->base, cpu); + + if (dc->base.is_jmp) { + return pc_next; + } + + if (dc->tf || (dc->base.tb->flags & HF_INHIBIT_IRQ_MASK)) { + /* if single step mode, we generate only one instruction and + generate an exception */ + /* if irq were inhibited with HF_INHIBIT_IRQ_MASK, we clear + the flag and abort the translation to give the irqs a + change to be happen */ + gen_jmp_im(pc_next - dc->cs_base); + gen_eob(dc); + dc->base.is_jmp = DISAS_TOO_MANY; + } else if ((dc->base.tb->cflags & CF_USE_ICOUNT) + && ((dc->base.pc_next & TARGET_PAGE_MASK) + != ((dc->base.pc_next + TARGET_MAX_INSN_SIZE - 1) + & TARGET_PAGE_MASK) + || (dc->base.pc_next & ~TARGET_PAGE_MASK) == 0)) { + /* Do not cross the boundary of the pages in icount mode, + it can cause an exception. Do it only when boundary is + crossed by the first instruction in the block. + If current instruction already crossed the bound - it's ok, + because an exception hasn't stopped this code. + */ + gen_jmp_im(pc_next - dc->cs_base); + gen_eob(dc); + dc->base.is_jmp = DISAS_TOO_MANY; + } else if ((pc_next - dc->base.pc_first) >= (TARGET_PAGE_SIZE - 32)) { + gen_jmp_im(pc_next - dc->cs_base); + gen_eob(dc); + dc->base.is_jmp = DISAS_TOO_MANY; + } + + return pc_next; +} + /* generate intermediate code for basic block 'tb'. */ void gen_intermediate_code(CPUState *cpu, TranslationBlock *tb) { - CPUX86State *env = cpu->env_ptr; DisasContext dc1, *dc = &dc1; int num_insns; int max_insns; @@ -8543,39 +8586,20 @@ void gen_intermediate_code(CPUState *cpu, TranslationBlock *tb) gen_io_start(); } - dc->base.pc_next = disas_insn(env, dc, dc->base.pc_next); + dc->base.pc_next = i386_trblock_translate_insn(&dc->base, cpu); /* stop translation if indicated */ if (dc->base.is_jmp) { break; } /* if single step mode, we generate only one instruction and generate an exception */ - /* if irq were inhibited with HF_INHIBIT_IRQ_MASK, we clear - the flag and abort the translation to give the irqs a - change to be happen */ - if (dc->tf || dc->base.singlestep_enabled || - (dc->base.tb->flags & HF_INHIBIT_IRQ_MASK)) { - gen_jmp_im(dc->base.pc_next - dc->cs_base); - gen_eob(dc); - break; - } - /* Do not cross the boundary of the pages in icount mode, - it can cause an exception. Do it only when boundary is - crossed by the first instruction in the block. - If current instruction already crossed the bound - it's ok, - because an exception hasn't stopped this code. - */ - if ((tb->cflags & CF_USE_ICOUNT) - && ((dc->base.pc_next & TARGET_PAGE_MASK) - != ((dc->base.pc_next + TARGET_MAX_INSN_SIZE - 1) & TARGET_PAGE_MASK) - || (dc->base.pc_next & ~TARGET_PAGE_MASK) == 0)) { + if (dc->base.singlestep_enabled) { gen_jmp_im(dc->base.pc_next - dc->cs_base); gen_eob(dc); break; } /* if too long translation, stop generation too */ if (tcg_op_buf_full() || - (dc->base.pc_next - dc->base.pc_first) >= (TARGET_PAGE_SIZE - 32) || num_insns >= max_insns) { gen_jmp_im(dc->base.pc_next - dc->cs_base); gen_eob(dc);
Incrementally paves the way towards using the generic instruction translation loop. Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu> --- target/i386/translate.c | 72 +++++++++++++++++++++++++++++++---------------- 1 file changed, 48 insertions(+), 24 deletions(-)