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

[PATCH] 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__config_valid just returns true.

After:

Single event:

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

   Performance counter stats for 'system wide':

                   0      cpu/r031234/

         1.001403757 seconds time elapsed

Multiple events:

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

   Performance counter stats for 'system wide':

                   0      cpu/rf01234/
                   0      cpu/r031234/

The warning is reported for invalid bits.

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

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 42c84adeb2fb..1820b2c0a241 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -356,6 +356,17 @@ __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) {
+		struct strbuf buf = STRBUF_INIT;
+
+		if (!perf_pmu__config_valid(pmu, attr->config, &buf)) {
+			pr_warning("WARNING: event config '%llx' not valid ("
+				   "bits%s not supported by kernel)!\n",
+				   attr->config, buf.buf);
+		}
+		strbuf_release(&buf);
+	}
+
 	if (init_attr)
 		event_attr_init(attr);
 
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 44ef28302fc7..5e361adb2698 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1812,3 +1812,41 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu)
 
 	return nr_caps;
 }
+
+bool perf_pmu__config_valid(struct perf_pmu *pmu, __u64 config,
+			    struct strbuf *buf)
+{
+	struct perf_pmu_format *format;
+	__u64 masks = 0, bits;
+	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)
+		return true;
+
+	bits = config & ~masks;
+	if (bits == 0)
+		return true;
+
+	for (i = 0; i < PERF_PMU_FORMAT_BITS; i++) {
+		if (bits & 0x1)
+			strbuf_addf(buf, " %d", i);
+
+		bits >>= 1;
+	}
+
+	return false;
+}
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index 8164388478c6..980d6430c833 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -8,6 +8,7 @@
 #include <stdbool.h>
 #include "parse-events.h"
 #include "pmu-events/pmu-events.h"
+#include "strbuf.h"
 
 struct evsel_config_term;
 
@@ -123,4 +124,7 @@ int perf_pmu__convert_scale(const char *scale, char **end, double *sval);
 
 int perf_pmu__caps_parse(struct perf_pmu *pmu);
 
+bool perf_pmu__config_valid(struct perf_pmu *pmu, __u64 config,
+			    struct strbuf *buf);
+
 #endif /* __PMU_H */
-- 
2.17.1

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

[Jiri Olsa]

On Wed, Mar 03, 2021 at 01:17:36PM +0800, Jin Yao wrote:

SNIP

> 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__config_valid just returns true.
> 
> After:
> 
> Single event:
> 
>   # ./perf stat -e cpu/r031234/ -a -- sleep 1
>   WARNING: event config '31234' not valid (bits 16 17 not supported by kernel)!
> 
>    Performance counter stats for 'system wide':
> 
>                    0      cpu/r031234/
> 
>          1.001403757 seconds time elapsed
> 
> Multiple events:
> 
>   # ./perf stat -e cpu/rf01234/,cpu/r031234/ -a -- sleep 1
>   WARNING: event config 'f01234' not valid (bits 20 22 not supported by kernel)!
>   WARNING: event config '31234' not valid (bits 16 17 not supported by kernel)!

right, seems useful

> 
>    Performance counter stats for 'system wide':
> 
>                    0      cpu/rf01234/
>                    0      cpu/r031234/
> 
> The warning is reported for invalid bits.
> 
> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
> ---
>  tools/perf/util/parse-events.c | 11 ++++++++++
>  tools/perf/util/pmu.c          | 38 ++++++++++++++++++++++++++++++++++
>  tools/perf/util/pmu.h          |  4 ++++
>  3 files changed, 53 insertions(+)
> 
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 42c84adeb2fb..1820b2c0a241 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -356,6 +356,17 @@ __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 we want it just for raw/numeric, we could add it earlier on,
like to parse_events_add_numeric call

but perhaps it might be helpful to check any pmu event,
could perhaps reveal some broken format 

> +	if (pmu && attr->type == PERF_TYPE_RAW) {
> +		struct strbuf buf = STRBUF_INIT;
> +
> +		if (!perf_pmu__config_valid(pmu, attr->config, &buf)) {
> +			pr_warning("WARNING: event config '%llx' not valid ("
> +				   "bits%s not supported by kernel)!\n",
> +				   attr->config, buf.buf);
> +		}
> +		strbuf_release(&buf);
> +	}
> +
>  	if (init_attr)
>  		event_attr_init(attr);
>  
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 44ef28302fc7..5e361adb2698 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -1812,3 +1812,41 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu)
>  
>  	return nr_caps;
>  }
> +
> +bool perf_pmu__config_valid(struct perf_pmu *pmu, __u64 config,
> +			    struct strbuf *buf)
> +{
> +	struct perf_pmu_format *format;
> +	__u64 masks = 0, bits;
> +	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)
> +		return true;
> +
> +	bits = config & ~masks;
> +	if (bits == 0)
> +		return true;

so in here you now that there's something wrong, so why
bother with the outside strbuf, when we can easily just
do all the printing in here?

> +
> +	for (i = 0; i < PERF_PMU_FORMAT_BITS; i++) {
> +		if (bits & 0x1)
> +			strbuf_addf(buf, " %d", i);
> +
> +		bits >>= 1;

could you use the for_each_set_bit in here?

thanks,
jirka

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

[Andi Kleen]

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

Just noticed that again. Can you please print the original event as 
string in the message? While it's obvious with rXXX which one it is, 
it may not be obvious with offsetted fields (like umask=0x121212),
and hard to find in a long command line.

-Andi

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

[Jin, Yao]

Hi Jiri,

On 3/5/2021 4:17 AM, Jiri Olsa wrote:
> On Wed, Mar 03, 2021 at 01:17:36PM +0800, Jin Yao wrote:
> 
> SNIP
> 
>> 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__config_valid just returns true.
>>
>> After:
>>
>> Single event:
>>
>>    # ./perf stat -e cpu/r031234/ -a -- sleep 1
>>    WARNING: event config '31234' not valid (bits 16 17 not supported by kernel)!
>>
>>     Performance counter stats for 'system wide':
>>
>>                     0      cpu/r031234/
>>
>>           1.001403757 seconds time elapsed
>>
>> Multiple events:
>>
>>    # ./perf stat -e cpu/rf01234/,cpu/r031234/ -a -- sleep 1
>>    WARNING: event config 'f01234' not valid (bits 20 22 not supported by kernel)!
>>    WARNING: event config '31234' not valid (bits 16 17 not supported by kernel)!
> 
> right, seems useful
> 

Thanks :)

>>
>>     Performance counter stats for 'system wide':
>>
>>                     0      cpu/rf01234/
>>                     0      cpu/r031234/
>>
>> The warning is reported for invalid bits.
>>
>> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
>> ---
>>   tools/perf/util/parse-events.c | 11 ++++++++++
>>   tools/perf/util/pmu.c          | 38 ++++++++++++++++++++++++++++++++++
>>   tools/perf/util/pmu.h          |  4 ++++
>>   3 files changed, 53 insertions(+)
>>
>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
>> index 42c84adeb2fb..1820b2c0a241 100644
>> --- a/tools/perf/util/parse-events.c
>> +++ b/tools/perf/util/parse-events.c
>> @@ -356,6 +356,17 @@ __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 we want it just for raw/numeric, we could add it earlier on,
> like to parse_events_add_numeric call
> 
> but perhaps it might be helpful to check any pmu event,
> could perhaps reveal some broken format
> 

Yes, I think so. So directly checking the attr->config here may cover more cases.

>> +	if (pmu && attr->type == PERF_TYPE_RAW) {
>> +		struct strbuf buf = STRBUF_INIT;
>> +
>> +		if (!perf_pmu__config_valid(pmu, attr->config, &buf)) {
>> +			pr_warning("WARNING: event config '%llx' not valid ("
>> +				   "bits%s not supported by kernel)!\n",
>> +				   attr->config, buf.buf);
>> +		}
>> +		strbuf_release(&buf);
>> +	}
>> +
>>   	if (init_attr)
>>   		event_attr_init(attr);
>>   
>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
>> index 44ef28302fc7..5e361adb2698 100644
>> --- a/tools/perf/util/pmu.c
>> +++ b/tools/perf/util/pmu.c
>> @@ -1812,3 +1812,41 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu)
>>   
>>   	return nr_caps;
>>   }
>> +
>> +bool perf_pmu__config_valid(struct perf_pmu *pmu, __u64 config,
>> +			    struct strbuf *buf)
>> +{
>> +	struct perf_pmu_format *format;
>> +	__u64 masks = 0, bits;
>> +	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)
>> +		return true;
>> +
>> +	bits = config & ~masks;
>> +	if (bits == 0)
>> +		return true;
> 
> so in here you now that there's something wrong, so why
> bother with the outside strbuf, when we can easily just
> do all the printing in here?
> 

For this patch, yes, I don't need to return the strbuf to caller then print outside.

Andi now comments to print the original event as well, so I need to choose #1 pass the event name to 
perf_pmu__config_valid or #2 return the strbuf to caller.

>> +
>> +	for (i = 0; i < PERF_PMU_FORMAT_BITS; i++) {
>> +		if (bits & 0x1)
>> +			strbuf_addf(buf, " %d", i);
>> +
>> +		bits >>= 1;
> 
> could you use the for_each_set_bit in here?
> 

Yes, maybe I can use the code such as :

for_each_set_bit(i, (unsigned long *) &bits, sizeof(bits) * 8)
	strbuf_addf(buf, " %d", i);

Thanks
Jin Yao

> thanks,
> jirka
> 

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

[Jin, Yao]

Hi Andi,

On 3/5/2021 9:10 AM, Andi Kleen wrote:
>> Single event:
>>
>>    # ./perf stat -e cpu/r031234/ -a -- sleep 1
>>    WARNING: event config '31234' not valid (bits 16 17 not supported by kernel)!
> 
> Just noticed that again. Can you please print the original event as
> string in the message? While it's obvious with rXXX which one it is,
> it may not be obvious with offsetted fields (like umask=0x121212),
> and hard to find in a long command line.
> 
> -Andi
> 

Sometime the event name is NULL. In v2, if the event name is not NULL, I will print the event name.

Thanks
Jin Yao