Adding Denis.
> On 16 Nov 2020, at 10:46, Leo Yan <leo.yan@linaro.org> wrote:
>
> Hi Suzuki,
>
> On Fri, Nov 13, 2020 at 09:47:42AM +0000, Suzuki Kuruppassery Poulose wrote:
>
> [...]
>
>>> Rather than heuristic method, I think we can use metadata to store
>>> "pid" entry in the function cs_etm_get_metadata(), so that in the
>>> record phase, the "pid" is stored into perf data file.
>>
>> True, that is a good idea. That makes it future proof.
>>
>>>
>>> When decode the trace data, we can retrieve "pid" value from the
>>> metadata, and can base on the "pid" value to make decision for using
>>> context_id or VMID.
>>
>>>
>>> The metadata can be accessed by referring
>>> cs_etm_queue::cs_etm_auxtrace::metadata; but the file
>>> util/cs-etm-decoder/cs-etm-decoder.c cannot directly access the
>>> metadata structure due to "cs_etm_queue" and "cs_etm_auxtrace" both
>>> are defined in util/cs-etm.c. It's better to introduce a helper
>>> function in util/cs-etm.c to retrieve "pid" value, like:
>>>
>>> u64 cs_etm__etmq_get_pid_type(struct cs_etm_queue *etmq);
>>
>> I am not an expert in that side, how it interacts with the OpenCSD.
>> thus left this patch as an RFC.
>>
>> I appreciate your feedback and thoughts. If you feel like you could
>> cleanup and implement this, please feel free to do so. Otherwise, I
>> might try it out whenever I have some spare cycles to understand
>> this side of the code (and this might be delayed).
>
> Below is the drafted patch for support PID format in the metadata and
> I tested for "perf record" and "perf script" command and can work as
> expected.
>
> P.s. I uploaded the patches into the github [1] and gave some minor
> refactoring for your last patch "perf cs-etm: Detect pid in VMID for
> kernel running at EL2".
I have tested the patches on Chrome OS EL2 kernel and they worked fine for me.
Note that "perf cs-etm: Add PID format into metadata" patch breaks perf backward compatibility. It may cause a problem in off-target decoding if there is a version skew in perf.
I understand that perf doesn't guarantee backward compatibility but in fact incompatibility issues occur rarely. I think if there is an (easy) way to do it the compatibility breakage should be avoided.
This is a critical fix for Chrome OS. Please let me know what you think.
Thanks,
Denis
>
> Please review and consider to consolidate the changes if look good for
> you.
>
> Thanks,
>
> ---8<---
>
> From ba70531217c51f2b4115965bd7e4b7b51770a626 Mon Sep 17 00:00:00 2001
> From: Leo Yan <leo.yan@linaro.org>
> Date: Sat, 14 Nov 2020 09:47:12 +0800
> Subject: [PATCH] perf cs-etm: Add PID format into metadata
>
> It's possible for CoreSight to trace PID in either CONTEXTIDR_EL1 or
> CONTEXTIDR_EL2, the PID format info can be used to distinguish the PID
> is traced in which register.
>
> This patch saves PID format value into the metadata when record the
> trace data; during the decoding phase, it provides a helper function
> cs_etm__get_pid_fmt() for easier retrieving PID format from metadata.
>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
> tools/perf/arch/arm/util/cs-etm.c | 21 +++++++++++++++++++++
> tools/perf/util/cs-etm.c | 21 +++++++++++++++++++++
> tools/perf/util/cs-etm.h | 3 +++
> 3 files changed, 45 insertions(+)
>
> diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
> index e6207ce7cc85..837eebe30a90 100644
> --- a/tools/perf/arch/arm/util/cs-etm.c
> +++ b/tools/perf/arch/arm/util/cs-etm.c
> @@ -606,6 +606,7 @@ static void cs_etm_get_metadata(int cpu, u32 *offset,
> struct cs_etm_recording *ptr =
> container_of(itr, struct cs_etm_recording, itr);
> struct perf_pmu *cs_etm_pmu = ptr->cs_etm_pmu;
> + u64 pid_fmt;
>
> /* first see what kind of tracer this cpu is affined to */
> if (cs_etm_is_etmv4(itr, cpu)) {
> @@ -634,6 +635,16 @@ static void cs_etm_get_metadata(int cpu, u32 *offset,
> metadata_etmv4_ro
> [CS_ETMV4_TRCAUTHSTATUS]);
>
> + /*
> + * The PID format will be used when decode the trace data;
> + * based on it the decoder will make decision for setting
> + * sample's PID as context_id or VMID.
> + */
> + pid_fmt = perf_pmu__format_bits(&cs_etm_pmu->format, "pid");
> + if (!pid_fmt)
> + pid_fmt = 1ULL << ETM_OPT_CTXTID;
> + info->priv[*offset + CS_ETMV4_PID_FMT] = pid_fmt;
> +
> /* How much space was used */
> increment = CS_ETMV4_PRIV_MAX;
> } else {
> @@ -651,6 +662,16 @@ static void cs_etm_get_metadata(int cpu, u32 *offset,
> cs_etm_get_ro(cs_etm_pmu, cpu,
> metadata_etmv3_ro[CS_ETM_ETMIDR]);
>
> + /*
> + * The PID format will be used when decode the trace data;
> + * based on it the decoder will make decision for setting
> + * sample's PID as context_id or VMID.
> + */
> + pid_fmt = perf_pmu__format_bits(&cs_etm_pmu->format, "pid");
> + if (!pid_fmt)
> + pid_fmt = 1ULL << ETM_OPT_CTXTID;
> + info->priv[*offset + CS_ETM_PID_FMT] = pid_fmt;
> +
> /* How much space was used */
> increment = CS_ETM_PRIV_MAX;
> }
> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index a2a369e2fbb6..ffd91c8dadf0 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c
> @@ -156,6 +156,25 @@ int cs_etm__get_cpu(u8 trace_chan_id, int *cpu)
> return 0;
> }
>
> +int cs_etm__get_pid_fmt(u8 trace_chan_id, u64 *pid_fmt)
> +{
> + struct int_node *inode;
> + u64 *metadata;
> +
> + inode = intlist__find(traceid_list, trace_chan_id);
> + if (!inode)
> + return -EINVAL;
> +
> + metadata = inode->priv;
> +
> + if (metadata[CS_ETM_MAGIC] == __perf_cs_etmv3_magic)
> + *pid_fmt = (int)metadata[CS_ETM_PID_FMT];
> + else
> + *pid_fmt = (int)metadata[CS_ETMV4_PID_FMT];
> +
> + return 0;
> +}
> +
> void cs_etm__etmq_set_traceid_queue_timestamp(struct cs_etm_queue *etmq,
> u8 trace_chan_id)
> {
> @@ -2447,6 +2466,7 @@ static const char * const cs_etm_priv_fmts[] = {
> [CS_ETM_ETMTRACEIDR] = " ETMTRACEIDR %llx\n",
> [CS_ETM_ETMCCER] = " ETMCCER %llx\n",
> [CS_ETM_ETMIDR] = " ETMIDR %llx\n",
> + [CS_ETM_PID_FMT] = " PID Format %llx\n",
> };
>
> static const char * const cs_etmv4_priv_fmts[] = {
> @@ -2459,6 +2479,7 @@ static const char * const cs_etmv4_priv_fmts[] = {
> [CS_ETMV4_TRCIDR2] = " TRCIDR2 %llx\n",
> [CS_ETMV4_TRCIDR8] = " TRCIDR8 %llx\n",
> [CS_ETMV4_TRCAUTHSTATUS] = " TRCAUTHSTATUS %llx\n",
> + [CS_ETMV4_PID_FMT] = " PID Format %llx\n",
> };
>
> static void cs_etm__print_auxtrace_info(__u64 *val, int num)
> diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h
> index 4ad925d6d799..98801040175f 100644
> --- a/tools/perf/util/cs-etm.h
> +++ b/tools/perf/util/cs-etm.h
> @@ -38,6 +38,7 @@ enum {
> /* RO, taken from sysFS */
> CS_ETM_ETMCCER,
> CS_ETM_ETMIDR,
> + CS_ETM_PID_FMT,
> CS_ETM_PRIV_MAX,
> };
>
> @@ -52,6 +53,7 @@ enum {
> CS_ETMV4_TRCIDR2,
> CS_ETMV4_TRCIDR8,
> CS_ETMV4_TRCAUTHSTATUS,
> + CS_ETMV4_PID_FMT,
> CS_ETMV4_PRIV_MAX,
> };
>
> @@ -173,6 +175,7 @@ struct cs_etm_packet_queue {
> int cs_etm__process_auxtrace_info(union perf_event *event,
> struct perf_session *session);
> 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);
> bool cs_etm__etmq_is_timeless(struct cs_etm_queue *etmq);
> --
> 2.17.1
>
>
> [1] https://github.com/Leo-Yan/linux/tree/perf_cs_etm_pid_tracing_vhe_for_suzuki
> [2] https://github.com/Leo-Yan/linux/commit/302600d5676c45ebfa8a221b2d5fa4172ff42a91
> _______________________________________________
> CoreSight mailing list
> CoreSight@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/coresight