| 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: <YTID5PkUAPHlbD18@kernel.org>
Date: Fri, 3 Sep 2021 08:15:48 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Suzuki K Poulose <suzuki.poulose@....com>
Cc: James Clark <james.clark@....com>, mathieu.poirier@...aro.org,
leo.yan@...aro.org, coresight@...ts.linaro.org,
linux-perf-users@...r.kernel.org, mike.leach@...aro.org,
John Garry <john.garry@...wei.com>,
Will Deacon <will@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Jiri Olsa <jolsa@...hat.com>,
Namhyung Kim <namhyung@...nel.org>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/9] perf cs-etm: Save TRCDEVARCH register
Em Fri, Sep 03, 2021 at 10:08:22AM +0100, Suzuki K Poulose escreveu:
> Hi James
>
> On 06/08/2021 14:41, James Clark wrote:
> > When ETE is present save the TRCDEVARCH register and set a new magic
> > number. It will be used to configure the decoder in a later commit.
> >
> > Old versions of perf will not be able to open files with this new magic
> > number, but old files will still work with newer versions of perf.
> >
>
> The patch looks good to me. There are some minor style related comments
> below.
>
> > Signed-off-by: James Clark <james.clark@....com>
> > ---
> > tools/perf/arch/arm/util/cs-etm.c | 49 +++++++++++++++++++++++++++----
> > tools/perf/util/cs-etm.c | 16 ++++++++--
> > tools/perf/util/cs-etm.h | 10 +++++++
> > 3 files changed, 68 insertions(+), 7 deletions(-)
> >
> > diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
> > index ecb6fa55a210..e3500b79d972 100644
> > --- a/tools/perf/arch/arm/util/cs-etm.c
> > +++ b/tools/perf/arch/arm/util/cs-etm.c
> > @@ -47,15 +47,17 @@ static const char *metadata_etmv3_ro[CS_ETM_PRIV_MAX] = {
> > [CS_ETM_ETMIDR] = "mgmt/etmidr",
> > };
> > -static const char *metadata_etmv4_ro[CS_ETMV4_PRIV_MAX] = {
> > +static const char * const metadata_etmv4_ro[] = {
> > [CS_ETMV4_TRCIDR0] = "trcidr/trcidr0",
> > [CS_ETMV4_TRCIDR1] = "trcidr/trcidr1",
> > [CS_ETMV4_TRCIDR2] = "trcidr/trcidr2",
> > [CS_ETMV4_TRCIDR8] = "trcidr/trcidr8",
> > [CS_ETMV4_TRCAUTHSTATUS] = "mgmt/trcauthstatus",
> > + [CS_ETE_TRCDEVARCH] = "mgmt/trcdevarch"
> > };
> > static bool cs_etm_is_etmv4(struct auxtrace_record *itr, int cpu);
> > +static bool cs_etm_is_ete(struct auxtrace_record *itr, int cpu);
> > static int cs_etm_set_context_id(struct auxtrace_record *itr,
> > struct evsel *evsel, int cpu)
> > @@ -533,7 +535,7 @@ cs_etm_info_priv_size(struct auxtrace_record *itr __maybe_unused,
> > struct evlist *evlist __maybe_unused)
> > {
> > int i;
> > - int etmv3 = 0, etmv4 = 0;
> > + int etmv3 = 0, etmv4 = 0, ete = 0;
> > struct perf_cpu_map *event_cpus = evlist->core.cpus;
> > struct perf_cpu_map *online_cpus = perf_cpu_map__new(NULL);
> > @@ -544,7 +546,9 @@ cs_etm_info_priv_size(struct auxtrace_record *itr __maybe_unused,
> > !cpu_map__has(online_cpus, i))
> > continue;
> > - if (cs_etm_is_etmv4(itr, i))
> > + if (cs_etm_is_ete(itr, i))
> > + ete++;
> > + else if (cs_etm_is_etmv4(itr, i))
> > etmv4++;
> > else
> > etmv3++;
> > @@ -555,7 +559,9 @@ cs_etm_info_priv_size(struct auxtrace_record *itr __maybe_unused,
> > if (!cpu_map__has(online_cpus, i))
> > continue;
> > - if (cs_etm_is_etmv4(itr, i))
> > + if (cs_etm_is_ete(itr, i))
> > + ete++;
> > + else if (cs_etm_is_etmv4(itr, i))
> > etmv4++;
> > else
> > etmv3++;
> > @@ -565,6 +571,7 @@ cs_etm_info_priv_size(struct auxtrace_record *itr __maybe_unused,
> > perf_cpu_map__put(online_cpus);
> > return (CS_ETM_HEADER_SIZE +
> > + (ete * CS_ETE_PRIV_SIZE) +
> > (etmv4 * CS_ETMV4_PRIV_SIZE) +
> > (etmv3 * CS_ETMV3_PRIV_SIZE));
> > }
> > @@ -607,6 +614,27 @@ static int cs_etm_get_ro(struct perf_pmu *pmu, int cpu, const char *path)
> > return val;
> > }
> > +#define TRCDEVARCH_ARCHPART_SHIFT 0
> > +#define TRCDEVARCH_ARCHPART_MASK GENMASK(11, 0)
> > +#define TRCDEVARCH_ARCHPART(x) (((x) & TRCDEVARCH_ARCHPART_MASK) >> TRCDEVARCH_ARCHPART_SHIFT)
> > +
> > +#define TRCDEVARCH_ARCHVER_SHIFT 12
> > +#define TRCDEVARCH_ARCHVER_MASK GENMASK(15, 12)
> > +#define TRCDEVARCH_ARCHVER(x) (((x) & TRCDEVARCH_ARCHVER_MASK) >> TRCDEVARCH_ARCHVER_SHIFT)
> > +
> > +static bool cs_etm_is_ete(struct auxtrace_record *itr, int cpu)
> > +{
> > + struct cs_etm_recording *ptr = container_of(itr, struct cs_etm_recording, itr);
> > + struct perf_pmu *cs_etm_pmu = ptr->cs_etm_pmu;
> > + int trcdevarch = cs_etm_get_ro(cs_etm_pmu, cpu, metadata_etmv4_ro[CS_ETE_TRCDEVARCH]);
> > +
> > + /*
> > + * ETE if ARCHVER is 5 (ARCHVER is 4 for ETM) and ARCHPART is 0xA13.
> > + * See ETM_DEVARCH_ETE_ARCH in coresight-etm4x.h
> > + */
> > + return TRCDEVARCH_ARCHVER(trcdevarch) == 5 && TRCDEVARCH_ARCHPART(trcdevarch) == 0xA13;
> > +}
> > +
> > static void cs_etm_save_etmv4_header(__u64 data[], struct auxtrace_record *itr, int cpu)
> > {
> > struct cs_etm_recording *ptr = container_of(itr, struct cs_etm_recording, itr);
> > @@ -640,7 +668,18 @@ static void cs_etm_get_metadata(int cpu, u32 *offset,
> > struct perf_pmu *cs_etm_pmu = ptr->cs_etm_pmu;
> > /* first see what kind of tracer this cpu is affined to */
> > - if (cs_etm_is_etmv4(itr, cpu)) {
> > + if (cs_etm_is_ete(itr, cpu)) {
> > + magic = __perf_cs_ete_magic;
> > + /* ETE uses the same registers as ETMv4 plus TRCDEVARCH */
> > + cs_etm_save_etmv4_header(&info->priv[*offset], itr, cpu);
> > + info->priv[*offset + CS_ETE_TRCDEVARCH] =
> > + cs_etm_get_ro(cs_etm_pmu, cpu,
> > + metadata_etmv4_ro[CS_ETE_TRCDEVARCH]);
> > +
>
> minor nit: It may be nicer to have a helper function like we do for etmv4.
> i.e,
> cs_etm_save_ete_header();
>
> to keep the code consistent.
>
> > + /* How much space was used */
> > + increment = CS_ETE_PRIV_MAX;
> > + nr_trc_params = CS_ETE_PRIV_MAX - CS_ETM_COMMON_BLK_MAX_V1;
> > + } else if (cs_etm_is_etmv4(itr, cpu)) {
> > magic = __perf_cs_etmv4_magic;
> > cs_etm_save_etmv4_header(&info->priv[*offset], itr, cpu);
> > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> > index f4b2bff533f3..d540512a3c96 100644
> > --- a/tools/perf/util/cs-etm.c
> > +++ b/tools/perf/util/cs-etm.c
>
>
> > for (j = 0; j < total_params; j++, i++) {
> > /* if newer record - could be excess params */
> > - if (j >= CS_ETMV4_PRIV_MAX)
> > + if (j >= CS_ETE_PRIV_MAX)
> > fprintf(stdout, param_unk_fmt, j, val[i]);
> > else
> > fprintf(stdout, cs_etmv4_priv_fmts[j], val[i]);
> > @@ -2943,6 +2949,12 @@ int cs_etm__process_auxtrace_info(union perf_event *event,
> > /* The traceID is our handle */
> > trcidr_idx = CS_ETMV4_TRCTRACEIDR;
> > + } else if (ptr[i] == __perf_cs_ete_magic) {
> > + metadata[j] =
> > + cs_etm__create_meta_blk(ptr, &i, CS_ETE_PRIV_MAX, -1);
>
> minor nit: I would prefer the arguments to be split into the next line,
> rather than moving the function call to the next line. i.e,
>
> metatdata[j] = cs_etm_create_meta_blk(ptr, &i,
> ....
He did it for consistency with the previous else blocks, but I think it
should even be all in just one line, 92 chars is ok, I'd say.
Here I have:
$ echo $COLUMNS
273
$
:-)
I did it as there are plenty of other lines in this file that are over
100 chars in width.
> > +
> > + /* ETE shares first part of metadata with ETMv4 */
> > + trcidr_idx = CS_ETMV4_TRCTRACEIDR;
> > }
> > if (!metadata[j]) {
> > diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h
> > index d65c7b19407d..08b84c21f065 100644
> > --- a/tools/perf/util/cs-etm.h
> > +++ b/tools/perf/util/cs-etm.h
> > @@ -76,6 +76,14 @@ enum {
> > /* define fixed version 0 length - allow new format reader to read old files. */
> > #define CS_ETMV4_NR_TRC_PARAMS_V0 (CS_ETMV4_TRCAUTHSTATUS - CS_ETMV4_TRCCONFIGR + 1)
> > +/* ETE metadata is ETMv4 plus TRCDEVARCH register and doesn't support header V0 since it was
>
> minor nit: comment style.
>
> /*
> * ETE metadata...
> *
> */
Yeah, being even consistent with other multi line comments on this same
file.
- Arnaldo
Powered by blists - more mailing lists