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