This patch seris adds support for sample flags so can facilitate perf
to print sample flags for branch instruction.
The patch 0001 is to set branch instruction flags in packet, this
patch has the core code in this series to set flags according to the
decoding element type, and also based on the elements including
instruction type, subtype and condition flag to help making decision
to set flags value.
The patch 0002 is to support sample flags by copying the flags value
from packet structure to sample structure, and it includes three fixing
up for TRACE_ON/TRACE_OFF and exception packets.
The patch series is based on OpenCSD v0.10.0 and Rob's patch 'perf:
Support for Arm A32/T32 instruction sets in CoreSight trace' also is
prerequisite to support A32/T32 ISAs.
This patch series is applied on the acme's perf core branch [1] with the
latest commit f1d23afaf677 ("perf bpf: Reduce the hardcoded .max_entries
for pid_maps") and has two prerequisites:
1) It's dependent on Rob's patch 'perf: Support for Arm A32/T32
instruction sets in CoreSight trace' [2];
2) It's dependent on another patch series 'perf cs-etm: Correct packets
handling' [3].
After applying the dependency patches and this patch series, we can
verify sample flags with below command:
# perf script -F,-time,+flags,+ip,+sym,+dso,+addr,+symoff -k vmlinux
Changes from v1:
* Moved exception packets handling patches into patch series 'perf
cs-etm: Correct packets handling'.
* Added sample flags fixing up for TRACE_OFF packet.
* Created a new function which is used to maintain flags fixing up.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/log/?h=perf/core
[2] http://archive.armlinux.org.uk/lurker/message/20181109.091126.9d69489d.en.html
[3] http://archive.armlinux.org.uk/lurker/message/20181111.045938.782b378b.en.html
Leo Yan (2):
perf cs-etm: Set branch instruction flags in packet
perf cs-etm: Add support sample flags
tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 168 ++++++++++++++++++++++++
tools/perf/util/cs-etm-decoder/cs-etm-decoder.h | 1 +
tools/perf/util/cs-etm.c | 43 +++++-
3 files changed, 210 insertions(+), 2 deletions(-)
--
2.7.4
The perf sample data contains flags to indicate the hardware trace data
is belonging to which type branch instruction, thus this can be used to
print out the human readable string. Arm CoreSight ETM sample data is
missed to set flags and it is always set to zeros, this results in perf
tool skips to print string for instruction types.
Arm CoreSight ETM supports different kinds instruction of A64, A32 and
T32; this patch is to set branch instruction flags in packet for these
ISAs.
The brief idea for patch implementation is describe as below:
- For element with OCSD_GEN_TRC_ELEM_TRACE_ON type, it is taken as trace
beginning packet; for element with OCSD_GEN_TRC_ELEM_NO_SYNC or
OCSD_GEN_TRC_ELEM_EO_TRACE, these two kinds elements are used to set
for trace end;
As Mike suggested the packet stream might have more than one two
TRACE_ON packets, the first one TRACE_ON packet indicates trace end
and the second one is taken as trace restarting. We will handle this
special case in the upper layer with packet queue handling, which has
more context so it's more suitable fix up for it. This will be
accomplished in the sequential patch.
- For instruction range packet, mainly base on three factors to decide
the branch instruction types:
elem->last_i_type
elem->last_i_subtype
elem->last_instr_cond
If the instruction is immediate branch but without link and return
flag, we consider it as function internal branch; in fact the
immediate branch also can be used to invoke the function entry,
usually this is only used in assembly code to directly call a symbol
and don't expect to return back; after reviewing kernel normal
functions and user space programs, both of them are very seldom to use
immediate branch for function call. On the other hand, if we want to
decide the immediate branch is for function branch jumping or for
function calling, we need to rely on the start address of next packet
and check the symbol offset for the start address, this will
introduce much complexity in the implementation. So for this version
we simply consider immediate branch as function internal branch.
Moreover, we rely on 'elem->last_instr_cond' to decide if the branch
instruction is a conditional branch or not.
If the instruction is immediate branch with link, it's instruction
'BL' and which is used for function call.
If the instruction is indirect branch and with subtype
OCSD_S_INSTR_V7_IMPLIED_RET, the decoders gives the hint the function
return for below cases related with A32/T32 instruction; set this
branch flag as function return (Thanks for Al's suggestion).
BX R14
MOV PC, LR
POP {…, PC}
LDR PC, [SP], #offset
If the instruction is indirect branch without link, this is
corresponding to instruction 'BR', this instruction usually is used
for dynamic link lib with below usage; so we think it's a return
instruction.
0000000000000680 <.plt>:
680: a9bf7bf0 stp x16, x30, [sp, #-16]!
684: 90000090 adrp x16, 10000 <__FRAME_END__+0xf630>
688: f947fe11 ldr x17, [x16, #4088]
68c: 913fe210 add x16, x16, #0xff8
690: d61f0220 br x17
If the instruction is indirect branch with link, e.g BLR, we think
it's a function call.
For function return, ARMv8 introduces a dedicated instruction 'ret',
which has flag of OCSD_S_INSTR_V8_RET.
- For exception packets, this patch divides into three types:
The first type of exception is caused by external logics like bus,
interrupt controller, debug module or PE reset or halt; this is
corresponding to flags "bcyi" which defined in doc perf-script.txt;
The second type is for system call, this is set as "bcs" by following
definition in the doc;
The third type is for CPU trap, data and instruction prefetch abort,
alignment abort; usually these exceptions are synchronous for CPU, so
set them as "bci" type.
Cc: Mathieu Poirier <[email protected]>
Cc: Mike Leach <[email protected]>
Cc: Robert Walker <[email protected]>
Cc: Al Grant <[email protected]>
Cc: Andi Kleen <[email protected]>
Cc: Adrian Hunter <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Signed-off-by: Leo Yan <[email protected]>
---
tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 168 ++++++++++++++++++++++++
tools/perf/util/cs-etm-decoder/cs-etm-decoder.h | 1 +
2 files changed, 169 insertions(+)
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 d1a6cbc..0e50c52 100644
--- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
+++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
@@ -303,6 +303,7 @@ cs_etm_decoder__buffer_packet(struct cs_etm_decoder *decoder,
decoder->packet_buffer[et].instr_count = 0;
decoder->packet_buffer[et].last_instr_taken_branch = false;
decoder->packet_buffer[et].last_instr_size = 0;
+ decoder->packet_buffer[et].flags = 0;
if (decoder->packet_count == MAX_BUFFER - 1)
return OCSD_RESP_WAIT;
@@ -437,6 +438,171 @@ cs_etm_decoder__buffer_exception_ret(struct cs_etm_decoder *decoder,
CS_ETM_EXCEPTION_RET);
}
+static void cs_etm_decoder__set_sample_flags(
+ const void *context,
+ const ocsd_generic_trace_elem *elem)
+{
+ struct cs_etm_decoder *decoder = (struct cs_etm_decoder *) context;
+ struct cs_etm_packet *packet;
+ u32 exc_num;
+
+ packet = &decoder->packet_buffer[decoder->tail];
+
+ switch (elem->elem_type) {
+ case OCSD_GEN_TRC_ELEM_TRACE_ON:
+ packet->flags = PERF_IP_FLAG_BRANCH |
+ PERF_IP_FLAG_TRACE_BEGIN;
+ break;
+
+ case OCSD_GEN_TRC_ELEM_NO_SYNC:
+ case OCSD_GEN_TRC_ELEM_EO_TRACE:
+ packet->flags = PERF_IP_FLAG_BRANCH |
+ PERF_IP_FLAG_TRACE_END;
+ break;
+
+ case OCSD_GEN_TRC_ELEM_INSTR_RANGE:
+ /*
+ * Immediate branch instruction without neither link nor
+ * return flag, it's normal branch instruction within
+ * the function.
+ */
+ if (elem->last_i_type == OCSD_INSTR_BR &&
+ elem->last_i_subtype == OCSD_S_INSTR_NONE) {
+ packet->flags = PERF_IP_FLAG_BRANCH;
+
+ if (elem->last_instr_cond)
+ packet->flags |= PERF_IP_FLAG_CONDITIONAL;
+ }
+
+ /*
+ * Immediate branch instruction with link (e.g. BL), this is
+ * branch instruction for function call.
+ */
+ if (elem->last_i_type == OCSD_INSTR_BR &&
+ elem->last_i_subtype == OCSD_S_INSTR_BR_LINK)
+ packet->flags = PERF_IP_FLAG_BRANCH |
+ PERF_IP_FLAG_CALL;
+
+ /*
+ * Indirect branch instruction with subtype of
+ * OCSD_S_INSTR_V7_IMPLIED_RET, this is explicit hint for
+ * function return for A32/T32.
+ */
+ if (elem->last_i_type == OCSD_INSTR_BR_INDIRECT &&
+ elem->last_i_subtype == OCSD_S_INSTR_V7_IMPLIED_RET)
+ packet->flags = PERF_IP_FLAG_BRANCH |
+ PERF_IP_FLAG_RETURN;
+
+ /*
+ * Indirect branch instruction without link (e.g. BR), usually
+ * this is used for function return, especially for functions
+ * within dynamic link lib.
+ */
+ if (elem->last_i_type == OCSD_INSTR_BR_INDIRECT &&
+ elem->last_i_subtype == OCSD_S_INSTR_NONE)
+ packet->flags = PERF_IP_FLAG_BRANCH |
+ PERF_IP_FLAG_RETURN;
+
+ /*
+ * Indirect branch instruction with link (e.g. BLR), this is
+ * branch instruction for function call.
+ */
+ if (elem->last_i_type == OCSD_INSTR_BR_INDIRECT &&
+ elem->last_i_subtype == OCSD_S_INSTR_BR_LINK)
+ packet->flags = PERF_IP_FLAG_BRANCH |
+ PERF_IP_FLAG_CALL;
+
+ /* Return instruction for function return. */
+ if (elem->last_i_type == OCSD_INSTR_BR_INDIRECT &&
+ elem->last_i_subtype == OCSD_S_INSTR_V8_RET)
+ packet->flags = PERF_IP_FLAG_BRANCH |
+ PERF_IP_FLAG_RETURN;
+
+ break;
+
+ case OCSD_GEN_TRC_ELEM_EXCEPTION:
+
+#define OCSD_EXC_RESET 0
+#define OCSD_EXC_DEBUG_HALT 1
+#define OCSD_EXC_CALL 2
+#define OCSD_EXC_TRAP 3
+#define OCSD_EXC_SYSTEM_ERROR 4
+#define OCSD_EXC_INST_DEBUG 6
+#define OCSD_EXC_DATA_DEBUG 7
+#define OCSD_EXC_ALIGNMENT 10
+#define OCSD_EXC_INST_FAULT 11
+#define OCSD_EXC_DATA_FAULT 12
+#define OCSD_EXC_IRQ 14
+#define OCSD_EXC_FIQ 15
+
+ exc_num = decoder->exc_num[packet->cpu];
+
+ /*
+ * The exceptions are triggered by external signals
+ * from bus, interrupt controller, debug module,
+ * PE reset or halt.
+ */
+ if (exc_num == OCSD_EXC_RESET ||
+ exc_num == OCSD_EXC_DEBUG_HALT ||
+ exc_num == OCSD_EXC_SYSTEM_ERROR ||
+ exc_num == OCSD_EXC_INST_DEBUG ||
+ exc_num == OCSD_EXC_DATA_DEBUG ||
+ exc_num == OCSD_EXC_IRQ ||
+ exc_num == OCSD_EXC_FIQ)
+ packet->flags = PERF_IP_FLAG_BRANCH |
+ PERF_IP_FLAG_CALL |
+ PERF_IP_FLAG_ASYNC |
+ PERF_IP_FLAG_INTERRUPT;
+
+ /* The exception is for system call. */
+ if (exc_num == OCSD_EXC_CALL)
+ packet->flags = PERF_IP_FLAG_BRANCH |
+ PERF_IP_FLAG_CALL |
+ PERF_IP_FLAG_SYSCALLRET;
+
+ /*
+ * The exception is introduced by trap, instruction &
+ * data fault or alignment errors.
+ */
+ if (exc_num == OCSD_EXC_TRAP ||
+ exc_num == OCSD_EXC_ALIGNMENT ||
+ exc_num == OCSD_EXC_INST_FAULT ||
+ exc_num == OCSD_EXC_DATA_FAULT)
+ packet->flags = PERF_IP_FLAG_BRANCH |
+ PERF_IP_FLAG_CALL |
+ PERF_IP_FLAG_INTERRUPT;
+
+ break;
+
+ case OCSD_GEN_TRC_ELEM_EXCEPTION_RET:
+
+ exc_num = decoder->exc_num[packet->cpu];
+
+ if (exc_num == OCSD_EXC_CALL)
+ packet->flags = PERF_IP_FLAG_BRANCH |
+ PERF_IP_FLAG_RETURN |
+ PERF_IP_FLAG_SYSCALLRET;
+ else
+ packet->flags = PERF_IP_FLAG_BRANCH |
+ PERF_IP_FLAG_RETURN |
+ PERF_IP_FLAG_INTERRUPT;
+
+ break;
+
+ case OCSD_GEN_TRC_ELEM_UNKNOWN:
+ case OCSD_GEN_TRC_ELEM_PE_CONTEXT:
+ case OCSD_GEN_TRC_ELEM_ADDR_NACC:
+ case OCSD_GEN_TRC_ELEM_TIMESTAMP:
+ case OCSD_GEN_TRC_ELEM_CYCLE_COUNT:
+ case OCSD_GEN_TRC_ELEM_ADDR_UNKNOWN:
+ case OCSD_GEN_TRC_ELEM_EVENT:
+ case OCSD_GEN_TRC_ELEM_SWTRACE:
+ case OCSD_GEN_TRC_ELEM_CUSTOM:
+ default:
+ break;
+ }
+}
+
static ocsd_datapath_resp_t cs_etm_decoder__gen_trace_elem_printer(
const void *context,
const ocsd_trc_index_t indx __maybe_unused,
@@ -484,6 +650,8 @@ static ocsd_datapath_resp_t cs_etm_decoder__gen_trace_elem_printer(
break;
}
+ cs_etm_decoder__set_sample_flags(context, elem);
+
return resp;
}
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 0d1c18d..71df908 100644
--- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h
+++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h
@@ -47,6 +47,7 @@ struct cs_etm_packet {
u8 last_instr_taken_branch;
u8 last_instr_size;
int cpu;
+ u32 flags;
};
struct cs_etm_queue;
--
2.7.4
We have prepared the flags in the packet structure, so need to copy
the related value into sample structure thus perf tool can facilitate
sample flags.
The PREV_PACKET contains the branch instruction flags and PACKET
actually contains the flags for next branch instruction. So this patch
is to set sample flags with 'etmq->prev_packet->flags'.
This patch includes three fixing up for sample flags based on the
packets context:
- If the packet is exception packet or exception return packet, update
the previous packet for exception specific flags;
- If there has TRACE_ON or TRACE_OFF packet in the middle of instruction
packets, this indicates the trace is discontinuous, so append the flag
PERF_IP_FLAG_TRACE_END to the previous packet to indicate the trace
has been ended;
- If one instruction packet is behind TRACE_OFF packet, this instruction
is restarting trace packet. So set flag PERF_IP_FLAG_TRACE_START to
TRACE_OFF packet if one, this flag isn't used by TRACE_OFF packet but
used to indicate trace restarting when generate sample.
Signed-off-by: Leo Yan <[email protected]>
---
tools/perf/util/cs-etm.c | 43 +++++++++++++++++++++++++++++++++++++++++--
1 file changed, 41 insertions(+), 2 deletions(-)
diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
index 455f132..afca6f3 100644
--- a/tools/perf/util/cs-etm.c
+++ b/tools/perf/util/cs-etm.c
@@ -676,7 +676,7 @@ static int cs_etm__synth_instruction_sample(struct cs_etm_queue *etmq,
sample.stream_id = etmq->etm->instructions_id;
sample.period = period;
sample.cpu = etmq->packet->cpu;
- sample.flags = 0;
+ sample.flags = etmq->prev_packet->flags;
sample.insn_len = 1;
sample.cpumode = event->sample.header.misc;
@@ -735,7 +735,7 @@ static int cs_etm__synth_branch_sample(struct cs_etm_queue *etmq)
sample.stream_id = etmq->etm->branches_id;
sample.period = 1;
sample.cpu = etmq->packet->cpu;
- sample.flags = 0;
+ sample.flags = etmq->prev_packet->flags;
sample.cpumode = event->sample.header.misc;
/*
@@ -878,6 +878,43 @@ static int cs_etm__synth_events(struct cs_etm_auxtrace *etm,
return 0;
}
+static void cs_etm__fixup_flags(struct cs_etm_queue *etmq)
+{
+ /*
+ * Decoding stream might insert one TRACE_OFF packet in the
+ * middle of instruction packets, this means it doesn't
+ * contain the pair packets with TRACE_OFF and TRACE_ON.
+ * For this case, the instruction packet follows with
+ * TRACE_OFF packet so we need to fixup prev_packet with flag
+ * PERF_IP_FLAG_TRACE_BEGIN, this flag finally is used by the
+ * instruction packet to generate samples.
+ */
+ if (etmq->prev_packet->sample_type == CS_ETM_TRACE_OFF &&
+ etmq->packet->sample_type == CS_ETM_RANGE)
+ etmq->prev_packet->flags = PERF_IP_FLAG_BRANCH |
+ PERF_IP_FLAG_TRACE_BEGIN;
+
+ if (etmq->prev_packet->sample_type == CS_ETM_RANGE) {
+ /*
+ * When the exception packet is inserted, update flags
+ * so tell perf it is exception related branches.
+ */
+ if (etmq->packet->sample_type == CS_ETM_EXCEPTION ||
+ etmq->packet->sample_type == CS_ETM_EXCEPTION_RET)
+ etmq->prev_packet->flags = etmq->packet->flags;
+
+ /*
+ * The trace is discontinuous, weather this is caused by
+ * TRACE_ON packet or TRACE_OFF packet is coming, if the
+ * previous packet is instruction packet, simply set flag
+ * PERF_IP_FLAG_TRACE_END for previous packet.
+ */
+ if (etmq->packet->sample_type == CS_ETM_TRACE_ON ||
+ etmq->packet->sample_type == CS_ETM_TRACE_OFF)
+ etmq->prev_packet->flags |= PERF_IP_FLAG_TRACE_END;
+ }
+}
+
static int cs_etm__sample(struct cs_etm_queue *etmq)
{
struct cs_etm_auxtrace *etm = etmq->etm;
@@ -1100,6 +1137,8 @@ static int cs_etm__run_decoder(struct cs_etm_queue *etmq)
*/
break;
+ cs_etm__fixup_flags(etmq);
+
switch (etmq->packet->sample_type) {
case CS_ETM_RANGE:
/*
--
2.7.4
On Sun, Nov 11, 2018 at 01:07:55PM +0800, Leo Yan wrote:
> The perf sample data contains flags to indicate the hardware trace data
> is belonging to which type branch instruction, thus this can be used to
> print out the human readable string. Arm CoreSight ETM sample data is
> missed to set flags and it is always set to zeros, this results in perf
> tool skips to print string for instruction types.
>
> Arm CoreSight ETM supports different kinds instruction of A64, A32 and
> T32; this patch is to set branch instruction flags in packet for these
> ISAs.
>
> The brief idea for patch implementation is describe as below:
>
> - For element with OCSD_GEN_TRC_ELEM_TRACE_ON type, it is taken as trace
> beginning packet; for element with OCSD_GEN_TRC_ELEM_NO_SYNC or
> OCSD_GEN_TRC_ELEM_EO_TRACE, these two kinds elements are used to set
> for trace end;
>
> As Mike suggested the packet stream might have more than one two
> TRACE_ON packets, the first one TRACE_ON packet indicates trace end
> and the second one is taken as trace restarting. We will handle this
> special case in the upper layer with packet queue handling, which has
> more context so it's more suitable fix up for it. This will be
> accomplished in the sequential patch.
>
> - For instruction range packet, mainly base on three factors to decide
> the branch instruction types:
>
> elem->last_i_type
> elem->last_i_subtype
> elem->last_instr_cond
>
> If the instruction is immediate branch but without link and return
> flag, we consider it as function internal branch; in fact the
> immediate branch also can be used to invoke the function entry,
> usually this is only used in assembly code to directly call a symbol
> and don't expect to return back; after reviewing kernel normal
> functions and user space programs, both of them are very seldom to use
> immediate branch for function call. On the other hand, if we want to
> decide the immediate branch is for function branch jumping or for
> function calling, we need to rely on the start address of next packet
> and check the symbol offset for the start address, this will
> introduce much complexity in the implementation. So for this version
> we simply consider immediate branch as function internal branch.
> Moreover, we rely on 'elem->last_instr_cond' to decide if the branch
> instruction is a conditional branch or not.
>
> If the instruction is immediate branch with link, it's instruction
> 'BL' and which is used for function call.
>
> If the instruction is indirect branch and with subtype
> OCSD_S_INSTR_V7_IMPLIED_RET, the decoders gives the hint the function
> return for below cases related with A32/T32 instruction; set this
> branch flag as function return (Thanks for Al's suggestion).
>
> BX R14
> MOV PC, LR
> POP {…, PC}
> LDR PC, [SP], #offset
>
> If the instruction is indirect branch without link, this is
> corresponding to instruction 'BR', this instruction usually is used
> for dynamic link lib with below usage; so we think it's a return
> instruction.
>
> 0000000000000680 <.plt>:
> 680: a9bf7bf0 stp x16, x30, [sp, #-16]!
> 684: 90000090 adrp x16, 10000 <__FRAME_END__+0xf630>
> 688: f947fe11 ldr x17, [x16, #4088]
> 68c: 913fe210 add x16, x16, #0xff8
> 690: d61f0220 br x17
>
> If the instruction is indirect branch with link, e.g BLR, we think
> it's a function call.
>
> For function return, ARMv8 introduces a dedicated instruction 'ret',
> which has flag of OCSD_S_INSTR_V8_RET.
>
> - For exception packets, this patch divides into three types:
>
> The first type of exception is caused by external logics like bus,
> interrupt controller, debug module or PE reset or halt; this is
> corresponding to flags "bcyi" which defined in doc perf-script.txt;
>
> The second type is for system call, this is set as "bcs" by following
> definition in the doc;
>
> The third type is for CPU trap, data and instruction prefetch abort,
> alignment abort; usually these exceptions are synchronous for CPU, so
> set them as "bci" type.
This is too long and needs to be broken down into pieces. I would split this
patch in 3 heat, one for NO_SYNC and TRACE_ON, one for INSTR_RANGE and one for
ELEM_EXCEPTION/ELEM_EXCEPTION_RET.
>
> Cc: Mathieu Poirier <[email protected]>
> Cc: Mike Leach <[email protected]>
> Cc: Robert Walker <[email protected]>
> Cc: Al Grant <[email protected]>
> Cc: Andi Kleen <[email protected]>
> Cc: Adrian Hunter <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> Signed-off-by: Leo Yan <[email protected]>
> ---
> tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 168 ++++++++++++++++++++++++
> tools/perf/util/cs-etm-decoder/cs-etm-decoder.h | 1 +
> 2 files changed, 169 insertions(+)
>
> 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 d1a6cbc..0e50c52 100644
> --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> @@ -303,6 +303,7 @@ cs_etm_decoder__buffer_packet(struct cs_etm_decoder *decoder,
> decoder->packet_buffer[et].instr_count = 0;
> decoder->packet_buffer[et].last_instr_taken_branch = false;
> decoder->packet_buffer[et].last_instr_size = 0;
> + decoder->packet_buffer[et].flags = 0;
Since PERF_IP_FLAG_BRANCH is '0', I would set this to UNINT32_MAX.
>
> if (decoder->packet_count == MAX_BUFFER - 1)
> return OCSD_RESP_WAIT;
> @@ -437,6 +438,171 @@ cs_etm_decoder__buffer_exception_ret(struct cs_etm_decoder *decoder,
> CS_ETM_EXCEPTION_RET);
> }
>
> +static void cs_etm_decoder__set_sample_flags(
> + const void *context,
> + const ocsd_generic_trace_elem *elem)
> +{
> + struct cs_etm_decoder *decoder = (struct cs_etm_decoder *) context;
> + struct cs_etm_packet *packet;
> + u32 exc_num;
> +
> + packet = &decoder->packet_buffer[decoder->tail];
> +
> + switch (elem->elem_type) {
> + case OCSD_GEN_TRC_ELEM_TRACE_ON:
> + packet->flags = PERF_IP_FLAG_BRANCH |
> + PERF_IP_FLAG_TRACE_BEGIN;
> + break;
> +
> + case OCSD_GEN_TRC_ELEM_NO_SYNC:
> + case OCSD_GEN_TRC_ELEM_EO_TRACE:
> + packet->flags = PERF_IP_FLAG_BRANCH |
> + PERF_IP_FLAG_TRACE_END;
> + break;
> +
> + case OCSD_GEN_TRC_ELEM_INSTR_RANGE:
> + /*
> + * Immediate branch instruction without neither link nor
> + * return flag, it's normal branch instruction within
> + * the function.
> + */
> + if (elem->last_i_type == OCSD_INSTR_BR &&
> + elem->last_i_subtype == OCSD_S_INSTR_NONE) {
> + packet->flags = PERF_IP_FLAG_BRANCH;
> +
> + if (elem->last_instr_cond)
> + packet->flags |= PERF_IP_FLAG_CONDITIONAL;
> + }
> +
> + /*
> + * Immediate branch instruction with link (e.g. BL), this is
> + * branch instruction for function call.
> + */
> + if (elem->last_i_type == OCSD_INSTR_BR &&
> + elem->last_i_subtype == OCSD_S_INSTR_BR_LINK)
> + packet->flags = PERF_IP_FLAG_BRANCH |
> + PERF_IP_FLAG_CALL;
> +
> + /*
> + * Indirect branch instruction with subtype of
> + * OCSD_S_INSTR_V7_IMPLIED_RET, this is explicit hint for
> + * function return for A32/T32.
> + */
> + if (elem->last_i_type == OCSD_INSTR_BR_INDIRECT &&
> + elem->last_i_subtype == OCSD_S_INSTR_V7_IMPLIED_RET)
> + packet->flags = PERF_IP_FLAG_BRANCH |
> + PERF_IP_FLAG_RETURN;
> +
> + /*
> + * Indirect branch instruction without link (e.g. BR), usually
> + * this is used for function return, especially for functions
> + * within dynamic link lib.
> + */
> + if (elem->last_i_type == OCSD_INSTR_BR_INDIRECT &&
> + elem->last_i_subtype == OCSD_S_INSTR_NONE)
> + packet->flags = PERF_IP_FLAG_BRANCH |
> + PERF_IP_FLAG_RETURN;
> +
> + /*
> + * Indirect branch instruction with link (e.g. BLR), this is
> + * branch instruction for function call.
> + */
> + if (elem->last_i_type == OCSD_INSTR_BR_INDIRECT &&
> + elem->last_i_subtype == OCSD_S_INSTR_BR_LINK)
> + packet->flags = PERF_IP_FLAG_BRANCH |
> + PERF_IP_FLAG_CALL;
> +
> + /* Return instruction for function return. */
> + if (elem->last_i_type == OCSD_INSTR_BR_INDIRECT &&
> + elem->last_i_subtype == OCSD_S_INSTR_V8_RET)
> + packet->flags = PERF_IP_FLAG_BRANCH |
> + PERF_IP_FLAG_RETURN;
I would swap the last to if() condition so that the (BRANCH | RETURN) flags
are all at the same place.
> +
> + break;
> +
> + case OCSD_GEN_TRC_ELEM_EXCEPTION:
> +
> +#define OCSD_EXC_RESET 0
> +#define OCSD_EXC_DEBUG_HALT 1
> +#define OCSD_EXC_CALL 2
> +#define OCSD_EXC_TRAP 3
> +#define OCSD_EXC_SYSTEM_ERROR 4
> +#define OCSD_EXC_INST_DEBUG 6
> +#define OCSD_EXC_DATA_DEBUG 7
> +#define OCSD_EXC_ALIGNMENT 10
> +#define OCSD_EXC_INST_FAULT 11
> +#define OCSD_EXC_DATA_FAULT 12
> +#define OCSD_EXC_IRQ 14
> +#define OCSD_EXC_FIQ 15
Where did you get the above? To me this is something that should come from the
library.
> +
> + exc_num = decoder->exc_num[packet->cpu];
> +
> + /*
> + * The exceptions are triggered by external signals
> + * from bus, interrupt controller, debug module,
> + * PE reset or halt.
> + */
> + if (exc_num == OCSD_EXC_RESET ||
> + exc_num == OCSD_EXC_DEBUG_HALT ||
> + exc_num == OCSD_EXC_SYSTEM_ERROR ||
> + exc_num == OCSD_EXC_INST_DEBUG ||
> + exc_num == OCSD_EXC_DATA_DEBUG ||
> + exc_num == OCSD_EXC_IRQ ||
> + exc_num == OCSD_EXC_FIQ)
> + packet->flags = PERF_IP_FLAG_BRANCH |
> + PERF_IP_FLAG_CALL |
> + PERF_IP_FLAG_ASYNC |
> + PERF_IP_FLAG_INTERRUPT;
> +
> + /* The exception is for system call. */
> + if (exc_num == OCSD_EXC_CALL)
> + packet->flags = PERF_IP_FLAG_BRANCH |
> + PERF_IP_FLAG_CALL |
> + PERF_IP_FLAG_SYSCALLRET;
> +
> + /*
> + * The exception is introduced by trap, instruction &
> + * data fault or alignment errors.
> + */
> + if (exc_num == OCSD_EXC_TRAP ||
> + exc_num == OCSD_EXC_ALIGNMENT ||
> + exc_num == OCSD_EXC_INST_FAULT ||
> + exc_num == OCSD_EXC_DATA_FAULT)
> + packet->flags = PERF_IP_FLAG_BRANCH |
> + PERF_IP_FLAG_CALL |
> + PERF_IP_FLAG_INTERRUPT;
> +
> + break;
> +
> + case OCSD_GEN_TRC_ELEM_EXCEPTION_RET:
> +
> + exc_num = decoder->exc_num[packet->cpu];
> +
> + if (exc_num == OCSD_EXC_CALL)
> + packet->flags = PERF_IP_FLAG_BRANCH |
> + PERF_IP_FLAG_RETURN |
> + PERF_IP_FLAG_SYSCALLRET;
> + else
> + packet->flags = PERF_IP_FLAG_BRANCH |
> + PERF_IP_FLAG_RETURN |
> + PERF_IP_FLAG_INTERRUPT;
> +
> + break;
> +
> + case OCSD_GEN_TRC_ELEM_UNKNOWN:
> + case OCSD_GEN_TRC_ELEM_PE_CONTEXT:
> + case OCSD_GEN_TRC_ELEM_ADDR_NACC:
> + case OCSD_GEN_TRC_ELEM_TIMESTAMP:
> + case OCSD_GEN_TRC_ELEM_CYCLE_COUNT:
> + case OCSD_GEN_TRC_ELEM_ADDR_UNKNOWN:
> + case OCSD_GEN_TRC_ELEM_EVENT:
> + case OCSD_GEN_TRC_ELEM_SWTRACE:
> + case OCSD_GEN_TRC_ELEM_CUSTOM:
> + default:
> + break;
> + }
> +}
> +
> static ocsd_datapath_resp_t cs_etm_decoder__gen_trace_elem_printer(
> const void *context,
> const ocsd_trc_index_t indx __maybe_unused,
> @@ -484,6 +650,8 @@ static ocsd_datapath_resp_t cs_etm_decoder__gen_trace_elem_printer(
> break;
> }
>
> + cs_etm_decoder__set_sample_flags(context, elem);
> +
I was toying with the idea of setting the flags in each of the case statement
found in cs_etm_decoder__gen_trace_elem_printer(). But that would move more
code around and the end result would be the same so let's keep it that way until
we have a good reason to split it.
Mathieu
> return resp;
> }
>
> 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 0d1c18d..71df908 100644
> --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h
> +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h
> @@ -47,6 +47,7 @@ struct cs_etm_packet {
> u8 last_instr_taken_branch;
> u8 last_instr_size;
> int cpu;
> + u32 flags;
> };
>
> struct cs_etm_queue;
> --
> 2.7.4
>
On Sun, Nov 11, 2018 at 01:07:56PM +0800, Leo Yan wrote:
> We have prepared the flags in the packet structure, so need to copy
> the related value into sample structure thus perf tool can facilitate
> sample flags.
>
> The PREV_PACKET contains the branch instruction flags and PACKET
> actually contains the flags for next branch instruction. So this patch
> is to set sample flags with 'etmq->prev_packet->flags'.
>
> This patch includes three fixing up for sample flags based on the
> packets context:
>
> - If the packet is exception packet or exception return packet, update
> the previous packet for exception specific flags;
> - If there has TRACE_ON or TRACE_OFF packet in the middle of instruction
> packets, this indicates the trace is discontinuous, so append the flag
> PERF_IP_FLAG_TRACE_END to the previous packet to indicate the trace
> has been ended;
> - If one instruction packet is behind TRACE_OFF packet, this instruction
> is restarting trace packet. So set flag PERF_IP_FLAG_TRACE_START to
> TRACE_OFF packet if one, this flag isn't used by TRACE_OFF packet but
> used to indicate trace restarting when generate sample.
>
> Signed-off-by: Leo Yan <[email protected]>
> ---
> tools/perf/util/cs-etm.c | 43 +++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index 455f132..afca6f3 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c
> @@ -676,7 +676,7 @@ static int cs_etm__synth_instruction_sample(struct cs_etm_queue *etmq,
> sample.stream_id = etmq->etm->instructions_id;
> sample.period = period;
> sample.cpu = etmq->packet->cpu;
> - sample.flags = 0;
> + sample.flags = etmq->prev_packet->flags;
> sample.insn_len = 1;
> sample.cpumode = event->sample.header.misc;
>
> @@ -735,7 +735,7 @@ static int cs_etm__synth_branch_sample(struct cs_etm_queue *etmq)
> sample.stream_id = etmq->etm->branches_id;
> sample.period = 1;
> sample.cpu = etmq->packet->cpu;
> - sample.flags = 0;
> + sample.flags = etmq->prev_packet->flags;
> sample.cpumode = event->sample.header.misc;
>
> /*
> @@ -878,6 +878,43 @@ static int cs_etm__synth_events(struct cs_etm_auxtrace *etm,
> return 0;
> }
>
> +static void cs_etm__fixup_flags(struct cs_etm_queue *etmq)
> +{
> + /*
> + * Decoding stream might insert one TRACE_OFF packet in the
> + * middle of instruction packets, this means it doesn't
> + * contain the pair packets with TRACE_OFF and TRACE_ON.
> + * For this case, the instruction packet follows with
> + * TRACE_OFF packet so we need to fixup prev_packet with flag
> + * PERF_IP_FLAG_TRACE_BEGIN, this flag finally is used by the
> + * instruction packet to generate samples.
> + */
> + if (etmq->prev_packet->sample_type == CS_ETM_TRACE_OFF &&
> + etmq->packet->sample_type == CS_ETM_RANGE)
> + etmq->prev_packet->flags = PERF_IP_FLAG_BRANCH |
> + PERF_IP_FLAG_TRACE_BEGIN;
> +
> + if (etmq->prev_packet->sample_type == CS_ETM_RANGE) {
> + /*
> + * When the exception packet is inserted, update flags
> + * so tell perf it is exception related branches.
> + */
> + if (etmq->packet->sample_type == CS_ETM_EXCEPTION ||
> + etmq->packet->sample_type == CS_ETM_EXCEPTION_RET)
> + etmq->prev_packet->flags = etmq->packet->flags;
> +
> + /*
> + * The trace is discontinuous, weather this is caused by
> + * TRACE_ON packet or TRACE_OFF packet is coming, if the
> + * previous packet is instruction packet, simply set flag
> + * PERF_IP_FLAG_TRACE_END for previous packet.
> + */
> + if (etmq->packet->sample_type == CS_ETM_TRACE_ON ||
> + etmq->packet->sample_type == CS_ETM_TRACE_OFF)
> + etmq->prev_packet->flags |= PERF_IP_FLAG_TRACE_END;
> + }
> +}
> +
I think it would be better to keep all the flag related processing in
cs-etm-decoder.c so that things in cs-etm.c are only concered with dealing with
perf.
Look at function cs_etm__alloc_queue(), there you'll find "d_params.data = etmq".
In function cs_etm_decoder__new(), decoder->data = d_params->data;
This means that anywhere you have a decoder, decoder->data is an etmq. I've
used this profusely in my work on CPU-wide trace scenarios. Because you're
getting there ahead of me you'll need to fix the declaration of struct
cs_etm_queue but that's easy.
Regards,
Mathieu
> static int cs_etm__sample(struct cs_etm_queue *etmq)
> {
> struct cs_etm_auxtrace *etm = etmq->etm;
> @@ -1100,6 +1137,8 @@ static int cs_etm__run_decoder(struct cs_etm_queue *etmq)
> */
> break;
>
> + cs_etm__fixup_flags(etmq);
> +
> switch (etmq->packet->sample_type) {
> case CS_ETM_RANGE:
> /*
> --
> 2.7.4
>
On Mon, 19 Nov 2018 at 16:22, Mathieu Poirier
<[email protected]> wrote:
>
> On Sun, Nov 11, 2018 at 01:07:56PM +0800, Leo Yan wrote:
> > We have prepared the flags in the packet structure, so need to copy
> > the related value into sample structure thus perf tool can facilitate
> > sample flags.
> >
> > The PREV_PACKET contains the branch instruction flags and PACKET
> > actually contains the flags for next branch instruction. So this patch
> > is to set sample flags with 'etmq->prev_packet->flags'.
> >
> > This patch includes three fixing up for sample flags based on the
> > packets context:
> >
> > - If the packet is exception packet or exception return packet, update
> > the previous packet for exception specific flags;
> > - If there has TRACE_ON or TRACE_OFF packet in the middle of instruction
> > packets, this indicates the trace is discontinuous, so append the flag
> > PERF_IP_FLAG_TRACE_END to the previous packet to indicate the trace
> > has been ended;
> > - If one instruction packet is behind TRACE_OFF packet, this instruction
> > is restarting trace packet. So set flag PERF_IP_FLAG_TRACE_START to
> > TRACE_OFF packet if one, this flag isn't used by TRACE_OFF packet but
> > used to indicate trace restarting when generate sample.
> >
> > Signed-off-by: Leo Yan <[email protected]>
> > ---
> > tools/perf/util/cs-etm.c | 43 +++++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 41 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> > index 455f132..afca6f3 100644
> > --- a/tools/perf/util/cs-etm.c
> > +++ b/tools/perf/util/cs-etm.c
> > @@ -676,7 +676,7 @@ static int cs_etm__synth_instruction_sample(struct cs_etm_queue *etmq,
> > sample.stream_id = etmq->etm->instructions_id;
> > sample.period = period;
> > sample.cpu = etmq->packet->cpu;
> > - sample.flags = 0;
> > + sample.flags = etmq->prev_packet->flags;
> > sample.insn_len = 1;
> > sample.cpumode = event->sample.header.misc;
> >
> > @@ -735,7 +735,7 @@ static int cs_etm__synth_branch_sample(struct cs_etm_queue *etmq)
> > sample.stream_id = etmq->etm->branches_id;
> > sample.period = 1;
> > sample.cpu = etmq->packet->cpu;
> > - sample.flags = 0;
> > + sample.flags = etmq->prev_packet->flags;
> > sample.cpumode = event->sample.header.misc;
> >
> > /*
> > @@ -878,6 +878,43 @@ static int cs_etm__synth_events(struct cs_etm_auxtrace *etm,
> > return 0;
> > }
> >
> > +static void cs_etm__fixup_flags(struct cs_etm_queue *etmq)
> > +{
> > + /*
> > + * Decoding stream might insert one TRACE_OFF packet in the
> > + * middle of instruction packets, this means it doesn't
> > + * contain the pair packets with TRACE_OFF and TRACE_ON.
> > + * For this case, the instruction packet follows with
> > + * TRACE_OFF packet so we need to fixup prev_packet with flag
> > + * PERF_IP_FLAG_TRACE_BEGIN, this flag finally is used by the
> > + * instruction packet to generate samples.
> > + */
> > + if (etmq->prev_packet->sample_type == CS_ETM_TRACE_OFF &&
> > + etmq->packet->sample_type == CS_ETM_RANGE)
> > + etmq->prev_packet->flags = PERF_IP_FLAG_BRANCH |
> > + PERF_IP_FLAG_TRACE_BEGIN;
> > +
> > + if (etmq->prev_packet->sample_type == CS_ETM_RANGE) {
> > + /*
> > + * When the exception packet is inserted, update flags
> > + * so tell perf it is exception related branches.
> > + */
> > + if (etmq->packet->sample_type == CS_ETM_EXCEPTION ||
> > + etmq->packet->sample_type == CS_ETM_EXCEPTION_RET)
> > + etmq->prev_packet->flags = etmq->packet->flags;
> > +
> > + /*
> > + * The trace is discontinuous, weather this is caused by
> > + * TRACE_ON packet or TRACE_OFF packet is coming, if the
> > + * previous packet is instruction packet, simply set flag
> > + * PERF_IP_FLAG_TRACE_END for previous packet.
> > + */
> > + if (etmq->packet->sample_type == CS_ETM_TRACE_ON ||
> > + etmq->packet->sample_type == CS_ETM_TRACE_OFF)
> > + etmq->prev_packet->flags |= PERF_IP_FLAG_TRACE_END;
> > + }
> > +}
> > +
>
> I think it would be better to keep all the flag related processing in
> cs-etm-decoder.c so that things in cs-etm.c are only concered with dealing with
> perf.
>
> Look at function cs_etm__alloc_queue(), there you'll find "d_params.data = etmq".
>
> In function cs_etm_decoder__new(), decoder->data = d_params->data;
>
> This means that anywhere you have a decoder, decoder->data is an etmq. I've
> used this profusely in my work on CPU-wide trace scenarios. Because you're
> getting there ahead of me you'll need to fix the declaration of struct
> cs_etm_queue but that's easy.
I've been thinking further about this and manipulating the etmq packet
and prev_packet from the cs-etm-decoder.c won't work because all we
have at that time is the decoder's packet queue. My goal is to
manipulate the flags in only one place - either in cs-etm.c or
cs-etm-decoder.c but not in both. It might be worth trying to do the
implementation in cs-etm.c since there is already a lot of packet flow
intelligence happening there.
>
> Regards,
> Mathieu
>
> > static int cs_etm__sample(struct cs_etm_queue *etmq)
> > {
> > struct cs_etm_auxtrace *etm = etmq->etm;
> > @@ -1100,6 +1137,8 @@ static int cs_etm__run_decoder(struct cs_etm_queue *etmq)
> > */
> > break;
> >
> > + cs_etm__fixup_flags(etmq);
> > +
> > switch (etmq->packet->sample_type) {
> > case CS_ETM_RANGE:
> > /*
> > --
> > 2.7.4
> >
On Mon, Nov 19, 2018 at 03:26:17PM -0700, Mathieu Poirier wrote:
> On Sun, Nov 11, 2018 at 01:07:55PM +0800, Leo Yan wrote:
> > The perf sample data contains flags to indicate the hardware trace data
> > is belonging to which type branch instruction, thus this can be used to
> > print out the human readable string. Arm CoreSight ETM sample data is
> > missed to set flags and it is always set to zeros, this results in perf
> > tool skips to print string for instruction types.
> >
> > Arm CoreSight ETM supports different kinds instruction of A64, A32 and
> > T32; this patch is to set branch instruction flags in packet for these
> > ISAs.
> >
> > The brief idea for patch implementation is describe as below:
> >
> > - For element with OCSD_GEN_TRC_ELEM_TRACE_ON type, it is taken as trace
> > beginning packet; for element with OCSD_GEN_TRC_ELEM_NO_SYNC or
> > OCSD_GEN_TRC_ELEM_EO_TRACE, these two kinds elements are used to set
> > for trace end;
> >
> > As Mike suggested the packet stream might have more than one two
> > TRACE_ON packets, the first one TRACE_ON packet indicates trace end
> > and the second one is taken as trace restarting. We will handle this
> > special case in the upper layer with packet queue handling, which has
> > more context so it's more suitable fix up for it. This will be
> > accomplished in the sequential patch.
> >
> > - For instruction range packet, mainly base on three factors to decide
> > the branch instruction types:
> >
> > elem->last_i_type
> > elem->last_i_subtype
> > elem->last_instr_cond
> >
> > If the instruction is immediate branch but without link and return
> > flag, we consider it as function internal branch; in fact the
> > immediate branch also can be used to invoke the function entry,
> > usually this is only used in assembly code to directly call a symbol
> > and don't expect to return back; after reviewing kernel normal
> > functions and user space programs, both of them are very seldom to use
> > immediate branch for function call. On the other hand, if we want to
> > decide the immediate branch is for function branch jumping or for
> > function calling, we need to rely on the start address of next packet
> > and check the symbol offset for the start address, this will
> > introduce much complexity in the implementation. So for this version
> > we simply consider immediate branch as function internal branch.
> > Moreover, we rely on 'elem->last_instr_cond' to decide if the branch
> > instruction is a conditional branch or not.
> >
> > If the instruction is immediate branch with link, it's instruction
> > 'BL' and which is used for function call.
> >
> > If the instruction is indirect branch and with subtype
> > OCSD_S_INSTR_V7_IMPLIED_RET, the decoders gives the hint the function
> > return for below cases related with A32/T32 instruction; set this
> > branch flag as function return (Thanks for Al's suggestion).
> >
> > BX R14
> > MOV PC, LR
> > POP {…, PC}
> > LDR PC, [SP], #offset
> >
> > If the instruction is indirect branch without link, this is
> > corresponding to instruction 'BR', this instruction usually is used
> > for dynamic link lib with below usage; so we think it's a return
> > instruction.
> >
> > 0000000000000680 <.plt>:
> > 680: a9bf7bf0 stp x16, x30, [sp, #-16]!
> > 684: 90000090 adrp x16, 10000 <__FRAME_END__+0xf630>
> > 688: f947fe11 ldr x17, [x16, #4088]
> > 68c: 913fe210 add x16, x16, #0xff8
> > 690: d61f0220 br x17
> >
> > If the instruction is indirect branch with link, e.g BLR, we think
> > it's a function call.
> >
> > For function return, ARMv8 introduces a dedicated instruction 'ret',
> > which has flag of OCSD_S_INSTR_V8_RET.
> >
> > - For exception packets, this patch divides into three types:
> >
> > The first type of exception is caused by external logics like bus,
> > interrupt controller, debug module or PE reset or halt; this is
> > corresponding to flags "bcyi" which defined in doc perf-script.txt;
> >
> > The second type is for system call, this is set as "bcs" by following
> > definition in the doc;
> >
> > The third type is for CPU trap, data and instruction prefetch abort,
> > alignment abort; usually these exceptions are synchronous for CPU, so
> > set them as "bci" type.
>
> This is too long and needs to be broken down into pieces. I would split this
> patch in 3 heat, one for NO_SYNC and TRACE_ON, one for INSTR_RANGE and one for
> ELEM_EXCEPTION/ELEM_EXCEPTION_RET.
This makes sense, will split to 3 patches.
> >
> > Cc: Mathieu Poirier <[email protected]>
> > Cc: Mike Leach <[email protected]>
> > Cc: Robert Walker <[email protected]>
> > Cc: Al Grant <[email protected]>
> > Cc: Andi Kleen <[email protected]>
> > Cc: Adrian Hunter <[email protected]>
> > Cc: Arnaldo Carvalho de Melo <[email protected]>
> > Signed-off-by: Leo Yan <[email protected]>
> > ---
> > tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 168 ++++++++++++++++++++++++
> > tools/perf/util/cs-etm-decoder/cs-etm-decoder.h | 1 +
> > 2 files changed, 169 insertions(+)
> >
> > 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 d1a6cbc..0e50c52 100644
> > --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> > +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> > @@ -303,6 +303,7 @@ cs_etm_decoder__buffer_packet(struct cs_etm_decoder *decoder,
> > decoder->packet_buffer[et].instr_count = 0;
> > decoder->packet_buffer[et].last_instr_taken_branch = false;
> > decoder->packet_buffer[et].last_instr_size = 0;
> > + decoder->packet_buffer[et].flags = 0;
>
> Since PERF_IP_FLAG_BRANCH is '0', I would set this to UNINT32_MAX.
PERF_IP_FLAG_BRANCH is bit 0 is set (so it's 1) but not 0. If
initialize value to UNINT32_MAX (0xFFFF,FFFF) that means all flags has
been set and this will introduce confusion. So will keep to init
flags to 0.
> >
> > if (decoder->packet_count == MAX_BUFFER - 1)
> > return OCSD_RESP_WAIT;
> > @@ -437,6 +438,171 @@ cs_etm_decoder__buffer_exception_ret(struct cs_etm_decoder *decoder,
> > CS_ETM_EXCEPTION_RET);
> > }
> >
> > +static void cs_etm_decoder__set_sample_flags(
> > + const void *context,
> > + const ocsd_generic_trace_elem *elem)
> > +{
> > + struct cs_etm_decoder *decoder = (struct cs_etm_decoder *) context;
> > + struct cs_etm_packet *packet;
> > + u32 exc_num;
> > +
> > + packet = &decoder->packet_buffer[decoder->tail];
> > +
> > + switch (elem->elem_type) {
> > + case OCSD_GEN_TRC_ELEM_TRACE_ON:
> > + packet->flags = PERF_IP_FLAG_BRANCH |
> > + PERF_IP_FLAG_TRACE_BEGIN;
> > + break;
> > +
> > + case OCSD_GEN_TRC_ELEM_NO_SYNC:
> > + case OCSD_GEN_TRC_ELEM_EO_TRACE:
> > + packet->flags = PERF_IP_FLAG_BRANCH |
> > + PERF_IP_FLAG_TRACE_END;
> > + break;
> > +
> > + case OCSD_GEN_TRC_ELEM_INSTR_RANGE:
> > + /*
> > + * Immediate branch instruction without neither link nor
> > + * return flag, it's normal branch instruction within
> > + * the function.
> > + */
> > + if (elem->last_i_type == OCSD_INSTR_BR &&
> > + elem->last_i_subtype == OCSD_S_INSTR_NONE) {
> > + packet->flags = PERF_IP_FLAG_BRANCH;
> > +
> > + if (elem->last_instr_cond)
> > + packet->flags |= PERF_IP_FLAG_CONDITIONAL;
> > + }
> > +
> > + /*
> > + * Immediate branch instruction with link (e.g. BL), this is
> > + * branch instruction for function call.
> > + */
> > + if (elem->last_i_type == OCSD_INSTR_BR &&
> > + elem->last_i_subtype == OCSD_S_INSTR_BR_LINK)
> > + packet->flags = PERF_IP_FLAG_BRANCH |
> > + PERF_IP_FLAG_CALL;
> > +
> > + /*
> > + * Indirect branch instruction with subtype of
> > + * OCSD_S_INSTR_V7_IMPLIED_RET, this is explicit hint for
> > + * function return for A32/T32.
> > + */
> > + if (elem->last_i_type == OCSD_INSTR_BR_INDIRECT &&
> > + elem->last_i_subtype == OCSD_S_INSTR_V7_IMPLIED_RET)
> > + packet->flags = PERF_IP_FLAG_BRANCH |
> > + PERF_IP_FLAG_RETURN;
> > +
> > + /*
> > + * Indirect branch instruction without link (e.g. BR), usually
> > + * this is used for function return, especially for functions
> > + * within dynamic link lib.
> > + */
> > + if (elem->last_i_type == OCSD_INSTR_BR_INDIRECT &&
> > + elem->last_i_subtype == OCSD_S_INSTR_NONE)
> > + packet->flags = PERF_IP_FLAG_BRANCH |
> > + PERF_IP_FLAG_RETURN;
> > +
> > + /*
> > + * Indirect branch instruction with link (e.g. BLR), this is
> > + * branch instruction for function call.
> > + */
> > + if (elem->last_i_type == OCSD_INSTR_BR_INDIRECT &&
> > + elem->last_i_subtype == OCSD_S_INSTR_BR_LINK)
> > + packet->flags = PERF_IP_FLAG_BRANCH |
> > + PERF_IP_FLAG_CALL;
> > +
> > + /* Return instruction for function return. */
> > + if (elem->last_i_type == OCSD_INSTR_BR_INDIRECT &&
> > + elem->last_i_subtype == OCSD_S_INSTR_V8_RET)
> > + packet->flags = PERF_IP_FLAG_BRANCH |
> > + PERF_IP_FLAG_RETURN;
>
> I would swap the last to if() condition so that the (BRANCH | RETURN) flags
> are all at the same place.
Will follow this in new patch.
> > +
> > + break;
> > +
> > + case OCSD_GEN_TRC_ELEM_EXCEPTION:
> > +
> > +#define OCSD_EXC_RESET 0
> > +#define OCSD_EXC_DEBUG_HALT 1
> > +#define OCSD_EXC_CALL 2
> > +#define OCSD_EXC_TRAP 3
> > +#define OCSD_EXC_SYSTEM_ERROR 4
> > +#define OCSD_EXC_INST_DEBUG 6
> > +#define OCSD_EXC_DATA_DEBUG 7
> > +#define OCSD_EXC_ALIGNMENT 10
> > +#define OCSD_EXC_INST_FAULT 11
> > +#define OCSD_EXC_DATA_FAULT 12
> > +#define OCSD_EXC_IRQ 14
> > +#define OCSD_EXC_FIQ 15
>
> Where did you get the above? To me this is something that should come from the
> library.
The concept is coming from OpenCSD lib but OpenCSD doesn't define
macros for them.
Will polish this and add these macros into OpenCSD header file.
> > +
> > + exc_num = decoder->exc_num[packet->cpu];
> > +
> > + /*
> > + * The exceptions are triggered by external signals
> > + * from bus, interrupt controller, debug module,
> > + * PE reset or halt.
> > + */
> > + if (exc_num == OCSD_EXC_RESET ||
> > + exc_num == OCSD_EXC_DEBUG_HALT ||
> > + exc_num == OCSD_EXC_SYSTEM_ERROR ||
> > + exc_num == OCSD_EXC_INST_DEBUG ||
> > + exc_num == OCSD_EXC_DATA_DEBUG ||
> > + exc_num == OCSD_EXC_IRQ ||
> > + exc_num == OCSD_EXC_FIQ)
> > + packet->flags = PERF_IP_FLAG_BRANCH |
> > + PERF_IP_FLAG_CALL |
> > + PERF_IP_FLAG_ASYNC |
> > + PERF_IP_FLAG_INTERRUPT;
> > +
> > + /* The exception is for system call. */
> > + if (exc_num == OCSD_EXC_CALL)
> > + packet->flags = PERF_IP_FLAG_BRANCH |
> > + PERF_IP_FLAG_CALL |
> > + PERF_IP_FLAG_SYSCALLRET;
> > +
> > + /*
> > + * The exception is introduced by trap, instruction &
> > + * data fault or alignment errors.
> > + */
> > + if (exc_num == OCSD_EXC_TRAP ||
> > + exc_num == OCSD_EXC_ALIGNMENT ||
> > + exc_num == OCSD_EXC_INST_FAULT ||
> > + exc_num == OCSD_EXC_DATA_FAULT)
> > + packet->flags = PERF_IP_FLAG_BRANCH |
> > + PERF_IP_FLAG_CALL |
> > + PERF_IP_FLAG_INTERRUPT;
> > +
> > + break;
> > +
> > + case OCSD_GEN_TRC_ELEM_EXCEPTION_RET:
> > +
> > + exc_num = decoder->exc_num[packet->cpu];
> > +
> > + if (exc_num == OCSD_EXC_CALL)
> > + packet->flags = PERF_IP_FLAG_BRANCH |
> > + PERF_IP_FLAG_RETURN |
> > + PERF_IP_FLAG_SYSCALLRET;
> > + else
> > + packet->flags = PERF_IP_FLAG_BRANCH |
> > + PERF_IP_FLAG_RETURN |
> > + PERF_IP_FLAG_INTERRUPT;
> > +
> > + break;
> > +
> > + case OCSD_GEN_TRC_ELEM_UNKNOWN:
> > + case OCSD_GEN_TRC_ELEM_PE_CONTEXT:
> > + case OCSD_GEN_TRC_ELEM_ADDR_NACC:
> > + case OCSD_GEN_TRC_ELEM_TIMESTAMP:
> > + case OCSD_GEN_TRC_ELEM_CYCLE_COUNT:
> > + case OCSD_GEN_TRC_ELEM_ADDR_UNKNOWN:
> > + case OCSD_GEN_TRC_ELEM_EVENT:
> > + case OCSD_GEN_TRC_ELEM_SWTRACE:
> > + case OCSD_GEN_TRC_ELEM_CUSTOM:
> > + default:
> > + break;
> > + }
> > +}
> > +
> > static ocsd_datapath_resp_t cs_etm_decoder__gen_trace_elem_printer(
> > const void *context,
> > const ocsd_trc_index_t indx __maybe_unused,
> > @@ -484,6 +650,8 @@ static ocsd_datapath_resp_t cs_etm_decoder__gen_trace_elem_printer(
> > break;
> > }
> >
> > + cs_etm_decoder__set_sample_flags(context, elem);
> > +
>
> I was toying with the idea of setting the flags in each of the case statement
> found in cs_etm_decoder__gen_trace_elem_printer(). But that would move more
> code around and the end result would be the same so let's keep it that way until
> we have a good reason to split it.
Do you sugguest to keep current implementation rather than to
split flags setting in each of the case statement in
cs_etm_decoder__gen_trace_elem_printer()?
I am not 100% sure if I understand correctly for "split it" (split flags
setting vs split functions). So please correct me if I misunderstand
this.
Thanks,
Leo Yan
> > return resp;
> > }
> >
> > 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 0d1c18d..71df908 100644
> > --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h
> > +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h
> > @@ -47,6 +47,7 @@ struct cs_etm_packet {
> > u8 last_instr_taken_branch;
> > u8 last_instr_size;
> > int cpu;
> > + u32 flags;
> > };
> >
> > struct cs_etm_queue;
> > --
> > 2.7.4
> >
On Tue, Nov 20, 2018 at 09:53:41AM -0700, Mathieu Poirier wrote:
[...]
> > > +static void cs_etm__fixup_flags(struct cs_etm_queue *etmq)
> > > +{
> > > + /*
> > > + * Decoding stream might insert one TRACE_OFF packet in the
> > > + * middle of instruction packets, this means it doesn't
> > > + * contain the pair packets with TRACE_OFF and TRACE_ON.
> > > + * For this case, the instruction packet follows with
> > > + * TRACE_OFF packet so we need to fixup prev_packet with flag
> > > + * PERF_IP_FLAG_TRACE_BEGIN, this flag finally is used by the
> > > + * instruction packet to generate samples.
> > > + */
> > > + if (etmq->prev_packet->sample_type == CS_ETM_TRACE_OFF &&
> > > + etmq->packet->sample_type == CS_ETM_RANGE)
> > > + etmq->prev_packet->flags = PERF_IP_FLAG_BRANCH |
> > > + PERF_IP_FLAG_TRACE_BEGIN;
> > > +
> > > + if (etmq->prev_packet->sample_type == CS_ETM_RANGE) {
> > > + /*
> > > + * When the exception packet is inserted, update flags
> > > + * so tell perf it is exception related branches.
> > > + */
> > > + if (etmq->packet->sample_type == CS_ETM_EXCEPTION ||
> > > + etmq->packet->sample_type == CS_ETM_EXCEPTION_RET)
> > > + etmq->prev_packet->flags = etmq->packet->flags;
> > > +
> > > + /*
> > > + * The trace is discontinuous, weather this is caused by
> > > + * TRACE_ON packet or TRACE_OFF packet is coming, if the
> > > + * previous packet is instruction packet, simply set flag
> > > + * PERF_IP_FLAG_TRACE_END for previous packet.
> > > + */
> > > + if (etmq->packet->sample_type == CS_ETM_TRACE_ON ||
> > > + etmq->packet->sample_type == CS_ETM_TRACE_OFF)
> > > + etmq->prev_packet->flags |= PERF_IP_FLAG_TRACE_END;
> > > + }
> > > +}
> > > +
> >
> > I think it would be better to keep all the flag related processing in
> > cs-etm-decoder.c so that things in cs-etm.c are only concered with dealing with
> > perf.
> >
> > Look at function cs_etm__alloc_queue(), there you'll find "d_params.data = etmq".
> >
> > In function cs_etm_decoder__new(), decoder->data = d_params->data;
> >
> > This means that anywhere you have a decoder, decoder->data is an etmq. I've
> > used this profusely in my work on CPU-wide trace scenarios. Because you're
> > getting there ahead of me you'll need to fix the declaration of struct
> > cs_etm_queue but that's easy.
>
> I've been thinking further about this and manipulating the etmq packet
> and prev_packet from the cs-etm-decoder.c won't work because all we
> have at that time is the decoder's packet queue. My goal is to
> manipulate the flags in only one place - either in cs-etm.c or
> cs-etm-decoder.c but not in both. It might be worth trying to do the
> implementation in cs-etm.c since there is already a lot of packet flow
> intelligence happening there.
Agree. cs-etm.c has more context info than cs-etm-decoder.c, will
try to refactor in single place in cs-etm.c.
[...]
Thanks,
Leo Yan
On Tue, 4 Dec 2018 at 23:26, <[email protected]> wrote:
>
> On Mon, Nov 19, 2018 at 03:26:17PM -0700, Mathieu Poirier wrote:
> > On Sun, Nov 11, 2018 at 01:07:55PM +0800, Leo Yan wrote:
> > > The perf sample data contains flags to indicate the hardware trace data
> > > is belonging to which type branch instruction, thus this can be used to
> > > print out the human readable string. Arm CoreSight ETM sample data is
> > > missed to set flags and it is always set to zeros, this results in perf
> > > tool skips to print string for instruction types.
> > >
> > > Arm CoreSight ETM supports different kinds instruction of A64, A32 and
> > > T32; this patch is to set branch instruction flags in packet for these
> > > ISAs.
> > >
> > > The brief idea for patch implementation is describe as below:
> > >
> > > - For element with OCSD_GEN_TRC_ELEM_TRACE_ON type, it is taken as trace
> > > beginning packet; for element with OCSD_GEN_TRC_ELEM_NO_SYNC or
> > > OCSD_GEN_TRC_ELEM_EO_TRACE, these two kinds elements are used to set
> > > for trace end;
> > >
> > > As Mike suggested the packet stream might have more than one two
> > > TRACE_ON packets, the first one TRACE_ON packet indicates trace end
> > > and the second one is taken as trace restarting. We will handle this
> > > special case in the upper layer with packet queue handling, which has
> > > more context so it's more suitable fix up for it. This will be
> > > accomplished in the sequential patch.
> > >
> > > - For instruction range packet, mainly base on three factors to decide
> > > the branch instruction types:
> > >
> > > elem->last_i_type
> > > elem->last_i_subtype
> > > elem->last_instr_cond
> > >
> > > If the instruction is immediate branch but without link and return
> > > flag, we consider it as function internal branch; in fact the
> > > immediate branch also can be used to invoke the function entry,
> > > usually this is only used in assembly code to directly call a symbol
> > > and don't expect to return back; after reviewing kernel normal
> > > functions and user space programs, both of them are very seldom to use
> > > immediate branch for function call. On the other hand, if we want to
> > > decide the immediate branch is for function branch jumping or for
> > > function calling, we need to rely on the start address of next packet
> > > and check the symbol offset for the start address, this will
> > > introduce much complexity in the implementation. So for this version
> > > we simply consider immediate branch as function internal branch.
> > > Moreover, we rely on 'elem->last_instr_cond' to decide if the branch
> > > instruction is a conditional branch or not.
> > >
> > > If the instruction is immediate branch with link, it's instruction
> > > 'BL' and which is used for function call.
> > >
> > > If the instruction is indirect branch and with subtype
> > > OCSD_S_INSTR_V7_IMPLIED_RET, the decoders gives the hint the function
> > > return for below cases related with A32/T32 instruction; set this
> > > branch flag as function return (Thanks for Al's suggestion).
> > >
> > > BX R14
> > > MOV PC, LR
> > > POP {…, PC}
> > > LDR PC, [SP], #offset
> > >
> > > If the instruction is indirect branch without link, this is
> > > corresponding to instruction 'BR', this instruction usually is used
> > > for dynamic link lib with below usage; so we think it's a return
> > > instruction.
> > >
> > > 0000000000000680 <.plt>:
> > > 680: a9bf7bf0 stp x16, x30, [sp, #-16]!
> > > 684: 90000090 adrp x16, 10000 <__FRAME_END__+0xf630>
> > > 688: f947fe11 ldr x17, [x16, #4088]
> > > 68c: 913fe210 add x16, x16, #0xff8
> > > 690: d61f0220 br x17
> > >
> > > If the instruction is indirect branch with link, e.g BLR, we think
> > > it's a function call.
> > >
> > > For function return, ARMv8 introduces a dedicated instruction 'ret',
> > > which has flag of OCSD_S_INSTR_V8_RET.
> > >
> > > - For exception packets, this patch divides into three types:
> > >
> > > The first type of exception is caused by external logics like bus,
> > > interrupt controller, debug module or PE reset or halt; this is
> > > corresponding to flags "bcyi" which defined in doc perf-script.txt;
> > >
> > > The second type is for system call, this is set as "bcs" by following
> > > definition in the doc;
> > >
> > > The third type is for CPU trap, data and instruction prefetch abort,
> > > alignment abort; usually these exceptions are synchronous for CPU, so
> > > set them as "bci" type.
> >
> > This is too long and needs to be broken down into pieces. I would split this
> > patch in 3 heat, one for NO_SYNC and TRACE_ON, one for INSTR_RANGE and one for
> > ELEM_EXCEPTION/ELEM_EXCEPTION_RET.
>
> This makes sense, will split to 3 patches.
>
> > >
> > > Cc: Mathieu Poirier <[email protected]>
> > > Cc: Mike Leach <[email protected]>
> > > Cc: Robert Walker <[email protected]>
> > > Cc: Al Grant <[email protected]>
> > > Cc: Andi Kleen <[email protected]>
> > > Cc: Adrian Hunter <[email protected]>
> > > Cc: Arnaldo Carvalho de Melo <[email protected]>
> > > Signed-off-by: Leo Yan <[email protected]>
> > > ---
> > > tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 168 ++++++++++++++++++++++++
> > > tools/perf/util/cs-etm-decoder/cs-etm-decoder.h | 1 +
> > > 2 files changed, 169 insertions(+)
> > >
> > > 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 d1a6cbc..0e50c52 100644
> > > --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> > > +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> > > @@ -303,6 +303,7 @@ cs_etm_decoder__buffer_packet(struct cs_etm_decoder *decoder,
> > > decoder->packet_buffer[et].instr_count = 0;
> > > decoder->packet_buffer[et].last_instr_taken_branch = false;
> > > decoder->packet_buffer[et].last_instr_size = 0;
> > > + decoder->packet_buffer[et].flags = 0;
> >
> > Since PERF_IP_FLAG_BRANCH is '0', I would set this to UNINT32_MAX.
>
> PERF_IP_FLAG_BRANCH is bit 0 is set (so it's 1) but not 0. If
> initialize value to UNINT32_MAX (0xFFFF,FFFF) that means all flags has
> been set and this will introduce confusion. So will keep to init
> flags to 0.
I just looked at this again - you are correct.
>
> > >
> > > if (decoder->packet_count == MAX_BUFFER - 1)
> > > return OCSD_RESP_WAIT;
> > > @@ -437,6 +438,171 @@ cs_etm_decoder__buffer_exception_ret(struct cs_etm_decoder *decoder,
> > > CS_ETM_EXCEPTION_RET);
> > > }
> > >
> > > +static void cs_etm_decoder__set_sample_flags(
> > > + const void *context,
> > > + const ocsd_generic_trace_elem *elem)
> > > +{
> > > + struct cs_etm_decoder *decoder = (struct cs_etm_decoder *) context;
> > > + struct cs_etm_packet *packet;
> > > + u32 exc_num;
> > > +
> > > + packet = &decoder->packet_buffer[decoder->tail];
> > > +
> > > + switch (elem->elem_type) {
> > > + case OCSD_GEN_TRC_ELEM_TRACE_ON:
> > > + packet->flags = PERF_IP_FLAG_BRANCH |
> > > + PERF_IP_FLAG_TRACE_BEGIN;
> > > + break;
> > > +
> > > + case OCSD_GEN_TRC_ELEM_NO_SYNC:
> > > + case OCSD_GEN_TRC_ELEM_EO_TRACE:
> > > + packet->flags = PERF_IP_FLAG_BRANCH |
> > > + PERF_IP_FLAG_TRACE_END;
> > > + break;
> > > +
> > > + case OCSD_GEN_TRC_ELEM_INSTR_RANGE:
> > > + /*
> > > + * Immediate branch instruction without neither link nor
> > > + * return flag, it's normal branch instruction within
> > > + * the function.
> > > + */
> > > + if (elem->last_i_type == OCSD_INSTR_BR &&
> > > + elem->last_i_subtype == OCSD_S_INSTR_NONE) {
> > > + packet->flags = PERF_IP_FLAG_BRANCH;
> > > +
> > > + if (elem->last_instr_cond)
> > > + packet->flags |= PERF_IP_FLAG_CONDITIONAL;
> > > + }
> > > +
> > > + /*
> > > + * Immediate branch instruction with link (e.g. BL), this is
> > > + * branch instruction for function call.
> > > + */
> > > + if (elem->last_i_type == OCSD_INSTR_BR &&
> > > + elem->last_i_subtype == OCSD_S_INSTR_BR_LINK)
> > > + packet->flags = PERF_IP_FLAG_BRANCH |
> > > + PERF_IP_FLAG_CALL;
> > > +
> > > + /*
> > > + * Indirect branch instruction with subtype of
> > > + * OCSD_S_INSTR_V7_IMPLIED_RET, this is explicit hint for
> > > + * function return for A32/T32.
> > > + */
> > > + if (elem->last_i_type == OCSD_INSTR_BR_INDIRECT &&
> > > + elem->last_i_subtype == OCSD_S_INSTR_V7_IMPLIED_RET)
> > > + packet->flags = PERF_IP_FLAG_BRANCH |
> > > + PERF_IP_FLAG_RETURN;
> > > +
> > > + /*
> > > + * Indirect branch instruction without link (e.g. BR), usually
> > > + * this is used for function return, especially for functions
> > > + * within dynamic link lib.
> > > + */
> > > + if (elem->last_i_type == OCSD_INSTR_BR_INDIRECT &&
> > > + elem->last_i_subtype == OCSD_S_INSTR_NONE)
> > > + packet->flags = PERF_IP_FLAG_BRANCH |
> > > + PERF_IP_FLAG_RETURN;
> > > +
> > > + /*
> > > + * Indirect branch instruction with link (e.g. BLR), this is
> > > + * branch instruction for function call.
> > > + */
> > > + if (elem->last_i_type == OCSD_INSTR_BR_INDIRECT &&
> > > + elem->last_i_subtype == OCSD_S_INSTR_BR_LINK)
> > > + packet->flags = PERF_IP_FLAG_BRANCH |
> > > + PERF_IP_FLAG_CALL;
> > > +
> > > + /* Return instruction for function return. */
> > > + if (elem->last_i_type == OCSD_INSTR_BR_INDIRECT &&
> > > + elem->last_i_subtype == OCSD_S_INSTR_V8_RET)
> > > + packet->flags = PERF_IP_FLAG_BRANCH |
> > > + PERF_IP_FLAG_RETURN;
> >
> > I would swap the last to if() condition so that the (BRANCH | RETURN) flags
> > are all at the same place.
>
> Will follow this in new patch.
>
> > > +
> > > + break;
> > > +
> > > + case OCSD_GEN_TRC_ELEM_EXCEPTION:
> > > +
> > > +#define OCSD_EXC_RESET 0
> > > +#define OCSD_EXC_DEBUG_HALT 1
> > > +#define OCSD_EXC_CALL 2
> > > +#define OCSD_EXC_TRAP 3
> > > +#define OCSD_EXC_SYSTEM_ERROR 4
> > > +#define OCSD_EXC_INST_DEBUG 6
> > > +#define OCSD_EXC_DATA_DEBUG 7
> > > +#define OCSD_EXC_ALIGNMENT 10
> > > +#define OCSD_EXC_INST_FAULT 11
> > > +#define OCSD_EXC_DATA_FAULT 12
> > > +#define OCSD_EXC_IRQ 14
> > > +#define OCSD_EXC_FIQ 15
> >
> > Where did you get the above? To me this is something that should come from the
> > library.
>
> The concept is coming from OpenCSD lib but OpenCSD doesn't define
> macros for them.
>
> Will polish this and add these macros into OpenCSD header file.
>
> > > +
> > > + exc_num = decoder->exc_num[packet->cpu];
> > > +
> > > + /*
> > > + * The exceptions are triggered by external signals
> > > + * from bus, interrupt controller, debug module,
> > > + * PE reset or halt.
> > > + */
> > > + if (exc_num == OCSD_EXC_RESET ||
> > > + exc_num == OCSD_EXC_DEBUG_HALT ||
> > > + exc_num == OCSD_EXC_SYSTEM_ERROR ||
> > > + exc_num == OCSD_EXC_INST_DEBUG ||
> > > + exc_num == OCSD_EXC_DATA_DEBUG ||
> > > + exc_num == OCSD_EXC_IRQ ||
> > > + exc_num == OCSD_EXC_FIQ)
> > > + packet->flags = PERF_IP_FLAG_BRANCH |
> > > + PERF_IP_FLAG_CALL |
> > > + PERF_IP_FLAG_ASYNC |
> > > + PERF_IP_FLAG_INTERRUPT;
> > > +
> > > + /* The exception is for system call. */
> > > + if (exc_num == OCSD_EXC_CALL)
> > > + packet->flags = PERF_IP_FLAG_BRANCH |
> > > + PERF_IP_FLAG_CALL |
> > > + PERF_IP_FLAG_SYSCALLRET;
> > > +
> > > + /*
> > > + * The exception is introduced by trap, instruction &
> > > + * data fault or alignment errors.
> > > + */
> > > + if (exc_num == OCSD_EXC_TRAP ||
> > > + exc_num == OCSD_EXC_ALIGNMENT ||
> > > + exc_num == OCSD_EXC_INST_FAULT ||
> > > + exc_num == OCSD_EXC_DATA_FAULT)
> > > + packet->flags = PERF_IP_FLAG_BRANCH |
> > > + PERF_IP_FLAG_CALL |
> > > + PERF_IP_FLAG_INTERRUPT;
> > > +
> > > + break;
> > > +
> > > + case OCSD_GEN_TRC_ELEM_EXCEPTION_RET:
> > > +
> > > + exc_num = decoder->exc_num[packet->cpu];
> > > +
> > > + if (exc_num == OCSD_EXC_CALL)
> > > + packet->flags = PERF_IP_FLAG_BRANCH |
> > > + PERF_IP_FLAG_RETURN |
> > > + PERF_IP_FLAG_SYSCALLRET;
> > > + else
> > > + packet->flags = PERF_IP_FLAG_BRANCH |
> > > + PERF_IP_FLAG_RETURN |
> > > + PERF_IP_FLAG_INTERRUPT;
> > > +
> > > + break;
> > > +
> > > + case OCSD_GEN_TRC_ELEM_UNKNOWN:
> > > + case OCSD_GEN_TRC_ELEM_PE_CONTEXT:
> > > + case OCSD_GEN_TRC_ELEM_ADDR_NACC:
> > > + case OCSD_GEN_TRC_ELEM_TIMESTAMP:
> > > + case OCSD_GEN_TRC_ELEM_CYCLE_COUNT:
> > > + case OCSD_GEN_TRC_ELEM_ADDR_UNKNOWN:
> > > + case OCSD_GEN_TRC_ELEM_EVENT:
> > > + case OCSD_GEN_TRC_ELEM_SWTRACE:
> > > + case OCSD_GEN_TRC_ELEM_CUSTOM:
> > > + default:
> > > + break;
> > > + }
> > > +}
> > > +
> > > static ocsd_datapath_resp_t cs_etm_decoder__gen_trace_elem_printer(
> > > const void *context,
> > > const ocsd_trc_index_t indx __maybe_unused,
> > > @@ -484,6 +650,8 @@ static ocsd_datapath_resp_t cs_etm_decoder__gen_trace_elem_printer(
> > > break;
> > > }
> > >
> > > + cs_etm_decoder__set_sample_flags(context, elem);
> > > +
> >
> > I was toying with the idea of setting the flags in each of the case statement
> > found in cs_etm_decoder__gen_trace_elem_printer(). But that would move more
> > code around and the end result would be the same so let's keep it that way until
> > we have a good reason to split it.
>
> Do you sugguest to keep current implementation rather than to
> split flags setting in each of the case statement in
> cs_etm_decoder__gen_trace_elem_printer()?
>
> I am not 100% sure if I understand correctly for "split it" (split flags
> setting vs split functions). So please correct me if I misunderstand
> this.
I find function cs_etm_decoder__set_sample_flags() overly long. Since
the case statements in it are the same as the ones in
cs_etm_decoder__gen_trace_elem_printer() a different way to proceed
would be to do flag setting there rather than all in
cs_etm_decoder__set_sample_flags(). But that would introduce more
code modification and tighter coupling. Since I don't have another
alternative I am suggesting to keep the current implementation.
>
> Thanks,
> Leo Yan
>
> > > return resp;
> > > }
> > >
> > > 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 0d1c18d..71df908 100644
> > > --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h
> > > +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h
> > > @@ -47,6 +47,7 @@ struct cs_etm_packet {
> > > u8 last_instr_taken_branch;
> > > u8 last_instr_size;
> > > int cpu;
> > > + u32 flags;
> > > };
> > >
> > > struct cs_etm_queue;
> > > --
> > > 2.7.4
> > >
On Wed, Dec 05, 2018 at 10:40:07AM -0700, Mathieu Poirier wrote:
[...]
> > > > static ocsd_datapath_resp_t cs_etm_decoder__gen_trace_elem_printer(
> > > > const void *context,
> > > > const ocsd_trc_index_t indx __maybe_unused,
> > > > @@ -484,6 +650,8 @@ static ocsd_datapath_resp_t cs_etm_decoder__gen_trace_elem_printer(
> > > > break;
> > > > }
> > > >
> > > > + cs_etm_decoder__set_sample_flags(context, elem);
> > > > +
> > >
> > > I was toying with the idea of setting the flags in each of the case statement
> > > found in cs_etm_decoder__gen_trace_elem_printer(). But that would move more
> > > code around and the end result would be the same so let's keep it that way until
> > > we have a good reason to split it.
> >
> > Do you sugguest to keep current implementation rather than to
> > split flags setting in each of the case statement in
> > cs_etm_decoder__gen_trace_elem_printer()?
> >
> > I am not 100% sure if I understand correctly for "split it" (split flags
> > setting vs split functions). So please correct me if I misunderstand
> > this.
>
> I find function cs_etm_decoder__set_sample_flags() overly long. Since
> the case statements in it are the same as the ones in
> cs_etm_decoder__gen_trace_elem_printer() a different way to proceed
> would be to do flag setting there rather than all in
> cs_etm_decoder__set_sample_flags(). But that would introduce more
> code modification and tighter coupling. Since I don't have another
> alternative I am suggesting to keep the current implementation.
Thanks for clarification.