Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 10 Mar 2021 12:38:02 +0100
From:   Jiri Olsa <jolsa@redhat.com>
To:     Jin Yao <yao.jin@linux.intel.com>
Cc:     acme@kernel.org, jolsa@kernel.org, peterz@infradead.org,
        mingo@redhat.com, alexander.shishkin@linux.intel.com,
        Linux-kernel@vger.kernel.org, ak@linux.intel.com,
        kan.liang@intel.com, yao.jin@intel.com
Subject: Re: [PATCH v4] perf pmu: Validate raw event with sysfs exported
 format bits
Message-ID: <YEivmv6WCWW9ae59@krava>
References: <20210310051138.12154-1-yao.jin@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210310051138.12154-1-yao.jin@linux.intel.com>
Precedence: bulk

On Wed, Mar 10, 2021 at 01:11:38PM +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
> 
> The valid bits (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 we use bitmap_scnprintf 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 'N/A' not valid (bits 16-17 of config '31234' not supported by kernel)!
> 
>    Performance counter stats for 'system wide':
> 
>                    0      cpu/r031234/
> 
>          1.001597373 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
>                    0      bbb
> 
>          1.001573787 seconds time elapsed
> 
> Warnings are reported for invalid bits.
> 
> Co-developed-by: Jiri Olsa <jolsa@redhat.com>
> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>

Reviewed-by: Jiri Olsa <jolsa@redhat.com>

thanks,
jirka