| Message ID | 20220808171022.49439-4-iii@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | linux-user: Fix siginfo_t contents when jumping to non-readable pages | expand |
On 8/8/22 10:10, Ilya Leoshkevich wrote: > Right now translator stops right *after* the end of a page, which > breaks reporting of fault locations when the last instruction of a > multi-insn translation block crosses a page boundary. > > An implementation, like the one arm and s390x have, would require an > i386 length disassembler, which is burdensome to maintain. Another > alternative would be to single-step at the end of a guest page, but > this may come with a performance impact. > > Fix by snapshotting disassembly state and restoring it after we figure > out we crossed a page boundary. This includes rolling back cc_op > updates and emitted ops. Even though i386 is the only architecture that > does rollback, split it into common and architecture-dependent parts to > improve readability. > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > --- > accel/tcg/translator.c | 8 ++++++++ > include/exec/translator.h | 3 +++ > target/i386/tcg/translate.c | 21 ++++++++++++++++++++- > 3 files changed, 31 insertions(+), 1 deletion(-) > > diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c > index fe7af9b943..2c4dd09df8 100644 > --- a/accel/tcg/translator.c > +++ b/accel/tcg/translator.c > @@ -56,6 +56,7 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db, > { > uint32_t cflags = tb_cflags(tb); > bool plugin_enabled; > + TCGOp *last_op; > > /* Initialize DisasContext */ > db->tb = tb; > @@ -82,6 +83,7 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db, > > while (true) { > db->num_insns++; > + last_op = tcg_last_op(); > ops->insn_start(db, cpu); > tcg_debug_assert(db->is_jmp == DISAS_NEXT); /* no early exit */ > > @@ -103,6 +105,12 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db, > ops->translate_insn(db, cpu); > } > > + if (db->is_jmp == DISAS_TOO_MANY_UNDO) { > + db->num_insns--; > + tcg_remove_ops_after(last_op); > + db->is_jmp = DISAS_TOO_MANY; > + } > + > /* Stop translation if translate_insn so indicated. */ > if (db->is_jmp != DISAS_NEXT) { > break; > diff --git a/include/exec/translator.h b/include/exec/translator.h > index d27f8c33b6..e1533aee87 100644 > --- a/include/exec/translator.h > +++ b/include/exec/translator.h > @@ -31,6 +31,8 @@ > * DisasJumpType: > * @DISAS_NEXT: Next instruction in program order. > * @DISAS_TOO_MANY: Too many instructions translated. > + * @DISAS_TOO_MANY_UNDO: Too many instructions translated. Everything that was > + * done for the current instruction must be undone. > * @DISAS_NORETURN: Following code is dead. > * @DISAS_TARGET_*: Start of target-specific conditions. > * > @@ -39,6 +41,7 @@ > typedef enum DisasJumpType { > DISAS_NEXT, > DISAS_TOO_MANY, > + DISAS_TOO_MANY_UNDO, Hmm, maybe. I'm not overly keen on the generic change, because I think it would be easy to use incorrectly. > + case 2: > + /* Restore state that may affect the next instruction. */ > + s->cc_op_dirty = orig_cc_op_dirty; > + s->cc_op = orig_cc_op; > + s->base.is_jmp = DISAS_TOO_MANY_UNDO; I think you can simply set s->prev_insn_end in i386_tr_insn_start, for discarding opcodes. r~
diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c index fe7af9b943..2c4dd09df8 100644 --- a/accel/tcg/translator.c +++ b/accel/tcg/translator.c @@ -56,6 +56,7 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db, { uint32_t cflags = tb_cflags(tb); bool plugin_enabled; + TCGOp *last_op; /* Initialize DisasContext */ db->tb = tb; @@ -82,6 +83,7 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db, while (true) { db->num_insns++; + last_op = tcg_last_op(); ops->insn_start(db, cpu); tcg_debug_assert(db->is_jmp == DISAS_NEXT); /* no early exit */ @@ -103,6 +105,12 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db, ops->translate_insn(db, cpu); } + if (db->is_jmp == DISAS_TOO_MANY_UNDO) { + db->num_insns--; + tcg_remove_ops_after(last_op); + db->is_jmp = DISAS_TOO_MANY; + } + /* Stop translation if translate_insn so indicated. */ if (db->is_jmp != DISAS_NEXT) { break; diff --git a/include/exec/translator.h b/include/exec/translator.h index d27f8c33b6..e1533aee87 100644 --- a/include/exec/translator.h +++ b/include/exec/translator.h @@ -31,6 +31,8 @@ * DisasJumpType: * @DISAS_NEXT: Next instruction in program order. * @DISAS_TOO_MANY: Too many instructions translated. + * @DISAS_TOO_MANY_UNDO: Too many instructions translated. Everything that was + * done for the current instruction must be undone. * @DISAS_NORETURN: Following code is dead. * @DISAS_TARGET_*: Start of target-specific conditions. * @@ -39,6 +41,7 @@ typedef enum DisasJumpType { DISAS_NEXT, DISAS_TOO_MANY, + DISAS_TOO_MANY_UNDO, DISAS_NORETURN, DISAS_TARGET_0, DISAS_TARGET_1, diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c index b7972f0ff5..14d4ed1412 100644 --- a/target/i386/tcg/translate.c +++ b/target/i386/tcg/translate.c @@ -2008,6 +2008,12 @@ static uint64_t advance_pc(CPUX86State *env, DisasContext *s, int num_bytes) { uint64_t pc = s->pc; + /* This is a subsequent insn that crosses a page boundary. */ + if (s->base.num_insns > 1 && + !is_same_page(&s->base, s->pc + num_bytes - 1)) { + siglongjmp(s->jmpbuf, 2); + } + s->pc += num_bytes; if (unlikely(s->pc - s->pc_start > X86_MAX_INSN_LENGTH)) { /* If the instruction's 16th byte is on a different page than the 1st, a @@ -4556,6 +4562,8 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu) int modrm, reg, rm, mod, op, opreg, val; target_ulong next_eip, tval; target_ulong pc_start = s->base.pc_next; + bool orig_cc_op_dirty = s->cc_op_dirty; + CCOp orig_cc_op = s->cc_op; s->pc_start = s->pc = pc_start; s->override = -1; @@ -4568,9 +4576,20 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu) s->rip_offset = 0; /* for relative ip address */ s->vex_l = 0; s->vex_v = 0; - if (sigsetjmp(s->jmpbuf, 0) != 0) { + switch (sigsetjmp(s->jmpbuf, 0)) { + case 0: + break; + case 1: gen_exception_gpf(s); return s->pc; + case 2: + /* Restore state that may affect the next instruction. */ + s->cc_op_dirty = orig_cc_op_dirty; + s->cc_op = orig_cc_op; + s->base.is_jmp = DISAS_TOO_MANY_UNDO; + return pc_start; + default: + g_assert_not_reached(); } prefixes = 0;
Right now translator stops right *after* the end of a page, which breaks reporting of fault locations when the last instruction of a multi-insn translation block crosses a page boundary. An implementation, like the one arm and s390x have, would require an i386 length disassembler, which is burdensome to maintain. Another alternative would be to single-step at the end of a guest page, but this may come with a performance impact. Fix by snapshotting disassembly state and restoring it after we figure out we crossed a page boundary. This includes rolling back cc_op updates and emitted ops. Even though i386 is the only architecture that does rollback, split it into common and architecture-dependent parts to improve readability. Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> --- accel/tcg/translator.c | 8 ++++++++ include/exec/translator.h | 3 +++ target/i386/tcg/translate.c | 21 ++++++++++++++++++++- 3 files changed, 31 insertions(+), 1 deletion(-)