* [PATCH v4 2/2] x86/VPMU: implement ipc and arch filter flags
@ 2015-12-01 0:39 Brendan Gregg
2015-12-07 19:13 ` Boris Ostrovsky
0 siblings, 1 reply; 5+ messages in thread
From: Brendan Gregg @ 2015-12-01 0:39 UTC (permalink / raw)
To: xen-devel; +Cc: boris.ostrovsky, Brendan Gregg, dietmar.hahn
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>
---
docs/misc/xen-command-line.markdown | 15 ++++++++++-
xen/arch/x86/cpu/vpmu.c | 51 +++++++++++++++++++++++++++++--------
xen/arch/x86/cpu/vpmu_intel.c | 50 ++++++++++++++++++++++++++++++++++++
xen/include/asm-x86/msr-index.h | 1 +
xen/include/public/pmu.h | 14 ++++++++--
5 files changed, 117 insertions(+), 14 deletions(-)
diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 70daa84..795661e 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,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 2f5156a..46b5324 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 8d83a1a..a6c5545 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 |
+ 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 |
+ 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,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 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..f9ad7b4 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 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v4 2/2] x86/VPMU: implement ipc and arch filter flags
2015-12-01 0:39 [PATCH v4 2/2] x86/VPMU: implement ipc and arch filter flags Brendan Gregg
@ 2015-12-07 19:13 ` Boris Ostrovsky
2015-12-11 7:54 ` Tian, Kevin
2015-12-18 6:12 ` Tian, Kevin
0 siblings, 2 replies; 5+ messages in thread
From: Boris Ostrovsky @ 2015-12-07 19:13 UTC (permalink / raw)
To: Brendan Gregg, xen-devel
Cc: Andrew Cooper, Tian, Kevin, Jan Beulich, Jun Nakajima, dietmar.hahn
On 11/30/2015 07:39 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>
> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
This needs to be reviewed also by Intel maintainers (copied). Plus x86
maintainers.
-boris
> ---
> docs/misc/xen-command-line.markdown | 15 ++++++++++-
> xen/arch/x86/cpu/vpmu.c | 51 +++++++++++++++++++++++++++++--------
> xen/arch/x86/cpu/vpmu_intel.c | 50 ++++++++++++++++++++++++++++++++++++
> xen/include/asm-x86/msr-index.h | 1 +
> xen/include/public/pmu.h | 14 ++++++++--
> 5 files changed, 117 insertions(+), 14 deletions(-)
>
> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
> index 70daa84..795661e 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,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 2f5156a..46b5324 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 8d83a1a..a6c5545 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 |
> + 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 |
> + 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,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 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..f9ad7b4 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 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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4 2/2] x86/VPMU: implement ipc and arch filter flags
2015-12-07 19:13 ` Boris Ostrovsky
@ 2015-12-11 7:54 ` Tian, Kevin
2015-12-18 6:12 ` Tian, Kevin
1 sibling, 0 replies; 5+ messages in thread
From: Tian, Kevin @ 2015-12-11 7:54 UTC (permalink / raw)
To: Boris Ostrovsky, Brendan Gregg, xen-devel
Cc: Andrew Cooper, Jan Beulich, Nakajima, Jun, dietmar.hahn
> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> Sent: Tuesday, December 08, 2015 3:14 AM
>
> On 11/30/2015 07:39 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>
> > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>
> This needs to be reviewed also by Intel maintainers (copied). Plus x86
> maintainers.
>
> -boris
>
In my review queue... since it introduces new logic I'll find a better time
to look at it carefully next week.
Thanks
Kevin
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4 2/2] x86/VPMU: implement ipc and arch filter flags
2015-12-07 19:13 ` Boris Ostrovsky
2015-12-11 7:54 ` Tian, Kevin
@ 2015-12-18 6:12 ` Tian, Kevin
2016-01-05 1:37 ` Brendan Gregg
1 sibling, 1 reply; 5+ messages in thread
From: Tian, Kevin @ 2015-12-18 6:12 UTC (permalink / raw)
To: 'Boris Ostrovsky', Brendan Gregg, xen-devel
Cc: Andrew Cooper, Jan Beulich, Nakajima, Jun, dietmar.hahn
> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> Sent: Tuesday, December 08, 2015 3:14 AM
>
> On 11/30/2015 07:39 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>
> > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>
> This needs to be reviewed also by Intel maintainers (copied). Plus x86
> maintainers.
>
> -boris
>
[...]
> > diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c
> > index 8d83a1a..a6c5545 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 |
> > + 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 |
> > + XENPMU_FEATURE_ARCH_ONLY)) &&
> > + !(vpmu_features & XENPMU_FEATURE_INTEL_BTS) )
> > + return -EINVAL;
should the check be made just based on BTS?
> > if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_DS) )
> > {
> > if ( !is_canonical_address(msr_content) )
> > @@ -652,12 +659,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 */
Better to copy same wording from SDM, e.g. "UnHalted Core Cycles */. same for below.
> > + 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..f9ad7b4 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.
PMC -> PMCs
> > + * 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 Performance Events exposed by
Architecteral -> Architectural
> > + * 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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4 2/2] x86/VPMU: implement ipc and arch filter flags
2015-12-18 6:12 ` Tian, Kevin
@ 2016-01-05 1:37 ` Brendan Gregg
0 siblings, 0 replies; 5+ messages in thread
From: Brendan Gregg @ 2016-01-05 1:37 UTC (permalink / raw)
To: Tian, Kevin
Cc: Nakajima, Jun, Andrew Cooper, dietmar.hahn, xen-devel,
Jan Beulich, Boris Ostrovsky
[-- Attachment #1.1: Type: text/plain, Size: 6960 bytes --]
Sorry for the delay...
On Thu, Dec 17, 2015 at 10:12 PM, Tian, Kevin <kevin.tian@intel.com> wrote:
> > From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> > Sent: Tuesday, December 08, 2015 3:14 AM
> >
> > On 11/30/2015 07:39 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>
> > > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> >
> > This needs to be reviewed also by Intel maintainers (copied). Plus x86
> > maintainers.
> >
> > -boris
> >
>
> [...]
>
> > > diff --git a/xen/arch/x86/cpu/vpmu_intel.c
> b/xen/arch/x86/cpu/vpmu_intel.c
> > > index 8d83a1a..a6c5545 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 |
> > > + 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 |
> > > + XENPMU_FEATURE_ARCH_ONLY)) &&
> > > + !(vpmu_features & XENPMU_FEATURE_INTEL_BTS) )
> > > + return -EINVAL;
>
> should the check be made just based on BTS?
>
Ah, yes. The BTS check was added after the new modes, but it should be
standalone. I don't think anything else uses DS_AREA other than BTS.
> > > if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_DS) )
> > > {
> > > if ( !is_canonical_address(msr_content) )
> > > @@ -652,12 +659,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 */
>
> Better to copy same wording from SDM, e.g. "UnHalted Core Cycles */. same
> for below.
>
Ok, yes.
>
> > > + 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..f9ad7b4 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.
>
> PMC -> PMCs
>
Ok.
>
> > > + * 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 Performance Events
> exposed by
>
> Architecteral -> Architectural
>
Ok.
>
> > > + * 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.
>
>
Thanks for checking! New patch (v5) coming...
Brendan
--
Brendan Gregg, Senior Performance Architect, Netflix
[-- Attachment #1.2: Type: text/html, Size: 10410 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-01-05 1:37 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-01 0:39 [PATCH v4 2/2] x86/VPMU: implement ipc and arch filter flags Brendan Gregg
2015-12-07 19:13 ` Boris Ostrovsky
2015-12-11 7:54 ` Tian, Kevin
2015-12-18 6:12 ` Tian, Kevin
2016-01-05 1:37 ` Brendan Gregg
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.