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

[Lluís Vilanova]

Lluís Vilanova writes:

> 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, ...);
>   }

Richard, do you prefer to keep the "TCG inline" feature or switch the internal
tracing API to this second approach?


Thanks,
  Lluis