| Message ID | 20230524131958.2139331-4-james.clark@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf cs-etm: Track exception level | expand |
Hi James, On Wed, 24 May 2023 at 14:20, James Clark <james.clark@arm.com> 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@arm.com> > --- > .../perf/util/cs-etm-decoder/cs-etm-decoder.c | 7 +- > tools/perf/util/cs-etm.c | 64 ++++++++++++++----- > tools/perf/util/cs-etm.h | 5 +- > 3 files changed, 56 insertions(+), 20 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 82a27ab90c8b..ac227cd03eb0 100644 > --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c > +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c > @@ -573,12 +573,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 a997fe79d458..b9ba19327f26 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" > @@ -87,6 +86,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; > @@ -479,6 +480,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); > @@ -618,6 +620,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); > } > @@ -879,11 +882,34 @@ 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_auxtrace *etm, > + ocsd_ex_level el) > { > - struct machine *machine; > + /* > + * Not perfect, but assume anything in EL1 is the default guest, and > + * everything else is the host. nHVE and pKVM may not work with this > + * assumption. And 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. > + */ What effect does this have if I am running with host only, kernel at EL1, e.g. any platform that is not running an EL2 kernel? Mike > + switch (el) { > + case ocsd_EL1: > + return machines__find_guest(&etm->session->machines, > + DEFAULT_GUEST_KERNEL_ID); > + case ocsd_EL3: > + case ocsd_EL2: > + case ocsd_EL0: > + case ocsd_EL_unknown: > + default: > + return &etm->session->machines.host; > + } > +} > > - machine = &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->etm, el); > > if (address >= machine__kernel_start(machine)) { > if (machine__is_host(machine)) > @@ -893,10 +919,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; > + } > } > } > > @@ -913,11 +943,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; > > @@ -1296,9 +1327,10 @@ cs_etm__get_trace(struct cs_etm_queue *etmq) > } > > static void cs_etm__set_thread(struct cs_etm_auxtrace *etm, > - struct cs_etm_traceid_queue *tidq, pid_t tid) > + 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(etm, el); > > if (tid != -1) { > thread__zput(tidq->thread); > @@ -1308,10 +1340,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; > > @@ -1319,7 +1353,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->etm, tidq, tid, el); > return 0; > } > > @@ -1389,7 +1423,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. */ > @@ -1448,7 +1482,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 70cac0375b34..88e9b25a8a9f 100644 > --- a/tools/perf/util/cs-etm.h > +++ b/tools/perf/util/cs-etm.h > @@ -232,10 +232,11 @@ int cs_etm__process_auxtrace_info(union perf_event *event, > struct perf_event_attr *cs_etm_get_default_config(struct perf_pmu *pmu); > > #ifdef HAVE_CSTRACE_SUPPORT > +#include <opencsd/ocsd_if_types.h> > int cs_etm__get_cpu(u8 trace_chan_id, int *cpu); > int cs_etm__get_pid_fmt(u8 trace_chan_id, u64 *pid_fmt); > -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); > 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 >
On Wed, May 24, 2023 at 02:19:57PM +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@arm.com> > --- > .../perf/util/cs-etm-decoder/cs-etm-decoder.c | 7 +- > tools/perf/util/cs-etm.c | 64 ++++++++++++++----- > tools/perf/util/cs-etm.h | 5 +- > 3 files changed, 56 insertions(+), 20 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 82a27ab90c8b..ac227cd03eb0 100644 > --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c > +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c > @@ -573,12 +573,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 a997fe79d458..b9ba19327f26 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" > @@ -87,6 +86,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; > @@ -479,6 +480,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); > @@ -618,6 +620,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); > } > @@ -879,11 +882,34 @@ 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_auxtrace *etm, > + ocsd_ex_level el) > { > - struct machine *machine; > + /* > + * Not perfect, but assume anything in EL1 is the default guest, and > + * everything else is the host. nHVE and pKVM may not work with this s/nHVE/nVHE ? And it's good to say "if any virtulisation (e.g. pKVM) based on nVHE may not work with this ....". > + * assumption. And 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(&etm->session->machines, > + DEFAULT_GUEST_KERNEL_ID); I share the same concern with Mike that this will break perf for any Armv8 CPUs with nVHE (like Cortex-CA53, etc). You could see CoreSight has trace data "elem->context.vmid", when a guest kernel runs in EL1, can we use "elem->context.vmid" as a guest kernel ID and finally retrieve the corresponding machine pointer? Thanks, Leo > + case ocsd_EL3: > + case ocsd_EL2: > + case ocsd_EL0: > + case ocsd_EL_unknown: > + default: > + return &etm->session->machines.host; > + } > +} > > - machine = &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->etm, el); > > if (address >= machine__kernel_start(machine)) { > if (machine__is_host(machine)) > @@ -893,10 +919,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; > + } > } > } > > @@ -913,11 +943,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; > > @@ -1296,9 +1327,10 @@ cs_etm__get_trace(struct cs_etm_queue *etmq) > } > > static void cs_etm__set_thread(struct cs_etm_auxtrace *etm, > - struct cs_etm_traceid_queue *tidq, pid_t tid) > + 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(etm, el); > > if (tid != -1) { > thread__zput(tidq->thread); > @@ -1308,10 +1340,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; > > @@ -1319,7 +1353,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->etm, tidq, tid, el); > return 0; > } > > @@ -1389,7 +1423,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. */ > @@ -1448,7 +1482,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 70cac0375b34..88e9b25a8a9f 100644 > --- a/tools/perf/util/cs-etm.h > +++ b/tools/perf/util/cs-etm.h > @@ -232,10 +232,11 @@ int cs_etm__process_auxtrace_info(union perf_event *event, > struct perf_event_attr *cs_etm_get_default_config(struct perf_pmu *pmu); > > #ifdef HAVE_CSTRACE_SUPPORT > +#include <opencsd/ocsd_if_types.h> > int cs_etm__get_cpu(u8 trace_chan_id, int *cpu); > int cs_etm__get_pid_fmt(u8 trace_chan_id, u64 *pid_fmt); > -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); > 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 >
On 25/05/2023 12:16, Mike Leach wrote: > Hi James, > > On Wed, 24 May 2023 at 14:20, James Clark <james.clark@arm.com> 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@arm.com> >> --- >> .../perf/util/cs-etm-decoder/cs-etm-decoder.c | 7 +- >> tools/perf/util/cs-etm.c | 64 ++++++++++++++----- >> tools/perf/util/cs-etm.h | 5 +- >> 3 files changed, 56 insertions(+), 20 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 82a27ab90c8b..ac227cd03eb0 100644 >> --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c >> +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c >> @@ -573,12 +573,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 a997fe79d458..b9ba19327f26 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" >> @@ -87,6 +86,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; >> @@ -479,6 +480,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); >> @@ -618,6 +620,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); >> } >> @@ -879,11 +882,34 @@ 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_auxtrace *etm, >> + ocsd_ex_level el) >> { >> - struct machine *machine; >> + /* >> + * Not perfect, but assume anything in EL1 is the default guest, and >> + * everything else is the host. nHVE and pKVM may not work with this >> + * assumption. And 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. >> + */ > > What effect does this have if I am running with host only, kernel at > EL1, e.g. any platform that is not running an EL2 kernel? I suppose I didn't think about that case. It looks like you'd have to start using --guest-vmlinux instead of --vmlinux to decode. But that's probably not what we want. Is that a standard configuration though? I'm wondering if we need to support that out of the box, or needing the extra command line argument is ok? It seems like it's hard to make anything just work without the user providing extra info. After this change we also wanted to start setting exclude_guest=1 by default, so technically this change is only needed when exclude_guest=0. Otherwise we could assume it's always the host regardless of the EL. It should be quite easy to check what that setting was in this switch statement, but it would be temporarily broken in the mean time. And also we can only make exclude_guest work with FEAT_TRF (v8.4 onwards) > > Mike > > >> + switch (el) { >> + case ocsd_EL1: >> + return machines__find_guest(&etm->session->machines, >> + DEFAULT_GUEST_KERNEL_ID); >> + case ocsd_EL3: >> + case ocsd_EL2: >> + case ocsd_EL0: >> + case ocsd_EL_unknown: >> + default: >> + return &etm->session->machines.host; >> + } >> +} >> >> - machine = &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->etm, el); >> >> if (address >= machine__kernel_start(machine)) { >> if (machine__is_host(machine)) >> @@ -893,10 +919,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; >> + } >> } >> } >> >> @@ -913,11 +943,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; >> >> @@ -1296,9 +1327,10 @@ cs_etm__get_trace(struct cs_etm_queue *etmq) >> } >> >> static void cs_etm__set_thread(struct cs_etm_auxtrace *etm, >> - struct cs_etm_traceid_queue *tidq, pid_t tid) >> + 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(etm, el); >> >> if (tid != -1) { >> thread__zput(tidq->thread); >> @@ -1308,10 +1340,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; >> >> @@ -1319,7 +1353,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->etm, tidq, tid, el); >> return 0; >> } >> >> @@ -1389,7 +1423,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. */ >> @@ -1448,7 +1482,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 70cac0375b34..88e9b25a8a9f 100644 >> --- a/tools/perf/util/cs-etm.h >> +++ b/tools/perf/util/cs-etm.h >> @@ -232,10 +232,11 @@ int cs_etm__process_auxtrace_info(union perf_event *event, >> struct perf_event_attr *cs_etm_get_default_config(struct perf_pmu *pmu); >> >> #ifdef HAVE_CSTRACE_SUPPORT >> +#include <opencsd/ocsd_if_types.h> >> int cs_etm__get_cpu(u8 trace_chan_id, int *cpu); >> int cs_etm__get_pid_fmt(u8 trace_chan_id, u64 *pid_fmt); >> -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); >> 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 >> > >
On 28/05/2023 12:05, Leo Yan wrote: > On Wed, May 24, 2023 at 02:19:57PM +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@arm.com> >> --- >> .../perf/util/cs-etm-decoder/cs-etm-decoder.c | 7 +- >> tools/perf/util/cs-etm.c | 64 ++++++++++++++----- >> tools/perf/util/cs-etm.h | 5 +- >> 3 files changed, 56 insertions(+), 20 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 82a27ab90c8b..ac227cd03eb0 100644 >> --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c >> +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c >> @@ -573,12 +573,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 a997fe79d458..b9ba19327f26 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" >> @@ -87,6 +86,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; >> @@ -479,6 +480,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); >> @@ -618,6 +620,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); >> } >> @@ -879,11 +882,34 @@ 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_auxtrace *etm, >> + ocsd_ex_level el) >> { >> - struct machine *machine; >> + /* >> + * Not perfect, but assume anything in EL1 is the default guest, and >> + * everything else is the host. nHVE and pKVM may not work with this > > s/nHVE/nVHE ? > > And it's good to say "if any virtulisation (e.g. pKVM) based on nVHE may > not work with this ....". > >> + * assumption. And 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(&etm->session->machines, >> + DEFAULT_GUEST_KERNEL_ID); > > I share the same concern with Mike that this will break perf for any > Armv8 CPUs with nVHE (like Cortex-CA53, etc). > I agree, I replied to Mike's comment with some thoughts. > You could see CoreSight has trace data "elem->context.vmid", when a > guest kernel runs in EL1, can we use "elem->context.vmid" as a guest > kernel ID and finally retrieve the corresponding machine pointer? > Do you mean something like this? static struct machine *cs_etm__get_machine(struct cs_etm_auxtrace *etm, ocsd_ex_level el) { u64 pid_fmt; cs_etm__get_pid_fmt(trace_chan_id, &pid_fmt); if (pid_fmt == BIT(ETM_OPT_CTXTID)) return return &etm->session->machines.host; switch (el) { case ocsd_EL1: return machines__find_guest(&etm->session->machines, DEFAULT_GUEST_KERNEL_ID); case ocsd_EL3: case ocsd_EL2: case ocsd_EL0: case ocsd_EL_unknown: default: return &etm->session->machines.host; } } I think this might work, maybe it's not as bad as I thought and we can make it work out of the box. > Thanks, > Leo > >> + case ocsd_EL3: >> + case ocsd_EL2: >> + case ocsd_EL0: >> + case ocsd_EL_unknown: >> + default: >> + return &etm->session->machines.host; >> + } >> +} >> >> - machine = &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->etm, el); >> >> if (address >= machine__kernel_start(machine)) { >> if (machine__is_host(machine)) >> @@ -893,10 +919,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; >> + } >> } >> } >> >> @@ -913,11 +943,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; >> >> @@ -1296,9 +1327,10 @@ cs_etm__get_trace(struct cs_etm_queue *etmq) >> } >> >> static void cs_etm__set_thread(struct cs_etm_auxtrace *etm, >> - struct cs_etm_traceid_queue *tidq, pid_t tid) >> + 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(etm, el); >> >> if (tid != -1) { >> thread__zput(tidq->thread); >> @@ -1308,10 +1340,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; >> >> @@ -1319,7 +1353,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->etm, tidq, tid, el); >> return 0; >> } >> >> @@ -1389,7 +1423,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. */ >> @@ -1448,7 +1482,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 70cac0375b34..88e9b25a8a9f 100644 >> --- a/tools/perf/util/cs-etm.h >> +++ b/tools/perf/util/cs-etm.h >> @@ -232,10 +232,11 @@ int cs_etm__process_auxtrace_info(union perf_event *event, >> struct perf_event_attr *cs_etm_get_default_config(struct perf_pmu *pmu); >> >> #ifdef HAVE_CSTRACE_SUPPORT >> +#include <opencsd/ocsd_if_types.h> >> int cs_etm__get_cpu(u8 trace_chan_id, int *cpu); >> int cs_etm__get_pid_fmt(u8 trace_chan_id, u64 *pid_fmt); >> -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); >> 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 >>
On Tue, May 30, 2023 at 10:24:45AM +0100, James Clark wrote: [...] > >> -static u8 cs_etm__cpu_mode(struct cs_etm_queue *etmq, u64 address) > >> +static struct machine *cs_etm__get_machine(struct cs_etm_auxtrace *etm, > >> + ocsd_ex_level el) > >> { > >> - struct machine *machine; > >> + /* > >> + * Not perfect, but assume anything in EL1 is the default guest, and > >> + * everything else is the host. nHVE and pKVM may not work with this > >> + * assumption. And 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. > >> + */ > > > > What effect does this have if I am running with host only, kernel at > > EL1, e.g. any platform that is not running an EL2 kernel? > > I suppose I didn't think about that case. It looks like you'd have to > start using --guest-vmlinux instead of --vmlinux to decode. But that's > probably not what we want. IIUC, it's not about how to pass kernel image path with options "--guest-vmlinux" or "--vmlinux". Let's think a case: Cortex-A53 CPU which doesn't support VHE, or any Arm CPUs which disable the VHE feature. In this case, the host OS and guest OS both run in EL1, only the a KVM hypervisor runs in EL2. You could see with below change, no matter the host OS and guest OS, both use DEFAULT_GUEST_KERNEL_ID to retrieve the guest machine. At the end, this change will break perf tool for host OS. > Is that a standard configuration though? I'm wondering if we need to > support that out of the box, or needing the extra command line argument > is ok? It seems like it's hard to make anything just work without the > user providing extra info. Essentially, we need more info to make decision if a system runs in host or guest, if only depend on it's EL1 or other execptions, it's hard to say it's running in host or guest. We can look into more details: - If CPU enables VHE, then the host kernel runs in EL2 and the guest kernel runs in EL1; - If CPU only enables nVHE (or CPU doesn't support VHE at all), then both the host kernel and the guest kernel runs in EL1; I think we need to retrieve more info from /sys/ node or /proc node: - We need to know if VHE is enabled or not; - If CPU doesn't enable VHE, then both host and guest kernels run in EL1, so we need to use extra info to distinguish it's a host kernel or guest kernel. To be honest, I don't have answers for above two items. I will think a bit more and will share back if have any finding. Thanks, Leo > After this change we also wanted to start setting exclude_guest=1 by > default, so technically this change is only needed when exclude_guest=0. > Otherwise we could assume it's always the host regardless of the EL. It > should be quite easy to check what that setting was in this switch > statement, but it would be temporarily broken in the mean time. And also > we can only make exclude_guest work with FEAT_TRF (v8.4 onwards) > > > > > Mike > > > > > >> + switch (el) { > >> + case ocsd_EL1: > >> + return machines__find_guest(&etm->session->machines, > >> + DEFAULT_GUEST_KERNEL_ID); > >> + case ocsd_EL3: > >> + case ocsd_EL2: > >> + case ocsd_EL0: > >> + case ocsd_EL_unknown: > >> + default: > >> + return &etm->session->machines.host; > >> + } > >> +} > >> > >> - machine = &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->etm, el); > >> > >> if (address >= machine__kernel_start(machine)) { > >> if (machine__is_host(machine)) > >> @@ -893,10 +919,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; > >> + } > >> } > >> } > >> > >> @@ -913,11 +943,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; > >> > >> @@ -1296,9 +1327,10 @@ cs_etm__get_trace(struct cs_etm_queue *etmq) > >> } > >> > >> static void cs_etm__set_thread(struct cs_etm_auxtrace *etm, > >> - struct cs_etm_traceid_queue *tidq, pid_t tid) > >> + 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(etm, el); > >> > >> if (tid != -1) { > >> thread__zput(tidq->thread); > >> @@ -1308,10 +1340,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; > >> > >> @@ -1319,7 +1353,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->etm, tidq, tid, el); > >> return 0; > >> } > >> > >> @@ -1389,7 +1423,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. */ > >> @@ -1448,7 +1482,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 70cac0375b34..88e9b25a8a9f 100644 > >> --- a/tools/perf/util/cs-etm.h > >> +++ b/tools/perf/util/cs-etm.h > >> @@ -232,10 +232,11 @@ int cs_etm__process_auxtrace_info(union perf_event *event, > >> struct perf_event_attr *cs_etm_get_default_config(struct perf_pmu *pmu); > >> > >> #ifdef HAVE_CSTRACE_SUPPORT > >> +#include <opencsd/ocsd_if_types.h> > >> int cs_etm__get_cpu(u8 trace_chan_id, int *cpu); > >> int cs_etm__get_pid_fmt(u8 trace_chan_id, u64 *pid_fmt); > >> -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); > >> 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 > >> > > > >
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 82a27ab90c8b..ac227cd03eb0 100644 --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c @@ -573,12 +573,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 a997fe79d458..b9ba19327f26 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" @@ -87,6 +86,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; @@ -479,6 +480,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); @@ -618,6 +620,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); } @@ -879,11 +882,34 @@ 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_auxtrace *etm, + ocsd_ex_level el) { - struct machine *machine; + /* + * Not perfect, but assume anything in EL1 is the default guest, and + * everything else is the host. nHVE and pKVM may not work with this + * assumption. And 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(&etm->session->machines, + DEFAULT_GUEST_KERNEL_ID); + case ocsd_EL3: + case ocsd_EL2: + case ocsd_EL0: + case ocsd_EL_unknown: + default: + return &etm->session->machines.host; + } +} - machine = &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->etm, el); if (address >= machine__kernel_start(machine)) { if (machine__is_host(machine)) @@ -893,10 +919,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; + } } } @@ -913,11 +943,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; @@ -1296,9 +1327,10 @@ cs_etm__get_trace(struct cs_etm_queue *etmq) } static void cs_etm__set_thread(struct cs_etm_auxtrace *etm, - struct cs_etm_traceid_queue *tidq, pid_t tid) + 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(etm, el); if (tid != -1) { thread__zput(tidq->thread); @@ -1308,10 +1340,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; @@ -1319,7 +1353,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->etm, tidq, tid, el); return 0; } @@ -1389,7 +1423,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. */ @@ -1448,7 +1482,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 70cac0375b34..88e9b25a8a9f 100644 --- a/tools/perf/util/cs-etm.h +++ b/tools/perf/util/cs-etm.h @@ -232,10 +232,11 @@ int cs_etm__process_auxtrace_info(union perf_event *event, struct perf_event_attr *cs_etm_get_default_config(struct perf_pmu *pmu); #ifdef HAVE_CSTRACE_SUPPORT +#include <opencsd/ocsd_if_types.h> int cs_etm__get_cpu(u8 trace_chan_id, int *cpu); int cs_etm__get_pid_fmt(u8 trace_chan_id, u64 *pid_fmt); -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); 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);
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@arm.com> --- .../perf/util/cs-etm-decoder/cs-etm-decoder.c | 7 +- tools/perf/util/cs-etm.c | 64 ++++++++++++++----- tools/perf/util/cs-etm.h | 5 +- 3 files changed, 56 insertions(+), 20 deletions(-)