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: Alex Bennée <alex.bennee@linaro.org>
---
 target/s390x/tcg/translate.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index f284870cd2..fe145ff2eb 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -6380,9 +6380,6 @@ 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);
-
     /* Not found means unimplemented/illegal opcode.  */
     if (insn == NULL) {
         qemu_log_mask(LOG_UNIMP, "unimplemented opcode 0x%02x%02x\n",
@@ -6550,8 +6547,30 @@ static void s390x_tr_tb_start(DisasContextBase *db, CPUState *cs)
 {
 }
 
+/*
+ * We just enough partial instruction decoding here to calculate the
+ * length of the instruction so we can drop the INDEX_op_insn_start
+ * before anything else is emitted in the TCGOp stream.
+ *
+ * See extract_insn for the full decode.
+ */
 static void s390x_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
 {
+    CPUS390XState *env = cs->env_ptr;
+    DisasContext *s = container_of(dcbase, DisasContext, base);
+    uint64_t insn, pc = s->base.pc_next;
+    int op, ilen;
+
+    if (unlikely(s->ex_value)) {
+        ilen = s->ex_value & 0xf;
+    } else {
+        insn = ld_code2(env, s, pc);  /* FIXME: don't reload same pc twice */
+        op = (insn >> 8) & 0xff;
+        ilen = get_ilen(op);
+    }
+
+    /* Emit insn_start now that we know the ILEN.  */
+    tcg_gen_insn_start(s->base.pc_next, s->cc_op, ilen);
 }
 
 static void s390x_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
-- 
2.30.2

On 10/11/21 8:57 AM, Alex Bennée wrote:
> 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: Alex Bennée <alex.bennee@linaro.org>
> ---
>   target/s390x/tcg/translate.c | 25 ++++++++++++++++++++++---
>   1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
> index f284870cd2..fe145ff2eb 100644
> --- a/target/s390x/tcg/translate.c
> +++ b/target/s390x/tcg/translate.c
> @@ -6380,9 +6380,6 @@ 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);
> -
>       /* Not found means unimplemented/illegal opcode.  */
>       if (insn == NULL) {
>           qemu_log_mask(LOG_UNIMP, "unimplemented opcode 0x%02x%02x\n",
> @@ -6550,8 +6547,30 @@ static void s390x_tr_tb_start(DisasContextBase *db, CPUState *cs)
>   {
>   }
>   
> +/*
> + * We just enough partial instruction decoding here to calculate the
> + * length of the instruction so we can drop the INDEX_op_insn_start
> + * before anything else is emitted in the TCGOp stream.
> + *
> + * See extract_insn for the full decode.
> + */
>   static void s390x_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
>   {
> +    CPUS390XState *env = cs->env_ptr;
> +    DisasContext *s = container_of(dcbase, DisasContext, base);
> +    uint64_t insn, pc = s->base.pc_next;
> +    int op, ilen;
> +
> +    if (unlikely(s->ex_value)) {
> +        ilen = s->ex_value & 0xf;
> +    } else {
> +        insn = ld_code2(env, s, pc);  /* FIXME: don't reload same pc twice */

I think we might as well delay the set of ilen until after the normal load, rather than 
introduce a fixme.

r~

> +        op = (insn >> 8) & 0xff;
> +        ilen = get_ilen(op);
> +    }
> +
> +    /* Emit insn_start now that we know the ILEN.  */
> +    tcg_gen_insn_start(s->base.pc_next, s->cc_op, ilen);
>   }
>   
>   static void s390x_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
> 

Richard Henderson <richard.henderson@linaro.org> writes:

> On 10/11/21 8:57 AM, Alex Bennée wrote:
>> 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: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>   target/s390x/tcg/translate.c | 25 ++++++++++++++++++++++---
>>   1 file changed, 22 insertions(+), 3 deletions(-)
>> diff --git a/target/s390x/tcg/translate.c
>> b/target/s390x/tcg/translate.c
>> index f284870cd2..fe145ff2eb 100644
>> --- a/target/s390x/tcg/translate.c
>> +++ b/target/s390x/tcg/translate.c
>> @@ -6380,9 +6380,6 @@ 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);
>> -
>>       /* Not found means unimplemented/illegal opcode.  */
>>       if (insn == NULL) {
>>           qemu_log_mask(LOG_UNIMP, "unimplemented opcode 0x%02x%02x\n",
>> @@ -6550,8 +6547,30 @@ static void s390x_tr_tb_start(DisasContextBase *db, CPUState *cs)
>>   {
>>   }
>>   +/*
>> + * We just enough partial instruction decoding here to calculate the
>> + * length of the instruction so we can drop the INDEX_op_insn_start
>> + * before anything else is emitted in the TCGOp stream.
>> + *
>> + * See extract_insn for the full decode.
>> + */
>>   static void s390x_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
>>   {
>> +    CPUS390XState *env = cs->env_ptr;
>> +    DisasContext *s = container_of(dcbase, DisasContext, base);
>> +    uint64_t insn, pc = s->base.pc_next;
>> +    int op, ilen;
>> +
>> +    if (unlikely(s->ex_value)) {
>> +        ilen = s->ex_value & 0xf;
>> +    } else {
>> +        insn = ld_code2(env, s, pc);  /* FIXME: don't reload same pc twice */
>
> I think we might as well delay the set of ilen until after the normal
> load, rather than introduce a fixme.

I'd got as far as this before thinking it was getting messy:

--8<---------------cut here---------------start------------->8---
squash! target/s390x: move tcg_gen_insn_start to s390x_tr_insn_start

1 file changed, 16 insertions(+), 19 deletions(-)
target/s390x/tcg/translate.c | 35 ++++++++++++++++-------------------

modified   target/s390x/tcg/translate.c
@@ -147,6 +147,7 @@ struct DisasContext {
      */
     uint64_t pc_tmp;
     uint32_t ilen;
+    uint16_t start_of_insn; /* collected at s390x_tr_insn_start */
     enum cc_op cc_op;
     bool do_debug;
 };
@@ -388,10 +389,10 @@ static void update_cc_op(DisasContext *s)
     }
 }
 
-static inline uint64_t ld_code2(CPUS390XState *env, DisasContext *s,
+static inline uint16_t ld_code2(CPUS390XState *env, DisasContext *s,
                                 uint64_t pc)
 {
-    return (uint64_t)translator_lduw(env, &s->base, pc);
+    return translator_lduw(env, &s->base, pc);
 }
 
 static inline uint64_t ld_code4(CPUS390XState *env, DisasContext *s,
@@ -6261,7 +6262,7 @@ static void extract_field(DisasFields *o, const DisasField *f, uint64_t insn)
 static const DisasInsn *extract_insn(CPUS390XState *env, DisasContext *s)
 {
     uint64_t insn, pc = s->base.pc_next;
-    int op, op2, ilen;
+    int op, op2;
     const DisasInsn *info;
 
     if (unlikely(s->ex_value)) {
@@ -6272,28 +6273,25 @@ static const DisasInsn *extract_insn(CPUS390XState *env, DisasContext *s)
 
         /* Extract the values saved by EXECUTE.  */
         insn = s->ex_value & 0xffffffffffff0000ull;
-        ilen = s->ex_value & 0xf;
+        s->ilen = s->ex_value & 0xf;
         op = insn >> 56;
     } else {
-        insn = ld_code2(env, s, pc);
-        op = (insn >> 8) & 0xff;
-        ilen = get_ilen(op);
-        switch (ilen) {
+        op = extract32(s->start_of_insn, 8, 8);
+        insn = deposit64(0, 48, 16, s->start_of_insn);
+        switch (s->ilen) {
         case 2:
-            insn = insn << 48;
             break;
         case 4:
-            insn = ld_code4(env, s, pc) << 32;
+            insn = deposit64(insn, 32, 16, ld_code2(env, s, pc + 2));
             break;
         case 6:
-            insn = (insn << 48) | (ld_code4(env, s, pc + 2) << 16);
+            insn = deposit64(insn, 16, 32, ld_code4(env, s, pc + 2));
             break;
         default:
             g_assert_not_reached();
         }
     }
-    s->pc_tmp = s->base.pc_next + ilen;
-    s->ilen = ilen;
+    s->pc_tmp = s->base.pc_next + s->ilen;
 
     /* We can't actually determine the insn format until we've looked up
        the full insn opcode.  Which we can't do without locating the
@@ -6558,18 +6556,17 @@ static void s390x_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
 {
     CPUS390XState *env = cs->env_ptr;
     DisasContext *s = container_of(dcbase, DisasContext, base);
-    uint64_t insn, pc = s->base.pc_next;
-    int op, ilen;
+    uint64_t pc = s->base.pc_next;
+    int ilen;
 
     if (unlikely(s->ex_value)) {
         ilen = s->ex_value & 0xf;
     } else {
-        insn = ld_code2(env, s, pc);  /* FIXME: don't reload same pc twice */
-        op = (insn >> 8) & 0xff;
-        ilen = get_ilen(op);
+        s->start_of_insn = ld_code2(env, s, pc);
+        ilen = get_ilen(extract64(s->start_of_insn, 8, 8));
     }
 
-    /* Emit insn_start now that we know the ILEN.  */
+    s->ilen = ilen;
     tcg_gen_insn_start(s->base.pc_next, s->cc_op, ilen);
 }
--8<---------------cut here---------------end--------------->8---
 

-- 
Alex Bennée