Re: [PATCH 62/77] target/microblaze: Try to keep imm and delay slot together

[Edgar E. Iglesias]

On Tue, Aug 25, 2020 at 01:59:35PM -0700, Richard Henderson wrote:
> If the last insn on a page is imm, or a branch with delay slot,
> then end a tb early if this has not begun the tb.  If it has
> begun the tb, then we can allow the tb to span two pages as if
> the imm plus its consumer, or branch plus delay, or imm plus
> branch plus delay, are a single insn.
> 
> If the insn in the delay slot faults, then the exception handler
> will have reset the PC to the beginning of this sequence anyway,
> via the stored D_FLAG and BIMM_FLAG bits.
> 
> Disable all of this when single-stepping.


Hi Richard,

We've got a Linux boot that fails after applying this patch.
It goes from always working to only working like 1 out of 3 times.
It fails deep in user-space so I don't have a good log for it.

I guess we'll have to review this patch more.

I can share images but the machine is (DTB) dynamic so it only runs with
Xilinx QEMU 5.1 branch (I've backported your series for testing).

Cheers,
Edgar

> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/microblaze/translate.c | 65 ++++++++++++++++++++++++++++++-----
>  1 file changed, 56 insertions(+), 9 deletions(-)
> 
> diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
> index 4675326083..fcfc1ac184 100644
> --- a/target/microblaze/translate.c
> +++ b/target/microblaze/translate.c
> @@ -530,11 +530,50 @@ static void gen_idivu(TCGv_i32 out, TCGv_i32 ina, 
> TCGv_i32 inb)
>  DO_TYPEA_CFG(idiv, use_div, true, gen_idiv)
>  DO_TYPEA_CFG(idivu, use_div, true, gen_idivu)
>  
> +/*
> + * Try to keep the current instruction with the one following.
> + * So if this insn is the last in the TB, and is not the first
> + * in the TB, and we are not singlestepping, then back up and
> + * exit the current TB.
> + */
> +static bool wait_for_next_tb(DisasContext *dc)
> +{
> +    if (dc->base.num_insns >= dc->base.max_insns
> +        && !dc->base.singlestep_enabled) {
> +        /* Also consider if this insn (e.g. brid) itself uses an imm. */
> +        int ninsns = (dc->tb_flags & IMM_FLAG ? 2 : 1);
> +
> +        /*
> +         * If this is not the first insn in the TB, back up and
> +         * start again with a new TB.
> +         */
> +        if (dc->base.num_insns > ninsns) {
> +            dc->base.pc_next -= ninsns * 4;
> +            dc->base.num_insns -= ninsns;
> +            dc->base.is_jmp = DISAS_TOO_MANY;
> +            return true;
> +        }
> +
> +        /*
> +         * Correspondingly, if this is the first insn of the TB,
> +         * then extend the TB as necessary to keep it with the
> +         * next insn.  Do this by *reducing* the number of insns
> +         * processed by this TB so that icount does fail an assertion.
> +         */
> +        if (dc->base.num_insns == ninsns) {
> +            dc->base.num_insns = 0;
> +        }
> +    }
> +    return false;
> +}
> +
>  static bool trans_imm(DisasContext *dc, arg_imm *arg)
>  {
> -    dc->ext_imm = arg->imm << 16;
> -    tcg_gen_movi_i32(cpu_imm, dc->ext_imm);
> -    dc->tb_flags_to_set = IMM_FLAG;
> +    if (!wait_for_next_tb(dc)) {
> +        dc->ext_imm = arg->imm << 16;
> +        tcg_gen_movi_i32(cpu_imm, dc->ext_imm);
> +        dc->tb_flags_to_set = IMM_FLAG;
> +    }
>      return true;
>  }
>  
> @@ -1311,12 +1350,17 @@ static void eval_cond_jmp(DisasContext *dc, TCGv_i32 
> pc_true, TCGv_i32 pc_false)
>      tcg_temp_free_i32(zero);
>  }
>  
> -static void dec_setup_dslot(DisasContext *dc)
> +static bool dec_setup_dslot(DisasContext *dc)
>  {
> +    if (wait_for_next_tb(dc)) {
> +        return true;
> +    }
> +
>      dc->tb_flags_to_set |= D_FLAG;
>      if (dc->type_b && (dc->tb_flags & IMM_FLAG)) {
>          dc->tb_flags_to_set |= BIMM_FLAG;
>      }
> +    return false;
>  }
>  
>  static void dec_bcc(DisasContext *dc)
> @@ -1327,8 +1371,8 @@ static void dec_bcc(DisasContext *dc)
>      cc = EXTRACT_FIELD(dc->ir, 21, 23);
>      dslot = dc->ir & (1 << 25);
>  
> -    if (dslot) {
> -        dec_setup_dslot(dc);
> +    if (dslot && dec_setup_dslot(dc)) {
> +        return;
>      }
>  
>      if (dc->type_b) {
> @@ -1402,9 +1446,10 @@ static void dec_br(DisasContext *dc)
>          }
>      }
>  
> -    if (dslot) {
> -        dec_setup_dslot(dc);
> +    if (dslot && dec_setup_dslot(dc)) {
> +        return;
>      }
> +
>      if (link && dc->rd) {
>          tcg_gen_movi_i32(cpu_R[dc->rd], dc->base.pc_next);
>      }
> @@ -1513,7 +1558,9 @@ static void dec_rts(DisasContext *dc)
>          return;
>      }
>  
> -    dec_setup_dslot(dc);
> +    if (dec_setup_dslot(dc)) {
> +        return;
> +    }
>  
>      if (i_bit) {
>          dc->tb_flags |= DRTI_FLAG;
> -- 
> 2.25.1
>