| Message ID | 20220906100932.343523-18-richard.henderson@linaro.org |
|---|---|
| State | New |
| 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: > > Create a common helper for pc-relative branches. > The jmp jb insn was missing a mask for CODE32. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> (Oops, my remark the previous patch should still have pointed to gen_jmp_tb). In gen_jz_ecx_string, in the translation for LOOPNZ/LOOPZ/LOOP/JECXZ and in i386_tr_tb_stop there is: > - gen_jmp_tb(s, s->pc - s->cs_base, 1); > + gen_jmp_rel(s, MO_32, 0, 1); What happens if the instruction's last byte is at 0xffff? Wraparound in the middle of an instruction is generally undefined, but I think it should work if the instruction does not cross the 64K/4G limit (and on real hardware, which obeys segment limits unlike TCG, said limit must be 64K/4G of course). In other words, why MO_32 and not "CODE32(s) ? MO_32 : MO_16"? Likewise, if you change that you need to change gen_repz/gen_repz2 too. Paolo > gen_set_label(l1); > return l2; > } > @@ -2756,6 +2757,18 @@ static void gen_jmp_tb(DisasContext *s, target_ulong eip, int tb_num) > } > } > > +static void gen_jmp_rel(DisasContext *s, MemOp ot, int diff, int tb_num) > +{ > + target_ulong dest = s->pc - s->cs_base + diff; > + > + if (ot == MO_16) { > + dest &= 0xffff; > + } else if (!CODE64(s)) { > + dest &= 0xffffffff; > + } > + gen_jmp_tb(s, dest, tb_num); > +} > + > static void gen_jmp(DisasContext *s, target_ulong eip) > { > gen_jmp_tb(s, eip, 0); > @@ -6816,20 +6829,12 @@ static bool disas_insn(DisasContext *s, CPUState *cpu) > break; > case 0xe8: /* call im */ > { > - if (dflag != MO_16) { > - tval = (int32_t)insn_get(env, s, MO_32); > - } else { > - tval = (int16_t)insn_get(env, s, MO_16); > - } > - tval += s->pc - s->cs_base; > - if (dflag == MO_16) { > - tval &= 0xffff; > - } else if (!CODE64(s)) { > - tval &= 0xffffffff; > - } > + int diff = (dflag != MO_16 > + ? (int32_t)insn_get(env, s, MO_32) > + : (int16_t)insn_get(env, s, MO_16)); > gen_push_v(s, eip_next_tl(s)); > gen_bnd_jmp(s); > - gen_jmp(s, tval); > + gen_jmp_rel(s, dflag, diff, 0); > } > break; > case 0x9a: /* lcall im */ > @@ -6847,19 +6852,13 @@ static bool disas_insn(DisasContext *s, CPUState *cpu) > } > goto do_lcall; > case 0xe9: /* jmp im */ > - 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_jmp_rel(s, dflag, diff, 0); > } > - tval += s->pc - s->cs_base; > - if (dflag == MO_16) { > - tval &= 0xffff; > - } else if (!CODE64(s)) { > - tval &= 0xffffffff; > - } > - gen_bnd_jmp(s); > - gen_jmp(s, tval); > break; > case 0xea: /* ljmp im */ > { > @@ -6876,12 +6875,10 @@ static bool disas_insn(DisasContext *s, CPUState *cpu) > } > goto do_ljmp; > case 0xeb: /* jmp Jb */ > - tval = (int8_t)insn_get(env, s, MO_8); > - tval += s->pc - s->cs_base; > - if (dflag == MO_16) { > - tval &= 0xffff; > + { > + int diff = (int8_t)insn_get(env, s, MO_8); > + gen_jmp_rel(s, dflag, diff, 0); > } > - gen_jmp(s, tval); > break; > case 0x70 ... 0x7f: /* jcc Jb */ > tval = (int8_t)insn_get(env, s, MO_8); > -- > 2.34.1 >
On 9/21/22 06:06, Paolo Bonzini wrote: > On Tue, Sep 6, 2022 at 12:09 PM Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> Create a common helper for pc-relative branches. >> The jmp jb insn was missing a mask for CODE32. >> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > > (Oops, my remark the previous patch should still have pointed to gen_jmp_tb). > > In gen_jz_ecx_string, in the translation for LOOPNZ/LOOPZ/LOOP/JECXZ > and in i386_tr_tb_stop there is: > >> - gen_jmp_tb(s, s->pc - s->cs_base, 1); >> + gen_jmp_rel(s, MO_32, 0, 1); > > What happens if the instruction's last byte is at 0xffff? Wraparound > in the middle of an instruction is generally undefined, but I think it > should work if the instruction does not cross the 64K/4G limit (and on > real hardware, which obeys segment limits unlike TCG, said limit must > be 64K/4G of course). > > In other words, why MO_32 and not "CODE32(s) ? MO_32 : MO_16"? I believe it really should be s->dflag, which makes all users of the function pass dflag (the manual consistently talks about "operand size"). At which point this parameter goes away and gen_jmp_rel grabs the operand size from DisasContext. Also, pre-existing bug vs CODE64 here -- operand size is always 64-bits for near jumps. r~
On Sat, Oct 1, 2022 at 2:53 AM Richard Henderson <richard.henderson@linaro.org> wrote: > I believe it really should be s->dflag, which makes all users of the function pass dflag > (the manual consistently talks about "operand size"). At which point this parameter goes > away and gen_jmp_rel grabs the operand size from DisasContext. > > Also, pre-existing bug vs CODE64 here -- operand size is always 64-bits for near jumps. Yes, sounds good. Paolo
diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c index cedc195837..07c7764649 100644 --- a/target/i386/tcg/translate.c +++ b/target/i386/tcg/translate.c @@ -226,6 +226,7 @@ static void gen_eob(DisasContext *s); static void gen_jr(DisasContext *s); static void gen_jmp(DisasContext *s, target_ulong eip); static void gen_jmp_tb(DisasContext *s, target_ulong eip, int tb_num); +static void gen_jmp_rel(DisasContext *s, MemOp ot, int diff, int tb_num); static void gen_op(DisasContext *s1, int op, MemOp ot, int d); static void gen_exception_gpf(DisasContext *s); @@ -1173,7 +1174,7 @@ static TCGLabel *gen_jz_ecx_string(DisasContext *s) TCGLabel *l2 = gen_new_label(); gen_op_jnz_ecx(s, s->aflag, l1); gen_set_label(l2); - gen_jmp_tb(s, s->pc - s->cs_base, 1); + gen_jmp_rel(s, MO_32, 0, 1); gen_set_label(l1); return l2; } @@ -2756,6 +2757,18 @@ static void gen_jmp_tb(DisasContext *s, target_ulong eip, int tb_num) } } +static void gen_jmp_rel(DisasContext *s, MemOp ot, int diff, int tb_num) +{ + target_ulong dest = s->pc - s->cs_base + diff; + + if (ot == MO_16) { + dest &= 0xffff; + } else if (!CODE64(s)) { + dest &= 0xffffffff; + } + gen_jmp_tb(s, dest, tb_num); +} + static void gen_jmp(DisasContext *s, target_ulong eip) { gen_jmp_tb(s, eip, 0); @@ -6816,20 +6829,12 @@ static bool disas_insn(DisasContext *s, CPUState *cpu) break; case 0xe8: /* call im */ { - if (dflag != MO_16) { - tval = (int32_t)insn_get(env, s, MO_32); - } else { - tval = (int16_t)insn_get(env, s, MO_16); - } - tval += s->pc - s->cs_base; - if (dflag == MO_16) { - tval &= 0xffff; - } else if (!CODE64(s)) { - tval &= 0xffffffff; - } + int diff = (dflag != MO_16 + ? (int32_t)insn_get(env, s, MO_32) + : (int16_t)insn_get(env, s, MO_16)); gen_push_v(s, eip_next_tl(s)); gen_bnd_jmp(s); - gen_jmp(s, tval); + gen_jmp_rel(s, dflag, diff, 0); } break; case 0x9a: /* lcall im */ @@ -6847,19 +6852,13 @@ static bool disas_insn(DisasContext *s, CPUState *cpu) } goto do_lcall; case 0xe9: /* jmp im */ - 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_jmp_rel(s, dflag, diff, 0); } - tval += s->pc - s->cs_base; - if (dflag == MO_16) { - tval &= 0xffff; - } else if (!CODE64(s)) { - tval &= 0xffffffff; - } - gen_bnd_jmp(s); - gen_jmp(s, tval); break; case 0xea: /* ljmp im */ { @@ -6876,12 +6875,10 @@ static bool disas_insn(DisasContext *s, CPUState *cpu) } goto do_ljmp; case 0xeb: /* jmp Jb */ - tval = (int8_t)insn_get(env, s, MO_8); - tval += s->pc - s->cs_base; - if (dflag == MO_16) { - tval &= 0xffff; + { + int diff = (int8_t)insn_get(env, s, MO_8); + gen_jmp_rel(s, dflag, diff, 0); } - gen_jmp(s, tval); break; case 0x70 ... 0x7f: /* jcc Jb */ tval = (int8_t)insn_get(env, s, MO_8);
Create a common helper for pc-relative branches. The jmp jb insn was missing a mask for CODE32. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/i386/tcg/translate.c | 57 ++++++++++++++++++------------------- 1 file changed, 27 insertions(+), 30 deletions(-)