| Message ID | 20220906100932.343523-20-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/i386: pc-relative translation blocks | expand |
On Tue, Sep 6, 2022 at 12:09 PM Richard Henderson <richard.henderson@linaro.org> wrote: > -static inline void gen_jcc(DisasContext *s, int b, > - target_ulong val, target_ulong next_eip) > +static void gen_jcc(DisasContext *s, MemOp ot, int b, int diff) > { > - TCGLabel *l1, *l2; > + TCGLabel *l1 = gen_new_label(); > > - if (s->jmp_opt) { > - l1 = gen_new_label(); > - gen_jcc1(s, b, l1); > - > - gen_goto_tb(s, 0, next_eip); > - > - gen_set_label(l1); > - gen_goto_tb(s, 1, val); > - } else { > - l1 = gen_new_label(); > - l2 = gen_new_label(); > - gen_jcc1(s, b, l1); > - > - gen_jmp_im(s, next_eip); > - tcg_gen_br(l2); > - > - gen_set_label(l1); > - gen_jmp_im(s, val); > - gen_set_label(l2); > - gen_eob(s); > - } > + gen_jcc1(s, b, l1); > + gen_jmp_rel(s, ot, 0, 1); > + gen_set_label(l1); > + gen_jmp_rel(s, ot, diff, 0); Might be worth a comment that jumps with 16-bit operand size truncate EIP even if the jump is not taken. Otherwise, Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Paolo > } > > static void gen_cmovcc1(CPUX86State *env, DisasContext *s, MemOp ot, int b, > @@ -4721,7 +4703,6 @@ static bool disas_insn(DisasContext *s, CPUState *cpu) > int shift; > MemOp ot, aflag, dflag; > int modrm, reg, rm, mod, op, opreg, val; > - target_ulong next_eip, tval; > bool orig_cc_op_dirty = s->cc_op_dirty; > CCOp orig_cc_op = s->cc_op; > > @@ -6881,22 +6862,20 @@ static bool disas_insn(DisasContext *s, CPUState *cpu) > } > break; > case 0x70 ... 0x7f: /* jcc Jb */ > - tval = (int8_t)insn_get(env, s, MO_8); > - goto do_jcc; > + { > + int diff = (int8_t)insn_get(env, s, MO_8); > + gen_bnd_jmp(s); > + gen_jcc(s, dflag, b, diff); > + } > + break; > case 0x180 ... 0x18f: /* jcc Jv */ > - if (dflag != MO_16) { > - tval = (int32_t)insn_get(env, s, MO_32); > - } else { > - tval = (int16_t)insn_get(env, s, MO_16); > + { > + int diff = (dflag != MO_16 > + ? (int32_t)insn_get(env, s, MO_32) > + : (int16_t)insn_get(env, s, MO_16)); > + gen_bnd_jmp(s); > + gen_jcc(s, dflag, b, diff); > } > - do_jcc: > - next_eip = s->pc - s->cs_base; > - tval += next_eip; > - if (dflag == MO_16) { > - tval &= 0xffff; > - } > - gen_bnd_jmp(s); > - gen_jcc(s, b, tval, next_eip); > break; > > case 0x190 ... 0x19f: /* setcc Gv */ > -- > 2.34.1 >
On 9/21/22 06:09, Paolo Bonzini wrote: > On Tue, Sep 6, 2022 at 12:09 PM Richard Henderson > <richard.henderson@linaro.org> wrote: >> -static inline void gen_jcc(DisasContext *s, int b, >> - target_ulong val, target_ulong next_eip) >> +static void gen_jcc(DisasContext *s, MemOp ot, int b, int diff) >> { >> - TCGLabel *l1, *l2; >> + TCGLabel *l1 = gen_new_label(); >> >> - if (s->jmp_opt) { >> - l1 = gen_new_label(); >> - gen_jcc1(s, b, l1); >> - >> - gen_goto_tb(s, 0, next_eip); >> - >> - gen_set_label(l1); >> - gen_goto_tb(s, 1, val); >> - } else { >> - l1 = gen_new_label(); >> - l2 = gen_new_label(); >> - gen_jcc1(s, b, l1); >> - >> - gen_jmp_im(s, next_eip); >> - tcg_gen_br(l2); >> - >> - gen_set_label(l1); >> - gen_jmp_im(s, val); >> - gen_set_label(l2); >> - gen_eob(s); >> - } >> + gen_jcc1(s, b, l1); >> + gen_jmp_rel(s, ot, 0, 1); >> + gen_set_label(l1); >> + gen_jmp_rel(s, ot, diff, 0); > > Might be worth a comment that jumps with 16-bit operand size truncate > EIP even if the jump is not taken. Hmm. But is that correct? That's not reflected by the pseudocode for Jcc. r~
On Sat, Oct 1, 2022 at 3:04 AM Richard Henderson <richard.henderson@linaro.org> wrote: > > On 9/21/22 06:09, Paolo Bonzini wrote: > > On Tue, Sep 6, 2022 at 12:09 PM Richard Henderson > > <richard.henderson@linaro.org> wrote: > > > + gen_jcc1(s, b, l1); > > > + gen_jmp_rel(s, ot, 0, 1); > > > + gen_set_label(l1); > > > + gen_jmp_rel(s, ot, diff, 0); > > > > Might be worth a comment that jumps with 16-bit operand size truncate > > EIP even if the jump is not taken. > > Hmm. But is that correct? That's not reflected by the pseudocode for Jcc. No, it's not: int main() { asm("clc; data16 jc 1f; 1:"); } does not crash (it does with stc) on real hardware, but it does with this series applied. So the various occurrences of gen_jmp_rel(s, ot, 0, 1) or gen_jmp_rel(s, MO_32, 0, 1) should stay as gen_jmp_tb(s, s->pc - s->cs_base, 1). Paolo
On 10/1/22 00:03, Paolo Bonzini wrote: > On Sat, Oct 1, 2022 at 3:04 AM Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> On 9/21/22 06:09, Paolo Bonzini wrote: >>> On Tue, Sep 6, 2022 at 12:09 PM Richard Henderson >>> <richard.henderson@linaro.org> wrote: >>>> + gen_jcc1(s, b, l1); >>>> + gen_jmp_rel(s, ot, 0, 1); >>>> + gen_set_label(l1); >>>> + gen_jmp_rel(s, ot, diff, 0); >>> >>> Might be worth a comment that jumps with 16-bit operand size truncate >>> EIP even if the jump is not taken. >> >> Hmm. But is that correct? That's not reflected by the pseudocode for Jcc. > > No, it's not: > > int main() { > asm("clc; data16 jc 1f; 1:"); > } > > does not crash (it does with stc) on real hardware, but it does with > this series applied. So the various occurrences of gen_jmp_rel(s, ot, > 0, 1) or gen_jmp_rel(s, MO_32, 0, 1) should stay as gen_jmp_tb(s, > s->pc - s->cs_base, 1). Nice test. I had an idea this would be the case, so I had already added a helper to perform the jump with truncation to the "current code size". It turned out that I needed that in other places too, like rep. New patch set coming up. r~
diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c index fdd17c3cf3..e27f36e4e9 100644 --- a/target/i386/tcg/translate.c +++ b/target/i386/tcg/translate.c @@ -2373,32 +2373,14 @@ static void gen_goto_tb(DisasContext *s, int tb_num, target_ulong eip) } } -static inline void gen_jcc(DisasContext *s, int b, - target_ulong val, target_ulong next_eip) +static void gen_jcc(DisasContext *s, MemOp ot, int b, int diff) { - TCGLabel *l1, *l2; + TCGLabel *l1 = gen_new_label(); - if (s->jmp_opt) { - l1 = gen_new_label(); - gen_jcc1(s, b, l1); - - gen_goto_tb(s, 0, next_eip); - - gen_set_label(l1); - gen_goto_tb(s, 1, val); - } else { - l1 = gen_new_label(); - l2 = gen_new_label(); - gen_jcc1(s, b, l1); - - gen_jmp_im(s, next_eip); - tcg_gen_br(l2); - - gen_set_label(l1); - gen_jmp_im(s, val); - gen_set_label(l2); - gen_eob(s); - } + gen_jcc1(s, b, l1); + gen_jmp_rel(s, ot, 0, 1); + gen_set_label(l1); + gen_jmp_rel(s, ot, diff, 0); } static void gen_cmovcc1(CPUX86State *env, DisasContext *s, MemOp ot, int b, @@ -4721,7 +4703,6 @@ static bool disas_insn(DisasContext *s, CPUState *cpu) int shift; MemOp ot, aflag, dflag; int modrm, reg, rm, mod, op, opreg, val; - target_ulong next_eip, tval; bool orig_cc_op_dirty = s->cc_op_dirty; CCOp orig_cc_op = s->cc_op; @@ -6881,22 +6862,20 @@ static bool disas_insn(DisasContext *s, CPUState *cpu) } break; case 0x70 ... 0x7f: /* jcc Jb */ - tval = (int8_t)insn_get(env, s, MO_8); - goto do_jcc; + { + int diff = (int8_t)insn_get(env, s, MO_8); + gen_bnd_jmp(s); + gen_jcc(s, dflag, b, diff); + } + break; case 0x180 ... 0x18f: /* jcc Jv */ - if (dflag != MO_16) { - tval = (int32_t)insn_get(env, s, MO_32); - } else { - tval = (int16_t)insn_get(env, s, MO_16); + { + int diff = (dflag != MO_16 + ? (int32_t)insn_get(env, s, MO_32) + : (int16_t)insn_get(env, s, MO_16)); + gen_bnd_jmp(s); + gen_jcc(s, dflag, b, diff); } - do_jcc: - next_eip = s->pc - s->cs_base; - tval += next_eip; - if (dflag == MO_16) { - tval &= 0xffff; - } - gen_bnd_jmp(s); - gen_jcc(s, b, tval, next_eip); break; case 0x190 ... 0x19f: /* setcc Gv */
Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/i386/tcg/translate.c | 57 ++++++++++++------------------------- 1 file changed, 18 insertions(+), 39 deletions(-)