[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] Alpha: fix locked loads/stores
From: |
Aurelien Jarno |
Subject: |
Re: [Qemu-devel] [PATCH] Alpha: fix locked loads/stores |
Date: |
Fri, 7 Nov 2008 15:00:34 +0100 |
User-agent: |
Mutt/1.5.18 (2008-05-17) |
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 <address@hidden>
> 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
`. `' address@hidden | address@hidden
`- people.debian.org/~aurel32 | www.aurel32.net
- [Qemu-devel] [PATCH] Alpha: fix locked loads/stores, Laurent Desnogues, 2008/11/07
- Re: [Qemu-devel] [PATCH] Alpha: fix locked loads/stores,
Aurelien Jarno <=
- Re: [Qemu-devel] [PATCH] Alpha: fix locked loads/stores, Laurent Desnogues, 2008/11/07
- Re: [Qemu-devel] [PATCH] Alpha: fix locked loads/stores, Aurelien Jarno, 2008/11/07
- Re: [Qemu-devel] [PATCH] Alpha: fix locked loads/stores, Laurent Desnogues, 2008/11/07
- Re: [Qemu-devel] [PATCH] Alpha: fix locked loads/stores, Vince Weaver, 2008/11/07
- Re: [Qemu-devel] [PATCH] Alpha: fix locked loads/stores, Aurelien Jarno, 2008/11/08
- Re: [Qemu-devel] [PATCH] Alpha: fix locked loads/stores, Laurent Desnogues, 2008/11/08
- Re: [Qemu-devel] [PATCH] Alpha: fix locked loads/stores, Paul Brook, 2008/11/07