Re: [PATCH v2 4/5] perf cs-etm: Track exception level

From: Leo Yan
Date: Fri Jun 09 2023 - 22:33:55 EST

Next message: Dan Williams: "RE: [PATCH v5 15/26] cxl/hdm: Use stored Component Register mappings to map HDM decoder capability"
Previous message: Yu Kuai: "Re: [PATCH] md/raid10: Only check QUEUE_FLAG_IO_STAT when issuing io"
In reply to: James Clark: "[PATCH v2 4/5] perf cs-etm: Track exception level"
Next in thread: James Clark: "Re: [PATCH v2 4/5] perf cs-etm: Track exception level"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, Jun 08, 2023 at 11:59:28AM +0100, James Clark wrote:
> Currently we assume all trace belongs to the host machine so when
> the decoder should be looking at the guest kernel maps it can crash
> because it looks at the host ones instead.
>
> Avoid one scenario (guest kernel running at EL1) by assigning the
> default guest machine to this trace. For userspace trace it's still not
> possible to determine guest vs host, but the PIDs should help in this
> case.
>
> Signed-off-by: James Clark <james.clark@xxxxxxx>

Reviewed-by: Leo Yan <leo.yan@xxxxxxxxxx>

Just input a thought for support both host and guest machines, seems to
me, a feasibe solution is CoreSight can extend its capability to trace
below system registers together:

- VTTBR_EL2.vmid
- CONTEXTIDR_EL2
- CONTEXTIDR_EL1

So:

VTTBR_EL2.vmid == 0: host machine, CONTEXTIDR_EL2 is for tracing PID
VTTBR_EL2.vmid != 0: guest machine, CONTEXTIDR_EL1 is for tracing PID

So far we only can trace either CONTEXTIDR_EL2 (and it's even a bit
mess that we save CONTEXTIDR_EL2 into 'elem->context.vmid') or
CONTEXTIDR_EL1, this is the main reason it's hard for perf to
distinguish host and guest. I know you might have discussed this yet,
here I just record some ideas in case later we can review it.

Thanks,
Leo

> ---
> .../perf/util/cs-etm-decoder/cs-etm-decoder.c | 7 +-
> tools/perf/util/cs-etm.c | 75 +++++++++++++++----
> tools/perf/util/cs-etm.h | 7 +-
> 3 files changed, 67 insertions(+), 22 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 2af641d26866..44c49acd6bff 100644
> --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> @@ -561,12 +561,13 @@ cs_etm_decoder__set_tid(struct cs_etm_queue *etmq,
> break;
> }
>
> + if (cs_etm__etmq_set_tid_el(etmq, tid, trace_chan_id,
> + elem->context.exception_level))
> + return OCSD_RESP_FATAL_SYS_ERR;
> +
> if (tid == -1)
> return OCSD_RESP_CONT;
>
> - if (cs_etm__etmq_set_tid(etmq, tid, trace_chan_id))
> - return OCSD_RESP_FATAL_SYS_ERR;
> -
> /*
> * A timestamp is generated after a PE_CONTEXT element so make sure
> * to rely on that coming one.
> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index e0904f276e89..916e86f003f4 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c
> @@ -14,7 +14,6 @@
> #include <linux/types.h>
> #include <linux/zalloc.h>
>
> -#include <opencsd/ocsd_if_types.h>
> #include <stdlib.h>
>
> #include "auxtrace.h"
> @@ -88,6 +87,8 @@ struct cs_etm_traceid_queue {
> union perf_event *event_buf;
> struct thread *thread;
> struct thread *prev_thread;
> + ocsd_ex_level prev_el;
> + ocsd_ex_level el;
> struct branch_stack *last_branch;
> struct branch_stack *last_branch_rb;
> struct cs_etm_packet *prev_packet;
> @@ -482,6 +483,7 @@ static int cs_etm__init_traceid_queue(struct cs_etm_queue *etmq,
>
> queue = &etmq->etm->queues.queue_array[etmq->queue_nr];
> tidq->trace_chan_id = trace_chan_id;
> + tidq->el = tidq->prev_el = ocsd_EL_unknown;
> tidq->thread = machine__findnew_thread(&etm->session->machines.host, -1,
> queue->tid);
> tidq->prev_thread = machine__idle_thread(&etm->session->machines.host);
> @@ -621,6 +623,7 @@ static void cs_etm__packet_swap(struct cs_etm_auxtrace *etm,
> tmp = tidq->packet;
> tidq->packet = tidq->prev_packet;
> tidq->prev_packet = tmp;
> + tidq->prev_el = tidq->el;
> thread__put(tidq->prev_thread);
> tidq->prev_thread = thread__get(tidq->thread);
> }
> @@ -882,11 +885,43 @@ static bool cs_etm__evsel_is_auxtrace(struct perf_session *session,
> return evsel->core.attr.type == aux->pmu_type;
> }
>
> -static u8 cs_etm__cpu_mode(struct cs_etm_queue *etmq, u64 address)
> +static struct machine *cs_etm__get_machine(struct cs_etm_queue *etmq,
> + ocsd_ex_level el)
> {
> - struct machine *machine;
> + enum cs_etm_pid_fmt pid_fmt = cs_etm__get_pid_fmt(etmq);
>
> - machine = &etmq->etm->session->machines.host;
> + /*
> + * For any virtualisation based on nVHE (e.g. pKVM), or host kernels
> + * running at EL1 assume everything is the host.
> + */
> + if (pid_fmt == CS_ETM_PIDFMT_CTXTID)
> + return &etmq->etm->session->machines.host;
> +
> + /*
> + * Not perfect, but otherwise assume anything in EL1 is the default
> + * guest, and everything else is the host. Distinguishing between guest
> + * and host userspaces isn't currently supported either. Neither is
> + * multiple guest support. All this does is reduce the likeliness of
> + * decode errors where we look into the host kernel maps when it should
> + * have been the guest maps.
> + */
> + switch (el) {
> + case ocsd_EL1:
> + return machines__find_guest(&etmq->etm->session->machines,
> + DEFAULT_GUEST_KERNEL_ID);
> + case ocsd_EL3:
> + case ocsd_EL2:
> + case ocsd_EL0:
> + case ocsd_EL_unknown:
> + default:
> + return &etmq->etm->session->machines.host;
> + }
> +}
> +
> +static u8 cs_etm__cpu_mode(struct cs_etm_queue *etmq, u64 address,
> + ocsd_ex_level el)
> +{
> + struct machine *machine = cs_etm__get_machine(etmq, el);
>
> if (address >= machine__kernel_start(machine)) {
> if (machine__is_host(machine))
> @@ -896,10 +931,14 @@ static u8 cs_etm__cpu_mode(struct cs_etm_queue *etmq, u64 address)
> } else {
> if (machine__is_host(machine))
> return PERF_RECORD_MISC_USER;
> - else if (perf_guest)
> + else {
> + /*
> + * Can't really happen at the moment because
> + * cs_etm__get_machine() will always return
> + * machines.host for any non EL1 trace.
> + */
> return PERF_RECORD_MISC_GUEST_USER;
> - else
> - return PERF_RECORD_MISC_HYPERVISOR;
> + }
> }
> }
>
> @@ -916,11 +955,12 @@ static u32 cs_etm__mem_access(struct cs_etm_queue *etmq, u8 trace_chan_id,
> if (!etmq)
> return 0;
>
> - cpumode = cs_etm__cpu_mode(etmq, address);
> tidq = cs_etm__etmq_get_traceid_queue(etmq, trace_chan_id);
> if (!tidq)
> return 0;
>
> + cpumode = cs_etm__cpu_mode(etmq, address, tidq->el);
> +
> if (!thread__find_map(tidq->thread, cpumode, address, &al))
> return 0;
>
> @@ -1298,10 +1338,11 @@ cs_etm__get_trace(struct cs_etm_queue *etmq)
> return etmq->buf_len;
> }
>
> -static void cs_etm__set_thread(struct cs_etm_auxtrace *etm,
> - struct cs_etm_traceid_queue *tidq, pid_t tid)
> +static void cs_etm__set_thread(struct cs_etm_queue *etmq,
> + struct cs_etm_traceid_queue *tidq, pid_t tid,
> + ocsd_ex_level el)
> {
> - struct machine *machine = &etm->session->machines.host;
> + struct machine *machine = cs_etm__get_machine(etmq, el);
>
> if (tid != -1) {
> thread__zput(tidq->thread);
> @@ -1311,10 +1352,12 @@ static void cs_etm__set_thread(struct cs_etm_auxtrace *etm,
> /* Couldn't find a known thread */
> if (!tidq->thread)
> tidq->thread = machine__idle_thread(machine);
> +
> + tidq->el = el;
> }
>
> -int cs_etm__etmq_set_tid(struct cs_etm_queue *etmq,
> - pid_t tid, u8 trace_chan_id)
> +int cs_etm__etmq_set_tid_el(struct cs_etm_queue *etmq, pid_t tid,
> + u8 trace_chan_id, ocsd_ex_level el)
> {
> struct cs_etm_traceid_queue *tidq;
>
> @@ -1322,7 +1365,7 @@ int cs_etm__etmq_set_tid(struct cs_etm_queue *etmq,
> if (!tidq)
> return -EINVAL;
>
> - cs_etm__set_thread(etmq->etm, tidq, tid);
> + cs_etm__set_thread(etmq, tidq, tid, el);
> return 0;
> }
>
> @@ -1392,7 +1435,7 @@ static int cs_etm__synth_instruction_sample(struct cs_etm_queue *etmq,
> struct perf_sample sample = {.ip = 0,};
>
> event->sample.header.type = PERF_RECORD_SAMPLE;
> - event->sample.header.misc = cs_etm__cpu_mode(etmq, addr);
> + event->sample.header.misc = cs_etm__cpu_mode(etmq, addr, tidq->el);
> event->sample.header.size = sizeof(struct perf_event_header);
>
> /* Set time field based on etm auxtrace config. */
> @@ -1451,7 +1494,7 @@ static int cs_etm__synth_branch_sample(struct cs_etm_queue *etmq,
> ip = cs_etm__last_executed_instr(tidq->prev_packet);
>
> event->sample.header.type = PERF_RECORD_SAMPLE;
> - event->sample.header.misc = cs_etm__cpu_mode(etmq, ip);
> + event->sample.header.misc = cs_etm__cpu_mode(etmq, ip, tidq->prev_el);
> event->sample.header.size = sizeof(struct perf_event_header);
>
> /* Set time field based on etm auxtrace config. */
> diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h
> index 2f47f4ec5b27..7cca37887917 100644
> --- a/tools/perf/util/cs-etm.h
> +++ b/tools/perf/util/cs-etm.h
> @@ -251,10 +251,11 @@ enum cs_etm_pid_fmt {
> };
>
> #ifdef HAVE_CSTRACE_SUPPORT
> +#include <opencsd/ocsd_if_types.h>
> int cs_etm__get_cpu(u8 trace_chan_id, int *cpu);
> -enum pid_fmt cs_etm__get_pid_fmt(struct cs_etm_queue *etmq);
> -int cs_etm__etmq_set_tid(struct cs_etm_queue *etmq,
> - pid_t tid, u8 trace_chan_id);
> +enum cs_etm_pid_fmt cs_etm__get_pid_fmt(struct cs_etm_queue *etmq);
> +int cs_etm__etmq_set_tid_el(struct cs_etm_queue *etmq, pid_t tid,
> + u8 trace_chan_id, ocsd_ex_level el);
> bool cs_etm__etmq_is_timeless(struct cs_etm_queue *etmq);
> void cs_etm__etmq_set_traceid_queue_timestamp(struct cs_etm_queue *etmq,
> u8 trace_chan_id);
> --
> 2.34.1
>

Next message: Dan Williams: "RE: [PATCH v5 15/26] cxl/hdm: Use stored Component Register mappings to map HDM decoder capability"
Previous message: Yu Kuai: "Re: [PATCH] md/raid10: Only check QUEUE_FLAG_IO_STAT when issuing io"
In reply to: James Clark: "[PATCH v2 4/5] perf cs-etm: Track exception level"
Next in thread: James Clark: "Re: [PATCH v2 4/5] perf cs-etm: Track exception level"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]