Re: [Qemu-devel] [PATCH 03/11] target-arm: Raw CPSR writes should skip checks and bank switching

[Sergey Fedorov]

On 15.02.2016 20:22, Peter Maydell wrote:
> Raw CPSR writes should skip the architectural checks for whether
> we're allowed to set the A or F bits and should also not do
> the switching of register banks if the mode changes. Handle
> this inside cpsr_write(), which allows us to drop the "manually
> set the mode bits to avoid the bank switch" code from all the
> callsites which are using CPSRWriteRaw.
>
> This fixes a bug in 32-bit KVM handling where we had forgotten
> the "manually set the mode bits" part and could thus potentially
> trash the register state if the mode from the last exit to userspace
> differed from the mode on this exit.
>
> Signed-off-by: Peter Maydell <address@hidden>

Reviewed-by: Sergey Fedorov <address@hidden>

> ---
>  target-arm/helper.c    | 5 +++--
>  target-arm/kvm64.c     | 1 -
>  target-arm/machine.c   | 2 --
>  target-arm/op_helper.c | 5 ++++-
>  4 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 828822b..d1919bb 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -5234,7 +5234,7 @@ void cpsr_write(CPUARMState *env, uint32_t val, 
> uint32_t mask,
>       * In a V8 implementation, it is permitted for privileged software to
>       * change the CPSR A/F bits regardless of the SCR.AW/FW bits.
>       */
> -    if (!arm_feature(env, ARM_FEATURE_V8) &&
> +    if (write_type != CPSRWriteRaw && !arm_feature(env, ARM_FEATURE_V8) &&
>          arm_feature(env, ARM_FEATURE_EL3) &&
>          !arm_feature(env, ARM_FEATURE_EL2) &&
>          !arm_is_secure(env)) {
> @@ -5281,7 +5281,8 @@ void cpsr_write(CPUARMState *env, uint32_t val, 
> uint32_t mask,
>      env->daif &= ~(CPSR_AIF & mask);
>      env->daif |= val & CPSR_AIF & mask;
>  
> -    if ((env->uncached_cpsr ^ val) & mask & CPSR_M) {
> +    if (write_type != CPSRWriteRaw &&
> +        ((env->uncached_cpsr ^ val) & mask & CPSR_M)) {
>          if (bad_mode_switch(env, val & CPSR_M)) {
>              /* Attempt to switch to an invalid mode: this is UNPREDICTABLE.
>               * We choose to ignore the attempt and leave the CPSR M field
> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> index 08c2c81..e8527bf 100644
> --- a/target-arm/kvm64.c
> +++ b/target-arm/kvm64.c
> @@ -722,7 +722,6 @@ int kvm_arch_get_registers(CPUState *cs)
>      if (is_a64(env)) {
>          pstate_write(env, val);
>      } else {
> -        env->uncached_cpsr = val & CPSR_M;
>          cpsr_write(env, val, 0xffffffff, CPSRWriteRaw);
>      }
>  
> diff --git a/target-arm/machine.c b/target-arm/machine.c
> index 0fc7df0..03a73d9 100644
> --- a/target-arm/machine.c
> +++ b/target-arm/machine.c
> @@ -173,8 +173,6 @@ static int get_cpsr(QEMUFile *f, void *opaque, size_t 
> size)
>          return 0;
>      }
>  
> -    /* Avoid mode switch when restoring CPSR */
> -    env->uncached_cpsr = val & CPSR_M;
>      cpsr_write(env, val, 0xffffffff, CPSRWriteRaw);
>      return 0;
>  }
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index 543d33a..4881e34 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -779,7 +779,10 @@ void HELPER(exception_return)(CPUARMState *env)
>  
>      if (!return_to_aa64) {
>          env->aarch64 = 0;
> -        env->uncached_cpsr = spsr & CPSR_M;
> +        /* We do a raw CPSR write because aarch64_sync_64_to_32()
> +         * will sort the register banks out for us, and we've already
> +         * caught all the bad-mode cases in el_from_spsr().
> +         */
>          cpsr_write(env, spsr, ~0, CPSRWriteRaw);
>          if (!arm_singlestep_active(env)) {
>              env->uncached_cpsr &= ~PSTATE_SS;