linux-kernel - Re: [PATCH] perf: Fix interpretation of branch records

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: <38c6bc97-0c18-c645-6973-55a0d29083ff@arm.com>
Date:   Thu, 1 Dec 2022 10:17:41 +0000
From:   James Clark <james.clark@....com>
To:     Sandipan Das <sandipan.das@....com>,
        linux-perf-users@...r.kernel.org, acme@...nel.org,
        Anshuman.Khandual@....com
Cc:     linux-kernel@...r.kernel.org,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>
Subject: Re: [PATCH] perf: Fix interpretation of branch records



On 01/12/2022 09:02, Sandipan Das wrote:
> On 11/30/2022 10:21 PM, James Clark wrote:
>> Commit 93315e46b000 ("perf/core: Add speculation info to branch
>> entries") added a new field in between type and new_type. Perf has
>> its own copy of this struct so update it to match the kernel side.
>>
>> This doesn't currently cause any issues because new_type is only used
>> by the Arm BRBE driver which isn't merged yet.
>>
>> Fixes: 93315e46b000 ("perf/core: Add speculation info to branch entries")
> 
> Technically, in the kernel sources, commit 93315e46b000 ("perf/core: Add
> speculation info to branch entries") landed before commit b190bc4ac9e6
> ("perf: Extend branch type classification").
> 
> So I think the Fixes tag should instead have commit 0ddea8e2a0c2
> ("perf branch: Extend branch type classification") which added the
> UAPI changes to the perf tool headers.

Yes maybe, or 831c05a7621b ("tools headers UAPI: Sync linux/perf_event.h
with the kernel sources") where spec was added to the perf copy of the
headers.

It's hard to say for sure which one is best. I think that the earliest
possible one is best in case anyone is debugging that kernel version
with userspace Perf. And to me it seems any kernel that has the spec
record should have it matching in userspace so I would still prefer
93315e46b000. Applying it on top of any other change would still lead to
misleading interpretation of the records.

> 
> Aside from that, the patch looks good to me.

Thanks for the review.

> 
>> Signed-off-by: James Clark <james.clark@....com>
>> ---
>>  tools/perf/util/branch.h | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/branch.h b/tools/perf/util/branch.h
>> index d6017c9b1872..3ed792db1125 100644
>> --- a/tools/perf/util/branch.h
>> +++ b/tools/perf/util/branch.h
>> @@ -22,9 +22,10 @@ struct branch_flags {
>>  			u64 abort:1;
>>  			u64 cycles:16;
>>  			u64 type:4;
>> +			u64 spec:2;
>>  			u64 new_type:4;
>>  			u64 priv:3;
>> -			u64 reserved:33;
>> +			u64 reserved:31;
>>  		};
>>  	};
>>  };
> 
>