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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <490badc1-443b-4f90-a06e-da19ba77583a@linux.intel.com>
Date: Wed, 19 Feb 2025 15:45:08 -0500
From: "Liang, Kan" <kan.liang@...ux.intel.com>
To: Sohil Mehta <sohil.mehta@...el.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 2025-02-19 3:31 p.m., Sohil Mehta wrote:
> 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;
> 	}

It looks good to me.

With the above change,

Reviewed-by: Kan Liang <kan.liang@...ux.intel.com>

Thanks,
Kan