Messages in this thread Patch in this message | | | Subject | Re: [PATCH v2 4/9] perf cs-etm: Save TRCDEVARCH register | From | Suzuki K Poulose <> | Date | Fri, 3 Sep 2021 10:08:22 +0100 |
| |
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@arm.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, .... > + > + /* 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... * */
Suzuki
| |