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

[James Clark <james.clark@linaro.org>]

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?