Re: [PATCH v1 5/5] perf cs-etm: Track exception number

[Mathieu Poirier]

On Tue, 4 Dec 2018 at 20:49, <leo.yan@xxxxxxxxxx> wrote:
>
> On Mon, Nov 19, 2018 at 01:47:49PM -0700, Mathieu Poirier wrote:
> > On Sun, Nov 11, 2018 at 12:59:43PM +0800, Leo Yan wrote:
> > > When an exception packet comes, it contains the info for exception
> > > number; the exception number indicates the exception types, so from it
> > > we can know if the exception is taken for interrupt, system call or
> > > other traps, etc.  But because the exception return packet cannot
> > > delivery exception number correctly by decoder thus when prepare sample
> > > flags we cannot know what's type for exception return.
> > >
> > > This patch adds a new 'exc_num' array in decoder structure to record
> > > exception number per CPU, the exception number is recorded in the array
> > > when the exception packet comes and this exception number can be used by
> > > exception return packet.  If detect there have discontinuous trace with
> > > TRACE_ON or TRACE_OFF packet, the exception number is set to invalid
> > > value.
> > >
> > > Signed-off-by: Leo Yan <leo.yan@xxxxxxxxxx>
> > > ---
> > >  tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 67 ++++++++++++++++++++++---
> > >  1 file changed, 59 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> > > index b8cb7a3e..d1a6cbc 100644
> > > --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> > > +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> > > @@ -43,6 +43,7 @@ struct cs_etm_decoder {
> > >     u32 packet_count;
> > >     u32 head;
> > >     u32 tail;
> > > +   u32 *exc_num;
> > >     struct cs_etm_packet packet_buffer[MAX_BUFFER];
> > >  };
> > >
> > > @@ -368,24 +369,64 @@ static ocsd_datapath_resp_t
> > >  cs_etm_decoder__buffer_trace_off(struct cs_etm_decoder *decoder,
> > >                              const uint8_t trace_chan_id)
> > >  {
> > > -   return cs_etm_decoder__buffer_packet(decoder, trace_chan_id,
> > > -                                        CS_ETM_TRACE_OFF);
> > > +   int ret;
> > > +   struct cs_etm_packet *packet;
> > > +
> > > +   ret = cs_etm_decoder__buffer_packet(decoder, trace_chan_id,
> > > +                                       CS_ETM_TRACE_OFF);
> > > +   if (ret != OCSD_RESP_CONT && ret != OCSD_RESP_WAIT)
> > > +           return ret;
> > > +
> > > +   packet = &decoder->packet_buffer[decoder->tail];
> > > +
> > > +   /* Clear execption number for discontinuous trace */
> > > +   decoder->exc_num[packet->cpu] = UINT32_MAX;
> > > +
> > > +   return ret;
> > >  }
> > >
> > >  static ocsd_datapath_resp_t
> > >  cs_etm_decoder__buffer_trace_on(struct cs_etm_decoder *decoder,
> > >                             const uint8_t trace_chan_id)
> > >  {
> > > -   return cs_etm_decoder__buffer_packet(decoder, trace_chan_id,
> > > -                                        CS_ETM_TRACE_ON);
> > > +   int ret;
> > > +   struct cs_etm_packet *packet;
> > > +
> > > +   ret = cs_etm_decoder__buffer_packet(decoder, trace_chan_id,
> > > +                                       CS_ETM_TRACE_ON);
> > > +   if (ret != OCSD_RESP_CONT && ret != OCSD_RESP_WAIT)
> > > +           return ret;
> > > +
> > > +   packet = &decoder->packet_buffer[decoder->tail];
> > > +
> > > +   /* Clear execption number for discontinuous trace */
> > > +   decoder->exc_num[packet->cpu] = UINT32_MAX;
> > > +
> > > +   return ret;
> > >  }
> > >
> > >  static ocsd_datapath_resp_t
> > >  cs_etm_decoder__buffer_exception(struct cs_etm_decoder *decoder,
> > > +                            const ocsd_generic_trace_elem *elem,
> > >                              const uint8_t trace_chan_id)
> > >  {
> > > -   return cs_etm_decoder__buffer_packet(decoder, trace_chan_id,
> > > -                                        CS_ETM_EXCEPTION);
> > > +   int ret;
> > > +   struct cs_etm_packet *packet;
> > > +
> > > +   ret = cs_etm_decoder__buffer_packet(decoder, trace_chan_id,
> > > +                                       CS_ETM_EXCEPTION);
> > > +   if (ret != OCSD_RESP_CONT && ret != OCSD_RESP_WAIT)
> > > +           return ret;
> > > +
> > > +   packet = &decoder->packet_buffer[decoder->tail];
> > > +
> > > +   /*
> > > +    * Exception number is recorded per CPU and later can be used
> > > +    * for exception return instruction analysis.
> > > +    */
> > > +   decoder->exc_num[packet->cpu] = elem->exception_number;
> >
> > Am I missing something or the information about the exception number that is
> > recorded here isn't used anywhere?
>
> The exception number will be used to set branch flag patch [1].

Right, I realised that when I started reviewing that set.  The rule of
thumb here is to introduce code in the same patchset it is used so
that we avoid adding needless code to the kernel.

> According to exception number we can know it's for system call,
> interrupt or other traps.
>
> [1] http://archive.armlinux.org.uk/lurker/message/20181111.050755.d1c1b257.en.html
>
> > If you want to use this in perf report/script,
> > the exception number will have to be added to the cs_etm_packet struct.
>
> Actually before has discussed this with Mike but found it's hard to
> save the exception number in cs_etm_packet struct.  The reason is the
> exception packet contains the correct exception number, but the
> exception return packet doesn't contain exception number.  Thus this
> patch uses cs_etm_decoder struct to save exception number per CPU
> context when receive exception packet, and later the saved exception
> number will be used by exception return packet.
>
> Please see related discussion at the end of page [2].

I find Mike's point about the possibility of seeing exception returns
without having a prior exception due to various factors very
interesting.  I will make sure to keep an eye out for that in the next
revision.

>
> [2] https://lists.linaro.org/pipermail/coresight/2018-October/001832.html
>
> > I am done with the revision of this set.
>
> Thanks a lot for reviewing.
>
> [...]
>
> Thanks,
> Leo Yan