| Message ID | 20210202163842.134734-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 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 >
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 > > >
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
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(-)