All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH] target/s390x: move tcg_gen_insn_start to s390x_tr_insn_start
@ 2021-10-11 15:57 Alex Bennée
  2021-10-11 18:34 ` Richard Henderson
  0 siblings, 1 reply; 3+ messages in thread
From: Alex Bennée @ 2021-10-11 15:57 UTC (permalink / raw)
  To: qemu-devel
  Cc: Thomas Huth, David Hildenbrand, Cornelia Huck, richard.henderson,
	open list:S390 TCG CPUs, Alex Bennée

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



^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [RFC PATCH] target/s390x: move tcg_gen_insn_start to s390x_tr_insn_start
  2021-10-11 15:57 [RFC PATCH] target/s390x: move tcg_gen_insn_start to s390x_tr_insn_start Alex Bennée
@ 2021-10-11 18:34 ` Richard Henderson
  2021-10-11 21:31   ` Alex Bennée
  0 siblings, 1 reply; 3+ messages in thread
From: Richard Henderson @ 2021-10-11 18:34 UTC (permalink / raw)
  To: Alex Bennée, qemu-devel
  Cc: Thomas Huth, Cornelia Huck, open list:S390 TCG CPUs, David Hildenbrand

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)
> 



^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [RFC PATCH] target/s390x: move tcg_gen_insn_start to s390x_tr_insn_start
  2021-10-11 18:34 ` Richard Henderson
@ 2021-10-11 21:31   ` Alex Bennée
  0 siblings, 0 replies; 3+ messages in thread
From: Alex Bennée @ 2021-10-11 21:31 UTC (permalink / raw)
  To: Richard Henderson
  Cc: open list:S390 TCG CPUs, Thomas Huth, Cornelia Huck, qemu-devel,
	David Hildenbrand


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


^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-10-11 21:35 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-11 15:57 [RFC PATCH] target/s390x: move tcg_gen_insn_start to s390x_tr_insn_start Alex Bennée
2021-10-11 18:34 ` Richard Henderson
2021-10-11 21:31   ` Alex Bennée

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.