* [PATCH 01/11] target/i386: fix pushed value of EFLAGS.RF
2024-06-04 7:18 [PATCH 00/11] target/i386: fixes for INHIBIT_IRQ, TF and RF Paolo Bonzini
@ 2024-06-04 7:18 ` Paolo Bonzini
2024-06-04 10:51 ` Richard Henderson
2024-06-04 7:18 ` [PATCH 02/11] target/i386: fix implementation of ICEBP Paolo Bonzini
` (9 subsequent siblings)
10 siblings, 1 reply; 27+ messages in thread
From: Paolo Bonzini @ 2024-06-04 7:18 UTC (permalink / raw)
To: qemu-devel; +Cc: richard.henderson
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
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 01/11] target/i386: fix pushed value of EFLAGS.RF
2024-06-04 7:18 ` [PATCH 01/11] target/i386: fix pushed value of EFLAGS.RF Paolo Bonzini
@ 2024-06-04 10:51 ` Richard Henderson
0 siblings, 0 replies; 27+ messages in thread
From: Richard Henderson @ 2024-06-04 10:51 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
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~
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 02/11] target/i386: fix implementation of ICEBP
2024-06-04 7:18 [PATCH 00/11] target/i386: fixes for INHIBIT_IRQ, TF and RF Paolo Bonzini
2024-06-04 7:18 ` [PATCH 01/11] target/i386: fix pushed value of EFLAGS.RF Paolo Bonzini
@ 2024-06-04 7:18 ` Paolo Bonzini
2024-06-04 13:50 ` Richard Henderson
2024-06-04 7:18 ` [PATCH 03/11] target/i386: cleanup HLT helpers Paolo Bonzini
` (8 subsequent siblings)
10 siblings, 1 reply; 27+ messages in thread
From: Paolo Bonzini @ 2024-06-04 7:18 UTC (permalink / raw)
To: qemu-devel; +Cc: richard.henderson
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
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 02/11] target/i386: fix implementation of ICEBP
2024-06-04 7:18 ` [PATCH 02/11] target/i386: fix implementation of ICEBP Paolo Bonzini
@ 2024-06-04 13:50 ` Richard Henderson
0 siblings, 0 replies; 27+ messages in thread
From: Richard Henderson @ 2024-06-04 13:50 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
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~
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 03/11] target/i386: cleanup HLT helpers
2024-06-04 7:18 [PATCH 00/11] target/i386: fixes for INHIBIT_IRQ, TF and RF Paolo Bonzini
2024-06-04 7:18 ` [PATCH 01/11] target/i386: fix pushed value of EFLAGS.RF Paolo Bonzini
2024-06-04 7:18 ` [PATCH 02/11] target/i386: fix implementation of ICEBP Paolo Bonzini
@ 2024-06-04 7:18 ` Paolo Bonzini
2024-06-04 10:54 ` Richard Henderson
2024-06-04 7:18 ` [PATCH 04/11] target/i386: cleanup PAUSE helpers Paolo Bonzini
` (7 subsequent siblings)
10 siblings, 1 reply; 27+ messages in thread
From: Paolo Bonzini @ 2024-06-04 7:18 UTC (permalink / raw)
To: qemu-devel; +Cc: richard.henderson
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
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 03/11] target/i386: cleanup HLT helpers
2024-06-04 7:18 ` [PATCH 03/11] target/i386: cleanup HLT helpers Paolo Bonzini
@ 2024-06-04 10:54 ` Richard Henderson
0 siblings, 0 replies; 27+ messages in thread
From: Richard Henderson @ 2024-06-04 10:54 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
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~
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 04/11] target/i386: cleanup PAUSE helpers
2024-06-04 7:18 [PATCH 00/11] target/i386: fixes for INHIBIT_IRQ, TF and RF Paolo Bonzini
` (2 preceding siblings ...)
2024-06-04 7:18 ` [PATCH 03/11] target/i386: cleanup HLT helpers Paolo Bonzini
@ 2024-06-04 7:18 ` Paolo Bonzini
2024-06-04 10:59 ` Richard Henderson
2024-06-04 7:18 ` [PATCH 05/11] target/i386: implement DR7.GD Paolo Bonzini
` (6 subsequent siblings)
10 siblings, 1 reply; 27+ messages in thread
From: Paolo Bonzini @ 2024-06-04 7:18 UTC (permalink / raw)
To: qemu-devel; +Cc: richard.henderson
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
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 04/11] target/i386: cleanup PAUSE helpers
2024-06-04 7:18 ` [PATCH 04/11] target/i386: cleanup PAUSE helpers Paolo Bonzini
@ 2024-06-04 10:59 ` Richard Henderson
2024-06-04 14:08 ` Paolo Bonzini
0 siblings, 1 reply; 27+ messages in thread
From: Richard Henderson @ 2024-06-04 10:59 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
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~
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 04/11] target/i386: cleanup PAUSE helpers
2024-06-04 10:59 ` Richard Henderson
@ 2024-06-04 14:08 ` Paolo Bonzini
0 siblings, 0 replies; 27+ messages in thread
From: Paolo Bonzini @ 2024-06-04 14:08 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 05/11] target/i386: implement DR7.GD
2024-06-04 7:18 [PATCH 00/11] target/i386: fixes for INHIBIT_IRQ, TF and RF Paolo Bonzini
` (3 preceding siblings ...)
2024-06-04 7:18 ` [PATCH 04/11] target/i386: cleanup PAUSE helpers Paolo Bonzini
@ 2024-06-04 7:18 ` Paolo Bonzini
2024-06-04 13:22 ` Richard Henderson
2024-06-04 7:18 ` [PATCH 06/11] target/i386: disable/enable breakpoints on vmentry/vmexit Paolo Bonzini
` (5 subsequent siblings)
10 siblings, 1 reply; 27+ messages in thread
From: Paolo Bonzini @ 2024-06-04 7:18 UTC (permalink / raw)
To: qemu-devel; +Cc: richard.henderson
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
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 06/11] target/i386: disable/enable breakpoints on vmentry/vmexit
2024-06-04 7:18 [PATCH 00/11] target/i386: fixes for INHIBIT_IRQ, TF and RF Paolo Bonzini
` (4 preceding siblings ...)
2024-06-04 7:18 ` [PATCH 05/11] target/i386: implement DR7.GD Paolo Bonzini
@ 2024-06-04 7:18 ` Paolo Bonzini
2024-06-04 13:24 ` Richard Henderson
2024-06-04 7:18 ` [PATCH 07/11] target/i386: fix INHIBIT_IRQ/TF/RF handling for VMRUN Paolo Bonzini
` (4 subsequent siblings)
10 siblings, 1 reply; 27+ messages in thread
From: Paolo Bonzini @ 2024-06-04 7:18 UTC (permalink / raw)
To: qemu-devel; +Cc: richard.henderson
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
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 06/11] target/i386: disable/enable breakpoints on vmentry/vmexit
2024-06-04 7:18 ` [PATCH 06/11] target/i386: disable/enable breakpoints on vmentry/vmexit Paolo Bonzini
@ 2024-06-04 13:24 ` Richard Henderson
0 siblings, 0 replies; 27+ messages in thread
From: Richard Henderson @ 2024-06-04 13:24 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
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~
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 07/11] target/i386: fix INHIBIT_IRQ/TF/RF handling for VMRUN
2024-06-04 7:18 [PATCH 00/11] target/i386: fixes for INHIBIT_IRQ, TF and RF Paolo Bonzini
` (5 preceding siblings ...)
2024-06-04 7:18 ` [PATCH 06/11] target/i386: disable/enable breakpoints on vmentry/vmexit Paolo Bonzini
@ 2024-06-04 7:18 ` Paolo Bonzini
2024-06-04 13:28 ` Richard Henderson
2024-06-04 7:18 ` [PATCH 08/11] target/i386: fix INHIBIT_IRQ/TF/RF handling for PAUSE Paolo Bonzini
` (3 subsequent siblings)
10 siblings, 1 reply; 27+ messages in thread
From: Paolo Bonzini @ 2024-06-04 7:18 UTC (permalink / raw)
To: qemu-devel; +Cc: richard.henderson
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
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 07/11] target/i386: fix INHIBIT_IRQ/TF/RF handling for VMRUN
2024-06-04 7:18 ` [PATCH 07/11] target/i386: fix INHIBIT_IRQ/TF/RF handling for VMRUN Paolo Bonzini
@ 2024-06-04 13:28 ` Richard Henderson
0 siblings, 0 replies; 27+ messages in thread
From: Richard Henderson @ 2024-06-04 13:28 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
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~
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 08/11] target/i386: fix INHIBIT_IRQ/TF/RF handling for PAUSE
2024-06-04 7:18 [PATCH 00/11] target/i386: fixes for INHIBIT_IRQ, TF and RF Paolo Bonzini
` (6 preceding siblings ...)
2024-06-04 7:18 ` [PATCH 07/11] target/i386: fix INHIBIT_IRQ/TF/RF handling for VMRUN Paolo Bonzini
@ 2024-06-04 7:18 ` Paolo Bonzini
2024-06-04 13:44 ` Richard Henderson
2024-06-04 7:18 ` [PATCH 09/11] target/i386: fix TF/RF handling for HLT Paolo Bonzini
` (2 subsequent siblings)
10 siblings, 1 reply; 27+ messages in thread
From: Paolo Bonzini @ 2024-06-04 7:18 UTC (permalink / raw)
To: qemu-devel; +Cc: richard.henderson
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
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 08/11] target/i386: fix INHIBIT_IRQ/TF/RF handling for PAUSE
2024-06-04 7:18 ` [PATCH 08/11] target/i386: fix INHIBIT_IRQ/TF/RF handling for PAUSE Paolo Bonzini
@ 2024-06-04 13:44 ` Richard Henderson
2024-06-04 13:49 ` Richard Henderson
0 siblings, 1 reply; 27+ messages in thread
From: Richard Henderson @ 2024-06-04 13:44 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
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~
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 08/11] target/i386: fix INHIBIT_IRQ/TF/RF handling for PAUSE
2024-06-04 13:44 ` Richard Henderson
@ 2024-06-04 13:49 ` Richard Henderson
2024-06-04 14:10 ` Paolo Bonzini
0 siblings, 1 reply; 27+ messages in thread
From: Richard Henderson @ 2024-06-04 13:49 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
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~
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 08/11] target/i386: fix INHIBIT_IRQ/TF/RF handling for PAUSE
2024-06-04 13:49 ` Richard Henderson
@ 2024-06-04 14:10 ` Paolo Bonzini
2024-06-04 14:14 ` Richard Henderson
0 siblings, 1 reply; 27+ messages in thread
From: Paolo Bonzini @ 2024-06-04 14:10 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 08/11] target/i386: fix INHIBIT_IRQ/TF/RF handling for PAUSE
2024-06-04 14:10 ` Paolo Bonzini
@ 2024-06-04 14:14 ` Richard Henderson
0 siblings, 0 replies; 27+ messages in thread
From: Richard Henderson @ 2024-06-04 14:14 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
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~
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 09/11] target/i386: fix TF/RF handling for HLT
2024-06-04 7:18 [PATCH 00/11] target/i386: fixes for INHIBIT_IRQ, TF and RF Paolo Bonzini
` (7 preceding siblings ...)
2024-06-04 7:18 ` [PATCH 08/11] target/i386: fix INHIBIT_IRQ/TF/RF handling for PAUSE Paolo Bonzini
@ 2024-06-04 7:18 ` Paolo Bonzini
2024-06-04 13:46 ` Richard Henderson
2024-06-04 7:18 ` [PATCH 10/11] target/i386: document incorrect semantics of watchpoint following MOV/POP SS Paolo Bonzini
2024-06-04 7:18 ` [PATCH 11/11] target/i386: document use of DISAS_NORETURN Paolo Bonzini
10 siblings, 1 reply; 27+ messages in thread
From: Paolo Bonzini @ 2024-06-04 7:18 UTC (permalink / raw)
To: qemu-devel; +Cc: richard.henderson
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
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 09/11] target/i386: fix TF/RF handling for HLT
2024-06-04 7:18 ` [PATCH 09/11] target/i386: fix TF/RF handling for HLT Paolo Bonzini
@ 2024-06-04 13:46 ` Richard Henderson
0 siblings, 0 replies; 27+ messages in thread
From: Richard Henderson @ 2024-06-04 13:46 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
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~
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 10/11] target/i386: document incorrect semantics of watchpoint following MOV/POP SS
2024-06-04 7:18 [PATCH 00/11] target/i386: fixes for INHIBIT_IRQ, TF and RF Paolo Bonzini
` (8 preceding siblings ...)
2024-06-04 7:18 ` [PATCH 09/11] target/i386: fix TF/RF handling for HLT Paolo Bonzini
@ 2024-06-04 7:18 ` Paolo Bonzini
2024-06-04 13:57 ` Richard Henderson
2024-06-04 7:18 ` [PATCH 11/11] target/i386: document use of DISAS_NORETURN Paolo Bonzini
10 siblings, 1 reply; 27+ messages in thread
From: Paolo Bonzini @ 2024-06-04 7:18 UTC (permalink / raw)
To: qemu-devel; +Cc: richard.henderson
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
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 10/11] target/i386: document incorrect semantics of watchpoint following MOV/POP SS
2024-06-04 7:18 ` [PATCH 10/11] target/i386: document incorrect semantics of watchpoint following MOV/POP SS Paolo Bonzini
@ 2024-06-04 13:57 ` Richard Henderson
0 siblings, 0 replies; 27+ messages in thread
From: Richard Henderson @ 2024-06-04 13:57 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
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~
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 11/11] target/i386: document use of DISAS_NORETURN
2024-06-04 7:18 [PATCH 00/11] target/i386: fixes for INHIBIT_IRQ, TF and RF Paolo Bonzini
` (9 preceding siblings ...)
2024-06-04 7:18 ` [PATCH 10/11] target/i386: document incorrect semantics of watchpoint following MOV/POP SS Paolo Bonzini
@ 2024-06-04 7:18 ` Paolo Bonzini
2024-06-04 13:58 ` Richard Henderson
10 siblings, 1 reply; 27+ messages in thread
From: Paolo Bonzini @ 2024-06-04 7:18 UTC (permalink / raw)
To: qemu-devel; +Cc: richard.henderson
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
^ permalink raw reply related [flat|nested] 27+ messages in thread