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

[leo . yan]

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].
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].

[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