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

[Liang, Kan]

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.

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;
>  	}
>  
>  	/*
> diff --git a/arch/x86/events/intel/p6.c b/arch/x86/events/intel/p6.c
> index a6cffb4f4ef5..65b45e9d7016 100644
> --- a/arch/x86/events/intel/p6.c
> +++ b/arch/x86/events/intel/p6.c
> @@ -2,6 +2,8 @@
>  #include <linux/perf_event.h>
>  #include <linux/types.h>
>  
> +#include <asm/cpu_device_id.h>
> +
>  #include "../perf_event.h"
>  
>  /*
> @@ -248,30 +250,8 @@ __init int p6_pmu_init(void)
>  {
>  	x86_pmu = p6_pmu;
>  
> -	switch (boot_cpu_data.x86_model) {
> -	case  1: /* Pentium Pro */
> +	if (boot_cpu_data.x86_vfm == INTEL_PENTIUM_PRO)
>  		x86_add_quirk(p6_pmu_rdpmc_quirk);
> -		break;
> -
> -	case  3: /* Pentium II - Klamath */
> -	case  5: /* Pentium II - Deschutes */
> -	case  6: /* Pentium II - Mendocino */
> -		break;
> -
> -	case  7: /* Pentium III - Katmai */
> -	case  8: /* Pentium III - Coppermine */
> -	case 10: /* Pentium III Xeon */
> -	case 11: /* Pentium III - Tualatin */
> -		break;
> -
> -	case  9: /* Pentium M - Banias */
> -	case 13: /* Pentium M - Dothan */
> -		break;
> -
> -	default:
> -		pr_cont("unsupported p6 CPU model %d ", boot_cpu_data.x86_model);
> -		return -ENODEV;
> -	}
>  
>  	memcpy(hw_cache_event_ids, p6_hw_cache_event_ids,
>  		sizeof(hw_cache_event_ids));