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

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

On Wed, Jan 10, 2024 at 03:52:49PM +0000, Andrew Cooper wrote:
> On 10/01/2024 3:34 pm, 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 socket 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.
> >
> > Fix by detecting when Architectural Performance Monitoring is available and
> > making sure the enable bit for PMC0 is set in PERF_GLOBAL_CTRL.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> > The fact that it's only the first CPU on each socket that's started with
> > PERF_GLOBAL_CTRL clear looks like a firmware bug to me, but in any case 
> > making
> > sure PERF_GLOBAL_CTRL is properly setup should be done regardless.
> 
> It's each package-BSP, and yes, this is clearly a firmware bug.  It's
> probably worth saying that we're raising it with Intel, but this bug is
> out in production firmware for SPR and EMR.
> 
> > ---
> >  xen/arch/x86/nmi.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/xen/arch/x86/nmi.c b/xen/arch/x86/nmi.c
> > index dc79c25e3ffd..7a6601c4fd31 100644
> > --- a/xen/arch/x86/nmi.c
> > +++ b/xen/arch/x86/nmi.c
> > @@ -335,6 +335,19 @@ static void setup_p6_watchdog(unsigned counter)
> >           nmi_p6_event_width > BITS_PER_LONG )
> >          return;
> >  
> > +    if ( cpu_has_arch_perfmon )
> > +    {
> > +        uint64_t global_ctrl;
> > +
> > +        rdmsrl(MSR_CORE_PERF_GLOBAL_CTRL, global_ctrl);
> > +        /*
> > +         * Make sure PMC0 is enabled in global control, as the enable bit 
> > in
> > +         * PERF_GLOBAL_CTRL is AND'ed with the enable bit in EVNTSEL0.
> > +         */
> > +        if ( !(global_ctrl & 1) )
> > +            wrmsrl(MSR_CORE_PERF_GLOBAL_CTRL, global_ctrl | 1);
> 
> My gut feeling is that we ought to reinstate all bits, not just bit 1. 
> If nothing else because that will make debugging using other counters
> more reliable too.

Hm, yes, I was borderline on enabling all possible counters in
PERF_GLOBAL_CTRL, as reported by CPUID.0AH: EAX[15:8].

But then wondered if it was going too far, as for the purposes here we
just care about PMC1.

My reasoning for not doing it would be that such wide setup of
PERF_GLOBAL_CTRL would then be gated on the watchdog being enabled,
usages of other counters apart from PMC0 will be gated on the watchdog
being enabled.  It seems more reliable to me to either do the setting
of PERF_GLOBAL_CTRL as part of CPU initialization, or defer to each
user of a PMC to take care of enabling it in PERF_GLOBAL_CTRL.

> vPMU (although mutually exclusive with watchdog) does context switch
> this register as a whole.
> 
> See how global_ctrl_mask gets set up, although I'm not sure how much of
> that infrastructure we really want to reuse here.

Yes, if we want to enable all possible counters we would need to use
something similar to what's done there, albeit without the fixed
counter part.

Thanks, Roger.