linux-kernel - Re: [PATCH v5 6/7] drivers/perf: Add support for ARMv8.2 Statistical Profiling Extension

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:   Mon, 2 Oct 2017 15:14:05 +0100
From:   Will Deacon <will.deacon@....com>
To:     Kim Phillips <kim.phillips@....com>
Cc:     Arnaldo Carvalho de Melo <acme@...hat.com>,
        linux-arm-kernel@...ts.infradead.org, marc.zyngier@....com,
        mark.rutland@....com, tglx@...utronix.de, peterz@...radead.org,
        alexander.shishkin@...ux.intel.com, robh@...nel.org,
        suzuki.poulose@....com, pawel.moll@....com,
        mathieu.poirier@...aro.org, mingo@...hat.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 6/7] drivers/perf: Add support for ARMv8.2 Statistical
 Profiling Extension

Hi Kim,

On Fri, Sep 29, 2017 at 05:19:40PM -0500, Kim Phillips wrote:
> On Thu, 28 Sep 2017 15:09:50 +0100
> Will Deacon <will.deacon@....com> wrote:
> 
> > +/* Perf callbacks */
> > +static int arm_spe_pmu_event_init(struct perf_event *event)
> > +{
> > +	u64 reg;
> > +	struct perf_event_attr *attr = &event->attr;
> > +	struct arm_spe_pmu *spe_pmu = to_spe_pmu(event->pmu);
> > +
> > +	/* This is, of course, deeply driver-specific */
> > +	if (attr->type != event->pmu->type)
> > +		return -ENOENT;
> > +
> > +	if (event->cpu >= 0 &&
> > +	    !cpumask_test_cpu(event->cpu, &spe_pmu->supported_cpus))
> > +		return -ENOENT;
> 
> So -ENOENT will make tools/perf/util/evsel.c tell the user "The %s event is not
> supported." whereas returning -ENODEV will say "No such device - did
> you specify an out-of-range profile CPU?" which may or may not be more
> appropriate for this check.

I agree that the message isn't great, but I'm returning ENOENT here to
be consistent with the CPU PMU behaviour (which necessarily has to return
ENOENT so that we can support ABI event types over there -- see
perf_init_event).

> > +	if (arm_spe_event_to_pmsevfr(event) & SYS_PMSEVFR_EL1_RES0)
> > +		return -EOPNOTSUPP;
> > +	if (attr->exclude_idle)
> > +		return -EOPNOTSUPP;
> 
> "PMU Hardware doesn't support sampling/overflow-interrupts." will be
> printed if the user didn't specify a sample period.  Otherwise, a
> string with "/bin/dmesg may provide additional information." will be
> printed.
> 
> I was hoping for a response from acme by now for this:
> 
> https://www.spinics.net/lists/linux-perf-users/msg04066.html
> 
> Alas, nothing.  Looking at the #ifdef x86 in evsel.c, I'm guessing
> it'll be ok, although I'm still not sure how PMU-specific we can get in
> evsel.c, nor whether it's ok to communicate lists of h/w supported
> sample periods through /sys/bus/event_source/devices/...
> 
> acme?  OK to refactor evsel messaging for Arm, including parsing for
> which PMUs are being used, so customize the message?

Arnaldo's probably got enough on his plate maintaining perf tool, so my
advice would be to post a patch as an RFC and use that as a concrete basis
for discussion. It often works out better starting with code, even if none
of it ends up getting merged (and you can include bits of your email above
in the cover letter).

Will