Re: [PATCH v2] x86/intel: ensure Global Performance Counter Control is setup correctly

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Jan 11, 2024 at 12:34:45PM +0100, Jan Beulich wrote:
> On 11.01.2024 11:40, Roger Pau Monné wrote:
> > On Thu, Jan 11, 2024 at 11:11:07AM +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);
> >>> +
> >>>           /* Check for version and the number of counters */
> >>> -         if ((eax & 0xff) && (((eax>>8) & 0xff) > 1))
> >>> +         if ((eax & 0xff) && (cnt > 1) && (cnt <= 32)) {
> >>
> >> I may not have looked closely enough, but I didn't find the limit of
> >> 32 being stated anywhere.
> > 
> > Hm, my copy of the SDM vol4 states that bits 31:n are the enable bits,
> > where n is CPUID.0AH: EAX[15:8].  Bits 32, 33 and 34 control the Fixed
> > PMCs.
> > 
> >>> +                 uint64_t global_ctrl;
> >>> +                 unsigned int cnt_mask = (1UL << cnt) - 1;
> >>
> >> Bits 2 + 4 * n have an additional qualification as per SDM vol 4.
> > 
> > Let me update my copy...
> > 
> > Looking at the version from December 2023, I see:
> > 
> > 0 EN_PMC0 If CPUID.0AH: EAX[15:8] > 0
> > 1 EN_PMC1 If CPUID.0AH: EAX[15:8] > 1
> > 2 EN_PMC2 If CPUID.0AH: EAX[15:8] > 2
> > n EN_PMCn If CPUID.0AH: EAX[15:8] > n
> > 31:n+1 Reserved.
> > 
> > And I'm afraid I'm not able to infer this additional qualification of
> > bits 2 + 4 * n.
> 
> I'm sorry, both earlier questions were my fault, due to looking at the
> table entry for the first match of PERF_GLOBAL_CTRL (i.e.
> IA32_FIXED_CTR_CTRL). Still need to get used to the new table layout,
> it seems.
> 
> Looking at the correct entry raises a question on cnt > 1 though, as
> EN_PMC0 is defined for cnt > 0.

Oh, indeed, can adjust on this same patch if that's OK (seeing as the
issue was already there previous to my change).

Thanks, Roger.