| Message ID | 20230620174556.3898824-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/vpmu: Simplify is_pmc_quirk | expand |
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.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> 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? Jan
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.cooper3@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@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
diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c index 35e350578b84..0291481da22e 100644 --- a/xen/arch/x86/cpu/vpmu_intel.c +++ b/xen/arch/x86/cpu/vpmu_intel.c @@ -91,22 +91,14 @@ static const unsigned int regs_off = * 1 (or another value != 0) into it. * There exist no errata and the real cause of this behaviour is unknown. */ -bool_t __read_mostly is_pmc_quirk; - -static void check_pmc_quirk(void) -{ - if ( current_cpu_data.x86 == 6 ) - is_pmc_quirk = 1; - else - is_pmc_quirk = 0; -} +static bool __ro_after_init pmc_quirk; static void handle_pmc_quirk(u64 msr_content) { int i; u64 val; - if ( !is_pmc_quirk ) + if ( !pmc_quirk ) return; val = msr_content; @@ -791,8 +783,9 @@ static int cf_check core2_vpmu_do_interrupt(struct cpu_user_regs *regs) rdmsrl(MSR_CORE_PERF_GLOBAL_STATUS, msr_content); if ( msr_content ) { - if ( is_pmc_quirk ) + if ( pmc_quirk ) handle_pmc_quirk(msr_content); + core2_vpmu_cxt->global_status |= msr_content; msr_content &= ~global_ovf_ctrl_mask; wrmsrl(MSR_CORE_PERF_GLOBAL_OVF_CTRL, msr_content); @@ -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; if ( sizeof(struct xen_pmu_data) + sizeof(uint64_t) * fixed_pmc_cnt + sizeof(struct xen_pmu_cntr_pair) * arch_pmc_cnt > PAGE_SIZE )
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.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> --- xen/arch/x86/cpu/vpmu_intel.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-)