| Message ID | 20200817140144.373403-5-edgar.iglesias@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | target/microblaze: Enable MTTCG | expand |
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~
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
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 --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) {