linux-kernel - Re: [PATCH 1/8] perf/x86: Add msr probe interface

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 20 Mar 2019 16:48:33 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Jiri Olsa <jolsa@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     "Liang, Kan" <kan.liang@...ux.intel.com>,
        Stephane Eranian <eranian@...gle.com>,
        Andy Lutomirski <luto@...nel.org>,
        lkml <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Vince Weaver <vincent.weaver@...ne.edu>,
        Thomas Gleixner <tglx@...utronix.de>,
        Arnaldo Carvalho de Melo <acme@...nel.org>
Subject: Re: [PATCH 1/8] perf/x86: Add msr probe interface

On Mon, Mar 18, 2019 at 07:21:09PM +0100, Jiri Olsa wrote:
> Adding perf_msr_probe function to provide interface for
> checking up on MSR register and add its related event
> attributes if it passes the check.
> 
> User defines following struct for each MSR register:
> 
>   struct perf_msr {
>        u64                       msr;
>        struct attribute        **attrs;
>        bool                    (*test)(int idx, void *data);
>        bool                      no_check;
>   };
> 
> Where:
>   msr      - is the MSR address
>   attrs    - is attributes array to add if the check passed
>   test     - is test function pointer
>   no_check - is bool that bypass the check and adds the
>               attribute without any test
> 
> The array of struct perf_msr is passed into:
> 
>   perf_msr_probe(struct perf_msr *msr, int cnt,
>                 struct attribute **attrs, void *data)
> 
> Together with:
>   cnt   - which is the number of struct msr array elements
>   attrs - which is an array placeholder for added attributes
>           and needs to be big enough
>   data  -which is user pointer passed to the test function
> 
> The perf_msr_probe will executed test code, read the MSR and
> check the value is != 0. If all these tests pass, related
> attributes are added into attrs array.
> 
> Also adding MSR_ATTR macro helper to define attribute array
> from single attribute. It will be used in following patches.

Somewhere along the line you lost the explanation of _why_ we're doing
this; namely: virt sucks.

Also, recently GregKH had a chance to look at perf code and we scored
fairly high on the WTF'o'meter for what we're doing with the attribute
stuff.

He pointed me to sysfs attribute_group::is_visible functions to replace
some of our 'creative' code.

> Signed-off-by: Jiri Olsa <jolsa@...nel.org>
> ---
>  arch/x86/events/Makefile |  2 +-
>  arch/x86/events/probe.c  | 36 ++++++++++++++++++++++++++++++++++++
>  arch/x86/events/probe.h  | 22 ++++++++++++++++++++++
>  3 files changed, 59 insertions(+), 1 deletion(-)
>  create mode 100644 arch/x86/events/probe.c
>  create mode 100644 arch/x86/events/probe.h
> 
> diff --git a/arch/x86/events/Makefile b/arch/x86/events/Makefile
> index b8ccdb5c9244..ec29a466444a 100644
> --- a/arch/x86/events/Makefile
> +++ b/arch/x86/events/Makefile
> @@ -1,4 +1,4 @@
> -obj-y					+= core.o
> +obj-y					+= core.o probe.o
>  obj-y					+= amd/
>  obj-$(CONFIG_X86_LOCAL_APIC)            += msr.o
>  obj-$(CONFIG_CPU_SUP_INTEL)		+= intel/
> diff --git a/arch/x86/events/probe.c b/arch/x86/events/probe.c
> new file mode 100644
> index 000000000000..0052b730c55e
> --- /dev/null
> +++ b/arch/x86/events/probe.c
> @@ -0,0 +1,36 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/export.h>
> +#include <linux/bits.h>
> +#include "probe.h"
> +
> +unsigned long
> +perf_msr_probe(struct perf_msr *msr, int cnt,
> +	       struct attribute **attrs, void *data)
> +{
> +	unsigned long avail = 0;
> +	unsigned int bit;
> +	u64 val;
> +
> +	if (cnt >= BITS_PER_LONG)
> +		return 0;
> +
> +	for (bit = 0; bit < cnt; bit++) {
> +		struct attribute **a = msr[bit].attrs;
> +
> +		if (!msr[bit].no_check) {
> +			if (msr[bit].test && !msr[bit].test(bit, data))
> +				continue;
> +			if (rdmsrl_safe(msr[bit].msr, &val) || !val)
> +				continue;
> +		}
> +
> +		while (*a)
> +			*attrs++ = *a++;
> +
> +		avail |= bit;
> +	}
> +
> +	*attrs = NULL;
> +	return avail;
> +}
> +EXPORT_SYMBOL_GPL(perf_msr_probe);
> diff --git a/arch/x86/events/probe.h b/arch/x86/events/probe.h
> new file mode 100644
> index 000000000000..42dd666533c3
> --- /dev/null
> +++ b/arch/x86/events/probe.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __ARCH_X86_EVENTS_PROBE_H__
> +#define __ARCH_X86_EVENTS_PROBE_H__
> +#include <linux/sysfs.h>
> +
> +#define MSR_ATTR(__n)				\
> +static struct attribute *msr_##__n[] = {	\
> +	&__n.attr.attr,				\
> +	NULL,					\
> +}
> +
> +struct perf_msr {
> +	u64			  msr;
> +	struct attribute	**attrs;
> +	bool			(*test)(int idx, void *data);
> +	bool			  no_check;
> +};
> +
> +unsigned long
> +perf_msr_probe(struct perf_msr *msr, int cnt,
> +	       struct attribute **attrs, void *data);
> +#endif /* __ARCH_X86_EVENTS_PROBE_H__ */
> -- 
> 2.17.2
>