linux-kernel - Re: [PATCH 4/9] perf/x86/intel: Support hardware TopDown metrics

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]

Message-ID: <20190529073421.GZ2623@hirez.programming.kicks-ass.net>
Date:   Wed, 29 May 2019 09:34:21 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     "Liang, Kan" <kan.liang@...ux.intel.com>
Cc:     acme@...nel.org, mingo@...hat.com, linux-kernel@...r.kernel.org,
        tglx@...utronix.de, jolsa@...nel.org, eranian@...gle.com,
        alexander.shishkin@...ux.intel.com, ak@...ux.intel.com
Subject: Re: [PATCH 4/9] perf/x86/intel: Support hardware TopDown metrics

On Tue, May 28, 2019 at 02:24:14PM -0400, Liang, Kan wrote:

> > > +		if (!(txn_flags & PERF_PMU_TXN_REMOVE)) {
> > > +			/* Reset the metric value when reading
> > > +			 * The SLOTS register must be reset when PERF_METRICS reset,
> > > +			 * otherwise PERF_METRICS may has wrong output.
> > > +			 */
> > 
> > broken comment style.. (and grammer)
> 
> Missed a full stop.
> Should be "Reset the metric value for each read."

s/may has wrong/may have wrong/

> > > +			wrmsrl(MSR_PERF_METRICS, 0);
> > > +			wrmsrl(MSR_CORE_PERF_FIXED_CTR3, 0);
> > 
> > I don't get this, overflow happens on when we flip sign, so why is
> > programming 0 a sane thing to do?
> 
> Reset the counters (programming 0) don't trigger overflow.

Right, so why then do you allow creating this thing as
is_sampling_event() ?

> We have to reset both registers for each read to avoid the known
> PERF_METRICS issue.

'the known PERF_METRICS issue' is unknown to me and any other reader.

> > > +	metric = (cpuc->last_metric >> ((hwc->idx - INTEL_PMC_IDX_FIXED_METRIC_BASE)*8)) & 0xff;
> > > +	last = (((metric * 0xffff) >> 8) * cpuc->last_slots) >> 16;
> > 
> > How is that cpuc->last_* crap not broken for NMIs ?
> 
> There should be no NMI for slots or metric events at the moment, because the
> MSR_PERF_METRICS and MSR_CORE_PERF_FIXED_CTR3 are reset in first read.
> Other NMIs will not touch the codes here.

What happens if someone does: read(perf_fd) and then has the NMI hit?