[Qemu-devel] [PATCH 5/7] trace: Add event "guest_bbl_after"

[Lluís Vilanova]

Need to use "TCG inlining" to avoid showing a trace entry for each exit
point (up to two per BBL).

Signed-off-by: Lluís Vilanova 
---
 accel/tcg/translator.c|   54 +
 include/exec/translator.h |   22 ++
 tcg/tcg-op.c  |2 ++
 tcg/tcg-op.h  |1 +
 tcg/tcg.h |5 
 trace-events  |   11 +
 6 files changed, 94 insertions(+), 1 deletion(-)

diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
index 6598931171..d66d601c89 100644
--- a/accel/tcg/translator.c
+++ b/accel/tcg/translator.c
@@ -35,6 +35,7 @@ void translator_loop_temp_check(DisasContextBase *db)
 void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
  CPUState *cpu, TranslationBlock *tb)
 {
+target_ulong pc_bbl;
 int max_insns;
 
 /* Initialize DisasContext */
@@ -63,6 +64,11 @@ void translator_loop(const TranslatorOps *ops, 
DisasContextBase *db,
 /* Reset the temp count so that we can identify leaks */
 tcg_clear_temp_count();
 
+/* Tracking gen_goto_tb / gen_exit_tb */
+pc_bbl = db->pc_first;
+tcg_ctx.disas.seen_goto_tb = false;
+tcg_ctx.disas.in_guest_code = false;
+
 /* Start translating.  */
 gen_tb_start(db->tb);
 ops->tb_start(db, cpu);
@@ -74,6 +80,11 @@ void translator_loop(const TranslatorOps *ops, 
DisasContextBase *db,
 int insn_size_opcode_idx;
 
 db->num_insns++;
+if (db->num_insns == 1) {
+tcg_ctx.disas.in_guest_code = true;
+tcg_ctx.disas.inline_label = NULL;
+}
+
 ops->insn_start(db, cpu);
 tcg_debug_assert(db->is_jmp == DISAS_NEXT);  /* no early exit */
 
@@ -144,6 +155,22 @@ void translator_loop(const TranslatorOps *ops, 
DisasContextBase *db,
 }
 }
 
+/* Tracing after */
+if (TRACE_GUEST_BBL_AFTER_ENABLED) {
+tcg_ctx.disas.in_guest_code = false;
+if (tcg_ctx.disas.inline_label == NULL) {
+tcg_ctx.disas.inline_label = gen_new_inline_label();
+}
+
+gen_set_inline_region_begin(tcg_ctx.disas.inline_label);
+
+if (TRACE_GUEST_BBL_AFTER_ENABLED) {
+trace_guest_bbl_after_tcg(cpu, tcg_ctx.tcg_env, pc_bbl);
+}
+
+gen_set_inline_region_end(tcg_ctx.disas.inline_label);
+}
+
 /* Emit code to exit the TB, as indicated by db->is_jmp.  */
 ops->tb_stop(db, cpu);
 gen_tb_end(db->tb, db->num_insns);
@@ -163,3 +190,30 @@ void translator_loop(const TranslatorOps *ops, 
DisasContextBase *db,
 }
 #endif
 }
+
+
+void translator__gen_goto_tb(TCGContext *ctx)
+{
+if (ctx->disas.in_guest_code &&
+(TRACE_GUEST_BBL_AFTER_ENABLED)) {
+if (ctx->disas.inline_label == NULL) {
+ctx->disas.inline_label = gen_new_inline_label();
+}
+gen_set_inline_point(ctx->disas.inline_label);
+/* disable next exit_tb */
+ctx->disas.seen_goto_tb = true;
+}
+}
+
+void translator__gen_exit_tb(TCGContext *ctx)
+{
+if (ctx->disas.in_guest_code && !ctx->disas.seen_goto_tb &&
+(TRACE_GUEST_BBL_AFTER_ENABLED)) {
+if (ctx->disas.inline_label == NULL) {
+ctx->disas.inline_label = gen_new_inline_label();
+}
+gen_set_inline_point(ctx->disas.inline_label);
+/* enable next exit_tb */
+ctx->disas.seen_goto_tb = false;
+}
+}
diff --git a/include/exec/translator.h b/include/exec/translator.h
index e2dc2a04ae..83aeea59a1 100644
--- a/include/exec/translator.h
+++ b/include/exec/translator.h
@@ -20,7 +20,6 @@
 
 
 #include "exec/exec-all.h"
-#include "tcg/tcg.h"
 
 
 /**
@@ -71,6 +70,21 @@ typedef struct DisasContextBase {
 bool singlestep_enabled;
 } DisasContextBase;
 
+/**
+ * TCGContextDisas:
+ * @seen_goto_tb: Whether we've seen a call to tcg_gen_goto_tb().
+ * @in_guest_code: Whether we're generating guest code (or supporting
+ * boilerplate otherwise).
+ * @inline_label: Inline label.
+ *
+ * Extensions to #TCGContext specific to the generic translation framework.
+ */
+typedef struct TCGContextDisas {
+bool seen_goto_tb;
+bool in_guest_code;
+TCGInlineLabel *inline_label;
+} TCGContextDisas;
+
 /**
  * TranslatorOps:
  * @init_disas_context:
@@ -117,6 +131,8 @@ typedef struct TranslatorOps {
 void (*disas_log)(const DisasContextBase *db, CPUState *cpu);
 } TranslatorOps;
 
+#include "tcg/tcg.h"
+
 /**
  * translator_loop:
  * @ops: Target-specific operations.
@@ -141,4 +157,8 @@ void translator_loop(const TranslatorOps *ops, 
DisasContextBase *db,
 
 void translator_loop_temp_check(DisasContextBase *db);
 
+/* Internal functions to hook tracing into */
+void translator__gen_goto_tb(TCGContext *ctx);
+void translator__gen_exit_tb(TCGContext *ctx);
+
 #endif  /* EXEC__TRANSLATOR_H */
diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
index 688d91755b..575b4faf84 100644
--- a/tcg/tcg-op.c
+++ b/tcg/tcg-op.c
@@ -2578,6 +2

Re: [Qemu-devel] [PATCH 5/7] trace: Add event "guest_bbl_after"

[Lluís Vilanova]

Richard Henderson writes:

> On 09/10/2017 09:31 AM, Lluís Vilanova wrote:
>> +void translator__gen_goto_tb(TCGContext *ctx)
>> +{
>> +if (ctx->disas.in_guest_code &&
>> +(TRACE_GUEST_BBL_AFTER_ENABLED)) {
>> +if (ctx->disas.inline_label == NULL) {
>> +ctx->disas.inline_label = gen_new_inline_label();
>> +}
>> +gen_set_inline_point(ctx->disas.inline_label);
>> +/* disable next exit_tb */
>> +ctx->disas.seen_goto_tb = true;
>> +}
>> +}
>> +
>> +void translator__gen_exit_tb(TCGContext *ctx)
>> +{
>> +if (ctx->disas.in_guest_code && !ctx->disas.seen_goto_tb &&
>> +(TRACE_GUEST_BBL_AFTER_ENABLED)) {
>> +if (ctx->disas.inline_label == NULL) {
>> +ctx->disas.inline_label = gen_new_inline_label();
>> +}
>> +gen_set_inline_point(ctx->disas.inline_label);
>> +/* enable next exit_tb */
>> +ctx->disas.seen_goto_tb = false;
>> +}
>> +}

> I don't understand why you wouldn't just modify tcg_gen_goto_tb and
> tcg_gen_exit_tb instead.

I prefer to keep all generic translation-related tracing on a single file, where
it is easier to reason about.

Cheers,
  Lluis

Re: [Qemu-devel] [PATCH 5/7] trace: Add event "guest_bbl_after"

[Richard Henderson]

On 09/14/2017 08:20 AM, Lluís Vilanova wrote:
>> I don't understand why you wouldn't just modify tcg_gen_goto_tb and
>> tcg_gen_exit_tb instead.
> 
> I prefer to keep all generic translation-related tracing on a single file, 
> where
> it is easier to reason about.

My point here was more about the inline_points.  Discussed elsewhere now.


r~

Re: [Qemu-devel] [PATCH 5/7] trace: Add event "guest_bbl_after"

[Richard Henderson]

On 09/10/2017 09:31 AM, Lluís Vilanova wrote:
> +void translator__gen_goto_tb(TCGContext *ctx)
> +{
> +if (ctx->disas.in_guest_code &&
> +(TRACE_GUEST_BBL_AFTER_ENABLED)) {
> +if (ctx->disas.inline_label == NULL) {
> +ctx->disas.inline_label = gen_new_inline_label();
> +}
> +gen_set_inline_point(ctx->disas.inline_label);
> +/* disable next exit_tb */
> +ctx->disas.seen_goto_tb = true;
> +}
> +}
> +
> +void translator__gen_exit_tb(TCGContext *ctx)
> +{
> +if (ctx->disas.in_guest_code && !ctx->disas.seen_goto_tb &&
> +(TRACE_GUEST_BBL_AFTER_ENABLED)) {
> +if (ctx->disas.inline_label == NULL) {
> +ctx->disas.inline_label = gen_new_inline_label();
> +}
> +gen_set_inline_point(ctx->disas.inline_label);
> +/* enable next exit_tb */
> +ctx->disas.seen_goto_tb = false;
> +}
> +}

I don't understand why you wouldn't just modify tcg_gen_goto_tb and
tcg_gen_exit_tb instead.


r~