From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KyP2h-0005Qu-9N for qemu-devel@nongnu.org; Fri, 07 Nov 2008 05:58:19 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KyP2f-0005PP-AT for qemu-devel@nongnu.org; Fri, 07 Nov 2008 05:58:17 -0500 Received: from [199.232.76.173] (port=52372 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KyP2e-0005P7-V3 for qemu-devel@nongnu.org; Fri, 07 Nov 2008 05:58:17 -0500 Received: from wf-out-1314.google.com ([209.85.200.172]:16384) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1KyP2e-0007D7-V1 for qemu-devel@nongnu.org; Fri, 07 Nov 2008 05:58:17 -0500 Received: by wf-out-1314.google.com with SMTP id 27so1164774wfd.4 for ; Fri, 07 Nov 2008 02:58:15 -0800 (PST) Message-ID: <761ea48b0811070258w7c3cfabdt30353a458ff82156@mail.gmail.com> Date: Fri, 7 Nov 2008 11:58:14 +0100 From: "Laurent Desnogues" Subject: Re: [Qemu-devel] [PATCH] alpha ldl_l stl_c fix In-Reply-To: <20081106230635.U85421@stanley.csl.cornell.edu> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <20081106230635.U85421@stanley.csl.cornell.edu> Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org On Fri, Nov 7, 2008 at 5:10 AM, Vince Weaver wrote: > > The following patch is needed for a hello world binary to run under > alpha-linux-user. Could you please provide me with the binary? > The issue is the ldl_l and stl_c instructions. The current implementation > is a bit confused. > ldl_l should set the lock bit, and write the address to a table > stl_c should write to memory only if lock bit is set, then return the > lock_bit, then reset the lock_bit to zero > > The current code does a weird mix of these things that don't seem to work. > > This patch fixes things enough for me for hello_world to work, but it still > isn't correct if we ever want to use these instructions to implement atomic > loads/stores or run multithreaded code. > > Note... the patch includes a commented out branch. For proper emulation > that branch should be included, but for some reason on my x86_64 system if > the branch is included, then the memory address calculation is converted to > nops for some reason (leading to a segfault). I'm not sure why that is > happening. Comments below. > Index: target-alpha/translate.c > =================================================================== > --- target-alpha/translate.c (revision 5643) > +++ target-alpha/translate.c (working copy) > @@ -138,13 +138,13 @@ > > static always_inline void gen_qemu_ldl_l (TCGv t0, TCGv t1, int flags) > { > - tcg_gen_mov_i64(cpu_lock, t1); > + tcg_gen_movi_i64(cpu_lock, 1); > tcg_gen_qemu_ld32s(t0, t1, flags); > } OK. > static always_inline void gen_qemu_ldq_l (TCGv t0, TCGv t1, int flags) > { > - tcg_gen_mov_i64(cpu_lock, t1); > + tcg_gen_movi_i64(cpu_lock, 1); > tcg_gen_qemu_ld64(t0, t1, flags); > } OK. > static always_inline void gen_qemu_stl_c (TCGv t0, TCGv t1, int flags) > { > - int l1, l2; > + int l1; > > l1 = gen_new_label(); > - l2 = gen_new_label(); > - tcg_gen_brcond_i64(TCG_COND_NE, cpu_lock, t1, l1); > +// tcg_gen_brcondi_i64(TCG_COND_EQ, cpu_lock, 0, l1); > tcg_gen_qemu_st32(t0, t1, flags); > - tcg_gen_movi_i64(t0, 0); > - tcg_gen_br(l2); > gen_set_label(l1); > - tcg_gen_movi_i64(t0, 1); > - gen_set_label(l2); > - tcg_gen_movi_i64(cpu_lock, -1); > + tcg_gen_mov_i64(t0, cpu_lock); + tcg_gen_movi_i64(cpu_lock, 0); It's not the first time your mailer seems to merge diff lines. Or is it mine that is wrong? > + > } > > static always_inline void gen_qemu_stq_c (TCGv t0, TCGv t1, int flags) > { > - int l1, l2; > + int l1; > > l1 = gen_new_label(); > - l2 = gen_new_label(); > - tcg_gen_brcond_i64(TCG_COND_NE, cpu_lock, t1, l1); > +// tcg_gen_brcondi_i64(TCG_COND_EQ, cpu_lock, 0, l1); > tcg_gen_qemu_st64(t0, t1, flags); > - tcg_gen_movi_i64(t0, 0); > - tcg_gen_br(l2); > gen_set_label(l1); > - tcg_gen_movi_i64(t0, 1); > - gen_set_label(l2); > - tcg_gen_movi_i64(cpu_lock, -1); > + tcg_gen_movi_i64(t0, cpu_lock); > + tcg_gen_movi_i64(cpu_lock, 0); > } Both brcond you commented out are wrong; they should be: tcg_gen_brcondi_i64(TCG_COND_NE, cpu_lock, 1, l1); at least if one follows the spec blindly :-) But that's not the root cause of the problem; they are equivalent given the way we set lock in ld[ql]_c; however lock is also used by helpers; I need to take a closer look; so to be on the safe side using != 1 seems the way to go. The issue here is that the st[ql]_c routine is called indirectly by gen_store_mem which allocates the address as a temp variable; st[ql]_c generates a brcond before using that temp and so the liveness analysis phase gets rid of the temp. That temp should be replaced by a local temp. I will propose a patch for that separately including your modifications. The remainder looks OK (except of course atomicity is not respected as you wrote). Laurent