From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KyRtD-0000SR-SN for qemu-devel@nongnu.org; Fri, 07 Nov 2008 09:00:44 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KyRtC-0000RJ-MQ for qemu-devel@nongnu.org; Fri, 07 Nov 2008 09:00:43 -0500 Received: from [199.232.76.173] (port=60475 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KyRtB-0000Qa-PO for qemu-devel@nongnu.org; Fri, 07 Nov 2008 09:00:42 -0500 Received: from hall.aurel32.net ([88.191.82.174]:41111) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1KyRtB-0006g3-1K for qemu-devel@nongnu.org; Fri, 07 Nov 2008 09:00:41 -0500 Received: from p-jarno.univ-lyon1.fr ([134.214.4.132] helo=volta.aurel32.net) by hall.aurel32.net with esmtpsa (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.63) (envelope-from ) id 1KyRt9-0000LK-Qh for qemu-devel@nongnu.org; Fri, 07 Nov 2008 15:00:39 +0100 Received: from aurel32 by volta.aurel32.net with local (Exim 4.69) (envelope-from ) id 1KyRt4-00086D-Ll for qemu-devel@nongnu.org; Fri, 07 Nov 2008 15:00:34 +0100 Date: Fri, 7 Nov 2008 15:00:34 +0100 From: Aurelien Jarno Subject: Re: [Qemu-devel] [PATCH] Alpha: fix locked loads/stores Message-ID: <20081107140034.GA28030@volta.aurel32.net> References: <761ea48b0811070334lb817c00x38624ca7bb41bb57@mail.gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-15 Content-Disposition: inline In-Reply-To: <761ea48b0811070334lb817c00x38624ca7bb41bb57@mail.gmail.com> 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 07, 2008 at 12:34:29PM +0100, Laurent Desnogues wrote: > Hello, > > this patch is based on Vince Weaver patch for locked loads/stores. > It was checked against Alpha architecture manual. > > Two fixes were done to Vince's patch: > > - use a comparison to 1 for lock instead of 0 to be closer to the > Alpha spec I don't agree with this part. The current code use a single variable for both address and lock_bit to spare a few tests. Basically it sets cpu_lock to -1 when not locked and stores the address when locked. Your patch does not compare the address, so it will break multi-threading. > - fix reading of cpu_lock in gen_qemu_stql_c. I agree with this part, I have applied it. > On top of Vince's modifications, a new flag was added to > gen_store_mem to allocate local temps instead of temps; this flag > should be set when the tcg_gen_qemu_store callback uses brcond > before using the temps or else liveness analysis will get rid of the > temps. > > This also adds lock printing in cpu_dump_state which can help > debug. > > > Laurent > > Signed-off-by: Laurent Desnogues > Index: target-alpha/helper.c > =================================================================== > --- target-alpha/helper.c (revision 5643) > +++ target-alpha/helper.c (working copy) > @@ -434,5 +434,6 @@ > if ((i % 3) == 2) > cpu_fprintf(f, "\n"); > } > + cpu_fprintf(f, "\nlock %d\n", env->lock); > } > > 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); > } > > 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); > } > > @@ -201,42 +201,38 @@ > > 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_NE, cpu_lock, 1, 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); > } > > 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_NE, cpu_lock, 1, 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_mov_i64(t0, cpu_lock); > + tcg_gen_movi_i64(cpu_lock, 0); > } > > static always_inline void gen_store_mem (DisasContext *ctx, > void (*tcg_gen_qemu_store)(TCGv t0, TCGv t1, int flags), > int ra, int rb, int32_t disp16, > - int fp, int clear) > + int fp, int clear, int local) > { > TCGv addr = tcg_temp_new(TCG_TYPE_I64); > + if (local) > + addr = tcg_temp_local_new(TCG_TYPE_I64); > + else > + addr = tcg_temp_new(TCG_TYPE_I64); > if (rb != 31) { > tcg_gen_addi_i64(addr, cpu_ir[rb], disp16); > if (clear) > @@ -252,7 +248,11 @@ > else > tcg_gen_qemu_store(cpu_ir[ra], addr, ctx->mem_idx); > } else { > - TCGv zero = tcg_const_i64(0); > + TCGv zero; > + if (local) > + zero = tcg_const_local_i64(0); > + else > + zero = tcg_const_i64(0); > tcg_gen_qemu_store(zero, addr, ctx->mem_idx); > tcg_temp_free(zero); > } > @@ -636,15 +636,15 @@ > break; > case 0x0D: > /* STW */ > - gen_store_mem(ctx, &tcg_gen_qemu_st16, ra, rb, disp16, 0, 0); > + gen_store_mem(ctx, &tcg_gen_qemu_st16, ra, rb, disp16, 0, 0, 0); > break; > case 0x0E: > /* STB */ > - gen_store_mem(ctx, &tcg_gen_qemu_st8, ra, rb, disp16, 0, 0); > + gen_store_mem(ctx, &tcg_gen_qemu_st8, ra, rb, disp16, 0, 0, 0); > break; > case 0x0F: > /* STQ_U */ > - gen_store_mem(ctx, &tcg_gen_qemu_st64, ra, rb, disp16, 0, 1); > + gen_store_mem(ctx, &tcg_gen_qemu_st64, ra, rb, disp16, 0, 1, 0); > break; > case 0x10: > switch (fn7) { > @@ -2090,19 +2090,19 @@ > break; > case 0x24: > /* STF */ > - gen_store_mem(ctx, &gen_qemu_stf, ra, rb, disp16, 1, 0); > + gen_store_mem(ctx, &gen_qemu_stf, ra, rb, disp16, 1, 0, 0); > break; > case 0x25: > /* STG */ > - gen_store_mem(ctx, &gen_qemu_stg, ra, rb, disp16, 1, 0); > + gen_store_mem(ctx, &gen_qemu_stg, ra, rb, disp16, 1, 0, 0); > break; > case 0x26: > /* STS */ > - gen_store_mem(ctx, &gen_qemu_sts, ra, rb, disp16, 1, 0); > + gen_store_mem(ctx, &gen_qemu_sts, ra, rb, disp16, 1, 0, 0); > break; > case 0x27: > /* STT */ > - gen_store_mem(ctx, &tcg_gen_qemu_st64, ra, rb, disp16, 1, 0); > + gen_store_mem(ctx, &tcg_gen_qemu_st64, ra, rb, disp16, 1, 0, 0); > break; > case 0x28: > /* LDL */ > @@ -2122,19 +2122,19 @@ > break; > case 0x2C: > /* STL */ > - gen_store_mem(ctx, &tcg_gen_qemu_st32, ra, rb, disp16, 0, 0); > + gen_store_mem(ctx, &tcg_gen_qemu_st32, ra, rb, disp16, 0, 0, 0); > break; > case 0x2D: > /* STQ */ > - gen_store_mem(ctx, &tcg_gen_qemu_st64, ra, rb, disp16, 0, 0); > + gen_store_mem(ctx, &tcg_gen_qemu_st64, ra, rb, disp16, 0, 0, 0); > break; > case 0x2E: > /* STL_C */ > - gen_store_mem(ctx, &gen_qemu_stl_c, ra, rb, disp16, 0, 0); > + gen_store_mem(ctx, &gen_qemu_stl_c, ra, rb, disp16, 0, 0, 1); > break; > case 0x2F: > /* STQ_C */ > - gen_store_mem(ctx, &gen_qemu_stq_c, ra, rb, disp16, 0, 0); > + gen_store_mem(ctx, &gen_qemu_stq_c, ra, rb, disp16, 0, 0, 1); > break; > case 0x30: > /* BR */ -- .''`. Aurelien Jarno | GPG: 1024D/F1BCDB73 : :' : Debian developer | Electrical Engineer `. `' aurel32@debian.org | aurelien@aurel32.net `- people.debian.org/~aurel32 | www.aurel32.net