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
