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 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?
> 
> 
>