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

[Liang, Kan]

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

It looks good to me.

With the above change,

Reviewed-by: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>

Thanks,
Kan