Re: [Qemu-devel] [RFC PATCH] accel/tcg/translator: add tb_enter TCG trace

[Richard Henderson <richard.henderson@linaro.org>]

On 6/28/19 1:39 PM, Luc Michel wrote:
> Add a TCG trace at the begining of a translation block recording the
> first and last (past-the-end) PC values.
> 
> Signed-off-by: Luc Michel <luc.michel@greensocs.com>
> ---
> This can be used to trace the execution of the guest quite efficiently.
> It will report each time a TB is entered (using the tb_enter_exec
> trace). The traces arguments give the PC start and past-the-end values.
> It has very little to no performance impact since the trace is actually
> emitted in the generated code only when it is enabled at run time.
> 
> It works already quite well on its own to trace guest execution. However
> it does not handle the case where a TB is exited in the middle of
> execution. I'm not sure how to properly trace that. A trace could be
> added when `cpu_loop_exit()' is called to report the current PC, but in
> most cases the interesting value (the PC of the instruction that
> caused the exit) is already lost at this stage.
> 
> I'm not sure there is a generic (i.e. not target specific) way of
> recovering the last PC executed when cpu_loop_exit() is called. Do you
> think of a better way?
> 
> Thanks to the Xilinx's QEMU team who sponsored this work.
> ---
>  accel/tcg/translator.c | 24 ++++++++++++++++++++++++
>  trace-events           |  3 +++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
> index 9226a348a3..c55377aa18 100644
> --- a/accel/tcg/translator.c
> +++ b/accel/tcg/translator.c
> @@ -14,10 +14,11 @@
>  #include "tcg/tcg-op.h"
>  #include "exec/exec-all.h"
>  #include "exec/gen-icount.h"
>  #include "exec/log.h"
>  #include "exec/translator.h"
> +#include "trace-tcg.h"
>  
>  /* Pairs with tcg_clear_temp_count.
>     To be called by #TranslatorOps.{translate_insn,tb_stop} if
>     (1) the target is sufficiently clean to support reporting,
>     (2) as and when all temporaries are known to be consumed.
> @@ -28,14 +29,31 @@ void translator_loop_temp_check(DisasContextBase *db)
>          qemu_log("warning: TCG temporary leaks before "
>                   TARGET_FMT_lx "\n", db->pc_next);
>      }
>  }
>  
> +static TCGOp *gen_trace_tb_enter(TranslationBlock *tb)
> +{
> +    TCGOp *last_pc_op;
> +
> +    TCGv pc_end = tcg_temp_new();
> +
> +    /* The last PC value is not known yet */
> +    tcg_gen_movi_tl(pc_end, 0xdeadbeef);
> +    last_pc_op = tcg_last_op();

TL is a target-specific type that does not necessarily correspond to uint64_t,
as you assume in the print message.  More importantly, on a 32-bit host with a
64-bit guest, this movi will generate *two* ops...

> +    /* Fixup the last PC value in the tb_enter trace now that we know it */
> +    tcg_set_insn_param(trace_pc_end, 1, db->pc_next);

... which means that this operation does not do what you think it does.  It
will only set one (unknown) half of the _i64 temporary.

Moreover, this isn't quite as zero overhead as I would like, because the pc_end
temporary is always created, even if the trace_tb condition is not satisfied
and so it (eventually) gets removed as unused.

I'm not quite sure what you're after with pc_end anyway.  As you note within
the cover, you can't reliably use it for anything.  If you remove that, then
you've also removed all of the other problems with this patch.


r~