diff mbox

[6/7] trace: Add event "guest_inst_after"

Message ID 150506131942.19604.1306593039321280342.stgit@frigg.lan (mailing list archive)
State New, archived
Headers show

Commit Message

Lluís Vilanova Sept. 10, 2017, 4:35 p.m. UTC 
Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 accel/tcg/translator.c |   23 ++++++++++++++++++-----
 trace-events           |    8 ++++++++
 2 files changed, 26 insertions(+), 5 deletions(-)

Comments

Richard Henderson Sept. 13, 2017, 6:01 p.m. UTC | #1 
On 09/10/2017 09:35 AM, Lluís Vilanova wrote:
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
>  accel/tcg/translator.c |   23 ++++++++++++++++++-----
>  trace-events           |    8 ++++++++
>  2 files changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
> index d66d601c89..c010aeee45 100644
> --- a/accel/tcg/translator.c
> +++ b/accel/tcg/translator.c
> @@ -35,7 +35,8 @@ void translator_loop_temp_check(DisasContextBase *db)
>  void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
>                       CPUState *cpu, TranslationBlock *tb)
>  {
> -    target_ulong pc_bbl;
> +    target_ulong pc_bbl, pc_insn = 0;
> +    bool translated_insn = false;
>      int max_insns;
>  
>      /* Initialize DisasContext */
> @@ -75,10 +76,15 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
>      tcg_debug_assert(db->is_jmp == DISAS_NEXT);  /* no early exit */
>  
>      while (true) {
> -        target_ulong pc_insn = db->pc_next;
>          TCGv_i32 insn_size_tcg = 0;
>          int insn_size_opcode_idx;
>  
> +        /* Tracing after (previous instruction) */
> +        if (db->num_insns > 0) {
> +            trace_guest_inst_after_tcg(cpu, tcg_ctx.tcg_env, pc_insn);
> +        }

How does this differ from "guest_inst"?  Why would you need two trace points?

Why are you placing this at the beginning of the while loop rather than the end?

> @@ -164,6 +172,9 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
>  
>          gen_set_inline_region_begin(tcg_ctx.disas.inline_label);
>  
> +        if (TRACE_GUEST_INST_AFTER_ENABLED && translated_insn) {
> +            trace_guest_inst_after_tcg(cpu, tcg_ctx.tcg_env, pc_insn);
> +        }
>          if (TRACE_GUEST_BBL_AFTER_ENABLED) {
>              trace_guest_bbl_after_tcg(cpu, tcg_ctx.tcg_env, pc_bbl);
>          }

I think I'm finally beginning to understand what you're after with your
inlining.  But I still think this should be doable in the appropriate opcode
generating functions.


r~
Lluís Vilanova Sept. 14, 2017, 4:23 p.m. UTC | #2 
Richard Henderson writes:

> On 09/10/2017 09:35 AM, Lluís Vilanova wrote:
>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>> ---
>> accel/tcg/translator.c |   23 ++++++++++++++++++-----
>> trace-events           |    8 ++++++++
>> 2 files changed, 26 insertions(+), 5 deletions(-)
>> 
>> diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
>> index d66d601c89..c010aeee45 100644
>> --- a/accel/tcg/translator.c
>> +++ b/accel/tcg/translator.c
>> @@ -35,7 +35,8 @@ void translator_loop_temp_check(DisasContextBase *db)
>> void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
>> CPUState *cpu, TranslationBlock *tb)
>> {
>> -    target_ulong pc_bbl;
>> +    target_ulong pc_bbl, pc_insn = 0;
>> +    bool translated_insn = false;
>> int max_insns;
>> 
>> /* Initialize DisasContext */
>> @@ -75,10 +76,15 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
>> tcg_debug_assert(db->is_jmp == DISAS_NEXT);  /* no early exit */
>> 
>> while (true) {
>> -        target_ulong pc_insn = db->pc_next;
>> TCGv_i32 insn_size_tcg = 0;
>> int insn_size_opcode_idx;
>> 
>> +        /* Tracing after (previous instruction) */
>> +        if (db->num_insns > 0) {
>> +            trace_guest_inst_after_tcg(cpu, tcg_ctx.tcg_env, pc_insn);
>> +        }

> How does this differ from "guest_inst"?  Why would you need two trace points?

I assume you mean how it differs from guest_inst_before. The two main ideas are:

* To be able to get a trace an execution-time event only after the instruction
  or TB have finished executing successfully (i.e., there could be an exception).
* Some values are only known *after* the instruction is translated (like the
  instruction size, or other extra information we might add in the future), so
  an efficient way to collect that is to trace guest_bbl_* and guest_insn_after
  at translation time (to build a TB "dictionary" as some call it), and trace
  guest_bbl_before at execution time (and use the detailed info above that you
  got at translation time).


> Why are you placing this at the beginning of the while loop rather than the end?

Yeah, that'll be much clearer.


>> @@ -164,6 +172,9 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
>> 
>> gen_set_inline_region_begin(tcg_ctx.disas.inline_label);
>> 
>> +        if (TRACE_GUEST_INST_AFTER_ENABLED && translated_insn) {
>> +            trace_guest_inst_after_tcg(cpu, tcg_ctx.tcg_env, pc_insn);
>> +        }
>> if (TRACE_GUEST_BBL_AFTER_ENABLED) {
>> trace_guest_bbl_after_tcg(cpu, tcg_ctx.tcg_env, pc_bbl);
>> }

> I think I'm finally beginning to understand what you're after with your
> inlining.  But I still think this should be doable in the appropriate opcode
> generating functions.

I'm not sure we can if we want to avoid having the duplicate translation-time
events I said in a previous response (since TB can have two exit points and
we're detecting them through goto_tb).


Thanks,
  Lluis
diff mbox

Patch

diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
index d66d601c89..c010aeee45 100644
--- a/accel/tcg/translator.c
+++ b/accel/tcg/translator.c
@@ -35,7 +35,8 @@  void translator_loop_temp_check(DisasContextBase *db)
 void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
                      CPUState *cpu, TranslationBlock *tb)
 {
-    target_ulong pc_bbl;
+    target_ulong pc_bbl, pc_insn = 0;
+    bool translated_insn = false;
     int max_insns;
 
     /* Initialize DisasContext */
@@ -75,10 +76,15 @@  void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
     tcg_debug_assert(db->is_jmp == DISAS_NEXT);  /* no early exit */
 
     while (true) {
-        target_ulong pc_insn = db->pc_next;
         TCGv_i32 insn_size_tcg = 0;
         int insn_size_opcode_idx;
 
+        /* Tracing after (previous instruction) */
+        if (db->num_insns > 0) {
+            trace_guest_inst_after_tcg(cpu, tcg_ctx.tcg_env, pc_insn);
+        }
+        pc_insn = db->pc_next;
+
         db->num_insns++;
         if (db->num_insns == 1) {
             tcg_ctx.disas.in_guest_code = true;
@@ -136,6 +142,7 @@  void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
             ops->translate_insn(db, cpu);
         }
 
+        translated_insn = true;
         /* Tracing after (patched values) */
         if (TRACE_GUEST_INST_INFO_BEFORE_EXEC_ENABLED) {
             unsigned int insn_size = db->pc_next - pc_insn;
@@ -156,7 +163,8 @@  void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
     }
 
     /* Tracing after */
-    if (TRACE_GUEST_BBL_AFTER_ENABLED) {
+    if (TRACE_GUEST_BBL_AFTER_ENABLED ||
+        TRACE_GUEST_INST_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();
@@ -164,6 +172,9 @@  void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
 
         gen_set_inline_region_begin(tcg_ctx.disas.inline_label);
 
+        if (TRACE_GUEST_INST_AFTER_ENABLED && translated_insn) {
+            trace_guest_inst_after_tcg(cpu, tcg_ctx.tcg_env, pc_insn);
+        }
         if (TRACE_GUEST_BBL_AFTER_ENABLED) {
             trace_guest_bbl_after_tcg(cpu, tcg_ctx.tcg_env, pc_bbl);
         }
@@ -195,7 +206,8 @@  void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
 void translator__gen_goto_tb(TCGContext *ctx)
 {
     if (ctx->disas.in_guest_code &&
-        (TRACE_GUEST_BBL_AFTER_ENABLED)) {
+        (TRACE_GUEST_BBL_AFTER_ENABLED ||
+         TRACE_GUEST_INST_AFTER_ENABLED)) {
         if (ctx->disas.inline_label == NULL) {
             ctx->disas.inline_label = gen_new_inline_label();
         }
@@ -208,7 +220,8 @@  void translator__gen_goto_tb(TCGContext *ctx)
 void translator__gen_exit_tb(TCGContext *ctx)
 {
     if (ctx->disas.in_guest_code && !ctx->disas.seen_goto_tb &&
-        (TRACE_GUEST_BBL_AFTER_ENABLED)) {
+        (TRACE_GUEST_BBL_AFTER_ENABLED ||
+         TRACE_GUEST_INST_AFTER_ENABLED)) {
         if (ctx->disas.inline_label == NULL) {
             ctx->disas.inline_label = gen_new_inline_label();
         }
diff --git a/trace-events b/trace-events
index ce54bb4993..c477302d8d 100644
--- a/trace-events
+++ b/trace-events
@@ -118,6 +118,14 @@  vcpu tcg guest_bbl_after(uint64_t vaddr) "vaddr=0x%016"PRIx64, "vaddr=0x%016"PRI
 # Targets: TCG(all)
 vcpu tcg guest_inst_before(uint64_t vaddr) "vaddr=0x%016"PRIx64, "vaddr=0x%016"PRIx64
 
+# @vaddr: Instruction's virtual address
+#
+# Mark end of instruction execution (after its operations have taken effect).
+#
+# Mode: user, softmmu
+# Targets: TCG(all)
+vcpu tcg guest_inst_after(uint64_t vaddr) "vaddr=0x%016"PRIx64, "vaddr=0x%016"PRIx64
+
 # @vaddr: Instruction's virtual address
 # @size: Instruction's size in bytes
 #