On 21/01/2025 3:03 pm, Jan Beulich wrote:
> On 21.01.2025 15:25, Andrew Cooper wrote:
>> Logic using performance counters needs to look at
>> MSR_MISC_ENABLE.PERF_AVAILABLE before touching any other resources.
>>
>> When virtualised under ESX, Xen dies with a #GP fault trying to read
>> MSR_CORE_PERF_GLOBAL_CTRL.
>>
>> Factor this logic out into a separate function (it's already too squashed to
>> the RHS), and insert a check of MSR_MISC_ENABLE.PERF_AVAILABLE.
>>
>> This also limits setting X86_FEATURE_ARCH_PERFMON, although oprofile (the
>> only
>> consumer of this flag) cross-checks too.
Fixes: 6bdb965178bb ("x86/intel: ensure Global Performance Counter
Control is setup correctly")
(fixed up locally).
>> Reported-by: Jonathan Katz <jonathan.k...@aptar.com>
>> Link: https://xcp-ng.org/forum/topic/10286/nesting-xcp-ng-on-esx-8
>> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
>> ---
>> CC: Jan Beulich <jbeul...@suse.com>
>> CC: Roger Pau Monné <roger....@citrix.com>
>> CC: Oleksii Kurochko <oleksii.kuroc...@gmail.com>
>>
>> Untested, but this is the same pattern used by oprofile and watchdog setup.
> Wow, in the oprofile case with pretty bad open-coding.
>
>> I've intentionally stopped using Intel style. This file is already mixed (as
>> visible even in context), and it doesn't remotely resemble it's Linux origin
>> any more.
> I guess you mean s/Intel/Linux/ here? (Yes, I'm entirely fine with using Xen
> style there.)
Oops yes.
>
>> --- a/xen/arch/x86/cpu/intel.c
>> +++ b/xen/arch/x86/cpu/intel.c
>> @@ -535,39 +535,49 @@ static void intel_log_freq(const struct cpuinfo_x86 *c)
>> printk("%u MHz\n", (factor * max_ratio + 50) / 100);
>> }
>>
>> +static void init_intel_perf(struct cpuinfo_x86 *c)
>> +{
>> + uint64_t val;
>> + unsigned int eax, ver, nr_cnt;
>> +
>> + if ( c->cpuid_level <= 9 ||
>> + rdmsr_safe(MSR_IA32_MISC_ENABLE, val) ||
> In e.g. intel_unlock_cpuid_leaves() and early_init_intel() and in particular
> also in boot/head.S we access this MSR without recovery attached. Is there a
> reason rdmsr_safe() needs using here?
Abundance of caution. cpufreq/hwp.c uses a safe accessor.
Given the regular NMI path works, I doubt we need the _safe() here.
As future work, it's accessed loads of times, so I'm highly tempted to
have the BSP sanitise it once, and have the APs copy the "global" value.
>
>> + !(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.
OTOH, this bit really doesn't serve much value. Given oprofile
cross-checks everything anyway, I think it can be dropped.
~Andrew
