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