linux-kernel - Re: [PATCH v3 1/3] perf/x86/amd/lbr: Use freeze based on availability

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] [day] [month] [year] [list]

Message-ID: <960ce5ba-a8b1-4db7-b76c-b97869c61f12@amd.com>
Date: Wed, 13 Mar 2024 15:56:43 +0530
From: Sandipan Das <sandipan.das@....com>
To: Ingo Molnar <mingo@...nel.org>
Cc: linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
 x86@...nel.org, peterz@...radead.org, mingo@...hat.com, acme@...nel.org,
 mark.rutland@....com, alexander.shishkin@...ux.intel.com, jolsa@...nel.org,
 namhyung@...nel.org, adrian.hunter@...el.com, tglx@...utronix.de,
 bp@...en8.de, eranian@...gle.com, irogers@...gle.com,
 mario.limonciello@....com, ravi.bangoria@....com, ananth.narayan@....com
Subject: Re: [PATCH v3 1/3] perf/x86/amd/lbr: Use freeze based on availability

On 3/13/2024 3:45 PM, Ingo Molnar wrote:
> 
> * Sandipan Das <sandipan.das@....com> wrote:
> 
>> Currently, it is assumed that LBR Freeze is supported on all processors
>> which have CPUID leaf 0x80000022[EAX] bit 1 set. This is incorrect as
> 
> That's X86_FEATURE_AMD_LBR_V2, right? Should probably be mentioned in the 
> changelog.
> 

Yes. I'll add it to the changelog.

>> the feature availability is additionally dependent on CPUID leaf 
>> 0x80000022[EAX] bit 2 being set which may not be set for all Zen 4 
>> processors. Define a new feature bit for LBR and PMC freeze and set the 
>> freeze enable bit (FLBRI) in DebugCtl (MSR 0x1d9) conditionally.
> 
> What happens on such Zen 4 CPUs that don't support LBR Freeze? Does the CPU 
> just ignore it, or something worse?
> 

In this case, LBR ignores PMC overflows and the branch records keep getting
updated continuously as execution progresses.

>> It should still be possible to use LBR without freeze for profile-guided
>> optimization of user programs by using an user-only branch filter during
>> profiling. When the user-only filter is enabled, branches are no longer
>> recorded after the transition to CPL 0 upon PMI arrival. When branch
>> entries are read in the PMI handler, the branch stack does not change.
>>
>> E.g.
>>
>>   $ perf record -j any,u -e ex_ret_brn_tkn ./workload
>>
>> Since the feature bit is visible under flags in /proc/cpuinfo, it can be 
>> used to determine the feasibility of use-cases which require LBR Freeze 
>> to be supported by the hardware such as profile-guided optimization of 
>> kernels.
> 
> Sounds good to me.
> 
>> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
>> index 4af140cf5719..e47ea31b019d 100644
>> --- a/arch/x86/include/asm/cpufeatures.h
>> +++ b/arch/x86/include/asm/cpufeatures.h
>> @@ -97,7 +97,7 @@
>>  #define X86_FEATURE_SYSENTER32		( 3*32+15) /* "" sysenter in IA32 userspace */
>>  #define X86_FEATURE_REP_GOOD		( 3*32+16) /* REP microcode works well */
>>  #define X86_FEATURE_AMD_LBR_V2		( 3*32+17) /* AMD Last Branch Record Extension Version 2 */
>> -/* FREE, was #define X86_FEATURE_LFENCE_RDTSC		( 3*32+18) "" LFENCE synchronizes RDTSC */
>> +#define X86_FEATURE_AMD_LBR_PMC_FREEZE	( 3*32+18) /* AMD LBR and PMC Freeze */
>>  #define X86_FEATURE_ACC_POWER		( 3*32+19) /* AMD Accumulated Power Mechanism */
>>  #define X86_FEATURE_NOPL		( 3*32+20) /* The NOPL (0F 1F) instructions */
>>  #define X86_FEATURE_ALWAYS		( 3*32+21) /* "" Always-present feature */
> 
> Could you please port this to the latest upstream kernel? The 3*32+18 slot 
> is now used for another purpose, and we need to define a new synthethic 
> CPUID word, word 21 if I'm counting it right.
> 
> Don't forget to increase NCAPINTS from 21 to 22, and consider the fixed 
> asserts in the x86_bug_flags[] definitions in <asm/cpufeature.h>, and the 
> asserts in <asm/disabled-features.h> and <asm/required-features.h>. This 
> new word should probably be added in a separate preparatory patch.
> 

Sure. I'll rebase and send this along with the other changes.
Thanks for the review.