[PATCH 0/3] coresight: etm-perf: Fix pid tracing with VHE

[PATCH 0/3] coresight: etm-perf: Fix pid tracing with VHE

[Suzuki K Poulose]

With the Virtualization Host Extensions, the kernel can run at EL2.
In this case the pid is written to CONTEXTIDR_EL2 instead of the
CONTEXTIDR_EL1. Thus the normal coresight tracing will be unable
to detect the PID of the thread generating the trace by looking
at the CONTEXTIDR_EL1. Thus, depending on the kernel EL, we must
switch to tracing the correct CONTEXTIDR register.

With VHE, we must set the TRCCONFIGR.VMID and TRCCONFIGR.VMID_OPT
to include the CONTEXTIDR_EL2 as the VMID in the trace. This
requires the perf tool to detect the changes in the TRCCONFIGR and
use the VMID / CID field for the PID. The challenge here is for
the perf tool to detect the kernel behavior.

Instead of the previously proposed invasive approaches, this set
implements a less intrusive mechanism, by playing with the
perf_event.attribute.config bits.

Some facts:

 - The perf tool requests pid tracing and timestamp in some
   scenarios. (e.g, system wide, task bound (!per-thread).

 - The default pid tracing is via requesting "contextid"
   But this only works for EL1 kernel.

 - "contextid" tracing is useful for tracing VMs (when
   we get to virtualization support). So we don't want
   to move this around.

So this patch series introduces two new format bits: 

- contextid_in_vmid -> Is only supported when the VMID tracing
  and CONTEXTIDR_EL2 both are supported. When requested the perf
  etm4x backend sets (TRCCONFIGR.VMID | TRCCONFIGR.VMID_OPT).
  As per ETMv4.4 TRM, when the core supports VHE, the CONTEXTIDR_EL2
  tracing is mandatory. (See the field TRCID2.VMIDOPT)

- pid -> Is an alias for the correct config to enable PID tracing
  on any kernel.
  i.e, in EL1 kernel -> pid == contextid
          EL2 kernel -> pid == contextid_in_vmid

With this, the perf tool is also updated to request the "pid"
tracing whenever available, falling back to "contextid" if it
is unavailable (to support new tool running on older kernels).

The perf tool will also set the TRCCONFIGR accordingly based
on the config bits, allowing the decoder to output the appropriate
fields.

I have another patch for the perf decoder to set the TID from VMID
when the cid is invalid and and the vmid is valid. But this doesn't
verify if the trcconfigr.vmid_opt was set. I will leave this to
Mike Leach to fix it properly.

Tested on Juno (EL1 kernel) and N1SDP (El2 kernel). Feedback welcome.

A tree with these patches are available here :

 git.gitlab.arm.com:linux-arm/linux-skp.git coresight/el2/pid

Suzuki K Poulose (3):
  coresight: etm-perf: Add support for PID tracing for kernel at EL2
  perf: cs_etm: Use pid tracing explicitly instead of contextid
  rfc: perf: cs_etm: Detect pid in VMID for kernel running at EL2

 .../hwtracing/coresight/coresight-etm-perf.c  | 14 ++++
 .../coresight/coresight-etm4x-core.c          |  9 +++
 include/linux/coresight-pmu.h                 | 11 ++--
 tools/include/linux/coresight-pmu.h           | 11 ++--
 tools/perf/arch/arm/util/cs-etm.c             | 65 ++++++++++++++-----
 .../perf/util/cs-etm-decoder/cs-etm-decoder.c | 28 ++++----
 6 files changed, 101 insertions(+), 37 deletions(-)

-- 
2.24.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/3] coresight: etm-perf: Add support for PID tracing for kernel at EL2

[Suzuki K Poulose]

When the kernel is running at EL2, the PID is stored in CONTEXTIDR_EL2.
So, tracing CONTEXTIDR_EL1 doesn't give us the pid of the process.
Thus we should trace the VMID with VMIDOPT set to trace
CONTEXTIDR_EL2 instead of VMID. Given that we have an existing
config option "contextid" and this will be useful for tracing
virtual machines (when we get to support virtualization). So instead,
this patch adds a new option, contextid_in_vmid as a separate config.
Thus on an EL2 kernel, we will have two options available for
the perf tool. However, to make it easier for the user to
do pid tracing, we add a new format which will default to
"contextid" (on EL1 kernel) or "contextid_in_vmid" (on EL2
kernel). So that the user doesn't have to bother which EL the
kernel is running.

 i.e, perf record -e cs_etm/pid/u --

will always do the "pid" tracing, independent of the kernel EL.

Also, the perf tool will be updated to automatically select
"pid" config instead of the "contextid" for system wide/CPU wide
mode.

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Al Grant <al.grant@arm.com>
Cc: Mike Leach <mike.leach@linaro.org>
Cc: Leo Yan <leo.yan@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-etm-perf.c   | 14 ++++++++++++++
 drivers/hwtracing/coresight/coresight-etm4x-core.c |  9 +++++++++
 include/linux/coresight-pmu.h                      | 11 +++++++----
 3 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index bdc34ca449f7..f763def145e4 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -30,14 +30,28 @@ static DEFINE_PER_CPU(struct coresight_device *, csdev_src);
 /* ETMv3.5/PTM's ETMCR is 'config' */
 PMU_FORMAT_ATTR(cycacc,		"config:" __stringify(ETM_OPT_CYCACC));
 PMU_FORMAT_ATTR(contextid,	"config:" __stringify(ETM_OPT_CTXTID));
+PMU_FORMAT_ATTR(contextid_in_vmid,	"config:" __stringify(ETM_OPT_CTXTID_IN_VMID));
 PMU_FORMAT_ATTR(timestamp,	"config:" __stringify(ETM_OPT_TS));
 PMU_FORMAT_ATTR(retstack,	"config:" __stringify(ETM_OPT_RETSTK));
 /* Sink ID - same for all ETMs */
 PMU_FORMAT_ATTR(sinkid,		"config2:0-31");
 
+static ssize_t format_attr_pid_show(struct device *dev,
+				    struct device_attribute *attr,
+				    char *page)
+{
+	int pid_fmt = is_kernel_in_hyp_mode() ? ETM_OPT_CTXTID_IN_VMID : ETM_OPT_CTXTID;
+
+	return sprintf(page, "config:%d\n", pid_fmt);
+}
+
+struct device_attribute format_attr_pid = __ATTR(pid, 0444, format_attr_pid_show, NULL);
+
 static struct attribute *etm_config_formats_attr[] = {
 	&format_attr_cycacc.attr,
 	&format_attr_contextid.attr,
+	&format_attr_contextid_in_vmid.attr,
+	&format_attr_pid.attr,
 	&format_attr_timestamp.attr,
 	&format_attr_retstack.attr,
 	&format_attr_sinkid.attr,
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index d78a37b6592c..8cb2cb1febce 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -382,6 +382,15 @@ static int etm4_parse_event_config(struct etmv4_drvdata *drvdata,
 		/* bit[6], Context ID tracing bit */
 		config->cfg |= BIT(ETM4_CFG_BIT_CTXTID);
 
+	/* Do not enable VMID tracing if we are not running in EL2 */
+	if (attr->config & BIT(ETM_OPT_CTXTID_IN_VMID)) {
+		if (!is_kernel_in_hyp_mode()) {
+			ret = -EINVAL;
+			goto out;
+		}
+		config->cfg |= BIT(ETM4_CFG_BIT_VMID) | BIT(ETM4_CFG_BIT_VMID_OPT);
+	}
+
 	/* return stack - enable if selected and supported */
 	if ((attr->config & BIT(ETM_OPT_RETSTK)) && drvdata->retstack)
 		/* bit[12], Return stack enable bit */
diff --git a/include/linux/coresight-pmu.h b/include/linux/coresight-pmu.h
index b0e35eec6499..927c6285ce5d 100644
--- a/include/linux/coresight-pmu.h
+++ b/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)
 {
-- 
2.24.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/3] perf: cs_etm: Use pid tracing explicitly instead of contextid

[Suzuki K Poulose]

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@linaro.org>
Cc: Mike Leach <mike.leach@linaro.org>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Al Grant <al.grant@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@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;
+
+	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) {
+			err = -EINVAL;
+			goto out;
+		}
+		break;
+	default:
 		err = -EINVAL;
 		goto out;
 	}
 
+
 	/* All good, let the kernel know */
-	evsel->core.attr.config |= (1 << ETM_OPT_CTXTID);
+	evsel->core.attr.config |= pid_fmt;
 	err = 0;
 
 out:
@@ -156,6 +179,10 @@ static int cs_etm_set_timestamp(struct auxtrace_record *itr,
 	return err;
 }
 
+#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);
 			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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Suzuki K Poulose]

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;
 }
-- 
2.24.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Suzuki K Poulose]

Mike,


Related to this patch, I have a question about the print decoder
in OpenCSD. Withe TRCCONFIGR.VMIDOPT, we have CONTEXTIDR_EL2 in
VMID instead of the real VMID. Does it make sense to change the
print decoder to reflect this in the o/p ?

i.e, instead of :

Idx:530; ID:14; I_ADDR_CTXT_L_64IS0 .. Ctxt: AArch64,EL0, NS; VMID=0xAA;

Could this be

Idx:530; ID:14; I_ADDR_CTXT_L_64IS0 .. Ctxt: AArch64,EL0, NS; CID2=0xAA;

with VMIDOPT ?

I understand it is a bit naive, and there are widespread users of
OpenCSD and it could potentially break any existing users. But worth
checking.

Cheers
Suzuki

On 11/10/20 6:33 PM, Suzuki K Poulose wrote:
> 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;
>   }
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Al Grant]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Suzuki K Poulose]

On 11/11/20 11:03 AM, Al Grant wrote:
> 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...

Please see my comment under the patch. This is precisely why it is an RFC.
As I said, unless we know that VMIDOPT == 1, we cant assume VMID is tid.

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

Yes, and it is ignored if CONTEXTID is valid.

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

Do you mean the CID is invalid in the trace or CID == 0 ? For a VM case, it
may still be valid (when we get to virtualization).

> 
> This seems the wrong way round. 

Why ?

> 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

This is still possible. The perf session could set :

contextid

and this would trace the PID of the guest applications in the CID (as the
guest kernel is at EL1). But if you want to trace guest applications of multiple
VMs, that becomes tricky (because we now have VMID and CID and the perf tool
must be explicitly taught how to decode the data) and I would rather have separate
perf sessions attached to the VMs.

This is precisely why we set "two" separate config bits for the
contextid and contextid_in_vmid. See my kernel patch description and
the cover letter.

> 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.

Yes, this is already part of the kernel patch. See :


+	/* Do not enable VMID tracing if we are not running in EL2 */
+	if (attr->config & BIT(ETM_OPT_CTXTID_IN_VMID)) {
+		if (!is_kernel_in_hyp_mode()) {
+			ret = -EINVAL;
+			goto out;
+		}
+		config->cfg |= BIT(ETM4_CFG_BIT_VMID) | BIT(ETM4_CFG_BIT_VMID_OPT);
+	}


> 
> 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

This is what the perf tool patch does now. Except for the decoder, which doesn't
have the info about VMID_OPT. I didn't want to hack it terribly bad to incorporate
this. I guess Mike understands this code better than I do and could do it neater.

> 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.

This may not be entirely correct, unless we correlate the VMID_OPT. Because,
when we add Virtualization support (vmid == actual vmid).

So we should do:
        if (VMID && VMIDOPT) {
	   tid = pid;
	} else if (CID) {

         }

Suzuki


> 
> Al
> 
> 
> 
>> --
>> 2.24.1
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] perf: cs_etm: Use pid tracing explicitly instead of contextid

[Leo Yan]

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@linaro.org>
> Cc: Mike Leach <mike.leach@linaro.org>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: Al Grant <al.grant@arm.com>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@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.

> +
> +	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) {

> +			err = -EINVAL;
> +			goto out;
> +		}
> +		break;
> +	default:
>  		err = -EINVAL;
>  		goto out;
>  	}
>  
> +
>  	/* All good, let the kernel know */
> -	evsel->core.attr.config |= (1 << ETM_OPT_CTXTID);
> +	evsel->core.attr.config |= pid_fmt;
>  	err = 0;
>  
>  out:
> @@ -156,6 +179,10 @@ static int cs_etm_set_timestamp(struct auxtrace_record *itr,
>  	return err;
>  }
>  
> +#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?

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
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] coresight: etm-perf: Add support for PID tracing for kernel at EL2

[Leo Yan]

On Tue, Nov 10, 2020 at 06:33:11PM +0000, Suzuki Kuruppassery Poulose wrote:
> When the kernel is running at EL2, the PID is stored in CONTEXTIDR_EL2.
> So, tracing CONTEXTIDR_EL1 doesn't give us the pid of the process.
> Thus we should trace the VMID with VMIDOPT set to trace
> CONTEXTIDR_EL2 instead of VMID. Given that we have an existing
> config option "contextid" and this will be useful for tracing
> virtual machines (when we get to support virtualization). So instead,
> this patch adds a new option, contextid_in_vmid as a separate config.
> Thus on an EL2 kernel, we will have two options available for
> the perf tool. However, to make it easier for the user to
> do pid tracing, we add a new format which will default to
> "contextid" (on EL1 kernel) or "contextid_in_vmid" (on EL2
> kernel). So that the user doesn't have to bother which EL the
> kernel is running.
> 
>  i.e, perf record -e cs_etm/pid/u --
> 
> will always do the "pid" tracing, independent of the kernel EL.
> 
> Also, the perf tool will be updated to automatically select
> "pid" config instead of the "contextid" for system wide/CPU wide
> mode.
> 
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: Al Grant <al.grant@arm.com>
> Cc: Mike Leach <mike.leach@linaro.org>
> Cc: Leo Yan <leo.yan@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  drivers/hwtracing/coresight/coresight-etm-perf.c   | 14 ++++++++++++++
>  drivers/hwtracing/coresight/coresight-etm4x-core.c |  9 +++++++++
>  include/linux/coresight-pmu.h                      | 11 +++++++----
>  3 files changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> index bdc34ca449f7..f763def145e4 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -30,14 +30,28 @@ static DEFINE_PER_CPU(struct coresight_device *, csdev_src);
>  /* ETMv3.5/PTM's ETMCR is 'config' */
>  PMU_FORMAT_ATTR(cycacc,		"config:" __stringify(ETM_OPT_CYCACC));
>  PMU_FORMAT_ATTR(contextid,	"config:" __stringify(ETM_OPT_CTXTID));
> +PMU_FORMAT_ATTR(contextid_in_vmid,	"config:" __stringify(ETM_OPT_CTXTID_IN_VMID));
>  PMU_FORMAT_ATTR(timestamp,	"config:" __stringify(ETM_OPT_TS));
>  PMU_FORMAT_ATTR(retstack,	"config:" __stringify(ETM_OPT_RETSTK));
>  /* Sink ID - same for all ETMs */
>  PMU_FORMAT_ATTR(sinkid,		"config2:0-31");
>  
> +static ssize_t format_attr_pid_show(struct device *dev,
> +				    struct device_attribute *attr,
> +				    char *page)
> +{
> +	int pid_fmt = is_kernel_in_hyp_mode() ? ETM_OPT_CTXTID_IN_VMID : ETM_OPT_CTXTID;
> +
> +	return sprintf(page, "config:%d\n", pid_fmt);
> +}
> +
> +struct device_attribute format_attr_pid = __ATTR(pid, 0444, format_attr_pid_show, NULL);
> +
>  static struct attribute *etm_config_formats_attr[] = {
>  	&format_attr_cycacc.attr,
>  	&format_attr_contextid.attr,
> +	&format_attr_contextid_in_vmid.attr,
> +	&format_attr_pid.attr,
>  	&format_attr_timestamp.attr,
>  	&format_attr_retstack.attr,
>  	&format_attr_sinkid.attr,
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index d78a37b6592c..8cb2cb1febce 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -382,6 +382,15 @@ static int etm4_parse_event_config(struct etmv4_drvdata *drvdata,
>  		/* bit[6], Context ID tracing bit */
>  		config->cfg |= BIT(ETM4_CFG_BIT_CTXTID);
>  
> +	/* Do not enable VMID tracing if we are not running in EL2 */
> +	if (attr->config & BIT(ETM_OPT_CTXTID_IN_VMID)) {
> +		if (!is_kernel_in_hyp_mode()) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +		config->cfg |= BIT(ETM4_CFG_BIT_VMID) | BIT(ETM4_CFG_BIT_VMID_OPT);
> +	}
> +

Nitpick: should the driver need to check the kernel running mode for the
config ETM_OPT_CTXTID?  I.e. if the BIT(ETM_OPT_CTXTID) is set in
"attr->config", does it mean the kernel should run in EL1 mode?

Just curious, does there have a scenario that we need to set BIT
ETM4_CFG_BIT_VMID but the bit ETM4_CFG_BIT_VMID_OPT is cleared?  I
think a bit for this, but cannot find a situation for tracing context
ID based on VTTBR_EL2.VMID.  So this might be not a question for us.

Thanks,
Leo

>  	/* return stack - enable if selected and supported */
>  	if ((attr->config & BIT(ETM_OPT_RETSTK)) && drvdata->retstack)
>  		/* bit[12], Return stack enable bit */
> diff --git a/include/linux/coresight-pmu.h b/include/linux/coresight-pmu.h
> index b0e35eec6499..927c6285ce5d 100644
> --- a/include/linux/coresight-pmu.h
> +++ b/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)
>  {
> -- 
> 2.24.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] perf: cs_etm: Use pid tracing explicitly instead of contextid

[Suzuki K Poulose]

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@linaro.org>
>> Cc: Mike Leach <mike.leach@linaro.org>
>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Cc: Al Grant <al.grant@arm.com>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@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
>>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] perf: cs_etm: Use pid tracing explicitly instead of contextid

[Leo Yan]

On Thu, Nov 12, 2020 at 10:54:23AM +0000, Suzuki Kuruppassery Poulose wrote:
> Hi Leo,
> 
> Thanks for looking at the patch.

Welcome!

[...]

> > > +	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.

So here you want to filter out the cases for 16-bit and 8-bit virtual
context ID size, but want to leave the possibility for support size >
32-bit in future.

It's fine to keep current code, but it's better to add description in
the comment, otherwise, this might be difficult for maintenance.

> > > +#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.

Makes sense for me; thanks for the explanation.

Leo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] coresight: etm-perf: Add support for PID tracing for kernel at EL2

[Suzuki K Poulose]

On 11/12/20 10:27 AM, Leo Yan wrote:
> On Tue, Nov 10, 2020 at 06:33:11PM +0000, Suzuki Kuruppassery Poulose wrote:
>> When the kernel is running at EL2, the PID is stored in CONTEXTIDR_EL2.
>> So, tracing CONTEXTIDR_EL1 doesn't give us the pid of the process.
>> Thus we should trace the VMID with VMIDOPT set to trace
>> CONTEXTIDR_EL2 instead of VMID. Given that we have an existing
>> config option "contextid" and this will be useful for tracing
>> virtual machines (when we get to support virtualization). So instead,
>> this patch adds a new option, contextid_in_vmid as a separate config.
>> Thus on an EL2 kernel, we will have two options available for
>> the perf tool. However, to make it easier for the user to
>> do pid tracing, we add a new format which will default to
>> "contextid" (on EL1 kernel) or "contextid_in_vmid" (on EL2
>> kernel). So that the user doesn't have to bother which EL the
>> kernel is running.
>>
>>   i.e, perf record -e cs_etm/pid/u --
>>
>> will always do the "pid" tracing, independent of the kernel EL.
>>
>> Also, the perf tool will be updated to automatically select
>> "pid" config instead of the "contextid" for system wide/CPU wide
>> mode.
>>
>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Cc: Al Grant <al.grant@arm.com>
>> Cc: Mike Leach <mike.leach@linaro.org>
>> Cc: Leo Yan <leo.yan@linaro.org>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>>   drivers/hwtracing/coresight/coresight-etm-perf.c   | 14 ++++++++++++++
>>   drivers/hwtracing/coresight/coresight-etm4x-core.c |  9 +++++++++
>>   include/linux/coresight-pmu.h                      | 11 +++++++----
>>   3 files changed, 30 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
>> index bdc34ca449f7..f763def145e4 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
>> @@ -30,14 +30,28 @@ static DEFINE_PER_CPU(struct coresight_device *, csdev_src);
>>   /* ETMv3.5/PTM's ETMCR is 'config' */
>>   PMU_FORMAT_ATTR(cycacc,		"config:" __stringify(ETM_OPT_CYCACC));
>>   PMU_FORMAT_ATTR(contextid,	"config:" __stringify(ETM_OPT_CTXTID));
>> +PMU_FORMAT_ATTR(contextid_in_vmid,	"config:" __stringify(ETM_OPT_CTXTID_IN_VMID));
>>   PMU_FORMAT_ATTR(timestamp,	"config:" __stringify(ETM_OPT_TS));
>>   PMU_FORMAT_ATTR(retstack,	"config:" __stringify(ETM_OPT_RETSTK));
>>   /* Sink ID - same for all ETMs */
>>   PMU_FORMAT_ATTR(sinkid,		"config2:0-31");
>>   
>> +static ssize_t format_attr_pid_show(struct device *dev,
>> +				    struct device_attribute *attr,
>> +				    char *page)
>> +{
>> +	int pid_fmt = is_kernel_in_hyp_mode() ? ETM_OPT_CTXTID_IN_VMID : ETM_OPT_CTXTID;
>> +
>> +	return sprintf(page, "config:%d\n", pid_fmt);
>> +}
>> +
>> +struct device_attribute format_attr_pid = __ATTR(pid, 0444, format_attr_pid_show, NULL);
>> +
>>   static struct attribute *etm_config_formats_attr[] = {
>>   	&format_attr_cycacc.attr,
>>   	&format_attr_contextid.attr,
>> +	&format_attr_contextid_in_vmid.attr,
>> +	&format_attr_pid.attr,
>>   	&format_attr_timestamp.attr,
>>   	&format_attr_retstack.attr,
>>   	&format_attr_sinkid.attr,
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> index d78a37b6592c..8cb2cb1febce 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> @@ -382,6 +382,15 @@ static int etm4_parse_event_config(struct etmv4_drvdata *drvdata,
>>   		/* bit[6], Context ID tracing bit */
>>   		config->cfg |= BIT(ETM4_CFG_BIT_CTXTID);
>>   
>> +	/* Do not enable VMID tracing if we are not running in EL2 */
>> +	if (attr->config & BIT(ETM_OPT_CTXTID_IN_VMID)) {
>> +		if (!is_kernel_in_hyp_mode()) {
>> +			ret = -EINVAL;
>> +			goto out;
>> +		}
>> +		config->cfg |= BIT(ETM4_CFG_BIT_VMID) | BIT(ETM4_CFG_BIT_VMID_OPT);
>> +	}
>> +
> 
> Nitpick: should the driver need to check the kernel running mode for the
> config ETM_OPT_CTXTID?  I.e. if the BIT(ETM_OPT_CTXTID) is set in
> "attr->config", does it mean the kernel should run in EL1 mode?

That doesn't matter. Irrespective of the kernel EL, CTXTIDR_EL1 could
have some value in it. e.g, tracing a VM, CTXTIDR_El1 could have the
guest process PID. So, we should simply leave it for the user to decide.

> 
> Just curious, does there have a scenario that we need to set BIT
> ETM4_CFG_BIT_VMID but the bit ETM4_CFG_BIT_VMID_OPT is cleared?  I
> think a bit for this, but cannot find a situation for tracing context
> ID based on VTTBR_EL2.VMID.  So this might be not a question for us.

Yes, it is possible to use to trace the VMID. e.g,tracing multiple VMs.
That way VMID could be used to separate the trace stream. This series
doesn't affect that. We could add a separate "config" bit for tracing
VMID, but make sure that selected configs don't conflict.

i.,e if (config & ETM_OPT_VMID) && (config & ETM_OPT_CTXTID_IN_VMID)
	error !

Thanks
Suzuki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Leo Yan]

On Wed, Nov 11, 2020 at 11:40:03AM +0000, Suzuki Kuruppassery Poulose wrote:
> On 11/11/20 11:03 AM, Al Grant wrote:

[...]

> > 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.
> 
> This may not be entirely correct, unless we correlate the VMID_OPT. Because,
> when we add Virtualization support (vmid == actual vmid).
> 
> So we should do:
>        if (VMID && VMIDOPT) {
> 	   tid = pid;
> 	} else if (CID) {
> 
>         }

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.

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);

Thanks,
Leo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Suzuki K Poulose]

Hi Leo

On 11/13/20 12:11 AM, Leo Yan wrote:
> On Wed, Nov 11, 2020 at 11:40:03AM +0000, Suzuki Kuruppassery Poulose wrote:
>> On 11/11/20 11:03 AM, Al Grant wrote:
> 
> [...]
> 
>>> 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.
>>
>> This may not be entirely correct, unless we correlate the VMID_OPT. Because,
>> when we add Virtualization support (vmid == actual vmid).
>>
>> So we should do:
>>         if (VMID && VMIDOPT) {
>> 	   tid = pid;
>> 	} else if (CID) {
>>
>>          }
> 
> 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).

Cheers
Suzuki



> 
> Thanks,
> Leo
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Leo Yan]

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).

You are welcome!  Sure, I will try to work out a change and share
back.

Thanks,
Leo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Leo Yan]

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".

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Daniel Kiss]

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".
> 
> 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Leo Yan]

Hi Denis,

On Tue, Dec 22, 2020 at 01:57:41AM -0800, Denis Nikitin wrote:

[...]

> > > 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.

Thanks a lot for the testing!

> 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.

Agreed.  After reading the code and I think it's possible to do an extra
checking the length of auxtrace info structure so that can know if
the item CS_ETMV4_PID_FMT is valid or not.  Thus we needs to write a
parity function of intel_pt_has() for perf cs-etm; I will try to rework
this patch.

> This is a critical fix for Chrome OS. Please let me know what you think.

So are you asking to upstream the changes to mainline kernel?  You
could see this patch is owned by Suzuki and I only proposed for one
patch for perf related change.

Suzuki, could you give some update for the plan of this patch set?
I can help to prepare the perf patch based on Suzuki's plan.

Thanks,
Leo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Suzuki K Poulose]

Hi Leo,

On 12/23/20 8:05 AM, Leo Yan wrote:
> Hi Denis,
> 
> On Tue, Dec 22, 2020 at 01:57:41AM -0800, Denis Nikitin wrote:
> 
> [...]
> 
>>>> 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.
> 
> Thanks a lot for the testing!
> 
>> 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.
> 
> Agreed.  After reading the code and I think it's possible to do an extra
> checking the length of auxtrace info structure so that can know if
> the item CS_ETMV4_PID_FMT is valid or not.  Thus we needs to write a
> parity function of intel_pt_has() for perf cs-etm; I will try to rework
> this patch.
> 
>> This is a critical fix for Chrome OS. Please let me know what you think.
> 
> So are you asking to upstream the changes to mainline kernel?  You
> could see this patch is owned by Suzuki and I only proposed for one
> patch for perf related change.
> 
> Suzuki, could you give some update for the plan of this patch set?
> I can help to prepare the perf patch based on Suzuki's plan.

I am happy for you to pick up the perf tool changes as needed. I believe
there are no changes required for the patches, 1 & 2, which adds the basic
kernel and perf tool support. As I have mentioned in the description of this
patch, this is clearly an RFC and is a hack. So I am happy that you can
fix this properly in perf tool decoding.

Mathieu,

Are you happy with the proposed series to solve this issue ? I could respin
the series on the latest upstream tree if you like.

Kind regards
Suzuki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mathieu Poirier]

Hi Suzuki,

On Mon, Jan 04, 2021 at 05:33:50PM +0000, Suzuki K Poulose wrote:
> Hi Leo,
> 
> On 12/23/20 8:05 AM, Leo Yan wrote:
> > Hi Denis,
> > 
> > On Tue, Dec 22, 2020 at 01:57:41AM -0800, Denis Nikitin wrote:
> > 
> > [...]
> > 
> > > > > 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.
> > 
> > Thanks a lot for the testing!
> > 
> > > 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.
> > 
> > Agreed.  After reading the code and I think it's possible to do an extra
> > checking the length of auxtrace info structure so that can know if
> > the item CS_ETMV4_PID_FMT is valid or not.  Thus we needs to write a
> > parity function of intel_pt_has() for perf cs-etm; I will try to rework
> > this patch.
> > 
> > > This is a critical fix for Chrome OS. Please let me know what you think.
> > 
> > So are you asking to upstream the changes to mainline kernel?  You
> > could see this patch is owned by Suzuki and I only proposed for one
> > patch for perf related change.
> > 
> > Suzuki, could you give some update for the plan of this patch set?
> > I can help to prepare the perf patch based on Suzuki's plan.
> 
> I am happy for you to pick up the perf tool changes as needed. I believe
> there are no changes required for the patches, 1 & 2, which adds the basic
> kernel and perf tool support. As I have mentioned in the description of this
> patch, this is clearly an RFC and is a hack. So I am happy that you can
> fix this properly in perf tool decoding.
>
> Mathieu,
> 
> Are you happy with the proposed series to solve this issue ? I could respin
> the series on the latest upstream tree if you like.

Looking at my (extensive) patch log this morning I was wondering what to do
about that patchset.  Yes, please respin it on rc2 and I'll look at it.

Reading through the above conversation with Leo, should I expect your next
patchset to replace your perf tools changes with Leo's work?  Alternatively and
knowing Leo is working on it I'd be happy with just the kernel changes. 

Thanks,
Mathieu

> 
> Kind regards
> Suzuki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel