[Qemu-devel] [PATCH] alpha ldl_l stl_c fix

[Qemu-devel] [PATCH] alpha ldl_l stl_c fix

[Vince Weaver]

Hello

The following patch is needed for a hello world binary to run under 
alpha-linux-user.

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.

Vince

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,34 +201,27 @@

  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);
+
  }

  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);
  }

  static always_inline void gen_store_mem (DisasContext *ctx,

Re: [Qemu-devel] [PATCH] alpha ldl_l stl_c fix

[Laurent Desnogues]

On Fri, Nov 7, 2008 at 5:10 AM, Vince Weaver <vince@csl.cornell.edu> 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