On Tue, Feb 10, 2015 at 10:13 PM, Peter Maydell <peter.mayd...@linaro.org>
wrote:
> On 10 February 2015 at 10:50, Greg Bellows <greg.bell...@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.bell...@linaro.org>
> >
> > ---
> >
> > v3 -> v4
> > - Rework sync routines to cover various exception levels
> > - Move sync routines to helper.c
> >
> > v2 -> v3
> > - Conditionalize interrupt handler update of aarch64.
> > ---
> > target-arm/cpu.h | 2 +
> > target-arm/helper-a64.c | 5 +-
> > target-arm/helper.c | 181
> ++++++++++++++++++++++++++++++++++++++++++++++++
> > target-arm/op_helper.c | 6 +-
> > 4 files changed, 186 insertions(+), 8 deletions(-)
> >
> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> > index 1830a12..11845a6 100644
> > --- a/target-arm/cpu.h
> > +++ b/target-arm/cpu.h
> > @@ -495,6 +495,8 @@ typedef struct CPUARMState {
> > ARMCPU *cpu_arm_init(const char *cpu_model);
> > int cpu_arm_exec(CPUARMState *s);
> > uint32_t do_arm_semihosting(CPUARMState *env);
> > +void aarch64_sync_32_to_64(CPUARMState *env);
> > +void aarch64_sync_64_to_32(CPUARMState *env);
> >
> > static inline bool is_a64(CPUARMState *env)
> > {
> > diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
> > index 8aa40e9..7e0d038 100644
> > --- a/target-arm/helper-a64.c
> > +++ b/target-arm/helper-a64.c
> > @@ -466,7 +466,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) {
> > @@ -530,9 +529,7 @@ 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;
> > }
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index 1a1a005..c1d6764 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -4096,6 +4096,15 @@ unsigned int arm_excp_target_el(CPUState *cs,
> unsigned int excp_idx)
> > return 1;
> > }
> >
> > +void aarch64_sync_64_to_32(CPUARMState *env)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < 15; i++) {
> > + env->regs[i] = env->xregs[i];
> > + }
> > +}
>
> This is inside CONFIG_USER_ONLY, right? Is it called at all?
> If so, when?
>
The exception_return helper calls the function so I had to either add a
USER_CONFIG version of wrap the call in exception return with
CONFIG_USER_ONLY. I chose the former, but either would work. As you would
already know, the exception_return helper is likely not getting called in a
USER_ONLY build.
>
> > +
> > #else
> >
> > /* Map CPU modes onto saved register banks. */
> > @@ -4425,6 +4434,178 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
> > env->thumb = addr & 1;
> > }
> >
> > +/* Function used to synchronize QEMU's AArch64 register set with AArch32
> > + * register set. This is necessary when switching between AArch32 and
> AArch64
> > + * execution state.
>
> This is a bit vague...
>
I'll elaborate.
>
> > + */
> > +void aarch64_sync_32_to_64(CPUARMState *env)
> > +{
> > + int i;
> > + uint32_t mode = env->uncached_cpsr & CPSR_M;
> > +
> > + /* We can blanket copy R[0:7] to X[0:7] */
> > + for (i = 0; i < 8; i++) {
> > + env->xregs[i] = env->regs[i];
> > + }
> > +
> > + /* The latest copy of some registers depend on the current
> executing mode.
>
> "depends"
>
Fixed in next version.
>
> > + * The general purpose copy is used when the current mode
> corresponds to
> > + * the mapping for the given register. Otherwise, the banked copy
> > + * corresponding to the register mapping is used.
> > + */
> > + if (mode == 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)];
> > + }
> > +
> > + if (mode == ARM_CPU_MODE_HYP) {
> > + env->xregs[15] = env->regs[13];
> > + } else {
> > + env->xregs[15] = env->banked_r13[bank_number(ARM_CPU_MODE_HYP)];
> > + }
> > +
> > + if (mode == ARM_CPU_MODE_IRQ) {
> > + env->xregs[16] = env->regs[13];
> > + env->xregs[17] = env->regs[14];
> > + } else {
> > + env->xregs[16] = env->banked_r13[bank_number(ARM_CPU_MODE_IRQ)];
> > + env->xregs[17] = env->banked_r14[bank_number(ARM_CPU_MODE_IRQ)];
> > + }
> > +
> > + if (mode == ARM_CPU_MODE_SVC) {
> > + env->xregs[18] = env->regs[13];
> > + env->xregs[19] = env->regs[14];
> > + } else {
> > + env->xregs[18] = env->banked_r13[bank_number(ARM_CPU_MODE_SVC)];
> > + env->xregs[19] = env->banked_r14[bank_number(ARM_CPU_MODE_SVC)];
> > + }
> > +
> > + if (mode == ARM_CPU_MODE_ABT) {
> > + env->xregs[20] = env->regs[13];
> > + env->xregs[21] = env->regs[14];
> > + } else {
> > + env->xregs[20] = env->banked_r13[bank_number(ARM_CPU_MODE_ABT)];
> > + env->xregs[21] = env->banked_r14[bank_number(ARM_CPU_MODE_ABT)];
> > + }
> > +
> > + if (mode == ARM_CPU_MODE_UND) {
> > + env->xregs[22] = env->regs[13];
> > + env->xregs[23] = env->regs[14];
> > + } else {
> > + env->xregs[22] = env->banked_r13[bank_number(ARM_CPU_MODE_UND)];
> > + env->xregs[23] = env->banked_r14[bank_number(ARM_CPU_MODE_UND)];
> > + }
> > +
> > + if (mode == ARM_CPU_MODE_FIQ) {
> > + for (i = 24; i < 31; i++) {
> > + env->xregs[i] = env->regs[i - 16]; /* X[24:30] -> R[8:14]
> */
> > + }
> > + } else {
> > + for (i = 24; i < 29; i++) {
> > + env->xregs[i] = env->fiq_regs[i - 24];
> > + }
> > + env->xregs[29] = env->banked_r13[bank_number(ARM_CPU_MODE_FIQ)];
> > + env->xregs[30] = env->banked_r14[bank_number(ARM_CPU_MODE_FIQ)];
> > + }
> > +
> > + env->pc = env->regs[15];
> > +}
> > +
> > +/* Function used to synchronize QEMU's AArch32 register set with AArch64
> > + * register set. This is necessary when switching between AArch32 and
> AArch64
> > + * execution state.
> > + */
> > +void aarch64_sync_64_to_32(CPUARMState *env)
> > +{
> > + int i;
> > + uint32_t mode = env->uncached_cpsr & CPSR_M;
> > +
> > + /* We can blanket copy X[0:7] to R[0:7] */
> > + for (i = 0; i < 8; i++) {
> > + env->regs[i] = env->xregs[i];
> > + }
> > +
> > + /* The destination of some registers depend on the current
> executing mode.
>
> "depends"
>
Fixed in next version.
>
> > + * The AArch32 registers 8-12 are only sync'd when we are in USR or
> FIQ
> > + * mode as they are the only modes where AArch64 registers map to
> these
> > + * registers. In all other cases, the respective USR and FIQ banks
> are
> > + * sync'd.
> > + * The AArch32 registers 13 & 14 are sync'd depending on the
> execution mode
> > + * we are in. If we are not in a given mode, the bank
> corresponding to the
> > + * AArch64 register is sync'd.
> > + */
> > + if (mode == ARM_CPU_MODE_USR) {
> > + for (i = 8; i < 15; i++) {
> > + env->regs[i] = env->xregs[i];
> > + }
>
> Something is wrong here, because we don't seem to be writing
> anything to env->regs[8..15] if mode is neither USR nor FIQ.
>
> I wrestled with this myself. As I understand it, nothing maps to
regs[8..15] unless we are in USR or FIQ, which I covered. This based on
the ARM ARM xregs[8:15] are defined to specifically map to USR, Outside
of the these modes, what should be copied to regs[8..15]?
> I think you should rearrange this function. The 32->64 sync
> needs to write a value to all of xregs[0..30], and it's
> ordered such that we basically write them all in that order,
> which makes it clear we don't miss any cases.
>
> This function, on the other hand, needs to write to
> regs[0..15] and also to the banked_* registers, so
> it should be written to update them in that order,
> rather than in xregs order.
>
> You may also find it easier to always update the banked_*
> copies regardless of the current mode -- it's safe to
> write the value to them always, it just might be ignored
> if we're currently executing for that mode.
>
> -- PMM
>
