Re: [PATCH for-4.20] x86/intel: Fix PERF_GLOBAL fixup when virtualised

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 21/01/2025 3:58 pm, Jan Beulich wrote:
> On 21.01.2025 16:23, Andrew Cooper wrote:
>> On 21/01/2025 3:03 pm, Jan Beulich wrote:
>>> On 21.01.2025 15:25, Andrew Cooper wrote:
>>>> +         !(val & MSR_IA32_MISC_ENABLE_PERF_AVAIL) )
>>>> +        return;
>>>> +
>>>> +    eax = cpuid_eax(10);
>>>> +    ver = eax & 0xff;
>>>> +    nr_cnt = (eax >> 8) & 0xff;
>>>> +
>>>> +    if ( ver && nr_cnt > 1 && nr_cnt <= 32 )
>>>> +    {
>>>> +        unsigned int cnt_mask = (1UL << nr_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, val);
>>>> +
>>>> +        if ( (val & cnt_mask) != cnt_mask )
>>>> +        {
>>>> +            printk("FIRMWARE BUG: CPU%u invalid PERF_GLOBAL_CTRL: 
>>>> %#"PRIx64" adjusting to %#"PRIx64"\n",
>>>> +                   smp_processor_id(), val, val | cnt_mask);
>>>> +            wrmsrl(MSR_CORE_PERF_GLOBAL_CTRL, val | cnt_mask);
>>>> +        }
>>>> +    }
>>>> +
>>>> +    __set_bit(X86_FEATURE_ARCH_PERFMON, c->x86_capability);
>>> This moved, without the description suggesting the move is intentional.
>>> It did live at the end of the earlier scope before, ...
>> Final paragraph of the commit message?
>>
>> If PERF_AVAIL is clear, we don't have ARCH_PERFMON, whatever the CPUID
>> leaves say.
> Hmm, the final paragraph in the posting that I got in my inbox was:
>
> "This also limits setting X86_FEATURE_ARCH_PERFMON, although oprofile (the 
> only
>  consumer of this flag) cross-checks too."
>
> Which says quite the opposite: You now set the bit in more cases, i.e.
> when nr_cnt is out of range or when ver is zero.

Oh, right.  Yeah, that was unintentional.  I'll adjust.

~Andrew