Re: [PATCH] target/arm: fix missing exception class

[Peter Maydell <peter.maydell@linaro.org>]

On Mon, 24 May 2021 at 13:36, Jamie Iles <jamie@nuviainc.com> wrote:
> On Mon, May 24, 2021 at 10:41:58AM +0100, Peter Maydell wrote:
> > raise_exception() and raise_exception_ra() are supposed to have
> > the same semantics apart from one of them being passed a return
> > address. So perhaps we should look at trying to fix this by
> > making raise_exception_ra() not first carefully set and then
> > very opaquely unconditionally trash env->exception.syndrome...
>
> The simplest fix for this would be the patch below and that is
> consistent with the TLB fault handling code where we first restore state
> then raise the exception, is this the sort of thing you were thinking
> of?
>
> Thanks,
>
> Jamie
>
> diff --git i/target/arm/op_helper.c w/target/arm/op_helper.c
> index efcb60099277..0b85403cb9f4 100644
> --- i/target/arm/op_helper.c
> +++ w/target/arm/op_helper.c
> @@ -63,8 +63,11 @@ void raise_exception(CPUARMState *env, uint32_t excp,
>  void raise_exception_ra(CPUARMState *env, uint32_t excp, uint32_t syndrome,
>                          uint32_t target_el, uintptr_t ra)
>  {
> -    CPUState *cs = do_raise_exception(env, excp, syndrome, target_el);
> -    cpu_loop_exit_restore(cs, ra);
> +    CPUState *cs = env_cpu(env);
> +
> +    cpu_restore_state(cs, ra, true);
> +    do_raise_exception(env, excp, syndrome, target_el);
> +    cpu_loop_exit(cs);
>  }
>

Broadly speaking, yes. I think it could be done slightly more neatly by
making raise_exception_ra() be:

    CPUState *cs = env_cpu(env);
    /*
     * restore_state_to_opc() will set env->exception.syndrome, so
     * we must restore CPU state here before setting the syndrome
     * the caller passed us, and cannot use cpu_loop_exit_restore().
     */
    cpu_restore_state(cs, ra, true);
    raise_exception(env, excp, syndrome, target_el);

Then there are some follow-up cleanup patches:
 * fold do_raise_exception() into raise_exception(), since that
   is now its only caller
 * mte_check_fail() currently does cpu_restore_state() and then
   raise_exception() with a comment about this being to avoid
   restore_state_to_opc trashing the syndrome; it can now be
   rewritten to just call raise_exception_ra()
 * the v7m_msr helper in m_helper.c does a cpu_restore_state/
   raise_exception sequence than can be rewritten to just call
   raise_exception_ra()

thanks
-- PMM