Re: [PATCH v3 11/15] target/cris: Add DISAS_DBRANCH

["Edgar E. Iglesias" <edgar.iglesias@gmail.com>]

On Tue, Jun 22, 2021 at 08:48:16AM -0700, Richard Henderson wrote:
> Move delayed branch handling to tb_stop, where we can re-use other
> end-of-tb code, e.g. the evaluation of flags.  Honor single stepping.
> Validate that we aren't losing state by overwriting is_jmp.

Hi Richard,

This patch breaks my kernel boot test:

edgar@zapote:cris-axisdev88$ ./qemu-run.sh
+ MACH=-M axis-dev88
+ QEMU_BUILD_PATH=/home/edgar/src/c/qemu/build-qemu/
+ QEMU=/home/edgar/src/c/qemu/build-qemu//cris-softmmu/qemu-system-cris
+ KERNEL=-kernel kimage
+ NIC0=-netdev user,id=net0,hostfwd=tcp::2256-10.0.2.15:21 -net nic,netdev=net0
+ /home/edgar/src/c/qemu/build-qemu//cris-softmmu/qemu-system-cris -M axis-dev88 -netdev user,id=net0,hostfwd=tcp::2256-10.0.2.15:21 -net nic,netdev=net0 -serial stdio -display none -kernel kimage
Linux version 2.6.33 (edgar@edde) (gcc version 4.3.1 20080521 (prerelease) [gcc-4_3-branch revision 135713] (GCC 4.3.1 Axis release R93/1.93) ) #4 Thu Jan 13 15:11:20 CET 2011
bootconsole [early0] enabled
ROM fs in RAM, size 6946816 bytes
Setting up paging and the MMU.
Linux/CRISv32 port on ETRAX FS (C) 2003, 2004 Axis Communications AB
Built 1 zonelists in Zone order, mobility grouping on.  Total pages: 4080
Kernel command line: root=/dev/mtdblock3 init=/linuxrc rootfstype=jffs2 mmc_core.use_spi_crc=0 mmc_spi.spi_mode=3
PID hash table entries: 128 (order: -4, 512 bytes)
Dentry cache hash table entries: 4096 (order: 1, 16384 bytes)
Inode-cache hash table entries: 2048 (order: 0, 8192 bytes)
Memory: 22864k/32768k available (2260k kernel code, 9904k reserved, 504k data, 80k init)
Hierarchical RCU implementation.
NR_IRQS:80
Enabling watchdog...
Calibrating delay loop... qemu-system-cris: ../qemu/target/cris/translate.c:3236: cris_tr_translate_insn: Assertion `dc->base.is_jmp == DISAS_NEXT || dc->cpustate_changed' failed.
Aborted (core dumped)


I can share the image if you like.

Best regards,
Edgar


> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/cris/translate.c | 96 ++++++++++++++++++++++++-----------------
>  1 file changed, 56 insertions(+), 40 deletions(-)
> 
> diff --git a/target/cris/translate.c b/target/cris/translate.c
> index c9822eae4c..f58f6f2e5e 100644
> --- a/target/cris/translate.c
> +++ b/target/cris/translate.c
> @@ -61,6 +61,8 @@
>  #define DISAS_UPDATE        DISAS_TARGET_1
>  /* Cpu state was modified dynamically, excluding pc -- use npc */
>  #define DISAS_UPDATE_NEXT   DISAS_TARGET_2
> +/* PC update for delayed branch, see cpustate_changed otherwise */
> +#define DISAS_DBRANCH       DISAS_TARGET_3
>  
>  /* Used by the decoder.  */
>  #define EXTRACT_FIELD(src, start, end) \
> @@ -3228,50 +3230,22 @@ static void cris_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
>      dc->cpustate_changed |= dc->flags_x != (dc->base.tb->flags & X_FLAG);
>  
>      /*
> -     * Check for delayed branches here.  If we do it before
> -     * actually generating any host code, the simulator will just
> -     * loop doing nothing for on this program location.
> +     * All branches are delayed branches, handled immediately below.
> +     * We don't expect to see odd combinations of exit conditions.
>       */
> +    assert(dc->base.is_jmp == DISAS_NEXT || dc->cpustate_changed);
> +
>      if (dc->delayed_branch && --dc->delayed_branch == 0) {
> -        if (dc->base.tb->flags & 7) {
> -            t_gen_movi_env_TN(dslot, 0);
> -        }
> +        dc->base.is_jmp = DISAS_DBRANCH;
> +        return;
> +    }
>  
> -        if (dc->cpustate_changed) {
> -            cris_store_direct_jmp(dc);
> -        }
> -
> -        if (dc->clear_locked_irq) {
> -            dc->clear_locked_irq = 0;
> -            t_gen_movi_env_TN(locked_irq, 0);
> -        }
> -
> -        if (dc->jmp == JMP_DIRECT_CC) {
> -            TCGLabel *l1 = gen_new_label();
> -            cris_evaluate_flags(dc);
> -
> -            /* Conditional jmp.  */
> -            tcg_gen_brcondi_tl(TCG_COND_EQ, env_btaken, 0, l1);
> -            gen_goto_tb(dc, 1, dc->jmp_pc);
> -            gen_set_label(l1);
> -            gen_goto_tb(dc, 0, dc->pc);
> -            dc->base.is_jmp = DISAS_NORETURN;
> -            dc->jmp = JMP_NOJMP;
> -        } else if (dc->jmp == JMP_DIRECT) {
> -            cris_evaluate_flags(dc);
> -            gen_goto_tb(dc, 0, dc->jmp_pc);
> -            dc->base.is_jmp = DISAS_NORETURN;
> -            dc->jmp = JMP_NOJMP;
> -        } else {
> -            TCGv c = tcg_const_tl(dc->pc);
> -            t_gen_cc_jmp(env_btarget, c);
> -            tcg_temp_free(c);
> -            dc->base.is_jmp = DISAS_JUMP;
> -        }
> +    if (dc->base.is_jmp != DISAS_NEXT) {
> +        return;
>      }
>  
>      /* Force an update if the per-tb cpu state has changed.  */
> -    if (dc->base.is_jmp == DISAS_NEXT && dc->cpustate_changed) {
> +    if (dc->cpustate_changed) {
>          dc->base.is_jmp = DISAS_UPDATE_NEXT;
>          return;
>      }
> @@ -3281,8 +3255,7 @@ static void cris_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
>       * If we can detect the length of the next insn easily, we should.
>       * In the meantime, simply stop when we do cross.
>       */
> -    if (dc->base.is_jmp == DISAS_NEXT
> -        && ((dc->pc ^ dc->base.pc_first) & TARGET_PAGE_MASK) != 0) {
> +    if ((dc->pc ^ dc->base.pc_first) & TARGET_PAGE_MASK) {
>          dc->base.is_jmp = DISAS_TOO_MANY;
>      }
>  }
> @@ -3312,6 +3285,49 @@ static void cris_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
>  
>      cris_evaluate_flags(dc);
>  
> +    /* Evaluate delayed branch destination and fold to another is_jmp case. */
> +    if (is_jmp == DISAS_DBRANCH) {
> +        if (dc->base.tb->flags & 7) {
> +            t_gen_movi_env_TN(dslot, 0);
> +        }
> +
> +        switch (dc->jmp) {
> +        case JMP_DIRECT:
> +            npc = dc->jmp_pc;
> +            is_jmp = dc->cpustate_changed ? DISAS_UPDATE_NEXT : DISAS_TOO_MANY;
> +            break;
> +
> +        case JMP_DIRECT_CC:
> +            /*
> +             * Use a conditional branch if either taken or not-taken path
> +             * can use goto_tb.  If neither can, then treat it as indirect.
> +             */
> +            if (likely(!dc->base.singlestep_enabled)
> +                && likely(!dc->cpustate_changed)
> +                && (use_goto_tb(dc, dc->jmp_pc) || use_goto_tb(dc, npc))) {
> +                TCGLabel *not_taken = gen_new_label();
> +
> +                tcg_gen_brcondi_tl(TCG_COND_EQ, env_btaken, 0, not_taken);
> +                gen_goto_tb(dc, 1, dc->jmp_pc);
> +                gen_set_label(not_taken);
> +
> +                /* not-taken case handled below. */
> +                is_jmp = DISAS_TOO_MANY;
> +                break;
> +            }
> +            tcg_gen_movi_tl(env_btarget, dc->jmp_pc);
> +            /* fall through */
> +
> +        case JMP_INDIRECT:
> +            t_gen_cc_jmp(env_btarget, tcg_constant_tl(npc));
> +            is_jmp = dc->cpustate_changed ? DISAS_UPDATE : DISAS_JUMP;
> +            break;
> +
> +        default:
> +            g_assert_not_reached();
> +        }
> +    }
> +
>      if (unlikely(dc->base.singlestep_enabled)) {
>          switch (is_jmp) {
>          case DISAS_TOO_MANY:
> -- 
> 2.25.1
>