Re: [RFC PATCH] target/s390x: move tcg_gen_insn_start to s390x_tr_insn_start

["Alex Bennée" <alex.bennee@linaro.org>]

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