| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ef5e7351-5f62-444a-b930-4dc2feb9f10d@linaro.org>
Date: Fri, 19 Jul 2024 11:48:16 +0100
From: James Clark <james.clark@...aro.org>
To: Mike Leach <mike.leach@...aro.org>
Cc: coresight@...ts.linaro.org, suzuki.poulose@....com,
gankulkarni@...amperecomputing.com, leo.yan@...ux.dev,
anshuman.khandual@....com, James Clark <james.clark@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Maxime Coquelin <mcoquelin.stm32@...il.com>,
Alexandre Torgue <alexandre.torgue@...s.st.com>,
John Garry <john.g.garry@...cle.com>, Will Deacon <will@...nel.org>,
Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
Arnaldo Carvalho de Melo <acme@...nel.org>,
Namhyung Kim <namhyung@...nel.org>, Mark Rutland <mark.rutland@....com>,
Jiri Olsa <jolsa@...nel.org>, Ian Rogers <irogers@...gle.com>,
Adrian Hunter <adrian.hunter@...el.com>,
"Liang, Kan" <kan.liang@...ux.intel.com>, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
linux-stm32@...md-mailman.stormreply.com, linux-perf-users@...r.kernel.org
Subject: Re: [PATCH v5 06/17] perf: cs-etm: Support version 0.1 of HW_ID
packets
On 18/07/2024 2:24 pm, Mike Leach wrote:
> On Fri, 12 Jul 2024 at 11:22, James Clark <james.clark@...aro.org> wrote:
>>
>> From: James Clark <james.clark@....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@....com>
>> Signed-off-by: James Clark <james.clark@...aro.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?
Powered by blists - more mailing lists