[PATCH v1 0/5] target/microblaze: Enable MTTCG

[PATCH v1 0/5] target/microblaze: Enable MTTCG

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

This series adds translation for memory barrier instructions and
changes the store-exclusive implementation to use cmpxhg rather
than relying on single-threaded TCG.

This is primarily in preparation for future AMP machines.

Cheers,
Edgar

Edgar E. Iglesias (5):
  target/microblaze: mbar: Transfer dc->rd to mbar_imm
  target/microblaze: mbar: Move LOG_DIS to before sleep
  target/microblaze: mbar: Add support for data-access barriers
  target/microblaze: swx: Use atomic_cmpxchg
  configure: microblaze: Enable mttcg

 configure                     |  1 +
 target/microblaze/translate.c | 33 +++++++++++++++++++++++----------
 2 files changed, 24 insertions(+), 10 deletions(-)

-- 
2.25.1

[PATCH v1 1/5] target/microblaze: mbar: Transfer dc->rd to mbar_imm

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Transfer dc->rd to mbar_imm to improve the readability when
comparing to the specs.

No functional change.

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

diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
index f6ff2591c3..47637f152b 100644
--- a/target/microblaze/translate.c
+++ b/target/microblaze/translate.c
@@ -1229,8 +1229,10 @@ static void dec_br(DisasContext *dc)
     /* Memory barrier.  */
     mbar = (dc->ir >> 16) & 31;
     if (mbar == 2 && dc->imm == 4) {
+        uint16_t mbar_imm = dc->rd;
+
         /* mbar IMM & 16 decodes to sleep.  */
-        if (dc->rd & 16) {
+        if (mbar_imm & 16) {
             TCGv_i32 tmp_hlt = tcg_const_i32(EXCP_HLT);
             TCGv_i32 tmp_1 = tcg_const_i32(1);
 
@@ -1246,7 +1248,7 @@ static void dec_br(DisasContext *dc)
             tcg_temp_free_i32(tmp_1);
             return;
         }
-        LOG_DIS("mbar %d\n", dc->rd);
+        LOG_DIS("mbar %d\n", mbar_imm);
         /* Break the TB.  */
         dc->cpustate_changed = 1;
         return;
-- 
2.25.1

[PATCH v1 2/5] target/microblaze: mbar: Move LOG_DIS to before sleep

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Move LOG_DIS log to before sleeping handling so that it logs
for sleep instructions aswell.

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

diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
index 47637f152b..c1be76d4c8 100644
--- a/target/microblaze/translate.c
+++ b/target/microblaze/translate.c
@@ -1231,6 +1231,8 @@ static void dec_br(DisasContext *dc)
     if (mbar == 2 && dc->imm == 4) {
         uint16_t mbar_imm = dc->rd;
 
+        LOG_DIS("mbar %d\n", mbar_imm);
+
         /* mbar IMM & 16 decodes to sleep.  */
         if (mbar_imm & 16) {
             TCGv_i32 tmp_hlt = tcg_const_i32(EXCP_HLT);
@@ -1248,7 +1250,6 @@ static void dec_br(DisasContext *dc)
             tcg_temp_free_i32(tmp_1);
             return;
         }
-        LOG_DIS("mbar %d\n", mbar_imm);
         /* Break the TB.  */
         dc->cpustate_changed = 1;
         return;
-- 
2.25.1

[PATCH v1 3/5] target/microblaze: mbar: Add support for data-access barriers

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Add support for data-access barriers.

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

diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
index c1be76d4c8..c58f334a0f 100644
--- a/target/microblaze/translate.c
+++ b/target/microblaze/translate.c
@@ -1233,6 +1233,11 @@ static void dec_br(DisasContext *dc)
 
         LOG_DIS("mbar %d\n", mbar_imm);
 
+        /* Data access memory barrier.  */
+        if ((mbar_imm & 2) == 0) {
+            tcg_gen_mb(TCG_BAR_SC | TCG_MO_ALL);
+        }
+
         /* mbar IMM & 16 decodes to sleep.  */
         if (mbar_imm & 16) {
             TCGv_i32 tmp_hlt = tcg_const_i32(EXCP_HLT);
-- 
2.25.1

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

[Edgar E. Iglesias]

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) {
-- 
2.25.1

[PATCH v1 5/5] configure: microblaze: Enable mttcg

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 configure | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configure b/configure
index 2acc4d1465..2f7adaa6ae 100755
--- a/configure
+++ b/configure
@@ -8162,6 +8162,7 @@ case "$target_name" in
   microblaze|microblazeel)
     TARGET_ARCH=microblaze
     TARGET_SYSTBL_ABI=common
+    mttcg="yes"
     bflt="yes"
     echo "TARGET_ABI32=y" >> $config_target_mak
   ;;
-- 
2.25.1

Re: [PATCH v1 1/5] target/microblaze: mbar: Transfer dc->rd to mbar_imm

[Richard Henderson]

On 8/17/20 7:01 AM, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> 
> Transfer dc->rd to mbar_imm to improve the readability when
> comparing to the specs.
> 
> No functional change.
> 
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target/microblaze/translate.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v1 2/5] target/microblaze: mbar: Move LOG_DIS to before sleep

[Richard Henderson]

On 8/17/20 7:01 AM, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> 
> Move LOG_DIS log to before sleeping handling so that it logs
> for sleep instructions aswell.
> 
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target/microblaze/translate.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v1 3/5] target/microblaze: mbar: Add support for data-access barriers

[Richard Henderson]

On 8/17/20 7:01 AM, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> 
> Add support for data-access barriers.
> 
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target/microblaze/translate.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
> index c1be76d4c8..c58f334a0f 100644
> --- a/target/microblaze/translate.c
> +++ b/target/microblaze/translate.c
> @@ -1233,6 +1233,11 @@ static void dec_br(DisasContext *dc)
>  
>          LOG_DIS("mbar %d\n", mbar_imm);
>  
> +        /* Data access memory barrier.  */
> +        if ((mbar_imm & 2) == 0) {
> +            tcg_gen_mb(TCG_BAR_SC | TCG_MO_ALL);
> +        }
> +
>          /* mbar IMM & 16 decodes to sleep.  */
>          if (mbar_imm & 16) {
>              TCGv_i32 tmp_hlt = tcg_const_i32(EXCP_HLT);
> 

The patch as written is fine, so
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

However, a couple of other notes for mbar:

(1) mbar_imm & 1 is insn memory barrier.  For ARM, we do:

    /*
     * We need to break the TB after this insn to execute
     * self-modifying code correctly and also to take
     * any pending interrupts immediately.
     */
    gen_goto_tb(s, 0, s->base.pc_next);

(2) mbar_imm & 16 (sleep) should check for user-mode and generate
    an illegal instruction.


r~

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

[Richard Henderson]

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~

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

[Edgar E. Iglesias]

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

Re: [PATCH v1 5/5] configure: microblaze: Enable mttcg

[Alistair Francis]

On Mon, Aug 17, 2020 at 7:03 AM Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
>
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

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

Alistair

> ---
>  configure | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/configure b/configure
> index 2acc4d1465..2f7adaa6ae 100755
> --- a/configure
> +++ b/configure
> @@ -8162,6 +8162,7 @@ case "$target_name" in
>    microblaze|microblazeel)
>      TARGET_ARCH=microblaze
>      TARGET_SYSTBL_ABI=common
> +    mttcg="yes"
>      bflt="yes"
>      echo "TARGET_ABI32=y" >> $config_target_mak
>    ;;
> --
> 2.25.1
>
>

Re: [PATCH v1 1/5] target/microblaze: mbar: Transfer dc->rd to mbar_imm

[Alistair Francis]

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>
>
> Transfer dc->rd to mbar_imm to improve the readability when
> comparing to the specs.
>
> No functional change.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

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

Alistair

> ---
>  target/microblaze/translate.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
> index f6ff2591c3..47637f152b 100644
> --- a/target/microblaze/translate.c
> +++ b/target/microblaze/translate.c
> @@ -1229,8 +1229,10 @@ static void dec_br(DisasContext *dc)
>      /* Memory barrier.  */
>      mbar = (dc->ir >> 16) & 31;
>      if (mbar == 2 && dc->imm == 4) {
> +        uint16_t mbar_imm = dc->rd;
> +
>          /* mbar IMM & 16 decodes to sleep.  */
> -        if (dc->rd & 16) {
> +        if (mbar_imm & 16) {
>              TCGv_i32 tmp_hlt = tcg_const_i32(EXCP_HLT);
>              TCGv_i32 tmp_1 = tcg_const_i32(1);
>
> @@ -1246,7 +1248,7 @@ static void dec_br(DisasContext *dc)
>              tcg_temp_free_i32(tmp_1);
>              return;
>          }
> -        LOG_DIS("mbar %d\n", dc->rd);
> +        LOG_DIS("mbar %d\n", mbar_imm);
>          /* Break the TB.  */
>          dc->cpustate_changed = 1;
>          return;
> --
> 2.25.1
>
>

Re: [PATCH v1 2/5] target/microblaze: mbar: Move LOG_DIS to before sleep

[Alistair Francis]

On Mon, Aug 17, 2020 at 7:02 AM Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
>
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Move LOG_DIS log to before sleeping handling so that it logs
> for sleep instructions aswell.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

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

Alistair

> ---
>  target/microblaze/translate.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
> index 47637f152b..c1be76d4c8 100644
> --- a/target/microblaze/translate.c
> +++ b/target/microblaze/translate.c
> @@ -1231,6 +1231,8 @@ static void dec_br(DisasContext *dc)
>      if (mbar == 2 && dc->imm == 4) {
>          uint16_t mbar_imm = dc->rd;
>
> +        LOG_DIS("mbar %d\n", mbar_imm);
> +
>          /* mbar IMM & 16 decodes to sleep.  */
>          if (mbar_imm & 16) {
>              TCGv_i32 tmp_hlt = tcg_const_i32(EXCP_HLT);
> @@ -1248,7 +1250,6 @@ static void dec_br(DisasContext *dc)
>              tcg_temp_free_i32(tmp_1);
>              return;
>          }
> -        LOG_DIS("mbar %d\n", mbar_imm);
>          /* Break the TB.  */
>          dc->cpustate_changed = 1;
>          return;
> --
> 2.25.1
>
>

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

[Alistair Francis]

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
>
>

Re: [PATCH v1 3/5] target/microblaze: mbar: Add support for data-access barriers

[Alistair Francis]

On Mon, Aug 17, 2020 at 7:02 AM Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
>
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Add support for data-access barriers.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

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

Alistair

> ---
>  target/microblaze/translate.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
> index c1be76d4c8..c58f334a0f 100644
> --- a/target/microblaze/translate.c
> +++ b/target/microblaze/translate.c
> @@ -1233,6 +1233,11 @@ static void dec_br(DisasContext *dc)
>
>          LOG_DIS("mbar %d\n", mbar_imm);
>
> +        /* Data access memory barrier.  */
> +        if ((mbar_imm & 2) == 0) {
> +            tcg_gen_mb(TCG_BAR_SC | TCG_MO_ALL);
> +        }
> +
>          /* mbar IMM & 16 decodes to sleep.  */
>          if (mbar_imm & 16) {
>              TCGv_i32 tmp_hlt = tcg_const_i32(EXCP_HLT);
> --
> 2.25.1
>
>

Re: [PATCH v1 3/5] target/microblaze: mbar: Add support for data-access barriers

[Edgar E. Iglesias]

On Mon, Aug 17, 2020 at 08:42:04AM -0700, Richard Henderson wrote:
> On 8/17/20 7:01 AM, Edgar E. Iglesias wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> > 
> > Add support for data-access barriers.
> > 
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> >  target/microblaze/translate.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
> > index c1be76d4c8..c58f334a0f 100644
> > --- a/target/microblaze/translate.c
> > +++ b/target/microblaze/translate.c
> > @@ -1233,6 +1233,11 @@ static void dec_br(DisasContext *dc)
> >  
> >          LOG_DIS("mbar %d\n", mbar_imm);
> >  
> > +        /* Data access memory barrier.  */
> > +        if ((mbar_imm & 2) == 0) {
> > +            tcg_gen_mb(TCG_BAR_SC | TCG_MO_ALL);
> > +        }
> > +
> >          /* mbar IMM & 16 decodes to sleep.  */
> >          if (mbar_imm & 16) {
> >              TCGv_i32 tmp_hlt = tcg_const_i32(EXCP_HLT);
> > 
> 
> The patch as written is fine, so
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> However, a couple of other notes for mbar:
> 
> (1) mbar_imm & 1 is insn memory barrier.  For ARM, we do:
> 
>     /*
>      * We need to break the TB after this insn to execute
>      * self-modifying code correctly and also to take
>      * any pending interrupts immediately.
>      */
>     gen_goto_tb(s, 0, s->base.pc_next);

Actually, we're already breaking the TB at the end of all mbars.
I thought of perhaps not breaking it for data-only barriers but IIRC,
we have some SW that depends on the current behavior (taking interrupts
after raised due to previous load/stores) so I left it as is.

> 
> (2) mbar_imm & 16 (sleep) should check for user-mode and generate
>     an illegal instruction.

Yes, I'll fix that in a follow-up patch!

Thanks,
Edgar