Hi Leo,


On Fri, Dec 18, 2020 at 2:46 AM Daniel Kiss <Daniel.Kiss@arm.com> wrote:
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 saw a discussion about perf compatibility in https://lists.linaro.org/pipermail/coresight/2020-November/005326.html.
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