RE: [PATCH 3/3] rfc: perf: cs_etm: Detect pid in VMID for kernel running at EL2

Al Grant
2020-11-11T11:04:11Z

From: Suzuki K Poulose <suzuki.poulose@arm.com>
> The pid of the task could be traced as VMID when the kernel is running at EL2.
> Teach the decoder to look for vmid when the context_id is invalid but we have a
> valid VMID.
>
> Cc: Mike Leach <mike.leach@linaro.org>
> Cc: Leo Yan <leo.yan@linaro.org>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>
> Ideally, we should also confirm that the VMID_OPT is also set in the trcconfigr
> for choosing the VMID. Hence the RFC. May be that is something we could
> cache in the "decoder" instance ?
> ---
> .../perf/util/cs-etm-decoder/cs-etm-decoder.c | 28 +++++++++++--------
> 1 file changed, 17 insertions(+), 11 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 cd007cc9c283..31ba7ff57914 100644
> --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> @@ -502,19 +502,25 @@ cs_etm_decoder__set_tid(struct cs_etm_queue
> *etmq, {
> pid_t tid;
>
> - /* Ignore PE_CONTEXT packets that don't have a valid contextID */
> - if (!elem->context.ctxt_id_valid)
> - return OCSD_RESP_CONT;
> -
> - tid = elem->context.context_id;
> - 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.
> + * Process the PE_CONTEXT packets if we have a valid
> + * contextID or VMID.
> + * If the kernel is running at EL2, the PID is traced
> + * in contextidr_el2 as VMID.
> */
> - cs_etm_decoder__reset_timestamp(packet_queue);
> + if (elem->context.ctxt_id_valid || elem->context.vmid_valid) {
> + if (elem->context.ctxt_id_valid)
> + tid = elem->context.context_id;
> + else
> + tid = elem->context.vmid;
> + 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.
> + */
> + cs_etm_decoder__reset_timestamp(packet_queue);
> + }
>
> return OCSD_RESP_CONT;
> }

So if both CONTEXTID and VMID fields are valid, it will take the TID from
CONTEXTID? That seems to make the assumption that...

- it is valid to have VMID in trace from an EL1 kernel (and it should be
ignored in favour of getting TID from CONTEXID)

- for an EL2 kernel, where we need to get TID from VMID, we are guaranteed
to not have valid CONTEXTID

This seems the wrong way round. An EL2 kernel might want to trace its
EL1 guests' CONTEXTID to disambiguate guest processes - right now I don't
believe we could make use of this in perf, but it's information that is
meaningful in an EL2 kernel's trace. On the other hand, an EL1 kernel's
trace doesn't have any business with VMID (which would be the actual
VM id) - if an EL1 kernel was capturing trace from other VMs in a trace
buffer, that's a big security hole.

It's safer if perf.data has an explicit indication of which field the TID is in -
that indicator should be either somewhere in AUXTRACE_INFO or in one
of the config fields in the attribute, or a bit in the PERF_RECORD_AUX
record. But if you must do it heuristically at decode time based on what
you see in packets, then it would be safer to say that if you see both
VMID and CONTEXIDR, the TID will be in VMID.

Al

> --
> 2.24.1