diff mbox series

[v5,06/17] perf: cs-etm: Support version 0.1 of HW_ID packets

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

Commit Message

James Clark July 12, 2024, 10:20 a.m. UTC
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(-)

Comments

Mike Leach July 18, 2024, 1:24 p.m. UTC | #1
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
James Clark July 19, 2024, 10:48 a.m. UTC | #2
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?
James Clark July 19, 2024, 10:49 a.m. UTC | #3
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?
Mike Leach July 19, 2024, 1:45 p.m. UTC | #4
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?
James Clark July 19, 2024, 1:57 p.m. UTC | #5
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 mbox series

Patch

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;
 }