Re: [Xen-devel] [PATCH v5] x86/VPMU: implement ipc and arch filter flags

[Brendan Gregg <bgregg@xxxxxxxxxxx>]

On Thu, Jan 7, 2016 at 6:12 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:

>>> On 05.01.16 at 02:43, <bgregg@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>

I was about to apply with

>Â 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)

static bool_t __init parse_vpmu_param(char *s, unsigned int len)

Ok.

Â

>Â {
> +Â Â if ( ! *s || ! len )

  if ( !*s || !len )

Ok.

Â

> +Â Â Â Â 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 )

      if ( !*sep )

Ok.

Â

> --- a/xen/arch/x86/cpu/vpmu_intel.c
> +++ b/xen/arch/x86/cpu/vpmu_intel.c
> @@ -604,12 +604,17 @@ 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) )

               XENPMU_FEATURE_ARCH_ONLY) )

Ok, yes, neater.

Â

> @@ -656,12 +661,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 )

      if ( vpmu_features & (XENPMU_FEATURE_IPC_ONLY |
                 XENPMU_FEATURE_ARCH_ONLY) )

Ok.

Â

> +Â Â Â Â Â Â {
> +Â Â Â Â Â Â Â Â 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 Reference Cycles */
> +Â Â Â Â Â Â Â Â case 0x00c0: /* Instructions Retired */
> +Â Â Â Â Â Â Â Â Â Â blocked = 0;
> +Â Â Â Â Â Â Â Â default:
> +Â Â Â Â Â Â Â Â Â Â break;

dropped last two lines

Ok.

Â

> +Â Â Â Â Â Â Â Â }
> +Â Â Â Â Â Â }
> +
> +Â Â Â Â Â Â if ( vpmu_features & XENPMU_FEATURE_ARCH_ONLY )
> +Â Â Â Â Â Â {
> +Â Â Â Â Â Â Â Â /* additional counters beyond IPC only; blocked already set */

        /* Additional counters beyond IPC only; blocked already set. */

> +Â Â Â Â Â Â Â Â switch ( umaskevent )
> +Â Â Â Â Â Â Â Â {
> +Â Â Â Â Â Â Â Â case 0x4f2e: /* Last Level Cache References */
> +Â Â Â Â Â Â Â Â case 0x412e: /* Last Level Cache Misses */
> +Â Â Â Â Â Â Â Â case 0x00c4: /* Branch Instructions Retired */
> +Â Â Â Â Â Â Â Â case 0x00c5: /* All Branch Mispredict Retired */
> +Â Â Â Â Â Â Â Â Â Â blocked = 0;
> +Â Â Â Â Â Â Â Â default:
> +Â Â Â Â Â Â Â Â Â Â break;

Again

Ok.

Â

> --- 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 PMCs 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
> + *Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Architectural 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)

when I reached this: Do these additions really need to go here?
And if they do, why does do_xenpmu_op()'s XENPMU_feature_set
case not get altered?

I'd say they do, as it's pmu.h, and it places them with the BTS feature, other PMU modes, etc. Unless I'm missing some consideration.

Yes, I missed the new sysfs PMU interface, so do_xenpmu_op() should do:

  case XENPMU_feature_set:

    if ( pmu_params.val & ~(XENPMU_FEATURE_INTEL_BTS |

                XENPMU_FEATURE_IPC_ONLY |

                XENPMU_FEATURE_ARCH_ONLY))

      return -EINVAL;

I can send along a v6 patch with the other changes too if that's desirable. Thanks, and sorry for the nuisance.

Brendan

--

Brendan Gregg, Senior Performance Architect, Netflix

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel