| 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: <CAJ9a7VgOO1pmS0chOXugC7eR06v_Tu8onjJA7rnbqcBdZ2m1iA@mail.gmail.com>
Date: Thu, 9 Oct 2025 17:02:27 +0100
From: Mike Leach <mike.leach@...aro.org>
To: James Clark <james.clark@...aro.org>
Cc: Suzuki K Poulose <suzuki.poulose@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jonathan Corbet <corbet@....net>,
Leo Yan <leo.yan@....com>, coresight@...ts.linaro.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-doc@...r.kernel.org
Subject: Re: [PATCH v3 3/5] coresight: Refactor etm4_config_timestamp_event()
Hi James,
On Thu, 2 Oct 2025 at 11:09, James Clark <james.clark@...aro.org> wrote:
>
> Remove some of the magic numbers and try to clarify some of the
> documentation so it's clearer how this sets up the timestamp interval.
>
> Return errors directly instead of jumping to out and returning ret,
> nothing needs to be cleaned up at the end and it only obscures the flow
> and return value.
>
> Reviewed-by: Leo Yan <leo.yan@....com>
> Signed-off-by: James Clark <james.clark@...aro.org>
> ---
> drivers/hwtracing/coresight/coresight-etm4x-core.c | 95 ++++++++++++++--------
> drivers/hwtracing/coresight/coresight-etm4x.h | 20 +++--
> 2 files changed, 77 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index 020f070bf17d..920d092ef862 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -609,18 +609,33 @@ static void etm4_enable_hw_smp_call(void *info)
> * TRCRSCTLR1 (always true) used to get the counter to decrement. From
> * there a resource selector is configured with the counter and the
> * timestamp control register to use the resource selector to trigger the
> - * event that will insert a timestamp packet in the stream.
> + * event that will insert a timestamp packet in the stream:
> + *
> + * +--------------+
> + * | Resource 1 | fixed "always-true" resource
> + * +--------------+
> + * |
> + * +------v-------+
> + * | Counter x | (reload to 1 on underflow)
> + * +--------------+
> + * |
> + * +------v--------------+
> + * | Resource Selector y | (trigger on counter x == 0)
> + * +---------------------+
> + * |
> + * +------v---------------+
> + * | Timestamp Generator | (timestamp on resource y)
> + * +----------------------+
> */
> static int etm4_config_timestamp_event(struct etmv4_drvdata *drvdata)
> {
> - int ctridx, ret = -EINVAL;
> - int counter, rselector;
> - u32 val = 0;
> + int ctridx;
> + int rselector;
> struct etmv4_config *config = &drvdata->config;
>
> /* No point in trying if we don't have at least one counter */
> if (!drvdata->nr_cntr)
> - goto out;
> + return -EINVAL;
As you mention elsewhere - TS will always be output for at least every
4096 bytes - so if we have no counter perhaps we can live with that.
Previously we where trying to get as fast as possible, now we want to
slow them down - so every 4096 looks a decent compromise if the
hardware has no counters.
TRCTSCTLR will be 0 - which selects the false event - so only the
non-event timestamps occur. Returning 0 here will enable the non event
timestamps
What this means is if the user selects timestamps, there will always
be at least some timestamps output.
Alternatively, TRCTSCTLR = 0x1 selects the TRUE event - which might
result once again in the etm attempting to insert timestamps at every
available opportunity. - though never tried this!
>
> /* Find a counter that hasn't been initialised */
> for (ctridx = 0; ctridx < drvdata->nr_cntr; ctridx++)
> @@ -630,15 +645,17 @@ static int etm4_config_timestamp_event(struct etmv4_drvdata *drvdata)
> /* All the counters have been configured already, bail out */
> if (ctridx == drvdata->nr_cntr) {
> pr_debug("%s: no available counter found\n", __func__);
> - ret = -ENOSPC;
> - goto out;
> + return -ENOSPC;
> }
>
> /*
> - * Searching for an available resource selector to use, starting at
> - * '2' since every implementation has at least 2 resource selector.
> - * ETMIDR4 gives the number of resource selector _pairs_,
> - * hence multiply by 2.
Well - from ETMv4.3 that statement is not true - there can be no RS
pairs - but no RS pairs forces no counters so should not actually get
here.
Needs comment to reflect this.
> + * Searching for an available resource selector to use, starting at '2'
> + * since resource 0 is the fixed 'always returns false' resource and 1
> + * is the fixed 'always returns true' resource. See IHI0064H_b '7.3.64
> + * TRCRSCTLRn, Resource Selection Control Registers, n=2-31'.
> + *
> + * ETMIDR4 gives the number of resource selector _pairs_, hence multiply
> + * by 2.
> */
> for (rselector = 2; rselector < drvdata->nr_resource * 2; rselector++)
> if (!config->res_ctrl[rselector])
> @@ -647,13 +664,9 @@ static int etm4_config_timestamp_event(struct etmv4_drvdata *drvdata)
> if (rselector == drvdata->nr_resource * 2) {
> pr_debug("%s: no available resource selector found\n",
> __func__);
> - ret = -ENOSPC;
> - goto out;
> + return -ENOSPC;
> }
>
> - /* Remember what counter we used */
> - counter = 1 << ctridx;
> -
> /*
> * Initialise original and reload counter value to the smallest
> * possible value in order to get as much precision as we can.
> @@ -661,26 +674,42 @@ static int etm4_config_timestamp_event(struct etmv4_drvdata *drvdata)
> config->cntr_val[ctridx] = 1;
> config->cntrldvr[ctridx] = 1;
>
> - /* Set the trace counter control register */
> - val = 0x1 << 16 | /* Bit 16, reload counter automatically */
> - 0x0 << 7 | /* Select single resource selector */
> - 0x1; /* Resource selector 1, i.e always true */
> -
> - config->cntr_ctrl[ctridx] = val;
> -
> - val = 0x2 << 16 | /* Group 0b0010 - Counter and sequencers */
> - counter << 0; /* Counter to use */
> -
> - config->res_ctrl[rselector] = val;
> + /*
> + * Trace Counter Control Register TRCCNTCTLRn
> + *
> + * CNTCHAIN = 0, don't reload on the previous counter
> + * RLDSELF = true, reload counter automatically on underflow
> + * RLDTYPE = 0, one reload input resource
> + * RLDSEL = 0, reload on resource 0 (fixed always false resource, never
> + * reload)
> + * CNTTYPE = 0, one count input resource
> + * CNTSEL = 1, count on resource 1 (fixed always true resource, always
> + * decrement)
> + */
> + config->cntr_ctrl[ctridx] = TRCCNTCTLRn_RLDSELF |
> + FIELD_PREP(TRCCNTCTLRn_CNTSEL_MASK, 1);
>
if we are eliminating magic numbers - the '1' here should be something
like RESOURCE_SEL_TRUE?
> - val = 0x0 << 7 | /* Select single resource selector */
> - rselector; /* Resource selector */
> + /*
> + * Resource Selection Control Register TRCRSCTLRn
> + *
> + * PAIRINV = 0, INV = 0, don't invert
> + * GROUP = 2, SELECT = ctridx, trigger when counter 'ctridx' reaches 0
> + *
> + * Multiple counters can be selected, and each bit signifies a counter,
> + * so set bit 'ctridx' to select our counter.
> + */
> + config->res_ctrl[rselector] = FIELD_PREP(TRCRSCTLRn_GROUP_MASK, 2) |
> + FIELD_PREP(TRCRSCTLRn_SELECT_MASK, 1 << ctridx);
>
> - config->ts_ctrl = val;
> + /*
> + * Global Timestamp Control Register TRCTSCTLR
> + *
> + * TYPE = 0, one input resource
> + * SEL = rselector, generate timestamp on resource 'rselector'
> + */
> + config->ts_ctrl = FIELD_PREP(TRCTSCTLR_SEL_MASK, rselector);
>
> - ret = 0;
> -out:
> - return ret;
> + return 0;
> }
>
> static int etm4_parse_event_config(struct coresight_device *csdev,
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
> index 813e7208ad17..a06338851ef5 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x.h
> +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
> @@ -225,6 +225,16 @@
> #define TRCRSCTLRn_GROUP_MASK GENMASK(19, 16)
> #define TRCRSCTLRn_SELECT_MASK GENMASK(15, 0)
>
> +#define TRCCNTCTLRn_CNTCHAIN BIT(17)
> +#define TRCCNTCTLRn_RLDSELF BIT(16)
> +#define TRCCNTCTLRn_RLDTYPE BIT(15)
> +#define TRCCNTCTLRn_RLDSEL_MASK GENMASK(12, 8)
> +#define TRCCNTCTLRn_CNTTYPE_MASK BIT(7)
> +#define TRCCNTCTLRn_CNTSEL_MASK GENMASK(4, 0)
> +
> +#define TRCTSCTLR_TYPE BIT(7)
> +#define TRCTSCTLR_SEL_MASK GENMASK(4, 0)
> +
> /*
> * System instructions to access ETM registers.
> * See ETMv4.4 spec ARM IHI0064F section 4.3.6 System instructions
> @@ -824,7 +834,7 @@ struct etmv4_config {
> u32 eventctrl0;
> u32 eventctrl1;
> u32 stall_ctrl;
> - u32 ts_ctrl;
> + u32 ts_ctrl; /* TRCTSCTLR */
> u32 ccctlr;
> u32 bb_ctrl;
> u32 vinst_ctrl;
> @@ -837,11 +847,11 @@ struct etmv4_config {
> u32 seq_rst;
> u32 seq_state;
> u8 cntr_idx;
> - u32 cntrldvr[ETMv4_MAX_CNTR];
> - u32 cntr_ctrl[ETMv4_MAX_CNTR];
> - u32 cntr_val[ETMv4_MAX_CNTR];
> + u32 cntrldvr[ETMv4_MAX_CNTR]; /* TRCCNTRLDVRn */
> + u32 cntr_ctrl[ETMv4_MAX_CNTR]; /* TRCCNTCTLRn */
> + u32 cntr_val[ETMv4_MAX_CNTR]; /* TRCCNTVRn */
> u8 res_idx;
> - u32 res_ctrl[ETM_MAX_RES_SEL];
> + u32 res_ctrl[ETM_MAX_RES_SEL]; /* TRCRSCTLRn */
> u8 ss_idx;
> u32 ss_ctrl[ETM_MAX_SS_CMP];
> u32 ss_status[ETM_MAX_SS_CMP];
>
> --
> 2.34.1
>
Regards
Mike
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
Powered by blists - more mailing lists