linux-kernel - Re: [RFC v2 4/4] perf tools: Support "branch-misses:pp" on arm64

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, 13 Nov 2019 14:47:43 +0000
From:   James Clark <James.Clark@....com>
To:     Tan Xiaojun <tanxiaojun@...wei.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-perf-users@...r.kernel.org" <linux-perf-users@...r.kernel.org>,
        nd <nd@....com>
Subject: Re: [RFC v2 4/4] perf tools: Support "branch-misses:pp" on arm64

Hi Xiaojun,

> I can't reproduce this problem. If the current system doesn't support spe, it shouldn't report an error. I use the latest codes of the mainline:

I think the problem is related to the 'type' attribute of the event. To open the SPE PMU the event type on the platform I'm using is '7'. If I change
the code like this, the problem is fixed:

@@ -914,13 +914,27 @@ void arm_spe_precise_ip_support(struct evlist *evlist, struct evsel *evsel)
                pmu = perf_pmu__find("arm_spe_0");
                if (pmu) {
                        evsel->pmu_name = pmu->name;
-                       evsel->core.attr.type = PERF_RECORD_AUXTRACE;
-                       evsel->core.attr.config = SPE_ATTR_TS_ENABLE
-                                               | SPE_ATTR_PA_ENABLE
-                                               | SPE_ATTR_JITTER
+                       evsel->core.attr.type = pmu->type;
+                       evsel->core.attr.config |= SPE_ATTR_TS_ENABLE
                                                | SPE_ATTR_BRANCH_FILTER;

Also do you think jitter should be enabled by default? I thought that it might make the data less precise, so I removed it here.

-James

> 
> commit f116b96685a046a89c25d4a6ba2da489145c8888 (mainline/master)
> Merge: f632bfaa33ed 603d9299da32
> Author: Linus Torvalds <torvalds@...ux-foundation.org>
> Date:   Thu Oct 24 06:13:45 2019 -0400
> 
>     Merge tag 'mfd-fixes-5.4' of git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd
> 
> I will go and see why this will be reported.
> 
>>
>>
>> I would have expected to use the event name that is listed in the SPE documentation for branch misses which is br_mis_pred or br_mis_pred_retired:
>>
>>     E[7], byte 0 bit [7]
>>     Mispredicted. The defined values of this bit are:
>>     0 Did not cause correction to the predicted program flow.
>>     1 A branch that caused a correction to the predicted program flow.
>>
>>     If PMUv3 is implemented this Event is required to be implemented consistently with either BR_MIS_PRED or BR_MIS_PRED_RETIRED.
>>
> 
> Do you mean that I can add these as new events to perf? If we think of them as new events, what should we do if the user does not add :pp for them?
> (Or for these events, users can only add :pp to use them?)
> 
>>
>> +       if (!strcmp(perf_env__arch(evlist->env), "arm64")
>> +                       && evsel->core.attr.config == PERF_COUNT_HW_BRANCH_MISSES
>> +                       && evsel->core.attr.precise_ip) {
>>
>> As I mentioned above PERF_COUNT_HW_BRANCH_MISSESdoesn't seem to match up with the actual event counter that is associated with this SPE event (BR_MIS_PRED). The fix for this is probably as simple as adding an OR for the other aliases for branch mispredicts.
> 
> What you mean is that we can filter with spe events(like BR_MIS_PRED) first, and if we have other events that are exactly the same(no more for now), then we can handle them by adding OR in the future?
> 
>>
>> +               pmu = perf_pmu__find("arm_spe_0");
>> +               if (pmu) {
>> +                       evsel->pmu_name = pmu->name;
>> +                       evsel->core.attr.type = PERF_RECORD_AUXTRACE;
>> +                       evsel->core.attr.config = SPE_ATTR_TS_ENABLE
>> +                                               | SPE_ATTR_PA_ENABLE
>>
>> I wouldn't set physical addresses by default as this requires root. I would leave that to the user if they want to manually configure SPE.
> 
> Yes. You are right. I got a error for this case. I will fix it.
> 
> ------------------
> ./perf record -e branch-misses:p ls
> Error:
> You may not have permission to collect stats.
> ...
> ------------------
> 
> Thanks.
> Xiaojun.
> 
>>
>> I have only looked briefly and I will do some more testing.
>>
>>
>> Thanks
>> James
>>
>>
> 
>