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

[Jan Beulich <jbeulich@xxxxxxxx>]

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