[Qemu-devel] [PATCH 0/6] ARMv7M: some simple bugfixes and cleanups

[Qemu-devel] [PATCH 0/6] ARMv7M: some simple bugfixes and cleanups

[Peter Maydell]

This set of six patches is some simple bug fixes which
I've pulled out of Michael Davidsaver's old NVIC rewrite
patchset, as an initial start on getting it upstream.
None of them are particularly exciting, but they're
self-contained so they might as well go through code
review and get into master ahead of the main rewrite.

NB the patch which pulls the FIELD macros out of register.h;
they're too useful to be only accessible in softmmu builds.

I've CC'd Michael as the original author and Liviu
as somebody interested in v7M; if either of you would
prefer not to be cc'd let me know and I'll leave you off
subsequent respins and later NVIC related patchset emails.

thanks
-- PMM

Michael Davidsaver (5):
  armv7m: MRS/MSR: handle unprivileged access
  armv7m: Replace armv7m.hack with unassigned_access handler
  armv7m: Explicit error for bad vector table
  armv7m: Fix reads of CONTROL register bit 1
  armv7m: Clear FAULTMASK on return from non-NMI exceptions

Peter Maydell (1):
  hw/registerfields.h: Pull FIELD etc macros out of hw/register.h

 include/hw/register.h       |  47 +-------------
 include/hw/registerfields.h |  60 ++++++++++++++++++
 target/arm/cpu.h            |   1 -
 target/arm/internals.h      |   7 +++
 hw/arm/armv7m.c             |   8 ---
 target/arm/cpu.c            |  28 +++++++++
 target/arm/helper.c         | 147 +++++++++++++++++++++++++++-----------------
 target/arm/machine.c        |   6 +-
 target/arm/translate.c      |  12 ++--
 9 files changed, 195 insertions(+), 121 deletions(-)
 create mode 100644 include/hw/registerfields.h

-- 
2.7.4

[Qemu-devel] [PATCH 1/6] armv7m: MRS/MSR: handle unprivileged access

[Peter Maydell]

From: Michael Davidsaver <mdavidsaver@gmail.com>

The MRS and MSR instruction handling has a number of flaws:
 * unprivileged accesses should only be able to read
   CONTROL and the xPSR subfields, and only write APSR
   (others RAZ/WI)
 * privileged access should not be able to write xPSR
   subfields other than APSR
 * accesses to unimplemented registers should log as
   guest errors, not abort QEMU

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
[PMM: rewrote commit message]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 79 +++++++++++++++++++++++++----------------------------
 1 file changed, 37 insertions(+), 42 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 7111c8c..ad23de3 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -8243,23 +8243,32 @@ hwaddr arm_cpu_get_phys_page_attrs_debug(CPUState *cs, vaddr addr,
 
 uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t reg)
 {
-    ARMCPU *cpu = arm_env_get_cpu(env);
+    uint32_t mask;
+    unsigned el = arm_current_el(env);
+
+    /* First handle registers which unprivileged can read */
+
+    switch (reg) {
+    case 0 ... 7: /* xPSR sub-fields */
+        mask = 0;
+        if ((reg & 1) && el) {
+            mask |= 0x000001ff; /* IPSR (unpriv. reads as zero) */
+        }
+        if (!(reg & 4)) {
+            mask |= 0xf8000000; /* APSR */
+        }
+        /* EPSR reads as zero */
+        return xpsr_read(env) & mask;
+        break;
+    case 20: /* CONTROL */
+        return env->v7m.control;
+    }
+
+    if (el == 0) {
+        return 0; /* unprivileged reads others as zero */
+    }
 
     switch (reg) {
-    case 0: /* APSR */
-        return xpsr_read(env) & 0xf8000000;
-    case 1: /* IAPSR */
-        return xpsr_read(env) & 0xf80001ff;
-    case 2: /* EAPSR */
-        return xpsr_read(env) & 0xff00fc00;
-    case 3: /* xPSR */
-        return xpsr_read(env) & 0xff00fdff;
-    case 5: /* IPSR */
-        return xpsr_read(env) & 0x000001ff;
-    case 6: /* EPSR */
-        return xpsr_read(env) & 0x0700fc00;
-    case 7: /* IEPSR */
-        return xpsr_read(env) & 0x0700edff;
     case 8: /* MSP */
         return env->v7m.current_sp ? env->v7m.other_sp : env->regs[13];
     case 9: /* PSP */
@@ -8271,40 +8280,26 @@ uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t reg)
         return env->v7m.basepri;
     case 19: /* FAULTMASK */
         return (env->daif & PSTATE_F) != 0;
-    case 20: /* CONTROL */
-        return env->v7m.control;
     default:
-        /* ??? For debugging only.  */
-        cpu_abort(CPU(cpu), "Unimplemented system register read (%d)\n", reg);
+        qemu_log_mask(LOG_GUEST_ERROR, "Attempt to read unknown special"
+                                       " register %d\n", reg);
         return 0;
     }
 }
 
 void HELPER(v7m_msr)(CPUARMState *env, uint32_t reg, uint32_t val)
 {
-    ARMCPU *cpu = arm_env_get_cpu(env);
+    if (arm_current_el(env) == 0 && reg > 7) {
+        /* only xPSR sub-fields may be written by unprivileged */
+        return;
+    }
 
     switch (reg) {
-    case 0: /* APSR */
-        xpsr_write(env, val, 0xf8000000);
-        break;
-    case 1: /* IAPSR */
-        xpsr_write(env, val, 0xf8000000);
-        break;
-    case 2: /* EAPSR */
-        xpsr_write(env, val, 0xfe00fc00);
-        break;
-    case 3: /* xPSR */
-        xpsr_write(env, val, 0xfe00fc00);
-        break;
-    case 5: /* IPSR */
-        /* IPSR bits are readonly.  */
-        break;
-    case 6: /* EPSR */
-        xpsr_write(env, val, 0x0600fc00);
-        break;
-    case 7: /* IEPSR */
-        xpsr_write(env, val, 0x0600fc00);
+    case 0 ... 7: /* xPSR sub-fields */
+        /* only APSR is actually writable */
+        if (reg & 4) {
+            xpsr_write(env, val, 0xf8000000); /* APSR */
+        }
         break;
     case 8: /* MSP */
         if (env->v7m.current_sp)
@@ -8345,8 +8340,8 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t reg, uint32_t val)
         switch_v7m_sp(env, (val & 2) != 0);
         break;
     default:
-        /* ??? For debugging only.  */
-        cpu_abort(CPU(cpu), "Unimplemented system register write (%d)\n", reg);
+        qemu_log_mask(LOG_GUEST_ERROR, "Attempt to write unknown special"
+                                       " register %d\n", reg);
         return;
     }
 }
-- 
2.7.4

[Qemu-devel] [PATCH 2/6] armv7m: Replace armv7m.hack with unassigned_access handler

[Peter Maydell]

From: Michael Davidsaver <mdavidsaver@gmail.com>

For v7m we need to catch attempts to execute from special
addresses at 0xfffffff0 and above. Previously we did this
with the aid of a hacky special purpose lump of memory
in the address space and a check in translate.c for whether
we were translating code at those addresses.

We can implement this more cleanly using a CPU
unassigned access handler which throws the exception
if the unassigned access is for one of the special addresses.

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
[PMM:
 * drop the deletion of the "don't interrupt if PC is magic"
   code in arm_v7m_cpu_exec_interrupt() -- this is still
   required
 * don't generate an exception for unassigned accesses
   which aren't to the magic address -- although doing
   this is in theory correct in practice it will break
   currently working guests which rely on the RAZ/WI
   behaviour when they touch devices which we haven't
   modelled.
 * trigger EXCP_EXCEPTION_EXIT on is_exec, not !is_write
]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/armv7m.c        |  8 --------
 target/arm/cpu.c       | 28 ++++++++++++++++++++++++++++
 target/arm/translate.c | 12 ++++++------
 3 files changed, 34 insertions(+), 14 deletions(-)

diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index 49d3078..0c9ca7b 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -180,7 +180,6 @@ DeviceState *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
     uint64_t entry;
     uint64_t lowaddr;
     int big_endian;
-    MemoryRegion *hack = g_new(MemoryRegion, 1);
 
     if (cpu_model == NULL) {
 	cpu_model = "cortex-m3";
@@ -225,13 +224,6 @@ DeviceState *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
         }
     }
 
-    /* Hack to map an additional page of ram at the top of the address
-       space.  This stops qemu complaining about executing code outside RAM
-       when returning from an exception.  */
-    memory_region_init_ram(hack, NULL, "armv7m.hack", 0x1000, &error_fatal);
-    vmstate_register_ram_global(hack);
-    memory_region_add_subregion(system_memory, 0xfffff000, hack);
-
     qemu_register_reset(armv7m_reset, cpu);
     return nvic;
 }
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 3f2cdb6..47759c9 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -292,6 +292,33 @@ bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 }
 
 #if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64)
+static void arm_v7m_unassigned_access(CPUState *cpu, hwaddr addr,
+                                      bool is_write, bool is_exec, int opaque,
+                                      unsigned size)
+{
+    ARMCPU *arm = ARM_CPU(cpu);
+    CPUARMState *env = &arm->env;
+
+    /* ARMv7-M interrupt return works by loading a magic value into the PC.
+     * On real hardware the load causes the return to occur.  The qemu
+     * implementation performs the jump normally, then does the exception
+     * return by throwing a special exception when when the CPU tries to
+     * execute code at the magic address.
+     */
+    if (env->v7m.exception != 0 && addr >= 0xfffffff0 && is_exec) {
+        cpu->exception_index = EXCP_EXCEPTION_EXIT;
+        cpu_loop_exit(cpu);
+    }
+
+    /* In real hardware an attempt to access parts of the address space
+     * with nothing there will usually cause an external abort.
+     * However our QEMU board models are often missing device models where
+     * the guest can boot anyway with the default read-as-zero/writes-ignored
+     * behaviour that you get without a QEMU unassigned_access hook.
+     * So just return here to retain that default behaviour.
+     */
+}
+
 static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 {
     CPUClass *cc = CPU_GET_CLASS(cs);
@@ -1016,6 +1043,7 @@ static void arm_v7m_class_init(ObjectClass *oc, void *data)
     cc->do_interrupt = arm_v7m_cpu_do_interrupt;
 #endif
 
+    cc->do_unassigned_access = arm_v7m_unassigned_access;
     cc->cpu_exec_interrupt = arm_v7m_cpu_exec_interrupt;
 }
 
diff --git a/target/arm/translate.c b/target/arm/translate.c
index c9186b6..a7c2abe 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -11719,12 +11719,12 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)
             break;
         }
 #else
-        if (dc->pc >= 0xfffffff0 && arm_dc_feature(dc, ARM_FEATURE_M)) {
-            /* We always get here via a jump, so know we are not in a
-               conditional execution block.  */
-            gen_exception_internal(EXCP_EXCEPTION_EXIT);
-            dc->is_jmp = DISAS_EXC;
-            break;
+        if (arm_dc_feature(dc, ARM_FEATURE_M)) {
+            /* Branches to the magic exception-return addresses should
+             * already have been caught via the arm_v7m_unassigned_access hook,
+             * and never get here.
+             */
+            assert(dc->pc < 0xfffffff0);
         }
 #endif
 
-- 
2.7.4

[Qemu-devel] [PATCH 3/6] armv7m: Explicit error for bad vector table

[Peter Maydell]

From: Michael Davidsaver <mdavidsaver@gmail.com>

Give an explicit error and abort when a load
from the vector table fails. Architecturally this
should HardFault (which will then immediately
fail to load the HardFault vector and go into Lockup).
Since we don't model Lockup, just report this guest
error via cpu_abort(). This is more helpful than the
previous behaviour of reading a zero, which is the
address of the reset stack pointer and not a sensible
location to jump to.

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
[PMM: expanded commit message]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index ad23de3..8edb08c 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6014,6 +6014,30 @@ static void arm_log_exception(int idx)
     }
 }
 
+static uint32_t arm_v7m_load_vector(ARMCPU *cpu)
+
+{
+    CPUState *cs = CPU(cpu);
+    CPUARMState *env = &cpu->env;
+    MemTxResult result;
+    hwaddr vec = env->v7m.vecbase + env->v7m.exception * 4;
+    uint32_t addr;
+
+    addr = address_space_ldl(cs->as, vec,
+                             MEMTXATTRS_UNSPECIFIED, &result);
+    if (result != MEMTX_OK) {
+        /* Architecturally this should cause a HardFault setting HSFR.VECTTBL,
+         * which would then be immediately followed by our failing to load
+         * the entry vector for that HardFault, which is a Lockup case.
+         * Since we don't model Lockup, we just report this guest error
+         * via cpu_abort().
+         */
+        cpu_abort(cs, "Failed to read from exception vector table "
+                  "entry %08x\n", (unsigned)vec);
+    }
+    return addr;
+}
+
 void arm_v7m_cpu_do_interrupt(CPUState *cs)
 {
     ARMCPU *cpu = ARM_CPU(cs);
@@ -6095,7 +6119,7 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
     /* Clear IT bits */
     env->condexec_bits = 0;
     env->regs[14] = lr;
-    addr = ldl_phys(cs->as, env->v7m.vecbase + env->v7m.exception * 4);
+    addr = arm_v7m_load_vector(cpu);
     env->regs[15] = addr & 0xfffffffe;
     env->thumb = addr & 1;
 }
-- 
2.7.4

[Qemu-devel] [PATCH 4/6] hw/registerfields.h: Pull FIELD etc macros out of hw/register.h

[Peter Maydell]

hw/register.h provides macros like FIELD which make it easy to define
shift, mask and length constants for the fields within a register.
Unfortunately register.h also includes a lot of other things, some
of which will only compile in the softmmu build.

Pull the FIELD macro and friends out into a separate header file,
so they can be used in places like target/arm files which also
get built in the user-only configs.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/register.h       | 47 +----------------------------------
 include/hw/registerfields.h | 60 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+), 46 deletions(-)
 create mode 100644 include/hw/registerfields.h

diff --git a/include/hw/register.h b/include/hw/register.h
index 8c12233..8bff5fb 100644
--- a/include/hw/register.h
+++ b/include/hw/register.h
@@ -13,6 +13,7 @@
 
 #include "hw/qdev-core.h"
 #include "exec/memory.h"
+#include "hw/registerfields.h"
 
 typedef struct RegisterInfo RegisterInfo;
 typedef struct RegisterAccessInfo RegisterAccessInfo;
@@ -206,50 +207,4 @@ RegisterInfoArray *register_init_block32(DeviceState *owner,
 
 void register_finalize_block(RegisterInfoArray *r_array);
 
-/* Define constants for a 32 bit register */
-
-/* This macro will define A_FOO, for the byte address of a register
- * as well as R_FOO for the uint32_t[] register number (A_FOO / 4).
- */
-#define REG32(reg, addr)                                                  \
-    enum { A_ ## reg = (addr) };                                          \
-    enum { R_ ## reg = (addr) / 4 };
-
-/* Define SHIFT, LENGTH and MASK constants for a field within a register */
-
-/* This macro will define FOO_BAR_MASK, FOO_BAR_SHIFT and FOO_BAR_LENGTH 
- * constants for field BAR in register FOO.
- */
-#define FIELD(reg, field, shift, length)                                  \
-    enum { R_ ## reg ## _ ## field ## _SHIFT = (shift)};                  \
-    enum { R_ ## reg ## _ ## field ## _LENGTH = (length)};                \
-    enum { R_ ## reg ## _ ## field ## _MASK =                             \
-                                        MAKE_64BIT_MASK(shift, length)};
-
-/* Extract a field from a register */
-#define FIELD_EX32(storage, reg, field)                                   \
-    extract32((storage), R_ ## reg ## _ ## field ## _SHIFT,               \
-              R_ ## reg ## _ ## field ## _LENGTH)
-
-/* Extract a field from an array of registers */
-#define ARRAY_FIELD_EX32(regs, reg, field)                                \
-    FIELD_EX32((regs)[R_ ## reg], reg, field)
-
-/* Deposit a register field.
- * Assigning values larger then the target field will result in
- * compilation warnings.
- */
-#define FIELD_DP32(storage, reg, field, val) ({                           \
-    struct {                                                              \
-        unsigned int v:R_ ## reg ## _ ## field ## _LENGTH;                \
-    } v = { .v = val };                                                   \
-    uint32_t d;                                                           \
-    d = deposit32((storage), R_ ## reg ## _ ## field ## _SHIFT,           \
-                  R_ ## reg ## _ ## field ## _LENGTH, v.v);               \
-    d; })
-
-/* Deposit a field to array of registers.  */
-#define ARRAY_FIELD_DP32(regs, reg, field, val)                           \
-    (regs)[R_ ## reg] = FIELD_DP32((regs)[R_ ## reg], reg, field, val);
-
 #endif
diff --git a/include/hw/registerfields.h b/include/hw/registerfields.h
new file mode 100644
index 0000000..af101d5
--- /dev/null
+++ b/include/hw/registerfields.h
@@ -0,0 +1,60 @@
+/*
+ * Register Definition API: field macros
+ *
+ * Copyright (c) 2016 Xilinx Inc.
+ * Copyright (c) 2013 Peter Crosthwaite <peter.crosthwaite@xilinx.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ */
+
+#ifndef REGISTERFIELDS_H
+#define REGISTERFIELDS_H
+
+/* Define constants for a 32 bit register */
+
+/* This macro will define A_FOO, for the byte address of a register
+ * as well as R_FOO for the uint32_t[] register number (A_FOO / 4).
+ */
+#define REG32(reg, addr)                                                  \
+    enum { A_ ## reg = (addr) };                                          \
+    enum { R_ ## reg = (addr) / 4 };
+
+/* Define SHIFT, LENGTH and MASK constants for a field within a register */
+
+/* This macro will define FOO_BAR_MASK, FOO_BAR_SHIFT and FOO_BAR_LENGTH 
+ * constants for field BAR in register FOO.
+ */
+#define FIELD(reg, field, shift, length)                                  \
+    enum { R_ ## reg ## _ ## field ## _SHIFT = (shift)};                  \
+    enum { R_ ## reg ## _ ## field ## _LENGTH = (length)};                \
+    enum { R_ ## reg ## _ ## field ## _MASK =                             \
+                                        MAKE_64BIT_MASK(shift, length)};
+
+/* Extract a field from a register */
+#define FIELD_EX32(storage, reg, field)                                   \
+    extract32((storage), R_ ## reg ## _ ## field ## _SHIFT,               \
+              R_ ## reg ## _ ## field ## _LENGTH)
+
+/* Extract a field from an array of registers */
+#define ARRAY_FIELD_EX32(regs, reg, field)                                \
+    FIELD_EX32((regs)[R_ ## reg], reg, field)
+
+/* Deposit a register field.
+ * Assigning values larger then the target field will result in
+ * compilation warnings.
+ */
+#define FIELD_DP32(storage, reg, field, val) ({                           \
+    struct {                                                              \
+        unsigned int v:R_ ## reg ## _ ## field ## _LENGTH;                \
+    } v = { .v = val };                                                   \
+    uint32_t d;                                                           \
+    d = deposit32((storage), R_ ## reg ## _ ## field ## _SHIFT,           \
+                  R_ ## reg ## _ ## field ## _LENGTH, v.v);               \
+    d; })
+
+/* Deposit a field to array of registers.  */
+#define ARRAY_FIELD_DP32(regs, reg, field, val)                           \
+    (regs)[R_ ## reg] = FIELD_DP32((regs)[R_ ## reg], reg, field, val);
+
+#endif
-- 
2.7.4

[Qemu-devel] [PATCH 5/6] armv7m: Fix reads of CONTROL register bit 1

[Peter Maydell]

From: Michael Davidsaver <mdavidsaver@gmail.com>

The v7m CONTROL register bit 1 is SPSEL, which indicates
the stack being used. We were storing this information
not in v7m.control but in the separate v7m.other_sp
structure field. Unfortunately, the code handling reads
of the CONTROL register didn't take account of this, and
so if SPSEL was updated by an exception entry or exit then
a subsequent guest read of CONTROL would get the wrong value.

Using a separate structure field doesn't really gain us
anything in efficiency, so drop this unnecessary complexity
in favour of simply storing all the bits in v7m.control.

This is a migration compatibility break for M profile
CPUs only.

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
[PMM: rewrote commit message;
 use deposit32(); use FIELD to define constants for
 masking and shifting of CONTROL register fields
]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.h       |  1 -
 target/arm/internals.h |  7 +++++++
 target/arm/helper.c    | 35 +++++++++++++++++++++++------------
 target/arm/machine.c   |  6 ++----
 4 files changed, 32 insertions(+), 17 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 151a5d7..521c11b 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -405,7 +405,6 @@ typedef struct CPUARMState {
         uint32_t vecbase;
         uint32_t basepri;
         uint32_t control;
-        int current_sp;
         int exception;
     } v7m;
 
diff --git a/target/arm/internals.h b/target/arm/internals.h
index 3cae5ff..2e65bc1 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -25,6 +25,8 @@
 #ifndef TARGET_ARM_INTERNALS_H
 #define TARGET_ARM_INTERNALS_H
 
+#include "hw/registerfields.h"
+
 /* register banks for CPU modes */
 #define BANK_USRSYS 0
 #define BANK_SVC    1
@@ -75,6 +77,11 @@ static const char * const excnames[] = {
  */
 #define GTIMER_SCALE 16
 
+/* Bit definitions for the v7M CONTROL register */
+FIELD(V7M_CONTROL, NPRIV, 0, 1)
+FIELD(V7M_CONTROL, SPSEL, 1, 1)
+FIELD(V7M_CONTROL, FPCA, 2, 1)
+
 /*
  * For AArch64, map a given EL to an index in the banked_spsr array.
  * Note that this mapping and the AArch32 mapping defined in bank_number()
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 8edb08c..dc383d1 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5947,14 +5947,19 @@ static uint32_t v7m_pop(CPUARMState *env)
 }
 
 /* Switch to V7M main or process stack pointer.  */
-static void switch_v7m_sp(CPUARMState *env, int process)
+static void switch_v7m_sp(CPUARMState *env, bool new_spsel)
 {
     uint32_t tmp;
-    if (env->v7m.current_sp != process) {
+    bool old_spsel = env->v7m.control & R_V7M_CONTROL_SPSEL_MASK;
+
+    if (old_spsel != new_spsel) {
         tmp = env->v7m.other_sp;
         env->v7m.other_sp = env->regs[13];
         env->regs[13] = tmp;
-        env->v7m.current_sp = process;
+
+        env->v7m.control = deposit32(env->v7m.control,
+                                     R_V7M_CONTROL_SPSEL_SHIFT,
+                                     R_V7M_CONTROL_SPSEL_LENGTH, new_spsel);
     }
 }
 
@@ -6049,8 +6054,9 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
     arm_log_exception(cs->exception_index);
 
     lr = 0xfffffff1;
-    if (env->v7m.current_sp)
+    if (env->v7m.control & R_V7M_CONTROL_SPSEL_MASK) {
         lr |= 4;
+    }
     if (env->v7m.exception == 0)
         lr |= 8;
 
@@ -8294,9 +8300,11 @@ uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t reg)
 
     switch (reg) {
     case 8: /* MSP */
-        return env->v7m.current_sp ? env->v7m.other_sp : env->regs[13];
+        return (env->v7m.control & R_V7M_CONTROL_SPSEL_MASK) ?
+            env->v7m.other_sp : env->regs[13];
     case 9: /* PSP */
-        return env->v7m.current_sp ? env->regs[13] : env->v7m.other_sp;
+        return (env->v7m.control & R_V7M_CONTROL_SPSEL_MASK) ?
+            env->regs[13] : env->v7m.other_sp;
     case 16: /* PRIMASK */
         return (env->daif & PSTATE_I) != 0;
     case 17: /* BASEPRI */
@@ -8326,16 +8334,18 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t reg, uint32_t val)
         }
         break;
     case 8: /* MSP */
-        if (env->v7m.current_sp)
+        if (env->v7m.control & R_V7M_CONTROL_SPSEL_MASK) {
             env->v7m.other_sp = val;
-        else
+        } else {
             env->regs[13] = val;
+        }
         break;
     case 9: /* PSP */
-        if (env->v7m.current_sp)
+        if (env->v7m.control & R_V7M_CONTROL_SPSEL_MASK) {
             env->regs[13] = val;
-        else
+        } else {
             env->v7m.other_sp = val;
+        }
         break;
     case 16: /* PRIMASK */
         if (val & 1) {
@@ -8360,8 +8370,9 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t reg, uint32_t val)
         }
         break;
     case 20: /* CONTROL */
-        env->v7m.control = val & 3;
-        switch_v7m_sp(env, (val & 2) != 0);
+        switch_v7m_sp(env, (val & R_V7M_CONTROL_SPSEL_MASK) != 0);
+        env->v7m.control = val & (R_V7M_CONTROL_SPSEL_MASK |
+                                  R_V7M_CONTROL_NPRIV_MASK);
         break;
     default:
         qemu_log_mask(LOG_GUEST_ERROR, "Attempt to write unknown special"
diff --git a/target/arm/machine.c b/target/arm/machine.c
index d90943b..8ed24bf 100644
--- a/target/arm/machine.c
+++ b/target/arm/machine.c
@@ -96,15 +96,13 @@ static bool m_needed(void *opaque)
 
 static const VMStateDescription vmstate_m = {
     .name = "cpu/m",
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .needed = m_needed,
     .fields = (VMStateField[]) {
-        VMSTATE_UINT32(env.v7m.other_sp, ARMCPU),
         VMSTATE_UINT32(env.v7m.vecbase, ARMCPU),
         VMSTATE_UINT32(env.v7m.basepri, ARMCPU),
         VMSTATE_UINT32(env.v7m.control, ARMCPU),
-        VMSTATE_INT32(env.v7m.current_sp, ARMCPU),
         VMSTATE_INT32(env.v7m.exception, ARMCPU),
         VMSTATE_END_OF_LIST()
     }
-- 
2.7.4

[Qemu-devel] [PATCH 6/6] armv7m: Clear FAULTMASK on return from non-NMI exceptions

[Peter Maydell]

From: Michael Davidsaver <mdavidsaver@gmail.com>

FAULTMASK must be cleared on return from all
exceptions other than NMI.

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index dc383d1..cfbc622 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5969,8 +5969,13 @@ static void do_v7m_exception_exit(CPUARMState *env)
     uint32_t xpsr;
 
     type = env->regs[15];
-    if (env->v7m.exception != 0)
+    if (env->v7m.exception != ARMV7M_EXCP_NMI) {
+        /* Auto-clear FAULTMASK on return from other than NMI */
+        env->daif &= ~PSTATE_F;
+    }
+    if (env->v7m.exception != 0) {
         armv7m_nvic_complete_irq(env->nvic, env->v7m.exception);
+    }
 
     /* Switch to the target stack.  */
     switch_v7m_sp(env, (type & 4) != 0);
-- 
2.7.4

Re: [Qemu-devel] [PATCH 4/6] hw/registerfields.h: Pull FIELD etc macros out of hw/register.h

[Alistair Francis]

On Fri, Jan 20, 2017 at 10:44 AM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> hw/register.h provides macros like FIELD which make it easy to define
> shift, mask and length constants for the fields within a register.
> Unfortunately register.h also includes a lot of other things, some
> of which will only compile in the softmmu build.
>
> Pull the FIELD macro and friends out into a separate header file,
> so they can be used in places like target/arm files which also
> get built in the user-only configs.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Looks good to me, I'm glad this is being used by others :)

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,

Alistair

> ---
>  include/hw/register.h       | 47 +----------------------------------
>  include/hw/registerfields.h | 60 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 61 insertions(+), 46 deletions(-)
>  create mode 100644 include/hw/registerfields.h
>
> diff --git a/include/hw/register.h b/include/hw/register.h
> index 8c12233..8bff5fb 100644
> --- a/include/hw/register.h
> +++ b/include/hw/register.h
> @@ -13,6 +13,7 @@
>
>  #include "hw/qdev-core.h"
>  #include "exec/memory.h"
> +#include "hw/registerfields.h"
>
>  typedef struct RegisterInfo RegisterInfo;
>  typedef struct RegisterAccessInfo RegisterAccessInfo;
> @@ -206,50 +207,4 @@ RegisterInfoArray *register_init_block32(DeviceState *owner,
>
>  void register_finalize_block(RegisterInfoArray *r_array);
>
> -/* Define constants for a 32 bit register */
> -
> -/* This macro will define A_FOO, for the byte address of a register
> - * as well as R_FOO for the uint32_t[] register number (A_FOO / 4).
> - */
> -#define REG32(reg, addr)                                                  \
> -    enum { A_ ## reg = (addr) };                                          \
> -    enum { R_ ## reg = (addr) / 4 };
> -
> -/* Define SHIFT, LENGTH and MASK constants for a field within a register */
> -
> -/* This macro will define FOO_BAR_MASK, FOO_BAR_SHIFT and FOO_BAR_LENGTH
> - * constants for field BAR in register FOO.
> - */
> -#define FIELD(reg, field, shift, length)                                  \
> -    enum { R_ ## reg ## _ ## field ## _SHIFT = (shift)};                  \
> -    enum { R_ ## reg ## _ ## field ## _LENGTH = (length)};                \
> -    enum { R_ ## reg ## _ ## field ## _MASK =                             \
> -                                        MAKE_64BIT_MASK(shift, length)};
> -
> -/* Extract a field from a register */
> -#define FIELD_EX32(storage, reg, field)                                   \
> -    extract32((storage), R_ ## reg ## _ ## field ## _SHIFT,               \
> -              R_ ## reg ## _ ## field ## _LENGTH)
> -
> -/* Extract a field from an array of registers */
> -#define ARRAY_FIELD_EX32(regs, reg, field)                                \
> -    FIELD_EX32((regs)[R_ ## reg], reg, field)
> -
> -/* Deposit a register field.
> - * Assigning values larger then the target field will result in
> - * compilation warnings.
> - */
> -#define FIELD_DP32(storage, reg, field, val) ({                           \
> -    struct {                                                              \
> -        unsigned int v:R_ ## reg ## _ ## field ## _LENGTH;                \
> -    } v = { .v = val };                                                   \
> -    uint32_t d;                                                           \
> -    d = deposit32((storage), R_ ## reg ## _ ## field ## _SHIFT,           \
> -                  R_ ## reg ## _ ## field ## _LENGTH, v.v);               \
> -    d; })
> -
> -/* Deposit a field to array of registers.  */
> -#define ARRAY_FIELD_DP32(regs, reg, field, val)                           \
> -    (regs)[R_ ## reg] = FIELD_DP32((regs)[R_ ## reg], reg, field, val);
> -
>  #endif
> diff --git a/include/hw/registerfields.h b/include/hw/registerfields.h
> new file mode 100644
> index 0000000..af101d5
> --- /dev/null
> +++ b/include/hw/registerfields.h
> @@ -0,0 +1,60 @@
> +/*
> + * Register Definition API: field macros
> + *
> + * Copyright (c) 2016 Xilinx Inc.
> + * Copyright (c) 2013 Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + */
> +
> +#ifndef REGISTERFIELDS_H
> +#define REGISTERFIELDS_H
> +
> +/* Define constants for a 32 bit register */
> +
> +/* This macro will define A_FOO, for the byte address of a register
> + * as well as R_FOO for the uint32_t[] register number (A_FOO / 4).
> + */
> +#define REG32(reg, addr)                                                  \
> +    enum { A_ ## reg = (addr) };                                          \
> +    enum { R_ ## reg = (addr) / 4 };
> +
> +/* Define SHIFT, LENGTH and MASK constants for a field within a register */
> +
> +/* This macro will define FOO_BAR_MASK, FOO_BAR_SHIFT and FOO_BAR_LENGTH
> + * constants for field BAR in register FOO.
> + */
> +#define FIELD(reg, field, shift, length)                                  \
> +    enum { R_ ## reg ## _ ## field ## _SHIFT = (shift)};                  \
> +    enum { R_ ## reg ## _ ## field ## _LENGTH = (length)};                \
> +    enum { R_ ## reg ## _ ## field ## _MASK =                             \
> +                                        MAKE_64BIT_MASK(shift, length)};
> +
> +/* Extract a field from a register */
> +#define FIELD_EX32(storage, reg, field)                                   \
> +    extract32((storage), R_ ## reg ## _ ## field ## _SHIFT,               \
> +              R_ ## reg ## _ ## field ## _LENGTH)
> +
> +/* Extract a field from an array of registers */
> +#define ARRAY_FIELD_EX32(regs, reg, field)                                \
> +    FIELD_EX32((regs)[R_ ## reg], reg, field)
> +
> +/* Deposit a register field.
> + * Assigning values larger then the target field will result in
> + * compilation warnings.
> + */
> +#define FIELD_DP32(storage, reg, field, val) ({                           \
> +    struct {                                                              \
> +        unsigned int v:R_ ## reg ## _ ## field ## _LENGTH;                \
> +    } v = { .v = val };                                                   \
> +    uint32_t d;                                                           \
> +    d = deposit32((storage), R_ ## reg ## _ ## field ## _SHIFT,           \
> +                  R_ ## reg ## _ ## field ## _LENGTH, v.v);               \
> +    d; })
> +
> +/* Deposit a field to array of registers.  */
> +#define ARRAY_FIELD_DP32(regs, reg, field, val)                           \
> +    (regs)[R_ ## reg] = FIELD_DP32((regs)[R_ ## reg], reg, field, val);
> +
> +#endif
> --
> 2.7.4
>
>

Re: [Qemu-devel] [PATCH 0/6] ARMv7M: some simple bugfixes and cleanups

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH 0/6] ARMv7M: some simple bugfixes and cleanups
Message-id: 1484937883-1068-1-git-send-email-peter.maydell@linaro.org
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
66302f7 armv7m: Clear FAULTMASK on return from non-NMI exceptions
172bcdf armv7m: Fix reads of CONTROL register bit 1
8db2d6a hw/registerfields.h: Pull FIELD etc macros out of hw/register.h
615dc6a armv7m: Explicit error for bad vector table
c20354e armv7m: Replace armv7m.hack with unassigned_access handler
8f50fe2 armv7m: MRS/MSR: handle unprivileged access

=== OUTPUT BEGIN ===
Checking PATCH 1/6: armv7m: MRS/MSR: handle unprivileged access...
Checking PATCH 2/6: armv7m: Replace armv7m.hack with unassigned_access handler...
Checking PATCH 3/6: armv7m: Explicit error for bad vector table...
Checking PATCH 4/6: hw/registerfields.h: Pull FIELD etc macros out of hw/register.h...
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#106: FILE: include/hw/registerfields.h:19:
+#define REG32(reg, addr)                                                  \
+    enum { A_ ## reg = (addr) };                                          \
+    enum { R_ ## reg = (addr) / 4 };

ERROR: trailing whitespace
#112: FILE: include/hw/registerfields.h:25:
+/* This macro will define FOO_BAR_MASK, FOO_BAR_SHIFT and FOO_BAR_LENGTH $

ERROR: Macros with multiple statements should be enclosed in a do - while loop
#115: FILE: include/hw/registerfields.h:28:
+#define FIELD(reg, field, shift, length)                                  \
+    enum { R_ ## reg ## _ ## field ## _SHIFT = (shift)};                  \
+    enum { R_ ## reg ## _ ## field ## _LENGTH = (length)};                \
+    enum { R_ ## reg ## _ ## field ## _MASK =                             \
+                                        MAKE_64BIT_MASK(shift, length)};

total: 3 errors, 0 warnings, 117 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 5/6: armv7m: Fix reads of CONTROL register bit 1...
Checking PATCH 6/6: armv7m: Clear FAULTMASK on return from non-NMI exceptions...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [Qemu-arm] [PATCH 1/6] armv7m: MRS/MSR: handle unprivileged access

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> From: Michael Davidsaver <mdavidsaver@gmail.com>
>
> The MRS and MSR instruction handling has a number of flaws:
>  * unprivileged accesses should only be able to read
>    CONTROL and the xPSR subfields, and only write APSR
>    (others RAZ/WI)
>  * privileged access should not be able to write xPSR
>    subfields other than APSR
>  * accesses to unimplemented registers should log as
>    guest errors, not abort QEMU
>
> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> [PMM: rewrote commit message]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 79 +++++++++++++++++++++++++----------------------------
>  1 file changed, 37 insertions(+), 42 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 7111c8c..ad23de3 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -8243,23 +8243,32 @@ hwaddr arm_cpu_get_phys_page_attrs_debug(CPUState *cs, vaddr addr,
>
>  uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t reg)
>  {
> -    ARMCPU *cpu = arm_env_get_cpu(env);
> +    uint32_t mask;
> +    unsigned el = arm_current_el(env);
> +
> +    /* First handle registers which unprivileged can read */
> +
> +    switch (reg) {
> +    case 0 ... 7: /* xPSR sub-fields */

This reads a little confusingly compared to the pseudo-code in the ARM
ARM. Would it be clearer if we just went:

  switch(extract32(reg, 3, 5)) {
    case 0: /* xPSR */
    ...
    case 1: /* SP */
    ...
    case 2: /* Priority Mask or CONTROL.. */
    ...
  }

?

> +        mask = 0;
> +        if ((reg & 1) && el) {
> +            mask |= 0x000001ff; /* IPSR (unpriv. reads as zero) */

As B5.2.2 doesn't imply any particular access limit perhaps the comment
should read /* ISPR (reads as zero when not in exception) */

> +        }
> +        if (!(reg & 4)) {
> +            mask |= 0xf8000000; /* APSR */
> +        }
> +        /* EPSR reads as zero */
> +        return xpsr_read(env) & mask;
> +        break;
> +    case 20: /* CONTROL */
> +        return env->v7m.control;

I'm fairly sure this was meant to be 0x20 and either way the result is
gated by current privilege.

> +    }
> +
> +    if (el == 0) {
> +        return 0; /* unprivileged reads others as zero */
> +    }
>
>      switch (reg) {
> -    case 0: /* APSR */
> -        return xpsr_read(env) & 0xf8000000;
> -    case 1: /* IAPSR */
> -        return xpsr_read(env) & 0xf80001ff;
> -    case 2: /* EAPSR */
> -        return xpsr_read(env) & 0xff00fc00;
> -    case 3: /* xPSR */
> -        return xpsr_read(env) & 0xff00fdff;
> -    case 5: /* IPSR */
> -        return xpsr_read(env) & 0x000001ff;
> -    case 6: /* EPSR */
> -        return xpsr_read(env) & 0x0700fc00;
> -    case 7: /* IEPSR */
> -        return xpsr_read(env) & 0x0700edff;
>      case 8: /* MSP */
>          return env->v7m.current_sp ? env->v7m.other_sp : env->regs[13];
>      case 9: /* PSP */
> @@ -8271,40 +8280,26 @@ uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t reg)
>          return env->v7m.basepri;
>      case 19: /* FAULTMASK */
>          return (env->daif & PSTATE_F) != 0;
> -    case 20: /* CONTROL */
> -        return env->v7m.control;
>      default:
> -        /* ??? For debugging only.  */
> -        cpu_abort(CPU(cpu), "Unimplemented system register read (%d)\n", reg);
> +        qemu_log_mask(LOG_GUEST_ERROR, "Attempt to read unknown special"
> +                                       " register %d\n", reg);
>          return 0;
>      }
>  }
>
>  void HELPER(v7m_msr)(CPUARMState *env, uint32_t reg, uint32_t val)
>  {
> -    ARMCPU *cpu = arm_env_get_cpu(env);
> +    if (arm_current_el(env) == 0 && reg > 7) {
> +        /* only xPSR sub-fields may be written by unprivileged */
> +        return;
> +    }
>
>      switch (reg) {
> -    case 0: /* APSR */
> -        xpsr_write(env, val, 0xf8000000);
> -        break;
> -    case 1: /* IAPSR */
> -        xpsr_write(env, val, 0xf8000000);
> -        break;
> -    case 2: /* EAPSR */
> -        xpsr_write(env, val, 0xfe00fc00);
> -        break;
> -    case 3: /* xPSR */
> -        xpsr_write(env, val, 0xfe00fc00);
> -        break;
> -    case 5: /* IPSR */
> -        /* IPSR bits are readonly.  */
> -        break;
> -    case 6: /* EPSR */
> -        xpsr_write(env, val, 0x0600fc00);
> -        break;
> -    case 7: /* IEPSR */
> -        xpsr_write(env, val, 0x0600fc00);
> +    case 0 ... 7: /* xPSR sub-fields */
> +        /* only APSR is actually writable */
> +        if (reg & 4) {
> +            xpsr_write(env, val, 0xf8000000); /* APSR */
> +        }

I assuming insn<10> selects a different helper....

>          break;
>      case 8: /* MSP */
>          if (env->v7m.current_sp)
> @@ -8345,8 +8340,8 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t reg, uint32_t val)
>          switch_v7m_sp(env, (val & 2) != 0);
>          break;
>      default:
> -        /* ??? For debugging only.  */
> -        cpu_abort(CPU(cpu), "Unimplemented system register write (%d)\n", reg);
> +        qemu_log_mask(LOG_GUEST_ERROR, "Attempt to write unknown special"
> +                                       " register %d\n", reg);
>          return;
>      }
>  }


--
Alex Bennée

Re: [Qemu-devel] [Qemu-arm] [PATCH 2/6] armv7m: Replace armv7m.hack with unassigned_access handler

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> From: Michael Davidsaver <mdavidsaver@gmail.com>
>
> For v7m we need to catch attempts to execute from special
> addresses at 0xfffffff0 and above. Previously we did this
> with the aid of a hacky special purpose lump of memory
> in the address space and a check in translate.c for whether
> we were translating code at those addresses.
>
> We can implement this more cleanly using a CPU
> unassigned access handler which throws the exception
> if the unassigned access is for one of the special addresses.
>
> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
> [PMM:
>  * drop the deletion of the "don't interrupt if PC is magic"
>    code in arm_v7m_cpu_exec_interrupt() -- this is still
>    required
>  * don't generate an exception for unassigned accesses
>    which aren't to the magic address -- although doing
>    this is in theory correct in practice it will break
>    currently working guests which rely on the RAZ/WI
>    behaviour when they touch devices which we haven't
>    modelled.
>  * trigger EXCP_EXCEPTION_EXIT on is_exec, not !is_write
> ]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/armv7m.c        |  8 --------
>  target/arm/cpu.c       | 28 ++++++++++++++++++++++++++++
>  target/arm/translate.c | 12 ++++++------
>  3 files changed, 34 insertions(+), 14 deletions(-)
>
> diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
> index 49d3078..0c9ca7b 100644
> --- a/hw/arm/armv7m.c
> +++ b/hw/arm/armv7m.c
> @@ -180,7 +180,6 @@ DeviceState *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
>      uint64_t entry;
>      uint64_t lowaddr;
>      int big_endian;
> -    MemoryRegion *hack = g_new(MemoryRegion, 1);
>
>      if (cpu_model == NULL) {
>  	cpu_model = "cortex-m3";
> @@ -225,13 +224,6 @@ DeviceState *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
>          }
>      }
>
> -    /* Hack to map an additional page of ram at the top of the address
> -       space.  This stops qemu complaining about executing code outside RAM
> -       when returning from an exception.  */
> -    memory_region_init_ram(hack, NULL, "armv7m.hack", 0x1000, &error_fatal);
> -    vmstate_register_ram_global(hack);
> -    memory_region_add_subregion(system_memory, 0xfffff000, hack);
> -

What stops a model inadvertently registering a block of memory on-top?
Should we check the memory region really is unassigned?

>      qemu_register_reset(armv7m_reset, cpu);
>      return nvic;
>  }
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 3f2cdb6..47759c9 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -292,6 +292,33 @@ bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>  }
>
>  #if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64)
> +static void arm_v7m_unassigned_access(CPUState *cpu, hwaddr addr,
> +                                      bool is_write, bool is_exec, int opaque,
> +                                      unsigned size)
> +{
> +    ARMCPU *arm = ARM_CPU(cpu);
> +    CPUARMState *env = &arm->env;
> +
> +    /* ARMv7-M interrupt return works by loading a magic value into the PC.
> +     * On real hardware the load causes the return to occur.  The qemu
> +     * implementation performs the jump normally, then does the exception
> +     * return by throwing a special exception when when the CPU tries to
> +     * execute code at the magic address.
> +     */
> +    if (env->v7m.exception != 0 && addr >= 0xfffffff0 && is_exec) {
> +        cpu->exception_index = EXCP_EXCEPTION_EXIT;
> +        cpu_loop_exit(cpu);
> +    }
> +
> +    /* In real hardware an attempt to access parts of the address space
> +     * with nothing there will usually cause an external abort.
> +     * However our QEMU board models are often missing device models where
> +     * the guest can boot anyway with the default read-as-zero/writes-ignored
> +     * behaviour that you get without a QEMU unassigned_access hook.
> +     * So just return here to retain that default behaviour.
> +     */
> +}
> +
>  static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cs);
> @@ -1016,6 +1043,7 @@ static void arm_v7m_class_init(ObjectClass *oc, void *data)
>      cc->do_interrupt = arm_v7m_cpu_do_interrupt;
>  #endif
>
> +    cc->do_unassigned_access = arm_v7m_unassigned_access;
>      cc->cpu_exec_interrupt = arm_v7m_cpu_exec_interrupt;
>  }
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index c9186b6..a7c2abe 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -11719,12 +11719,12 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)
>              break;
>          }
>  #else
> -        if (dc->pc >= 0xfffffff0 && arm_dc_feature(dc, ARM_FEATURE_M)) {
> -            /* We always get here via a jump, so know we are not in a
> -               conditional execution block.  */
> -            gen_exception_internal(EXCP_EXCEPTION_EXIT);
> -            dc->is_jmp = DISAS_EXC;
> -            break;
> +        if (arm_dc_feature(dc, ARM_FEATURE_M)) {
> +            /* Branches to the magic exception-return addresses should
> +             * already have been caught via the arm_v7m_unassigned_access hook,
> +             * and never get here.
> +             */
> +            assert(dc->pc < 0xfffffff0);
>          }
>  #endif

Otherwise:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

--
Alex Bennée

Re: [Qemu-devel] [Qemu-arm] [PATCH 3/6] armv7m: Explicit error for bad vector table

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> From: Michael Davidsaver <mdavidsaver@gmail.com>
>
> Give an explicit error and abort when a load
> from the vector table fails. Architecturally this
> should HardFault (which will then immediately
> fail to load the HardFault vector and go into Lockup).
> Since we don't model Lockup, just report this guest
> error via cpu_abort(). This is more helpful than the
> previous behaviour of reading a zero, which is the
> address of the reset stack pointer and not a sensible
> location to jump to.

Word wrap has gone a little aggressive in the commit message ;-)

>
> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> [PMM: expanded commit message]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index ad23de3..8edb08c 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6014,6 +6014,30 @@ static void arm_log_exception(int idx)
>      }
>  }
>
> +static uint32_t arm_v7m_load_vector(ARMCPU *cpu)
> +
> +{
> +    CPUState *cs = CPU(cpu);
> +    CPUARMState *env = &cpu->env;
> +    MemTxResult result;
> +    hwaddr vec = env->v7m.vecbase + env->v7m.exception * 4;
> +    uint32_t addr;
> +
> +    addr = address_space_ldl(cs->as, vec,
> +                             MEMTXATTRS_UNSPECIFIED, &result);
> +    if (result != MEMTX_OK) {
> +        /* Architecturally this should cause a HardFault setting HSFR.VECTTBL,
> +         * which would then be immediately followed by our failing to load
> +         * the entry vector for that HardFault, which is a Lockup case.
> +         * Since we don't model Lockup, we just report this guest error
> +         * via cpu_abort().
> +         */
> +        cpu_abort(cs, "Failed to read from exception vector table "
> +                  "entry %08x\n", (unsigned)vec);
> +    }
> +    return addr;
> +}
> +
>  void arm_v7m_cpu_do_interrupt(CPUState *cs)
>  {
>      ARMCPU *cpu = ARM_CPU(cs);
> @@ -6095,7 +6119,7 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>      /* Clear IT bits */
>      env->condexec_bits = 0;
>      env->regs[14] = lr;
> -    addr = ldl_phys(cs->as, env->v7m.vecbase + env->v7m.exception * 4);
> +    addr = arm_v7m_load_vector(cpu);
>      env->regs[15] = addr & 0xfffffffe;
>      env->thumb = addr & 1;
>  }

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

--
Alex Bennée

Re: [Qemu-devel] [Qemu-arm] [PATCH 4/6] hw/registerfields.h: Pull FIELD etc macros out of hw/register.h

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> hw/register.h provides macros like FIELD which make it easy to define
> shift, mask and length constants for the fields within a register.
> Unfortunately register.h also includes a lot of other things, some
> of which will only compile in the softmmu build.
>
> Pull the FIELD macro and friends out into a separate header file,
> so they can be used in places like target/arm files which also
> get built in the user-only configs.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/hw/register.h       | 47 +----------------------------------
>  include/hw/registerfields.h | 60 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 61 insertions(+), 46 deletions(-)
>  create mode 100644 include/hw/registerfields.h
>
> diff --git a/include/hw/register.h b/include/hw/register.h
> index 8c12233..8bff5fb 100644
> --- a/include/hw/register.h
> +++ b/include/hw/register.h
> @@ -13,6 +13,7 @@
>
>  #include "hw/qdev-core.h"
>  #include "exec/memory.h"
> +#include "hw/registerfields.h"
>
>  typedef struct RegisterInfo RegisterInfo;
>  typedef struct RegisterAccessInfo RegisterAccessInfo;
> @@ -206,50 +207,4 @@ RegisterInfoArray *register_init_block32(DeviceState *owner,
>
>  void register_finalize_block(RegisterInfoArray *r_array);
>
> -/* Define constants for a 32 bit register */
> -
> -/* This macro will define A_FOO, for the byte address of a register
> - * as well as R_FOO for the uint32_t[] register number (A_FOO / 4).
> - */
> -#define REG32(reg, addr)                                                  \
> -    enum { A_ ## reg = (addr) };                                          \
> -    enum { R_ ## reg = (addr) / 4 };
> -
> -/* Define SHIFT, LENGTH and MASK constants for a field within a register */
> -
> -/* This macro will define FOO_BAR_MASK, FOO_BAR_SHIFT and FOO_BAR_LENGTH
> - * constants for field BAR in register FOO.
> - */
> -#define FIELD(reg, field, shift, length)                                  \
> -    enum { R_ ## reg ## _ ## field ## _SHIFT = (shift)};                  \
> -    enum { R_ ## reg ## _ ## field ## _LENGTH = (length)};                \
> -    enum { R_ ## reg ## _ ## field ## _MASK =                             \
> -                                        MAKE_64BIT_MASK(shift, length)};
> -
> -/* Extract a field from a register */
> -#define FIELD_EX32(storage, reg, field)                                   \
> -    extract32((storage), R_ ## reg ## _ ## field ## _SHIFT,               \
> -              R_ ## reg ## _ ## field ## _LENGTH)
> -
> -/* Extract a field from an array of registers */
> -#define ARRAY_FIELD_EX32(regs, reg, field)                                \
> -    FIELD_EX32((regs)[R_ ## reg], reg, field)
> -
> -/* Deposit a register field.
> - * Assigning values larger then the target field will result in
> - * compilation warnings.
> - */
> -#define FIELD_DP32(storage, reg, field, val) ({                           \
> -    struct {                                                              \
> -        unsigned int v:R_ ## reg ## _ ## field ## _LENGTH;                \
> -    } v = { .v = val };                                                   \
> -    uint32_t d;                                                           \
> -    d = deposit32((storage), R_ ## reg ## _ ## field ## _SHIFT,           \
> -                  R_ ## reg ## _ ## field ## _LENGTH, v.v);               \
> -    d; })
> -
> -/* Deposit a field to array of registers.  */
> -#define ARRAY_FIELD_DP32(regs, reg, field, val)                           \
> -    (regs)[R_ ## reg] = FIELD_DP32((regs)[R_ ## reg], reg, field, val);
> -
>  #endif
> diff --git a/include/hw/registerfields.h b/include/hw/registerfields.h
> new file mode 100644
> index 0000000..af101d5
> --- /dev/null
> +++ b/include/hw/registerfields.h
> @@ -0,0 +1,60 @@
> +/*
> + * Register Definition API: field macros
> + *
> + * Copyright (c) 2016 Xilinx Inc.
> + * Copyright (c) 2013 Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + */
> +
> +#ifndef REGISTERFIELDS_H
> +#define REGISTERFIELDS_H
> +
> +/* Define constants for a 32 bit register */
> +
> +/* This macro will define A_FOO, for the byte address of a register
> + * as well as R_FOO for the uint32_t[] register number (A_FOO / 4).
> + */
> +#define REG32(reg, addr)                                                  \
> +    enum { A_ ## reg = (addr) };                                          \
> +    enum { R_ ## reg = (addr) / 4 };
> +
> +/* Define SHIFT, LENGTH and MASK constants for a field within a register */
> +
> +/* This macro will define FOO_BAR_MASK, FOO_BAR_SHIFT and FOO_BAR_LENGTH
> + * constants for field BAR in register FOO.
> + */
> +#define FIELD(reg, field, shift, length)                                  \
> +    enum { R_ ## reg ## _ ## field ## _SHIFT = (shift)};                  \
> +    enum { R_ ## reg ## _ ## field ## _LENGTH = (length)};                \
> +    enum { R_ ## reg ## _ ## field ## _MASK =                             \
> +                                        MAKE_64BIT_MASK(shift, length)};
> +
> +/* Extract a field from a register */
> +#define FIELD_EX32(storage, reg, field)                                   \
> +    extract32((storage), R_ ## reg ## _ ## field ## _SHIFT,               \
> +              R_ ## reg ## _ ## field ## _LENGTH)
> +
> +/* Extract a field from an array of registers */
> +#define ARRAY_FIELD_EX32(regs, reg, field)                                \
> +    FIELD_EX32((regs)[R_ ## reg], reg, field)
> +
> +/* Deposit a register field.
> + * Assigning values larger then the target field will result in
> + * compilation warnings.
> + */
> +#define FIELD_DP32(storage, reg, field, val) ({                           \
> +    struct {                                                              \
> +        unsigned int v:R_ ## reg ## _ ## field ## _LENGTH;                \
> +    } v = { .v = val };                                                   \
> +    uint32_t d;                                                           \
> +    d = deposit32((storage), R_ ## reg ## _ ## field ## _SHIFT,           \
> +                  R_ ## reg ## _ ## field ## _LENGTH, v.v);               \
> +    d; })
> +
> +/* Deposit a field to array of registers.  */
> +#define ARRAY_FIELD_DP32(regs, reg, field, val)                           \
> +    (regs)[R_ ## reg] = FIELD_DP32((regs)[R_ ## reg], reg, field, val);
> +
> +#endif


Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

--
Alex Bennée

Re: [Qemu-devel] [Qemu-arm] [PATCH 1/6] armv7m: MRS/MSR: handle unprivileged access

[Peter Maydell]

On 24 January 2017 at 16:25, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> From: Michael Davidsaver <mdavidsaver@gmail.com>
>>
>> The MRS and MSR instruction handling has a number of flaws:
>>  * unprivileged accesses should only be able to read
>>    CONTROL and the xPSR subfields, and only write APSR
>>    (others RAZ/WI)
>>  * privileged access should not be able to write xPSR
>>    subfields other than APSR
>>  * accesses to unimplemented registers should log as
>>    guest errors, not abort QEMU
>>
>> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>> [PMM: rewrote commit message]
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  target/arm/helper.c | 79 +++++++++++++++++++++++++----------------------------
>>  1 file changed, 37 insertions(+), 42 deletions(-)
>>
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index 7111c8c..ad23de3 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -8243,23 +8243,32 @@ hwaddr arm_cpu_get_phys_page_attrs_debug(CPUState *cs, vaddr addr,
>>
>>  uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t reg)
>>  {
>> -    ARMCPU *cpu = arm_env_get_cpu(env);
>> +    uint32_t mask;
>> +    unsigned el = arm_current_el(env);
>> +
>> +    /* First handle registers which unprivileged can read */
>> +
>> +    switch (reg) {
>> +    case 0 ... 7: /* xPSR sub-fields */
>
> This reads a little confusingly compared to the pseudo-code in the ARM
> ARM. Would it be clearer if we just went:
>
>   switch(extract32(reg, 3, 5)) {
>     case 0: /* xPSR */
>     ...
>     case 1: /* SP */
>     ...
>     case 2: /* Priority Mask or CONTROL.. */
>     ...
>   }
>
> ?

I think a simple switch on the register number is rather
less confusing than the pseudocode's nested switches on
the two halves of the register number -- this is a case where
we don't have to follow the pseudocode if it phrases something
weirdly.

>> +        mask = 0;
>> +        if ((reg & 1) && el) {
>> +            mask |= 0x000001ff; /* IPSR (unpriv. reads as zero) */
>
> As B5.2.2 doesn't imply any particular access limit perhaps the comment
> should read /* ISPR (reads as zero when not in exception) */

B5.2.2 Notes says "If unprivileged code attempts to read [...] the IPSR,
the read returns zero". In the pseudocode this is handled by an
architectural guarantee that you can't get here in unprivileged
mode with an IPSR value that isn't zero; however given the current
state of the QEMU code I'm not really prepared to make that assertion,
so it seems better to just report 0 here.

>> +        }
>> +        if (!(reg & 4)) {
>> +            mask |= 0xf8000000; /* APSR */
>> +        }
>> +        /* EPSR reads as zero */
>> +        return xpsr_read(env) & mask;
>> +        break;
>> +    case 20: /* CONTROL */
>> +        return env->v7m.control;
>
> I'm fairly sure this was meant to be 0x20 and either way the result is
> gated by current privilege.

No, it's decimal 20, binary 0b10100. CONTROL is the case
of 00010 in the outer switch and 100 in the inner, and
it isn't privilege-checked. (See how confusing the
pseudocode phrasing is ? :-))

>> +    }
>> +
>> +    if (el == 0) {
>> +        return 0; /* unprivileged reads others as zero */
>> +    }
>>
>>      switch (reg) {
>> -    case 0: /* APSR */
>> -        return xpsr_read(env) & 0xf8000000;
>> -    case 1: /* IAPSR */
>> -        return xpsr_read(env) & 0xf80001ff;
>> -    case 2: /* EAPSR */
>> -        return xpsr_read(env) & 0xff00fc00;
>> -    case 3: /* xPSR */
>> -        return xpsr_read(env) & 0xff00fdff;
>> -    case 5: /* IPSR */
>> -        return xpsr_read(env) & 0x000001ff;
>> -    case 6: /* EPSR */
>> -        return xpsr_read(env) & 0x0700fc00;
>> -    case 7: /* IEPSR */
>> -        return xpsr_read(env) & 0x0700edff;
>>      case 8: /* MSP */
>>          return env->v7m.current_sp ? env->v7m.other_sp : env->regs[13];
>>      case 9: /* PSP */
>> @@ -8271,40 +8280,26 @@ uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t reg)
>>          return env->v7m.basepri;
>>      case 19: /* FAULTMASK */
>>          return (env->daif & PSTATE_F) != 0;
>> -    case 20: /* CONTROL */
>> -        return env->v7m.control;
>>      default:
>> -        /* ??? For debugging only.  */
>> -        cpu_abort(CPU(cpu), "Unimplemented system register read (%d)\n", reg);
>> +        qemu_log_mask(LOG_GUEST_ERROR, "Attempt to read unknown special"
>> +                                       " register %d\n", reg);
>>          return 0;
>>      }
>>  }
>>
>>  void HELPER(v7m_msr)(CPUARMState *env, uint32_t reg, uint32_t val)
>>  {
>> -    ARMCPU *cpu = arm_env_get_cpu(env);
>> +    if (arm_current_el(env) == 0 && reg > 7) {
>> +        /* only xPSR sub-fields may be written by unprivileged */
>> +        return;
>> +    }
>>
>>      switch (reg) {
>> -    case 0: /* APSR */
>> -        xpsr_write(env, val, 0xf8000000);
>> -        break;
>> -    case 1: /* IAPSR */
>> -        xpsr_write(env, val, 0xf8000000);
>> -        break;
>> -    case 2: /* EAPSR */
>> -        xpsr_write(env, val, 0xfe00fc00);
>> -        break;
>> -    case 3: /* xPSR */
>> -        xpsr_write(env, val, 0xfe00fc00);
>> -        break;
>> -    case 5: /* IPSR */
>> -        /* IPSR bits are readonly.  */
>> -        break;
>> -    case 6: /* EPSR */
>> -        xpsr_write(env, val, 0x0600fc00);
>> -        break;
>> -    case 7: /* IEPSR */
>> -        xpsr_write(env, val, 0x0600fc00);
>> +    case 0 ... 7: /* xPSR sub-fields */
>> +        /* only APSR is actually writable */
>> +        if (reg & 4) {
>> +            xpsr_write(env, val, 0xf8000000); /* APSR */
>> +        }
>
> I assuming insn<10> selects a different helper....

insn<10> is mask<0> -- the behaviour is UNPREDICTABLE unless
the DSP extension is implemented. This code all predates the
attempt to support that in our cortex-m4 model (and indeed
predates the existence of M profile DSP extns), so it doesn't
care about the mask bits. I think the translate.c code is
probably just going to get that decode wrong at the moment.
We should tighten it up but it will likely be a different
helper and definitely a separate patch.

>>          break;
>>      case 8: /* MSP */
>>          if (env->v7m.current_sp)
>> @@ -8345,8 +8340,8 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t reg, uint32_t val)
>>          switch_v7m_sp(env, (val & 2) != 0);
>>          break;
>>      default:
>> -        /* ??? For debugging only.  */
>> -        cpu_abort(CPU(cpu), "Unimplemented system register write (%d)\n", reg);
>> +        qemu_log_mask(LOG_GUEST_ERROR, "Attempt to write unknown special"
>> +                                       " register %d\n", reg);
>>          return;
>>      }
>>  }

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH 2/6] armv7m: Replace armv7m.hack with unassigned_access handler

[Peter Maydell]

On 24 January 2017 at 16:31, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>> -    /* Hack to map an additional page of ram at the top of the address
>> -       space.  This stops qemu complaining about executing code outside RAM
>> -       when returning from an exception.  */
>> -    memory_region_init_ram(hack, NULL, "armv7m.hack", 0x1000, &error_fatal);
>> -    vmstate_register_ram_global(hack);
>> -    memory_region_add_subregion(system_memory, 0xfffff000, hack);
>> -
>
> What stops a model inadvertently registering a block of memory on-top?
> Should we check the memory region really is unassigned?

It would just be a board code bug, and a pretty implausible one
I think (at least no less plausible than any other typoing of
a device or memory address, 99% of which are uncheckable).

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH 5/6] armv7m: Fix reads of CONTROL register bit 1

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> From: Michael Davidsaver <mdavidsaver@gmail.com>
>
> The v7m CONTROL register bit 1 is SPSEL, which indicates
> the stack being used. We were storing this information
> not in v7m.control but in the separate v7m.other_sp
> structure field. Unfortunately, the code handling reads
> of the CONTROL register didn't take account of this, and
> so if SPSEL was updated by an exception entry or exit then
> a subsequent guest read of CONTROL would get the wrong value.
>
> Using a separate structure field doesn't really gain us
> anything in efficiency, so drop this unnecessary complexity
> in favour of simply storing all the bits in v7m.control.
>
> This is a migration compatibility break for M profile
> CPUs only.
>
> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
> [PMM: rewrote commit message;
>  use deposit32(); use FIELD to define constants for
>  masking and shifting of CONTROL register fields
> ]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/cpu.h       |  1 -
>  target/arm/internals.h |  7 +++++++
>  target/arm/helper.c    | 35 +++++++++++++++++++++++------------
>  target/arm/machine.c   |  6 ++----
>  4 files changed, 32 insertions(+), 17 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 151a5d7..521c11b 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -405,7 +405,6 @@ typedef struct CPUARMState {
>          uint32_t vecbase;
>          uint32_t basepri;
>          uint32_t control;
> -        int current_sp;
>          int exception;
>      } v7m;
>
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 3cae5ff..2e65bc1 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -25,6 +25,8 @@
>  #ifndef TARGET_ARM_INTERNALS_H
>  #define TARGET_ARM_INTERNALS_H
>
> +#include "hw/registerfields.h"
> +
>  /* register banks for CPU modes */
>  #define BANK_USRSYS 0
>  #define BANK_SVC    1
> @@ -75,6 +77,11 @@ static const char * const excnames[] = {
>   */
>  #define GTIMER_SCALE 16
>
> +/* Bit definitions for the v7M CONTROL register */
> +FIELD(V7M_CONTROL, NPRIV, 0, 1)
> +FIELD(V7M_CONTROL, SPSEL, 1, 1)
> +FIELD(V7M_CONTROL, FPCA, 2, 1)
> +
>  /*
>   * For AArch64, map a given EL to an index in the banked_spsr array.
>   * Note that this mapping and the AArch32 mapping defined in bank_number()
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 8edb08c..dc383d1 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -5947,14 +5947,19 @@ static uint32_t v7m_pop(CPUARMState *env)
>  }
>
>  /* Switch to V7M main or process stack pointer.  */
> -static void switch_v7m_sp(CPUARMState *env, int process)
> +static void switch_v7m_sp(CPUARMState *env, bool new_spsel)
>  {
>      uint32_t tmp;
> -    if (env->v7m.current_sp != process) {
> +    bool old_spsel = env->v7m.control & R_V7M_CONTROL_SPSEL_MASK;
> +
> +    if (old_spsel != new_spsel) {
>          tmp = env->v7m.other_sp;
>          env->v7m.other_sp = env->regs[13];
>          env->regs[13] = tmp;
> -        env->v7m.current_sp = process;
> +
> +        env->v7m.control = deposit32(env->v7m.control,
> +                                     R_V7M_CONTROL_SPSEL_SHIFT,
> +                                     R_V7M_CONTROL_SPSEL_LENGTH,
> new_spsel);

I was thrown by the use of deposit32 here without the extract32. However
I see we are dealing with a bit here - shame there isn't set_bit32()
method.

>      }
>  }
>
> @@ -6049,8 +6054,9 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>      arm_log_exception(cs->exception_index);
>
>      lr = 0xfffffff1;
> -    if (env->v7m.current_sp)
> +    if (env->v7m.control & R_V7M_CONTROL_SPSEL_MASK) {
>          lr |= 4;
> +    }
>      if (env->v7m.exception == 0)
>          lr |= 8;
>
> @@ -8294,9 +8300,11 @@ uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t reg)
>
>      switch (reg) {
>      case 8: /* MSP */
> -        return env->v7m.current_sp ? env->v7m.other_sp : env->regs[13];
> +        return (env->v7m.control & R_V7M_CONTROL_SPSEL_MASK) ?
> +            env->v7m.other_sp : env->regs[13];
>      case 9: /* PSP */
> -        return env->v7m.current_sp ? env->regs[13] : env->v7m.other_sp;
> +        return (env->v7m.control & R_V7M_CONTROL_SPSEL_MASK) ?
> +            env->regs[13] : env->v7m.other_sp;
>      case 16: /* PRIMASK */
>          return (env->daif & PSTATE_I) != 0;
>      case 17: /* BASEPRI */
> @@ -8326,16 +8334,18 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t reg, uint32_t val)
>          }
>          break;
>      case 8: /* MSP */
> -        if (env->v7m.current_sp)
> +        if (env->v7m.control & R_V7M_CONTROL_SPSEL_MASK) {
>              env->v7m.other_sp = val;
> -        else
> +        } else {
>              env->regs[13] = val;
> +        }
>          break;
>      case 9: /* PSP */
> -        if (env->v7m.current_sp)
> +        if (env->v7m.control & R_V7M_CONTROL_SPSEL_MASK) {
>              env->regs[13] = val;
> -        else
> +        } else {
>              env->v7m.other_sp = val;
> +        }
>          break;
>      case 16: /* PRIMASK */
>          if (val & 1) {
> @@ -8360,8 +8370,9 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t reg, uint32_t val)
>          }
>          break;
>      case 20: /* CONTROL */
> -        env->v7m.control = val & 3;
> -        switch_v7m_sp(env, (val & 2) != 0);
> +        switch_v7m_sp(env, (val & R_V7M_CONTROL_SPSEL_MASK) != 0);
> +        env->v7m.control = val & (R_V7M_CONTROL_SPSEL_MASK |
> +                                  R_V7M_CONTROL_NPRIV_MASK);
>          break;
>      default:
>          qemu_log_mask(LOG_GUEST_ERROR, "Attempt to write unknown special"
> diff --git a/target/arm/machine.c b/target/arm/machine.c
> index d90943b..8ed24bf 100644
> --- a/target/arm/machine.c
> +++ b/target/arm/machine.c
> @@ -96,15 +96,13 @@ static bool m_needed(void *opaque)
>
>  static const VMStateDescription vmstate_m = {
>      .name = "cpu/m",
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> +    .version_id = 2,
> +    .minimum_version_id = 2,
>      .needed = m_needed,
>      .fields = (VMStateField[]) {
> -        VMSTATE_UINT32(env.v7m.other_sp, ARMCPU),
>          VMSTATE_UINT32(env.v7m.vecbase, ARMCPU),
>          VMSTATE_UINT32(env.v7m.basepri, ARMCPU),
>          VMSTATE_UINT32(env.v7m.control, ARMCPU),
> -        VMSTATE_INT32(env.v7m.current_sp, ARMCPU),
>          VMSTATE_INT32(env.v7m.exception, ARMCPU),
>          VMSTATE_END_OF_LIST()
>      }

Not that it matters for this but is there a way to add another level of
indirection to the reading and writing of these fields to keep the same
migration format?

Anyway:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

--
Alex Bennée

Re: [Qemu-devel] [Qemu-arm] [PATCH 6/6] armv7m: Clear FAULTMASK on return from non-NMI exceptions

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> From: Michael Davidsaver <mdavidsaver@gmail.com>
>
> FAULTMASK must be cleared on return from all
> exceptions other than NMI.
>
> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index dc383d1..cfbc622 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -5969,8 +5969,13 @@ static void do_v7m_exception_exit(CPUARMState *env)
>      uint32_t xpsr;
>
>      type = env->regs[15];
> -    if (env->v7m.exception != 0)
> +    if (env->v7m.exception != ARMV7M_EXCP_NMI) {
> +        /* Auto-clear FAULTMASK on return from other than NMI */
> +        env->daif &= ~PSTATE_F;
> +    }
> +    if (env->v7m.exception != 0) {
>          armv7m_nvic_complete_irq(env->nvic, env->v7m.exception);
> +    }
>
>      /* Switch to the target stack.  */
>      switch_v7m_sp(env, (type & 4) != 0);

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

--
Alex Bennée

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/6] ARMv7M: some simple bugfixes and cleanups

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> This set of six patches is some simple bug fixes which
> I've pulled out of Michael Davidsaver's old NVIC rewrite
> patchset, as an initial start on getting it upstream.
> None of them are particularly exciting, but they're
> self-contained so they might as well go through code
> review and get into master ahead of the main rewrite.

I've completed my pass through - it all looks pretty good to me
(squashed tab stops on the commit messages not-withstanding ;-)

--
Alex Bennée

Re: [Qemu-devel] [Qemu-arm] [PATCH 5/6] armv7m: Fix reads of CONTROL register bit 1

[Peter Maydell]

On 24 January 2017 at 16:58, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>> --- a/target/arm/machine.c
>> +++ b/target/arm/machine.c
>> @@ -96,15 +96,13 @@ static bool m_needed(void *opaque)
>>
>>  static const VMStateDescription vmstate_m = {
>>      .name = "cpu/m",
>> -    .version_id = 1,
>> -    .minimum_version_id = 1,
>> +    .version_id = 2,
>> +    .minimum_version_id = 2,
>>      .needed = m_needed,
>>      .fields = (VMStateField[]) {
>> -        VMSTATE_UINT32(env.v7m.other_sp, ARMCPU),
>>          VMSTATE_UINT32(env.v7m.vecbase, ARMCPU),
>>          VMSTATE_UINT32(env.v7m.basepri, ARMCPU),
>>          VMSTATE_UINT32(env.v7m.control, ARMCPU),
>> -        VMSTATE_INT32(env.v7m.current_sp, ARMCPU),
>>          VMSTATE_INT32(env.v7m.exception, ARMCPU),
>>          VMSTATE_END_OF_LIST()
>>      }
>
> Not that it matters for this but is there a way to add another level of
> indirection to the reading and writing of these fields to keep the same
> migration format?

It's possible. One approach would be to make the fields
versioned, so that if the source doesn't have them they stay
at whatever the CPU's reset values are. Or you could have
them in a separate vmstate subsection with a needed function
that does something clever. But since we don't claim to
support cross-version migration for M profile yet (and I
suspect also that none of our M profile boards have working
migration in all the devices) I took the easy route of
just bumping the version_id on this subfield. (It doesn't
affect migration compat for anything that's not M profile.)

thanks
-- PMM