[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] alpha ldl_l stl_c fix
From: |
Laurent Desnogues |
Subject: |
Re: [Qemu-devel] [PATCH] alpha ldl_l stl_c fix |
Date: |
Fri, 7 Nov 2008 11:58:14 +0100 |
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