RE: [PATCH] coresight: etm: Make cycle count threshold user configurable

[Al Grant]

> -----Original Message-----
> From: James Clark <james.clark@xxxxxxx>
> Sent: Friday, August 4, 2023 10:23 AM
> To: Anshuman Khandual <Anshuman.Khandual@xxxxxxx>; Al Grant
> <Al.Grant@xxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Cc: Mike Leach <mike.leach@xxxxxxxxxx>; coresight@xxxxxxxxxxxxxxxx; linux-
> doc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] coresight: etm: Make cycle count threshold user
> configurable
> 
> 
> 
> On 04/08/2023 09:45, Anshuman Khandual wrote:
> >
> >
> > On 8/4/23 13:34, Al Grant wrote:
> >>
> >>
> >>> -----Original Message-----
> >>> From: Anshuman Khandual <anshuman.khandual@xxxxxxx>
> >>> Sent: Friday, August 4, 2023 5:47 AM
> >>> To: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> >>> Cc: Anshuman Khandual <Anshuman.Khandual@xxxxxxx>; Mike Leach
> >>> <mike.leach@xxxxxxxxxx>; coresight@xxxxxxxxxxxxxxxx;
> >>> linux-doc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> >>> Subject: [PATCH] coresight: etm: Make cycle count threshold user
> >>> configurable
> >>>
> >>> Cycle counting is enabled, when requested and supported but with a
> >>> default threshold value ETM_CYC_THRESHOLD_DEFAULT i.e 0x100 getting
> >>> into TRCCCCTLR, representing the minimum interval between cycle
> >>> count trace packets.
> >>>
> >>> This makes cycle threshold user configurable, from the user space
> >>> via perf event attributes. Although it falls back using
> >>> ETM_CYC_THRESHOLD_DEFAULT, in case no explicit request. As expected it
> creates a sysfs file as well.
> >>>
> >>> /sys/bus/event_source/devices/cs_etm/format/cc_threshold
> >>>
> >>> New 'cc_threshold' uses 'event->attr.config3' as no more space is
> >>> available in 'event->attr.config1' or 'event->attr.config2'.
> >>>
> >>> Cc: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> >>> Cc: Mike Leach <mike.leach@xxxxxxxxxx>
> >>> Cc: James Clark <james.clark@xxxxxxx>
> >>> Cc: Leo Yan <leo.yan@xxxxxxxxxx>
> >>> Cc: coresight@xxxxxxxxxxxxxxxx
> >>> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> >>> Cc: linux-doc@xxxxxxxxxxxxxxx
> >>> Cc: linux-kernel@xxxxxxxxxxxxxxx
> >>> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
> >>> ---
> >>>  Documentation/trace/coresight/coresight.rst        |  2 ++
> >>>  drivers/hwtracing/coresight/coresight-etm-perf.c   |  2 ++
> >>>  drivers/hwtracing/coresight/coresight-etm4x-core.c | 12
> >>> ++++++++++--
> >>>  3 files changed, 14 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/Documentation/trace/coresight/coresight.rst
> >>> b/Documentation/trace/coresight/coresight.rst
> >>> index 4a71ea6cb390..b88d83b59531 100644
> >>> --- a/Documentation/trace/coresight/coresight.rst
> >>> +++ b/Documentation/trace/coresight/coresight.rst
> >>> @@ -624,6 +624,8 @@ They are also listed in the folder
> >>> /sys/bus/event_source/devices/cs_etm/format/
> >>>     * - timestamp
> >>>       - Session local version of the system wide setting:
> >>> :ref:`ETMv4_MODE_TIMESTAMP
> >>>         <coresight-timestamp>`
> >>> +   * - cc_treshold
> >>
> >> Spelling: cc_threshold
> >
> > Will fix this, besides does it require some more description for this
> > new config option i.e cc_threshold ?
> >
> >>
> >>> +     - Cycle count treshhold value
> >>>
> >>>  How to use the STM module
> >>>  -------------------------
> >>> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c
> >>> b/drivers/hwtracing/coresight/coresight-etm-perf.c
> >>> index 5ca6278baff4..09f75dffae60 100644
> >>> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> >>> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> >>> @@ -68,6 +68,7 @@ PMU_FORMAT_ATTR(preset,		"config:0-3");
> >>>  PMU_FORMAT_ATTR(sinkid,		"config2:0-31");
> >>>  /* config ID - set if a system configuration is selected */
> >>>  PMU_FORMAT_ATTR(configid,	"config2:32-63");
> >>> +PMU_FORMAT_ATTR(cc_threshold,	"config3:0-11");
> >>>
> >>>
> >>>  /*
> >>> @@ -101,6 +102,7 @@ static struct attribute *etm_config_formats_attr[] = {
> >>>  	&format_attr_preset.attr,
> >>>  	&format_attr_configid.attr,
> >>>  	&format_attr_branch_broadcast.attr,
> >>> +	&format_attr_cc_threshold.attr,
> >>>  	NULL,
> >>>  };
> >>>
> >>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> >>> b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> >>> index 9d186af81ea0..9a2766f68416 100644
> >>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> >>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> >>> @@ -644,7 +644,7 @@ static int etm4_parse_event_config(struct
> >>> coresight_device *csdev,
> >>>  	struct etmv4_config *config = &drvdata->config;
> >>>  	struct perf_event_attr *attr = &event->attr;
> >>>  	unsigned long cfg_hash;
> >>> -	int preset;
> >>> +	int preset, cc_threshold;
> >>>
> >>>  	/* Clear configuration from previous run */
> >>>  	memset(config, 0, sizeof(struct etmv4_config)); @@ -667,7 +667,15
> >>> @@ static int etm4_parse_event_config(struct coresight_device *csdev,
> >>>  	if (attr->config & BIT(ETM_OPT_CYCACC)) {
> >>>  		config->cfg |= TRCCONFIGR_CCI;
> >>>  		/* TRM: Must program this for cycacc to work */
> >>> -		config->ccctlr = ETM_CYC_THRESHOLD_DEFAULT;
> >>> +		cc_treshold = attr->config3 & ETM_CYC_THRESHOLD_MASK;
> >>
> >> Spelling again
> >
> > Yikes, this does not even build. Seems like I had missed the
> > applicable config i.e CONFIG_CORESIGHT_SOURCE_ETM4X this time around.
> Apologies.
> >
> >>
> >>> +		if (cc_treshold) {
> >>> +			if (cc_treshold < drvdata->ccitmin)
> >>> +				config->ccctlr = drvdata->ccitmin;
> >>> +			else
> >>> +				config->ccctlr = cc_threshold;
> >>> +		} else {
> >>> +			config->ccctlr = ETM_CYC_THRESHOLD_DEFAULT;
> >>> +		}
> >>
> >> Consider dropping the check against CCITMIN. There are CPUs where
> >> CCITMIN is incorrect, e.g. see published errata 1490853 where the
> >> value 0x100 should be 0b100 i.e. 4. On these ETMs it is possible to
> >> set the timing threshold to four cycles instead of 256 cycles,
> >> providing much better timing resolution. The kernel currently does
> >> not work around this errata and uses the incorrect value of ccitmin.
> >> If you drop the check, and trust the value provided by userspace, you
> >> allow userspace to work around it.
> > Why ? We could just work around the errata #1490853 while initializing
> > the drvdata->ccitmin if that is where the problem exists.
> 
> 
> > I dont think
> > user space should be required to know about the erratas, and provide a
> 
> I think that becomes less true for the tracing and PMU stuff. If you are using it you
> likely need to know a lot about the platform you are working on anyway.
> 
> For example right now I'm trying to upstream some metric formulas which have a
> workaround where userspace needs to know the variant of the processor. It's not
> possible for the kernel to do anything about it.
> 
> In this case as it's only one known errata we could add the workaround.
> 
> Unless we expect there to be the same issue again in the future? Or we know
> there are already more CPUs than #1490853 mentions?

It's a common-mode failure affecting several CPUs, each with their
own set of affected/fixed versions, so there would be several MIDRs
to check. The ones that have been published in errata notices include:

- #1490853: Neoverse N1, fixed r4p1
- #1490853: Cortex-A76, fixed r4p1 (n.b. same number as above)
- #1619801: Neoverse V1, fixed r1p0
- #1502854: Cortex-X1, fixed r1p0
- #1491015: Cortex-A77, fixed r1p1

Hopefully that's the lot.

For this issue you don't really need to check the fix version, as the
number you'd put in ccitmin is the same anyway.

Al


> 
> > right value instead.
> > _______________________________________________
> > CoreSight mailing list -- coresight@xxxxxxxxxxxxxxxx To unsubscribe
> > send an email to coresight-leave@xxxxxxxxxxxxxxxx