| Message ID | 20230804044720.1478900-1-anshuman.khandual@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | coresight: etm: Make cycle count threshold user configurable | expand |
> -----Original Message----- > From: Anshuman Khandual <anshuman.khandual@arm.com> > Sent: Friday, August 4, 2023 5:47 AM > To: linux-arm-kernel@lists.infradead.org > Cc: Anshuman Khandual <Anshuman.Khandual@arm.com>; Mike Leach > <mike.leach@linaro.org>; coresight@lists.linaro.org; linux-doc@vger.kernel.org; > linux-kernel@vger.kernel.org > 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@arm.com> > Cc: Mike Leach <mike.leach@linaro.org> > Cc: James Clark <james.clark@arm.com> > Cc: Leo Yan <leo.yan@linaro.org> > Cc: coresight@lists.linaro.org > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-doc@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> > --- > 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 > + - 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 > + 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. Al > } > if (attr->config & BIT(ETM_OPT_TS)) { > /* > -- > 2.25.1 > > _______________________________________________ > CoreSight mailing list -- coresight@lists.linaro.org To unsubscribe send an email to > coresight-leave@lists.linaro.org
On 8/4/23 13:34, Al Grant wrote: > > >> -----Original Message----- >> From: Anshuman Khandual <anshuman.khandual@arm.com> >> Sent: Friday, August 4, 2023 5:47 AM >> To: linux-arm-kernel@lists.infradead.org >> Cc: Anshuman Khandual <Anshuman.Khandual@arm.com>; Mike Leach >> <mike.leach@linaro.org>; coresight@lists.linaro.org; linux-doc@vger.kernel.org; >> linux-kernel@vger.kernel.org >> 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@arm.com> >> Cc: Mike Leach <mike.leach@linaro.org> >> Cc: James Clark <james.clark@arm.com> >> Cc: Leo Yan <leo.yan@linaro.org> >> Cc: coresight@lists.linaro.org >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: linux-doc@vger.kernel.org >> Cc: linux-kernel@vger.kernel.org >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >> --- >> 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 right value instead.
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@arm.com> >>> Sent: Friday, August 4, 2023 5:47 AM >>> To: linux-arm-kernel@lists.infradead.org >>> Cc: Anshuman Khandual <Anshuman.Khandual@arm.com>; Mike Leach >>> <mike.leach@linaro.org>; coresight@lists.linaro.org; linux-doc@vger.kernel.org; >>> linux-kernel@vger.kernel.org >>> 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@arm.com> >>> Cc: Mike Leach <mike.leach@linaro.org> >>> Cc: James Clark <james.clark@arm.com> >>> Cc: Leo Yan <leo.yan@linaro.org> >>> Cc: coresight@lists.linaro.org >>> Cc: linux-arm-kernel@lists.infradead.org >>> Cc: linux-doc@vger.kernel.org >>> Cc: linux-kernel@vger.kernel.org >>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >>> --- >>> 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? > right value instead. > _______________________________________________ > CoreSight mailing list -- coresight@lists.linaro.org > To unsubscribe send an email to coresight-leave@lists.linaro.org
> -----Original Message----- > From: James Clark <james.clark@arm.com> > Sent: Friday, August 4, 2023 10:23 AM > To: Anshuman Khandual <Anshuman.Khandual@arm.com>; Al Grant > <Al.Grant@arm.com>; linux-arm-kernel@lists.infradead.org > Cc: Mike Leach <mike.leach@linaro.org>; coresight@lists.linaro.org; linux- > doc@vger.kernel.org; linux-kernel@vger.kernel.org > 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@arm.com> > >>> Sent: Friday, August 4, 2023 5:47 AM > >>> To: linux-arm-kernel@lists.infradead.org > >>> Cc: Anshuman Khandual <Anshuman.Khandual@arm.com>; Mike Leach > >>> <mike.leach@linaro.org>; coresight@lists.linaro.org; > >>> linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org > >>> 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@arm.com> > >>> Cc: Mike Leach <mike.leach@linaro.org> > >>> Cc: James Clark <james.clark@arm.com> > >>> Cc: Leo Yan <leo.yan@linaro.org> > >>> Cc: coresight@lists.linaro.org > >>> Cc: linux-arm-kernel@lists.infradead.org > >>> Cc: linux-doc@vger.kernel.org > >>> Cc: linux-kernel@vger.kernel.org > >>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> > >>> --- > >>> 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@lists.linaro.org To unsubscribe > > send an email to coresight-leave@lists.linaro.org
On 04/08/2023 11:10, Al Grant wrote: > > >> -----Original Message----- >> From: James Clark <james.clark@arm.com> >> Sent: Friday, August 4, 2023 10:23 AM >> To: Anshuman Khandual <Anshuman.Khandual@arm.com>; Al Grant >> <Al.Grant@arm.com>; linux-arm-kernel@lists.infradead.org >> Cc: Mike Leach <mike.leach@linaro.org>; coresight@lists.linaro.org; linux- >> doc@vger.kernel.org; linux-kernel@vger.kernel.org >> 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@arm.com> >>>>> Sent: Friday, August 4, 2023 5:47 AM >>>>> To: linux-arm-kernel@lists.infradead.org >>>>> Cc: Anshuman Khandual <Anshuman.Khandual@arm.com>; Mike Leach >>>>> <mike.leach@linaro.org>; coresight@lists.linaro.org; >>>>> linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org >>>>> 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@arm.com> >>>>> Cc: Mike Leach <mike.leach@linaro.org> >>>>> Cc: James Clark <james.clark@arm.com> >>>>> Cc: Leo Yan <leo.yan@linaro.org> >>>>> Cc: coresight@lists.linaro.org >>>>> Cc: linux-arm-kernel@lists.infradead.org >>>>> Cc: linux-doc@vger.kernel.org >>>>> Cc: linux-kernel@vger.kernel.org >>>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >>>>> --- >>>>> 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 > It could make sense to not clamp it then. Should we at least expose CCITMIN in sysfs so userspace can attempt to put the right value if it's not trying to work around something? If it's not already exposed. > >> >>> right value instead. >>> _______________________________________________ >>> CoreSight mailing list -- coresight@lists.linaro.org To unsubscribe >>> send an email to coresight-leave@lists.linaro.org
Hi Anshuman, kernel test robot noticed the following build errors: [auto build test ERROR on soc/for-next] [also build test ERROR on linus/master v6.5-rc4 next-20230804] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Anshuman-Khandual/coresight-etm-Make-cycle-count-threshold-user-configurable/20230804-124850 base: https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git for-next patch link: https://lore.kernel.org/r/20230804044720.1478900-1-anshuman.khandual%40arm.com patch subject: [PATCH] coresight: etm: Make cycle count threshold user configurable config: arm64-randconfig-r004-20230731 (https://download.01.org/0day-ci/archive/20230805/202308051014.Uzl7XY32-lkp@intel.com/config) compiler: aarch64-linux-gcc (GCC) 12.3.0 reproduce: (https://download.01.org/0day-ci/archive/20230805/202308051014.Uzl7XY32-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202308051014.Uzl7XY32-lkp@intel.com/ All errors (new ones prefixed by >>): drivers/hwtracing/coresight/coresight-etm4x-core.c: In function 'etm4_parse_event_config': >> drivers/hwtracing/coresight/coresight-etm4x-core.c:661:17: error: 'cc_treshold' undeclared (first use in this function); did you mean 'cc_threshold'? 661 | cc_treshold = attr->config3 & ETM_CYC_THRESHOLD_MASK; | ^~~~~~~~~~~ | cc_threshold drivers/hwtracing/coresight/coresight-etm4x-core.c:661:17: note: each undeclared identifier is reported only once for each function it appears in vim +661 drivers/hwtracing/coresight/coresight-etm4x-core.c 629 630 static int etm4_parse_event_config(struct coresight_device *csdev, 631 struct perf_event *event) 632 { 633 int ret = 0; 634 struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); 635 struct etmv4_config *config = &drvdata->config; 636 struct perf_event_attr *attr = &event->attr; 637 unsigned long cfg_hash; 638 int preset, cc_threshold; 639 640 /* Clear configuration from previous run */ 641 memset(config, 0, sizeof(struct etmv4_config)); 642 643 if (attr->exclude_kernel) 644 config->mode = ETM_MODE_EXCL_KERN; 645 646 if (attr->exclude_user) 647 config->mode = ETM_MODE_EXCL_USER; 648 649 /* Always start from the default config */ 650 etm4_set_default_config(config); 651 652 /* Configure filters specified on the perf cmd line, if any. */ 653 ret = etm4_set_event_filters(drvdata, event); 654 if (ret) 655 goto out; 656 657 /* Go from generic option to ETMv4 specifics */ 658 if (attr->config & BIT(ETM_OPT_CYCACC)) { 659 config->cfg |= TRCCONFIGR_CCI; 660 /* TRM: Must program this for cycacc to work */ > 661 cc_treshold = attr->config3 & ETM_CYC_THRESHOLD_MASK; 662 if (cc_treshold) { 663 if (cc_treshold < drvdata->ccitmin) 664 config->ccctlr = drvdata->ccitmin; 665 else 666 config->ccctlr = cc_threshold; 667 } else { 668 config->ccctlr = ETM_CYC_THRESHOLD_DEFAULT; 669 } 670 } 671 if (attr->config & BIT(ETM_OPT_TS)) { 672 /* 673 * Configure timestamps to be emitted at regular intervals in 674 * order to correlate instructions executed on different CPUs 675 * (CPU-wide trace scenarios). 676 */ 677 ret = etm4_config_timestamp_event(drvdata); 678 679 /* 680 * No need to go further if timestamp intervals can't 681 * be configured. 682 */ 683 if (ret) 684 goto out; 685 686 /* bit[11], Global timestamp tracing bit */ 687 config->cfg |= TRCCONFIGR_TS; 688 } 689 690 /* Only trace contextID when runs in root PID namespace */ 691 if ((attr->config & BIT(ETM_OPT_CTXTID)) && 692 task_is_in_init_pid_ns(current)) 693 /* bit[6], Context ID tracing bit */ 694 config->cfg |= TRCCONFIGR_CID; 695 696 /* 697 * If set bit ETM_OPT_CTXTID2 in perf config, this asks to trace VMID 698 * for recording CONTEXTIDR_EL2. Do not enable VMID tracing if the 699 * kernel is not running in EL2. 700 */ 701 if (attr->config & BIT(ETM_OPT_CTXTID2)) { 702 if (!is_kernel_in_hyp_mode()) { 703 ret = -EINVAL; 704 goto out; 705 } 706 /* Only trace virtual contextID when runs in root PID namespace */ 707 if (task_is_in_init_pid_ns(current)) 708 config->cfg |= TRCCONFIGR_VMID | TRCCONFIGR_VMIDOPT; 709 } 710 711 /* return stack - enable if selected and supported */ 712 if ((attr->config & BIT(ETM_OPT_RETSTK)) && drvdata->retstack) 713 /* bit[12], Return stack enable bit */ 714 config->cfg |= TRCCONFIGR_RS; 715 716 /* 717 * Set any selected configuration and preset. 718 * 719 * This extracts the values of PMU_FORMAT_ATTR(configid) and PMU_FORMAT_ATTR(preset) 720 * in the perf attributes defined in coresight-etm-perf.c. 721 * configid uses bits 63:32 of attr->config2, preset uses bits 3:0 of attr->config. 722 * A zero configid means no configuration active, preset = 0 means no preset selected. 723 */ 724 if (attr->config2 & GENMASK_ULL(63, 32)) { 725 cfg_hash = (u32)(attr->config2 >> 32); 726 preset = attr->config & 0xF; 727 ret = cscfg_csdev_enable_active_config(csdev, cfg_hash, preset); 728 } 729 730 /* branch broadcast - enable if selected and supported */ 731 if (attr->config & BIT(ETM_OPT_BRANCH_BROADCAST)) { 732 if (!drvdata->trcbb) { 733 /* 734 * Missing BB support could cause silent decode errors 735 * so fail to open if it's not supported. 736 */ 737 ret = -EINVAL; 738 goto out; 739 } else { 740 config->cfg |= BIT(ETM4_CFG_BIT_BB); 741 } 742 } 743 744 out: 745 return ret; 746 } 747
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 + - 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; + if (cc_treshold) { + if (cc_treshold < drvdata->ccitmin) + config->ccctlr = drvdata->ccitmin; + else + config->ccctlr = cc_threshold; + } else { + config->ccctlr = ETM_CYC_THRESHOLD_DEFAULT; + } } if (attr->config & BIT(ETM_OPT_TS)) { /*
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@arm.com> Cc: Mike Leach <mike.leach@linaro.org> Cc: James Clark <james.clark@arm.com> Cc: Leo Yan <leo.yan@linaro.org> Cc: coresight@lists.linaro.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-doc@vger.kernel.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> --- 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(-)