[PATCH] x86/vpmu_intel: Handle SMT consistently for programmable and fixed counters

[PATCH] x86/vpmu_intel: Handle SMT consistently for programmable and fixed counters

[Mohit Gambhir]

This patch masks .AnyThread bits in IA32_FIXED_CTR_CTRL MSR for all
versions of Intel Arhcitectural Performance Monitoring. Note that
.AnyThread bit (21) is already masked in IA32_PERFEVTSELx MSRs since
hyperthreading is not exposed to guests and Intel SDM discourages the use of
.AnyThread bit in virtualized environments (per section 18.2.3.1
AnyThread Counting and Software Evolution)

Signed-off-by: Mohit Gambhir <mohit.gambhir@oracle.com>
---
 xen/arch/x86/cpu/vpmu_intel.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c
index 0d66ecb..7944e22 100644
--- a/xen/arch/x86/cpu/vpmu_intel.c
+++ b/xen/arch/x86/cpu/vpmu_intel.c
@@ -979,8 +979,7 @@ int __init core2_vpmu_init(void)
     full_width_write = (caps >> 13) & 1;
 
     fixed_ctrl_mask = ~((1ull << (fixed_pmc_cnt * FIXED_CTR_CTRL_BITS)) - 1);
-    if ( version == 2 )
-        fixed_ctrl_mask |= 0x444;
+    fixed_ctrl_mask |= 0x444;
     fixed_counters_mask = ~((1ull << core2_get_bitwidth_fix_count()) - 1);
     global_ctrl_mask = ~((((1ULL << fixed_pmc_cnt) - 1) << 32) |
                          ((1ULL << arch_pmc_cnt) - 1));
-- 
2.9.3


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] x86/vpmu_intel: Handle SMT consistently for programmable and fixed counters

[Boris Ostrovsky]

On 03/31/2017 10:46 AM, Mohit Gambhir wrote:
> This patch masks .AnyThread bits in IA32_FIXED_CTR_CTRL MSR for all
> versions of Intel Arhcitectural Performance Monitoring. Note that
> .AnyThread bit (21) is already masked in IA32_PERFEVTSELx MSRs since
> hyperthreading is not exposed to guests and Intel SDM discourages the use of
> .AnyThread bit in virtualized environments (per section 18.2.3.1
> AnyThread Counting and Software Evolution)
>
> Signed-off-by: Mohit Gambhir <mohit.gambhir@oracle.com>

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

(+Andrew and Jan)

> ---
>  xen/arch/x86/cpu/vpmu_intel.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c
> index 0d66ecb..7944e22 100644
> --- a/xen/arch/x86/cpu/vpmu_intel.c
> +++ b/xen/arch/x86/cpu/vpmu_intel.c
> @@ -979,8 +979,7 @@ int __init core2_vpmu_init(void)
>      full_width_write = (caps >> 13) & 1;
>  
>      fixed_ctrl_mask = ~((1ull << (fixed_pmc_cnt * FIXED_CTR_CTRL_BITS)) - 1);
> -    if ( version == 2 )
> -        fixed_ctrl_mask |= 0x444;
> +    fixed_ctrl_mask |= 0x444;
>      fixed_counters_mask = ~((1ull << core2_get_bitwidth_fix_count()) - 1);
>      global_ctrl_mask = ~((((1ULL << fixed_pmc_cnt) - 1) << 32) |
>                           ((1ULL << arch_pmc_cnt) - 1));


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] x86/vpmu_intel: Handle SMT consistently for programmable and fixed counters

[Jan Beulich]

>>> On 31.03.17 at 16:46, <mohit.gambhir@oracle.com> wrote:
> This patch masks .AnyThread bits in IA32_FIXED_CTR_CTRL MSR for all
> versions of Intel Arhcitectural Performance Monitoring. Note that
> .AnyThread bit (21) is already masked in IA32_PERFEVTSELx MSRs since
> hyperthreading is not exposed to guests and Intel SDM discourages the use of
> .AnyThread bit in virtualized environments (per section 18.2.3.1
> AnyThread Counting and Software Evolution)

All nice and presumably correct, but the main thing is missing: The
bits aren't defined prior to version 3 afaics, so ...

> --- a/xen/arch/x86/cpu/vpmu_intel.c
> +++ b/xen/arch/x86/cpu/vpmu_intel.c
> @@ -979,8 +979,7 @@ int __init core2_vpmu_init(void)
>      full_width_write = (caps >> 13) & 1;
>  
>      fixed_ctrl_mask = ~((1ull << (fixed_pmc_cnt * FIXED_CTR_CTRL_BITS)) - 1);
> -    if ( version == 2 )
> -        fixed_ctrl_mask |= 0x444;
> +    fixed_ctrl_mask |= 0x444;

... the main thing to explain is why removing the conditional is
(a) correct and (b) necessary (going through the uses of the
variable I can see (a) to be true, but not (b)). And of course it
would be quite helpful if the literal number changed to a
manifest constant at once, or a comment was attached to
clarify what the number represents.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] x86/vpmu_intel: Handle SMT consistently for programmable and fixed counters

[Mohit Gambhir]

[-- Attachment #1.1: Type: text/plain, Size: 2503 bytes --]



On 04/03/2017 06:36 AM, Jan Beulich wrote:
>>>> On 31.03.17 at 16:46, <mohit.gambhir@oracle.com> wrote:
>> This patch masks .AnyThread bits in IA32_FIXED_CTR_CTRL MSR for all
>> versions of Intel Arhcitectural Performance Monitoring. Note that
>> .AnyThread bit (21) is already masked in IA32_PERFEVTSELx MSRs since
>> hyperthreading is not exposed to guests and Intel SDM discourages the use of
>> .AnyThread bit in virtualized environments (per section 18.2.3.1
>> AnyThread Counting and Software Evolution)
> All nice and presumably correct, but the main thing is missing: The
> bits aren't defined prior to version 3 afaics, so ...
>
>> --- a/xen/arch/x86/cpu/vpmu_intel.c
>> +++ b/xen/arch/x86/cpu/vpmu_intel.c
>> @@ -979,8 +979,7 @@ int __init core2_vpmu_init(void)
>>       full_width_write = (caps >> 13) & 1;
>>   
>>       fixed_ctrl_mask = ~((1ull << (fixed_pmc_cnt * FIXED_CTR_CTRL_BITS)) - 1);
>> -    if ( version == 2 )
>> -        fixed_ctrl_mask |= 0x444;
>> +    fixed_ctrl_mask |= 0x444;
> ... the main thing to explain is why removing the conditional is
> (a) correct and (b) necessary (going through the uses of the
> variable I can see (a) to be true, but not (b)). And of course it
> would be quite helpful if the literal number changed to a
> manifest constant at once, or a comment was attached to
> clarify what the number represents.

I do agree that replacing the hard coded constant with a macro would be 
nice and I will update the patch with that.

The answer to why this change is (b) necessary is two folds -

1. We need to be consistent in the implementation. As said in the commit 
log - we disable .Anythread bit in
programmable counters (regardless of the version) by masking bit 21 in 
IA32_PERFEVTSELx.  (See code snippet
below from vpmu_intel.c)

  /* Masks used for testing whether and MSR is valid */
  #define ARCH_CTRL_MASK  (~((1ull << 32) - 1) | (1ull << 21))

But we leave it enabled in fixed function counters for version 3. 
Removing the condition disables the bit in fixed function
counters regardless of the version,  which is consistent with what is 
done for programmable counters.

2. We don't want to expose event counts from another guest (or 
hypervisor) which can happen if .AnyThread bit is not masked and
a VCPU is only scheduled to run on one of the hardware threads in a 
hyper-threaded CPU.

> Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 3822 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] x86/vpmu_intel: Handle SMT consistently for programmable and fixed counters

[Jan Beulich]

>>> On 03.04.17 at 21:23, <mohit.gambhir@oracle.com> wrote:

> 
> On 04/03/2017 06:36 AM, Jan Beulich wrote:
>>>>> On 31.03.17 at 16:46, <mohit.gambhir@oracle.com> wrote:
>>> This patch masks .AnyThread bits in IA32_FIXED_CTR_CTRL MSR for all
>>> versions of Intel Arhcitectural Performance Monitoring. Note that
>>> .AnyThread bit (21) is already masked in IA32_PERFEVTSELx MSRs since
>>> hyperthreading is not exposed to guests and Intel SDM discourages the use of
>>> .AnyThread bit in virtualized environments (per section 18.2.3.1
>>> AnyThread Counting and Software Evolution)
>> All nice and presumably correct, but the main thing is missing: The
>> bits aren't defined prior to version 3 afaics, so ...
>>
>>> --- a/xen/arch/x86/cpu/vpmu_intel.c
>>> +++ b/xen/arch/x86/cpu/vpmu_intel.c
>>> @@ -979,8 +979,7 @@ int __init core2_vpmu_init(void)
>>>       full_width_write = (caps >> 13) & 1;
>>>   
>>>       fixed_ctrl_mask = ~((1ull << (fixed_pmc_cnt * FIXED_CTR_CTRL_BITS)) - 
> 1);
>>> -    if ( version == 2 )
>>> -        fixed_ctrl_mask |= 0x444;
>>> +    fixed_ctrl_mask |= 0x444;
>> ... the main thing to explain is why removing the conditional is
>> (a) correct and (b) necessary (going through the uses of the
>> variable I can see (a) to be true, but not (b)). And of course it
>> would be quite helpful if the literal number changed to a
>> manifest constant at once, or a comment was attached to
>> clarify what the number represents.
> 
> I do agree that replacing the hard coded constant with a macro would be 
> nice and I will update the patch with that.
> 
> The answer to why this change is (b) necessary is two folds -
> 
> 1. We need to be consistent in the implementation. As said in the commit 
> log - we disable .Anythread bit in
> programmable counters (regardless of the version) by masking bit 21 in 
> IA32_PERFEVTSELx.  (See code snippet
> below from vpmu_intel.c)
> 
>   /* Masks used for testing whether and MSR is valid */
>   #define ARCH_CTRL_MASK  (~((1ull << 32) - 1) | (1ull << 21))
> 
> But we leave it enabled in fixed function counters for version 3. 
> Removing the condition disables the bit in fixed function
> counters regardless of the version,  which is consistent with what is 
> done for programmable counters.
> 
> 2. We don't want to expose event counts from another guest (or 
> hypervisor) which can happen if .AnyThread bit is not masked and
> a VCPU is only scheduled to run on one of the hardware threads in a 
> hyper-threaded CPU.

Okay, I can see now that I could, with some difficulty, have
read this out of the original description. I'd appreciate if this
was made more clear though (along the lines of the explanation
you gave in reply to my question).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] x86/vpmu_intel: Handle SMT consistently for programmable and fixed counters

[Tian, Kevin]

> From: Mohit Gambhir [mailto:mohit.gambhir@oracle.com]
> Sent: Friday, March 31, 2017 10:46 PM
> 
> This patch masks .AnyThread bits in IA32_FIXED_CTR_CTRL MSR for all
> versions of Intel Arhcitectural Performance Monitoring. Note that
> .AnyThread bit (21) is already masked in IA32_PERFEVTSELx MSRs since
> hyperthreading is not exposed to guests and Intel SDM discourages the use
> of
> .AnyThread bit in virtualized environments (per section 18.2.3.1
> AnyThread Counting and Software Evolution)
> 
> Signed-off-by: Mohit Gambhir <mohit.gambhir@oracle.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel