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,
     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;
-- 
2.37.1

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~