Re: [PATCH] perf cs-etm: Improve version detection and error reporting

[Ian Rogers]

On Tue, Apr 30, 2024 at 6:32 AM James Clark <james.clark@xxxxxxx> wrote:
>
> When the config validation functions are warning about ETMv3, they do it
> based on "not ETMv4". If the drivers aren't all loaded or the hardware
> doesn't support Coresight it will appear as "not ETMv4" and then Perf
> will print the error message "... not supported in ETMv3 ..." which is
> wrong and confusing.
>
> cs_etm_is_etmv4() is also misnamed because it also returns true for
> ETE because ETE has a superset of the ETMv4 metadata files. Although
> this was always done in the correct order so it wasn't a bug.
>
> Improve all this by making a single get version function which also
> handles not present as a separate case. Change the ETMv3 error message
> to only print when ETMv3 is detected, and add a new error message for
> the not present case.
>
> Signed-off-by: James Clark <james.clark@xxxxxxx>
> ---
>  tools/perf/arch/arm/util/cs-etm.c | 64 +++++++++++++++++++++++--------
>  1 file changed, 48 insertions(+), 16 deletions(-)
>
> diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
> index 07be32d99805..2763c6758b91 100644
> --- a/tools/perf/arch/arm/util/cs-etm.c
> +++ b/tools/perf/arch/arm/util/cs-etm.c
> @@ -66,9 +66,25 @@ static const char * const metadata_ete_ro[] = {
>         [CS_ETE_TS_SOURCE]              = "ts_source",
>  };
>
> -static bool cs_etm_is_etmv4(struct auxtrace_record *itr, int cpu);
> +enum cs_etm_version { CS_NOT_PRESENT, CS_ETMV3, CS_ETMV4, CS_ETE };
> +
> +static bool cs_etm_pmu_file_present(struct auxtrace_record *itr, int cpu,
> +                                      const char *file);
>  static bool cs_etm_is_ete(struct auxtrace_record *itr, int cpu);
>
> +static enum cs_etm_version cs_etm_get_version(struct auxtrace_record *itr,
> +                                             int cpu)

nit: perhaps use "struct perf_cpu" rather than "int cpu" for a little
bit of type safety. Fwiw, most of the tool uses an index into a CPU
map but then have labelled that index "cpu" which is confusing
particularly in the uncore case, whilst a perf_cpu is never an index.

Reviewed-by: Ian Rogers <irogers@xxxxxxxxxx>

Thanks,
Ian

> +{
> +       if (cs_etm_is_ete(itr, cpu))
> +               return CS_ETE;
> +       else if (cs_etm_pmu_file_present(itr, cpu, metadata_etmv4_ro[CS_ETMV4_TRCIDR0]))
> +               return CS_ETMV4;
> +       else if (cs_etm_pmu_file_present(itr, cpu, metadata_etmv3_ro[CS_ETM_ETMCCER]))
> +               return CS_ETMV3;
> +
> +       return CS_NOT_PRESENT;
> +}
> +
>  static int cs_etm_validate_context_id(struct auxtrace_record *itr,
>                                       struct evsel *evsel, int cpu)
>  {
> @@ -87,7 +103,7 @@ static int cs_etm_validate_context_id(struct auxtrace_record *itr,
>                 return 0;
>
>         /* Not supported in etmv3 */
> -       if (!cs_etm_is_etmv4(itr, cpu)) {
> +       if (cs_etm_get_version(itr, cpu) == CS_ETMV3) {
>                 pr_err("%s: contextid not supported in ETMv3, disable with %s/contextid=0/\n",
>                        CORESIGHT_ETM_PMU_NAME, CORESIGHT_ETM_PMU_NAME);
>                 return -EINVAL;
> @@ -154,7 +170,7 @@ static int cs_etm_validate_timestamp(struct auxtrace_record *itr,
>               perf_pmu__format_bits(cs_etm_pmu, "timestamp")))
>                 return 0;
>
> -       if (!cs_etm_is_etmv4(itr, cpu)) {
> +       if (cs_etm_get_version(itr, cpu) == CS_ETMV3) {
>                 pr_err("%s: timestamp not supported in ETMv3, disable with %s/timestamp=0/\n",
>                        CORESIGHT_ETM_PMU_NAME, CORESIGHT_ETM_PMU_NAME);
>                 return -EINVAL;
> @@ -218,6 +234,11 @@ static int cs_etm_validate_config(struct auxtrace_record *itr,
>         }
>
>         perf_cpu_map__for_each_cpu_skip_any(cpu, idx, intersect_cpus) {
> +               if (cs_etm_get_version(itr, cpu.cpu) == CS_NOT_PRESENT) {
> +                       pr_err("%s: Not found on CPU %d. Check hardware and firmware support and that all Coresight drivers are loaded\n",
> +                              CORESIGHT_ETM_PMU_NAME, cpu.cpu);
> +                       return -EINVAL;
> +               }
>                 err = cs_etm_validate_context_id(itr, evsel, cpu.cpu);
>                 if (err)
>                         break;
> @@ -548,13 +569,13 @@ cs_etm_info_priv_size(struct auxtrace_record *itr __maybe_unused,
>                 /* Event can be "any" CPU so count all online CPUs. */
>                 intersect_cpus = perf_cpu_map__new_online_cpus();
>         }
> +       /* Count number of each type of ETM. Don't count if that CPU has CS_NOT_PRESENT. */
>         perf_cpu_map__for_each_cpu_skip_any(cpu, idx, intersect_cpus) {
> -               if (cs_etm_is_ete(itr, cpu.cpu))
> -                       ete++;
> -               else if (cs_etm_is_etmv4(itr, cpu.cpu))
> -                       etmv4++;
> -               else
> -                       etmv3++;
> +               enum cs_etm_version v = cs_etm_get_version(itr, cpu.cpu);
> +
> +               ete   += v == CS_ETE;
> +               etmv4 += v == CS_ETMV4;
> +               etmv3 += v == CS_ETMV3;
>         }
>         perf_cpu_map__put(intersect_cpus);
>
> @@ -564,7 +585,8 @@ cs_etm_info_priv_size(struct auxtrace_record *itr __maybe_unused,
>                (etmv3 * CS_ETMV3_PRIV_SIZE));
>  }
>
> -static bool cs_etm_is_etmv4(struct auxtrace_record *itr, int cpu)
> +static bool cs_etm_pmu_file_present(struct auxtrace_record *itr, int cpu,
> +                                      const char *file)
>  {
>         bool ret = false;
>         char path[PATH_MAX];
> @@ -574,9 +596,7 @@ static bool cs_etm_is_etmv4(struct auxtrace_record *itr, int cpu)
>                         container_of(itr, struct cs_etm_recording, itr);
>         struct perf_pmu *cs_etm_pmu = ptr->cs_etm_pmu;
>
> -       /* Take any of the RO files for ETMv4 and see if it present */
> -       snprintf(path, PATH_MAX, "cpu%d/%s",
> -                cpu, metadata_etmv4_ro[CS_ETMV4_TRCIDR0]);
> +       snprintf(path, PATH_MAX, "cpu%d/%s", cpu, file);
>         scan = perf_pmu__scan_file(cs_etm_pmu, path, "%x", &val);
>
>         /* The file was read successfully, we have a winner */
> @@ -735,21 +755,26 @@ 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_ete(itr, cpu)) {
> +       switch (cs_etm_get_version(itr, cpu)) {
> +       case CS_ETE:
>                 magic = __perf_cs_ete_magic;
>                 cs_etm_save_ete_header(&info->priv[*offset], itr, cpu);
>
>                 /* 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)) {
> +               break;
> +
> +       case CS_ETMV4:
>                 magic = __perf_cs_etmv4_magic;
>                 cs_etm_save_etmv4_header(&info->priv[*offset], itr, cpu);
>
>                 /* How much space was used */
>                 increment = CS_ETMV4_PRIV_MAX;
>                 nr_trc_params = CS_ETMV4_PRIV_MAX - CS_ETMV4_TRCCONFIGR;
> -       } else {
> +               break;
> +
> +       case CS_ETMV3:
>                 magic = __perf_cs_etmv3_magic;
>                 /* Get configuration register */
>                 info->priv[*offset + CS_ETM_ETMCR] = cs_etm_get_config(itr);
> @@ -767,6 +792,13 @@ static void cs_etm_get_metadata(int cpu, u32 *offset,
>                 /* How much space was used */
>                 increment = CS_ETM_PRIV_MAX;
>                 nr_trc_params = CS_ETM_PRIV_MAX - CS_ETM_ETMCR;
> +               break;
> +
> +       default:
> +       case CS_NOT_PRESENT:
> +               /* Unreachable, CPUs already validated in cs_etm_validate_config() */
> +               assert(true);
> +               return;
>         }
>
>         /* Build generic header portion */
> --
> 2.34.1
>