Re: [PATCH 4/4] perf cs-etm: Add exception level consistency check
From: Mike Leach
Date: Thu May 25 2023 - 07:40:18 EST
Hi James,
My concern here is that for etmv3 trace, OpenCSD will only provide
memory spaces as either secure or non-secure, The ETMv3 does not
trace, and hence OpenCSD cannot provide the different ELs.
The memory callback will be either OCSD_MEM_SPACE_S or OCSD_MEM_SPACE_N.
Can this patch - and the set handle this. (assuming perf supports our
ETMv3 coresight kernel driver)
Regards
Mike
On Wed, 24 May 2023 at 14:20, James Clark <james.clark@xxxxxxx> wrote:
>
> Assert that our own tracking of the exception level matches what
> OpenCSD provides. OpenCSD doesn't distinguish between EL0 and EL1 in the
> memory access callback so the extra tracking was required. But a rough
> assert can still be done.
>
> Signed-off-by: James Clark <james.clark@xxxxxxx>
> ---
> .../perf/util/cs-etm-decoder/cs-etm-decoder.c | 6 +--
> .../perf/util/cs-etm-decoder/cs-etm-decoder.h | 4 +-
> tools/perf/util/cs-etm.c | 37 +++++++++++++------
> 3 files changed, 32 insertions(+), 15 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 ac227cd03eb0..50b3c248d1e5 100644
> --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> @@ -52,15 +52,15 @@ struct cs_etm_decoder {
> static u32
> cs_etm_decoder__mem_access(const void *context,
> const ocsd_vaddr_t address,
> - const ocsd_mem_space_acc_t mem_space __maybe_unused,
> + const ocsd_mem_space_acc_t mem_space,
> const u8 trace_chan_id,
> const u32 req_size,
> u8 *buffer)
> {
> struct cs_etm_decoder *decoder = (struct cs_etm_decoder *) context;
>
> - return decoder->mem_access(decoder->data, trace_chan_id,
> - address, req_size, buffer);
> + return decoder->mem_access(decoder->data, trace_chan_id, address,
> + req_size, buffer, mem_space);
> }
>
> int cs_etm_decoder__add_mem_access_cb(struct cs_etm_decoder *decoder,
> 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 21d403f55d96..272c2efe78ee 100644
> --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h
> +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h
> @@ -11,6 +11,7 @@
> #define INCLUDE__CS_ETM_DECODER_H__
>
> #include <linux/types.h>
> +#include <opencsd/ocsd_if_types.h>
> #include <stdio.h>
>
> struct cs_etm_decoder;
> @@ -19,7 +20,8 @@ struct cs_etm_packet_queue;
>
> struct cs_etm_queue;
>
> -typedef u32 (*cs_etm_mem_cb_type)(struct cs_etm_queue *, u8, u64, size_t, u8 *);
> +typedef u32 (*cs_etm_mem_cb_type)(struct cs_etm_queue *, u8, u64, size_t, u8 *,
> + const ocsd_mem_space_acc_t);
>
> struct cs_etmv3_trace_params {
> u32 reg_ctrl;
> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index b9ba19327f26..ccf34ed8ddf2 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c
> @@ -931,7 +931,8 @@ static u8 cs_etm__cpu_mode(struct cs_etm_queue *etmq, u64 address,
> }
>
> static u32 cs_etm__mem_access(struct cs_etm_queue *etmq, u8 trace_chan_id,
> - u64 address, size_t size, u8 *buffer)
> + u64 address, size_t size, u8 *buffer,
> + const ocsd_mem_space_acc_t mem_space)
> {
> u8 cpumode;
> u64 offset;
> @@ -947,6 +948,20 @@ static u32 cs_etm__mem_access(struct cs_etm_queue *etmq, u8 trace_chan_id,
> if (!tidq)
> return 0;
>
> + /*
> + * We've already tracked EL along side the PID in cs_etm__set_thread()
> + * so double check that it matches what OpenCSD thinks as well. It
> + * doesn't distinguish between EL0 and EL1 for this mem access callback
> + * so we had to do the extra tracking.
> + */
> + if (mem_space & OCSD_MEM_SPACE_EL1N) {
> + /* Includes both non secure EL1 and EL0 */
> + assert(tidq->el == ocsd_EL1 || tidq->el == ocsd_EL0);
> + } else if (mem_space & OCSD_MEM_SPACE_EL2)
> + assert(tidq->el == ocsd_EL2);
> + else if (mem_space & OCSD_MEM_SPACE_EL3)
> + assert(tidq->el == ocsd_EL3);
> +
> cpumode = cs_etm__cpu_mode(etmq, address, tidq->el);
>
> if (!thread__find_map(tidq->thread, cpumode, address, &al))
> @@ -1195,8 +1210,8 @@ static inline int cs_etm__t32_instr_size(struct cs_etm_queue *etmq,
> {
> u8 instrBytes[2];
>
> - cs_etm__mem_access(etmq, trace_chan_id, addr,
> - ARRAY_SIZE(instrBytes), instrBytes);
> + cs_etm__mem_access(etmq, trace_chan_id, addr, ARRAY_SIZE(instrBytes),
> + instrBytes, 0);
> /*
> * T32 instruction size is indicated by bits[15:11] of the first
> * 16-bit word of the instruction: 0b11101, 0b11110 and 0b11111
> @@ -1387,8 +1402,8 @@ static void cs_etm__copy_insn(struct cs_etm_queue *etmq,
> else
> sample->insn_len = 4;
>
> - cs_etm__mem_access(etmq, trace_chan_id, sample->ip,
> - sample->insn_len, (void *)sample->insn);
> + cs_etm__mem_access(etmq, trace_chan_id, sample->ip, sample->insn_len,
> + (void *)sample->insn, 0);
> }
>
> u64 cs_etm__convert_sample_time(struct cs_etm_queue *etmq, u64 cs_timestamp)
> @@ -1940,8 +1955,8 @@ static bool cs_etm__is_svc_instr(struct cs_etm_queue *etmq, u8 trace_chan_id,
> * so below only read 2 bytes as instruction size for T32.
> */
> addr = end_addr - 2;
> - cs_etm__mem_access(etmq, trace_chan_id, addr,
> - sizeof(instr16), (u8 *)&instr16);
> + cs_etm__mem_access(etmq, trace_chan_id, addr, sizeof(instr16),
> + (u8 *)&instr16, 0);
> if ((instr16 & 0xFF00) == 0xDF00)
> return true;
>
> @@ -1956,8 +1971,8 @@ static bool cs_etm__is_svc_instr(struct cs_etm_queue *etmq, u8 trace_chan_id,
> * +---------+---------+-------------------------+
> */
> addr = end_addr - 4;
> - cs_etm__mem_access(etmq, trace_chan_id, addr,
> - sizeof(instr32), (u8 *)&instr32);
> + cs_etm__mem_access(etmq, trace_chan_id, addr, sizeof(instr32),
> + (u8 *)&instr32, 0);
> if ((instr32 & 0x0F000000) == 0x0F000000 &&
> (instr32 & 0xF0000000) != 0xF0000000)
> return true;
> @@ -1973,8 +1988,8 @@ static bool cs_etm__is_svc_instr(struct cs_etm_queue *etmq, u8 trace_chan_id,
> * +-----------------------+---------+-----------+
> */
> addr = end_addr - 4;
> - cs_etm__mem_access(etmq, trace_chan_id, addr,
> - sizeof(instr32), (u8 *)&instr32);
> + cs_etm__mem_access(etmq, trace_chan_id, addr, sizeof(instr32),
> + (u8 *)&instr32, 0);
> if ((instr32 & 0xFFE0001F) == 0xd4000001)
> return true;
>
> --
> 2.34.1
>
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK