Re: [PATCH 2/2 v2 RESEND] x86/cpu/amd: Enable the fixed intructions retired free counter IRPERF

[Borislav Petkov <bp@alien8.de>]

On Fri, Feb 07, 2020 at 05:04:27PM -0600, Kim Phillips wrote:
> commit aaf248848db50 ("perf/x86/msr: Add AMD IRPERF (Instructions
> Retired) performance counter") added support for 'perf -e msr/irperf/',
> but when exercised, we always get a 0 count:
> 
> BEFORE:
> 
> $ sudo perf stat -e instructions,msr/irperf/ true
> 
>  Performance counter stats for 'true':
> 
>            624,833      instructions
>                                                   #    0.00  stalled cycles per insn
>                  0      msr/irperf/
> 
> It turns out it simply needs its enable bit - HWCR bit 30 - set.  This patch

Avoid having "This patch" or "This commit" in the commit message. It is
tautologically useless.

Also, do

$ git grep 'This patch' Documentation/process

for more details.

> does just that.
> 
> Enablement is restricted to all machines advertising IRPERF capability,
> except those susceptible to an erratum that makes the IRPERF return
> bad values.
> 
> That erratum occurs in Family 17h models 00-1fh [1], but not in F17h
> models 20h and above [2].
> 
> AFTER (on a family 17h model 31h machine):
> 
> $ sudo perf stat -e instructions,msr/irperf/ true
> 
>  Performance counter stats for 'true':
> 
>            621,690      instructions
>                                                   #    0.00  stalled cycles per insn
>            622,490      msr/irperf/
> 
> [1] "Revision Guide for AMD Family 17h Models 00h-0Fh Processors",
>     currently available here:
> 
>     https://www.amd.com/system/files/TechDocs/55449_Fam_17h_M_00h-0Fh_Rev_Guide.pdf
> 
> [2] "Revision Guide for AMD Family 17h Models 30h-3Fh Processors",
>     currently available here:
> 
>     https://developer.amd.com/wp-content/resources/56323-PUB_0.74.pdf

How stable are those links? Past experience shows not very.

Please upload those to a bugzilla.kernel.org entry and add that URL here
with a Link: tag.

> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Babu Moger <babu.moger@amd.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Fenghua Yu <fenghua.yu@intel.com>
> Cc: Frank van der Linden <fllinden@amazon.com>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: Huang Rui <ray.huang@amd.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Jiaxun Yang <jiaxun.yang@flygoat.com>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Luwei Kang <luwei.kang@intel.com>
> Cc: Martin Liška <mliska@suse.cz>
> Cc: Matt Fleming <matt@codeblueprint.co.uk>
> Cc: Michael Petlan <mpetlan@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: x86@kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: stable@vger.kernel.org
> Fixes: aaf248848db50 ("perf/x86/msr: Add AMD IRPERF (Instructions Retired) performance counter")
> Signed-off-by: Kim Phillips <kim.phillips@amd.com>
> ---
> RESEND, adding Michael Petlan to cc. Original v2:
> 
> https://lore.kernel.org/lkml/20200121171232.28839-2-kim.phillips@amd.com/
> 
> v2: Based on Andi Kleen's review:
> 
>     https://lore.kernel.org/lkml/20200116040324.GI302770@tassilo.jf.intel.com/
> 
>     Add an amd_erratum_1054 and use cpu_has_bug infrastructure instead of
>     open-coding the {family,model} check.
> 
>  arch/x86/include/asm/cpufeatures.h |  1 +
>  arch/x86/include/asm/msr-index.h   |  2 ++
>  arch/x86/kernel/cpu/amd.c          | 17 +++++++++++++++++
>  3 files changed, 20 insertions(+)
> 
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index f3327cb56edf..1c1600e7476b 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -404,5 +404,6 @@
>  #define X86_BUG_SWAPGS			X86_BUG(21) /* CPU is affected by speculation through SWAPGS */
>  #define X86_BUG_TAA			X86_BUG(22) /* CPU is affected by TSX Async Abort(TAA) */
>  #define X86_BUG_ITLB_MULTIHIT		X86_BUG(23) /* CPU may incur MCE during certain page attribute changes */
> +#define X86_BUG_AMD_E1054		X86_BUG(24) /* CPU is among the affected by Erratum 1054 */

That is visible in /proc/cpuinfo and the string "amd_e1054" means
nothing. Call that

X86_BUG_IRPERF

or so to at least give some hint as to what the bug is.

>  
>  #endif /* _ASM_X86_CPUFEATURES_H */
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index ebe1685e92dd..d5e517d1c3dd 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -512,6 +512,8 @@
>  #define MSR_K7_HWCR			0xc0010015
>  #define MSR_K7_HWCR_SMMLOCK_BIT		0
>  #define MSR_K7_HWCR_SMMLOCK		BIT_ULL(MSR_K7_HWCR_SMMLOCK_BIT)
> +#define MSR_K7_HWCR_IRPERF_EN_BIT	30
> +#define MSR_K7_HWCR_IRPERF_EN		BIT_ULL(MSR_K7_HWCR_IRPERF_EN_BIT)
>  #define MSR_K7_FID_VID_CTL		0xc0010041
>  #define MSR_K7_FID_VID_STATUS		0xc0010042
>  
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index 62c30279be77..c067234a271f 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -28,6 +28,7 @@
>  
>  static const int amd_erratum_383[];
>  static const int amd_erratum_400[];
> +static const int amd_erratum_1054[];
>  static bool cpu_has_amd_erratum(struct cpuinfo_x86 *cpu, const int *erratum);
>  
>  /*
> @@ -701,6 +702,9 @@ static void early_init_amd(struct cpuinfo_x86 *c)
>  	if (cpu_has_amd_erratum(c, amd_erratum_400))
>  		set_cpu_bug(c, X86_BUG_AMD_E400);
>  
> +	if (cpu_has_amd_erratum(c, amd_erratum_1054))
> +		set_cpu_bug(c, X86_BUG_AMD_E1054);
> +
>  	early_detect_mem_encrypt(c);
>  
>  	/* Re-enable TopologyExtensions if switched off by BIOS */
> @@ -978,6 +982,15 @@ static void init_amd(struct cpuinfo_x86 *c)
>  	/* AMD CPUs don't reset SS attributes on SYSRET, Xen does. */
>  	if (!cpu_has(c, X86_FEATURE_XENPV))
>  		set_cpu_bug(c, X86_BUG_SYSRET_SS_ATTRS);
> +
> +	/*
> +	 * Turn on the Instructions Retired free counter on machines not
> +	 * susceptible to erratum #1054 "Instructions Retired Performance
> +	 * Counter May Be Inaccurate"
				     .
				     ^
				     |--- fullstop.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette