On 5 February 2018 at 04:15, Robert Walker robert.walker@arm.com wrote:
On 02/02/18 22:10, Mathieu Poirier wrote:
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);@@ -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.
I'm not sure what you mean here - the first patch doesn't really change anything in this file other than add the last_instr_exec field. This patch makes cs_etm_decoder__buffer_packet() the common code for creating a packet and adds functions that call this for execution range and trace on packets. Do you mean move this refactoring into the first patch?
Yes
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,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);*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 becomesPREV_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 inthe
* circular buffer at the end of the trace.** Use the address of the end of the last reportedexecution
* 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 becomesPREV_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.
You could make it a local in cs_etm__run_decoder(), but would then have to pass it as a parameter to the other functions. And you would need a memcpy to save it in prev_packet instead of a pointer swap.
I don't care much about passing the local reference as a parameter to other functions (that's how it was before) but I agree the memcpy() is expensive. Ok, leave it as it is.
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
Thanks for the review. Do you mean linux-arm-kernel as the public mailing list?
Both linux-arm-kernel@lists.infradead.org and linux-kernel@vger.kernel.org.
Rob
}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 aninstruction
* range, generate instructionsequence
* 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