On Fri, May 24, 2024 at 5:02 PM Paolo Bonzini <pbonz...@redhat.com> wrote:
>
> On Fri, May 24, 2024 at 4:23 PM Richard Henderson
> <richard.hender...@linaro.org> wrote:
> >
> > On 5/24/24 01:10, Paolo Bonzini wrote:
> > > Place DISAS_* constants that update cpu_eip first, and
> > > the "jump" ones last. Add comments explaining the differences
> > > and usage.
> > >
> > > Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
> > > ---
> > > target/i386/tcg/translate.c | 25 ++++++++++++++++++++++---
> > > 1 file changed, 22 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
> > > index 3c7d8d72144..52d758a224b 100644
> > > --- a/target/i386/tcg/translate.c
> > > +++ b/target/i386/tcg/translate.c
> > > @@ -144,9 +144,28 @@ typedef struct DisasContext {
> > > TCGOp *prev_insn_end;
> > > } DisasContext;
> > >
> > > -#define DISAS_EOB_ONLY DISAS_TARGET_0
> > > -#define DISAS_EOB_NEXT DISAS_TARGET_1
> > > -#define DISAS_EOB_INHIBIT_IRQ DISAS_TARGET_2
> > > +/*
> > > + * Point EIP to next instruction before ending translation.
> > > + * For instructions that can change hflags.
> > > + */
> > > +#define DISAS_EOB_NEXT DISAS_TARGET_0
> > > +
> > > +/*
> > > + * Point EIP to next instruction and set HF_INHIBIT_IRQ if not
> > > + * already set. For instructions that activate interrupt shadow.
> > > + */
> > > +#define DISAS_EOB_INHIBIT_IRQ DISAS_TARGET_1
> > > +
> > > +/*
> > > + * EIP has already been updated. For jumps that do not use
> > > + * lookup_and_goto_ptr()
> > > + */
> > > +#define DISAS_EOB_ONLY DISAS_TARGET_2
> >
> > Better as "for instructions that must return to the main loop", because
> > pure jumps should
> > either use goto_tb (DISAS_NORETURN) or lookup_and_goto_ptr (DISAS_JUMP).
>
> I guess it depends on what you mean by jump... Instructions that use
> DISAS_EOB_ONLY are "jumpy" - IRET, RETF, SYSENTER, SYSEXIT, RSM. They
> cannot use DISAS_NORETURN because they need to call gen_update_cc_op()
> before finishing the TB.
>
> Except now that I think about it, at the end of the series they don't
> set cc_op anymore, so probably there's room for some more cleanup
> there and DISAS_EOB_ONLY can be removed.
... and nope, it's the other way round - DISAS_NORETURN is a bug
waiting to happen for x86 translation because it doesn't process any
of HF_INHIBIT_IRQ_MASK, HF_RF_MASK or HF_TF_MASK.
Paolo
