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

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

[Leo Yan]

This patch series is to support PID tracing with Virtualization Host
Extensions (VHE).

Since the patch series v1 was sent out for reviewing, we had some
discussion and finalized the solution which is implemented in this
version.  Simply to say, to be backward compatibility, and can both
support PID tracing for the kernel is running at either EL1 or EL2,
the two new PMU formats "contextid1" and "contextid2" are introduced,
which works as a switch to trace PID for EL1 kernel and EL2 kernel
respectively.

The existed PMU format "contextid" needs to be backward compatible for
users, it's changed to an alias for "contextid1" on EL1 kernel and for
"contextid2" on EL2 kernel.  Therefore, even without setting "contextid"
config, the perf tool can dynamically pick up the config for PID
tracing, so the user doesn't have to set the "contexid" config manually.

As results, we now have three PMU formats, for easier understanding the
implementation, just copy and paste the descriptions for these three PMU
formats from the patch 07/07:

  "contextid1": Available on both EL1 kernel and EL2 kernel.  When the
                kernel is running at EL1, "contextid1" enables the PID
                tracing; when the kernel is running at EL2, this enables
                tracing the PID of guest applications.

  "contextid2": Only usable when the kernel is running at EL2.  When
                selected, enables PID tracing on EL2 kernel.

  "contextid":  Will be an alias for the option that enables PID
                tracing.  I.e,
                contextid == contextid1, on EL1 kernel.
                contextid == contextid2, on EL2 kernel.

This patch series can be cleanly applied on perf/core branch:

  commit cd07e536b020 ("Merge remote-tracking branch 'torvalds/master' into perf/core")

... and applied on the mainline kernel:

  commit 88bb507a74ea ("Merge tag 'media/v5.11-3' of git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media")

The patch series has been tested on Arm Juno-r2 board.  Verified the
kernel with EL1 and didn't find issue; though absenting the platform for
kernel with EL2, after some hacking in kernel driver and tool to emulate
the code paths for kernel on EL2, can see the code path is hit without
failure.


Changes from v1:
* Refactored PMU formats, added formats "contextid1"/"contextid2", and
  reworked format "contextid" (Suzuki/Mathieu);
* Refined the comments for perf configs (Leo/Mike);
* Added patch 07/07 for description PID tracing in docs;
* Found the issue for bitmap for option, extracted patch 03/07 for the
  fixing.

Changes from RFC:
* Added comments to clarify cases requested (Leo);
* Explain the change to generic flags for cs_etm_set_option() in the
  commit description;
* Stored PID format in metadata and passed it to decoder (Leo);
* Enhanced cs-etm for backward compatibility (Denis Nikitin).


Leo Yan (3):
  coresight: etm-perf: Clarify comment on perf options
  perf cs-etm: Add helper cs_etm__get_pid_fmt()
  Documentation: coresight: Add PID tracing description

Suzuki K Poulose (4):
  coresight: etm-perf: Support PID tracing for kernel at EL2
  perf cs-etm: Fix bitmap for option
  perf cs-etm: Support PID tracing in config
  perf cs-etm: Detect pid in VMID for kernel running at EL2

 Documentation/trace/coresight/coresight.rst   | 37 ++++++++++
 .../hwtracing/coresight/coresight-etm-perf.c  | 32 +++++++-
 .../coresight/coresight-etm4x-core.c          | 13 ++++
 include/linux/coresight-pmu.h                 | 20 +++--
 tools/include/linux/coresight-pmu.h           | 20 +++--
 tools/perf/arch/arm/util/cs-etm.c             | 73 +++++++++++++++----
 .../perf/util/cs-etm-decoder/cs-etm-decoder.c | 32 +++++++-
 tools/perf/util/cs-etm.c                      | 43 +++++++++++
 tools/perf/util/cs-etm.h                      |  1 +
 9 files changed, 239 insertions(+), 32 deletions(-)

-- 
2.25.1

[PATCH v2 1/7] coresight: etm-perf: Clarify comment on perf options

[Leo Yan]

In theory, the options should be arbitrary values and are neutral for
any ETM version; so far perf tool uses ETMv3.5/PTM ETMCR config bits
except for register's bit definitions, also uses as options.

This can introduce confusion, especially if we want to add a new option
but the new option is not supported by ETMv3.5/PTM ETMCR.  But on the
other hand, we cannot change options since these options are generic
CoreSight PMU ABI.

For easier maintenance and avoid confusion, this patch refines the
comment to clarify perf options, and gives out the background info for
these bits are coming from ETMv3.5/PTM.  Afterwards, we should take
these options as general knobs, and if there have any confliction with
ETMv3.5/PTM, should consider to define saperate macros for ETMv3.5/PTM
ETMCR config bits.

Suggested-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 .../hwtracing/coresight/coresight-etm-perf.c    |  5 ++++-
 include/linux/coresight-pmu.h                   | 17 ++++++++++++-----
 tools/include/linux/coresight-pmu.h             | 17 ++++++++++++-----
 3 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index bdc34ca449f7..465ef1aa8c82 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -27,7 +27,10 @@ static bool etm_perf_up;
 static DEFINE_PER_CPU(struct perf_output_handle, ctx_handle);
 static DEFINE_PER_CPU(struct coresight_device *, csdev_src);
 
-/* ETMv3.5/PTM's ETMCR is 'config' */
+/*
+ * The PMU formats were orignally for ETMv3.5/PTM's ETMCR 'config';
+ * now take them as general formats and apply on all ETMs.
+ */
 PMU_FORMAT_ATTR(cycacc,		"config:" __stringify(ETM_OPT_CYCACC));
 PMU_FORMAT_ATTR(contextid,	"config:" __stringify(ETM_OPT_CTXTID));
 PMU_FORMAT_ATTR(timestamp,	"config:" __stringify(ETM_OPT_TS));
diff --git a/include/linux/coresight-pmu.h b/include/linux/coresight-pmu.h
index b0e35eec6499..5dc47cfdcf07 100644
--- a/include/linux/coresight-pmu.h
+++ b/include/linux/coresight-pmu.h
@@ -10,11 +10,18 @@
 #define CORESIGHT_ETM_PMU_NAME "cs_etm"
 #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
+/*
+ * Below are the definition of bit offsets for perf option, and works as
+ * arbitrary values for all ETM versions.
+ *
+ * Most of them are orignally from ETMv3.5/PTM's ETMCR config, therefore,
+ * ETMv3.5/PTM doesn't define ETMCR config bits with prefix "ETM3_" and
+ * directly use below macros as config bits.
+ */
+#define ETM_OPT_CYCACC		12
+#define ETM_OPT_CTXTID		14
+#define ETM_OPT_TS		28
+#define ETM_OPT_RETSTK		29
 
 /* ETMv4 CONFIGR programming bits for the ETM OPTs */
 #define ETM4_CFG_BIT_CYCACC	4
diff --git a/tools/include/linux/coresight-pmu.h b/tools/include/linux/coresight-pmu.h
index b0e35eec6499..5dc47cfdcf07 100644
--- a/tools/include/linux/coresight-pmu.h
+++ b/tools/include/linux/coresight-pmu.h
@@ -10,11 +10,18 @@
 #define CORESIGHT_ETM_PMU_NAME "cs_etm"
 #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
+/*
+ * Below are the definition of bit offsets for perf option, and works as
+ * arbitrary values for all ETM versions.
+ *
+ * Most of them are orignally from ETMv3.5/PTM's ETMCR config, therefore,
+ * ETMv3.5/PTM doesn't define ETMCR config bits with prefix "ETM3_" and
+ * directly use below macros as config bits.
+ */
+#define ETM_OPT_CYCACC		12
+#define ETM_OPT_CTXTID		14
+#define ETM_OPT_TS		28
+#define ETM_OPT_RETSTK		29
 
 /* ETMv4 CONFIGR programming bits for the ETM OPTs */
 #define ETM4_CFG_BIT_CYCACC	4
-- 
2.25.1

[PATCH v2 2/7] coresight: etm-perf: Support PID tracing for kernel at EL2

[Leo Yan]

From: Suzuki K Poulose <suzuki.poulose@arm.com>

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 CONTEXTIDR_EL1.  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 extends option CTXTID with an extra bit
ETM_OPT_CTXTID2 (bit 15), thus on an EL2 kernel, we will have another
bit available for the perf tool: ETM_OPT_CTXTID is for kernel running in
EL1, ETM_OPT_CTXTID2 is used when kernel runs in EL2 with VHE enabled.

The tool must be backward compatible for users, i.e, "contextid" today
traces PID and that should remain the same; for this purpose, the perf
tool is updated to automatically set corresponding bit for the
"contextid" config, therefore, the user doesn't have to bother which EL
the kernel is running.

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

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

The driver parses the format "contextid", which traces CONTEXTIDR_EL1
for ETM_OPT_CTXTID (on EL1 kernel) and traces CONTEXTIDR_EL2 for
ETM_OPT_CTXTID2 (on EL2 kernel).

Besides the enhancement for format "contexid", extra two formats are
introduced: "contextid1" and "contextid2".  This considers to support
tracing both CONTEXTIDR_EL1 and CONTEXTIDR_EL2 when the kernel is
running at EL2.  Finally, the PMU formats are defined as follow:

  "contextid1": Available on both EL1 kernel and EL2 kernel.  When the
                kernel is running at EL1, "contextid1" enables the PID
		tracing; when the kernel is running at EL2, this enables
		tracing the PID of guest applications.

  "contextid2": Only usable when the kernel is running at EL2.  When
                selected, enables PID tracing on EL2 kernel.

  "contextid":  Will be an alias for the option that enables PID
                tracing.  I.e,
                contextid == contextid1, on EL1 kernel.
                contextid == contextid2, on EL2 kernel.

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>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 .../hwtracing/coresight/coresight-etm-perf.c  | 27 ++++++++++++++++++-
 .../coresight/coresight-etm4x-core.c          | 13 +++++++++
 include/linux/coresight-pmu.h                 |  3 +++
 3 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 465ef1aa8c82..0f603b4094f2 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -32,15 +32,40 @@ static DEFINE_PER_CPU(struct coresight_device *, csdev_src);
  * now take them as general formats and apply on all ETMs.
  */
 PMU_FORMAT_ATTR(cycacc,		"config:" __stringify(ETM_OPT_CYCACC));
-PMU_FORMAT_ATTR(contextid,	"config:" __stringify(ETM_OPT_CTXTID));
+/* contextid1 enables tracing CONTEXTIDR_EL1 for ETMv4 */
+PMU_FORMAT_ATTR(contextid1,	"config:" __stringify(ETM_OPT_CTXTID));
+/* contextid2 enables tracing CONTEXTIDR_EL2 for ETMv4 */
+PMU_FORMAT_ATTR(contextid2,	"config:" __stringify(ETM_OPT_CTXTID2));
 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");
 
+/*
+ * contextid always traces the "PID".  The PID is in CONTEXTIDR_EL1
+ * when the kernel is running at EL1; when the kernel is at EL2,
+ * the PID is in CONTEXTIDR_EL2.
+ */
+static ssize_t format_attr_contextid_show(struct device *dev,
+					  struct device_attribute *attr,
+					  char *page)
+{
+	int pid_fmt = ETM_OPT_CTXTID;
+
+#if defined(CONFIG_CORESIGHT_SOURCE_ETM4X)
+	pid_fmt = is_kernel_in_hyp_mode() ? ETM_OPT_CTXTID2 : ETM_OPT_CTXTID;
+#endif
+	return sprintf(page, "config:%d\n", pid_fmt);
+}
+
+struct device_attribute format_attr_contextid =
+	__ATTR(contextid, 0444, format_attr_contextid_show, NULL);
+
 static struct attribute *etm_config_formats_attr[] = {
 	&format_attr_cycacc.attr,
 	&format_attr_contextid.attr,
+	&format_attr_contextid1.attr,
+	&format_attr_contextid2.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 b20b6ff17cf6..8681c225b0ba 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -477,6 +477,19 @@ static int etm4_parse_event_config(struct etmv4_drvdata *drvdata,
 		/* bit[6], Context ID tracing bit */
 		config->cfg |= BIT(ETM4_CFG_BIT_CTXTID);
 
+	/*
+	 * If set bit ETM_OPT_CTXTID2 in perf config, this asks to trace VMID
+	 * for recording CONTEXTIDR_EL2.  Do not enable VMID tracing if the
+	 * kernel is not running in EL2.
+	 */
+	if (attr->config & BIT(ETM_OPT_CTXTID2)) {
+		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 5dc47cfdcf07..4ac5c081af93 100644
--- a/include/linux/coresight-pmu.h
+++ b/include/linux/coresight-pmu.h
@@ -20,14 +20,17 @@
  */
 #define ETM_OPT_CYCACC		12
 #define ETM_OPT_CTXTID		14
+#define ETM_OPT_CTXTID2		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.25.1

[PATCH v2 3/7] perf cs-etm: Fix bitmap for option

[Leo Yan]

From: Suzuki K Poulose <suzuki.poulose@arm.com>

When set option with macros ETM_OPT_CTXTID and ETM_OPT_TS, it wrongly
takes these two values (14 and 28 prespectively) as bit masks, but
actually both are the offset for bits.  But this doesn't lead to
further failure due to the AND logic operation will be always true for
ETM_OPT_CTXTID / ETM_OPT_TS.

This patch defines new independent macros (rather than using the
"config" bits) for requesting the "contextid" and "timestamp" for
cs_etm_set_option().

[leoy: Extract the change as a separate patch for easier review]
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 tools/perf/arch/arm/util/cs-etm.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
index bd446aba64f7..c25c878fd06c 100644
--- a/tools/perf/arch/arm/util/cs-etm.c
+++ b/tools/perf/arch/arm/util/cs-etm.c
@@ -156,6 +156,10 @@ static int cs_etm_set_timestamp(struct auxtrace_record *itr,
 	return err;
 }
 
+#define ETM_SET_OPT_CTXTID	(1 << 0)
+#define ETM_SET_OPT_TS		(1 << 1)
+#define ETM_SET_OPT_MASK	(ETM_SET_OPT_CTXTID | ETM_SET_OPT_TS)
+
 static int cs_etm_set_option(struct auxtrace_record *itr,
 			     struct evsel *evsel, u32 option)
 {
@@ -169,17 +173,17 @@ static int cs_etm_set_option(struct auxtrace_record *itr,
 		    !cpu_map__has(online_cpus, i))
 			continue;
 
-		if (option & ETM_OPT_CTXTID) {
+		if (option & ETM_SET_OPT_CTXTID) {
 			err = cs_etm_set_context_id(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 +410,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_CTXTID | ETM_SET_OPT_TS);
 		if (err)
 			goto out;
 	}
-- 
2.25.1

[PATCH v2 4/7] perf cs-etm: Support PID tracing in config

[Leo Yan]

From: Suzuki K Poulose <suzuki.poulose@arm.com>

If the kernel is running at EL2, the pid of a task is exposed via VMID
instead of the CONTEXTID.  Add support for this in the perf tool.

This patch respects user setting if user has specified any configs
from "contextid", "contextid1" or "contextid2"; otherwise, it
dynamically sets config based on PMU format "contextid".

Cc: Mike Leach <mike.leach@linaro.org>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Al Grant <al.grant@arm.com>
Co-developed-by: Leo Yan <leo.yan@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 tools/include/linux/coresight-pmu.h |  3 ++
 tools/perf/arch/arm/util/cs-etm.c   | 61 +++++++++++++++++++++++------
 2 files changed, 52 insertions(+), 12 deletions(-)

diff --git a/tools/include/linux/coresight-pmu.h b/tools/include/linux/coresight-pmu.h
index 5dc47cfdcf07..4ac5c081af93 100644
--- a/tools/include/linux/coresight-pmu.h
+++ b/tools/include/linux/coresight-pmu.h
@@ -20,14 +20,17 @@
  */
 #define ETM_OPT_CYCACC		12
 #define ETM_OPT_CTXTID		14
+#define ETM_OPT_CTXTID2		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 c25c878fd06c..fa6f91a7c8a1 100644
--- a/tools/perf/arch/arm/util/cs-etm.c
+++ b/tools/perf/arch/arm/util/cs-etm.c
@@ -67,6 +67,7 @@ static int cs_etm_set_context_id(struct auxtrace_record *itr,
 	char path[PATH_MAX];
 	int err = -EINVAL;
 	u32 val;
+	u64 contextid;
 
 	ptr = container_of(itr, struct cs_etm_recording, itr);
 	cs_etm_pmu = ptr->cs_etm_pmu;
@@ -86,25 +87,59 @@ static int cs_etm_set_context_id(struct auxtrace_record *itr,
 		goto out;
 	}
 
+	/* User has configured for PID tracing, respects it. */
+	contextid = evsel->core.attr.config &
+			(BIT(ETM_OPT_CTXTID) | BIT(ETM_OPT_CTXTID2));
+
 	/*
-	 * 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.
+	 * If user doesn't configure the contextid format, parse PMU format and
+	 * enable PID tracing according to the "contextid" format bits:
+	 *
+	 *   If bit ETM_OPT_CTXTID is set, trace CONTEXTIDR_EL1;
+	 *   If bit ETM_OPT_CTXTID2 is set, trace CONTEXTIDR_EL2.
 	 */
-	val = BMVAL(val, 5, 9);
-	if (!val || val != 0x4) {
-		err = -EINVAL;
-		goto out;
+	if (!contextid)
+		contextid = perf_pmu__format_bits(&cs_etm_pmu->format,
+						  "contextid");
+
+	if (contextid & BIT(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) {
+			pr_err("%s: CONTEXTIDR_EL1 isn't supported\n",
+			       CORESIGHT_ETM_PMU_NAME);
+			err = -EINVAL;
+			goto out;
+		}
+	}
+
+	if (contextid & BIT(ETM_OPT_CTXTID2)) {
+		/*
+		 * TRCIDR2.VMIDOPT[30:29] != 0 and
+		 * TRCIDR2.VMIDSIZE[14:10] == 0b00100 (32bit virtual contextid)
+		 * We can't support CONTEXTIDR in VMID if the size of the
+		 * virtual context id is < 32bit.
+		 * Any value of VMIDSIZE >= 4 (i.e, > 32bit) is fine for us.
+		 */
+		if (!BMVAL(val, 29, 30) || BMVAL(val, 10, 14) < 4) {
+			pr_err("%s: CONTEXTIDR_EL2 isn't supported\n",
+			       CORESIGHT_ETM_PMU_NAME);
+			err = -EINVAL;
+			goto out;
+		}
 	}
 
 	/* All good, let the kernel know */
-	evsel->core.attr.config |= (1 << ETM_OPT_CTXTID);
+	evsel->core.attr.config |= contextid;
 	err = 0;
 
 out:
-
 	return err;
 }
 
@@ -489,7 +524,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_CTXTID2))
+		config |= BIT(ETM4_CFG_BIT_VMID) |
+			  BIT(ETM4_CFG_BIT_VMID_OPT);
 	return config;
 }
 
-- 
2.25.1

[PATCH v2 5/7] perf cs-etm: Add helper cs_etm__get_pid_fmt()

[Leo Yan]

This patch adds helper function cs_etm__get_pid_fmt(), by passing
parameter "traceID", it returns the PID format.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 tools/perf/util/cs-etm.c | 43 ++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/cs-etm.h |  1 +
 2 files changed, 44 insertions(+)

diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
index a2a369e2fbb6..8194ddbd01e5 100644
--- a/tools/perf/util/cs-etm.c
+++ b/tools/perf/util/cs-etm.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/bitops.h>
+#include <linux/coresight-pmu.h>
 #include <linux/err.h>
 #include <linux/kernel.h>
 #include <linux/log2.h>
@@ -156,6 +157,48 @@ int cs_etm__get_cpu(u8 trace_chan_id, int *cpu)
 	return 0;
 }
 
+/*
+ * The returned PID format is presented by two bits:
+ *
+ *   Bit ETM_OPT_CTXTID: CONTEXTIDR or CONTEXTIDR_EL1 is traced;
+ *   Bit ETM_OPT_CTXTID2: CONTEXTIDR_EL2 is traced.
+ *
+ * It's possible that these two bits are set together, this means the tracing
+ * contains PIDs for both CONTEXTIDR_EL1 and CONTEXTIDR_EL2.
+ */
+int cs_etm__get_pid_fmt(u8 trace_chan_id, u64 *pid_fmt)
+{
+	struct int_node *inode;
+	u64 *metadata, val;
+
+	inode = intlist__find(traceid_list, trace_chan_id);
+	if (!inode)
+		return -EINVAL;
+
+	metadata = inode->priv;
+
+	if (metadata[CS_ETM_MAGIC] == __perf_cs_etmv3_magic) {
+		val = metadata[CS_ETM_ETMCR];
+		/* CONTEXTIDR is traced */
+		if (val & BIT(ETM_OPT_CTXTID))
+			*pid_fmt = BIT(ETM_OPT_CTXTID);
+	} else {
+		val = metadata[CS_ETMV4_TRCCONFIGR];
+
+		*pid_fmt = 0;
+
+		/* CONTEXTIDR_EL2 is traced */
+		if (val & (BIT(ETM4_CFG_BIT_VMID) | BIT(ETM4_CFG_BIT_VMID_OPT)))
+			*pid_fmt = BIT(ETM_OPT_CTXTID2);
+
+		/* CONTEXTIDR_EL1 is traced */
+		if (val & BIT(ETM4_CFG_BIT_CTXTID))
+			*pid_fmt |= BIT(ETM_OPT_CTXTID);
+	}
+
+	return 0;
+}
+
 void cs_etm__etmq_set_traceid_queue_timestamp(struct cs_etm_queue *etmq,
 					      u8 trace_chan_id)
 {
diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h
index 4ad925d6d799..7cc3bba0017d 100644
--- a/tools/perf/util/cs-etm.h
+++ b/tools/perf/util/cs-etm.h
@@ -173,6 +173,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.25.1

[PATCH v2 6/7] perf cs-etm: Detect pid in VMID for kernel running at EL2

[Leo Yan]

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 CONTEXTIDR (Arm32)
or CONTEXTIDR_EL1 (Arm64) is invalid but we have a valid VMID.

Cc: Mike Leach <mike.leach@linaro.org>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Al Grant <al.grant@arm.com>
Co-developed-by: Leo Yan <leo.yan@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 .../perf/util/cs-etm-decoder/cs-etm-decoder.c | 32 ++++++++++++++++---
 1 file changed, 28 insertions(+), 4 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 3f4bc4050477..fb2a163ff74e 100644
--- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
+++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
@@ -6,6 +6,7 @@
  * Author: Mathieu Poirier <mathieu.poirier@linaro.org>
  */
 
+#include <linux/coresight-pmu.h>
 #include <linux/err.h>
 #include <linux/list.h>
 #include <linux/zalloc.h>
@@ -491,13 +492,36 @@ cs_etm_decoder__set_tid(struct cs_etm_queue *etmq,
 			const ocsd_generic_trace_elem *elem,
 			const uint8_t trace_chan_id)
 {
-	pid_t tid;
+	pid_t tid = -1;
+	u64 pid_fmt;
+	int ret;
 
-	/* Ignore PE_CONTEXT packets that don't have a valid contextID */
-	if (!elem->context.ctxt_id_valid)
+	ret = cs_etm__get_pid_fmt(trace_chan_id, &pid_fmt);
+	if (ret)
+		return OCSD_RESP_FATAL_SYS_ERR;
+
+	/*
+	 * 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, Bit ETM_OPT_CTXTID2 is set in this case.
+	 */
+	switch (pid_fmt) {
+	case BIT(ETM_OPT_CTXTID):
+		if (elem->context.ctxt_id_valid)
+			tid = elem->context.context_id;
+		break;
+	case BIT(ETM_OPT_CTXTID2) | BIT(ETM_OPT_CTXTID):
+	case BIT(ETM_OPT_CTXTID2):
+		if (elem->context.vmid_valid)
+			tid = elem->context.vmid;
+		break;
+	default:
+		break;
+	}
+
+	if (tid == -1)
 		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;
 
-- 
2.25.1

[PATCH v2 7/7] Documentation: coresight: Add PID tracing description

[Leo Yan]

After support the PID tracing for the kernel in EL1 or EL2, the usage
gets more complicated.

This patch gives description for the PMU formats of contextID configs,
this can help users to understand how to control the knobs for PID
tracing when the kernel is in different ELs.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 Documentation/trace/coresight/coresight.rst | 37 +++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/Documentation/trace/coresight/coresight.rst b/Documentation/trace/coresight/coresight.rst
index 0b73acb44efa..771558f22938 100644
--- a/Documentation/trace/coresight/coresight.rst
+++ b/Documentation/trace/coresight/coresight.rst
@@ -512,6 +512,43 @@ The --itrace option controls the type and frequency of synthesized events
 Note that only 64-bit programs are currently supported - further work is
 required to support instruction decode of 32-bit Arm programs.
 
+2.2) Tracing PID
+
+When the kernel is running at EL2 with Virtualization Host Extensions (VHE),
+perf records CONTEXTIDR_EL2 in the trace data and can be used as PID when
+decoding; and if the kernel is running at EL1 with nVHE, CONTEXTIDR_EL1 is
+traced for PID.
+
+To support tracing PID for the kernel runs at different exception levels,
+the PMU formats are defined as follow:
+
+  "contextid1": Available on both EL1 kernel and EL2 kernel.  When the
+                kernel is running at EL1, "contextid1" enables the PID
+                tracing; when the kernel is running at EL2, this enables
+                tracing the PID of guest applications.
+
+  "contextid2": Only usable when the kernel is running at EL2.  When
+                selected, enables PID tracing on EL2 kernel.
+
+  "contextid":  Will be an alias for the option that enables PID
+                tracing.  I.e,
+                contextid == contextid1, on EL1 kernel.
+                contextid == contextid2, on EL2 kernel.
+
+The perf tool automatically sets corresponding bit for the "contextid" config,
+therefore, the user doesn't have to bother which EL the kernel is running.
+
+  i.e, perf record -e cs_etm/contextid/u -- uname
+    or perf record -e cs_etm//u -- uname
+
+will always do the "PID" tracing, independent of the kernel EL.
+
+When the kernel is running at EL2 with VHE, if user wants to trace both the
+PIDs for both host and guest, the two configs "contextid1" and "contextid2"
+can be set at the same time:
+
+  perf record -e cs_etm/contextid1,contextid2/u -- uname
+
 
 Generating coverage files for Feedback Directed Optimization: AutoFDO
 ---------------------------------------------------------------------
-- 
2.25.1

Re: [PATCH v2 1/7] coresight: etm-perf: Clarify comment on perf options

[Suzuki K Poulose]

Hi Leo

On 2/2/21 4:38 PM, Leo Yan wrote:
> In theory, the options should be arbitrary values and are neutral for
> any ETM version; so far perf tool uses ETMv3.5/PTM ETMCR config bits
> except for register's bit definitions, also uses as options.
> 
> This can introduce confusion, especially if we want to add a new option
> but the new option is not supported by ETMv3.5/PTM ETMCR.  But on the
> other hand, we cannot change options since these options are generic
> CoreSight PMU ABI.
> 
> For easier maintenance and avoid confusion, this patch refines the
> comment to clarify perf options, and gives out the background info for
> these bits are coming from ETMv3.5/PTM.  Afterwards, we should take
> these options as general knobs, and if there have any confliction with
> ETMv3.5/PTM, should consider to define saperate macros for ETMv3.5/PTM
> ETMCR config bits.
> 
> Suggested-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>

The patch looks good to me.  The only concern I have is, whether
we should split this patch to kernel vs tools ? As the kernel
changes go via coresight tree and the tools patch may go via
perf tree ?

Either way, for the patch:

Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>


> ---
>   .../hwtracing/coresight/coresight-etm-perf.c    |  5 ++++-
>   include/linux/coresight-pmu.h                   | 17 ++++++++++++-----
>   tools/include/linux/coresight-pmu.h             | 17 ++++++++++++-----
>   3 files changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> index bdc34ca449f7..465ef1aa8c82 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -27,7 +27,10 @@ static bool etm_perf_up;
>   static DEFINE_PER_CPU(struct perf_output_handle, ctx_handle);
>   static DEFINE_PER_CPU(struct coresight_device *, csdev_src);
>   
> -/* ETMv3.5/PTM's ETMCR is 'config' */
> +/*
> + * The PMU formats were orignally for ETMv3.5/PTM's ETMCR 'config';
> + * now take them as general formats and apply on all ETMs.
> + */
>   PMU_FORMAT_ATTR(cycacc,		"config:" __stringify(ETM_OPT_CYCACC));
>   PMU_FORMAT_ATTR(contextid,	"config:" __stringify(ETM_OPT_CTXTID));
>   PMU_FORMAT_ATTR(timestamp,	"config:" __stringify(ETM_OPT_TS));
> diff --git a/include/linux/coresight-pmu.h b/include/linux/coresight-pmu.h
> index b0e35eec6499..5dc47cfdcf07 100644
> --- a/include/linux/coresight-pmu.h
> +++ b/include/linux/coresight-pmu.h
> @@ -10,11 +10,18 @@
>   #define CORESIGHT_ETM_PMU_NAME "cs_etm"
>   #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
> +/*
> + * Below are the definition of bit offsets for perf option, and works as
> + * arbitrary values for all ETM versions.
> + *
> + * Most of them are orignally from ETMv3.5/PTM's ETMCR config, therefore,
> + * ETMv3.5/PTM doesn't define ETMCR config bits with prefix "ETM3_" and
> + * directly use below macros as config bits.
> + */
> +#define ETM_OPT_CYCACC		12
> +#define ETM_OPT_CTXTID		14
> +#define ETM_OPT_TS		28
> +#define ETM_OPT_RETSTK		29
>   
>   /* ETMv4 CONFIGR programming bits for the ETM OPTs */
>   #define ETM4_CFG_BIT_CYCACC	4
> diff --git a/tools/include/linux/coresight-pmu.h b/tools/include/linux/coresight-pmu.h
> index b0e35eec6499..5dc47cfdcf07 100644
> --- a/tools/include/linux/coresight-pmu.h
> +++ b/tools/include/linux/coresight-pmu.h
> @@ -10,11 +10,18 @@
>   #define CORESIGHT_ETM_PMU_NAME "cs_etm"
>   #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
> +/*
> + * Below are the definition of bit offsets for perf option, and works as
> + * arbitrary values for all ETM versions.
> + *
> + * Most of them are orignally from ETMv3.5/PTM's ETMCR config, therefore,
> + * ETMv3.5/PTM doesn't define ETMCR config bits with prefix "ETM3_" and
> + * directly use below macros as config bits.
> + */
> +#define ETM_OPT_CYCACC		12
> +#define ETM_OPT_CTXTID		14
> +#define ETM_OPT_TS		28
> +#define ETM_OPT_RETSTK		29
>   
>   /* ETMv4 CONFIGR programming bits for the ETM OPTs */
>   #define ETM4_CFG_BIT_CYCACC	4
> 

Re: [PATCH v2 2/7] coresight: etm-perf: Support PID tracing for kernel at EL2

[Suzuki K Poulose]

On 2/2/21 4:38 PM, Leo Yan wrote:
> From: Suzuki K Poulose <suzuki.poulose@arm.com>
> 
> 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 CONTEXTIDR_EL1.  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 extends option CTXTID with an extra bit
> ETM_OPT_CTXTID2 (bit 15), thus on an EL2 kernel, we will have another
> bit available for the perf tool: ETM_OPT_CTXTID is for kernel running in
> EL1, ETM_OPT_CTXTID2 is used when kernel runs in EL2 with VHE enabled.
> 
> The tool must be backward compatible for users, i.e, "contextid" today
> traces PID and that should remain the same; for this purpose, the perf
> tool is updated to automatically set corresponding bit for the
> "contextid" config, therefore, the user doesn't have to bother which EL
> the kernel is running.
> 
>    i.e, perf record -e cs_etm/contextid/u --
> 
> will always do the "pid" tracing, independent of the kernel EL.
> 
> The driver parses the format "contextid", which traces CONTEXTIDR_EL1
> for ETM_OPT_CTXTID (on EL1 kernel) and traces CONTEXTIDR_EL2 for
> ETM_OPT_CTXTID2 (on EL2 kernel).
> 
> Besides the enhancement for format "contexid", extra two formats are
> introduced: "contextid1" and "contextid2".  This considers to support
> tracing both CONTEXTIDR_EL1 and CONTEXTIDR_EL2 when the kernel is
> running at EL2.  Finally, the PMU formats are defined as follow:
> 
>    "contextid1": Available on both EL1 kernel and EL2 kernel.  When the
>                  kernel is running at EL1, "contextid1" enables the PID
> 		tracing; when the kernel is running at EL2, this enables
> 		tracing the PID of guest applications.
> 
>    "contextid2": Only usable when the kernel is running at EL2.  When
>                  selected, enables PID tracing on EL2 kernel.
> 
>    "contextid":  Will be an alias for the option that enables PID
>                  tracing.  I.e,
>                  contextid == contextid1, on EL1 kernel.
>                  contextid == contextid2, on EL2 kernel.
> 
> 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>

You may add the following line here :

[ Added two config formats: contextid1, contextid2 ]

> Signed-off-by: Leo Yan <leo.yan@linaro.org>

The patch as such looks good to me.

Suzuki

Re: [PATCH v2 5/7] perf cs-etm: Add helper cs_etm__get_pid_fmt()

[Suzuki K Poulose]

On 2/2/21 4:38 PM, Leo Yan wrote:
> This patch adds helper function cs_etm__get_pid_fmt(), by passing
> parameter "traceID", it returns the PID format.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>   tools/perf/util/cs-etm.c | 43 ++++++++++++++++++++++++++++++++++++++++
>   tools/perf/util/cs-etm.h |  1 +
>   2 files changed, 44 insertions(+)
> 
> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index a2a369e2fbb6..8194ddbd01e5 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c
> @@ -7,6 +7,7 @@
>    */
>   
>   #include <linux/bitops.h>
> +#include <linux/coresight-pmu.h>
>   #include <linux/err.h>
>   #include <linux/kernel.h>
>   #include <linux/log2.h>
> @@ -156,6 +157,48 @@ int cs_etm__get_cpu(u8 trace_chan_id, int *cpu)
>   	return 0;
>   }
>   
> +/*
> + * The returned PID format is presented by two bits:
> + *
> + *   Bit ETM_OPT_CTXTID: CONTEXTIDR or CONTEXTIDR_EL1 is traced;
> + *   Bit ETM_OPT_CTXTID2: CONTEXTIDR_EL2 is traced.
> + *
> + * It's possible that these two bits are set together, this means the tracing
> + * contains PIDs for both CONTEXTIDR_EL1 and CONTEXTIDR_EL2.

This is a bit confusing. If both the bits are set, the session
was run on an EL2 kernel. Thus, the PID is always in CONTEXTIDR_EL2.

> + */
> +int cs_etm__get_pid_fmt(u8 trace_chan_id, u64 *pid_fmt)
> +{
> +	struct int_node *inode;
> +	u64 *metadata, val;
> +
> +	inode = intlist__find(traceid_list, trace_chan_id);
> +	if (!inode)
> +		return -EINVAL;
> +
> +	metadata = inode->priv;
> +
> +	if (metadata[CS_ETM_MAGIC] == __perf_cs_etmv3_magic) {
> +		val = metadata[CS_ETM_ETMCR];
> +		/* CONTEXTIDR is traced */
> +		if (val & BIT(ETM_OPT_CTXTID))
> +			*pid_fmt = BIT(ETM_OPT_CTXTID);
> +	} else {
> +		val = metadata[CS_ETMV4_TRCCONFIGR];
> +
> +		*pid_fmt = 0;
> +
> +		/* CONTEXTIDR_EL2 is traced */
> +		if (val & (BIT(ETM4_CFG_BIT_VMID) | BIT(ETM4_CFG_BIT_VMID_OPT)))
> +			*pid_fmt = BIT(ETM_OPT_CTXTID2);
> +
> +		/* CONTEXTIDR_EL1 is traced */
> +		if (val & BIT(ETM4_CFG_BIT_CTXTID))

I haven't looked at how this gets used. But, Shouldn't this be :

		else if (val & BIT(ETM4_CFG_BIT_CTXTID)) ?

Suzuki

Re: [PATCH v2 7/7] Documentation: coresight: Add PID tracing description

[Suzuki K Poulose]

On 2/2/21 4:38 PM, Leo Yan wrote:
> After support the PID tracing for the kernel in EL1 or EL2, the usage
> gets more complicated.
> 
> This patch gives description for the PMU formats of contextID configs,
> this can help users to understand how to control the knobs for PID
> tracing when the kernel is in different ELs.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>   Documentation/trace/coresight/coresight.rst | 37 +++++++++++++++++++++
>   1 file changed, 37 insertions(+)
> 
> diff --git a/Documentation/trace/coresight/coresight.rst b/Documentation/trace/coresight/coresight.rst
> index 0b73acb44efa..771558f22938 100644
> --- a/Documentation/trace/coresight/coresight.rst
> +++ b/Documentation/trace/coresight/coresight.rst
> @@ -512,6 +512,43 @@ The --itrace option controls the type and frequency of synthesized events
>   Note that only 64-bit programs are currently supported - further work is
>   required to support instruction decode of 32-bit Arm programs.
>   
> +2.2) Tracing PID
> +
> +When the kernel is running at EL2 with Virtualization Host Extensions (VHE),
> +perf records CONTEXTIDR_EL2 in the trace data and can be used as PID when
> +decoding; and if the kernel is running at EL1 with nVHE, CONTEXTIDR_EL1 is
> +traced for PID.
> +
> +To support tracing PID for the kernel runs at different exception levels,
> +the PMU formats are defined as follow:
> +
> +  "contextid1": Available on both EL1 kernel and EL2 kernel.  When the
> +                kernel is running at EL1, "contextid1" enables the PID
> +                tracing; when the kernel is running at EL2, this enables
> +                tracing the PID of guest applications.
> +
> +  "contextid2": Only usable when the kernel is running at EL2.  When
> +                selected, enables PID tracing on EL2 kernel.
> +
> +  "contextid":  Will be an alias for the option that enables PID
> +                tracing.  I.e,
> +                contextid == contextid1, on EL1 kernel.
> +                contextid == contextid2, on EL2 kernel.
> +
> +The perf tool automatically sets corresponding bit for the "contextid" config,
> +therefore, the user doesn't have to bother which EL the kernel is running.
> +
> +  i.e, perf record -e cs_etm/contextid/u -- uname
> +    or perf record -e cs_etm//u -- uname
> +
> +will always do the "PID" tracing, independent of the kernel EL.
> +
> +When the kernel is running at EL2 with VHE, if user wants to trace both the
> +PIDs for both host and guest, the two configs "contextid1" and "contextid2"
> +can be set at the same time:
> +
> +  perf record -e cs_etm/contextid1,contextid2/u -- uname
> +

To make this case clear, we could change the command from uname to
something like:

     perf record -e cs_etm/contextid1,contextid2/u -- vm

Otherwise looks good to me.

With the above fixed,

Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>

Re: [PATCH v2 6/7] perf cs-etm: Detect pid in VMID for kernel running at EL2

[Suzuki K Poulose]

On 2/2/21 4:38 PM, Leo Yan 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 CONTEXTIDR (Arm32)
> or CONTEXTIDR_EL1 (Arm64) is invalid but we have a valid VMID.
> 
> Cc: Mike Leach <mike.leach@linaro.org>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: Al Grant <al.grant@arm.com>
> Co-developed-by: Leo Yan <leo.yan@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>   .../perf/util/cs-etm-decoder/cs-etm-decoder.c | 32 ++++++++++++++++---
>   1 file changed, 28 insertions(+), 4 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 3f4bc4050477..fb2a163ff74e 100644
> --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> @@ -6,6 +6,7 @@
>    * Author: Mathieu Poirier <mathieu.poirier@linaro.org>
>    */
>   
> +#include <linux/coresight-pmu.h>
>   #include <linux/err.h>
>   #include <linux/list.h>
>   #include <linux/zalloc.h>
> @@ -491,13 +492,36 @@ cs_etm_decoder__set_tid(struct cs_etm_queue *etmq,
>   			const ocsd_generic_trace_elem *elem,
>   			const uint8_t trace_chan_id)
>   {
> -	pid_t tid;
> +	pid_t tid = -1;
> +	u64 pid_fmt;
> +	int ret;
>   
> -	/* Ignore PE_CONTEXT packets that don't have a valid contextID */
> -	if (!elem->context.ctxt_id_valid)
> +	ret = cs_etm__get_pid_fmt(trace_chan_id, &pid_fmt);
> +	if (ret)

Is this something we can cache in this function ? e.g,
	static u64 pid_fmt;

	if (!pid_pfmt)
		ret = cs_etm__get_pid_fmt(trace_chan_id, &pid_fmt);

As all the ETMs will be running at the same exception level.

> +		return OCSD_RESP_FATAL_SYS_ERR;
> +
> +	/*
> +	 * 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, Bit ETM_OPT_CTXTID2 is set in this case.
> +	 */
> +	switch (pid_fmt) {
> +	case BIT(ETM_OPT_CTXTID):
> +		if (elem->context.ctxt_id_valid)
> +			tid = elem->context.context_id;
> +		break;
> +	case BIT(ETM_OPT_CTXTID2) | BIT(ETM_OPT_CTXTID):

I would rather fix the cs_etm__get_pid_fmt() to return either of these
as commented. i.e, ETM_OPT_CTXTID or ETM_OPT_CTXTID2. Thus we don't
need the this case.


With the above two addressed:

Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>

Re: [PATCH v2 7/7] Documentation: coresight: Add PID tracing description

[Mike Leach]

Hi,

On Tue, 2 Feb 2021 at 16:39, Leo Yan <leo.yan@linaro.org> wrote:
>
> After support the PID tracing for the kernel in EL1 or EL2, the usage
> gets more complicated.
>
> This patch gives description for the PMU formats of contextID configs,
> this can help users to understand how to control the knobs for PID
> tracing when the kernel is in different ELs.
>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  Documentation/trace/coresight/coresight.rst | 37 +++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>
> diff --git a/Documentation/trace/coresight/coresight.rst b/Documentation/trace/coresight/coresight.rst
> index 0b73acb44efa..771558f22938 100644
> --- a/Documentation/trace/coresight/coresight.rst
> +++ b/Documentation/trace/coresight/coresight.rst
> @@ -512,6 +512,43 @@ The --itrace option controls the type and frequency of synthesized events
>  Note that only 64-bit programs are currently supported - further work is
>  required to support instruction decode of 32-bit Arm programs.
>
> +2.2) Tracing PID
> +
> +When the kernel is running at EL2 with Virtualization Host Extensions (VHE),
> +perf records CONTEXTIDR_EL2 in the trace data and can be used as PID when
> +decoding; and if the kernel is running at EL1 with nVHE, CONTEXTIDR_EL1 is
> +traced for PID.
> +

Would this introductory paragraph be better if is explained where the
kernel stores the PID for the different levels, then we logically move
on to how to trace this in perf.

e.g:-

"The lernel can be built to write the PID value into the PE ContextID registers.
For a kernel running at EL1, the PID is stored in CONTEXTIDR_EL1.
A PE may implement ARM Virtualisation Host Extensions (VHE), were the
kernel can run at EL2 as a virtualisation host.
In this case the PID value is stored in CONTEXTIDR_EL2.
perf provides PMU options which program the ETM to insert these values
into the trace data."

> +To support tracing PID for the kernel runs at different exception levels,
> +the PMU formats are defined as follow:
> +
> +  "contextid1": Available on both EL1 kernel and EL2 kernel.  When the
> +                kernel is running at EL1, "contextid1" enables the PID
> +                tracing; when the kernel is running at EL2, this enables
> +                tracing the PID of guest applications.
> +
> +  "contextid2": Only usable when the kernel is running at EL2.  When
> +                selected, enables PID tracing on EL2 kernel.
> +
> +  "contextid":  Will be an alias for the option that enables PID
> +                tracing.  I.e,
> +                contextid == contextid1, on EL1 kernel.
> +                contextid == contextid2, on EL2 kernel.
> +
> +The perf tool automatically sets corresponding bit for the "contextid" config,
> +therefore, the user doesn't have to bother which EL the kernel is running.
> +
> +  i.e, perf record -e cs_etm/contextid/u -- uname
> +    or perf record -e cs_etm//u -- uname
> +
> +will always do the "PID" tracing, independent of the kernel EL.
> +

This is telling me that both cs_etm// and cs_etm/contextid/ have the
same effect - trace PID. Is this correct?
If so, then contextid, contextid1 and contextid2 have no effect except
in specific EL2 circumstances.


> +When the kernel is running at EL2 with VHE, if user wants to trace both the
> +PIDs for both host and guest, the two configs "contextid1" and "contextid2"
> +can be set at the same time:
> +
> +  perf record -e cs_etm/contextid1,contextid2/u -- uname
> +
>


Regards

Mike


>  Generating coverage files for Feedback Directed Optimization: AutoFDO
>  ---------------------------------------------------------------------
> --
> 2.25.1
>


--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

Re: [PATCH v2 1/7] coresight: etm-perf: Clarify comment on perf options

[Leo Yan]

Hi Suzuki,

On Tue, Feb 02, 2021 at 11:00:42PM +0000, Suzuki Kuruppassery Poulose wrote:
> Hi Leo
> 
> On 2/2/21 4:38 PM, Leo Yan wrote:
> > In theory, the options should be arbitrary values and are neutral for
> > any ETM version; so far perf tool uses ETMv3.5/PTM ETMCR config bits
> > except for register's bit definitions, also uses as options.
> > 
> > This can introduce confusion, especially if we want to add a new option
> > but the new option is not supported by ETMv3.5/PTM ETMCR.  But on the
> > other hand, we cannot change options since these options are generic
> > CoreSight PMU ABI.
> > 
> > For easier maintenance and avoid confusion, this patch refines the
> > comment to clarify perf options, and gives out the background info for
> > these bits are coming from ETMv3.5/PTM.  Afterwards, we should take
> > these options as general knobs, and if there have any confliction with
> > ETMv3.5/PTM, should consider to define saperate macros for ETMv3.5/PTM
> > ETMCR config bits.
> > 
> > Suggested-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> 
> The patch looks good to me.  The only concern I have is, whether
> we should split this patch to kernel vs tools ? As the kernel
> changes go via coresight tree and the tools patch may go via
> perf tree ?

Yes, will split the patch.

> Either way, for the patch:
> 
> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>

Thanks for review and suggestion.

> > ---
> >   .../hwtracing/coresight/coresight-etm-perf.c    |  5 ++++-
> >   include/linux/coresight-pmu.h                   | 17 ++++++++++++-----
> >   tools/include/linux/coresight-pmu.h             | 17 ++++++++++++-----
> >   3 files changed, 28 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > index bdc34ca449f7..465ef1aa8c82 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > @@ -27,7 +27,10 @@ static bool etm_perf_up;
> >   static DEFINE_PER_CPU(struct perf_output_handle, ctx_handle);
> >   static DEFINE_PER_CPU(struct coresight_device *, csdev_src);
> > -/* ETMv3.5/PTM's ETMCR is 'config' */
> > +/*
> > + * The PMU formats were orignally for ETMv3.5/PTM's ETMCR 'config';
> > + * now take them as general formats and apply on all ETMs.
> > + */
> >   PMU_FORMAT_ATTR(cycacc,		"config:" __stringify(ETM_OPT_CYCACC));
> >   PMU_FORMAT_ATTR(contextid,	"config:" __stringify(ETM_OPT_CTXTID));
> >   PMU_FORMAT_ATTR(timestamp,	"config:" __stringify(ETM_OPT_TS));
> > diff --git a/include/linux/coresight-pmu.h b/include/linux/coresight-pmu.h
> > index b0e35eec6499..5dc47cfdcf07 100644
> > --- a/include/linux/coresight-pmu.h
> > +++ b/include/linux/coresight-pmu.h
> > @@ -10,11 +10,18 @@
> >   #define CORESIGHT_ETM_PMU_NAME "cs_etm"
> >   #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
> > +/*
> > + * Below are the definition of bit offsets for perf option, and works as
> > + * arbitrary values for all ETM versions.
> > + *
> > + * Most of them are orignally from ETMv3.5/PTM's ETMCR config, therefore,
> > + * ETMv3.5/PTM doesn't define ETMCR config bits with prefix "ETM3_" and
> > + * directly use below macros as config bits.
> > + */
> > +#define ETM_OPT_CYCACC		12
> > +#define ETM_OPT_CTXTID		14
> > +#define ETM_OPT_TS		28
> > +#define ETM_OPT_RETSTK		29
> >   /* ETMv4 CONFIGR programming bits for the ETM OPTs */
> >   #define ETM4_CFG_BIT_CYCACC	4
> > diff --git a/tools/include/linux/coresight-pmu.h b/tools/include/linux/coresight-pmu.h
> > index b0e35eec6499..5dc47cfdcf07 100644
> > --- a/tools/include/linux/coresight-pmu.h
> > +++ b/tools/include/linux/coresight-pmu.h
> > @@ -10,11 +10,18 @@
> >   #define CORESIGHT_ETM_PMU_NAME "cs_etm"
> >   #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
> > +/*
> > + * Below are the definition of bit offsets for perf option, and works as
> > + * arbitrary values for all ETM versions.
> > + *
> > + * Most of them are orignally from ETMv3.5/PTM's ETMCR config, therefore,
> > + * ETMv3.5/PTM doesn't define ETMCR config bits with prefix "ETM3_" and
> > + * directly use below macros as config bits.
> > + */
> > +#define ETM_OPT_CYCACC		12
> > +#define ETM_OPT_CTXTID		14
> > +#define ETM_OPT_TS		28
> > +#define ETM_OPT_RETSTK		29
> >   /* ETMv4 CONFIGR programming bits for the ETM OPTs */
> >   #define ETM4_CFG_BIT_CYCACC	4
> > 
> 

Re: [PATCH v2 5/7] perf cs-etm: Add helper cs_etm__get_pid_fmt()

[Leo Yan]

On Tue, Feb 02, 2021 at 11:19:22PM +0000, Suzuki Kuruppassery Poulose wrote:
> On 2/2/21 4:38 PM, Leo Yan wrote:
> > This patch adds helper function cs_etm__get_pid_fmt(), by passing
> > parameter "traceID", it returns the PID format.
> > 
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >   tools/perf/util/cs-etm.c | 43 ++++++++++++++++++++++++++++++++++++++++
> >   tools/perf/util/cs-etm.h |  1 +
> >   2 files changed, 44 insertions(+)
> > 
> > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> > index a2a369e2fbb6..8194ddbd01e5 100644
> > --- a/tools/perf/util/cs-etm.c
> > +++ b/tools/perf/util/cs-etm.c
> > @@ -7,6 +7,7 @@
> >    */
> >   #include <linux/bitops.h>
> > +#include <linux/coresight-pmu.h>
> >   #include <linux/err.h>
> >   #include <linux/kernel.h>
> >   #include <linux/log2.h>
> > @@ -156,6 +157,48 @@ int cs_etm__get_cpu(u8 trace_chan_id, int *cpu)
> >   	return 0;
> >   }
> > +/*
> > + * The returned PID format is presented by two bits:
> > + *
> > + *   Bit ETM_OPT_CTXTID: CONTEXTIDR or CONTEXTIDR_EL1 is traced;
> > + *   Bit ETM_OPT_CTXTID2: CONTEXTIDR_EL2 is traced.
> > + *
> > + * It's possible that these two bits are set together, this means the tracing
> > + * contains PIDs for both CONTEXTIDR_EL1 and CONTEXTIDR_EL2.
> 
> This is a bit confusing. If both the bits are set, the session
> was run on an EL2 kernel. Thus, the PID is always in CONTEXTIDR_EL2.

Sorry for confusion.  I'd like to rephrase as:

It's possible that the two bits ETM_OPT_CTXTID and ETM_OPT_CTXTID2 are
enabled at the same time when the session runs on an EL2 kernel.  This
means the CONTEXTIDR_EL1 and CONTEXTIDR_EL2 both will be recorded in
the trace data, the tool will selectively use CONTEXTIDR_EL2 as PID.

> > + */
> > +int cs_etm__get_pid_fmt(u8 trace_chan_id, u64 *pid_fmt)
> > +{
> > +	struct int_node *inode;
> > +	u64 *metadata, val;
> > +
> > +	inode = intlist__find(traceid_list, trace_chan_id);
> > +	if (!inode)
> > +		return -EINVAL;
> > +
> > +	metadata = inode->priv;
> > +
> > +	if (metadata[CS_ETM_MAGIC] == __perf_cs_etmv3_magic) {
> > +		val = metadata[CS_ETM_ETMCR];
> > +		/* CONTEXTIDR is traced */
> > +		if (val & BIT(ETM_OPT_CTXTID))
> > +			*pid_fmt = BIT(ETM_OPT_CTXTID);
> > +	} else {
> > +		val = metadata[CS_ETMV4_TRCCONFIGR];
> > +
> > +		*pid_fmt = 0;
> > +
> > +		/* CONTEXTIDR_EL2 is traced */
> > +		if (val & (BIT(ETM4_CFG_BIT_VMID) | BIT(ETM4_CFG_BIT_VMID_OPT)))
> > +			*pid_fmt = BIT(ETM_OPT_CTXTID2);
> > +
> > +		/* CONTEXTIDR_EL1 is traced */
> > +		if (val & BIT(ETM4_CFG_BIT_CTXTID))
> 
> I haven't looked at how this gets used. But, Shouldn't this be :
> 
> 		else if (val & BIT(ETM4_CFG_BIT_CTXTID)) ?

Actually it's deliberately to set both bits ETM_OPT_CTXTID2 and
ETM_OPT_CTXTID if user has enable configs "contextid1" and
"contextid2".  So this is exactly the reversed flow in the
function cs_etmv4_get_config().

Thanks,
Leo

Re: [PATCH v2 6/7] perf cs-etm: Detect pid in VMID for kernel running at EL2

[Leo Yan]

On Tue, Feb 02, 2021 at 11:29:47PM +0000, Suzuki Kuruppassery Poulose wrote:
> On 2/2/21 4:38 PM, Leo Yan 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 CONTEXTIDR (Arm32)
> > or CONTEXTIDR_EL1 (Arm64) is invalid but we have a valid VMID.
> > 
> > Cc: Mike Leach <mike.leach@linaro.org>
> > Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> > Cc: Al Grant <al.grant@arm.com>
> > Co-developed-by: Leo Yan <leo.yan@linaro.org>
> > Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >   .../perf/util/cs-etm-decoder/cs-etm-decoder.c | 32 ++++++++++++++++---
> >   1 file changed, 28 insertions(+), 4 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 3f4bc4050477..fb2a163ff74e 100644
> > --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> > +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> > @@ -6,6 +6,7 @@
> >    * Author: Mathieu Poirier <mathieu.poirier@linaro.org>
> >    */
> > +#include <linux/coresight-pmu.h>
> >   #include <linux/err.h>
> >   #include <linux/list.h>
> >   #include <linux/zalloc.h>
> > @@ -491,13 +492,36 @@ cs_etm_decoder__set_tid(struct cs_etm_queue *etmq,
> >   			const ocsd_generic_trace_elem *elem,
> >   			const uint8_t trace_chan_id)
> >   {
> > -	pid_t tid;
> > +	pid_t tid = -1;
> > +	u64 pid_fmt;
> > +	int ret;
> > -	/* Ignore PE_CONTEXT packets that don't have a valid contextID */
> > -	if (!elem->context.ctxt_id_valid)
> > +	ret = cs_etm__get_pid_fmt(trace_chan_id, &pid_fmt);
> > +	if (ret)
> 
> Is this something we can cache in this function ? e.g,
> 	static u64 pid_fmt;
> 
> 	if (!pid_pfmt)
> 		ret = cs_etm__get_pid_fmt(trace_chan_id, &pid_fmt);
> 
> As all the ETMs will be running at the same exception level.

Sorry that I let you repeated your comments again.

To be honest, I considered this after read your comment in the previous
series, but I thought it's possible that multiple CPUs have different
PID format, especially for big.LITTLE arch.  After read your suggestion
again, I think my concern is not valid, even for big.LITTLE, all CPUs
should run on the same kernel exception level.

So will follow up your suggestion to cache "pid_fmt".

> 
> > +		return OCSD_RESP_FATAL_SYS_ERR;
> > +
> > +	/*
> > +	 * 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, Bit ETM_OPT_CTXTID2 is set in this case.
> > +	 */
> > +	switch (pid_fmt) {
> > +	case BIT(ETM_OPT_CTXTID):
> > +		if (elem->context.ctxt_id_valid)
> > +			tid = elem->context.context_id;
> > +		break;
> > +	case BIT(ETM_OPT_CTXTID2) | BIT(ETM_OPT_CTXTID):
> 
> I would rather fix the cs_etm__get_pid_fmt() to return either of these
> as commented. i.e, ETM_OPT_CTXTID or ETM_OPT_CTXTID2. Thus we don't
> need the this case.

I explained why I set both bits for ETM_OPT_CTXTID and ETM_OPT_CTXTID2
in the patch 05/07.  Could you take a look for it?

> With the above two addressed:
> 
> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>

Thanks,
Leo

Re: [PATCH v2 7/7] Documentation: coresight: Add PID tracing description

[Leo Yan]

On Tue, Feb 02, 2021 at 11:24:48PM +0000, Suzuki Kuruppassery Poulose wrote:
> On 2/2/21 4:38 PM, Leo Yan wrote:
> > After support the PID tracing for the kernel in EL1 or EL2, the usage
> > gets more complicated.
> > 
> > This patch gives description for the PMU formats of contextID configs,
> > this can help users to understand how to control the knobs for PID
> > tracing when the kernel is in different ELs.
> > 
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >   Documentation/trace/coresight/coresight.rst | 37 +++++++++++++++++++++
> >   1 file changed, 37 insertions(+)
> > 
> > diff --git a/Documentation/trace/coresight/coresight.rst b/Documentation/trace/coresight/coresight.rst
> > index 0b73acb44efa..771558f22938 100644
> > --- a/Documentation/trace/coresight/coresight.rst
> > +++ b/Documentation/trace/coresight/coresight.rst
> > @@ -512,6 +512,43 @@ The --itrace option controls the type and frequency of synthesized events
> >   Note that only 64-bit programs are currently supported - further work is
> >   required to support instruction decode of 32-bit Arm programs.
> > +2.2) Tracing PID
> > +
> > +When the kernel is running at EL2 with Virtualization Host Extensions (VHE),
> > +perf records CONTEXTIDR_EL2 in the trace data and can be used as PID when
> > +decoding; and if the kernel is running at EL1 with nVHE, CONTEXTIDR_EL1 is
> > +traced for PID.
> > +
> > +To support tracing PID for the kernel runs at different exception levels,
> > +the PMU formats are defined as follow:
> > +
> > +  "contextid1": Available on both EL1 kernel and EL2 kernel.  When the
> > +                kernel is running at EL1, "contextid1" enables the PID
> > +                tracing; when the kernel is running at EL2, this enables
> > +                tracing the PID of guest applications.
> > +
> > +  "contextid2": Only usable when the kernel is running at EL2.  When
> > +                selected, enables PID tracing on EL2 kernel.
> > +
> > +  "contextid":  Will be an alias for the option that enables PID
> > +                tracing.  I.e,
> > +                contextid == contextid1, on EL1 kernel.
> > +                contextid == contextid2, on EL2 kernel.
> > +
> > +The perf tool automatically sets corresponding bit for the "contextid" config,
> > +therefore, the user doesn't have to bother which EL the kernel is running.
> > +
> > +  i.e, perf record -e cs_etm/contextid/u -- uname
> > +    or perf record -e cs_etm//u -- uname
> > +
> > +will always do the "PID" tracing, independent of the kernel EL.
> > +
> > +When the kernel is running at EL2 with VHE, if user wants to trace both the
> > +PIDs for both host and guest, the two configs "contextid1" and "contextid2"
> > +can be set at the same time:
> > +
> > +  perf record -e cs_etm/contextid1,contextid2/u -- uname
> > +
> 
> To make this case clear, we could change the command from uname to
> something like:
> 
>     perf record -e cs_etm/contextid1,contextid2/u -- vm

Exactly, will refine.

> Otherwise looks good to me.
> 
> With the above fixed,
> 
> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>

Thanks,
Leo

Re: [PATCH v2 7/7] Documentation: coresight: Add PID tracing description

[Leo Yan]

Hi Mike,

On Wed, Feb 03, 2021 at 05:39:54PM +0000, Mike Leach wrote:

[...]

> > +2.2) Tracing PID
> > +
> > +When the kernel is running at EL2 with Virtualization Host Extensions (VHE),
> > +perf records CONTEXTIDR_EL2 in the trace data and can be used as PID when
> > +decoding; and if the kernel is running at EL1 with nVHE, CONTEXTIDR_EL1 is
> > +traced for PID.
> > +
> 
> Would this introductory paragraph be better if is explained where the
> kernel stores the PID for the different levels, then we logically move
> on to how to trace this in perf.
> 
> e.g:-
> 
> "The lernel can be built to write the PID value into the PE ContextID registers.
> For a kernel running at EL1, the PID is stored in CONTEXTIDR_EL1.
> A PE may implement ARM Virtualisation Host Extensions (VHE), were the
> kernel can run at EL2 as a virtualisation host.
> In this case the PID value is stored in CONTEXTIDR_EL2.
> perf provides PMU options which program the ETM to insert these values
> into the trace data."

Will in next spin; thanks a lot for writing up!

> > +To support tracing PID for the kernel runs at different exception levels,
> > +the PMU formats are defined as follow:
> > +
> > +  "contextid1": Available on both EL1 kernel and EL2 kernel.  When the
> > +                kernel is running at EL1, "contextid1" enables the PID
> > +                tracing; when the kernel is running at EL2, this enables
> > +                tracing the PID of guest applications.
> > +
> > +  "contextid2": Only usable when the kernel is running at EL2.  When
> > +                selected, enables PID tracing on EL2 kernel.
> > +
> > +  "contextid":  Will be an alias for the option that enables PID
> > +                tracing.  I.e,
> > +                contextid == contextid1, on EL1 kernel.
> > +                contextid == contextid2, on EL2 kernel.
> > +
> > +The perf tool automatically sets corresponding bit for the "contextid" config,
> > +therefore, the user doesn't have to bother which EL the kernel is running.
> > +
> > +  i.e, perf record -e cs_etm/contextid/u -- uname
> > +    or perf record -e cs_etm//u -- uname
> > +
> > +will always do the "PID" tracing, independent of the kernel EL.
> > +
> 
> This is telling me that both cs_etm// and cs_etm/contextid/ have the
> same effect - trace PID. Is this correct?

Correct.

> If so, then contextid, contextid1 and contextid2 have no effect except
> in specific EL2 circumstances.

Yes, exactly.

Thanks,
Leo

> > +When the kernel is running at EL2 with VHE, if user wants to trace both the
> > +PIDs for both host and guest, the two configs "contextid1" and "contextid2"
> > +can be set at the same time:
> > +
> > +  perf record -e cs_etm/contextid1,contextid2/u -- uname
> > +
> >
> 
> 
> Regards
> 
> Mike
> 
> 
> >  Generating coverage files for Feedback Directed Optimization: AutoFDO
> >  ---------------------------------------------------------------------
> > --
> > 2.25.1
> >
> 
> 
> --
> Mike Leach
> Principal Engineer, ARM Ltd.
> Manchester Design Centre. UK

Re: [PATCH v2 5/7] perf cs-etm: Add helper cs_etm__get_pid_fmt()

[Suzuki K Poulose]

On 2/4/21 3:47 AM, Leo Yan wrote:
> On Tue, Feb 02, 2021 at 11:19:22PM +0000, Suzuki Kuruppassery Poulose wrote:
>> On 2/2/21 4:38 PM, Leo Yan wrote:
>>> This patch adds helper function cs_etm__get_pid_fmt(), by passing
>>> parameter "traceID", it returns the PID format.
>>>
>>> Signed-off-by: Leo Yan <leo.yan@linaro.org>
>>> ---
>>>    tools/perf/util/cs-etm.c | 43 ++++++++++++++++++++++++++++++++++++++++
>>>    tools/perf/util/cs-etm.h |  1 +
>>>    2 files changed, 44 insertions(+)
>>>
>>> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
>>> index a2a369e2fbb6..8194ddbd01e5 100644
>>> --- a/tools/perf/util/cs-etm.c
>>> +++ b/tools/perf/util/cs-etm.c
>>> @@ -7,6 +7,7 @@
>>>     */
>>>    #include <linux/bitops.h>
>>> +#include <linux/coresight-pmu.h>
>>>    #include <linux/err.h>
>>>    #include <linux/kernel.h>
>>>    #include <linux/log2.h>
>>> @@ -156,6 +157,48 @@ int cs_etm__get_cpu(u8 trace_chan_id, int *cpu)
>>>    	return 0;
>>>    }
>>> +/*
>>> + * The returned PID format is presented by two bits:
>>> + *
>>> + *   Bit ETM_OPT_CTXTID: CONTEXTIDR or CONTEXTIDR_EL1 is traced;
>>> + *   Bit ETM_OPT_CTXTID2: CONTEXTIDR_EL2 is traced.
>>> + *
>>> + * It's possible that these two bits are set together, this means the tracing
>>> + * contains PIDs for both CONTEXTIDR_EL1 and CONTEXTIDR_EL2.
>>
>> This is a bit confusing. If both the bits are set, the session
>> was run on an EL2 kernel. Thus, the PID is always in CONTEXTIDR_EL2.
> 
> Sorry for confusion.  I'd like to rephrase as:
> 
> It's possible that the two bits ETM_OPT_CTXTID and ETM_OPT_CTXTID2 are
> enabled at the same time when the session runs on an EL2 kernel.  This
> means the CONTEXTIDR_EL1 and CONTEXTIDR_EL2 both will be recorded in
> the trace data, the tool will selectively use CONTEXTIDR_EL2 as PID.
> 
>>> + */
>>> +int cs_etm__get_pid_fmt(u8 trace_chan_id, u64 *pid_fmt)
>>> +{
>>> +	struct int_node *inode;
>>> +	u64 *metadata, val;
>>> +
>>> +	inode = intlist__find(traceid_list, trace_chan_id);
>>> +	if (!inode)
>>> +		return -EINVAL;
>>> +
>>> +	metadata = inode->priv;
>>> +
>>> +	if (metadata[CS_ETM_MAGIC] == __perf_cs_etmv3_magic) {
>>> +		val = metadata[CS_ETM_ETMCR];
>>> +		/* CONTEXTIDR is traced */
>>> +		if (val & BIT(ETM_OPT_CTXTID))
>>> +			*pid_fmt = BIT(ETM_OPT_CTXTID);
>>> +	} else {
>>> +		val = metadata[CS_ETMV4_TRCCONFIGR];
>>> +
>>> +		*pid_fmt = 0;
>>> +
>>> +		/* CONTEXTIDR_EL2 is traced */
>>> +		if (val & (BIT(ETM4_CFG_BIT_VMID) | BIT(ETM4_CFG_BIT_VMID_OPT)))
>>> +			*pid_fmt = BIT(ETM_OPT_CTXTID2);
>>> +
>>> +		/* CONTEXTIDR_EL1 is traced */
>>> +		if (val & BIT(ETM4_CFG_BIT_CTXTID))
>>
>> I haven't looked at how this gets used. But, Shouldn't this be :
>>
>> 		else if (val & BIT(ETM4_CFG_BIT_CTXTID)) ?
> 
> Actually it's deliberately to set both bits ETM_OPT_CTXTID2 and
> ETM_OPT_CTXTID if user has enable configs "contextid1" and
> "contextid2".  So this is exactly the reversed flow in the
> function cs_etmv4_get_config().

The point is, we don't care if the user selected both options. What we
care is, where can we find the PID. CONTEXTIDR_EL1 or CONTEXTIDR_EL2.
As such, get_pid_fmt simply should make that decision and pass it on.
So, if the CONTEXTIDR_EL2 is selected (which can only be done successfully
on an EL2 kernel), thats our pid.

So we should return the format for the PID here. i.e
  ETM_OPT_CTXTID2 OR ETM_OPT_CTXTID. But not both.

Cheers
Suzuki

Re: [PATCH v2 6/7] perf cs-etm: Detect pid in VMID for kernel running at EL2

[Suzuki K Poulose]

On 2/4/21 4:00 AM, Leo Yan wrote:
> On Tue, Feb 02, 2021 at 11:29:47PM +0000, Suzuki Kuruppassery Poulose wrote:
>> On 2/2/21 4:38 PM, Leo Yan 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 CONTEXTIDR (Arm32)
>>> or CONTEXTIDR_EL1 (Arm64) is invalid but we have a valid VMID.
>>>
>>> Cc: Mike Leach <mike.leach@linaro.org>
>>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>>> Cc: Al Grant <al.grant@arm.com>
>>> Co-developed-by: Leo Yan <leo.yan@linaro.org>
>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>> Signed-off-by: Leo Yan <leo.yan@linaro.org>
>>> ---
>>>    .../perf/util/cs-etm-decoder/cs-etm-decoder.c | 32 ++++++++++++++++---
>>>    1 file changed, 28 insertions(+), 4 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 3f4bc4050477..fb2a163ff74e 100644
>>> --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
>>> +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
>>> @@ -6,6 +6,7 @@
>>>     * Author: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>     */
>>> +#include <linux/coresight-pmu.h>
>>>    #include <linux/err.h>
>>>    #include <linux/list.h>
>>>    #include <linux/zalloc.h>
>>> @@ -491,13 +492,36 @@ cs_etm_decoder__set_tid(struct cs_etm_queue *etmq,
>>>    			const ocsd_generic_trace_elem *elem,
>>>    			const uint8_t trace_chan_id)
>>>    {
>>> -	pid_t tid;
>>> +	pid_t tid = -1;
>>> +	u64 pid_fmt;
>>> +	int ret;
>>> -	/* Ignore PE_CONTEXT packets that don't have a valid contextID */
>>> -	if (!elem->context.ctxt_id_valid)
>>> +	ret = cs_etm__get_pid_fmt(trace_chan_id, &pid_fmt);
>>> +	if (ret)
>>
>> Is this something we can cache in this function ? e.g,
>> 	static u64 pid_fmt;
>>
>> 	if (!pid_pfmt)
>> 		ret = cs_etm__get_pid_fmt(trace_chan_id, &pid_fmt);
>>
>> As all the ETMs will be running at the same exception level.
> 
> Sorry that I let you repeated your comments again.
> 
> To be honest, I considered this after read your comment in the previous
> series, but I thought it's possible that multiple CPUs have different
> PID format, especially for big.LITTLE arch.  After read your suggestion
> again, I think my concern is not valid, even for big.LITTLE, all CPUs
> should run on the same kernel exception level.
> 
> So will follow up your suggestion to cache "pid_fmt".

No problem.

> 
>>
>>> +		return OCSD_RESP_FATAL_SYS_ERR;
>>> +
>>> +	/*
>>> +	 * 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, Bit ETM_OPT_CTXTID2 is set in this case.
>>> +	 */
>>> +	switch (pid_fmt) {
>>> +	case BIT(ETM_OPT_CTXTID):
>>> +		if (elem->context.ctxt_id_valid)
>>> +			tid = elem->context.context_id;
>>> +		break;
>>> +	case BIT(ETM_OPT_CTXTID2) | BIT(ETM_OPT_CTXTID):
>>
>> I would rather fix the cs_etm__get_pid_fmt() to return either of these
>> as commented. i.e, ETM_OPT_CTXTID or ETM_OPT_CTXTID2. Thus we don't
>> need the this case.
> 
> I explained why I set both bits for ETM_OPT_CTXTID and ETM_OPT_CTXTID2
> in the patch 05/07.  Could you take a look for it?

I have responded to the comment in the patch.

> 
>> With the above two addressed:
>>
>> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> 


Thanks
Suzuki

Re: [PATCH v2 7/7] Documentation: coresight: Add PID tracing description

[Suzuki K Poulose]

On 2/4/21 4:09 AM, Leo Yan wrote:
> Hi Mike,
> 
> On Wed, Feb 03, 2021 at 05:39:54PM +0000, Mike Leach wrote:
> 
> [...]
> 
>>> +2.2) Tracing PID
>>> +
>>> +When the kernel is running at EL2 with Virtualization Host Extensions (VHE),
>>> +perf records CONTEXTIDR_EL2 in the trace data and can be used as PID when
>>> +decoding; and if the kernel is running at EL1 with nVHE, CONTEXTIDR_EL1 is
>>> +traced for PID.
>>> +
>>
>> Would this introductory paragraph be better if is explained where the
>> kernel stores the PID for the different levels, then we logically move
>> on to how to trace this in perf.
>>
>> e.g:-
>>
>> "The lernel can be built to write the PID value into the PE ContextID registers.
>> For a kernel running at EL1, the PID is stored in CONTEXTIDR_EL1.
>> A PE may implement ARM Virtualisation Host Extensions (VHE), were the
>> kernel can run at EL2 as a virtualisation host.
>> In this case the PID value is stored in CONTEXTIDR_EL2.
>> perf provides PMU options which program the ETM to insert these values
>> into the trace data."
> 
> Will in next spin; thanks a lot for writing up!
> 
>>> +To support tracing PID for the kernel runs at different exception levels,
>>> +the PMU formats are defined as follow:
>>> +
>>> +  "contextid1": Available on both EL1 kernel and EL2 kernel.  When the
>>> +                kernel is running at EL1, "contextid1" enables the PID
>>> +                tracing; when the kernel is running at EL2, this enables
>>> +                tracing the PID of guest applications.
>>> +
>>> +  "contextid2": Only usable when the kernel is running at EL2.  When
>>> +                selected, enables PID tracing on EL2 kernel.
>>> +
>>> +  "contextid":  Will be an alias for the option that enables PID
>>> +                tracing.  I.e,
>>> +                contextid == contextid1, on EL1 kernel.
>>> +                contextid == contextid2, on EL2 kernel.
>>> +
>>> +The perf tool automatically sets corresponding bit for the "contextid" config,
>>> +therefore, the user doesn't have to bother which EL the kernel is running.
>>> +
>>> +  i.e, perf record -e cs_etm/contextid/u -- uname
>>> +    or perf record -e cs_etm//u -- uname
>>> +
>>> +will always do the "PID" tracing, independent of the kernel EL.
>>> +
>>
>> This is telling me that both cs_etm// and cs_etm/contextid/ have the
>> same effect - trace PID. Is this correct?
> 

Just to make this clear, this is not a side effect of the patch. The perf
tool driver automatically adds the "contextid" tracing and timestamp for
"system wide" and process bound events, as they traces get mixed into
the single sink. So these options are added implicitly by the perf tool
to make the decoding easier.

> Correct.
> 
>> If so, then contextid, contextid1 and contextid2 have no effect except
>> in specific EL2 circumstances.

These are required when perf tool may not automatically request them.
With this series the EL2 is on par with the EL1, where we get the PID
automatcially in the trace.

And as you rightly said, contextid1, contextid2 are for EL2 specific
usage.

Cheers
Suzuki

> 
> Yes, exactly.
> 
> Thanks,
> Leo
> 
>>> +When the kernel is running at EL2 with VHE, if user wants to trace both the
>>> +PIDs for both host and guest, the two configs "contextid1" and "contextid2"
>>> +can be set at the same time:
>>> +
>>> +  perf record -e cs_etm/contextid1,contextid2/u -- uname
>>> +
>>>
>>
>>
>> Regards
>>
>> Mike
>>
>>
>>>   Generating coverage files for Feedback Directed Optimization: AutoFDO
>>>   ---------------------------------------------------------------------
>>> --
>>> 2.25.1
>>>
>>
>>
>> --
>> Mike Leach
>> Principal Engineer, ARM Ltd.
>> Manchester Design Centre. UK

Re: [PATCH v2 5/7] perf cs-etm: Add helper cs_etm__get_pid_fmt()

[Leo Yan]

On Thu, Feb 04, 2021 at 10:54:24AM +0000, Suzuki Kuruppassery Poulose wrote:

[...]

> > > > +int cs_etm__get_pid_fmt(u8 trace_chan_id, u64 *pid_fmt)
> > > > +{
> > > > +	struct int_node *inode;
> > > > +	u64 *metadata, val;
> > > > +
> > > > +	inode = intlist__find(traceid_list, trace_chan_id);
> > > > +	if (!inode)
> > > > +		return -EINVAL;
> > > > +
> > > > +	metadata = inode->priv;
> > > > +
> > > > +	if (metadata[CS_ETM_MAGIC] == __perf_cs_etmv3_magic) {
> > > > +		val = metadata[CS_ETM_ETMCR];
> > > > +		/* CONTEXTIDR is traced */
> > > > +		if (val & BIT(ETM_OPT_CTXTID))
> > > > +			*pid_fmt = BIT(ETM_OPT_CTXTID);
> > > > +	} else {
> > > > +		val = metadata[CS_ETMV4_TRCCONFIGR];
> > > > +
> > > > +		*pid_fmt = 0;
> > > > +
> > > > +		/* CONTEXTIDR_EL2 is traced */
> > > > +		if (val & (BIT(ETM4_CFG_BIT_VMID) | BIT(ETM4_CFG_BIT_VMID_OPT)))
> > > > +			*pid_fmt = BIT(ETM_OPT_CTXTID2);
> > > > +
> > > > +		/* CONTEXTIDR_EL1 is traced */
> > > > +		if (val & BIT(ETM4_CFG_BIT_CTXTID))
> > > 
> > > I haven't looked at how this gets used. But, Shouldn't this be :
> > > 
> > > 		else if (val & BIT(ETM4_CFG_BIT_CTXTID)) ?
> > 
> > Actually it's deliberately to set both bits ETM_OPT_CTXTID2 and
> > ETM_OPT_CTXTID if user has enable configs "contextid1" and
> > "contextid2".  So this is exactly the reversed flow in the
> > function cs_etmv4_get_config().
> 
> The point is, we don't care if the user selected both options. What we
> care is, where can we find the PID. CONTEXTIDR_EL1 or CONTEXTIDR_EL2.
> As such, get_pid_fmt simply should make that decision and pass it on.
> So, if the CONTEXTIDR_EL2 is selected (which can only be done successfully
> on an EL2 kernel), thats our pid.
> 
> So we should return the format for the PID here. i.e
>  ETM_OPT_CTXTID2 OR ETM_OPT_CTXTID. But not both.

Okay, if so I will follow your suggestion.

Thanks,
Leo

Re: [PATCH v2 7/7] Documentation: coresight: Add PID tracing description

[Mike Leach]

Hi,

On Thu, 4 Feb 2021 at 11:08, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>
> On 2/4/21 4:09 AM, Leo Yan wrote:
> > Hi Mike,
> >
> > On Wed, Feb 03, 2021 at 05:39:54PM +0000, Mike Leach wrote:
> >
> > [...]
> >
> >>> +2.2) Tracing PID
> >>> +
> >>> +When the kernel is running at EL2 with Virtualization Host Extensions (VHE),
> >>> +perf records CONTEXTIDR_EL2 in the trace data and can be used as PID when
> >>> +decoding; and if the kernel is running at EL1 with nVHE, CONTEXTIDR_EL1 is
> >>> +traced for PID.
> >>> +
> >>
> >> Would this introductory paragraph be better if is explained where the
> >> kernel stores the PID for the different levels, then we logically move
> >> on to how to trace this in perf.
> >>
> >> e.g:-
> >>
> >> "The lernel can be built to write the PID value into the PE ContextID registers.
> >> For a kernel running at EL1, the PID is stored in CONTEXTIDR_EL1.
> >> A PE may implement ARM Virtualisation Host Extensions (VHE), were the
> >> kernel can run at EL2 as a virtualisation host.
> >> In this case the PID value is stored in CONTEXTIDR_EL2.
> >> perf provides PMU options which program the ETM to insert these values
> >> into the trace data."
> >
> > Will in next spin; thanks a lot for writing up!
> >
> >>> +To support tracing PID for the kernel runs at different exception levels,
> >>> +the PMU formats are defined as follow:
> >>> +
> >>> +  "contextid1": Available on both EL1 kernel and EL2 kernel.  When the
> >>> +                kernel is running at EL1, "contextid1" enables the PID
> >>> +                tracing; when the kernel is running at EL2, this enables
> >>> +                tracing the PID of guest applications.
> >>> +
> >>> +  "contextid2": Only usable when the kernel is running at EL2.  When
> >>> +                selected, enables PID tracing on EL2 kernel.
> >>> +
> >>> +  "contextid":  Will be an alias for the option that enables PID
> >>> +                tracing.  I.e,
> >>> +                contextid == contextid1, on EL1 kernel.
> >>> +                contextid == contextid2, on EL2 kernel.
> >>> +
> >>> +The perf tool automatically sets corresponding bit for the "contextid" config,
> >>> +therefore, the user doesn't have to bother which EL the kernel is running.
> >>> +
> >>> +  i.e, perf record -e cs_etm/contextid/u -- uname
> >>> +    or perf record -e cs_etm//u -- uname
> >>> +
> >>> +will always do the "PID" tracing, independent of the kernel EL.
> >>> +
> >>
> >> This is telling me that both cs_etm// and cs_etm/contextid/ have the
> >> same effect - trace PID. Is this correct?
> >
>
> Just to make this clear, this is not a side effect of the patch.

Which is fine - but the documentation should accurately reflect what
is happening on the system.
This is a new paragraph about the PID tracing or otherwise, Even if
some of the effects pre-date this patch, they have to be accurately
communicated.
I am also reading the new paragraph in the context of the rest of the
coresight.rst document - which is a user level document explaining the
basic operation of the coresight system and tools.
This document mentions no other perf command line parameters relevant
to coresight other than the @sink option.It actually calls out to the
OpenCSD docs to provide further information.

> The perf
> tool driver automatically adds the "contextid" tracing and timestamp for
> "system wide" and process bound events, as they traces get mixed into
> the single sink. So these options are added implicitly by the perf tool
> to make the decoding easier.
>

That's fine - I have no problem with contextID trace enabled by
default. Context ID is relatively low overhead - and only emitted at
start of trace  / context changes.
But the explanation of the parameters currently reads as though they
always have an effect - and not putting them in there will omit the
effect - unless you spot the very subtle line at the end.

The user does not need to know about parameters that have no effect!

Perhaps a better approach would be to explain the above - an explicit
statement that "perf will always enable PID/ contextID tracing at the
relevant EL - but for EL2 it is possible to make specific adjustments
using parameters......."

Cheers

Mike


> > Correct.
> >
> >> If so, then contextid, contextid1 and contextid2 have no effect except
> >> in specific EL2 circumstances.
>
> These are required when perf tool may not automatically request them.
> With this series the EL2 is on par with the EL1, where we get the PID
> automatcially in the trace.
>
> And as you rightly said, contextid1, contextid2 are for EL2 specific
> usage.
>
> Cheers
> Suzuki
>
> >
> > Yes, exactly.
> >
> > Thanks,
> > Leo
> >
> >>> +When the kernel is running at EL2 with VHE, if user wants to trace both the
> >>> +PIDs for both host and guest, the two configs "contextid1" and "contextid2"
> >>> +can be set at the same time:
> >>> +
> >>> +  perf record -e cs_etm/contextid1,contextid2/u -- uname
> >>> +
> >>>
> >>
> >>
> >> Regards
> >>
> >> Mike
> >>
> >>
> >>>   Generating coverage files for Feedback Directed Optimization: AutoFDO
> >>>   ---------------------------------------------------------------------
> >>> --
> >>> 2.25.1
> >>>
> >>
> >>
> >> --
> >> Mike Leach
> >> Principal Engineer, ARM Ltd.
> >> Manchester Design Centre. UK
>


-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

Re: [PATCH v2 7/7] Documentation: coresight: Add PID tracing description

[Leo Yan]

On Thu, Feb 04, 2021 at 12:14:12PM +0000, Mike Leach wrote:

[...]

> > >>> +To support tracing PID for the kernel runs at different exception levels,
> > >>> +the PMU formats are defined as follow:
> > >>> +
> > >>> +  "contextid1": Available on both EL1 kernel and EL2 kernel.  When the
> > >>> +                kernel is running at EL1, "contextid1" enables the PID
> > >>> +                tracing; when the kernel is running at EL2, this enables
> > >>> +                tracing the PID of guest applications.
> > >>> +
> > >>> +  "contextid2": Only usable when the kernel is running at EL2.  When
> > >>> +                selected, enables PID tracing on EL2 kernel.
> > >>> +
> > >>> +  "contextid":  Will be an alias for the option that enables PID
> > >>> +                tracing.  I.e,
> > >>> +                contextid == contextid1, on EL1 kernel.
> > >>> +                contextid == contextid2, on EL2 kernel.
> > >>> +
> > >>> +The perf tool automatically sets corresponding bit for the "contextid" config,
> > >>> +therefore, the user doesn't have to bother which EL the kernel is running.
> > >>> +
> > >>> +  i.e, perf record -e cs_etm/contextid/u -- uname
> > >>> +    or perf record -e cs_etm//u -- uname
> > >>> +
> > >>> +will always do the "PID" tracing, independent of the kernel EL.
> > >>> +
> > >>
> > >> This is telling me that both cs_etm// and cs_etm/contextid/ have the
> > >> same effect - trace PID. Is this correct?
> > >
> >
> > Just to make this clear, this is not a side effect of the patch.
> 
> Which is fine - but the documentation should accurately reflect what
> is happening on the system.
> This is a new paragraph about the PID tracing or otherwise, Even if
> some of the effects pre-date this patch, they have to be accurately
> communicated.
> I am also reading the new paragraph in the context of the rest of the
> coresight.rst document - which is a user level document explaining the
> basic operation of the coresight system and tools.
> This document mentions no other perf command line parameters relevant
> to coresight other than the @sink option.It actually calls out to the
> OpenCSD docs to provide further information.
> 
> > The perf
> > tool driver automatically adds the "contextid" tracing and timestamp for
> > "system wide" and process bound events, as they traces get mixed into
> > the single sink. So these options are added implicitly by the perf tool
> > to make the decoding easier.
> >
> 
> That's fine - I have no problem with contextID trace enabled by
> default. Context ID is relatively low overhead - and only emitted at
> start of trace  / context changes.
> But the explanation of the parameters currently reads as though they
> always have an effect - and not putting them in there will omit the
> effect - unless you spot the very subtle line at the end.
> 
> The user does not need to know about parameters that have no effect!

Thanks for the suggestion, Mike.

> Perhaps a better approach would be to explain the above - an explicit
> statement that "perf will always enable PID/ contextID tracing at the
> relevant EL - but for EL2 it is possible to make specific adjustments
> using parameters......."

Usually users assume the PMU format has no effect if without set it; but
this is not the case for the config "contextid", this config has been
automatically enabled by perf tool.

Based on your suggesiton, will refine the descrption for two things:
clarify what's the common usage for EL1/EL2, and what's specific for
EL2.

Thanks,
Leo

Re: [PATCH v2 3/7] perf cs-etm: Fix bitmap for option

[Mike Leach]

On Tue, 2 Feb 2021 at 16:39, Leo Yan <leo.yan@linaro.org> wrote:
>
> From: Suzuki K Poulose <suzuki.poulose@arm.com>
>
> When set option with macros ETM_OPT_CTXTID and ETM_OPT_TS, it wrongly
> takes these two values (14 and 28 prespectively) as bit masks, but
> actually both are the offset for bits.  But this doesn't lead to
> further failure due to the AND logic operation will be always true for
> ETM_OPT_CTXTID / ETM_OPT_TS.
>
> This patch defines new independent macros (rather than using the
> "config" bits) for requesting the "contextid" and "timestamp" for
> cs_etm_set_option().
>
> [leoy: Extract the change as a separate patch for easier review]
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  tools/perf/arch/arm/util/cs-etm.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
> index bd446aba64f7..c25c878fd06c 100644
> --- a/tools/perf/arch/arm/util/cs-etm.c
> +++ b/tools/perf/arch/arm/util/cs-etm.c
> @@ -156,6 +156,10 @@ static int cs_etm_set_timestamp(struct auxtrace_record *itr,
>         return err;
>  }
>
> +#define ETM_SET_OPT_CTXTID     (1 << 0)
> +#define ETM_SET_OPT_TS         (1 << 1)
> +#define ETM_SET_OPT_MASK       (ETM_SET_OPT_CTXTID | ETM_SET_OPT_TS)
> +
>  static int cs_etm_set_option(struct auxtrace_record *itr,
>                              struct evsel *evsel, u32 option)
>  {
> @@ -169,17 +173,17 @@ static int cs_etm_set_option(struct auxtrace_record *itr,
>                     !cpu_map__has(online_cpus, i))
>                         continue;
>
> -               if (option & ETM_OPT_CTXTID) {
> +               if (option & ETM_SET_OPT_CTXTID) {
>                         err = cs_etm_set_context_id(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 +410,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_CTXTID | ETM_SET_OPT_TS);
>                 if (err)
>                         goto out;
>         }
> --
> 2.25.1
>

Reivewed-by: Mike Leach <mike.leach@linaro.org>
-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

Re: [PATCH v2 2/7] coresight: etm-perf: Support PID tracing for kernel at EL2

[Mike Leach]

On Tue, 2 Feb 2021 at 23:06, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>
> On 2/2/21 4:38 PM, Leo Yan wrote:
> > From: Suzuki K Poulose <suzuki.poulose@arm.com>
> >
> > 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 CONTEXTIDR_EL1.  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 extends option CTXTID with an extra bit
> > ETM_OPT_CTXTID2 (bit 15), thus on an EL2 kernel, we will have another
> > bit available for the perf tool: ETM_OPT_CTXTID is for kernel running in
> > EL1, ETM_OPT_CTXTID2 is used when kernel runs in EL2 with VHE enabled.
> >
> > The tool must be backward compatible for users, i.e, "contextid" today
> > traces PID and that should remain the same; for this purpose, the perf
> > tool is updated to automatically set corresponding bit for the
> > "contextid" config, therefore, the user doesn't have to bother which EL
> > the kernel is running.
> >
> >    i.e, perf record -e cs_etm/contextid/u --
> >
> > will always do the "pid" tracing, independent of the kernel EL.
> >
> > The driver parses the format "contextid", which traces CONTEXTIDR_EL1
> > for ETM_OPT_CTXTID (on EL1 kernel) and traces CONTEXTIDR_EL2 for
> > ETM_OPT_CTXTID2 (on EL2 kernel).
> >
> > Besides the enhancement for format "contexid", extra two formats are
> > introduced: "contextid1" and "contextid2".  This considers to support
> > tracing both CONTEXTIDR_EL1 and CONTEXTIDR_EL2 when the kernel is
> > running at EL2.  Finally, the PMU formats are defined as follow:
> >
> >    "contextid1": Available on both EL1 kernel and EL2 kernel.  When the
> >                  kernel is running at EL1, "contextid1" enables the PID
> >               tracing; when the kernel is running at EL2, this enables
> >               tracing the PID of guest applications.
> >
> >    "contextid2": Only usable when the kernel is running at EL2.  When
> >                  selected, enables PID tracing on EL2 kernel.
> >
> >    "contextid":  Will be an alias for the option that enables PID
> >                  tracing.  I.e,
> >                  contextid == contextid1, on EL1 kernel.
> >                  contextid == contextid2, on EL2 kernel.
> >
> > 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>
>
> You may add the following line here :
>
> [ Added two config formats: contextid1, contextid2 ]
>
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
>
> The patch as such looks good to me.
>
> Suzuki

Reviewed-by: Mike Leach <mike.leach@linaro.org>


--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

Re: [PATCH v2 4/7] perf cs-etm: Support PID tracing in config

[Mike Leach]

On Tue, 2 Feb 2021 at 16:39, Leo Yan <leo.yan@linaro.org> wrote:
>
> From: Suzuki K Poulose <suzuki.poulose@arm.com>
>
> If the kernel is running at EL2, the pid of a task is exposed via VMID
> instead of the CONTEXTID.  Add support for this in the perf tool.
>
> This patch respects user setting if user has specified any configs
> from "contextid", "contextid1" or "contextid2"; otherwise, it
> dynamically sets config based on PMU format "contextid".
>
> Cc: Mike Leach <mike.leach@linaro.org>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: Al Grant <al.grant@arm.com>
> Co-developed-by: Leo Yan <leo.yan@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  tools/include/linux/coresight-pmu.h |  3 ++
>  tools/perf/arch/arm/util/cs-etm.c   | 61 +++++++++++++++++++++++------
>  2 files changed, 52 insertions(+), 12 deletions(-)
>
> diff --git a/tools/include/linux/coresight-pmu.h b/tools/include/linux/coresight-pmu.h
> index 5dc47cfdcf07..4ac5c081af93 100644
> --- a/tools/include/linux/coresight-pmu.h
> +++ b/tools/include/linux/coresight-pmu.h
> @@ -20,14 +20,17 @@
>   */
>  #define ETM_OPT_CYCACC         12
>  #define ETM_OPT_CTXTID         14
> +#define ETM_OPT_CTXTID2                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 c25c878fd06c..fa6f91a7c8a1 100644
> --- a/tools/perf/arch/arm/util/cs-etm.c
> +++ b/tools/perf/arch/arm/util/cs-etm.c
> @@ -67,6 +67,7 @@ static int cs_etm_set_context_id(struct auxtrace_record *itr,
>         char path[PATH_MAX];
>         int err = -EINVAL;
>         u32 val;
> +       u64 contextid;
>
>         ptr = container_of(itr, struct cs_etm_recording, itr);
>         cs_etm_pmu = ptr->cs_etm_pmu;
> @@ -86,25 +87,59 @@ static int cs_etm_set_context_id(struct auxtrace_record *itr,
>                 goto out;
>         }
>
> +       /* User has configured for PID tracing, respects it. */
> +       contextid = evsel->core.attr.config &
> +                       (BIT(ETM_OPT_CTXTID) | BIT(ETM_OPT_CTXTID2));
> +
>         /*
> -        * 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.
> +        * If user doesn't configure the contextid format, parse PMU format and
> +        * enable PID tracing according to the "contextid" format bits:
> +        *
> +        *   If bit ETM_OPT_CTXTID is set, trace CONTEXTIDR_EL1;
> +        *   If bit ETM_OPT_CTXTID2 is set, trace CONTEXTIDR_EL2.
>          */
> -       val = BMVAL(val, 5, 9);
> -       if (!val || val != 0x4) {
> -               err = -EINVAL;
> -               goto out;
> +       if (!contextid)
> +               contextid = perf_pmu__format_bits(&cs_etm_pmu->format,
> +                                                 "contextid");
> +
> +       if (contextid & BIT(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) {
> +                       pr_err("%s: CONTEXTIDR_EL1 isn't supported\n",
> +                              CORESIGHT_ETM_PMU_NAME);
> +                       err = -EINVAL;
> +                       goto out;
> +               }
> +       }
> +
> +       if (contextid & BIT(ETM_OPT_CTXTID2)) {
> +               /*
> +                * TRCIDR2.VMIDOPT[30:29] != 0 and
> +                * TRCIDR2.VMIDSIZE[14:10] == 0b00100 (32bit virtual contextid)
> +                * We can't support CONTEXTIDR in VMID if the size of the
> +                * virtual context id is < 32bit.
> +                * Any value of VMIDSIZE >= 4 (i.e, > 32bit) is fine for us.
> +                */
> +               if (!BMVAL(val, 29, 30) || BMVAL(val, 10, 14) < 4) {
> +                       pr_err("%s: CONTEXTIDR_EL2 isn't supported\n",
> +                              CORESIGHT_ETM_PMU_NAME);
> +                       err = -EINVAL;
> +                       goto out;
> +               }
>         }
>
>         /* All good, let the kernel know */
> -       evsel->core.attr.config |= (1 << ETM_OPT_CTXTID);
> +       evsel->core.attr.config |= contextid;
>         err = 0;
>
>  out:
> -
>         return err;
>  }
>
> @@ -489,7 +524,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_CTXTID2))
> +               config |= BIT(ETM4_CFG_BIT_VMID) |
> +                         BIT(ETM4_CFG_BIT_VMID_OPT);
>         return config;
>  }
>
> --
> 2.25.1
>

reviewed-by: Mike Leach <mike.leach@linaro.org>


--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK