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

[Laurent Desnogues]

On Fri, Nov 7, 2008 at 5:10 AM, Vince Weaver <address@hidden> 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