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

["Emilio G. Cota" <cota@braap.org>]

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