[PATCH] target/s390x: move tcg_gen_insn_start to s390x_tr_insn_start

Richard Henderson posted 1 patch 2 years, 6 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20211011185332.166763-1-richard.henderson@linaro.org
Maintainers: Thomas Huth <thuth@redhat.com>, David Hildenbrand <david@redhat.com>, Cornelia Huck <cohuck@redhat.com>, Richard Henderson <richard.henderson@linaro.org>
target/s390x/tcg/translate.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
[PATCH] target/s390x: move tcg_gen_insn_start to s390x_tr_insn_start
Posted by Richard Henderson 2 years, 6 months ago
We use INDEX_op_insn_start to make the start of instruction boundaries.
If we don't do it in the .insn_start hook things get confused especially
now plugins want to use that marking to identify the start of instructions
and will bomb out if it sees instrumented ops before the first instruction
boundary.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/tcg/translate.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index f284870cd2..a2d6fa5cca 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -138,6 +138,7 @@ struct DisasFields {
 struct DisasContext {
     DisasContextBase base;
     const DisasInsn *insn;
+    TCGOp *insn_start;
     DisasFields fields;
     uint64_t ex_value;
     /*
@@ -6380,8 +6381,8 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
     /* Search for the insn in the table.  */
     insn = extract_insn(env, s);
 
-    /* Emit insn_start now that we know the ILEN.  */
-    tcg_gen_insn_start(s->base.pc_next, s->cc_op, s->ilen);
+    /* Update insn_start now that we know the ILEN.  */
+    tcg_set_insn_start_param(s->insn_start, 2, s->ilen);
 
     /* Not found means unimplemented/illegal opcode.  */
     if (insn == NULL) {
@@ -6552,6 +6553,11 @@ static void s390x_tr_tb_start(DisasContextBase *db, CPUState *cs)
 
 static void s390x_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
 {
+    DisasContext *dc = container_of(dcbase, DisasContext, base);
+
+    /* Delay the set of ilen until we've read the insn. */
+    tcg_gen_insn_start(dc->base.pc_next, dc->cc_op, 0);
+    dc->insn_start = tcg_last_op();
 }
 
 static void s390x_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
-- 
2.25.1


Re: [PATCH] target/s390x: move tcg_gen_insn_start to s390x_tr_insn_start
Posted by Alex Bennée 2 years, 6 months ago
Richard Henderson <richard.henderson@linaro.org> writes:

> We use INDEX_op_insn_start to make the start of instruction boundaries.
> If we don't do it in the .insn_start hook things get confused especially
> now plugins want to use that marking to identify the start of instructions
> and will bomb out if it sees instrumented ops before the first instruction
> boundary.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Well that is considerably simpler than my patch... I'll apply it to the
PR and resubmit ;-)

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>


> ---
>  target/s390x/tcg/translate.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
> index f284870cd2..a2d6fa5cca 100644
> --- a/target/s390x/tcg/translate.c
> +++ b/target/s390x/tcg/translate.c
> @@ -138,6 +138,7 @@ struct DisasFields {
>  struct DisasContext {
>      DisasContextBase base;
>      const DisasInsn *insn;
> +    TCGOp *insn_start;
>      DisasFields fields;
>      uint64_t ex_value;
>      /*
> @@ -6380,8 +6381,8 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
>      /* Search for the insn in the table.  */
>      insn = extract_insn(env, s);
>  
> -    /* Emit insn_start now that we know the ILEN.  */
> -    tcg_gen_insn_start(s->base.pc_next, s->cc_op, s->ilen);
> +    /* Update insn_start now that we know the ILEN.  */
> +    tcg_set_insn_start_param(s->insn_start, 2, s->ilen);
>  
>      /* Not found means unimplemented/illegal opcode.  */
>      if (insn == NULL) {
> @@ -6552,6 +6553,11 @@ static void s390x_tr_tb_start(DisasContextBase *db, CPUState *cs)
>  
>  static void s390x_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
>  {
> +    DisasContext *dc = container_of(dcbase, DisasContext, base);
> +
> +    /* Delay the set of ilen until we've read the insn. */
> +    tcg_gen_insn_start(dc->base.pc_next, dc->cc_op, 0);
> +    dc->insn_start = tcg_last_op();
>  }
>  
>  static void s390x_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)


-- 
Alex Bennée