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: <8ef9f52d-0ec6-8093-d1ea-ffa3450699fa@linux.intel.com>
Date:   Wed, 29 May 2019 10:41:49 -0400
From:   "Liang, Kan" <kan.liang@...ux.intel.com>
To:     Peter Zijlstra <peterz@...radead.org>
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 5/29/2019 3:34 AM, Peter Zijlstra wrote:
>>>> +			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() ?

Perf metrics event doesn't support sampling.
SLOTs/FIXED3 event + Perf metrics event don't support sampling either.

Only the case SLOTs/FIXED3 event without Perf metrics events support 
sampling. But the case doesn't reach this code path. It is handled by 
the normal routing, like other sampling events.

So there is no sampling event in this code path.

> 
>> 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?

Right, it looks like there is a corner case we cannot handle. The NMI 
hit can happen right after rdpmcl, but before reset.

My current thought is to disable the METRICS and SLOTs counter for first 
read. I will think about it.

Thanks,
Kan