Messages in this thread Patch in this message | | | Date | Mon, 22 Aug 2022 09:24:57 -0400 | Subject | Re: [RESEND PATCH] perf/x86/intel: Fix unchecked MSR access error for Alder Lake N | From | "Liang, Kan" <> |
| |
On 2022-08-19 4:33 a.m., Peter Zijlstra wrote: > On Fri, Aug 19, 2022 at 10:05:40AM +0200, Peter Zijlstra wrote: >> On Thu, Aug 18, 2022 at 11:15:30AM -0700, kan.liang@linux.intel.com wrote: >> >>> The Alder Lake N only has e-cores. The X86_FEATURE_HYBRID_CPU flag is >>> not set. The perf cannot retrieve the correct CPU type via >>> get_this_hybrid_cpu_type(). The model specific get_hybrid_cpu_type() is >>> hardcode to p-core. The wrong CPU type is given to the PMU of the >>> Alder Lake N. >> >> If ADL-N isn't in fact a hybrid CPU, then *WHY* are we running >> init_hybrid_pmu() and setting up all that nonsense? >> >> That is, wouldn't the right thing be to remove ALDERLAKE_N from the rest >> of {ALDER,RAPTOP}LAKE and create a non-hybrid PMU setup for it? >
I think the only issue should be the PMU name. The non-hybrid PMU name is "cpu". The hybrid PMU name is "cpu_$coretype". If we move the ALDERLAKE_N to the non-hybrid PMU, the PMU name will be changed from "cpu_atom" to "cpu". It will be different from the rest of {ALDER,RAPTOP}LAKE.
Also, I think we have to update the perf tool for the events because of the PMU name change.
But I guess it should be OK, since the ALDERLAKE_N was just added and we know its an Atom-only system.
> Something like the *completely* untested below.. which adds it like a > regular atom chip (which it is).
I will do more tests and send out a V2.
Thanks, Kan > > (I basically did copy/paste of tremont and added bits from the cpu_atom > thing from alderlake -- but might well have missed something) > > diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c > index 2db93498ff71..e509f1033a2d 100644 > --- a/arch/x86/events/intel/core.c > +++ b/arch/x86/events/intel/core.c > @@ -5974,6 +5974,38 @@ __init int intel_pmu_init(void) > name = "Tremont"; > break; > > + case INTEL_FAM6_ALDERLAKE_N: > + x86_pmu.mid_ack = true; > + memcpy(hw_cache_event_ids, glp_hw_cache_event_ids, > + sizeof(hw_cache_event_ids)); > + memcpy(hw_cache_extra_regs, tnt_hw_cache_extra_regs, > + sizeof(hw_cache_extra_regs)); > + hw_cache_event_ids[C(ITLB)][C(OP_READ)][C(RESULT_ACCESS)] = -1; > + > + intel_pmu_lbr_init_skl(); > + > + x86_pmu.event_constraints = intel_slm_event_constraints; > + x86_pmu.pebs_constraints = intel_grt_pebs_event_constraints; > + x86_pmu.extra_regs = intel_grt_extra_regs; > + /* > + * It's recommended to use CPU_CLK_UNHALTED.CORE_P + NPEBS > + * for precise cycles. > + */ > + x86_pmu.pebs_aliases = NULL; > + x86_pmu.pebs_prec_dist = true; > + x86_pmu.lbr_pt_coexist = true; > + x86_pmu.flags |= PMU_FL_HAS_RSP_1; > + x86_pmu.flags |= PMU_FL_NO_HT_SHARING; > + x86_pmu.flags |= PMU_FL_PEBS_ALL; > + x86_pmu.flags |= PMU_FL_INSTR_LATENCY; > + x86_pmu.flags |= PMU_FL_MEM_LOADS_AUX; > + x86_pmu.get_event_constraints = tnt_get_event_constraints; > + td_attr = tnt_events_attrs; > + extra_attr = slm_format_attr; > + pr_cont("Gracemont events, "); > + name = "Gracemont"; > + break; > + > case INTEL_FAM6_WESTMERE: > case INTEL_FAM6_WESTMERE_EP: > case INTEL_FAM6_WESTMERE_EX: > @@ -6318,7 +6350,6 @@ __init int intel_pmu_init(void) > > case INTEL_FAM6_ALDERLAKE: > case INTEL_FAM6_ALDERLAKE_L: > - case INTEL_FAM6_ALDERLAKE_N: > case INTEL_FAM6_RAPTORLAKE: > case INTEL_FAM6_RAPTORLAKE_P: > /*
| |