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

[Brendan Gregg <bgregg@xxxxxxxxxxx>]

Sorry for the delay...

On Thu, Dec 17, 2015 at 10:12 PM, Tian, Kevin <kevin.tian@xxxxxxxxx> wrote:

> From: Boris Ostrovsky [mailto:boris.ostrovsky@xxxxxxxxxx]
> Sent: Tuesday, December 08, 2015 3:14 AM
>
> 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@xxxxxxxxxxx>
> > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
>
> This needs to be reviewed also by Intel maintainers (copied). Plus x86
> maintainers.
>
> -boris
>

[...]

> > 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;

should the check be made just based on BTS?

Ah, yes. The BTS check was added after the new modes, but it should be standalone. I don't think anything else uses DS_AREA other than BTS.

> >Â Â Â Â Â Â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 */

Better to copy same wording from SDM, e.g. "UnHalted Core Cycles */. same for below.

Ok, yes.

Â

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

PMC -> PMCs

Ok.

Â

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

Architecteral -> Architectural

Ok.

Â

> > + *Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â 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.

Thanks for checking! New patch (v5) coming...

Brendan

--

Brendan Gregg, Senior Performance Architect, Netflix

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