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 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.

> >             /* 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.

Thanks, Roger.