[PATCH 00/11] target/i386: fixes for INHIBIT_IRQ, TF and RF

[PATCH 00/11] target/i386: fixes for INHIBIT_IRQ, TF and RF

[Paolo Bonzini]

While Richard pointed out that there _are_ good reasons to use
DISAS_NORETURN in the TCG x86 frontend, most instructions that
use it (VMRUN, HLT, MWAIT, PAUSE) are broken because they do
not do the work that gen_eob() does on INHIBIT_IRQ, EFLAGS.TF
and EFLAGS.RF.

This series tackles this, plus it has a few more fixes for
failures in kvm-unit-tests debug.flat and svm.flat.  Note that
neither of the two completely pass, but the situation is
improved a lot.  Comments are added when things are more
complicated and probably deserve their own series.

Paolo

Paolo Bonzini (11):
  target/i386: fix pushed value of EFLAGS.RF
  target/i386: fix implementation of ICEBP
  target/i386: cleanup HLT helpers
  target/i386: cleanup PAUSE helpers
  target/i386: implement DR7.GD
  target/i386: disable/enable breakpoints on vmentry/vmexit
  target/i386: fix INHIBIT_IRQ/TF/RF handling for VMRUN
  target/i386: fix INHIBIT_IRQ/TF/RF handling for PAUSE
  target/i386: fix TF/RF handling for HLT
  target/i386: document incorrect semantics of watchpoint following
    MOV/POP SS
  target/i386: document use of DISAS_NORETURN

 include/hw/core/tcg-cpu-ops.h        | 15 +++++-
 target/i386/helper.h                 |  5 +-
 target/i386/tcg/helper-tcg.h         |  6 ++-
 accel/tcg/cpu-exec.c                 |  7 ++-
 target/i386/tcg/bpt_helper.c         |  6 +++
 target/i386/tcg/excp_helper.c        | 20 ++++++++
 target/i386/tcg/misc_helper.c        | 14 ++----
 target/i386/tcg/seg_helper.c         | 49 +++++++++++++++++--
 target/i386/tcg/sysemu/bpt_helper.c  | 18 +++++++
 target/i386/tcg/sysemu/misc_helper.c | 17 ++-----
 target/i386/tcg/sysemu/seg_helper.c  | 18 +++++--
 target/i386/tcg/sysemu/svm_helper.c  | 71 +++++++++++++++++++---------
 target/i386/tcg/translate.c          | 37 +++++++++++++++
 target/i386/tcg/decode-new.c.inc     | 19 ++++++--
 target/i386/tcg/emit.c.inc           | 29 ++++++------
 15 files changed, 252 insertions(+), 79 deletions(-)

-- 
2.45.1

[PATCH 01/11] target/i386: fix pushed value of EFLAGS.RF

[Paolo Bonzini]

When preparing an exception stack frame for a fault exception, the value
pushed for RF is 1.  Take that into account.  The same should be true
of interrupts for repeated string instructions, but the situation there
is complicated.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/tcg/seg_helper.c | 49 ++++++++++++++++++++++++++++++++----
 target/i386/tcg/translate.c  |  8 ++++++
 2 files changed, 52 insertions(+), 5 deletions(-)

diff --git a/target/i386/tcg/seg_helper.c b/target/i386/tcg/seg_helper.c
index 0301459004e..715db1f2326 100644
--- a/target/i386/tcg/seg_helper.c
+++ b/target/i386/tcg/seg_helper.c
@@ -526,6 +526,24 @@ static inline unsigned int get_sp_mask(unsigned int e2)
     }
 }
 
+static int exception_is_fault(int intno)
+{
+    switch (intno) {
+        /*
+         * #DB can be both fault- and trap-like, but it never sets RF=1
+         * in the RFLAGS value pushed on the stack.
+         */
+    case EXCP01_DB:
+    case EXCP03_INT3:
+    case EXCP04_INTO:
+    case EXCP08_DBLE:
+    case EXCP12_MCHK:
+        return 0;
+    }
+    /* Everything else including reserved exception is a fault.  */
+    return 1;
+}
+
 int exception_has_error_code(int intno)
 {
     switch (intno) {
@@ -605,8 +623,9 @@ static void do_interrupt_protected(CPUX86State *env, int intno, int is_int,
     int type, dpl, selector, ss_dpl, cpl;
     int has_error_code, new_stack, shift;
     uint32_t e1, e2, offset, ss = 0, esp, ss_e1 = 0, ss_e2 = 0;
-    uint32_t old_eip, sp_mask;
+    uint32_t old_eip, sp_mask, eflags;
     int vm86 = env->eflags & VM_MASK;
+    bool set_rf;
 
     has_error_code = 0;
     if (!is_int && !is_hw) {
@@ -614,8 +633,10 @@ static void do_interrupt_protected(CPUX86State *env, int intno, int is_int,
     }
     if (is_int) {
         old_eip = next_eip;
+        set_rf = false;
     } else {
         old_eip = env->eip;
+        set_rf = exception_is_fault(intno);
     }
 
     dt = &env->idt;
@@ -748,6 +769,15 @@ static void do_interrupt_protected(CPUX86State *env, int intno, int is_int,
     }
     push_size <<= shift;
 #endif
+    eflags = cpu_compute_eflags(env);
+    /*
+     * AMD states that code breakpoint #DBs clear RF=0, Intel leaves it
+     * as is.  AMD behavior could be implemented in check_hw_breakpoints().
+     */
+    if (set_rf) {
+        eflags |= RF_MASK;
+    }
+
     if (shift == 1) {
         if (new_stack) {
             if (vm86) {
@@ -759,7 +789,7 @@ static void do_interrupt_protected(CPUX86State *env, int intno, int is_int,
             PUSHL(ssp, esp, sp_mask, env->segs[R_SS].selector);
             PUSHL(ssp, esp, sp_mask, env->regs[R_ESP]);
         }
-        PUSHL(ssp, esp, sp_mask, cpu_compute_eflags(env));
+        PUSHL(ssp, esp, sp_mask, eflags);
         PUSHL(ssp, esp, sp_mask, env->segs[R_CS].selector);
         PUSHL(ssp, esp, sp_mask, old_eip);
         if (has_error_code) {
@@ -776,7 +806,7 @@ static void do_interrupt_protected(CPUX86State *env, int intno, int is_int,
             PUSHW(ssp, esp, sp_mask, env->segs[R_SS].selector);
             PUSHW(ssp, esp, sp_mask, env->regs[R_ESP]);
         }
-        PUSHW(ssp, esp, sp_mask, cpu_compute_eflags(env));
+        PUSHW(ssp, esp, sp_mask, eflags);
         PUSHW(ssp, esp, sp_mask, env->segs[R_CS].selector);
         PUSHW(ssp, esp, sp_mask, old_eip);
         if (has_error_code) {
@@ -868,8 +898,9 @@ static void do_interrupt64(CPUX86State *env, int intno, int is_int,
     target_ulong ptr;
     int type, dpl, selector, cpl, ist;
     int has_error_code, new_stack;
-    uint32_t e1, e2, e3, ss;
+    uint32_t e1, e2, e3, ss, eflags;
     target_ulong old_eip, esp, offset;
+    bool set_rf;
 
     has_error_code = 0;
     if (!is_int && !is_hw) {
@@ -877,8 +908,10 @@ static void do_interrupt64(CPUX86State *env, int intno, int is_int,
     }
     if (is_int) {
         old_eip = next_eip;
+        set_rf = false;
     } else {
         old_eip = env->eip;
+        set_rf = exception_is_fault(intno);
     }
 
     dt = &env->idt;
@@ -950,9 +983,15 @@ static void do_interrupt64(CPUX86State *env, int intno, int is_int,
     }
     esp &= ~0xfLL; /* align stack */
 
+    /* See do_interrupt_protected.  */
+    eflags = cpu_compute_eflags(env);
+    if (set_rf) {
+        eflags |= RF_MASK;
+    }
+
     PUSHQ(esp, env->segs[R_SS].selector);
     PUSHQ(esp, env->regs[R_ESP]);
-    PUSHQ(esp, cpu_compute_eflags(env));
+    PUSHQ(esp, eflags);
     PUSHQ(esp, env->segs[R_CS].selector);
     PUSHQ(esp, old_eip);
     if (has_error_code) {
diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index 0486ab69112..d438f8f76f7 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -4630,6 +4630,14 @@ static void i386_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cpu)
      * If jmp_opt, we want to handle each string instruction individually.
      * For icount also disable repz optimization so that each iteration
      * is accounted separately.
+     *
+     * FIXME: this is messy; it makes REP string instructions a lot less
+     * efficient than they should be and it gets in the way of correct
+     * handling of RF (interrupts or traps arriving after any iteration
+     * of a repeated string instruction but the last should set RF to 1).
+     * Perhaps it would be more efficient if REP string instructions were
+     * always at the beginning of the TB, or even their own TB?  That
+     * would even allow accounting up to 64k iterations at once for icount.
      */
     dc->repz_opt = !dc->jmp_opt && !(cflags & CF_USE_ICOUNT);
 
-- 
2.45.1

[PATCH 02/11] target/i386: fix implementation of ICEBP

[Paolo Bonzini]

ICEBP generates a trap-like exception, while gen_exception() produces
a fault.  Resurrect gen_update_eip_next() to implement the desired
semantics.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/helper.h          |  1 +
 target/i386/tcg/helper-tcg.h  |  3 +++
 target/i386/tcg/bpt_helper.c  |  6 ++++++
 target/i386/tcg/excp_helper.c | 20 ++++++++++++++++++++
 target/i386/tcg/translate.c   | 13 +++++++++++++
 target/i386/tcg/emit.c.inc    |  5 ++++-
 6 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/target/i386/helper.h b/target/i386/helper.h
index a52a1bf0f21..8f291a5f66f 100644
--- a/target/i386/helper.h
+++ b/target/i386/helper.h
@@ -56,6 +56,7 @@ DEF_HELPER_2(sysret, void, env, int)
 DEF_HELPER_FLAGS_2(pause, TCG_CALL_NO_WG, noreturn, env, int)
 DEF_HELPER_FLAGS_3(raise_interrupt, TCG_CALL_NO_WG, noreturn, env, int, int)
 DEF_HELPER_FLAGS_2(raise_exception, TCG_CALL_NO_WG, noreturn, env, int)
+DEF_HELPER_FLAGS_1(icebp, TCG_CALL_NO_WG, noreturn, env)
 DEF_HELPER_3(boundw, void, env, tl, int)
 DEF_HELPER_3(boundl, void, env, tl, int)
 
diff --git a/target/i386/tcg/helper-tcg.h b/target/i386/tcg/helper-tcg.h
index effc2c1c984..6a5505e7b4c 100644
--- a/target/i386/tcg/helper-tcg.h
+++ b/target/i386/tcg/helper-tcg.h
@@ -112,6 +112,9 @@ int exception_has_error_code(int intno);
 void do_smm_enter(X86CPU *cpu);
 
 /* bpt_helper.c */
+void do_end_instruction(CPUX86State *env);
+
+/* sysemu/bpt_helper.c */
 bool check_hw_breakpoints(CPUX86State *env, bool force_dr6_update);
 
 #endif /* I386_HELPER_TCG_H */
diff --git a/target/i386/tcg/bpt_helper.c b/target/i386/tcg/bpt_helper.c
index bc34ac27fea..9695b9dd041 100644
--- a/target/i386/tcg/bpt_helper.c
+++ b/target/i386/tcg/bpt_helper.c
@@ -37,3 +37,9 @@ void helper_rechecking_single_step(CPUX86State *env)
         helper_single_step(env);
     }
 }
+
+void do_end_instruction(CPUX86State *env)
+{
+    env->hflags &= ~HF_INHIBIT_IRQ_MASK; /* needed if sti is just before */
+    env->eflags &= ~HF_RF_MASK;
+}
diff --git a/target/i386/tcg/excp_helper.c b/target/i386/tcg/excp_helper.c
index 65e37ae2a0c..72387aac24f 100644
--- a/target/i386/tcg/excp_helper.c
+++ b/target/i386/tcg/excp_helper.c
@@ -140,6 +140,26 @@ G_NORETURN void raise_exception_ra(CPUX86State *env, int exception_index,
     raise_interrupt2(env, exception_index, 0, 0, 0, retaddr);
 }
 
+G_NORETURN void helper_icebp(CPUX86State *env)
+{
+    CPUState *cs = env_cpu(env);
+
+    do_end_instruction(env);
+
+    /*
+     * INT1 aka ICEBP generates a trap-like #DB, but it is pretty special.
+     *
+     * "Although the ICEBP instruction dispatches through IDT vector 1,
+     * that event is not interceptable by means of the #DB exception
+     * intercept".  Instead there is a separate fault-like ICEBP intercept.
+     */
+    cs->exception_index = EXCP01_DB;
+    env->error_code = 0;
+    env->exception_is_int = 0;
+    env->exception_next_eip = env->eip;
+    cpu_loop_exit(cs);
+}
+
 G_NORETURN void handle_unaligned_access(CPUX86State *env, vaddr vaddr,
                                         MMUAccessType access_type,
                                         uintptr_t retaddr)
diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index d438f8f76f7..77ed9c1db47 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -549,6 +549,19 @@ static inline void gen_op_st_rm_T0_A0(DisasContext *s, int idx, int d)
     }
 }
 
+static void gen_update_eip_next(DisasContext *s)
+{
+    assert(s->pc_save != -1);
+    if (tb_cflags(s->base.tb) & CF_PCREL) {
+        tcg_gen_addi_tl(cpu_eip, cpu_eip, s->pc - s->pc_save);
+    } else if (CODE64(s)) {
+        tcg_gen_movi_tl(cpu_eip, s->pc);
+    } else {
+        tcg_gen_movi_tl(cpu_eip, (uint32_t)(s->pc - s->cs_base));
+    }
+    s->pc_save = s->pc;
+}
+
 static void gen_update_eip_cur(DisasContext *s)
 {
     assert(s->pc_save != -1);
diff --git a/target/i386/tcg/emit.c.inc b/target/i386/tcg/emit.c.inc
index e990141454b..36127d99943 100644
--- a/target/i386/tcg/emit.c.inc
+++ b/target/i386/tcg/emit.c.inc
@@ -1858,7 +1858,10 @@ static void gen_INT(DisasContext *s, CPUX86State *env, X86DecodedInsn *decode)
 
 static void gen_INT1(DisasContext *s, CPUX86State *env, X86DecodedInsn *decode)
 {
-    gen_exception(s, EXCP01_DB);
+    gen_update_cc_op(s);
+    gen_update_eip_next(s);
+    gen_helper_icebp(tcg_env);
+    s->base.is_jmp = DISAS_NORETURN;
 }
 
 static void gen_INT3(DisasContext *s, CPUX86State *env, X86DecodedInsn *decode)
-- 
2.45.1

[PATCH 03/11] target/i386: cleanup HLT helpers

[Paolo Bonzini]

Use decode.c's support for intercepts, doing the check in TCG-generated
code rather than the helper.  This is cleaner because it allows removing
the eip_addend argument to helper_hlt().

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/helper.h                 |  2 +-
 target/i386/tcg/sysemu/misc_helper.c | 13 ++-----------
 target/i386/tcg/decode-new.c.inc     |  4 ++--
 target/i386/tcg/emit.c.inc           |  4 ++--
 4 files changed, 7 insertions(+), 16 deletions(-)

diff --git a/target/i386/helper.h b/target/i386/helper.h
index 8f291a5f66f..c244dbb4812 100644
--- a/target/i386/helper.h
+++ b/target/i386/helper.h
@@ -90,7 +90,7 @@ DEF_HELPER_2(vmsave, void, env, int)
 DEF_HELPER_1(stgi, void, env)
 DEF_HELPER_1(clgi, void, env)
 DEF_HELPER_FLAGS_2(flush_page, TCG_CALL_NO_RWG, void, env, tl)
-DEF_HELPER_FLAGS_2(hlt, TCG_CALL_NO_WG, noreturn, env, int)
+DEF_HELPER_FLAGS_1(hlt, TCG_CALL_NO_WG, noreturn, env)
 DEF_HELPER_FLAGS_2(monitor, TCG_CALL_NO_WG, void, env, tl)
 DEF_HELPER_FLAGS_2(mwait, TCG_CALL_NO_WG, noreturn, env, int)
 DEF_HELPER_1(rdmsr, void, env)
diff --git a/target/i386/tcg/sysemu/misc_helper.c b/target/i386/tcg/sysemu/misc_helper.c
index edb7c3d8940..e41c88346cb 100644
--- a/target/i386/tcg/sysemu/misc_helper.c
+++ b/target/i386/tcg/sysemu/misc_helper.c
@@ -516,8 +516,7 @@ void helper_flush_page(CPUX86State *env, target_ulong addr)
     tlb_flush_page(env_cpu(env), addr);
 }
 
-static G_NORETURN
-void do_hlt(CPUX86State *env)
+G_NORETURN void helper_hlt(CPUX86State *env)
 {
     CPUState *cs = env_cpu(env);
 
@@ -527,14 +526,6 @@ void do_hlt(CPUX86State *env)
     cpu_loop_exit(cs);
 }
 
-G_NORETURN void helper_hlt(CPUX86State *env, int next_eip_addend)
-{
-    cpu_svm_check_intercept_param(env, SVM_EXIT_HLT, 0, GETPC());
-    env->eip += next_eip_addend;
-
-    do_hlt(env);
-}
-
 void helper_monitor(CPUX86State *env, target_ulong ptr)
 {
     if ((uint32_t)env->regs[R_ECX] != 0) {
@@ -558,6 +549,6 @@ G_NORETURN void helper_mwait(CPUX86State *env, int next_eip_addend)
     if (cs->cpu_index != 0 || CPU_NEXT(cs) != NULL) {
         do_pause(env);
     } else {
-        do_hlt(env);
+        helper_hlt(env);
     }
 }
diff --git a/target/i386/tcg/decode-new.c.inc b/target/i386/tcg/decode-new.c.inc
index 0ff0866e8f3..376d2bdabe1 100644
--- a/target/i386/tcg/decode-new.c.inc
+++ b/target/i386/tcg/decode-new.c.inc
@@ -1496,7 +1496,7 @@ static const X86OpEntry opcodes_root[256] = {
     [0xE7] = X86_OP_ENTRYrr(OUT,   0,v, I_unsigned,b), /* AX/EAX */
 
     [0xF1] = X86_OP_ENTRY0(INT1,   svm(ICEBP)),
-    [0xF4] = X86_OP_ENTRY0(HLT,    chk(cpl0)),
+    [0xF4] = X86_OP_ENTRY0(HLT,    chk(cpl0) svm(HLT)),
     [0xF5] = X86_OP_ENTRY0(CMC),
     [0xF6] = X86_OP_GROUP1(group3, E,b),
     [0xF7] = X86_OP_GROUP1(group3, E,v),
@@ -2539,7 +2539,7 @@ static void disas_insn(DisasContext *s, CPUState *cpu)
 
     /*
      * Checks that result in #GP or VMEXIT come second.  Intercepts are
-     * generally checked after non-memory exceptions (i.e. before all
+     * generally checked after non-memory exceptions (i.e. after all
      * exceptions if there is no memory operand).  Exceptions are
      * vm86 checks (INTn, IRET, PUSHF/POPF), RSM and XSETBV (!).
      *
diff --git a/target/i386/tcg/emit.c.inc b/target/i386/tcg/emit.c.inc
index 36127d99943..2e94e8ec56f 100644
--- a/target/i386/tcg/emit.c.inc
+++ b/target/i386/tcg/emit.c.inc
@@ -1638,8 +1638,8 @@ static void gen_HLT(DisasContext *s, CPUX86State *env, X86DecodedInsn *decode)
 {
 #ifdef CONFIG_SYSTEM_ONLY
     gen_update_cc_op(s);
-    gen_update_eip_cur(s);
-    gen_helper_hlt(tcg_env, cur_insn_len_i32(s));
+    gen_update_eip_next(s);
+    gen_helper_hlt(tcg_env);
     s->base.is_jmp = DISAS_NORETURN;
 #endif
 }
-- 
2.45.1

[PATCH 04/11] target/i386: cleanup PAUSE helpers

[Paolo Bonzini]

Use decode.c's support for intercepts, doing the check in TCG-generated
code rather than the helper.  This is cleaner because it allows removing
the eip_addend argument to helper_pause(), even though it adds a bit of
bloat for opcode 0x90's new decoding function.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/helper.h                 |  2 +-
 target/i386/tcg/helper-tcg.h         |  1 -
 target/i386/tcg/misc_helper.c        | 10 +---------
 target/i386/tcg/sysemu/misc_helper.c |  2 +-
 target/i386/tcg/decode-new.c.inc     | 15 ++++++++++++++-
 target/i386/tcg/emit.c.inc           | 20 ++++++++------------
 6 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/target/i386/helper.h b/target/i386/helper.h
index c244dbb4812..2f46cffabd8 100644
--- a/target/i386/helper.h
+++ b/target/i386/helper.h
@@ -53,7 +53,7 @@ DEF_HELPER_1(sysenter, void, env)
 DEF_HELPER_2(sysexit, void, env, int)
 DEF_HELPER_2(syscall, void, env, int)
 DEF_HELPER_2(sysret, void, env, int)
-DEF_HELPER_FLAGS_2(pause, TCG_CALL_NO_WG, noreturn, env, int)
+DEF_HELPER_FLAGS_1(pause, TCG_CALL_NO_WG, noreturn, env)
 DEF_HELPER_FLAGS_3(raise_interrupt, TCG_CALL_NO_WG, noreturn, env, int, int)
 DEF_HELPER_FLAGS_2(raise_exception, TCG_CALL_NO_WG, noreturn, env, int)
 DEF_HELPER_FLAGS_1(icebp, TCG_CALL_NO_WG, noreturn, env)
diff --git a/target/i386/tcg/helper-tcg.h b/target/i386/tcg/helper-tcg.h
index 6a5505e7b4c..43180b58314 100644
--- a/target/i386/tcg/helper-tcg.h
+++ b/target/i386/tcg/helper-tcg.h
@@ -91,7 +91,6 @@ extern const uint8_t parity_table[256];
 
 /* misc_helper.c */
 void cpu_load_eflags(CPUX86State *env, int eflags, int update_mask);
-G_NORETURN void do_pause(CPUX86State *env);
 
 /* sysemu/svm_helper.c */
 #ifndef CONFIG_USER_ONLY
diff --git a/target/i386/tcg/misc_helper.c b/target/i386/tcg/misc_helper.c
index b0f0f7b893b..8316d42ffcd 100644
--- a/target/i386/tcg/misc_helper.c
+++ b/target/i386/tcg/misc_helper.c
@@ -88,7 +88,7 @@ G_NORETURN void helper_rdpmc(CPUX86State *env)
     raise_exception_err(env, EXCP06_ILLOP, 0);
 }
 
-G_NORETURN void do_pause(CPUX86State *env)
+G_NORETURN void helper_pause(CPUX86State *env)
 {
     CPUState *cs = env_cpu(env);
 
@@ -97,14 +97,6 @@ G_NORETURN void do_pause(CPUX86State *env)
     cpu_loop_exit(cs);
 }
 
-G_NORETURN void helper_pause(CPUX86State *env, int next_eip_addend)
-{
-    cpu_svm_check_intercept_param(env, SVM_EXIT_PAUSE, 0, GETPC());
-    env->eip += next_eip_addend;
-
-    do_pause(env);
-}
-
 uint64_t helper_rdpkru(CPUX86State *env, uint32_t ecx)
 {
     if ((env->cr[4] & CR4_PKE_MASK) == 0) {
diff --git a/target/i386/tcg/sysemu/misc_helper.c b/target/i386/tcg/sysemu/misc_helper.c
index e41c88346cb..093cc2d0f90 100644
--- a/target/i386/tcg/sysemu/misc_helper.c
+++ b/target/i386/tcg/sysemu/misc_helper.c
@@ -547,7 +547,7 @@ G_NORETURN void helper_mwait(CPUX86State *env, int next_eip_addend)
 
     /* XXX: not complete but not completely erroneous */
     if (cs->cpu_index != 0 || CPU_NEXT(cs) != NULL) {
-        do_pause(env);
+        helper_pause(env);
     } else {
         helper_hlt(env);
     }
diff --git a/target/i386/tcg/decode-new.c.inc b/target/i386/tcg/decode-new.c.inc
index 376d2bdabe1..c2d8da8d14e 100644
--- a/target/i386/tcg/decode-new.c.inc
+++ b/target/i386/tcg/decode-new.c.inc
@@ -1359,6 +1359,19 @@ static void decode_group11(DisasContext *s, CPUX86State *env, X86OpEntry *entry,
     }
 }
 
+static void decode_90(DisasContext *s, CPUX86State *env, X86OpEntry *entry, uint8_t *b)
+{
+    static X86OpEntry pause = X86_OP_ENTRY0(PAUSE, svm(PAUSE));
+    static X86OpEntry nop = X86_OP_ENTRY0(NOP);
+    static X86OpEntry xchg_ax = X86_OP_ENTRY2(XCHG, 0,v, LoBits,v);
+
+    if (REX_B(s)) {
+        *entry = xchg_ax;
+    } else {
+        *entry = (s->prefix & PREFIX_REPZ) ? pause : nop;
+    }
+}
+
 static const X86OpEntry opcodes_root[256] = {
     [0x00] = X86_OP_ENTRY2(ADD, E,b, G,b, lock),
     [0x01] = X86_OP_ENTRY2(ADD, E,v, G,v, lock),
@@ -1441,7 +1454,7 @@ static const X86OpEntry opcodes_root[256] = {
     [0x86] = X86_OP_ENTRY2(XCHG, E,b, G,b, xchg),
     [0x87] = X86_OP_ENTRY2(XCHG, E,v, G,v, xchg),
 
-    [0x90] = X86_OP_ENTRY2(XCHG, 0,v, LoBits,v),
+    [0x90] = X86_OP_GROUP0(90),
     [0x91] = X86_OP_ENTRY2(XCHG, 0,v, LoBits,v),
     [0x92] = X86_OP_ENTRY2(XCHG, 0,v, LoBits,v),
     [0x93] = X86_OP_ENTRY2(XCHG, 0,v, LoBits,v),
diff --git a/target/i386/tcg/emit.c.inc b/target/i386/tcg/emit.c.inc
index 2e94e8ec56f..f90f3d3c589 100644
--- a/target/i386/tcg/emit.c.inc
+++ b/target/i386/tcg/emit.c.inc
@@ -2350,6 +2350,14 @@ static void gen_PANDN(DisasContext *s, CPUX86State *env, X86DecodedInsn *decode)
                       decode->op[1].offset, vec_len, vec_len);
 }
 
+static void gen_PAUSE(DisasContext *s, CPUX86State *env, X86DecodedInsn *decode)
+{
+    gen_update_cc_op(s);
+    gen_update_eip_next(s);
+    gen_helper_pause(tcg_env);
+    s->base.is_jmp = DISAS_NORETURN;
+}
+
 static void gen_PCMPESTRI(DisasContext *s, CPUX86State *env, X86DecodedInsn *decode)
 {
     TCGv_i32 imm = tcg_constant8u_i32(decode->immediate);
@@ -4014,18 +4022,6 @@ static void gen_WAIT(DisasContext *s, CPUX86State *env, X86DecodedInsn *decode)
 
 static void gen_XCHG(DisasContext *s, CPUX86State *env, X86DecodedInsn *decode)
 {
-    if (decode->b == 0x90 && !REX_B(s)) {
-        if (s->prefix & PREFIX_REPZ) {
-            gen_update_cc_op(s);
-            gen_update_eip_cur(s);
-            gen_helper_pause(tcg_env, cur_insn_len_i32(s));
-            s->base.is_jmp = DISAS_NORETURN;
-        }
-        /* No writeback.  */
-        decode->op[0].unit = X86_OP_SKIP;
-        return;
-    }
-
     if (s->prefix & PREFIX_LOCK) {
         tcg_gen_atomic_xchg_tl(s->T0, s->A0, s->T1,
                                s->mem_index, decode->op[0].ot | MO_LE);
-- 
2.45.1

[PATCH 05/11] target/i386: implement DR7.GD

[Paolo Bonzini]

DR7.GD triggers a #DB exception on any access to debug registers.
The GD bit is cleared so that the #DB handler itself can access
the debug registers.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/tcg/sysemu/bpt_helper.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/target/i386/tcg/sysemu/bpt_helper.c b/target/i386/tcg/sysemu/bpt_helper.c
index 4d96a48a3ca..c1d5fce250c 100644
--- a/target/i386/tcg/sysemu/bpt_helper.c
+++ b/target/i386/tcg/sysemu/bpt_helper.c
@@ -238,6 +238,12 @@ target_ulong helper_get_dr(CPUX86State *env, int reg)
         }
     }
 
+    if (env->dr[7] & DR7_GD) {
+        env->dr[7] &= ~DR7_GD;
+        env->dr[6] |= DR6_BD;
+        raise_exception_ra(env, EXCP01_DB, GETPC());
+    }
+
     return env->dr[reg];
 }
 
@@ -251,6 +257,12 @@ void helper_set_dr(CPUX86State *env, int reg, target_ulong t0)
         }
     }
 
+    if (env->dr[7] & DR7_GD) {
+        env->dr[7] &= ~DR7_GD;
+        env->dr[6] |= DR6_BD;
+        raise_exception_ra(env, EXCP01_DB, GETPC());
+    }
+
     if (reg < 4) {
         if (hw_breakpoint_enabled(env->dr[7], reg)
             && hw_breakpoint_type(env->dr[7], reg) != DR7_TYPE_IO_RW) {
-- 
2.45.1

[PATCH 06/11] target/i386: disable/enable breakpoints on vmentry/vmexit

[Paolo Bonzini]

If the required DR7 (either from the VMCB or from the host save
area) disables a breakpoint that was enabled prior to vmentry
or vmexit, it is left enabled and will trigger EXCP_DEBUG.
This causes a spurious #DB on the next crossing of the breakpoint.

To disable it, vmentry/vmexit must use cpu_x86_update_dr7
to load DR7.

Because cpu_x86_update_dr7 takes a 32-bit argument, check
reserved bits prior to calling cpu_x86_update_dr7, and do the
same for DR6 as well for consistency.

This scenario is tested by the "host_rflags" test in kvm-unit-tests.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/tcg/sysemu/svm_helper.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/target/i386/tcg/sysemu/svm_helper.c b/target/i386/tcg/sysemu/svm_helper.c
index 5d6de2294fa..922d8964f8e 100644
--- a/target/i386/tcg/sysemu/svm_helper.c
+++ b/target/i386/tcg/sysemu/svm_helper.c
@@ -163,6 +163,8 @@ void helper_vmrun(CPUX86State *env, int aflag, int next_eip_addend)
     uint64_t new_cr0;
     uint64_t new_cr3;
     uint64_t new_cr4;
+    uint64_t new_dr6;
+    uint64_t new_dr7;
 
     if (aflag == 2) {
         addr = env->regs[R_EAX];
@@ -361,20 +363,22 @@ void helper_vmrun(CPUX86State *env, int aflag, int next_eip_addend)
                                 env->vm_vmcb + offsetof(struct vmcb, save.rsp));
     env->regs[R_EAX] = x86_ldq_phys(cs,
                                 env->vm_vmcb + offsetof(struct vmcb, save.rax));
-    env->dr[7] = x86_ldq_phys(cs,
-                          env->vm_vmcb + offsetof(struct vmcb, save.dr7));
-    env->dr[6] = x86_ldq_phys(cs,
-                          env->vm_vmcb + offsetof(struct vmcb, save.dr6));
+
+    new_dr7 = x86_ldq_phys(cs, env->vm_vmcb + offsetof(struct vmcb, save.dr7));
+    new_dr6 = x86_ldq_phys(cs, env->vm_vmcb + offsetof(struct vmcb, save.dr6));
 
 #ifdef TARGET_X86_64
-    if (env->dr[6] & DR_RESERVED_MASK) {
+    if (new_dr7 & DR_RESERVED_MASK) {
         cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC());
     }
-    if (env->dr[7] & DR_RESERVED_MASK) {
+    if (new_dr6 & DR_RESERVED_MASK) {
         cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC());
     }
 #endif
 
+    cpu_x86_update_dr7(env, new_dr7);
+    env->dr[6] = new_dr6;
+
     if (is_efer_invalid_state(env)) {
         cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC());
     }
@@ -864,8 +868,11 @@ void do_vmexit(CPUX86State *env)
 
     env->dr[6] = x86_ldq_phys(cs,
                           env->vm_hsave + offsetof(struct vmcb, save.dr6));
-    env->dr[7] = x86_ldq_phys(cs,
-                          env->vm_hsave + offsetof(struct vmcb, save.dr7));
+
+    /* Disables all breakpoints in the host DR7 register. */
+    cpu_x86_update_dr7(env,
+             x86_ldq_phys(cs,
+                          env->vm_hsave + offsetof(struct vmcb, save.dr7)) & ~0xff);
 
     /* other setups */
     x86_stl_phys(cs,
@@ -891,8 +898,6 @@ void do_vmexit(CPUX86State *env)
        from the page table indicated the host's CR3. If the PDPEs contain
        illegal state, the processor causes a shutdown. */
 
-    /* Disables all breakpoints in the host DR7 register. */
-
     /* Checks the reloaded host state for consistency. */
 
     /* If the host's rIP reloaded by #VMEXIT is outside the limit of the
-- 
2.45.1

[PATCH 07/11] target/i386: fix INHIBIT_IRQ/TF/RF handling for VMRUN

[Paolo Bonzini]

From vm entry to exit, VMRUN is handled as a single instruction.  It
uses DISAS_NORETURN in order to avoid processing TF or RF before
the first instruction executes in the guest.  However, the corresponding
handling is missing in vmexit.  Add it, and at the same time reorganize
the comments with quotes from the manual about the tasks performed
by a #VMEXIT.

Another gen_eob() task that is missing in VMRUN is preparing the
HF_INHIBIT_IRQ flag for the next instruction, in this case by loading
it from the VMCB control state.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/tcg/sysemu/svm_helper.c | 46 +++++++++++++++++++++--------
 target/i386/tcg/translate.c         |  5 ++++
 2 files changed, 38 insertions(+), 13 deletions(-)

diff --git a/target/i386/tcg/sysemu/svm_helper.c b/target/i386/tcg/sysemu/svm_helper.c
index 922d8964f8e..9db8ad62a01 100644
--- a/target/i386/tcg/sysemu/svm_helper.c
+++ b/target/i386/tcg/sysemu/svm_helper.c
@@ -254,6 +254,13 @@ void helper_vmrun(CPUX86State *env, int aflag, int next_eip_addend)
                                                   control.intercept_exceptions
                                                   ));
 
+    env->hflags &= ~HF_INHIBIT_IRQ_MASK;
+    if (x86_ldl_phys(cs, env->vm_vmcb +
+                offsetof(struct vmcb, control.int_state)) &
+                 SVM_INTERRUPT_SHADOW_MASK) {
+        env->hflags |= HF_INHIBIT_IRQ_MASK;
+    }
+
     nested_ctl = x86_ldq_phys(cs, env->vm_vmcb + offsetof(struct vmcb,
                                                           control.nested_ctl));
     asid = x86_ldq_phys(cs, env->vm_vmcb + offsetof(struct vmcb,
@@ -815,8 +822,12 @@ void do_vmexit(CPUX86State *env)
     env->hflags &= ~HF_GUEST_MASK;
     env->intercept = 0;
     env->intercept_exceptions = 0;
+
+    /* Clears the V_IRQ and V_INTR_MASKING bits inside the processor. */
     cs->interrupt_request &= ~CPU_INTERRUPT_VIRQ;
     env->int_ctl = 0;
+
+    /* Clears the TSC_OFFSET inside the processor. */
     env->tsc_offset = 0;
 
     env->gdt.base  = x86_ldq_phys(cs, env->vm_hsave + offsetof(struct vmcb,
@@ -836,6 +847,15 @@ void do_vmexit(CPUX86State *env)
     cpu_x86_update_cr4(env, x86_ldq_phys(cs,
                                      env->vm_hsave + offsetof(struct vmcb,
                                                               save.cr4)));
+
+    /*
+     * Resets the current ASID register to zero (host ASID; TLB flush).
+     *
+     * If the host is in PAE mode, the processor reloads the host's PDPEs
+     * from the page table indicated the host's CR3. FIXME: If the PDPEs
+     * contain illegal state, the processor causes a shutdown (QEMU does
+     * not implement PDPTRs).
+     */
     cpu_x86_update_cr3(env, x86_ldq_phys(cs,
                                      env->vm_hsave + offsetof(struct vmcb,
                                                               save.cr3)));
@@ -843,12 +863,14 @@ void do_vmexit(CPUX86State *env)
        set properly */
     cpu_load_efer(env, x86_ldq_phys(cs, env->vm_hsave + offsetof(struct vmcb,
                                                          save.efer)));
+
+    /* Completion of the VMRUN instruction clears the host EFLAGS.RF bit.  */
     env->eflags = 0;
     cpu_load_eflags(env, x86_ldq_phys(cs,
                                   env->vm_hsave + offsetof(struct vmcb,
                                                            save.rflags)),
                     ~(CC_O | CC_S | CC_Z | CC_A | CC_P | CC_C | DF_MASK |
-                      VM_MASK));
+                      RF_MASK | VM_MASK));
 
     svm_load_seg_cache(env, MMU_PHYS_IDX,
                        env->vm_hsave + offsetof(struct vmcb, save.es), R_ES);
@@ -888,19 +910,17 @@ void do_vmexit(CPUX86State *env)
 
     env->hflags2 &= ~HF2_GIF_MASK;
     env->hflags2 &= ~HF2_VGIF_MASK;
-    /* FIXME: Resets the current ASID register to zero (host ASID). */
 
-    /* Clears the V_IRQ and V_INTR_MASKING bits inside the processor. */
 
-    /* Clears the TSC_OFFSET inside the processor. */
+    /* FIXME: Checks the reloaded host state for consistency. */
 
-    /* If the host is in PAE mode, the processor reloads the host's PDPEs
-       from the page table indicated the host's CR3. If the PDPEs contain
-       illegal state, the processor causes a shutdown. */
-
-    /* Checks the reloaded host state for consistency. */
-
-    /* If the host's rIP reloaded by #VMEXIT is outside the limit of the
-       host's code segment or non-canonical (in the case of long mode), a
-       #GP fault is delivered inside the host. */
+    /*
+     * EFLAGS.TF causes a #DB trap after the VMRUN completes on the host
+     * side (i.e., after the #VMEXIT from the guest). Since we're running
+     * in the main loop, call do_interrupt_all directly.
+     */
+    if ((env->eflags & TF_MASK) != 0) {
+        env->dr[6] |= DR6_BS;
+        do_interrupt_all(X86_CPU(cs), EXCP01_DB, 0, 0, env->eip, 0);
+    }
 }
diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index 77ed9c1db47..a9c6424c7df 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -3745,6 +3745,11 @@ static void disas_insn_old(DisasContext *s, CPUState *cpu, int b)
             }
             gen_update_cc_op(s);
             gen_update_eip_cur(s);
+            /*
+             * Reloads INHIBIT_IRQ mask as well as TF and RF with guest state.
+             * The usual gen_eob() handling is performed on vmexit after
+             * host state is reloaded.
+             */
             gen_helper_vmrun(tcg_env, tcg_constant_i32(s->aflag - 1),
                              cur_insn_len_i32(s));
             tcg_gen_exit_tb(NULL, 0);
-- 
2.45.1

[PATCH 08/11] target/i386: fix INHIBIT_IRQ/TF/RF handling for PAUSE

[Paolo Bonzini]

PAUSE uses DISAS_NORETURN because the corresponding helper
calls cpu_loop_exit().  However, while HLT clear HF_INHIBIT_IRQ_MASK
to correctly handle "STI; HLT", the same is missing from PAUSE.
And also gen_eob() clears HF_RF_MASK and synthesizes a #DB exception
if single-step is active; none of this is done by HLT and PAUSE.
Start fixing PAUSE, HLT will follow.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/tcg/misc_helper.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target/i386/tcg/misc_helper.c b/target/i386/tcg/misc_helper.c
index 8316d42ffcd..ed4cda8001e 100644
--- a/target/i386/tcg/misc_helper.c
+++ b/target/i386/tcg/misc_helper.c
@@ -92,6 +92,10 @@ G_NORETURN void helper_pause(CPUX86State *env)
 {
     CPUState *cs = env_cpu(env);
 
+    /* Do gen_eob() tasks before going back to the main loop.  */
+    do_end_instruction(env);
+    helper_rechecking_single_step(env);
+
     /* Just let another CPU run.  */
     cs->exception_index = EXCP_INTERRUPT;
     cpu_loop_exit(cs);
-- 
2.45.1

[PATCH 09/11] target/i386: fix TF/RF handling for HLT

[Paolo Bonzini]

HLT uses DISAS_NORETURN because the corresponding helper calls
cpu_loop_exit().  However, while gen_eob() clears HF_RF_MASK and
synthesizes a #DB exception if single-step is active, none of this is
done by HLT.  Note that the single-step trap is generated after the halt
is finished.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/tcg/sysemu/misc_helper.c |  2 +-
 target/i386/tcg/sysemu/seg_helper.c  | 17 ++++++++++++++---
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/target/i386/tcg/sysemu/misc_helper.c b/target/i386/tcg/sysemu/misc_helper.c
index 093cc2d0f90..7fa0c5a06de 100644
--- a/target/i386/tcg/sysemu/misc_helper.c
+++ b/target/i386/tcg/sysemu/misc_helper.c
@@ -520,7 +520,7 @@ G_NORETURN void helper_hlt(CPUX86State *env)
 {
     CPUState *cs = env_cpu(env);
 
-    env->hflags &= ~HF_INHIBIT_IRQ_MASK; /* needed if sti is just before */
+    do_end_instruction(env);
     cs->halted = 1;
     cs->exception_index = EXCP_HLT;
     cpu_loop_exit(cs);
diff --git a/target/i386/tcg/sysemu/seg_helper.c b/target/i386/tcg/sysemu/seg_helper.c
index 9ba94deb3aa..05174a79e73 100644
--- a/target/i386/tcg/sysemu/seg_helper.c
+++ b/target/i386/tcg/sysemu/seg_helper.c
@@ -130,15 +130,26 @@ void x86_cpu_do_interrupt(CPUState *cs)
 
 bool x86_cpu_exec_halt(CPUState *cpu)
 {
-    if (cpu->interrupt_request & CPU_INTERRUPT_POLL) {
-        X86CPU *x86_cpu = X86_CPU(cpu);
+    X86CPU *x86_cpu = X86_CPU(cpu);
+    CPUX86State *env = &x86_cpu->env;
 
+    if (cpu->interrupt_request & CPU_INTERRUPT_POLL) {
         bql_lock();
         apic_poll_irq(x86_cpu->apic_state);
         cpu_reset_interrupt(cpu, CPU_INTERRUPT_POLL);
         bql_unlock();
     }
-    return cpu_has_work(cpu);
+
+    if (!cpu_has_work(cpu)) {
+        return false;
+    }
+
+    /* Complete HLT instruction.  */
+    if (env->eflags & TF_MASK) {
+        env->dr[6] |= DR6_BS;
+        do_interrupt_all(x86_cpu, EXCP01_DB, 0, 0, env->eip, 0);
+    }
+    return true;
 }
 
 bool x86_need_replay_interrupt(int interrupt_request)
-- 
2.45.1

[PATCH 10/11] target/i386: document incorrect semantics of watchpoint following MOV/POP SS

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/tcg/sysemu/bpt_helper.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/target/i386/tcg/sysemu/bpt_helper.c b/target/i386/tcg/sysemu/bpt_helper.c
index c1d5fce250c..b29acf41c38 100644
--- a/target/i386/tcg/sysemu/bpt_helper.c
+++ b/target/i386/tcg/sysemu/bpt_helper.c
@@ -215,6 +215,12 @@ void breakpoint_handler(CPUState *cs)
         if (cs->watchpoint_hit->flags & BP_CPU) {
             cs->watchpoint_hit = NULL;
             if (check_hw_breakpoints(env, false)) {
+                /*
+                 * FIXME: #DB should be delayed by one instruction if
+                 * INHIBIT_IRQ is set (STI cannot trigger a watchpoint).
+                 * The delayed #DB should also fuse with one generated
+                 * by ICEBP (aka INT1).
+                 */
                 raise_exception(env, EXCP01_DB);
             } else {
                 cpu_loop_exit_noexc(cs);
-- 
2.45.1

[PATCH 11/11] target/i386: document use of DISAS_NORETURN

[Paolo Bonzini]

DISAS_NORETURN suppresses the work normally done by gen_eob(), and therefore
must be used in special cases only.  Document them.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/tcg/translate.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index a9c6424c7df..2b6f67be40b 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -4761,6 +4761,17 @@ static void i386_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
 
     switch (dc->base.is_jmp) {
     case DISAS_NORETURN:
+        /*
+         * Most instructions should not use DISAS_NORETURN, as that suppresses
+         * the handling of hflags normally done by gen_eob().  We can
+         * get here:
+         * - for exception and interrupts
+         * - for jump optimization (which is disabled by INHIBIT_IRQ/RF/TF)
+         * - for VMRUN because RF/TF handling for the host is done after vmexit,
+         *   and INHIBIT_IRQ is loaded from the VMCB
+         * - for HLT/PAUSE/MWAIT to exit the main loop with specific EXCP_* values;
+         *   the helpers handle themselves the tasks normally done by gen_eob().
+         */
         break;
     case DISAS_TOO_MANY:
         gen_update_cc_op(dc);
-- 
2.45.1

Re: [PATCH 01/11] target/i386: fix pushed value of EFLAGS.RF

[Richard Henderson]

On 6/4/24 02:18, Paolo Bonzini wrote:
> When preparing an exception stack frame for a fault exception, the value
> pushed for RF is 1.  Take that into account.  The same should be true
> of interrupts for repeated string instructions, but the situation there
> is complicated.
> 
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   target/i386/tcg/seg_helper.c | 49 ++++++++++++++++++++++++++++++++----
>   target/i386/tcg/translate.c  |  8 ++++++
>   2 files changed, 52 insertions(+), 5 deletions(-)

Acked-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 03/11] target/i386: cleanup HLT helpers

[Richard Henderson]

On 6/4/24 02:18, Paolo Bonzini wrote:
> Use decode.c's support for intercepts, doing the check in TCG-generated
> code rather than the helper.  This is cleaner because it allows removing
> the eip_addend argument to helper_hlt().
> 
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   target/i386/helper.h                 |  2 +-
>   target/i386/tcg/sysemu/misc_helper.c | 13 ++-----------
>   target/i386/tcg/decode-new.c.inc     |  4 ++--
>   target/i386/tcg/emit.c.inc           |  4 ++--
>   4 files changed, 7 insertions(+), 16 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 04/11] target/i386: cleanup PAUSE helpers

[Richard Henderson]

On 6/4/24 02:18, Paolo Bonzini wrote:
> Use decode.c's support for intercepts, doing the check in TCG-generated
> code rather than the helper.  This is cleaner because it allows removing
> the eip_addend argument to helper_pause(), even though it adds a bit of
> bloat for opcode 0x90's new decoding function.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   target/i386/helper.h                 |  2 +-
>   target/i386/tcg/helper-tcg.h         |  1 -
>   target/i386/tcg/misc_helper.c        | 10 +---------
>   target/i386/tcg/sysemu/misc_helper.c |  2 +-
>   target/i386/tcg/decode-new.c.inc     | 15 ++++++++++++++-
>   target/i386/tcg/emit.c.inc           | 20 ++++++++------------
>   6 files changed, 25 insertions(+), 25 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

> +static void decode_90(DisasContext *s, CPUX86State *env, X86OpEntry *entry, uint8_t *b)
> +{
> +    static X86OpEntry pause = X86_OP_ENTRY0(PAUSE, svm(PAUSE));
> +    static X86OpEntry nop = X86_OP_ENTRY0(NOP);
> +    static X86OpEntry xchg_ax = X86_OP_ENTRY2(XCHG, 0,v, LoBits,v);
> +
> +    if (REX_B(s)) {
> +        *entry = xchg_ax;
> +    } else {
> +        *entry = (s->prefix & PREFIX_REPZ) ? pause : nop;
> +    }
> +}

Thanks.  I had wished for this instead of

>   static void gen_XCHG(DisasContext *s, CPUX86State *env, X86DecodedInsn *decode)
>   {
> -    if (decode->b == 0x90 && !REX_B(s)) {
> -        if (s->prefix & PREFIX_REPZ) {
> -            gen_update_cc_op(s);
> -            gen_update_eip_cur(s);
> -            gen_helper_pause(tcg_env, cur_insn_len_i32(s));
> -            s->base.is_jmp = DISAS_NORETURN;
> -        }
> -        /* No writeback.  */
> -        decode->op[0].unit = X86_OP_SKIP;
> -        return;
> -    }

this from the beginning, but since it wasn't wrong, I didn't mention it.


r~

Re: [PATCH 05/11] target/i386: implement DR7.GD

[Richard Henderson]

On 6/4/24 02:18, Paolo Bonzini wrote:
> DR7.GD triggers a #DB exception on any access to debug registers.
> The GD bit is cleared so that the #DB handler itself can access
> the debug registers.
> 
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   target/i386/tcg/sysemu/bpt_helper.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 06/11] target/i386: disable/enable breakpoints on vmentry/vmexit

[Richard Henderson]

On 6/4/24 02:18, Paolo Bonzini wrote:
> If the required DR7 (either from the VMCB or from the host save
> area) disables a breakpoint that was enabled prior to vmentry
> or vmexit, it is left enabled and will trigger EXCP_DEBUG.
> This causes a spurious #DB on the next crossing of the breakpoint.
> 
> To disable it, vmentry/vmexit must use cpu_x86_update_dr7
> to load DR7.
> 
> Because cpu_x86_update_dr7 takes a 32-bit argument, check
> reserved bits prior to calling cpu_x86_update_dr7, and do the
> same for DR6 as well for consistency.
> 
> This scenario is tested by the "host_rflags" test in kvm-unit-tests.
> 
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   target/i386/tcg/sysemu/svm_helper.c | 25 +++++++++++++++----------
>   1 file changed, 15 insertions(+), 10 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 07/11] target/i386: fix INHIBIT_IRQ/TF/RF handling for VMRUN

[Richard Henderson]

On 6/4/24 02:18, Paolo Bonzini wrote:
>  From vm entry to exit, VMRUN is handled as a single instruction.  It
> uses DISAS_NORETURN in order to avoid processing TF or RF before
> the first instruction executes in the guest.  However, the corresponding
> handling is missing in vmexit.  Add it, and at the same time reorganize
> the comments with quotes from the manual about the tasks performed
> by a #VMEXIT.
> 
> Another gen_eob() task that is missing in VMRUN is preparing the
> HF_INHIBIT_IRQ flag for the next instruction, in this case by loading
> it from the VMCB control state.
> 
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   target/i386/tcg/sysemu/svm_helper.c | 46 +++++++++++++++++++++--------
>   target/i386/tcg/translate.c         |  5 ++++
>   2 files changed, 38 insertions(+), 13 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 08/11] target/i386: fix INHIBIT_IRQ/TF/RF handling for PAUSE

[Richard Henderson]

On 6/4/24 02:18, Paolo Bonzini wrote:
> PAUSE uses DISAS_NORETURN because the corresponding helper
> calls cpu_loop_exit().  However, while HLT clear HF_INHIBIT_IRQ_MASK
> to correctly handle "STI; HLT", the same is missing from PAUSE.
> And also gen_eob() clears HF_RF_MASK and synthesizes a #DB exception
> if single-step is active; none of this is done by HLT and PAUSE.
> Start fixing PAUSE, HLT will follow.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   target/i386/tcg/misc_helper.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/target/i386/tcg/misc_helper.c b/target/i386/tcg/misc_helper.c
> index 8316d42ffcd..ed4cda8001e 100644
> --- a/target/i386/tcg/misc_helper.c
> +++ b/target/i386/tcg/misc_helper.c
> @@ -92,6 +92,10 @@ G_NORETURN void helper_pause(CPUX86State *env)
>   {
>       CPUState *cs = env_cpu(env);
>   
> +    /* Do gen_eob() tasks before going back to the main loop.  */
> +    do_end_instruction(env);
> +    helper_rechecking_single_step(env);
> +
>       /* Just let another CPU run.  */
>       cs->exception_index = EXCP_INTERRUPT;
>       cpu_loop_exit(cs);

Perhaps it would be better to do

void helper_cpu_exit(CPUX86State *env)
{
     cpu_exit(env_cpu(env));
}

static void gen_PAUSE(...)
{
     helper_cpu_exit(tcg_env);
     s->base.is_jmp = DISAS_EOB_NEXT;
}

to keep from replicating gen_eob?

Anyway, this is correct, so,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 09/11] target/i386: fix TF/RF handling for HLT

[Richard Henderson]

On 6/4/24 02:18, Paolo Bonzini wrote:
> HLT uses DISAS_NORETURN because the corresponding helper calls
> cpu_loop_exit().  However, while gen_eob() clears HF_RF_MASK and
> synthesizes a #DB exception if single-step is active, none of this is
> done by HLT.  Note that the single-step trap is generated after the halt
> is finished.
> 
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   target/i386/tcg/sysemu/misc_helper.c |  2 +-
>   target/i386/tcg/sysemu/seg_helper.c  | 17 ++++++++++++++---
>   2 files changed, 15 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 08/11] target/i386: fix INHIBIT_IRQ/TF/RF handling for PAUSE

[Richard Henderson]

On 6/4/24 08:44, Richard Henderson wrote:
> On 6/4/24 02:18, Paolo Bonzini wrote:
>> PAUSE uses DISAS_NORETURN because the corresponding helper
>> calls cpu_loop_exit().  However, while HLT clear HF_INHIBIT_IRQ_MASK
>> to correctly handle "STI; HLT", the same is missing from PAUSE.
>> And also gen_eob() clears HF_RF_MASK and synthesizes a #DB exception
>> if single-step is active; none of this is done by HLT and PAUSE.
>> Start fixing PAUSE, HLT will follow.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   target/i386/tcg/misc_helper.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/target/i386/tcg/misc_helper.c b/target/i386/tcg/misc_helper.c
>> index 8316d42ffcd..ed4cda8001e 100644
>> --- a/target/i386/tcg/misc_helper.c
>> +++ b/target/i386/tcg/misc_helper.c
>> @@ -92,6 +92,10 @@ G_NORETURN void helper_pause(CPUX86State *env)
>>   {
>>       CPUState *cs = env_cpu(env);
>> +    /* Do gen_eob() tasks before going back to the main loop.  */
>> +    do_end_instruction(env);
>> +    helper_rechecking_single_step(env);
>> +
>>       /* Just let another CPU run.  */
>>       cs->exception_index = EXCP_INTERRUPT;
>>       cpu_loop_exit(cs);
> 
> Perhaps it would be better to do
> 
> void helper_cpu_exit(CPUX86State *env)
> {
>      cpu_exit(env_cpu(env));
> }
> 
> static void gen_PAUSE(...)
> {
>      helper_cpu_exit(tcg_env);
>      s->base.is_jmp = DISAS_EOB_NEXT;
> }
> 
> to keep from replicating gen_eob?
> 
> Anyway, this is correct, so,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Oh, based on the next patch, it would appear that PAUSE does not single-step properly 
because it sets EXCP_INTERRUPT, and end-of-insn single-step depends on exception_index == 
-1.  I'm thinking of the bottom of cpu_tb_exec().


r~

Re: [PATCH 02/11] target/i386: fix implementation of ICEBP

[Richard Henderson]

On 6/4/24 02:18, Paolo Bonzini wrote:
> ICEBP generates a trap-like exception, while gen_exception() produces
> a fault.  Resurrect gen_update_eip_next() to implement the desired
> semantics.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   target/i386/helper.h          |  1 +
>   target/i386/tcg/helper-tcg.h  |  3 +++
>   target/i386/tcg/bpt_helper.c  |  6 ++++++
>   target/i386/tcg/excp_helper.c | 20 ++++++++++++++++++++
>   target/i386/tcg/translate.c   | 13 +++++++++++++
>   target/i386/tcg/emit.c.inc    |  5 ++++-
>   6 files changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/target/i386/helper.h b/target/i386/helper.h
> index a52a1bf0f21..8f291a5f66f 100644
> --- a/target/i386/helper.h
> +++ b/target/i386/helper.h
> @@ -56,6 +56,7 @@ DEF_HELPER_2(sysret, void, env, int)
>   DEF_HELPER_FLAGS_2(pause, TCG_CALL_NO_WG, noreturn, env, int)
>   DEF_HELPER_FLAGS_3(raise_interrupt, TCG_CALL_NO_WG, noreturn, env, int, int)
>   DEF_HELPER_FLAGS_2(raise_exception, TCG_CALL_NO_WG, noreturn, env, int)
> +DEF_HELPER_FLAGS_1(icebp, TCG_CALL_NO_WG, noreturn, env)
>   DEF_HELPER_3(boundw, void, env, tl, int)
>   DEF_HELPER_3(boundl, void, env, tl, int)
>   
> diff --git a/target/i386/tcg/helper-tcg.h b/target/i386/tcg/helper-tcg.h
> index effc2c1c984..6a5505e7b4c 100644
> --- a/target/i386/tcg/helper-tcg.h
> +++ b/target/i386/tcg/helper-tcg.h
> @@ -112,6 +112,9 @@ int exception_has_error_code(int intno);
>   void do_smm_enter(X86CPU *cpu);
>   
>   /* bpt_helper.c */
> +void do_end_instruction(CPUX86State *env);
> +
> +/* sysemu/bpt_helper.c */
>   bool check_hw_breakpoints(CPUX86State *env, bool force_dr6_update);
>   
>   #endif /* I386_HELPER_TCG_H */
> diff --git a/target/i386/tcg/bpt_helper.c b/target/i386/tcg/bpt_helper.c
> index bc34ac27fea..9695b9dd041 100644
> --- a/target/i386/tcg/bpt_helper.c
> +++ b/target/i386/tcg/bpt_helper.c
> @@ -37,3 +37,9 @@ void helper_rechecking_single_step(CPUX86State *env)
>           helper_single_step(env);
>       }
>   }
> +
> +void do_end_instruction(CPUX86State *env)
> +{
> +    env->hflags &= ~HF_INHIBIT_IRQ_MASK; /* needed if sti is just before */
> +    env->eflags &= ~HF_RF_MASK;
> +}

Two and insns.  Perhaps place as static inline in helper-tcg.h?

Either way,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 10/11] target/i386: document incorrect semantics of watchpoint following MOV/POP SS

[Richard Henderson]

On 6/4/24 02:18, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   target/i386/tcg/sysemu/bpt_helper.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/target/i386/tcg/sysemu/bpt_helper.c b/target/i386/tcg/sysemu/bpt_helper.c
> index c1d5fce250c..b29acf41c38 100644
> --- a/target/i386/tcg/sysemu/bpt_helper.c
> +++ b/target/i386/tcg/sysemu/bpt_helper.c
> @@ -215,6 +215,12 @@ void breakpoint_handler(CPUState *cs)
>           if (cs->watchpoint_hit->flags & BP_CPU) {
>               cs->watchpoint_hit = NULL;
>               if (check_hw_breakpoints(env, false)) {
> +                /*
> +                 * FIXME: #DB should be delayed by one instruction if
> +                 * INHIBIT_IRQ is set (STI cannot trigger a watchpoint).
> +                 * The delayed #DB should also fuse with one generated
> +                 * by ICEBP (aka INT1).
> +                 */
>                   raise_exception(env, EXCP01_DB);
>               } else {
>                   cpu_loop_exit_noexc(cs);

Should be fixable with some sort of state machine initiated with 
TCGCPUOps.debug_check_watchpoint, but not easy.

Acked-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 11/11] target/i386: document use of DISAS_NORETURN

[Richard Henderson]

On 6/4/24 02:18, Paolo Bonzini wrote:
> DISAS_NORETURN suppresses the work normally done by gen_eob(), and therefore
> must be used in special cases only.  Document them.
> 
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   target/i386/tcg/translate.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 04/11] target/i386: cleanup PAUSE helpers

[Paolo Bonzini]

On Tue, Jun 4, 2024 at 12:59 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 6/4/24 02:18, Paolo Bonzini wrote:
> > Use decode.c's support for intercepts, doing the check in TCG-generated
> > code rather than the helper.  This is cleaner because it allows removing
> > the eip_addend argument to helper_pause(), even though it adds a bit of
> > bloat for opcode 0x90's new decoding function.
> >
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >   target/i386/helper.h                 |  2 +-
> >   target/i386/tcg/helper-tcg.h         |  1 -
> >   target/i386/tcg/misc_helper.c        | 10 +---------
> >   target/i386/tcg/sysemu/misc_helper.c |  2 +-
> >   target/i386/tcg/decode-new.c.inc     | 15 ++++++++++++++-
> >   target/i386/tcg/emit.c.inc           | 20 ++++++++------------
> >   6 files changed, 25 insertions(+), 25 deletions(-)
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
> > +static void decode_90(DisasContext *s, CPUX86State *env, X86OpEntry *entry, uint8_t *b)
> > +{
> > +    static X86OpEntry pause = X86_OP_ENTRY0(PAUSE, svm(PAUSE));
> > +    static X86OpEntry nop = X86_OP_ENTRY0(NOP);
> > +    static X86OpEntry xchg_ax = X86_OP_ENTRY2(XCHG, 0,v, LoBits,v);
> > +
> > +    if (REX_B(s)) {
> > +        *entry = xchg_ax;
> > +    } else {
> > +        *entry = (s->prefix & PREFIX_REPZ) ? pause : nop;
> > +    }
> > +}
>
> Thanks.  I had wished for this
> from the beginning, but since it wasn't wrong, I didn't mention it.

Yeah, I was still making up my mind on the style. Using set_cc_op()
for BMI instructions was also silly, I'll fix it soon.

Basically I want to get to a point where the common code and the
helpers are all set for implementing APX, then we'll see.

Paolo

Re: [PATCH 08/11] target/i386: fix INHIBIT_IRQ/TF/RF handling for PAUSE

[Paolo Bonzini]

On Tue, Jun 4, 2024 at 3:49 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
> Oh, based on the next patch, it would appear that PAUSE does not single-step properly
> because it sets EXCP_INTERRUPT, and end-of-insn single-step depends on exception_index ==
> -1.  I'm thinking of the bottom of cpu_tb_exec().

I'm not sure we're talking of the same thing, but that's why I'm
calling helper_rechecking_single_step() before setting EXCP_INTERRUPT.

Paolo

Re: [PATCH 08/11] target/i386: fix INHIBIT_IRQ/TF/RF handling for PAUSE

[Richard Henderson]

On 6/4/24 09:10, Paolo Bonzini wrote:
> On Tue, Jun 4, 2024 at 3:49 PM Richard Henderson
> <richard.henderson@linaro.org> wrote:
>> Oh, based on the next patch, it would appear that PAUSE does not single-step properly
>> because it sets EXCP_INTERRUPT, and end-of-insn single-step depends on exception_index ==
>> -1.  I'm thinking of the bottom of cpu_tb_exec().
> 
> I'm not sure we're talking of the same thing, but that's why I'm
> calling helper_rechecking_single_step() before setting EXCP_INTERRUPT.

Oh yes, that does it.


r~