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

[Paolo Bonzini <pbonzini@redhat.com>]

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