Re: [Qemu-devel] [RFC 23/48] translator: add plugin_insn argument to translate_insn

[Alex Bennée]

Emilio G. Cota <address@hidden> 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