On 21/06/2023 9:16 am, Jan Beulich wrote: > On 20.06.2023 19:45, Andrew Cooper wrote: >> This should be static, and there's no need for a separate (non-init, even) >> function to perform a simple equality test. Drop the is_ prefix which is >> gramatically questionable, and make it __ro_after_init. >> >> Leave a TODO, because the behaviour is definitely wrong to be applied to ~all >> modern Intel CPUs, and has been raised on xen-devel previously without >> conclusion. >> >> No functional change. >> >> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com> > Reviewed-by: Jan Beulich <jbeul...@suse.com>
Thanks. > with one request: > >> @@ -967,7 +960,8 @@ const struct arch_vpmu_ops *__init core2_vpmu_init(void) >> sizeof(uint64_t) * fixed_pmc_cnt + >> sizeof(struct xen_pmu_cntr_pair) * arch_pmc_cnt; >> >> - check_pmc_quirk(); >> + /* TODO: This is surely wrong. */ >> + pmc_quirk = current_cpu_data.x86 == 6; > In the description you say "~all modern Intel CPUs", which suggests it might > be correct for old enough ones. Would you mind weakening the comment to > "This surely isn't universally correct" or some such? I'm happy to tweak the wording as appropriate, but Kyle (who is a regular contributor to perf in Linux) questioned that there was an issue. There's insufficient information to figure out why it was introduced in the first place, and IIRC he wasn't aware of any errata that manifested in this way. It's possible it's entirely bogus, or it may be a misunderstood errata. It needs looking into by someone with some copious free time. ~Andrew
