| Message ID | b33cc365-6537-d816-8a89-eadd514a2427@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/vPMU: don't blindly assume IA32_PERF_CAPABILITIES MSR exists | expand |
On 26/02/2020 09:19, Jan Beulich wrote: > Just like VMX'es lbr_tsx_fixup_check() the respective CPUID bit should > be consulted first. > > Reported-by: Farrah Chen <farrah.chen@intel.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
On Wed, Feb 26, 2020 at 10:19:19AM +0100, Jan Beulich wrote: > Just like VMX'es lbr_tsx_fixup_check() the respective CPUID bit should > be consulted first. > > Reported-by: Farrah Chen <farrah.chen@intel.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/x86/cpu/vpmu_intel.c > +++ b/xen/arch/x86/cpu/vpmu_intel.c > @@ -900,7 +900,6 @@ int vmx_vpmu_initialise(struct vcpu *v) > > int __init core2_vpmu_init(void) > { > - u64 caps; > unsigned int version = 0; > unsigned int i; > > @@ -932,8 +931,14 @@ int __init core2_vpmu_init(void) > > arch_pmc_cnt = core2_get_arch_pmc_count(); > fixed_pmc_cnt = core2_get_fixed_pmc_count(); > - rdmsrl(MSR_IA32_PERF_CAPABILITIES, caps); > - full_width_write = (caps >> 13) & 1; > + > + if ( cpu_has_pdcm ) > + { > + uint64_t caps; > + > + rdmsrl(MSR_IA32_PERF_CAPABILITIES, caps); > + full_width_write = (caps >> 13) & 1; Will PMU work without PDCM? I've been grepping the Intel SDMs, but the only mention is that PDCM signal the availability of MSR_IA32_PERF_CAPABILITIES. Thanks, Roger.
On 26.02.2020 11:09, Roger Pau Monné wrote: > On Wed, Feb 26, 2020 at 10:19:19AM +0100, Jan Beulich wrote: >> Just like VMX'es lbr_tsx_fixup_check() the respective CPUID bit should >> be consulted first. >> >> Reported-by: Farrah Chen <farrah.chen@intel.com> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> --- a/xen/arch/x86/cpu/vpmu_intel.c >> +++ b/xen/arch/x86/cpu/vpmu_intel.c >> @@ -900,7 +900,6 @@ int vmx_vpmu_initialise(struct vcpu *v) >> >> int __init core2_vpmu_init(void) >> { >> - u64 caps; >> unsigned int version = 0; >> unsigned int i; >> >> @@ -932,8 +931,14 @@ int __init core2_vpmu_init(void) >> >> arch_pmc_cnt = core2_get_arch_pmc_count(); >> fixed_pmc_cnt = core2_get_fixed_pmc_count(); >> - rdmsrl(MSR_IA32_PERF_CAPABILITIES, caps); >> - full_width_write = (caps >> 13) & 1; >> + >> + if ( cpu_has_pdcm ) >> + { >> + uint64_t caps; >> + >> + rdmsrl(MSR_IA32_PERF_CAPABILITIES, caps); >> + full_width_write = (caps >> 13) & 1; > > Will PMU work without PDCM? > > I've been grepping the Intel SDMs, but the only mention is that PDCM > signal the availability of MSR_IA32_PERF_CAPABILITIES. Well, there's no other use of the MSR afaics except for getting the one bit here, so I assume it'll work. Jan
On 26/02/2020 10:39, Jan Beulich wrote: > On 26.02.2020 11:09, Roger Pau Monné wrote: >> On Wed, Feb 26, 2020 at 10:19:19AM +0100, Jan Beulich wrote: >>> Just like VMX'es lbr_tsx_fixup_check() the respective CPUID bit should >>> be consulted first. >>> >>> Reported-by: Farrah Chen <farrah.chen@intel.com> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> >>> --- a/xen/arch/x86/cpu/vpmu_intel.c >>> +++ b/xen/arch/x86/cpu/vpmu_intel.c >>> @@ -900,7 +900,6 @@ int vmx_vpmu_initialise(struct vcpu *v) >>> >>> int __init core2_vpmu_init(void) >>> { >>> - u64 caps; >>> unsigned int version = 0; >>> unsigned int i; >>> >>> @@ -932,8 +931,14 @@ int __init core2_vpmu_init(void) >>> >>> arch_pmc_cnt = core2_get_arch_pmc_count(); >>> fixed_pmc_cnt = core2_get_fixed_pmc_count(); >>> - rdmsrl(MSR_IA32_PERF_CAPABILITIES, caps); >>> - full_width_write = (caps >> 13) & 1; >>> + >>> + if ( cpu_has_pdcm ) >>> + { >>> + uint64_t caps; >>> + >>> + rdmsrl(MSR_IA32_PERF_CAPABILITIES, caps); >>> + full_width_write = (caps >> 13) & 1; >> Will PMU work without PDCM? The performance counter interface in CPUs predate the introduction of PERF_CAPS. >> I've been grepping the Intel SDMs, but the only mention is that PDCM >> signal the availability of MSR_IA32_PERF_CAPABILITIES. > Well, there's no other use of the MSR afaics except for getting > the one bit here, so I assume it'll work. It is an off-by-default, outside security support area of functionality with known functional bugs outstanding against it. "not crash" is a fine improvement on the status quo. ~Andrew
I applied this patch to Xen and retested, Xen on KVM booted up successfully, thanks a lot. Thanks, Fan -----Original Message----- From: Andrew Cooper <andrew.cooper3@citrix.com> Sent: Wednesday, February 26, 2020 6:56 PM To: Jan Beulich <jbeulich@suse.com>; Roger Pau Monné <roger.pau@citrix.com> Cc: xen-devel@lists.xenproject.org; Chen, Farrah <farrah.chen@intel.com>; Hao, Xudong <xudong.hao@intel.com>; Gao, Chao <chao.gao@intel.com>; Wei Liu <wl@xen.org> Subject: Re: [PATCH] x86/vPMU: don't blindly assume IA32_PERF_CAPABILITIES MSR exists On 26/02/2020 10:39, Jan Beulich wrote: > On 26.02.2020 11:09, Roger Pau Monné wrote: >> On Wed, Feb 26, 2020 at 10:19:19AM +0100, Jan Beulich wrote: >>> Just like VMX'es lbr_tsx_fixup_check() the respective CPUID bit >>> should be consulted first. >>> >>> Reported-by: Farrah Chen <farrah.chen@intel.com> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> >>> --- a/xen/arch/x86/cpu/vpmu_intel.c >>> +++ b/xen/arch/x86/cpu/vpmu_intel.c >>> @@ -900,7 +900,6 @@ int vmx_vpmu_initialise(struct vcpu *v) >>> >>> int __init core2_vpmu_init(void) >>> { >>> - u64 caps; >>> unsigned int version = 0; >>> unsigned int i; >>> >>> @@ -932,8 +931,14 @@ int __init core2_vpmu_init(void) >>> >>> arch_pmc_cnt = core2_get_arch_pmc_count(); >>> fixed_pmc_cnt = core2_get_fixed_pmc_count(); >>> - rdmsrl(MSR_IA32_PERF_CAPABILITIES, caps); >>> - full_width_write = (caps >> 13) & 1; >>> + >>> + if ( cpu_has_pdcm ) >>> + { >>> + uint64_t caps; >>> + >>> + rdmsrl(MSR_IA32_PERF_CAPABILITIES, caps); >>> + full_width_write = (caps >> 13) & 1; >> Will PMU work without PDCM? The performance counter interface in CPUs predate the introduction of PERF_CAPS. >> I've been grepping the Intel SDMs, but the only mention is that PDCM >> signal the availability of MSR_IA32_PERF_CAPABILITIES. > Well, there's no other use of the MSR afaics except for getting the > one bit here, so I assume it'll work. It is an off-by-default, outside security support area of functionality with known functional bugs outstanding against it. "not crash" is a fine improvement on the status quo. ~Andrew
--- a/xen/arch/x86/cpu/vpmu_intel.c +++ b/xen/arch/x86/cpu/vpmu_intel.c @@ -900,7 +900,6 @@ int vmx_vpmu_initialise(struct vcpu *v) int __init core2_vpmu_init(void) { - u64 caps; unsigned int version = 0; unsigned int i; @@ -932,8 +931,14 @@ int __init core2_vpmu_init(void) arch_pmc_cnt = core2_get_arch_pmc_count(); fixed_pmc_cnt = core2_get_fixed_pmc_count(); - rdmsrl(MSR_IA32_PERF_CAPABILITIES, caps); - full_width_write = (caps >> 13) & 1; + + if ( cpu_has_pdcm ) + { + uint64_t caps; + + rdmsrl(MSR_IA32_PERF_CAPABILITIES, caps); + full_width_write = (caps >> 13) & 1; + } fixed_ctrl_mask = ~((1ull << (fixed_pmc_cnt * FIXED_CTR_CTRL_BITS)) - 1); /* mask .AnyThread bits for all fixed counters */
Just like VMX'es lbr_tsx_fixup_check() the respective CPUID bit should be consulted first. Reported-by: Farrah Chen <farrah.chen@intel.com> Signed-off-by: Jan Beulich <jbeulich@suse.com>