[v5,3/4] target-arm: Add 32/64-bit register sync

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>

---

v4 -> v5
- Rework sync routines a bit more.

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     | 198 ++++++++++++++++++++++++++++++++++++++++++++++++
 target-arm/op_helper.c  |   6 +-
 4 files changed, 203 insertions(+), 8 deletions(-)

On 12 February 2015 at 02:59, 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>
>
> ---
>
> v4 -> v5
> - Rework sync routines a bit more.
>
> 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     | 198 ++++++++++++++++++++++++++++++++++++++++++++++++
>  target-arm/op_helper.c  |   6 +-
>  4 files changed, 203 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..2b7c28e 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -4096,6 +4096,11 @@ unsigned int arm_excp_target_el(CPUState *cs, unsigned int excp_idx)
>      return 1;
>  }
>
> +void aarch64_sync_64_to_32(CPUARMState *env)
> +{
> +    g_assert_not_reached();
> +}
> +
>  #else
>
>  /* Map CPU modes onto saved register banks.  */
> @@ -4425,6 +4430,199 @@ 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.
> + */
> +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];
> +    }
> +
> +    /* Unless we are in FIQ mode, x8-x12 come from the active user registers
> +     * r8-r12.  Otherwise, they come from the banked user regs.
> +     */
> +    if (mode == ARM_CPU_MODE_FIQ) {
> +        for (i = 8; i < 13; i++) {
> +            env->xregs[i] = env->usr_regs[i - 8];
> +        }
> +    } else {
> +        for (i = 8; i < 13; i++) {
> +            env->xregs[i] = env->regs[i];
> +        }
> +    }
> +
> +    /* Registers x13-x23 are the various mode SP and FP registers.  The active
> +     * r13 and r14 are only copied if we are in that mode, otherwise we copy
> +     * from the mode banked copy.
> +     */
> +    if (mode == ARM_CPU_MODE_USR || mode == ARM_CPU_MODE_SYS) {
> +        env->xregs[13] = env->regs[13];
> +        env->xregs[14] = env->regs[14];
> +    } else {
> +        env->xregs[13] = env->banked_r13[bank_number(ARM_CPU_MODE_USR)];
> +        env->xregs[14] = env->banked_r14[bank_number(ARM_CPU_MODE_USR)];
> +    }

Hyp mode uses the USR mode r14, so if we're in Hyp you need to copy
regs[14] into xregs[14]. (But not r13, which has a separate banked
Hyp version, so xregs[13] and [14] need their own if () { .. } else { .. },
and can't share the same one.)

Otherwise this function is correct now.

> +
> +    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)];
> +    }
> +
> +    /* Registers x24-x30 are mapped to r8-r14 in FIQ mode.  If we are in FIQ
> +     * mode, then we can copy the active r8-r14.  Otherwise, we copy from the
> +     * FIQ banks for r8-r14.
> +     */
> +    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];
> +    }
> +
> +    /* Unless we are in FIQ mode, r8-r12 come from the active user registers

"active" here should be deleted...

> +     * x8-x12.  Otherwise, they come from the FIQ mapped x24-x30..
> +     */
> +    if (mode == ARM_CPU_MODE_FIQ) {
> +        for (i = 8; i < 13; i++) {
> +            env->regs[i] = env->xregs[i + 16];   /* X[24:30] -> R[8:12] */
> +        }
> +    } else {
> +        for (i = 8; i < 13; i++) {
> +            env->regs[i] = env->xregs[i];
> +        }
> +    }
> +
> +    /* Registers r13 & r14 are dependent on the mode we are in.  For the
> +     * current mode we actually copy from the respective x13 & x14 registers.

This comment doesn't seem to make sense.

> +     * Otherwise, we copy x13 and x14 into banked storage corresponding to the
> +     * mode.
> +     */
> +    if (mode == ARM_CPU_MODE_USR || mode == ARM_CPU_MODE_SYS) {
> +        env->regs[13] = env->xregs[13];
> +        env->regs[14] = env->xregs[14];
> +    } else {
> +        for (i = 0; i < 5; i++) {
> +            env->usr_regs[i] = env->xregs[i + 8];
> +        }
> +        env->banked_r13[bank_number(ARM_CPU_MODE_USR)] = env->xregs[13];
> +        env->banked_r14[bank_number(ARM_CPU_MODE_USR)] = env->xregs[14];
> +    }

I think a clearer way to structure this function is going to be
to have it basically proceed through the xregs[] in order.
Every xreg[] from 0 to 30 should get copied to exactly one
place (in regs, or a banked reg, or whatever). Then it's
clear that we've handled every register we need to sync and
haven't left out any cases.


> +
> +    if (mode == ARM_CPU_MODE_HYP) {
> +        env->regs[13] = env->xregs[15];
> +        env->regs[14] = env->xregs[14];
> +    } else {
> +        /* Only x15 maps to something in HYP mode, specifically r13 */
> +        env->banked_r13[bank_number(ARM_CPU_MODE_HYP)] = env->xregs[15];
> +    }
> +
> +    if (mode == ARM_CPU_MODE_IRQ) {
> +        env->regs[13] = env->xregs[16];
> +        env->regs[14] = env->xregs[17];
> +    } else {
> +        env->banked_r13[bank_number(ARM_CPU_MODE_IRQ)] = env->xregs[16];
> +        env->banked_r14[bank_number(ARM_CPU_MODE_IRQ)] = env->xregs[17];
> +    }
> +
> +    if (mode == ARM_CPU_MODE_SVC) {
> +        env->regs[13] = env->xregs[18];
> +        env->regs[14] = env->xregs[19];
> +    } else {
> +        env->banked_r13[bank_number(ARM_CPU_MODE_SVC)] = env->xregs[18];
> +        env->banked_r14[bank_number(ARM_CPU_MODE_SVC)] = env->xregs[19];
> +    }
> +
> +    if (mode == ARM_CPU_MODE_ABT) {
> +        env->regs[13] = env->xregs[20];
> +        env->regs[14] = env->xregs[21];
> +    } else {
> +        env->banked_r13[bank_number(ARM_CPU_MODE_ABT)] = env->xregs[20];
> +        env->banked_r14[bank_number(ARM_CPU_MODE_ABT)] = env->xregs[21];
> +    }
> +
> +    if (mode == ARM_CPU_MODE_UND) {
> +        env->regs[13] = env->xregs[22];
> +        env->regs[14] = env->xregs[23];
> +    } else {
> +        env->banked_r13[bank_number(ARM_CPU_MODE_UND)] = env->xregs[22];
> +        env->banked_r14[bank_number(ARM_CPU_MODE_UND)] = env->xregs[23];
> +    }
> +
> +    if (mode == ARM_CPU_MODE_FIQ) {
> +        env->regs[13] = env->xregs[29];
> +        env->regs[14] = env->xregs[30];
> +    } else {
> +        for (i = 0; i < 5; i++) {
> +            env->fiq_regs[i] = env->xregs[i + 24];
> +        }
> +        env->banked_r13[bank_number(ARM_CPU_MODE_FIQ)] = env->xregs[29];
> +        env->banked_r14[bank_number(ARM_CPU_MODE_FIQ)] = env->xregs[30];
> +    }
> +
> +    env->regs[15] = env->pc;
> +}
> +
>  /* Handle a CPU exception.  */
>  void arm_cpu_do_interrupt(CPUState *cs)
>  {
> 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 {
> --
> 1.8.3.2
>

thanks
-- PMM