On Tue, 4 Dec 2018 at 20:49, <leo....@linaro.org> 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....@linaro.org>
> > > ---
> > > 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
