[PATCH v5 06/17] perf: cs-etm: Support version 0.1 of HW_ID packets
James Clark
james.clark at linaro.org
Fri Jul 19 06:57:45 PDT 2024
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 at 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 at linaro.org> wrote:
>>>>>
>>>>> From: James Clark <james.clark at 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 at arm.com>
>>>>> Signed-off-by: James Clark <james.clark at 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?
>
>
>
More information about the linux-arm-kernel
mailing list