| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <bfce6b68-783f-4aa1-b9db-d905230be609@linaro.org>
Date: Wed, 15 Oct 2025 16:19:04 +0100
From: James Clark <james.clark@...aro.org>
To: Mike Leach <mike.leach@...aro.org>,
Suzuki K Poulose <suzuki.poulose@....com>, Leo Yan <leo.yan@....com>
Cc: Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Jonathan Corbet <corbet@....net>, coresight@...ts.linaro.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-doc@...r.kernel.org
Subject: Re: [PATCH v3 4/5] coresight: Add format attribute for setting the
timestamp interval
On 09/10/2025 4:50 pm, Mike Leach wrote:
> Hi James
>
> On Thu, 2 Oct 2025 at 11:10, James Clark <james.clark@...aro.org> wrote:
>>
>> Timestamps are currently emitted at the maximum rate possible, which is
>> much too frequent for most use cases. Add an attribute to be able to set
>> the interval. Granular control is not required, so save space in the
>> config by interpreting it as 2 ^ ts_interval. And then 4 bits (0 - 15) is
>> enough to set the interval to be larger than the existing SYNC timestamp
>> interval.
>>
>> No sysfs file is needed for this attribute because counter generated
>> timestamps are only configured for Perf mode.
>>
>> Only show this attribute for ETM4x because timestamps aren't configured
>> in the same way for ETM3x. The attribute is only ever read in
>> coresight-etm4x-core.c.
>>
>> Reviewed-by: Leo Yan <leo.yan@....com>
>> Tested-by: Leo Yan <leo.yan@....com>
>> Signed-off-by: James Clark <james.clark@...aro.org>
>> ---
>> drivers/hwtracing/coresight/coresight-etm-perf.c | 16 +++++++++++++++-
>> drivers/hwtracing/coresight/coresight-etm-perf.h | 7 +++++++
>> drivers/hwtracing/coresight/coresight-etm4x-core.c | 21 ++++++++++++---------
>> 3 files changed, 34 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
>> index f677c08233ba..0c1b990fc56e 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
>> @@ -13,6 +13,7 @@
>> #include <linux/mm.h>
>> #include <linux/init.h>
>> #include <linux/perf_event.h>
>> +#include <linux/perf/arm_pmu.h>
>> #include <linux/percpu-defs.h>
>> #include <linux/slab.h>
>> #include <linux/stringhash.h>
>> @@ -69,7 +70,8 @@ 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");
>> -
>> +/* Interval = (2 ^ ts_level) */
>> +GEN_PMU_FORMAT_ATTR(ts_level);
>>
>> /*
>> * contextid always traces the "PID". The PID is in CONTEXTIDR_EL1
>> @@ -103,11 +105,23 @@ static struct attribute *etm_config_formats_attr[] = {
>> &format_attr_configid.attr,
>> &format_attr_branch_broadcast.attr,
>> &format_attr_cc_threshold.attr,
>> + &format_attr_ts_level.attr,
>> NULL,
>> };
>>
>> +static umode_t etm_format_attr_is_visible(struct kobject *kobj,
>> + struct attribute *attr, int unused)
>> +{
>> + if (attr == &format_attr_ts_level.attr &&
>> + !IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X))
>> + return 0;
>> +
>> + return attr->mode;
>> +}
>> +
>> static const struct attribute_group etm_pmu_format_group = {
>> .name = "format",
>> + .is_visible = etm_format_attr_is_visible,
>> .attrs = etm_config_formats_attr,
>> };
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.h b/drivers/hwtracing/coresight/coresight-etm-perf.h
>> index 5febbcdb8696..d2664ffb33e5 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm-perf.h
>> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.h
>> @@ -7,6 +7,7 @@
>> #ifndef _CORESIGHT_ETM_PERF_H
>> #define _CORESIGHT_ETM_PERF_H
>>
>> +#include <linux/bits.h>
>> #include <linux/percpu-defs.h>
>> #include "coresight-priv.h"
>>
>> @@ -20,6 +21,12 @@ struct cscfg_config_desc;
>> */
>> #define ETM_ADDR_CMP_MAX 8
>>
>> +#define ATTR_CFG_FLD_ts_level_CFG config3
>> +#define ATTR_CFG_FLD_ts_level_LO 12
>> +#define ATTR_CFG_FLD_ts_level_HI 15
>> +#define ATTR_CFG_FLD_ts_level_MASK GENMASK(ATTR_CFG_FLD_ts_level_HI, \
>> + ATTR_CFG_FLD_ts_level_LO)
>> +
>> /**
>> * struct etm_filter - single instruction range or start/stop configuration.
>> * @start_addr: The address to start tracing on.
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> index 920d092ef862..034844f52bb2 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> @@ -28,6 +28,7 @@
>> #include <linux/amba/bus.h>
>> #include <linux/seq_file.h>
>> #include <linux/uaccess.h>
>> +#include <linux/perf/arm_pmu.h>
>> #include <linux/perf_event.h>
>> #include <linux/platform_device.h>
>> #include <linux/pm_runtime.h>
>> @@ -616,7 +617,7 @@ static void etm4_enable_hw_smp_call(void *info)
>> * +--------------+
>> * |
>> * +------v-------+
>> - * | Counter x | (reload to 1 on underflow)
>> + * | Counter x | (reload to 2 ^ ts_level on underflow)
>> * +--------------+
>> * |
>> * +------v--------------+
>> @@ -627,11 +628,17 @@ static void etm4_enable_hw_smp_call(void *info)
>> * | Timestamp Generator | (timestamp on resource y)
>> * +----------------------+
>> */
>> -static int etm4_config_timestamp_event(struct etmv4_drvdata *drvdata)
>> +static int etm4_config_timestamp_event(struct etmv4_drvdata *drvdata,
>> + struct perf_event_attr *attr)
>> {
>> int ctridx;
>> int rselector;
>> struct etmv4_config *config = &drvdata->config;
>> + u8 ts_level = ATTR_CFG_GET_FLD(attr, ts_level);
>> +
>> + /* Disable when ts_level == MAX */
>> + if (ts_level == FIELD_GET(ATTR_CFG_FLD_ts_level_MASK, UINT_MAX))
>> + return 0;
>>
>
> Returning 0 from this function _enables_ the timestamps
>
Returning 0 just means that etm4_parse_event_config() doesn't exit with
an error. For ts_level == MAX we want to disable timestamps generated by
the counter, but we still want the minimum periodic timestamps.
To disable all timestamps you'd need to have attr->config &
BIT(ETM_OPT_TS) == false. This is set by the "timestamp" format flag
which I tried to explain that in the docs change.
I could also change the comment to say "/* Disable counter generated
timestamps with ts_level == MAX */"
It's unfortunate that there are now two format options for timestamps.
Maybe instead of adding a second option we can change "timestamp" from a
1 bit field to 4 bits, with the following meanings:
0: No counter timestamps or SYNC timestamps
1-14: Counter timestamps = 2 ^ x. Plus SYNC timestamps
15: Only SYNC timestamps
Now we basically have the same meanings except you also have to set the
timestamp bit. Seems a bit pointless.
Previous versions of Perf were hard coding the timestamp format bit
rather than reading it out of
"/sys/bus/event_source/devices/cs_etm/format/timestamp" though:
- /* All good, let the kernel know */
- evsel->core.attr.config |= (1 << ETM_OPT_TS);
For that reason we'd have to leave that one where it is for backwards
compatibility. If it's set it would be equivalent to the new wider
timestamp field == 1.
I don't know if there's any precedent for changing the bitfield that
backs a format field, but presumably that's the point of publishing them
in files rather than a header.
>> /* No point in trying if we don't have at least one counter */
>> if (!drvdata->nr_cntr)
>> @@ -667,12 +674,8 @@ static int etm4_config_timestamp_event(struct etmv4_drvdata *drvdata)
>> return -ENOSPC;
>> }
>>
>> - /*
>> - * Initialise original and reload counter value to the smallest
>> - * possible value in order to get as much precision as we can.
>> - */
>> - config->cntr_val[ctridx] = 1;
>> - config->cntrldvr[ctridx] = 1;
>> + /* Initialise original and reload counter value. */
>> + config->cntr_val[ctridx] = config->cntrldvr[ctridx] = 1 << ts_level;
>>
>> /*
>> * Trace Counter Control Register TRCCNTCTLRn
>> @@ -762,7 +765,7 @@ static int etm4_parse_event_config(struct coresight_device *csdev,
>> * order to correlate instructions executed on different CPUs
>> * (CPU-wide trace scenarios).
>> */
>> - ret = etm4_config_timestamp_event(drvdata);
>> + ret = etm4_config_timestamp_event(drvdata, attr);
>>
>> /*
>> * No need to go further if timestamp intervals can't
>>
>> --
>> 2.34.1
>>
>
> Regards
>
>
> Mike
>
> --
> Mike Leach
> Principal Engineer, ARM Ltd.
> Manchester Design Centre. UK
Powered by blists - more mailing lists