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