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

["Edgar E. Iglesias" <edgar.iglesias@xilinx.com>]

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