Re: [PATCH v2 6/6] perf cs-etm: Generate branch sample for exception packet

From: Mathieu Poirier
Date: Mon Dec 10 2018 - 18:07:19 EST


On Mon, Dec 10, 2018 at 04:53:01PM +0800, Leo Yan wrote:
> The exception packet appears as one element with 'elem_type' ==
> OCSD_GEN_TRC_ELEM_EXCEPTION or OCSD_GEN_TRC_ELEM_EXCEPTION_RET,
> which present for exception entry and exit respectively. The decoder
> set packet fields 'packet->exc' and 'packet->exc_ret' to indicate the
> exception packets; but exception packets don't have dedicated sample
> type and shares the same sample type CS_ETM_RANGE with normal
> instruction packets.
>
> As result, the exception packets are taken as normal instruction packets
> and this introduces confusion to mix different packet types.
> Furthermore, these instruction range packets will be processed for
> branch sample only when 'packet->last_instr_taken_branch' is true,
> otherwise they will be omitted, this can introduce mess for exception
> and exception returning due we don't have complete address range info
> for context switching.
>
> To process exception packets properly, this patch introduce two new
> sample type: CS_ETM_EXCEPTION and CS_ETM_EXCEPTION_RET; for these two
> kind packets, they will be handled by cs_etm__exception(). The func
> cs_etm__exception() forces to set previous CS_ETM_RANGE packet flag
> 'prev_packet->last_instr_taken_branch' to true, this matches well with
> the program flow when the exception is trapped from user space to kernel
> space, no matter if the most recent flow has branch taken or not; this
> is also safe for returning to user space after exception handling.
>
> After exception packets have their own sample type, the packet fields
> 'packet->exc' and 'packet->exc_ret' aren't needed anymore, so remove
> them.
>
> Cc: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
> Cc: Mike Leach <mike.leach@xxxxxxxxxx>
> Cc: Robert Walker <robert.walker@xxxxxxx>
> Signed-off-by: Leo Yan <leo.yan@xxxxxxxxxx>
> ---
> tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 26 +++++++++++++++++------
> tools/perf/util/cs-etm-decoder/cs-etm-decoder.h | 4 ++--
> tools/perf/util/cs-etm.c | 28 +++++++++++++++++++++++++
> 3 files changed, 50 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 bcb5c98..6d89d0e 100644
> --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> @@ -291,8 +291,6 @@ 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].exc = false;
> - decoder->packet_buffer[i].exc_ret = false;
> decoder->packet_buffer[i].cpu = INT_MIN;
> }
> }
> @@ -320,8 +318,6 @@ 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].exc = false;
> - decoder->packet_buffer[et].exc_ret = false;
> decoder->packet_buffer[et].cpu = *((int *)inode->priv);
> decoder->packet_buffer[et].start_addr = CS_ETM_INVAL_ADDR;
> decoder->packet_buffer[et].end_addr = CS_ETM_INVAL_ADDR;
> @@ -398,6 +394,22 @@ cs_etm_decoder__buffer_discontinuity(struct cs_etm_decoder *decoder,
> CS_ETM_DISCONTINUITY);
> }
>
> +static ocsd_datapath_resp_t
> +cs_etm_decoder__buffer_exception(struct cs_etm_decoder *decoder,
> + const uint8_t trace_chan_id)
> +{
> + return cs_etm_decoder__buffer_packet(decoder, trace_chan_id,
> + CS_ETM_EXCEPTION);
> +}
> +
> +static ocsd_datapath_resp_t
> +cs_etm_decoder__buffer_exception_ret(struct cs_etm_decoder *decoder,
> + const uint8_t trace_chan_id)
> +{
> + return cs_etm_decoder__buffer_packet(decoder, trace_chan_id,
> + CS_ETM_EXCEPTION_RET);
> +}
> +
> static ocsd_datapath_resp_t cs_etm_decoder__gen_trace_elem_printer(
> const void *context,
> const ocsd_trc_index_t indx __maybe_unused,
> @@ -426,10 +438,12 @@ static ocsd_datapath_resp_t cs_etm_decoder__gen_trace_elem_printer(
> trace_chan_id);
> break;
> case OCSD_GEN_TRC_ELEM_EXCEPTION:
> - decoder->packet_buffer[decoder->tail].exc = true;
> + resp = cs_etm_decoder__buffer_exception(decoder,
> + trace_chan_id);
> break;
> case OCSD_GEN_TRC_ELEM_EXCEPTION_RET:
> - decoder->packet_buffer[decoder->tail].exc_ret = true;
> + resp = cs_etm_decoder__buffer_exception_ret(decoder,
> + trace_chan_id);
> break;
> case OCSD_GEN_TRC_ELEM_PE_CONTEXT:
> case OCSD_GEN_TRC_ELEM_ADDR_NACC:
> 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 a272317..a6407d4 100644
> --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h
> +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h
> @@ -26,6 +26,8 @@ enum cs_etm_sample_type {
> CS_ETM_EMPTY,
> CS_ETM_RANGE,
> CS_ETM_DISCONTINUITY,
> + CS_ETM_EXCEPTION,
> + CS_ETM_EXCEPTION_RET,
> };
>
> enum cs_etm_isa {
> @@ -43,8 +45,6 @@ struct cs_etm_packet {
> u32 instr_count;
> u8 last_instr_taken_branch;
> u8 last_instr_size;
> - u8 exc;
> - u8 exc_ret;
> int cpu;
> };
>
> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index cea3158..27a374d 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c
> @@ -1000,6 +1000,25 @@ static int cs_etm__sample(struct cs_etm_queue *etmq)
> return 0;
> }
>
> +static int cs_etm__exception(struct cs_etm_queue *etmq)
> +{
> + /*
> + * When the exception packet is inserted, whether the last instruction
> + * in previous range packet is taken branch or not, we need to force
> + * to set 'prev_packet->last_instr_taken_branch' to true. This ensures
> + * to generate branch sample for the instruction range before the
> + * exception is trapped to kernel or before the exception returning.
> + *
> + * The exception packet includes the dummy address values, so don't
> + * swap PACKET with PREV_PACKET. This keeps PREV_PACKET to be useful
> + * for generating instruction and branch samples.
> + */
> + if (etmq->prev_packet->sample_type == CS_ETM_RANGE)
> + etmq->prev_packet->last_instr_taken_branch = true;
> +
> + return 0;
> +}
> +
> static int cs_etm__flush(struct cs_etm_queue *etmq)
> {
> int err = 0;
> @@ -1148,6 +1167,15 @@ static int cs_etm__run_decoder(struct cs_etm_queue *etmq)
> */
> cs_etm__sample(etmq);
> break;
> + case CS_ETM_EXCEPTION:
> + case CS_ETM_EXCEPTION_RET:
> + /*
> + * If the exception packet is coming,
> + * make sure the previous instruction
> + * range packet to be handled properly.
> + */
> + cs_etm__exception(etmq);
> + break;
> case CS_ETM_DISCONTINUITY:
> /*
> * Discontinuity in trace, flush
> --
> 2.7.4
>

Reviewed-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>