| Message ID | 1422403117-16921-4-git-send-email-greg.bellows@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 27 January 2015 at 23:58, Greg Bellows <greg.bellows@linaro.org> wrote: > Add AArch32 to AArch64 register sychronization functions. > Replace manual register synchronization with new functions in > aarch64_cpu_do_interrupt() and HELPER(exception_return)(). > > Signed-off-by: Greg Bellows <greg.bellows@linaro.org> > > --- > > v2 -> v3 > - Conditionalize interrupt handler update of aarch64. > --- > target-arm/helper-a64.c | 9 +++-- > target-arm/internals.h | 89 +++++++++++++++++++++++++++++++++++++++++++++++++ > target-arm/op_helper.c | 6 ++-- > 3 files changed, 95 insertions(+), 9 deletions(-) > > diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c > index 81066ca..8c6a100 100644 > --- a/target-arm/helper-a64.c > +++ b/target-arm/helper-a64.c > @@ -448,7 +448,6 @@ void aarch64_cpu_do_interrupt(CPUState *cs) > unsigned int new_el = arm_excp_target_el(cs, cs->exception_index); > target_ulong addr = env->cp15.vbar_el[new_el]; > unsigned int new_mode = aarch64_pstate_mode(new_el, true); > - int i; > > if (arm_current_el(env) < new_el) { > if (env->aarch64) { > @@ -512,15 +511,15 @@ void aarch64_cpu_do_interrupt(CPUState *cs) > } > env->elr_el[new_el] = env->regs[15]; > > - for (i = 0; i < 15; i++) { > - env->xregs[i] = env->regs[i]; > - } > + aarch64_sync_32_to_64(env); > > env->condexec_bits = 0; > } > > pstate_write(env, PSTATE_DAIF | new_mode); > - env->aarch64 = 1; > + if (arm_feature(env, ARM_FEATURE_AARCH64)) { > + env->aarch64 = 1; > + } This doesn't look correct. If this CPU doesn't have the AArch64 feature then pretty much all of what this function does is wrong, because it is the "take exception to an EL that is using AArch64" function. We need to ensure that CPUs without the feature bit never call this, either by setting the CPU class method pointer correctly (which is what happens at the moment for the traditional 32-bit-only CPUs) or else we need to arrange that we dynamically call the right function depending on the register width of the EL we're about to take the exception to. We'll definitely need to handle that latter case for 64-bit EL3 or EL2 support (since suddenly EL1 can be 32-bit even in a CPU with 64-bit support enabled). > aarch64_restore_sp(env, new_el); > > env->pc = addr; > diff --git a/target-arm/internals.h b/target-arm/internals.h > index bb171a7..626ea7d 100644 > --- a/target-arm/internals.h > +++ b/target-arm/internals.h > @@ -128,6 +128,95 @@ static inline void aarch64_restore_sp(CPUARMState *env, int el) > } > } > > +static inline void aarch64_sync_32_to_64(CPUARMState *env) These functions are pretty big; I would just put them in a suitable .c file rather than have them be static inline in a .h file. A few lines of doc comment describing the purpose of the functions would be nice too. > +{ > + int i; > + > + /* We can blanket copy R[0:7] to X[0:7] */ > + for (i = 0; i < 8; i++) { > + env->xregs[i] = env->regs[i]; > + } > + > + /* If we are in USR mode then we just want to complete the above blanket > + * copy so we get the accurate register values. If not, then we have to go > + * to the saved and banked user regs. > + */ > + if ((env->uncached_cpsr & 0x1f) == ARM_CPU_MODE_USR) { > + for (i = 8; i < 15; i++) { > + env->xregs[i] = env->regs[i]; > + } > + } else { > + for (i = 8; i < 13; i++) { > + env->xregs[i] = env->usr_regs[i-8]; > + } > + env->xregs[13] = env->banked_r13[bank_number(ARM_CPU_MODE_USR)]; > + env->xregs[14] = env->banked_r14[bank_number(ARM_CPU_MODE_USR)]; > + } > + env->pc = env->regs[15]; > + > + env->xregs[15] = env->banked_r13[bank_number(ARM_CPU_MODE_HYP)]; > + env->xregs[16] = env->banked_r13[bank_number(ARM_CPU_MODE_IRQ)]; > + env->xregs[17] = env->banked_r14[bank_number(ARM_CPU_MODE_IRQ)]; > + env->xregs[18] = env->banked_r13[bank_number(ARM_CPU_MODE_SVC)]; > + env->xregs[19] = env->banked_r14[bank_number(ARM_CPU_MODE_SVC)]; > + env->xregs[20] = env->banked_r13[bank_number(ARM_CPU_MODE_ABT)]; > + env->xregs[21] = env->banked_r14[bank_number(ARM_CPU_MODE_ABT)]; > + env->xregs[22] = env->banked_r13[bank_number(ARM_CPU_MODE_UND)]; > + env->xregs[23] = env->banked_r14[bank_number(ARM_CPU_MODE_UND)]; This isn't going to give the right answers if we happen to be in one of these modes. For instance if we're in SVC mode then SVC's r13 (which needs to be copied into xregs[18]) is going to be in env->regs[13], not in the banked_r13 slot. > + > + for (i = 0; i < 5; i++) { > + env->xregs[24+i] = env->fiq_regs[i]; Similarly if we're in FIQ mode then the remainder of the FIQ registers are in env->regs[8] to [14], not in the fiq_regs[] or banked_r13[] or banked_r14[]. PS: spaces round operators like '+', please. > + } > + env->xregs[29] = env->banked_r13[bank_number(ARM_CPU_MODE_FIQ)]; > + env->xregs[30] = env->banked_r14[bank_number(ARM_CPU_MODE_FIQ)]; > +} > + > +static inline void aarch64_sync_64_to_32(CPUARMState *env) > +{ > + int i; > + > + /* We can blanket copy R[0:7] to X[0:7] */ > + for (i = 0; i < 8; i++) { > + env->regs[i] = env->xregs[i]; > + } > + > + /* If we are in USR mode then we want to complete the above blanket > + * copy as the XREGs will contain the most recent value. > + */ > + if ((env->uncached_cpsr & 0x1f) == ARM_CPU_MODE_USR) { use the CPSR_M mask, not 0x1f. > + for (i = 8; i < 15; i++) { > + env->regs[i] = env->xregs[i]; > + } > + } This is missing the update of regs[8..14] for modes other than USR (which is sort of the inverse of the problem in the preceding function). > + > + /* Update the user copies and banked registers so they are also up to > + * date. > + */ > + for (i = 8; i < 13; i++) { > + env->usr_regs[i-8] = env->xregs[i]; > + } > + env->banked_r13[bank_number(ARM_CPU_MODE_USR)] = env->xregs[13]; > + env->banked_r14[bank_number(ARM_CPU_MODE_USR)] = env->xregs[14]; > + > + env->regs[15] = env->pc; This feels a bit out of place -- I'd put it up with the rest of the updates of env->regs[]. > + env->banked_r13[bank_number(ARM_CPU_MODE_HYP)] = env->xregs[15]; > + env->banked_r13[bank_number(ARM_CPU_MODE_IRQ)] = env->xregs[16]; > + env->banked_r14[bank_number(ARM_CPU_MODE_IRQ)] = env->xregs[17]; > + env->banked_r13[bank_number(ARM_CPU_MODE_SVC)] = env->xregs[18]; > + env->banked_r14[bank_number(ARM_CPU_MODE_SVC)] = env->xregs[19]; > + env->banked_r13[bank_number(ARM_CPU_MODE_ABT)] = env->xregs[20]; > + env->banked_r14[bank_number(ARM_CPU_MODE_ABT)] = env->xregs[21]; > + env->banked_r13[bank_number(ARM_CPU_MODE_UND)] = env->xregs[22]; > + env->banked_r14[bank_number(ARM_CPU_MODE_UND)] = env->xregs[23]; > + > + for (i = 0; i < 5; i++) { > + env->fiq_regs[i] = env->xregs[24+i]; > + } > + env->banked_r13[bank_number(ARM_CPU_MODE_FIQ)] = env->xregs[29]; > + env->banked_r14[bank_number(ARM_CPU_MODE_FIQ)] = env->xregs[30]; > +} > + > static inline void update_spsel(CPUARMState *env, uint32_t imm) > { > unsigned int cur_el = arm_current_el(env); > diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c > index 2bed914..7713022 100644 > --- a/target-arm/op_helper.c > +++ b/target-arm/op_helper.c > @@ -465,7 +465,7 @@ void HELPER(exception_return)(CPUARMState *env) > int cur_el = arm_current_el(env); > unsigned int spsr_idx = aarch64_banked_spsr_index(cur_el); > uint32_t spsr = env->banked_spsr[spsr_idx]; > - int new_el, i; > + int new_el; > > aarch64_save_sp(env, cur_el); > > @@ -491,9 +491,7 @@ void HELPER(exception_return)(CPUARMState *env) > if (!arm_singlestep_active(env)) { > env->uncached_cpsr &= ~PSTATE_SS; > } > - for (i = 0; i < 15; i++) { > - env->regs[i] = env->xregs[i]; > - } > + aarch64_sync_64_to_32(env); > > env->regs[15] = env->elr_el[1] & ~0x1; > } else { thanks -- PMM
On Tue, Feb 3, 2015 at 12:54 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 27 January 2015 at 23:58, Greg Bellows <greg.bellows@linaro.org> wrote: > > Add AArch32 to AArch64 register sychronization functions. > > Replace manual register synchronization with new functions in > > aarch64_cpu_do_interrupt() and HELPER(exception_return)(). > > > > Signed-off-by: Greg Bellows <greg.bellows@linaro.org> > > > > --- > > > > v2 -> v3 > > - Conditionalize interrupt handler update of aarch64. > > --- > > target-arm/helper-a64.c | 9 +++-- > > target-arm/internals.h | 89 > +++++++++++++++++++++++++++++++++++++++++++++++++ > > target-arm/op_helper.c | 6 ++-- > > 3 files changed, 95 insertions(+), 9 deletions(-) > > > > diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c > > index 81066ca..8c6a100 100644 > > --- a/target-arm/helper-a64.c > > +++ b/target-arm/helper-a64.c > > @@ -448,7 +448,6 @@ void aarch64_cpu_do_interrupt(CPUState *cs) > > unsigned int new_el = arm_excp_target_el(cs, cs->exception_index); > > target_ulong addr = env->cp15.vbar_el[new_el]; > > unsigned int new_mode = aarch64_pstate_mode(new_el, true); > > - int i; > > > > if (arm_current_el(env) < new_el) { > > if (env->aarch64) { > > @@ -512,15 +511,15 @@ void aarch64_cpu_do_interrupt(CPUState *cs) > > } > > env->elr_el[new_el] = env->regs[15]; > > > > - for (i = 0; i < 15; i++) { > > - env->xregs[i] = env->regs[i]; > > - } > > + aarch64_sync_32_to_64(env); > > > > env->condexec_bits = 0; > > } > > > > pstate_write(env, PSTATE_DAIF | new_mode); > > - env->aarch64 = 1; > > + if (arm_feature(env, ARM_FEATURE_AARCH64)) { > > + env->aarch64 = 1; > > + } > > This doesn't look correct. If this CPU doesn't have the AArch64 > feature then pretty much all of what this function does is wrong, > because it is the "take exception to an EL that is using AArch64" > function. We need to ensure that CPUs without the feature > bit never call this, either by setting the CPU class method > pointer correctly (which is what happens at the moment for > the traditional 32-bit-only CPUs) or else we need to arrange that > we dynamically call the right function depending on the register > width of the EL we're about to take the exception to. We'll > definitely need to handle that latter case for 64-bit EL3 or EL2 > support (since suddenly EL1 can be 32-bit even in a CPU with > 64-bit support enabled). > > > aarch64_restore_sp(env, new_el); > > > > env->pc = addr; > > diff --git a/target-arm/internals.h b/target-arm/internals.h > > index bb171a7..626ea7d 100644 > > --- a/target-arm/internals.h > > +++ b/target-arm/internals.h > > @@ -128,6 +128,95 @@ static inline void aarch64_restore_sp(CPUARMState > *env, int el) > > } > > } > > > > +static inline void aarch64_sync_32_to_64(CPUARMState *env) > > These functions are pretty big; I would just put them in a > suitable .c file rather than have them be static inline in > a .h file. A few lines of doc comment describing the purpose > of the functions would be nice too. > Moved and commented in v4 > > > +{ > > + int i; > > + > > + /* We can blanket copy R[0:7] to X[0:7] */ > > + for (i = 0; i < 8; i++) { > > + env->xregs[i] = env->regs[i]; > > + } > > + > > + /* If we are in USR mode then we just want to complete the above > blanket > > + * copy so we get the accurate register values. If not, then we > have to go > > + * to the saved and banked user regs. > > + */ > > + if ((env->uncached_cpsr & 0x1f) == ARM_CPU_MODE_USR) { > > + for (i = 8; i < 15; i++) { > > + env->xregs[i] = env->regs[i]; > > + } > > + } else { > > + for (i = 8; i < 13; i++) { > > + env->xregs[i] = env->usr_regs[i-8]; > > + } > > + env->xregs[13] = env->banked_r13[bank_number(ARM_CPU_MODE_USR)]; > > + env->xregs[14] = env->banked_r14[bank_number(ARM_CPU_MODE_USR)]; > > + } > > + env->pc = env->regs[15]; > > + > > + env->xregs[15] = env->banked_r13[bank_number(ARM_CPU_MODE_HYP)]; > > + env->xregs[16] = env->banked_r13[bank_number(ARM_CPU_MODE_IRQ)]; > > + env->xregs[17] = env->banked_r14[bank_number(ARM_CPU_MODE_IRQ)]; > > + env->xregs[18] = env->banked_r13[bank_number(ARM_CPU_MODE_SVC)]; > > + env->xregs[19] = env->banked_r14[bank_number(ARM_CPU_MODE_SVC)]; > > + env->xregs[20] = env->banked_r13[bank_number(ARM_CPU_MODE_ABT)]; > > + env->xregs[21] = env->banked_r14[bank_number(ARM_CPU_MODE_ABT)]; > > + env->xregs[22] = env->banked_r13[bank_number(ARM_CPU_MODE_UND)]; > > + env->xregs[23] = env->banked_r14[bank_number(ARM_CPU_MODE_UND)]; > > This isn't going to give the right answers if we happen to be in > one of these modes. For instance if we're in SVC mode then SVC's > r13 (which needs to be copied into xregs[18]) is going to be > in env->regs[13], not in the banked_r13 slot. > Yes, you are correct and I dealt with this for USR mode but for some reason failed to do so for the other modes. Fixed in v4. > > > + > > + for (i = 0; i < 5; i++) { > > + env->xregs[24+i] = env->fiq_regs[i]; > > Similarly if we're in FIQ mode then the remainder of the FIQ > registers are in env->regs[8] to [14], not in the fiq_regs[] or > banked_r13[] or banked_r14[]. > > PS: spaces round operators like '+', please. > Fixed in v4 > > > + } > > + env->xregs[29] = env->banked_r13[bank_number(ARM_CPU_MODE_FIQ)]; > > + env->xregs[30] = env->banked_r14[bank_number(ARM_CPU_MODE_FIQ)]; > > +} > > + > > +static inline void aarch64_sync_64_to_32(CPUARMState *env) > > +{ > > + int i; > > + > > + /* We can blanket copy R[0:7] to X[0:7] */ > > + for (i = 0; i < 8; i++) { > > + env->regs[i] = env->xregs[i]; > > + } > > + > > + /* If we are in USR mode then we want to complete the above blanket > > + * copy as the XREGs will contain the most recent value. > > + */ > > + if ((env->uncached_cpsr & 0x1f) == ARM_CPU_MODE_USR) { > > use the CPSR_M mask, not 0x1f. > > > + for (i = 8; i < 15; i++) { > > + env->regs[i] = env->xregs[i]; > > + } > > + } > > This is missing the update of regs[8..14] for modes other than USR > (which is sort of the inverse of the problem in the preceding function). > Yes, this is due to the same omission you pointed out above. I should also point out that r8:r12 only get sync'd when in USR or FIQ mode. > > > + > > + /* Update the user copies and banked registers so they are also up > to > > + * date. > > + */ > > + for (i = 8; i < 13; i++) { > > + env->usr_regs[i-8] = env->xregs[i]; > > + } > > + env->banked_r13[bank_number(ARM_CPU_MODE_USR)] = env->xregs[13]; > > + env->banked_r14[bank_number(ARM_CPU_MODE_USR)] = env->xregs[14]; > > + > > + env->regs[15] = env->pc; > > This feels a bit out of place -- I'd put it up with the rest > of the updates of env->regs[]. > I originally put this at the end because regs[15] was the last register numerically to be updated. I think the end makes even more sense given my v4 changes. Let me know if you agree. > > > + env->banked_r13[bank_number(ARM_CPU_MODE_HYP)] = env->xregs[15]; > > + env->banked_r13[bank_number(ARM_CPU_MODE_IRQ)] = env->xregs[16]; > > + env->banked_r14[bank_number(ARM_CPU_MODE_IRQ)] = env->xregs[17]; > > + env->banked_r13[bank_number(ARM_CPU_MODE_SVC)] = env->xregs[18]; > > + env->banked_r14[bank_number(ARM_CPU_MODE_SVC)] = env->xregs[19]; > > + env->banked_r13[bank_number(ARM_CPU_MODE_ABT)] = env->xregs[20]; > > + env->banked_r14[bank_number(ARM_CPU_MODE_ABT)] = env->xregs[21]; > > + env->banked_r13[bank_number(ARM_CPU_MODE_UND)] = env->xregs[22]; > > + env->banked_r14[bank_number(ARM_CPU_MODE_UND)] = env->xregs[23]; > > + > > + for (i = 0; i < 5; i++) { > > + env->fiq_regs[i] = env->xregs[24+i]; > > + } > > + env->banked_r13[bank_number(ARM_CPU_MODE_FIQ)] = env->xregs[29]; > > + env->banked_r14[bank_number(ARM_CPU_MODE_FIQ)] = env->xregs[30]; > > +} > > + > > static inline void update_spsel(CPUARMState *env, uint32_t imm) > > { > > unsigned int cur_el = arm_current_el(env); > > diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c > > index 2bed914..7713022 100644 > > --- a/target-arm/op_helper.c > > +++ b/target-arm/op_helper.c > > @@ -465,7 +465,7 @@ void HELPER(exception_return)(CPUARMState *env) > > int cur_el = arm_current_el(env); > > unsigned int spsr_idx = aarch64_banked_spsr_index(cur_el); > > uint32_t spsr = env->banked_spsr[spsr_idx]; > > - int new_el, i; > > + int new_el; > > > > aarch64_save_sp(env, cur_el); > > > > @@ -491,9 +491,7 @@ void HELPER(exception_return)(CPUARMState *env) > > if (!arm_singlestep_active(env)) { > > env->uncached_cpsr &= ~PSTATE_SS; > > } > > - for (i = 0; i < 15; i++) { > > - env->regs[i] = env->xregs[i]; > > - } > > + aarch64_sync_64_to_32(env); > > > > env->regs[15] = env->elr_el[1] & ~0x1; > > } else { > > thanks > -- PMM >
On Tue, Feb 3, 2015 at 12:54 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 27 January 2015 at 23:58, Greg Bellows <greg.bellows@linaro.org> wrote: > > Add AArch32 to AArch64 register sychronization functions. > > Replace manual register synchronization with new functions in > > aarch64_cpu_do_interrupt() and HELPER(exception_return)(). > > > > Signed-off-by: Greg Bellows <greg.bellows@linaro.org> > > > > --- > > > > v2 -> v3 > > - Conditionalize interrupt handler update of aarch64. > > --- > > target-arm/helper-a64.c | 9 +++-- > > target-arm/internals.h | 89 > +++++++++++++++++++++++++++++++++++++++++++++++++ > > target-arm/op_helper.c | 6 ++-- > > 3 files changed, 95 insertions(+), 9 deletions(-) > > > > diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c > > index 81066ca..8c6a100 100644 > > --- a/target-arm/helper-a64.c > > +++ b/target-arm/helper-a64.c > > @@ -448,7 +448,6 @@ void aarch64_cpu_do_interrupt(CPUState *cs) > > unsigned int new_el = arm_excp_target_el(cs, cs->exception_index); > > target_ulong addr = env->cp15.vbar_el[new_el]; > > unsigned int new_mode = aarch64_pstate_mode(new_el, true); > > - int i; > > > > if (arm_current_el(env) < new_el) { > > if (env->aarch64) { > > @@ -512,15 +511,15 @@ void aarch64_cpu_do_interrupt(CPUState *cs) > > } > > env->elr_el[new_el] = env->regs[15]; > > > > - for (i = 0; i < 15; i++) { > > - env->xregs[i] = env->regs[i]; > > - } > > + aarch64_sync_32_to_64(env); > > > > env->condexec_bits = 0; > > } > > > > pstate_write(env, PSTATE_DAIF | new_mode); > > - env->aarch64 = 1; > > + if (arm_feature(env, ARM_FEATURE_AARCH64)) { > > + env->aarch64 = 1; > > + } > > This doesn't look correct. If this CPU doesn't have the AArch64 > feature then pretty much all of what this function does is wrong, > because it is the "take exception to an EL that is using AArch64" > function. We need to ensure that CPUs without the feature > bit never call this, either by setting the CPU class method > pointer correctly (which is what happens at the moment for > the traditional 32-bit-only CPUs) or else we need to arrange that > we dynamically call the right function depending on the register > width of the EL we're about to take the exception to. We'll > definitely need to handle that latter case for 64-bit EL3 or EL2 > support (since suddenly EL1 can be 32-bit even in a CPU with > 64-bit support enabled). > I think this is deeper than the one routine. Essentially, we have to convert the AArch64 CPU object into an ARM CPU object on the fly. This would have to happen after class instantiation because we don't have the properties until after. From looking at the code, I'm not convinced it is safe to override the fields, but I need to look a bit more. I wanted to understand more, so I ran without my changes to see if aarch64 ever gets cleared and sure enough it does. Is this expected? I wasn't convinced this should occur, so I investigated further. The only place I see us explicitly set it to 0 is in exception return when spsr has nRW set. So I set a watch point on the spsr, and sure enough I see an update occur setting nRW which means aarch64 gets set to 0. I realize it is possible for the PSR to get set but are we prepared to handle it? > > aarch64_restore_sp(env, new_el); > > > > env->pc = addr; > > diff --git a/target-arm/internals.h b/target-arm/internals.h > > index bb171a7..626ea7d 100644 > > --- a/target-arm/internals.h > > +++ b/target-arm/internals.h > > @@ -128,6 +128,95 @@ static inline void aarch64_restore_sp(CPUARMState > *env, int el) > > } > > } > > > > +static inline void aarch64_sync_32_to_64(CPUARMState *env) > > These functions are pretty big; I would just put them in a > suitable .c file rather than have them be static inline in > a .h file. A few lines of doc comment describing the purpose > of the functions would be nice too. > > > +{ > > + int i; > > + > > + /* We can blanket copy R[0:7] to X[0:7] */ > > + for (i = 0; i < 8; i++) { > > + env->xregs[i] = env->regs[i]; > > + } > > + > > + /* If we are in USR mode then we just want to complete the above > blanket > > + * copy so we get the accurate register values. If not, then we > have to go > > + * to the saved and banked user regs. > > + */ > > + if ((env->uncached_cpsr & 0x1f) == ARM_CPU_MODE_USR) { > > + for (i = 8; i < 15; i++) { > > + env->xregs[i] = env->regs[i]; > > + } > > + } else { > > + for (i = 8; i < 13; i++) { > > + env->xregs[i] = env->usr_regs[i-8]; > > + } > > + env->xregs[13] = env->banked_r13[bank_number(ARM_CPU_MODE_USR)]; > > + env->xregs[14] = env->banked_r14[bank_number(ARM_CPU_MODE_USR)]; > > + } > > + env->pc = env->regs[15]; > > + > > + env->xregs[15] = env->banked_r13[bank_number(ARM_CPU_MODE_HYP)]; > > + env->xregs[16] = env->banked_r13[bank_number(ARM_CPU_MODE_IRQ)]; > > + env->xregs[17] = env->banked_r14[bank_number(ARM_CPU_MODE_IRQ)]; > > + env->xregs[18] = env->banked_r13[bank_number(ARM_CPU_MODE_SVC)]; > > + env->xregs[19] = env->banked_r14[bank_number(ARM_CPU_MODE_SVC)]; > > + env->xregs[20] = env->banked_r13[bank_number(ARM_CPU_MODE_ABT)]; > > + env->xregs[21] = env->banked_r14[bank_number(ARM_CPU_MODE_ABT)]; > > + env->xregs[22] = env->banked_r13[bank_number(ARM_CPU_MODE_UND)]; > > + env->xregs[23] = env->banked_r14[bank_number(ARM_CPU_MODE_UND)]; > > This isn't going to give the right answers if we happen to be in > one of these modes. For instance if we're in SVC mode then SVC's > r13 (which needs to be copied into xregs[18]) is going to be > in env->regs[13], not in the banked_r13 slot. > > > + > > + for (i = 0; i < 5; i++) { > > + env->xregs[24+i] = env->fiq_regs[i]; > > Similarly if we're in FIQ mode then the remainder of the FIQ > registers are in env->regs[8] to [14], not in the fiq_regs[] or > banked_r13[] or banked_r14[]. > > PS: spaces round operators like '+', please. > > > + } > > + env->xregs[29] = env->banked_r13[bank_number(ARM_CPU_MODE_FIQ)]; > > + env->xregs[30] = env->banked_r14[bank_number(ARM_CPU_MODE_FIQ)]; > > +} > > + > > +static inline void aarch64_sync_64_to_32(CPUARMState *env) > > +{ > > + int i; > > + > > + /* We can blanket copy R[0:7] to X[0:7] */ > > + for (i = 0; i < 8; i++) { > > + env->regs[i] = env->xregs[i]; > > + } > > + > > + /* If we are in USR mode then we want to complete the above blanket > > + * copy as the XREGs will contain the most recent value. > > + */ > > + if ((env->uncached_cpsr & 0x1f) == ARM_CPU_MODE_USR) { > > use the CPSR_M mask, not 0x1f. > > > + for (i = 8; i < 15; i++) { > > + env->regs[i] = env->xregs[i]; > > + } > > + } > > This is missing the update of regs[8..14] for modes other than USR > (which is sort of the inverse of the problem in the preceding function). > > > + > > + /* Update the user copies and banked registers so they are also up > to > > + * date. > > + */ > > + for (i = 8; i < 13; i++) { > > + env->usr_regs[i-8] = env->xregs[i]; > > + } > > + env->banked_r13[bank_number(ARM_CPU_MODE_USR)] = env->xregs[13]; > > + env->banked_r14[bank_number(ARM_CPU_MODE_USR)] = env->xregs[14]; > > + > > + env->regs[15] = env->pc; > > This feels a bit out of place -- I'd put it up with the rest > of the updates of env->regs[]. > > > + env->banked_r13[bank_number(ARM_CPU_MODE_HYP)] = env->xregs[15]; > > + env->banked_r13[bank_number(ARM_CPU_MODE_IRQ)] = env->xregs[16]; > > + env->banked_r14[bank_number(ARM_CPU_MODE_IRQ)] = env->xregs[17]; > > + env->banked_r13[bank_number(ARM_CPU_MODE_SVC)] = env->xregs[18]; > > + env->banked_r14[bank_number(ARM_CPU_MODE_SVC)] = env->xregs[19]; > > + env->banked_r13[bank_number(ARM_CPU_MODE_ABT)] = env->xregs[20]; > > + env->banked_r14[bank_number(ARM_CPU_MODE_ABT)] = env->xregs[21]; > > + env->banked_r13[bank_number(ARM_CPU_MODE_UND)] = env->xregs[22]; > > + env->banked_r14[bank_number(ARM_CPU_MODE_UND)] = env->xregs[23]; > > + > > + for (i = 0; i < 5; i++) { > > + env->fiq_regs[i] = env->xregs[24+i]; > > + } > > + env->banked_r13[bank_number(ARM_CPU_MODE_FIQ)] = env->xregs[29]; > > + env->banked_r14[bank_number(ARM_CPU_MODE_FIQ)] = env->xregs[30]; > > +} > > + > > static inline void update_spsel(CPUARMState *env, uint32_t imm) > > { > > unsigned int cur_el = arm_current_el(env); > > diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c > > index 2bed914..7713022 100644 > > --- a/target-arm/op_helper.c > > +++ b/target-arm/op_helper.c > > @@ -465,7 +465,7 @@ void HELPER(exception_return)(CPUARMState *env) > > int cur_el = arm_current_el(env); > > unsigned int spsr_idx = aarch64_banked_spsr_index(cur_el); > > uint32_t spsr = env->banked_spsr[spsr_idx]; > > - int new_el, i; > > + int new_el; > > > > aarch64_save_sp(env, cur_el); > > > > @@ -491,9 +491,7 @@ void HELPER(exception_return)(CPUARMState *env) > > if (!arm_singlestep_active(env)) { > > env->uncached_cpsr &= ~PSTATE_SS; > > } > > - for (i = 0; i < 15; i++) { > > - env->regs[i] = env->xregs[i]; > > - } > > + aarch64_sync_64_to_32(env); > > > > env->regs[15] = env->elr_el[1] & ~0x1; > > } else { > > thanks > -- PMM >
On 4 February 2015 at 23:37, Greg Bellows <greg.bellows@linaro.org> wrote: > Peter Maydell wrote: >> This doesn't look correct. If this CPU doesn't have the AArch64 >> feature then pretty much all of what this function does is wrong, >> because it is the "take exception to an EL that is using AArch64" >> function. > I think this is deeper than the one routine. Essentially, we have to > convert the AArch64 CPU object into an ARM CPU object on the fly. This would > have to happen after class instantiation because we don't have the > properties until after. From looking at the code, I'm not convinced it is > safe to override the fields, but I need to look a bit more. I think we probably want the other approach -- leave the object being the class it is, but the functions have to cope with the CPU possibly being in 32 bit mode. We need that anyway for interworking between 64 bit EL3 and 32 bit EL1. > I wanted to understand more, so I ran without my changes to see if aarch64 > ever gets cleared and sure enough it does. Is this expected? I'm not sure what you mean here. > I wasn't convinced this should occur, so I investigated further. The only > place I see us explicitly set it to 0 is in exception return when spsr has > nRW set. So I set a watch point on the spsr, and sure enough I see an update > occur setting nRW which means aarch64 gets set to 0. This sounds like "exception return to a 32-bit EL0", which obviously clears the aarch64 flag. > I realize it is possible for the PSR to get set but are we prepared to > handle it? To handle what? At the moment the code assumes that if we're a 64 bit CPU then we can only be in 32 bit mode for an AArch32 EL0. This obviously needs to be fixed to support both 32-bit EL1 under 64-bit EL3 and also in this case to support a 64-bit CPU that only has 32-bit EL1. My original point is that the change to aarch64_cpu_do_interrupt() as you have it in this patch can't possibly work, because it is not actually doing what you need to do to handle taking exceptions to a 32 bit EL. -- PMM
diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c index 81066ca..8c6a100 100644 --- a/target-arm/helper-a64.c +++ b/target-arm/helper-a64.c @@ -448,7 +448,6 @@ void aarch64_cpu_do_interrupt(CPUState *cs) unsigned int new_el = arm_excp_target_el(cs, cs->exception_index); target_ulong addr = env->cp15.vbar_el[new_el]; unsigned int new_mode = aarch64_pstate_mode(new_el, true); - int i; if (arm_current_el(env) < new_el) { if (env->aarch64) { @@ -512,15 +511,15 @@ void aarch64_cpu_do_interrupt(CPUState *cs) } env->elr_el[new_el] = env->regs[15]; - for (i = 0; i < 15; i++) { - env->xregs[i] = env->regs[i]; - } + aarch64_sync_32_to_64(env); env->condexec_bits = 0; } pstate_write(env, PSTATE_DAIF | new_mode); - env->aarch64 = 1; + if (arm_feature(env, ARM_FEATURE_AARCH64)) { + env->aarch64 = 1; + } aarch64_restore_sp(env, new_el); env->pc = addr; diff --git a/target-arm/internals.h b/target-arm/internals.h index bb171a7..626ea7d 100644 --- a/target-arm/internals.h +++ b/target-arm/internals.h @@ -128,6 +128,95 @@ static inline void aarch64_restore_sp(CPUARMState *env, int el) } } +static inline void aarch64_sync_32_to_64(CPUARMState *env) +{ + int i; + + /* We can blanket copy R[0:7] to X[0:7] */ + for (i = 0; i < 8; i++) { + env->xregs[i] = env->regs[i]; + } + + /* If we are in USR mode then we just want to complete the above blanket + * copy so we get the accurate register values. If not, then we have to go + * to the saved and banked user regs. + */ + if ((env->uncached_cpsr & 0x1f) == ARM_CPU_MODE_USR) { + for (i = 8; i < 15; i++) { + env->xregs[i] = env->regs[i]; + } + } else { + for (i = 8; i < 13; i++) { + env->xregs[i] = env->usr_regs[i-8]; + } + env->xregs[13] = env->banked_r13[bank_number(ARM_CPU_MODE_USR)]; + env->xregs[14] = env->banked_r14[bank_number(ARM_CPU_MODE_USR)]; + } + env->pc = env->regs[15]; + + env->xregs[15] = env->banked_r13[bank_number(ARM_CPU_MODE_HYP)]; + env->xregs[16] = env->banked_r13[bank_number(ARM_CPU_MODE_IRQ)]; + env->xregs[17] = env->banked_r14[bank_number(ARM_CPU_MODE_IRQ)]; + env->xregs[18] = env->banked_r13[bank_number(ARM_CPU_MODE_SVC)]; + env->xregs[19] = env->banked_r14[bank_number(ARM_CPU_MODE_SVC)]; + env->xregs[20] = env->banked_r13[bank_number(ARM_CPU_MODE_ABT)]; + env->xregs[21] = env->banked_r14[bank_number(ARM_CPU_MODE_ABT)]; + env->xregs[22] = env->banked_r13[bank_number(ARM_CPU_MODE_UND)]; + env->xregs[23] = env->banked_r14[bank_number(ARM_CPU_MODE_UND)]; + + for (i = 0; i < 5; i++) { + env->xregs[24+i] = env->fiq_regs[i]; + } + env->xregs[29] = env->banked_r13[bank_number(ARM_CPU_MODE_FIQ)]; + env->xregs[30] = env->banked_r14[bank_number(ARM_CPU_MODE_FIQ)]; +} + +static inline void aarch64_sync_64_to_32(CPUARMState *env) +{ + int i; + + /* We can blanket copy R[0:7] to X[0:7] */ + for (i = 0; i < 8; i++) { + env->regs[i] = env->xregs[i]; + } + + /* If we are in USR mode then we want to complete the above blanket + * copy as the XREGs will contain the most recent value. + */ + if ((env->uncached_cpsr & 0x1f) == ARM_CPU_MODE_USR) { + for (i = 8; i < 15; i++) { + env->regs[i] = env->xregs[i]; + } + } + + /* Update the user copies and banked registers so they are also up to + * date. + */ + for (i = 8; i < 13; i++) { + env->usr_regs[i-8] = env->xregs[i]; + } + env->banked_r13[bank_number(ARM_CPU_MODE_USR)] = env->xregs[13]; + env->banked_r14[bank_number(ARM_CPU_MODE_USR)] = env->xregs[14]; + + env->regs[15] = env->pc; + + env->banked_r13[bank_number(ARM_CPU_MODE_HYP)] = env->xregs[15]; + env->banked_r13[bank_number(ARM_CPU_MODE_IRQ)] = env->xregs[16]; + env->banked_r14[bank_number(ARM_CPU_MODE_IRQ)] = env->xregs[17]; + env->banked_r13[bank_number(ARM_CPU_MODE_SVC)] = env->xregs[18]; + env->banked_r14[bank_number(ARM_CPU_MODE_SVC)] = env->xregs[19]; + env->banked_r13[bank_number(ARM_CPU_MODE_ABT)] = env->xregs[20]; + env->banked_r14[bank_number(ARM_CPU_MODE_ABT)] = env->xregs[21]; + env->banked_r13[bank_number(ARM_CPU_MODE_UND)] = env->xregs[22]; + env->banked_r14[bank_number(ARM_CPU_MODE_UND)] = env->xregs[23]; + + for (i = 0; i < 5; i++) { + env->fiq_regs[i] = env->xregs[24+i]; + } + env->banked_r13[bank_number(ARM_CPU_MODE_FIQ)] = env->xregs[29]; + env->banked_r14[bank_number(ARM_CPU_MODE_FIQ)] = env->xregs[30]; +} + static inline void update_spsel(CPUARMState *env, uint32_t imm) { unsigned int cur_el = arm_current_el(env); diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c index 2bed914..7713022 100644 --- a/target-arm/op_helper.c +++ b/target-arm/op_helper.c @@ -465,7 +465,7 @@ void HELPER(exception_return)(CPUARMState *env) int cur_el = arm_current_el(env); unsigned int spsr_idx = aarch64_banked_spsr_index(cur_el); uint32_t spsr = env->banked_spsr[spsr_idx]; - int new_el, i; + int new_el; aarch64_save_sp(env, cur_el); @@ -491,9 +491,7 @@ void HELPER(exception_return)(CPUARMState *env) if (!arm_singlestep_active(env)) { env->uncached_cpsr &= ~PSTATE_SS; } - for (i = 0; i < 15; i++) { - env->regs[i] = env->xregs[i]; - } + aarch64_sync_64_to_32(env); env->regs[15] = env->elr_el[1] & ~0x1; } else {
Add AArch32 to AArch64 register sychronization functions. Replace manual register synchronization with new functions in aarch64_cpu_do_interrupt() and HELPER(exception_return)(). Signed-off-by: Greg Bellows <greg.bellows@linaro.org> --- v2 -> v3 - Conditionalize interrupt handler update of aarch64. --- target-arm/helper-a64.c | 9 +++-- target-arm/internals.h | 89 +++++++++++++++++++++++++++++++++++++++++++++++++ target-arm/op_helper.c | 6 ++-- 3 files changed, 95 insertions(+), 9 deletions(-)