| Message ID | 20240524081019.1141359-4-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | target/i386/tcg: translation cleanups | expand |
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<pbonzini@redhat.com> > --- > target/i386/tcg/translate.c | 25 ++++++++++++++++++++++--- > 1 file changed, 22 insertions(+), 3 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
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 <pbonzini@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). Otherwise, Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On Fri, May 24, 2024 at 4:23 PM Richard Henderson <richard.henderson@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 <pbonzini@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. Paolo
On Fri, May 24, 2024 at 5:02 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On Fri, May 24, 2024 at 4:23 PM Richard Henderson > <richard.henderson@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 <pbonzini@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
On 5/24/24 08:04, Paolo Bonzini wrote: > ... 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. Do you need to suppress use_goto_tb in these cases? r~
On Fri, May 24, 2024 at 5:13 PM Richard Henderson <richard.henderson@linaro.org> wrote: > > On 5/24/24 08:04, Paolo Bonzini wrote: > > ... 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. > > Do you need to suppress use_goto_tb in these cases? HF_INHIBIT_IRQ_MASK and HF_TF_MASK already do, HF_RF_MASK is missing. Nice catch. Paolo
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 + +/* + * EIP has already been updated. For jumps that wish to use + * lookup_and_goto_ptr() + */ #define DISAS_JUMP DISAS_TARGET_3 /* The environment in which user-only runs is constrained. */
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 <pbonzini@redhat.com> --- target/i386/tcg/translate.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-)