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