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