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

[Sohil Mehta]

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@xxxxxxxxx>
>> ---
>> 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;
	}