Re: [PATCH 1/2] perf cs-etm: Support sample flags 'insn' and 'insnlen'

Mathieu Poirier
2019-08-19T14:36:50Z

On Mon, 19 Aug 2019 at 08:23, Arnaldo Carvalho de Melo
<arnaldo.melo@gmail.com> wrote:
>
> Em Thu, Aug 15, 2019 at 04:28:54PM +0800, Leo Yan escreveu:
> > The synthetic branch and instruction samples are missed to set
> > instruction related info, thus perf tool fails to display samples with
> > flags '-F,+insn,+insnlen'.
> >
> > CoreSight trace decoder has provided sufficient information to decide
> > the instruction size based on the isa type: A64/A32 instruction are
> > 32-bit size, but one exception is the T32 instruction size, which might
> > be 32-bit or 16-bit.
> >
> > This patch handles for these cases and it reads the instruction values
> > from DSO file; thus can support flags '-F,+insn,+insnlen'.
>
> Mathieu, can I have your Acked-by/Reviewed-by?

Yes, as soon as I have the opportunity to test it.

>
> - Arnaldo
>
> > Before:
> >
> > # perf script -F,insn,insnlen,ip,sym
> > 0 [unknown] ilen: 0
> > ffff97174044 _start ilen: 0
> > ffff97174938 _dl_start ilen: 0
> > ffff97174938 _dl_start ilen: 0
> > ffff97174938 _dl_start ilen: 0
> > ffff97174938 _dl_start ilen: 0
> > ffff97174938 _dl_start ilen: 0
> > ffff97174938 _dl_start ilen: 0
> > ffff97174938 _dl_start ilen: 0
> > ffff97174938 _dl_start ilen: 0
> >
> > [...]
> >
> > After:
> >
> > # perf script -F,insn,insnlen,ip,sym
> > 0 [unknown] ilen: 0
> > ffff97174044 _start ilen: 4 insn: 2f 02 00 94
> > ffff97174938 _dl_start ilen: 4 insn: c1 ff ff 54
> > ffff97174938 _dl_start ilen: 4 insn: c1 ff ff 54
> > ffff97174938 _dl_start ilen: 4 insn: c1 ff ff 54
> > ffff97174938 _dl_start ilen: 4 insn: c1 ff ff 54
> > ffff97174938 _dl_start ilen: 4 insn: c1 ff ff 54
> > ffff97174938 _dl_start ilen: 4 insn: c1 ff ff 54
> > ffff97174938 _dl_start ilen: 4 insn: c1 ff ff 54
> > ffff97174938 _dl_start ilen: 4 insn: c1 ff ff 54
> >
> > [...]
> >
> > Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> > Cc: Suzuki Poulouse <suzuki.poulose@arm.com>
> > Cc: Mike Leach <mike.leach@linaro.org>
> > Cc: Robert Walker <robert.walker@arm.com>
> > Cc: coresight@lists.linaro.org
> > Cc: linux-arm-kernel@lists.infradead.org
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> > tools/perf/util/cs-etm.c | 35 ++++++++++++++++++++++++++++++++++-
> > 1 file changed, 34 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> > index ed6f7fd5b90b..b3a5daaf1a8f 100644
> > --- a/tools/perf/util/cs-etm.c
> > +++ b/tools/perf/util/cs-etm.c
> > @@ -1076,6 +1076,35 @@ bool cs_etm__etmq_is_timeless(struct cs_etm_queue *etmq)
> > return !!etmq->etm->timeless_decoding;
> > }
> >
> > +static void cs_etm__copy_insn(struct cs_etm_queue *etmq,
> > + u64 trace_chan_id,
> > + const struct cs_etm_packet *packet,
> > + struct perf_sample *sample)
> > +{
> > + /*
> > + * It's pointless to read instructions for the CS_ETM_DISCONTINUITY
> > + * packet, so directly bail out with 'insn_len' = 0.
> > + */
> > + if (packet->sample_type == CS_ETM_DISCONTINUITY) {
> > + sample->insn_len = 0;
> > + return;
> > + }
> > +
> > + /*
> > + * T32 instruction size might be 32-bit or 16-bit, decide by calling
> > + * cs_etm__t32_instr_size().
> > + */
> > + if (packet->isa == CS_ETM_ISA_T32)
> > + sample->insn_len = cs_etm__t32_instr_size(etmq, trace_chan_id,
> > + sample->ip);
> > + /* Otherwise, A64 and A32 instruction size are always 32-bit. */
> > + else
> > + sample->insn_len = 4;
> > +
> > + cs_etm__mem_access(etmq, trace_chan_id, sample->ip,
> > + sample->insn_len, (void *)sample->insn);
> > +}
> > +
> > static int cs_etm__synth_instruction_sample(struct cs_etm_queue *etmq,
> > struct cs_etm_traceid_queue *tidq,
> > u64 addr, u64 period)
> > @@ -1097,9 +1126,10 @@ static int cs_etm__synth_instruction_sample(struct cs_etm_queue *etmq,
> > sample.period = period;
> > sample.cpu = tidq->packet->cpu;
> > sample.flags = tidq->prev_packet->flags;
> > - sample.insn_len = 1;
> > sample.cpumode = event->sample.header.misc;
> >
> > + cs_etm__copy_insn(etmq, tidq->trace_chan_id, tidq->packet, &sample);
> > +
> > if (etm->synth_opts.last_branch) {
> > cs_etm__copy_last_branch_rb(etmq, tidq);
> > sample.branch_stack = tidq->last_branch;
> > @@ -1159,6 +1189,9 @@ static int cs_etm__synth_branch_sample(struct cs_etm_queue *etmq,
> > sample.flags = tidq->prev_packet->flags;
> > sample.cpumode = event->sample.header.misc;
> >
> > + cs_etm__copy_insn(etmq, tidq->trace_chan_id, tidq->prev_packet,
> > + &sample);
> > +
> > /*
> > * perf report cannot handle events without a branch stack
> > */
> > --
> > 2.17.1
>
> --
>
> - Arnaldo