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

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~

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

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