Re: [PATCH v3 05/12] perf evsel: Add a helper to get the value of a config field

[Ian Rogers <irogers@google.com>]

On Fri, Dec 12, 2025 at 7:32 AM James Clark <james.clark@linaro.org> wrote:
>
> This will be used by aux PMUs to read an already written value for
> configuring their events and for also testing.
>
> Its helper pmu_format_unpack() does the opposite of the existing
> pmu_format_value() so rename that one to pmu_format_pack() so it's clear
> how they are related.
>
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  tools/perf/util/evsel.h |  2 ++
>  tools/perf/util/pmu.c   | 77 ++++++++++++++++++++++++++++++++++++++++++-------
>  2 files changed, 68 insertions(+), 11 deletions(-)
>
> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> index a08130ff2e47a887b19f6c47bfa9f51e0c40d226..092904a61ec7afdc59253f9b78a9fe8b7cb5bfa7 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -575,6 +575,8 @@ void evsel__uniquify_counter(struct evsel *counter);
>         ((((src) >> (pos)) & ((1ull << (size)) - 1)) << (63 - ((pos) + (size) - 1)))
>
>  u64 evsel__bitfield_swap_branch_flags(u64 value);
> +int evsel__get_config_val(struct perf_pmu *pmu, struct evsel *evsel,
> +                         const char *config_name, u64 *val);
>  void evsel__set_config_if_unset(struct perf_pmu *pmu, struct evsel *evsel,
>                                 const char *config_name, u64 val);
>
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 514cba91f5d99b399d2d6a1e350971660c54a9fc..ef7358ad1fb955f29f2e68b3d0ce711754e4d67c 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -144,8 +144,8 @@ struct perf_pmu_format {
>  };
>
>  static int pmu_aliases_parse(struct perf_pmu *pmu);
> -static void pmu_format_value(unsigned long *format, __u64 value, __u64 *v,
> -                            bool zero);
> +static void pmu_format_pack(unsigned long *format, __u64 value, __u64 *v,
> +                           bool zero);
>  static struct perf_pmu_format *pmu_find_format(const struct list_head *formats,
>                                                const char *name);
>
> @@ -1377,6 +1377,61 @@ bool evsel__is_aux_event(const struct evsel *evsel)
>         return pmu && pmu->auxtrace;
>  }
>
> +/*
> + * Unpacks a raw config[n] value using the sparse bitfield that defines a
> + * format attr. For example "config1:1,6-7,44" defines a 4 bit value across non
> + * contiguous bits and this function returns those 4 bits as a value.
> + */
> +static u64 pmu_format_unpack(u64 format, u64 config_val)
> +{
> +       int val_bit = 0;
> +       u64 res = 0;
> +       int fmt_bit;
> +
> +       for_each_set_bit(fmt_bit, &format, PERF_PMU_FORMAT_BITS) {
> +               if (test_bit(fmt_bit, &config_val))
> +                       res |= BIT_ULL(val_bit);
> +
> +               val_bit++;
> +       }
> +       return res;
> +}
> +
> +int evsel__get_config_val(struct perf_pmu *pmu, struct evsel *evsel,
> +                         const char *config_name, u64 *val)

nits:
This is an evsel__ function but the evsel is the 2nd argument not the first.
Why pass the PMU and not just read evsel->pmu (or better evsel__find_pmu) ?
Why not place this in evsel.c to match the header file declaration?
The evsel could be likely be const.

Thanks,
Ian

> +{
> +       struct perf_pmu_format *format = pmu_find_format(&pmu->format, config_name);
> +       u64 bits = perf_pmu__format_bits(pmu, config_name);
> +
> +       if (!format || !bits) {
> +               pr_err("Unknown/empty format name: %s\n", config_name);
> +               *val = 0;
> +               return -EINVAL;
> +       }
> +
> +       switch (format->value) {
> +       case PERF_PMU_FORMAT_VALUE_CONFIG:
> +               *val = pmu_format_unpack(bits, evsel->core.attr.config);
> +               return 0;
> +       case PERF_PMU_FORMAT_VALUE_CONFIG1:
> +               *val = pmu_format_unpack(bits, evsel->core.attr.config1);
> +               return 0;
> +       case PERF_PMU_FORMAT_VALUE_CONFIG2:
> +               *val = pmu_format_unpack(bits, evsel->core.attr.config2);
> +               return 0;
> +       case PERF_PMU_FORMAT_VALUE_CONFIG3:
> +               *val = pmu_format_unpack(bits, evsel->core.attr.config3);
> +               return 0;
> +       case PERF_PMU_FORMAT_VALUE_CONFIG4:
> +               *val = pmu_format_unpack(bits, evsel->core.attr.config4);
> +               return 0;
> +       default:
> +               pr_err("Unknown format value: %d\n", format->value);
> +               *val = 0;
> +               return -EINVAL;
> +       }
> +}
> +
>  /*
>   * Set @config_name to @val as long as the user hasn't already set or cleared it
>   * by passing a config term on the command line.
> @@ -1432,7 +1487,7 @@ void evsel__set_config_if_unset(struct perf_pmu *pmu, struct evsel *evsel,
>                 return;
>
>         /* Otherwise replace it */
> -       pmu_format_value(&bits, val, vp, /*zero=*/true);
> +       pmu_format_pack(&bits, val, vp, /*zero=*/true);
>  }
>
>  static struct perf_pmu_format *
> @@ -1477,8 +1532,8 @@ int perf_pmu__format_type(struct perf_pmu *pmu, const char *name)
>   * Sets value based on the format definition (format parameter)
>   * and unformatted value (value parameter).
>   */
> -static void pmu_format_value(unsigned long *format, __u64 value, __u64 *v,
> -                            bool zero)
> +static void pmu_format_pack(unsigned long *format, __u64 value, __u64 *v,
> +                           bool zero)
>  {
>         unsigned long fbit, vbit;
>
> @@ -1595,23 +1650,23 @@ static int pmu_config_term(const struct perf_pmu *pmu,
>                 switch (term->type_term) {
>                 case PARSE_EVENTS__TERM_TYPE_CONFIG:
>                         assert(term->type_val == PARSE_EVENTS__TERM_TYPE_NUM);
> -                       pmu_format_value(bits, term->val.num, &attr->config, zero);
> +                       pmu_format_pack(bits, term->val.num, &attr->config, zero);
>                         break;
>                 case PARSE_EVENTS__TERM_TYPE_CONFIG1:
>                         assert(term->type_val == PARSE_EVENTS__TERM_TYPE_NUM);
> -                       pmu_format_value(bits, term->val.num, &attr->config1, zero);
> +                       pmu_format_pack(bits, term->val.num, &attr->config1, zero);
>                         break;
>                 case PARSE_EVENTS__TERM_TYPE_CONFIG2:
>                         assert(term->type_val == PARSE_EVENTS__TERM_TYPE_NUM);
> -                       pmu_format_value(bits, term->val.num, &attr->config2, zero);
> +                       pmu_format_pack(bits, term->val.num, &attr->config2, zero);
>                         break;
>                 case PARSE_EVENTS__TERM_TYPE_CONFIG3:
>                         assert(term->type_val == PARSE_EVENTS__TERM_TYPE_NUM);
> -                       pmu_format_value(bits, term->val.num, &attr->config3, zero);
> +                       pmu_format_pack(bits, term->val.num, &attr->config3, zero);
>                         break;
>                 case PARSE_EVENTS__TERM_TYPE_CONFIG4:
>                         assert(term->type_val == PARSE_EVENTS__TERM_TYPE_NUM);
> -                       pmu_format_value(bits, term->val.num, &attr->config4, zero);
> +                       pmu_format_pack(bits, term->val.num, &attr->config4, zero);
>                         break;
>                 case PARSE_EVENTS__TERM_TYPE_LEGACY_HARDWARE_CONFIG:
>                         assert(term->type_val == PARSE_EVENTS__TERM_TYPE_NUM);
> @@ -1749,7 +1804,7 @@ static int pmu_config_term(const struct perf_pmu *pmu,
>                  */
>         }
>
> -       pmu_format_value(format->bits, val, vp, zero);
> +       pmu_format_pack(format->bits, val, vp, zero);
>         return 0;
>  }
>
>
> --
> 2.34.1
>