[Qemu-devel] [PATCH 00/10] More M profile bugfixes

[Qemu-devel] [PATCH 00/10] More M profile bugfixes

[Peter Maydell]

More easy bugfixes pulled out from (or inspired by,
or noticed during the course of working on) Michael's
NVIC patchset.

These patches expect to sit on top of my previous set.
You can find a git tree with the whole stack here:
 https://git.linaro.org/people/peter.maydell/qemu-arm.git m-profile-fixes

(I think the next part of this is going to be actually
digging into the NVIC rewrite proper.)

thanks
-- PMM

Michael Davidsaver (6):
  armv7m_nvic: keep a pointer to the CPU
  armv7m: implement CCR, CFSR, HFSR, DFSR, BFAR, and MMFAR
  armv7m: honour CCR.STACKALIGN on exception entry
  armv7m: set CFSR.UNDEFINSTR on undefined instructions
  armv7m: Honour CCR.USERSETMPEND
  armv7m: FAULTMASK should be 0 on reset

Peter Maydell (4):
  target/arm: Drop IS_M() macro
  armv7m: add state for v7M CCR, CFSR, HFSR, DFSR, MMFAR, BFAR
  armv7m: Report no-coprocessor faults correctly
  armv7m: R14 should reset to 0xffffffff

 target/arm/cpu.h       | 61 +++++++++++++++++++++++++++++++++++++++++++++-----
 hw/intc/armv7m_nvic.c  | 58 +++++++++++++++++++++++++++++++++--------------
 linux-user/main.c      |  1 +
 target/arm/cpu.c       | 22 +++++++++++++-----
 target/arm/helper.c    | 13 ++++++-----
 target/arm/machine.c   | 10 +++++++--
 target/arm/translate.c |  8 +++++++
 7 files changed, 138 insertions(+), 35 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 01/10] target/arm: Drop IS_M() macro

[Peter Maydell]

We only use the IS_M() macro in two places, and it's a bit of a
namespace grab to put in cpu.h.  Drop it in favour of just explicitly
calling arm_feature() in the places where it was used.

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

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 521c11b..b2cc329 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1762,12 +1762,6 @@ bool write_list_to_cpustate(ARMCPU *cpu);
  */
 bool write_cpustate_to_list(ARMCPU *cpu);
 
-/* Does the core conform to the "MicroController" profile. e.g. Cortex-M3.
-   Note the M in older cores (eg. ARM7TDMI) stands for Multiply. These are
-   conventional cores (ie. Application or Realtime profile).  */
-
-#define IS_M(env) arm_feature(env, ARM_FEATURE_M)
-
 #define ARM_CPUID_TI915T      0x54029152
 #define ARM_CPUID_TI925T      0x54029252
 
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 9075989..6395d5a 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -182,7 +182,7 @@ static void arm_cpu_reset(CPUState *s)
     /* On ARMv7-M the CPSR_I is the value of the PRIMASK register, and is
      * clear at reset. Initial SP and PC are loaded from ROM.
      */
-    if (IS_M(env)) {
+    if (arm_feature(env, ARM_FEATURE_M)) {
         uint32_t initial_msp; /* Loaded from 0x0 */
         uint32_t initial_pc; /* Loaded from 0x4 */
         uint8_t *rom;
diff --git a/target/arm/helper.c b/target/arm/helper.c
index cfbc622..ce7e43b 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6695,7 +6695,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
     CPUARMState *env = &cpu->env;
     unsigned int new_el = env->exception.target_el;
 
-    assert(!IS_M(env));
+    assert(!arm_feature(env, ARM_FEATURE_M));
 
     arm_log_exception(cs->exception_index);
     qemu_log_mask(CPU_LOG_INT, "...from EL%d to EL%d\n", arm_current_el(env),
-- 
2.7.4

Re: [Qemu-devel] [PATCH 01/10] target/arm: Drop IS_M() macro

[Alex Bennée]

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

> We only use the IS_M() macro in two places, and it's a bit of a
> namespace grab to put in cpu.h.  Drop it in favour of just explicitly
> calling arm_feature() in the places where it was used.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

> ---
>  target/arm/cpu.h    | 6 ------
>  target/arm/cpu.c    | 2 +-
>  target/arm/helper.c | 2 +-
>  3 files changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 521c11b..b2cc329 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1762,12 +1762,6 @@ bool write_list_to_cpustate(ARMCPU *cpu);
>   */
>  bool write_cpustate_to_list(ARMCPU *cpu);
>
> -/* Does the core conform to the "MicroController" profile. e.g. Cortex-M3.
> -   Note the M in older cores (eg. ARM7TDMI) stands for Multiply. These are
> -   conventional cores (ie. Application or Realtime profile).  */
> -
> -#define IS_M(env) arm_feature(env, ARM_FEATURE_M)
> -
>  #define ARM_CPUID_TI915T      0x54029152
>  #define ARM_CPUID_TI925T      0x54029252
>
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 9075989..6395d5a 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -182,7 +182,7 @@ static void arm_cpu_reset(CPUState *s)
>      /* On ARMv7-M the CPSR_I is the value of the PRIMASK register, and is
>       * clear at reset. Initial SP and PC are loaded from ROM.
>       */
> -    if (IS_M(env)) {
> +    if (arm_feature(env, ARM_FEATURE_M)) {
>          uint32_t initial_msp; /* Loaded from 0x0 */
>          uint32_t initial_pc; /* Loaded from 0x4 */
>          uint8_t *rom;
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index cfbc622..ce7e43b 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6695,7 +6695,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
>      CPUARMState *env = &cpu->env;
>      unsigned int new_el = env->exception.target_el;
>
> -    assert(!IS_M(env));
> +    assert(!arm_feature(env, ARM_FEATURE_M));
>
>      arm_log_exception(cs->exception_index);
>      qemu_log_mask(CPU_LOG_INT, "...from EL%d to EL%d\n", arm_current_el(env),


--
Alex Bennée

[Qemu-devel] [PATCH 02/10] armv7m_nvic: keep a pointer to the CPU

[Peter Maydell]

From: Michael Davidsaver <mdavidsaver@gmail.com>

Many NVIC operations access the CPU state, so store a pointer in
struct nvic_state rather than fetching it via qemu_get_cpu() every
time we need it.

As with the arm_gicv3_common code, we currently just call
qemu_get_cpu() in the NVIC's realize method, but in future we might
want to use a QOM property to pass the CPU to the NVIC.

This imposes an ordering requirement that the CPU is
realized before the NVIC, but that is always true since
both are dealt with in armv7m_init().

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
[PMM: Use qemu_get_cpu(0) rather than first_cpu; expand
 commit message]
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/armv7m_nvic.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 06d8db6..81dcb83 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -23,6 +23,7 @@
 
 typedef struct {
     GICState gic;
+    ARMCPU *cpu;
     struct {
         uint32_t control;
         uint32_t reload;
@@ -155,7 +156,7 @@ void armv7m_nvic_complete_irq(void *opaque, int irq)
 
 static uint32_t nvic_readl(nvic_state *s, uint32_t offset)
 {
-    ARMCPU *cpu;
+    ARMCPU *cpu = s->cpu;
     uint32_t val;
     int irq;
 
@@ -187,11 +188,9 @@ static uint32_t nvic_readl(nvic_state *s, uint32_t offset)
     case 0x1c: /* SysTick Calibration Value.  */
         return 10000;
     case 0xd00: /* CPUID Base.  */
-        cpu = ARM_CPU(qemu_get_cpu(0));
         return cpu->midr;
     case 0xd04: /* Interrupt Control State.  */
         /* VECTACTIVE */
-        cpu = ARM_CPU(qemu_get_cpu(0));
         val = cpu->env.v7m.exception;
         if (val == 1023) {
             val = 0;
@@ -222,7 +221,6 @@ static uint32_t nvic_readl(nvic_state *s, uint32_t offset)
             val |= (1 << 31);
         return val;
     case 0xd08: /* Vector Table Offset.  */
-        cpu = ARM_CPU(qemu_get_cpu(0));
         return cpu->env.v7m.vecbase;
     case 0xd0c: /* Application Interrupt/Reset Control.  */
         return 0xfa050000;
@@ -296,7 +294,7 @@ static uint32_t nvic_readl(nvic_state *s, uint32_t offset)
 
 static void nvic_writel(nvic_state *s, uint32_t offset, uint32_t value)
 {
-    ARMCPU *cpu;
+    ARMCPU *cpu = s->cpu;
     uint32_t oldval;
     switch (offset) {
     case 0x10: /* SysTick Control and Status.  */
@@ -349,7 +347,6 @@ static void nvic_writel(nvic_state *s, uint32_t offset, uint32_t value)
         }
         break;
     case 0xd08: /* Vector Table Offset.  */
-        cpu = ARM_CPU(qemu_get_cpu(0));
         cpu->env.v7m.vecbase = value & 0xffffff80;
         break;
     case 0xd0c: /* Application Interrupt/Reset Control.  */
@@ -495,6 +492,8 @@ static void armv7m_nvic_realize(DeviceState *dev, Error **errp)
     NVICClass *nc = NVIC_GET_CLASS(s);
     Error *local_err = NULL;
 
+    s->cpu = ARM_CPU(qemu_get_cpu(0));
+    assert(s->cpu);
     /* The NVIC always has only one CPU */
     s->gic.num_cpu = 1;
     /* Tell the common code we're an NVIC */
-- 
2.7.4

Re: [Qemu-devel] [PATCH 02/10] armv7m_nvic: keep a pointer to the CPU

[Alex Bennée]

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

> From: Michael Davidsaver <mdavidsaver@gmail.com>
>
> Many NVIC operations access the CPU state, so store a pointer in
> struct nvic_state rather than fetching it via qemu_get_cpu() every
> time we need it.
>
> As with the arm_gicv3_common code, we currently just call
> qemu_get_cpu() in the NVIC's realize method, but in future we might
> want to use a QOM property to pass the CPU to the NVIC.
>
> This imposes an ordering requirement that the CPU is
> realized before the NVIC, but that is always true since
> both are dealt with in armv7m_init().
>
> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
> [PMM: Use qemu_get_cpu(0) rather than first_cpu; expand
>  commit message]
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/intc/armv7m_nvic.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 06d8db6..81dcb83 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -23,6 +23,7 @@
>
>  typedef struct {
>      GICState gic;
> +    ARMCPU *cpu;
>      struct {
>          uint32_t control;
>          uint32_t reload;
> @@ -155,7 +156,7 @@ void armv7m_nvic_complete_irq(void *opaque, int irq)
>
>  static uint32_t nvic_readl(nvic_state *s, uint32_t offset)
>  {
> -    ARMCPU *cpu;
> +    ARMCPU *cpu = s->cpu;
>      uint32_t val;
>      int irq;
>
> @@ -187,11 +188,9 @@ static uint32_t nvic_readl(nvic_state *s, uint32_t offset)
>      case 0x1c: /* SysTick Calibration Value.  */
>          return 10000;
>      case 0xd00: /* CPUID Base.  */
> -        cpu = ARM_CPU(qemu_get_cpu(0));
>          return cpu->midr;
>      case 0xd04: /* Interrupt Control State.  */
>          /* VECTACTIVE */
> -        cpu = ARM_CPU(qemu_get_cpu(0));
>          val = cpu->env.v7m.exception;
>          if (val == 1023) {
>              val = 0;
> @@ -222,7 +221,6 @@ static uint32_t nvic_readl(nvic_state *s, uint32_t offset)
>              val |= (1 << 31);
>          return val;
>      case 0xd08: /* Vector Table Offset.  */
> -        cpu = ARM_CPU(qemu_get_cpu(0));
>          return cpu->env.v7m.vecbase;
>      case 0xd0c: /* Application Interrupt/Reset Control.  */
>          return 0xfa050000;
> @@ -296,7 +294,7 @@ static uint32_t nvic_readl(nvic_state *s, uint32_t offset)
>
>  static void nvic_writel(nvic_state *s, uint32_t offset, uint32_t value)
>  {
> -    ARMCPU *cpu;
> +    ARMCPU *cpu = s->cpu;
>      uint32_t oldval;
>      switch (offset) {
>      case 0x10: /* SysTick Control and Status.  */
> @@ -349,7 +347,6 @@ static void nvic_writel(nvic_state *s, uint32_t offset, uint32_t value)
>          }
>          break;
>      case 0xd08: /* Vector Table Offset.  */
> -        cpu = ARM_CPU(qemu_get_cpu(0));
>          cpu->env.v7m.vecbase = value & 0xffffff80;

Given it is only used once here you could just indirect it:

           s->cpu->env.v7m.vecbase = value & 0xffffff80;

But I assume the compiler would DTRT if the load wasn't needed.

>          break;
>      case 0xd0c: /* Application Interrupt/Reset Control.  */
> @@ -495,6 +492,8 @@ static void armv7m_nvic_realize(DeviceState *dev, Error **errp)
>      NVICClass *nc = NVIC_GET_CLASS(s);
>      Error *local_err = NULL;
>
> +    s->cpu = ARM_CPU(qemu_get_cpu(0));
> +    assert(s->cpu);
>      /* The NVIC always has only one CPU */
>      s->gic.num_cpu = 1;
>      /* Tell the common code we're an NVIC */

Anyway:

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

--
Alex Bennée

Re: [Qemu-devel] [PATCH 02/10] armv7m_nvic: keep a pointer to the CPU

[Peter Maydell]

On 27 January 2017 at 12:41, Alex Bennée <alex.bennee@linaro.org> wrote:
>> @@ -349,7 +347,6 @@ static void nvic_writel(nvic_state *s, uint32_t offset, uint32_t value)
>>          }
>>          break;
>>      case 0xd08: /* Vector Table Offset.  */
>> -        cpu = ARM_CPU(qemu_get_cpu(0));
>>          cpu->env.v7m.vecbase = value & 0xffffff80;
>
> Given it is only used once here you could just indirect it:
>
>            s->cpu->env.v7m.vecbase = value & 0xffffff80;

Two reasons not to do that:
(1) it makes this patch easier to review if all it's
doing is deleting lines that set cpu
(2) future patches improving the NVIC support are going
to add more cases that want to use the cpu pointer

thanks
-- PMM

[Qemu-devel] [PATCH 03/10] armv7m: add state for v7M CCR, CFSR, HFSR, DFSR, MMFAR, BFAR

[Peter Maydell]

Add the structure fields, VMState fields, reset code and macros for
the v7M system control registers CCR, CFSR, HFSR, DFSR, MMFAR and
BFAR.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.h     | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 target/arm/cpu.c     |  7 +++++++
 target/arm/machine.c | 10 ++++++++--
 3 files changed, 69 insertions(+), 2 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index b2cc329..4b062d2 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -21,6 +21,7 @@
 #define ARM_CPU_H
 
 #include "kvm-consts.h"
+#include "hw/registerfields.h"
 
 #if defined(TARGET_AARCH64)
   /* AArch64 definitions */
@@ -405,6 +406,12 @@ typedef struct CPUARMState {
         uint32_t vecbase;
         uint32_t basepri;
         uint32_t control;
+        uint32_t ccr; /* Configuration and Control */
+        uint32_t cfsr; /* Configurable Fault Status */
+        uint32_t hfsr; /* HardFault Status */
+        uint32_t dfsr; /* Debug Fault Status Register */
+        uint32_t mmfar; /* MemManage Fault Address */
+        uint32_t bfar; /* BusFault Address */
         int exception;
     } v7m;
 
@@ -1086,6 +1093,53 @@ enum arm_cpu_mode {
 #define ARM_IWMMXT_wCGR2	10
 #define ARM_IWMMXT_wCGR3	11
 
+/* V7M CCR bits */
+FIELD(V7M_CCR, NONBASETHRDENA, 0, 1)
+FIELD(V7M_CCR, USERSETMPEND, 1, 1)
+FIELD(V7M_CCR, UNALIGN_TRP, 3, 1)
+FIELD(V7M_CCR, DIV_0_TRP, 4, 1)
+FIELD(V7M_CCR, BFHFNMIGN, 8, 1)
+FIELD(V7M_CCR, STKALIGN, 9, 1)
+FIELD(V7M_CCR, DC, 16, 1)
+FIELD(V7M_CCR, IC, 17, 1)
+
+/* V7M CFSR bits for MMFSR */
+FIELD(V7M_CFSR, IACCVIOL, 0, 1)
+FIELD(V7M_CFSR, DACCVIOL, 1, 1)
+FIELD(V7M_CFSR, MUNSTKERR, 3, 1)
+FIELD(V7M_CFSR, MSTKERR, 4, 1)
+FIELD(V7M_CFSR, MLSPERR, 5, 1)
+FIELD(V7M_CFSR, MMARVALID, 7, 1)
+
+/* V7M CFSR bits for BFSR */
+FIELD(V7M_CFSR, IBUSERR, 8 + 0, 1)
+FIELD(V7M_CFSR, PRECISERR, 8 + 1, 1)
+FIELD(V7M_CFSR, IMPRECISERR, 8 + 2, 1)
+FIELD(V7M_CFSR, UNSTKERR, 8 + 3, 1)
+FIELD(V7M_CFSR, STKERR, 8 + 4, 1)
+FIELD(V7M_CFSR, LSPERR, 8 + 5, 1)
+FIELD(V7M_CFSR, BFARVALID, 8 + 7, 1)
+
+/* V7M CFSR bits for UFSR */
+FIELD(V7M_CFSR, UNDEFINSTR, 16 + 0, 1)
+FIELD(V7M_CFSR, INVSTATE, 16 + 1, 1)
+FIELD(V7M_CFSR, INVPC, 16 + 2, 1)
+FIELD(V7M_CFSR, NOCP, 16 + 3, 1)
+FIELD(V7M_CFSR, UNALIGNED, 16 + 8, 1)
+FIELD(V7M_CFSR, DIVBYZERO, 16 + 9, 1)
+
+/* V7M HFSR bits */
+FIELD(V7M_HFSR, VECTTBL, 1, 1)
+FIELD(V7M_HFSR, FORCED, 30, 1)
+FIELD(V7M_HFSR, DEBUGEVT, 31, 1)
+
+/* V7M DFSR bits */
+FIELD(V7M_DFSR, HALTED, 0, 1)
+FIELD(V7M_DFSR, BKPT, 1, 1)
+FIELD(V7M_DFSR, DWTTRAP, 2, 1)
+FIELD(V7M_DFSR, VCATCH, 3, 1)
+FIELD(V7M_DFSR, EXTERNAL, 4, 1)
+
 /* If adding a feature bit which corresponds to a Linux ELF
  * HWCAP bit, remember to update the feature-bit-to-hwcap
  * mapping in linux-user/elfload.c:get_elf_hwcap().
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 6395d5a..c804f59 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -188,6 +188,13 @@ static void arm_cpu_reset(CPUState *s)
         uint8_t *rom;
 
         env->daif &= ~PSTATE_I;
+
+        /* The reset value of this bit is IMPDEF, but ARM recommends
+         * that it resets to 1, so QEMU always does that rather than making
+         * it dependent on CPU model.
+         */
+        env->v7m.ccr = R_V7M_CCR_STKALIGN_MASK;
+
         rom = rom_ptr(0);
         if (rom) {
             /* Address zero is covered by ROM which hasn't yet been
diff --git a/target/arm/machine.c b/target/arm/machine.c
index 8ed24bf..49e09a8 100644
--- a/target/arm/machine.c
+++ b/target/arm/machine.c
@@ -96,13 +96,19 @@ static bool m_needed(void *opaque)
 
 static const VMStateDescription vmstate_m = {
     .name = "cpu/m",
-    .version_id = 2,
-    .minimum_version_id = 2,
+    .version_id = 3,
+    .minimum_version_id = 3,
     .needed = m_needed,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(env.v7m.vecbase, ARMCPU),
         VMSTATE_UINT32(env.v7m.basepri, ARMCPU),
         VMSTATE_UINT32(env.v7m.control, ARMCPU),
+        VMSTATE_UINT32(env.v7m.ccr, ARMCPU),
+        VMSTATE_UINT32(env.v7m.cfsr, ARMCPU),
+        VMSTATE_UINT32(env.v7m.hfsr, ARMCPU),
+        VMSTATE_UINT32(env.v7m.dfsr, ARMCPU),
+        VMSTATE_UINT32(env.v7m.mmfar, ARMCPU),
+        VMSTATE_UINT32(env.v7m.bfar, ARMCPU),
         VMSTATE_INT32(env.v7m.exception, ARMCPU),
         VMSTATE_END_OF_LIST()
     }
-- 
2.7.4

Re: [Qemu-devel] [PATCH 03/10] armv7m: add state for v7M CCR, CFSR, HFSR, DFSR, MMFAR, BFAR

[Alex Bennée]

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

> Add the structure fields, VMState fields, reset code and macros for
> the v7M system control registers CCR, CFSR, HFSR, DFSR, MMFAR and
> BFAR.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/cpu.h     | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  target/arm/cpu.c     |  7 +++++++
>  target/arm/machine.c | 10 ++++++++--
>  3 files changed, 69 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index b2cc329..4b062d2 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -21,6 +21,7 @@
>  #define ARM_CPU_H
>
>  #include "kvm-consts.h"
> +#include "hw/registerfields.h"
>
>  #if defined(TARGET_AARCH64)
>    /* AArch64 definitions */
> @@ -405,6 +406,12 @@ typedef struct CPUARMState {
>          uint32_t vecbase;
>          uint32_t basepri;
>          uint32_t control;
> +        uint32_t ccr; /* Configuration and Control */
> +        uint32_t cfsr; /* Configurable Fault Status */
> +        uint32_t hfsr; /* HardFault Status */
> +        uint32_t dfsr; /* Debug Fault Status Register */
> +        uint32_t mmfar; /* MemManage Fault Address */
> +        uint32_t bfar; /* BusFault Address */

Given the CPUARMState needs to be accessed via env do we need to start
getting concerned about its size?

>          int exception;
>      } v7m;
>
> @@ -1086,6 +1093,53 @@ enum arm_cpu_mode {
>  #define ARM_IWMMXT_wCGR2	10
>  #define ARM_IWMMXT_wCGR3	11
>
> +/* V7M CCR bits */
> +FIELD(V7M_CCR, NONBASETHRDENA, 0, 1)
> +FIELD(V7M_CCR, USERSETMPEND, 1, 1)
> +FIELD(V7M_CCR, UNALIGN_TRP, 3, 1)
> +FIELD(V7M_CCR, DIV_0_TRP, 4, 1)
> +FIELD(V7M_CCR, BFHFNMIGN, 8, 1)
> +FIELD(V7M_CCR, STKALIGN, 9, 1)
> +FIELD(V7M_CCR, DC, 16, 1)
> +FIELD(V7M_CCR, IC, 17, 1)
> +
> +/* V7M CFSR bits for MMFSR */
> +FIELD(V7M_CFSR, IACCVIOL, 0, 1)
> +FIELD(V7M_CFSR, DACCVIOL, 1, 1)
> +FIELD(V7M_CFSR, MUNSTKERR, 3, 1)
> +FIELD(V7M_CFSR, MSTKERR, 4, 1)
> +FIELD(V7M_CFSR, MLSPERR, 5, 1)
> +FIELD(V7M_CFSR, MMARVALID, 7, 1)
> +
> +/* V7M CFSR bits for BFSR */
> +FIELD(V7M_CFSR, IBUSERR, 8 + 0, 1)
> +FIELD(V7M_CFSR, PRECISERR, 8 + 1, 1)
> +FIELD(V7M_CFSR, IMPRECISERR, 8 + 2, 1)
> +FIELD(V7M_CFSR, UNSTKERR, 8 + 3, 1)
> +FIELD(V7M_CFSR, STKERR, 8 + 4, 1)
> +FIELD(V7M_CFSR, LSPERR, 8 + 5, 1)
> +FIELD(V7M_CFSR, BFARVALID, 8 + 7, 1)
> +
> +/* V7M CFSR bits for UFSR */
> +FIELD(V7M_CFSR, UNDEFINSTR, 16 + 0, 1)
> +FIELD(V7M_CFSR, INVSTATE, 16 + 1, 1)
> +FIELD(V7M_CFSR, INVPC, 16 + 2, 1)
> +FIELD(V7M_CFSR, NOCP, 16 + 3, 1)
> +FIELD(V7M_CFSR, UNALIGNED, 16 + 8, 1)
> +FIELD(V7M_CFSR, DIVBYZERO, 16 + 9, 1)
> +
> +/* V7M HFSR bits */
> +FIELD(V7M_HFSR, VECTTBL, 1, 1)
> +FIELD(V7M_HFSR, FORCED, 30, 1)
> +FIELD(V7M_HFSR, DEBUGEVT, 31, 1)
> +
> +/* V7M DFSR bits */
> +FIELD(V7M_DFSR, HALTED, 0, 1)
> +FIELD(V7M_DFSR, BKPT, 1, 1)
> +FIELD(V7M_DFSR, DWTTRAP, 2, 1)
> +FIELD(V7M_DFSR, VCATCH, 3, 1)
> +FIELD(V7M_DFSR, EXTERNAL, 4, 1)
> +
>  /* If adding a feature bit which corresponds to a Linux ELF
>   * HWCAP bit, remember to update the feature-bit-to-hwcap
>   * mapping in linux-user/elfload.c:get_elf_hwcap().
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 6395d5a..c804f59 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -188,6 +188,13 @@ static void arm_cpu_reset(CPUState *s)
>          uint8_t *rom;
>
>          env->daif &= ~PSTATE_I;
> +
> +        /* The reset value of this bit is IMPDEF, but ARM recommends
> +         * that it resets to 1, so QEMU always does that rather than making
> +         * it dependent on CPU model.
> +         */
> +        env->v7m.ccr = R_V7M_CCR_STKALIGN_MASK;
> +
>          rom = rom_ptr(0);
>          if (rom) {
>              /* Address zero is covered by ROM which hasn't yet been
> diff --git a/target/arm/machine.c b/target/arm/machine.c
> index 8ed24bf..49e09a8 100644
> --- a/target/arm/machine.c
> +++ b/target/arm/machine.c
> @@ -96,13 +96,19 @@ static bool m_needed(void *opaque)
>
>  static const VMStateDescription vmstate_m = {
>      .name = "cpu/m",
> -    .version_id = 2,
> -    .minimum_version_id = 2,
> +    .version_id = 3,
> +    .minimum_version_id = 3,
>      .needed = m_needed,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT32(env.v7m.vecbase, ARMCPU),
>          VMSTATE_UINT32(env.v7m.basepri, ARMCPU),
>          VMSTATE_UINT32(env.v7m.control, ARMCPU),
> +        VMSTATE_UINT32(env.v7m.ccr, ARMCPU),
> +        VMSTATE_UINT32(env.v7m.cfsr, ARMCPU),
> +        VMSTATE_UINT32(env.v7m.hfsr, ARMCPU),
> +        VMSTATE_UINT32(env.v7m.dfsr, ARMCPU),
> +        VMSTATE_UINT32(env.v7m.mmfar, ARMCPU),
> +        VMSTATE_UINT32(env.v7m.bfar, ARMCPU),
>          VMSTATE_INT32(env.v7m.exception, ARMCPU),
>          VMSTATE_END_OF_LIST()
>      }

Otherwise:

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

--
Alex Bennée

Re: [Qemu-devel] [PATCH 03/10] armv7m: add state for v7M CCR, CFSR, HFSR, DFSR, MMFAR, BFAR

[Peter Maydell]

On 27 January 2017 at 12:28, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> Add the structure fields, VMState fields, reset code and macros for
>> the v7M system control registers CCR, CFSR, HFSR, DFSR, MMFAR and
>> BFAR.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  target/arm/cpu.h     | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  target/arm/cpu.c     |  7 +++++++
>>  target/arm/machine.c | 10 ++++++++--
>>  3 files changed, 69 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index b2cc329..4b062d2 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -21,6 +21,7 @@
>>  #define ARM_CPU_H
>>
>>  #include "kvm-consts.h"
>> +#include "hw/registerfields.h"
>>
>>  #if defined(TARGET_AARCH64)
>>    /* AArch64 definitions */
>> @@ -405,6 +406,12 @@ typedef struct CPUARMState {
>>          uint32_t vecbase;
>>          uint32_t basepri;
>>          uint32_t control;
>> +        uint32_t ccr; /* Configuration and Control */
>> +        uint32_t cfsr; /* Configurable Fault Status */
>> +        uint32_t hfsr; /* HardFault Status */
>> +        uint32_t dfsr; /* Debug Fault Status Register */
>> +        uint32_t mmfar; /* MemManage Fault Address */
>> +        uint32_t bfar; /* BusFault Address */
>
> Given the CPUARMState needs to be accessed via env do we need to start
> getting concerned about its size?

We only care that accesses to the front of it are within easy
reach (specifically, accesses to fields via frequently used
TCG globals); it doesn't matter if more things are added at
the end.

thanks
-- PMM

[Qemu-devel] [PATCH 04/10] armv7m: implement CCR, CFSR, HFSR, DFSR, BFAR, and MMFAR

[Peter Maydell]

From: Michael Davidsaver <mdavidsaver@gmail.com>

Implement the v7M system registers CCR, CFSR, HFSR, DFSR, BFAR and
MMFAR.  For the moment these simply read as written (with some basic
handling of RAZ/WI bits and W1C semantics).

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
[PMM: drop warning about setting unimplemented CCR bits;
 tweak commit message; add DFSR]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/armv7m_nvic.c | 42 ++++++++++++++++++++++++++++++++----------
 1 file changed, 32 insertions(+), 10 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 81dcb83..60e72d7 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -228,8 +228,7 @@ static uint32_t nvic_readl(nvic_state *s, uint32_t offset)
         /* TODO: Implement SLEEPONEXIT.  */
         return 0;
     case 0xd14: /* Configuration Control.  */
-        /* TODO: Implement Configuration Control bits.  */
-        return 0;
+        return cpu->env.v7m.ccr;
     case 0xd24: /* System Handler Status.  */
         val = 0;
         if (s->gic.irq_state[ARMV7M_EXCP_MEM].active) val |= (1 << 0);
@@ -248,16 +247,19 @@ static uint32_t nvic_readl(nvic_state *s, uint32_t offset)
         if (s->gic.irq_state[ARMV7M_EXCP_USAGE].enabled) val |= (1 << 18);
         return val;
     case 0xd28: /* Configurable Fault Status.  */
-        /* TODO: Implement Fault Status.  */
-        qemu_log_mask(LOG_UNIMP, "Configurable Fault Status unimplemented\n");
-        return 0;
+        return cpu->env.v7m.cfsr;
     case 0xd2c: /* Hard Fault Status.  */
+        return cpu->env.v7m.hfsr;
     case 0xd30: /* Debug Fault Status.  */
-    case 0xd34: /* Mem Manage Address.  */
+        return cpu->env.v7m.dfsr;
+    case 0xd34: /* MMFAR MemManage Fault Address */
+        return cpu->env.v7m.mmfar;
     case 0xd38: /* Bus Fault Address.  */
+        return cpu->env.v7m.bfar;
     case 0xd3c: /* Aux Fault Status.  */
         /* TODO: Implement fault status registers.  */
-        qemu_log_mask(LOG_UNIMP, "Fault status registers unimplemented\n");
+        qemu_log_mask(LOG_UNIMP,
+                      "Aux Fault status registers unimplemented\n");
         return 0;
     case 0xd40: /* PFR0.  */
         return 0x00000030;
@@ -366,9 +368,19 @@ static void nvic_writel(nvic_state *s, uint32_t offset, uint32_t value)
         }
         break;
     case 0xd10: /* System Control.  */
-    case 0xd14: /* Configuration Control.  */
         /* TODO: Implement control registers.  */
-        qemu_log_mask(LOG_UNIMP, "NVIC: SCR and CCR unimplemented\n");
+        qemu_log_mask(LOG_UNIMP, "NVIC: SCR unimplemented\n");
+        break;
+    case 0xd14: /* Configuration Control.  */
+        /* Enforce RAZ/WI on reserved and must-RAZ/WI bits */
+        value &= (R_V7M_CCR_STKALIGN_MASK |
+                  R_V7M_CCR_BFHFNMIGN_MASK |
+                  R_V7M_CCR_DIV_0_TRP_MASK |
+                  R_V7M_CCR_UNALIGN_TRP_MASK |
+                  R_V7M_CCR_USERSETMPEND_MASK |
+                  R_V7M_CCR_NONBASETHRDENA_MASK);
+
+        cpu->env.v7m.ccr = value;
         break;
     case 0xd24: /* System Handler Control.  */
         /* TODO: Real hardware allows you to set/clear the active bits
@@ -378,13 +390,23 @@ static void nvic_writel(nvic_state *s, uint32_t offset, uint32_t value)
         s->gic.irq_state[ARMV7M_EXCP_USAGE].enabled = (value & (1 << 18)) != 0;
         break;
     case 0xd28: /* Configurable Fault Status.  */
+        cpu->env.v7m.cfsr &= ~value; /* W1C */
+        break;
     case 0xd2c: /* Hard Fault Status.  */
+        cpu->env.v7m.hfsr &= ~value; /* W1C */
+        break;
     case 0xd30: /* Debug Fault Status.  */
+        cpu->env.v7m.dfsr &= ~value; /* W1C */
+        break;
     case 0xd34: /* Mem Manage Address.  */
+        cpu->env.v7m.mmfar = value;
+        return;
     case 0xd38: /* Bus Fault Address.  */
+        cpu->env.v7m.bfar = value;
+        return;
     case 0xd3c: /* Aux Fault Status.  */
         qemu_log_mask(LOG_UNIMP,
-                      "NVIC: fault status registers unimplemented\n");
+                      "NVIC: Aux fault status registers unimplemented\n");
         break;
     case 0xf00: /* Software Triggered Interrupt Register */
         if ((value & 0x1ff) < s->num_irq) {
-- 
2.7.4

Re: [Qemu-devel] [PATCH 04/10] armv7m: implement CCR, CFSR, HFSR, DFSR, BFAR, and MMFAR

[Alex Bennée]

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

> From: Michael Davidsaver <mdavidsaver@gmail.com>
>
> Implement the v7M system registers CCR, CFSR, HFSR, DFSR, BFAR and
> MMFAR.  For the moment these simply read as written (with some basic
> handling of RAZ/WI bits and W1C semantics).
>
> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
> [PMM: drop warning about setting unimplemented CCR bits;
>  tweak commit message; add DFSR]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

> ---
>  hw/intc/armv7m_nvic.c | 42 ++++++++++++++++++++++++++++++++----------
>  1 file changed, 32 insertions(+), 10 deletions(-)
>
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 81dcb83..60e72d7 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -228,8 +228,7 @@ static uint32_t nvic_readl(nvic_state *s, uint32_t offset)
>          /* TODO: Implement SLEEPONEXIT.  */
>          return 0;
>      case 0xd14: /* Configuration Control.  */
> -        /* TODO: Implement Configuration Control bits.  */
> -        return 0;
> +        return cpu->env.v7m.ccr;
>      case 0xd24: /* System Handler Status.  */
>          val = 0;
>          if (s->gic.irq_state[ARMV7M_EXCP_MEM].active) val |= (1 << 0);
> @@ -248,16 +247,19 @@ static uint32_t nvic_readl(nvic_state *s, uint32_t offset)
>          if (s->gic.irq_state[ARMV7M_EXCP_USAGE].enabled) val |= (1 << 18);
>          return val;
>      case 0xd28: /* Configurable Fault Status.  */
> -        /* TODO: Implement Fault Status.  */
> -        qemu_log_mask(LOG_UNIMP, "Configurable Fault Status unimplemented\n");
> -        return 0;
> +        return cpu->env.v7m.cfsr;
>      case 0xd2c: /* Hard Fault Status.  */
> +        return cpu->env.v7m.hfsr;
>      case 0xd30: /* Debug Fault Status.  */
> -    case 0xd34: /* Mem Manage Address.  */
> +        return cpu->env.v7m.dfsr;
> +    case 0xd34: /* MMFAR MemManage Fault Address */
> +        return cpu->env.v7m.mmfar;
>      case 0xd38: /* Bus Fault Address.  */
> +        return cpu->env.v7m.bfar;
>      case 0xd3c: /* Aux Fault Status.  */
>          /* TODO: Implement fault status registers.  */
> -        qemu_log_mask(LOG_UNIMP, "Fault status registers unimplemented\n");
> +        qemu_log_mask(LOG_UNIMP,
> +                      "Aux Fault status registers unimplemented\n");
>          return 0;
>      case 0xd40: /* PFR0.  */
>          return 0x00000030;
> @@ -366,9 +368,19 @@ static void nvic_writel(nvic_state *s, uint32_t offset, uint32_t value)
>          }
>          break;
>      case 0xd10: /* System Control.  */
> -    case 0xd14: /* Configuration Control.  */
>          /* TODO: Implement control registers.  */
> -        qemu_log_mask(LOG_UNIMP, "NVIC: SCR and CCR unimplemented\n");
> +        qemu_log_mask(LOG_UNIMP, "NVIC: SCR unimplemented\n");
> +        break;
> +    case 0xd14: /* Configuration Control.  */
> +        /* Enforce RAZ/WI on reserved and must-RAZ/WI bits */
> +        value &= (R_V7M_CCR_STKALIGN_MASK |
> +                  R_V7M_CCR_BFHFNMIGN_MASK |
> +                  R_V7M_CCR_DIV_0_TRP_MASK |
> +                  R_V7M_CCR_UNALIGN_TRP_MASK |
> +                  R_V7M_CCR_USERSETMPEND_MASK |
> +                  R_V7M_CCR_NONBASETHRDENA_MASK);
> +
> +        cpu->env.v7m.ccr = value;
>          break;
>      case 0xd24: /* System Handler Control.  */
>          /* TODO: Real hardware allows you to set/clear the active bits
> @@ -378,13 +390,23 @@ static void nvic_writel(nvic_state *s, uint32_t offset, uint32_t value)
>          s->gic.irq_state[ARMV7M_EXCP_USAGE].enabled = (value & (1 << 18)) != 0;
>          break;
>      case 0xd28: /* Configurable Fault Status.  */
> +        cpu->env.v7m.cfsr &= ~value; /* W1C */
> +        break;
>      case 0xd2c: /* Hard Fault Status.  */
> +        cpu->env.v7m.hfsr &= ~value; /* W1C */
> +        break;
>      case 0xd30: /* Debug Fault Status.  */
> +        cpu->env.v7m.dfsr &= ~value; /* W1C */
> +        break;
>      case 0xd34: /* Mem Manage Address.  */
> +        cpu->env.v7m.mmfar = value;
> +        return;
>      case 0xd38: /* Bus Fault Address.  */
> +        cpu->env.v7m.bfar = value;
> +        return;
>      case 0xd3c: /* Aux Fault Status.  */
>          qemu_log_mask(LOG_UNIMP,
> -                      "NVIC: fault status registers unimplemented\n");
> +                      "NVIC: Aux fault status registers unimplemented\n");
>          break;
>      case 0xf00: /* Software Triggered Interrupt Register */
>          if ((value & 0x1ff) < s->num_irq) {


--
Alex Bennée

[Qemu-devel] [PATCH 05/10] armv7m: honour CCR.STACKALIGN on exception entry

[Peter Maydell]

From: Michael Davidsaver <mdavidsaver@gmail.com>

The CCR.STACKALIGN bit controls whether the CPU is supposed to force
8-alignment of the stack pointer on entry to the exception handler.

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
[PMM: commit message and comment tweaks]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index ce7e43b..7dc30f5 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6110,10 +6110,8 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
         return; /* Never happens.  Keep compiler happy.  */
     }
 
-    /* Align stack pointer.  */
-    /* ??? Should only do this if Configuration Control Register
-       STACKALIGN bit is set.  */
-    if (env->regs[13] & 4) {
+    /* Align stack pointer if the guest wants that */
+    if ((env->regs[13] & 4) && (env->v7m.ccr & R_V7M_CCR_STKALIGN_MASK)) {
         env->regs[13] -= 4;
         xpsr |= 0x200;
     }
-- 
2.7.4

Re: [Qemu-devel] [PATCH 05/10] armv7m: honour CCR.STACKALIGN on exception entry

[Richard Henderson]

On 01/24/2017 11:16 AM, Peter Maydell wrote:
> The CCR.STACKALIGN bit controls whether the CPU is supposed to force
> 8-alignment of the stack pointer on entry to the exception handler.

8...

> +    /* Align stack pointer if the guest wants that */
> +    if ((env->regs[13] & 4) && (env->v7m.ccr & R_V7M_CCR_STKALIGN_MASK)) {

4...

>          env->regs[13] -= 4;

Not alignment.  "&= -4"?


r~

Re: [Qemu-devel] [PATCH 05/10] armv7m: honour CCR.STACKALIGN on exception entry

[Peter Maydell]

On 24 January 2017 at 19:33, Richard Henderson <rth@twiddle.net> wrote:
> On 01/24/2017 11:16 AM, Peter Maydell wrote:
>> The CCR.STACKALIGN bit controls whether the CPU is supposed to force
>> 8-alignment of the stack pointer on entry to the exception handler.
>
> 8...
>
>> +    /* Align stack pointer if the guest wants that */
>> +    if ((env->regs[13] & 4) && (env->v7m.ccr & R_V7M_CCR_STKALIGN_MASK)) {
>
> 4...
>
>>          env->regs[13] -= 4;
>
> Not alignment.  "&= -4"?

We know SP is always at least a multiple of 4. If it's already
a multiple of 8 then (sp & 4) will be false and we leave sp alone.
Otherwise it's a multiple of 4 but not 8, and subtracting 4
makes it 8 aligned (and we set the saved-XPSR bit to indicate
that we need to undo that on exception exit).

You could maybe rephrase the code to look a bit closer to the
v7M ARM ARM pseudocode, but the way it's written now isn't wrong,
so since this patch is only trying to say "do this if STKALIGN
is set rather than all the time" just adjusting the if conditional
seemed the best thing to me.

(The pseudocode checks for "do we need to align" with
"SP<2> AND forcealign", and does the alignment by
ANDing with a mask constructed with "NOT(ZeroExtend(forcealign:'00',32))".
So we do the check the same way it does, but use a subtract
rather than an AND-NOT. (Since we know that bit 2 must be set
then subtracting 4 and masking that bit to 0 are the same thing.)

thanks
-- PMM

[Qemu-devel] [PATCH 06/10] armv7m: set CFSR.UNDEFINSTR on undefined instructions

[Peter Maydell]

From: Michael Davidsaver <mdavidsaver@gmail.com>

When we take an exception for an undefined instruction, set the
appropriate CFSR bit.

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

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 7dc30f5..e6b1c36 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6072,6 +6072,7 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
     switch (cs->exception_index) {
     case EXCP_UDEF:
         armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE);
+        env->v7m.cfsr |= R_V7M_CFSR_UNDEFINSTR_MASK;
         return;
     case EXCP_SWI:
         /* The PC already points to the next instruction.  */
-- 
2.7.4

Re: [Qemu-devel] [PATCH 06/10] armv7m: set CFSR.UNDEFINSTR on undefined instructions

[Alex Bennée]

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

> From: Michael Davidsaver <mdavidsaver@gmail.com>
>
> When we take an exception for an undefined instruction, set the
> appropriate CFSR bit.
>
> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
> [PMM: tweaked commit message, comment]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

> ---
>  target/arm/helper.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 7dc30f5..e6b1c36 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6072,6 +6072,7 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>      switch (cs->exception_index) {
>      case EXCP_UDEF:
>          armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE);
> +        env->v7m.cfsr |= R_V7M_CFSR_UNDEFINSTR_MASK;
>          return;
>      case EXCP_SWI:
>          /* The PC already points to the next instruction.  */


--
Alex Bennée

[Qemu-devel] [PATCH 07/10] armv7m: Report no-coprocessor faults correctly

[Peter Maydell]

For v7M attempts to access a nonexistent coprocessor are reported
differently from plain undefined instructions (as UsageFaults of type
NOCP rather than type UNDEFINSTR).  Split them out into a new
EXCP_NOCP so we can report the FSR value correctly.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.h       | 1 +
 linux-user/main.c      | 1 +
 target/arm/helper.c    | 4 ++++
 target/arm/translate.c | 8 ++++++++
 4 files changed, 14 insertions(+)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 4b062d2..39bff86 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -53,6 +53,7 @@
 #define EXCP_VIRQ           14
 #define EXCP_VFIQ           15
 #define EXCP_SEMIHOST       16   /* semihosting call */
+#define EXCP_NOCP           17   /* v7M NOCP UsageFault */
 
 #define ARMV7M_EXCP_RESET   1
 #define ARMV7M_EXCP_NMI     2
diff --git a/linux-user/main.c b/linux-user/main.c
index db4eb68..f40d45a 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -570,6 +570,7 @@ void cpu_loop(CPUARMState *env)
 
         switch(trapnr) {
         case EXCP_UDEF:
+        case EXCP_NOCP:
             {
                 TaskState *ts = cs->opaque;
                 uint32_t opcode;
diff --git a/target/arm/helper.c b/target/arm/helper.c
index e6b1c36..c23df1b 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6074,6 +6074,10 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
         armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE);
         env->v7m.cfsr |= R_V7M_CFSR_UNDEFINSTR_MASK;
         return;
+    case EXCP_NOCP:
+        armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE);
+        env->v7m.cfsr |= R_V7M_CFSR_NOCP_MASK;
+        return;
     case EXCP_SWI:
         /* The PC already points to the next instruction.  */
         armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_SVC);
diff --git a/target/arm/translate.c b/target/arm/translate.c
index a7c2abe..493c627 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -10217,6 +10217,14 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
         break;
     case 6: case 7: case 14: case 15:
         /* Coprocessor.  */
+        if (arm_dc_feature(s, ARM_FEATURE_M)) {
+            /* We don't currently implement M profile FP support,
+             * so this entire space should give a NOCP fault.
+             */
+            gen_exception_insn(s, 4, EXCP_NOCP, syn_uncategorized(),
+                               default_exception_el(s));
+            break;
+        }
         if (((insn >> 24) & 3) == 3) {
             /* Translate into the equivalent ARM encoding.  */
             insn = (insn & 0xe2ffffff) | ((insn & (1 << 28)) >> 4) | (1 << 28);
-- 
2.7.4

Re: [Qemu-devel] [PATCH 07/10] armv7m: Report no-coprocessor faults correctly

[Alex Bennée]

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

> For v7M attempts to access a nonexistent coprocessor are reported
> differently from plain undefined instructions (as UsageFaults of type
> NOCP rather than type UNDEFINSTR).  Split them out into a new
> EXCP_NOCP so we can report the FSR value correctly.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

> ---
>  target/arm/cpu.h       | 1 +
>  linux-user/main.c      | 1 +
>  target/arm/helper.c    | 4 ++++
>  target/arm/translate.c | 8 ++++++++
>  4 files changed, 14 insertions(+)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 4b062d2..39bff86 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -53,6 +53,7 @@
>  #define EXCP_VIRQ           14
>  #define EXCP_VFIQ           15
>  #define EXCP_SEMIHOST       16   /* semihosting call */
> +#define EXCP_NOCP           17   /* v7M NOCP UsageFault */
>
>  #define ARMV7M_EXCP_RESET   1
>  #define ARMV7M_EXCP_NMI     2
> diff --git a/linux-user/main.c b/linux-user/main.c
> index db4eb68..f40d45a 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -570,6 +570,7 @@ void cpu_loop(CPUARMState *env)
>
>          switch(trapnr) {
>          case EXCP_UDEF:
> +        case EXCP_NOCP:
>              {
>                  TaskState *ts = cs->opaque;
>                  uint32_t opcode;
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index e6b1c36..c23df1b 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6074,6 +6074,10 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>          armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE);
>          env->v7m.cfsr |= R_V7M_CFSR_UNDEFINSTR_MASK;
>          return;
> +    case EXCP_NOCP:
> +        armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE);
> +        env->v7m.cfsr |= R_V7M_CFSR_NOCP_MASK;
> +        return;
>      case EXCP_SWI:
>          /* The PC already points to the next instruction.  */
>          armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_SVC);
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index a7c2abe..493c627 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -10217,6 +10217,14 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
>          break;
>      case 6: case 7: case 14: case 15:
>          /* Coprocessor.  */
> +        if (arm_dc_feature(s, ARM_FEATURE_M)) {
> +            /* We don't currently implement M profile FP support,
> +             * so this entire space should give a NOCP fault.
> +             */
> +            gen_exception_insn(s, 4, EXCP_NOCP, syn_uncategorized(),
> +                               default_exception_el(s));
> +            break;
> +        }
>          if (((insn >> 24) & 3) == 3) {
>              /* Translate into the equivalent ARM encoding.  */
>              insn = (insn & 0xe2ffffff) | ((insn & (1 << 28)) >> 4) | (1 << 28);


--
Alex Bennée

[Qemu-devel] [PATCH 08/10] armv7m: Honour CCR.USERSETMPEND

[Peter Maydell]

From: Michael Davidsaver <mdavidsaver@gmail.com>

The CCR.USERSETMPEND bit has to be set to permit unprivileged code to
write to the Software Triggered Interrupt register; honour this bit
rather than letting any code write to the register.

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
[PMM: Tweak commit message, comment, phrasing of condition]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/armv7m_nvic.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 60e72d7..fe5c303 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -409,7 +409,10 @@ static void nvic_writel(nvic_state *s, uint32_t offset, uint32_t value)
                       "NVIC: Aux fault status registers unimplemented\n");
         break;
     case 0xf00: /* Software Triggered Interrupt Register */
-        if ((value & 0x1ff) < s->num_irq) {
+        /* user mode can only write to STIR if CCR.USERSETMPEND permits it */
+        if ((value & 0x1ff) < s->num_irq &&
+            (arm_current_el(&cpu->env) ||
+             (cpu->env.v7m.ccr & R_V7M_CCR_USERSETMPEND_MASK))) {
             gic_set_pending_private(&s->gic, 0, value & 0x1ff);
         }
         break;
-- 
2.7.4

Re: [Qemu-devel] [PATCH 08/10] armv7m: Honour CCR.USERSETMPEND

[Alex Bennée]

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

> From: Michael Davidsaver <mdavidsaver@gmail.com>
>
> The CCR.USERSETMPEND bit has to be set to permit unprivileged code to
> write to the Software Triggered Interrupt register; honour this bit
> rather than letting any code write to the register.
>
> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
> [PMM: Tweak commit message, comment, phrasing of condition]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

> ---
>  hw/intc/armv7m_nvic.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 60e72d7..fe5c303 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -409,7 +409,10 @@ static void nvic_writel(nvic_state *s, uint32_t offset, uint32_t value)
>                        "NVIC: Aux fault status registers unimplemented\n");
>          break;
>      case 0xf00: /* Software Triggered Interrupt Register */
> -        if ((value & 0x1ff) < s->num_irq) {
> +        /* user mode can only write to STIR if CCR.USERSETMPEND permits it */
> +        if ((value & 0x1ff) < s->num_irq &&
> +            (arm_current_el(&cpu->env) ||
> +             (cpu->env.v7m.ccr & R_V7M_CCR_USERSETMPEND_MASK))) {
>              gic_set_pending_private(&s->gic, 0, value & 0x1ff);
>          }
>          break;


--
Alex Bennée

[Qemu-devel] [PATCH 09/10] armv7m: FAULTMASK should be 0 on reset

[Peter Maydell]

From: Michael Davidsaver <mdavidsaver@gmail.com>

For M profile CPUs, FAULTMASK should be 0 on reset, like PRIMASK.
QEMU stores FAULTMASK in the PSTATE F bit, so (as with PRIMASK in the
I bit) we have to clear these to undo the A profile default of 1.

Update the comment accordingly and move it so that it's closer to the
code it's referring to.

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
[PMM: rewrote commit message, moved comments]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index c804f59..0814f73 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -179,15 +179,16 @@ static void arm_cpu_reset(CPUState *s)
     /* SVC mode with interrupts disabled.  */
     env->uncached_cpsr = ARM_CPU_MODE_SVC;
     env->daif = PSTATE_D | PSTATE_A | PSTATE_I | PSTATE_F;
-    /* On ARMv7-M the CPSR_I is the value of the PRIMASK register, and is
-     * clear at reset. Initial SP and PC are loaded from ROM.
-     */
+
     if (arm_feature(env, ARM_FEATURE_M)) {
         uint32_t initial_msp; /* Loaded from 0x0 */
         uint32_t initial_pc; /* Loaded from 0x4 */
         uint8_t *rom;
 
-        env->daif &= ~PSTATE_I;
+        /* For M profile we store FAULTMASK and PRIMASK in the
+         * PSTATE F and I bits; these are both clear at reset.
+         */
+        env->daif &= ~(PSTATE_I | PSTATE_F);
 
         /* The reset value of this bit is IMPDEF, but ARM recommends
          * that it resets to 1, so QEMU always does that rather than making
@@ -195,6 +196,7 @@ static void arm_cpu_reset(CPUState *s)
          */
         env->v7m.ccr = R_V7M_CCR_STKALIGN_MASK;
 
+        /* Load the initial SP and PC from the vector table at address 0 */
         rom = rom_ptr(0);
         if (rom) {
             /* Address zero is covered by ROM which hasn't yet been
-- 
2.7.4

Re: [Qemu-devel] [PATCH 09/10] armv7m: FAULTMASK should be 0 on reset

[Alex Bennée]

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

> From: Michael Davidsaver <mdavidsaver@gmail.com>
>
> For M profile CPUs, FAULTMASK should be 0 on reset, like PRIMASK.
> QEMU stores FAULTMASK in the PSTATE F bit, so (as with PRIMASK in the
> I bit) we have to clear these to undo the A profile default of 1.
>
> Update the comment accordingly and move it so that it's closer to the
> code it's referring to.
>
> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
> [PMM: rewrote commit message, moved comments]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

> ---
>  target/arm/cpu.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index c804f59..0814f73 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -179,15 +179,16 @@ static void arm_cpu_reset(CPUState *s)
>      /* SVC mode with interrupts disabled.  */
>      env->uncached_cpsr = ARM_CPU_MODE_SVC;
>      env->daif = PSTATE_D | PSTATE_A | PSTATE_I | PSTATE_F;
> -    /* On ARMv7-M the CPSR_I is the value of the PRIMASK register, and is
> -     * clear at reset. Initial SP and PC are loaded from ROM.
> -     */
> +
>      if (arm_feature(env, ARM_FEATURE_M)) {
>          uint32_t initial_msp; /* Loaded from 0x0 */
>          uint32_t initial_pc; /* Loaded from 0x4 */
>          uint8_t *rom;
>
> -        env->daif &= ~PSTATE_I;
> +        /* For M profile we store FAULTMASK and PRIMASK in the
> +         * PSTATE F and I bits; these are both clear at reset.
> +         */
> +        env->daif &= ~(PSTATE_I | PSTATE_F);
>
>          /* The reset value of this bit is IMPDEF, but ARM recommends
>           * that it resets to 1, so QEMU always does that rather than making
> @@ -195,6 +196,7 @@ static void arm_cpu_reset(CPUState *s)
>           */
>          env->v7m.ccr = R_V7M_CCR_STKALIGN_MASK;
>
> +        /* Load the initial SP and PC from the vector table at address 0 */
>          rom = rom_ptr(0);
>          if (rom) {
>              /* Address zero is covered by ROM which hasn't yet been


--
Alex Bennée

[Qemu-devel] [PATCH 10/10] armv7m: R14 should reset to 0xffffffff

[Peter Maydell]

For M profile (unlike A profile) the reset value of R14 is specified
as 0xffffffff.  (The rationale is that this is an illegal exception
return value, so if guest code tries to return to it it will result
in a helpful exception.)

Registers r0 to r12 and the flags are architecturally UNKNOWN on
reset, so we leave those at zero.

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

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 0814f73..e9f10f7 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -196,6 +196,9 @@ static void arm_cpu_reset(CPUState *s)
          */
         env->v7m.ccr = R_V7M_CCR_STKALIGN_MASK;
 
+        /* Unlike A/R profile, M profile defines the reset LR value */
+        env->regs[14] = 0xffffffff;
+
         /* Load the initial SP and PC from the vector table at address 0 */
         rom = rom_ptr(0);
         if (rom) {
-- 
2.7.4

Re: [Qemu-devel] [PATCH 10/10] armv7m: R14 should reset to 0xffffffff

[Alex Bennée]

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

> For M profile (unlike A profile) the reset value of R14 is specified
> as 0xffffffff.  (The rationale is that this is an illegal exception
> return value, so if guest code tries to return to it it will result
> in a helpful exception.)
>
> Registers r0 to r12 and the flags are architecturally UNKNOWN on
> reset, so we leave those at zero.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

> ---
>  target/arm/cpu.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 0814f73..e9f10f7 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -196,6 +196,9 @@ static void arm_cpu_reset(CPUState *s)
>           */
>          env->v7m.ccr = R_V7M_CCR_STKALIGN_MASK;
>
> +        /* Unlike A/R profile, M profile defines the reset LR value */
> +        env->regs[14] = 0xffffffff;
> +
>          /* Load the initial SP and PC from the vector table at address 0 */
>          rom = rom_ptr(0);
>          if (rom) {


--
Alex Bennée