[Qemu-devel] [PATCH 00/11] target-arm: clean up cpsr_write mode changing

[Qemu-devel] [PATCH 00/11] target-arm: clean up cpsr_write mode changing

[Peter Maydell]

This patchset cleans up our usage of cpsr_write() somewhat, and in
particular its handling of attempts to change the MODE bits.
Currently we use cpsr_write() for multiple purposes:
 * writes on inbound migration and sync with KVM, where we want
   a raw "just write these bits" and have to actively mess with
   uncached_cpsr before the write to avoid it doing a register bank
   switch
 * writes caused by guest instructions MSR and CPS
 * writes caused by guest 32-bit exception returns
 * a few other things like the gdb stub

However the required handling differs for these different cases;
in particular the set of conditions which are architecturally
defined to be an illegal mode switch is different for writes due
to MSR/CPS versus writes due to exception returns.
To fix this we add an extra argument to cpsr_write() which is an
enum indicating what kind of write is being performed.

This allows us to drop the irritating "manually write the mode
bits before the call" code in KVM/migration (and fix a bug where
we missed this out in 32-bit KVM!). We can also add the various
missing conditions:
 * changes to hyp mode
 * changes from hyp mode by MSR/CPS
 * changes to Mon from Secure EL1
 * changes from Mon to NS PL1 when HCR.TGE is set

Finally we can implement the v8 behaviour of setting PSTATE.IL
for illegal mode changes.

The series also fixes the behaviour of attempted illegal mode
changes from the gdb stub -- we ignore them, but don't set PSTATE.IL.
(Previously we had a slightly weird setup where we would permit
changes from User to the PL1 modes but not to Mon.) Not permitting
illegal changes from the debugger:
 * means we don't have to consider weird "transition is possible
   but only for the debugger" corner cases
 * matches the behaviour of a JTAG debugger doing external debug
   on real ARM hardware, where architecturally the illegal state
   transitions are enforced

PS: we don't currently implement the PSTATE.IL "any attempt to
execute an insn will UNDEF" behaviour, but it would not be hard
to add.

thanks
-- PMM


Peter Maydell (11):
  target-arm: Give CPSR setting on 32-bit exception return its own
    helper
  target-arm: Add write_type argument to cpsr_write()
  target-arm: Raw CPSR writes should skip checks and bank switching
  linux-user: Use restrictive mask when calling cpsr_write()
  target-arm: In cpsr_write() ignore mode switches from User mode
  target-arm: Add comment about not implementing NSACR.RFR
  target-arm: Add Hyp mode checks to bad_mode_switch()
  target-arm: Forbid mode switch to Mon from Secure EL1
  target-arm: In v8, make illegal AArch32 mode changes set PSTATE.IL
  target-arm: Make mode switches from Hyp via CPS and MRS illegal
  target-arm: Make Monitor->NS PL1 mode changes illegal if HCR.TGE is 1

 linux-user/arm/nwfpe/fpa11.h |  2 +-
 linux-user/main.c            |  7 +++---
 linux-user/signal.c          |  4 ++--
 target-arm/cpu.h             | 13 +++++++++--
 target-arm/gdbstub.c         |  2 +-
 target-arm/helper.c          | 54 ++++++++++++++++++++++++++++++++++++--------
 target-arm/helper.h          |  1 +
 target-arm/kvm32.c           |  2 +-
 target-arm/kvm64.c           |  3 +--
 target-arm/machine.c         |  4 +---
 target-arm/op_helper.c       | 15 +++++++++---
 target-arm/translate.c       |  6 ++---
 12 files changed, 83 insertions(+), 30 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 01/11] target-arm: Give CPSR setting on 32-bit exception return its own helper

[Peter Maydell]

The rules for setting the CPSR on a 32-bit exception return are
subtly different from those for setting the CPSR via an instruction
like MSR or CPS. (In particular, in Hyp mode changing the mode bits
is not valid via MSR or CPS.) Split the exception-return case into
its own helper for setting CPSR, so we can eventually handle them
differently in the helper function.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.h    | 1 +
 target-arm/op_helper.c | 6 ++++++
 target-arm/translate.c | 6 +++---
 3 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/target-arm/helper.h b/target-arm/helper.h
index c98e9ce..ea13202 100644
--- a/target-arm/helper.h
+++ b/target-arm/helper.h
@@ -57,6 +57,7 @@ DEF_HELPER_2(pre_smc, void, env, i32)
 DEF_HELPER_1(check_breakpoints, void, env)
 
 DEF_HELPER_3(cpsr_write, void, env, i32, i32)
+DEF_HELPER_2(cpsr_write_eret, void, env, i32)
 DEF_HELPER_1(cpsr_read, i32, env)
 
 DEF_HELPER_3(v7m_msr, void, env, i32, i32)
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index 538887c..e3ddd5a 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -425,6 +425,12 @@ void HELPER(cpsr_write)(CPUARMState *env, uint32_t val, uint32_t mask)
     cpsr_write(env, val, mask);
 }
 
+/* Write the CPSR for a 32-bit exception return */
+void HELPER(cpsr_write_eret)(CPUARMState *env, uint32_t val)
+{
+    cpsr_write(env, val, CPSR_ERET_MASK);
+}
+
 /* Access to user mode registers from privileged modes.  */
 uint32_t HELPER(get_user_reg)(CPUARMState *env, uint32_t regno)
 {
diff --git a/target-arm/translate.c b/target-arm/translate.c
index e69145d..413f7de 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -4094,7 +4094,7 @@ static void gen_exception_return(DisasContext *s, TCGv_i32 pc)
     TCGv_i32 tmp;
     store_reg(s, 15, pc);
     tmp = load_cpu_field(spsr);
-    gen_set_cpsr(tmp, CPSR_ERET_MASK);
+    gen_helper_cpsr_write_eret(cpu_env, tmp);
     tcg_temp_free_i32(tmp);
     s->is_jmp = DISAS_JUMP;
 }
@@ -4102,7 +4102,7 @@ static void gen_exception_return(DisasContext *s, TCGv_i32 pc)
 /* Generate a v6 exception return.  Marks both values as dead.  */
 static void gen_rfe(DisasContext *s, TCGv_i32 pc, TCGv_i32 cpsr)
 {
-    gen_set_cpsr(cpsr, CPSR_ERET_MASK);
+    gen_helper_cpsr_write_eret(cpu_env, cpsr);
     tcg_temp_free_i32(cpsr);
     store_reg(s, 15, pc);
     s->is_jmp = DISAS_JUMP;
@@ -9094,7 +9094,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
                 if (exc_return) {
                     /* Restore CPSR from SPSR.  */
                     tmp = load_cpu_field(spsr);
-                    gen_set_cpsr(tmp, CPSR_ERET_MASK);
+                    gen_helper_cpsr_write_eret(cpu_env, tmp);
                     tcg_temp_free_i32(tmp);
                     s->is_jmp = DISAS_JUMP;
                 }
-- 
1.9.1

[Qemu-devel] [PATCH 02/11] target-arm: Add write_type argument to cpsr_write()

[Peter Maydell]

Add an argument to cpsr_write() to indicate what kind of CPSR
write is being requested, since the exact behaviour should
differ for the different cases.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/arm/nwfpe/fpa11.h |  2 +-
 linux-user/main.c            |  6 +++---
 linux-user/signal.c          |  4 ++--
 target-arm/cpu.h             | 13 +++++++++++--
 target-arm/gdbstub.c         |  2 +-
 target-arm/helper.c          |  3 ++-
 target-arm/kvm32.c           |  2 +-
 target-arm/kvm64.c           |  2 +-
 target-arm/machine.c         |  2 +-
 target-arm/op_helper.c       |  6 +++---
 10 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/linux-user/arm/nwfpe/fpa11.h b/linux-user/arm/nwfpe/fpa11.h
index bb9ac65..faa6b00 100644
--- a/linux-user/arm/nwfpe/fpa11.h
+++ b/linux-user/arm/nwfpe/fpa11.h
@@ -108,7 +108,7 @@ static inline void writeRegister(unsigned int x, unsigned int y)
 
 static inline void writeConditionCodes(unsigned int x)
 {
-        cpsr_write(user_registers,x,CPSR_NZCV);
+    cpsr_write(user_registers, x, CPSR_NZCV, CPSRWriteByInstr);
 }
 
 #define ARM_REG_PC 15
diff --git a/linux-user/main.c b/linux-user/main.c
index e719a2d..1269470 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -513,7 +513,7 @@ static void arm_kernel_cmpxchg64_helper(CPUARMState *env)
         env->regs[0] = -1;
         cpsr &= ~CPSR_C;
     }
-    cpsr_write(env, cpsr, CPSR_C);
+    cpsr_write(env, cpsr, CPSR_C, CPSRWriteByInstr);
     end_exclusive();
     return;
 
@@ -562,7 +562,7 @@ do_kernel_trap(CPUARMState *env)
             env->regs[0] = -1;
             cpsr &= ~CPSR_C;
         }
-        cpsr_write(env, cpsr, CPSR_C);
+        cpsr_write(env, cpsr, CPSR_C, CPSRWriteByInstr);
         end_exclusive();
         break;
     case 0xffff0fe0: /* __kernel_get_tls */
@@ -4446,7 +4446,7 @@ int main(int argc, char **argv, char **envp)
 #elif defined(TARGET_ARM)
     {
         int i;
-        cpsr_write(env, regs->uregs[16], 0xffffffff);
+        cpsr_write(env, regs->uregs[16], 0xffffffff, CPSRWriteByInstr);
         for(i = 0; i < 16; i++) {
             env->regs[i] = regs->uregs[i];
         }
diff --git a/linux-user/signal.c b/linux-user/signal.c
index 327c032..82f81c7 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -1611,7 +1611,7 @@ setup_return(CPUARMState *env, struct target_sigaction *ka,
 	env->regs[13] = frame_addr;
 	env->regs[14] = retcode;
 	env->regs[15] = handler & (thumb ? ~1 : ~3);
-	cpsr_write(env, cpsr, 0xffffffff);
+        cpsr_write(env, cpsr, 0xffffffff, CPSRWriteByInstr);
 }
 
 static abi_ulong *setup_sigframe_v2_vfp(abi_ulong *regspace, CPUARMState *env)
@@ -1843,7 +1843,7 @@ restore_sigcontext(CPUARMState *env, struct target_sigcontext *sc)
     __get_user(env->regs[15], &sc->arm_pc);
 #ifdef TARGET_CONFIG_CPU_32
     __get_user(cpsr, &sc->arm_cpsr);
-        cpsr_write(env, cpsr, CPSR_USER | CPSR_EXEC);
+    cpsr_write(env, cpsr, CPSR_USER | CPSR_EXEC, CPSRWriteByInstr);
 #endif
 
 	err |= !valid_user_regs(env);
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 1623821..e72e33b 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -719,8 +719,17 @@ static inline void pstate_write(CPUARMState *env, uint32_t val)
 
 /* Return the current CPSR value.  */
 uint32_t cpsr_read(CPUARMState *env);
-/* Set the CPSR.  Note that some bits of mask must be all-set or all-clear.  */
-void cpsr_write(CPUARMState *env, uint32_t val, uint32_t mask);
+
+typedef enum CPSRWriteType {
+    CPSRWriteByInstr = 0,         /* from guest MSR or CPS */
+    CPSRWriteExceptionReturn = 1, /* from guest exception return insn */
+    CPSRWriteRaw = 2,             /* trust values, do not switch reg banks */
+    CPSRWriteByGDBStub = 3,       /* from the GDB stub */
+} CPSRWriteType;
+
+/* Set the CPSR.  Note that some bits of mask must be all-set or all-clear.*/
+void cpsr_write(CPUARMState *env, uint32_t val, uint32_t mask,
+                CPSRWriteType write_type);
 
 /* Return the current xPSR value.  */
 static inline uint32_t xpsr_read(CPUARMState *env)
diff --git a/target-arm/gdbstub.c b/target-arm/gdbstub.c
index 08b91a4..3ba9aad 100644
--- a/target-arm/gdbstub.c
+++ b/target-arm/gdbstub.c
@@ -94,7 +94,7 @@ int arm_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
         return 4;
     case 25:
         /* CPSR */
-        cpsr_write(env, tmp, 0xffffffff);
+        cpsr_write(env, tmp, 0xffffffff, CPSRWriteByGDBStub);
         return 4;
     }
     /* Unknown register.  */
diff --git a/target-arm/helper.c b/target-arm/helper.c
index a420a2a..828822b 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -5199,7 +5199,8 @@ uint32_t cpsr_read(CPUARMState *env)
         | (env->GE << 16) | (env->daif & CPSR_AIF);
 }
 
-void cpsr_write(CPUARMState *env, uint32_t val, uint32_t mask)
+void cpsr_write(CPUARMState *env, uint32_t val, uint32_t mask,
+                CPSRWriteType write_type)
 {
     uint32_t changed_daif;
 
diff --git a/target-arm/kvm32.c b/target-arm/kvm32.c
index ea01932..d44a7f9 100644
--- a/target-arm/kvm32.c
+++ b/target-arm/kvm32.c
@@ -428,7 +428,7 @@ int kvm_arch_get_registers(CPUState *cs)
     if (ret) {
         return ret;
     }
-    cpsr_write(env, cpsr, 0xffffffff);
+    cpsr_write(env, cpsr, 0xffffffff, CPSRWriteRaw);
 
     /* Make sure the current mode regs are properly set */
     mode = env->uncached_cpsr & CPSR_M;
diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
index 0f1b4d6..08c2c81 100644
--- a/target-arm/kvm64.c
+++ b/target-arm/kvm64.c
@@ -723,7 +723,7 @@ int kvm_arch_get_registers(CPUState *cs)
         pstate_write(env, val);
     } else {
         env->uncached_cpsr = val & CPSR_M;
-        cpsr_write(env, val, 0xffffffff);
+        cpsr_write(env, val, 0xffffffff, CPSRWriteRaw);
     }
 
     /* KVM puts SP_EL0 in regs.sp and SP_EL1 in regs.sp_el1. On the
diff --git a/target-arm/machine.c b/target-arm/machine.c
index ed1925a..0fc7df0 100644
--- a/target-arm/machine.c
+++ b/target-arm/machine.c
@@ -175,7 +175,7 @@ static int get_cpsr(QEMUFile *f, void *opaque, size_t size)
 
     /* Avoid mode switch when restoring CPSR */
     env->uncached_cpsr = val & CPSR_M;
-    cpsr_write(env, val, 0xffffffff);
+    cpsr_write(env, val, 0xffffffff, CPSRWriteRaw);
     return 0;
 }
 
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index e3ddd5a..543d33a 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -422,13 +422,13 @@ uint32_t HELPER(cpsr_read)(CPUARMState *env)
 
 void HELPER(cpsr_write)(CPUARMState *env, uint32_t val, uint32_t mask)
 {
-    cpsr_write(env, val, mask);
+    cpsr_write(env, val, mask, CPSRWriteByInstr);
 }
 
 /* Write the CPSR for a 32-bit exception return */
 void HELPER(cpsr_write_eret)(CPUARMState *env, uint32_t val)
 {
-    cpsr_write(env, val, CPSR_ERET_MASK);
+    cpsr_write(env, val, CPSR_ERET_MASK, CPSRWriteExceptionReturn);
 }
 
 /* Access to user mode registers from privileged modes.  */
@@ -780,7 +780,7 @@ void HELPER(exception_return)(CPUARMState *env)
     if (!return_to_aa64) {
         env->aarch64 = 0;
         env->uncached_cpsr = spsr & CPSR_M;
-        cpsr_write(env, spsr, ~0);
+        cpsr_write(env, spsr, ~0, CPSRWriteRaw);
         if (!arm_singlestep_active(env)) {
             env->uncached_cpsr &= ~PSTATE_SS;
         }
-- 
1.9.1

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

[Peter Maydell]

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 <peter.maydell@linaro.org>
---
 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;
-- 
1.9.1

[Qemu-devel] [PATCH 04/11] linux-user: Use restrictive mask when calling cpsr_write()

[Peter Maydell]

When linux-user code is calling cpsr_write(), use a restrictive
mask to ensure we are limiting the set of CPSR bits we update.
In particular, don't allow the mode bits to be changed.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/main.c   | 3 ++-
 linux-user/signal.c | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/linux-user/main.c b/linux-user/main.c
index 1269470..c467fa7 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -4446,7 +4446,8 @@ int main(int argc, char **argv, char **envp)
 #elif defined(TARGET_ARM)
     {
         int i;
-        cpsr_write(env, regs->uregs[16], 0xffffffff, CPSRWriteByInstr);
+        cpsr_write(env, regs->uregs[16], CPSR_USER | CPSR_EXEC,
+                   CPSRWriteByInstr);
         for(i = 0; i < 16; i++) {
             env->regs[i] = regs->uregs[i];
         }
diff --git a/linux-user/signal.c b/linux-user/signal.c
index 82f81c7..962111c 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -1611,7 +1611,7 @@ setup_return(CPUARMState *env, struct target_sigaction *ka,
 	env->regs[13] = frame_addr;
 	env->regs[14] = retcode;
 	env->regs[15] = handler & (thumb ? ~1 : ~3);
-        cpsr_write(env, cpsr, 0xffffffff, CPSRWriteByInstr);
+        cpsr_write(env, cpsr, CPSR_IT | CPSR_T, CPSRWriteByInstr);
 }
 
 static abi_ulong *setup_sigframe_v2_vfp(abi_ulong *regspace, CPUARMState *env)
-- 
1.9.1

[Qemu-devel] [PATCH 05/11] target-arm: In cpsr_write() ignore mode switches from User mode

[Peter Maydell]

The only case where we can attempt a cpsr_write() mode switch from
User is from the gdbstub; all other cases are handled in the
calling code (notably translate.c). Architecturally attempts to
alter the mode bits from user mode are simply ignored (and not
treated as a bad mode switch, which in v8 sets CPSR.IL). Make
mode switches from User ignored in cpsr_write() as well, for
consistency.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index d1919bb..9998a25 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -5282,6 +5282,7 @@ void cpsr_write(CPUARMState *env, uint32_t val, uint32_t mask,
     env->daif |= val & CPSR_AIF & mask;
 
     if (write_type != CPSRWriteRaw &&
+        (env->uncached_cpsr & CPSR_M) != CPSR_USER &&
         ((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.
-- 
1.9.1

[Qemu-devel] [PATCH 06/11] target-arm: Add comment about not implementing NSACR.RFR

[Peter Maydell]

QEMU doesn't implement the NSACR.RFR bit, which is a permitted
IMPDEF in choice in ARMv7 and the only permitted choice in ARMv8.
Add a comment to bad_mode_switch() to note that this is why
FIQ is always a valid mode regardless of the CPU's Secure state.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 9998a25..37b5439 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -5180,6 +5180,9 @@ static int bad_mode_switch(CPUARMState *env, int mode)
     case ARM_CPU_MODE_UND:
     case ARM_CPU_MODE_IRQ:
     case ARM_CPU_MODE_FIQ:
+        /* Note that we don't implement the IMPDEF NSACR.RFR which in v7
+         * allows FIQ mode to be Secure-only. (In v8 this doesn't exist.)
+         */
         return 0;
     case ARM_CPU_MODE_MON:
         return !arm_is_secure(env);
-- 
1.9.1

[Qemu-devel] [PATCH 07/11] target-arm: Add Hyp mode checks to bad_mode_switch()

[Peter Maydell]

We don't actually support Hyp mode yet, but add the correct
checks for it to the bad_mode_switch() function for completeness.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 37b5439..4074b97 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -5184,6 +5184,9 @@ static int bad_mode_switch(CPUARMState *env, int mode)
          * allows FIQ mode to be Secure-only. (In v8 this doesn't exist.)
          */
         return 0;
+    case ARM_CPU_MODE_HYP:
+        return !arm_feature(env, ARM_FEATURE_EL2)
+            || arm_current_el(env) < 2 || arm_is_secure(env);
     case ARM_CPU_MODE_MON:
         return !arm_is_secure(env);
     default:
-- 
1.9.1

[Qemu-devel] [PATCH 08/11] target-arm: Forbid mode switch to Mon from Secure EL1

[Peter Maydell]

In v8 trying to switch mode to Mon from Secure EL1 is an
illegal mode switch. (In v7 this is impossible as all secure
modes except User are at EL3.) We can handle this case by
making a switch to Mon valid only if the current EL is 3,
which then gives the correct answer whether EL3 is AArch32
or AArch64.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 4074b97..e7b3eb3 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -5188,7 +5188,7 @@ static int bad_mode_switch(CPUARMState *env, int mode)
         return !arm_feature(env, ARM_FEATURE_EL2)
             || arm_current_el(env) < 2 || arm_is_secure(env);
     case ARM_CPU_MODE_MON:
-        return !arm_is_secure(env);
+        return arm_current_el(env) < 3;
     default:
         return 1;
     }
-- 
1.9.1

[Qemu-devel] [PATCH 09/11] target-arm: In v8, make illegal AArch32 mode changes set PSTATE.IL

[Peter Maydell]

In v8, the illegal mode changes which are UNPREDICTABLE in v7 are
given architected behaviour:
 * the mode field is unchanged
 * PSTATE.IL is set (so any subsequent instructions will UNDEF)
 * any other CPSR fields are written to as normal

This is pretty much the same behaviour we picked for our
UNPREDICTABLE handling, with the exception that for v8 we
need to set the IL bit.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index e7b3eb3..69e93a2 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -5291,11 +5291,20 @@ void cpsr_write(CPUARMState *env, uint32_t val, uint32_t mask,
         (env->uncached_cpsr & CPSR_M) != CPSR_USER &&
         ((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
-             * untouched.
+            /* Attempt to switch to an invalid mode: this is UNPREDICTABLE in
+             * v7, and has defined behaviour in v8:
+             *  + leave CPSR.M untouched
+             *  + allow changes to the other CPSR fields
+             *  + set PSTATE.IL
+             * For user changes via the GDB stub, we don't set PSTATE.IL,
+             * as this would be unnecessarily harsh for a user error.
              */
             mask &= ~CPSR_M;
+            if (write_type != CPSRWriteByGDBStub &&
+                arm_feature(env, ARM_FEATURE_V8)) {
+                mask |= CPSR_IL;
+                val |= CPSR_IL;
+            }
         } else {
             switch_mode(env, val & CPSR_M);
         }
-- 
1.9.1

[Qemu-devel] [PATCH 10/11] target-arm: Make mode switches from Hyp via CPS and MRS illegal

[Peter Maydell]

Mode switches from Hyp to any other mode via the CPS and MRS
instructions are illegal mode switches (though obviously switching
via exception return is valid).  Add this check to bad_mode_switch().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 69e93a2..e1af9d5 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -5166,12 +5166,20 @@ void arm_cp_reset_ignore(CPUARMState *env, const ARMCPRegInfo *opaque)
     /* Helper coprocessor reset function for do-nothing-on-reset registers */
 }
 
-static int bad_mode_switch(CPUARMState *env, int mode)
+static int bad_mode_switch(CPUARMState *env, int mode, CPSRWriteType write_type)
 {
     /* Return true if it is not valid for us to switch to
      * this CPU mode (ie all the UNPREDICTABLE cases in
      * the ARM ARM CPSRWriteByInstr pseudocode).
      */
+
+    /* Changes to or from Hyp via MSR and CPS are illegal. */
+    if (write_type == CPSRWriteByInstr &&
+        ((env->uncached_cpsr & CPSR_M) == ARM_CPU_MODE_HYP ||
+         mode == ARM_CPU_MODE_HYP)) {
+        return 1;
+    }
+
     switch (mode) {
     case ARM_CPU_MODE_USR:
     case ARM_CPU_MODE_SYS:
@@ -5290,7 +5298,7 @@ void cpsr_write(CPUARMState *env, uint32_t val, uint32_t mask,
     if (write_type != CPSRWriteRaw &&
         (env->uncached_cpsr & CPSR_M) != CPSR_USER &&
         ((env->uncached_cpsr ^ val) & mask & CPSR_M)) {
-        if (bad_mode_switch(env, val & CPSR_M)) {
+        if (bad_mode_switch(env, val & CPSR_M, write_type)) {
             /* Attempt to switch to an invalid mode: this is UNPREDICTABLE in
              * v7, and has defined behaviour in v8:
              *  + leave CPSR.M untouched
-- 
1.9.1

[Qemu-devel] [PATCH 11/11] target-arm: Make Monitor->NS PL1 mode changes illegal if HCR.TGE is 1

[Peter Maydell]

If HCR.TGE is 1 then mode changes via CPS and MSR from Monitor to
NonSecure PL1 modes are illegal mode changes. Implement this check
in bad_mode_switch().

(We don't currently implement HCR.TGE, but this is the only missing
check from the v8 ARM ARM G1.9.3 and so it's worth adding now; the
rest of the HCR.TGE checks can be added later as necessary.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index e1af9d5..93a0b63 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -5182,6 +5182,7 @@ static int bad_mode_switch(CPUARMState *env, int mode, CPSRWriteType write_type)
 
     switch (mode) {
     case ARM_CPU_MODE_USR:
+        return 0;
     case ARM_CPU_MODE_SYS:
     case ARM_CPU_MODE_SVC:
     case ARM_CPU_MODE_ABT:
@@ -5191,6 +5192,15 @@ static int bad_mode_switch(CPUARMState *env, int mode, CPSRWriteType write_type)
         /* Note that we don't implement the IMPDEF NSACR.RFR which in v7
          * allows FIQ mode to be Secure-only. (In v8 this doesn't exist.)
          */
+        /* If HCR.TGE is set then changes from Monitor to NS PL1 via MSR
+         * and CPS are treated as illegal mode changes.
+         */
+        if (write_type == CPSRWriteByInstr &&
+            (env->cp15.hcr_el2 & HCR_TGE) &&
+            (env->uncached_cpsr & CPSR_M) == ARM_CPU_MODE_MON &&
+            !arm_is_secure_below_el3(env)) {
+            return 1;
+        }
         return 0;
     case ARM_CPU_MODE_HYP:
         return !arm_feature(env, ARM_FEATURE_EL2)
-- 
1.9.1

Re: [Qemu-devel] [PATCH 01/11] target-arm: Give CPSR setting on 32-bit exception return its own helper

[Sergey Fedorov]

On 15.02.2016 20:22, Peter Maydell wrote:
> The rules for setting the CPSR on a 32-bit exception return are
> subtly different from those for setting the CPSR via an instruction
> like MSR or CPS. (In particular, in Hyp mode changing the mode bits
> is not valid via MSR or CPS.) Split the exception-return case into
> its own helper for setting CPSR, so we can eventually handle them
> differently in the helper function.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.com>

> ---
>  target-arm/helper.h    | 1 +
>  target-arm/op_helper.c | 6 ++++++
>  target-arm/translate.c | 6 +++---
>  3 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/target-arm/helper.h b/target-arm/helper.h
> index c98e9ce..ea13202 100644
> --- a/target-arm/helper.h
> +++ b/target-arm/helper.h
> @@ -57,6 +57,7 @@ DEF_HELPER_2(pre_smc, void, env, i32)
>  DEF_HELPER_1(check_breakpoints, void, env)
>  
>  DEF_HELPER_3(cpsr_write, void, env, i32, i32)
> +DEF_HELPER_2(cpsr_write_eret, void, env, i32)
>  DEF_HELPER_1(cpsr_read, i32, env)
>  
>  DEF_HELPER_3(v7m_msr, void, env, i32, i32)
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index 538887c..e3ddd5a 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -425,6 +425,12 @@ void HELPER(cpsr_write)(CPUARMState *env, uint32_t val, uint32_t mask)
>      cpsr_write(env, val, mask);
>  }
>  
> +/* Write the CPSR for a 32-bit exception return */
> +void HELPER(cpsr_write_eret)(CPUARMState *env, uint32_t val)
> +{
> +    cpsr_write(env, val, CPSR_ERET_MASK);
> +}
> +
>  /* Access to user mode registers from privileged modes.  */
>  uint32_t HELPER(get_user_reg)(CPUARMState *env, uint32_t regno)
>  {
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index e69145d..413f7de 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -4094,7 +4094,7 @@ static void gen_exception_return(DisasContext *s, TCGv_i32 pc)
>      TCGv_i32 tmp;
>      store_reg(s, 15, pc);
>      tmp = load_cpu_field(spsr);
> -    gen_set_cpsr(tmp, CPSR_ERET_MASK);
> +    gen_helper_cpsr_write_eret(cpu_env, tmp);
>      tcg_temp_free_i32(tmp);
>      s->is_jmp = DISAS_JUMP;
>  }
> @@ -4102,7 +4102,7 @@ static void gen_exception_return(DisasContext *s, TCGv_i32 pc)
>  /* Generate a v6 exception return.  Marks both values as dead.  */
>  static void gen_rfe(DisasContext *s, TCGv_i32 pc, TCGv_i32 cpsr)
>  {
> -    gen_set_cpsr(cpsr, CPSR_ERET_MASK);
> +    gen_helper_cpsr_write_eret(cpu_env, cpsr);
>      tcg_temp_free_i32(cpsr);
>      store_reg(s, 15, pc);
>      s->is_jmp = DISAS_JUMP;
> @@ -9094,7 +9094,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
>                  if (exc_return) {
>                      /* Restore CPSR from SPSR.  */
>                      tmp = load_cpu_field(spsr);
> -                    gen_set_cpsr(tmp, CPSR_ERET_MASK);
> +                    gen_helper_cpsr_write_eret(cpu_env, tmp);
>                      tcg_temp_free_i32(tmp);
>                      s->is_jmp = DISAS_JUMP;
>                  }

Re: [Qemu-devel] [PATCH 02/11] target-arm: Add write_type argument to cpsr_write()

[Sergey Fedorov]

On 15.02.2016 20:22, Peter Maydell wrote:
> Add an argument to cpsr_write() to indicate what kind of CPSR
> write is being requested, since the exact behaviour should
> differ for the different cases.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.com>

> ---
>  linux-user/arm/nwfpe/fpa11.h |  2 +-
>  linux-user/main.c            |  6 +++---
>  linux-user/signal.c          |  4 ++--
>  target-arm/cpu.h             | 13 +++++++++++--
>  target-arm/gdbstub.c         |  2 +-
>  target-arm/helper.c          |  3 ++-
>  target-arm/kvm32.c           |  2 +-
>  target-arm/kvm64.c           |  2 +-
>  target-arm/machine.c         |  2 +-
>  target-arm/op_helper.c       |  6 +++---
>  10 files changed, 26 insertions(+), 16 deletions(-)
>
> diff --git a/linux-user/arm/nwfpe/fpa11.h b/linux-user/arm/nwfpe/fpa11.h
> index bb9ac65..faa6b00 100644
> --- a/linux-user/arm/nwfpe/fpa11.h
> +++ b/linux-user/arm/nwfpe/fpa11.h
> @@ -108,7 +108,7 @@ static inline void writeRegister(unsigned int x, unsigned int y)
>  
>  static inline void writeConditionCodes(unsigned int x)
>  {
> -        cpsr_write(user_registers,x,CPSR_NZCV);
> +    cpsr_write(user_registers, x, CPSR_NZCV, CPSRWriteByInstr);
>  }
>  
>  #define ARM_REG_PC 15
> diff --git a/linux-user/main.c b/linux-user/main.c
> index e719a2d..1269470 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -513,7 +513,7 @@ static void arm_kernel_cmpxchg64_helper(CPUARMState *env)
>          env->regs[0] = -1;
>          cpsr &= ~CPSR_C;
>      }
> -    cpsr_write(env, cpsr, CPSR_C);
> +    cpsr_write(env, cpsr, CPSR_C, CPSRWriteByInstr);
>      end_exclusive();
>      return;
>  
> @@ -562,7 +562,7 @@ do_kernel_trap(CPUARMState *env)
>              env->regs[0] = -1;
>              cpsr &= ~CPSR_C;
>          }
> -        cpsr_write(env, cpsr, CPSR_C);
> +        cpsr_write(env, cpsr, CPSR_C, CPSRWriteByInstr);
>          end_exclusive();
>          break;
>      case 0xffff0fe0: /* __kernel_get_tls */
> @@ -4446,7 +4446,7 @@ int main(int argc, char **argv, char **envp)
>  #elif defined(TARGET_ARM)
>      {
>          int i;
> -        cpsr_write(env, regs->uregs[16], 0xffffffff);
> +        cpsr_write(env, regs->uregs[16], 0xffffffff, CPSRWriteByInstr);
>          for(i = 0; i < 16; i++) {
>              env->regs[i] = regs->uregs[i];
>          }
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 327c032..82f81c7 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -1611,7 +1611,7 @@ setup_return(CPUARMState *env, struct target_sigaction *ka,
>  	env->regs[13] = frame_addr;
>  	env->regs[14] = retcode;
>  	env->regs[15] = handler & (thumb ? ~1 : ~3);
> -	cpsr_write(env, cpsr, 0xffffffff);
> +        cpsr_write(env, cpsr, 0xffffffff, CPSRWriteByInstr);
>  }
>  
>  static abi_ulong *setup_sigframe_v2_vfp(abi_ulong *regspace, CPUARMState *env)
> @@ -1843,7 +1843,7 @@ restore_sigcontext(CPUARMState *env, struct target_sigcontext *sc)
>      __get_user(env->regs[15], &sc->arm_pc);
>  #ifdef TARGET_CONFIG_CPU_32
>      __get_user(cpsr, &sc->arm_cpsr);
> -        cpsr_write(env, cpsr, CPSR_USER | CPSR_EXEC);
> +    cpsr_write(env, cpsr, CPSR_USER | CPSR_EXEC, CPSRWriteByInstr);
>  #endif
>  
>  	err |= !valid_user_regs(env);
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 1623821..e72e33b 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -719,8 +719,17 @@ static inline void pstate_write(CPUARMState *env, uint32_t val)
>  
>  /* Return the current CPSR value.  */
>  uint32_t cpsr_read(CPUARMState *env);
> -/* Set the CPSR.  Note that some bits of mask must be all-set or all-clear.  */
> -void cpsr_write(CPUARMState *env, uint32_t val, uint32_t mask);
> +
> +typedef enum CPSRWriteType {
> +    CPSRWriteByInstr = 0,         /* from guest MSR or CPS */
> +    CPSRWriteExceptionReturn = 1, /* from guest exception return insn */
> +    CPSRWriteRaw = 2,             /* trust values, do not switch reg banks */
> +    CPSRWriteByGDBStub = 3,       /* from the GDB stub */
> +} CPSRWriteType;
> +
> +/* Set the CPSR.  Note that some bits of mask must be all-set or all-clear.*/
> +void cpsr_write(CPUARMState *env, uint32_t val, uint32_t mask,
> +                CPSRWriteType write_type);
>  
>  /* Return the current xPSR value.  */
>  static inline uint32_t xpsr_read(CPUARMState *env)
> diff --git a/target-arm/gdbstub.c b/target-arm/gdbstub.c
> index 08b91a4..3ba9aad 100644
> --- a/target-arm/gdbstub.c
> +++ b/target-arm/gdbstub.c
> @@ -94,7 +94,7 @@ int arm_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
>          return 4;
>      case 25:
>          /* CPSR */
> -        cpsr_write(env, tmp, 0xffffffff);
> +        cpsr_write(env, tmp, 0xffffffff, CPSRWriteByGDBStub);
>          return 4;
>      }
>      /* Unknown register.  */
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index a420a2a..828822b 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -5199,7 +5199,8 @@ uint32_t cpsr_read(CPUARMState *env)
>          | (env->GE << 16) | (env->daif & CPSR_AIF);
>  }
>  
> -void cpsr_write(CPUARMState *env, uint32_t val, uint32_t mask)
> +void cpsr_write(CPUARMState *env, uint32_t val, uint32_t mask,
> +                CPSRWriteType write_type)
>  {
>      uint32_t changed_daif;
>  
> diff --git a/target-arm/kvm32.c b/target-arm/kvm32.c
> index ea01932..d44a7f9 100644
> --- a/target-arm/kvm32.c
> +++ b/target-arm/kvm32.c
> @@ -428,7 +428,7 @@ int kvm_arch_get_registers(CPUState *cs)
>      if (ret) {
>          return ret;
>      }
> -    cpsr_write(env, cpsr, 0xffffffff);
> +    cpsr_write(env, cpsr, 0xffffffff, CPSRWriteRaw);
>  
>      /* Make sure the current mode regs are properly set */
>      mode = env->uncached_cpsr & CPSR_M;
> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> index 0f1b4d6..08c2c81 100644
> --- a/target-arm/kvm64.c
> +++ b/target-arm/kvm64.c
> @@ -723,7 +723,7 @@ int kvm_arch_get_registers(CPUState *cs)
>          pstate_write(env, val);
>      } else {
>          env->uncached_cpsr = val & CPSR_M;
> -        cpsr_write(env, val, 0xffffffff);
> +        cpsr_write(env, val, 0xffffffff, CPSRWriteRaw);
>      }
>  
>      /* KVM puts SP_EL0 in regs.sp and SP_EL1 in regs.sp_el1. On the
> diff --git a/target-arm/machine.c b/target-arm/machine.c
> index ed1925a..0fc7df0 100644
> --- a/target-arm/machine.c
> +++ b/target-arm/machine.c
> @@ -175,7 +175,7 @@ static int get_cpsr(QEMUFile *f, void *opaque, size_t size)
>  
>      /* Avoid mode switch when restoring CPSR */
>      env->uncached_cpsr = val & CPSR_M;
> -    cpsr_write(env, val, 0xffffffff);
> +    cpsr_write(env, val, 0xffffffff, CPSRWriteRaw);
>      return 0;
>  }
>  
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index e3ddd5a..543d33a 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -422,13 +422,13 @@ uint32_t HELPER(cpsr_read)(CPUARMState *env)
>  
>  void HELPER(cpsr_write)(CPUARMState *env, uint32_t val, uint32_t mask)
>  {
> -    cpsr_write(env, val, mask);
> +    cpsr_write(env, val, mask, CPSRWriteByInstr);
>  }
>  
>  /* Write the CPSR for a 32-bit exception return */
>  void HELPER(cpsr_write_eret)(CPUARMState *env, uint32_t val)
>  {
> -    cpsr_write(env, val, CPSR_ERET_MASK);
> +    cpsr_write(env, val, CPSR_ERET_MASK, CPSRWriteExceptionReturn);
>  }
>  
>  /* Access to user mode registers from privileged modes.  */
> @@ -780,7 +780,7 @@ void HELPER(exception_return)(CPUARMState *env)
>      if (!return_to_aa64) {
>          env->aarch64 = 0;
>          env->uncached_cpsr = spsr & CPSR_M;
> -        cpsr_write(env, spsr, ~0);
> +        cpsr_write(env, spsr, ~0, CPSRWriteRaw);
>          if (!arm_singlestep_active(env)) {
>              env->uncached_cpsr &= ~PSTATE_SS;
>          }

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 <peter.maydell@linaro.org>

Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.com>

> ---
>  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;

Re: [Qemu-devel] [PATCH 04/11] linux-user: Use restrictive mask when calling cpsr_write()

[Sergey Fedorov]

On 15.02.2016 20:22, Peter Maydell wrote:
> When linux-user code is calling cpsr_write(), use a restrictive
> mask to ensure we are limiting the set of CPSR bits we update.
> In particular, don't allow the mode bits to be changed.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.com>

> ---
>  linux-user/main.c   | 3 ++-
>  linux-user/signal.c | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 1269470..c467fa7 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -4446,7 +4446,8 @@ int main(int argc, char **argv, char **envp)
>  #elif defined(TARGET_ARM)
>      {
>          int i;
> -        cpsr_write(env, regs->uregs[16], 0xffffffff, CPSRWriteByInstr);
> +        cpsr_write(env, regs->uregs[16], CPSR_USER | CPSR_EXEC,
> +                   CPSRWriteByInstr);
>          for(i = 0; i < 16; i++) {
>              env->regs[i] = regs->uregs[i];
>          }
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 82f81c7..962111c 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -1611,7 +1611,7 @@ setup_return(CPUARMState *env, struct target_sigaction *ka,
>  	env->regs[13] = frame_addr;
>  	env->regs[14] = retcode;
>  	env->regs[15] = handler & (thumb ? ~1 : ~3);
> -        cpsr_write(env, cpsr, 0xffffffff, CPSRWriteByInstr);
> +        cpsr_write(env, cpsr, CPSR_IT | CPSR_T, CPSRWriteByInstr);
>  }
>  
>  static abi_ulong *setup_sigframe_v2_vfp(abi_ulong *regspace, CPUARMState *env)

Re: [Qemu-devel] [PATCH 05/11] target-arm: In cpsr_write() ignore mode switches from User mode

[Sergey Fedorov]

On 15.02.2016 20:22, Peter Maydell wrote:
> The only case where we can attempt a cpsr_write() mode switch from
> User is from the gdbstub; all other cases are handled in the
> calling code (notably translate.c). Architecturally attempts to
> alter the mode bits from user mode are simply ignored (and not
> treated as a bad mode switch, which in v8 sets CPSR.IL). Make
> mode switches from User ignored in cpsr_write() as well, for
> consistency.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.com>

> ---
>  target-arm/helper.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index d1919bb..9998a25 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -5282,6 +5282,7 @@ void cpsr_write(CPUARMState *env, uint32_t val, uint32_t mask,
>      env->daif |= val & CPSR_AIF & mask;
>  
>      if (write_type != CPSRWriteRaw &&
> +        (env->uncached_cpsr & CPSR_M) != CPSR_USER &&
>          ((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.

Re: [Qemu-devel] [PATCH 06/11] target-arm: Add comment about not implementing NSACR.RFR

[Sergey Fedorov]

On 15.02.2016 20:22, Peter Maydell wrote:
> QEMU doesn't implement the NSACR.RFR bit, which is a permitted
> IMPDEF in choice in ARMv7 and the only permitted choice in ARMv8.
> Add a comment to bad_mode_switch() to note that this is why
> FIQ is always a valid mode regardless of the CPU's Secure state.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.com>

> ---
>  target-arm/helper.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 9998a25..37b5439 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -5180,6 +5180,9 @@ static int bad_mode_switch(CPUARMState *env, int mode)
>      case ARM_CPU_MODE_UND:
>      case ARM_CPU_MODE_IRQ:
>      case ARM_CPU_MODE_FIQ:
> +        /* Note that we don't implement the IMPDEF NSACR.RFR which in v7
> +         * allows FIQ mode to be Secure-only. (In v8 this doesn't exist.)
> +         */
>          return 0;
>      case ARM_CPU_MODE_MON:
>          return !arm_is_secure(env);

Re: [Qemu-devel] [PATCH 07/11] target-arm: Add Hyp mode checks to bad_mode_switch()

[Sergey Fedorov]

On 15.02.2016 20:22, Peter Maydell wrote:
> We don't actually support Hyp mode yet, but add the correct
> checks for it to the bad_mode_switch() function for completeness.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.com>

> ---
>  target-arm/helper.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 37b5439..4074b97 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -5184,6 +5184,9 @@ static int bad_mode_switch(CPUARMState *env, int mode)
>           * allows FIQ mode to be Secure-only. (In v8 this doesn't exist.)
>           */
>          return 0;
> +    case ARM_CPU_MODE_HYP:
> +        return !arm_feature(env, ARM_FEATURE_EL2)
> +            || arm_current_el(env) < 2 || arm_is_secure(env);
>      case ARM_CPU_MODE_MON:
>          return !arm_is_secure(env);
>      default:

Re: [Qemu-devel] [PATCH 08/11] target-arm: Forbid mode switch to Mon from Secure EL1

[Sergey Fedorov]

On 15.02.2016 20:22, Peter Maydell wrote:
> In v8 trying to switch mode to Mon from Secure EL1 is an
> illegal mode switch. (In v7 this is impossible as all secure
> modes except User are at EL3.) We can handle this case by
> making a switch to Mon valid only if the current EL is 3,
> which then gives the correct answer whether EL3 is AArch32
> or AArch64.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.com>

> ---
>  target-arm/helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 4074b97..e7b3eb3 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -5188,7 +5188,7 @@ static int bad_mode_switch(CPUARMState *env, int mode)
>          return !arm_feature(env, ARM_FEATURE_EL2)
>              || arm_current_el(env) < 2 || arm_is_secure(env);
>      case ARM_CPU_MODE_MON:
> -        return !arm_is_secure(env);
> +        return arm_current_el(env) < 3;
>      default:
>          return 1;
>      }

Re: [Qemu-devel] [PATCH 09/11] target-arm: In v8, make illegal AArch32 mode changes set PSTATE.IL

[Sergey Fedorov]

On 15.02.2016 20:22, Peter Maydell wrote:
> In v8, the illegal mode changes which are UNPREDICTABLE in v7 are
> given architected behaviour:
>  * the mode field is unchanged
>  * PSTATE.IL is set (so any subsequent instructions will UNDEF)
>  * any other CPSR fields are written to as normal
>
> This is pretty much the same behaviour we picked for our
> UNPREDICTABLE handling, with the exception that for v8 we
> need to set the IL bit.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.com>

> ---
>  target-arm/helper.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index e7b3eb3..69e93a2 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -5291,11 +5291,20 @@ void cpsr_write(CPUARMState *env, uint32_t val, uint32_t mask,
>          (env->uncached_cpsr & CPSR_M) != CPSR_USER &&
>          ((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
> -             * untouched.
> +            /* Attempt to switch to an invalid mode: this is UNPREDICTABLE in
> +             * v7, and has defined behaviour in v8:
> +             *  + leave CPSR.M untouched
> +             *  + allow changes to the other CPSR fields
> +             *  + set PSTATE.IL
> +             * For user changes via the GDB stub, we don't set PSTATE.IL,
> +             * as this would be unnecessarily harsh for a user error.
>               */
>              mask &= ~CPSR_M;
> +            if (write_type != CPSRWriteByGDBStub &&
> +                arm_feature(env, ARM_FEATURE_V8)) {
> +                mask |= CPSR_IL;
> +                val |= CPSR_IL;
> +            }
>          } else {
>              switch_mode(env, val & CPSR_M);
>          }

Re: [Qemu-devel] [PATCH 10/11] target-arm: Make mode switches from Hyp via CPS and MRS illegal

[Sergey Fedorov]

On 15.02.2016 20:22, Peter Maydell wrote:
> Mode switches from Hyp to any other mode via the CPS and MRS
> instructions are illegal mode switches (though obviously switching
> via exception return is valid).  Add this check to bad_mode_switch().
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.com>

> ---
>  target-arm/helper.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 69e93a2..e1af9d5 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -5166,12 +5166,20 @@ void arm_cp_reset_ignore(CPUARMState *env, const ARMCPRegInfo *opaque)
>      /* Helper coprocessor reset function for do-nothing-on-reset registers */
>  }
>  
> -static int bad_mode_switch(CPUARMState *env, int mode)
> +static int bad_mode_switch(CPUARMState *env, int mode, CPSRWriteType write_type)
>  {
>      /* Return true if it is not valid for us to switch to
>       * this CPU mode (ie all the UNPREDICTABLE cases in
>       * the ARM ARM CPSRWriteByInstr pseudocode).
>       */
> +
> +    /* Changes to or from Hyp via MSR and CPS are illegal. */
> +    if (write_type == CPSRWriteByInstr &&
> +        ((env->uncached_cpsr & CPSR_M) == ARM_CPU_MODE_HYP ||
> +         mode == ARM_CPU_MODE_HYP)) {
> +        return 1;
> +    }
> +
>      switch (mode) {
>      case ARM_CPU_MODE_USR:
>      case ARM_CPU_MODE_SYS:
> @@ -5290,7 +5298,7 @@ void cpsr_write(CPUARMState *env, uint32_t val, uint32_t mask,
>      if (write_type != CPSRWriteRaw &&
>          (env->uncached_cpsr & CPSR_M) != CPSR_USER &&
>          ((env->uncached_cpsr ^ val) & mask & CPSR_M)) {
> -        if (bad_mode_switch(env, val & CPSR_M)) {
> +        if (bad_mode_switch(env, val & CPSR_M, write_type)) {
>              /* Attempt to switch to an invalid mode: this is UNPREDICTABLE in
>               * v7, and has defined behaviour in v8:
>               *  + leave CPSR.M untouched

Re: [Qemu-devel] [PATCH 11/11] target-arm: Make Monitor->NS PL1 mode changes illegal if HCR.TGE is 1

[Sergey Fedorov]

On 15.02.2016 20:22, Peter Maydell wrote:
> If HCR.TGE is 1 then mode changes via CPS and MSR from Monitor to
> NonSecure PL1 modes are illegal mode changes. Implement this check
> in bad_mode_switch().
>
> (We don't currently implement HCR.TGE, but this is the only missing
> check from the v8 ARM ARM G1.9.3 and so it's worth adding now; the
> rest of the HCR.TGE checks can be added later as necessary.)
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.com>

> ---
>  target-arm/helper.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index e1af9d5..93a0b63 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -5182,6 +5182,7 @@ static int bad_mode_switch(CPUARMState *env, int mode, CPSRWriteType write_type)
>  
>      switch (mode) {
>      case ARM_CPU_MODE_USR:
> +        return 0;
>      case ARM_CPU_MODE_SYS:
>      case ARM_CPU_MODE_SVC:
>      case ARM_CPU_MODE_ABT:
> @@ -5191,6 +5192,15 @@ static int bad_mode_switch(CPUARMState *env, int mode, CPSRWriteType write_type)
>          /* Note that we don't implement the IMPDEF NSACR.RFR which in v7
>           * allows FIQ mode to be Secure-only. (In v8 this doesn't exist.)
>           */
> +        /* If HCR.TGE is set then changes from Monitor to NS PL1 via MSR
> +         * and CPS are treated as illegal mode changes.
> +         */
> +        if (write_type == CPSRWriteByInstr &&
> +            (env->cp15.hcr_el2 & HCR_TGE) &&
> +            (env->uncached_cpsr & CPSR_M) == ARM_CPU_MODE_MON &&
> +            !arm_is_secure_below_el3(env)) {
> +            return 1;
> +        }
>          return 0;
>      case ARM_CPU_MODE_HYP:
>          return !arm_feature(env, ARM_FEATURE_EL2)