| Message ID | 20240712102029.3697965-7-james.clark@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | coresight: Use per-sink trace ID maps for Perf sessions | expand |
On Fri, 12 Jul 2024 at 11:22, James Clark <james.clark@linaro.org> wrote: > > From: James Clark <james.clark@arm.com> > > v0.1 HW_ID packets have a new field that describes which sink each CPU > writes to. Use the sink ID to link trace ID maps to each other so that > mappings are shared wherever the sink is shared. > > Also update the error message to show that overlapping IDs aren't an > error in per-thread mode, just not supported. In the future we can > use the CPU ID from the AUX records, or watch for changing sink IDs on > HW_ID packets to use the correct decoders. > > Signed-off-by: James Clark <james.clark@arm.com> > Signed-off-by: James Clark <james.clark@linaro.org> > --- > tools/include/linux/coresight-pmu.h | 17 +++-- > tools/perf/util/cs-etm.c | 100 +++++++++++++++++++++++++--- > 2 files changed, 103 insertions(+), 14 deletions(-) > > diff --git a/tools/include/linux/coresight-pmu.h b/tools/include/linux/coresight-pmu.h > index 51ac441a37c3..89b0ac0014b0 100644 > --- a/tools/include/linux/coresight-pmu.h > +++ b/tools/include/linux/coresight-pmu.h > @@ -49,12 +49,21 @@ > * Interpretation of the PERF_RECORD_AUX_OUTPUT_HW_ID payload. > * Used to associate a CPU with the CoreSight Trace ID. > * [07:00] - Trace ID - uses 8 bits to make value easy to read in file. > - * [59:08] - Unused (SBZ) > - * [63:60] - Version > + * [39:08] - Sink ID - as reported in /sys/bus/event_source/devices/cs_etm/sinks/ > + * Added in minor version 1. > + * [55:40] - Unused (SBZ) > + * [59:56] - Minor Version - previously existing fields are compatible with > + * all minor versions. > + * [63:60] - Major Version - previously existing fields mean different things > + * in new major versions. > */ > #define CS_AUX_HW_ID_TRACE_ID_MASK GENMASK_ULL(7, 0) > -#define CS_AUX_HW_ID_VERSION_MASK GENMASK_ULL(63, 60) > +#define CS_AUX_HW_ID_SINK_ID_MASK GENMASK_ULL(39, 8) > > -#define CS_AUX_HW_ID_CURR_VERSION 0 > +#define CS_AUX_HW_ID_MINOR_VERSION_MASK GENMASK_ULL(59, 56) > +#define CS_AUX_HW_ID_MAJOR_VERSION_MASK GENMASK_ULL(63, 60) > + > +#define CS_AUX_HW_ID_MAJOR_VERSION 0 > +#define CS_AUX_HW_ID_MINOR_VERSION 1 > > #endif > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c > index 954a6f7bedf3..87e983da19be 100644 > --- a/tools/perf/util/cs-etm.c > +++ b/tools/perf/util/cs-etm.c > @@ -118,6 +118,12 @@ struct cs_etm_queue { > struct cs_etm_traceid_queue **traceid_queues; > /* Conversion between traceID and metadata pointers */ > struct intlist *traceid_list; > + /* > + * Same as traceid_list, but traceid_list may be a reference to another > + * queue's which has a matching sink ID. > + */ > + struct intlist *own_traceid_list; > + u32 sink_id; > }; > > static int cs_etm__process_timestamped_queues(struct cs_etm_auxtrace *etm); > @@ -142,6 +148,7 @@ static int cs_etm__metadata_set_trace_id(u8 trace_chan_id, u64 *cpu_metadata); > (queue_nr << 16 | trace_chan_id) > #define TO_QUEUE_NR(cs_queue_nr) (cs_queue_nr >> 16) > #define TO_TRACE_CHAN_ID(cs_queue_nr) (cs_queue_nr & 0x0000ffff) > +#define SINK_UNSET ((u32) -1) > > static u32 cs_etm__get_v7_protocol_version(u32 etmidr) > { > @@ -241,7 +248,16 @@ static int cs_etm__insert_trace_id_node(struct cs_etm_queue *etmq, > int err; > > if (curr_cpu_data[CS_ETM_CPU] != cpu_metadata[CS_ETM_CPU]) { > - pr_err("CS_ETM: map mismatch between HW_ID packet CPU and Trace ID\n"); > + /* > + * With > CORESIGHT_TRACE_IDS_MAX ETMs, overlapping IDs > + * are expected (but not supported) in per-thread mode, > + * rather than signifying an error. > + */ > + if (etmq->etm->per_thread_decoding) > + pr_err("CS_ETM: overlapping Trace IDs aren't currently supported in per-thread mode\n"); > + else > + pr_err("CS_ETM: map mismatch between HW_ID packet CPU and Trace ID\n"); > + > return -EINVAL; > } > > @@ -326,6 +342,64 @@ static int cs_etm__process_trace_id_v0(struct cs_etm_auxtrace *etm, int cpu, > return cs_etm__metadata_set_trace_id(trace_chan_id, cpu_data); > } > > +static int cs_etm__process_trace_id_v0_1(struct cs_etm_auxtrace *etm, int cpu, > + u64 hw_id) > +{ > + struct cs_etm_queue *etmq = cs_etm__get_queue(etm, cpu); > + int ret; > + u64 *cpu_data; > + u32 sink_id = FIELD_GET(CS_AUX_HW_ID_SINK_ID_MASK, hw_id); > + u8 trace_id = FIELD_GET(CS_AUX_HW_ID_TRACE_ID_MASK, hw_id); > + > + /* > + * Check sink id hasn't changed in per-cpu mode. In per-thread mode, > + * let it pass for now until an actual overlapping trace ID is hit. In > + * most cases IDs won't overlap even if the sink changes. > + */ > + if (!etmq->etm->per_thread_decoding && etmq->sink_id != SINK_UNSET && > + etmq->sink_id != sink_id) { > + pr_err("CS_ETM: mismatch between sink IDs\n"); > + return -EINVAL; > + } > + > + etmq->sink_id = sink_id; > + > + /* Find which other queues use this sink and link their ID maps */ > + for (unsigned int i = 0; i < etm->queues.nr_queues; ++i) { > + struct cs_etm_queue *other_etmq = etm->queues.queue_array[i].priv; > + > + /* Different sinks, skip */ > + if (other_etmq->sink_id != etmq->sink_id) > + continue; > + > + /* Already linked, skip */ > + if (other_etmq->traceid_list == etmq->traceid_list) > + continue; > + > + /* At the point of first linking, this one should be empty */ > + if (!intlist__empty(etmq->traceid_list)) { > + pr_err("CS_ETM: Can't link populated trace ID lists\n"); > + return -EINVAL; > + } > + > + etmq->own_traceid_list = NULL; > + intlist__delete(etmq->traceid_list); > + etmq->traceid_list = other_etmq->traceid_list; > + break; > + } > + > + cpu_data = get_cpu_data(etm, cpu); > + ret = cs_etm__insert_trace_id_node(etmq, trace_id, cpu_data); > + if (ret) > + return ret; > + > + ret = cs_etm__metadata_set_trace_id(trace_id, cpu_data); > + if (ret) > + return ret; > + > + return 0; > +} > + > static int cs_etm__metadata_get_trace_id(u8 *trace_chan_id, u64 *cpu_metadata) > { > u64 cs_etm_magic = cpu_metadata[CS_ETM_MAGIC]; > @@ -414,10 +488,10 @@ static int cs_etm__process_aux_output_hw_id(struct perf_session *session, > > /* extract and parse the HW ID */ > hw_id = event->aux_output_hw_id.hw_id; > - version = FIELD_GET(CS_AUX_HW_ID_VERSION_MASK, hw_id); > + version = FIELD_GET(CS_AUX_HW_ID_MAJOR_VERSION_MASK, hw_id); > > /* check that we can handle this version */ > - if (version > CS_AUX_HW_ID_CURR_VERSION) { > + if (version > CS_AUX_HW_ID_MAJOR_VERSION) { > pr_err("CS ETM Trace: PERF_RECORD_AUX_OUTPUT_HW_ID version %d not supported. Please update Perf.\n", > version); > return -EINVAL; > @@ -442,7 +516,10 @@ static int cs_etm__process_aux_output_hw_id(struct perf_session *session, > return -EINVAL; > } > > - return cs_etm__process_trace_id_v0(etm, cpu, hw_id); Perhaps leave this as the final statement of the function > + if (FIELD_GET(CS_AUX_HW_ID_MINOR_VERSION_MASK, hw_id) == 0) this could be moved before and be if (FIELD_GET(CS_AUX_HW_ID_MINOR_VERSION_MASK, hw_id) == 1) return cs_etm__process_trace_id_v0_1(etm, cpu, hw_id); > + return cs_etm__process_trace_id_v0(etm, cpu, hw_id); > + else > + return cs_etm__process_trace_id_v0_1(etm, cpu, hw_id); > } > > void cs_etm__etmq_set_traceid_queue_timestamp(struct cs_etm_queue *etmq, > @@ -882,12 +959,14 @@ static void cs_etm__free_queue(void *priv) > cs_etm_decoder__free(etmq->decoder); > cs_etm__free_traceid_queues(etmq); > > - /* First remove all traceID/metadata nodes for the RB tree */ > - intlist__for_each_entry_safe(inode, tmp, etmq->traceid_list) > - intlist__remove(etmq->traceid_list, inode); > + if (etmq->own_traceid_list) { > + /* First remove all traceID/metadata nodes for the RB tree */ > + intlist__for_each_entry_safe(inode, tmp, etmq->own_traceid_list) > + intlist__remove(etmq->own_traceid_list, inode); > > - /* Then the RB tree itself */ > - intlist__delete(etmq->traceid_list); > + /* Then the RB tree itself */ > + intlist__delete(etmq->own_traceid_list); > + } > > free(etmq); > } > @@ -1081,7 +1160,7 @@ static struct cs_etm_queue *cs_etm__alloc_queue(void) > * has to be made for each packet that gets decoded, optimizing access > * in anything other than a sequential array is worth doing. > */ > - etmq->traceid_list = intlist__new(NULL); > + etmq->traceid_list = etmq->own_traceid_list = intlist__new(NULL); > if (!etmq->traceid_list) > goto out_free; > > @@ -1113,6 +1192,7 @@ static int cs_etm__setup_queue(struct cs_etm_auxtrace *etm, > etmq->queue_nr = queue_nr; > queue->cpu = queue_nr; /* Placeholder, may be reset to -1 in per-thread mode */ > etmq->offset = 0; > + etmq->sink_id = SINK_UNSET; > > return 0; > } > -- > 2.34.1 > Reviewed-by: Mike Leach <mike.leach@linaro.org> -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK
On 18/07/2024 2:24 pm, Mike Leach wrote: > On Fri, 12 Jul 2024 at 11:22, James Clark <james.clark@linaro.org> wrote: >> >> From: James Clark <james.clark@arm.com> >> >> v0.1 HW_ID packets have a new field that describes which sink each CPU >> writes to. Use the sink ID to link trace ID maps to each other so that >> mappings are shared wherever the sink is shared. >> >> Also update the error message to show that overlapping IDs aren't an >> error in per-thread mode, just not supported. In the future we can >> use the CPU ID from the AUX records, or watch for changing sink IDs on >> HW_ID packets to use the correct decoders. >> >> Signed-off-by: James Clark <james.clark@arm.com> >> Signed-off-by: James Clark <james.clark@linaro.org> >> --- >> tools/include/linux/coresight-pmu.h | 17 +++-- >> tools/perf/util/cs-etm.c | 100 +++++++++++++++++++++++++--- >> 2 files changed, 103 insertions(+), 14 deletions(-) >> >> diff --git a/tools/include/linux/coresight-pmu.h b/tools/include/linux/coresight-pmu.h >> index 51ac441a37c3..89b0ac0014b0 100644 >> --- a/tools/include/linux/coresight-pmu.h >> +++ b/tools/include/linux/coresight-pmu.h >> @@ -49,12 +49,21 @@ >> * Interpretation of the PERF_RECORD_AUX_OUTPUT_HW_ID payload. >> * Used to associate a CPU with the CoreSight Trace ID. >> * [07:00] - Trace ID - uses 8 bits to make value easy to read in file. >> - * [59:08] - Unused (SBZ) >> - * [63:60] - Version >> + * [39:08] - Sink ID - as reported in /sys/bus/event_source/devices/cs_etm/sinks/ >> + * Added in minor version 1. >> + * [55:40] - Unused (SBZ) >> + * [59:56] - Minor Version - previously existing fields are compatible with >> + * all minor versions. >> + * [63:60] - Major Version - previously existing fields mean different things >> + * in new major versions. >> */ >> #define CS_AUX_HW_ID_TRACE_ID_MASK GENMASK_ULL(7, 0) >> -#define CS_AUX_HW_ID_VERSION_MASK GENMASK_ULL(63, 60) >> +#define CS_AUX_HW_ID_SINK_ID_MASK GENMASK_ULL(39, 8) >> >> -#define CS_AUX_HW_ID_CURR_VERSION 0 >> +#define CS_AUX_HW_ID_MINOR_VERSION_MASK GENMASK_ULL(59, 56) >> +#define CS_AUX_HW_ID_MAJOR_VERSION_MASK GENMASK_ULL(63, 60) >> + >> +#define CS_AUX_HW_ID_MAJOR_VERSION 0 >> +#define CS_AUX_HW_ID_MINOR_VERSION 1 >> >> #endif >> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c >> index 954a6f7bedf3..87e983da19be 100644 >> --- a/tools/perf/util/cs-etm.c >> +++ b/tools/perf/util/cs-etm.c >> @@ -118,6 +118,12 @@ struct cs_etm_queue { >> struct cs_etm_traceid_queue **traceid_queues; >> /* Conversion between traceID and metadata pointers */ >> struct intlist *traceid_list; >> + /* >> + * Same as traceid_list, but traceid_list may be a reference to another >> + * queue's which has a matching sink ID. >> + */ >> + struct intlist *own_traceid_list; >> + u32 sink_id; >> }; >> >> static int cs_etm__process_timestamped_queues(struct cs_etm_auxtrace *etm); >> @@ -142,6 +148,7 @@ static int cs_etm__metadata_set_trace_id(u8 trace_chan_id, u64 *cpu_metadata); >> (queue_nr << 16 | trace_chan_id) >> #define TO_QUEUE_NR(cs_queue_nr) (cs_queue_nr >> 16) >> #define TO_TRACE_CHAN_ID(cs_queue_nr) (cs_queue_nr & 0x0000ffff) >> +#define SINK_UNSET ((u32) -1) >> >> static u32 cs_etm__get_v7_protocol_version(u32 etmidr) >> { >> @@ -241,7 +248,16 @@ static int cs_etm__insert_trace_id_node(struct cs_etm_queue *etmq, >> int err; >> >> if (curr_cpu_data[CS_ETM_CPU] != cpu_metadata[CS_ETM_CPU]) { >> - pr_err("CS_ETM: map mismatch between HW_ID packet CPU and Trace ID\n"); >> + /* >> + * With > CORESIGHT_TRACE_IDS_MAX ETMs, overlapping IDs >> + * are expected (but not supported) in per-thread mode, >> + * rather than signifying an error. >> + */ >> + if (etmq->etm->per_thread_decoding) >> + pr_err("CS_ETM: overlapping Trace IDs aren't currently supported in per-thread mode\n"); >> + else >> + pr_err("CS_ETM: map mismatch between HW_ID packet CPU and Trace ID\n"); >> + >> return -EINVAL; >> } >> >> @@ -326,6 +342,64 @@ static int cs_etm__process_trace_id_v0(struct cs_etm_auxtrace *etm, int cpu, >> return cs_etm__metadata_set_trace_id(trace_chan_id, cpu_data); >> } >> >> +static int cs_etm__process_trace_id_v0_1(struct cs_etm_auxtrace *etm, int cpu, >> + u64 hw_id) >> +{ >> + struct cs_etm_queue *etmq = cs_etm__get_queue(etm, cpu); >> + int ret; >> + u64 *cpu_data; >> + u32 sink_id = FIELD_GET(CS_AUX_HW_ID_SINK_ID_MASK, hw_id); >> + u8 trace_id = FIELD_GET(CS_AUX_HW_ID_TRACE_ID_MASK, hw_id); >> + >> + /* >> + * Check sink id hasn't changed in per-cpu mode. In per-thread mode, >> + * let it pass for now until an actual overlapping trace ID is hit. In >> + * most cases IDs won't overlap even if the sink changes. >> + */ >> + if (!etmq->etm->per_thread_decoding && etmq->sink_id != SINK_UNSET && >> + etmq->sink_id != sink_id) { >> + pr_err("CS_ETM: mismatch between sink IDs\n"); >> + return -EINVAL; >> + } >> + >> + etmq->sink_id = sink_id; >> + >> + /* Find which other queues use this sink and link their ID maps */ >> + for (unsigned int i = 0; i < etm->queues.nr_queues; ++i) { >> + struct cs_etm_queue *other_etmq = etm->queues.queue_array[i].priv; >> + >> + /* Different sinks, skip */ >> + if (other_etmq->sink_id != etmq->sink_id) >> + continue; >> + >> + /* Already linked, skip */ >> + if (other_etmq->traceid_list == etmq->traceid_list) >> + continue; >> + >> + /* At the point of first linking, this one should be empty */ >> + if (!intlist__empty(etmq->traceid_list)) { >> + pr_err("CS_ETM: Can't link populated trace ID lists\n"); >> + return -EINVAL; >> + } >> + >> + etmq->own_traceid_list = NULL; >> + intlist__delete(etmq->traceid_list); >> + etmq->traceid_list = other_etmq->traceid_list; >> + break; >> + } >> + >> + cpu_data = get_cpu_data(etm, cpu); >> + ret = cs_etm__insert_trace_id_node(etmq, trace_id, cpu_data); >> + if (ret) >> + return ret; >> + >> + ret = cs_etm__metadata_set_trace_id(trace_id, cpu_data); >> + if (ret) >> + return ret; >> + >> + return 0; >> +} >> + >> static int cs_etm__metadata_get_trace_id(u8 *trace_chan_id, u64 *cpu_metadata) >> { >> u64 cs_etm_magic = cpu_metadata[CS_ETM_MAGIC]; >> @@ -414,10 +488,10 @@ static int cs_etm__process_aux_output_hw_id(struct perf_session *session, >> >> /* extract and parse the HW ID */ >> hw_id = event->aux_output_hw_id.hw_id; >> - version = FIELD_GET(CS_AUX_HW_ID_VERSION_MASK, hw_id); >> + version = FIELD_GET(CS_AUX_HW_ID_MAJOR_VERSION_MASK, hw_id); >> >> /* check that we can handle this version */ >> - if (version > CS_AUX_HW_ID_CURR_VERSION) { >> + if (version > CS_AUX_HW_ID_MAJOR_VERSION) { >> pr_err("CS ETM Trace: PERF_RECORD_AUX_OUTPUT_HW_ID version %d not supported. Please update Perf.\n", >> version); >> return -EINVAL; >> @@ -442,7 +516,10 @@ static int cs_etm__process_aux_output_hw_id(struct perf_session *session, >> return -EINVAL; >> } >> >> - return cs_etm__process_trace_id_v0(etm, cpu, hw_id); > > Perhaps leave this as the final statement of the function > >> + if (FIELD_GET(CS_AUX_HW_ID_MINOR_VERSION_MASK, hw_id) == 0) > > this could be moved before and be > > if (FIELD_GET(CS_AUX_HW_ID_MINOR_VERSION_MASK, hw_id) == 1) > return cs_etm__process_trace_id_v0_1(etm, cpu, hw_id); > > Because I was intending minor version changes to be backwards compatible I have it so that any value other than 0 is treated as v0.1. Otherwise version updates will break old versions of Perf. And then if we added a v0.3 it would look like this: if (FIELD_GET(CS_AUX_HW_ID_MINOR_VERSION_MASK, hw_id) == 0) return cs_etm__process_trace_id_v0(etm, cpu, hw_id); else if (FIELD_GET(CS_AUX_HW_ID_MINOR_VERSION_MASK, hw_id) == 1) return cs_etm__process_trace_id_v0_1(etm, cpu, hw_id); else return cs_etm__process_trace_id_v0_2(etm, cpu, hw_id); Based on that I'm not sure if you still think it should be changed?
On 19/07/2024 11:48 am, James Clark wrote: > > > On 18/07/2024 2:24 pm, Mike Leach wrote: >> On Fri, 12 Jul 2024 at 11:22, James Clark <james.clark@linaro.org> wrote: >>> >>> From: James Clark <james.clark@arm.com> >>> >>> v0.1 HW_ID packets have a new field that describes which sink each CPU >>> writes to. Use the sink ID to link trace ID maps to each other so that >>> mappings are shared wherever the sink is shared. >>> >>> Also update the error message to show that overlapping IDs aren't an >>> error in per-thread mode, just not supported. In the future we can >>> use the CPU ID from the AUX records, or watch for changing sink IDs on >>> HW_ID packets to use the correct decoders. >>> >>> Signed-off-by: James Clark <james.clark@arm.com> >>> Signed-off-by: James Clark <james.clark@linaro.org> >>> --- >>> tools/include/linux/coresight-pmu.h | 17 +++-- >>> tools/perf/util/cs-etm.c | 100 +++++++++++++++++++++++++--- >>> 2 files changed, 103 insertions(+), 14 deletions(-) >>> >>> diff --git a/tools/include/linux/coresight-pmu.h >>> b/tools/include/linux/coresight-pmu.h >>> index 51ac441a37c3..89b0ac0014b0 100644 >>> --- a/tools/include/linux/coresight-pmu.h >>> +++ b/tools/include/linux/coresight-pmu.h >>> @@ -49,12 +49,21 @@ >>> * Interpretation of the PERF_RECORD_AUX_OUTPUT_HW_ID payload. >>> * Used to associate a CPU with the CoreSight Trace ID. >>> * [07:00] - Trace ID - uses 8 bits to make value easy to read in >>> file. >>> - * [59:08] - Unused (SBZ) >>> - * [63:60] - Version >>> + * [39:08] - Sink ID - as reported in >>> /sys/bus/event_source/devices/cs_etm/sinks/ >>> + * Added in minor version 1. >>> + * [55:40] - Unused (SBZ) >>> + * [59:56] - Minor Version - previously existing fields are >>> compatible with >>> + * all minor versions. >>> + * [63:60] - Major Version - previously existing fields mean >>> different things >>> + * in new major versions. >>> */ >>> #define CS_AUX_HW_ID_TRACE_ID_MASK GENMASK_ULL(7, 0) >>> -#define CS_AUX_HW_ID_VERSION_MASK GENMASK_ULL(63, 60) >>> +#define CS_AUX_HW_ID_SINK_ID_MASK GENMASK_ULL(39, 8) >>> >>> -#define CS_AUX_HW_ID_CURR_VERSION 0 >>> +#define CS_AUX_HW_ID_MINOR_VERSION_MASK GENMASK_ULL(59, 56) >>> +#define CS_AUX_HW_ID_MAJOR_VERSION_MASK GENMASK_ULL(63, 60) >>> + >>> +#define CS_AUX_HW_ID_MAJOR_VERSION 0 >>> +#define CS_AUX_HW_ID_MINOR_VERSION 1 >>> >>> #endif >>> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c >>> index 954a6f7bedf3..87e983da19be 100644 >>> --- a/tools/perf/util/cs-etm.c >>> +++ b/tools/perf/util/cs-etm.c >>> @@ -118,6 +118,12 @@ struct cs_etm_queue { >>> struct cs_etm_traceid_queue **traceid_queues; >>> /* Conversion between traceID and metadata pointers */ >>> struct intlist *traceid_list; >>> + /* >>> + * Same as traceid_list, but traceid_list may be a reference >>> to another >>> + * queue's which has a matching sink ID. >>> + */ >>> + struct intlist *own_traceid_list; >>> + u32 sink_id; >>> }; >>> >>> static int cs_etm__process_timestamped_queues(struct >>> cs_etm_auxtrace *etm); >>> @@ -142,6 +148,7 @@ static int cs_etm__metadata_set_trace_id(u8 >>> trace_chan_id, u64 *cpu_metadata); >>> (queue_nr << 16 | trace_chan_id) >>> #define TO_QUEUE_NR(cs_queue_nr) (cs_queue_nr >> 16) >>> #define TO_TRACE_CHAN_ID(cs_queue_nr) (cs_queue_nr & 0x0000ffff) >>> +#define SINK_UNSET ((u32) -1) >>> >>> static u32 cs_etm__get_v7_protocol_version(u32 etmidr) >>> { >>> @@ -241,7 +248,16 @@ static int cs_etm__insert_trace_id_node(struct >>> cs_etm_queue *etmq, >>> int err; >>> >>> if (curr_cpu_data[CS_ETM_CPU] != >>> cpu_metadata[CS_ETM_CPU]) { >>> - pr_err("CS_ETM: map mismatch between HW_ID >>> packet CPU and Trace ID\n"); >>> + /* >>> + * With > CORESIGHT_TRACE_IDS_MAX ETMs, >>> overlapping IDs >>> + * are expected (but not supported) in >>> per-thread mode, >>> + * rather than signifying an error. >>> + */ >>> + if (etmq->etm->per_thread_decoding) >>> + pr_err("CS_ETM: overlapping Trace IDs >>> aren't currently supported in per-thread mode\n"); >>> + else >>> + pr_err("CS_ETM: map mismatch between >>> HW_ID packet CPU and Trace ID\n"); >>> + >>> return -EINVAL; >>> } >>> >>> @@ -326,6 +342,64 @@ static int cs_etm__process_trace_id_v0(struct >>> cs_etm_auxtrace *etm, int cpu, >>> return cs_etm__metadata_set_trace_id(trace_chan_id, cpu_data); >>> } >>> >>> +static int cs_etm__process_trace_id_v0_1(struct cs_etm_auxtrace >>> *etm, int cpu, >>> + u64 hw_id) >>> +{ >>> + struct cs_etm_queue *etmq = cs_etm__get_queue(etm, cpu); >>> + int ret; >>> + u64 *cpu_data; >>> + u32 sink_id = FIELD_GET(CS_AUX_HW_ID_SINK_ID_MASK, hw_id); >>> + u8 trace_id = FIELD_GET(CS_AUX_HW_ID_TRACE_ID_MASK, hw_id); >>> + >>> + /* >>> + * Check sink id hasn't changed in per-cpu mode. In >>> per-thread mode, >>> + * let it pass for now until an actual overlapping trace ID >>> is hit. In >>> + * most cases IDs won't overlap even if the sink changes. >>> + */ >>> + if (!etmq->etm->per_thread_decoding && etmq->sink_id != >>> SINK_UNSET && >>> + etmq->sink_id != sink_id) { >>> + pr_err("CS_ETM: mismatch between sink IDs\n"); >>> + return -EINVAL; >>> + } >>> + >>> + etmq->sink_id = sink_id; >>> + >>> + /* Find which other queues use this sink and link their ID >>> maps */ >>> + for (unsigned int i = 0; i < etm->queues.nr_queues; ++i) { >>> + struct cs_etm_queue *other_etmq = >>> etm->queues.queue_array[i].priv; >>> + >>> + /* Different sinks, skip */ >>> + if (other_etmq->sink_id != etmq->sink_id) >>> + continue; >>> + >>> + /* Already linked, skip */ >>> + if (other_etmq->traceid_list == etmq->traceid_list) >>> + continue; >>> + >>> + /* At the point of first linking, this one should be >>> empty */ >>> + if (!intlist__empty(etmq->traceid_list)) { >>> + pr_err("CS_ETM: Can't link populated trace ID >>> lists\n"); >>> + return -EINVAL; >>> + } >>> + >>> + etmq->own_traceid_list = NULL; >>> + intlist__delete(etmq->traceid_list); >>> + etmq->traceid_list = other_etmq->traceid_list; >>> + break; >>> + } >>> + >>> + cpu_data = get_cpu_data(etm, cpu); >>> + ret = cs_etm__insert_trace_id_node(etmq, trace_id, cpu_data); >>> + if (ret) >>> + return ret; >>> + >>> + ret = cs_etm__metadata_set_trace_id(trace_id, cpu_data); >>> + if (ret) >>> + return ret; >>> + >>> + return 0; >>> +} >>> + >>> static int cs_etm__metadata_get_trace_id(u8 *trace_chan_id, u64 >>> *cpu_metadata) >>> { >>> u64 cs_etm_magic = cpu_metadata[CS_ETM_MAGIC]; >>> @@ -414,10 +488,10 @@ static int >>> cs_etm__process_aux_output_hw_id(struct perf_session *session, >>> >>> /* extract and parse the HW ID */ >>> hw_id = event->aux_output_hw_id.hw_id; >>> - version = FIELD_GET(CS_AUX_HW_ID_VERSION_MASK, hw_id); >>> + version = FIELD_GET(CS_AUX_HW_ID_MAJOR_VERSION_MASK, hw_id); >>> >>> /* check that we can handle this version */ >>> - if (version > CS_AUX_HW_ID_CURR_VERSION) { >>> + if (version > CS_AUX_HW_ID_MAJOR_VERSION) { >>> pr_err("CS ETM Trace: PERF_RECORD_AUX_OUTPUT_HW_ID >>> version %d not supported. Please update Perf.\n", >>> version); >>> return -EINVAL; >>> @@ -442,7 +516,10 @@ static int >>> cs_etm__process_aux_output_hw_id(struct perf_session *session, >>> return -EINVAL; >>> } >>> >>> - return cs_etm__process_trace_id_v0(etm, cpu, hw_id); >> >> Perhaps leave this as the final statement of the function >> >>> + if (FIELD_GET(CS_AUX_HW_ID_MINOR_VERSION_MASK, hw_id) == 0) >> >> this could be moved before and be >> >> if (FIELD_GET(CS_AUX_HW_ID_MINOR_VERSION_MASK, hw_id) == 1) >> return cs_etm__process_trace_id_v0_1(etm, cpu, hw_id); >> >> > > Because I was intending minor version changes to be backwards compatible > I have it so that any value other than 0 is treated as v0.1. Otherwise > version updates will break old versions of Perf. And then if we added a > v0.3 it would look like this: That should have said v0.2 ^ > > if (FIELD_GET(CS_AUX_HW_ID_MINOR_VERSION_MASK, hw_id) == 0) > return cs_etm__process_trace_id_v0(etm, cpu, hw_id); > else if (FIELD_GET(CS_AUX_HW_ID_MINOR_VERSION_MASK, hw_id) == 1) > return cs_etm__process_trace_id_v0_1(etm, cpu, hw_id); > else > return cs_etm__process_trace_id_v0_2(etm, cpu, hw_id); > > Based on that I'm not sure if you still think it should be changed?
Fair enough - less worried about the ordering as the final :
else
return fn()
}
where there's no unconditional return at the end of the function. The
last else looks redundant to me. More a stylistic thing, not sure if
there is a hard and fast rule either way
Mike
On Fri, 19 Jul 2024 at 11:49, James Clark <james.clark@linaro.org> wrote:
>
>
>
> On 19/07/2024 11:48 am, James Clark wrote:
> >
> >
> > On 18/07/2024 2:24 pm, Mike Leach wrote:
> >> On Fri, 12 Jul 2024 at 11:22, James Clark <james.clark@linaro.org> wrote:
> >>>
> >>> From: James Clark <james.clark@arm.com>
> >>>
> >>> v0.1 HW_ID packets have a new field that describes which sink each CPU
> >>> writes to. Use the sink ID to link trace ID maps to each other so that
> >>> mappings are shared wherever the sink is shared.
> >>>
> >>> Also update the error message to show that overlapping IDs aren't an
> >>> error in per-thread mode, just not supported. In the future we can
> >>> use the CPU ID from the AUX records, or watch for changing sink IDs on
> >>> HW_ID packets to use the correct decoders.
> >>>
> >>> Signed-off-by: James Clark <james.clark@arm.com>
> >>> Signed-off-by: James Clark <james.clark@linaro.org>
> >>> ---
> >>> tools/include/linux/coresight-pmu.h | 17 +++--
> >>> tools/perf/util/cs-etm.c | 100 +++++++++++++++++++++++++---
> >>> 2 files changed, 103 insertions(+), 14 deletions(-)
> >>>
> >>> diff --git a/tools/include/linux/coresight-pmu.h
> >>> b/tools/include/linux/coresight-pmu.h
> >>> index 51ac441a37c3..89b0ac0014b0 100644
> >>> --- a/tools/include/linux/coresight-pmu.h
> >>> +++ b/tools/include/linux/coresight-pmu.h
> >>> @@ -49,12 +49,21 @@
> >>> * Interpretation of the PERF_RECORD_AUX_OUTPUT_HW_ID payload.
> >>> * Used to associate a CPU with the CoreSight Trace ID.
> >>> * [07:00] - Trace ID - uses 8 bits to make value easy to read in
> >>> file.
> >>> - * [59:08] - Unused (SBZ)
> >>> - * [63:60] - Version
> >>> + * [39:08] - Sink ID - as reported in
> >>> /sys/bus/event_source/devices/cs_etm/sinks/
> >>> + * Added in minor version 1.
> >>> + * [55:40] - Unused (SBZ)
> >>> + * [59:56] - Minor Version - previously existing fields are
> >>> compatible with
> >>> + * all minor versions.
> >>> + * [63:60] - Major Version - previously existing fields mean
> >>> different things
> >>> + * in new major versions.
> >>> */
> >>> #define CS_AUX_HW_ID_TRACE_ID_MASK GENMASK_ULL(7, 0)
> >>> -#define CS_AUX_HW_ID_VERSION_MASK GENMASK_ULL(63, 60)
> >>> +#define CS_AUX_HW_ID_SINK_ID_MASK GENMASK_ULL(39, 8)
> >>>
> >>> -#define CS_AUX_HW_ID_CURR_VERSION 0
> >>> +#define CS_AUX_HW_ID_MINOR_VERSION_MASK GENMASK_ULL(59, 56)
> >>> +#define CS_AUX_HW_ID_MAJOR_VERSION_MASK GENMASK_ULL(63, 60)
> >>> +
> >>> +#define CS_AUX_HW_ID_MAJOR_VERSION 0
> >>> +#define CS_AUX_HW_ID_MINOR_VERSION 1
> >>>
> >>> #endif
> >>> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> >>> index 954a6f7bedf3..87e983da19be 100644
> >>> --- a/tools/perf/util/cs-etm.c
> >>> +++ b/tools/perf/util/cs-etm.c
> >>> @@ -118,6 +118,12 @@ struct cs_etm_queue {
> >>> struct cs_etm_traceid_queue **traceid_queues;
> >>> /* Conversion between traceID and metadata pointers */
> >>> struct intlist *traceid_list;
> >>> + /*
> >>> + * Same as traceid_list, but traceid_list may be a reference
> >>> to another
> >>> + * queue's which has a matching sink ID.
> >>> + */
> >>> + struct intlist *own_traceid_list;
> >>> + u32 sink_id;
> >>> };
> >>>
> >>> static int cs_etm__process_timestamped_queues(struct
> >>> cs_etm_auxtrace *etm);
> >>> @@ -142,6 +148,7 @@ static int cs_etm__metadata_set_trace_id(u8
> >>> trace_chan_id, u64 *cpu_metadata);
> >>> (queue_nr << 16 | trace_chan_id)
> >>> #define TO_QUEUE_NR(cs_queue_nr) (cs_queue_nr >> 16)
> >>> #define TO_TRACE_CHAN_ID(cs_queue_nr) (cs_queue_nr & 0x0000ffff)
> >>> +#define SINK_UNSET ((u32) -1)
> >>>
> >>> static u32 cs_etm__get_v7_protocol_version(u32 etmidr)
> >>> {
> >>> @@ -241,7 +248,16 @@ static int cs_etm__insert_trace_id_node(struct
> >>> cs_etm_queue *etmq,
> >>> int err;
> >>>
> >>> if (curr_cpu_data[CS_ETM_CPU] !=
> >>> cpu_metadata[CS_ETM_CPU]) {
> >>> - pr_err("CS_ETM: map mismatch between HW_ID
> >>> packet CPU and Trace ID\n");
> >>> + /*
> >>> + * With > CORESIGHT_TRACE_IDS_MAX ETMs,
> >>> overlapping IDs
> >>> + * are expected (but not supported) in
> >>> per-thread mode,
> >>> + * rather than signifying an error.
> >>> + */
> >>> + if (etmq->etm->per_thread_decoding)
> >>> + pr_err("CS_ETM: overlapping Trace IDs
> >>> aren't currently supported in per-thread mode\n");
> >>> + else
> >>> + pr_err("CS_ETM: map mismatch between
> >>> HW_ID packet CPU and Trace ID\n");
> >>> +
> >>> return -EINVAL;
> >>> }
> >>>
> >>> @@ -326,6 +342,64 @@ static int cs_etm__process_trace_id_v0(struct
> >>> cs_etm_auxtrace *etm, int cpu,
> >>> return cs_etm__metadata_set_trace_id(trace_chan_id, cpu_data);
> >>> }
> >>>
> >>> +static int cs_etm__process_trace_id_v0_1(struct cs_etm_auxtrace
> >>> *etm, int cpu,
> >>> + u64 hw_id)
> >>> +{
> >>> + struct cs_etm_queue *etmq = cs_etm__get_queue(etm, cpu);
> >>> + int ret;
> >>> + u64 *cpu_data;
> >>> + u32 sink_id = FIELD_GET(CS_AUX_HW_ID_SINK_ID_MASK, hw_id);
> >>> + u8 trace_id = FIELD_GET(CS_AUX_HW_ID_TRACE_ID_MASK, hw_id);
> >>> +
> >>> + /*
> >>> + * Check sink id hasn't changed in per-cpu mode. In
> >>> per-thread mode,
> >>> + * let it pass for now until an actual overlapping trace ID
> >>> is hit. In
> >>> + * most cases IDs won't overlap even if the sink changes.
> >>> + */
> >>> + if (!etmq->etm->per_thread_decoding && etmq->sink_id !=
> >>> SINK_UNSET &&
> >>> + etmq->sink_id != sink_id) {
> >>> + pr_err("CS_ETM: mismatch between sink IDs\n");
> >>> + return -EINVAL;
> >>> + }
> >>> +
> >>> + etmq->sink_id = sink_id;
> >>> +
> >>> + /* Find which other queues use this sink and link their ID
> >>> maps */
> >>> + for (unsigned int i = 0; i < etm->queues.nr_queues; ++i) {
> >>> + struct cs_etm_queue *other_etmq =
> >>> etm->queues.queue_array[i].priv;
> >>> +
> >>> + /* Different sinks, skip */
> >>> + if (other_etmq->sink_id != etmq->sink_id)
> >>> + continue;
> >>> +
> >>> + /* Already linked, skip */
> >>> + if (other_etmq->traceid_list == etmq->traceid_list)
> >>> + continue;
> >>> +
> >>> + /* At the point of first linking, this one should be
> >>> empty */
> >>> + if (!intlist__empty(etmq->traceid_list)) {
> >>> + pr_err("CS_ETM: Can't link populated trace ID
> >>> lists\n");
> >>> + return -EINVAL;
> >>> + }
> >>> +
> >>> + etmq->own_traceid_list = NULL;
> >>> + intlist__delete(etmq->traceid_list);
> >>> + etmq->traceid_list = other_etmq->traceid_list;
> >>> + break;
> >>> + }
> >>> +
> >>> + cpu_data = get_cpu_data(etm, cpu);
> >>> + ret = cs_etm__insert_trace_id_node(etmq, trace_id, cpu_data);
> >>> + if (ret)
> >>> + return ret;
> >>> +
> >>> + ret = cs_etm__metadata_set_trace_id(trace_id, cpu_data);
> >>> + if (ret)
> >>> + return ret;
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> static int cs_etm__metadata_get_trace_id(u8 *trace_chan_id, u64
> >>> *cpu_metadata)
> >>> {
> >>> u64 cs_etm_magic = cpu_metadata[CS_ETM_MAGIC];
> >>> @@ -414,10 +488,10 @@ static int
> >>> cs_etm__process_aux_output_hw_id(struct perf_session *session,
> >>>
> >>> /* extract and parse the HW ID */
> >>> hw_id = event->aux_output_hw_id.hw_id;
> >>> - version = FIELD_GET(CS_AUX_HW_ID_VERSION_MASK, hw_id);
> >>> + version = FIELD_GET(CS_AUX_HW_ID_MAJOR_VERSION_MASK, hw_id);
> >>>
> >>> /* check that we can handle this version */
> >>> - if (version > CS_AUX_HW_ID_CURR_VERSION) {
> >>> + if (version > CS_AUX_HW_ID_MAJOR_VERSION) {
> >>> pr_err("CS ETM Trace: PERF_RECORD_AUX_OUTPUT_HW_ID
> >>> version %d not supported. Please update Perf.\n",
> >>> version);
> >>> return -EINVAL;
> >>> @@ -442,7 +516,10 @@ static int
> >>> cs_etm__process_aux_output_hw_id(struct perf_session *session,
> >>> return -EINVAL;
> >>> }
> >>>
> >>> - return cs_etm__process_trace_id_v0(etm, cpu, hw_id);
> >>
> >> Perhaps leave this as the final statement of the function
> >>
> >>> + if (FIELD_GET(CS_AUX_HW_ID_MINOR_VERSION_MASK, hw_id) == 0)
> >>
> >> this could be moved before and be
> >>
> >> if (FIELD_GET(CS_AUX_HW_ID_MINOR_VERSION_MASK, hw_id) == 1)
> >> return cs_etm__process_trace_id_v0_1(etm, cpu, hw_id);
> >>
> >>
> >
> > Because I was intending minor version changes to be backwards compatible
> > I have it so that any value other than 0 is treated as v0.1. Otherwise
> > version updates will break old versions of Perf. And then if we added a
> > v0.3 it would look like this:
>
> That should have said v0.2 ^
>
> >
> > if (FIELD_GET(CS_AUX_HW_ID_MINOR_VERSION_MASK, hw_id) == 0)
> > return cs_etm__process_trace_id_v0(etm, cpu, hw_id);
> > else if (FIELD_GET(CS_AUX_HW_ID_MINOR_VERSION_MASK, hw_id) == 1)
> > return cs_etm__process_trace_id_v0_1(etm, cpu, hw_id);
> > else
> > return cs_etm__process_trace_id_v0_2(etm, cpu, hw_id);
> >
> > Based on that I'm not sure if you still think it should be changed?
On 19/07/2024 2:45 pm, Mike Leach wrote: > Fair enough - less worried about the ordering as the final : > > else > return fn() > } > > where there's no unconditional return at the end of the function. The > last else looks redundant to me. More a stylistic thing, not sure if > there is a hard and fast rule either way > > Mike > > > Ok yeah I can update that. > On Fri, 19 Jul 2024 at 11:49, James Clark <james.clark@linaro.org> wrote: >> >> >> >> On 19/07/2024 11:48 am, James Clark wrote: >>> >>> >>> On 18/07/2024 2:24 pm, Mike Leach wrote: >>>> On Fri, 12 Jul 2024 at 11:22, James Clark <james.clark@linaro.org> wrote: >>>>> >>>>> From: James Clark <james.clark@arm.com> >>>>> >>>>> v0.1 HW_ID packets have a new field that describes which sink each CPU >>>>> writes to. Use the sink ID to link trace ID maps to each other so that >>>>> mappings are shared wherever the sink is shared. >>>>> >>>>> Also update the error message to show that overlapping IDs aren't an >>>>> error in per-thread mode, just not supported. In the future we can >>>>> use the CPU ID from the AUX records, or watch for changing sink IDs on >>>>> HW_ID packets to use the correct decoders. >>>>> >>>>> Signed-off-by: James Clark <james.clark@arm.com> >>>>> Signed-off-by: James Clark <james.clark@linaro.org> >>>>> --- >>>>> tools/include/linux/coresight-pmu.h | 17 +++-- >>>>> tools/perf/util/cs-etm.c | 100 +++++++++++++++++++++++++--- >>>>> 2 files changed, 103 insertions(+), 14 deletions(-) >>>>> >>>>> diff --git a/tools/include/linux/coresight-pmu.h >>>>> b/tools/include/linux/coresight-pmu.h >>>>> index 51ac441a37c3..89b0ac0014b0 100644 >>>>> --- a/tools/include/linux/coresight-pmu.h >>>>> +++ b/tools/include/linux/coresight-pmu.h >>>>> @@ -49,12 +49,21 @@ >>>>> * Interpretation of the PERF_RECORD_AUX_OUTPUT_HW_ID payload. >>>>> * Used to associate a CPU with the CoreSight Trace ID. >>>>> * [07:00] - Trace ID - uses 8 bits to make value easy to read in >>>>> file. >>>>> - * [59:08] - Unused (SBZ) >>>>> - * [63:60] - Version >>>>> + * [39:08] - Sink ID - as reported in >>>>> /sys/bus/event_source/devices/cs_etm/sinks/ >>>>> + * Added in minor version 1. >>>>> + * [55:40] - Unused (SBZ) >>>>> + * [59:56] - Minor Version - previously existing fields are >>>>> compatible with >>>>> + * all minor versions. >>>>> + * [63:60] - Major Version - previously existing fields mean >>>>> different things >>>>> + * in new major versions. >>>>> */ >>>>> #define CS_AUX_HW_ID_TRACE_ID_MASK GENMASK_ULL(7, 0) >>>>> -#define CS_AUX_HW_ID_VERSION_MASK GENMASK_ULL(63, 60) >>>>> +#define CS_AUX_HW_ID_SINK_ID_MASK GENMASK_ULL(39, 8) >>>>> >>>>> -#define CS_AUX_HW_ID_CURR_VERSION 0 >>>>> +#define CS_AUX_HW_ID_MINOR_VERSION_MASK GENMASK_ULL(59, 56) >>>>> +#define CS_AUX_HW_ID_MAJOR_VERSION_MASK GENMASK_ULL(63, 60) >>>>> + >>>>> +#define CS_AUX_HW_ID_MAJOR_VERSION 0 >>>>> +#define CS_AUX_HW_ID_MINOR_VERSION 1 >>>>> >>>>> #endif >>>>> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c >>>>> index 954a6f7bedf3..87e983da19be 100644 >>>>> --- a/tools/perf/util/cs-etm.c >>>>> +++ b/tools/perf/util/cs-etm.c >>>>> @@ -118,6 +118,12 @@ struct cs_etm_queue { >>>>> struct cs_etm_traceid_queue **traceid_queues; >>>>> /* Conversion between traceID and metadata pointers */ >>>>> struct intlist *traceid_list; >>>>> + /* >>>>> + * Same as traceid_list, but traceid_list may be a reference >>>>> to another >>>>> + * queue's which has a matching sink ID. >>>>> + */ >>>>> + struct intlist *own_traceid_list; >>>>> + u32 sink_id; >>>>> }; >>>>> >>>>> static int cs_etm__process_timestamped_queues(struct >>>>> cs_etm_auxtrace *etm); >>>>> @@ -142,6 +148,7 @@ static int cs_etm__metadata_set_trace_id(u8 >>>>> trace_chan_id, u64 *cpu_metadata); >>>>> (queue_nr << 16 | trace_chan_id) >>>>> #define TO_QUEUE_NR(cs_queue_nr) (cs_queue_nr >> 16) >>>>> #define TO_TRACE_CHAN_ID(cs_queue_nr) (cs_queue_nr & 0x0000ffff) >>>>> +#define SINK_UNSET ((u32) -1) >>>>> >>>>> static u32 cs_etm__get_v7_protocol_version(u32 etmidr) >>>>> { >>>>> @@ -241,7 +248,16 @@ static int cs_etm__insert_trace_id_node(struct >>>>> cs_etm_queue *etmq, >>>>> int err; >>>>> >>>>> if (curr_cpu_data[CS_ETM_CPU] != >>>>> cpu_metadata[CS_ETM_CPU]) { >>>>> - pr_err("CS_ETM: map mismatch between HW_ID >>>>> packet CPU and Trace ID\n"); >>>>> + /* >>>>> + * With > CORESIGHT_TRACE_IDS_MAX ETMs, >>>>> overlapping IDs >>>>> + * are expected (but not supported) in >>>>> per-thread mode, >>>>> + * rather than signifying an error. >>>>> + */ >>>>> + if (etmq->etm->per_thread_decoding) >>>>> + pr_err("CS_ETM: overlapping Trace IDs >>>>> aren't currently supported in per-thread mode\n"); >>>>> + else >>>>> + pr_err("CS_ETM: map mismatch between >>>>> HW_ID packet CPU and Trace ID\n"); >>>>> + >>>>> return -EINVAL; >>>>> } >>>>> >>>>> @@ -326,6 +342,64 @@ static int cs_etm__process_trace_id_v0(struct >>>>> cs_etm_auxtrace *etm, int cpu, >>>>> return cs_etm__metadata_set_trace_id(trace_chan_id, cpu_data); >>>>> } >>>>> >>>>> +static int cs_etm__process_trace_id_v0_1(struct cs_etm_auxtrace >>>>> *etm, int cpu, >>>>> + u64 hw_id) >>>>> +{ >>>>> + struct cs_etm_queue *etmq = cs_etm__get_queue(etm, cpu); >>>>> + int ret; >>>>> + u64 *cpu_data; >>>>> + u32 sink_id = FIELD_GET(CS_AUX_HW_ID_SINK_ID_MASK, hw_id); >>>>> + u8 trace_id = FIELD_GET(CS_AUX_HW_ID_TRACE_ID_MASK, hw_id); >>>>> + >>>>> + /* >>>>> + * Check sink id hasn't changed in per-cpu mode. In >>>>> per-thread mode, >>>>> + * let it pass for now until an actual overlapping trace ID >>>>> is hit. In >>>>> + * most cases IDs won't overlap even if the sink changes. >>>>> + */ >>>>> + if (!etmq->etm->per_thread_decoding && etmq->sink_id != >>>>> SINK_UNSET && >>>>> + etmq->sink_id != sink_id) { >>>>> + pr_err("CS_ETM: mismatch between sink IDs\n"); >>>>> + return -EINVAL; >>>>> + } >>>>> + >>>>> + etmq->sink_id = sink_id; >>>>> + >>>>> + /* Find which other queues use this sink and link their ID >>>>> maps */ >>>>> + for (unsigned int i = 0; i < etm->queues.nr_queues; ++i) { >>>>> + struct cs_etm_queue *other_etmq = >>>>> etm->queues.queue_array[i].priv; >>>>> + >>>>> + /* Different sinks, skip */ >>>>> + if (other_etmq->sink_id != etmq->sink_id) >>>>> + continue; >>>>> + >>>>> + /* Already linked, skip */ >>>>> + if (other_etmq->traceid_list == etmq->traceid_list) >>>>> + continue; >>>>> + >>>>> + /* At the point of first linking, this one should be >>>>> empty */ >>>>> + if (!intlist__empty(etmq->traceid_list)) { >>>>> + pr_err("CS_ETM: Can't link populated trace ID >>>>> lists\n"); >>>>> + return -EINVAL; >>>>> + } >>>>> + >>>>> + etmq->own_traceid_list = NULL; >>>>> + intlist__delete(etmq->traceid_list); >>>>> + etmq->traceid_list = other_etmq->traceid_list; >>>>> + break; >>>>> + } >>>>> + >>>>> + cpu_data = get_cpu_data(etm, cpu); >>>>> + ret = cs_etm__insert_trace_id_node(etmq, trace_id, cpu_data); >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> + ret = cs_etm__metadata_set_trace_id(trace_id, cpu_data); >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> static int cs_etm__metadata_get_trace_id(u8 *trace_chan_id, u64 >>>>> *cpu_metadata) >>>>> { >>>>> u64 cs_etm_magic = cpu_metadata[CS_ETM_MAGIC]; >>>>> @@ -414,10 +488,10 @@ static int >>>>> cs_etm__process_aux_output_hw_id(struct perf_session *session, >>>>> >>>>> /* extract and parse the HW ID */ >>>>> hw_id = event->aux_output_hw_id.hw_id; >>>>> - version = FIELD_GET(CS_AUX_HW_ID_VERSION_MASK, hw_id); >>>>> + version = FIELD_GET(CS_AUX_HW_ID_MAJOR_VERSION_MASK, hw_id); >>>>> >>>>> /* check that we can handle this version */ >>>>> - if (version > CS_AUX_HW_ID_CURR_VERSION) { >>>>> + if (version > CS_AUX_HW_ID_MAJOR_VERSION) { >>>>> pr_err("CS ETM Trace: PERF_RECORD_AUX_OUTPUT_HW_ID >>>>> version %d not supported. Please update Perf.\n", >>>>> version); >>>>> return -EINVAL; >>>>> @@ -442,7 +516,10 @@ static int >>>>> cs_etm__process_aux_output_hw_id(struct perf_session *session, >>>>> return -EINVAL; >>>>> } >>>>> >>>>> - return cs_etm__process_trace_id_v0(etm, cpu, hw_id); >>>> >>>> Perhaps leave this as the final statement of the function >>>> >>>>> + if (FIELD_GET(CS_AUX_HW_ID_MINOR_VERSION_MASK, hw_id) == 0) >>>> >>>> this could be moved before and be >>>> >>>> if (FIELD_GET(CS_AUX_HW_ID_MINOR_VERSION_MASK, hw_id) == 1) >>>> return cs_etm__process_trace_id_v0_1(etm, cpu, hw_id); >>>> >>>> >>> >>> Because I was intending minor version changes to be backwards compatible >>> I have it so that any value other than 0 is treated as v0.1. Otherwise >>> version updates will break old versions of Perf. And then if we added a >>> v0.3 it would look like this: >> >> That should have said v0.2 ^ >> >>> >>> if (FIELD_GET(CS_AUX_HW_ID_MINOR_VERSION_MASK, hw_id) == 0) >>> return cs_etm__process_trace_id_v0(etm, cpu, hw_id); >>> else if (FIELD_GET(CS_AUX_HW_ID_MINOR_VERSION_MASK, hw_id) == 1) >>> return cs_etm__process_trace_id_v0_1(etm, cpu, hw_id); >>> else >>> return cs_etm__process_trace_id_v0_2(etm, cpu, hw_id); >>> >>> Based on that I'm not sure if you still think it should be changed? > > >
diff --git a/tools/include/linux/coresight-pmu.h b/tools/include/linux/coresight-pmu.h index 51ac441a37c3..89b0ac0014b0 100644 --- a/tools/include/linux/coresight-pmu.h +++ b/tools/include/linux/coresight-pmu.h @@ -49,12 +49,21 @@ * Interpretation of the PERF_RECORD_AUX_OUTPUT_HW_ID payload. * Used to associate a CPU with the CoreSight Trace ID. * [07:00] - Trace ID - uses 8 bits to make value easy to read in file. - * [59:08] - Unused (SBZ) - * [63:60] - Version + * [39:08] - Sink ID - as reported in /sys/bus/event_source/devices/cs_etm/sinks/ + * Added in minor version 1. + * [55:40] - Unused (SBZ) + * [59:56] - Minor Version - previously existing fields are compatible with + * all minor versions. + * [63:60] - Major Version - previously existing fields mean different things + * in new major versions. */ #define CS_AUX_HW_ID_TRACE_ID_MASK GENMASK_ULL(7, 0) -#define CS_AUX_HW_ID_VERSION_MASK GENMASK_ULL(63, 60) +#define CS_AUX_HW_ID_SINK_ID_MASK GENMASK_ULL(39, 8) -#define CS_AUX_HW_ID_CURR_VERSION 0 +#define CS_AUX_HW_ID_MINOR_VERSION_MASK GENMASK_ULL(59, 56) +#define CS_AUX_HW_ID_MAJOR_VERSION_MASK GENMASK_ULL(63, 60) + +#define CS_AUX_HW_ID_MAJOR_VERSION 0 +#define CS_AUX_HW_ID_MINOR_VERSION 1 #endif diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index 954a6f7bedf3..87e983da19be 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -118,6 +118,12 @@ struct cs_etm_queue { struct cs_etm_traceid_queue **traceid_queues; /* Conversion between traceID and metadata pointers */ struct intlist *traceid_list; + /* + * Same as traceid_list, but traceid_list may be a reference to another + * queue's which has a matching sink ID. + */ + struct intlist *own_traceid_list; + u32 sink_id; }; static int cs_etm__process_timestamped_queues(struct cs_etm_auxtrace *etm); @@ -142,6 +148,7 @@ static int cs_etm__metadata_set_trace_id(u8 trace_chan_id, u64 *cpu_metadata); (queue_nr << 16 | trace_chan_id) #define TO_QUEUE_NR(cs_queue_nr) (cs_queue_nr >> 16) #define TO_TRACE_CHAN_ID(cs_queue_nr) (cs_queue_nr & 0x0000ffff) +#define SINK_UNSET ((u32) -1) static u32 cs_etm__get_v7_protocol_version(u32 etmidr) { @@ -241,7 +248,16 @@ static int cs_etm__insert_trace_id_node(struct cs_etm_queue *etmq, int err; if (curr_cpu_data[CS_ETM_CPU] != cpu_metadata[CS_ETM_CPU]) { - pr_err("CS_ETM: map mismatch between HW_ID packet CPU and Trace ID\n"); + /* + * With > CORESIGHT_TRACE_IDS_MAX ETMs, overlapping IDs + * are expected (but not supported) in per-thread mode, + * rather than signifying an error. + */ + if (etmq->etm->per_thread_decoding) + pr_err("CS_ETM: overlapping Trace IDs aren't currently supported in per-thread mode\n"); + else + pr_err("CS_ETM: map mismatch between HW_ID packet CPU and Trace ID\n"); + return -EINVAL; } @@ -326,6 +342,64 @@ static int cs_etm__process_trace_id_v0(struct cs_etm_auxtrace *etm, int cpu, return cs_etm__metadata_set_trace_id(trace_chan_id, cpu_data); } +static int cs_etm__process_trace_id_v0_1(struct cs_etm_auxtrace *etm, int cpu, + u64 hw_id) +{ + struct cs_etm_queue *etmq = cs_etm__get_queue(etm, cpu); + int ret; + u64 *cpu_data; + u32 sink_id = FIELD_GET(CS_AUX_HW_ID_SINK_ID_MASK, hw_id); + u8 trace_id = FIELD_GET(CS_AUX_HW_ID_TRACE_ID_MASK, hw_id); + + /* + * Check sink id hasn't changed in per-cpu mode. In per-thread mode, + * let it pass for now until an actual overlapping trace ID is hit. In + * most cases IDs won't overlap even if the sink changes. + */ + if (!etmq->etm->per_thread_decoding && etmq->sink_id != SINK_UNSET && + etmq->sink_id != sink_id) { + pr_err("CS_ETM: mismatch between sink IDs\n"); + return -EINVAL; + } + + etmq->sink_id = sink_id; + + /* Find which other queues use this sink and link their ID maps */ + for (unsigned int i = 0; i < etm->queues.nr_queues; ++i) { + struct cs_etm_queue *other_etmq = etm->queues.queue_array[i].priv; + + /* Different sinks, skip */ + if (other_etmq->sink_id != etmq->sink_id) + continue; + + /* Already linked, skip */ + if (other_etmq->traceid_list == etmq->traceid_list) + continue; + + /* At the point of first linking, this one should be empty */ + if (!intlist__empty(etmq->traceid_list)) { + pr_err("CS_ETM: Can't link populated trace ID lists\n"); + return -EINVAL; + } + + etmq->own_traceid_list = NULL; + intlist__delete(etmq->traceid_list); + etmq->traceid_list = other_etmq->traceid_list; + break; + } + + cpu_data = get_cpu_data(etm, cpu); + ret = cs_etm__insert_trace_id_node(etmq, trace_id, cpu_data); + if (ret) + return ret; + + ret = cs_etm__metadata_set_trace_id(trace_id, cpu_data); + if (ret) + return ret; + + return 0; +} + static int cs_etm__metadata_get_trace_id(u8 *trace_chan_id, u64 *cpu_metadata) { u64 cs_etm_magic = cpu_metadata[CS_ETM_MAGIC]; @@ -414,10 +488,10 @@ static int cs_etm__process_aux_output_hw_id(struct perf_session *session, /* extract and parse the HW ID */ hw_id = event->aux_output_hw_id.hw_id; - version = FIELD_GET(CS_AUX_HW_ID_VERSION_MASK, hw_id); + version = FIELD_GET(CS_AUX_HW_ID_MAJOR_VERSION_MASK, hw_id); /* check that we can handle this version */ - if (version > CS_AUX_HW_ID_CURR_VERSION) { + if (version > CS_AUX_HW_ID_MAJOR_VERSION) { pr_err("CS ETM Trace: PERF_RECORD_AUX_OUTPUT_HW_ID version %d not supported. Please update Perf.\n", version); return -EINVAL; @@ -442,7 +516,10 @@ static int cs_etm__process_aux_output_hw_id(struct perf_session *session, return -EINVAL; } - return cs_etm__process_trace_id_v0(etm, cpu, hw_id); + if (FIELD_GET(CS_AUX_HW_ID_MINOR_VERSION_MASK, hw_id) == 0) + return cs_etm__process_trace_id_v0(etm, cpu, hw_id); + else + return cs_etm__process_trace_id_v0_1(etm, cpu, hw_id); } void cs_etm__etmq_set_traceid_queue_timestamp(struct cs_etm_queue *etmq, @@ -882,12 +959,14 @@ static void cs_etm__free_queue(void *priv) cs_etm_decoder__free(etmq->decoder); cs_etm__free_traceid_queues(etmq); - /* First remove all traceID/metadata nodes for the RB tree */ - intlist__for_each_entry_safe(inode, tmp, etmq->traceid_list) - intlist__remove(etmq->traceid_list, inode); + if (etmq->own_traceid_list) { + /* First remove all traceID/metadata nodes for the RB tree */ + intlist__for_each_entry_safe(inode, tmp, etmq->own_traceid_list) + intlist__remove(etmq->own_traceid_list, inode); - /* Then the RB tree itself */ - intlist__delete(etmq->traceid_list); + /* Then the RB tree itself */ + intlist__delete(etmq->own_traceid_list); + } free(etmq); } @@ -1081,7 +1160,7 @@ static struct cs_etm_queue *cs_etm__alloc_queue(void) * has to be made for each packet that gets decoded, optimizing access * in anything other than a sequential array is worth doing. */ - etmq->traceid_list = intlist__new(NULL); + etmq->traceid_list = etmq->own_traceid_list = intlist__new(NULL); if (!etmq->traceid_list) goto out_free; @@ -1113,6 +1192,7 @@ static int cs_etm__setup_queue(struct cs_etm_auxtrace *etm, etmq->queue_nr = queue_nr; queue->cpu = queue_nr; /* Placeholder, may be reset to -1 in per-thread mode */ etmq->offset = 0; + etmq->sink_id = SINK_UNSET; return 0; }