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

From: James Clark
Date: Tue May 30 2023 - 05:43:36 EST

Next message: Greg KH: "Re: [PATCH v5] usb: typec: intel_pmc_mux: Expose IOM port status to debugfs"
Previous message: Powen Kao (高伯文): "Re: [PATCH v2 1/3] scsi: ufs: core: Introduce mcq ops to config cqid"
In reply to: Leo Yan: "Re: [PATCH 3/4] perf cs-etm: Track exception level"
Next in thread: James Clark: "[PATCH 4/4] perf cs-etm: Add exception level consistency check"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxx>
>> ---
>> .../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
>>

Next message: Greg KH: "Re: [PATCH v5] usb: typec: intel_pmc_mux: Expose IOM port status to debugfs"
Previous message: Powen Kao (高伯文): "Re: [PATCH v2 1/3] scsi: ufs: core: Introduce mcq ops to config cqid"
In reply to: Leo Yan: "Re: [PATCH 3/4] perf cs-etm: Track exception level"
Next in thread: James Clark: "[PATCH 4/4] perf cs-etm: Add exception level consistency check"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]