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

[Peter Maydell]

On Tue, 8 Jun 2021 at 00:33, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> 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.

I'll use tcg_const_i32(), yep. (I think I copied this absent-mindedly
from some of the existing code in translate.c that uses tcg_gen_movi_i32().)
Can't use tcg_constant_i32() because store_cpu_field() wants to
tcg_temp_free_i32() its argument.

> > +    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.

Strictly speaking it doesn't, because gen_intermediate_code
zero-initializes the DisasContext with a "{ }" struct initializer.
But it's clearer to explicitly initialize here I guess. Fixed.

> >       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.

There's quite a wide separation between where we set insn_start and here
(we set insn_start in arm_tr_insn_start, then there's whatever the accel/tcg
framework chooses to do between the insn_start callback and the translate_insn
callback, then the arm_pre_translate_insn() code). So I felt that a separate
pointer was easier to reason about.

In fact, looking again at the accel/tcg code, if we rewind to insn_start
that will delete any code emitted by the breakpoint_check hook,
anything emitted by plugin_gen_insn_start(), and anything emitted by
gen_io_start() if this is a CF_LAST_IO insn. I think we want to keep
all of those.

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

Done.

-- PMM