Re: [Qemu-devel] [PATCH v1 01/16] target-arm: A64: Break out aarch64_save/restore_sp

[Alex Bennée]

Edgar E. Iglesias writes:

> From: "Edgar E. Iglesias" <address@hidden>
>
> Break out code to save/restore AArch64 SP into functions.
>
> Signed-off-by: Edgar E. Iglesias <address@hidden>
> ---
>  target-arm/internals.h | 29 ++++++++++++++++++++---------
>  target-arm/kvm64.c     | 13 +++----------
>  target-arm/op_helper.c |  6 +-----
>  3 files changed, 24 insertions(+), 24 deletions(-)
>
> diff --git a/target-arm/internals.h b/target-arm/internals.h
> index 564b5fa..08fa697 100644
> --- a/target-arm/internals.h
> +++ b/target-arm/internals.h
> @@ -105,6 +105,24 @@ enum arm_fprounding {
>  
>  int arm_rmode_to_sf(int rmode);
>  
> +static inline void aarch64_save_sp(CPUARMState *env, int el)
> +{
> +    if (env->pstate & PSTATE_SP) {
> +        env->sp_el[el] = env->xregs[31];
> +    } else {
> +        env->sp_el[0] = env->xregs[31];
> +    }
> +}
> +
> +static inline void aarch64_restore_sp(CPUARMState *env, int el)
> +{
> +    if (env->pstate & PSTATE_SP) {
> +        env->xregs[31] = env->sp_el[el];
> +    } else {
> +        env->xregs[31] = env->sp_el[0];
> +    }
> +}
> +

Just a note to say I'm currently looking at rationalising
env->pstate/env->uncached_cpsr and the various access functions. However
conveniently this moves everything to one place so I approve ;-)

>  static inline void update_spsel(CPUARMState *env, uint32_t imm)
>  {
>      unsigned int cur_el = arm_current_pl(env);
> @@ -114,21 +132,14 @@ static inline void update_spsel(CPUARMState *env, 
> uint32_t imm)
>      if (!((imm ^ env->pstate) & PSTATE_SP)) {
>          return;
>      }
> +    aarch64_save_sp(env, cur_el);
>      env->pstate = deposit32(env->pstate, 0, 1, imm);
>  
>      /* We rely on illegal updates to SPsel from EL0 to get trapped
>       * at translation time.
>       */
>      assert(cur_el >= 1 && cur_el <= 3);
> -    if (env->pstate & PSTATE_SP) {
> -        /* Switch from using SP_EL0 to using SP_ELx */
> -        env->sp_el[0] = env->xregs[31];
> -        env->xregs[31] = env->sp_el[cur_el];
> -    } else {
> -        /* Switch from SP_EL0 to SP_ELx */
> -        env->sp_el[cur_el] = env->xregs[31];
> -        env->xregs[31] = env->sp_el[0];
> -    }
> +    aarch64_restore_sp(env, cur_el);
>  }
>  
>  /* Valid Syndrome Register EC field values */
> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> index 70f311b..0542cd1 100644
> --- a/target-arm/kvm64.c
> +++ b/target-arm/kvm64.c
> @@ -21,6 +21,7 @@
>  #include "sysemu/kvm.h"
>  #include "kvm_arm.h"
>  #include "cpu.h"
> +#include "internals.h"
>  #include "hw/arm/arm.h"
>  
>  static inline void set_feature(uint64_t *features, int feature)
> @@ -124,11 +125,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>      /* KVM puts SP_EL0 in regs.sp and SP_EL1 in regs.sp_el1. On the
>       * QEMU side we keep the current SP in xregs[31] as well.
>       */
> -    if (env->pstate & PSTATE_SP) {
> -        env->sp_el[1] = env->xregs[31];
> -    } else {
> -        env->sp_el[0] = env->xregs[31];
> -    }
> +    aarch64_save_sp(env, 1);
>  
>      reg.id = AARCH64_CORE_REG(regs.sp);
>      reg.addr = (uintptr_t) &env->sp_el[0];
> @@ -227,11 +224,7 @@ int kvm_arch_get_registers(CPUState *cs)
>      /* KVM puts SP_EL0 in regs.sp and SP_EL1 in regs.sp_el1. On the
>       * QEMU side we keep the current SP in xregs[31] as well.
>       */
> -    if (env->pstate & PSTATE_SP) {
> -        env->xregs[31] = env->sp_el[1];
> -    } else {
> -        env->xregs[31] = env->sp_el[0];
> -    }
> +    aarch64_restore_sp(env, 1);
>  
>      reg.id = AARCH64_CORE_REG(regs.pc);
>      reg.addr = (uintptr_t) &env->pc;
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index b28f694..2e2429a 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -391,11 +391,7 @@ void HELPER(exception_return)(CPUARMState *env)
>      uint32_t spsr = env->banked_spsr[spsr_idx];
>      int new_el, i;
>  
> -    if (env->pstate & PSTATE_SP) {
> -        env->sp_el[cur_el] = env->xregs[31];
> -    } else {
> -        env->sp_el[0] = env->xregs[31];
> -    }
> +    aarch64_save_sp(env, cur_el);
>  
>      env->exclusive_addr = -1;

Reviewed-by: Alex Bennée <address@hidden>


-- 
Alex Bennée