Re: [PATCH 04/55] target/arm: Add handling for PSR.ECI/ICI

[Richard Henderson]

On 6/7/21 9:57 AM, Peter Maydell wrote:
> +void clear_eci_state(DisasContext *s)
> +{
> +    /*
> +     * Clear any ECI/ICI state: used when a load multiple/store
> +     * multiple insn executes.
> +     */
> +    if (s->eci) {
> +        TCGv_i32 tmp = tcg_temp_new_i32();
> +        tcg_gen_movi_i32(tmp, 0);

tcg_const_i32 or preferably tcg_constant_i32.


> +    /*
> +     * the CONDEXEC TB flags are CPSR bits [15:10][26:25]. On A-profile this
> +     * is always the IT bits. On M-profile, some of the reserved encodings
> +     * of IT are used instead to indicate either ICI or ECI, which
> +     * indicate partial progress of a restartable insn that was interrupted
> +     * partway through by an exception:
> +     *  * if CONDEXEC[3:0] != 0b0000 : CONDEXEC is IT bits
> +     *  * if CONDEXEC[3:0] == 0b0000 : CONDEXEC is ICI or ECI bits
> +     * In all cases CONDEXEC == 0 means "not in IT block or restartable
> +     * insn, behave normally".
> +     */
> +    if (condexec & 0xf) {
> +        dc->condexec_mask = (condexec & 0xf) << 1;
> +        dc->condexec_cond = condexec >> 4;
> +        dc->eci = 0;
> +    } else {
> +        dc->condexec_mask = 0;
> +        dc->condexec_cond = 0;
> +        if (arm_feature(env, ARM_FEATURE_M)) {
> +            dc->eci = condexec >> 4;
> +        }

This else leaves eci uninitialized.

>       dc->insn = insn;
>   
> +    if (dc->eci) {
> +        /*
> +         * For M-profile continuable instructions, ECI/ICI handling
> +         * falls into these cases:
> +         *  - interrupt-continuable instructions
> +         *     These are the various load/store multiple insns (both
> +         *     integer and fp). The ICI bits indicate the register
> +         *     where the load/store can resume. We make the IMPDEF
> +         *     choice to always do "instruction restart", ie ignore
> +         *     the ICI value and always execute the ldm/stm from the
> +         *     start. So all we need to do is zero PSR.ICI if the
> +         *     insn executes.
> +         *  - MVE instructions subject to beat-wise execution
> +         *     Here the ECI bits indicate which beats have already been
> +         *     executed, and we must honour this. Each insn of this
> +         *     type will handle it correctly. We will update PSR.ECI
> +         *     in the helper function for the insn (some ECI values
> +         *     mean that the following insn also has been partially
> +         *     executed).
> +         *  - Special cases which don't advance ECI
> +         *     The insns LE, LETP and BKPT leave the ECI/ICI state
> +         *     bits untouched.
> +         *  - all other insns (the common case)
> +         *     Non-zero ECI/ICI means an INVSTATE UsageFault.
> +         *     We place a rewind-marker here. Insns in the previous
> +         *     three categories will set a flag in the DisasContext.
> +         *     If the flag isn't set after we call disas_thumb_insn()
> +         *     or disas_thumb2_insn() then we know we have a "some other
> +         *     insn" case. We will rewind to the marker (ie throwing away
> +         *     all the generated code) and instead emit "take exception".
> +         */
> +        dc->eci_handled = false;

This should be done in arm_tr_init_disas_context, I think, unconditionally, 
next to eci.

> +        dc->insn_eci_rewind = tcg_last_op();

I believe that this is identical to dc->insn_start.  Certainly there does not 
seem to be any possibility of any opcodes emitted in between.

If you think we should use a different field, then initialize it to null next 
to eci/eci_handled.


r~