Hi Leo,

On Sat, 9 Jan 2021 at 07:44, Leo Yan leo.yan@linaro.org 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 adds a new option, contextid_in_vmid as a separate config. Thus on an EL2 kernel, we will have two options available for the perf tool. However, to make it easier for the user to do pid tracing, we add a new format which will default to "contextid" (on EL1 kernel) or "contextid_in_vmid" (on EL2 kernel). So that the user doesn't have to bother which EL the kernel is running.

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

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

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

Cc: Mathieu Poirier mathieu.poirier@linaro.org Cc: Al Grant al.grant@arm.com Cc: Mike Leach mike.leach@linaro.org Signed-off-by: Suzuki K Poulose suzuki.poulose@arm.com Signed-off-by: Leo Yan leo.yan@linaro.org

---

drivers/hwtracing/coresight/coresight-etm-perf.c | 14 ++++++++++++++ drivers/hwtracing/coresight/coresight-etm4x-core.c | 9 +++++++++ include/linux/coresight-pmu.h | 11 +++++++---- 3 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index bdc34ca449f7..f763def145e4 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -30,14 +30,28 @@ static DEFINE_PER_CPU(struct coresight_device *, csdev_src); /* ETMv3.5/PTM's ETMCR is 'config' */ PMU_FORMAT_ATTR(cycacc, "config:" __stringify(ETM_OPT_CYCACC)); PMU_FORMAT_ATTR(contextid, "config:" __stringify(ETM_OPT_CTXTID)); +PMU_FORMAT_ATTR(contextid_in_vmid, "config:" __stringify(ETM_OPT_CTXTID_IN_VMID)); PMU_FORMAT_ATTR(timestamp, "config:" __stringify(ETM_OPT_TS)); PMU_FORMAT_ATTR(retstack, "config:" __stringify(ETM_OPT_RETSTK)); /* Sink ID - same for all ETMs */ PMU_FORMAT_ATTR(sinkid, "config2:0-31");

+static ssize_t format_attr_pid_show(struct device *dev,

•                               struct device_attribute *attr,
  

•                               char *page)
  

+{

•   int pid_fmt = is_kernel_in_hyp_mode() ? ETM_OPT_CTXTID_IN_VMID : ETM_OPT_CTXTID;
  

•   return sprintf(page, "config:%d\n", pid_fmt);
  

+}

+struct device_attribute format_attr_pid = __ATTR(pid, 0444, format_attr_pid_show, NULL);

static struct attribute *etm_config_formats_attr[] = { &format_attr_cycacc.attr, &format_attr_contextid.attr,

•   &format_attr_contextid_in_vmid.attr,
  

•   &format_attr_pid.attr,
    &format_attr_timestamp.attr,
    &format_attr_retstack.attr,
    &format_attr_sinkid.attr,
  

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index b20b6ff17cf6..8b7c7a8b2874 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -477,6 +477,15 @@ static int etm4_parse_event_config(struct etmv4_drvdata *drvdata, /* bit[6], Context ID tracing bit */ config->cfg |= BIT(ETM4_CFG_BIT_CTXTID);

•   /* Do not enable VMID tracing if we are not running in EL2 */
  

•   if (attr->config & BIT(ETM_OPT_CTXTID_IN_VMID)) {
  

•           if (!is_kernel_in_hyp_mode()) {
  

•                   ret = -EINVAL;
  

•                   goto out;
  

•           }
  

•           config->cfg |= BIT(ETM4_CFG_BIT_VMID) | BIT(ETM4_CFG_BIT_VMID_OPT);
  

•   }
  

•   /* return stack - enable if selected and supported */
    if ((attr->config & BIT(ETM_OPT_RETSTK)) && drvdata->retstack)
            /* bit[12], Return stack enable bit */
  

diff --git a/include/linux/coresight-pmu.h b/include/linux/coresight-pmu.h index b0e35eec6499..927c6285ce5d 100644 --- a/include/linux/coresight-pmu.h +++ b/include/linux/coresight-pmu.h @@ -11,16 +11,19 @@ #define CORESIGHT_ETM_PMU_SEED 0x10

/* ETMv3.5/PTM's ETMCR config bit */ -#define ETM_OPT_CYCACC 12 -#define ETM_OPT_CTXTID 14 -#define ETM_OPT_TS 28 -#define ETM_OPT_RETSTK 29 +#define ETM_OPT_CYCACC 12 +#define ETM_OPT_CTXTID 14 +#define ETM_OPT_CTXTID_IN_VMID 15

Minor issue here - ETMv3.x / PTM cannot trace CXTID in VMID so this may better be named ETM4_OPT_CTXTID_IN_VMID, rather than be grouped with the ETM3.5 options?

Regards

Mike

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

Hi Mike,

On Mon, Jan 11, 2021 at 04:22:39PM +0000, Mike Leach wrote:

[...]

...

diff --git a/include/linux/coresight-pmu.h b/include/linux/coresight-pmu.h index b0e35eec6499..927c6285ce5d 100644 --- a/include/linux/coresight-pmu.h +++ b/include/linux/coresight-pmu.h @@ -11,16 +11,19 @@ #define CORESIGHT_ETM_PMU_SEED 0x10

/* ETMv3.5/PTM's ETMCR config bit */ -#define ETM_OPT_CYCACC 12 -#define ETM_OPT_CTXTID 14 -#define ETM_OPT_TS 28 -#define ETM_OPT_RETSTK 29 +#define ETM_OPT_CYCACC 12 +#define ETM_OPT_CTXTID 14 +#define ETM_OPT_CTXTID_IN_VMID 15

Minor issue here - ETMv3.x / PTM cannot trace CXTID in VMID so this may better be named ETM4_OPT_CTXTID_IN_VMID, rather than be grouped with the ETM3.5 options?

I looked into this suggestion but found it's complex than I assumed. This config bits are not only used for ETMv3.x / PTM, it's also used as an configuration interface between user space in Perf and kernel drivers.

For example, in the userspace, perf tool sets bit ETM_OPT_TS to enable timestamp [1], this is same for ETMv3 and ETMv4. In the kernel side, the configuration is directly used ETMv3 (in coresight-etm3x-core.c), but the configuration bits are converted for ETMv4 in the function etm4_parse_event_config() [2].

So this is a historical issue, at the early period ETMv3 and ETMv4 can be compatible with each other for configurations, but after evoluation, some configs only belong to ETMv4 and cannot be applied on ETMv3 anymore, but we still use ETMv3.5 config bits as the interface between kernel and userspace.

I'd like suggest we use a saperate patch set to refactor the configuration bits, e.g. define the arbitrary configuration bits as interface between user space and kernel, these bits are neutral for any ETM version, in the kernel ETM drivers need to convert to its own configuration formats. For this patch, I want to keep current change as it is, is this okay for you?

Thanks, Leo

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tool... [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driv...

Hi Leo,

On Tue, 12 Jan 2021 at 08:58, Leo Yan leo.yan@linaro.org wrote:

Hi Mike,

On Mon, Jan 11, 2021 at 04:22:39PM +0000, Mike Leach wrote:

[...]

...

...

diff --git a/include/linux/coresight-pmu.h b/include/linux/coresight-pmu.h index b0e35eec6499..927c6285ce5d 100644 --- a/include/linux/coresight-pmu.h +++ b/include/linux/coresight-pmu.h @@ -11,16 +11,19 @@ #define CORESIGHT_ETM_PMU_SEED 0x10

/* ETMv3.5/PTM's ETMCR config bit */ -#define ETM_OPT_CYCACC 12 -#define ETM_OPT_CTXTID 14 -#define ETM_OPT_TS 28 -#define ETM_OPT_RETSTK 29 +#define ETM_OPT_CYCACC 12 +#define ETM_OPT_CTXTID 14 +#define ETM_OPT_CTXTID_IN_VMID 15

Minor issue here - ETMv3.x / PTM cannot trace CXTID in VMID so this may better be named ETM4_OPT_CTXTID_IN_VMID, rather than be grouped with the ETM3.5 options?

I looked into this suggestion but found it's complex than I assumed. This config bits are not only used for ETMv3.x / PTM, it's also used as an configuration interface between user space in Perf and kernel drivers.

For example, in the userspace, perf tool sets bit ETM_OPT_TS to enable timestamp [1], this is same for ETMv3 and ETMv4. In the kernel side, the configuration is directly used ETMv3 (in coresight-etm3x-core.c), but the configuration bits are converted for ETMv4 in the function etm4_parse_event_config() [2].

So this is a historical issue, at the early period ETMv3 and ETMv4 can be compatible with each other for configurations, but after evoluation, some configs only belong to ETMv4 and cannot be applied on ETMv3 anymore, but we still use ETMv3.5 config bits as the interface between kernel and userspace.

I was aware that etm3/ptm used these bits as both the options and the bit values for direct writing to the ETMCR register for ETMv3, and re-mapped to appropriate register values in ETMv4. In the past we have re-used etmv3.5 bit definitions ETM_xxx when appropriate, but where unique to ETM4 we have used a ETM4_xxx naming convention.

I am not suggesting re-factoring the options completely, just re-naming this single option to make it clear it is unique to ETM4+.

Looking at the etmv3 driver, at present it does not actually appear to support contextid tracing - and when it does, both bits 14 and 15 will be required to be used - as ETMCR defines these bits as ContextID size. Should this ever get fixed. then having an overlapping option bit - that appears to be valid for ETMv3 will be confusing.

Regards

Mike

I'd like suggest we use a saperate patch set to refactor the configuration bits, e.g. define the arbitrary configuration bits as interface between user space and kernel, these bits are neutral for any ETM version, in the kernel ETM drivers need to convert to its own configuration formats. For this patch, I want to keep current change as it is, is this okay for you?

Thanks, Leo

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tool... [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driv...

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

Hi Leo,

On Tue, 12 Jan 2021 at 14:15, Leo Yan leo.yan@linaro.org wrote:

Hi Mike,

On Tue, Jan 12, 2021 at 11:23:03AM +0000, Mike Leach wrote:

...

Hi Leo,

On Tue, 12 Jan 2021 at 08:58, Leo Yan leo.yan@linaro.org wrote:

...

Hi Mike,

On Mon, Jan 11, 2021 at 04:22:39PM +0000, Mike Leach wrote:

[...]

...

...

diff --git a/include/linux/coresight-pmu.h b/include/linux/coresight-pmu.h index b0e35eec6499..927c6285ce5d 100644 --- a/include/linux/coresight-pmu.h +++ b/include/linux/coresight-pmu.h @@ -11,16 +11,19 @@ #define CORESIGHT_ETM_PMU_SEED 0x10

/* ETMv3.5/PTM's ETMCR config bit */ -#define ETM_OPT_CYCACC 12 -#define ETM_OPT_CTXTID 14 -#define ETM_OPT_TS 28 -#define ETM_OPT_RETSTK 29 +#define ETM_OPT_CYCACC 12 +#define ETM_OPT_CTXTID 14 +#define ETM_OPT_CTXTID_IN_VMID 15

Minor issue here - ETMv3.x / PTM cannot trace CXTID in VMID so this may better be named ETM4_OPT_CTXTID_IN_VMID, rather than be grouped with the ETM3.5 options?

I looked into this suggestion but found it's complex than I assumed. This config bits are not only used for ETMv3.x / PTM, it's also used as an configuration interface between user space in Perf and kernel drivers.

For example, in the userspace, perf tool sets bit ETM_OPT_TS to enable timestamp [1], this is same for ETMv3 and ETMv4. In the kernel side, the configuration is directly used ETMv3 (in coresight-etm3x-core.c), but the configuration bits are converted for ETMv4 in the function etm4_parse_event_config() [2].

So this is a historical issue, at the early period ETMv3 and ETMv4 can be compatible with each other for configurations, but after evoluation, some configs only belong to ETMv4 and cannot be applied on ETMv3 anymore, but we still use ETMv3.5 config bits as the interface between kernel and userspace.

I was aware that etm3/ptm used these bits as both the options and the bit values for direct writing to the ETMCR register for ETMv3, and re-mapped to appropriate register values in ETMv4. In the past we have re-used etmv3.5 bit definitions ETM_xxx when appropriate, but where unique to ETM4 we have used a ETM4_xxx naming convention.

I am concern this approach is not friendly for extension; for example, let's say IP ETM5 with defined bit 28 as CTXTID, if add a new option for it, we need to define macro as:

    #define ETM5_OPT_CTXTID         28

This will result in confliction with the existed option ETM_OPT_TS and it is hard for maintenance for options, since there have different prefixes (like ETM_OPT_xxx, ETM4_OPT_xxx, ETM5_OPT_xxx, etc).

No it will not - we don't need a new option for CTXTID in a hypothetical ETM5 - as we use the existing one for ETM3 and map it to the correct bit, just as ETM4 does.

I'd like to take option as knob to enable or disable hardware feature; the low level drivers should set the appropriate values for registers based on different options.

Furthermore, ETM driver should report error when detect any option is not supported, I.e. ETM3 driver should report failure if user wrongly set the option ETM_OPT_CTXTID_IN_VMID.

...

I am not suggesting re-factoring the options completely, just re-naming this single option to make it clear it is unique to ETM4+.

Here I perfer Suzuki's suggestion to simply refine comments, something like below:

/*

• Below are bit offsets for perf options, most of them are orignally
• coming from ETMv3.5/PTM's ETMCR config bits (so far except
• ETM_OPT_CTXTID_IN_VMID is only used for ETMv4).
• 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_CTXTID_IN_VMID 15 #define ETM_OPT_TS 28 #define ETM_OPT_RETSTK 29

...

Looking at the etmv3 driver, at present it does not actually appear to support contextid tracing - and when it does, both bits 14 and 15 will be required to be used - as ETMCR defines these bits as ContextID size. Should this ever get fixed.

Good catch! Seems to me, this is a good example that we should distinguish the definition between Perf options and config bits :)

...

then having an overlapping option bit - that appears to be valid for ETMv3 will be confusing.

I hope the the proposed change can avoid the confusion, if have concern, please let me know.

Thanks a lot for suggestions, Leo

If you think that clarification via comment is better than a change of name then go ahead.

Regards

Mike

Hi Mathieu

On 1/15/21 10:30 PM, Mathieu Poirier wrote:

Hey guys,

On Sat, Jan 09, 2021 at 03:44:29PM +0800, 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 adds a new option, contextid_in_vmid as a separate config. Thus on an EL2 kernel, we will have two options available for the perf tool. However, to make it easier for the user to do pid tracing, we add a new format which will default to "contextid" (on EL1 kernel) or "contextid_in_vmid" (on EL2 kernel). So that the user doesn't have to bother which EL the kernel is running.

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

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

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

Cc: Mathieu Poirier mathieu.poirier@linaro.org Cc: Al Grant al.grant@arm.com Cc: Mike Leach mike.leach@linaro.org Signed-off-by: Suzuki K Poulose suzuki.poulose@arm.com Signed-off-by: Leo Yan leo.yan@linaro.org

---

drivers/hwtracing/coresight/coresight-etm-perf.c | 14 ++++++++++++++ drivers/hwtracing/coresight/coresight-etm4x-core.c | 9 +++++++++ include/linux/coresight-pmu.h | 11 +++++++---- 3 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index bdc34ca449f7..f763def145e4 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -30,14 +30,28 @@ static DEFINE_PER_CPU(struct coresight_device *, csdev_src); /* ETMv3.5/PTM's ETMCR is 'config' */ PMU_FORMAT_ATTR(cycacc, "config:" __stringify(ETM_OPT_CYCACC)); PMU_FORMAT_ATTR(contextid, "config:" __stringify(ETM_OPT_CTXTID)); +PMU_FORMAT_ATTR(contextid_in_vmid, "config:" __stringify(ETM_OPT_CTXTID_IN_VMID));

I am not convinced adding this new contextid_in_vmid is the best way forward.

...

PMU_FORMAT_ATTR(timestamp, "config:" __stringify(ETM_OPT_TS)); PMU_FORMAT_ATTR(retstack, "config:" __stringify(ETM_OPT_RETSTK)); /* Sink ID - same for all ETMs */ PMU_FORMAT_ATTR(sinkid, "config2:0-31"); +static ssize_t format_attr_pid_show(struct device *dev,

• 		    struct device_attribute *attr,
  

• 		    char *page)
  

+{

• int pid_fmt = is_kernel_in_hyp_mode() ? ETM_OPT_CTXTID_IN_VMID : ETM_OPT_CTXTID;
• return sprintf(page, "config:%d\n", pid_fmt);

+}

+struct device_attribute format_attr_pid = __ATTR(pid, 0444, format_attr_pid_show, NULL);

The same applies here. PMU format bits are options the PMU supports rather than a representation of the hardware, making bit numbering arbitrary. A such we don't explicitly need a contextid_in_vmid option. Making the current contextid variable, the same it was done for 'pid', should be sufficient. Based on the value carried by contexid, i.e 14 or 15, the perf tools will know where to get the contextID.

With regards to backward functionality, user who hard code 'config' on the perf command line won't get the results they want when the kernel is at EL2 anyway.

The kernel, with function is_kernel_in_hyp_mode(), is not an issue.

I did think about that. The reason behind using a new alias is vaguely mentioned in the description. If a host perf session wants to trace a VM with the contextid_el1, there is no option for that with "contextid" flipped to trace "contextid_el2". This is precisely why I preferred keeping both the hardware configurations and let the kernel choose the right one for the EL, by having an alias. i.e,

perf record -e cs_etm/contextid,contextid_in_vmid/ vm

could still trace the VM vcpu threads and the CID changes within the VM. (This is triggered from the host, so VM support is not necessary).

If we decide not to do this, or change the meaning of contextid now to mean "pid" and change it in the future back to what it really means for supporting such scenarios above, then we are going to be back where we are with the proposal.

Suzuki

On Sat, Jan 09, 2021 at 03:44:30PM +0800, Leo Yan wrote:

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

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

By default the perf tool requests contextid and timestamp for task bound events. Instead of hard coding contextid, switch to "pid" config exposed by the kernel. While at it, define new independent macros (rather than using the "config" bits) for requesting the "pid" and "timestamp" for cs_etm_set_option(), since the PID config is now dynamic depending on the kernel exception level.

Cc: Mike Leach mike.leach@linaro.org Cc: Mathieu Poirier mathieu.poirier@linaro.org Cc: Al Grant al.grant@arm.com Signed-off-by: Suzuki K Poulose suzuki.poulose@arm.com Signed-off-by: Leo Yan leo.yan@linaro.org

---

tools/include/linux/coresight-pmu.h | 11 +++-- tools/perf/arch/arm/util/cs-etm.c | 68 ++++++++++++++++++++++------- 2 files changed, 59 insertions(+), 20 deletions(-)

diff --git a/tools/include/linux/coresight-pmu.h b/tools/include/linux/coresight-pmu.h index b0e35eec6499..927c6285ce5d 100644 --- a/tools/include/linux/coresight-pmu.h +++ b/tools/include/linux/coresight-pmu.h @@ -11,16 +11,19 @@ #define CORESIGHT_ETM_PMU_SEED 0x10 /* ETMv3.5/PTM's ETMCR config bit */ -#define ETM_OPT_CYCACC 12 -#define ETM_OPT_CTXTID 14 -#define ETM_OPT_TS 28 -#define ETM_OPT_RETSTK 29 +#define ETM_OPT_CYCACC 12 +#define ETM_OPT_CTXTID 14 +#define ETM_OPT_CTXTID_IN_VMID 15 +#define ETM_OPT_TS 28 +#define ETM_OPT_RETSTK 29 /* ETMv4 CONFIGR programming bits for the ETM OPTs */ #define ETM4_CFG_BIT_CYCACC 4 #define ETM4_CFG_BIT_CTXTID 6 +#define ETM4_CFG_BIT_VMID 7 #define ETM4_CFG_BIT_TS 11 #define ETM4_CFG_BIT_RETSTK 12 +#define ETM4_CFG_BIT_VMID_OPT 15

Wasn't this done in the previous patch?

static inline int coresight_get_trace_id(int cpu) { diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c index cad7bf783413..fad7b6e13ccc 100644 --- a/tools/perf/arch/arm/util/cs-etm.c +++ b/tools/perf/arch/arm/util/cs-etm.c @@ -59,14 +59,15 @@ static const char *metadata_etmv4_ro[CS_ETMV4_PRIV_MAX] = { static bool cs_etm_is_etmv4(struct auxtrace_record *itr, int cpu); -static int cs_etm_set_context_id(struct auxtrace_record *itr,

• 		 struct evsel *evsel, int cpu)
  

+static int cs_etm_set_pid(struct auxtrace_record *itr,

• 	  struct evsel *evsel, int cpu)
  

{ struct cs_etm_recording *ptr; struct perf_pmu *cs_etm_pmu; char path[PATH_MAX]; int err = -EINVAL; u32 val;

• u64 pid_fmt;

ptr = container_of(itr, struct cs_etm_recording, itr); cs_etm_pmu = ptr->cs_etm_pmu; @@ -86,21 +87,50 @@ static int cs_etm_set_context_id(struct auxtrace_record *itr, goto out; }

• pid_fmt = perf_pmu__format_bits(&cs_etm_pmu->format, "pid");

If we go with my suggestion from patch 1 this would become "contextid".

I think what follows below is all good.

/*

• * 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 the kernel doesn't support the "pid" format (older kernel),
  

• * fall back to using the CTXTID.
  

  */

• val = BMVAL(val, 5, 9);
• if (!val || val != 0x4) {

• if (!pid_fmt)

• pid_fmt = 1ULL << ETM_OPT_CTXTID;
  

• switch (pid_fmt) {
• case (1ULL << ETM_OPT_CTXTID):

• /*
  

•  * TRCIDR2.CIDSIZE, bit [9-5], indicates whether contextID
  

•  * tracing is supported:
  

•  *  0b00000 Context ID tracing is not supported.
  

•  *  0b00100 Maximum of 32-bit Context ID size.
  

•  *  All other values are reserved.
  

•  */
  

• val = BMVAL(val, 5, 9);
  

• if (!val || val != 0x4) {
  

• 	err = -EINVAL;
  

• 	goto out;
  

• }
  

• break;
  

• case (1ULL << ETM_OPT_CTXTID_IN_VMID):

• /*
  

•  * TRCIDR2.VMIDOPT[30:29] != 0 and
  

•  * TRCIDR2.VMIDSIZE[14:10] == 0b00100 (32bit virtual 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) {
  

• 	err = -EINVAL;
  

• 	goto out;
  

• }
  

• break;
  

• default: err = -EINVAL; goto out; }

• /* All good, let the kernel know */

• evsel->core.attr.config |= (1 << ETM_OPT_CTXTID);

• evsel->core.attr.config |= pid_fmt; err = 0;

out: @@ -156,6 +186,10 @@ static int cs_etm_set_timestamp(struct auxtrace_record *itr, return err; } +#define ETM_SET_OPT_PID (1 << 0) +#define ETM_SET_OPT_TS (1 << 1) +#define ETM_SET_OPT_MASK (ETM_SET_OPT_PID | ETM_SET_OPT_TS)

I don't think we need this as part of the current patchset. Back in the days I thought it would be a good idea to use the ETMv3.5/PTM configuration bits but in hindsight it wasn't. That's what happens when your crystal ball isn't working.

That being said we can start treating the perf_event_attr::config bits arbitrarily. Someone pointed out that it might get confusing but I don't think it is anything that can't be overcomed with good in-line documentation.

static int cs_etm_set_option(struct auxtrace_record *itr, struct evsel *evsel, u32 option) { @@ -169,17 +203,17 @@ static int cs_etm_set_option(struct auxtrace_record *itr, !cpu_map__has(online_cpus, i)) continue;

• if (option & ETM_OPT_CTXTID) {
  

• 	err = cs_etm_set_context_id(itr, evsel, i);
  

• if (option & ETM_SET_OPT_PID) {
  

• 	err = cs_etm_set_pid(itr, evsel, i);
  if (err)
  	goto out;
  

  }

• if (option & ETM_OPT_TS) {
  

• if (option & ETM_SET_OPT_TS) {
  err = cs_etm_set_timestamp(itr, evsel, i);
  if (err)
  	goto out;
  

  }

• if (option & ~(ETM_OPT_CTXTID | ETM_OPT_TS))
  

• if (option & ~(ETM_SET_OPT_MASK))
  /* Nothing else is currently supported */
  goto out;
  

  }

@@ -406,7 +440,7 @@ static int cs_etm_recording_options(struct auxtrace_record *itr, evsel__set_sample_bit(cs_etm_evsel, CPU); err = cs_etm_set_option(itr, cs_etm_evsel,

• 			ETM_OPT_CTXTID | ETM_OPT_TS);
  

• 			ETM_SET_OPT_PID | ETM_SET_OPT_TS);
  

  if (err) goto out; }

@@ -485,7 +519,9 @@ static u64 cs_etmv4_get_config(struct auxtrace_record *itr) config |= BIT(ETM4_CFG_BIT_TS); if (config_opts & BIT(ETM_OPT_RETSTK)) config |= BIT(ETM4_CFG_BIT_RETSTK);

• if (config_opts & BIT(ETM_OPT_CTXTID_IN_VMID))

• config |= BIT(ETM4_CFG_BIT_VMID) |
  

• 	  BIT(ETM4_CFG_BIT_VMID_OPT);
  

  return config;

} -- 2.25.1

Hi Leo,

On Mon, 11 Jan 2021 at 15:06, Leo Yan leo.yan@linaro.org wrote:

Hi Mike,

On Mon, Jan 11, 2021 at 12:09:12PM +0000, Mike Leach wrote:

...

Hi Leo,

I think there is an issue here in that your modification assumes that all cpus in the system are of the same ETM type. The original routine allowed for differing ETM types, thus differing cpu ETM field lengths between ETMv4 / ETMv3, the field size was used after the relevant magic number for the cpu ETM was read.

You have replaced two different sizes - with a single calculated size.

Thanks for pointing out this.

...

Moving forwards we are seeing the newer FEAT_ETE protocol drivers appearing on the list, which will ultimately need a new metadata structure.

We have had discussions within ARM regarding the changing of the format to be more self describing - which should probably be opened out to the CS mailing list.

I think here have two options. One option is I think we can use __perf_cs_etmv3_magic/__perf_cs_etmv4_magic as indicator for the starting of next metadata array; when copy the metadata, always check the next item in the buffer, if it's __perf_cs_etmv3_magic or __perf_cs_etmv4_magic, will break loop and start copying metadata array for next CPU. The suggested change is pasted in below.

Another option is I drop patches 03,05/07 in the series and leave the backward compatibility fixing for a saperate patch series with self describing method. Especially, if you think the first option will introduce trouble for enabling self describing later, then I am happy to drop patches 03,05.

How about you think for this?

Thanks, Leo

---8<---

diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index a2a369e2fbb6..edaec57362f0 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -2558,12 +2558,19 @@ int cs_etm__process_auxtrace_info(union perf_event *event, err = -ENOMEM; goto err_free_metadata; }

•                   for (k = 0; k < CS_ETM_PRIV_MAX; k++)
  

•                   for (k = 0; k < CS_ETM_PRIV_MAX; k++) {
                            metadata[j][k] = ptr[i + k];
  
  

•                           if (ptr[i + k + 1] == __perf_cs_etmv3_magic ||
  

•                               ptr[i + k + 1] == __perf_cs_etmv4_magic) {
  

•                                   k++;
  

•                                   break;
  

•                           }
  

•                   }
  

•                   /* The traceID is our handle */
                    idx = metadata[j][CS_ETM_ETMTRACEIDR];
  

•                   i += CS_ETM_PRIV_MAX;
  

•                   i += k;
            } else if (ptr[i] == __perf_cs_etmv4_magic) {
                    metadata[j] = zalloc(sizeof(*metadata[j]) *
                                         CS_ETMV4_PRIV_MAX);
  

@@ -2571,12 +2578,19 @@ int cs_etm__process_auxtrace_info(union perf_event *event, err = -ENOMEM; goto err_free_metadata; }

•                   for (k = 0; k < CS_ETMV4_PRIV_MAX; k++)
  

•                   for (k = 0; k < CS_ETMV4_PRIV_MAX; k++) {
                            metadata[j][k] = ptr[i + k];
  
  

•                           if (ptr[i + k + 1] == __perf_cs_etmv3_magic ||
  

•                               ptr[i + k + 1] == __perf_cs_etmv4_magic) {
  

•                                   k++;
  

•                                   break;
  

•                           }
  

•                   }
  

•                   /* The traceID is our handle */
                    idx = metadata[j][CS_ETMV4_TRCTRACEIDR];
  

•                   i += CS_ETMV4_PRIV_MAX;
  

•                   i += k;
            }
  
            /* Get an RB node for this CPU */
  

That would be a spot fix for the read /copy case, but will not fix the print routine which will still bail out on older versions of the format. (when using perf report --dump).

The "self describing" format I have been looking at will add an NR_PARAMS value to the common block in the CPU metadata parameter list, increment the header version to '1' and update the format writer to use the version 1 format while having the reader understand both v0 and v1 formats.

i..e in cs-etm.h perf I add: /* * Update the version for new format. * * New version 1 format adds a param count to the per cpu metadata. * This allows easy adding of new metadata parameters. * Requires that new params always added after current ones. * Also allows client reader to handle file versions that are different by * checking the number of params in the file vs the number expected. */ #define CS_HEADER_CURRENT_VERSION 1

/* Beginning of header common to both ETMv3 and V4 */ enum { CS_ETM_MAGIC, CS_ETM_CPU, CS_ETM_NR_PARAMS, /* number of parameters to follow in this block */ };

where in verison 1, NR_PARAMS indicates the total number of params that follow - so adding new parameters can be added to the metadata enums and the tool will automatically adjust, and will handle v0 files, plus older and newer files that have differing numbers of parameters, as long as the parameters are only ever added to the end of the list.

I have been working on a patch for this today, which took a little longer than expected as it was a little more complex than expected (the printing routines in for the --dump command!).

I will post this tomorrow when tested - and if we agree it works it could be rolled into your set - it would make adding the PID parameter easier, and ensure that this new format is available for the upcoming developments.

Regards

Mike

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

On Mon, Jan 11, 2021 at 09:45:12AM +0000, Suzuki Kuruppassery Poulose wrote:

Hi Leo,

On 1/9/21 7:44 AM, Leo Yan wrote:

...

It's possible for CoreSight to trace PID in either CONTEXTIDR_EL1 or CONTEXTIDR_EL2, the PID format info is used to distinguish the PID is traced in which register.

This patch saves PID format into the metadata when record.

The patch looks good to me. One minor suggestion below

...

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

tools/perf/arch/arm/util/cs-etm.c | 21 +++++++++++++++++++++ tools/perf/util/cs-etm.c | 2 ++ tools/perf/util/cs-etm.h | 2 ++ 3 files changed, 25 insertions(+)

diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c index fad7b6e13ccc..ee78df3b1b07 100644 --- a/tools/perf/arch/arm/util/cs-etm.c +++ b/tools/perf/arch/arm/util/cs-etm.c @@ -613,6 +613,7 @@ static void cs_etm_get_metadata(int cpu, u32 *offset, struct cs_etm_recording *ptr = container_of(itr, struct cs_etm_recording, itr); struct perf_pmu *cs_etm_pmu = ptr->cs_etm_pmu;

• u64 pid_fmt; /* first see what kind of tracer this cpu is affined to */ if (cs_etm_is_etmv4(itr, cpu)) {

@@ -641,6 +642,16 @@ static void cs_etm_get_metadata(int cpu, u32 *offset, metadata_etmv4_ro [CS_ETMV4_TRCAUTHSTATUS]);

• /*
  

•  * The PID format will be used when decode the trace data;
  

•  * based on it the decoder will make decision for setting
  

•  * sample's PID as context_id or VMID.
  

•  */
  

• pid_fmt = perf_pmu__format_bits(&cs_etm_pmu->format, "pid");
  

• if (!pid_fmt)
  

• 	pid_fmt = 1ULL << ETM_OPT_CTXTID;
  

• info->priv[*offset + CS_ETMV4_PID_FMT] = pid_fmt;
  

Given we do this same step twice here in this function and also in patch 2. I am wondering if this could be made into a small helper function ?

static u64 cs_etm_pmu_format_pid(cs_etm_pm) { pid_fmt = perf_pmu__format_bits(&cs_etm_pmu->format, "pid"); /* * An older kernel doesn't expose this, so fall back to using * CTXTID. */ if (!pid_fmt) pid_fmt = 1ULL << ETM_OPT_CTXTID; return pid_fmt; }

Agreed; will follow up this suggestion.

Thanks, Leo

On 1/9/21 7:44 AM, Leo Yan wrote:

If the metadata item CS_ETM_PID_FMT/CS_ETMV4_PID_FMT is zero, this means the perf data file is recorded with old version tool and the tool has not extended to support the item.

For this case, this patch fixes up PID_FMT entry to set the value as BIT(ETM_OPT_CTXTID), this info will be delivered to the decoder to extract PID from packet's field "context_id".

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

Acked-by: Suzuki K Poulose suzuki.poulose@arm.com

---

tools/perf/util/cs-etm.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)

diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index 763085db29ae..8c125134a756 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> @@ -2577,6 +2578,15 @@ int cs_etm__process_auxtrace_info(union perf_event *event, for (k = 0; k < metadata_cpu_array_size; k++) metadata[j][k] = ptr[i + k];

• 	/*
  

• 	 * If the data in CS_ETM_PID_FMT is zero, means the
  

• 	 * information isn't stored in the data file, this is
  

• 	 * because the old perf tool hasn't yet supported
  

• 	 * CS_ETM_PID_FMT.  Fixup the item to option "CTXTID".
  

• 	 */
  

• 	if (!metadata[j][CS_ETM_PID_FMT])
  

• 		metadata[j][CS_ETM_PID_FMT] = BIT(ETM_OPT_CTXTID);
  

• /* The traceID is our handle */
  idx = metadata[j][CS_ETM_ETMTRACEIDR];
  i += metadata_cpu_array_size;
  

@@ -2590,6 +2600,15 @@ int cs_etm__process_auxtrace_info(union perf_event *event, for (k = 0; k < metadata_cpu_array_size; k++) metadata[j][k] = ptr[i + k];

• 	/*
  

• 	 * If the data in CS_ETMV4_PID_FMT is zero, means the
  

• 	 * information isn't stored in the data file, this is
  

• 	 * because the old perf tool hasn't yet supported
  

• 	 * CS_ETMV4_PID_FMT.  Fixup the item to option "CTXTID".
  

• 	 */
  

• 	if (!metadata[j][CS_ETMV4_PID_FMT])
  

• 		metadata[j][CS_ETMV4_PID_FMT] = BIT(ETM_OPT_CTXTID);
  

• /* The traceID is our handle */
  idx = metadata[j][CS_ETMV4_TRCTRACEIDR];
  i += metadata_cpu_array_size;
  
  

On 1/9/21 7:44 AM, Leo Yan wrote:

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

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

Acked-by: Suzuki K Poulose suzuki.poulose@arm.com

---

tools/perf/util/cs-etm.c | 18 ++++++++++++++++++ tools/perf/util/cs-etm.h | 1 + 2 files changed, 19 insertions(+)

diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index 8c125134a756..6705d39c8cee 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -157,6 +157,24 @@ int cs_etm__get_cpu(u8 trace_chan_id, int *cpu) return 0; } +int cs_etm__get_pid_fmt(u8 trace_chan_id, u64 *pid_fmt) +{

• struct int_node *inode;
• u64 *metadata;
• inode = intlist__find(traceid_list, trace_chan_id);
• if (!inode)

• return -EINVAL;
  

• metadata = inode->priv;
• if (metadata[CS_ETM_MAGIC] == __perf_cs_etmv3_magic)

• *pid_fmt = metadata[CS_ETM_PID_FMT];
  

• else

• *pid_fmt = metadata[CS_ETMV4_PID_FMT];
  

• 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 8cbbea6100a1..98801040175f 100644 --- a/tools/perf/util/cs-etm.h +++ b/tools/perf/util/cs-etm.h @@ -175,6 +175,7 @@ struct cs_etm_packet_queue { int cs_etm__process_auxtrace_info(union perf_event *event, struct perf_session *session); int cs_etm__get_cpu(u8 trace_chan_id, int *cpu); +int cs_etm__get_pid_fmt(u8 trace_chan_id, u64 *pid_fmt); int cs_etm__etmq_set_tid(struct cs_etm_queue *etmq, pid_t tid, u8 trace_chan_id); bool cs_etm__etmq_is_timeless(struct cs_etm_queue *etmq);

Hi Leo

On 1/9/21 7:44 AM, 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 context_id is invalid but we have a valid VMID.

Thank you again for cleaning up this ! Please see one comment below.

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 cd007cc9c283..9e81169dfa76 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> @@ -500,13 +501,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;
  

The patch looks fine to me. I am wondering if this can be cached somewhere in the etmq to avoid doing the search everytime we hit a CID ? Surely for a session, this woudn't change and thus for the decoder life time.

Cheers Suzuki