Re: [Qemu-devel] [PATCH v4 3/4] target-arm: Add 32/64-bit register sync

[Peter Maydell]

On 10 February 2015 at 10:50, Greg Bellows <address@hidden> 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 <address@hidden>
>
> ---
>
> 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?

> +
>  #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...

> + */
> +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"

> +     * 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"

> +     * 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 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