On Mon, 24 May 2021 at 13:36, Jamie Iles <ja...@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
