From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH 3/3 v2] vpmu intel: Dump vpmu infos in 'q' keyhandler Date: Fri, 5 Apr 2013 09:33:38 -0400 Message-ID: <20130405133338.GC20093@phenom.dumpdata.com> References: <8751926.IvtI3fQmuJ@amur> <1614708.69dJLtaETJ@amur> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Content-Disposition: inline In-Reply-To: <1614708.69dJLtaETJ@amur> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Dietmar Hahn Cc: Eddie Dong , Jun Nakajima , suravee.suthikulpanit@amd.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On Thu, Apr 04, 2013 at 02:10:32PM +0200, Dietmar Hahn wrote: > Am Donnerstag 28 M=E4rz 2013, 14:01:10 schrieb Dietmar Hahn: > > This patch works only on top of 2/3. > > = > > This patch extends the printout of the VPCU infos of the keyhandler 'q'. > > If vPMU is enabled is on the VCPU and active lines are printed like > > (when running HVM openSuSE-12.3 with 'perf top'); > > = > > (XEN) vPMU running > > (XEN) general_0: 0x000000ffffff3ae1 ctrl: 0x000000000053003c > > (XEN) fixed_1: 0x000000ff90799188 ctrl: 0xb > > = > > This means general counter 0 and fixed counter 1 are running with showi= ng > > their contents and the contents of their configuration msr. > > Thanks. > > Dietmar. > > = > > Signed-off-by: Dietmar Hahn > = > Ping? > = > > Changes from v1: As Konrad suggested the names of the defines are chang= ed too > > and the readability is much better now. > > = > > = > > = > > --- a/xen/arch/x86/domain.c > > +++ b/xen/arch/x86/domain.c > > @@ -2093,6 +2093,9 @@ void arch_dump_domain_info(struct domain > > void arch_dump_vcpu_info(struct vcpu *v) > > { > > paging_dump_vcpu_info(v); > > + > > + if ( is_hvm_vcpu(v) ) > > + vpmu_dump(v); > > } > > = > > void domain_cpuid( > > --- a/xen/arch/x86/hvm/vmx/vpmu_core2.c > > +++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c > > @@ -132,6 +132,16 @@ static const u32 core2_fix_counters_msr[ > > MSR_CORE_PERF_FIXED_CTR2 > > }; > > = > > +/* > > + * MSR_CORE_PERF_FIXED_CTR_CTRL contains the configuration of all fixed > > + * counters. 4 bits for every counter. > > + */ > > +#define FIXED_CTR_CTRL_BITS 4 That is a lot of 'CTR' in it. Can it be just one CTRL ? Say FIXED_CTRL_BITS? One of them is for 'Control', what is the other CTR fo= r? > > +#define FIXED_CTR_CTRL_MASK ((1 << FIXED_CTR_CTRL_BITS) - 1) Ditto. > > + > > +/* The index into the core2_ctrls_msr[] of this MSR used in core2_vpmu= _dump() */ > > +#define MSR_CORE_PERF_FIXED_CTR_CTRL_IDX 0 Here as well. > > + > > /* Core 2 Non-architectual Performance Control MSRs. */ > > static const u32 core2_ctrls_msr[] =3D { > > MSR_CORE_PERF_FIXED_CTR_CTRL, > > @@ -507,7 +517,7 @@ static int core2_vpmu_do_wrmsr(unsigned > > { > > core2_vpmu_cxt->pmu_enable->fixed_ctr_enable[i] =3D > > (global_ctrl & 1) & ((non_global_ctrl & 0x3)? 1: 0); > > - non_global_ctrl >>=3D 4; > > + non_global_ctrl >>=3D FIXED_CTR_CTRL_BITS; > > global_ctrl >>=3D 1; > > } > > break; > > @@ -564,7 +574,7 @@ static int core2_vpmu_do_wrmsr(unsigned > > if ( msr =3D=3D MSR_IA32_DS_AREA ) > > break; > > /* 4 bits per counter, currently 3 fixed counters implemen= ted. */ > > - mask =3D ~((1ull << (3 * 4)) - 1); > > + mask =3D ~((1ull << (VPMU_CORE2_NUM_FIXED * FIXED_CTR_CTRL= _BITS)) - 1); > > if (msr_content & mask) > > inject_gp =3D 1; > > break; > > @@ -644,6 +654,52 @@ static void core2_vpmu_do_cpuid(unsigned > > } > > } > > = > > +/* Dump vpmu info on console, called in the context of keyhandler 'q'.= */ > > +static void core2_vpmu_dump(struct vcpu *v) > > +{ > > + struct vpmu_struct *vpmu =3D vcpu_vpmu(v); > > + int i, num; > > + struct core2_vpmu_context *core2_vpmu_cxt =3D NULL; > > + u64 val; > > + > > + if ( !vpmu_is_set(vpmu, VPMU_CONTEXT_ALLOCATED) ) > > + return; > > + > > + if ( !vpmu_is_set(vpmu, VPMU_RUNNING) ) > > + { > > + if ( vpmu_set(vpmu, VPMU_CONTEXT_LOADED) ) > > + printk(" vPMU loaded\n"); > > + else > > + printk(" vPMU allocated\n"); > > + return; > > + } > > + > > + printk(" vPMU running\n"); > > + core2_vpmu_cxt =3D vpmu->context; > > + num =3D core2_get_pmc_count(); > > + /* Print the contents of the counter and its configuration msr. */ > > + for ( i =3D 0; i < num; i++ ) > > + { > > + struct arch_msr_pair* msr_pair =3D core2_vpmu_cxt->arch_msr_pa= ir; > > + if ( core2_vpmu_cxt->pmu_enable->arch_pmc_enable[i] ) > > + printk(" general_%d: 0x%016lx ctrl: 0x%016lx\n", > > + i, msr_pair[i].counter, msr_pair[i].contr= ol); > > + } > > + /* > > + * The configuration of the fixed counter is 4 bits each in the > > + * MSR_CORE_PERF_FIXED_CTR_CTRL. > > + */ > > + val =3D core2_vpmu_cxt->ctrls[MSR_CORE_PERF_FIXED_CTR_CTRL_IDX]; > > + for ( i =3D 0; i < core2_fix_counters.num; i++ ) > > + { > > + if ( core2_vpmu_cxt->pmu_enable->fixed_ctr_enable[i] ) > > + printk(" fixed_%d: 0x%016lx ctrl: 0x%lx\n", > > + i, core2_vpmu_cxt->fix_counters[i], > > + val & FIXED_CTR_CTRL_MASK); > > + val >>=3D FIXED_CTR_CTRL_BITS; This is a bit odd? Was it meant to be on the same line as 'printk'? > > + } > > +} > > + > > static int core2_vpmu_do_interrupt(struct cpu_user_regs *regs) > > { > > struct vcpu *v =3D current; > > @@ -757,7 +813,8 @@ struct arch_vpmu_ops core2_vpmu_ops =3D { > > .do_cpuid =3D core2_vpmu_do_cpuid, > > .arch_vpmu_destroy =3D core2_vpmu_destroy, > > .arch_vpmu_save =3D core2_vpmu_save, > > - .arch_vpmu_load =3D core2_vpmu_load > > + .arch_vpmu_load =3D core2_vpmu_load, > > + .arch_vpmu_dump =3D core2_vpmu_dump > > }; > > = > > static void core2_no_vpmu_do_cpuid(unsigned int input, > > --- a/xen/arch/x86/hvm/vpmu.c > > +++ b/xen/arch/x86/hvm/vpmu.c > > @@ -154,3 +154,12 @@ void vpmu_destroy(struct vcpu *v) > > vpmu->arch_vpmu_ops->arch_vpmu_destroy(v); > > } > > = > > +/* Dump some vpmu informations on console. Used in keyhandler dump_dom= ains(). */ > > +void vpmu_dump(struct vcpu *v) > > +{ > > + struct vpmu_struct *vpmu =3D vcpu_vpmu(v); > > + > > + if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->arch_vpmu_dump ) > > + vpmu->arch_vpmu_ops->arch_vpmu_dump(v); > > +} > > + > > --- a/xen/include/asm-x86/hvm/vpmu.h > > +++ b/xen/include/asm-x86/hvm/vpmu.h > > @@ -54,6 +54,7 @@ struct arch_vpmu_ops { > > void (*arch_vpmu_destroy)(struct vcpu *v); > > void (*arch_vpmu_save)(struct vcpu *v); > > void (*arch_vpmu_load)(struct vcpu *v); > > + void (*arch_vpmu_dump)(struct vcpu *v); > > }; > > = > > int vmx_vpmu_initialise(struct vcpu *, unsigned int flags); > > @@ -87,6 +88,7 @@ void vpmu_initialise(struct vcpu *v); > > void vpmu_destroy(struct vcpu *v); > > void vpmu_save(struct vcpu *v); > > void vpmu_load(struct vcpu *v); > > +void vpmu_dump(struct vcpu *v); > > = > > extern int acquire_pmu_ownership(int pmu_ownership); > > extern void release_pmu_ownership(int pmu_ownership); > > = > > = Otherwise it looks OK to me. > -- = > Company details: http://ts.fujitsu.com/imprint.html