* [PATCH] target/i386: always go through gen_eob*()
@ 2024-05-24 15:33 Paolo Bonzini
2024-05-24 16:50 ` Richard Henderson
0 siblings, 1 reply; 3+ messages in thread
From: Paolo Bonzini @ 2024-05-24 15:33 UTC (permalink / raw)
To: qemu-devel; +Cc: richard.henderson, qemu-stable
Using DISAS_NORETURN does not process any of HF_INHIBIT_IRQ_MASK,
HF_RF_MASK or HF_TF_MASK. Never use it, instead there is
DISAS_EOB_ONLY.
Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/i386/tcg/translate.c | 18 ++++++++++++------
target/i386/tcg/emit.c.inc | 4 ++--
2 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index ebcff8766cf..df10e7d8a6a 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -1406,7 +1406,7 @@ static void gen_exception(DisasContext *s, int trapno)
gen_update_cc_op(s);
gen_update_eip_cur(s);
gen_helper_raise_exception(tcg_env, tcg_constant_i32(trapno));
- s->base.is_jmp = DISAS_NORETURN;
+ s->base.is_jmp = DISAS_EOB_ONLY;
}
/* Generate #UD for the current instruction. The assumption here is that
@@ -2191,7 +2191,7 @@ static void gen_interrupt(DisasContext *s, uint8_t intno)
gen_update_eip_cur(s);
gen_helper_raise_interrupt(tcg_env, tcg_constant_i32(intno),
cur_insn_len_i32(s));
- s->base.is_jmp = DISAS_NORETURN;
+ s->base.is_jmp = DISAS_EOB_ONLY;
}
static void gen_set_hflag(DisasContext *s, uint32_t mask)
@@ -2354,7 +2354,7 @@ static void gen_jmp_rel(DisasContext *s, MemOp ot, int diff, int tb_num)
tcg_gen_movi_tl(cpu_eip, new_eip);
}
tcg_gen_exit_tb(s->base.tb, tb_num);
- s->base.is_jmp = DISAS_NORETURN;
+ s->base.is_jmp = DISAS_EOB_ONLY;
} else {
if (!(tb_cflags(s->base.tb) & CF_PCREL)) {
tcg_gen_movi_tl(cpu_eip, new_eip);
@@ -3520,7 +3520,7 @@ static void disas_insn_old(DisasContext *s, CPUState *cpu, int b)
gen_update_cc_op(s);
gen_update_eip_cur(s);
gen_helper_rdpmc(tcg_env);
- s->base.is_jmp = DISAS_NORETURN;
+ s->base.is_jmp = DISAS_EOB_ONLY;
break;
case 0x134: /* sysenter */
/* For AMD SYSENTER is not valid in long mode */
@@ -3690,7 +3690,7 @@ static void disas_insn_old(DisasContext *s, CPUState *cpu, int b)
gen_update_cc_op(s);
gen_update_eip_cur(s);
gen_helper_mwait(tcg_env, cur_insn_len_i32(s));
- s->base.is_jmp = DISAS_NORETURN;
+ s->base.is_jmp = DISAS_EOB_ONLY;
break;
case 0xca: /* clac */
@@ -3769,7 +3769,7 @@ static void disas_insn_old(DisasContext *s, CPUState *cpu, int b)
gen_helper_vmrun(tcg_env, tcg_constant_i32(s->aflag - 1),
cur_insn_len_i32(s));
tcg_gen_exit_tb(NULL, 0);
- s->base.is_jmp = DISAS_NORETURN;
+ s->base.is_jmp = DISAS_EOB_ONLY;
break;
case 0xd9: /* VMMCALL */
@@ -4770,6 +4770,11 @@ static void i386_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
switch (dc->base.is_jmp) {
case DISAS_NORETURN:
+ /*
+ * Nothing to do, gen_eob*() was already called. DISAS_NORETURN is
+ * never set explicitly except in gen_eob_worker(), because that is
+ * where HF_INHIBIT_IRQ_MASK, HF_RF_MASK and HF_TF_MASK are handled.
+ */
break;
case DISAS_TOO_MANY:
gen_update_cc_op(dc);
@@ -4793,6 +4798,7 @@ static void i386_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
default:
g_assert_not_reached();
}
+ assert(dc->base.is_jmp == DISAS_NORETURN);
}
static const TranslatorOps i386_tr_ops = {
diff --git a/target/i386/tcg/emit.c.inc b/target/i386/tcg/emit.c.inc
index c78e35b1e28..14464074d5a 100644
--- a/target/i386/tcg/emit.c.inc
+++ b/target/i386/tcg/emit.c.inc
@@ -1642,7 +1642,7 @@ static void gen_HLT(DisasContext *s, CPUX86State *env, X86DecodedInsn *decode)
gen_update_cc_op(s);
gen_update_eip_cur(s);
gen_helper_hlt(tcg_env, cur_insn_len_i32(s));
- s->base.is_jmp = DISAS_NORETURN;
+ s->base.is_jmp = DISAS_EOB_ONLY;
#endif
}
@@ -4022,7 +4022,7 @@ static void gen_XCHG(DisasContext *s, CPUX86State *env, X86DecodedInsn *decode)
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;
+ s->base.is_jmp = DISAS_EOB_ONLY;
}
/* No writeback. */
decode->op[0].unit = X86_OP_SKIP;
--
2.45.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] target/i386: always go through gen_eob*()
2024-05-24 15:33 [PATCH] target/i386: always go through gen_eob*() Paolo Bonzini
@ 2024-05-24 16:50 ` Richard Henderson
2024-05-25 8:57 ` Paolo Bonzini
0 siblings, 1 reply; 3+ messages in thread
From: Richard Henderson @ 2024-05-24 16:50 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: qemu-stable
On 5/24/24 08:33, Paolo Bonzini wrote:
> Using DISAS_NORETURN does not process any of HF_INHIBIT_IRQ_MASK,
> HF_RF_MASK or HF_TF_MASK. Never use it, instead there is
> DISAS_EOB_ONLY.
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> target/i386/tcg/translate.c | 18 ++++++++++++------
> target/i386/tcg/emit.c.inc | 4 ++--
> 2 files changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
> index ebcff8766cf..df10e7d8a6a 100644
> --- a/target/i386/tcg/translate.c
> +++ b/target/i386/tcg/translate.c
> @@ -1406,7 +1406,7 @@ static void gen_exception(DisasContext *s, int trapno)
> gen_update_cc_op(s);
> gen_update_eip_cur(s);
> gen_helper_raise_exception(tcg_env, tcg_constant_i32(trapno));
> - s->base.is_jmp = DISAS_NORETURN;
> + s->base.is_jmp = DISAS_EOB_ONLY;
This is wrong, because we exit via exception, right here.
Anything you add afterward is unreachable.
> }
>
> /* Generate #UD for the current instruction. The assumption here is that
> @@ -2191,7 +2191,7 @@ static void gen_interrupt(DisasContext *s, uint8_t intno)
> gen_update_eip_cur(s);
> gen_helper_raise_interrupt(tcg_env, tcg_constant_i32(intno),
> cur_insn_len_i32(s));
> - s->base.is_jmp = DISAS_NORETURN;
> + s->base.is_jmp = DISAS_EOB_ONLY;
Likewise.
> }
>
> static void gen_set_hflag(DisasContext *s, uint32_t mask)
> @@ -2354,7 +2354,7 @@ static void gen_jmp_rel(DisasContext *s, MemOp ot, int diff, int tb_num)
> tcg_gen_movi_tl(cpu_eip, new_eip);
> }
> tcg_gen_exit_tb(s->base.tb, tb_num);
> - s->base.is_jmp = DISAS_NORETURN;
> + s->base.is_jmp = DISAS_EOB_ONLY;
This is wrong because exit_tb exits, and anything you add after is unreachable.
I think you simply want to remove the exit_tb call as well, but there may be more cleanup
possible in the wider context; I haven't checked.
> } else {
> if (!(tb_cflags(s->base.tb) & CF_PCREL)) {
> tcg_gen_movi_tl(cpu_eip, new_eip);
> @@ -3520,7 +3520,7 @@ static void disas_insn_old(DisasContext *s, CPUState *cpu, int b)
> gen_update_cc_op(s);
> gen_update_eip_cur(s);
> gen_helper_rdpmc(tcg_env);
> - s->base.is_jmp = DISAS_NORETURN;
> + s->base.is_jmp = DISAS_EOB_ONLY;
This is wrong because helper_rdpmc is noreturn, always raising an exception.
> @@ -3690,7 +3690,7 @@ static void disas_insn_old(DisasContext *s, CPUState *cpu, int b)
> gen_update_cc_op(s);
> gen_update_eip_cur(s);
> gen_helper_mwait(tcg_env, cur_insn_len_i32(s));
> - s->base.is_jmp = DISAS_NORETURN;
> + s->base.is_jmp = DISAS_EOB_ONLY;
Likewise.
> @@ -3769,7 +3769,7 @@ static void disas_insn_old(DisasContext *s, CPUState *cpu, int b)
> gen_helper_vmrun(tcg_env, tcg_constant_i32(s->aflag - 1),
> cur_insn_len_i32(s));
> tcg_gen_exit_tb(NULL, 0);
> - s->base.is_jmp = DISAS_NORETURN;
> + s->base.is_jmp = DISAS_EOB_ONLY;
Calls exit_tb, which is probably bogus here and EOB_ONLY is correct.
But I'd need to look deeper into what vmrun does.
> switch (dc->base.is_jmp) {
> case DISAS_NORETURN:
> + /*
> + * Nothing to do, gen_eob*() was already called. DISAS_NORETURN is
> + * never set explicitly except in gen_eob_worker(), because that is
> + * where HF_INHIBIT_IRQ_MASK, HF_RF_MASK and HF_TF_MASK are handled.
> + */
Comment is wrong because exceptions *should* set NORETURN.
All of the masks are irrelevant to #gp or #ud etc.
> @@ -1642,7 +1642,7 @@ static void gen_HLT(DisasContext *s, CPUX86State *env, X86DecodedInsn *decode)
> gen_update_cc_op(s);
> gen_update_eip_cur(s);
> gen_helper_hlt(tcg_env, cur_insn_len_i32(s));
> - s->base.is_jmp = DISAS_NORETURN;
> + s->base.is_jmp = DISAS_EOB_ONLY;
noreturn.
> @@ -4022,7 +4022,7 @@ static void gen_XCHG(DisasContext *s, CPUX86State *env, X86DecodedInsn *decode)
> 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;
> + s->base.is_jmp = DISAS_EOB_ONLY;
noreturn.
r~
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] target/i386: always go through gen_eob*()
2024-05-24 16:50 ` Richard Henderson
@ 2024-05-25 8:57 ` Paolo Bonzini
0 siblings, 0 replies; 3+ messages in thread
From: Paolo Bonzini @ 2024-05-25 8:57 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-devel, qemu-stable
On Fri, May 24, 2024 at 6:51 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
> > static void gen_set_hflag(DisasContext *s, uint32_t mask)
> > @@ -2354,7 +2354,7 @@ static void gen_jmp_rel(DisasContext *s, MemOp ot, int diff, int tb_num)
> > tcg_gen_movi_tl(cpu_eip, new_eip);
> > }
> > tcg_gen_exit_tb(s->base.tb, tb_num);
> > - s->base.is_jmp = DISAS_NORETURN;
> > + s->base.is_jmp = DISAS_EOB_ONLY;
>
> This is wrong because exit_tb exits, and anything you add after is unreachable.
> I think you simply want to remove the exit_tb call as well, but there may be more cleanup
> possible in the wider context; I haven't checked.
Ok, I'll check this one more closely.
> > @@ -3769,7 +3769,7 @@ static void disas_insn_old(DisasContext *s, CPUState *cpu, int b)
> > gen_helper_vmrun(tcg_env, tcg_constant_i32(s->aflag - 1),
> > cur_insn_len_i32(s));
> > tcg_gen_exit_tb(NULL, 0);
> > - s->base.is_jmp = DISAS_NORETURN;
> > + s->base.is_jmp = DISAS_EOB_ONLY;
>
> Calls exit_tb, which is probably bogus here and EOB_ONLY is correct.
> But I'd need to look deeper into what vmrun does.
This is correct, but do_vmexit() needs to clear RF and handle
singlestep, and the helper needs to clear HF_INHIBIT_IRQ_MASK. In this
respect VMRUN/vmexit are is not unlike SYSRET/SYSCALL respectively,
except that EFLAGS.TF is never set right after VMRUN. That is, the
guest EFLAGS value has its effect only after the first instruction in
the guest, while the SYSCALL EFLAGS value interrupts before the first
instruction in CPL0.
> > @@ -1642,7 +1642,7 @@ static void gen_HLT(DisasContext *s, CPUX86State *env, X86DecodedInsn *decode)
> > gen_update_cc_op(s);
> > gen_update_eip_cur(s);
> > gen_helper_hlt(tcg_env, cur_insn_len_i32(s));
> > - s->base.is_jmp = DISAS_NORETURN;
> > + s->base.is_jmp = DISAS_EOB_ONLY;
>
> noreturn.
>
> > @@ -4022,7 +4022,7 @@ static void gen_XCHG(DisasContext *s, CPUX86State *env, X86DecodedInsn *decode)
> > 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;
> > + s->base.is_jmp = DISAS_EOB_ONLY;
>
> noreturn.
But these should handle HF_INHIBIT_IRQ_MASK/RF/TF and they don't
(except for HLT clearing HF_INHIBIT_IRQ_MASK). So there is a bug but
it's in the helpers.
Paolo
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-05-25 8:59 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-24 15:33 [PATCH] target/i386: always go through gen_eob*() Paolo Bonzini
2024-05-24 16:50 ` Richard Henderson
2024-05-25 8:57 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).