1. Patchew
  2. QEMU
  3. View series

[PATCH] target/i386: always go through gen_eob*()

Paolo Bonzini posted 1 patch 6 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20240524153323.1267511-2-pbonzini@redhat.com
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Eduardo Habkost <eduardo@habkost.net>
target/i386/tcg/translate.c | 18 ++++++++++++------
target/i386/tcg/emit.c.inc  |  4 ++--
2 files changed, 14 insertions(+), 8 deletions(-)
[PATCH] target/i386: always go through gen_eob*()
Posted by Paolo Bonzini 6 months ago 
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
Re: [PATCH] target/i386: always go through gen_eob*()
Posted by Richard Henderson 6 months ago 
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~
Re: [PATCH] target/i386: always go through gen_eob*()
Posted by Paolo Bonzini 6 months ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.