On Wed, Nov 25, 2015 at 7:13 AM, Boris Ostrovsky <boris.ostrov...@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 <bgr...@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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
