On 30/05/2023 11:40, Mike Leach wrote:
Hi James,
Further thought here - this consistency check may be redundant.
You are setting the EL according to that in the traced context packet. This is correct and sufficient to match the EL that OpenCSD is expecting, if added to the security state from the same packet. The EL memory space values that OpenCSD provides in the memory callback are based on that same data from this context packet. Thus it is not necessary to use the EL in the memory callback. This is primarily used for matching when searching for a suitable memory source - especially when clients register files etc with the library, rather than provide a global callback as perf does.
Regards
It was supposed to be defensive coding against some future refactor where the Perf side tracking goes wrong. I think it makes sense to keep it as it could be quite easy to make a mistake there and extra tests can't be bad.
For ETMv3 I did have to make this change because OpenCSD was returning the 'any' values so I made it skip the validation in that case:
if (!(mem_space == OCSD_MEM_SPACE_ANY || mem_space == OCSD_MEM_SPACE_N || mem_space == OCSD_MEM_SPACE_S)) { 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); }
Mike
On Tue, 30 May 2023 at 10:12, James Clark james.clark@arm.com wrote:
On 25/05/2023 12:39, Mike Leach wrote:
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.
As long as none of the bits are set for EL1-EL3 then no validation will be done so it should be fine. But I will try to test ETMv3.
Can this patch - and the set handle this. (assuming perf supports our ETMv3 coresight kernel driver)
For the whole set, the tracked EL will stay as ocsd_EL_unknown and cs_etm__get_machine() will return host so the behavior will be unchanged from before. This is assuming OpenCSD will set the EL to unknown in elem->context.exception_level in this case.
Regards
Mike
On Wed, 24 May 2023 at 14:20, James Clark james.clark@arm.com 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@arm.com
.../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