linux-kernel - Re: [PATCH v3 14/15] perf/x86: Simplify Intel PMU initialization

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: <3f3f6589-8074-4a9a-936f-513013c43477@intel.com>
Date: Wed, 19 Feb 2025 12:31:55 -0800
From: Sohil Mehta <sohil.mehta@...el.com>
To: "Liang, Kan" <kan.liang@...ux.intel.com>, <x86@...nel.org>, Dave Hansen
	<dave.hansen@...ux.intel.com>, Tony Luck <tony.luck@...el.com>
CC: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
	Arnaldo Carvalho de Melo <acme@...nel.org>, Namhyung Kim
	<namhyung@...nel.org>, Mark Rutland <mark.rutland@....com>, "Alexander
 Shishkin" <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>,
	Ian Rogers <irogers@...gle.com>, Adrian Hunter <adrian.hunter@...el.com>,
	Thomas Gleixner <tglx@...utronix.de>, Borislav Petkov <bp@...en8.de>, "H .
 Peter Anvin" <hpa@...or.com>, "Rafael J . Wysocki" <rafael@...nel.org>, "Len
 Brown" <lenb@...nel.org>, Andy Lutomirski <luto@...nel.org>, Viresh Kumar
	<viresh.kumar@...aro.org>, Jean Delvare <jdelvare@...e.com>, Guenter Roeck
	<linux@...ck-us.net>, Zhang Rui <rui.zhang@...el.com>, Andrew Cooper
	<andrew.cooper3@...rix.com>, David Laight <david.laight.linux@...il.com>,
	Dapeng Mi <dapeng1.mi@...ux.intel.com>, <linux-perf-users@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <linux-acpi@...r.kernel.org>,
	<linux-pm@...r.kernel.org>
Subject: Re: [PATCH v3 14/15] perf/x86: Simplify Intel PMU initialization

On 2/19/2025 12:10 PM, Liang, Kan wrote:
> 
> 
> On 2025-02-19 1:41 p.m., Sohil Mehta wrote:
>> Architectural Perfmon was introduced on the Family 6 "Core" processors
>> starting with Yonah. Processors before Yonah need their own customized
>> PMU initialization.
>>
>> p6_pmu_init() is expected to provide that initialization for early
>> Family 6 processors. But, due to the unrestricted call to p6_pmu_init(),
>> it could get called for any Family 6 processor if the architectural
>> perfmon feature is disabled on that processor.
>>
>> To simplify, restrict the call to p6_pmu_init() to early Family 6
>> processors that do not have architectural perfmon support. As a result,
>> the "unsupported" console print becomes practically unreachable because
>> all the released P6 processors are covered by the switch cases.
>>
>> Move the console print to a common location where it can cover all
>> modern processors that do not have architectural perfmon support.
>>
>> Also, use this opportunity to get rid of the unnecessary switch cases in
>> p6_pmu_init().  Only the Pentium Pro processor needs a quirk, and the
>> rest of the processors do not need any special handling. The gaps in the
>> case numbers are only due to no processor with those model numbers being
>> released.
>>
>> Converting to a VFM based check gets rid of one last few Intel x86_model
>> comparisons.
>>
>> Signed-off-by: Sohil Mehta <sohil.mehta@...el.com>
>> ---
>> v3: Restrict calling p6_pmu_init() to only when needed.
>>     Move the console print to a common location.
>>
>> v2: No change.
>> ---
>>  arch/x86/events/intel/core.c | 16 +++++++++++-----
>>  arch/x86/events/intel/p6.c   | 26 +++-----------------------
>>  2 files changed, 14 insertions(+), 28 deletions(-)
>>
>> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
>> index 7601196d1d18..c645d8c8ab87 100644
>> --- a/arch/x86/events/intel/core.c
>> +++ b/arch/x86/events/intel/core.c
>> @@ -6466,16 +6466,22 @@ __init int intel_pmu_init(void)
>>  	char *name;
>>  	struct x86_hybrid_pmu *pmu;
>>  
>> +	/* Architectural Perfmon was introduced starting with INTEL_CORE_YONAH */
>>  	if (!cpu_has(&boot_cpu_data, X86_FEATURE_ARCH_PERFMON)) {
>>  		switch (boot_cpu_data.x86) {
>> -		case 0x6:
>> -			return p6_pmu_init();
>> -		case 0xb:
>> +		case 6:
>> +			if (boot_cpu_data.x86_vfm < INTEL_CORE_YONAH)
>> +				return p6_pmu_init();
>> +			break;
> 
> We may need a return -ENODEV here.
> 

That makes sense. See below.

> I think it's possible that some weird hypervisor doesn't enumerate the
> ARCH_PERFMON for a modern CPU. Perf should not touch the leaf 10 if the
> ARCH_PERFMON is not supported.
> 
> Thanks,
> Kan
> 
>> +		case 11:
>>  			return knc_pmu_init();
>> -		case 0xf:
>> +		case 15:
>>  			return p4_pmu_init();
>> +		default:
>> +			pr_cont("unsupported CPU family %d model %d ",
>> +				boot_cpu_data.x86, boot_cpu_data.x86_model);
>> +			return -ENODEV;
>>  		}
>> -		return -ENODEV;
>>  	}
>>  


How about moving the default case out of the switch statement as shown?
That would make sure that the unsupported print would also get included
with the -ENODEV.

	/* Architectural Perfmon was introduced starting with INTEL_CORE_YONAH */
	if (!cpu_has(&boot_cpu_data, X86_FEATURE_ARCH_PERFMON)) {
		switch (boot_cpu_data.x86) {
		case 6:
			if (boot_cpu_data.x86_vfm < INTEL_CORE_YONAH)
				return p6_pmu_init();
			break;
		case 11:
			return knc_pmu_init();
		case 15:
			return p4_pmu_init();
		}

		pr_cont("unsupported CPU family %d model %d ",
			boot_cpu_data.x86, boot_cpu_data.x86_model);
		return -ENODEV;
	}