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

[PATCH v5] 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>
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
Changes in v5:
* addressing review comments from Kevin:
* simplified BTS DS_AREA filter
* improvements to comments, incl. matching Intel SDM descriptions

Changes in v4:
* changed table reference in comments, suggested by Dietmar

Changes in v3:
* addressing review comments from Boris:
* ensure final flag is validated
* code tidy

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
---
 docs/misc/xen-command-line.markdown | 15 ++++++++++-
 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(+), 14 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 467dc8f..5ed0730 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1476,7 +1476,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`
 
@@ -1492,6 +1492,19 @@ 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 the Pre-Defined Architectural Performance Events table 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 4856e98..004f71a 100644
--- a/xen/arch/x86/cpu/vpmu.c
+++ b/xen/arch/x86/cpu/vpmu.c
@@ -43,34 +43,59 @@ 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
+        return 1;
+    return 0;
+}
+
+static void __init parse_vpmu_params(char *s)
+{
+    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 )
+        while (1)
         {
-            printk("VPMU: unknown flag: %s - vpmu disabled!\n", s);
-            break;
+            sep = strchr(p, ',');
+            if ( sep == NULL )
+                sep = strchr(p, 0);
+            if ( parse_vpmu_param(p, sep - p) )
+                goto error;
+            if ( *sep == 0 )
+                /* reached end of flags */
+                break;
+            p = sep + 1;
         }
         /* fall through */
     case 1:
@@ -79,6 +104,10 @@ static void __init parse_vpmu_param(char *s)
         opt_vpmu_enabled = 1;
         break;
     }
+    return;
+
+ error:
+    printk("VPMU: unknown flags: %s - vpmu disabled!\n", s);
 }
 
 void vpmu_lvtpc_update(uint32_t val)
diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c
index 3eff1ae..4d1bae0 100644
--- a/xen/arch/x86/cpu/vpmu_intel.c
+++ b/xen/arch/x86/cpu/vpmu_intel.c
@@ -604,12 +604,17 @@ 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 |
+             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_INTEL_BTS) )
+            return -EINVAL;
         if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_DS) )
         {
             if ( !(has_hvm_container_vcpu(v)
@@ -656,12 +661,55 @@ 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 the Pre-Defined Architectural Performance Events table
+                 * 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 Reference Cycles */
+                case 0x00c0:	/* Instructions Retired */
+                    blocked = 0;
+                default:
+                    break;
+                }
+            }
+
+            if ( vpmu_features & XENPMU_FEATURE_ARCH_ONLY )
+            {
+                /* additional counters beyond IPC only; blocked already set */
+                switch ( umaskevent )
+                {
+                case 0x4f2e:	/* Last Level Cache References */
+                case 0x412e:	/* Last Level Cache Misses */
+                case 0x00c4:	/* Branch Instructions Retired */
+                case 0x00c5:	/* All Branch Mispredict Retired */
+                    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 2b97861..84da631 100644
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -329,6 +329,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..0e1312c 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 PMCs 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
+ *                              Architectural Performance Events exposed by
+ *                              cpuid and listed in the Intel 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 v5] x86/VPMU: implement ipc and arch filter flags

[Tian, Kevin]

> From: Brendan Gregg [mailto:bgregg@netflix.com]
> Sent: Tuesday, January 05, 2016 9:44 AM
> 
> 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>
> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

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

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

[Jan Beulich]

>>> On 05.01.16 at 02:43, <bgregg@netflix.com> 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>
> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Does this hold with ...

> ---
> Changes in v5:
> * addressing review comments from Kevin:
> * simplified BTS DS_AREA filter
> * improvements to comments, incl. matching Intel SDM descriptions

... all of these?

Jan

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

[Boris Ostrovsky]

On 01/06/2016 06:43 AM, Jan Beulich wrote:
>>>> On 05.01.16 at 02:43, <bgregg@netflix.com> 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>
>> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Does this hold with ...
>
>> ---
>> Changes in v5:
>> * addressing review comments from Kevin:
>> * simplified BTS DS_AREA filter
>> * improvements to comments, incl. matching Intel SDM descriptions
> ... all of these?
>

Yes.

Note that this patch will conflict with 
http://lists.xenproject.org/archives/html/xen-devel/2015-12/msg02753.html (for 
'case MSR_IA32_PEBS_ENABLE' in core2_vpmu_do_wrmsr()). I can rebase it 
on top of Brendan's if you prefer.


-boris

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

[Jan Beulich]

>>> On 05.01.16 at 02:43, <bgregg@netflix.com> 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>
> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

I was about to apply with

>  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)

static bool_t __init parse_vpmu_param(char *s, unsigned int len)

>  {
> +    if ( ! *s || ! 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
> +        return 1;
> +    return 0;
> +}
> +
> +static void __init parse_vpmu_params(char *s)
> +{
> +    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 )
> +        while (1)
>          {
> -            printk("VPMU: unknown flag: %s - vpmu disabled!\n", s);
> -            break;
> +            sep = strchr(p, ',');
> +            if ( sep == NULL )
> +                sep = strchr(p, 0);
> +            if ( parse_vpmu_param(p, sep - p) )
> +                goto error;
> +            if ( *sep == 0 )

            if ( !*sep )

> --- a/xen/arch/x86/cpu/vpmu_intel.c
> +++ b/xen/arch/x86/cpu/vpmu_intel.c
> @@ -604,12 +604,17 @@ 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 |
> +             XENPMU_FEATURE_ARCH_ONLY) )

                              XENPMU_FEATURE_ARCH_ONLY) )

> @@ -656,12 +661,55 @@ 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 )

            if ( vpmu_features & (XENPMU_FEATURE_IPC_ONLY |
                                  XENPMU_FEATURE_ARCH_ONLY) )

> +            {
> +                blocked = 1;
> +                switch ( umaskevent )
> +                {
> +                /*
> +                 * See the Pre-Defined Architectural Performance Events table
> +                 * 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 Reference Cycles */
> +                case 0x00c0:	/* Instructions Retired */
> +                    blocked = 0;
> +                default:
> +                    break;

dropped last two lines

> +                }
> +            }
> +
> +            if ( vpmu_features & XENPMU_FEATURE_ARCH_ONLY )
> +            {
> +                /* additional counters beyond IPC only; blocked already set */

                /* Additional counters beyond IPC only; blocked already set. */

> +                switch ( umaskevent )
> +                {
> +                case 0x4f2e:	/* Last Level Cache References */
> +                case 0x412e:	/* Last Level Cache Misses */
> +                case 0x00c4:	/* Branch Instructions Retired */
> +                case 0x00c5:	/* All Branch Mispredict Retired */
> +                    blocked = 0;
> +                default:
> +                    break;

Again

> --- 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 PMCs 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
> + *                              Architectural Performance Events exposed by
> + *                              cpuid and listed in the Intel 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)

when I reached this: Do these additions really need to go here?
And if they do, why does do_xenpmu_op()'s XENPMU_feature_set
case not get altered?

Jan

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

[Brendan Gregg]

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

On Thu, Jan 7, 2016 at 6:12 AM, Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 05.01.16 at 02:43, <bgregg@netflix.com> 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>
> > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>
> I was about to apply with
>
> >  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)
>
> static bool_t __init parse_vpmu_param(char *s, unsigned int len)
>

Ok.


>
> >  {
> > +    if ( ! *s || ! len )
>
>     if ( !*s || !len )
>
>
Ok.


> > +        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
> > +        return 1;
> > +    return 0;
> > +}
> > +
> > +static void __init parse_vpmu_params(char *s)
> > +{
> > +    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 )
> > +        while (1)
> >          {
> > -            printk("VPMU: unknown flag: %s - vpmu disabled!\n", s);
> > -            break;
> > +            sep = strchr(p, ',');
> > +            if ( sep == NULL )
> > +                sep = strchr(p, 0);
> > +            if ( parse_vpmu_param(p, sep - p) )
> > +                goto error;
> > +            if ( *sep == 0 )
>
>             if ( !*sep )
>
>
Ok.


> > --- a/xen/arch/x86/cpu/vpmu_intel.c
> > +++ b/xen/arch/x86/cpu/vpmu_intel.c
> > @@ -604,12 +604,17 @@ 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 |
> > +             XENPMU_FEATURE_ARCH_ONLY) )
>
>                               XENPMU_FEATURE_ARCH_ONLY) )
>
>
Ok, yes, neater.


> > @@ -656,12 +661,55 @@ 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 )
>
>             if ( vpmu_features & (XENPMU_FEATURE_IPC_ONLY |
>                                   XENPMU_FEATURE_ARCH_ONLY) )
>
>
Ok.


> > +            {
> > +                blocked = 1;
> > +                switch ( umaskevent )
> > +                {
> > +                /*
> > +                 * See the Pre-Defined Architectural Performance Events
> table
> > +                 * 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 Reference Cycles */
> > +                case 0x00c0: /* Instructions Retired */
> > +                    blocked = 0;
> > +                default:
> > +                    break;
>
> dropped last two lines
>
>
Ok.


> > +                }
> > +            }
> > +
> > +            if ( vpmu_features & XENPMU_FEATURE_ARCH_ONLY )
> > +            {
> > +                /* additional counters beyond IPC only; blocked already
> set */
>
>                 /* Additional counters beyond IPC only; blocked already
> set. */
>
> > +                switch ( umaskevent )
> > +                {
> > +                case 0x4f2e: /* Last Level Cache References */
> > +                case 0x412e: /* Last Level Cache Misses */
> > +                case 0x00c4: /* Branch Instructions Retired */
> > +                case 0x00c5: /* All Branch Mispredict Retired */
> > +                    blocked = 0;
> > +                default:
> > +                    break;
>
> Again
>

Ok.


>
> > --- 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 PMCs 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
> > + *                              Architectural Performance Events
> exposed by
> > + *                              cpuid and listed in the Intel
> 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)
>
> when I reached this: Do these additions really need to go here?
> And if they do, why does do_xenpmu_op()'s XENPMU_feature_set
> case not get altered?
>

I'd say they do, as it's pmu.h, and it places them with the BTS feature,
other PMU modes, etc. Unless I'm missing some consideration.

Yes, I missed the new sysfs PMU interface, so do_xenpmu_op() should do:

    case XENPMU_feature_set:
        if ( pmu_params.val & ~(XENPMU_FEATURE_INTEL_BTS |
                                XENPMU_FEATURE_IPC_ONLY |
                                XENPMU_FEATURE_ARCH_ONLY))
            return -EINVAL;

I can send along a v6 patch with the other changes too if that's desirable.
Thanks, and sorry for the nuisance.

Brendan

-- 
Brendan Gregg, Senior Performance Architect, Netflix

[-- Attachment #1.2: Type: text/html, Size: 11053 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 v5] x86/VPMU: implement ipc and arch filter flags

[Jan Beulich]

>>> On 07.01.16 at 22:40, <bgregg@netflix.com> wrote:
> On Thu, Jan 7, 2016 at 6:12 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 05.01.16 at 02:43, <bgregg@netflix.com> wrote:
>> > -#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)
>>
>> when I reached this: Do these additions really need to go here?
>> And if they do, why does do_xenpmu_op()'s XENPMU_feature_set
>> case not get altered?
>>
> 
> I'd say they do, as it's pmu.h, and it places them with the BTS feature,
> other PMU modes, etc. Unless I'm missing some consideration.
> 
> Yes, I missed the new sysfs PMU interface, so do_xenpmu_op() should do:
> 
>     case XENPMU_feature_set:
>         if ( pmu_params.val & ~(XENPMU_FEATURE_INTEL_BTS |
>                                 XENPMU_FEATURE_IPC_ONLY |
>                                 XENPMU_FEATURE_ARCH_ONLY))
>             return -EINVAL;
> 
> I can send along a v6 patch with the other changes too if that's desirable.

That's not just desirable, but what I would expect to happen. I'm
certainly not going to have a second go at fixing up the patch for
you.

Jan