Re: [PATCH 64/77] target/microblaze: Convert mbar to decodetree

[Edgar E. Iglesias]

On Tue, Aug 25, 2020 at 01:59:37PM -0700, Richard Henderson wrote:
> Split this out of the normal branch instructions, as it requires
> special handling.  End the TB only for an instruction barrier.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/microblaze/insns.decode |  2 +
>  target/microblaze/translate.c  | 81 ++++++++++++++++++----------------
>  2 files changed, 45 insertions(+), 38 deletions(-)
> 
> diff --git a/target/microblaze/insns.decode b/target/microblaze/insns.decode
> index 53da2b75aa..77b073be9e 100644
> --- a/target/microblaze/insns.decode
> +++ b/target/microblaze/insns.decode
> @@ -124,6 +124,8 @@ lwea            110010 ..... ..... ..... 0001 000 0000  
> @typea
>  lwx             110010 ..... ..... ..... 1000 000 0000  @typea
>  lwi             111010 ..... ..... ................     @typeb
>  
> +mbar            101110 imm:5 00010 0000 0000 0000 0100
> +
>  mul             010000 ..... ..... ..... 000 0000 0000  @typea
>  mulh            010000 ..... ..... ..... 000 0000 0001  @typea
>  mulhu           010000 ..... ..... ..... 000 0000 0011  @typea
> diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
> index fc1c661368..a391e80fb9 100644
> --- a/target/microblaze/translate.c
> +++ b/target/microblaze/translate.c
> @@ -1166,6 +1166,48 @@ static bool trans_brki(DisasContext *dc, arg_typeb_br 
> *arg)
>      return true;
>  }
>  
> +static bool trans_mbar(DisasContext *dc, arg_mbar *arg)
> +{
> +    int mbar_imm = arg->imm;
> +
> +    /*
> +     * Instruction access memory barrier.
> +     * End the TB so that we recognize self-modified code immediately.
> +     */
> +    if (mbar_imm & 1) {
> +        dc->cpustate_changed = 1;
> +    }

This should be (mbar_imm & 1) == 0
But even with that fixed it fails some of our tests because interrupts
that should become visible within a couple of cycles after a
memory data barrier can now be delayed longer.

I think we should always break the TB.

> +    /* Data access memory barrier.  */
> +    if (mbar_imm & 2) {
> +        tcg_gen_mb(TCG_BAR_SC | TCG_MO_ALL);
> +    }

This should be (mbar_imm & 2) == 0


> +
> +    /* Sleep. */
> +    if (mbar_imm & 16) {
> +        TCGv_i32 tmp_1;
> +
> +        if (trap_userspace(dc, true)) {
> +            /* Sleep is a privileged instruction.  */
> +            return true;
> +        }
> +
> +        t_sync_flags(dc);
> +
> +        tmp_1 = tcg_const_i32(1);
> +        tcg_gen_st_i32(tmp_1, cpu_env,
> +                       -offsetof(MicroBlazeCPU, env)
> +                       +offsetof(CPUState, halted));
> +        tcg_temp_free_i32(tmp_1);
> +
> +        tcg_gen_movi_i32(cpu_pc, dc->base.pc_next + 4);
> +
> +        gen_raise_exception(dc, EXCP_HLT);
> +    }
> +    return true;
> +}
> +
> +
>  static void msr_read(DisasContext *dc, TCGv_i32 d)
>  {
>      TCGv_i32 t;
> @@ -1447,50 +1489,13 @@ static void dec_bcc(DisasContext *dc)
>  
>  static void dec_br(DisasContext *dc)
>  {
> -    unsigned int dslot, link, abs, mbar;
> +    unsigned int dslot, link, abs;
>      uint32_t add_pc;
>  
>      dslot = dc->ir & (1 << 20);
>      abs = dc->ir & (1 << 19);
>      link = dc->ir & (1 << 18);
>  
> -    /* Memory barrier.  */
> -    mbar = (dc->ir >> 16) & 31;
> -    if (mbar == 2 && dc->imm == 4) {
> -        uint16_t mbar_imm = dc->rd;
> -
> -        /* 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_1;
> -
> -            if (trap_userspace(dc, true)) {
> -                /* Sleep is a privileged instruction.  */
> -                return;
> -            }
> -
> -            t_sync_flags(dc);
> -
> -            tmp_1 = tcg_const_i32(1);
> -            tcg_gen_st_i32(tmp_1, cpu_env,
> -                           -offsetof(MicroBlazeCPU, env)
> -                           +offsetof(CPUState, halted));
> -            tcg_temp_free_i32(tmp_1);
> -
> -            tcg_gen_movi_i32(cpu_pc, dc->base.pc_next + 4);
> -
> -            gen_raise_exception(dc, EXCP_HLT);
> -            return;
> -        }
> -        /* Break the TB.  */
> -        dc->cpustate_changed = 1;
> -        return;
> -    }
> -
>      if (dslot && dec_setup_dslot(dc)) {
>          return;
>      }
> -- 
> 2.25.1
>