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

[Richard Henderson <richard.henderson@linaro.org>]

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~