| Message ID | 20220906100932.343523-3-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/i386: pc-relative translation blocks | expand |
On 6/9/22 12:09, Richard Henderson wrote: > Instead of returning the new pc, which is present in > DisasContext, return true if an insn was translated. > This is false when we detect a page crossing and must > undo the insn under translation. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/i386/tcg/translate.c | 42 +++++++++++++++++++------------------ > 1 file changed, 22 insertions(+), 20 deletions(-) > > diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c > index 1e24bb2985..46300ffd91 100644 > --- a/target/i386/tcg/translate.c > +++ b/target/i386/tcg/translate.c > @@ -4665,7 +4665,7 @@ 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(DisasContext *s, CPUState *cpu) > +static bool disas_insn(DisasContext *s, CPUState *cpu) > { > CPUX86State *env = cpu->env_ptr; > int b, prefixes; > @@ -4695,12 +4695,13 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu) > return s->pc; Shouldn't we return 'true' here? > case 2: > /* Restore state that may affect the next instruction. */ > + s->pc = s->base.pc_next; > s->cc_op_dirty = orig_cc_op_dirty; > s->cc_op = orig_cc_op; > s->base.num_insns--; > tcg_remove_ops_after(s->prev_insn_end); > s->base.is_jmp = DISAS_TOO_MANY; > - return s->base.pc_next; > + return false; > default: > g_assert_not_reached(); > } > @@ -8609,13 +8610,13 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu) > default: > goto unknown_op; > } > - return s->pc; > + return true; > illegal_op: > gen_illegal_opcode(s); > - return s->pc; > + return true; > unknown_op: > gen_unknown_opcode(env, s); > - return s->pc; > + return true; > }
On 9/6/22 15:42, Philippe Mathieu-Daudé wrote: > On 6/9/22 12:09, Richard Henderson wrote: >> Instead of returning the new pc, which is present in >> DisasContext, return true if an insn was translated. >> This is false when we detect a page crossing and must >> undo the insn under translation. >> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> target/i386/tcg/translate.c | 42 +++++++++++++++++++------------------ >> 1 file changed, 22 insertions(+), 20 deletions(-) >> >> diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c >> index 1e24bb2985..46300ffd91 100644 >> --- a/target/i386/tcg/translate.c >> +++ b/target/i386/tcg/translate.c >> @@ -4665,7 +4665,7 @@ 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(DisasContext *s, CPUState *cpu) >> +static bool disas_insn(DisasContext *s, CPUState *cpu) >> { >> CPUX86State *env = cpu->env_ptr; >> int b, prefixes; >> @@ -4695,12 +4695,13 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu) >> return s->pc; > > Shouldn't we return 'true' here? Whoops, yes. r~
On 8/9/22 14:14, Richard Henderson wrote: > On 9/6/22 15:42, Philippe Mathieu-Daudé wrote: >> On 6/9/22 12:09, Richard Henderson wrote: >>> Instead of returning the new pc, which is present in >>> DisasContext, return true if an insn was translated. >>> This is false when we detect a page crossing and must >>> undo the insn under translation. >>> >>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >>> --- >>> target/i386/tcg/translate.c | 42 +++++++++++++++++++------------------ >>> 1 file changed, 22 insertions(+), 20 deletions(-) >>> >>> diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c >>> index 1e24bb2985..46300ffd91 100644 >>> --- a/target/i386/tcg/translate.c >>> +++ b/target/i386/tcg/translate.c >>> @@ -4665,7 +4665,7 @@ 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(DisasContext *s, CPUState *cpu) >>> +static bool disas_insn(DisasContext *s, CPUState *cpu) >>> { >>> CPUX86State *env = cpu->env_ptr; >>> int b, prefixes; >>> @@ -4695,12 +4695,13 @@ static target_ulong disas_insn(DisasContext >>> *s, CPUState *cpu) >>> return s->pc; >> >> Shouldn't we return 'true' here? > > Whoops, yes. Returning 'true': Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c index 1e24bb2985..46300ffd91 100644 --- a/target/i386/tcg/translate.c +++ b/target/i386/tcg/translate.c @@ -4665,7 +4665,7 @@ 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(DisasContext *s, CPUState *cpu) +static bool disas_insn(DisasContext *s, CPUState *cpu) { CPUX86State *env = cpu->env_ptr; int b, prefixes; @@ -4695,12 +4695,13 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu) return s->pc; case 2: /* Restore state that may affect the next instruction. */ + s->pc = s->base.pc_next; s->cc_op_dirty = orig_cc_op_dirty; s->cc_op = orig_cc_op; s->base.num_insns--; tcg_remove_ops_after(s->prev_insn_end); s->base.is_jmp = DISAS_TOO_MANY; - return s->base.pc_next; + return false; default: g_assert_not_reached(); } @@ -8609,13 +8610,13 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu) default: goto unknown_op; } - return s->pc; + return true; illegal_op: gen_illegal_opcode(s); - return s->pc; + return true; unknown_op: gen_unknown_opcode(env, s); - return s->pc; + return true; } void tcg_x86_init(void) @@ -8780,7 +8781,6 @@ static void i386_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu) static void i386_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu) { DisasContext *dc = container_of(dcbase, DisasContext, base); - target_ulong pc_next; #ifdef TARGET_VSYSCALL_PAGE /* @@ -8793,21 +8793,23 @@ static void i386_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu) } #endif - pc_next = disas_insn(dc, cpu); - dc->base.pc_next = pc_next; + if (disas_insn(dc, cpu)) { + target_ulong pc_next = dc->pc; + dc->base.pc_next = pc_next; - if (dc->base.is_jmp == DISAS_NEXT) { - if (dc->flags & (HF_TF_MASK | 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 - * chance to happen. - */ - dc->base.is_jmp = DISAS_TOO_MANY; - } else if (!is_same_page(&dc->base, pc_next)) { - dc->base.is_jmp = DISAS_TOO_MANY; + if (dc->base.is_jmp == DISAS_NEXT) { + if (dc->flags & (HF_TF_MASK | 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 + * chance to happen. + */ + dc->base.is_jmp = DISAS_TOO_MANY; + } else if (!is_same_page(&dc->base, pc_next)) { + dc->base.is_jmp = DISAS_TOO_MANY; + } } } }
Instead of returning the new pc, which is present in DisasContext, return true if an insn was translated. This is false when we detect a page crossing and must undo the insn under translation. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/i386/tcg/translate.c | 42 +++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 20 deletions(-)