| Message ID | 20220523204742.740932-2-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: tidy exception routing | expand |
On Mon, 23 May 2022 at 21:49, Richard Henderson <richard.henderson@linaro.org> wrote: > > The work of finding the correct target EL for an exception is > currently split between raise_exception and target_exception_el. > Begin merging these by allowing the input to raise_exception > to be zero and use exception_target_el for that case. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/arm/internals.h | 11 ++++++----- > target/arm/op_helper.c | 13 +++++++++---- > 2 files changed, 15 insertions(+), 9 deletions(-) > > diff --git a/target/arm/internals.h b/target/arm/internals.h > index b654bee468..03363b0f32 100644 > --- a/target/arm/internals.h > +++ b/target/arm/internals.h > @@ -111,18 +111,19 @@ FIELD(DBGWCR, SSCE, 29, 1) > /** > * raise_exception: Raise the specified exception. > * Raise a guest exception with the specified value, syndrome register > - * and target exception level. This should be called from helper functions, > - * and never returns because we will longjump back up to the CPU main loop. > + * and the current or target exception level. This should be called from > + * helper functions, and never returns because we will longjump back up > + * to the CPU main loop. > */ > G_NORETURN void raise_exception(CPUARMState *env, uint32_t excp, > - uint32_t syndrome, uint32_t target_el); > + uint32_t syndrome, uint32_t cur_or_target_el); "cur_or_target_el" is odd, because it's mixing what the architecture sets up as two distinct things: the state the exception is "taken from", and the state the exception is "taken to". I was hoping this was just a temporary thing for the purposes of the refactoring and it would go away near the end of the series, but it doesn't seem to. -- PMM
On 5/30/22 05:44, Peter Maydell wrote: >> G_NORETURN void raise_exception(CPUARMState *env, uint32_t excp, >> - uint32_t syndrome, uint32_t target_el); >> + uint32_t syndrome, uint32_t cur_or_target_el); > > "cur_or_target_el" is odd, because it's mixing what the architecture > sets up as two distinct things: the state the exception is > "taken from", and the state the exception is "taken to". I was > hoping this was just a temporary thing for the purposes of the > refactoring and it would go away near the end of the series, but > it doesn't seem to. No, sorry. Most of the time it's cur_el, except from cpregs, where we get directed to a specific higher el. There may be some way to split the helpers... I'll have another go at this reorg this week. If it still doesn't feel cleaner, we can drop it, and I'll make some changes to the SME patch set building on this. r~
On Mon, 30 May 2022 at 17:39, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 5/30/22 05:44, Peter Maydell wrote: > >> G_NORETURN void raise_exception(CPUARMState *env, uint32_t excp, > >> - uint32_t syndrome, uint32_t target_el); > >> + uint32_t syndrome, uint32_t cur_or_target_el); > > > > "cur_or_target_el" is odd, because it's mixing what the architecture > > sets up as two distinct things: the state the exception is > > "taken from", and the state the exception is "taken to". I was > > hoping this was just a temporary thing for the purposes of the > > refactoring and it would go away near the end of the series, but > > it doesn't seem to. > > No, sorry. Most of the time it's cur_el, except from cpregs, where we get directed to a > specific higher el. There may be some way to split the helpers... > > I'll have another go at this reorg this week. If it still doesn't feel cleaner, we can > drop it, and I'll make some changes to the SME patch set building on this. I was wondering if it would work better the other way around, so that raise_exception() doesn't mess with the target_el at all, and all the "work out which EL to take the exception to" is done in target_exception_el() and similar ? -- PMM
On 5/30/22 12:01, Peter Maydell wrote: >> I'll have another go at this reorg this week. If it still doesn't feel cleaner, we can >> drop it, and I'll make some changes to the SME patch set building on this. > > I was wondering if it would work better the other way around, so that > raise_exception() doesn't mess with the target_el at all, and all the > "work out which EL to take the exception to" is done in > target_exception_el() and similar ? We need some place to put the EC_ADVSIMDFPACCESSTRAP special case, and it has to be done at the same time we're handling HCR_EL2.TGE. Perhaps that can be separated out to its own helper. r~
diff --git a/target/arm/internals.h b/target/arm/internals.h index b654bee468..03363b0f32 100644 --- a/target/arm/internals.h +++ b/target/arm/internals.h @@ -111,18 +111,19 @@ FIELD(DBGWCR, SSCE, 29, 1) /** * raise_exception: Raise the specified exception. * Raise a guest exception with the specified value, syndrome register - * and target exception level. This should be called from helper functions, - * and never returns because we will longjump back up to the CPU main loop. + * and the current or target exception level. This should be called from + * helper functions, and never returns because we will longjump back up + * to the CPU main loop. */ G_NORETURN void raise_exception(CPUARMState *env, uint32_t excp, - uint32_t syndrome, uint32_t target_el); + uint32_t syndrome, uint32_t cur_or_target_el); /* * Similarly, but also use unwinding to restore cpu state. */ G_NORETURN void raise_exception_ra(CPUARMState *env, uint32_t excp, - uint32_t syndrome, uint32_t target_el, - uintptr_t ra); + uint32_t syndrome, + uint32_t cur_or_target_el, uintptr_t ra); /* * For AArch64, map a given EL to an index in the banked_spsr array. diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c index c4bd668870..6b9141b79a 100644 --- a/target/arm/op_helper.c +++ b/target/arm/op_helper.c @@ -28,10 +28,15 @@ #define SIGNBIT (uint32_t)0x80000000 #define SIGNBIT64 ((uint64_t)1 << 63) -void raise_exception(CPUARMState *env, uint32_t excp, - uint32_t syndrome, uint32_t target_el) +void raise_exception(CPUARMState *env, uint32_t excp, uint32_t syndrome, + uint32_t cur_or_target_el) { CPUState *cs = env_cpu(env); + int target_el = cur_or_target_el; + + if (cur_or_target_el == 0) { + target_el = exception_target_el(env); + } if (target_el == 1 && (arm_hcr_el2_eff(env) & HCR_TGE)) { /* @@ -54,7 +59,7 @@ 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) + uint32_t cur_or_target_el, uintptr_t ra) { CPUState *cs = env_cpu(env); @@ -64,7 +69,7 @@ void raise_exception_ra(CPUARMState *env, uint32_t excp, uint32_t 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); + raise_exception(env, excp, syndrome, cur_or_target_el); } uint64_t HELPER(neon_tbl)(CPUARMState *env, uint32_t desc,
The work of finding the correct target EL for an exception is currently split between raise_exception and target_exception_el. Begin merging these by allowing the input to raise_exception to be zero and use exception_target_el for that case. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/internals.h | 11 ++++++----- target/arm/op_helper.c | 13 +++++++++---- 2 files changed, 15 insertions(+), 9 deletions(-)