1. Patchew
  2. QEMU
  3. [PATCH 00/16] target/i386/tcg: translation cleanups
  4. View patch

[PATCH 03/16] target/i386: document and group DISAS_* constants

Paolo Bonzini posted 16 patches 1 year, 10 months ago
[PATCH 03/16] target/i386: document and group DISAS_* constants
Posted by Paolo Bonzini 1 year, 10 months ago 
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
+
+/*
+ * 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. */
-- 
2.45.1
Re: [PATCH 03/16] target/i386: document and group DISAS_* constants
Posted by Richard Henderson 1 year, 10 months ago 
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~
Re: [PATCH 03/16] target/i386: document and group DISAS_* constants
Posted by Paolo Bonzini 1 year, 10 months ago 
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
Re: [PATCH 03/16] target/i386: document and group DISAS_* constants
Posted by Paolo Bonzini 1 year, 10 months ago 
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
Re: [PATCH 03/16] target/i386: document and group DISAS_* constants
Posted by Richard Henderson 1 year, 10 months ago 
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~
Re: [PATCH 03/16] target/i386: document and group DISAS_* constants
Posted by Paolo Bonzini 1 year, 10 months ago 
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
Re: [PATCH 03/16] target/i386: document and group DISAS_* constants
Posted by Richard Henderson 1 year, 10 months ago 
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~

Patchew 2.1-devel-8c64711

© 2016 - 2026 Red Hat, Inc.