| Message ID | 20250219184133.816753-15-sohil.mehta@intel.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | Prepare for new Intel Family numbers | expand |
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@intel.com> > --- > 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));
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@intel.com> >> --- >> 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; }
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@intel.com> >>> --- >>> 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@linux.intel.com> Thanks, Kan
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; + 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));
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@intel.com> --- 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(-)