[1/7] trace: Add event "guest_bbl_before"

Commit Message

Lluís Vilanova Sept. 10, 2017, 4:15 p.m. UTC

Signed-off-by: Lluís Vilanova <[e-mail]>
---
 accel/tcg/translator.c |    6 ++++++
 trace-events           |   11 +++++++++++
 2 files changed, 17 insertions(+)

Comments

Richard Henderson Sept. 13, 2017, 4:59 p.m. UTC | #1

On 09/10/2017 09:15 AM, Lluís Vilanova wrote:
> Signed-off-by: Lluís Vilanova <[e-mail]>
> ---
>  accel/tcg/translator.c |    6 ++++++
>  trace-events           |   11 +++++++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
> index afa3af478a..91b3b0da32 100644
> --- a/accel/tcg/translator.c
> +++ b/accel/tcg/translator.c
> @@ -13,6 +13,7 @@
>  #include "cpu.h"
>  #include "tcg/tcg.h"
>  #include "tcg/tcg-op.h"
> +#include "trace-tcg.h"
>  #include "exec/exec-all.h"
>  #include "exec/gen-icount.h"
>  #include "exec/log.h"
> @@ -91,6 +92,11 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
>              }
>          }
>  
> +        /* Tracing before */
> +        if (db->num_insns == 1) {
> +            trace_guest_bbl_before_tcg(cpu, tcg_ctx.tcg_env, db->pc_first);
> +        }

Why not place this before the loop, so that you don't
have to check num_insns == 1?

> +vcpu tcg guest_bbl_before(uint64_t vaddr) "vaddr=0x%016"PRIx64, "vaddr=0x%016"PRIx64

You're really going to print both ENV and PC tagged with "vaddr"?
That just seems confusing.

Also, terminology.  A "basic block" ("bb" by preference, not "bbl"), has a
specific meaning (https://en.wikipedia.org/wiki/Basic_block).  What we're
generating here is a TranslationBlock (which may consist of many basic blocks),
and oft contracted within the source as "tb".


r~

Lluís Vilanova Sept. 14, 2017, 2:21 p.m. UTC | #2

Richard Henderson writes:

> On 09/10/2017 09:15 AM, Lluís Vilanova wrote:
>> Signed-off-by: Lluís Vilanova <[e-mail]>
>> ---
>> accel/tcg/translator.c |    6 ++++++
>> trace-events           |   11 +++++++++++
>> 2 files changed, 17 insertions(+)
>> 
>> diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
>> index afa3af478a..91b3b0da32 100644
>> --- a/accel/tcg/translator.c
>> +++ b/accel/tcg/translator.c
>> @@ -13,6 +13,7 @@
>> #include "cpu.h"
>> #include "tcg/tcg.h"
>> #include "tcg/tcg-op.h"
>> +#include "trace-tcg.h"
>> #include "exec/exec-all.h"
>> #include "exec/gen-icount.h"
>> #include "exec/log.h"
>> @@ -91,6 +92,11 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
>> }
>> }
>> 
>> +        /* Tracing before */
>> +        if (db->num_insns == 1) {
>> +            trace_guest_bbl_before_tcg(cpu, tcg_ctx.tcg_env, db->pc_first);
>> +        }

> Why not place this before the loop, so that you don't
> have to check num_insns == 1?

In case the translation hits a breakpoint. I do not want the BBL/TB to appear
once before breaking, and then again after continuing.


>> +vcpu tcg guest_bbl_before(uint64_t vaddr) "vaddr=0x%016"PRIx64, "vaddr=0x%016"PRIx64

> You're really going to print both ENV and PC tagged with "vaddr"?
> That just seems confusing.

I'm not sure I follow. The ENV/CPUState is automatically added to the print
format (due to the "vcpu" property). The two format strings are necessary due to
the "tcg" property (translation-time format and execution-time format, since
TCGv arguments are not passed to the tracing function at translation time).

I'm not sure if this addresses your issue.


> Also, terminology.  A "basic block" ("bb" by preference, not "bbl"), has a
> specific meaning (https://en.wikipedia.org/wiki/Basic_block).  What we're
> generating here is a TranslationBlock (which may consist of many basic blocks),
> and oft contracted within the source as "tb".

Yes, I got carried away by the naming used by some binary analysis tools like
pin. I will change it to TB and that will probably make it clearer.


Thanks,
  Lluis

Patch

diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
index afa3af478a..91b3b0da32 100644
--- a/accel/tcg/translator.c
+++ b/accel/tcg/translator.c
@@ -13,6 +13,7 @@ 
 #include "cpu.h"
 #include "tcg/tcg.h"
 #include "tcg/tcg-op.h"
+#include "trace-tcg.h"
 #include "exec/exec-all.h"
 #include "exec/gen-icount.h"
 #include "exec/log.h"
@@ -91,6 +92,11 @@  void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
             }
         }
 
+        /* Tracing before */
+        if (db->num_insns == 1) {
+            trace_guest_bbl_before_tcg(cpu, tcg_ctx.tcg_env, db->pc_first);
+        }
+
         /* Disassemble one instruction.  The translate_insn hook should
            update db->pc_next and db->is_jmp to indicate what should be
            done next -- either exiting this loop or locate the start of
diff --git a/trace-events b/trace-events
index 1f50f56d9d..d242f54254 100644
--- a/trace-events
+++ b/trace-events
@@ -88,6 +88,17 @@  vcpu guest_cpu_exit(void)
 # Targets: all
 vcpu guest_cpu_reset(void)
 
+
+## Guest code
+
+# @vaddr: BBL's starting virtual address
+#
+# Mark start of BBL execution (before anything gets really executed).
+#
+# Mode: user, softmmu
+# Targets: TCG(all)
+vcpu tcg guest_bbl_before(uint64_t vaddr) "vaddr=0x%016"PRIx64, "vaddr=0x%016"PRIx64
+
 # @vaddr: Access' virtual address.
 # @info : Access' information (see below).
 #