[v2,3/4] target/i386: Make translator stop before the end of a page

Commit Message

Ilya Leoshkevich Aug. 5, 2022, 4:09 p.m. UTC

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.

We may find out that we crossed page boundary after some ops were
emitted and cc_op was updated. In theory it might be possible to
rearrange the code to disassemble first, but this is too error-prone.
Simply snapshot and restore the disassembly state instead.

Signed-off-by: Ilya Leoshkevich <[e-mail]>
---
 target/i386/tcg/translate.c | 42 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

Comments

Richard Henderson Aug. 5, 2022, 8:19 p.m. UTC | #1

On 8/5/22 09:09, Ilya Leoshkevich wrote:
> @@ -4568,9 +4598,19 @@ 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) {
> +    disas_save(&snapshot, s);
> +    switch (sigsetjmp(s->jmpbuf, 0)) {
> +    case 0:
> +        break;
> +    case 1:
>           gen_exception_gpf(s);
>           return s->pc;
> +    case 2:
> +        disas_restore(&snapshot, s);
> +        s->base.is_jmp = DISAS_TOO_MANY;
> +        return pc_start;
> +    default:

Similarly, this is too late for DISAS_TOO_MANY.

It will be more difficult to fix this for x86, since unlike s390x (or arm for that 
matter), we cannot probe for the total insn length within the first few bits of the insn.

The simplest possibility would to force single-stepping when we're within 16 bytes of the 
end of the page, as that's a hard maximum on the number of bytes within an x86 insn.

We could probably still use this sort of longjmp thing, but we'd need to unwind more than 
you're doing here.  We'd need to discard the insn_start opcode (which is before the 
last_op that you're currently saving), and decrement s->base.num_insns.


r~

Patch

diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index b7972f0ff5..ea749b0a04 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
@@ -4545,6 +4551,29 @@  static void gen_sse(CPUX86State *env, DisasContext *s, int b,
     }
 }
 
+/* Disassembly state that may affect the next instruction. */
+typedef struct {
+    TCGOp *last_op;
+    bool cc_op_dirty;
+    CCOp cc_op;
+} DisasSnapshot;
+
+/* Save disassembly state. */
+static void disas_save(DisasSnapshot *snapshot, const DisasContext *s)
+{
+    snapshot->last_op = tcg_last_op();
+    snapshot->cc_op_dirty = s->cc_op_dirty;
+    snapshot->cc_op = s->cc_op;
+}
+
+/* Restore disassembly state. */
+static void disas_restore(const DisasSnapshot *snapshot, DisasContext *s)
+{
+    tcg_remove_ops_after(snapshot->last_op);
+    s->cc_op_dirty = snapshot->cc_op_dirty;
+    s->cc_op = snapshot->cc_op;
+}
+
 /* 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)
@@ -4556,6 +4585,7 @@  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;
+    DisasSnapshot snapshot;
 
     s->pc_start = s->pc = pc_start;
     s->override = -1;
@@ -4568,9 +4598,19 @@  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) {
+    disas_save(&snapshot, s);
+    switch (sigsetjmp(s->jmpbuf, 0)) {
+    case 0:
+        break;
+    case 1:
         gen_exception_gpf(s);
         return s->pc;
+    case 2:
+        disas_restore(&snapshot, s);
+        s->base.is_jmp = DISAS_TOO_MANY;
+        return pc_start;
+    default:
+        g_assert_not_reached();
     }
 
     prefixes = 0;