Hi Mathieu,

On Tue, 5 Apr 2022 at 18:02, Mathieu Poirier mathieu.poirier@linaro.org wrote:

Good morning,

To avoid getting tunnel vision, I haven't red any of the comments already posted about this set. As such some of my observations may be redundant.

On Tue, Mar 08, 2022 at 08:49:51PM +0000, Mike Leach wrote:

...

The existing mechanism to assign Trace ID values to sources is limited and does not scale for larger multicore / multi trace source systems.

The API introduces functions that reserve IDs based on availabilty represented by a coresight_trace_id_map structure. This records the used and free IDs in a bitmap.

CPU bound sources such as ETMs use the coresight_trace_id_get_cpu_id / coresight_trace_id_put_cpu_id pair of functions. The API will record the ID associated with the CPU. This ensures that the same ID will be re-used while perf events are active on the CPU. The put_cpu_id function will pend release of the ID until all perf cs_etm sessions are complete.

Non-cpu sources, such as the STM can use coresight_trace_id_get_system_id / coresight_trace_id_put_system_id.

Signed-off-by: Mike Leach mike.leach@linaro.org

drivers/hwtracing/coresight/Makefile | 2 +- .../hwtracing/coresight/coresight-trace-id.c | 222 ++++++++++++++++++ .../hwtracing/coresight/coresight-trace-id.h | 69 ++++++ 3 files changed, 292 insertions(+), 1 deletion(-) create mode 100644 drivers/hwtracing/coresight/coresight-trace-id.c create mode 100644 drivers/hwtracing/coresight/coresight-trace-id.h

diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile index b6c4a48140ec..329a0c704b87 100644 --- a/drivers/hwtracing/coresight/Makefile +++ b/drivers/hwtracing/coresight/Makefile @@ -6,7 +6,7 @@ obj-$(CONFIG_CORESIGHT) += coresight.o coresight-y := coresight-core.o coresight-etm-perf.o coresight-platform.o \ coresight-sysfs.o coresight-syscfg.o coresight-config.o \ coresight-cfg-preload.o coresight-cfg-afdo.o \

•         coresight-syscfg-configfs.o
  

•         coresight-syscfg-configfs.o coresight-trace-id.o
  

obj-$(CONFIG_CORESIGHT_LINK_AND_SINK_TMC) += coresight-tmc.o coresight-tmc-y := coresight-tmc-core.o coresight-tmc-etf.o \ coresight-tmc-etr.o diff --git a/drivers/hwtracing/coresight/coresight-trace-id.c b/drivers/hwtracing/coresight/coresight-trace-id.c new file mode 100644 index 000000000000..ce6c7d7b55d6 --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-trace-id.c @@ -0,0 +1,222 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (c) 2022, Linaro Limited, All rights reserved.
• • Author: Mike Leach mike.leach@linaro.org
• */

+#include <linux/kernel.h> +#include <linux/types.h> +#include <linux/spinlock.h>

+#include "coresight-trace-id.h"

+/* need to keep data on ids & association with cpus. */ +struct cpu_id_info {

• int id;
  

• bool pend_rel;
  

+};

+/* maintain a record of the current mapping of cpu IDs */ +static DEFINE_PER_CPU(struct cpu_id_info, cpu_ids);

+/* a list of currently used id_maps */ +static LIST_HEAD(id_map_list);

+/* perf session active flag */ +static int perf_cs_etm_session_active;

+/* lock to protect id_map list and cpu data */ +static DEFINE_SPINLOCK(id_map_lock);

+/* ID 0 is reserved */ +#define CORESIGHT_TRACE_ID_RES_0 0

+/* ID 0x70 onwards are reserved */ +#define CORESIGHT_TRACE_ID_RES_RANGE_LO 0x70 +#define CORESIGHT_TRACE_ID_RES_RANGE_HI 0x7F

+#define IS_VALID_ID(id) \

• ((id > CORESIGHT_TRACE_ID_RES_0) && (id < CORESIGHT_TRACE_ID_RES_RANGE_LO))
  

+static void coresight_trace_id_set_inuse(int id, struct coresight_trace_id_map *id_map) +{

• if (IS_VALID_ID(id))
  

•         set_bit(id, id_map->avail_ids);
  

+}

+static void coresight_trace_id_clear_inuse(int id, struct coresight_trace_id_map *id_map) +{

• if (IS_VALID_ID(id))
  

•         clear_bit(id, id_map->avail_ids);
  

+}

+static void coresight_trace_id_set_pend_rel(int id, struct coresight_trace_id_map *id_map) +{

• if (IS_VALID_ID(id))
  

•         set_bit(id, id_map->pend_rel_ids);
  

+}

+static void coresight_trace_id_clear_pend_rel(int id, struct coresight_trace_id_map *id_map) +{

• if (IS_VALID_ID(id))
  

•         clear_bit(id, id_map->pend_rel_ids);
  

+}

+static int coresight_trace_id_find_new_id(struct coresight_trace_id_map *id_map) +{

• int id;
  

• id = find_first_zero_bit(id_map->avail_ids, CORESIGHT_TRACE_IDS_MAX);
  

• if (id >= CORESIGHT_TRACE_IDS_MAX)
  

•         id = -EINVAL;
  

• return id;
  

+}

+/* release all pending IDs for all current maps & clear CPU associations */ +static void coresight_trace_id_release_all_pending(void) +{

• struct coresight_trace_id_map *id_map;
  

• int cpu, bit;
  

• list_for_each_entry(id_map, &id_map_list, node) {
  

•         for_each_set_bit(bit, id_map->pend_rel_ids, CORESIGHT_TRACE_IDS_MAX) {
  

•                 clear_bit(bit, id_map->avail_ids);
  

•                 clear_bit(bit, id_map->pend_rel_ids);
  

•         }
  

• }
  

• for_each_possible_cpu(cpu) {
  

•         if (per_cpu(cpu_ids, cpu).pend_rel) {
  

•                 per_cpu(cpu_ids, cpu).pend_rel = false;
  

•                 per_cpu(cpu_ids, cpu).id = 0;
  

•         }
  

• }
  

+}

+int coresight_trace_id_get_cpu_id(int cpu, struct coresight_trace_id_map *id_map) +{

• unsigned long flags;
  

• int id;
  

• spin_lock_irqsave(&id_map_lock, flags);
  

• id = per_cpu(cpu_ids, cpu).id;
  

• if (!id) {
  

•         id = coresight_trace_id_find_new_id(id_map);
  

•         if (id < 0)
  

•                 goto get_cpu_id_out;
  

• }
  

• per_cpu(cpu_ids, cpu).id = id;
  

• per_cpu(cpu_ids, cpu).pend_rel = false;
  

• coresight_trace_id_set_inuse(id, id_map);
  

• coresight_trace_id_clear_pend_rel(id, id_map);
  

The above should have been done when the ID for this CPU has been set for the first time. Therefore we should simply release the spinlock and return if an ID has already been set.

Agreed.

...

+get_cpu_id_out:

• spin_unlock_irqrestore(&id_map_lock, flags);
  

• return id;
  

+} +EXPORT_SYMBOL_GPL(coresight_trace_id_get_cpu_id);

+void coresight_trace_id_put_cpu_id(int cpu, struct coresight_trace_id_map *id_map) +{

• unsigned long flags;
  

• int id;
  

• spin_lock_irqsave(&id_map_lock, flags);
  

• id = per_cpu(cpu_ids, cpu).id;
  

• if (!id)
  

•         goto put_cpu_id_out;
  

• if (perf_cs_etm_session_active) {
  

•         /* set release at pending if perf still active */
  

•         coresight_trace_id_set_pend_rel(id, id_map);
  

•         per_cpu(cpu_ids, cpu).pend_rel = true;
  

• } else {
  

•         /* otherwise clear id */
  

•         coresight_trace_id_clear_inuse(id, id_map);
  

•         per_cpu(cpu_ids, cpu).id = 0;
  

• }
  

• put_cpu_id_out:

• spin_unlock_irqrestore(&id_map_lock, flags);
  

+} +EXPORT_SYMBOL_GPL(coresight_trace_id_put_cpu_id);

+int coresight_trace_id_get_system_id(struct coresight_trace_id_map *id_map) +{

So far I have reviewed until patch 05 and I find passing the id_map every time we need an ID quite heavy, especially since it is always the same one.

My initital thought was to ensure that the API would handle the forthcoming requirements for per-sink id maps without having to change too much code, both within the API and the client code.

However an alternative method could be to drop the id_map passing in the external API calls - simplifying it and the client code, and simply extend the API once passing in the id_map is really required. This would mean being able to drop the list of maps for now, use a default id_map withing the ID map code. However I would retain the passing of the id_map for the internal functions as I feel it is better practice to have functions operate on data passed to them rather then a global blob.

Regards

Mike

...

• unsigned long flags;
  

• int id;
  

• spin_lock_irqsave(&id_map_lock, flags);
  

• id = coresight_trace_id_find_new_id(id_map);
  

• if (id > 0)
  

•         coresight_trace_id_set_inuse(id, id_map);
  

• spin_unlock_irqrestore(&id_map_lock, flags);
  

• return id;
  

+} +EXPORT_SYMBOL_GPL(coresight_trace_id_get_system_id);

+void coresight_trace_id_put_system_id(struct coresight_trace_id_map *id_map, int id) +{

• unsigned long flags;
  

• spin_lock_irqsave(&id_map_lock, flags);
  

• coresight_trace_id_clear_inuse(id, id_map);
  

• spin_unlock_irqrestore(&id_map_lock, flags);
  

+} +EXPORT_SYMBOL_GPL(coresight_trace_id_put_system_id);

+void coresight_trace_id_perf_start(void) +{

• unsigned long flags;
  

• spin_lock_irqsave(&id_map_lock, flags);
  

• perf_cs_etm_session_active++;
  

• spin_unlock_irqrestore(&id_map_lock, flags);
  

+} +EXPORT_SYMBOL_GPL(coresight_trace_id_perf_start);

+void coresight_trace_id_perf_stop(void) +{

• unsigned long flags;
  

• spin_lock_irqsave(&id_map_lock, flags);
  

• perf_cs_etm_session_active--;
  

• if (!perf_cs_etm_session_active)
  

•         coresight_trace_id_release_all_pending();
  

• spin_unlock_irqrestore(&id_map_lock, flags);
  

+} +EXPORT_SYMBOL_GPL(coresight_trace_id_perf_stop);

+void coresight_trace_id_init_id_map(struct coresight_trace_id_map *id_map) +{

• unsigned long flags;
  

• int bit;
  

• /* set all reserved bits as in-use */
  

• set_bit(CORESIGHT_TRACE_ID_RES_0, id_map->avail_ids);
  

• for (bit = CORESIGHT_TRACE_ID_RES_RANGE_LO;
  

•      bit <= CORESIGHT_TRACE_ID_RES_RANGE_HI; bit++)
  

•         set_bit(bit, id_map->avail_ids);
  

• spin_lock_irqsave(&id_map_lock, flags);
  

• /* add id_map to the list */
  

• list_add(&id_map->node, &id_map_list);
  

• spin_unlock_irqrestore(&id_map_lock, flags);
  

+} +EXPORT_SYMBOL_GPL(coresight_trace_id_init_id_map);

+void coresight_trace_id_release_id_map(struct coresight_trace_id_map *id_map) +{

• unsigned long flags;
  

• spin_lock_irqsave(&id_map_lock, flags);
  

• /* remove id_map from list */
  

• list_del(&id_map->node);
  

• spin_unlock_irqrestore(&id_map_lock, flags);
  

+} +EXPORT_SYMBOL_GPL(coresight_trace_id_release_id_map); diff --git a/drivers/hwtracing/coresight/coresight-trace-id.h b/drivers/hwtracing/coresight/coresight-trace-id.h new file mode 100644 index 000000000000..01db2441cee6 --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-trace-id.h @@ -0,0 +1,69 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*

• • Copyright(C) 2022 Linaro Limited. All rights reserved.
• • Author: Mike Leach mike.leach@linaro.org
• */

+#ifndef _CORESIGHT_TRACE_ID_H +#define _CORESIGHT_TRACE_ID_H

+/*

• • Coresight trace ID allocation API
• • With multi cpu systems, and more additional trace sources a scalable
• • trace ID reservation system is required.
• • The system will allocate Ids on a demand basis, and allow them to be
• • released when done.
• • In order to ensure that a consistent cpu / ID matching is maintained
• • throughout a perf cs_etm event session - a session in progress flag will
• • be maintained, and released IDs not cleared until the perf session is
• • complete. This allows the same CPU to be re-allocated its prior ID.
• • Trace ID maps will be created and initialised to prevent architecturally
• • reserved IDs from being allocated.
• • API permits multiple maps to be maintained - for large systems where
• • different sets of cpus trace into different independent sinks.
• */

+#include <linux/bitops.h> +#include <linux/types.h>

+/* architecturally we have 128 IDs some of which are reserved */ +#define CORESIGHT_TRACE_IDS_MAX 128

+/**

• • Trace ID map.
• • @avail_ids: Bitmap to register available (bit = 0) and in use (bit = 1) IDs.

• •       Initialised so that the reserved IDs are permanently marked as in use.
    

• • @pend_rel_ids: CPU IDs that have been released by the trace source but not yet marked

• •            as available, to allow re-allocation to the same CPU during a perf session.
    

• • @node: List entry to add to list of managed trace id maps.
• */

+struct coresight_trace_id_map {

• DECLARE_BITMAP(avail_ids, CORESIGHT_TRACE_IDS_MAX);
  

• DECLARE_BITMAP(pend_rel_ids, CORESIGHT_TRACE_IDS_MAX);
  

• struct list_head node;
  

+};

+/* Allocate and release IDs */ +int coresight_trace_id_get_cpu_id(int cpu, struct coresight_trace_id_map *id_map); +int coresight_trace_id_get_system_id(struct coresight_trace_id_map *id_map); +void coresight_trace_id_put_cpu_id(int cpu, struct coresight_trace_id_map *id_map); +void coresight_trace_id_put_system_id(struct coresight_trace_id_map *id_map, int id);

+/* initialise ID map - block reserved IDs, add to list of ID maps */ +void coresight_trace_id_init_id_map(struct coresight_trace_id_map *id_map); +/* remove ID maps from list of maps */ +void coresight_trace_id_release_id_map(struct coresight_trace_id_map *id_map);

+/* notifiers for perf session start and stop */ +void coresight_trace_id_perf_start(void); +void coresight_trace_id_perf_stop(void);

+#endif /* _CORESIGHT_TRACE_ID_H */

2.17.1

Mathieu

On Thu, 7 Apr 2022 at 19:08, Mathieu Poirier mathieu.poirier@linaro.org wrote:

On Wed, Apr 06, 2022 at 08:45:21PM +0100, Mike Leach wrote:

...

Hi Mathieu,

On Tue, 5 Apr 2022 at 18:02, Mathieu Poirier mathieu.poirier@linaro.org wrote:

...

Good morning,

To avoid getting tunnel vision, I haven't red any of the comments already posted about this set. As such some of my observations may be redundant.

On Tue, Mar 08, 2022 at 08:49:51PM +0000, Mike Leach wrote:

...

The existing mechanism to assign Trace ID values to sources is limited and does not scale for larger multicore / multi trace source systems.

The API introduces functions that reserve IDs based on availabilty represented by a coresight_trace_id_map structure. This records the used and free IDs in a bitmap.

CPU bound sources such as ETMs use the coresight_trace_id_get_cpu_id / coresight_trace_id_put_cpu_id pair of functions. The API will record the ID associated with the CPU. This ensures that the same ID will be re-used while perf events are active on the CPU. The put_cpu_id function will pend release of the ID until all perf cs_etm sessions are complete.

Non-cpu sources, such as the STM can use coresight_trace_id_get_system_id / coresight_trace_id_put_system_id.

Signed-off-by: Mike Leach mike.leach@linaro.org

drivers/hwtracing/coresight/Makefile | 2 +- .../hwtracing/coresight/coresight-trace-id.c | 222 ++++++++++++++++++ .../hwtracing/coresight/coresight-trace-id.h | 69 ++++++ 3 files changed, 292 insertions(+), 1 deletion(-) create mode 100644 drivers/hwtracing/coresight/coresight-trace-id.c create mode 100644 drivers/hwtracing/coresight/coresight-trace-id.h

diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile index b6c4a48140ec..329a0c704b87 100644 --- a/drivers/hwtracing/coresight/Makefile +++ b/drivers/hwtracing/coresight/Makefile @@ -6,7 +6,7 @@ obj-$(CONFIG_CORESIGHT) += coresight.o coresight-y := coresight-core.o coresight-etm-perf.o coresight-platform.o \ coresight-sysfs.o coresight-syscfg.o coresight-config.o \ coresight-cfg-preload.o coresight-cfg-afdo.o \

•         coresight-syscfg-configfs.o
  

•         coresight-syscfg-configfs.o coresight-trace-id.o
  

obj-$(CONFIG_CORESIGHT_LINK_AND_SINK_TMC) += coresight-tmc.o coresight-tmc-y := coresight-tmc-core.o coresight-tmc-etf.o \ coresight-tmc-etr.o diff --git a/drivers/hwtracing/coresight/coresight-trace-id.c b/drivers/hwtracing/coresight/coresight-trace-id.c new file mode 100644 index 000000000000..ce6c7d7b55d6 --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-trace-id.c @@ -0,0 +1,222 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (c) 2022, Linaro Limited, All rights reserved.
• • Author: Mike Leach mike.leach@linaro.org
• */

+#include <linux/kernel.h> +#include <linux/types.h> +#include <linux/spinlock.h>

+#include "coresight-trace-id.h"

+/* need to keep data on ids & association with cpus. */ +struct cpu_id_info {

• int id;
  

• bool pend_rel;
  

+};

+/* maintain a record of the current mapping of cpu IDs */ +static DEFINE_PER_CPU(struct cpu_id_info, cpu_ids);

+/* a list of currently used id_maps */ +static LIST_HEAD(id_map_list);

+/* perf session active flag */ +static int perf_cs_etm_session_active;

+/* lock to protect id_map list and cpu data */ +static DEFINE_SPINLOCK(id_map_lock);

+/* ID 0 is reserved */ +#define CORESIGHT_TRACE_ID_RES_0 0

+/* ID 0x70 onwards are reserved */ +#define CORESIGHT_TRACE_ID_RES_RANGE_LO 0x70 +#define CORESIGHT_TRACE_ID_RES_RANGE_HI 0x7F

+#define IS_VALID_ID(id) \

• ((id > CORESIGHT_TRACE_ID_RES_0) && (id < CORESIGHT_TRACE_ID_RES_RANGE_LO))
  

+static void coresight_trace_id_set_inuse(int id, struct coresight_trace_id_map *id_map) +{

• if (IS_VALID_ID(id))
  

•         set_bit(id, id_map->avail_ids);
  

+}

+static void coresight_trace_id_clear_inuse(int id, struct coresight_trace_id_map *id_map) +{

• if (IS_VALID_ID(id))
  

•         clear_bit(id, id_map->avail_ids);
  

+}

+static void coresight_trace_id_set_pend_rel(int id, struct coresight_trace_id_map *id_map) +{

• if (IS_VALID_ID(id))
  

•         set_bit(id, id_map->pend_rel_ids);
  

+}

+static void coresight_trace_id_clear_pend_rel(int id, struct coresight_trace_id_map *id_map) +{

• if (IS_VALID_ID(id))
  

•         clear_bit(id, id_map->pend_rel_ids);
  

+}

+static int coresight_trace_id_find_new_id(struct coresight_trace_id_map *id_map) +{

• int id;
  

• id = find_first_zero_bit(id_map->avail_ids, CORESIGHT_TRACE_IDS_MAX);
  

• if (id >= CORESIGHT_TRACE_IDS_MAX)
  

•         id = -EINVAL;
  

• return id;
  

+}

+/* release all pending IDs for all current maps & clear CPU associations */ +static void coresight_trace_id_release_all_pending(void) +{

• struct coresight_trace_id_map *id_map;
  

• int cpu, bit;
  

• list_for_each_entry(id_map, &id_map_list, node) {
  

•         for_each_set_bit(bit, id_map->pend_rel_ids, CORESIGHT_TRACE_IDS_MAX) {
  

•                 clear_bit(bit, id_map->avail_ids);
  

•                 clear_bit(bit, id_map->pend_rel_ids);
  

•         }
  

• }
  

• for_each_possible_cpu(cpu) {
  

•         if (per_cpu(cpu_ids, cpu).pend_rel) {
  

•                 per_cpu(cpu_ids, cpu).pend_rel = false;
  

•                 per_cpu(cpu_ids, cpu).id = 0;
  

•         }
  

• }
  

+}

+int coresight_trace_id_get_cpu_id(int cpu, struct coresight_trace_id_map *id_map) +{

• unsigned long flags;
  

• int id;
  

• spin_lock_irqsave(&id_map_lock, flags);
  

• id = per_cpu(cpu_ids, cpu).id;
  

• if (!id) {
  

•         id = coresight_trace_id_find_new_id(id_map);
  

•         if (id < 0)
  

•                 goto get_cpu_id_out;
  

• }
  

• per_cpu(cpu_ids, cpu).id = id;
  

• per_cpu(cpu_ids, cpu).pend_rel = false;
  

• coresight_trace_id_set_inuse(id, id_map);
  

• coresight_trace_id_clear_pend_rel(id, id_map);
  

The above should have been done when the ID for this CPU has been set for the first time. Therefore we should simply release the spinlock and return if an ID has already been set.

Agreed.

...

...

+get_cpu_id_out:

• spin_unlock_irqrestore(&id_map_lock, flags);
  

• return id;
  

+} +EXPORT_SYMBOL_GPL(coresight_trace_id_get_cpu_id);

+void coresight_trace_id_put_cpu_id(int cpu, struct coresight_trace_id_map *id_map) +{

• unsigned long flags;
  

• int id;
  

• spin_lock_irqsave(&id_map_lock, flags);
  

• id = per_cpu(cpu_ids, cpu).id;
  

• if (!id)
  

•         goto put_cpu_id_out;
  

• if (perf_cs_etm_session_active) {
  

•         /* set release at pending if perf still active */
  

•         coresight_trace_id_set_pend_rel(id, id_map);
  

•         per_cpu(cpu_ids, cpu).pend_rel = true;
  

• } else {
  

•         /* otherwise clear id */
  

•         coresight_trace_id_clear_inuse(id, id_map);
  

•         per_cpu(cpu_ids, cpu).id = 0;
  

• }
  

• put_cpu_id_out:

• spin_unlock_irqrestore(&id_map_lock, flags);
  

+} +EXPORT_SYMBOL_GPL(coresight_trace_id_put_cpu_id);

+int coresight_trace_id_get_system_id(struct coresight_trace_id_map *id_map) +{

So far I have reviewed until patch 05 and I find passing the id_map every time we need an ID quite heavy, especially since it is always the same one.

My initital thought was to ensure that the API would handle the forthcoming requirements for per-sink id maps without having to change too much code, both within the API and the client code.

I think going with a perf-sink id map would fix a lot of problems.

I think you misread here - were are talking about a "per-sink" not "perF-sink" ID map.

The issue being that multi-cluster devices can have a set of CPU/ETMs running into one sink, while the other cluster runs CPUs/ETMs into another sink. Trace IDs only have to be unique for a given sink - and associated with the CPU/ETM writing to that sink.

Each trace sink can have its own ID map - but both sinks could be linked to the same perf session - if the session runs on both clusters.

We don't have any systems big enough to need or test that yet - i.e we do not currently run out of IDs - which is why it is not implemented in this set - but it is anticipated as a necessity for the future.

Thanks

Mike

The id map would need to be linked with the perf session, as we already do with sink buffers, i.e CPUs in the same perf session id would get the same id map.

In etm_event_start() a CPU's traceID could be fetch from the etm_event_data and sent down to the driver with an additional parameter to source_ops()->enable().

I am done revewing this set.

Thanks, Mathieu

...

However an alternative method could be to drop the id_map passing in the external API calls - simplifying it and the client code, and simply extend the API once passing in the id_map is really required. This would mean being able to drop the list of maps for now, use a default id_map withing the ID map code. However I would retain the passing of the id_map for the internal functions as I feel it is better practice to have functions operate on data passed to them rather then a global blob.

Regards

Mike

...

...

• unsigned long flags;
  

• int id;
  

• spin_lock_irqsave(&id_map_lock, flags);
  

• id = coresight_trace_id_find_new_id(id_map);
  

• if (id > 0)
  

•         coresight_trace_id_set_inuse(id, id_map);
  

• spin_unlock_irqrestore(&id_map_lock, flags);
  

• return id;
  

+} +EXPORT_SYMBOL_GPL(coresight_trace_id_get_system_id);

+void coresight_trace_id_put_system_id(struct coresight_trace_id_map *id_map, int id) +{

• unsigned long flags;
  

• spin_lock_irqsave(&id_map_lock, flags);
  

• coresight_trace_id_clear_inuse(id, id_map);
  

• spin_unlock_irqrestore(&id_map_lock, flags);
  

+} +EXPORT_SYMBOL_GPL(coresight_trace_id_put_system_id);

+void coresight_trace_id_perf_start(void) +{

• unsigned long flags;
  

• spin_lock_irqsave(&id_map_lock, flags);
  

• perf_cs_etm_session_active++;
  

• spin_unlock_irqrestore(&id_map_lock, flags);
  

+} +EXPORT_SYMBOL_GPL(coresight_trace_id_perf_start);

+void coresight_trace_id_perf_stop(void) +{

• unsigned long flags;
  

• spin_lock_irqsave(&id_map_lock, flags);
  

• perf_cs_etm_session_active--;
  

• if (!perf_cs_etm_session_active)
  

•         coresight_trace_id_release_all_pending();
  

• spin_unlock_irqrestore(&id_map_lock, flags);
  

+} +EXPORT_SYMBOL_GPL(coresight_trace_id_perf_stop);

+void coresight_trace_id_init_id_map(struct coresight_trace_id_map *id_map) +{

• unsigned long flags;
  

• int bit;
  

• /* set all reserved bits as in-use */
  

• set_bit(CORESIGHT_TRACE_ID_RES_0, id_map->avail_ids);
  

• for (bit = CORESIGHT_TRACE_ID_RES_RANGE_LO;
  

•      bit <= CORESIGHT_TRACE_ID_RES_RANGE_HI; bit++)
  

•         set_bit(bit, id_map->avail_ids);
  

• spin_lock_irqsave(&id_map_lock, flags);
  

• /* add id_map to the list */
  

• list_add(&id_map->node, &id_map_list);
  

• spin_unlock_irqrestore(&id_map_lock, flags);
  

+} +EXPORT_SYMBOL_GPL(coresight_trace_id_init_id_map);

+void coresight_trace_id_release_id_map(struct coresight_trace_id_map *id_map) +{

• unsigned long flags;
  

• spin_lock_irqsave(&id_map_lock, flags);
  

• /* remove id_map from list */
  

• list_del(&id_map->node);
  

• spin_unlock_irqrestore(&id_map_lock, flags);
  

+} +EXPORT_SYMBOL_GPL(coresight_trace_id_release_id_map); diff --git a/drivers/hwtracing/coresight/coresight-trace-id.h b/drivers/hwtracing/coresight/coresight-trace-id.h new file mode 100644 index 000000000000..01db2441cee6 --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-trace-id.h @@ -0,0 +1,69 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*

• • Copyright(C) 2022 Linaro Limited. All rights reserved.
• • Author: Mike Leach mike.leach@linaro.org
• */

+#ifndef _CORESIGHT_TRACE_ID_H +#define _CORESIGHT_TRACE_ID_H

+/*

• • Coresight trace ID allocation API
• • With multi cpu systems, and more additional trace sources a scalable
• • trace ID reservation system is required.
• • The system will allocate Ids on a demand basis, and allow them to be
• • released when done.
• • In order to ensure that a consistent cpu / ID matching is maintained
• • throughout a perf cs_etm event session - a session in progress flag will
• • be maintained, and released IDs not cleared until the perf session is
• • complete. This allows the same CPU to be re-allocated its prior ID.
• • Trace ID maps will be created and initialised to prevent architecturally
• • reserved IDs from being allocated.
• • API permits multiple maps to be maintained - for large systems where
• • different sets of cpus trace into different independent sinks.
• */

+#include <linux/bitops.h> +#include <linux/types.h>

+/* architecturally we have 128 IDs some of which are reserved */ +#define CORESIGHT_TRACE_IDS_MAX 128

+/**

• • Trace ID map.
• • @avail_ids: Bitmap to register available (bit = 0) and in use (bit = 1) IDs.

• •       Initialised so that the reserved IDs are permanently marked as in use.
    

• • @pend_rel_ids: CPU IDs that have been released by the trace source but not yet marked

• •            as available, to allow re-allocation to the same CPU during a perf session.
    

• • @node: List entry to add to list of managed trace id maps.
• */

+struct coresight_trace_id_map {

• DECLARE_BITMAP(avail_ids, CORESIGHT_TRACE_IDS_MAX);
  

• DECLARE_BITMAP(pend_rel_ids, CORESIGHT_TRACE_IDS_MAX);
  

• struct list_head node;
  

+};

+/* Allocate and release IDs */ +int coresight_trace_id_get_cpu_id(int cpu, struct coresight_trace_id_map *id_map); +int coresight_trace_id_get_system_id(struct coresight_trace_id_map *id_map); +void coresight_trace_id_put_cpu_id(int cpu, struct coresight_trace_id_map *id_map); +void coresight_trace_id_put_system_id(struct coresight_trace_id_map *id_map, int id);

+/* initialise ID map - block reserved IDs, add to list of ID maps */ +void coresight_trace_id_init_id_map(struct coresight_trace_id_map *id_map); +/* remove ID maps from list of maps */ +void coresight_trace_id_release_id_map(struct coresight_trace_id_map *id_map);

+/* notifiers for perf session start and stop */ +void coresight_trace_id_perf_start(void); +void coresight_trace_id_perf_stop(void);

+#endif /* _CORESIGHT_TRACE_ID_H */

2.17.1

-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK

---

linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Tue, Mar 08, 2022 at 08:49:54PM +0000, Mike Leach wrote:

The trace ID API is now used to allocate trace IDs for ETM4.x / ETE devices.

For perf sessions, these will be allocated on enable, and released on disable.

For sysfs sessions, these will be allocated on enable, but only released on reset. This allows the sysfs session to interrogate the Trace ID used after the session is over - maintaining functional consistency with the previous allocation scheme.

The trace ID will also be allocated on read of the mgmt/trctraceid file. This ensures that if perf or sysfs read this before enabling trace, the value will be the one used for the trace session.

Trace ID initialisation is removed from the _probe() function.

Signed-off-by: Mike Leach mike.leach@linaro.org

.../coresight/coresight-etm4x-core.c | 63 +++++++++++++++++-- .../coresight/coresight-etm4x-sysfs.c | 32 +++++++++- drivers/hwtracing/coresight/coresight-etm4x.h | 3 + 3 files changed, 89 insertions(+), 9 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 7f416a12000e..aa7ea5ad8b06 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -42,6 +42,7 @@ #include "coresight-etm4x-cfg.h" #include "coresight-self-hosted-trace.h" #include "coresight-syscfg.h" +#include "coresight-trace-id.h" static int boot_enable; module_param(boot_enable, int, 0444); @@ -234,6 +235,36 @@ static int etm4_trace_id(struct coresight_device *csdev) return drvdata->trcid; } +int etm4_read_alloc_trace_id(struct etmv4_drvdata *drvdata) +{

• int trace_id;
• /*

• * This will allocate a trace ID to the cpu,
  

• * or return the one currently allocated.
  

• */
  

• trace_id = coresight_trace_id_get_cpu_id(drvdata->cpu,

• 				 coresight_get_trace_id_map());
  

• if (trace_id > 0) {

• spin_lock(&drvdata->spinlock);
  

• drvdata->trcid = (u8)trace_id;
  

• spin_unlock(&drvdata->spinlock);
  

• } else {

• pr_err("Failed to allocate trace ID for %s on CPU%d\n",
  

•        dev_name(&drvdata->csdev->dev), drvdata->cpu);
  

• }
• return trace_id;

+}

+void etm4_release_trace_id(struct etmv4_drvdata *drvdata) +{

• coresight_trace_id_put_cpu_id(drvdata->cpu,

• 		      coresight_get_trace_id_map());
  

• spin_lock(&drvdata->spinlock);
• drvdata->trcid = 0;
• spin_unlock(&drvdata->spinlock);

+}

struct etm4_enable_arg { struct etmv4_drvdata *drvdata; int rc; @@ -717,9 +748,18 @@ static int etm4_enable_perf(struct coresight_device *csdev, ret = etm4_parse_event_config(csdev, event); if (ret) goto out;

• /* allocate a trace ID */
• ret = etm4_read_alloc_trace_id(drvdata);
• if (ret < 0)

• goto out;
  

• /* And enable it */ ret = etm4_enable_hw(drvdata);

• /* failed to enable */
• if (ret)

• etm4_release_trace_id(drvdata);
  

out: return ret; } @@ -739,6 +779,11 @@ static int etm4_enable_sysfs(struct coresight_device *csdev) return ret; }

• /* allocate a trace ID */
• ret = etm4_read_alloc_trace_id(drvdata);
• if (ret < 0)

• return ret;
  

• spin_lock(&drvdata->spinlock);

/* @@ -756,6 +801,8 @@ static int etm4_enable_sysfs(struct coresight_device *csdev) if (!ret) dev_dbg(&csdev->dev, "ETM tracing enabled\n");

• else

• etm4_release_trace_id(drvdata);
  

  return ret;

} @@ -883,6 +930,9 @@ static int etm4_disable_perf(struct coresight_device *csdev, /* TRCVICTLR::SSSTATUS, bit[9] */ filters->ssstatus = (control & BIT(9));

• /* release trace ID - this may pend release if perf session is still active */
• etm4_release_trace_id(drvdata);
• return 0;

} @@ -908,6 +958,13 @@ static void etm4_disable_sysfs(struct coresight_device *csdev) spin_unlock(&drvdata->spinlock); cpus_read_unlock();

• /*

• * unlike for perf session - we only release trace IDs when resetting
  

• * sysfs. This permits sysfs users to read the trace ID after the trace
  

• * session has completed. This maintains operational behaviour with
  

• * prior trace id allocation method
  

• */
  

• dev_dbg(&csdev->dev, "ETM tracing disabled\n");

} @@ -1596,11 +1653,6 @@ static int etm4_dying_cpu(unsigned int cpu) return 0; } -static void etm4_init_trace_id(struct etmv4_drvdata *drvdata) -{

• drvdata->trcid = coresight_get_trace_id(drvdata->cpu);

-}

static int __etm4_cpu_save(struct etmv4_drvdata *drvdata) { int i, ret = 0; @@ -2005,7 +2057,6 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid) if (!desc.name) return -ENOMEM;

• etm4_init_trace_id(drvdata); etm4_set_default(&drvdata->config);

pdata = coresight_get_platform_data(dev); diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c index 21687cc1e4e2..bb69a203b833 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c @@ -266,10 +266,11 @@ static ssize_t reset_store(struct device *dev, config->vmid_mask0 = 0x0; config->vmid_mask1 = 0x0;

• drvdata->trcid = drvdata->cpu + 1;

That was really broken... I'm surprised we never caught it before this patchset.

More comments to come tomorrow.

Thanks, Mathieu

spin_unlock(&drvdata->spinlock);

• /* for sysfs - only release trace id when resetting */
• etm4_release_trace_id(drvdata);
• cscfg_csdev_reset_feats(to_coresight_device(dev));

return size; @@ -2355,6 +2356,31 @@ static struct attribute *coresight_etmv4_attrs[] = { NULL, }; +/*

• • Trace ID allocated dynamically on enable - but also allocate on read
• • in case sysfs or perf read before enable to ensure consistent metadata
• • information for trace decode
• */

+static ssize_t trctraceid_show(struct device *dev,

• 	       struct device_attribute *attr,
  

• 	       char *buf)
  

+{

• int trace_id;
• struct etmv4_drvdata *drvdata = dev_get_drvdata(dev->parent);
• trace_id = etm4_read_alloc_trace_id(drvdata);
• if (trace_id < 0)

• return trace_id;
  

• return scnprintf(buf, PAGE_SIZE, "0x%x\n", trace_id);

+}

+/* mgmt group uses extended attributes - no standard macro available */ +static struct dev_ext_attribute dev_attr_trctraceid = {

• __ATTR(trctraceid, 0444, trctraceid_show, NULL),
  

• (void *)(unsigned long)TRCTRACEIDR
  

+};

struct etmv4_reg { struct coresight_device *csdev; u32 offset; @@ -2491,7 +2517,7 @@ static struct attribute *coresight_etmv4_mgmt_attrs[] = { coresight_etm4x_reg(trcpidr3, TRCPIDR3), coresight_etm4x_reg(trcoslsr, TRCOSLSR), coresight_etm4x_reg(trcconfig, TRCCONFIGR),

• coresight_etm4x_reg(trctraceid, TRCTRACEIDR),

• &dev_attr_trctraceid.attr.attr, coresight_etm4x_reg(trcdevarch, TRCDEVARCH), NULL,

}; diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h index 3c4d69b096ca..64976a00c839 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.h +++ b/drivers/hwtracing/coresight/coresight-etm4x.h @@ -1010,4 +1010,7 @@ static inline bool etm4x_is_ete(struct etmv4_drvdata *drvdata) { return drvdata->arch >= ETM_ARCH_ETE; }

+int etm4_read_alloc_trace_id(struct etmv4_drvdata *drvdata); +void etm4_release_trace_id(struct etmv4_drvdata *drvdata);

#endif

2.17.1

On Tue, Mar 08, 2022 at 08:49:55PM +0000, Mike Leach wrote:

Use the TraceID API to allocate ETM trace IDs dynamically.

As with the etm4x we allocate on enable / disable for perf, allocate on enable / reset for sysfs.

Additionally we allocate on sysfs file read as both perf and sysfs can read the ID before enabling the hardware.

Remove sysfs option to write trace ID - which is inconsistent with both the dynamic allocation method and the fixed allocation method previously used.

Signed-off-by: Mike Leach mike.leach@linaro.org

drivers/hwtracing/coresight/coresight-etm.h | 2 + .../coresight/coresight-etm3x-core.c | 72 ++++++++++++++++--- .../coresight/coresight-etm3x-sysfs.c | 28 +++----- 3 files changed, 71 insertions(+), 31 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm.h b/drivers/hwtracing/coresight/coresight-etm.h index f3ab96eaf44e..3667428d38b6 100644 --- a/drivers/hwtracing/coresight/coresight-etm.h +++ b/drivers/hwtracing/coresight/coresight-etm.h @@ -287,4 +287,6 @@ int etm_get_trace_id(struct etm_drvdata *drvdata); void etm_set_default(struct etm_config *config); void etm_config_trace_mode(struct etm_config *config); struct etm_config *get_etm_config(struct etm_drvdata *drvdata); +int etm_read_alloc_trace_id(struct etm_drvdata *drvdata); +void etm_release_trace_id(struct etm_drvdata *drvdata); #endif diff --git a/drivers/hwtracing/coresight/coresight-etm3x-core.c b/drivers/hwtracing/coresight/coresight-etm3x-core.c index 7d413ba8b823..98213503bd09 100644 --- a/drivers/hwtracing/coresight/coresight-etm3x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c @@ -32,6 +32,7 @@ #include "coresight-etm.h" #include "coresight-etm-perf.h" +#include "coresight-trace-id.h" /*

• Not really modular but using module_param is the easiest way to

@@ -490,18 +491,57 @@ static int etm_trace_id(struct coresight_device *csdev) return etm_get_trace_id(drvdata); } +int etm_read_alloc_trace_id(struct etm_drvdata *drvdata) +{

• int trace_id;
• /*

• * This will allocate a trace ID to the cpu,
  

• * or return the one currently allocated.
  

• */
  

• trace_id = coresight_trace_id_get_cpu_id(drvdata->cpu,

• 				 coresight_get_trace_id_map());
  

• if (trace_id > 0) {

• spin_lock(&drvdata->spinlock);
  

• drvdata->traceid = (u8)trace_id;
  

• spin_unlock(&drvdata->spinlock);
  

• } else {

• pr_err("Failed to allocate trace ID for %s on CPU%d\n",
  

•        dev_name(&drvdata->csdev->dev), drvdata->cpu);
  

• }
• return trace_id;

+}

+void etm_release_trace_id(struct etm_drvdata *drvdata) +{

• coresight_trace_id_put_cpu_id(drvdata->cpu,

• 		      coresight_get_trace_id_map());
  

• spin_lock(&drvdata->spinlock);
• drvdata->traceid = 0;
• spin_unlock(&drvdata->spinlock);

+}

static int etm_enable_perf(struct coresight_device *csdev, struct perf_event *event) { struct etm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);

• int ret;

if (WARN_ON_ONCE(drvdata->cpu != smp_processor_id())) return -EINVAL;

• ret = etm_read_alloc_trace_id(drvdata);
• if (ret < 0)

• return ret;
  
  

For etm4x, this was done after etm_parse_event_config(). Please do the same here.

/* Configure the tracer based on the session's specifics */ etm_parse_event_config(drvdata, event); /* And enable it */

• return etm_enable_hw(drvdata);

• ret = etm_enable_hw(drvdata);
• if (ret)

• etm_release_trace_id(drvdata);
  

• return ret;

} static int etm_enable_sysfs(struct coresight_device *csdev) @@ -510,6 +550,10 @@ static int etm_enable_sysfs(struct coresight_device *csdev) struct etm_enable_arg arg = { }; int ret;

• ret = etm_read_alloc_trace_id(drvdata);
• if (ret < 0)

• return ret;
  

Please add the same comment as in etm4_disable_sysfs() in etm_disable_sysfs().

spin_lock(&drvdata->spinlock); /* @@ -532,6 +576,8 @@ static int etm_enable_sysfs(struct coresight_device *csdev) if (!ret) dev_dbg(&csdev->dev, "ETM tracing enabled\n");

• else

• etm_release_trace_id(drvdata);
  

  return ret;

} @@ -611,6 +657,8 @@ static void etm_disable_perf(struct coresight_device *csdev) coresight_disclaim_device_unlocked(csdev); CS_LOCK(drvdata->base);

• etm_release_trace_id(drvdata);

} static void etm_disable_sysfs(struct coresight_device *csdev) @@ -781,11 +829,6 @@ static void etm_init_arch_data(void *info) CS_LOCK(drvdata->base); } -static void etm_init_trace_id(struct etm_drvdata *drvdata) -{

• drvdata->traceid = coresight_get_trace_id(drvdata->cpu);

-}

static int __init etm_hp_setup(void) { int ret; @@ -871,7 +914,6 @@ static int etm_probe(struct amba_device *adev, const struct amba_id *id) if (etm_arch_supported(drvdata->arch) == false) return -EINVAL;

• etm_init_trace_id(drvdata); etm_set_default(&drvdata->config);

pdata = coresight_get_platform_data(dev); @@ -891,10 +933,12 @@ static int etm_probe(struct amba_device *adev, const struct amba_id *id) return PTR_ERR(drvdata->csdev); ret = etm_perf_symlink(drvdata->csdev, true);

• if (ret) {

• coresight_unregister(drvdata->csdev);
  

• return ret;
  

• }

• if (ret)

• goto cs_unregister;
  

• ret = etm_read_alloc_trace_id(drvdata);
• if (ret < 0)

• goto cs_unregister;
  

etmdrvdata[drvdata->cpu] = drvdata; @@ -907,6 +951,10 @@ static int etm_probe(struct amba_device *adev, const struct amba_id *id) } return 0;

+cs_unregister:

• coresight_unregister(drvdata->csdev);
• return ret;

} static void clear_etmdrvdata(void *info) @@ -922,6 +970,8 @@ static void etm_remove(struct amba_device *adev) etm_perf_symlink(drvdata->csdev, false);

• etm_release_trace_id(drvdata);
• /*
  • Taking hotplug lock here to avoid racing between etm_remove and
  • CPU hotplug call backs.

diff --git a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c index e8c7649f123e..3ee70b174240 100644 --- a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c @@ -86,6 +86,8 @@ static ssize_t reset_store(struct device *dev, etm_set_default(config); spin_unlock(&drvdata->spinlock);

• /* release trace id outside the spinlock as this fn uses it */
  

• etm_release_trace_id(drvdata);
  

  }

return size; @@ -1189,30 +1191,16 @@ static DEVICE_ATTR_RO(cpu); static ssize_t traceid_show(struct device *dev, struct device_attribute *attr, char *buf) {

• unsigned long val;
• struct etm_drvdata *drvdata = dev_get_drvdata(dev->parent);
• val = etm_get_trace_id(drvdata);
• return sprintf(buf, "%#lx\n", val);

-}

-static ssize_t traceid_store(struct device *dev,

• 	     struct device_attribute *attr,
  

• 	     const char *buf, size_t size)
  

-{

• int ret;
• unsigned long val;

• int trace_id; struct etm_drvdata *drvdata = dev_get_drvdata(dev->parent);

• ret = kstrtoul(buf, 16, &val);
• if (ret)

• return ret;
  

• trace_id = etm_read_alloc_trace_id(drvdata);
• if (trace_id < 0)

• return trace_id;
  

• drvdata->traceid = val & ETM_TRACEID_MASK;
• return size;

• return sprintf(buf, "%#x\n", trace_id);

} -static DEVICE_ATTR_RW(traceid); +static DEVICE_ATTR_RO(traceid); static struct attribute *coresight_etm_attrs[] = { &dev_attr_nr_addr_cmp.attr, -- 2.17.1

Hi Mathieu,

On Wed, 6 Apr 2022 at 18:11, Mathieu Poirier mathieu.poirier@linaro.org wrote:

On Tue, Mar 08, 2022 at 08:49:56PM +0000, Mike Leach wrote:

...

Adds in notifier calls to the trace ID allocator that perf events are starting and stopping.

This ensures that Trace IDs associated with CPUs remain the same throughout the perf session, and are only release when all perf sessions are complete.

Signed-off-by: Mike Leach mike.leach@linaro.org

drivers/hwtracing/coresight/coresight-etm-perf.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index c039b6ae206f..008f9dac429d 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -22,6 +22,7 @@ #include "coresight-etm-perf.h" #include "coresight-priv.h" #include "coresight-syscfg.h" +#include "coresight-trace-id.h"

static struct pmu etm_pmu; static bool etm_perf_up; @@ -223,11 +224,21 @@ static void free_event_data(struct work_struct *work) struct list_head **ppath;

          ppath = etm_event_cpu_path_ptr(event_data, cpu);

•         if (!(IS_ERR_OR_NULL(*ppath)))
  

•         if (!(IS_ERR_OR_NULL(*ppath))) {
                  coresight_release_path(*ppath);
  

•                 /*
  

•                  * perf may have read a trace id for a cpu, but never actually
  

•                  * executed code on that cpu - which means the trace id would
  

•                  * not release on disable. Re-release here to be sure.
  

•                  */
  

•                 coresight_trace_id_put_cpu_id(cpu, coresight_get_trace_id_map());
  

A CPU gets a traceID in event_etm_start() when the event is installed for running. Do you see a scenario where etm_free_aux() is called without previously calling event_etm_stop()?

To ensure that IDs are obtained in a timely manner, they assign on read. Therefore when cs_etm.c::cs_etm_info_fill() is called, potentially the ID will be read for all CPUs - and dumped into the AUXINFO data at the top of the perf.data file. However, a --per-thread execution may actually only start the event on a subset of cpus, hence we ensure that all cpus are released. This was proven during testing when I ran both --per-thread and cpu wide tests with logging monitoring the ID assignments.

I have programmed this deliberately defensively - on the basis that the timings and orderings of the code/callbacks in todays perf may not necessarily be the same in tomorrows. perf.

In future we may be able to use Suzuki's idea of embedding the ID into an alternative packet in the perf.data file. but I think that should be a separate change as it affects decode in a big way.

Regards

Mike

...

•         }
          *ppath = NULL;
  }
  
  

• /* mark perf event as done for trace id allocator */
  

• coresight_trace_id_perf_stop();
  

• free_percpu(event_data->path);
  kfree(event_data);
  

} @@ -314,6 +325,9 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, sink = user_sink = coresight_get_sink_by_id(id); }

• /* tell the trace ID allocator that a perf event is starting up */
  

• coresight_trace_id_perf_start();
  

• /* check if user wants a coresight configuration selected */
  cfg_hash = (u32)((event->attr.config2 & GENMASK_ULL(63, 32)) >> 32);
  if (cfg_hash) {
  

-- 2.17.1

Hi Mike,

On 2022/3/9 4:49, Mike Leach wrote:

This static 'cpu * 2 + seed' was outdated and broken for systems with high core counts (>46).

This has been replaced by a dynamic allocation system.

Signed-off-by: Mike Leach mike.leach@linaro.org

include/linux/coresight-pmu.h | 12 ------------ 1 file changed, 12 deletions(-)

Seems coresight_get_trace_id() in tools/include/linux/coresight-pmu.h need to be deleted too.

Thanks, Qi

diff --git a/include/linux/coresight-pmu.h b/include/linux/coresight-pmu.h index 4ac5c081af93..bb4eb4de3c77 100644 --- a/include/linux/coresight-pmu.h +++ b/include/linux/coresight-pmu.h @@ -8,7 +8,6 @@ #define _LINUX_CORESIGHT_PMU_H #define CORESIGHT_ETM_PMU_NAME "cs_etm" -#define CORESIGHT_ETM_PMU_SEED 0x10 /*

• Below are the definition of bit offsets for perf option, and works as

@@ -32,15 +31,4 @@ #define ETM4_CFG_BIT_RETSTK 12 #define ETM4_CFG_BIT_VMID_OPT 15 -static inline int coresight_get_trace_id(int cpu) -{

• /*

• * A trace ID of value 0 is invalid, so let's start at some
  

• * random value that fits in 7 bits and go from there.  Since
  

• * the common convention is to have data trace IDs be I(N) + 1,
  

• * set instruction trace IDs as a function of the CPU number.
  

• */
  

• return (CORESIGHT_ETM_PMU_SEED + (cpu * 2));

-}

• #endif