| Message ID | 20210109074435.626855-2-leo.yan@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | coresight: etm-perf: Fix pid tracing with VHE | expand |
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 >
On Mon, Jan 11, 2021 at 04:22:39PM +0000, Mike Leach wrote: > 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? Will fix it. Thanks for pointing out. > 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 > > > > > -- > Mike Leach > Principal Engineer, ARM Ltd. > Manchester Design Centre. UK
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/tools/perf/arch/arm/util/cs-etm.c?h=v5.11-rc3#n152 [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hwtracing/coresight/coresight-etm4x-core.c?h=v5.11-rc3#n422
On 1/12/21 8:58 AM, Leo Yan 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. > Exactly. I believe this problematic. We are stuckwith using the ETM3.x/PTM config bits for the CS_ETM pmu, which is a bit wierd and the allocation of the config bits are sparse. The problem with changing them now, will break older perf tools decoding a perf.data from a newer kernel. I believe we are stuck with this. I would recommend simply updating the comment to reflect that, thats the generic CS PMU ABI for configuration which was initially based on ETM3.x. Suzuki
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/tools/perf/arch/arm/util/cs-etm.c?h=v5.11-rc3#n152 > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hwtracing/coresight/coresight-etm4x-core.c?h=v5.11-rc3#n422 -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK
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). 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
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
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. > + > 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 > +#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 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
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 +#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) {