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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAJ9a7VhL18eWFw6T6HdrhbY_v8oeyuzM62D1w1CO1Psumb-EBQ@mail.gmail.com>
Date: Fri, 19 Jul 2024 14:45:36 +0100
From: Mike Leach <mike.leach@...aro.org>
To: James Clark <james.clark@...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

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@...aro.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@...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:
>
> 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
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ