Re: [Qemu-devel] [PATCH 4/7] tcg: Add support for "inlining" regions of code

["Lluís Vilanova" <vilanova@ac.upc.edu>]

Richard Henderson writes:

> On 09/14/2017 08:20 AM, Lluís Vilanova wrote:
>> Richard Henderson writes:
>> 
>>> On 09/10/2017 09:27 AM, Lluís Vilanova wrote:
>>>> TCG BBLs and instructions have multiple exit points from where to raise
>>>> tracing events, but some of the necessary information in the generic
>>>> disassembly infrastructure is not available until after generating these
>>>> exit points.
>>>> 
>>>> This patch adds support for "inline points" (where the tracing code will
>>>> be placed), and "inline regions" (which identify the TCG code that must
>>>> be inlined). The TCG compiler will basically copy each inline region to
>>>> any inline points that reference it.
>> 
>>> I am not keen on this.
>> 
>>> Is there a reason you can't just emit the tracing code at the appropriate place
>>> to begin with?  Perhaps I have to wait to see how this is used...
>> 
>> As I tried to briefly explain on next patch, the main problem without inlining
>> is that we will see guest_tb_after_trans twice on the trace for each TB in
>> conditional instructions on the guest, since they have two exit points (which we
>> capture when emitting goto_tb in TCG).

> Without seeing the code, I suspect this is because you didn't examine the
> argument to tcg_gen_exit_tb.  You can tell when goto_tb must have been emitted
> and avoid logging twice.

The generated tracing code for 'guest_*_after' must be right before the
"goto_tb" opcode at the end of a TB (AFAIU generated by
tcg_gen_lookup_and_goto_ptr()), and we have two of those when decoding a guest
conditional jump.

If we couple this with the semantics of the trace_*_tcg functions (trace the
event at translation time, and generate TCG code to trace the event at execution
time), we get the case I described (we don't want to call trace_tb_after_tcg()
or trace_insn_after_tcg() twice for the same TB or instruction).

That is, unless I've missed something.


The only alternative I can think of is changing tracetool to offer an additional
API that provides separate functions for translation-time tracing and
execution-time generation. So from this:

  static inline void trace_event_tcg(CPUState *cpu, TCGv_env env, ...)
  {
      trace_event_trans(cpu, ...);
      if (trace_event_get_vcpu_state(cpu, EVENT_EXEC)) {
          gen_helper_trace_event_exec(env, ...);
      }
  }

We can extend it into this:

  static inline void gen_trace_event_exec(TCGv_env env, ...)
      if (trace_event_get_vcpu_state(cpu, EVENT_EXEC)) {
          gen_helper_trace_event_exec(env, ...);
      }
  }
  static inline void trace_event_tcg(CPUState *cpu, TCGv_env env, ...)
  {
      trace_event_trans(cpu, ...);
      gen_trace_event_exec(env, ...);
  }


Cheers,
  Lluis