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

[Edgar E. Iglesias]

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 <address@hidden>
> > ---
> > 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...

Cheers,
Edgar