From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47230) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dwt6Y-0005iN-Ky for qemu-devel@nongnu.org; Tue, 26 Sep 2017 12:53:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dwt6V-0003PP-IG for qemu-devel@nongnu.org; Tue, 26 Sep 2017 12:53:06 -0400 Received: from mail-pg0-x22d.google.com ([2607:f8b0:400e:c05::22d]:56313) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1dwt6V-0003OH-Cg for qemu-devel@nongnu.org; Tue, 26 Sep 2017 12:53:03 -0400 Received: by mail-pg0-x22d.google.com with SMTP id b11so6221018pgn.12 for ; Tue, 26 Sep 2017 09:53:03 -0700 (PDT) Sender: Richard Henderson References: <150505986682.19604.11937392314067517230.stgit@frigg.lan> <150506083546.19604.543091497330269756.stgit@frigg.lan> <169db8d7-fe49-05c4-aca7-ad818b12c9c5@linaro.org> <87a81xpasb.fsf@frigg.lan> <7e4260a8-89a3-9c52-4f09-afe9035ceac2@twiddle.net> <87ingki0je.fsf@frigg.lan> <87poadto9u.fsf@frigg.lan> From: Richard Henderson Message-ID: Date: Tue, 26 Sep 2017 09:52:58 -0700 MIME-Version: 1.0 In-Reply-To: <87poadto9u.fsf@frigg.lan> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH 4/7] tcg: Add support for "inlining" regions of code List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Richard Henderson , qemu-devel@nongnu.org, Stefan Hajnoczi On 09/26/2017 09:31 AM, Lluís Vilanova wrote: > 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? I don't think I fully understand what you're proposing. The example transformation above is merely syntactic and has no functional change. As previously stated, I'm not keen on the "tcg inline" approach. I would prefer that you hook into tcg_gen_{exit_tb,goto_tb,goto_ptr} functions within tcg/tcg-op.c to log transitions between TBs. r~