[PATCH v2 0/2] Restricted VPMU filter flags

[PATCH v2 0/2] Restricted VPMU filter flags

[Brendan Gregg]

This patch series fixes a minor bug with cpuid register usage for fixed PMC
counts, and implements two VPMU filter flags.

The VPMU feature of Xen is incredibly useful for performance analysis, however,
it is currently all counters or nothing. In secure environments, there can be
hesitation to enable access to all PMCs. This series introduces two new flags
(in addition to the existing "bts"):

vpmu=ipc: As the most restricted minimum set. This enables cycles, reference
cycles, and instructions only. This is enough to calculate instructions per
cycle (IPC).

vpm=arch: This enables the 7 pre-defined architectural events as listed in
cpuid, and in Table 18-1 of the Intel software developer's manual, vol 3B.

There can be additional flags added later on, to allow access to other groups
of PMCs.

As an example of these flags, here is Linux perf running in a PVHVM guest with
the new vpmu=ipc mode:

root@vm0hvm:~# perf stat -d ./noploop

 Performance counter stats for './noploop':

       1511.326375 task-clock (msec)         #    0.999 CPUs utilized          
                24 context-switches          #    0.016 K/sec                  
                 0 cpu-migrations            #    0.000 K/sec                  
               113 page-faults               #    0.075 K/sec                  
     5,028,638,883 cycles                    #    3.327 GHz                    
                 0 stalled-cycles-frontend   #    0.00% frontend cycles idle   
                 0 stalled-cycles-backend    #    0.00% backend  cycles idle   
    20,043,427,933 instructions              #    3.99  insns per cycle        
                 0 branches                  #    0.000 K/sec                  
                 0 branch-misses             #    0.00% of all branches        
                 0 L1-dcache-loads           #    0.000 K/sec                  
                 0 L1-dcache-load-misses     #    0.00% of all L1-dcache hits  
                 0 LLC-loads                 #    0.000 K/sec                  
   <not supported> LLC-load-misses:HG    

Note that IPC is shown ("insns per cycle"), but other counters are not.

Changes in v2:
* feature flags can now be combined (eg, "vpmu=ipc,bts")
* addressing review comments from Boris:
* restrict DS_AREA and PEBS_ENABLE access when filters are in use
* better variable types
* include MSR_IA32_CMT_EVTSEL_UE_MASK flag

Brendan Gregg (2):
  x86/VPMU: return correct fixed PMC count
  x86/VPMU: implement ipc and arch filter flags

 docs/misc/xen-command-line.markdown | 14 +++++++++-
 xen/arch/x86/cpu/vpmu.c             | 51 ++++++++++++++++++++++++++++-------
 xen/arch/x86/cpu/vpmu_intel.c       | 54 ++++++++++++++++++++++++++++++++++---
 xen/include/asm-x86/msr-index.h     |  1 +
 xen/include/public/pmu.h            | 14 ++++++++--
 5 files changed, 118 insertions(+), 16 deletions(-)

-- 
1.9.1

[PATCH v2 1/2] x86/VPMU: return correct fixed PMC count

[Brendan Gregg]

Fixes a register typo.

Signed-off-by: Brendan Gregg <bgregg@netflix.com>
---
 xen/arch/x86/cpu/vpmu_intel.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c
index 12f80ae..8d83a1a 100644
--- a/xen/arch/x86/cpu/vpmu_intel.c
+++ b/xen/arch/x86/cpu/vpmu_intel.c
@@ -166,10 +166,10 @@ static int core2_get_arch_pmc_count(void)
  */
 static int core2_get_fixed_pmc_count(void)
 {
-    u32 eax;
+    u32 edx;
 
-    eax = cpuid_eax(0xa);
-    return MASK_EXTR(eax, PMU_FIXED_NR_MASK);
+    edx = cpuid_edx(0xa);
+    return MASK_EXTR(edx, PMU_FIXED_NR_MASK);
 }
 
 /* edx bits 5-12: Bit width of fixed-function performance counters  */
-- 
1.9.1

Re: [PATCH v2 1/2] x86/VPMU: return correct fixed PMC count

[Dietmar Hahn]

Am Dienstag 24 November 2015, 15:53:11 schrieb Brendan Gregg:
> Fixes a register typo.
> 
> Signed-off-by: Brendan Gregg <bgregg@netflix.com>
> ---
>  xen/arch/x86/cpu/vpmu_intel.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Dietmar Hahn <dietmar.hahn@ts.fujitsu.com>

> 
> diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c
> index 12f80ae..8d83a1a 100644
> --- a/xen/arch/x86/cpu/vpmu_intel.c
> +++ b/xen/arch/x86/cpu/vpmu_intel.c
> @@ -166,10 +166,10 @@ static int core2_get_arch_pmc_count(void)
>   */
>  static int core2_get_fixed_pmc_count(void)
>  {
> -    u32 eax;
> +    u32 edx;
>  
> -    eax = cpuid_eax(0xa);
> -    return MASK_EXTR(eax, PMU_FIXED_NR_MASK);
> +    edx = cpuid_edx(0xa);
> +    return MASK_EXTR(edx, PMU_FIXED_NR_MASK);
>  }
>  
>  /* edx bits 5-12: Bit width of fixed-function performance counters  */
> 

-- 
Company details: http://ts.fujitsu.com/imprint.html

Re: [PATCH v2 1/2] x86/VPMU: return correct fixed PMC count

[Jan Beulich]

>>> On 25.11.15 at 00:53, <bgregg@netflix.com> wrote:
> --- a/xen/arch/x86/cpu/vpmu_intel.c
> +++ b/xen/arch/x86/cpu/vpmu_intel.c
> @@ -166,10 +166,10 @@ static int core2_get_arch_pmc_count(void)
>   */
>  static int core2_get_fixed_pmc_count(void)
>  {
> -    u32 eax;
> +    u32 edx;
>  
> -    eax = cpuid_eax(0xa);
> -    return MASK_EXTR(eax, PMU_FIXED_NR_MASK);
> +    edx = cpuid_edx(0xa);
> +    return MASK_EXTR(edx, PMU_FIXED_NR_MASK);
>  }
>  
>  /* edx bits 5-12: Bit width of fixed-function performance counters  */

I'll commit as is since it's an immediate improvement, but I don't think
this is sufficient: The SDM clearly says "if Version ID > 1", which isn't
being tested here or in the immediately following function.

Looking at this I'd also like to note that the triplets
PMU_*_{SHIFT,BITS,MASK} seem to be rather less readable than if
there were just PMU_*_MASK with a simple hex number on the right
side (the SHIFT and BITS ones aren't being used for other than
defining MASK afaics).

Jan

Re: [PATCH v2 1/2] x86/VPMU: return correct fixed PMC count

[Boris Ostrovsky]

On 11/25/2015 04:55 AM, Jan Beulich wrote:
>>>> On 25.11.15 at 00:53, <bgregg@netflix.com> wrote:
>> --- a/xen/arch/x86/cpu/vpmu_intel.c
>> +++ b/xen/arch/x86/cpu/vpmu_intel.c
>> @@ -166,10 +166,10 @@ static int core2_get_arch_pmc_count(void)
>>    */
>>   static int core2_get_fixed_pmc_count(void)
>>   {
>> -    u32 eax;
>> +    u32 edx;
>>   
>> -    eax = cpuid_eax(0xa);
>> -    return MASK_EXTR(eax, PMU_FIXED_NR_MASK);
>> +    edx = cpuid_edx(0xa);
>> +    return MASK_EXTR(edx, PMU_FIXED_NR_MASK);
>>   }
>>   
>>   /* edx bits 5-12: Bit width of fixed-function performance counters  */
> I'll commit as is since it's an immediate improvement, but I don't think
> this is sufficient: The SDM clearly says "if Version ID > 1", which isn't
> being tested here or in the immediately following function.

Are you referring to the statement in 18.2.2:
   "The enhanced features provided by architectural performance 
monitoring version 2 include the following" ?

I'd expect CPUID to report zeroes for those enhanced featured in v1.

However, I just noticed that various control and status registers are 
not available for v1. I wonder whether we should even support version 1 
since we'd need to add whole lot of 'if (supported)' throughout the code 
plus there are some assumptions about existence of IA32_PERF_GLOBAL_CTRL 
so we'll need to add additional logic to handle that too. And it's not 
clear to me if it's all worth it.

-boris

Re: [PATCH v2 1/2] x86/VPMU: return correct fixed PMC count

[Jan Beulich]

>>> On 25.11.15 at 15:26, <boris.ostrovsky@oracle.com> wrote:
> On 11/25/2015 04:55 AM, Jan Beulich wrote:
>>>>> On 25.11.15 at 00:53, <bgregg@netflix.com> wrote:
>>> --- a/xen/arch/x86/cpu/vpmu_intel.c
>>> +++ b/xen/arch/x86/cpu/vpmu_intel.c
>>> @@ -166,10 +166,10 @@ static int core2_get_arch_pmc_count(void)
>>>    */
>>>   static int core2_get_fixed_pmc_count(void)
>>>   {
>>> -    u32 eax;
>>> +    u32 edx;
>>>   
>>> -    eax = cpuid_eax(0xa);
>>> -    return MASK_EXTR(eax, PMU_FIXED_NR_MASK);
>>> +    edx = cpuid_edx(0xa);
>>> +    return MASK_EXTR(edx, PMU_FIXED_NR_MASK);
>>>   }
>>>   
>>>   /* edx bits 5-12: Bit width of fixed-function performance counters  */
>> I'll commit as is since it's an immediate improvement, but I don't think
>> this is sufficient: The SDM clearly says "if Version ID > 1", which isn't
>> being tested here or in the immediately following function.
> 
> Are you referring to the statement in 18.2.2:
>    "The enhanced features provided by architectural performance 
> monitoring version 2 include the following" ?

No, I was actually referring to the statement in the CPUID table
in Vol 2.

> However, I just noticed that various control and status registers are 
> not available for v1. I wonder whether we should even support version 1 
> since we'd need to add whole lot of 'if (supported)' throughout the code 
> plus there are some assumptions about existence of IA32_PERF_GLOBAL_CTRL 
> so we'll need to add additional logic to handle that too. And it's not 
> clear to me if it's all worth it.

Indeed, let's not support v1 then for now and leave the exercise
to add all the if()s to whoever cares for such support.

Jan

Re: [PATCH v2 1/2] x86/VPMU: return correct fixed PMC count

[Boris Ostrovsky]

On 11/25/2015 10:31 AM, Jan Beulich wrote:

>> However, I just noticed that various control and status registers are
>> not available for v1. I wonder whether we should even support version 1
>> since we'd need to add whole lot of 'if (supported)' throughout the code
>> plus there are some assumptions about existence of IA32_PERF_GLOBAL_CTRL
>> so we'll need to add additional logic to handle that too. And it's not
>> clear to me if it's all worth it.
> Indeed, let's not support v1 then for now and leave the exercise
> to add all the if()s to whoever cares for such support.

And, in fact, I think we should drop model check in core2_vpmu_init() 
and only test for PMU version. Especially in light of XSA-163.

We could limit support to versions 2 and 3 only if we want to be on the 
safe side.

If people agree I'll send a patch (on Monday).


-boris

Re: [PATCH v2 1/2] x86/VPMU: return correct fixed PMC count

[Jan Beulich]

>>> On 25.11.15 at 20:32, <boris.ostrovsky@oracle.com> wrote:
> On 11/25/2015 10:31 AM, Jan Beulich wrote:
>>> However, I just noticed that various control and status registers are
>>> not available for v1. I wonder whether we should even support version 1
>>> since we'd need to add whole lot of 'if (supported)' throughout the code
>>> plus there are some assumptions about existence of IA32_PERF_GLOBAL_CTRL
>>> so we'll need to add additional logic to handle that too. And it's not
>>> clear to me if it's all worth it.
>> Indeed, let's not support v1 then for now and leave the exercise
>> to add all the if()s to whoever cares for such support.
> 
> And, in fact, I think we should drop model check in core2_vpmu_init() 
> and only test for PMU version. Especially in light of XSA-163.
> 
> We could limit support to versions 2 and 3 only if we want to be on the 
> safe side.
> 
> If people agree I'll send a patch (on Monday).

Yes please.

Jan

[PATCH v2 2/2] x86/VPMU: implement ipc and arch filter flags

[Brendan Gregg]

This introduces a way to have a restricted VPMU, by specifying one of two
predefined groups of PMCs to make available. For secure environments, this
allows the VPMU to be used without needing to enable all PMCs.

Signed-off-by: Brendan Gregg <bgregg@netflix.com>
---
 docs/misc/xen-command-line.markdown | 14 +++++++++-
 xen/arch/x86/cpu/vpmu.c             | 51 +++++++++++++++++++++++++++++--------
 xen/arch/x86/cpu/vpmu_intel.c       | 48 ++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/msr-index.h     |  1 +
 xen/include/public/pmu.h            | 14 ++++++++--
 5 files changed, 115 insertions(+), 13 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 70daa84..6055a68 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1452,7 +1452,7 @@ Use Virtual Processor ID support if available.  This prevents the need for TLB
 flushes on VM entry and exit, increasing performance.
 
 ### vpmu
-> `= ( bts )`
+> `= ( <boolean> | { bts | ipc | arch [, ...] } )`
 
 > Default: `off`
 
@@ -1468,6 +1468,18 @@ wrong behaviour (see handle\_pmc\_quirk()).
 If 'vpmu=bts' is specified the virtualisation of the Branch Trace Store (BTS)
 feature is switched on on Intel processors supporting this feature.
 
+vpmu=ipc enables performance monitoring, but restricts the counters to the
+most minimum set possible: instructions, cycles, and reference cycles. These
+can be used to calculate instructions per cycle (IPC).
+
+vpmu=arch enables performance monitoring, but restricts the counters to the
+pre-defined architectural events only. These are exposed by cpuid, and listed
+in Table 18-1 from the Intel 64 and IA-32 Architectures Software Developer's
+Manual, Volume 3B, System Programming Guide, Part 2.
+
+If a boolean is not used, combinations of flags are allowed, comma separated.
+For example, vpmu=arch,bts.
+
 Note that if **watchdog** option is also specified vpmu will be turned off.
 
 *Warning:*
diff --git a/xen/arch/x86/cpu/vpmu.c b/xen/arch/x86/cpu/vpmu.c
index 2f5156a..bb0ca37 100644
--- a/xen/arch/x86/cpu/vpmu.c
+++ b/xen/arch/x86/cpu/vpmu.c
@@ -43,33 +43,64 @@ CHECK_pmu_data;
 CHECK_pmu_params;
 
 /*
- * "vpmu" :     vpmu generally enabled
- * "vpmu=off" : vpmu generally disabled
- * "vpmu=bts" : vpmu enabled and Intel BTS feature switched on.
+ * "vpmu" :     vpmu generally enabled (all counters)
+ * "vpmu=off"  : vpmu generally disabled
+ * "vpmu=bts"  : vpmu enabled and Intel BTS feature switched on.
+ * "vpmu=ipc"  : vpmu enabled for IPC counters only (most restrictive)
+ * "vpmu=arch" : vpmu enabled for predef arch counters only (restrictive)
+ * flag combinations are allowed, eg, "vpmu=ipc,bts".
  */
 static unsigned int __read_mostly opt_vpmu_enabled;
 unsigned int __read_mostly vpmu_mode = XENPMU_MODE_OFF;
 unsigned int __read_mostly vpmu_features = 0;
-static void parse_vpmu_param(char *s);
-custom_param("vpmu", parse_vpmu_param);
+static void parse_vpmu_params(char *s);
+custom_param("vpmu", parse_vpmu_params);
 
 static DEFINE_SPINLOCK(vpmu_lock);
 static unsigned vpmu_count;
 
 static DEFINE_PER_CPU(struct vcpu *, last_vcpu);
 
-static void __init parse_vpmu_param(char *s)
+static int parse_vpmu_param(char *s, int len)
 {
+    if ( ! *s || ! len )
+        return 0;
+    if ( !strncmp(s, "bts", len) )
+        vpmu_features |= XENPMU_FEATURE_INTEL_BTS;
+    else if ( !strncmp(s, "ipc", len) )
+        vpmu_features |= XENPMU_FEATURE_IPC_ONLY;
+    else if ( !strncmp(s, "arch", len) )
+        vpmu_features |= XENPMU_FEATURE_ARCH_ONLY;
+    else if ( *s )
+    {
+        return 1;
+    }
+    return 0;
+}
+
+static void __init parse_vpmu_params(char *s)
+{
+    bool_t badflag = 0;
+    char *sep, *p = s;
+
     switch ( parse_bool(s) )
     {
     case 0:
         break;
     default:
-        if ( !strcmp(s, "bts") )
-            vpmu_features |= XENPMU_FEATURE_INTEL_BTS;
-        else if ( *s )
+        sep = strchr(p, ',');
+        while (sep != NULL)
+        {
+            if ( parse_vpmu_param(p, sep - p) )
+                badflag = 1;
+            p = sep + 1;
+            sep = strchr(p, ',');
+        }
+        sep = strchr(p, 0);
+        parse_vpmu_param(p, sep - p);
+        if ( badflag )
         {
-            printk("VPMU: unknown flag: %s - vpmu disabled!\n", s);
+            printk("VPMU: unknown flags: %s - vpmu disabled!\n", s);
             break;
         }
         /* fall through */
diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c
index 8d83a1a..b26a20b 100644
--- a/xen/arch/x86/cpu/vpmu_intel.c
+++ b/xen/arch/x86/cpu/vpmu_intel.c
@@ -602,12 +602,19 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content,
                  "MSR_PERF_GLOBAL_STATUS(0x38E)!\n");
         return -EINVAL;
     case MSR_IA32_PEBS_ENABLE:
+        if ( vpmu_features & XENPMU_FEATURE_IPC_ONLY ||
+             vpmu_features & XENPMU_FEATURE_ARCH_ONLY )
+            return -EINVAL;
         if ( msr_content & 1 )
             gdprintk(XENLOG_WARNING, "Guest is trying to enable PEBS, "
                      "which is not supported.\n");
         core2_vpmu_cxt->pebs_enable = msr_content;
         return 0;
     case MSR_IA32_DS_AREA:
+        if ( (vpmu_features & XENPMU_FEATURE_IPC_ONLY ||
+             vpmu_features & XENPMU_FEATURE_ARCH_ONLY) &&
+             !(vpmu_features & XENPMU_FEATURE_INTEL_BTS) )
+            return -EINVAL;
         if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_DS) )
         {
             if ( !is_canonical_address(msr_content) )
@@ -652,12 +659,53 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content,
         tmp = msr - MSR_P6_EVNTSEL(0);
         if ( tmp >= 0 && tmp < arch_pmc_cnt )
         {
+            bool_t blocked = 0;
+            uint64_t umaskevent;
             struct xen_pmu_cntr_pair *xen_pmu_cntr_pair =
                 vpmu_reg_pointer(core2_vpmu_cxt, arch_counters);
 
             if ( msr_content & ARCH_CTRL_MASK )
                 return -EINVAL;
 
+            /* PMC filters */
+            umaskevent = msr_content & MSR_IA32_CMT_EVTSEL_UE_MASK;
+            if ( vpmu_features & XENPMU_FEATURE_IPC_ONLY ||
+                 vpmu_features & XENPMU_FEATURE_ARCH_ONLY )
+            {
+                blocked = 1;
+                switch ( umaskevent )
+                {
+                /*
+                 * See Table 18-1 from the Intel 64 and IA-32 Architectures Software
+                 * Developer's Manual, Volume 3B, System Programming Guide, Part 2.
+                 */
+                case 0x003c:	/* unhalted core cycles */
+                case 0x013c:	/* unhalted ref cycles */
+                case 0x00c0:	/* instruction retired */
+                    blocked = 0;
+                default:
+                    break;
+                }
+            }
+
+            if ( vpmu_features & XENPMU_FEATURE_ARCH_ONLY )
+            {
+                /* additional counters beyond IPC only; blocked already set */
+                switch ( umaskevent )
+                {
+                case 0x4f2e:	/* LLC reference */
+                case 0x412e:	/* LLC misses */
+                case 0x00c4:	/* branch instruction retired */
+                case 0x00c5:	/* branch */
+                    blocked = 0;
+                default:
+                    break;
+               }
+            }
+
+            if ( blocked )
+                return -EINVAL;
+
             if ( has_hvm_container_vcpu(v) )
                 vmx_read_guest_msr(MSR_CORE_PERF_GLOBAL_CTRL,
                                    &core2_vpmu_cxt->global_ctrl);
diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
index b8ad93c..0542064 100644
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -328,6 +328,7 @@
 
 /* Platform Shared Resource MSRs */
 #define MSR_IA32_CMT_EVTSEL		0x00000c8d
+#define MSR_IA32_CMT_EVTSEL_UE_MASK	0x0000ffff
 #define MSR_IA32_CMT_CTR		0x00000c8e
 #define MSR_IA32_PSR_ASSOC		0x00000c8f
 #define MSR_IA32_PSR_L3_QOS_CFG	0x00000c81
diff --git a/xen/include/public/pmu.h b/xen/include/public/pmu.h
index 7753df0..c7b5648 100644
--- a/xen/include/public/pmu.h
+++ b/xen/include/public/pmu.h
@@ -84,9 +84,19 @@ DEFINE_XEN_GUEST_HANDLE(xen_pmu_params_t);
 
 /*
  * PMU features:
- * - XENPMU_FEATURE_INTEL_BTS: Intel BTS support (ignored on AMD)
+ * - XENPMU_FEATURE_INTEL_BTS:  Intel BTS support (ignored on AMD)
+ * - XENPMU_FEATURE_IPC_ONLY:   Restrict PMC to the most minimum set possible.
+ *                              Instructions, cycles, and ref cycles. Can be
+ *                              used to calculate instructions-per-cycle (IPC)
+ *                              (ignored on AMD).
+ * - XENPMU_FEATURE_ARCH_ONLY:  Restrict PMCs to the Intel pre-defined
+ *                              architecteral events exposed by cpuid and
+ *                              listed in Table 18-1 of the developer's manual
+ *                              (ignored on AMD).
  */
-#define XENPMU_FEATURE_INTEL_BTS  1
+#define XENPMU_FEATURE_INTEL_BTS  (1<<0)
+#define XENPMU_FEATURE_IPC_ONLY   (1<<1)
+#define XENPMU_FEATURE_ARCH_ONLY  (1<<2)
 
 /*
  * Shared PMU data between hypervisor and PV(H) domains.
-- 
1.9.1

Re: [PATCH v2 2/2] x86/VPMU: implement ipc and arch filter flags

[Boris Ostrovsky]

On 11/24/2015 06:53 PM, Brendan Gregg wrote:
> This introduces a way to have a restricted VPMU, by specifying one of two
> predefined groups of PMCs to make available. For secure environments, this
> allows the VPMU to be used without needing to enable all PMCs.
>
> Signed-off-by: Brendan Gregg <bgregg@netflix.com>
> ---
>   docs/misc/xen-command-line.markdown | 14 +++++++++-
>   xen/arch/x86/cpu/vpmu.c             | 51 +++++++++++++++++++++++++++++--------
>   xen/arch/x86/cpu/vpmu_intel.c       | 48 ++++++++++++++++++++++++++++++++++
>   xen/include/asm-x86/msr-index.h     |  1 +
>   xen/include/public/pmu.h            | 14 ++++++++--
>   5 files changed, 115 insertions(+), 13 deletions(-)
>
> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
> index 70daa84..6055a68 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1452,7 +1452,7 @@ Use Virtual Processor ID support if available.  This prevents the need for TLB
>   flushes on VM entry and exit, increasing performance.
>   
>   ### vpmu
> -> `= ( bts )`
> +> `= ( <boolean> | { bts | ipc | arch [, ...] } )`
>   
>   > Default: `off`
>   
> @@ -1468,6 +1468,18 @@ wrong behaviour (see handle\_pmc\_quirk()).
>   If 'vpmu=bts' is specified the virtualisation of the Branch Trace Store (BTS)
>   feature is switched on on Intel processors supporting this feature.
>   
> +vpmu=ipc enables performance monitoring, but restricts the counters to the
> +most minimum set possible: instructions, cycles, and reference cycles. These
> +can be used to calculate instructions per cycle (IPC).
> +
> +vpmu=arch enables performance monitoring, but restricts the counters to the
> +pre-defined architectural events only. These are exposed by cpuid, and listed
> +in Table 18-1 from the Intel 64 and IA-32 Architectures Software Developer's
> +Manual, Volume 3B, System Programming Guide, Part 2.
> +
> +If a boolean is not used, combinations of flags are allowed, comma separated.
> +For example, vpmu=arch,bts.
> +
>   Note that if **watchdog** option is also specified vpmu will be turned off.
>   
>   *Warning:*
> diff --git a/xen/arch/x86/cpu/vpmu.c b/xen/arch/x86/cpu/vpmu.c
> index 2f5156a..bb0ca37 100644
> --- a/xen/arch/x86/cpu/vpmu.c
> +++ b/xen/arch/x86/cpu/vpmu.c
> @@ -43,33 +43,64 @@ CHECK_pmu_data;
>   CHECK_pmu_params;
>   
>   /*
> - * "vpmu" :     vpmu generally enabled
> - * "vpmu=off" : vpmu generally disabled
> - * "vpmu=bts" : vpmu enabled and Intel BTS feature switched on.
> + * "vpmu" :     vpmu generally enabled (all counters)
> + * "vpmu=off"  : vpmu generally disabled
> + * "vpmu=bts"  : vpmu enabled and Intel BTS feature switched on.
> + * "vpmu=ipc"  : vpmu enabled for IPC counters only (most restrictive)
> + * "vpmu=arch" : vpmu enabled for predef arch counters only (restrictive)
> + * flag combinations are allowed, eg, "vpmu=ipc,bts".
>    */
>   static unsigned int __read_mostly opt_vpmu_enabled;
>   unsigned int __read_mostly vpmu_mode = XENPMU_MODE_OFF;
>   unsigned int __read_mostly vpmu_features = 0;
> -static void parse_vpmu_param(char *s);
> -custom_param("vpmu", parse_vpmu_param);
> +static void parse_vpmu_params(char *s);
> +custom_param("vpmu", parse_vpmu_params);
>   
>   static DEFINE_SPINLOCK(vpmu_lock);
>   static unsigned vpmu_count;
>   
>   static DEFINE_PER_CPU(struct vcpu *, last_vcpu);
>   
> -static void __init parse_vpmu_param(char *s)
> +static int parse_vpmu_param(char *s, int len)
>   {
> +    if ( ! *s || ! len )
> +        return 0;
> +    if ( !strncmp(s, "bts", len) )
> +        vpmu_features |= XENPMU_FEATURE_INTEL_BTS;
> +    else if ( !strncmp(s, "ipc", len) )
> +        vpmu_features |= XENPMU_FEATURE_IPC_ONLY;
> +    else if ( !strncmp(s, "arch", len) )
> +        vpmu_features |= XENPMU_FEATURE_ARCH_ONLY;
> +    else if ( *s )

Why not just "else return 1;" ? We've already tested above that *s is 
not '\0'. (And you don't need curly braces for single-line clauses)


> +    {
> +        return 1;
> +    }
> +    return 0;
> +}
> +
> +static void __init parse_vpmu_params(char *s)
> +{
> +    bool_t badflag = 0;
> +    char *sep, *p = s;
> +
>       switch ( parse_bool(s) )
>       {
>       case 0:
>           break;
>       default:
> -        if ( !strcmp(s, "bts") )
> -            vpmu_features |= XENPMU_FEATURE_INTEL_BTS;
> -        else if ( *s )
> +        sep = strchr(p, ',');
> +        while (sep != NULL)
> +        {
> +            if ( parse_vpmu_param(p, sep - p) )
> +                badflag = 1;
> +            p = sep + 1;
> +            sep = strchr(p, ',');
> +        }
> +        sep = strchr(p, 0);
> +        parse_vpmu_param(p, sep - p);

This can find unsupported flag too but we are not setting badflag so we 
will miss it (i.e. "vpmu=foo").

Can you just say something like
     sep = strchr(p, ',')
     if ( sep == NULL )
         sep = strchr(p, 0);

and keep both parse_vpmu_param() invocations in a single loop? And then, 
instead of having badflags simply print the warning and break.

> +        if ( badflag )
>           {
> -            printk("VPMU: unknown flag: %s - vpmu disabled!\n", s);
> +            printk("VPMU: unknown flags: %s - vpmu disabled!\n", s);
>               break;
>           }
>           /* fall through */
> diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c
> index 8d83a1a..b26a20b 100644
> --- a/xen/arch/x86/cpu/vpmu_intel.c
> +++ b/xen/arch/x86/cpu/vpmu_intel.c
> @@ -602,12 +602,19 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content,
>                    "MSR_PERF_GLOBAL_STATUS(0x38E)!\n");
>           return -EINVAL;
>       case MSR_IA32_PEBS_ENABLE:
> +        if ( vpmu_features & XENPMU_FEATURE_IPC_ONLY ||
> +             vpmu_features & XENPMU_FEATURE_ARCH_ONLY )

It's is a matter of taste but you could also use
     vpmu_features & (XENPMU_FEATURE_IPC_ONLY | XENPMU_FEATURE_ARCH_ONLY)

-boris

Re: [PATCH v2 2/2] x86/VPMU: implement ipc and arch filter flags

[Brendan Gregg]

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

On Wed, Nov 25, 2015 at 7:13 AM, Boris Ostrovsky <boris.ostrovsky@oracle.com
> wrote:

> On 11/24/2015 06:53 PM, Brendan Gregg wrote:
>
>> This introduces a way to have a restricted VPMU, by specifying one of two
>> predefined groups of PMCs to make available. For secure environments, this
>> allows the VPMU to be used without needing to enable all PMCs.
>>
>> Signed-off-by: Brendan Gregg <bgregg@netflix.com>
>> ---
>>   docs/misc/xen-command-line.markdown | 14 +++++++++-
>>   xen/arch/x86/cpu/vpmu.c             | 51
>> +++++++++++++++++++++++++++++--------
>>   xen/arch/x86/cpu/vpmu_intel.c       | 48
>> ++++++++++++++++++++++++++++++++++
>>   xen/include/asm-x86/msr-index.h     |  1 +
>>   xen/include/public/pmu.h            | 14 ++++++++--
>>   5 files changed, 115 insertions(+), 13 deletions(-)
>>
>> diff --git a/docs/misc/xen-command-line.markdown
>> b/docs/misc/xen-command-line.markdown
>> index 70daa84..6055a68 100644
>> --- a/docs/misc/xen-command-line.markdown
>> +++ b/docs/misc/xen-command-line.markdown
>> @@ -1452,7 +1452,7 @@ Use Virtual Processor ID support if available.
>> This prevents the need for TLB
>>   flushes on VM entry and exit, increasing performance.
>>     ### vpmu
>> -> `= ( bts )`
>> +> `= ( <boolean> | { bts | ipc | arch [, ...] } )`
>>     > Default: `off`
>>   @@ -1468,6 +1468,18 @@ wrong behaviour (see handle\_pmc\_quirk()).
>>   If 'vpmu=bts' is specified the virtualisation of the Branch Trace Store
>> (BTS)
>>   feature is switched on on Intel processors supporting this feature.
>>   +vpmu=ipc enables performance monitoring, but restricts the counters to
>> the
>> +most minimum set possible: instructions, cycles, and reference cycles.
>> These
>> +can be used to calculate instructions per cycle (IPC).
>> +
>> +vpmu=arch enables performance monitoring, but restricts the counters to
>> the
>> +pre-defined architectural events only. These are exposed by cpuid, and
>> listed
>> +in Table 18-1 from the Intel 64 and IA-32 Architectures Software
>> Developer's
>> +Manual, Volume 3B, System Programming Guide, Part 2.
>> +
>> +If a boolean is not used, combinations of flags are allowed, comma
>> separated.
>> +For example, vpmu=arch,bts.
>> +
>>   Note that if **watchdog** option is also specified vpmu will be turned
>> off.
>>     *Warning:*
>> diff --git a/xen/arch/x86/cpu/vpmu.c b/xen/arch/x86/cpu/vpmu.c
>> index 2f5156a..bb0ca37 100644
>> --- a/xen/arch/x86/cpu/vpmu.c
>> +++ b/xen/arch/x86/cpu/vpmu.c
>> @@ -43,33 +43,64 @@ CHECK_pmu_data;
>>   CHECK_pmu_params;
>>     /*
>> - * "vpmu" :     vpmu generally enabled
>> - * "vpmu=off" : vpmu generally disabled
>> - * "vpmu=bts" : vpmu enabled and Intel BTS feature switched on.
>> + * "vpmu" :     vpmu generally enabled (all counters)
>> + * "vpmu=off"  : vpmu generally disabled
>> + * "vpmu=bts"  : vpmu enabled and Intel BTS feature switched on.
>> + * "vpmu=ipc"  : vpmu enabled for IPC counters only (most restrictive)
>> + * "vpmu=arch" : vpmu enabled for predef arch counters only (restrictive)
>> + * flag combinations are allowed, eg, "vpmu=ipc,bts".
>>    */
>>   static unsigned int __read_mostly opt_vpmu_enabled;
>>   unsigned int __read_mostly vpmu_mode = XENPMU_MODE_OFF;
>>   unsigned int __read_mostly vpmu_features = 0;
>> -static void parse_vpmu_param(char *s);
>> -custom_param("vpmu", parse_vpmu_param);
>> +static void parse_vpmu_params(char *s);
>> +custom_param("vpmu", parse_vpmu_params);
>>     static DEFINE_SPINLOCK(vpmu_lock);
>>   static unsigned vpmu_count;
>>     static DEFINE_PER_CPU(struct vcpu *, last_vcpu);
>>   -static void __init parse_vpmu_param(char *s)
>> +static int parse_vpmu_param(char *s, int len)
>>   {
>> +    if ( ! *s || ! len )
>> +        return 0;
>> +    if ( !strncmp(s, "bts", len) )
>> +        vpmu_features |= XENPMU_FEATURE_INTEL_BTS;
>> +    else if ( !strncmp(s, "ipc", len) )
>> +        vpmu_features |= XENPMU_FEATURE_IPC_ONLY;
>> +    else if ( !strncmp(s, "arch", len) )
>> +        vpmu_features |= XENPMU_FEATURE_ARCH_ONLY;
>> +    else if ( *s )
>>
>
> Why not just "else return 1;" ? We've already tested above that *s is not
> '\0'. (And you don't need curly braces for single-line clauses)
>
>
Ok, thanks.


> +    {
>> +        return 1;
>> +    }
>> +    return 0;
>> +}
>> +
>> +static void __init parse_vpmu_params(char *s)
>> +{
>> +    bool_t badflag = 0;
>> +    char *sep, *p = s;
>> +
>>       switch ( parse_bool(s) )
>>       {
>>       case 0:
>>           break;
>>       default:
>> -        if ( !strcmp(s, "bts") )
>> -            vpmu_features |= XENPMU_FEATURE_INTEL_BTS;
>> -        else if ( *s )
>> +        sep = strchr(p, ',');
>> +        while (sep != NULL)
>> +        {
>> +            if ( parse_vpmu_param(p, sep - p) )
>> +                badflag = 1;
>> +            p = sep + 1;
>> +            sep = strchr(p, ',');
>> +        }
>> +        sep = strchr(p, 0);
>> +        parse_vpmu_param(p, sep - p);
>>
>
> This can find unsupported flag too but we are not setting badflag so we
> will miss it (i.e. "vpmu=foo").
>
> Can you just say something like
>     sep = strchr(p, ',')
>     if ( sep == NULL )
>         sep = strchr(p, 0);
>
> and keep both parse_vpmu_param() invocations in a single loop? And then,
> instead of having badflags simply print the warning and break.


Ah, thanks, I'll tidy it up. I still need to do something to avoid the fall
through and enabling the VPMU, so either use of a badflags, or a goto
error, or something.


>
> +        if ( badflag )
>>           {
>> -            printk("VPMU: unknown flag: %s - vpmu disabled!\n", s);
>> +            printk("VPMU: unknown flags: %s - vpmu disabled!\n", s);
>>               break;
>>           }
>>           /* fall through */
>> diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c
>> index 8d83a1a..b26a20b 100644
>> --- a/xen/arch/x86/cpu/vpmu_intel.c
>> +++ b/xen/arch/x86/cpu/vpmu_intel.c
>> @@ -602,12 +602,19 @@ static int core2_vpmu_do_wrmsr(unsigned int msr,
>> uint64_t msr_content,
>>                    "MSR_PERF_GLOBAL_STATUS(0x38E)!\n");
>>           return -EINVAL;
>>       case MSR_IA32_PEBS_ENABLE:
>> +        if ( vpmu_features & XENPMU_FEATURE_IPC_ONLY ||
>> +             vpmu_features & XENPMU_FEATURE_ARCH_ONLY )
>>
>
> It's is a matter of taste but you could also use
>     vpmu_features & (XENPMU_FEATURE_IPC_ONLY | XENPMU_FEATURE_ARCH_ONLY)


Ok, thanks!

Brendan


>
>
> -boris
>
>


-- 
Brendan Gregg, Senior Performance Architect, Netflix

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

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

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

Re: [PATCH v2 2/2] x86/VPMU: implement ipc and arch filter flags

[Dietmar Hahn]

Am Dienstag 24 November 2015, 15:53:12 schrieb Brendan Gregg:
> This introduces a way to have a restricted VPMU, by specifying one of two
> predefined groups of PMCs to make available. For secure environments, this
> allows the VPMU to be used without needing to enable all PMCs.
> 
> Signed-off-by: Brendan Gregg <bgregg@netflix.com>
> ---
>  docs/misc/xen-command-line.markdown | 14 +++++++++-
>  xen/arch/x86/cpu/vpmu.c             | 51 +++++++++++++++++++++++++++++--------
>  xen/arch/x86/cpu/vpmu_intel.c       | 48 ++++++++++++++++++++++++++++++++++
>  xen/include/asm-x86/msr-index.h     |  1 +
>  xen/include/public/pmu.h            | 14 ++++++++--
>  5 files changed, 115 insertions(+), 13 deletions(-)
> 
> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
> index 70daa84..6055a68 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1452,7 +1452,7 @@ Use Virtual Processor ID support if available.  This prevents the need for TLB
>  flushes on VM entry and exit, increasing performance.
>  
>  ### vpmu
> -> `= ( bts )`
> +> `= ( <boolean> | { bts | ipc | arch [, ...] } )`
>  
>  > Default: `off`
>  
> @@ -1468,6 +1468,18 @@ wrong behaviour (see handle\_pmc\_quirk()).
>  If 'vpmu=bts' is specified the virtualisation of the Branch Trace Store (BTS)
>  feature is switched on on Intel processors supporting this feature.
>  
> +vpmu=ipc enables performance monitoring, but restricts the counters to the
> +most minimum set possible: instructions, cycles, and reference cycles. These
> +can be used to calculate instructions per cycle (IPC).
> +
> +vpmu=arch enables performance monitoring, but restricts the counters to the
> +pre-defined architectural events only. These are exposed by cpuid, and listed
> +in Table 18-1 from the Intel 64 and IA-32 Architectures Software Developer's
> +Manual, Volume 3B, System Programming Guide, Part 2.

Maybe you better should write
section "Pre-defined Architectural Performance Events"
instead of "Table 18-1" because the number may change in the document.
And below in the comment of the code too.

Dietmar.

> +
> +If a boolean is not used, combinations of flags are allowed, comma separated.
> +For example, vpmu=arch,bts.
> +
>  Note that if **watchdog** option is also specified vpmu will be turned off.
>  
>  *Warning:*
> diff --git a/xen/arch/x86/cpu/vpmu.c b/xen/arch/x86/cpu/vpmu.c
> index 2f5156a..bb0ca37 100644
> --- a/xen/arch/x86/cpu/vpmu.c
> +++ b/xen/arch/x86/cpu/vpmu.c
> @@ -43,33 +43,64 @@ CHECK_pmu_data;
>  CHECK_pmu_params;
>  
>  /*
> - * "vpmu" :     vpmu generally enabled
> - * "vpmu=off" : vpmu generally disabled
> - * "vpmu=bts" : vpmu enabled and Intel BTS feature switched on.
> + * "vpmu" :     vpmu generally enabled (all counters)
> + * "vpmu=off"  : vpmu generally disabled
> + * "vpmu=bts"  : vpmu enabled and Intel BTS feature switched on.
> + * "vpmu=ipc"  : vpmu enabled for IPC counters only (most restrictive)
> + * "vpmu=arch" : vpmu enabled for predef arch counters only (restrictive)
> + * flag combinations are allowed, eg, "vpmu=ipc,bts".
>   */
>  static unsigned int __read_mostly opt_vpmu_enabled;
>  unsigned int __read_mostly vpmu_mode = XENPMU_MODE_OFF;
>  unsigned int __read_mostly vpmu_features = 0;
> -static void parse_vpmu_param(char *s);
> -custom_param("vpmu", parse_vpmu_param);
> +static void parse_vpmu_params(char *s);
> +custom_param("vpmu", parse_vpmu_params);
>  
>  static DEFINE_SPINLOCK(vpmu_lock);
>  static unsigned vpmu_count;
>  
>  static DEFINE_PER_CPU(struct vcpu *, last_vcpu);
>  
> -static void __init parse_vpmu_param(char *s)
> +static int parse_vpmu_param(char *s, int len)
>  {
> +    if ( ! *s || ! len )
> +        return 0;
> +    if ( !strncmp(s, "bts", len) )
> +        vpmu_features |= XENPMU_FEATURE_INTEL_BTS;
> +    else if ( !strncmp(s, "ipc", len) )
> +        vpmu_features |= XENPMU_FEATURE_IPC_ONLY;
> +    else if ( !strncmp(s, "arch", len) )
> +        vpmu_features |= XENPMU_FEATURE_ARCH_ONLY;
> +    else if ( *s )
> +    {
> +        return 1;
> +    }
> +    return 0;
> +}
> +
> +static void __init parse_vpmu_params(char *s)
> +{
> +    bool_t badflag = 0;
> +    char *sep, *p = s;
> +
>      switch ( parse_bool(s) )
>      {
>      case 0:
>          break;
>      default:
> -        if ( !strcmp(s, "bts") )
> -            vpmu_features |= XENPMU_FEATURE_INTEL_BTS;
> -        else if ( *s )
> +        sep = strchr(p, ',');
> +        while (sep != NULL)
> +        {
> +            if ( parse_vpmu_param(p, sep - p) )
> +                badflag = 1;
> +            p = sep + 1;
> +            sep = strchr(p, ',');
> +        }
> +        sep = strchr(p, 0);
> +        parse_vpmu_param(p, sep - p);
> +        if ( badflag )
>          {
> -            printk("VPMU: unknown flag: %s - vpmu disabled!\n", s);
> +            printk("VPMU: unknown flags: %s - vpmu disabled!\n", s);
>              break;
>          }
>          /* fall through */
> diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c
> index 8d83a1a..b26a20b 100644
> --- a/xen/arch/x86/cpu/vpmu_intel.c
> +++ b/xen/arch/x86/cpu/vpmu_intel.c
> @@ -602,12 +602,19 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content,
>                   "MSR_PERF_GLOBAL_STATUS(0x38E)!\n");
>          return -EINVAL;
>      case MSR_IA32_PEBS_ENABLE:
> +        if ( vpmu_features & XENPMU_FEATURE_IPC_ONLY ||
> +             vpmu_features & XENPMU_FEATURE_ARCH_ONLY )
> +            return -EINVAL;
>          if ( msr_content & 1 )
>              gdprintk(XENLOG_WARNING, "Guest is trying to enable PEBS, "
>                       "which is not supported.\n");
>          core2_vpmu_cxt->pebs_enable = msr_content;
>          return 0;
>      case MSR_IA32_DS_AREA:
> +        if ( (vpmu_features & XENPMU_FEATURE_IPC_ONLY ||
> +             vpmu_features & XENPMU_FEATURE_ARCH_ONLY) &&
> +             !(vpmu_features & XENPMU_FEATURE_INTEL_BTS) )
> +            return -EINVAL;
>          if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_DS) )
>          {
>              if ( !is_canonical_address(msr_content) )
> @@ -652,12 +659,53 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content,
>          tmp = msr - MSR_P6_EVNTSEL(0);
>          if ( tmp >= 0 && tmp < arch_pmc_cnt )
>          {
> +            bool_t blocked = 0;
> +            uint64_t umaskevent;
>              struct xen_pmu_cntr_pair *xen_pmu_cntr_pair =
>                  vpmu_reg_pointer(core2_vpmu_cxt, arch_counters);
>  
>              if ( msr_content & ARCH_CTRL_MASK )
>                  return -EINVAL;
>  
> +            /* PMC filters */
> +            umaskevent = msr_content & MSR_IA32_CMT_EVTSEL_UE_MASK;
> +            if ( vpmu_features & XENPMU_FEATURE_IPC_ONLY ||
> +                 vpmu_features & XENPMU_FEATURE_ARCH_ONLY )
> +            {
> +                blocked = 1;
> +                switch ( umaskevent )
> +                {
> +                /*
> +                 * See Table 18-1 from the Intel 64 and IA-32 Architectures Software
> +                 * Developer's Manual, Volume 3B, System Programming Guide, Part 2.
> +                 */
> +                case 0x003c:	/* unhalted core cycles */
> +                case 0x013c:	/* unhalted ref cycles */
> +                case 0x00c0:	/* instruction retired */
> +                    blocked = 0;
> +                default:
> +                    break;
> +                }
> +            }
> +
> +            if ( vpmu_features & XENPMU_FEATURE_ARCH_ONLY )
> +            {
> +                /* additional counters beyond IPC only; blocked already set */
> +                switch ( umaskevent )
> +                {
> +                case 0x4f2e:	/* LLC reference */
> +                case 0x412e:	/* LLC misses */
> +                case 0x00c4:	/* branch instruction retired */
> +                case 0x00c5:	/* branch */
> +                    blocked = 0;
> +                default:
> +                    break;
> +               }
> +            }
> +
> +            if ( blocked )
> +                return -EINVAL;
> +
>              if ( has_hvm_container_vcpu(v) )
>                  vmx_read_guest_msr(MSR_CORE_PERF_GLOBAL_CTRL,
>                                     &core2_vpmu_cxt->global_ctrl);
> diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
> index b8ad93c..0542064 100644
> --- a/xen/include/asm-x86/msr-index.h
> +++ b/xen/include/asm-x86/msr-index.h
> @@ -328,6 +328,7 @@
>  
>  /* Platform Shared Resource MSRs */
>  #define MSR_IA32_CMT_EVTSEL		0x00000c8d
> +#define MSR_IA32_CMT_EVTSEL_UE_MASK	0x0000ffff
>  #define MSR_IA32_CMT_CTR		0x00000c8e
>  #define MSR_IA32_PSR_ASSOC		0x00000c8f
>  #define MSR_IA32_PSR_L3_QOS_CFG	0x00000c81
> diff --git a/xen/include/public/pmu.h b/xen/include/public/pmu.h
> index 7753df0..c7b5648 100644
> --- a/xen/include/public/pmu.h
> +++ b/xen/include/public/pmu.h
> @@ -84,9 +84,19 @@ DEFINE_XEN_GUEST_HANDLE(xen_pmu_params_t);
>  
>  /*
>   * PMU features:
> - * - XENPMU_FEATURE_INTEL_BTS: Intel BTS support (ignored on AMD)
> + * - XENPMU_FEATURE_INTEL_BTS:  Intel BTS support (ignored on AMD)
> + * - XENPMU_FEATURE_IPC_ONLY:   Restrict PMC to the most minimum set possible.
> + *                              Instructions, cycles, and ref cycles. Can be
> + *                              used to calculate instructions-per-cycle (IPC)
> + *                              (ignored on AMD).
> + * - XENPMU_FEATURE_ARCH_ONLY:  Restrict PMCs to the Intel pre-defined
> + *                              architecteral events exposed by cpuid and
> + *                              listed in Table 18-1 of the developer's manual
> + *                              (ignored on AMD).
>   */
> -#define XENPMU_FEATURE_INTEL_BTS  1
> +#define XENPMU_FEATURE_INTEL_BTS  (1<<0)
> +#define XENPMU_FEATURE_IPC_ONLY   (1<<1)
> +#define XENPMU_FEATURE_ARCH_ONLY  (1<<2)
>  
>  /*
>   * Shared PMU data between hypervisor and PV(H) domains.
> 

-- 
Company details: http://ts.fujitsu.com/imprint.html

Re: [PATCH v2 2/2] x86/VPMU: implement ipc and arch filter flags

[Brendan Gregg]

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

On Thu, Nov 26, 2015 at 12:11 AM, Dietmar Hahn <dietmar.hahn@ts.fujitsu.com>
wrote:

> Am Dienstag 24 November 2015, 15:53:12 schrieb Brendan Gregg:
> > This introduces a way to have a restricted VPMU, by specifying one of two
> > predefined groups of PMCs to make available. For secure environments,
> this
> > allows the VPMU to be used without needing to enable all PMCs.
> >
> > Signed-off-by: Brendan Gregg <bgregg@netflix.com>
> > ---
> >  docs/misc/xen-command-line.markdown | 14 +++++++++-
> >  xen/arch/x86/cpu/vpmu.c             | 51
> +++++++++++++++++++++++++++++--------
> >  xen/arch/x86/cpu/vpmu_intel.c       | 48
> ++++++++++++++++++++++++++++++++++
> >  xen/include/asm-x86/msr-index.h     |  1 +
> >  xen/include/public/pmu.h            | 14 ++++++++--
> >  5 files changed, 115 insertions(+), 13 deletions(-)
> >
> > diff --git a/docs/misc/xen-command-line.markdown
> b/docs/misc/xen-command-line.markdown
> > index 70daa84..6055a68 100644
> > --- a/docs/misc/xen-command-line.markdown
> > +++ b/docs/misc/xen-command-line.markdown
> > @@ -1452,7 +1452,7 @@ Use Virtual Processor ID support if available.
> This prevents the need for TLB
> >  flushes on VM entry and exit, increasing performance.
> >
> >  ### vpmu
> > -> `= ( bts )`
> > +> `= ( <boolean> | { bts | ipc | arch [, ...] } )`
> >
> >  > Default: `off`
> >
> > @@ -1468,6 +1468,18 @@ wrong behaviour (see handle\_pmc\_quirk()).
> >  If 'vpmu=bts' is specified the virtualisation of the Branch Trace Store
> (BTS)
> >  feature is switched on on Intel processors supporting this feature.
> >
> > +vpmu=ipc enables performance monitoring, but restricts the counters to
> the
> > +most minimum set possible: instructions, cycles, and reference cycles.
> These
> > +can be used to calculate instructions per cycle (IPC).
> > +
> > +vpmu=arch enables performance monitoring, but restricts the counters to
> the
> > +pre-defined architectural events only. These are exposed by cpuid, and
> listed
> > +in Table 18-1 from the Intel 64 and IA-32 Architectures Software
> Developer's
> > +Manual, Volume 3B, System Programming Guide, Part 2.
>
> Maybe you better should write
> section "Pre-defined Architectural Performance Events"
> instead of "Table 18-1" because the number may change in the document.
> And below in the comment of the code too.
>

Ah, thanks, yes, otherwise it'd be pretty confusing if it changed and I was
pointing people to the wrong table.

Brendan

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

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

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