Messages in this thread | | | From | Mike Leach <> | Date | Tue, 26 Jan 2021 15:33:16 +0000 | Subject | Re: [PATCH v3] perf: cs-etm: update ETM metadata format |
| |
Hi Leo,
On Sat, 23 Jan 2021 at 15:23, Leo Yan <leo.yan@linaro.org> wrote: > > Hi Mike, > > On Tue, Jan 19, 2021 at 06:01:10PM +0000, Mike Leach wrote: > > [...] > > > +/* > > + * Read a single cpu parameter block from the auxtrace_info priv block. > > + * > > + * For version 1 there is a per cpu nr_params entry. If we are handling > > + * version 1 file, then there may be less, the same, or more params > > + * indicated by this value than the compile time number we understand. > > + * > > + * For a version 0 info block, there are a fixed number, and we need to > > + * fill out the nr_param value in the metadata we create. > > + */ > > +static u64 *cs_etm__create_meta_blk(u64 *buff_in, int *buff_in_offset, > > + int out_blk_size, int nr_params_v0) > > +{ > > + u64 *metadata = NULL; > > + int hdr_version; > > + int nr_in_params, nr_out_params, nr_cmn_params; > > + int i, k; > > + > > + metadata = zalloc(sizeof(*metadata) * out_blk_size); > > + if (!metadata) > > + return NULL; > > + > > + /* read block current index & version */ > > + i = *buff_in_offset; > > + hdr_version = buff_in[CS_HEADER_VERSION]; > > + > > + if (!hdr_version) { > > + /* read version 0 info block into a version 1 metadata block */ > > + nr_in_params = nr_params_v0; > > + metadata[CS_ETM_MAGIC] = buff_in[i + CS_ETM_MAGIC]; > > + metadata[CS_ETM_CPU] = buff_in[i + CS_ETM_CPU]; > > + metadata[CS_ETM_NR_TRC_PARAMS] = nr_in_params; > > + /* remaining block params at offset +1 from source */ > > + for (k = CS_ETM_ETMCR; k < nr_in_params; k++) > > + metadata[k+1] = buff_in[i + k]; > > Here it misses to copy data into metadata[CS_ETM_ETMCR], you could see > it starts to copy from offset 3 (k = CS_ETM_ETMCR) from buff_in, > actually it's the item for CS_ETM_ETMTRACEIDR in the metadata v0. > > So need to change the logic as below; please confirm if I > misunderstand or not. > > for (k = CS_ETM_ETMCR - 1; k < nr_in_params; k++) > metadata[k+1] = buff_in[i + k]; >
You are correct - this is the correct code for v0 metadata - I will fix for v4 of the patch.
Thanks
Mike
> Except this issue for ETMv3, I didn't find any other logic issue for > ETMv4; I tested and confirmed the tool shows the consistent result for > metadata v0 before and after applying this patch; and it handles > metadata v1 correctly. > > So after fixed the issue for ETMv3, the change looks good to me: > > Reviewed-by: Leo Yan <leo.yan@linaro.org> > Tested-by: Leo Yan <leo.yan@linaro.org> > > Thanks! > > > + /* version 0 has 2 common params */ > > + nr_cmn_params = 2; > > + } else { > > + /* read version 1 info block - input and output nr_params may differ */ > > + /* version 1 has 3 common params */ > > + nr_cmn_params = 3; > > + nr_in_params = buff_in[i + CS_ETM_NR_TRC_PARAMS]; > > + > > + /* if input has more params than output - skip excess */ > > + nr_out_params = nr_in_params + nr_cmn_params; > > + if (nr_out_params > out_blk_size) > > + nr_out_params = out_blk_size; > > + > > + for (k = CS_ETM_MAGIC; k < nr_out_params; k++) > > + metadata[k] = buff_in[i + k]; > > + > > + /* record the actual nr params we copied */ > > + metadata[CS_ETM_NR_TRC_PARAMS] = nr_out_params - nr_cmn_params; > > + } > > + > > + /* adjust in offset by number of in params used */ > > + i += nr_in_params + nr_cmn_params; > > + *buff_in_offset = i; > > + return metadata; > > +} > > + > > int cs_etm__process_auxtrace_info(union perf_event *event, > > struct perf_session *session) > > { > > @@ -2492,11 +2635,12 @@ int cs_etm__process_auxtrace_info(union perf_event *event, > > int info_header_size; > > int total_size = auxtrace_info->header.size; > > int priv_size = 0; > > - int num_cpu; > > - int err = 0, idx = -1; > > - int i, j, k; > > + int num_cpu, trcidr_idx; > > + int err = 0; > > + int i, j; > > u64 *ptr, *hdr = NULL; > > u64 **metadata = NULL; > > + u64 hdr_version; > > > > /* > > * sizeof(auxtrace_info_event::type) + > > @@ -2512,16 +2656,21 @@ int cs_etm__process_auxtrace_info(union perf_event *event, > > /* First the global part */ > > ptr = (u64 *) auxtrace_info->priv; > > > > - /* Look for version '0' of the header */ > > - if (ptr[0] != 0) > > + /* Look for version of the header */ > > + hdr_version = ptr[0]; > > + if (hdr_version > CS_HEADER_CURRENT_VERSION) { > > + /* print routine will print an error on bad version */ > > + if (dump_trace) > > + cs_etm__print_auxtrace_info(auxtrace_info->priv, 0); > > return -EINVAL; > > + } > > > > - hdr = zalloc(sizeof(*hdr) * CS_HEADER_VERSION_0_MAX); > > + hdr = zalloc(sizeof(*hdr) * CS_HEADER_VERSION_MAX); > > if (!hdr) > > return -ENOMEM; > > > > /* Extract header information - see cs-etm.h for format */ > > - for (i = 0; i < CS_HEADER_VERSION_0_MAX; i++) > > + for (i = 0; i < CS_HEADER_VERSION_MAX; i++) > > hdr[i] = ptr[i]; > > num_cpu = hdr[CS_PMU_TYPE_CPUS] & 0xffffffff; > > pmu_type = (unsigned int) ((hdr[CS_PMU_TYPE_CPUS] >> 32) & > > @@ -2552,35 +2701,31 @@ int cs_etm__process_auxtrace_info(union perf_event *event, > > */ > > for (j = 0; j < num_cpu; j++) { > > if (ptr[i] == __perf_cs_etmv3_magic) { > > - metadata[j] = zalloc(sizeof(*metadata[j]) * > > - CS_ETM_PRIV_MAX); > > - if (!metadata[j]) { > > - err = -ENOMEM; > > - goto err_free_metadata; > > - } > > - for (k = 0; k < CS_ETM_PRIV_MAX; k++) > > - metadata[j][k] = ptr[i + k]; > > + metadata[j] = > > + cs_etm__create_meta_blk(ptr, &i, > > + CS_ETM_PRIV_MAX, > > + CS_ETM_NR_TRC_PARAMS_V0); > > > > /* The traceID is our handle */ > > - idx = metadata[j][CS_ETM_ETMTRACEIDR]; > > - i += CS_ETM_PRIV_MAX; > > + trcidr_idx = CS_ETM_ETMTRACEIDR; > > + > > } else if (ptr[i] == __perf_cs_etmv4_magic) { > > - metadata[j] = zalloc(sizeof(*metadata[j]) * > > - CS_ETMV4_PRIV_MAX); > > - if (!metadata[j]) { > > - err = -ENOMEM; > > - goto err_free_metadata; > > - } > > - for (k = 0; k < CS_ETMV4_PRIV_MAX; k++) > > - metadata[j][k] = ptr[i + k]; > > + metadata[j] = > > + cs_etm__create_meta_blk(ptr, &i, > > + CS_ETMV4_PRIV_MAX, > > + CS_ETMV4_NR_TRC_PARAMS_V0); > > > > /* The traceID is our handle */ > > - idx = metadata[j][CS_ETMV4_TRCTRACEIDR]; > > - i += CS_ETMV4_PRIV_MAX; > > + trcidr_idx = CS_ETMV4_TRCTRACEIDR; > > + } > > + > > + if (!metadata[j]) { > > + err = -ENOMEM; > > + goto err_free_metadata; > > } > > > > /* Get an RB node for this CPU */ > > - inode = intlist__findnew(traceid_list, idx); > > + inode = intlist__findnew(traceid_list, metadata[j][trcidr_idx]); > > > > /* Something went wrong, no need to continue */ > > if (!inode) { > > @@ -2601,7 +2746,7 @@ int cs_etm__process_auxtrace_info(union perf_event *event, > > } > > > > /* > > - * Each of CS_HEADER_VERSION_0_MAX, CS_ETM_PRIV_MAX and > > + * Each of CS_HEADER_VERSION_MAX, CS_ETM_PRIV_MAX and > > * CS_ETMV4_PRIV_MAX mark how many double words are in the > > * global metadata, and each cpu's metadata respectively. > > * The following tests if the correct number of double words was > > @@ -2703,6 +2848,12 @@ int cs_etm__process_auxtrace_info(union perf_event *event, > > intlist__delete(traceid_list); > > err_free_hdr: > > zfree(&hdr); > > - > > + /* > > + * At this point, as a minimum we have valid header. Dump the rest of > > + * the info section - the print routines will error out on structural > > + * issues. > > + */ > > + if (dump_trace) > > + cs_etm__print_auxtrace_info(auxtrace_info->priv, num_cpu); > > return err; > > } > > diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h > > index 4ad925d6d799..e153d02df0de 100644 > > --- a/tools/perf/util/cs-etm.h > > +++ b/tools/perf/util/cs-etm.h > > @@ -17,23 +17,37 @@ struct perf_session; > > */ > > enum { > > /* Starting with 0x0 */ > > - CS_HEADER_VERSION_0, > > + CS_HEADER_VERSION, > > /* PMU->type (32 bit), total # of CPUs (32 bit) */ > > CS_PMU_TYPE_CPUS, > > CS_ETM_SNAPSHOT, > > - CS_HEADER_VERSION_0_MAX, > > + CS_HEADER_VERSION_MAX, > > }; > > > > +/* > > + * Update the version for new format. > > + * > > + * New version 1 format adds a param count to the per cpu metadata. > > + * This allows easy adding of new metadata parameters. > > + * Requires that new params always added after current ones. > > + * Also allows client reader to handle file versions that are different by > > + * checking the number of params in the file vs the number expected. > > + */ > > +#define CS_HEADER_CURRENT_VERSION 1 > > + > > /* Beginning of header common to both ETMv3 and V4 */ > > enum { > > CS_ETM_MAGIC, > > CS_ETM_CPU, > > + /* Number of trace config params in following ETM specific block */ > > + CS_ETM_NR_TRC_PARAMS, > > + CS_ETM_COMMON_BLK_MAX_V1, > > }; > > > > /* ETMv3/PTM metadata */ > > enum { > > /* Dynamic, configurable parameters */ > > - CS_ETM_ETMCR = CS_ETM_CPU + 1, > > + CS_ETM_ETMCR = CS_ETM_COMMON_BLK_MAX_V1, > > CS_ETM_ETMTRACEIDR, > > /* RO, taken from sysFS */ > > CS_ETM_ETMCCER, > > @@ -41,10 +55,13 @@ enum { > > CS_ETM_PRIV_MAX, > > }; > > > > +/* define fixed version 0 length - allow new format reader to read old files. */ > > +#define CS_ETM_NR_TRC_PARAMS_V0 (CS_ETM_ETMIDR - CS_ETM_ETMCR + 1) > > + > > /* ETMv4 metadata */ > > enum { > > /* Dynamic, configurable parameters */ > > - CS_ETMV4_TRCCONFIGR = CS_ETM_CPU + 1, > > + CS_ETMV4_TRCCONFIGR = CS_ETM_COMMON_BLK_MAX_V1, > > CS_ETMV4_TRCTRACEIDR, > > /* RO, taken from sysFS */ > > CS_ETMV4_TRCIDR0, > > @@ -55,6 +72,9 @@ enum { > > CS_ETMV4_PRIV_MAX, > > }; > > > > +/* 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) > > + > > /* > > * ETMv3 exception encoding number: > > * See Embedded Trace Macrocell spcification (ARM IHI 0014Q) > > @@ -162,7 +182,7 @@ struct cs_etm_packet_queue { > > > > #define BMVAL(val, lsb, msb) ((val & GENMASK(msb, lsb)) >> lsb) > > > > -#define CS_ETM_HEADER_SIZE (CS_HEADER_VERSION_0_MAX * sizeof(u64)) > > +#define CS_ETM_HEADER_SIZE (CS_HEADER_VERSION_MAX * sizeof(u64)) > > > > #define __perf_cs_etmv3_magic 0x3030303030303030ULL > > #define __perf_cs_etmv4_magic 0x4040404040404040ULL > > -- > > 2.17.1 > >
-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK
| |