From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rusty Russell Subject: Re: [PATCH 2/3] ARM: KVM: Fake up performance counters a little more precisely. Date: Thu, 17 May 2012 09:42:21 +0930 Message-ID: <87ipfv3btm.fsf@rustcorp.com.au> References: <20120312065134.8074.36949.stgit@ubuntu> <87y5qkxa88.fsf@rustcorp.com.au> <87r4wcx9yj.fsf@rustcorp.com.au> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: android-virt@lists.cs.columbia.edu, kvm@vger.kernel.org, patches@linaro.org To: Christoffer Dall , Rusty Russell Return-path: Received: from ozlabs.org ([203.10.76.45]:48354 "EHLO ozlabs.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1760952Ab2EQDhw convert rfc822-to-8bit (ORCPT ); Wed, 16 May 2012 23:37:52 -0400 In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: On Mon, 14 May 2012 18:49:40 -0400, Christoffer Dall wrote: > On Thu, Mar 29, 2012 at 1:17 AM, Rusty Russell wrote: > > Rather than just making all of c9 read-zero/write-discard, this cha= nges > > it to the explicit profiling registers we need. =C2=A0This is a sta= rt for the > > future implementation were we actually implement performance monito= ring, > > and makes sure we're not discarding important things. > > > > Signed-off-by: Rusty Russell > > --- > > =C2=A0arch/arm/kvm/emulate.c | =C2=A0 67 ++++++++++++++++++++++++++= +++++++++++++++++++++- > > =C2=A01 files changed, 65 insertions(+), 2 deletions(-) > > > > diff --git a/arch/arm/kvm/emulate.c b/arch/arm/kvm/emulate.c > > index aec1b6e..4bdab8f 100644 > > --- a/arch/arm/kvm/emulate.c > > +++ b/arch/arm/kvm/emulate.c > > @@ -238,6 +238,64 @@ static bool read_l2ctlr(struct kvm_vcpu *vcpu, > > =C2=A0 =C2=A0 =C2=A0 =C2=A0return true; > > =C2=A0} > > > > +static bool read_pmcr(struct kvm_vcpu *vcpu, > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 const struct coproc_params *p, > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 unsigned long arg) > > +{ > > + =C2=A0 =C2=A0 =C2=A0 u32 imp, idcode, num; > > + > > + =C2=A0 =C2=A0 =C2=A0 imp =3D (vcpu->arch.cp15[c0_MIDR] & 0xFF0000= 00) >> 24; > > + =C2=A0 =C2=A0 =C2=A0 idcode =3D (vcpu->arch.cp15[c0_MIDR] & 0x000= 00FF0) >> 4; >=20 > where does it say that idcode is always te same as MIDR? Good point, it doesn't. It is true for the Cortex A-15, at least. Thi= s would be more generically a switch statement on "what CPU are we" ioctl= , as I mentioned in the previous mail. > > + =C2=A0 =C2=A0 =C2=A0 /* No counters. */ > > + =C2=A0 =C2=A0 =C2=A0 num =3D 0; > > + > > + =C2=A0 =C2=A0 =C2=A0 /* Other bits are at reset value. */ >=20 > what's the point of writing anything below then? I would assume that > you can have really weird behavior if you read something different > from what you once wrote, shouldn't you read num from the vcpu value > with the bit-filter below? I think I meant that the other bits are 0 at reset. But yes, some should be read back as written. This would mean saving what they actually wrote, which is not a bad idea. > > +/* FIXME: We ignore them enabling performance monitoring. */ >=20 > if this is a FIXME, then how is it eventually going to be fixed? I was thinking that eventually we implement performance monitoring? > > +static bool write_pmcr(struct kvm_vcpu *vcpu, > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0const struct coproc_params *p, > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0unsigned long arg) > > +{ > > + =C2=A0 =C2=A0 =C2=A0 u32 val =3D *vcpu_reg(vcpu, p->Rt1); > > + > > + =C2=A0 =C2=A0 =C2=A0 kvm_debug("pmcr write:%s%s%s%s%s%s\n", > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 val & (1 = << 5) ? " DP" : "", > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 val & (1 = << 4) ? " X" : "", > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 val & (1 = << 3) ? " D" : "", > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 val & (1 = << 2) ? " C" : "", > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 val & (1 = << 1) ? " P" : "", > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 val & (1 = << 0) ? " E" : ""); > > + =C2=A0 =C2=A0 =C2=A0 return true; > > +} > > + > > +static bool read_pmcntenclr(struct kvm_vcpu *vcpu, > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 const struct coproc_params *p, > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 unsigned long arg) > > +{ > > + =C2=A0 =C2=A0 =C2=A0 /* Cycle counter is off, there are no others= =2E */ > > + =C2=A0 =C2=A0 =C2=A0 *vcpu_reg(vcpu, p->Rt1) =3D 0; > > + =C2=A0 =C2=A0 =C2=A0 return true; > > +} > > + > > +static bool write_pmcntenclr(struct kvm_vcpu *vcpu, > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 const struct coproc_params *p, > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 unsigned long arg) > > +{ > > + =C2=A0 =C2=A0 =C2=A0 /* Writing a 1 means disable a counter. =C2=A0= That's cool. */ > > + =C2=A0 =C2=A0 =C2=A0 return true; > > +} > > + > > +static bool write_pmintenclr(struct kvm_vcpu *vcpu, > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0const struct coproc_params *p, > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0unsigned long arg) > > +{ > > + =C2=A0 =C2=A0 =C2=A0 /* Writing a 1 means disable an overflow int= errupt. =C2=A0That's cool. */ > > + =C2=A0 =C2=A0 =C2=A0 return true; > > +} > > + > > =C2=A0static bool access_cp15_reg(struct kvm_vcpu *vcpu, > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0const struct coproc_params *p, > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0unsigned long cp15_reg) > > @@ -279,13 +337,18 @@ struct coproc_emulate { > > =C2=A0static const struct coproc_emulate coproc_emulate[] =3D { > > =C2=A0 =C2=A0 =C2=A0 =C2=A0/* > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 * L2CTLR access (guest wants to know #C= PUs). > > - =C2=A0 =C2=A0 =C2=A0 =C2=A0* > > - =C2=A0 =C2=A0 =C2=A0 =C2=A0* FIXME: Hack Alert: Read zero as defa= ult case. > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 */ > > =C2=A0 =C2=A0 =C2=A0 =C2=A0{ CRn( 9), CRm( 0), Op1( 1), Op2( 2), is= 32, =C2=A0READ, =C2=A0read_l2ctlr}, > > =C2=A0 =C2=A0 =C2=A0 =C2=A0{ CRn( 9), CRm(DF), Op1(DF), Op2(DF), is= 32, =C2=A0WRITE, ignore_write}, > > =C2=A0 =C2=A0 =C2=A0 =C2=A0{ CRn( 9), CRm(DF), Op1(DF), Op2(DF), is= 32, =C2=A0READ, =C2=A0read_zero}, > > > > + =C2=A0 =C2=A0 =C2=A0 /* Guest reads/writes PMU, assuming there wi= ll be one. */ > > + =C2=A0 =C2=A0 =C2=A0 { CRn( 9), CRm(12), Op1( 0), Op2( 0), is32, = =C2=A0READ, =C2=A0read_pmcr}, > > + =C2=A0 =C2=A0 =C2=A0 { CRn( 9), CRm(12), Op1( 0), Op2( 0), is32, = =C2=A0WRITE, write_pmcr}, > > + =C2=A0 =C2=A0 =C2=A0 { CRn( 9), CRm(12), Op1( 0), Op2( 2), is32, = =C2=A0READ, =C2=A0read_pmcntenclr}, > > + =C2=A0 =C2=A0 =C2=A0 { CRn( 9), CRm(12), Op1( 0), Op2( 2), is32, = =C2=A0WRITE, write_pmcntenclr}, > > + =C2=A0 =C2=A0 =C2=A0 { CRn( 9), CRm(14), Op1( 0), Op2( 2), is32, = =C2=A0WRITE, write_pmintenclr}, > > + >=20 > won't all the DF versions above "eat" these calls? Should they just g= o > away or are there still default cases to catch in which case the > comment about reading zero should perhaps be modified to specifically > mention these cases? Posted in too much of a hurry. Yes, the DFs to get removed. I'll re-spin this, and re-test. Thanks, Rusty.