Re: [Qemu-devel] [PATCH] Alpha: fix locked loads/stores

[Aurelien Jarno]

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