[PATCH 2/3] perf: cs_etm: Use pid tracing explicitly instead of contextid
Suzuki K Poulose
suzuki.poulose at arm.com
Thu Nov 12 05:54:23 EST 2020
Hi Leo,
Thanks for looking at the patch.
On 11/12/20 10:00 AM, Leo Yan wrote:
> On Tue, Nov 10, 2020 at 06:33:12PM +0000, Suzuki Kuruppassery Poulose wrote:
>> If the kernel is running at EL2, the pid of the task is exposed
>> via VMID instead of the CONTEXTID. Add support for this in the
>> perf tool.
>>
>> By default the perf tool requests contextid and timestamp for
>> task bound events. Instead of hard coding contextid, switch
>> to "pid" config exposed by the kernel.
>>
>> Cc: Leo Yan <leo.yan at linaro.org>
>> Cc: Mike Leach <mike.leach at linaro.org>
>> Cc: Mathieu Poirier <mathieu.poirier at linaro.org>
>> Cc: Al Grant <al.grant at arm.com>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose at arm.com>
>> ---
>> tools/include/linux/coresight-pmu.h | 11 +++--
>> tools/perf/arch/arm/util/cs-etm.c | 65 +++++++++++++++++++++--------
>> 2 files changed, 54 insertions(+), 22 deletions(-)
>>
>> diff --git a/tools/include/linux/coresight-pmu.h b/tools/include/linux/coresight-pmu.h
>> index b0e35eec6499..927c6285ce5d 100644
>> --- a/tools/include/linux/coresight-pmu.h
>> +++ b/tools/include/linux/coresight-pmu.h
>> @@ -11,16 +11,19 @@
>> #define CORESIGHT_ETM_PMU_SEED 0x10
>>
>> /* ETMv3.5/PTM's ETMCR config bit */
>> -#define ETM_OPT_CYCACC 12
>> -#define ETM_OPT_CTXTID 14
>> -#define ETM_OPT_TS 28
>> -#define ETM_OPT_RETSTK 29
>> +#define ETM_OPT_CYCACC 12
>> +#define ETM_OPT_CTXTID 14
>> +#define ETM_OPT_CTXTID_IN_VMID 15
>> +#define ETM_OPT_TS 28
>> +#define ETM_OPT_RETSTK 29
>>
>> /* ETMv4 CONFIGR programming bits for the ETM OPTs */
>> #define ETM4_CFG_BIT_CYCACC 4
>> #define ETM4_CFG_BIT_CTXTID 6
>> +#define ETM4_CFG_BIT_VMID 7
>> #define ETM4_CFG_BIT_TS 11
>> #define ETM4_CFG_BIT_RETSTK 12
>> +#define ETM4_CFG_BIT_VMID_OPT 15
>>
>> static inline int coresight_get_trace_id(int cpu)
>> {
>> diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
>> index cad7bf783413..e6207ce7cc85 100644
>> --- a/tools/perf/arch/arm/util/cs-etm.c
>> +++ b/tools/perf/arch/arm/util/cs-etm.c
>> @@ -59,14 +59,15 @@ static const char *metadata_etmv4_ro[CS_ETMV4_PRIV_MAX] = {
>>
>> static bool cs_etm_is_etmv4(struct auxtrace_record *itr, int cpu);
>>
>> -static int cs_etm_set_context_id(struct auxtrace_record *itr,
>> - struct evsel *evsel, int cpu)
>> +static int cs_etm_set_pid(struct auxtrace_record *itr,
>> + struct evsel *evsel, int cpu)
>> {
>> struct cs_etm_recording *ptr;
>> struct perf_pmu *cs_etm_pmu;
>> char path[PATH_MAX];
>> int err = -EINVAL;
>> u32 val;
>> + u64 pid_fmt;
>>
>> ptr = container_of(itr, struct cs_etm_recording, itr);
>> cs_etm_pmu = ptr->cs_etm_pmu;
>> @@ -86,21 +87,43 @@ static int cs_etm_set_context_id(struct auxtrace_record *itr,
>> goto out;
>> }
>>
>> - /*
>> - * TRCIDR2.CIDSIZE, bit [9-5], indicates whether contextID tracing
>> - * is supported:
>> - * 0b00000 Context ID tracing is not supported.
>> - * 0b00100 Maximum of 32-bit Context ID size.
>> - * All other values are reserved.
>> - */
>> - val = BMVAL(val, 5, 9);
>> - if (!val || val != 0x4) {
>> + pid_fmt = perf_pmu__format_bits(&cs_etm_pmu->format, "pid");
>> + if (!pid_fmt)
>> + pid_fmt = 1ULL << ETM_OPT_CTXTID;
>
> If doesn't find "pid" format bits, should it mean the kernel is
> running an old version so doesn't support "pid" entry? It's good to
> add comment for this.
Yes, you're right. I will add it.
>
>> +
>> + switch (pid_fmt) {
>> + case (1ULL << ETM_OPT_CTXTID):
>> + /*
>> + * TRCIDR2.CIDSIZE, bit [9-5], indicates whether contextID tracing
>> + * is supported:
>> + * 0b00000 Context ID tracing is not supported.
>> + * 0b00100 Maximum of 32-bit Context ID size.
>> + * All other values are reserved.
>> + */
>> + val = BMVAL(val, 5, 9);
>> + if (!val || val != 0x4) {
>> + err = -EINVAL;
>> + goto out;
>> + }
>> + break;
>> + case (1ULL << ETM_OPT_CTXTID_IN_VMID):
>> + /*
>> + * TRCIDR2.VMIDOPT[30:29] != 0 and
>> + * TRCIDR2.VMIDSIZE[14:10] == 0b00100 (32bit virtual context id size)
>> + */
>> + if (!BMVAL(val, 29, 30) || BMVAL(val, 10, 14) < 4) {
>
> The comment is not alignment with the code. Based on the comment, the
> code should be:
>
> if (!BMVAL(val, 29, 30) || BMVAL(val, 10, 14) != 4) {
>
The reasoning is any value > 4, would imply a size > 32. This is really making
it future proof. I could restrict this to 4 now if you insist.
>> +#define ETM_SET_OPT_PID (1 << 0)
>> +#define ETM_SET_OPT_TS (1 << 1)
>> +#define ETM_SET_OPT_MASK (ETM_SET_OPT_PID | ETM_SET_OPT_TS)
>> +
>> static int cs_etm_set_option(struct auxtrace_record *itr,
>> struct evsel *evsel, u32 option)
>> {
>> @@ -169,17 +196,17 @@ static int cs_etm_set_option(struct auxtrace_record *itr,
>> !cpu_map__has(online_cpus, i))
>> continue;
>>
>> - if (option & ETM_OPT_CTXTID) {
>> - err = cs_etm_set_context_id(itr, evsel, i);
>> + if (option & ETM_SET_OPT_PID) {
>> + err = cs_etm_set_pid(itr, evsel, i);
>
> I don't understand what's the reason for introducing the new macros
> "ETM_SET_OPT_XXX", seems to me the old macros still can be used at
> here. Could you help explian for this?
Sure, with the change now, we either set ETM_OPT_CTXTID or ETM_OPT_CTXTID_IN_VMID
in the config. So, using the flag ETM_OPT_CTXTID to indicate the same is going to
be confusing. This makes it explicit and uses a separate flag. Or the in otherways
it is really making clear that we want to set PID tracing and de-coupling it from
"CONTEXTID" as it is not the correct config anymore and has to be determined at
runtime. I could make it explicit in a comment.
Cheers
Suzuki
>
> Thanks,
> Leo
>
>> if (err)
>> goto out;
>> }
>> - if (option & ETM_OPT_TS) {
>> + if (option & ETM_SET_OPT_TS) {
>> err = cs_etm_set_timestamp(itr, evsel, i);
>> if (err)
>> goto out;
>> }
>> - if (option & ~(ETM_OPT_CTXTID | ETM_OPT_TS))
>> + if (option & ~(ETM_SET_OPT_MASK))
>> /* Nothing else is currently supported */
>> goto out;
>> }
>> @@ -406,7 +433,7 @@ static int cs_etm_recording_options(struct auxtrace_record *itr,
>> evsel__set_sample_bit(cs_etm_evsel, CPU);
>>
>> err = cs_etm_set_option(itr, cs_etm_evsel,
>> - ETM_OPT_CTXTID | ETM_OPT_TS);
>> + ETM_SET_OPT_PID | ETM_SET_OPT_TS);
>> if (err)
>> goto out;
>> }
>> @@ -485,7 +512,9 @@ static u64 cs_etmv4_get_config(struct auxtrace_record *itr)
>> config |= BIT(ETM4_CFG_BIT_TS);
>> if (config_opts & BIT(ETM_OPT_RETSTK))
>> config |= BIT(ETM4_CFG_BIT_RETSTK);
>> -
>> + if (config_opts & BIT(ETM_OPT_CTXTID_IN_VMID))
>> + config |= BIT(ETM4_CFG_BIT_VMID) |
>> + BIT(ETM4_CFG_BIT_VMID_OPT);
>> return config;
>> }
>>
>> --
>> 2.24.1
>>
More information about the linux-arm-kernel
mailing list