Re: [Qemu-devel] [PATCH v21 4/7] target/avr: Add instruction translation

[Richard Henderson <richard.henderson@linaro.org>]

On 6/6/19 12:30 PM, Michael Rolnik wrote:
> +enum {
> +    BS_NONE = 0, /* Nothing special (none of the below) */
> +    BS_STOP = 1, /* We want to stop translation for any reason */
> +    BS_BRANCH = 2, /* A branch condition is reached */
> +    BS_EXCP = 3, /* An exception condition is reached */
> +};

This set of exit conditions is confused and incomplete.

(1) BS_BRANCH and BS_EXCP do the same thing, namely they
    equate exactly to DISAS_NORETURN.

(2) BS_NONE equates exactly to DISAS_NEXT.

(3) BS_STOP is probably better named DISAS_EXIT, just so
    it matches the other naming above, and that it will
    result in a call to tcg_gen_exit_tb.

(4) You're missing a case that applies to indirect branches
    which should use tcg_gen_lookup_and_goto_tb().
    I suggest this be called DISAS_LOOKUP.

> +static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
> +{
> +    TranslationBlock *tb = ctx->tb;
> +
> +    if (ctx->singlestep == 0) {
> +        tcg_gen_goto_tb(n);
> +        tcg_gen_movi_i32(cpu_pc, dest);
> +        tcg_gen_exit_tb(tb, n);
> +    } else {
> +        tcg_gen_movi_i32(cpu_pc, dest);
> +        gen_helper_debug(cpu_env);
> +        tcg_gen_exit_tb(NULL, 0);
> +    }
> +}

Should set ctx->bstate = DISAS_NORETURN here, and not to BS_BRANCH in the callers.

> +    if (avr_feature(ctx->env, AVR_FEATURE_ADIW_SBIW) == false) {
> +        gen_helper_unsupported(cpu_env);
> +
> +        ctx->bstate = BS_EXCP;
> +        return true;
> +    }

It would be good to define a helper function

static bool have_feature(DisasContext *ctx, int feature)
{
    if (!avr_feature(ctx->env, feature)) {
        gen_helper_unsupported(cpu_env);
        ctx->bstate = DISAS_NORETURN;
        return false;
    }
    return true;
}

so that this pattern becomes

    if (!have_feature(ctx, AVR_FEATURE_FOO)) {
        return true;
    }
    // Lots of code
    return true;

or

    if (have_feature(ctx, AVR_FEATURE_FOO)) {
        // Just a few lines
    }
    return true;

> +/*
> + *  Returns from subroutine. The return address is loaded from the STACK.
> + *  The Stack Pointer uses a preincrement scheme during RET.
> + */
> +static bool trans_RET(DisasContext *ctx, arg_RET *a)
> +{
> +    gen_pop_ret(ctx, cpu_pc);
> +
> +    tcg_gen_exit_tb(NULL, 0);
> +
> +    ctx->bstate = BS_BRANCH;
> +    return true;
> +}

With very few exceptions, the lone use of tcg_gen_exit_tb is a bug, because you
have failed to take ctx->singlestep into account.

In this case, this is one of the indirect branch places which you should be
using DISAS_LOOKUP.

> +static bool trans_RETI(DisasContext *ctx, arg_RETI *a)
> +{
> +    gen_pop_ret(ctx, cpu_pc);
> +
> +    tcg_gen_movi_tl(cpu_If, 1);
> +
> +    tcg_gen_exit_tb(NULL, 0);
> +
> +    ctx->bstate = BS_BRANCH;
> +    return true;
> +}

Here you need to use DISAS_EXIT, because instructions that enable interrupts
also need to exit to the main loop so that we re-evaluate whether interrupts
need to be delivered.

> +    if (ctx.singlestep) {
> +        if (ctx.bstate == BS_STOP || ctx.bstate == BS_NONE) {
> +            tcg_gen_movi_tl(cpu_pc, ctx.npc);
> +        }
> +        gen_helper_debug(cpu_env);
> +        tcg_gen_exit_tb(NULL, 0);
> +    } else {
> +        switch (ctx.bstate) {
> +        case BS_STOP:
> +        case BS_NONE:
> +            gen_goto_tb(&ctx, 0, ctx.npc);
> +            break;
> +        case BS_EXCP:
> +        case BS_BRANCH:
> +            tcg_gen_exit_tb(NULL, 0);
> +            break;
> +        default:
> +            break;
> +        }
> +    }

Better written as

    switch (ctx.bstate) {
    case DISAS_NORETURN:
        break;
    case DISAS_NEXT:
    case DISAS_CHAIN:
        /* Note gen_goto_tb checks singlestep. */
        gen_goto_tb(&ctx, 0, ctx.npc);
        break;
    case DISAS_LOOKUP:
        if (!ctx.singlestep) {
            tcg_gen_lookup_and_goto_ptr();
            break;
        }
        /* fall through */
    case DISAS_EXIT:
        if (ctx.singlestep) {
            gen_helper_debug(cpu_env);
        } else {
            tcg_gen_exit_tb(NULL, 0);
        }
        break;
    default:
        g_assert_not_reached();
    }


r~