diff mbox series

[RFC,23/48] translator: add plugin_insn argument to translate_insn

Message ID 20181025172057.20414-24-cota@braap.org
State New
Headers show
Series Plugin support | expand

Commit Message

Emilio Cota Oct. 25, 2018, 5:20 p.m. UTC
Signed-off-by: Emilio G. Cota <cota@braap.org>
---
 include/exec/translator.h   | 4 +++-
 accel/tcg/translator.c      | 4 ++--
 target/alpha/translate.c    | 3 ++-
 target/arm/translate-a64.c  | 3 ++-
 target/arm/translate.c      | 6 ++++--
 target/hppa/translate.c     | 3 ++-
 target/i386/translate.c     | 3 ++-
 target/m68k/translate.c     | 3 ++-
 target/mips/translate.c     | 3 ++-
 target/openrisc/translate.c | 3 ++-
 target/ppc/translate.c      | 3 ++-
 target/riscv/translate.c    | 3 ++-
 target/s390x/translate.c    | 3 ++-
 target/sh4/translate.c      | 3 ++-
 target/sparc/translate.c    | 3 ++-
 target/xtensa/translate.c   | 3 ++-
 16 files changed, 35 insertions(+), 18 deletions(-)

Comments

Alex Bennée Nov. 26, 2018, 2:52 p.m. UTC | #1
Emilio G. Cota <cota@braap.org> writes:

> Signed-off-by: Emilio G. Cota <cota@braap.org>
> ---
>  include/exec/translator.h   | 4 +++-
>  accel/tcg/translator.c      | 4 ++--
>  target/alpha/translate.c    | 3 ++-
>  target/arm/translate-a64.c  | 3 ++-
>  target/arm/translate.c      | 6 ++++--
>  target/hppa/translate.c     | 3 ++-
>  target/i386/translate.c     | 3 ++-
>  target/m68k/translate.c     | 3 ++-
>  target/mips/translate.c     | 3 ++-
>  target/openrisc/translate.c | 3 ++-
>  target/ppc/translate.c      | 3 ++-
>  target/riscv/translate.c    | 3 ++-
>  target/s390x/translate.c    | 3 ++-
>  target/sh4/translate.c      | 3 ++-
>  target/sparc/translate.c    | 3 ++-
>  target/xtensa/translate.c   | 3 ++-
>  16 files changed, 35 insertions(+), 18 deletions(-)
>
> diff --git a/include/exec/translator.h b/include/exec/translator.h
> index 71e7b2c347..a28147b3dd 100644
> --- a/include/exec/translator.h
> +++ b/include/exec/translator.h
> @@ -20,6 +20,7 @@
>
>
>  #include "exec/exec-all.h"
> +#include "qemu/plugin.h"
>  #include "tcg/tcg.h"
>
>
> @@ -112,7 +113,8 @@ typedef struct TranslatorOps {
>      void (*insn_start)(DisasContextBase *db, CPUState *cpu);
>      bool (*breakpoint_check)(DisasContextBase *db, CPUState *cpu,
>                               const CPUBreakpoint *bp);
> -    void (*translate_insn)(DisasContextBase *db, CPUState *cpu);
> +    void (*translate_insn)(DisasContextBase *db, CPUState *cpu,
> +                           struct qemu_plugin_insn *plugin_insn);

I'm not convinced this is the best way to go about it. We end up having
to sprinkle the plugin calls into each decoder rather than keeping all
the infrastructure in the common main loop. However the common loop will
need to know the total number of bytes decoded so we could change the
declaration to:

  int (*translate_insn)(DisasContextBase *db, CPUState *cpu);

and return the number of bytes decoded. It would mean a minor
inefficiency in having to re-read the instruction bytes into a buffer in
preparation for passing to the plugin but it would all at least be in
one place.

--
Alex Bennée
Richard Henderson Nov. 26, 2018, 6:27 p.m. UTC | #2
On 11/26/18 6:52 AM, Alex Bennée wrote:
> I'm not convinced this is the best way to go about it. We end up having
> to sprinkle the plugin calls into each decoder rather than keeping all
> the infrastructure in the common main loop. However the common loop will
> need to know the total number of bytes decoded so we could change the
> declaration to:
> 
>   int (*translate_insn)(DisasContextBase *db, CPUState *cpu);
> 
> and return the number of bytes decoded. 

Returning the number of bytes is more difficult than simply just

    old_pc = db->pc_next;
    opc->translate_insn(db, cpu);
    bytes = db->pc_next - old_pc;

requiring no target changes at all.


r~
Alex Bennée Nov. 26, 2018, 6:56 p.m. UTC | #3
On Mon, 26 Nov 2018, 18:27 Richard Henderson <richard.henderson@linaro.org
wrote:

> On 11/26/18 6:52 AM, Alex Bennée wrote:
> > I'm not convinced this is the best way to go about it. We end up having
> > to sprinkle the plugin calls into each decoder rather than keeping all
> > the infrastructure in the common main loop. However the common loop will
> > need to know the total number of bytes decoded so we could change the
> > declaration to:
> >
> >   int (*translate_insn)(DisasContextBase *db, CPUState *cpu);
> >
> > and return the number of bytes decoded.
>
> Returning the number of bytes is more difficult than simply just
>
>     old_pc = db->pc_next;
>     opc->translate_insn(db, cpu);
>     bytes = db->pc_next - old_pc;
>
> requiring no target changes at all.
>

If that's always true then great, but what happens with direct branches?

>
> r~
>
Emilio Cota Nov. 26, 2018, 7:07 p.m. UTC | #4
On Mon, Nov 26, 2018 at 10:27:12 -0800, Richard Henderson wrote:
> On 11/26/18 6:52 AM, Alex Bennée wrote:
> > I'm not convinced this is the best way to go about it. We end up having
> > to sprinkle the plugin calls into each decoder rather than keeping all
> > the infrastructure in the common main loop. However the common loop will
> > need to know the total number of bytes decoded so we could change the
> > declaration to:
> > 
> >   int (*translate_insn)(DisasContextBase *db, CPUState *cpu);
> > 
> > and return the number of bytes decoded. 
> 
> Returning the number of bytes is more difficult than simply just
> 
>     old_pc = db->pc_next;
>     opc->translate_insn(db, cpu);
>     bytes = db->pc_next - old_pc;
> 
> requiring no target changes at all.

The main reason why I added the qemu_plugin_insn_append calls
was to avoid reading the instructions twice from guest memory,
because I was worried that doing so might somehow alter the
guest's execution, e.g. what if we read a cross-page instruction,
and both pages mapped to the same TLB entry? We'd end up having
more TLB misses because instrumentation was enabled.

If you think that's not really a concern, we could just re-do
the reads in the translator loop and get the size as above.

Thanks,

		Emilio
Richard Henderson Nov. 26, 2018, 7:19 p.m. UTC | #5
On 11/26/18 10:56 AM, Alex Bennée wrote:
> 
> 
> On Mon, 26 Nov 2018, 18:27 Richard Henderson <richard.henderson@linaro.org
> <mailto:richard.henderson@linaro.org> wrote:
> 
>     On 11/26/18 6:52 AM, Alex Bennée wrote:
>     > I'm not convinced this is the best way to go about it. We end up having
>     > to sprinkle the plugin calls into each decoder rather than keeping all
>     > the infrastructure in the common main loop. However the common loop will
>     > need to know the total number of bytes decoded so we could change the
>     > declaration to:
>     >
>     >   int (*translate_insn)(DisasContextBase *db, CPUState *cpu);
>     >
>     > and return the number of bytes decoded.
> 
>     Returning the number of bytes is more difficult than simply just
> 
>         old_pc = db->pc_next;
>         opc->translate_insn(db, cpu);
>         bytes = db->pc_next - old_pc;
> 
>     requiring no target changes at all.
> 
> 
> If that's always true then great, but what happens with direct branches?

pc_next is still updated by the size of the branch, not it's destination;
db->is_jmp will be != DISAS_NEXT, ending the TB.


r~
Richard Henderson Nov. 26, 2018, 7:30 p.m. UTC | #6
On 11/26/18 11:07 AM, Emilio G. Cota wrote:
> The main reason why I added the qemu_plugin_insn_append calls
> was to avoid reading the instructions twice from guest memory,
> because I was worried that doing so might somehow alter the
> guest's execution, e.g. what if we read a cross-page instruction,
> and both pages mapped to the same TLB entry? We'd end up having
> more TLB misses because instrumentation was enabled.

A better solution for this, I think is to change direct calls from

  cpu_ldl_code(env, pc);
to
  translator_ldl_code(dc_base, env, pc);

instead of passing around a new argument separate from DisasContextBase?


r~
Emilio Cota Nov. 27, 2018, 1:38 a.m. UTC | #7
On Mon, Nov 26, 2018 at 11:30:25 -0800, Richard Henderson wrote:
> On 11/26/18 11:07 AM, Emilio G. Cota wrote:
> > The main reason why I added the qemu_plugin_insn_append calls
> > was to avoid reading the instructions twice from guest memory,
> > because I was worried that doing so might somehow alter the
> > guest's execution, e.g. what if we read a cross-page instruction,
> > and both pages mapped to the same TLB entry? We'd end up having
> > more TLB misses because instrumentation was enabled.
> 
> A better solution for this, I think is to change direct calls from
> 
>   cpu_ldl_code(env, pc);
> to
>   translator_ldl_code(dc_base, env, pc);
> 
> instead of passing around a new argument separate from DisasContextBase?

I think this + diff'ing pc_next should work to figure out the
contents and size of each instruction.

I'll do it this way in v2.

Thanks,

		Emilio
Pavel Dovgalyuk Nov. 27, 2018, 1:08 p.m. UTC | #8
> From: Emilio G. Cota [mailto:cota@braap.org]
> On Mon, Nov 26, 2018 at 10:27:12 -0800, Richard Henderson wrote:
> > On 11/26/18 6:52 AM, Alex Bennée wrote:
> > > I'm not convinced this is the best way to go about it. We end up having
> > > to sprinkle the plugin calls into each decoder rather than keeping all
> > > the infrastructure in the common main loop. However the common loop will
> > > need to know the total number of bytes decoded so we could change the
> > > declaration to:
> > >
> > >   int (*translate_insn)(DisasContextBase *db, CPUState *cpu);
> > >
> > > and return the number of bytes decoded.
> >
> > Returning the number of bytes is more difficult than simply just
> >
> >     old_pc = db->pc_next;
> >     opc->translate_insn(db, cpu);
> >     bytes = db->pc_next - old_pc;
> >
> > requiring no target changes at all.
> 
> The main reason why I added the qemu_plugin_insn_append calls
> was to avoid reading the instructions twice from guest memory,
> because I was worried that doing so might somehow alter the
> guest's execution, e.g. what if we read a cross-page instruction,
> and both pages mapped to the same TLB entry? We'd end up having
> more TLB misses because instrumentation was enabled.

In our plugins we use cpu_debug_rw function.
But I think that your example with mapping of the page and simultaneous
unmapping of the previous is impossible, because both pages should
be available to the translator for creating the TB.
The translation immediately interrupted with TLB miss and repeated
again after mapping. It means that the cross-page instruction
should not be unmapped until it completely executes.

Pavel Dovgalyuk
Emilio Cota Nov. 28, 2018, 12:54 a.m. UTC | #9
On Mon, Nov 26, 2018 at 20:38:25 -0500, Emilio G. Cota wrote:
> On Mon, Nov 26, 2018 at 11:30:25 -0800, Richard Henderson wrote:
> > On 11/26/18 11:07 AM, Emilio G. Cota wrote:
> > > The main reason why I added the qemu_plugin_insn_append calls
> > > was to avoid reading the instructions twice from guest memory,
> > > because I was worried that doing so might somehow alter the
> > > guest's execution, e.g. what if we read a cross-page instruction,
> > > and both pages mapped to the same TLB entry? We'd end up having
> > > more TLB misses because instrumentation was enabled.
> > 
> > A better solution for this, I think is to change direct calls from
> > 
> >   cpu_ldl_code(env, pc);
> > to
> >   translator_ldl_code(dc_base, env, pc);
> > 
> > instead of passing around a new argument separate from DisasContextBase?
> 
> I think this + diff'ing pc_next should work to figure out the
> contents and size of each instruction.

I just tried doing things this way.

For some targets like i386, the translator_ld* helpers work
great; the instruction contents are copied, and through
the helpers we get the sizes of the instructions as well.

For ARM though (and maybe others, I haven't gone
through all of them yet), arm_ldl_code does the following:

/* Load an instruction and return it in the standard little-endian order */
static inline uint32_t arm_ldl_code(CPUARMState *env, target_ulong addr,
                                    bool sctlr_b)
{
    uint32_t insn = cpu_ldl_code(env, addr);
    if (bswap_code(sctlr_b)) {
        return bswap32(insn);
    }
    return insn;
}

To avoid altering the signature of .translate_insn, I've modified
arm_ldl_code directly, as follows:

     uint32_t insn = cpu_ldl_code(env, addr);
+
     if (bswap_code(sctlr_b)) {
-        return bswap32(insn);
+        insn = bswap32(insn);
+    }
+    if (tcg_ctx->plugin_insn) {
+        qemu_plugin_insn_append(tcg_ctx->plugin_insn, &insn, sizeof(insn));
     }
     return insn;
 }

(NB. tcg_ctx->plugin_insn is updated by translator_loop
on every iteration.)

Let me know if you think I should do this differently.

Thanks,

		Emilio
Emilio Cota Nov. 28, 2018, 1:12 a.m. UTC | #10
On Tue, Nov 27, 2018 at 19:54:02 -0500, Emilio G. Cota wrote:
> To avoid altering the signature of .translate_insn, I've modified
> arm_ldl_code directly, as follows:
> 
>      uint32_t insn = cpu_ldl_code(env, addr);
> +
>      if (bswap_code(sctlr_b)) {
> -        return bswap32(insn);
> +        insn = bswap32(insn);
> +    }
> +    if (tcg_ctx->plugin_insn) {
> +        qemu_plugin_insn_append(tcg_ctx->plugin_insn, &insn, sizeof(insn));
>      }
>      return insn;
>  }

Turns out it got even more complicated with thumb, since instructions
can be 16 or 32 bits.

I ended up with the appended (qemu_plugin_insn_append() returns
when the first argument is NULL).

		Emilio

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 88195ab949..e6caaff976 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -38,6 +38,7 @@
 #include "trace-tcg.h"
 #include "translate-a64.h"
 #include "qemu/atomic128.h"
+#include "qemu/plugin.h"
 
 static TCGv_i64 cpu_X[32];
 static TCGv_i64 cpu_pc;
@@ -13321,6 +13322,7 @@ static void disas_a64_insn(CPUARMState *env, DisasContext *s)
     uint32_t insn;
 
     insn = arm_ldl_code(env, s->pc, s->sctlr_b);
+    qemu_plugin_insn_append(tcg_ctx->plugin_insn, &insn, sizeof(insn));
     s->insn = insn;
     s->pc += 4;
 
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 7c4675ffd8..7523257b85 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -36,6 +36,7 @@
 
 #include "trace-tcg.h"
 #include "exec/log.h"
+#include "qemu/plugin.h"
 
 
 #define ENABLE_ARCH_4T    arm_dc_feature(s, ARM_FEATURE_V4T)
@@ -13234,6 +13235,7 @@ static void arm_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
     }
 
     insn = arm_ldl_code(env, dc->pc, dc->sctlr_b);
+    qemu_plugin_insn_append(tcg_ctx->plugin_insn, &insn, sizeof(insn));
     dc->insn = insn;
     dc->pc += 4;
     disas_arm_insn(dc, insn);
@@ -13304,11 +13306,16 @@ static void thumb_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
     insn = arm_lduw_code(env, dc->pc, dc->sctlr_b);
     is_16bit = thumb_insn_is_16bit(dc, insn);
     dc->pc += 2;
-    if (!is_16bit) {
+    if (is_16bit) {
+        uint16_t insn16 = insn;
+
+        qemu_plugin_insn_append(tcg_ctx->plugin_insn, &insn16, sizeof(insn16));
+    } else {
         uint32_t insn2 = arm_lduw_code(env, dc->pc, dc->sctlr_b);
 
         insn = insn << 16 | insn2;
         dc->pc += 2;
+        qemu_plugin_insn_append(tcg_ctx->plugin_insn, &insn, sizeof(insn));
     }
     dc->insn = insn;
Alex Bennée Nov. 28, 2018, 12:40 p.m. UTC | #11
Emilio G. Cota <cota@braap.org> writes:

> On Mon, Nov 26, 2018 at 20:38:25 -0500, Emilio G. Cota wrote:
>> On Mon, Nov 26, 2018 at 11:30:25 -0800, Richard Henderson wrote:
>> > On 11/26/18 11:07 AM, Emilio G. Cota wrote:
>> > > The main reason why I added the qemu_plugin_insn_append calls
>> > > was to avoid reading the instructions twice from guest memory,
>> > > because I was worried that doing so might somehow alter the
>> > > guest's execution, e.g. what if we read a cross-page instruction,
>> > > and both pages mapped to the same TLB entry? We'd end up having
>> > > more TLB misses because instrumentation was enabled.
>> >
>> > A better solution for this, I think is to change direct calls from
>> >
>> >   cpu_ldl_code(env, pc);
>> > to
>> >   translator_ldl_code(dc_base, env, pc);
>> >
>> > instead of passing around a new argument separate from DisasContextBase?
>>
>> I think this + diff'ing pc_next should work to figure out the
>> contents and size of each instruction.
>
> I just tried doing things this way.
>
> For some targets like i386, the translator_ld* helpers work
> great; the instruction contents are copied, and through
> the helpers we get the sizes of the instructions as well.
>
> For ARM though (and maybe others, I haven't gone
> through all of them yet), arm_ldl_code does the following:
>
> /* Load an instruction and return it in the standard little-endian order */
> static inline uint32_t arm_ldl_code(CPUARMState *env, target_ulong addr,
>                                     bool sctlr_b)
> {
>     uint32_t insn = cpu_ldl_code(env, addr);
>     if (bswap_code(sctlr_b)) {
>         return bswap32(insn);
>     }
>     return insn;
> }
>
> To avoid altering the signature of .translate_insn, I've modified
> arm_ldl_code directly, as follows:
>
>      uint32_t insn = cpu_ldl_code(env, addr);
> +
>      if (bswap_code(sctlr_b)) {
> -        return bswap32(insn);
> +        insn = bswap32(insn);
> +    }
> +    if (tcg_ctx->plugin_insn) {
> +        qemu_plugin_insn_append(tcg_ctx->plugin_insn, &insn, sizeof(insn));
>      }
>      return insn;
>  }
>
> (NB. tcg_ctx->plugin_insn is updated by translator_loop
> on every iteration.)
>
> Let me know if you think I should do this differently.

I was envisioning something more like the following so all the plugin
gubins could be kept in the core code:

accel/tcg/translator.c    | 25 +++++++++++++++++++++++++
include/exec/translator.h | 10 ++++++++++
target/arm/arm_ldst.h     | 16 +++-------------

modified   accel/tcg/translator.c
@@ -10,6 +10,7 @@
 #include "qemu/osdep.h"
 #include "qemu-common.h"
 #include "qemu/error-report.h"
+#include "qemu/bswap.h"
 #include "cpu.h"
 #include "tcg/tcg.h"
 #include "tcg/tcg-op.h"
@@ -18,6 +19,30 @@
 #include "exec/log.h"
 #include "exec/translator.h"
 #include "exec/plugin-gen.h"
+#include "exec/cpu_ldst.h"
+
+/*
+ * Translator Code Load Helpers
+ */
+
+uint16_t translator_ld16(CPUArchState *env, target_ulong ptr, bool bswap)
+{
+    uint16_t insn = cpu_lduw_code(env, ptr);
+    if (bswap) {
+        insn = bswap16(insn);
+    }
+    return insn;
+}
+
+uint32_t translator_ld32(CPUArchState *env, target_ulong ptr, bool bswap)
+{
+    uint32_t insn = cpu_ldl_code(env, ptr);
+    if (bswap) {
+        insn = bswap32(insn);
+    }
+    return insn;
+}
+

 /* Pairs with tcg_clear_temp_count.
    To be called by #TranslatorOps.{translate_insn,tb_stop} if
modified   include/exec/translator.h
@@ -147,4 +147,14 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db,

 void translator_loop_temp_check(DisasContextBase *db);

+/*
+ * Translator Code Load Helpers
+ *
+ * These allow the core translator code to track where code is being
+ * loaded from.
+ */
+
+uint16_t translator_ld16(CPUArchState *env, target_ulong ptr, bool bswap);
+uint32_t translator_ld32(CPUArchState *env, target_ulong ptr, bool bswap);
+
 #endif  /* EXEC__TRANSLATOR_H */
modified   target/arm/arm_ldst.h
@@ -20,25 +20,19 @@
 #ifndef ARM_LDST_H
 #define ARM_LDST_H

-#include "exec/cpu_ldst.h"
-#include "qemu/bswap.h"
+#include "exec/translator.h"

 /* Load an instruction and return it in the standard little-endian order */
 static inline uint32_t arm_ldl_code(CPUARMState *env, target_ulong addr,
                                     bool sctlr_b)
 {
-    uint32_t insn = cpu_ldl_code(env, addr);
-    if (bswap_code(sctlr_b)) {
-        return bswap32(insn);
-    }
-    return insn;
+    return translator_ld32(env, addr, bswap_code(sctlr_b));
 }

 /* Ditto, for a halfword (Thumb) instruction */
 static inline uint16_t arm_lduw_code(CPUARMState *env, target_ulong addr,
                                      bool sctlr_b)
 {
-    uint16_t insn;
 #ifndef CONFIG_USER_ONLY
     /* In big-endian (BE32) mode, adjacent Thumb instructions have been swapped
        within each word.  Undo that now.  */
@@ -46,11 +40,7 @@ static inline uint16_t arm_lduw_code(CPUARMState *env, target_ulong addr,
         addr ^= 2;
     }
 #endif
-    insn = cpu_lduw_code(env, addr);
-    if (bswap_code(sctlr_b)) {
-        return bswap16(insn);
-    }
-    return insn;
+    return translator_ld16(env, addr, bswap_code(sctlr_b));
 }

 #endif
Emilio Cota Nov. 28, 2018, 2:43 p.m. UTC | #12
On Wed, Nov 28, 2018 at 12:40:23 +0000, Alex Bennée wrote:
> I was envisioning something more like the following so all the plugin
> gubins could be kept in the core code:
(snip)
>  static inline uint32_t arm_ldl_code(CPUARMState *env, target_ulong addr,
>                                      bool sctlr_b)
>  {
> -    uint32_t insn = cpu_ldl_code(env, addr);
> -    if (bswap_code(sctlr_b)) {
> -        return bswap32(insn);
> -    }
> -    return insn;
> +    return translator_ld32(env, addr, bswap_code(sctlr_b));
>  }
> 
>  /* Ditto, for a halfword (Thumb) instruction */
>  static inline uint16_t arm_lduw_code(CPUARMState *env, target_ulong addr,
>                                       bool sctlr_b)
>  {
> -    uint16_t insn;
>  #ifndef CONFIG_USER_ONLY
>      /* In big-endian (BE32) mode, adjacent Thumb instructions have been swapped
>         within each word.  Undo that now.  */
> @@ -46,11 +40,7 @@ static inline uint16_t arm_lduw_code(CPUARMState *env, target_ulong addr,
>          addr ^= 2;
>      }
>  #endif
> -    insn = cpu_lduw_code(env, addr);
> -    if (bswap_code(sctlr_b)) {
> -        return bswap16(insn);
> -    }
> -    return insn;
> +    return translator_ld16(env, addr, bswap_code(sctlr_b));
>  }

I like this, thanks.

However, for Thumb I think we still need to call qemu_plugin_insn_append
directly:

@@ -13304,11 +13306,16 @@ static void thumb_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
     insn = arm_lduw_code(env, dc->pc, dc->sctlr_b);
     is_16bit = thumb_insn_is_16bit(dc, insn);
     dc->pc += 2;
-    if (!is_16bit) {
+    if (is_16bit) {
+        uint16_t insn16 = insn;
+
+        qemu_plugin_insn_append(tcg_ctx->plugin_insn, &insn16, sizeof(insn16));
+    } else {
         uint32_t insn2 = arm_lduw_code(env, dc->pc, dc->sctlr_b);

         insn = insn << 16 | insn2;
         dc->pc += 2;
+        qemu_plugin_insn_append(tcg_ctx->plugin_insn, &insn, sizeof(insn));
     }
 
Otherwise we might mess up the contents of 32-bit insns.

Thanks,

		E.
diff mbox series

Patch

diff --git a/include/exec/translator.h b/include/exec/translator.h
index 71e7b2c347..a28147b3dd 100644
--- a/include/exec/translator.h
+++ b/include/exec/translator.h
@@ -20,6 +20,7 @@ 
 
 
 #include "exec/exec-all.h"
+#include "qemu/plugin.h"
 #include "tcg/tcg.h"
 
 
@@ -112,7 +113,8 @@  typedef struct TranslatorOps {
     void (*insn_start)(DisasContextBase *db, CPUState *cpu);
     bool (*breakpoint_check)(DisasContextBase *db, CPUState *cpu,
                              const CPUBreakpoint *bp);
-    void (*translate_insn)(DisasContextBase *db, CPUState *cpu);
+    void (*translate_insn)(DisasContextBase *db, CPUState *cpu,
+                           struct qemu_plugin_insn *plugin_insn);
     void (*tb_stop)(DisasContextBase *db, CPUState *cpu);
     void (*disas_log)(const DisasContextBase *db, CPUState *cpu);
 } TranslatorOps;
diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
index afd0a49ea6..8591e4b72a 100644
--- a/accel/tcg/translator.c
+++ b/accel/tcg/translator.c
@@ -101,10 +101,10 @@  void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
             && (tb_cflags(db->tb) & CF_LAST_IO)) {
             /* Accept I/O on the last instruction.  */
             gen_io_start();
-            ops->translate_insn(db, cpu);
+            ops->translate_insn(db, cpu, NULL);
             gen_io_end();
         } else {
-            ops->translate_insn(db, cpu);
+            ops->translate_insn(db, cpu, NULL);
         }
 
         /* Stop translation if translate_insn so indicated.  */
diff --git a/target/alpha/translate.c b/target/alpha/translate.c
index 25cd95931d..72a302e102 100644
--- a/target/alpha/translate.c
+++ b/target/alpha/translate.c
@@ -2983,7 +2983,8 @@  static bool alpha_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
     return true;
 }
 
-static void alpha_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
+static void alpha_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu,
+                                    struct qemu_plugin_insn *plugin_insn)
 {
     DisasContext *ctx = container_of(dcbase, DisasContext, base);
     CPUAlphaState *env = cpu->env_ptr;
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index bb9c4d8ac7..8b1e20dd59 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -13937,7 +13937,8 @@  static bool aarch64_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
     return true;
 }
 
-static void aarch64_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
+static void aarch64_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu,
+                                      struct qemu_plugin_insn *plugin_insn)
 {
     DisasContext *dc = container_of(dcbase, DisasContext, base);
     CPUARMState *env = cpu->env_ptr;
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 1b4bacb522..2fd32a2684 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -12787,7 +12787,8 @@  static void arm_post_translate_insn(DisasContext *dc)
     translator_loop_temp_check(&dc->base);
 }
 
-static void arm_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
+static void arm_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu,
+                                  struct qemu_plugin_insn *plugin_insn)
 {
     DisasContext *dc = container_of(dcbase, DisasContext, base);
     CPUARMState *env = cpu->env_ptr;
@@ -12854,7 +12855,8 @@  static bool thumb_insn_is_unconditional(DisasContext *s, uint32_t insn)
     return false;
 }
 
-static void thumb_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
+static void thumb_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu,
+                                    struct qemu_plugin_insn *plugin_insn)
 {
     DisasContext *dc = container_of(dcbase, DisasContext, base);
     CPUARMState *env = cpu->env_ptr;
diff --git a/target/hppa/translate.c b/target/hppa/translate.c
index df9179e70f..6c2a7fbc46 100644
--- a/target/hppa/translate.c
+++ b/target/hppa/translate.c
@@ -4737,7 +4737,8 @@  static bool hppa_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
     return true;
 }
 
-static void hppa_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
+static void hppa_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs,
+                                   struct qemu_plugin_insn *plugin_insn)
 {
     DisasContext *ctx = container_of(dcbase, DisasContext, base);
     CPUHPPAState *env = cs->env_ptr;
diff --git a/target/i386/translate.c b/target/i386/translate.c
index 83c1ebe491..86e59d7bf7 100644
--- a/target/i386/translate.c
+++ b/target/i386/translate.c
@@ -8525,7 +8525,8 @@  static bool i386_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
     }
 }
 
-static void i386_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
+static void i386_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu,
+                                   struct qemu_plugin_insn *plugin_insn)
 {
     DisasContext *dc = container_of(dcbase, DisasContext, base);
     target_ulong pc_next = disas_insn(dc, cpu);
diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index d55e707cf6..dd7d868b25 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -6087,7 +6087,8 @@  static bool m68k_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
     return true;
 }
 
-static void m68k_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
+static void m68k_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu,
+                                   struct qemu_plugin_insn *plugin_insn)
 {
     DisasContext *dc = container_of(dcbase, DisasContext, base);
     CPUM68KState *env = cpu->env_ptr;
diff --git a/target/mips/translate.c b/target/mips/translate.c
index 544e4dc19c..efafc6e795 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -25349,7 +25349,8 @@  static bool mips_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
     return true;
 }
 
-static void mips_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
+static void mips_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs,
+                                   struct qemu_plugin_insn *plugin_insn)
 {
     CPUMIPSState *env = cs->env_ptr;
     DisasContext *ctx = container_of(dcbase, DisasContext, base);
diff --git a/target/openrisc/translate.c b/target/openrisc/translate.c
index a271cd3903..947330e10a 100644
--- a/target/openrisc/translate.c
+++ b/target/openrisc/translate.c
@@ -1301,7 +1301,8 @@  static bool openrisc_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
     return true;
 }
 
-static void openrisc_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
+static void openrisc_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs,
+                                       struct qemu_plugin_insn *plugin_insn)
 {
     DisasContext *dc = container_of(dcbase, DisasContext, base);
     OpenRISCCPU *cpu = OPENRISC_CPU(cs);
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 2d31b5f7a1..34c3ed0a41 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -7545,7 +7545,8 @@  static bool ppc_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
     return true;
 }
 
-static void ppc_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
+static void ppc_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs,
+                                  struct qemu_plugin_insn *plugin_insn)
 {
     DisasContext *ctx = container_of(dcbase, DisasContext, base);
     CPUPPCState *env = cs->env_ptr;
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 18d7b6d147..a33cf6802b 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -1843,7 +1843,8 @@  static bool riscv_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
 }
 
 
-static void riscv_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
+static void riscv_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu,
+                                    struct qemu_plugin_insn *plugin_insn)
 {
     DisasContext *ctx = container_of(dcbase, DisasContext, base);
     CPURISCVState *env = cpu->env_ptr;
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index b5bd56b7ee..6ac1a8d821 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -6228,7 +6228,8 @@  static bool s390x_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
     return true;
 }
 
-static void s390x_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
+static void s390x_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs,
+                                    struct qemu_plugin_insn *plugin_insn)
 {
     CPUS390XState *env = cs->env_ptr;
     DisasContext *dc = container_of(dcbase, DisasContext, base);
diff --git a/target/sh4/translate.c b/target/sh4/translate.c
index ab254b0e8d..ea88d46c04 100644
--- a/target/sh4/translate.c
+++ b/target/sh4/translate.c
@@ -2313,7 +2313,8 @@  static bool sh4_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
     return true;
 }
 
-static void sh4_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
+static void sh4_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs,
+                                  struct qemu_plugin_insn *plugin_insn)
 {
     CPUSH4State *env = cs->env_ptr;
     DisasContext *ctx = container_of(dcbase, DisasContext, base);
diff --git a/target/sparc/translate.c b/target/sparc/translate.c
index 74315cdf09..2fa8b68e0a 100644
--- a/target/sparc/translate.c
+++ b/target/sparc/translate.c
@@ -5894,7 +5894,8 @@  static bool sparc_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
     return true;
 }
 
-static void sparc_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
+static void sparc_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs,
+                                    struct qemu_plugin_insn *plugin_insn)
 {
     DisasContext *dc = container_of(dcbase, DisasContext, base);
     CPUSPARCState *env = cs->env_ptr;
diff --git a/target/xtensa/translate.c b/target/xtensa/translate.c
index 46e1338448..14ab1c5ceb 100644
--- a/target/xtensa/translate.c
+++ b/target/xtensa/translate.c
@@ -1146,7 +1146,8 @@  static bool xtensa_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
     return true;
 }
 
-static void xtensa_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
+static void xtensa_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu,
+                                     struct qemu_plugin_insn *plugin_insn)
 {
     DisasContext *dc = container_of(dcbase, DisasContext, base);
     CPUXtensaState *env = cpu->env_ptr;