[Qemu-devel] [RCF PATCH 0/8] aarch64 migration fixes and psr cleanup

[Qemu-devel] [RCF PATCH 0/8] aarch64 migration fixes and psr cleanup

[Alex Bennée]

Hi,

This started with a hack-patch Peter had written to get migration
working for aarch64 system images. It was noted that cpu.h was
starting to proliferate program state functions. This series attempts
to fix that by:

  - creating a "universal" save/restore_to/from_spsr function
  - using that for migration purposes
  - removing existing use of cpsr/pstate_read/write functions
  - fixing some places to manipulate integer flags directly

Hanging questions:

  - should xpsr_read/write get merged or left as it's own special
    snowflake?
  - should env->uncached_cpsr and env->pstate be totally hidden behind
    access functions so other flags can get split (i.e. env->el_mode?)

I've tested this with ARMv7 and ARMv8 with aarch32 user-space system
images. I've not given it a good linux-user test yet so I'm less sure
about the sigcontext manipulation code.

Comments?

Cheers,

--
Alex Bennée
QEMU/KVM Hacker for Linaro


Alex Bennée (8):
  target-arm/cpu.h: document various program state functions
  target-arm/cpu.h: implement common state save/restore
  target-arm: Support save/load for 64 bit CPUs
  target-arm: replace cpsr_read/pstate_read calls
  arm/nwfps: remove use of cpsr_write() and set flags directly
  linux-user/main.c: __kernel_cmpxchg set env->CF directly
  target-arm: remove last users of cpsr_write
  target-arm: remove final users of pstate_write

 linux-user/arm/nwfpe/fpa11.h |   5 +-
 linux-user/elfload.c         |   4 +-
 linux-user/main.c            |  16 ++--
 linux-user/signal.c          |  64 ++++++++--------
 target-arm/cpu.h             | 173 ++++++++++++++++++++++++++++++++++---------
 target-arm/gdbstub.c         |  10 ++-
 target-arm/gdbstub64.c       |   6 +-
 target-arm/helper-a64.c      |  11 +--
 target-arm/helper.c          |  76 +------------------
 target-arm/kvm32.c           |   4 +-
 target-arm/kvm64.c           |   4 +-
 target-arm/machine.c         |  27 ++++---
 target-arm/op_helper.c       |  55 +++++++++++---
 target-arm/translate-a64.c   |   2 +-
 target-arm/translate.c       |   2 +-
 15 files changed, 266 insertions(+), 193 deletions(-)

-- 
2.0.0

[Qemu-devel] [RCF PATCH 1/8] target-arm/cpu.h: document various program state functions

[Alex Bennée]

We have a number of program state saving functions (pstate, cpsr, xpsr)
which are dependant on the mode the CPU is in. This commit adds a little
documentation to each function and asserts to defend against incorrect
use.

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 8d04385..5e6df38 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -459,22 +459,34 @@ int arm_cpu_handle_mmu_fault(CPUState *cpu, vaddr address, int rw,
 #define PSTATE_MODE_EL1t 4
 #define PSTATE_MODE_EL0t 0
 
-/* Return the current PSTATE value. For the moment we don't support 32<->64 bit
- * interprocessing, so we don't attempt to sync with the cpsr state used by
- * the 32 bit decoder.
+/* ARMv8 ARM D1.7 Process state, PSTATE
+ *
+ *  31  28 27  24 23   22 21 20 22    21  20 19  16 15   8 7   5 4    0
+ * +------+------+-------+-----+--------+---+------+------+-----+------+
+ * | NZCV | DAIF | SS IL |  EL | nRW SP | Q |  GE  |  IT  | JTE | Mode |
+ * +------+------+-------+-----+--------+---+------+------+-----+------+
+ *
+ * The PSTATE is an abstraction of a number of Return the current
+ * PSTATE value. This is only valid for A64 hardware although can be
+ * read when in AArch32 mode.
  */
 static inline uint32_t pstate_read(CPUARMState *env)
 {
     int ZF;
 
+    g_assert(is_a64(env));
+
     ZF = (env->ZF == 0);
     return (env->NF & 0x80000000) | (ZF << 30)
         | (env->CF << 29) | ((env->VF & 0x80000000) >> 3)
         | env->pstate | env->daif;
 }
 
+/* Update the current PSTATE value. This doesn't include nRW which is */
 static inline void pstate_write(CPUARMState *env, uint32_t val)
 {
+    g_assert(is_a64(env));
+
     env->ZF = (~val) & PSTATE_Z;
     env->NF = val;
     env->CF = (val >> 29) & 1;
@@ -483,15 +495,23 @@ static inline void pstate_write(CPUARMState *env, uint32_t val)
     env->pstate = val & ~CACHED_PSTATE_BITS;
 }
 
-/* Return the current CPSR value.  */
+/* ARMv7-AR ARM B1.3.3 Current Program Status Register, CPSR
+ *
+ * Unlike the above PSTATE implementation these functions will attempt
+ * to switch processor mode when the M[4:0] bits are set.
+ */
 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);
 
-/* Return the current xPSR value.  */
+/* ARMv7-M ARM B1.4.2, special purpose program status register xPSR */
 static inline uint32_t xpsr_read(CPUARMState *env)
 {
     int ZF;
+
+    g_assert(!is_a64(env));
+//    g_assert(IS_M(env));
+
     ZF = (env->ZF == 0);
     return (env->NF & 0x80000000) | (ZF << 30)
         | (env->CF << 29) | ((env->VF & 0x80000000) >> 3) | (env->QF << 27)
@@ -503,6 +523,9 @@ static inline uint32_t xpsr_read(CPUARMState *env)
 /* Set the xPSR.  Note that some bits of mask must be all-set or all-clear.  */
 static inline void xpsr_write(CPUARMState *env, uint32_t val, uint32_t mask)
 {
+    g_assert(!is_a64(env));
+//    g_assert(IS_M(env));
+
     if (mask & CPSR_NZCV) {
         env->ZF = (~val) & CPSR_Z;
         env->NF = val;
diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
index cccda74..48ca351 100644
--- a/target-arm/helper-a64.c
+++ b/target-arm/helper-a64.c
@@ -506,8 +506,8 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
         env->condexec_bits = 0;
     }
 
-    pstate_write(env, PSTATE_DAIF | PSTATE_MODE_EL1h);
     env->aarch64 = 1;
+    pstate_write(env, PSTATE_DAIF | PSTATE_MODE_EL1h);
 
     env->pc = addr;
     cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
-- 
2.0.0

[Qemu-devel] [RCF PATCH 2/8] target-arm/cpu.h: implement common state save/restore

[Alex Bennée]

This adds a universal program state save and restore function. This is
intended to simplify the migration serialisation functionality and avoid
special casing depending on the mode of the CPU at serialisation time.

---

WIP notes:
    - From this I'll look at
      - cpsr_read/cpsr_write
      - pstate_read/pstate_write
      - xpsr_read/xpsr_write
      - direct access to env->uncached_cpsr
      - direct access to env->pstate

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 5e6df38..9e41f82 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -430,26 +430,27 @@ int arm_cpu_handle_mmu_fault(CPUState *cpu, vaddr address, int rw,
 /* Execution state bits.  MRS read as zero, MSR writes ignored.  */
 #define CPSR_EXEC (CPSR_T | CPSR_IT | CPSR_J)
 
-/* Bit definitions for ARMv8 SPSR (PSTATE) format.
- * Only these are valid when in AArch64 mode; in
- * AArch32 mode SPSRs are basically CPSR-format.
- */
+
 #define PSTATE_SP (1U)
 #define PSTATE_M (0xFU)
 #define PSTATE_nRW (1U << 4)
+#define PSTATE_T (1U << 5)
 #define PSTATE_F (1U << 6)
 #define PSTATE_I (1U << 7)
 #define PSTATE_A (1U << 8)
 #define PSTATE_D (1U << 9)
 #define PSTATE_IL (1U << 20)
 #define PSTATE_SS (1U << 21)
+#define PSTATE_Q (1U << 27)
 #define PSTATE_V (1U << 28)
 #define PSTATE_C (1U << 29)
 #define PSTATE_Z (1U << 30)
 #define PSTATE_N (1U << 31)
 #define PSTATE_NZCV (PSTATE_N | PSTATE_Z | PSTATE_C | PSTATE_V)
-#define PSTATE_DAIF (PSTATE_D | PSTATE_A | PSTATE_I | PSTATE_F)
-#define CACHED_PSTATE_BITS (PSTATE_NZCV | PSTATE_DAIF)
+#define PSTATE_AIF  (PSTATE_A | PSTATE_I | PSTATE_F)
+#define PSTATE_DAIF (PSTATE_D | PSTATE_AIF)
+#define AARCH64_CACHED_PSTATE_BITS (PSTATE_NZCV | PSTATE_DAIF)
+#define AARCH32_CACHED_PSTATE_BITS (PSTATE_NZCV | PSTATE_Q | PSTATE_AIF | CACHED_CPSR_BITS)
 /* Mode values for AArch64 */
 #define PSTATE_MODE_EL3h 13
 #define PSTATE_MODE_EL3t 12
@@ -492,7 +493,7 @@ static inline void pstate_write(CPUARMState *env, uint32_t val)
     env->CF = (val >> 29) & 1;
     env->VF = (val << 3) & 0x80000000;
     env->daif = val & PSTATE_DAIF;
-    env->pstate = val & ~CACHED_PSTATE_BITS;
+    env->pstate = val & ~AARCH64_CACHED_PSTATE_BITS;
 }
 
 /* ARMv7-AR ARM B1.3.3 Current Program Status Register, CPSR
@@ -681,6 +682,122 @@ static inline bool arm_el_is_aa64(CPUARMState *env, int el)
     return arm_feature(env, ARM_FEATURE_AARCH64);
 }
 
+/* ARMv8 ARM D1.6.4, Saved Program Status Registers
+ *
+ * These are formats used to save program status when exceptions are
+ * taken. There are two forms:
+ *
+ * AArch64 -> AArch64 Exception
+ *  31  30  28  29  27  22  21   20  19  10  9   8   7   6   5  4   3      0
+ * +---+---+---+---+------+----+----+------+---+---+---+---+---+------------+
+ * | N | Z | C | V | RES0 | SS | IL | RES0 | D | A | I | F | 0 | 0 | M[3:0] |
+ * +---+---+---+---+------+----+----+------+---+---+---+---+---+------------+
+ *
+ * AArch32 -> AArch64 Exception
+ * (see also ARMv7-AR ARM B1.3.3 CSPR/SPSR formats)
+ *  31  30  29  28  27  26     25 24  23  22  21   20  19     16
+ * +---+---+---+---+---+---------+---+------+----+----+---------+
+ * | N | Z | C | V | Q | IT[1:0] | J | RES0 | SS | IL | GE[3:0] |
+ * +---+---+---+---+---+---------+---+------+----+----+---------+
+ *  15     10  9   8   7   6   5   4  3      0
+ * +---------+---+---+---+---+---+---+--------+
+ * | IT[7:2] | E | A | I | F | T | 1 | M[3:0] |
+ * +---------+---+---+---+---+---+---+--------+
+ *
+ * M[4] = nRW - 0 = 64bit, 1 = 32bit
+ * Bit definitions for ARMv8 SPSR (PSTATE) format.
+ * Only these are valid when in AArch64 mode; in
+ * AArch32 mode SPSRs are basically CPSR-format.
+ */
+
+/* Save the program state */
+static inline uint32_t save_state_to_spsr(CPUARMState *env)
+{
+    uint32_t final_spsr = 0;
+
+    /* Calculate integer flags */
+    final_spsr |= (env->NF & 0x80000000) ? PSTATE_N : 0; // bit 31
+    final_spsr |= (env->ZF == 0) ? PSTATE_Z : 0;
+    final_spsr |= env->CF ? PSTATE_C : 0;       // or |= env->CF << 29
+    final_spsr |= (env->VF & 0x80000000) ? PSTATE_V : 0; // bit 31
+
+    /* will the compiler do better with the original?
+       int ZF;
+       ZF = (env->ZF == 0);
+       nzcv = (env->NF & 0x80000000)
+       | (ZF << 30)
+       | (env->CF << 29)
+       | ((env->VF & 0x80000000) >> 3);
+    */
+
+    if (is_a64(env)) {
+        /* SS - Software Step */
+        /* IL - Illegal execution state */
+        /* DAIF flags - should we mask?*/
+        final_spsr |= (env->daif & PSTATE_DAIF);
+        /* And the final un-cached bits */
+        final_spsr |= (env->pstate & ~AARCH64_CACHED_PSTATE_BITS);
+    } else {
+        if (arm_feature(env, ARM_FEATURE_M)) {
+            fprintf(stderr,"%s: M state not yet implmented\n", __func__);
+        } else {
+            final_spsr |= env->QF ? PSTATE_Q : 0;
+            /* condexec_bits is split over two parts */
+            final_spsr |= ((env->condexec_bits & 3) << 25);
+            final_spsr |= ((env->condexec_bits & 0xfc) << 8);
+            /* GE needs shifting into place */
+            final_spsr |= (env->GE << 16);
+            /* AIF is a a subset of DAIF */
+            final_spsr |= (env->daif & PSTATE_AIF);
+            final_spsr |= env->thumb ? PSTATE_T : 0;
+
+            final_spsr |= PSTATE_nRW;
+
+            /* And the final un-cached bits */
+            final_spsr |= (env->uncached_cpsr & ~AARCH32_CACHED_PSTATE_BITS);
+        }
+    }
+    return final_spsr;
+}
+
+static inline void restore_state_from_spsr(CPUARMState *env, uint32_t saved_state)
+{
+    /* Process the integer flags directly */
+    env->ZF = (~saved_state) & CPSR_Z;
+    env->NF = saved_state;
+    env->CF = (saved_state >> 29) & 1;
+    env->VF = (saved_state << 3) & 0x80000000;
+
+    /* the rest depends on the mode we end up in */
+    env->aarch64 = (saved_state & PSTATE_nRW) ? 0 : 1;
+
+    if (is_a64(env)) {
+        env->daif = saved_state & PSTATE_DAIF;
+        env->pstate = (saved_state & ~AARCH64_CACHED_PSTATE_BITS);
+    } else {
+        if (arm_feature(env, ARM_FEATURE_M)) {
+            fprintf(stderr,"%s: M state not yet implmented\n", __func__);
+        } else {
+            env->QF = saved_state & PSTATE_Q ? 1 : 0;
+            /* condexec_bits is split over two parts */
+            env->condexec_bits = (saved_state & CPSR_IT_0_1) >> 25;
+            env->condexec_bits |= (saved_state & CPSR_IT_2_7) >> 8;
+
+            /* GE needs shifting into place */
+            env->GE = (saved_state >> 16) & 0xf;
+
+            /* AIF is a a subset of DAIF */
+            env->daif = (saved_state & PSTATE_AIF);
+
+            env->thumb = saved_state & PSTATE_T ? 1 : 0;
+
+            /* And the final un-cached bits */
+            env->uncached_cpsr = saved_state & ~AARCH32_CACHED_PSTATE_BITS;
+        }
+    }
+}
+
+
 void arm_cpu_list(FILE *f, fprintf_function cpu_fprintf);
 
 /* Interface between CPU and Interrupt controller.  */
-- 
2.0.0

[Qemu-devel] [RCF PATCH 3/8] target-arm: Support save/load for 64 bit CPUs

[Alex Bennée]

This enables the saving and restoring of machine state by including the
current program state (*psr) and xregs. The save_state_to_spsr hides the
details of if the processor is in 32 or 64 bit mode at the time.

---

v2 (ajb)
  - use common state save functions
  - re-base to latest origin/master
  - clean up commented out code

diff --git a/target-arm/machine.c b/target-arm/machine.c
index 3bcc7cc..759610c 100644
--- a/target-arm/machine.c
+++ b/target-arm/machine.c
@@ -120,30 +120,27 @@ static const VMStateDescription vmstate_thumb2ee = {
     }
 };
 
-static int get_cpsr(QEMUFile *f, void *opaque, size_t size)
+static int get_psr(QEMUFile *f, void *opaque, size_t size)
 {
     ARMCPU *cpu = opaque;
     CPUARMState *env = &cpu->env;
     uint32_t val = qemu_get_be32(f);
 
-    /* Avoid mode switch when restoring CPSR */
-    env->uncached_cpsr = val & CPSR_M;
-    cpsr_write(env, val, 0xffffffff);
+    restore_state_from_spsr(env, val);
     return 0;
 }
 
-static void put_cpsr(QEMUFile *f, void *opaque, size_t size)
+static void put_psr(QEMUFile *f, void *opaque, size_t size)
 {
     ARMCPU *cpu = opaque;
     CPUARMState *env = &cpu->env;
-
-    qemu_put_be32(f, cpsr_read(env));
+    qemu_put_be32(f, save_state_to_spsr(env));
 }
 
-static const VMStateInfo vmstate_cpsr = {
+static const VMStateInfo vmstate_psr = {
     .name = "cpsr",
-    .get = get_cpsr,
-    .put = put_cpsr,
+    .get = get_psr,
+    .put = put_psr,
 };
 
 static void cpu_pre_save(void *opaque)
@@ -218,17 +215,19 @@ static int cpu_post_load(void *opaque, int version_id)
 
 const VMStateDescription vmstate_arm_cpu = {
     .name = "cpu",
-    .version_id = 20,
-    .minimum_version_id = 20,
+    .version_id = 21,
+    .minimum_version_id = 21,
     .pre_save = cpu_pre_save,
     .post_load = cpu_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32_ARRAY(env.regs, ARMCPU, 16),
+        VMSTATE_UINT64_ARRAY(env.xregs, ARMCPU, 32),
+        VMSTATE_UINT64(env.pc, ARMCPU),
         {
-            .name = "cpsr",
+            .name = "psr",
             .version_id = 0,
             .size = sizeof(uint32_t),
-            .info = &vmstate_cpsr,
+            .info = &vmstate_psr,
             .flags = VMS_SINGLE,
             .offset = 0,
         },
-- 
2.0.0

[Qemu-devel] [RCF PATCH 4/8] target-arm: replace cpsr_read/pstate_read calls

[Alex Bennée]

Use the unified save_state_to_spsr. I've also updated the interrupt
helpers to restore via the restore_state_from_spsr() functions. In the
aarch32 case this also needs to call switch_mode() to do the appropriate
fiddling.

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 995f999..a93a969 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -321,7 +321,7 @@ static void elf_core_copy_regs(target_elf_gregset_t *regs, const CPUARMState *en
     (*regs)[14] = tswapreg(env->regs[14]);
     (*regs)[15] = tswapreg(env->regs[15]);
 
-    (*regs)[16] = tswapreg(cpsr_read((CPUARMState *)env));
+    (*regs)[16] = tswapreg(save_state_to_spsr((CPUARMState *)env));
     (*regs)[17] = tswapreg(env->regs[0]); /* XXX */
 }
 
@@ -506,7 +506,7 @@ static void elf_core_copy_regs(target_elf_gregset_t *regs,
         (*regs)[i] = tswapreg(env->xregs[i]);
     }
     (*regs)[32] = tswapreg(env->pc);
-    (*regs)[33] = tswapreg(pstate_read((CPUARMState *)env));
+    (*regs)[33] = tswapreg(save_state_to_spsr((CPUARMState *)env));
 }
 
 #define USE_ELF_CORE_DUMP
diff --git a/linux-user/main.c b/linux-user/main.c
index 882186e..22d0197 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -547,7 +547,7 @@ do_kernel_trap(CPUARMState *env)
             operations. However things like ldrex/strex are much harder so
             there's not much point trying.  */
         start_exclusive();
-        cpsr = cpsr_read(env);
+        cpsr = save_state_to_spsr(env);
         addr = env->regs[2];
         /* FIXME: This should SEGV if the access fails.  */
         if (get_user_u32(val, addr))
diff --git a/linux-user/signal.c b/linux-user/signal.c
index 5b8a01f..ee2a79f 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -1240,7 +1240,7 @@ static int target_setup_sigframe(struct target_rt_sigframe *sf,
     }
     __put_user(env->xregs[31], &sf->uc.tuc_mcontext.sp);
     __put_user(env->pc, &sf->uc.tuc_mcontext.pc);
-    __put_user(pstate_read(env), &sf->uc.tuc_mcontext.pstate);
+    __put_user(save_state_to_spsr(env), &sf->uc.tuc_mcontext.pstate);
 
     __put_user(env->exception.vaddress, &sf->uc.tuc_mcontext.fault_address);
 
@@ -1592,7 +1592,7 @@ setup_sigcontext(struct target_sigcontext *sc, /*struct _fpstate *fpstate,*/
 	__put_user(env->regs[14], &sc->arm_lr);
 	__put_user(env->regs[15], &sc->arm_pc);
 #ifdef TARGET_CONFIG_CPU_32
-	__put_user(cpsr_read(env), &sc->arm_cpsr);
+	__put_user(save_state_to_spsr(env), &sc->arm_cpsr);
 #endif
 
 	__put_user(/* current->thread.trap_no */ 0, &sc->trap_no);
@@ -1624,7 +1624,7 @@ setup_return(CPUARMState *env, struct target_sigaction *ka,
 	abi_ulong handler = ka->_sa_handler;
 	abi_ulong retcode;
 	int thumb = handler & 1;
-	uint32_t cpsr = cpsr_read(env);
+	uint32_t cpsr = save_state_to_spsr(env);
 
 	cpsr &= ~CPSR_IT;
 	if (thumb) {
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 9e41f82..cfdc1cb 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -460,30 +460,8 @@ int arm_cpu_handle_mmu_fault(CPUState *cpu, vaddr address, int rw,
 #define PSTATE_MODE_EL1t 4
 #define PSTATE_MODE_EL0t 0
 
-/* ARMv8 ARM D1.7 Process state, PSTATE
- *
- *  31  28 27  24 23   22 21 20 22    21  20 19  16 15   8 7   5 4    0
- * +------+------+-------+-----+--------+---+------+------+-----+------+
- * | NZCV | DAIF | SS IL |  EL | nRW SP | Q |  GE  |  IT  | JTE | Mode |
- * +------+------+-------+-----+--------+---+------+------+-----+------+
- *
- * The PSTATE is an abstraction of a number of Return the current
- * PSTATE value. This is only valid for A64 hardware although can be
- * read when in AArch32 mode.
- */
-static inline uint32_t pstate_read(CPUARMState *env)
-{
-    int ZF;
-
-    g_assert(is_a64(env));
-
-    ZF = (env->ZF == 0);
-    return (env->NF & 0x80000000) | (ZF << 30)
-        | (env->CF << 29) | ((env->VF & 0x80000000) >> 3)
-        | env->pstate | env->daif;
-}
-
-/* Update the current PSTATE value. This doesn't include nRW which is */
+/* Update the current PSTATE value. This doesn't include nRW which
+ * indicates if we are in 64 or 32 bit mode */
 static inline void pstate_write(CPUARMState *env, uint32_t val)
 {
     g_assert(is_a64(env));
@@ -500,9 +478,8 @@ static inline void pstate_write(CPUARMState *env, uint32_t val)
  *
  * Unlike the above PSTATE implementation these functions will attempt
  * to switch processor mode when the M[4:0] bits are set.
- */
-uint32_t cpsr_read(CPUARMState *env);
-/* Set the CPSR.  Note that some bits of mask must be all-set or all-clear.  */
+ *
+ * Note that some bits of mask must be all-set or all-clear.  */
 void cpsr_write(CPUARMState *env, uint32_t val, uint32_t mask);
 
 /* ARMv7-M ARM B1.4.2, special purpose program status register xPSR */
@@ -760,6 +737,14 @@ static inline uint32_t save_state_to_spsr(CPUARMState *env)
     return final_spsr;
 }
 
+/* Restore the program state and mode.
+ *
+ * There is nothing that stops the execution state from being changed
+ * here and any restrictions should be enforced by the calling
+ * function. Also aarch32 execution state changes (new EL) need extra
+ * special handling which should be done by the calling function
+ * before the switch.
+ */
 static inline void restore_state_from_spsr(CPUARMState *env, uint32_t saved_state)
 {
     /* Process the integer flags directly */
diff --git a/target-arm/gdbstub.c b/target-arm/gdbstub.c
index 1c34396..ec25f30 100644
--- a/target-arm/gdbstub.c
+++ b/target-arm/gdbstub.c
@@ -53,7 +53,7 @@ int arm_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n)
         return gdb_get_reg32(mem_buf, 0);
     case 25:
         /* CPSR */
-        return gdb_get_reg32(mem_buf, cpsr_read(env));
+        return gdb_get_reg32(mem_buf, save_state_to_spsr(env));
     }
     /* Unknown register.  */
     return 0;
diff --git a/target-arm/gdbstub64.c b/target-arm/gdbstub64.c
index 8f3b8d1..76d1b91 100644
--- a/target-arm/gdbstub64.c
+++ b/target-arm/gdbstub64.c
@@ -35,7 +35,7 @@ int aarch64_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n)
     case 32:
         return gdb_get_reg64(mem_buf, env->pc);
     case 33:
-        return gdb_get_reg32(mem_buf, pstate_read(env));
+        return gdb_get_reg32(mem_buf, save_state_to_spsr(env));
     }
     /* Unknown register.  */
     return 0;
@@ -62,7 +62,7 @@ int aarch64_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
         env->pc = tmp;
         return 8;
     case 33:
-        /* CPSR */
+        /* SPSR */
         pstate_write(env, tmp);
         return 4;
     }
diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
index 48ca351..1ca3164 100644
--- a/target-arm/helper-a64.c
+++ b/target-arm/helper-a64.c
@@ -445,6 +445,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
     CPUARMState *env = &cpu->env;
     target_ulong addr = env->cp15.vbar_el[1];
     int i;
+    uint32_t spsr = save_state_to_spsr(env);
 
     if (arm_current_pl(env) == 0) {
         if (env->aarch64) {
@@ -452,7 +453,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
         } else {
             addr += 0x600;
         }
-    } else if (pstate_read(env) & PSTATE_SP) {
+    } else if (spsr & PSTATE_SP) {
         addr += 0x200;
     }
 
@@ -488,12 +489,12 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
     }
 
     if (is_a64(env)) {
-        env->banked_spsr[aarch64_banked_spsr_index(1)] = pstate_read(env);
+        env->banked_spsr[aarch64_banked_spsr_index(1)] = spsr;
         env->sp_el[arm_current_pl(env)] = env->xregs[31];
         env->xregs[31] = env->sp_el[1];
         env->elr_el[1] = env->pc;
     } else {
-        env->banked_spsr[0] = cpsr_read(env);
+        env->banked_spsr[0] = spsr;
         if (!env->thumb) {
             env->cp15.esr_el[1] |= 1 << 25;
         }
@@ -506,6 +507,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
         env->condexec_bits = 0;
     }
 
+    // TODO: restore_state_from_spsr()
     env->aarch64 = 1;
     pstate_write(env, PSTATE_DAIF | PSTATE_MODE_EL1h);
 
diff --git a/target-arm/helper.c b/target-arm/helper.c
index ec031f5..e23cec4 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -2979,17 +2979,6 @@ static int bad_mode_switch(CPUARMState *env, int mode)
     }
 }
 
-uint32_t cpsr_read(CPUARMState *env)
-{
-    int ZF;
-    ZF = (env->ZF == 0);
-    return env->uncached_cpsr | (env->NF & 0x80000000) | (ZF << 30) |
-        (env->CF << 29) | ((env->VF & 0x80000000) >> 3) | (env->QF << 27)
-        | (env->thumb << 5) | ((env->condexec_bits & 3) << 25)
-        | ((env->condexec_bits & 0xfc) << 8)
-        | (env->GE << 16) | (env->daif & CPSR_AIF);
-}
-
 void cpsr_write(CPUARMState *env, uint32_t val, uint32_t mask)
 {
     if (mask & CPSR_NZCV) {
@@ -3468,7 +3457,8 @@ void arm_cpu_do_interrupt(CPUState *cs)
         addr += env->cp15.vbar_el[1];
     }
     switch_mode (env, new_mode);
-    env->spsr = cpsr_read(env);
+    env->spsr = save_state_to_spsr(env);
+    /* TODO: restore_state_from_spsr */
     /* Clear IT bits.  */
     env->condexec_bits = 0;
     /* Switch to the new mode, and to the correct instruction set.  */
diff --git a/target-arm/kvm32.c b/target-arm/kvm32.c
index b79750c..de3063f 100644
--- a/target-arm/kvm32.c
+++ b/target-arm/kvm32.c
@@ -385,7 +385,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
     }
 
     /* Special cases which aren't a single CPUARMState field */
-    cpsr = cpsr_read(env);
+    cpsr = save_state_to_spsr(env);
     r.id = KVM_REG_ARM | KVM_REG_SIZE_U32 |
         KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(usr_regs.ARM_cpsr);
     r.addr = (uintptr_t)(&cpsr);
diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
index 70f311b..83df952 100644
--- a/target-arm/kvm64.c
+++ b/target-arm/kvm64.c
@@ -145,7 +145,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
     }
 
     /* Note that KVM thinks pstate is 64 bit but we use a uint32_t */
-    val = pstate_read(env);
+    val = save_state_to_spsr(env);
     reg.id = AARCH64_CORE_REG(regs.pstate);
     reg.addr = (uintptr_t) &val;
     ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index b28f694..b58bcdf 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -273,7 +273,7 @@ void HELPER(exception_with_syndrome)(CPUARMState *env, uint32_t excp,
 
 uint32_t HELPER(cpsr_read)(CPUARMState *env)
 {
-    return cpsr_read(env) & ~CPSR_EXEC;
+    return save_state_to_spsr(env) & ~CPSR_EXEC;
 }
 
 void HELPER(cpsr_write)(CPUARMState *env, uint32_t val, uint32_t mask)
@@ -401,10 +401,9 @@ void HELPER(exception_return)(CPUARMState *env)
 
     if (spsr & PSTATE_nRW) {
         /* TODO: We currently assume EL1/2/3 are running in AArch64.  */
-        env->aarch64 = 0;
         new_el = 0;
-        env->uncached_cpsr = 0x10;
-        cpsr_write(env, spsr, ~0);
+        switch_mode(env, spsr & CPSR_M);
+
         for (i = 0; i < 15; i++) {
             env->regs[i] = env->xregs[i];
         }
@@ -427,11 +426,11 @@ void HELPER(exception_return)(CPUARMState *env)
             /* Return to EL0 with M[0] bit set */
             goto illegal_return;
         }
-        env->aarch64 = 1;
-        pstate_write(env, spsr);
         env->xregs[31] = env->sp_el[new_el];
         env->pc = env->elr_el[cur_el];
     }
+    /* This will set env->aarch64 as appropriate */
+    restore_state_from_spsr(env, spsr);
 
     return;
 
@@ -446,8 +445,8 @@ illegal_return:
     env->pstate |= PSTATE_IL;
     env->pc = env->elr_el[cur_el];
     spsr &= PSTATE_NZCV | PSTATE_DAIF;
-    spsr |= pstate_read(env) & ~(PSTATE_NZCV | PSTATE_DAIF);
-    pstate_write(env, spsr);
+    spsr |= save_state_to_spsr(env) & ~(PSTATE_NZCV | PSTATE_DAIF);
+    restore_state_from_spsr(env, spsr);
 }
 
 /* ??? Flag setting arithmetic is awkward because we need to do comparisons.
diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index 9f964df..28216a3 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -123,7 +123,7 @@ void aarch64_cpu_dump_state(CPUState *cs, FILE *f,
 {
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
-    uint32_t psr = pstate_read(env);
+    uint32_t psr = save_state_to_spsr(env);
     int i;
 
     cpu_fprintf(f, "PC=%016"PRIx64"  SP=%016"PRIx64"\n",
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 7f6fcd6..2429744 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -11075,7 +11075,7 @@ void arm_cpu_dump_state(CPUState *cs, FILE *f, fprintf_function cpu_fprintf,
         else
             cpu_fprintf(f, " ");
     }
-    psr = cpsr_read(env);
+    psr = save_state_to_spsr(env);
     cpu_fprintf(f, "PSR=%08x %c%c%c%c %c %s%d\n",
                 psr,
                 psr & (1 << 31) ? 'N' : '-',
-- 
2.0.0

[Qemu-devel] [RCF PATCH 5/8] arm/nwfps: remove use of cpsr_write() and set flags directly

[Alex Bennée]

This is a pre-cursor to removing the cpsr_write function.

diff --git a/linux-user/arm/nwfpe/fpa11.h b/linux-user/arm/nwfpe/fpa11.h
index bb9ac65..0dbdf75 100644
--- a/linux-user/arm/nwfpe/fpa11.h
+++ b/linux-user/arm/nwfpe/fpa11.h
@@ -108,7 +108,10 @@ static inline void writeRegister(unsigned int x, unsigned int y)
 
 static inline void writeConditionCodes(unsigned int x)
 {
-        cpsr_write(user_registers,x,CPSR_NZCV);
+        user_registers->ZF = (~val) & CPSR_Z;
+        user_registers->NF = val;
+        user_registers->CF = (val >> 29) & 1;
+        user_registers->VF = (val << 3) & 0x80000000;
 }
 
 #define ARM_REG_PC 15
-- 
2.0.0

[Qemu-devel] [RCF PATCH 6/8] linux-user/main.c: __kernel_cmpxchg set env->CF directly

[Alex Bennée]

As we only need to manipulate the single flag do it directly though env.

---

Q: should we add a new flags only access functions?

diff --git a/linux-user/main.c b/linux-user/main.c
index 22d0197..5d9d5f7 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -469,7 +469,7 @@ void cpu_loop(CPUX86State *env)
 static void arm_kernel_cmpxchg64_helper(CPUARMState *env)
 {
     uint64_t oldval, newval, val;
-    uint32_t addr, cpsr;
+    uint32_t addr;
     target_siginfo_t info;
 
     /* Based on the 32 bit code in do_kernel_trap */
@@ -479,7 +479,6 @@ static void arm_kernel_cmpxchg64_helper(CPUARMState *env)
        operations. However things like ldrex/strex are much harder so
        there's not much point trying.  */
     start_exclusive();
-    cpsr = cpsr_read(env);
     addr = env->regs[2];
 
     if (get_user_u64(oldval, env->regs[0])) {
@@ -506,12 +505,11 @@ static void arm_kernel_cmpxchg64_helper(CPUARMState *env)
         };
 
         env->regs[0] = 0;
-        cpsr |= CPSR_C;
+        env->CF = 1;
     } else {
         env->regs[0] = -1;
-        cpsr &= ~CPSR_C;
+        env->CF = 0;
     }
-    cpsr_write(env, cpsr, CPSR_C);
     end_exclusive();
     return;
 
@@ -547,7 +545,6 @@ do_kernel_trap(CPUARMState *env)
             operations. However things like ldrex/strex are much harder so
             there's not much point trying.  */
         start_exclusive();
-        cpsr = save_state_to_spsr(env);
         addr = env->regs[2];
         /* FIXME: This should SEGV if the access fails.  */
         if (get_user_u32(val, addr))
@@ -557,12 +554,11 @@ do_kernel_trap(CPUARMState *env)
             /* FIXME: Check for segfaults.  */
             put_user_u32(val, addr);
             env->regs[0] = 0;
-            cpsr |= CPSR_C;
+            env->CF = 1;
         } else {
             env->regs[0] = -1;
-            cpsr &= ~CPSR_C;
+            env->CF = 0;
         }
-        cpsr_write(env, cpsr, CPSR_C);
         end_exclusive();
         break;
     case 0xffff0fe0: /* __kernel_get_tls */
-- 
2.0.0

[Qemu-devel] [RCF PATCH 7/8] target-arm: remove last users of cpsr_write

[Alex Bennée]

And use the new machinery to to save and restore program state. The old
cpsr_write function did some special handling for mode switches which
has been moved into the helper function.

diff --git a/linux-user/main.c b/linux-user/main.c
index 5d9d5f7..e594741 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -4179,7 +4179,7 @@ int main(int argc, char **argv, char **envp)
 #elif defined(TARGET_ARM)
     {
         int i;
-        cpsr_write(env, regs->uregs[16], 0xffffffff);
+        restore_state_from_spsr(env, regs->uregs[16]);
         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 ee2a79f..c155bbc 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -1619,41 +1619,41 @@ get_sigframe(struct target_sigaction *ka, CPUARMState *regs, int framesize)
 
 static int
 setup_return(CPUARMState *env, struct target_sigaction *ka,
-	     abi_ulong *rc, abi_ulong frame_addr, int usig, abi_ulong rc_addr)
+             abi_ulong *rc, abi_ulong frame_addr, int usig, abi_ulong rc_addr)
 {
-	abi_ulong handler = ka->_sa_handler;
-	abi_ulong retcode;
-	int thumb = handler & 1;
-	uint32_t cpsr = save_state_to_spsr(env);
+        abi_ulong handler = ka->_sa_handler;
+        abi_ulong retcode;
+        int thumb = handler & 1;
+        uint32_t cpsr = save_state_to_spsr(env);
 
-	cpsr &= ~CPSR_IT;
-	if (thumb) {
-		cpsr |= CPSR_T;
-	} else {
-		cpsr &= ~CPSR_T;
-	}
+        cpsr &= ~CPSR_IT;
+        if (thumb) {
+                cpsr |= CPSR_T;
+        } else {
+                cpsr &= ~CPSR_T;
+        }
 
-	if (ka->sa_flags & TARGET_SA_RESTORER) {
-		retcode = ka->sa_restorer;
-	} else {
-		unsigned int idx = thumb;
+        if (ka->sa_flags & TARGET_SA_RESTORER) {
+                retcode = ka->sa_restorer;
+        } else {
+                unsigned int idx = thumb;
 
-		if (ka->sa_flags & TARGET_SA_SIGINFO)
-			idx += 2;
+                if (ka->sa_flags & TARGET_SA_SIGINFO)
+                        idx += 2;
 
-		if (__put_user(retcodes[idx], rc))
-			return 1;
+                if (__put_user(retcodes[idx], rc))
+                        return 1;
 
-		retcode = rc_addr + thumb;
-	}
+                retcode = rc_addr + thumb;
+        }
 
-	env->regs[0] = usig;
-	env->regs[13] = frame_addr;
-	env->regs[14] = retcode;
-	env->regs[15] = handler & (thumb ? ~1 : ~3);
-	cpsr_write(env, cpsr, 0xffffffff);
+        env->regs[0] = usig;
+        env->regs[13] = frame_addr;
+        env->regs[14] = retcode;
+        env->regs[15] = handler & (thumb ? ~1 : ~3);
+        restore_state_from_spsr(env, cpsr);
 
-	return 0;
+        return 0;
 }
 
 static abi_ulong *setup_sigframe_v2_vfp(abi_ulong *regspace, CPUARMState *env)
@@ -1885,7 +1885,9 @@ restore_sigcontext(CPUARMState *env, struct target_sigcontext *sc)
 	__get_user_error(env->regs[15], &sc->arm_pc, err);
 #ifdef TARGET_CONFIG_CPU_32
 	__get_user_error(cpsr, &sc->arm_cpsr, err);
-        cpsr_write(env, cpsr, CPSR_USER | CPSR_EXEC);
+        cpsr &= (CPSR_USER | CPSR_EXEC);
+        cpsr |= (save_state_to_spsr(env) & ~(CPSR_USER | CPSR_EXEC));
+        restore_state_from_spsr(cpsr);
 #endif
 
 	err |= !valid_user_regs(env);
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index cfdc1cb..c4727f7 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -474,14 +474,6 @@ static inline void pstate_write(CPUARMState *env, uint32_t val)
     env->pstate = val & ~AARCH64_CACHED_PSTATE_BITS;
 }
 
-/* ARMv7-AR ARM B1.3.3 Current Program Status Register, CPSR
- *
- * Unlike the above PSTATE implementation these functions will attempt
- * to switch processor mode when the M[4:0] bits are set.
- *
- * Note that some bits of mask must be all-set or all-clear.  */
-void cpsr_write(CPUARMState *env, uint32_t val, uint32_t mask);
-
 /* ARMv7-M ARM B1.4.2, special purpose program status register xPSR */
 static inline uint32_t xpsr_read(CPUARMState *env)
 {
diff --git a/target-arm/gdbstub.c b/target-arm/gdbstub.c
index ec25f30..5e60589 100644
--- a/target-arm/gdbstub.c
+++ b/target-arm/gdbstub.c
@@ -93,8 +93,12 @@ 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
+         * FIXME?: as restore_state_from_spsr() doesn't do aarch32
+         * special mode fixups this may break. However GDB doesn't
+         * seem to be able to handle tracing over a mode switch anyway
+         */
+        restore_state_from_spsr(env, tmp);
         return 4;
     }
     /* Unknown register.  */
diff --git a/target-arm/helper.c b/target-arm/helper.c
index e23cec4..517764e 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -2959,68 +2959,6 @@ 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)
-{
-    /* 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).
-     */
-    switch (mode) {
-    case ARM_CPU_MODE_USR:
-    case ARM_CPU_MODE_SYS:
-    case ARM_CPU_MODE_SVC:
-    case ARM_CPU_MODE_ABT:
-    case ARM_CPU_MODE_UND:
-    case ARM_CPU_MODE_IRQ:
-    case ARM_CPU_MODE_FIQ:
-        return 0;
-    default:
-        return 1;
-    }
-}
-
-void cpsr_write(CPUARMState *env, uint32_t val, uint32_t mask)
-{
-    if (mask & CPSR_NZCV) {
-        env->ZF = (~val) & CPSR_Z;
-        env->NF = val;
-        env->CF = (val >> 29) & 1;
-        env->VF = (val << 3) & 0x80000000;
-    }
-    if (mask & CPSR_Q)
-        env->QF = ((val & CPSR_Q) != 0);
-    if (mask & CPSR_T)
-        env->thumb = ((val & CPSR_T) != 0);
-    if (mask & CPSR_IT_0_1) {
-        env->condexec_bits &= ~3;
-        env->condexec_bits |= (val >> 25) & 3;
-    }
-    if (mask & CPSR_IT_2_7) {
-        env->condexec_bits &= 3;
-        env->condexec_bits |= (val >> 8) & 0xfc;
-    }
-    if (mask & CPSR_GE) {
-        env->GE = (val >> 16) & 0xf;
-    }
-
-    env->daif &= ~(CPSR_AIF & mask);
-    env->daif |= val & CPSR_AIF & mask;
-
-    if ((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.
-             */
-            mask &= ~CPSR_M;
-        } else {
-            switch_mode(env, val & CPSR_M);
-        }
-    }
-    mask &= ~CACHED_CPSR_BITS;
-    env->uncached_cpsr = (env->uncached_cpsr & ~mask) | (val & mask);
-}
-
 /* Sign/zero extend */
 uint32_t HELPER(sxtb16)(uint32_t x)
 {
diff --git a/target-arm/kvm32.c b/target-arm/kvm32.c
index de3063f..efe7b5a 100644
--- a/target-arm/kvm32.c
+++ b/target-arm/kvm32.c
@@ -465,7 +465,7 @@ int kvm_arch_get_registers(CPUState *cs)
     if (ret) {
         return ret;
     }
-    cpsr_write(env, cpsr, 0xffffffff);
+    restore_state_from_spsr(env, cpsr);
 
     /* Make sure the current mode regs are properly set */
     mode = env->uncached_cpsr & CPSR_M;
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index b58bcdf..dca0988 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -276,9 +276,47 @@ uint32_t HELPER(cpsr_read)(CPUARMState *env)
     return save_state_to_spsr(env) & ~CPSR_EXEC;
 }
 
+static int bad_mode_switch(CPUARMState *env, int mode)
+{
+    /* 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).
+     */
+    switch (mode) {
+    case ARM_CPU_MODE_USR:
+    case ARM_CPU_MODE_SYS:
+    case ARM_CPU_MODE_SVC:
+    case ARM_CPU_MODE_ABT:
+    case ARM_CPU_MODE_UND:
+    case ARM_CPU_MODE_IRQ:
+    case ARM_CPU_MODE_FIQ:
+        return 0;
+    default:
+        return 1;
+    }
+}
+
 void HELPER(cpsr_write)(CPUARMState *env, uint32_t val, uint32_t mask)
 {
-    cpsr_write(env, val, mask);
+    uint32_t current_cpsr = save_state_to_spsr(env);
+    uint32_t new_cpsr;
+
+    /* we may be triggering a mode change */
+    if ((current_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.
+             */
+            mask &= ~CPSR_M;
+        } else {
+            switch_mode(env, val & CPSR_M);
+        }
+    }
+
+    new_cpsr = current_cpsr & ~mask;
+    new_cpsr |= (val & mask);
+    restore_state_from_spsr(env, new_cpsr);
 }
 
 /* Access to user mode registers from privileged modes.  */
-- 
2.0.0

[Qemu-devel] [RCF PATCH 8/8] target-arm: remove final users of pstate_write

[Alex Bennée]

This converts all users of pstate_write to use the common state
save/restore functionality.

diff --git a/linux-user/signal.c b/linux-user/signal.c
index c155bbc..48885e2 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -1290,7 +1290,7 @@ static int target_restore_sigframe(CPUARMState *env,
     __get_user(env->xregs[31], &sf->uc.tuc_mcontext.sp);
     __get_user(env->pc, &sf->uc.tuc_mcontext.pc);
     __get_user(pstate, &sf->uc.tuc_mcontext.pstate);
-    pstate_write(env, pstate);
+    restore_state_from_spsr(env, pstate);
 
     __get_user(magic, &aux->fpsimd.head.magic);
     __get_user(size, &aux->fpsimd.head.size);
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index c4727f7..e6723dc 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -460,20 +460,6 @@ int arm_cpu_handle_mmu_fault(CPUState *cpu, vaddr address, int rw,
 #define PSTATE_MODE_EL1t 4
 #define PSTATE_MODE_EL0t 0
 
-/* Update the current PSTATE value. This doesn't include nRW which
- * indicates if we are in 64 or 32 bit mode */
-static inline void pstate_write(CPUARMState *env, uint32_t val)
-{
-    g_assert(is_a64(env));
-
-    env->ZF = (~val) & PSTATE_Z;
-    env->NF = val;
-    env->CF = (val >> 29) & 1;
-    env->VF = (val << 3) & 0x80000000;
-    env->daif = val & PSTATE_DAIF;
-    env->pstate = val & ~AARCH64_CACHED_PSTATE_BITS;
-}
-
 /* ARMv7-M ARM B1.4.2, special purpose program status register xPSR */
 static inline uint32_t xpsr_read(CPUARMState *env)
 {
diff --git a/target-arm/gdbstub64.c b/target-arm/gdbstub64.c
index 76d1b91..366335a 100644
--- a/target-arm/gdbstub64.c
+++ b/target-arm/gdbstub64.c
@@ -63,7 +63,7 @@ int aarch64_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
         return 8;
     case 33:
         /* SPSR */
-        pstate_write(env, tmp);
+        restore_state_from_spsr(env, tmp);
         return 4;
     }
     /* Unknown register.  */
diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
index 1ca3164..bb48014 100644
--- a/target-arm/helper-a64.c
+++ b/target-arm/helper-a64.c
@@ -507,9 +507,8 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
         env->condexec_bits = 0;
     }
 
-    // TODO: restore_state_from_spsr()
-    env->aarch64 = 1;
-    pstate_write(env, PSTATE_DAIF | PSTATE_MODE_EL1h);
+    /* start IRQ with a clean program state */
+    restore_state_from_spsr(env, PSTATE_DAIF | PSTATE_MODE_EL1h);
 
     env->pc = addr;
     cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
index 83df952..8578fa9 100644
--- a/target-arm/kvm64.c
+++ b/target-arm/kvm64.c
@@ -222,7 +222,7 @@ int kvm_arch_get_registers(CPUState *cs)
     if (ret) {
         return ret;
     }
-    pstate_write(env, val);
+    restore_state_from_spsr(env, val);
 
     /* 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.
-- 
2.0.0

Re: [Qemu-devel] [RCF PATCH 1/8] target-arm/cpu.h: document various program state functions

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 3996 bytes --]

Did you mean to keep the commented-out asserts in xpsr_read/write?


On 2 June 2014 11:21, Alex Bennée <alex.bennee@linaro.org> wrote:

> We have a number of program state saving functions (pstate, cpsr, xpsr)
> which are dependant on the mode the CPU is in. This commit adds a little
> documentation to each function and asserts to defend against incorrect
> use.
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 8d04385..5e6df38 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -459,22 +459,34 @@ int arm_cpu_handle_mmu_fault(CPUState *cpu, vaddr
> address, int rw,
>  #define PSTATE_MODE_EL1t 4
>  #define PSTATE_MODE_EL0t 0
>
> -/* Return the current PSTATE value. For the moment we don't support
> 32<->64 bit
> - * interprocessing, so we don't attempt to sync with the cpsr state used
> by
> - * the 32 bit decoder.
> +/* ARMv8 ARM D1.7 Process state, PSTATE
> + *
> + *  31  28 27  24 23   22 21 20 22    21  20 19  16 15   8 7   5 4    0
> + * +------+------+-------+-----+--------+---+------+------+-----+------+
> + * | NZCV | DAIF | SS IL |  EL | nRW SP | Q |  GE  |  IT  | JTE | Mode |
> + * +------+------+-------+-----+--------+---+------+------+-----+------+
> + *
> + * The PSTATE is an abstraction of a number of Return the current
> + * PSTATE value. This is only valid for A64 hardware although can be
> + * read when in AArch32 mode.
>   */
>  static inline uint32_t pstate_read(CPUARMState *env)
>  {
>      int ZF;
>
> +    g_assert(is_a64(env));
> +
>      ZF = (env->ZF == 0);
>      return (env->NF & 0x80000000) | (ZF << 30)
>          | (env->CF << 29) | ((env->VF & 0x80000000) >> 3)
>          | env->pstate | env->daif;
>  }
>
> +/* Update the current PSTATE value. This doesn't include nRW which is */
>  static inline void pstate_write(CPUARMState *env, uint32_t val)
>  {
> +    g_assert(is_a64(env));
> +
>      env->ZF = (~val) & PSTATE_Z;
>      env->NF = val;
>      env->CF = (val >> 29) & 1;
> @@ -483,15 +495,23 @@ static inline void pstate_write(CPUARMState *env,
> uint32_t val)
>      env->pstate = val & ~CACHED_PSTATE_BITS;
>  }
>
> -/* Return the current CPSR value.  */
> +/* ARMv7-AR ARM B1.3.3 Current Program Status Register, CPSR
> + *
> + * Unlike the above PSTATE implementation these functions will attempt
> + * to switch processor mode when the M[4:0] bits are set.
> + */
>  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);
>
> -/* Return the current xPSR value.  */
> +/* ARMv7-M ARM B1.4.2, special purpose program status register xPSR */
>  static inline uint32_t xpsr_read(CPUARMState *env)
>  {
>      int ZF;
> +
> +    g_assert(!is_a64(env));
> +//    g_assert(IS_M(env));
> +
>      ZF = (env->ZF == 0);
>      return (env->NF & 0x80000000) | (ZF << 30)
>          | (env->CF << 29) | ((env->VF & 0x80000000) >> 3) | (env->QF <<
> 27)
> @@ -503,6 +523,9 @@ static inline uint32_t xpsr_read(CPUARMState *env)
>  /* Set the xPSR.  Note that some bits of mask must be all-set or
> all-clear.  */
>  static inline void xpsr_write(CPUARMState *env, uint32_t val, uint32_t
> mask)
>  {
> +    g_assert(!is_a64(env));
> +//    g_assert(IS_M(env));
> +
>      if (mask & CPSR_NZCV) {
>          env->ZF = (~val) & CPSR_Z;
>          env->NF = val;
> diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
> index cccda74..48ca351 100644
> --- a/target-arm/helper-a64.c
> +++ b/target-arm/helper-a64.c
> @@ -506,8 +506,8 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>          env->condexec_bits = 0;
>      }
>
> -    pstate_write(env, PSTATE_DAIF | PSTATE_MODE_EL1h);
>      env->aarch64 = 1;
> +    pstate_write(env, PSTATE_DAIF | PSTATE_MODE_EL1h);
>
>      env->pc = addr;
>      cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
> --
> 2.0.0
>
>

[-- Attachment #2: Type: text/html, Size: 4828 bytes --]

Re: [Qemu-devel] [RCF PATCH 8/8] target-arm: remove final users of pstate_write

[Alex Bennée]

Alex Bennée writes:

> This converts all users of pstate_write to use the common state
> save/restore functionality.

Oops, now I'll clean that up. I need to decide if it worth merge the
xpsr_ stuff anyway?

There is also some commented out code in the main save_state_to_spsr()
function which I need to clean out.

-- 
Alex Bennée

Re: [Qemu-devel] [RCF PATCH 5/8] arm/nwfps: remove use of cpsr_write() and set flags directly

[Peter Maydell]

On 2 June 2014 17:21, Alex Bennée <alex.bennee@linaro.org> wrote:
> This is a pre-cursor to removing the cpsr_write function.
>
> diff --git a/linux-user/arm/nwfpe/fpa11.h b/linux-user/arm/nwfpe/fpa11.h
> index bb9ac65..0dbdf75 100644
> --- a/linux-user/arm/nwfpe/fpa11.h
> +++ b/linux-user/arm/nwfpe/fpa11.h
> @@ -108,7 +108,10 @@ static inline void writeRegister(unsigned int x, unsigned int y)
>
>  static inline void writeConditionCodes(unsigned int x)
>  {
> -        cpsr_write(user_registers,x,CPSR_NZCV);
> +        user_registers->ZF = (~val) & CPSR_Z;
> +        user_registers->NF = val;
> +        user_registers->CF = (val >> 29) & 1;
> +        user_registers->VF = (val << 3) & 0x80000000;
>  }
>
>  #define ARM_REG_PC 15

This seems like it's clearly making things worse.
We definitely don't want to have to have code in
linux-user be aware of the "interesting" definitions
of our ZF/NF/CF/VF fields.

thanks
-- PMM

Re: [Qemu-devel] [RCF PATCH 5/8] arm/nwfps: remove use of cpsr_write() and set flags directly

[Alex Bennée]

Peter Maydell writes:

> On 2 June 2014 17:21, Alex Bennée <alex.bennee@linaro.org> wrote:
>> This is a pre-cursor to removing the cpsr_write function.
>>
>> diff --git a/linux-user/arm/nwfpe/fpa11.h b/linux-user/arm/nwfpe/fpa11.h
>> index bb9ac65..0dbdf75 100644
>> --- a/linux-user/arm/nwfpe/fpa11.h
>> +++ b/linux-user/arm/nwfpe/fpa11.h
>> @@ -108,7 +108,10 @@ static inline void writeRegister(unsigned int x, unsigned int y)
>>
>>  static inline void writeConditionCodes(unsigned int x)
>>  {
>> -        cpsr_write(user_registers,x,CPSR_NZCV);
>> +        user_registers->ZF = (~val) & CPSR_Z;
>> +        user_registers->NF = val;
>> +        user_registers->CF = (val >> 29) & 1;
>> +        user_registers->VF = (val << 3) & 0x80000000;
>>  }
>>
>>  #define ARM_REG_PC 15
>
> This seems like it's clearly making things worse.
> We definitely don't want to have to have code in
> linux-user be aware of the "interesting" definitions
> of our ZF/NF/CF/VF fields.
<snip>

You are right. I could make restore_state_from_spsr use a mask like the
old cpsr_write did or as the flags are a special case have a flag only
setting function for these cases.

-- 
Alex Bennée

Re: [Qemu-devel] [RCF PATCH 5/8] arm/nwfps: remove use of cpsr_write() and set flags directly

[Peter Maydell]

On 4 June 2014 12:11, Alex Bennée <alex.bennee@linaro.org> wrote:
> Peter Maydell writes:
>> This seems like it's clearly making things worse.
>> We definitely don't want to have to have code in
>> linux-user be aware of the "interesting" definitions
>> of our ZF/NF/CF/VF fields.
> <snip>
>
> You are right. I could make restore_state_from_spsr use a mask like the
> old cpsr_write did

But restore_state_from_spsr is the "just load state, no side effects"
function which machine.c is using, isn't it? I think part of the
problem here is that you're trying to have one function do
both the "read/write like the CPU would with all the modeswitch
stuff that entails" and also "side effect free access for state
save/restore".

thanks
-- PMM