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

["Alex Bennée" <alex.bennee@linaro.org>]

Edgar E. Iglesias <edgar.iglesias@xilinx.com> writes:

> On Fri, Jun 28, 2019 at 02:16:41PM +0200, Richard Henderson wrote:
>> 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.
>>
>
> Hi,
>
> One of the use cases is to be able to collect code-coverage from these traces.
> In that case we're going to need a reliable pc_end. We may need to recover it
> from outside of TCG in some cases though...

Why?

The only place you need to recover pc_end is when processing an
exception and for that case the front end should be using
cpu_loop_exit_restore() to ensure registers are valid before the
exception is processed. Otherwise you know where you've been given the
next block starts at pc_next (with -d nochain).

However I suspect if you want to build more sophisticated tools to track
execution the plugin approach might be better. This seems like a bit of
an arbitrary addition to the TCG core for a single use case where we
already have logging facilities that will give you the same information.

--
Alex Bennée