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];
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....@linaro.org>
Tested-by: Leo Yan <leo....@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
>
