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

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

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.