Re: [PATCH 2/2] perf inject: Emit instruction records on ETM trace discontinuity

On Tue, Jan 30, 2018 at 02:42:25PM +0000, Robert Walker wrote:

There may be discontinuities in the ETM trace stream due to overflows or ETM configuration for selective trace. This patch emits an instruction sample with the pending branch stack when a TRACE ON packet occurs indicating a discontinuity in the trace data.

A new packet type CS_ETM_TRACE_ON is added, which is emitted by the low level decoder when a TRACE ON occurs. The higher level decoder flushes the branch stack when this packet is emitted.

Signed-off-by: Robert Walker robert.walker@arm.com

tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 71 ++++++++++++++------ tools/perf/util/cs-etm-decoder/cs-etm-decoder.h | 1 + tools/perf/util/cs-etm.c | 89 +++++++++++++++++-------- 3 files changed, 113 insertions(+), 48 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 ecf1780..d199f58 100644 --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c @@ -78,6 +78,8 @@ int cs_etm_decoder__reset(struct cs_etm_decoder *decoder) { ocsd_datapath_resp_t dp_ret;

• decoder->prev_return = OCSD_RESP_CONT;
• dp_ret = ocsd_dt_process_data(decoder->dcd_tree, OCSD_OP_RESET, 0, 0, NULL, NULL); if (OCSD_DATA_RESP_IS_FATAL(dp_ret))

@@ -263,7 +265,6 @@ static void cs_etm_decoder__clear_buffer(struct cs_etm_decoder *decoder) static ocsd_datapath_resp_t cs_etm_decoder__buffer_packet(struct cs_etm_decoder *decoder,

• 	      const ocsd_generic_trace_elem *elem,
        const u8 trace_chan_id,
        enum cs_etm_sample_type sample_type)
  

{ @@ -279,35 +280,62 @@ static void cs_etm_decoder__clear_buffer(struct cs_etm_decoder *decoder) return OCSD_RESP_FATAL_SYS_ERR; et = decoder->tail;

• decoder->packet_buffer[et].sample_type = sample_type;
• decoder->packet_buffer[et].start_addr = elem->st_addr;
• decoder->packet_buffer[et].end_addr = elem->en_addr;

• et = (et + 1) & (MAX_BUFFER - 1);
• decoder->tail = et;
• decoder->packet_count++;
• decoder->packet_buffer[et].sample_type = sample_type;
• decoder->packet_buffer[et].exc = false;
• decoder->packet_buffer[et].exc_ret = false;
• decoder->packet_buffer[et].cpu = *((int *)inode->priv);
• decoder->packet_buffer[et].start_addr = 0xdeadbeefdeadbeefUL;
• decoder->packet_buffer[et].end_addr = 0xdeadbeefdeadbeefUL;

Please drop the tabulations.

• if (decoder->packet_count == MAX_BUFFER - 1)

• return OCSD_RESP_WAIT;
  

• return OCSD_RESP_CONT;

+}

+static ocsd_datapath_resp_t +cs_etm_decoder__buffer_range(struct cs_etm_decoder *decoder,

• 	     const ocsd_generic_trace_elem *elem,
  

• 	     const uint8_t trace_chan_id)
  

+{

• int ret = 0;
• struct cs_etm_packet *packet;
• ret = cs_etm_decoder__buffer_packet(decoder, trace_chan_id,

• 			    CS_ETM_RANGE);
  

• if (ret != OCSD_RESP_CONT && ret != OCSD_RESP_WAIT)

• return ret;
  

• packet = &decoder->packet_buffer[decoder->tail];
• packet->start_addr = elem->st_addr;
• packet->end_addr = elem->en_addr;

Same here.

switch (elem->last_i_type) { case OCSD_INSTR_BR: case OCSD_INSTR_BR_INDIRECT:

• decoder->packet_buffer[et].last_instr_taken_branch = elem->last_instr_exec;
  

• packet->last_instr_taken_branch = elem->last_instr_exec;
  

  break; case OCSD_INSTR_ISB: case OCSD_INSTR_DSB_DMB: case OCSD_INSTR_OTHER: default:

• decoder->packet_buffer[et].last_instr_taken_branch = false;
  

• packet->last_instr_taken_branch = false;
  

  break; }

• decoder->packet_buffer[et].exc = false;
• decoder->packet_buffer[et].exc_ret = false;
• decoder->packet_buffer[et].cpu = *((int *)inode->priv);
• /* Wrap around if need be */
• et = (et + 1) & (MAX_BUFFER - 1);
• decoder->tail = et;
• decoder->packet_count++;

• if (decoder->packet_count == MAX_BUFFER - 1)

• return OCSD_RESP_WAIT;
  

• return ret;

+}

• return OCSD_RESP_CONT;

+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);
  

} static ocsd_datapath_resp_t cs_etm_decoder__gen_trace_elem_printer( @@ -326,12 +354,13 @@ static ocsd_datapath_resp_t cs_etm_decoder__gen_trace_elem_printer( decoder->trace_on = false; break; case OCSD_GEN_TRC_ELEM_TRACE_ON:

• resp = cs_etm_decoder__buffer_trace_on(decoder,
  

• 				       trace_chan_id);
  

  decoder->trace_on = true; break; case OCSD_GEN_TRC_ELEM_INSTR_RANGE:

• resp = cs_etm_decoder__buffer_packet(decoder, elem,
  

• 				     trace_chan_id,
  

• 				     CS_ETM_RANGE);
  

• resp = cs_etm_decoder__buffer_range(decoder, elem,
  

• 				    trace_chan_id);
  

I think the two patchset should be kept as they are since one is clearly building on top of the other. In your next revision keep functions cs_etm_decoder__buffer_range() so that in the second patch you don't undo the work you've done in the first patch.

break;

case OCSD_GEN_TRC_ELEM_EXCEPTION: decoder->packet_buffer[decoder->tail].exc = true; diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h index a4fdd28..743f5f4 100644 --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h @@ -24,6 +24,7 @@ struct cs_etm_buffer { enum cs_etm_sample_type { CS_ETM_RANGE = 1 << 0,

• CS_ETM_TRACE_ON = 1 << 1,

}; struct cs_etm_packet { diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index 43cf1d4..a8d07bd 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -866,6 +866,8 @@ static int cs_etm__sample(struct cs_etm_queue *etmq) * PREV_PACKET is a branch. */ if (etm->synth_opts.last_branch &&

•    etmq->prev_packet &&
  

•    etmq->prev_packet->sample_type == CS_ETM_RANGE &&
   etmq->prev_packet->last_instr_taken_branch)
  

  cs_etm__update_last_branch_rb(etmq);

@@ -898,14 +900,14 @@ static int cs_etm__sample(struct cs_etm_queue *etmq) } if (etm->sample_branches &&

•    etmq->packet->last_instr_taken_branch) {
  

•    etmq->prev_packet->sample_type == CS_ETM_RANGE &&
  

•    etmq->prev_packet->last_instr_taken_branch) {
  

Much better.

ret = cs_etm__synth_branch_sample(etmq);
if (ret)
	return ret;

}

• if (etm->sample_branches ||

•    etm->synth_opts.last_branch) {
  

• if (etm->sample_branches || etm->synth_opts.last_branch) { /*
  • Swap PACKET with PREV_PACKET: PACKET becomes PREV_PACKET for
  • the next incoming packet.

@@ -918,6 +920,40 @@ static int cs_etm__sample(struct cs_etm_queue *etmq) return 0; } +static int cs_etm__flush(struct cs_etm_queue *etmq) +{

• int err = 0;
• struct cs_etm_packet *tmp;
• if (etmq->etm->synth_opts.last_branch &&

•    etmq->prev_packet &&
  

•    etmq->prev_packet->sample_type == CS_ETM_RANGE) {
  

• /*
  

•  * Generate a last branch event for the branches left in the
  

•  * circular buffer at the end of the trace.
  

•  *
  

•  * Use the address of the end of the last reported execution
  

•  * range
  

•  */
  

• u64 addr = cs_etm__last_executed_instr(etmq->prev_packet);
  

• err = cs_etm__synth_instruction_sample(
  

• 	etmq, addr,
  

• 	etmq->period_instructions);
  

• etmq->period_instructions = 0;
  

• /*
  

•  * Swap PACKET with PREV_PACKET: PACKET becomes PREV_PACKET for
  

•  * the next incoming packet.
  

•  */
  

• tmp = etmq->packet;
  

• etmq->packet = etmq->prev_packet;
  

• etmq->prev_packet = tmp;
  

I've been thinking about etmq->packet yesterday. From what I see there is no need to have it in the cs_etm_queue structure... Perhaps I missed something.

Finally I tested things on my side and things work as advertised.

I'm done reviewing your patches - thanks for the submission. Unless you see a reason not to I think the next revision should be sent to the public mailing list once -rc1 comes out. That will give time to other people on the CS list to review and test your work.

Mathieu

• }
• return err;

+}

static int cs_etm__run_decoder(struct cs_etm_queue *etmq) { struct cs_etm_auxtrace *etm = etmq->etm; @@ -946,20 +982,19 @@ static int cs_etm__run_decoder(struct cs_etm_queue *etmq) /* Run trace decoder until buffer consumed or end of trace */ do { processed = 0;

• err = cs_etm_decoder__process_data_block(
  	etmq->decoder,
  	etmq->offset,
  	&buffer.buf[buffer_used],
  	buffer.len - buffer_used,
  	&processed);
  

• if (err)
  	return err;
  

etmq->offset += processed; buffer_used += processed;

• 	/* Process each packet in this chunk */
  while (1) {
  	err = cs_etm_decoder__get_packet(etmq->decoder,
  					 etmq->packet);
  

@@ -970,31 +1005,31 @@ static int cs_etm__run_decoder(struct cs_etm_queue *etmq) */ break;

• 		/*
  

• 		 * If the packet contains an instruction
  

• 		 * range, generate instruction sequence
  

• 		 * events.
  

• 		 */
  

• 		if (etmq->packet->sample_type & CS_ETM_RANGE)
  

• 			err = cs_etm__sample(etmq);
  

• 		switch (etmq->packet->sample_type) {
  

• 		case CS_ETM_RANGE:
  

• 			/*
  

• 			 * If the packet contains an instruction
  

• 			 * range, generate instruction sequence
  

• 			 * events.
  

• 			 */
  

• 			cs_etm__sample(etmq);
  

• 			break;
  

• 		case CS_ETM_TRACE_ON:
  

• 			/*
  

• 			 * Discontinuity in trace, flush
  

• 			 * previous branch stack
  

• 			 */
  

• 			cs_etm__flush(etmq);
  

• 			break;
  

• 		default:
  

• 			break;
  

• 		}
  }
  

  } while (buffer.len > buffer_used);

• /*
  

•  * Generate a last branch event for the branches left in
  

•  * the circular buffer at the end of the trace.
  

•  */
  

• if (etm->sample_instructions &&
  

•     etmq->etm->synth_opts.last_branch) {
  

• 	struct branch_stack *bs = etmq->last_branch_rb;
  

• 	struct branch_entry *be = &bs->entries[etmq->last_branch_pos];
  

• 	err = cs_etm__synth_instruction_sample(etmq, be->to,
  

• 					       etmq->period_instructions);
  

• 	if (err)
  

• 		return err;
  

• }
  

• if (err == 0)
  

• 	/* Flush any remaining branch stack entries */
  

• 	err = cs_etm__flush(etmq);
  

  }

return err; -- 1.9.1

---

CoreSight mailing list CoreSight@lists.linaro.org https://lists.linaro.org/mailman/listinfo/coresight