x86/vPMU: don't blindly assume IA32_PERF_CAPABILITIES MSR exists
diff mbox series

Message ID b33cc365-6537-d816-8a89-eadd514a2427@suse.com
State New
Headers show
Series
  • x86/vPMU: don't blindly assume IA32_PERF_CAPABILITIES MSR exists
Related show

Commit Message

Jan Beulich Feb. 26, 2020, 9:19 a.m. UTC
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>

Comments

Andrew Cooper Feb. 26, 2020, 10:09 a.m. UTC | #1
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>
Roger Pau Monné Feb. 26, 2020, 10:09 a.m. UTC | #2
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.
Jan Beulich Feb. 26, 2020, 10:39 a.m. UTC | #3
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
Andrew Cooper Feb. 26, 2020, 10:56 a.m. UTC | #4
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
Chen, Farrah Feb. 26, 2020, 12:47 p.m. UTC | #5
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

Patch
diff mbox series

--- 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 */