On Wed, Jan 09, 2019 at 07:37:00PM +0800, Leo Yan wrote:

Decoder provides last instruction related information, these information can be used for trace analysis; specifically we can get to know what kind of branch instruction has been executed, mainly the information are contained in three element fields:

last_i_type: this is significant type for waypoint calculation, it indicates the last instruction is one of immediate branch instruction, indirect branch instruction, instruction barrier (ISB), or data barrier (DSB/DMB).

last_i_subtype: this is used for instruction sub type, it can be branch with link, ARMv8 return instruction, ARMv8 eret instruction (return from exception), or ARMv7 instruction which could imply return (e.g. MOV PC, LR; POP { ,PC}).

last_instr_cond: it indicates if the last instruction was conditional.

But these three fields are not saved into cs_etm_packet struct, thus cs-etm layer don't know related information and cannot generate sample flags for branch instructions.

This patch add corresponding three new fields in cs_etm_packet struct and save related value into the packet structure, it is preparation for supporting sample flags.

Signed-off-by: Leo Yan leo.yan@linaro.org

tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 9 +++++++++ tools/perf/util/cs-etm-decoder/cs-etm-decoder.h | 3 +++ 2 files changed, 12 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 8c155575c6c5..8a19310500d9 100644 --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c @@ -290,6 +290,9 @@ static void cs_etm_decoder__clear_buffer(struct cs_etm_decoder *decoder) decoder->packet_buffer[i].instr_count = 0; decoder->packet_buffer[i].last_instr_taken_branch = false; decoder->packet_buffer[i].last_instr_size = 0;

• decoder->packet_buffer[i].last_instr_type = 0;
  

• decoder->packet_buffer[i].last_instr_subtype = 0;
  

• decoder->packet_buffer[i].last_instr_cond = 0;
  

  decoder->packet_buffer[i].cpu = INT_MIN; }

} @@ -323,6 +326,9 @@ 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].last_instr_type = 0;
• decoder->packet_buffer[et].last_instr_subtype = 0;
• decoder->packet_buffer[et].last_instr_cond = 0;

if (decoder->packet_count == MAX_BUFFER - 1) return OCSD_RESP_WAIT; @@ -366,6 +372,9 @@ cs_etm_decoder__buffer_range(struct cs_etm_decoder *decoder, packet->start_addr = elem->st_addr; packet->end_addr = elem->en_addr; packet->instr_count = elem->num_instr_range;

• packet->last_instr_type = elem->last_i_type;
• packet->last_instr_subtype = elem->last_i_subtype;
• packet->last_instr_cond = elem->last_instr_cond;

switch (elem->last_i_type) { case OCSD_INSTR_BR: 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 a6407d41598f..7cdd6a9c68a7 100644 --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h @@ -43,6 +43,9 @@ struct cs_etm_packet { u64 start_addr; u64 end_addr; u32 instr_count;

• u32 last_instr_type;
• u32 last_instr_subtype;
• u8 last_instr_cond; u8 last_instr_taken_branch; u8 last_instr_size; int cpu;

-- 2.17.1

Now that this set is starting to take shape I will use the normal tags so that I don't end up reviewing patches twice. Since patches that end up on the public mailing list with internal review tags are not well received, you will need to remove them before going public (when the time comes). Let me know if I am not expressing myself clearly.

Reviewed-by: Mathieu Poirier mathieu.poirier@linaro.org

On Wed, Jan 09, 2019 at 07:37:01PM +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.

This patch is to set branch instruction flags for instruction range packet.

Signed-off-by: Leo Yan leo.yan@linaro.org

.../perf/util/cs-etm-decoder/cs-etm-decoder.h | 1 + tools/perf/util/cs-etm.c | 92 ++++++++++++++++++- 2 files changed, 91 insertions(+), 2 deletions(-)

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 7cdd6a9c68a7..23600e57a215 100644 --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h @@ -45,6 +45,7 @@ struct cs_etm_packet { u32 instr_count; u32 last_instr_type; u32 last_instr_subtype;

• u32 flags; u8 last_instr_cond; u8 last_instr_taken_branch; u8 last_instr_size;

diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index 27a374ddf661..a30506d54be0 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -13,6 +13,7 @@ #include <linux/types.h> #include <stdlib.h> +#include <opencsd/ocsd_if_types.h>

Alphabetical order. This is very pedantic but if I let it go here someone else on the public mailing list will call you out on it.

#include "auxtrace.h" #include "color.h" @@ -719,7 +720,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;

@@ -778,7 +779,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;

/* @@ -1107,6 +1108,82 @@ static int cs_etm__end_block(struct cs_etm_queue *etmq) return 0; } +static int cs_etm__set_sample_flags(struct cs_etm_queue *etmq) +{

• struct cs_etm_packet *packet = etmq->packet;
• packet->flags = 0;

Why is this here? Can't the flag initialisation be done in cs_etm_decoder__buffer_packet() and cs_etm_decoder__clear_buffer() like the reset of the other members of struct cs_etm_packet?

• switch (packet->sample_type) {
• case CS_ETM_RANGE:

• /*
  

•  * Immediate branch instruction without neither link nor
  

•  * return flag, it's normal branch instruction within
  

•  * the function.
  

•  */
  

• if (packet->last_instr_type == OCSD_INSTR_BR &&
  

•     packet->last_instr_subtype == OCSD_S_INSTR_NONE) {
  

• 	packet->flags = PERF_IP_FLAG_BRANCH;
  

• 	if (packet->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 (packet->last_instr_type == OCSD_INSTR_BR &&
  

•     packet->last_instr_subtype == OCSD_S_INSTR_BR_LINK)
  

• 	packet->flags = PERF_IP_FLAG_BRANCH |
  

• 			PERF_IP_FLAG_CALL;
  

• /*
  

•  * Indirect branch instruction with link (e.g. BLR), this is
  

•  * branch instruction for function call.
  

•  */
  

• if (packet->last_instr_type == OCSD_INSTR_BR_INDIRECT &&
  

•     packet->last_instr_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 (packet->last_instr_type == OCSD_INSTR_BR_INDIRECT &&
  

•     packet->last_instr_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 (packet->last_instr_type == OCSD_INSTR_BR_INDIRECT &&
  

•     packet->last_instr_subtype == OCSD_S_INSTR_NONE)
  

• 	packet->flags = PERF_IP_FLAG_BRANCH |
  

• 			PERF_IP_FLAG_RETURN;
  

• /* Return instruction for function return. */
  

• if (packet->last_instr_type == OCSD_INSTR_BR_INDIRECT &&
  

•     packet->last_instr_subtype == OCSD_S_INSTR_V8_RET)
  

• 	packet->flags = PERF_IP_FLAG_BRANCH |
  

• 			PERF_IP_FLAG_RETURN;
  

• break;
  

• case CS_ETM_DISCONTINUITY:
• case CS_ETM_EXCEPTION:
• case CS_ETM_EXCEPTION_RET:
• case CS_ETM_EMPTY:
• default:

• break;
  

• }

This switch() statement looks much better now - thanks for fixing it.

• return 0;

+}

static int cs_etm__run_decoder(struct cs_etm_queue *etmq) { struct cs_etm_auxtrace *etm = etmq->etm; @@ -1158,6 +1235,17 @@ static int cs_etm__run_decoder(struct cs_etm_queue *etmq) */ break;

• 		/*
  

• 		 * Since packet addresses are swaped in packet
  

s/swaped/swapped

• 		 * handling within below switch() statements,
  

• 		 * thus setting sample flags must be called
  

• 		 * prior to switch() statement to use address
  

• 		 * information before packets swapping.
  

• 		 */
  

• 		err = cs_etm__set_sample_flags(etmq);
  

• 		if (err < 0)
  

• 			break;
  

• 	switch (etmq->packet->sample_type) {
  	case CS_ETM_RANGE:
  		/*
  

With the above changes:

Reviewed-by: Mathieu Poirier mathieu.poirier@linaro.org

-- 2.17.1

On Thu, Jan 10, 2019 at 03:40:52PM -0700, Mathieu Poirier wrote:

On Wed, Jan 09, 2019 at 07:37:03PM +0800, Leo Yan wrote:

...

When an exception packet comes, it contains the information 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 exception return packet doesn't contain a valid exception number, so we cannot use the exception number for exception return packet and need to explore other methods to decide exception types for return packet.

The above paragraph is not needed to understand what is going on in this patch.

Will remove it.

...

This patch simply adds a new 'exc_num' field in cs_etm_packet struct,

'exc_num' doesn't exist in struct cs_etm_packet - you called it "exception_number". Moreover there is no point in being this precise in the changelog, all you need to do is mention that you're adding a field in the cs_etm_packet struct.

...

it records exception number for exception packet.

... it records exception number for exception packet that will then be used to properly identify exception types to the perf synthesize mechanic.

Thanks for rephrasing. Will refine for commit log.

...

Signed-off-by: Leo Yan leo.yan@linaro.org

.../perf/util/cs-etm-decoder/cs-etm-decoder.c | 20 +++++++++++++++---- .../perf/util/cs-etm-decoder/cs-etm-decoder.h | 1 + 2 files changed, 17 insertions(+), 4 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 8a19310500d9..05830fe1fb70 100644 --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c @@ -293,6 +293,7 @@ static void cs_etm_decoder__clear_buffer(struct cs_etm_decoder *decoder) decoder->packet_buffer[i].last_instr_type = 0; decoder->packet_buffer[i].last_instr_subtype = 0; decoder->packet_buffer[i].last_instr_cond = 0;

• decoder->packet_buffer[i].exception_number = UINT32_MAX;
  

  decoder->packet_buffer[i].cpu = INT_MIN; }

} @@ -329,6 +330,7 @@ cs_etm_decoder__buffer_packet(struct cs_etm_decoder *decoder, decoder->packet_buffer[et].last_instr_type = 0; decoder->packet_buffer[et].last_instr_subtype = 0; decoder->packet_buffer[et].last_instr_cond = 0;

• decoder->packet_buffer[et].exception_number = UINT32_MAX;

if (decoder->packet_count == MAX_BUFFER - 1) return OCSD_RESP_WAIT; @@ -404,10 +406,20 @@ cs_etm_decoder__buffer_discontinuity(struct cs_etm_decoder *decoder, 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 = 0;

• 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];
• packet->exception_number = elem->exception_number;
• return ret;

} static ocsd_datapath_resp_t @@ -441,7 +453,7 @@ static ocsd_datapath_resp_t cs_etm_decoder__gen_trace_elem_printer( trace_chan_id); break; case OCSD_GEN_TRC_ELEM_EXCEPTION:

• resp = cs_etm_decoder__buffer_exception(decoder,
  

• resp = cs_etm_decoder__buffer_exception(decoder, elem,
  				trace_chan_id);
  

  break; case OCSD_GEN_TRC_ELEM_EXCEPTION_RET:

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 23600e57a215..012b4728a46f 100644 --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h @@ -46,6 +46,7 @@ struct cs_etm_packet { u32 last_instr_type; u32 last_instr_subtype; u32 flags;

• u32 exception_number; u8 last_instr_cond; u8 last_instr_taken_branch; u8 last_instr_size;

-- 2.17.1

On Wed, Jan 09, 2019 at 07:37:04PM +0800, Leo Yan wrote:

If packet processing wants to check ETM version, it needs to access metadata to decide that; but we cannot simply to use CPU logic ID number as index to access metadata sequential array, especially when system have hotplugged out CPUs, the metadata array are only allocated for online CPUs but not offline CPUs, so the CPU logic number doesn't match with its index in the array.

For this reason, a more reliable way to access metadata array is to use traceID to find metadata; by accessing metadata content we can know not only the CPU number but also for ETM version, which can be used for sequential change for setting sample flags for exception packets.

This patch is to change tuple from traceID-CPU# to traceID-metadata, thus it can use the tuple to retrieve metadata pointer according to traceID.

Signed-off-by: Leo Yan leo.yan@linaro.org

.../perf/util/cs-etm-decoder/cs-etm-decoder.c | 5 +++- tools/perf/util/cs-etm.c | 23 ++++++++++++++----- tools/perf/util/cs-etm.h | 2 +- 3 files changed, 22 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 05830fe1fb70..a537e13c45a0 100644 --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c @@ -305,6 +305,7 @@ cs_etm_decoder__buffer_packet(struct cs_etm_decoder *decoder, { u32 et = 0; struct int_node *inode = NULL;

• u64 *metadata;

if (decoder->packet_count >= MAX_BUFFER - 1) return OCSD_RESP_FATAL_SYS_ERR; @@ -314,6 +315,8 @@ cs_etm_decoder__buffer_packet(struct cs_etm_decoder *decoder, if (!inode) return OCSD_RESP_FATAL_SYS_ERR;

• metadata = inode->priv;
• et = decoder->tail; et = (et + 1) & (MAX_BUFFER - 1); decoder->tail = et;

@@ -321,7 +324,7 @@ cs_etm_decoder__buffer_packet(struct cs_etm_decoder *decoder, decoder->packet_buffer[et].sample_type = sample_type; decoder->packet_buffer[et].isa = CS_ETM_ISA_UNKNOWN;

• decoder->packet_buffer[et].cpu = *((int *)inode->priv);

• decoder->packet_buffer[et].cpu = (int)metadata[CS_ETM_CPU]; decoder->packet_buffer[et].start_addr = CS_ETM_INVAL_ADDR; decoder->packet_buffer[et].end_addr = CS_ETM_INVAL_ADDR; decoder->packet_buffer[et].instr_count = 0;

diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index f07ebc4f57bc..7c1399eda135 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -252,7 +252,7 @@ static void cs_etm__free(struct perf_session *session) cs_etm__free_events(session); session->auxtrace = NULL;

• /* First remove all traceID/CPU# nodes for the RB tree */

• /* First remove all traceID/metadata nodes for the RB tree */ intlist__for_each_entry_safe(inode, tmp, traceid_list) intlist__remove(traceid_list, inode); /* Then the RB tree itself */

@@ -1521,9 +1521,20 @@ int cs_etm__process_auxtrace_info(union perf_event *event, 0xffffffff); /*

• * Create an RB tree for traceID-CPU# tuple. Since the conversion has
  

• * to be made for each packet that gets decoded, optimizing access in
  

• * anything other than a sequential array is worth doing.
  

• * Create an RB tree for traceID-metadata tuple.
  

• *
  

• * The conversion between traceID and CPU logic ID number has to
  

• * be made for each packet that gets decoded: firstly retrive
  

s/retrive/retrieve

Checkpacth give me a big warning about it... I'm surprised you didn't get it on your side.

• * metadata pointer from trace ID by using traceID-metadata tuple,
  

• * then read CPU logic ID number in metadata.
  

• *
  

• * It's not safe to directly use CPU logic ID number as index to
  

• * access metadata sequential array, e.g. when system have
  

• * hotplugged out CPUs, the metadata array are only allocated for
  

• * online CPUs but not offline CPUs, thus the CPU logic number is
  

• * not consistent with its index in the arrary.  For this reason,
  

• * we need to fallback to use TraceID-metadata tuple as a reliable
  

• * method to access metadata.
  

  */ traceid_list = intlist__new(NULL); if (!traceid_list) {

@@ -1589,8 +1600,8 @@ int cs_etm__process_auxtrace_info(union perf_event *event, err = -EINVAL; goto err_free_metadata; }

• /* All good, associate the traceID with the CPU# */
  

• inode->priv = &metadata[j][CS_ETM_CPU];
  

• /* All good, associate the traceID with the metadata pointer */
  

• inode->priv = metadata[j];
  

Ouch - I understand why you're doing this but freely dabbing in the metadata array makes me queasy. Could we record the traceID in the packets and make the tuple traceID-index instead?

Since the metadata array is available pretty much everywhere we could have a set of helper functions like xyz_get_cpu(index, metadata) for anything we want.

} /* diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h index 37f8d48179ca..a145aafb020a 100644 --- a/tools/perf/util/cs-etm.h +++ b/tools/perf/util/cs-etm.h @@ -53,7 +53,7 @@ enum { CS_ETMV4_PRIV_MAX, }; -/* RB tree for quick conversion between traceID and CPUs */ +/* RB tree for quick conversion between traceID and metadata pointers */ struct intlist *traceid_list;

#define KiB(x) ((x) * 1024)

2.17.1

On Fri, Jan 11, 2019 at 10:43:52PM +0800, Leo Yan wrote:

On Thu, Jan 10, 2019 at 03:51:47PM -0700, Mathieu Poirier wrote:

...

On Wed, Jan 09, 2019 at 07:37:04PM +0800, Leo Yan wrote:

...

If packet processing wants to check ETM version, it needs to access metadata to decide that; but we cannot simply to use CPU logic ID number as index to access metadata sequential array, especially when system have hotplugged out CPUs, the metadata array are only allocated for online CPUs but not offline CPUs, so the CPU logic number doesn't match with its index in the array.

For this reason, a more reliable way to access metadata array is to use traceID to find metadata; by accessing metadata content we can know not only the CPU number but also for ETM version, which can be used for sequential change for setting sample flags for exception packets.

This patch is to change tuple from traceID-CPU# to traceID-metadata, thus it can use the tuple to retrieve metadata pointer according to traceID.

Signed-off-by: Leo Yan leo.yan@linaro.org

.../perf/util/cs-etm-decoder/cs-etm-decoder.c | 5 +++- tools/perf/util/cs-etm.c | 23 ++++++++++++++----- tools/perf/util/cs-etm.h | 2 +- 3 files changed, 22 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 05830fe1fb70..a537e13c45a0 100644 --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c @@ -305,6 +305,7 @@ cs_etm_decoder__buffer_packet(struct cs_etm_decoder *decoder, { u32 et = 0; struct int_node *inode = NULL;

• u64 *metadata;

if (decoder->packet_count >= MAX_BUFFER - 1) return OCSD_RESP_FATAL_SYS_ERR; @@ -314,6 +315,8 @@ cs_etm_decoder__buffer_packet(struct cs_etm_decoder *decoder, if (!inode) return OCSD_RESP_FATAL_SYS_ERR;

• metadata = inode->priv;
• et = decoder->tail; et = (et + 1) & (MAX_BUFFER - 1); decoder->tail = et;

@@ -321,7 +324,7 @@ cs_etm_decoder__buffer_packet(struct cs_etm_decoder *decoder, decoder->packet_buffer[et].sample_type = sample_type; decoder->packet_buffer[et].isa = CS_ETM_ISA_UNKNOWN;

• decoder->packet_buffer[et].cpu = *((int *)inode->priv);

• decoder->packet_buffer[et].cpu = (int)metadata[CS_ETM_CPU]; decoder->packet_buffer[et].start_addr = CS_ETM_INVAL_ADDR; decoder->packet_buffer[et].end_addr = CS_ETM_INVAL_ADDR; decoder->packet_buffer[et].instr_count = 0;

diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index f07ebc4f57bc..7c1399eda135 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -252,7 +252,7 @@ static void cs_etm__free(struct perf_session *session) cs_etm__free_events(session); session->auxtrace = NULL;

• /* First remove all traceID/CPU# nodes for the RB tree */

• /* First remove all traceID/metadata nodes for the RB tree */ intlist__for_each_entry_safe(inode, tmp, traceid_list) intlist__remove(traceid_list, inode); /* Then the RB tree itself */

@@ -1521,9 +1521,20 @@ int cs_etm__process_auxtrace_info(union perf_event *event, 0xffffffff); /*

• * Create an RB tree for traceID-CPU# tuple. Since the conversion has
  

• * to be made for each packet that gets decoded, optimizing access in
  

• * anything other than a sequential array is worth doing.
  

• * Create an RB tree for traceID-metadata tuple.
  

• *
  

• * The conversion between traceID and CPU logic ID number has to
  

• * be made for each packet that gets decoded: firstly retrive
  

s/retrive/retrieve

Checkpacth give me a big warning about it... I'm surprised you didn't get it on your side.

Sorry, my bad. Will fix it and later will ensure to use checkpatch before send out.

The last time someone sent me patches without using checkpatch, I deleted their set without reviewing it and I didn't tell them. Consider yourself warned.

...

...

• * metadata pointer from trace ID by using traceID-metadata tuple,
  

• * then read CPU logic ID number in metadata.
  

• *
  

• * It's not safe to directly use CPU logic ID number as index to
  

• * access metadata sequential array, e.g. when system have
  

• * hotplugged out CPUs, the metadata array are only allocated for
  

• * online CPUs but not offline CPUs, thus the CPU logic number is
  

• * not consistent with its index in the arrary.  For this reason,
  

• * we need to fallback to use TraceID-metadata tuple as a reliable
  

• * method to access metadata.
  

  */ traceid_list = intlist__new(NULL); if (!traceid_list) {

@@ -1589,8 +1600,8 @@ int cs_etm__process_auxtrace_info(union perf_event *event, err = -EINVAL; goto err_free_metadata; }

• /* All good, associate the traceID with the CPU# */
  

• inode->priv = &metadata[j][CS_ETM_CPU];
  

• /* All good, associate the traceID with the metadata pointer */
  

• inode->priv = metadata[j];
  

Ouch - I understand why you're doing this but freely dabbing in the metadata array makes me queasy. Could we record the traceID in the packets and make the tuple traceID-index instead?

Changing to traceID-index tuple seems better for me. But have one question for it:

If we change to use traceID-index tuple, this will introduce trouble for function cs_etm_decoder__buffer_packet(). In this function, we have trace_chan_id and need to use it to get CPU number; after using traceID-index tuple we can get index value, but we cannot get cs_etm_auxtrace::metadata due structure cs_etm_auxtrace is only a private structure in cs-etm.c and doesn't export to cs-etm-decoder/cs-etm-decoder.c.

Maybe we can keep traceID-CPU# tuple and it's only used for traceID to CPU number conversion in cs-etm.c, and add a new tuple traceID-index and this can be used as you suggested. Or other ideas?

My first thought was to move the needed structures to cs-etm.h but after taking a second look I think we can do better. First let's keep your idea of having a traceID-metadata tuple. But to access the information we could use helper functions - here is an example for CPUs:

int cs_etm__get_cpu(u8 traceid, *int cpu) { struct int_node *inode; u64 *metadata;

inode = intlist__find(traceid_list, trace_chan_id); if (!inode) return -EINVAL;

metadata = inode->priv; *cpu = (int)metadata[CS_ETM_CPU]; }

By proceeding this way we remove code duplication associated with getting the inode and functions that need access to CPU and ETM architecture information don't play directly with the metadata array.

Let me know if you find a way to improve on this.

Mathieu

...

...

} /* diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h index 37f8d48179ca..a145aafb020a 100644 --- a/tools/perf/util/cs-etm.h +++ b/tools/perf/util/cs-etm.h @@ -53,7 +53,7 @@ enum { CS_ETMV4_PRIV_MAX, }; -/* RB tree for quick conversion between traceID and CPUs */ +/* RB tree for quick conversion between traceID and metadata pointers */ struct intlist *traceid_list;

#define KiB(x) ((x) * 1024)

2.17.1

On Wed, Jan 09, 2019 at 07:37:06PM +0800, Leo Yan wrote:

The exception taken and returning are typical flow for instruction jump but it needs to be handled with exception packets. This patch is to set sample flags for exception packet.

Since the exception packet contains the exception number, according to the exception number this patch divide exception types into three classes:

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 documentation perf-script.txt;

The second type is for system call, this is set as "bcs" by following definition in the documentation;

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.

The above 3 subparagraph are not need in the changelog. Remember that a changelog should be describy "why" you're doing something rather than "what" you're doing.

Neither exception packet nor exception return packet is standalone packet which can be used to generate samples; essentially they must affiliate with instruction range packets for samples generation. The previous instruction range packet sample flags are assigned with its following exception packet.

One again I think the above can be left out.

The decoder have defined different exception number for ETMv3 and ETMv4 separately, hence this patch needs firstly decide the ETM version by using the metadata magic number and then use corresponding exception numbers for the specific ETM version.

In this patch, it introduces helper function cs_etm__is_svc_instr(); for ETMv4 CS_ETMV4_EXC_CALL covers SVC, SMC and HVC cases in the single exception number, thus need to use cs_etm__is_svc_instr() to decide an exception taken for system call.

Signed-off-by: Leo Yan leo.yan@linaro.org

tools/perf/util/cs-etm.c | 205 +++++++++++++++++++++++++++++++++++++++ tools/perf/util/cs-etm.h | 41 ++++++++ 2 files changed, 246 insertions(+)

diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index 7c1399eda135..c01f2ea55d2b 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -1108,10 +1108,174 @@ static int cs_etm__end_block(struct cs_etm_queue *etmq) return 0; } +static bool cs_etm__is_svc_instr(struct cs_etm_queue *etmq,

• 		 struct cs_etm_packet *packet,
  

• 		 u64 end_addr)
  

+{

• u16 instr16;
• u32 instr32;
• u64 addr;
• switch (packet->isa) {
• case CS_ETM_ISA_T32:

• /*
  

•  * The SVC of T32 is defined in ARM DDI 0487D.a, F5.1.247:
  

•  *
  

•  *  b'15         b'8
  

•  * +-----------------+--------+
  

•  * | 1 1 0 1 1 1 1 1 |  imm8  |
  

•  * +-----------------+--------+
  

•  *
  

•  * According to the specifiction, it only defines SVC for T32
  

•  * with 16 bits instruction and has no defintion for 32bits;
  

Again, checkpatch gives me a spelling error for "defintion". You don't get them on your side?

•  * so below only read 2 bytes as instruction size for T32.
  

•  */
  

• addr = end_addr - 2;
  

• cs_etm__mem_access(etmq, addr, sizeof(instr16), (u8 *)&instr16);
  

• if ((instr16 & 0xFF00) == 0xDF00)
  

• 	return true;
  

• break;
  

• case CS_ETM_ISA_A32:

• /*
  

•  * The SVC of A32 is defined in ARM DDI 0487D.a, F5.1.247:
  

•  *
  

•  *  b'31 b'28 b'27 b'24
  

•  * +---------+---------+-------------------------+
  

•  * |  !1111  | 1 1 1 1 |        imm24            |
  

•  * +---------+---------+-------------------------+
  

•  */
  

• addr = end_addr - 4;
  

• cs_etm__mem_access(etmq, addr, sizeof(instr32), (u8 *)&instr32);
  

• if ((instr32 & 0x0F000000) == 0x0F000000 &&
  

•     (instr32 & 0xF0000000) != 0xF0000000)
  

• 	return true;
  

• break;
  

• case CS_ETM_ISA_A64:

• /*
  

•  * The SVC of A64 is defined in ARM DDI 0487D.a, C6.2.294:
  

•  *
  

•  *  b'31               b'21           b'4     b'0
  

•  * +-----------------------+---------+-----------+
  

•  * | 1 1 0 1 0 1 0 0 0 0 0 |  imm16  | 0 0 0 0 1 |
  

•  * +-----------------------+---------+-----------+
  

•  */
  

• addr = end_addr - 4;
  

• cs_etm__mem_access(etmq, addr, sizeof(instr32), (u8 *)&instr32);
  

• if ((instr32 & 0xFFE0001F) == 0xd4000001)
  

• 	return true;
  

• break;
  

• case CS_ETM_ISA_UNKNOWN:
• default:

• break;
  

• }
• return false;

+}

+static bool cs_etm__is_syscall(struct cs_etm_queue *etmq, u64 *metadata) +{

• struct cs_etm_packet *packet = etmq->packet;
• struct cs_etm_packet *prev_packet = etmq->prev_packet;
• if (metadata[CS_ETM_MAGIC] == __perf_cs_etmv3_magic)

• if (packet->exception_number == CS_ETMV3_EXC_SVC)
  

• 	return true;
  

• /*

• * ETMv4 exception type CS_ETMV4_EXC_CALL covers SVC, SMC and
  

• * HVC cases; need to check if it's SVC instruction based on
  

• * packet address.
  

• */
  

• if (metadata[CS_ETM_MAGIC] == __perf_cs_etmv4_magic) {

• if (packet->exception_number == CS_ETMV4_EXC_CALL &&
  

•     cs_etm__is_svc_instr(etmq, prev_packet,
  

• 			 prev_packet->end_addr))
  

• 	return true;
  

• }
• return false;

+}

+static bool cs_etm__is_async_exception(struct cs_etm_queue *etmq, u64 *metadata) +{

• struct cs_etm_packet *packet = etmq->packet;
• if (metadata[CS_ETM_MAGIC] == __perf_cs_etmv3_magic)

• if (packet->exception_number == CS_ETMV3_EXC_DEBUG_HALT ||
  

•     packet->exception_number == CS_ETMV3_EXC_ASYNC_DATA_ABORT ||
  

•     packet->exception_number == CS_ETMV3_EXC_PE_RESET ||
  

•     packet->exception_number == CS_ETMV3_EXC_IRQ ||
  

•     packet->exception_number == CS_ETMV3_EXC_FIQ)
  

• 	return true;
  

• if (metadata[CS_ETM_MAGIC] == __perf_cs_etmv4_magic)

• if (packet->exception_number == CS_ETMV4_EXC_RESET ||
  

•     packet->exception_number == CS_ETMV4_EXC_DEBUG_HALT ||
  

•     packet->exception_number == CS_ETMV4_EXC_SYSTEM_ERROR ||
  

•     packet->exception_number == CS_ETMV4_EXC_INST_DEBUG ||
  

•     packet->exception_number == CS_ETMV4_EXC_DATA_DEBUG ||
  

•     packet->exception_number == CS_ETMV4_EXC_IRQ ||
  

•     packet->exception_number == CS_ETMV4_EXC_FIQ)
  

• 	return true;
  

• return false;

+}

+static bool cs_etm__is_sync_exception(struct cs_etm_queue *etmq, u64 *metadata) +{

• struct cs_etm_packet *packet = etmq->packet;
• struct cs_etm_packet *prev_packet = etmq->prev_packet;
• if (metadata[CS_ETM_MAGIC] == __perf_cs_etmv3_magic)

• if (packet->exception_number == CS_ETMV3_EXC_SMC ||
  

•     packet->exception_number == CS_ETMV3_EXC_HYP ||
  

•     packet->exception_number == CS_ETMV3_EXC_JAZELLE_THUMBEE ||
  

•     packet->exception_number == CS_ETMV3_EXC_UNDEFINED_INSTR ||
  

•     packet->exception_number == CS_ETMV3_EXC_PREFETCH_ABORT ||
  

•     packet->exception_number == CS_ETMV3_EXC_DATA_FAULT ||
  

•     packet->exception_number == CS_ETMV3_EXC_GENERIC)
  

• 	return true;
  

• if (metadata[CS_ETM_MAGIC] == __perf_cs_etmv4_magic) {

• if (packet->exception_number == CS_ETMV4_EXC_TRAP ||
  

•     packet->exception_number == CS_ETMV4_EXC_ALIGNMENT ||
  

•     packet->exception_number == CS_ETMV4_EXC_INST_FAULT ||
  

•     packet->exception_number == CS_ETMV4_EXC_DATA_FAULT)
  

• 	return true;
  

• /*
  

•  * For CS_ETMV4_EXC_CALL, except SVC other instructions
  

•  * (SMC, HVC) are taken as sync exceptions.
  

•  */
  

• if (packet->exception_number == CS_ETMV4_EXC_CALL &&
  

•     !cs_etm__is_svc_instr(etmq, prev_packet,
  

• 			  prev_packet->end_addr))
  

• 	return true;
  

• /*
  

•  * ETMv4 has 5 bits for exception number; if the numbers
  

•  * are in the range ( CS_ETMV4_EXC_FIQ, CS_ETMV4_EXC_END ]
  

•  * they are implementation defined exceptions.
  

•  *
  

•  * For this case, simply take it as sync exception.
  

•  */
  

• if (packet->exception_number > CS_ETMV4_EXC_FIQ &&
  

•     packet->exception_number <= CS_ETMV4_EXC_END)
  

• 	return true;
  

• }
• return false;

+}

static int cs_etm__set_sample_flags(struct cs_etm_queue *etmq) { struct cs_etm_packet *packet = etmq->packet; struct cs_etm_packet *prev_packet = etmq->prev_packet;

• struct int_node *inode;
• u64 *metadata;

packet->flags = 0; @@ -1192,8 +1356,49 @@ static int cs_etm__set_sample_flags(struct cs_etm_queue *etmq) if (prev_packet->sample_type == CS_ETM_RANGE) prev_packet->flags |= PERF_IP_FLAG_BRANCH | PERF_IP_FLAG_TRACE_END;

Accidental newline addition.

break;

case CS_ETM_EXCEPTION:

• /* Search the RB tree for the metadata with traceID */
  

• inode = intlist__find(traceid_list, packet->trace_chan_id);
  

• if (!inode)
  

• 	return -EINVAL;
  

• metadata = inode->priv;
  

• /* The exception is for system call. */
  

• if (cs_etm__is_syscall(etmq, metadata))
  

• 	packet->flags = PERF_IP_FLAG_BRANCH |
  

• 			PERF_IP_FLAG_CALL |
  

• 			PERF_IP_FLAG_SYSCALLRET;
  

• /*
  

•  * The exceptions are triggered by external signals from bus,
  

•  * interrupt controller, debug module, PE reset or halt.
  

•  */
  

• else if (cs_etm__is_async_exception(etmq, metadata))
  

• 	packet->flags = PERF_IP_FLAG_BRANCH |
  

• 			PERF_IP_FLAG_CALL |
  

• 			PERF_IP_FLAG_ASYNC |
  

• 			PERF_IP_FLAG_INTERRUPT;
  

• /*
  

•  * Otherwise, exception is caused by trap, instruction &
  

•  * data fault, or alignment errors.
  

•  */
  

• else if (cs_etm__is_sync_exception(etmq, metadata))
  

• 	packet->flags = PERF_IP_FLAG_BRANCH |
  

• 			PERF_IP_FLAG_CALL |
  

• 			PERF_IP_FLAG_INTERRUPT;
  

• /*
  

•  * When the exception packet is inserted, since exception
  

•  * packet is not used standalone for generating samples
  

•  * and it's affiliation to the previous instruction range
  

•  * packet; so set previous range packet flags to tell perf
  

•  * it is an exception taken branch.
  

•  */
  

• if (prev_packet->sample_type == CS_ETM_RANGE)
  

• 	prev_packet->flags = packet->flags;
  

• break;
  

  case CS_ETM_EXCEPTION_RET: case CS_ETM_EMPTY: default:

diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h index a145aafb020a..5a11236ee694 100644 --- a/tools/perf/util/cs-etm.h +++ b/tools/perf/util/cs-etm.h @@ -53,6 +53,47 @@ enum { CS_ETMV4_PRIV_MAX, }; +/* ETMv3 exception number */ +enum {

• CS_ETMV3_EXC_NONE,
• CS_ETMV3_EXC_DEBUG_HALT,
• CS_ETMV3_EXC_SMC,
• CS_ETMV3_EXC_HYP,
• CS_ETMV3_EXC_ASYNC_DATA_ABORT,
• CS_ETMV3_EXC_JAZELLE_THUMBEE,
• CS_ETMV3_EXC_RESERVED1,
• CS_ETMV3_EXC_RESERVED2,
• CS_ETMV3_EXC_PE_RESET,
• CS_ETMV3_EXC_UNDEFINED_INSTR,
• CS_ETMV3_EXC_SVC,
• CS_ETMV3_EXC_PREFETCH_ABORT,
• CS_ETMV3_EXC_DATA_FAULT,
• CS_ETMV3_EXC_GENERIC,
• CS_ETMV3_EXC_IRQ,
• CS_ETMV3_EXC_FIQ,

+};

+/* ETMv4 exception number */ +enum {

• CS_ETMV4_EXC_RESET,
• CS_ETMV4_EXC_DEBUG_HALT,
• CS_ETMV4_EXC_CALL,
• CS_ETMV4_EXC_TRAP,
• CS_ETMV4_EXC_SYSTEM_ERROR,
• CS_ETMV4_EXC_RESERVED1,
• CS_ETMV4_EXC_INST_DEBUG,
• CS_ETMV4_EXC_DATA_DEBUG,
• CS_ETMV4_EXC_RESERVED2,
• CS_ETMV4_EXC_RESERVED3,
• CS_ETMV4_EXC_ALIGNMENT,
• CS_ETMV4_EXC_INST_FAULT,
• CS_ETMV4_EXC_DATA_FAULT,
• CS_ETMV4_EXC_RESERVED4,
• CS_ETMV4_EXC_IRQ,
• CS_ETMV4_EXC_FIQ,
• CS_ETMV4_EXC_END = 31,

+};

From my perspective everything you're doing in this patch works but I'll need

Mike or Robert to give me a thumbs up.

/* RB tree for quick conversion between traceID and metadata pointers */ struct intlist *traceid_list; -- 2.17.1

On Wed, Jan 09, 2019 at 07:37:07PM +0800, Leo Yan wrote:

When return from exception, we need to distinguish if it's system call return or for other type exceptions for setting sample flags. Due to the exception return packet doesn't contain exception number, so we cannot decide sample flags based on exception number.

On the other hand, the exception return packet is followed by an instruction range packet; this range packet deliveries the start address after exception handling, we can check if it is a SVC instruction just before the start address. If there has one SVC instruction is found ahead the return address, this means it's an exception return for system call; otherwise it is an normal return for other exceptions.

This patch is to set sample flags for exception return packet, firstly it simply set sample flags as PERF_IP_FLAG_INTERRUPT for all exception returns since at this point it doesn't know what's exactly the exception type. We will defer to decide if it's an exception return for system call when the next instruction range packet comes, it checks if there has one SVC instruction prior to the start address and if so we will change sample flags to PERF_IP_FLAG_SYSCALLRET for system call return.

Signed-off-by: Leo Yan leo.yan@linaro.org

tools/perf/util/cs-etm.c | 44 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)

diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index c01f2ea55d2b..74aac7e771ba 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -1346,6 +1346,20 @@ static int cs_etm__set_sample_flags(struct cs_etm_queue *etmq) if (prev_packet->sample_type == CS_ETM_DISCONTINUITY) prev_packet->flags |= PERF_IP_FLAG_BRANCH | PERF_IP_FLAG_TRACE_BEGIN;

• /*
  

•  * If the previous packet is an exception return packet
  

•  * and the return address just follows SVC instuction,
  

•  * it needs to calibrate the previous packet sample flags
  

•  * as PERF_IP_FLAG_SYSCALLRET.
  

•  */
  

• if (prev_packet->flags == (PERF_IP_FLAG_BRANCH |
  

• 			   PERF_IP_FLAG_RETURN |
  

• 			   PERF_IP_FLAG_INTERRUPT) &&
  

•     cs_etm__is_svc_instr(etmq, packet, packet->start_addr))
  

• 	prev_packet->flags = PERF_IP_FLAG_BRANCH |
  

• 			     PERF_IP_FLAG_RETURN |
  

• 			     PERF_IP_FLAG_SYSCALLRET;
  

  break; case CS_ETM_DISCONTINUITY: /*

@@ -1400,6 +1414,36 @@ static int cs_etm__set_sample_flags(struct cs_etm_queue *etmq) prev_packet->flags = packet->flags; break; case CS_ETM_EXCEPTION_RET:

• /*
  

•  * When the exception return packet is inserted, since
  

•  * exception return packet is not used standalone for
  

•  * generating samples and it's affiliation to the previous
  

•  * instruction range packet; so set previous range packet
  

•  * flags to tell perf it is an exception return branch.
  

•  *
  

•  * The exception return can be for either system call or
  

•  * other exception types; unfortunately the packet doesn't
  

•  * contain exception type related info so we cannot decide
  

•  * the exception type purely based on exception return packet.
  

•  * If we record the exception number from exception packet and
  

•  * reuse it for excpetion return packet, this is not reliable
  

•  * due the trace can be discontinuity or the interrupt can
  

•  * be nested, thus the recorded exception number cannot be
  

•  * used for exception return packet for these two cases.
  

•  *
  

•  * For exception return packet, we only need to distinguish the
  

•  * packet is for system call or for other types.  Thus the
  

•  * decision can be deferred when receive the next packet which
  

•  * contains the return address, based on the return address we
  

•  * can read out the previous instruction and check if it's a
  

•  * system call instruction and then calibrate the sample flag
  

•  * as needed.
  

•  */
  

• if (prev_packet->sample_type == CS_ETM_RANGE)
  

• 	prev_packet->flags = PERF_IP_FLAG_BRANCH |
  

• 			     PERF_IP_FLAG_RETURN |
  

• 			     PERF_IP_FLAG_INTERRUPT;
  

• break;
  

Once again this looks good to me but I'll need Mike or Robert to confirm the validity of the implementation.

I am done reviewing this set.

Thanks, Mathieu

case CS_ETM_EMPTY: default: break; -- 2.17.1