[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2] x86/intel: ensure Global Performance Counter Control is setup correctly
On 11.01.2024 11:32, Roger Pau Monné wrote: > On Thu, Jan 11, 2024 at 11:00:28AM +0100, Jan Beulich wrote: >> On 11.01.2024 10:08, Roger Pau Monne wrote: >>> When Architectural Performance Monitoring is available, the PERF_GLOBAL_CTRL >>> MSR contains per-counter enable bits that is ANDed with the enable bit in >>> the >>> counter EVNTSEL MSR in order for a PMC counter to be enabled. >>> >>> So far the watchdog code seems to have relied on the PERF_GLOBAL_CTRL enable >>> bits being set by default, but at least on some Intel Sapphire and Emerald >>> Rapids this is no longer the case, and Xen reports: >>> >>> Testing NMI watchdog on all CPUs: 0 40 stuck >>> >>> The first CPU on each package is started with PERF_GLOBAL_CTRL zeroed, so >>> PMC0 >>> doesn't start counting when the enable bit in EVNTSEL0 is set, due to the >>> relevant enable bit in PERF_GLOBAL_CTRL not being set. >>> >>> Check and adjust PERF_GLOBAL_CTRL during CPU initialization so that all the >>> general-purpose PMCs are enabled. Doing so brings the state of the >>> package-BSP >>> PERF_GLOBAL_CTRL in line with the rest of the CPUs on the system. >>> >>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx> >>> --- >>> Changes since v1: >>> - Do the adjustment of PERF_GLOBAL_CTRL even if the watchdog is not used, >>> and >>> enable all counters. >>> --- >>> Unsure whether printing a warning if the value of PERF_GLOBAL_CTRL is not >>> correct is of any value, hence I haven't added it. >>> --- >>> xen/arch/x86/cpu/intel.c | 18 +++++++++++++++++- >>> 1 file changed, 17 insertions(+), 1 deletion(-) >>> >>> diff --git a/xen/arch/x86/cpu/intel.c b/xen/arch/x86/cpu/intel.c >>> index dfee64689ffe..40d3eb0e18a7 100644 >>> --- a/xen/arch/x86/cpu/intel.c >>> +++ b/xen/arch/x86/cpu/intel.c >>> @@ -533,9 +533,25 @@ static void cf_check init_intel(struct cpuinfo_x86 *c) >>> init_intel_cacheinfo(c); >>> if (c->cpuid_level > 9) { >>> unsigned eax = cpuid_eax(10); >>> + unsigned int cnt = (uint8_t)(eax >> 8); >> >> (Not just) since ./CODING_STYLE wants us to avoid fixed width types where >> possible, personally I'd prefer "& 0xff" here. > > Hm, OK. I got confused and somehow was under the impression we prefer > to truncate using fixed types rather than masks. It's on the edge I admit, and iirc e.g. Andrew would rather see ./CODING_STYLE be relaxed some again. >>> /* Check for version and the number of counters */ >>> - if ((eax & 0xff) && (((eax>>8) & 0xff) > 1)) >>> + if ((eax & 0xff) && (cnt > 1) && (cnt <= 32)) { >>> + uint64_t global_ctrl; >>> + unsigned int cnt_mask = (1UL << cnt) - 1; >>> + >>> + /* >>> + * On (some?) Sapphire/Emerald Rapids platforms each >>> + * package-BSP starts with all the enable bits for the >>> + * general-purpose PMCs cleared. Adjust so counters >>> + * can be enabled from EVNTSEL. >>> + */ >>> + rdmsrl(MSR_CORE_PERF_GLOBAL_CTRL, global_ctrl); >>> + if ((global_ctrl & cnt_mask) != cnt_mask) >>> + wrmsrl(MSR_CORE_PERF_GLOBAL_CTRL, >>> + global_ctrl | cnt_mask); >> >> Should there perhaps be a log message? > > I had a post-commit remark about that exactly. I can add one. Oh, sorry, missed that. Imo for firmware bugs it's useful to record. Further, if such a messages is emitted first, in the (hopefully) unlikely event of the WRMSR causing an issue, there'll be an immediate reference of what's going on. Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |