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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

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