[PATCH v2] perf pmu: Validate raw event with sysfs exported format bits

[PATCH v2] perf pmu: Validate raw event with sysfs exported format bits

[Jin Yao]

A raw PMU event (eventsel+umask) in the form of rNNN is supported
by perf but lacks of checking for the validity of raw encoding.

For example, bit 16 and bit 17 are not valid on KBL but perf doesn't
report warning when encoding with these bits.

Before:

  # ./perf stat -e cpu/r031234/ -a -- sleep 1

   Performance counter stats for 'system wide':

                   0      cpu/r031234/

         1.003798924 seconds time elapsed

It may silently measure the wrong event!

The kernel supported bits have been exported through
/sys/devices/<pmu>/format/. Perf collects the information to
'struct perf_pmu_format' and links it to 'pmu->format' list.

The 'struct perf_pmu_format' has a bitmap which records the
valid bits for this format. For example,

  root@kbl-ppc:/sys/devices/cpu/format# cat umask
  config:8-15

bit8-bit15 are recorded in bitmap of format 'umask'.

We collect total valid bits of all formats, save to a local variable
'masks' and reverse it. Now '~masks' represents total invalid bits.

bits = config & ~masks;

The set bits in 'bits' indicate the invalid bits used in config.
Finally use strbuf to report the invalid bits.

Some architectures may not export supported bits through sysfs,
so if masks is 0, perf_pmu__warn_invalid_config directly returns.

After:

Single event without name:

  # ./perf stat -e cpu/r031234/ -a -- sleep 1
  WARNING: event not valid (bits 16 17 of config '31234' not supported by kernel)!

   Performance counter stats for 'system wide':

                   0      cpu/r031234/

         1.001414935 seconds time elapsed

Multiple events with names:

  # ./perf stat -e cpu/rf01234,name=aaa/,cpu/r031234,name=bbb/ -a -- sleep 1
  WARNING: event 'aaa' not valid (bits 20 22 of config 'f01234' not supported by kernel)!
  WARNING: event 'bbb' not valid (bits 16 17 of config '31234' not supported by kernel)!

   Performance counter stats for 'system wide':

                   0      aaa
                  36      bbb

         1.001565999 seconds time elapsed

Warning is reported for invalid bits.

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/util/parse-events.c |  3 +++
 tools/perf/util/pmu.c          | 46 ++++++++++++++++++++++++++++++++++
 tools/perf/util/pmu.h          |  3 +++
 3 files changed, 52 insertions(+)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 42c84adeb2fb..c0c0fab22cb8 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -356,6 +356,9 @@ __add_event(struct list_head *list, int *idx,
 	struct perf_cpu_map *cpus = pmu ? perf_cpu_map__get(pmu->cpus) :
 			       cpu_list ? perf_cpu_map__new(cpu_list) : NULL;
 
+	if (pmu && attr->type == PERF_TYPE_RAW)
+		perf_pmu__warn_invalid_config(pmu, attr->config, name);
+
 	if (init_attr)
 		event_attr_init(attr);
 
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 44ef28302fc7..31e975b75766 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1812,3 +1812,49 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu)
 
 	return nr_caps;
 }
+
+void perf_pmu__warn_invalid_config(struct perf_pmu *pmu, __u64 config,
+				   char *name)
+{
+	struct perf_pmu_format *format;
+	__u64 masks = 0, bits;
+	struct strbuf buf = STRBUF_INIT;
+	unsigned int i;
+
+	list_for_each_entry(format, &pmu->format, list)	{
+		/*
+		 * Skip extra configs such as config1/config2.
+		 */
+		if (format->value > 0)
+			continue;
+
+		for_each_set_bit(i, format->bits, PERF_PMU_FORMAT_BITS)
+			masks |= 1ULL << i;
+	}
+
+	/*
+	 * Kernel doesn't export any valid format bits.
+	 */
+	if (masks == 0)
+		goto out;
+
+	bits = config & ~masks;
+	if (bits == 0)
+		goto out;
+
+	for_each_set_bit(i, (unsigned long *)&bits, sizeof(bits) * 8)
+		strbuf_addf(&buf, " %d", i);
+
+	if (name) {
+		pr_warning("WARNING: event '%s' not valid (bits%s of config "
+			   "'%llx' not supported by kernel)!\n",
+			   name, buf.buf, config);
+	} else {
+		pr_warning("WARNING: event not valid (bits%s of config "
+			   "'%llx' not supported by kernel)!\n",
+			   buf.buf, config);
+	}
+
+out:
+	strbuf_release(&buf);
+}
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index 8164388478c6..160b0f561771 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -123,4 +123,7 @@ int perf_pmu__convert_scale(const char *scale, char **end, double *sval);
 
 int perf_pmu__caps_parse(struct perf_pmu *pmu);
 
+void perf_pmu__warn_invalid_config(struct perf_pmu *pmu, __u64 config,
+				   char *name);
+
 #endif /* __PMU_H */
-- 
2.17.1

Re: [PATCH v2] perf pmu: Validate raw event with sysfs exported format bits

[Jiri Olsa]

On Fri, Mar 05, 2021 at 08:56:25AM +0800, Jin Yao wrote:
> A raw PMU event (eventsel+umask) in the form of rNNN is supported
> by perf but lacks of checking for the validity of raw encoding.
> 
> For example, bit 16 and bit 17 are not valid on KBL but perf doesn't
> report warning when encoding with these bits.
> 
> Before:
> 
>   # ./perf stat -e cpu/r031234/ -a -- sleep 1
> 
>    Performance counter stats for 'system wide':
> 
>                    0      cpu/r031234/
> 
>          1.003798924 seconds time elapsed
> 
> It may silently measure the wrong event!
> 
> The kernel supported bits have been exported through
> /sys/devices/<pmu>/format/. Perf collects the information to
> 'struct perf_pmu_format' and links it to 'pmu->format' list.
> 
> The 'struct perf_pmu_format' has a bitmap which records the
> valid bits for this format. For example,
> 
>   root@kbl-ppc:/sys/devices/cpu/format# cat umask
>   config:8-15
> 
> bit8-bit15 are recorded in bitmap of format 'umask'.
> 
> We collect total valid bits of all formats, save to a local variable
> 'masks' and reverse it. Now '~masks' represents total invalid bits.
> 
> bits = config & ~masks;
> 
> The set bits in 'bits' indicate the invalid bits used in config.
> Finally use strbuf to report the invalid bits.
> 
> Some architectures may not export supported bits through sysfs,
> so if masks is 0, perf_pmu__warn_invalid_config directly returns.
> 
> After:
> 
> Single event without name:
> 
>   # ./perf stat -e cpu/r031234/ -a -- sleep 1
>   WARNING: event not valid (bits 16 17 of config '31234' not supported by kernel)!
> 
>    Performance counter stats for 'system wide':
> 
>                    0      cpu/r031234/
> 
>          1.001414935 seconds time elapsed
> 
> Multiple events with names:
> 
>   # ./perf stat -e cpu/rf01234,name=aaa/,cpu/r031234,name=bbb/ -a -- sleep 1
>   WARNING: event 'aaa' not valid (bits 20 22 of config 'f01234' not supported by kernel)!
>   WARNING: event 'bbb' not valid (bits 16 17 of config '31234' not supported by kernel)!
> 
>    Performance counter stats for 'system wide':
> 
>                    0      aaa
>                   36      bbb
> 
>          1.001565999 seconds time elapsed
> 
> Warning is reported for invalid bits.
> 
> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
> ---
>  tools/perf/util/parse-events.c |  3 +++
>  tools/perf/util/pmu.c          | 46 ++++++++++++++++++++++++++++++++++
>  tools/perf/util/pmu.h          |  3 +++
>  3 files changed, 52 insertions(+)
> 
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 42c84adeb2fb..c0c0fab22cb8 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -356,6 +356,9 @@ __add_event(struct list_head *list, int *idx,
>  	struct perf_cpu_map *cpus = pmu ? perf_cpu_map__get(pmu->cpus) :
>  			       cpu_list ? perf_cpu_map__new(cpu_list) : NULL;
>  
> +	if (pmu && attr->type == PERF_TYPE_RAW)
> +		perf_pmu__warn_invalid_config(pmu, attr->config, name);
> +
>  	if (init_attr)
>  		event_attr_init(attr);
>  
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 44ef28302fc7..31e975b75766 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -1812,3 +1812,49 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu)
>  
>  	return nr_caps;
>  }
> +
> +void perf_pmu__warn_invalid_config(struct perf_pmu *pmu, __u64 config,
> +				   char *name)
> +{
> +	struct perf_pmu_format *format;
> +	__u64 masks = 0, bits;
> +	struct strbuf buf = STRBUF_INIT;
> +	unsigned int i;
> +
> +	list_for_each_entry(format, &pmu->format, list)	{
> +		/*
> +		 * Skip extra configs such as config1/config2.
> +		 */
> +		if (format->value > 0)
> +			continue;
> +
> +		for_each_set_bit(i, format->bits, PERF_PMU_FORMAT_BITS)
> +			masks |= 1ULL << i;
> +	}
> +
> +	/*
> +	 * Kernel doesn't export any valid format bits.
> +	 */
> +	if (masks == 0)
> +		goto out;
> +
> +	bits = config & ~masks;
> +	if (bits == 0)
> +		goto out;
> +
> +	for_each_set_bit(i, (unsigned long *)&bits, sizeof(bits) * 8)
> +		strbuf_addf(&buf, " %d", i);
> +
> +	if (name) {
> +		pr_warning("WARNING: event '%s' not valid (bits%s of config "
> +			   "'%llx' not supported by kernel)!\n",
> +			   name, buf.buf, config);
> +	} else {
> +		pr_warning("WARNING: event not valid (bits%s of config "
> +			   "'%llx' not supported by kernel)!\n",
> +			   buf.buf, config);
> +	}
> +
> +out:
> +	strbuf_release(&buf);
> +}

actualy we do have bitmap_scnprintf, which is already doing the
printing in a nice way.. we could use it like below, only compile
tested

jirka


---
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 31e975b75766..37ca5eee2667 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1818,7 +1818,7 @@ void perf_pmu__warn_invalid_config(struct perf_pmu *pmu, __u64 config,
 {
 	struct perf_pmu_format *format;
 	__u64 masks = 0, bits;
-	struct strbuf buf = STRBUF_INIT;
+	char buf[50];
 	unsigned int i;
 
 	list_for_each_entry(format, &pmu->format, list)	{
@@ -1836,25 +1836,15 @@ void perf_pmu__warn_invalid_config(struct perf_pmu *pmu, __u64 config,
 	 * Kernel doesn't export any valid format bits.
 	 */
 	if (masks == 0)
-		goto out;
+		return;
 
 	bits = config & ~masks;
 	if (bits == 0)
-		goto out;
-
-	for_each_set_bit(i, (unsigned long *)&bits, sizeof(bits) * 8)
-		strbuf_addf(&buf, " %d", i);
+		return;
 
-	if (name) {
-		pr_warning("WARNING: event '%s' not valid (bits%s of config "
-			   "'%llx' not supported by kernel)!\n",
-			   name, buf.buf, config);
-	} else {
-		pr_warning("WARNING: event not valid (bits%s of config "
-			   "'%llx' not supported by kernel)!\n",
-			   buf.buf, config);
-	}
+	bitmap_scnprintf((unsigned long *)&bits, sizeof(bits) * 8, buf, sizeof(buf));
 
-out:
-	strbuf_release(&buf);
+	pr_warning("WARNING: event '%s' not valid (bits%s of config "
+		   "'%llx' not supported by kernel)!\n",
+		   name ?: "N/A", buf, config);
 }

Re: [PATCH v2] perf pmu: Validate raw event with sysfs exported format bits

[Jin, Yao]

Hi Jiri,

On 3/7/2021 3:12 AM, Jiri Olsa wrote:
> actualy we do have bitmap_scnprintf, which is already doing the
> printing in a nice way.. we could use it like below, only compile
> tested
> 
> jirka
> 
> 
> ---
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 31e975b75766..37ca5eee2667 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -1818,7 +1818,7 @@ void perf_pmu__warn_invalid_config(struct perf_pmu *pmu, __u64 config,
>   {
>   	struct perf_pmu_format *format;
>   	__u64 masks = 0, bits;
> -	struct strbuf buf = STRBUF_INIT;
> +	char buf[50];
>   	unsigned int i;
>   
>   	list_for_each_entry(format, &pmu->format, list)	{
> @@ -1836,25 +1836,15 @@ void perf_pmu__warn_invalid_config(struct perf_pmu *pmu, __u64 config,
>   	 * Kernel doesn't export any valid format bits.
>   	 */
>   	if (masks == 0)
> -		goto out;
> +		return;
>   
>   	bits = config & ~masks;
>   	if (bits == 0)
> -		goto out;
> -
> -	for_each_set_bit(i, (unsigned long *)&bits, sizeof(bits) * 8)
> -		strbuf_addf(&buf, " %d", i);
> +		return;
>   
> -	if (name) {
> -		pr_warning("WARNING: event '%s' not valid (bits%s of config "
> -			   "'%llx' not supported by kernel)!\n",
> -			   name, buf.buf, config);
> -	} else {
> -		pr_warning("WARNING: event not valid (bits%s of config "
> -			   "'%llx' not supported by kernel)!\n",
> -			   buf.buf, config);
> -	}
> +	bitmap_scnprintf((unsigned long *)&bits, sizeof(bits) * 8, buf, sizeof(buf));
>   
> -out:
> -	strbuf_release(&buf);
> +	pr_warning("WARNING: event '%s' not valid (bits%s of config "
> +		   "'%llx' not supported by kernel)!\n",
> +		   name ?: "N/A", buf, config);
>   }

The bitmap_scnprintf works fine. Yes, it's really better than original 'strbuf' way, thanks!

BTW, I added "Co-developed-by: Jiri Olsa <jolsa@redhat.com>" in v3, but the patch checking shows the 
warning:

WARNING: Co-developed-by and Signed-off-by: name/email do not match
#76:
Co-developed-by: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Jin Yao <yao.jin@linux.intel.com>

I'm not sure if this warning is important or not, but anyway I add the line to v3. Thanks for your 
help！

Jin Yao