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] [day] [month] [year] [list]

Date:   Thu, 21 Mar 2019 12:09:01 +0100
From:   Jiri Olsa <jolsa@...hat.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Jiri Olsa <jolsa@...nel.org>,
        "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 Wed, Mar 20, 2019 at 05:03:29PM +0100, Greg Kroah-Hartman wrote:
> On Wed, Mar 20, 2019 at 04:48:33PM +0100, Peter Zijlstra wrote:
> > 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;
> 
> Please use attribute groups where ever possible.  I've been working to
> fix up the remaining places that use list of attributes as that is not
> flexible at all (and broken in places.)
> 
> And this is a device, so why not device attributes?

ok, will check

> 
> > >        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.
> 
> Please no, don't we have enough ATTR macros?  Why do you need another
> one?  What are you trying to save code on?
> 
> > 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.
> 
> Yes, that would be very good to do.  If no one is working on it, I can
> take a look next week as I have long plane rides...

if I dont send v2 till then, it's all yours ;-)

thanks,
jirka