From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v2 2/2] x86/VPMU: implement ipc and arch filter flags Date: Wed, 25 Nov 2015 10:13:04 -0500 Message-ID: <5655D000.6070103@oracle.com> References: <1448409192-4914-1-git-send-email-bgregg@netflix.com> <1448409192-4914-3-git-send-email-bgregg@netflix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1448409192-4914-3-git-send-email-bgregg@netflix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Brendan Gregg , xen-devel@lists.xen.org Cc: dietmar.hahn@ts.fujitsu.com List-Id: xen-devel@lists.xenproject.org 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 > --- > 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 )` > +> `= ( | { 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