From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1O9l9R-0004Dg-TV for qemu-devel@nongnu.org; Wed, 05 May 2010 16:25:01 -0400 Received: from [140.186.70.92] (port=33786 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1O9l9P-0004Ay-K4 for qemu-devel@nongnu.org; Wed, 05 May 2010 16:25:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1O9l9N-0006ug-4g for qemu-devel@nongnu.org; Wed, 05 May 2010 16:24:59 -0400 Received: from mail-fx0-f45.google.com ([209.85.161.45]:40867) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1O9l9M-0006uR-TJ for qemu-devel@nongnu.org; Wed, 05 May 2010 16:24:57 -0400 Received: by fxm12 with SMTP id 12so4587782fxm.4 for ; Wed, 05 May 2010 13:24:54 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: Date: Thu, 6 May 2010 00:24:54 +0400 Message-ID: From: Igor Kovalenko Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: [Qemu-devel] Re: sparc64 lazy conditional codes evaluation List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Blue Swirl Cc: qemu-devel On Wed, May 5, 2010 at 12:21 AM, Blue Swirl wrote: > On 5/3/10, Igor Kovalenko wrote: >> On Mon, May 3, 2010 at 11:54 PM, Blue Swirl wrote= : >> =A0> On 5/3/10, Igor Kovalenko wrote: >> =A0>> On Mon, May 3, 2010 at 11:24 PM, Blue Swirl = wrote: >> =A0>> =A0> On 5/3/10, Igor Kovalenko wrote: >> =A0>> =A0>> Hi! >> =A0>> =A0>> >> =A0>> =A0>> =A0There is an issue with lazy conditional codes evaluation = where >> =A0>> =A0>> =A0we return from trap handler with mismatching conditionals= . >> =A0>> =A0>> >> =A0>> =A0>> =A0I seldom reproduce it here when dragging qemu window whil= e >> =A0>> =A0>> =A0machine is working through silo initialization. I use gen= too minimal cd >> =A0>> =A0>> =A0install-sparc64-minimal-20100322.iso but I think anything= with silo boot >> =A0>> =A0>> =A0would experience the same. Once in a while it would repor= t crc error, >> =A0>> =A0>> =A0unable to open cd partition or it would fail to decompres= s image. >> =A0>> =A0> >> =A0>> =A0> I think I've also seen this. >> =A0>> =A0> >> =A0>> =A0>> =A0Pattern that fails appears to require a sequence of compa= re insn >> =A0>> =A0>> =A0possibly followed by a few instructions which do not touc= h conditionals, >> =A0>> =A0>> =A0then conditional branch insn. If it happens that we trap = while processing >> =A0>> =A0>> =A0conditional branch insn so it is restarted after return f= rom trap then >> =A0>> =A0>> =A0seldom conditional codes are calculated incorrectly. >> =A0>> =A0>> >> =A0>> =A0>> =A0I cannot point to exact cause but it appears that after t= rap return >> =A0>> =A0>> =A0we may have CC_OP and CC_SRC* mismatch somewhere, >> =A0>> =A0>> =A0since adding more cond evaluation flushes over the code h= elps. >> =A0>> =A0>> >> =A0>> =A0>> =A0We already tried doing flush more frequently and it is st= ill not >> =A0>> =A0>> =A0complete, so the question is how to finally do this once = and right :) >> =A0>> =A0>> >> =A0>> =A0>> =A0Obviously I do not get the design of lazy evaluation righ= t, but >> =A0>> =A0>> =A0the following list appears to be good start. Plan is to p= repare >> =A0>> =A0>> =A0a change to qemu and find a way to test it. >> =A0>> =A0>> >> =A0>> =A0>> =A01. Since SPARC* is a RISC CPU it seems to be not profitab= le to >> =A0>> =A0>> =A0 =A0use DisasContext->cc_op to predict if flags should be= not evaluated >> =A0>> =A0>> =A0 =A0due to overriding insn. Instead we can drop cc_op fro= m disassembler >> =A0>> =A0>> =A0 =A0context and simplify code to only use cc_op from env. >> =A0>> =A0> >> =A0>> =A0> Not currently, but in the future we may use that to do even l= azier >> =A0>> =A0> flags computation. For example the sequence 'cmp x, y; bne ta= rget' >> =A0>> =A0> could be much more optimal by changing the branch to do the >> =A0>> =A0> comparison. Here's an old unfinished patch to do some of this= . I wonder if it buys anything. Sparc RISC architecture means optimizing comp= iler would prevent any extra flags computation, right? So it is basically 1-to-1 conditional computation and use. Or even worse, if we delay computation until there are two or more consumers, correct? >> =A0>> =A0> >> =A0>> =A0>> =A0 =A0Another point is that we always write to env->cc_op w= hen >> =A0>> =A0>> =A0translating *cc insns >> =A0>> =A0>> =A0 =A0This should solve any issue with dc->cc_op prediction= going >> =A0>> =A0>> =A0 =A0out of sync with env->cc_op and cpu_cc_src* >> =A0>> =A0> >> =A0>> =A0> I think this is what is happening now. >> =A0>> =A0> >> =A0>> =A0>> =A02. We must flush lazy evaluation back to CC_OP_FLAGS in a= few cases when >> =A0>> =A0>> =A0 =A0a. conditional code is required by insn (like addc, c= ond branch etc.) >> =A0>> =A0>> =A0 =A0 =A0 - here we can optimize by evaluating specific bi= ts (carry?) >> =A0>> =A0>> =A0 =A0 =A0 - not sure if it works in case we have two cond = consuming insns, >> =A0>> =A0>> =A0 =A0 =A0 =A0 where first needs carry another needs the re= st of flags >> =A0>> =A0> >> =A0>> =A0> Here's another patch to optimize C flag handling. It doesn't = pass my >> =A0>> =A0> tests though. >> =A0>> =A0> >> =A0>> =A0>> =A0 =A0b. CCR is read by rdccr (helper_rdccr) >> =A0>> =A0>> =A0 =A0 =A0 - have to compute all flags >> =A0>> =A0>> =A0 =A0c. trap occurs and we prepare trap level context (sav= ing pstate) >> =A0>> =A0>> =A0 =A0 =A0 - have to compute all flags >> =A0>> =A0>> =A0 =A0d. control goes out of tcg runtime (so gdbstub reads = correct value from env) >> =A0>> =A0>> =A0 =A0 =A0 - have to compute all flags >> =A0>> =A0> >> =A0>> =A0> Fully agree. >> =A0>> >> =A0>> >> =A0>> Cool >> =A0>> >> =A0>> =A0Still I'd propose to kill dc->cc_op, find a reliable way to tes= t it >> =A0>> =A0and then add it back possibly with more optimizations. >> =A0>> =A0I'm lost in the code up to the point where I believe we need to >> =A0>> =A0save/restore cc_op and cpu_cc* while switching trap levels. >> =A0> >> =A0> I'd think this should do the trick: >> =A0> >> =A0> diff --git a/target-sparc/op_helper.c b/target-sparc/op_helper.c >> =A0> index b27778b..94921cd 100644 >> =A0> --- a/target-sparc/op_helper.c >> =A0> +++ b/target-sparc/op_helper.c >> =A0> @@ -3506,6 +3506,8 @@ void do_interrupt(CPUState *env) >> =A0> =A0 =A0 } >> =A0> =A0 =A0 tsptr =3D cpu_tsptr(env); >> =A0> >> =A0> + =A0 =A0helper_compute_psr(); >> =A0> + >> =A0> =A0 =A0 tsptr->tstate =3D ((uint64_t)GET_CCR(env) << 32) | >> =A0> =A0 =A0 =A0 =A0 ((env->asi & 0xff) << 24) | ((env->pstate & 0xf3f) = << 8) | >> =A0> =A0 =A0 =A0 =A0 GET_CWP64(env); >> =A0> >> >> >> Thanks, this change seems to work here for silo issue. >> >> =A0Another change would be to flush for gdbstub use of GET_CCR and for >> =A0helper_rdccr. >> =A0I tried to embed flush into GET_CCR but the code looks ugly since we >> =A0need to proxy a call to helper_compute_psr from gdbstub passing >> =A0available env pointer. >> >> =A0Not really tested with your changes, but still what is the breakage y= ou see? > > Aurora 2.0 (http://distro.ibiblio.org/pub/linux/distributions/aurora/buil= d-2.0/sparc/iso/) > breaks. > > This is what I get with git HEAD, having pressed enter key twice: > Welcome to Aurora SPARC Linux > > > > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 +--------------+ CD Found +----------= ----+ > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 | =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0| > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 | To begin testing the CD media befor= e =A0 | > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 | installation press OK. =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 | > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 | =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0| > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 | Choose Skip to skip the media test = =A0 =A0 | > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 | and start the installation. =A0 =A0= =A0 =A0 =A0 =A0| > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 | =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0| > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 | =A0 =A0 =A0+----+ =A0 =A0 =A0 =A0 = =A0 =A0 +------+ =A0 =A0 =A0 | > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 | =A0 =A0 =A0| OK | =A0 =A0 =A0 =A0 = =A0 =A0 | Skip | =A0 =A0 =A0 | > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 | =A0 =A0 =A0+----+ =A0 =A0 =A0 =A0 = =A0 =A0 +------+ =A0 =A0 =A0 | > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 | =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0| > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 | =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0| > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 +------------------------------------= ----+ > > > > > =A0/ between elements =A0| selects | next scr= een > > This is what I get with the C flag patch applied: > Welcome to Aurora SPARC Linux > > > > > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0+--------------+ Error +----------= -----+ > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0| =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0| > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0| failed to read keymap informatio= n: =A0 | > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0| Success =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0| > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0| =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0| > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0| =A0 =A0 =A0 =A0 =A0 =A0 =A0 +---= -+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 | > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0| =A0 =A0 =A0 =A0 =A0 =A0 =A0 | OK= | =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 | > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0| =A0 =A0 =A0 =A0 =A0 =A0 =A0 +---= -+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 | > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0| =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0| > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0| =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0| > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0+---------------------------------= -----+ > > > > > > > =A0/ between elements =A0| selects | next scr= een > I do reproduce the issue here with 0001-Convert-C-flag-input-BROKEN.patch --=20 Kind regards, Igor V. Kovalenko