diff mbox series

[v1,4/5] target/microblaze: swx: Use atomic_cmpxchg

Message ID 20200817140144.373403-5-edgar.iglesias@gmail.com
State New
Headers show
Series target/microblaze: Enable MTTCG | expand

Commit Message

Edgar E. Iglesias Aug. 17, 2020, 2:01 p.m. UTC
From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Use atomic_cmpxchg to implement the atomic cmpxchg sequence.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target/microblaze/translate.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

Comments

Richard Henderson Aug. 17, 2020, 3:52 p.m. UTC | #1
On 8/17/20 7:01 AM, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> 
> Use atomic_cmpxchg to implement the atomic cmpxchg sequence.
> 
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target/microblaze/translate.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
> index c58f334a0f..530c15e5ad 100644
> --- a/target/microblaze/translate.c
> +++ b/target/microblaze/translate.c
> @@ -1075,14 +1075,16 @@ static void dec_store(DisasContext *dc)
>          swx_skip = gen_new_label();
>          tcg_gen_brcond_tl(TCG_COND_NE, env_res_addr, addr, swx_skip);
>  
> -        /* Compare the value loaded at lwx with current contents of
> -           the reserved location.
> -           FIXME: This only works for system emulation where we can expect
> -           this compare and the following write to be atomic. For user
> -           emulation we need to add atomicity between threads.  */
> +        /*
> +         * Compare the value loaded at lwx with current contents of
> +         * the reserved location.
> +         */
>          tval = tcg_temp_new_i32();
> -        tcg_gen_qemu_ld_i32(tval, addr, cpu_mmu_index(&dc->cpu->env, false),
> -                            MO_TEUL);
> +
> +        tcg_gen_atomic_cmpxchg_i32(tval, addr, env_res_val,
> +                                   cpu_R[dc->rd], mem_index,
> +                                   mop);
> +
>          tcg_gen_brcond_i32(TCG_COND_NE, env_res_val, tval, swx_skip);
>          write_carryi(dc, 0);
>          tcg_temp_free_i32(tval);
> @@ -1108,7 +1110,10 @@ static void dec_store(DisasContext *dc)
>                  break;
>          }
>      }
> -    tcg_gen_qemu_st_i32(cpu_R[dc->rd], addr, mem_index, mop);
> +
> +    if (!ex) {
> +        tcg_gen_qemu_st_i32(cpu_R[dc->rd], addr, mem_index, mop);
> +    }
>  
>      /* Verify alignment if needed.  */
>      if (dc->cpu->cfg.unaligned_exceptions && size > 1) {
> 

This is fine as far as the actual swx instruction goes, so
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

However, some notes for dec_store,

There seems to be a large under-decode here.  E.g. rev should never be set for
swx.  But rev is accepted and partially implemented.  E.g. there is no sbx
instruction, but the ex bit is accepted for any store instruction.

Microblaze has a relatively small instruction set.  Would you be open to a
conversion to decodetree?  It should be fairly easy.


r~
Edgar E. Iglesias Aug. 17, 2020, 3:59 p.m. UTC | #2
On Mon, Aug 17, 2020 at 08:52:16AM -0700, Richard Henderson wrote:
> On 8/17/20 7:01 AM, Edgar E. Iglesias wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> > 
> > Use atomic_cmpxchg to implement the atomic cmpxchg sequence.
> > 
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> >  target/microblaze/translate.c | 21 +++++++++++++--------
> >  1 file changed, 13 insertions(+), 8 deletions(-)
> > 
> > diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
> > index c58f334a0f..530c15e5ad 100644
> > --- a/target/microblaze/translate.c
> > +++ b/target/microblaze/translate.c
> > @@ -1075,14 +1075,16 @@ static void dec_store(DisasContext *dc)
> >          swx_skip = gen_new_label();
> >          tcg_gen_brcond_tl(TCG_COND_NE, env_res_addr, addr, swx_skip);
> >  
> > -        /* Compare the value loaded at lwx with current contents of
> > -           the reserved location.
> > -           FIXME: This only works for system emulation where we can expect
> > -           this compare and the following write to be atomic. For user
> > -           emulation we need to add atomicity between threads.  */
> > +        /*
> > +         * Compare the value loaded at lwx with current contents of
> > +         * the reserved location.
> > +         */
> >          tval = tcg_temp_new_i32();
> > -        tcg_gen_qemu_ld_i32(tval, addr, cpu_mmu_index(&dc->cpu->env, false),
> > -                            MO_TEUL);
> > +
> > +        tcg_gen_atomic_cmpxchg_i32(tval, addr, env_res_val,
> > +                                   cpu_R[dc->rd], mem_index,
> > +                                   mop);
> > +
> >          tcg_gen_brcond_i32(TCG_COND_NE, env_res_val, tval, swx_skip);
> >          write_carryi(dc, 0);
> >          tcg_temp_free_i32(tval);
> > @@ -1108,7 +1110,10 @@ static void dec_store(DisasContext *dc)
> >                  break;
> >          }
> >      }
> > -    tcg_gen_qemu_st_i32(cpu_R[dc->rd], addr, mem_index, mop);
> > +
> > +    if (!ex) {
> > +        tcg_gen_qemu_st_i32(cpu_R[dc->rd], addr, mem_index, mop);
> > +    }
> >  
> >      /* Verify alignment if needed.  */
> >      if (dc->cpu->cfg.unaligned_exceptions && size > 1) {
> > 
> 
> This is fine as far as the actual swx instruction goes, so
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> However, some notes for dec_store,
> 
> There seems to be a large under-decode here.  E.g. rev should never be set for
> swx.  But rev is accepted and partially implemented.  E.g. there is no sbx
> instruction, but the ex bit is accepted for any store instruction.
> 
> Microblaze has a relatively small instruction set.  Would you be open to a
> conversion to decodetree?  It should be fairly easy.
>

Thanks Richard,

Yes, I've got a conversion to decodetree on my TODO list (before adding 64bit support).
I'll probably bug you when I get to it!

Best regards,
Edgar
Alistair Francis Aug. 17, 2020, 4:11 p.m. UTC | #3
On Mon, Aug 17, 2020 at 7:04 AM Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
>
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Use atomic_cmpxchg to implement the atomic cmpxchg sequence.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/microblaze/translate.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
> index c58f334a0f..530c15e5ad 100644
> --- a/target/microblaze/translate.c
> +++ b/target/microblaze/translate.c
> @@ -1075,14 +1075,16 @@ static void dec_store(DisasContext *dc)
>          swx_skip = gen_new_label();
>          tcg_gen_brcond_tl(TCG_COND_NE, env_res_addr, addr, swx_skip);
>
> -        /* Compare the value loaded at lwx with current contents of
> -           the reserved location.
> -           FIXME: This only works for system emulation where we can expect
> -           this compare and the following write to be atomic. For user
> -           emulation we need to add atomicity between threads.  */
> +        /*
> +         * Compare the value loaded at lwx with current contents of
> +         * the reserved location.
> +         */
>          tval = tcg_temp_new_i32();
> -        tcg_gen_qemu_ld_i32(tval, addr, cpu_mmu_index(&dc->cpu->env, false),
> -                            MO_TEUL);
> +
> +        tcg_gen_atomic_cmpxchg_i32(tval, addr, env_res_val,
> +                                   cpu_R[dc->rd], mem_index,
> +                                   mop);
> +
>          tcg_gen_brcond_i32(TCG_COND_NE, env_res_val, tval, swx_skip);
>          write_carryi(dc, 0);
>          tcg_temp_free_i32(tval);
> @@ -1108,7 +1110,10 @@ static void dec_store(DisasContext *dc)
>                  break;
>          }
>      }
> -    tcg_gen_qemu_st_i32(cpu_R[dc->rd], addr, mem_index, mop);
> +
> +    if (!ex) {
> +        tcg_gen_qemu_st_i32(cpu_R[dc->rd], addr, mem_index, mop);
> +    }
>
>      /* Verify alignment if needed.  */
>      if (dc->cpu->cfg.unaligned_exceptions && size > 1) {
> --
> 2.25.1
>
>
diff mbox series

Patch

diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
index c58f334a0f..530c15e5ad 100644
--- a/target/microblaze/translate.c
+++ b/target/microblaze/translate.c
@@ -1075,14 +1075,16 @@  static void dec_store(DisasContext *dc)
         swx_skip = gen_new_label();
         tcg_gen_brcond_tl(TCG_COND_NE, env_res_addr, addr, swx_skip);
 
-        /* Compare the value loaded at lwx with current contents of
-           the reserved location.
-           FIXME: This only works for system emulation where we can expect
-           this compare and the following write to be atomic. For user
-           emulation we need to add atomicity between threads.  */
+        /*
+         * Compare the value loaded at lwx with current contents of
+         * the reserved location.
+         */
         tval = tcg_temp_new_i32();
-        tcg_gen_qemu_ld_i32(tval, addr, cpu_mmu_index(&dc->cpu->env, false),
-                            MO_TEUL);
+
+        tcg_gen_atomic_cmpxchg_i32(tval, addr, env_res_val,
+                                   cpu_R[dc->rd], mem_index,
+                                   mop);
+
         tcg_gen_brcond_i32(TCG_COND_NE, env_res_val, tval, swx_skip);
         write_carryi(dc, 0);
         tcg_temp_free_i32(tval);
@@ -1108,7 +1110,10 @@  static void dec_store(DisasContext *dc)
                 break;
         }
     }
-    tcg_gen_qemu_st_i32(cpu_R[dc->rd], addr, mem_index, mop);
+
+    if (!ex) {
+        tcg_gen_qemu_st_i32(cpu_R[dc->rd], addr, mem_index, mop);
+    }
 
     /* Verify alignment if needed.  */
     if (dc->cpu->cfg.unaligned_exceptions && size > 1) {