[Qemu-devel] [PATCH 00/18] Fix exception handling and msr/mrs access

[Qemu-devel] [PATCH 00/18] Fix exception handling and msr/mrs access

[Michael Davidsaver]

This series grew from a previous incorrect patch attempting to fix some incorrect behavior.  After spending some time going through the arch. ref. manual for v7-M I think I understand better how this should work and have made a number of changes which actually improve the situation.

These changes have not yet been cross checked against real hardware, and I therefore don't consider them mergeable.  It's gotten big enough though that I'd like to get some feedback.

I think the changes in this series effect only ARMv7-M specific code with the exception of removing references to NVIC from the GIC code.

* Add unprivileged access case for MRS/MSR instructions
* Priority based exception masking with PRIMASK, FAULTMASK, and BASEPRI.
* Auto-clear FAULTMASK on exception return (except NMI)
* Validation and consistency checking on exception return
* Exception priorities using PRIGROUP
* Exception escalation to HardFault when priority permits
* Escalation to unrecoverable exception otherwise (though the action is not correct, see below)
* Correct calculation of the RETTOBASE field of ICSR
* Remove the need for the armv7m.hack MemoryRegion to catch exception returns
* Fill in previously unimplemented HFSR, CFSR, and CCR registers

This series removes the dependence of the NVIC code on the GIC.  The GIC doesn't have the concept of PRIGROUP to change the size of the group priority field.  Also, there are a lot of cases in this code which I don't understand and worry about breaking.  Now that I have things working (I think), I could look at recombining them if this is desired.

Some additional state is also added to v7m in struct CPUARMState so that all the information needed
in arm_v7m_cpu_exec_interrupt() is found in one place.  I started by having this state split between CPU and struct nvic_state, but found this confusing.  Some guidance would be helpful.

I add a pointer to ARMCPU* in struct nvic_state which is populated in armv7m_nvic_realize().  I think this is reasonable given the tight coupling between NVIC and CPU, but it does look ugly.

At the moment I've left the action of an unrecoverable exception to call cpu_abort().  I'm not sure of the value of implementing the actual defined behavior in the context of QEMU.

I've tried to add VMState as appropriate, but have not tested it.

I looked briefly at qtest, but can't quite see how to use it given the need to execute code to test most of the exception behavior.  Is something like this feasible at present?

Regards,
Michael


Michael Davidsaver (18):
  armv7m: MRS/MSR handle unprivileged access
  armv7m: Undo armv7m.hack
  armv7m: Complain about incorrect exception table entries.
  armv7m: Explicit error for bad vector table
  armv7m: expand NVIC state
  armv7m: new NVIC utility functions
  armv7m: Update NVIC registers
  armv7m: fix RETTOBASE
  armv7m: NVIC update vmstate
  armv7m: NVIC initialization
  armv7m: fix I and F flag handling
  armv7m: simpler/faster exception start
  armv7m: implement CFSR and HFSR
  armv7m: auto-clear FAULTMASK
  arm: gic: Remove references to NVIC
  armv7m: check exception return consistency
  armv7m: implement CCR
  armv7m: prevent unprivileged write to STIR

 hw/arm/armv7m.c          |   8 -
 hw/intc/arm_gic.c        |  14 +-
 hw/intc/arm_gic_common.c |  23 +-
 hw/intc/armv7m_nvic.c    | 777 ++++++++++++++++++++++++++++++++++++-----------
 hw/intc/gic_internal.h   |   7 +-
 target-arm/cpu.c         |  44 +--
 target-arm/cpu.h         |  35 ++-
 target-arm/helper.c      | 222 ++++++++++----
 target-arm/machine.c     |   7 +-
 9 files changed, 843 insertions(+), 294 deletions(-)

-- 
2.1.4

[Qemu-devel] [PATCH 01/18] armv7m: MRS/MSR handle unprivileged access

[Michael Davidsaver]

The MRS and MSR instruction handling isn't checking
the current permission level.

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
---
 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 4ecae61..4408100 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -7365,23 +7365,32 @@ uint32_t HELPER(get_r13_banked)(CPUARMState *env, uint32_t mode)
 
 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 */
@@ -7393,40 +7402,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)
@@ -7467,8 +7462,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.1.4

[Qemu-devel] [PATCH 02/18] armv7m: Undo armv7m.hack

[Michael Davidsaver]

Add CPU unassigned access handler in place of special
MemoryRegion to catch exception returns.

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
---
 hw/arm/armv7m.c  |  8 --------
 target-arm/cpu.c | 18 ++++++++++++++++++
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index a80d2ad..68146de 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -176,7 +176,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";
@@ -221,13 +220,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 30739fc..be026bc 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -280,6 +280,23 @@ 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;
+
+    if (env->v7m.exception != 0 && addr >= 0xfffffff0 && !is_write) {
+        cpu->exception_index = EXCP_EXCEPTION_EXIT;
+        cpu_loop_exit(cpu);
+    } else {
+        /* TODO, signal some *Fault? */
+        cpu_abort(cpu, "Trying to access outside RAM or ROM at 0x"
+                  TARGET_FMT_plx "\n", addr);
+    }
+}
+
 static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 {
     CPUClass *cc = CPU_GET_CLASS(cs);
@@ -909,6 +926,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;
 }
 
-- 
2.1.4

[Qemu-devel] [PATCH 03/18] armv7m: Complain about incorrect exception table entries.

[Michael Davidsaver]

For -M  These should always be thumb mode.
Log a message if this is seen.

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
---
 target-arm/helper.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 4408100..4178400 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -5396,7 +5396,8 @@ static void do_v7m_exception_exit(CPUARMState *env)
         qemu_log_mask(LOG_GUEST_ERROR,
                       "M profile return from interrupt with misaligned "
                       "PC is UNPREDICTABLE\n");
-        /* Actual hardware seems to ignore the lsbit, and there are several
+        /* The ARM calls for UsageFault when the T bit isn't set, but
+         * actual hardware seems to ignore the lsbit, and there are several
          * RTOSes out there which incorrectly assume the r15 in the stack
          * frame should be a Thumb-style "lsbit indicates ARM/Thumb" value.
          */
@@ -5498,6 +5499,12 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
     addr = ldl_phys(cs->as, env->v7m.vecbase + env->v7m.exception * 4);
     env->regs[15] = addr & 0xfffffffe;
     env->thumb = addr & 1;
+    if (!env->thumb) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "M profile interrupt handler %d with incorrect "
+                      "instruction mode in PC is UNPREDICTABLE\n",
+                      env->v7m.exception);
+    }
 }
 
 /* Function used to synchronize QEMU's AArch64 register set with AArch32
-- 
2.1.4

[Qemu-devel] [PATCH 04/18] armv7m: Explicit error for bad vector table

[Michael Davidsaver]

Give an explicit error and abort when a load
from VECBASE fails.  Otherwise would likely
jump to 0, which for v7-m holds the reset stack
pointer address.

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
---
 target-arm/helper.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 4178400..1d7ac43 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -5496,7 +5496,17 @@ 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);
+    {
+        MemTxResult result;
+        addr = address_space_ldl(cs->as,
+                                 env->v7m.vecbase + env->v7m.exception * 4,
+                                 MEMTXATTRS_UNSPECIFIED, &result);
+        if (result != MEMTX_OK) {
+            cpu_abort(cs, "Failed to read from exception vector table "
+                      "entry %08x\n",
+                      env->v7m.vecbase + env->v7m.exception * 4);
+        }
+    }
     env->regs[15] = addr & 0xfffffffe;
     env->thumb = addr & 1;
     if (!env->thumb) {
-- 
2.1.4

[Qemu-devel] [PATCH 05/18] armv7m: expand NVIC state

[Michael Davidsaver]

Expand the NVIC to fully support -M priorities and masking.
Doesn't use GIC code.

Move some state to ARMCPU to allow calculation of exception masking.

Add storage for PRIGROUP to configure group/sub-group split.
Track group and sub-group in separate fields for quick comparison.
Mix in vector # with sub-group as per tie breaking rules.

NVIC now derives directly from SysBusDevice, and
struct NVICClass is eliminated.

Also add DPRINTF() macro.

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
---
 hw/intc/armv7m_nvic.c | 74 ++++++++++++++++++++++++++++++++++-----------------
 target-arm/cpu.h      |  3 +++
 2 files changed, 52 insertions(+), 25 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 6fc167e..487a09a 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -13,43 +13,67 @@
 #include "hw/sysbus.h"
 #include "qemu/timer.h"
 #include "hw/arm/arm.h"
+#include "target-arm/cpu.h"
 #include "exec/address-spaces.h"
-#include "gic_internal.h"
 
-typedef struct {
-    GICState gic;
+/*#define DEBUG_NVIC 0
+ */
+#ifdef DEBUG_NVIC
+#define DPRINTF(LVL, fmt, ...) \
+do { if ((LVL) <= DEBUG_NVIC) { \
+    fprintf(stderr, "armv7m_nvic: " fmt , ## __VA_ARGS__); \
+} } while (0)
+#else
+#define DPRINTF(LVL, fmt, ...) do {} while (0)
+#endif
+
+/* the number of IRQ lines in addition to the 16 internal
+ * exception vectors.
+ */
+#define NVIC_MAX_IRQ 496
+
+#define NVIC_MAX_VECTORS 512
+
+struct vec_info {
+    uint16_t prio_sub; /* sub-group priority*512 + exception# */
+    int8_t prio_group; /* group priority [-2, 0x7f] */
+    uint8_t raw_prio; /* value writen by guest */
+    uint8_t enabled;
+    uint8_t pending;
+    uint8_t active;
+    uint8_t level;
+    /* exceptions <=15 never set level */
+};
+typedef struct vec_info vec_info;
+
+struct nvic_state {
+    /*< private >*/
+    SysBusDevice parent_obj;
+    /*< public >*/
+
+    ARMCPU *cpu; /* NVIC is so closely tied to the CPU, just keep a ref */
+
+    vec_info vectors[NVIC_MAX_VECTORS];
+
+    uint8_t prigroup;
+
     struct {
         uint32_t control;
         uint32_t reload;
         int64_t tick;
         QEMUTimer *timer;
     } systick;
-    MemoryRegion sysregmem;
-    MemoryRegion gic_iomem_alias;
-    MemoryRegion container;
+
+    MemoryRegion iomem; /* system control space and NVIC */
+
     uint32_t num_irq;
+    qemu_irq excpout;
     qemu_irq sysresetreq;
-} nvic_state;
+};
+typedef struct nvic_state nvic_state;
 
 #define TYPE_NVIC "armv7m_nvic"
-/**
- * NVICClass:
- * @parent_reset: the parent class' reset handler.
- *
- * A model of the v7M NVIC and System Controller
- */
-typedef struct NVICClass {
-    /*< private >*/
-    ARMGICClass parent_class;
-    /*< public >*/
-    DeviceRealize parent_realize;
-    void (*parent_reset)(DeviceState *dev);
-} NVICClass;
-
-#define NVIC_CLASS(klass) \
-    OBJECT_CLASS_CHECK(NVICClass, (klass), TYPE_NVIC)
-#define NVIC_GET_CLASS(obj) \
-    OBJECT_GET_CLASS(NVICClass, (obj), TYPE_NVIC)
+
 #define NVIC(obj) \
     OBJECT_CHECK(nvic_state, (obj), TYPE_NVIC)
 
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 815fef8..c193fbb 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -398,6 +398,9 @@ typedef struct CPUARMState {
         uint32_t control;
         int current_sp;
         int exception;
+        int exception_prio;
+        unsigned pending;
+        int pending_prio;
     } v7m;
 
     /* Information associated with an exception about to be taken:
-- 
2.1.4

[Qemu-devel] [PATCH 06/18] armv7m: new NVIC utility functions

[Michael Davidsaver]

Internal functions for operations previously done
by GIC internals.

nvic_irq_update() recalculates highest pending/active
exceptions.

armv7m_nvic_set_pending() include exception escalation
logic.

armv7m_nvic_acknowledge_irq() and nvic_irq_update()
update ARMCPU fields.

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
---
 hw/intc/armv7m_nvic.c | 250 +++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 235 insertions(+), 15 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 487a09a..ebb4d4e 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -140,36 +140,256 @@ static void systick_reset(nvic_state *s)
     timer_del(s->systick.timer);
 }
 
-/* The external routines use the hardware vector numbering, ie. the first
-   IRQ is #16.  The internal GIC routines use #32 as the first IRQ.  */
+/* caller must call nvic_irq_update() after this */
+static
+void set_prio(nvic_state *s, unsigned irq, uint8_t prio)
+{
+    unsigned submask = (1<<(s->prigroup+1))-1;
+
+    assert(irq > 3); /* only use for configurable prios */
+    assert(irq < NVIC_MAX_VECTORS);
+
+    s->vectors[irq].raw_prio = prio;
+    s->vectors[irq].prio_group = (prio>>(s->prigroup+1));
+    s->vectors[irq].prio_sub = irq + (prio&submask)*NVIC_MAX_VECTORS;
+
+    DPRINTF(0, "Set %u priority grp %d sub %u\n", irq,
+            s->vectors[irq].prio_group, s->vectors[irq].prio_sub);
+}
+
+/* recompute highest pending */
+static
+void nvic_irq_update(nvic_state *s, int update_active)
+{
+    unsigned i;
+    int lvl;
+    CPUARMState *env = &s->cpu->env;
+    int16_t act_group = 0x100, pend_group = 0x100;
+    uint16_t act_sub = 0, pend_sub = 0;
+    uint16_t act_irq = 0, pend_irq = 0;
+
+    /* find highest priority */
+    for (i = 1; i < s->num_irq; i++) {
+        vec_info *I = &s->vectors[i];
+
+        DPRINTF(2, " VECT %d %d:%u\n", i, I->prio_group, I->prio_sub);
+
+        if (I->active && ((I->prio_group < act_group)
+                || (I->prio_group == act_group && I->prio_sub < act_sub)))
+        {
+            act_group = I->prio_group;
+            act_sub = I->prio_sub;
+            act_irq = i;
+        }
+
+        if (I->enabled && I->pending && ((I->prio_group < pend_group)
+                || (I->prio_group == pend_group && I->prio_sub < pend_sub)))
+        {
+            pend_group = I->prio_group;
+            pend_sub = I->prio_sub;
+            pend_irq = i;
+        }
+    }
+
+    env->v7m.pending = pend_irq;
+    env->v7m.pending_prio = pend_group;
+
+    if (update_active) {
+        env->v7m.exception = act_irq;
+        env->v7m.exception_prio = act_group;
+    }
+
+    /* Raise NVIC output even if pend_group is masked.
+     * This is necessary as we get no notification
+     * when PRIMASK et al. are changed.
+     * As long as our output is high cpu_exec() will call
+     * into arm_v7m_cpu_exec_interrupt() frequently, which
+     * then tests to see if the pending exception
+     * is permitted.
+     */
+    lvl = pend_irq > 0;
+    DPRINTF(1, "highest pending %d %d:%u\n", pend_irq, pend_group, pend_sub);
+    DPRINTF(1, "highest active  %d %d:%u\n", act_irq, act_group, act_sub);
+
+    DPRINTF(0, "IRQ %c highest act %d pend %d\n",
+            lvl ? 'X' : '_', act_irq, pend_irq);
+
+    qemu_set_irq(s->excpout, lvl);
+}
+
+static
+void armv7m_nvic_clear_pending(void *opaque, int irq)
+{
+    nvic_state *s = (nvic_state *)opaque;
+    vec_info *I;
+
+    assert(irq >= 0);
+    assert(irq < NVIC_MAX_VECTORS);
+
+    I = &s->vectors[irq];
+    if (I->pending) {
+        I->pending = 0;
+        nvic_irq_update(s, 0);
+    }
+}
+
 void armv7m_nvic_set_pending(void *opaque, int irq)
 {
     nvic_state *s = (nvic_state *)opaque;
-    if (irq >= 16)
-        irq += 16;
-    gic_set_pending_private(&s->gic, 0, irq);
+    CPUARMState *env = &s->cpu->env;
+    vec_info *I;
+    int active = s->cpu->env.v7m.exception;
+
+    assert(irq > 0);
+    assert(irq < NVIC_MAX_VECTORS);
+
+    I = &s->vectors[irq];
+
+    if (irq < ARMV7M_EXCP_SYSTICK && irq != ARMV7M_EXCP_DEBUG) {
+        int runnable = armv7m_excp_unmasked(s->cpu);
+        /* test for exception escalation for vectors other than:
+         * Debug (12), SysTick (15), and all external IRQs (>=16)
+         */
+        unsigned escalate = 0;
+        if (I->active) {
+            /* trying to pend an active fault (possibly nested).
+             * eg. UsageFault in UsageFault handler
+             */
+            escalate = 1;
+            DPRINTF(0, " Escalate, active\n");
+        } else if (!I->enabled) {
+            /* trying to pend a disabled fault
+             * eg. UsageFault while USGFAULTENA in SHCSR is clear.
+             */
+            escalate = 1;
+            DPRINTF(0, " Escalate, not enabled\n");
+        } else if (I->prio_group > runnable) {
+            /* trying to pend a fault which is not immediately
+             * runnable due to masking by PRIMASK, FAULTMASK, BASEPRI,
+             * or the priority of the active exception
+             */
+            DPRINTF(0, " Escalate, mask %d >= %d\n",
+                    I->prio_group, runnable);
+            escalate = 1;
+        }
+
+        if (escalate) {
+#ifdef DEBUG_NVIC
+            int oldirq = irq;
+#endif
+            if (runnable < -1) {
+                /* TODO: actual unrecoverable exception actions */
+                cpu_abort(&s->cpu->parent_obj,
+                          "%d in %d escalates to unrecoverable exception\n",
+                          irq, active);
+            } else {
+                irq = ARMV7M_EXCP_HARD;
+            }
+            I = &s->vectors[irq];
+
+            DPRINTF(0, "Escalate %d to %d\n", oldirq, irq);
+        }
+    }
+
+    I->pending = 1;
+    if (I->enabled && (I->prio_group < env->v7m.pending_prio)) {
+        env->v7m.pending_prio = I->prio_group;
+        env->v7m.pending = irq;
+        qemu_set_irq(s->excpout, irq > 0);
+    }
+    DPRINTF(0, "Pending %d at %d%s runnable %d\n",
+            irq, I->prio_group,
+            env->v7m.pending == irq ? " (highest)" : "",
+            armv7m_excp_unmasked(s->cpu));
 }
 
 /* Make pending IRQ active.  */
-int armv7m_nvic_acknowledge_irq(void *opaque)
+void armv7m_nvic_acknowledge_irq(void *opaque)
 {
     nvic_state *s = (nvic_state *)opaque;
-    uint32_t irq;
+    CPUARMState *env = &s->cpu->env;
+    const int pending = env->v7m.pending;
+    const int runnable = armv7m_excp_unmasked(s->cpu);
+    vec_info *I;
 
-    irq = gic_acknowledge_irq(&s->gic, 0, MEMTXATTRS_UNSPECIFIED);
-    if (irq == 1023)
+    if (!pending) {
         hw_error("Interrupt but no vector\n");
-    if (irq >= 32)
-        irq -= 16;
-    return irq;
+    }
+
+    assert(pending < s->num_irq);
+    I = &s->vectors[pending];
+
+    assert(I->enabled);
+
+    assert(env->v7m.pending_prio == I->prio_group);
+    if (env->v7m.pending_prio > runnable) {
+        hw_error("Interrupt ack. while masked %d > %d",
+                 env->v7m.pending_prio, runnable);
+    }
+
+    DPRINTF(0, "ACT %d at %d\n", pending, I->prio_group);
+
+    assert(I->pending);
+    I->active = 1;
+    I->pending = 0;
+
+    env->v7m.exception = env->v7m.pending;
+    env->v7m.exception_prio = env->v7m.pending_prio;
+
+    nvic_irq_update(s, 0);
+
+    assert(env->v7m.exception > 0); /* spurious exception? */
 }
 
 void armv7m_nvic_complete_irq(void *opaque, int irq)
 {
     nvic_state *s = (nvic_state *)opaque;
-    if (irq >= 16)
-        irq += 16;
-    gic_complete_irq(&s->gic, 0, irq, MEMTXATTRS_UNSPECIFIED);
+    vec_info *I;
+
+    assert(irq > 0);
+    assert(irq < NVIC_MAX_VECTORS);
+
+    I = &s->vectors[irq];
+
+    I->active = 0;
+    I->pending = I->level;
+    assert(!I->level || irq >= 16);
+
+    nvic_irq_update(s, 1);
+    DPRINTF(0, "EOI %d\n", irq);
+}
+
+/* Only called for external interrupt (vector>=16) */
+static
+void set_irq_level(void *opaque, int n, int level)
+{
+    nvic_state *s = opaque;
+    vec_info *I;
+
+    assert(n >= 0);
+    assert(n < NVIC_MAX_IRQ);
+
+    n += 16;
+
+    if (n >= s->num_irq) {
+        return;
+    }
+
+    /* The pending status of an external interrupt is
+     * latched on rising edge and exception handler return.
+     *
+     * Pulsing the IRQ will always run the handler
+     * once, and the handler will re-run until the
+     * level is low when the handler completes.
+     */
+    I = &s->vectors[n];
+    I->level = level;
+    if (level) {
+        DPRINTF(1, "assert IRQ %d\n", n-16);
+        armv7m_nvic_set_pending(s, n-16);
+    } else {
+        DPRINTF(2, "deassert IRQ %d\n", n-16);
+    }
 }
 
 static uint32_t nvic_readl(nvic_state *s, uint32_t offset)
-- 
2.1.4

[Qemu-devel] [PATCH 07/18] armv7m: Update NVIC registers

[Michael Davidsaver]

Replace use of GIC state/functions with new NVIC.

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
---
 hw/intc/armv7m_nvic.c | 233 ++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 168 insertions(+), 65 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index ebb4d4e..30e349e 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -394,13 +394,13 @@ void set_irq_level(void *opaque, int n, int level)
 
 static uint32_t nvic_readl(nvic_state *s, uint32_t offset)
 {
-    ARMCPU *cpu;
+    ARMCPU *cpu = s->cpu;
     uint32_t val;
     int irq;
 
     switch (offset) {
     case 4: /* Interrupt Control Type.  */
-        return (s->num_irq / 32) - 1;
+        return ((s->num_irq - 16) / 32) - 1;
     case 0x10: /* SysTick Control and Status.  */
         val = s->systick.control;
         s->systick.control &= ~SYSTICK_COUNTFLAG;
@@ -426,45 +426,39 @@ 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(current_cpu);
         return cpu->midr;
     case 0xd04: /* Interrupt Control State.  */
         /* VECTACTIVE */
-        cpu = ARM_CPU(current_cpu);
         val = cpu->env.v7m.exception;
-        if (val == 1023) {
-            val = 0;
-        } else if (val >= 32) {
-            val -= 16;
-        }
         /* VECTPENDING */
-        if (s->gic.current_pending[0] != 1023)
-            val |= (s->gic.current_pending[0] << 12);
+        val |= (cpu->env.v7m.pending << 12)&0x1ff;
         /* ISRPENDING and RETTOBASE */
-        for (irq = 32; irq < s->num_irq; irq++) {
-            if (s->gic.irq_state[irq].pending) {
+        for (irq = 16; irq < s->num_irq; irq++) {
+            if (s->vectors[irq].pending) {
                 val |= (1 << 22);
                 break;
             }
-            if (irq != cpu->env.v7m.exception && s->gic.irq_state[irq].active) {
+            if (irq != cpu->env.v7m.exception && s->vectors[irq].active) {
                 val |= (1 << 11);
             }
         }
         /* PENDSTSET */
-        if (s->gic.irq_state[ARMV7M_EXCP_SYSTICK].pending)
+        if (s->vectors[ARMV7M_EXCP_SYSTICK].pending) {
             val |= (1 << 26);
+        }
         /* PENDSVSET */
-        if (s->gic.irq_state[ARMV7M_EXCP_PENDSV].pending)
+        if (s->vectors[ARMV7M_EXCP_PENDSV].pending) {
             val |= (1 << 28);
+        }
         /* NMIPENDSET */
-        if (s->gic.irq_state[ARMV7M_EXCP_NMI].pending)
+        if (s->vectors[ARMV7M_EXCP_NMI].pending) {
             val |= (1 << 31);
+        }
         return val;
     case 0xd08: /* Vector Table Offset.  */
-        cpu = ARM_CPU(current_cpu);
         return cpu->env.v7m.vecbase;
     case 0xd0c: /* Application Interrupt/Reset Control.  */
-        return 0xfa050000;
+        return 0xfa050000 | (s->prigroup<<8);
     case 0xd10: /* System Control.  */
         /* TODO: Implement SLEEPONEXIT.  */
         return 0;
@@ -473,20 +467,20 @@ static uint32_t nvic_readl(nvic_state *s, uint32_t offset)
         return 0;
     case 0xd24: /* System Handler Status.  */
         val = 0;
-        if (s->gic.irq_state[ARMV7M_EXCP_MEM].active) val |= (1 << 0);
-        if (s->gic.irq_state[ARMV7M_EXCP_BUS].active) val |= (1 << 1);
-        if (s->gic.irq_state[ARMV7M_EXCP_USAGE].active) val |= (1 << 3);
-        if (s->gic.irq_state[ARMV7M_EXCP_SVC].active) val |= (1 << 7);
-        if (s->gic.irq_state[ARMV7M_EXCP_DEBUG].active) val |= (1 << 8);
-        if (s->gic.irq_state[ARMV7M_EXCP_PENDSV].active) val |= (1 << 10);
-        if (s->gic.irq_state[ARMV7M_EXCP_SYSTICK].active) val |= (1 << 11);
-        if (s->gic.irq_state[ARMV7M_EXCP_USAGE].pending) val |= (1 << 12);
-        if (s->gic.irq_state[ARMV7M_EXCP_MEM].pending) val |= (1 << 13);
-        if (s->gic.irq_state[ARMV7M_EXCP_BUS].pending) val |= (1 << 14);
-        if (s->gic.irq_state[ARMV7M_EXCP_SVC].pending) val |= (1 << 15);
-        if (s->gic.irq_state[ARMV7M_EXCP_MEM].enabled) val |= (1 << 16);
-        if (s->gic.irq_state[ARMV7M_EXCP_BUS].enabled) val |= (1 << 17);
-        if (s->gic.irq_state[ARMV7M_EXCP_USAGE].enabled) val |= (1 << 18);
+        if (s->vectors[ARMV7M_EXCP_MEM].active) val |= (1 << 0);
+        if (s->vectors[ARMV7M_EXCP_BUS].active) val |= (1 << 1);
+        if (s->vectors[ARMV7M_EXCP_USAGE].active) val |= (1 << 3);
+        if (s->vectors[ARMV7M_EXCP_SVC].active) val |= (1 << 7);
+        if (s->vectors[ARMV7M_EXCP_DEBUG].active) val |= (1 << 8);
+        if (s->vectors[ARMV7M_EXCP_PENDSV].active) val |= (1 << 10);
+        if (s->vectors[ARMV7M_EXCP_SYSTICK].active) val |= (1 << 11);
+        if (s->vectors[ARMV7M_EXCP_USAGE].pending) val |= (1 << 12);
+        if (s->vectors[ARMV7M_EXCP_MEM].pending) val |= (1 << 13);
+        if (s->vectors[ARMV7M_EXCP_BUS].pending) val |= (1 << 14);
+        if (s->vectors[ARMV7M_EXCP_SVC].pending) val |= (1 << 15);
+        if (s->vectors[ARMV7M_EXCP_MEM].enabled) val |= (1 << 16);
+        if (s->vectors[ARMV7M_EXCP_BUS].enabled) val |= (1 << 17);
+        if (s->vectors[ARMV7M_EXCP_USAGE].enabled) val |= (1 << 18);
         return val;
     case 0xd28: /* Configurable Fault Status.  */
         /* TODO: Implement Fault Status.  */
@@ -535,7 +529,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.  */
@@ -577,18 +571,15 @@ static void nvic_writel(nvic_state *s, uint32_t offset, uint32_t value)
         if (value & (1 << 28)) {
             armv7m_nvic_set_pending(s, ARMV7M_EXCP_PENDSV);
         } else if (value & (1 << 27)) {
-            s->gic.irq_state[ARMV7M_EXCP_PENDSV].pending = 0;
-            gic_update(&s->gic);
+            armv7m_nvic_clear_pending(s, ARMV7M_EXCP_PENDSV);
         }
         if (value & (1 << 26)) {
             armv7m_nvic_set_pending(s, ARMV7M_EXCP_SYSTICK);
         } else if (value & (1 << 25)) {
-            s->gic.irq_state[ARMV7M_EXCP_SYSTICK].pending = 0;
-            gic_update(&s->gic);
+            armv7m_nvic_clear_pending(s, ARMV7M_EXCP_SYSTICK);
         }
         break;
     case 0xd08: /* Vector Table Offset.  */
-        cpu = ARM_CPU(current_cpu);
         cpu->env.v7m.vecbase = value & 0xffffff80;
         break;
     case 0xd0c: /* Application Interrupt/Reset Control.  */
@@ -603,7 +594,13 @@ static void nvic_writel(nvic_state *s, uint32_t offset, uint32_t value)
                 qemu_log_mask(LOG_UNIMP, "AIRCR system reset unimplemented\n");
             }
             if (value & 0x700) {
-                qemu_log_mask(LOG_UNIMP, "PRIGROUP unimplemented\n");
+                unsigned i;
+                s->prigroup = (value>>8)&0xf;
+                /* recalculate prio_ord for exceptions w/ configurable prio */
+                for (i = 4; i < s->num_irq; i++) {
+                    set_prio(s, i, s->vectors[i].raw_prio);
+                }
+                nvic_irq_update(s, 0);
             }
         }
         break;
@@ -615,9 +612,12 @@ static void nvic_writel(nvic_state *s, uint32_t offset, uint32_t value)
     case 0xd24: /* System Handler Control.  */
         /* TODO: Real hardware allows you to set/clear the active bits
            under some circumstances.  We don't implement this.  */
-        s->gic.irq_state[ARMV7M_EXCP_MEM].enabled = (value & (1 << 16)) != 0;
-        s->gic.irq_state[ARMV7M_EXCP_BUS].enabled = (value & (1 << 17)) != 0;
-        s->gic.irq_state[ARMV7M_EXCP_USAGE].enabled = (value & (1 << 18)) != 0;
+        s->vectors[ARMV7M_EXCP_MEM].enabled = (value & (1 << 16)) != 0;
+        s->vectors[ARMV7M_EXCP_BUS].enabled = (value & (1 << 17)) != 0;
+        s->vectors[ARMV7M_EXCP_USAGE].enabled = (value & (1 << 18)) != 0;
+        /* no need to call nvic_irq_update() since any pending while
+         * disabled would have been escalated to HardFault
+         */
         break;
     case 0xd28: /* Configurable Fault Status.  */
     case 0xd2c: /* Hard Fault Status.  */
@@ -629,8 +629,8 @@ static void nvic_writel(nvic_state *s, uint32_t offset, uint32_t value)
                       "NVIC: fault status registers unimplemented\n");
         break;
     case 0xf00: /* Software Triggered Interrupt Register */
-        if ((value & 0x1ff) < s->num_irq) {
-            gic_set_pending_private(&s->gic, 0, value & 0x1ff);
+        if ((value & 0x1ff) < NVIC_MAX_IRQ) {
+            armv7m_nvic_set_pending(s, (value&0x1ff)+16);
         }
         break;
     default:
@@ -644,28 +644,81 @@ static uint64_t nvic_sysreg_read(void *opaque, hwaddr addr,
 {
     nvic_state *s = (nvic_state *)opaque;
     uint32_t offset = addr;
-    int i;
+    unsigned i, end;
     uint32_t val;
 
     switch (offset) {
+    /* reads of set and clear both return the status */
+    case 0x100 ... 0x13c: /* NVIC Set enable */
+        offset += 0x80;
+        /* fall through */
+    case 0x180 ... 0x1bc: /* NVIC Clear enable */
+        val = 0;
+        offset = offset-0x180+16; /* vector # */
+
+        for (i = 0, end = size*8; i < end && offset+i < s->num_irq; i++) {
+            if (s->vectors[offset+i].enabled) {
+                val |= (1<<i);
+            }
+        }
+        break;
+    case 0x200 ... 0x23c: /* NVIC Set pend */
+        offset += 0x80;
+        /* fall through */
+    case 0x280 ... 0x2bc: /* NVIC Clear pend */
+        val = 0;
+        offset = offset-0x280+16; /* vector # */
+
+        for (i = 0, end = size*8; i < end && offset+i < s->num_irq; i++) {
+            if (s->vectors[offset+i].pending) {
+                val |= (1<<i);
+            }
+        }
+        break;
+    case 0x300 ... 0x37c: /* NVIC Active */
+        val = 0;
+        offset = offset-0x300+16; /* vector # */
+
+        for (i = 0, end = size*8; i < end && offset+i < s->num_irq; i++) {
+            if (s->vectors[offset+i].active) {
+                val |= (1<<i);
+            }
+        }
+        break;
+    case 0x400 ... 0x7ec: /* NVIC Priority */
+        val = 0;
+        offset = offset-0x400+16; /* vector # */
+
+        for (i = 0; i < size && offset+i < s->num_irq; i++) {
+            val |= s->vectors[offset+i].raw_prio<<(8*i);
+        }
+        break;
     case 0xd18 ... 0xd23: /* System Handler Priority.  */
         val = 0;
         for (i = 0; i < size; i++) {
-            val |= s->gic.priority1[(offset - 0xd14) + i][0] << (i * 8);
+            val |= s->vectors[(offset - 0xd14) + i].raw_prio << (i * 8);
         }
-        return val;
+        break;
     case 0xfe0 ... 0xfff: /* ID.  */
         if (offset & 3) {
             return 0;
         }
-        return nvic_id[(offset - 0xfe0) >> 2];
-    }
-    if (size == 4) {
-        return nvic_readl(s, offset);
+        val = nvic_id[(offset - 0xfe0) >> 2];
+        break;
+    default:
+        if (size == 4) {
+            val = nvic_readl(s, offset);
+        } else {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "NVIC: Bad read of size %d at offset 0x%x\n",
+                          size, offset);
+            val = 0;
+        }
     }
-    qemu_log_mask(LOG_GUEST_ERROR,
-                  "NVIC: Bad read of size %d at offset 0x%x\n", size, offset);
-    return 0;
+
+    DPRINTF(0, "sysreg read%u "TARGET_FMT_plx" -> %08x\n",
+            size*8, addr, (unsigned)val);
+    return val;
 }
 
 static void nvic_sysreg_write(void *opaque, hwaddr addr,
@@ -673,23 +726,73 @@ static void nvic_sysreg_write(void *opaque, hwaddr addr,
 {
     nvic_state *s = (nvic_state *)opaque;
     uint32_t offset = addr;
-    int i;
+    unsigned i, end;
+    unsigned setval = 0;
+
+    DPRINTF(0, "sysreg write%u "TARGET_FMT_plx" <- %08x\n",
+            size*8, addr, (unsigned)value);
 
     switch (offset) {
-    case 0xd18 ... 0xd23: /* System Handler Priority.  */
+    case 0x100 ... 0x13c: /* NVIC Set enable */
+        offset += 0x80;
+        setval = 1;
+        /* fall through */
+    case 0x180 ... 0x1bc: /* NVIC Clear enable */
+        offset = offset-0x180+16; /* vector # */
+
+        for (i = 0, end = size*8; i < end && offset+i < s->num_irq; i++) {
+            if (value&(1<<i)) {
+                s->vectors[offset+i].enabled = setval;
+            }
+        }
+        nvic_irq_update(s, 0);
+        return;
+    case 0x200 ... 0x23c: /* NVIC Set pend */
+        /* the special logic in armv7m_nvic_set_pending()
+         * is not needed since IRQs are never escalated
+         */
+        offset += 0x80;
+        setval = 1;
+        /* fall through */
+    case 0x280 ... 0x2bc: /* NVIC Clear pend */
+        offset = offset-0x280+16; /* vector # */
+
+        for (i = 0, end = size*8; i < end && offset+i < s->num_irq; i++) {
+            if (value&(1<<i)) {
+                s->vectors[offset+i].pending = setval;
+            }
+        }
+        nvic_irq_update(s, 0);
+        return;
+    case 0x300 ... 0x37c: /* NVIC Active */
+        return; /* R/O */
+    case 0x400 ... 0x7ec: /* NVIC Priority */
+        offset = offset-0x400+16; /* vector # */
+
         for (i = 0; i < size; i++) {
-            s->gic.priority1[(offset - 0xd14) + i][0] =
-                (value >> (i * 8)) & 0xff;
+            set_prio(s, offset+i, (value>>(i*8))&0xff);
         }
-        gic_update(&s->gic);
+        nvic_irq_update(s, 0);
         return;
-    }
-    if (size == 4) {
-        nvic_writel(s, offset, value);
+    case 0xd18 ... 0xd23: /* System Handler Priority.  */
+        for (i = 0; i < size; i++) {
+            unsigned hdlidx = (offset - 0xd14) + i;
+            set_prio(s, hdlidx, (value >> (i * 8)) & 0xff);
+            DPRINTF(0, "Set Handler prio %u = %u\n",
+                    (unsigned)hdlidx,
+                    (unsigned)s->vectors[hdlidx].raw_prio);
+        }
+        nvic_irq_update(s, 0);
         return;
+    default:
+        if (size == 4) {
+            nvic_writel(s, offset, value);
+            return;
+        }
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "NVIC: Bad write of size %d at offset 0x%x\n",
+                      size, offset);
     }
-    qemu_log_mask(LOG_GUEST_ERROR,
-                  "NVIC: Bad write of size %d at offset 0x%x\n", size, offset);
 }
 
 static const MemoryRegionOps nvic_sysreg_ops = {
-- 
2.1.4

[Qemu-devel] [PATCH 08/18] armv7m: fix RETTOBASE

[Michael Davidsaver]

The polarity is reversed, and it should include
internal exceptions.

Should be set when # of active exceptions <= 1.

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
---
 hw/intc/armv7m_nvic.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 30e349e..3b10dee 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -432,16 +432,20 @@ static uint32_t nvic_readl(nvic_state *s, uint32_t offset)
         val = cpu->env.v7m.exception;
         /* VECTPENDING */
         val |= (cpu->env.v7m.pending << 12)&0x1ff;
-        /* ISRPENDING and RETTOBASE */
+        /* ISRPENDING - Set it any externel IRQ pending (vector>=16) */
         for (irq = 16; irq < s->num_irq; irq++) {
             if (s->vectors[irq].pending) {
                 val |= (1 << 22);
                 break;
             }
+        }
+        /* RETTOBASE - Set if no (other) handler is active */
+        for (irq = 1; irq < s->num_irq; irq++) {
             if (irq != cpu->env.v7m.exception && s->vectors[irq].active) {
-                val |= (1 << 11);
+                val |= (1 << 11); /* some other handler is active */
             }
         }
+        val ^= (1<<11); /* invert */
         /* PENDSTSET */
         if (s->vectors[ARMV7M_EXCP_SYSTICK].pending) {
             val |= (1 << 26);
@@ -454,6 +458,7 @@ static uint32_t nvic_readl(nvic_state *s, uint32_t offset)
         if (s->vectors[ARMV7M_EXCP_NMI].pending) {
             val |= (1 << 31);
         }
+        /* ISRPREEMPT not implemented */
         return val;
     case 0xd08: /* Vector Table Offset.  */
         return cpu->env.v7m.vecbase;
@@ -588,10 +593,14 @@ static void nvic_writel(nvic_state *s, uint32_t offset, uint32_t value)
                 qemu_irq_pulse(s->sysresetreq);
             }
             if (value & 2) {
-                qemu_log_mask(LOG_UNIMP, "VECTCLRACTIVE unimplemented\n");
+                qemu_log_mask(LOG_GUEST_ERROR,
+                              "Setting VECTCLRACTIVE when not in DEBUG mode "
+                              "is UNPREDICTABLE\n");
             }
             if (value & 1) {
-                qemu_log_mask(LOG_UNIMP, "AIRCR system reset unimplemented\n");
+                qemu_log_mask(LOG_GUEST_ERROR,
+                              "Setting VECTRESET when not in DEBUG mode "
+                              "is UNPREDICTABLE\n");
             }
             if (value & 0x700) {
                 unsigned i;
-- 
2.1.4

[Qemu-devel] [PATCH 09/18] armv7m: NVIC update vmstate

[Michael Davidsaver]

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
---
 hw/intc/armv7m_nvic.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 62 insertions(+), 2 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 3b10dee..c860b36 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -810,15 +810,75 @@ static const MemoryRegionOps nvic_sysreg_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
+static
+int nvic_post_load(void *opaque, int version_id)
+{
+    nvic_state *s = opaque;
+    unsigned i;
+
+    /* evil hack to get ARMCPU* ahead of time */
+    assert(cpus.tqh_first);
+    assert(!CPU_NEXT(cpus.tqh_first));
+    s->cpu = ARM_CPU(cpus.tqh_first);
+    assert(s->cpu);
+
+    /* recalculate priorities */
+    for (i = 4; i < s->num_irq; i++) {
+        set_prio(s, i, s->vectors[i].raw_prio);
+    }
+
+    nvic_irq_update(s, highest_runnable_prio(s->cpu));
+
+    return 0;
+}
+
+static
+int vec_info_get(QEMUFile *f, void *pv, size_t size)
+{
+    vec_info *I = pv;
+    I->prio_sub = qemu_get_be16(f);
+    I->prio_group = qemu_get_byte(f);
+    I->raw_prio = qemu_get_ubyte(f);
+    I->enabled = qemu_get_ubyte(f);
+    I->pending = qemu_get_ubyte(f);
+    I->active = qemu_get_ubyte(f);
+    I->level = qemu_get_ubyte(f);
+    return 0;
+}
+
+static
+void vec_info_put(QEMUFile *f, void *pv, size_t size)
+{
+    vec_info *I = pv;
+    qemu_put_be16(f, I->prio_sub);
+    qemu_put_byte(f, I->prio_group);
+    qemu_put_ubyte(f, I->raw_prio);
+    qemu_put_ubyte(f, I->enabled);
+    qemu_put_ubyte(f, I->pending);
+    qemu_put_ubyte(f, I->active);
+    qemu_put_ubyte(f, I->level);
+}
+
+static const VMStateInfo vmstate_info = {
+    .name = "armv7m_nvic_info",
+    .get = vec_info_get,
+    .put = vec_info_put,
+};
+
 static const VMStateDescription vmstate_nvic = {
     .name = "armv7m_nvic",
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
+    .post_load = &nvic_post_load,
     .fields = (VMStateField[]) {
+        VMSTATE_ARRAY(vectors, nvic_state, NVIC_MAX_VECTORS, 0,
+                      vmstate_info, vec_info),
+        VMSTATE_UINT8(prigroup, nvic_state),
         VMSTATE_UINT32(systick.control, nvic_state),
         VMSTATE_UINT32(systick.reload, nvic_state),
         VMSTATE_INT64(systick.tick, nvic_state),
         VMSTATE_TIMER_PTR(systick.timer, nvic_state),
+        VMSTATE_UINT32(num_irq, nvic_state),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
2.1.4

[Qemu-devel] [PATCH 10/18] armv7m: NVIC initialization

[Michael Davidsaver]

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
---
 hw/intc/armv7m_nvic.c | 107 ++++++++++++++++++++++++--------------------------
 1 file changed, 51 insertions(+), 56 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index c860b36..8eaf677 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -827,7 +827,7 @@ int nvic_post_load(void *opaque, int version_id)
         set_prio(s, i, s->vectors[i].raw_prio);
     }
 
-    nvic_irq_update(s, highest_runnable_prio(s->cpu));
+    nvic_irq_update(s, 0);
 
     return 0;
 }
@@ -883,66 +883,66 @@ static const VMStateDescription vmstate_nvic = {
     }
 };
 
+static Property props_nvic[] = {
+    DEFINE_PROP_UINT32("num-irq", nvic_state, num_irq, 64),
+    DEFINE_PROP_END_OF_LIST()
+};
+
 static void armv7m_nvic_reset(DeviceState *dev)
 {
     nvic_state *s = NVIC(dev);
-    NVICClass *nc = NVIC_GET_CLASS(s);
-    nc->parent_reset(dev);
-    /* Common GIC reset resets to disabled; the NVIC doesn't have
-     * per-CPU interfaces so mark our non-existent CPU interface
-     * as enabled by default, and with a priority mask which allows
-     * all interrupts through.
-     */
-    s->gic.cpu_ctlr[0] = GICC_CTLR_EN_GRP0;
-    s->gic.priority_mask[0] = 0x100;
-    /* The NVIC as a whole is always enabled. */
-    s->gic.ctlr = 1;
+
+    s->vectors[ARMV7M_EXCP_RESET].enabled = 1;
+    s->vectors[ARMV7M_EXCP_NMI].enabled = 1;
+    s->vectors[ARMV7M_EXCP_HARD].enabled = 1;
+    s->vectors[ARMV7M_EXCP_SVC].enabled = 1;
+    s->vectors[ARMV7M_EXCP_DEBUG].enabled = 1;
+    s->vectors[ARMV7M_EXCP_PENDSV].enabled = 1;
+
+    s->vectors[ARMV7M_EXCP_RESET].prio_group = -3;
+    s->vectors[ARMV7M_EXCP_NMI].prio_group = -2;
+    s->vectors[ARMV7M_EXCP_HARD].prio_group = -1;
+
     systick_reset(s);
 }
 
 static void armv7m_nvic_realize(DeviceState *dev, Error **errp)
 {
     nvic_state *s = NVIC(dev);
-    NVICClass *nc = NVIC_GET_CLASS(s);
-    Error *local_err = NULL;
-
-    /* The NVIC always has only one CPU */
-    s->gic.num_cpu = 1;
-    /* Tell the common code we're an NVIC */
-    s->gic.revision = 0xffffffff;
-    s->num_irq = s->gic.num_irq;
-    nc->parent_realize(dev, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+
+    /* evil hack to get ARMCPU* ahead of time */
+    assert(cpus.tqh_first);
+    assert(!CPU_NEXT(cpus.tqh_first));
+    s->cpu = ARM_CPU(cpus.tqh_first);
+    assert(s->cpu);
+
+    if (s->num_irq > NVIC_MAX_IRQ) {
+        error_setg(errp, TYPE_NVIC " num_irq too large");
         return;
     }
-    gic_init_irqs_and_distributor(&s->gic);
-    /* The NVIC and system controller register area looks like this:
-     *  0..0xff : system control registers, including systick
-     *  0x100..0xcff : GIC-like registers
-     *  0xd00..0xfff : system control registers
-     * We use overlaying to put the GIC like registers
-     * over the top of the system control register region.
-     */
-    memory_region_init(&s->container, OBJECT(s), "nvic", 0x1000);
-    /* The system register region goes at the bottom of the priority
-     * stack as it covers the whole page.
+
+    qdev_init_gpio_in(dev, set_irq_level, s->num_irq);
+
+    s->num_irq += 16; /* include space for internal exception vectors */
+
+    /* The NVIC and system controller register area starts at 0xe000e000
+     * and looks like this:
+     *  0x004 - ICTR
+     *  0x010 - 0x1c - systick
+     *  0x100..0x7ec - NVIC
+     *  0x7f0..0xcff - Reserved
+     *  0xd00..0xd3c - SCS registers
+     *  0xd40..0xeff - Reserved or Not implemented
+     *  0xf00 - STIR
      */
-    memory_region_init_io(&s->sysregmem, OBJECT(s), &nvic_sysreg_ops, s,
+
+    memory_region_init_io(&s->iomem, OBJECT(s), &nvic_sysreg_ops, s,
                           "nvic_sysregs", 0x1000);
-    memory_region_add_subregion(&s->container, 0, &s->sysregmem);
-    /* Alias the GIC region so we can get only the section of it
-     * we need, and layer it on top of the system register region.
-     */
-    memory_region_init_alias(&s->gic_iomem_alias, OBJECT(s),
-                             "nvic-gic", &s->gic.iomem,
-                             0x100, 0xc00);
-    memory_region_add_subregion_overlap(&s->container, 0x100,
-                                        &s->gic_iomem_alias, 1);
+
     /* Map the whole thing into system memory at the location required
      * by the v7M architecture.
      */
-    memory_region_add_subregion(get_system_memory(), 0xe000e000, &s->container);
+    memory_region_add_subregion(get_system_memory(), 0xe000e000, &s->iomem);
     s->systick.timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, systick_timer_tick, s);
 }
 
@@ -954,36 +954,31 @@ static void armv7m_nvic_instance_init(Object *obj)
      * any user-specified property setting, so just modify the
      * value in the GICState struct.
      */
-    GICState *s = ARM_GIC_COMMON(obj);
     DeviceState *dev = DEVICE(obj);
     nvic_state *nvic = NVIC(obj);
-    /* The ARM v7m may have anything from 0 to 496 external interrupt
-     * IRQ lines. We default to 64. Other boards may differ and should
-     * set the num-irq property appropriately.
-     */
-    s->num_irq = 64;
+    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+
+    sysbus_init_irq(sbd, &nvic->excpout);
     qdev_init_gpio_out_named(dev, &nvic->sysresetreq, "SYSRESETREQ", 1);
 }
 
 static void armv7m_nvic_class_init(ObjectClass *klass, void *data)
 {
-    NVICClass *nc = NVIC_CLASS(klass);
     DeviceClass *dc = DEVICE_CLASS(klass);
 
-    nc->parent_reset = dc->reset;
-    nc->parent_realize = dc->realize;
     dc->vmsd  = &vmstate_nvic;
+    dc->props = props_nvic;
     dc->reset = armv7m_nvic_reset;
     dc->realize = armv7m_nvic_realize;
 }
 
 static const TypeInfo armv7m_nvic_info = {
     .name          = TYPE_NVIC,
-    .parent        = TYPE_ARM_GIC_COMMON,
+    .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_init = armv7m_nvic_instance_init,
     .instance_size = sizeof(nvic_state),
     .class_init    = armv7m_nvic_class_init,
-    .class_size    = sizeof(NVICClass),
+    .class_size    = sizeof(SysBusDeviceClass),
 };
 
 static void armv7m_nvic_register_types(void)
-- 
2.1.4

[Qemu-devel] [PATCH 11/18] armv7m: fix I and F flag handling

[Michael Davidsaver]

Despite having the same notation, these bits
have completely different meaning than -AR.

Add armv7m_excp_unmasked()
to calculate the currently runable exception priority
taking into account masks and active handlers.
Use this in conjunction with the pending exception
priority to determine if the pending exception
can interrupt execution.

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
---
 target-arm/cpu.c | 26 +++++++-------------------
 target-arm/cpu.h | 27 ++++++++++++++++++++++++++-
 2 files changed, 33 insertions(+), 20 deletions(-)

diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index be026bc..5d03117 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -173,6 +173,8 @@ static void arm_cpu_reset(CPUState *s)
         uint32_t initial_pc; /* Loaded from 0x4 */
         uint8_t *rom;
 
+        env->v7m.exception_prio = env->v7m.pending_prio = 0x100;
+
         env->daif &= ~PSTATE_I;
         rom = rom_ptr(0);
         if (rom) {
@@ -301,29 +303,15 @@ static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 {
     CPUClass *cc = CPU_GET_CLASS(cs);
     ARMCPU *cpu = ARM_CPU(cs);
-    CPUARMState *env = &cpu->env;
     bool ret = false;
 
-
-    if (interrupt_request & CPU_INTERRUPT_FIQ
-        && !(env->daif & PSTATE_F)) {
-        cs->exception_index = EXCP_FIQ;
-        cc->do_interrupt(cs);
-        ret = true;
-    }
-    /* 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 when the
-     * CPU tries to execute code at the magic address.
-     * This will cause the magic PC value to be pushed to
-     * the stack if an interrupt occurred at the wrong time.
-     * We avoid this by disabling interrupts when
-     * pc contains a magic address.
+    /* ARMv7-M interrupt masking works differently than -A or -R.
+     * There is no FIQ/IRQ distinction.
+     * Instead of masking interrupt sources, the I and F bits
+     * (along with basepri) mask certain exception priority levels.
      */
     if (interrupt_request & CPU_INTERRUPT_HARD
-        && !(env->daif & PSTATE_I)
-        && (env->regs[15] < 0xfffffff0)) {
+            && (armv7m_excp_unmasked(cpu) >= cpu->env.v7m.pending_prio)) {
         cs->exception_index = EXCP_IRQ;
         cc->do_interrupt(cs);
         ret = true;
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index c193fbb..29d89ce 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -1033,9 +1033,34 @@ void arm_cpu_list(FILE *f, fprintf_function cpu_fprintf);
 uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx,
                                  uint32_t cur_el, bool secure);
 
+/* @returns highest (numerically lowest) unmasked exception priority
+ */
+static inline
+int armv7m_excp_unmasked(ARMCPU *cpu)
+{
+    CPUARMState *env = &cpu->env;
+    int runnable;
+
+    /* find highest (numerically lowest) priority which could
+     * run based on masks
+     */
+    if (env->daif&PSTATE_F) { /* FAULTMASK */
+        runnable = -2;
+    } else if (env->daif&PSTATE_I) { /* PRIMASK */
+        runnable = -1;
+    } else if (env->v7m.basepri > 0) {
+        /* BASEPRI==1 -> runnable==-1 (same as PRIMASK==1) */
+        runnable = env->v7m.basepri-2;
+    } else {
+        runnable = 0x100; /* lower than any possible priority */
+    }
+    /* consider priority of active handler */
+    return MIN(runnable, env->v7m.exception_prio-1);
+}
+
 /* Interface between CPU and Interrupt controller.  */
 void armv7m_nvic_set_pending(void *opaque, int irq);
-int armv7m_nvic_acknowledge_irq(void *opaque);
+void armv7m_nvic_acknowledge_irq(void *opaque);
 void armv7m_nvic_complete_irq(void *opaque, int irq);
 
 /* Interface for defining coprocessor registers.
-- 
2.1.4

[Qemu-devel] [PATCH 12/18] armv7m: simpler/faster exception start

[Michael Davidsaver]

No need to bounce through EXCP_IRQ handling
for non-IRQ exceptions.  just update CPU
state directly.

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
---
 target-arm/helper.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 1d7ac43..2541890 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -5433,23 +5433,21 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
 
     /* For exceptions we just mark as pending on the NVIC, and let that
        handle it.  */
-    /* TODO: Need to escalate if the current priority is higher than the
-       one we're raising.  */
     switch (cs->exception_index) {
     case EXCP_UDEF:
         armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE);
-        return;
+        break;
     case EXCP_SWI:
         /* The PC already points to the next instruction.  */
         armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_SVC);
-        return;
+        break;
     case EXCP_PREFETCH_ABORT:
     case EXCP_DATA_ABORT:
         /* TODO: if we implemented the MPU registers, this is where we
          * should set the MMFAR, etc from exception.fsr and exception.vaddress.
          */
         armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_MEM);
-        return;
+        break;
     case EXCP_BKPT:
         if (semihosting_enabled()) {
             int nr;
@@ -5466,7 +5464,6 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
         armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_DEBUG);
         return;
     case EXCP_IRQ:
-        env->v7m.exception = armv7m_nvic_acknowledge_irq(env->nvic);
         break;
     case EXCP_EXCEPTION_EXIT:
         do_v7m_exception_exit(env);
@@ -5476,6 +5473,10 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
         return; /* Never happens.  Keep compiler happy.  */
     }
 
+    armv7m_nvic_acknowledge_irq(env->nvic);
+
+    qemu_log_mask(CPU_LOG_INT, "... as %d\n", env->v7m.exception);
+
     /* Align stack pointer.  */
     /* ??? Should only do this if Configuration Control Register
        STACKALIGN bit is set.  */
-- 
2.1.4

[Qemu-devel] [PATCH 13/18] armv7m: implement CFSR and HFSR

[Michael Davidsaver]

Add the Configurable and Hard Fault Status registers.
Note undefined instructions and escalations

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
---
 hw/intc/armv7m_nvic.c | 10 +++++++---
 target-arm/cpu.h      |  2 ++
 target-arm/helper.c   |  1 +
 target-arm/machine.c  |  6 ++++--
 4 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 8eaf677..734f6f8 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -287,6 +287,7 @@ void armv7m_nvic_set_pending(void *opaque, int irq)
             }
             I = &s->vectors[irq];
 
+            s->cpu->env.v7m.hfsr |= 1<<30; /* FORCED */
             DPRINTF(0, "Escalate %d to %d\n", oldirq, irq);
         }
     }
@@ -488,10 +489,9 @@ static uint32_t nvic_readl(nvic_state *s, uint32_t offset)
         if (s->vectors[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.  */
     case 0xd38: /* Bus Fault Address.  */
@@ -629,7 +629,11 @@ static void nvic_writel(nvic_state *s, uint32_t offset, uint32_t value)
          */
         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.  */
     case 0xd34: /* Mem Manage Address.  */
     case 0xd38: /* Bus Fault Address.  */
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 29d89ce..e98bca0 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -396,6 +396,8 @@ typedef struct CPUARMState {
         uint32_t vecbase;
         uint32_t basepri;
         uint32_t control;
+        uint32_t cfsr; /* Configurable Fault Status */
+        uint32_t hfsr; /* HardFault Status */
         int current_sp;
         int exception;
         int exception_prio;
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 2541890..5be09b8 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -5436,6 +5436,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 |= 1<<16; /* UNDEFINSTR */
         break;
     case EXCP_SWI:
         /* The PC already points to the next instruction.  */
diff --git a/target-arm/machine.c b/target-arm/machine.c
index 36a0d15..d7c2034 100644
--- a/target-arm/machine.c
+++ b/target-arm/machine.c
@@ -92,14 +92,16 @@ 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_UINT32(env.v7m.cfsr, ARMCPU),
+        VMSTATE_UINT32(env.v7m.hfsr, ARMCPU),
         VMSTATE_INT32(env.v7m.current_sp, ARMCPU),
         VMSTATE_INT32(env.v7m.exception, ARMCPU),
         VMSTATE_END_OF_LIST()
-- 
2.1.4

[Qemu-devel] [PATCH 14/18] armv7m: auto-clear FAULTMASK

[Michael Davidsaver]

on return from all exceptions other than NMI

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
---
 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 5be09b8..83af528 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -5379,8 +5379,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.1.4

[Qemu-devel] [PATCH 15/18] arm: gic: Remove references to NVIC

[Michael Davidsaver]

armv7m_nvic.c no longer relies on the GIC.
Remove REV_NVIC and conditionals which use it.

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
---
 hw/intc/arm_gic.c        | 14 +++++++-------
 hw/intc/arm_gic_common.c | 23 ++++++++---------------
 hw/intc/gic_internal.h   |  7 ++-----
 3 files changed, 17 insertions(+), 27 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 8bad132..d543a93 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -184,7 +184,7 @@ static void gic_set_irq(void *opaque, int irq, int level)
         return;
     }
 
-    if (s->revision == REV_11MPCORE || s->revision == REV_NVIC) {
+    if (s->revision == REV_11MPCORE) {
         gic_set_irq_11mpcore(s, irq, level, cm, target);
     } else {
         gic_set_irq_generic(s, irq, level, cm, target);
@@ -335,7 +335,7 @@ uint32_t gic_acknowledge_irq(GICState *s, int cpu, MemTxAttrs attrs)
         return 1023;
     }
 
-    if (s->revision == REV_11MPCORE || s->revision == REV_NVIC) {
+    if (s->revision == REV_11MPCORE) {
         /* Clear pending flags for both level and edge triggered interrupts.
          * Level triggered IRQs will be reasserted once they become inactive.
          */
@@ -514,7 +514,7 @@ void gic_complete_irq(GICState *s, int cpu, int irq, MemTxAttrs attrs)
         return; /* No active IRQ.  */
     }
 
-    if (s->revision == REV_11MPCORE || s->revision == REV_NVIC) {
+    if (s->revision == REV_11MPCORE) {
         /* Mark level triggered interrupts as pending if they are still
            raised.  */
         if (!GIC_TEST_EDGE_TRIGGER(irq) && GIC_TEST_ENABLED(irq, cm)
@@ -672,7 +672,7 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs)
     } else if (offset < 0xf10) {
         goto bad_reg;
     } else if (offset < 0xf30) {
-        if (s->revision == REV_11MPCORE || s->revision == REV_NVIC) {
+        if (s->revision == REV_11MPCORE) {
             goto bad_reg;
         }
 
@@ -883,7 +883,7 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
         if (irq < GIC_NR_SGIS)
             value |= 0xaa;
         for (i = 0; i < 4; i++) {
-            if (s->revision == REV_11MPCORE || s->revision == REV_NVIC) {
+            if (s->revision == REV_11MPCORE) {
                 if (value & (1 << (i * 2))) {
                     GIC_SET_MODEL(irq + i);
                 } else {
@@ -901,7 +901,7 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
         goto bad_reg;
     } else if (offset < 0xf20) {
         /* GICD_CPENDSGIRn */
-        if (s->revision == REV_11MPCORE || s->revision == REV_NVIC) {
+        if (s->revision == REV_11MPCORE) {
             goto bad_reg;
         }
         irq = (offset - 0xf10);
@@ -912,7 +912,7 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
         }
     } else if (offset < 0xf30) {
         /* GICD_SPENDSGIRn */
-        if (s->revision == REV_11MPCORE || s->revision == REV_NVIC) {
+        if (s->revision == REV_11MPCORE) {
             goto bad_reg;
         }
         irq = (offset - 0xf20);
diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index 9c82b97..4987047 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -97,9 +97,7 @@ void gic_init_irqs_and_mmio(GICState *s, qemu_irq_handler handler,
      *  [N+32..N+63] PPIs for CPU 1
      *   ...
      */
-    if (s->revision != REV_NVIC) {
-        i += (GIC_INTERNAL * s->num_cpu);
-    }
+    i += (GIC_INTERNAL * s->num_cpu);
     qdev_init_gpio_in(DEVICE(s), handler, i);
 
     for (i = 0; i < s->num_cpu; i++) {
@@ -113,16 +111,12 @@ void gic_init_irqs_and_mmio(GICState *s, qemu_irq_handler handler,
     memory_region_init_io(&s->iomem, OBJECT(s), ops, s, "gic_dist", 0x1000);
     sysbus_init_mmio(sbd, &s->iomem);
 
-    if (s->revision != REV_NVIC) {
-        /* This is the main CPU interface "for this core". It is always
-         * present because it is required by both software emulation and KVM.
-         * NVIC is not handled here because its CPU interface is different,
-         * neither it can use KVM.
-         */
-        memory_region_init_io(&s->cpuiomem[0], OBJECT(s), ops ? &ops[1] : NULL,
-                              s, "gic_cpu", s->revision == 2 ? 0x1000 : 0x100);
-        sysbus_init_mmio(sbd, &s->cpuiomem[0]);
-    }
+    /* This is the main CPU interface "for this core". It is always
+     * present because it is required by both software emulation and KVM.
+     */
+    memory_region_init_io(&s->cpuiomem[0], OBJECT(s), ops ? &ops[1] : NULL,
+                          s, "gic_cpu", s->revision == 2 ? 0x1000 : 0x100);
+    sysbus_init_mmio(sbd, &s->cpuiomem[0]);
 }
 
 static void arm_gic_common_realize(DeviceState *dev, Error **errp)
@@ -154,7 +148,7 @@ static void arm_gic_common_realize(DeviceState *dev, Error **errp)
     }
 
     if (s->security_extn &&
-        (s->revision == REV_11MPCORE || s->revision == REV_NVIC)) {
+        (s->revision == REV_11MPCORE)) {
         error_setg(errp, "this GIC revision does not implement "
                    "the security extensions");
         return;
@@ -247,7 +241,6 @@ static Property arm_gic_common_properties[] = {
     DEFINE_PROP_UINT32("num-irq", GICState, num_irq, 32),
     /* Revision can be 1 or 2 for GIC architecture specification
      * versions 1 or 2, or 0 to indicate the legacy 11MPCore GIC.
-     * (Internally, 0xffffffff also indicates "not a GIC but an NVIC".)
      */
     DEFINE_PROP_UINT32("revision", GICState, revision, 1),
     /* True if the GIC should implement the security extensions */
diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
index 20c1e8a..a1f9320 100644
--- a/hw/intc/gic_internal.h
+++ b/hw/intc/gic_internal.h
@@ -25,9 +25,7 @@
 
 #define ALL_CPU_MASK ((unsigned)(((1 << GIC_NCPU) - 1)))
 
-/* The NVIC has 16 internal vectors.  However these are not exposed
-   through the normal GIC interface.  */
-#define GIC_BASE_IRQ ((s->revision == REV_NVIC) ? 32 : 0)
+#define GIC_BASE_IRQ 0
 
 #define GIC_SET_ENABLED(irq, cm) s->irq_state[irq].enabled |= (cm)
 #define GIC_CLEAR_ENABLED(irq, cm) s->irq_state[irq].enabled &= ~(cm)
@@ -75,7 +73,6 @@
 
 /* The special cases for the revision property: */
 #define REV_11MPCORE 0
-#define REV_NVIC 0xffffffff
 
 void gic_set_pending_private(GICState *s, int cpu, int irq);
 uint32_t gic_acknowledge_irq(GICState *s, int cpu, MemTxAttrs attrs);
@@ -87,7 +84,7 @@ void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val,
 
 static inline bool gic_test_pending(GICState *s, int irq, int cm)
 {
-    if (s->revision == REV_NVIC || s->revision == REV_11MPCORE) {
+    if (s->revision == REV_11MPCORE) {
         return s->irq_state[irq].pending & cm;
     } else {
         /* Edge-triggered interrupts are marked pending on a rising edge, but
-- 
2.1.4

[Qemu-devel] [PATCH 16/18] armv7m: check exception return consistency

[Michael Davidsaver]

Detect use of reserved exception return codes
and return to thread mode from nested
exception handler.

Also check consistency between NVIC and CPU
wrt. the active exception.

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
---
 hw/intc/armv7m_nvic.c |  7 +++-
 target-arm/cpu.h      |  2 +-
 target-arm/helper.c   | 97 ++++++++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 96 insertions(+), 10 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 734f6f8..a75dd3c 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -342,7 +342,7 @@ void armv7m_nvic_acknowledge_irq(void *opaque)
     assert(env->v7m.exception > 0); /* spurious exception? */
 }
 
-void armv7m_nvic_complete_irq(void *opaque, int irq)
+bool armv7m_nvic_complete_irq(void *opaque, int irq)
 {
     nvic_state *s = (nvic_state *)opaque;
     vec_info *I;
@@ -352,12 +352,17 @@ void armv7m_nvic_complete_irq(void *opaque, int irq)
 
     I = &s->vectors[irq];
 
+    if (!I->active) {
+        return true;
+    }
+
     I->active = 0;
     I->pending = I->level;
     assert(!I->level || irq >= 16);
 
     nvic_irq_update(s, 1);
     DPRINTF(0, "EOI %d\n", irq);
+    return false;
 }
 
 /* Only called for external interrupt (vector>=16) */
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index e98bca0..72b0b32 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -1063,7 +1063,7 @@ int armv7m_excp_unmasked(ARMCPU *cpu)
 /* Interface between CPU and Interrupt controller.  */
 void armv7m_nvic_set_pending(void *opaque, int irq);
 void armv7m_nvic_acknowledge_irq(void *opaque);
-void armv7m_nvic_complete_irq(void *opaque, int irq);
+bool armv7m_nvic_complete_irq(void *opaque, int irq);
 
 /* Interface for defining coprocessor registers.
  * Registers are defined in tables of arm_cp_reginfo structs
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 83af528..3993f77 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -5375,18 +5375,65 @@ static void switch_v7m_sp(CPUARMState *env, int process)
 
 static void do_v7m_exception_exit(CPUARMState *env)
 {
+    unsigned ufault = 0;
     uint32_t type;
     uint32_t xpsr;
+    uint32_t nvic_active;
+
+    if (env->v7m.exception == 0) {
+        hw_error("Return from exception w/o active exception.  Logic error.");
+    }
 
-    type = env->regs[15];
     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);
+
+    if (armv7m_nvic_complete_irq(env->nvic, env->v7m.exception)) {
+        qemu_log_mask(LOG_GUEST_ERROR, "Requesting return from exception "
+                      "from inactive exception %d\n",
+                      env->v7m.exception);
+        ufault = 1;
+    }
+    /* env->v7m.exception and friends updated by armv7m_nvic_complete_irq() */
+
+    type = env->regs[15]&0xf;
+    /* QEMU seems to clear the LSB at some point. */
+    type |= 1;
+
+    switch (type) {
+    case 0x1: /* Return to Handler mode */
+        if (env->v7m.exception == 0) {
+            qemu_log_mask(LOG_GUEST_ERROR, "Requesting return from exception "
+                          "to Handler mode not allowed at base level of "
+                          "activation");
+            ufault = 1;
+        }
+        break;
+    case 0x9: /* Return to Thread mode w/ Main stack */
+    case 0xd: /* Return to Thread mode w/ Process stack */
+        if (env->v7m.exception != 0) {
+            /* Attempt to return to Thread mode
+             * from nested handler while NONBASETHRDENA not set.
+             */
+            qemu_log_mask(LOG_GUEST_ERROR, "Nested exception return to %d w/"
+                          " Thread mode while NONBASETHRDENA not set\n",
+                          env->v7m.exception);
+            ufault = 1;
+        }
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "Exception return w/ reserved code"
+                                       " %02x\n", (unsigned)type);
+        ufault = 1;
     }
 
+    /* TODO? if ufault==1 ARM calls for entering exception handler
+     * directly w/o popping stack.
+     * We pop anyway since the active UsageFault will push on entry
+     * which should happen before execution resumes?
+     */
+
     /* Switch to the target stack.  */
     switch_v7m_sp(env, (type & 4) != 0);
     /* Pop registers.  */
@@ -5409,15 +5456,49 @@ static void do_v7m_exception_exit(CPUARMState *env)
         env->regs[15] &= ~1U;
     }
     xpsr = v7m_pop(env);
+    nvic_active = env->v7m.exception;
     xpsr_write(env, xpsr, 0xfffffdff);
+
     /* Undo stack alignment.  */
     if (xpsr & 0x200)
         env->regs[13] |= 4;
-    /* ??? The exception return type specifies Thread/Handler mode.  However
-       this is also implied by the xPSR value. Not sure what to do
-       if there is a mismatch.  */
-    /* ??? Likewise for mismatches between the CONTROL register and the stack
-       pointer.  */
+
+    /* TODO? if v7m.exception>0 && nvic_active != v7m.exception
+     * Adjust v7m.exception_prio for the exception being returned to?
+     * This could be different now that v7m.exception is restored
+     * from the guest stack.
+     * This can happen if the guest adjusts exception priorities
+     * while in a handler.
+     * At present retain the priority of the highest active
+     * exception.
+     */
+
+    if (!ufault) {
+        if (nvic_active != 0 && env->v7m.exception == 0) {
+            ufault = 1;
+        } else if (nvic_active == 0 && env->v7m.exception != 0) {
+            ufault = 1;
+        }
+        if (ufault) {
+            qemu_log_mask(LOG_GUEST_ERROR, "Return from exception with "
+                          "inconsistency between Exception return code "
+                          "and stacked xPSR\n");
+        }
+        /* ARM calls for PushStack() here, which should happen
+         * went we return with a pending exception
+         */
+    }
+
+    if (ufault) {
+        armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE);
+        env->v7m.cfsr |= 1<<18; /* INVPC */
+    }
+
+    /* Ensure that priority is consistent.  Clear for Thread mode
+     * and set for Handler mode
+     */
+    assert((env->v7m.exception == 0 && env->v7m.exception_prio > 0xff)
+           || (env->v7m.exception != 0 && env->v7m.exception_prio <= 0xff));
 }
 
 void arm_v7m_cpu_do_interrupt(CPUState *cs)
-- 
2.1.4

[Qemu-devel] [PATCH 17/18] armv7m: implement CCR

[Michael Davidsaver]

Implement Configuration and Control register.
Handle STACKALIGN and USERSETMPEND bits.

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
---
 hw/intc/armv7m_nvic.c | 15 +++++++++++----
 target-arm/cpu.h      |  1 +
 target-arm/helper.c   |  8 +++-----
 target-arm/machine.c  |  1 +
 4 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index a75dd3c..ca8c93c 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -474,8 +474,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->vectors[ARMV7M_EXCP_MEM].active) val |= (1 << 0);
@@ -619,9 +618,17 @@ 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.  */
+        value &= 0x31b;
+        if (value&0x118) {
+            qemu_log_mask(LOG_UNIMP, "CCR unimplemented bits"
+                                     " BFHFNMIGN, DIV_0_TRP, UNALIGN_TRP");
+            value &= ~0x118;
+        }
+        cpu->env.v7m.ccr = value;
         break;
     case 0xd24: /* System Handler Control.  */
         /* TODO: Real hardware allows you to set/clear the active bits
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 72b0b32..90ccdcd 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -396,6 +396,7 @@ 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 */
         int current_sp;
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 3993f77..402bfc5 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -5412,7 +5412,7 @@ static void do_v7m_exception_exit(CPUARMState *env)
         break;
     case 0x9: /* Return to Thread mode w/ Main stack */
     case 0xd: /* Return to Thread mode w/ Process stack */
-        if (env->v7m.exception != 0) {
+        if ((env->v7m.exception != 0) && !(env->v7m.ccr&1)) {
             /* Attempt to return to Thread mode
              * from nested handler while NONBASETHRDENA not set.
              */
@@ -5564,10 +5564,8 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
 
     qemu_log_mask(CPU_LOG_INT, "... as %d\n", env->v7m.exception);
 
-    /* Align stack pointer.  */
-    /* ??? Should only do this if Configuration Control Register
-       STACKALIGN bit is set.  */
-    if (env->regs[13] & 4) {
+    /* Align stack pointer (STACKALIGN)  */
+    if (env->v7m.ccr&(1<<9)) {
         env->regs[13] -= 4;
         xpsr |= 0x200;
     }
diff --git a/target-arm/machine.c b/target-arm/machine.c
index d7c2034..e8b710d 100644
--- a/target-arm/machine.c
+++ b/target-arm/machine.c
@@ -100,6 +100,7 @@ static const VMStateDescription vmstate_m = {
         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_INT32(env.v7m.current_sp, ARMCPU),
-- 
2.1.4

[Qemu-devel] [PATCH 18/18] armv7m: prevent unprivileged write to STIR

[Michael Davidsaver]

Prevent unprivileged from writing to the
Software Triggered Interrupt register

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
---
 hw/intc/armv7m_nvic.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index ca8c93c..b744cd5 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -654,7 +654,9 @@ static void nvic_writel(nvic_state *s, uint32_t offset, uint32_t value)
                       "NVIC: fault status registers unimplemented\n");
         break;
     case 0xf00: /* Software Triggered Interrupt Register */
-        if ((value & 0x1ff) < NVIC_MAX_IRQ) {
+        /* STIR write allowed if privlaged or USERSETMPEND set */
+        if ((arm_current_el(&cpu->env) || (cpu->env.v7m.ccr&2))
+            && ((value & 0x1ff) < NVIC_MAX_IRQ)) {
             armv7m_nvic_set_pending(s, (value&0x1ff)+16);
         }
         break;
-- 
2.1.4

Re: [Qemu-devel] [PATCH 00/18] Fix exception handling and msr/mrs access

[Peter Maydell]

On 9 November 2015 at 01:11, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> This series grew from a previous incorrect patch attempting to fix
> some incorrect behavior.  After spending some time going through the
> arch. ref. manual for v7-M I think I understand better how this should
> work and have made a number of changes which actually improve the situation.
>
> These changes have not yet been cross checked against real hardware, and
> I therefore don't consider them mergeable.  It's gotten big enough though
> that I'd like to get some feedback.

Thanks for this -- our M profile code has been in need of some love
for quite a while now.

> This series removes the dependence of the NVIC code on the GIC.  The GIC
> doesn't have the concept of PRIGROUP to change the size of the group
> priority field.  Also, there are a lot of cases in this code which
> I don't understand and worry about breaking.  Now that I have things
> working (I think), I could look at recombining them if this is desired.

I think separating out the NVIC is the right thing. Some of the
programmer-visible register views are superficially similar to the GIC,
but the underlying logic of how exceptions are dealt with is definitely
different, and trying to share code just makes it harder to maintain
both. (In particular for M profile external interrupts are just another
kind of exception and get prioritised along with all the internal
exceptions; for A/R profile the GIC really is an external thing
that prioritises external interrupts only to forward to the CPU.)

> I looked briefly at qtest, but can't quite see how to use it given
> the need to execute code to test most of the exception behavior.
>  Is something like this feasible at present?

You're correct that we don't have a very good story for how to test
parts of the system that really need execution of guest code.

I'll reply to the various patches individually with comments.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 01/18] armv7m: MRS/MSR handle unprivileged access

[Peter Maydell]

On 9 November 2015 at 01:11, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> The MRS and MSR instruction handling isn't checking
> the current permission level.
>
> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
> ---
>  target-arm/helper.c | 79 +++++++++++++++++++++++++----------------------------
>  1 file changed, 37 insertions(+), 42 deletions(-)

This patch looks good overall, but there's one style nit:

> +    case 0 ... 7: /* xPSR sub-fields */
> +        mask = 0;
> +        if ((reg&1) && el) {

you want spaces around operators, so "reg & 1" here and elsewhere.

It would also be good to mention in the commit message the
other things this patch is fixing:
 * privileged attempts to write EPSR should do nothing
 * accessing an unknown special register now triggers a
   guest-error warning rather than aborting QEMU

thanks
-- PMM

Re: [Qemu-devel] [PATCH 03/18] armv7m: Complain about incorrect exception table entries.

[Peter Maydell]

On 9 November 2015 at 01:11, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> For -M  These should always be thumb mode.
> Log a message if this is seen.
>
> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>

This one's not really correct, I'm afraid (though the spec-mandated
behaviour is a bit subtle).

> ---
>  target-arm/helper.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 4408100..4178400 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -5396,7 +5396,8 @@ static void do_v7m_exception_exit(CPUARMState *env)
>          qemu_log_mask(LOG_GUEST_ERROR,
>                        "M profile return from interrupt with misaligned "
>                        "PC is UNPREDICTABLE\n");
> -        /* Actual hardware seems to ignore the lsbit, and there are several
> +        /* The ARM calls for UsageFault when the T bit isn't set, but
> +         * actual hardware seems to ignore the lsbit, and there are several

No, this is UNPREDICTABLE, as the error message above says. See the
PopStack() pseudocode. No fault of any kind is mandated here (and
ignoring the lsbit in the exception return frame is within the set
of things UNPREDICTABLE allows us and the hardware to do).

>           * RTOSes out there which incorrectly assume the r15 in the stack
>           * frame should be a Thumb-style "lsbit indicates ARM/Thumb" value.
>           */
> @@ -5498,6 +5499,12 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>      addr = ldl_phys(cs->as, env->v7m.vecbase + env->v7m.exception * 4);
>      env->regs[15] = addr & 0xfffffffe;
>      env->thumb = addr & 1;
> +    if (!env->thumb) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "M profile interrupt handler %d with incorrect "
> +                      "instruction mode in PC is UNPREDICTABLE\n",
> +                      env->v7m.exception);
> +    }

This is also wrong -- the behaviour here is defined by the
ExceptionTaken pseudocode, which requires that we set the EPSR.T
bit from the low bit of the exception vector table entry, so it's
not UNPREDICTABLE at all. (See also section B1.5.3 of the v7M ARM ARM
rev E.b.)

The part we do not implement of this is that the architecture
mandates that attempts to *execute an instruction* with the T bit
clear should generate a UsageFault. (Similarly, it's valid to
execute a branch instruction with BX that clears the T bit, but
when you get to the destination you'll get a UsageFault on first
attempt to execute an instruction.)

The correct place to implement this behaviour is in translate.c:
in the bit of code that currently reads:

        if (dc->thumb) {
            disas_thumb_insn(env, dc);
            if (dc->condexec_mask) {
                dc->condexec_cond = (dc->condexec_cond & 0xe)
                                   | ((dc->condexec_mask >> 4) & 1);
                dc->condexec_mask = (dc->condexec_mask << 1) & 0x1f;
                if (dc->condexec_mask == 0) {
                    dc->condexec_cond = 0;
                }
            }
        } else {
            unsigned int insn = arm_ldl_code(env, dc->pc, dc->bswap_code);
            dc->pc += 4;
            disas_arm_insn(dc, insn);
        }

the else branch should have:
    if (arm_dc_feature(dc, ARM_FEATURE_M)) {
        /* For M profile any attempt to execute with EPSR.T clear triggers
         * an INVSTATE UsageFault.
         */
        gen_exception(something);
        goto done_generating;
    }

thanks
-- PMM

Re: [Qemu-devel] [PATCH 04/18] armv7m: Explicit error for bad vector table

[Peter Maydell]

On 9 November 2015 at 01:11, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> Give an explicit error and abort when a load
> from VECBASE fails.  Otherwise would likely
> jump to 0, which for v7-m holds the reset stack
> pointer address.
>
> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
> ---
>  target-arm/helper.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 4178400..1d7ac43 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -5496,7 +5496,17 @@ 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);
> +    {
> +        MemTxResult result;
> +        addr = address_space_ldl(cs->as,
> +                                 env->v7m.vecbase + env->v7m.exception * 4,
> +                                 MEMTXATTRS_UNSPECIFIED, &result);
> +        if (result != MEMTX_OK) {
> +            cpu_abort(cs, "Failed to read from exception vector table "
> +                      "entry %08x\n",
> +                      env->v7m.vecbase + env->v7m.exception * 4);
> +        }
> +    }

The behaviour on a failed vector table read is actually architecturally
specified: we should take a nested exception (escalated to HardFault).
If it happens while we're trying to take a HardFault in the first place
then we go into Lockup (where the CPU sits around repeatedly trying
to execute an instruction at 0xFFFFFFFE; it is technically possible
to get back out of Lockup by taking an NMI or a system reset).

That said, trying to get nested exceptions and priority escalation
right is fairly involved, and implementing lockup is both involved
and an exercise in pointlessness. So I think this code is an
improvement overall. I would suggest some small changes, though:

(1) factor this out into its own function, something like:
static uint32_t v7m_read_vector(CPUARMState *env, int excnum)
so the calling code can just do
   addr = v7m_read_vector(env, env->v7m.exception);
(2) use a local variable for "env->v7m.vecbase + excnum * 4"
rather than calculating it twice

thanks
-- PMM

Re: [Qemu-devel] [PATCH 09/18] armv7m: NVIC update vmstate

[Peter Maydell]

On 9 November 2015 at 01:11, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
> ---
>  hw/intc/armv7m_nvic.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 62 insertions(+), 2 deletions(-)
>
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 3b10dee..c860b36 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -810,15 +810,75 @@ static const MemoryRegionOps nvic_sysreg_ops = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>
> +static
> +int nvic_post_load(void *opaque, int version_id)
> +{
> +    nvic_state *s = opaque;
> +    unsigned i;
> +
> +    /* evil hack to get ARMCPU* ahead of time */
> +    assert(cpus.tqh_first);
> +    assert(!CPU_NEXT(cpus.tqh_first));
> +    s->cpu = ARM_CPU(cpus.tqh_first);
> +    assert(s->cpu);

Why do we need to do this? By the time we get to loading
state into the system, s->cpu should already have been set.

> +
> +    /* recalculate priorities */
> +    for (i = 4; i < s->num_irq; i++) {
> +        set_prio(s, i, s->vectors[i].raw_prio);
> +    }
> +
> +    nvic_irq_update(s, highest_runnable_prio(s->cpu));
> +
> +    return 0;
> +}
> +
> +static
> +int vec_info_get(QEMUFile *f, void *pv, size_t size)
> +{
> +    vec_info *I = pv;
> +    I->prio_sub = qemu_get_be16(f);
> +    I->prio_group = qemu_get_byte(f);
> +    I->raw_prio = qemu_get_ubyte(f);
> +    I->enabled = qemu_get_ubyte(f);
> +    I->pending = qemu_get_ubyte(f);
> +    I->active = qemu_get_ubyte(f);
> +    I->level = qemu_get_ubyte(f);
> +    return 0;
> +}
> +
> +static
> +void vec_info_put(QEMUFile *f, void *pv, size_t size)
> +{
> +    vec_info *I = pv;
> +    qemu_put_be16(f, I->prio_sub);
> +    qemu_put_byte(f, I->prio_group);
> +    qemu_put_ubyte(f, I->raw_prio);
> +    qemu_put_ubyte(f, I->enabled);
> +    qemu_put_ubyte(f, I->pending);
> +    qemu_put_ubyte(f, I->active);
> +    qemu_put_ubyte(f, I->level);
> +}
> +
> +static const VMStateInfo vmstate_info = {
> +    .name = "armv7m_nvic_info",
> +    .get = vec_info_get,
> +    .put = vec_info_put,
> +};

I don't think there's any need to use get and put functions here:
better to define a VMStateDescripton for the struct vec_info,
and then you can just refer to that from the main vmstate_nvic
description. (hw/audio/pl041.c has some examples of this.)

> +
>  static const VMStateDescription vmstate_nvic = {
>      .name = "armv7m_nvic",
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> +    .version_id = 2,
> +    .minimum_version_id = 2,
> +    .post_load = &nvic_post_load,
>      .fields = (VMStateField[]) {
> +        VMSTATE_ARRAY(vectors, nvic_state, NVIC_MAX_VECTORS, 0,
> +                      vmstate_info, vec_info),
> +        VMSTATE_UINT8(prigroup, nvic_state),
>          VMSTATE_UINT32(systick.control, nvic_state),
>          VMSTATE_UINT32(systick.reload, nvic_state),
>          VMSTATE_INT64(systick.tick, nvic_state),
>          VMSTATE_TIMER_PTR(systick.timer, nvic_state),
> +        VMSTATE_UINT32(num_irq, nvic_state),
>          VMSTATE_END_OF_LIST()
>      }
>  };

Ideally the VMState should be added in the same patches that
add new fields or structures, rather than in its own patch
at the end.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 15/18] arm: gic: Remove references to NVIC

[Peter Maydell]

On 9 November 2015 at 01:11, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> armv7m_nvic.c no longer relies on the GIC.
> Remove REV_NVIC and conditionals which use it.
>
> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
> ---
>  hw/intc/arm_gic.c        | 14 +++++++-------
>  hw/intc/arm_gic_common.c | 23 ++++++++---------------
>  hw/intc/gic_internal.h   |  7 ++-----
>  3 files changed, 17 insertions(+), 27 deletions(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 05/18] armv7m: expand NVIC state

[Peter Maydell]

On 9 November 2015 at 01:11, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> Expand the NVIC to fully support -M priorities and masking.
> Doesn't use GIC code.
>
> Move some state to ARMCPU to allow calculation of exception masking.
>
> Add storage for PRIGROUP to configure group/sub-group split.
> Track group and sub-group in separate fields for quick comparison.
> Mix in vector # with sub-group as per tie breaking rules.
>
> NVIC now derives directly from SysBusDevice, and
> struct NVICClass is eliminated.
>
> Also add DPRINTF() macro.
>
> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>

This patch doesn't compile, because you've removed the definition of
NVICClass, NVIC_CLASS, etc, but not their uses. A patchset needs to
compile after every patch in it, not just at the end when all patches
are applied. You'll need to rearrange your changes between patches
a bit.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 06/18] armv7m: new NVIC utility functions

[Peter Maydell]

On 9 November 2015 at 01:11, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> Internal functions for operations previously done
> by GIC internals.
>
> nvic_irq_update() recalculates highest pending/active
> exceptions.
>
> armv7m_nvic_set_pending() include exception escalation
> logic.
>
> armv7m_nvic_acknowledge_irq() and nvic_irq_update()
> update ARMCPU fields.

Hi; I have a lot of review comments on this patch set, but that's
really because v7M exception logic is pretty complicated and
our current code is a long way away from correct. You might
find it helpful to separate out "restructure the NVIC code
into its own device" into separate patches from "and now add
functionality like exception escalation", perhaps.

> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
> ---
>  hw/intc/armv7m_nvic.c | 250 +++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 235 insertions(+), 15 deletions(-)
>
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 487a09a..ebb4d4e 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -140,36 +140,256 @@ static void systick_reset(nvic_state *s)
>      timer_del(s->systick.timer);
>  }
>
> -/* The external routines use the hardware vector numbering, ie. the first
> -   IRQ is #16.  The internal GIC routines use #32 as the first IRQ.  */
> +/* caller must call nvic_irq_update() after this */
> +static
> +void set_prio(nvic_state *s, unsigned irq, uint8_t prio)
> +{
> +    unsigned submask = (1<<(s->prigroup+1))-1;
> +
> +    assert(irq > 3); /* only use for configurable prios */
> +    assert(irq < NVIC_MAX_VECTORS);
> +
> +    s->vectors[irq].raw_prio = prio;
> +    s->vectors[irq].prio_group = (prio>>(s->prigroup+1));
> +    s->vectors[irq].prio_sub = irq + (prio&submask)*NVIC_MAX_VECTORS;

Precalculating the group priority seems a bit odd, since it
will change later anyway if the guest writes to PRIGROUP.

> +
> +    DPRINTF(0, "Set %u priority grp %d sub %u\n", irq,
> +            s->vectors[irq].prio_group, s->vectors[irq].prio_sub);
> +}
> +
> +/* recompute highest pending */
> +static
> +void nvic_irq_update(nvic_state *s, int update_active)
> +{
> +    unsigned i;
> +    int lvl;
> +    CPUARMState *env = &s->cpu->env;
> +    int16_t act_group = 0x100, pend_group = 0x100;
> +    uint16_t act_sub = 0, pend_sub = 0;
> +    uint16_t act_irq = 0, pend_irq = 0;
> +
> +    /* find highest priority */
> +    for (i = 1; i < s->num_irq; i++) {
> +        vec_info *I = &s->vectors[i];

Minor style issue: we prefer lower case for variable names,
and CamelCase for structure type names (see CODING_STYLE).
So this would be VecInfo *vec; or something similar.

> +
> +        DPRINTF(2, " VECT %d %d:%u\n", i, I->prio_group, I->prio_sub);
> +
> +        if (I->active && ((I->prio_group < act_group)
> +                || (I->prio_group == act_group && I->prio_sub < act_sub)))

Because the group priority and sub priority are just two contiguous
parts of the raw priority, you get the same effect here by just
doing a comparison on the raw value: "I->raw_prio <  act_raw_prio".

Note however that the running priority is actually only the
group priority part (see the pseudocode ExecutionPriority function)
and so you want something more like:

    highestpri = 0x100;
    for (each vector) {
        if (vector->active && vector->raw_prio < highestpri) {
            highestpri = vector->raw_prio & prigroup_mask;
            act_sub = vector->raw_prio & ~prigroup_mask; // if you need it
        }
    }

(this is why it's not worth precalculating the group and
subpriorities -- it's cheap enough to do at this point.)

> +        {
> +            act_group = I->prio_group;
> +            act_sub = I->prio_sub;
> +            act_irq = i;
> +        }
> +
> +        if (I->enabled && I->pending && ((I->prio_group < pend_group)
> +                || (I->prio_group == pend_group && I->prio_sub < pend_sub)))
> +        {
> +            pend_group = I->prio_group;
> +            pend_sub = I->prio_sub;
> +            pend_irq = i;
> +        }
> +    }
> +
> +    env->v7m.pending = pend_irq;
> +    env->v7m.pending_prio = pend_group;
> +
> +    if (update_active) {
> +        env->v7m.exception = act_irq;
> +        env->v7m.exception_prio = act_group;
> +    }
> +
> +    /* Raise NVIC output even if pend_group is masked.
> +     * This is necessary as we get no notification
> +     * when PRIMASK et al. are changed.
> +     * As long as our output is high cpu_exec() will call
> +     * into arm_v7m_cpu_exec_interrupt() frequently, which
> +     * then tests to see if the pending exception
> +     * is permitted.
> +     */

This is bad -- we should instead arrange to update our idea
of the current running priority when PRIMASK &c are updated.
(Also it's not clear why we need to have an outbound qemu_irq
just to tell the core there's a pending exception. I think
we should just be able to call cpu_interrupt().)

> +    lvl = pend_irq > 0;
> +    DPRINTF(1, "highest pending %d %d:%u\n", pend_irq, pend_group, pend_sub);
> +    DPRINTF(1, "highest active  %d %d:%u\n", act_irq, act_group, act_sub);
> +
> +    DPRINTF(0, "IRQ %c highest act %d pend %d\n",
> +            lvl ? 'X' : '_', act_irq, pend_irq);
> +
> +    qemu_set_irq(s->excpout, lvl);
> +}
> +
> +static
> +void armv7m_nvic_clear_pending(void *opaque, int irq)
> +{
> +    nvic_state *s = (nvic_state *)opaque;
> +    vec_info *I;
> +
> +    assert(irq >= 0);
> +    assert(irq < NVIC_MAX_VECTORS);
> +
> +    I = &s->vectors[irq];
> +    if (I->pending) {
> +        I->pending = 0;
> +        nvic_irq_update(s, 0);
> +    }
> +}
> +
>  void armv7m_nvic_set_pending(void *opaque, int irq)
>  {
>      nvic_state *s = (nvic_state *)opaque;
> -    if (irq >= 16)
> -        irq += 16;
> -    gic_set_pending_private(&s->gic, 0, irq);
> +    CPUARMState *env = &s->cpu->env;
> +    vec_info *I;
> +    int active = s->cpu->env.v7m.exception;
> +
> +    assert(irq > 0);
> +    assert(irq < NVIC_MAX_VECTORS);
> +
> +    I = &s->vectors[irq];
> +
> +    if (irq < ARMV7M_EXCP_SYSTICK && irq != ARMV7M_EXCP_DEBUG) {

This will include NMI, which is wrong (NMI doesn't escalate
to HardFault because it's already higher priority than HardFault).
It also includes PendSV, which is classified as an interrupt
(like SysTick) and so also doesn't get escalated.

Also worth noting in a comment somewhere that for QEMU
BusFault is always synchronous and so we have no asynchronous
faults. (Async faults don't escalate.)

> +        int runnable = armv7m_excp_unmasked(s->cpu);

"running_prio" might be a better name for this variable, since
it's the current execution (running) priority.

> +        /* test for exception escalation for vectors other than:
> +         * Debug (12), SysTick (15), and all external IRQs (>=16)
> +         */

This isn't really getting the DebugMonitor fault case right:
DebugMonitor exceptions caused by executing a BKPT insn must be
escalated if the running priority is too low; other DebugMonitor
exceptions are just ignored (*not* set Pending).

> +        unsigned escalate = 0;
> +        if (I->active) {
> +            /* trying to pend an active fault (possibly nested).
> +             * eg. UsageFault in UsageFault handler
> +             */
> +            escalate = 1;
> +            DPRINTF(0, " Escalate, active\n");

You don't need to explicitly check for this case, because it
will be caught by the "is the running priority too high" check
(the current running priority must be at least as high as the
priority of an active fault handler). If you look at the ARM ARM
section on priority escalation you'll see that "undef in a
UsageFault handler" is listed in the "examples of things that
cause priority escalation" bullet list, not the "definition of
what causes escalation" list.

> +        } else if (!I->enabled) {
> +            /* trying to pend a disabled fault
> +             * eg. UsageFault while USGFAULTENA in SHCSR is clear.
> +             */
> +            escalate = 1;
> +            DPRINTF(0, " Escalate, not enabled\n");
> +        } else if (I->prio_group > runnable) {

This should be <= : (a) equal priority to current execution
priority gets escalated and (b) prio values are "low numbers
are higher priorities".

> +            /* trying to pend a fault which is not immediately
> +             * runnable due to masking by PRIMASK, FAULTMASK, BASEPRI,
> +             * or the priority of the active exception
> +             */
> +            DPRINTF(0, " Escalate, mask %d >= %d\n",
> +                    I->prio_group, runnable);

The condition printed in the debug log doesn't match the
condition tested by the code.

> +            escalate = 1;
> +        }
> +
> +        if (escalate) {
> +#ifdef DEBUG_NVIC
> +            int oldirq = irq;
> +#endif
> +            if (runnable < -1) {
> +                /* TODO: actual unrecoverable exception actions */
> +                cpu_abort(&s->cpu->parent_obj,
> +                          "%d in %d escalates to unrecoverable exception\n",
> +                          irq, active);
> +            } else {
> +                irq = ARMV7M_EXCP_HARD;
> +            }
> +            I = &s->vectors[irq];
> +
> +            DPRINTF(0, "Escalate %d to %d\n", oldirq, irq);

Faults escalated to HardFault retain the ReturnAddress
behaviour of the original fault (ie what return-address
is saved to the stack as part of the exception stacking and
so whether we resume execution on the faulting insn or the
next one). So it's not sufficient to just say "treat it
exactly as if it were a HardFault" like this.

> +        }
> +    }
> +
> +    I->pending = 1;
> +    if (I->enabled && (I->prio_group < env->v7m.pending_prio)) {
> +        env->v7m.pending_prio = I->prio_group;
> +        env->v7m.pending = irq;
> +        qemu_set_irq(s->excpout, irq > 0);
> +    }
> +    DPRINTF(0, "Pending %d at %d%s runnable %d\n",
> +            irq, I->prio_group,
> +            env->v7m.pending == irq ? " (highest)" : "",
> +            armv7m_excp_unmasked(s->cpu));
>  }

thanks
-- PMM

Re: [Qemu-devel] [PATCH 11/18] armv7m: fix I and F flag handling

[Peter Maydell]

On 9 November 2015 at 01:11, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> Despite having the same notation, these bits
> have completely different meaning than -AR.
>
> Add armv7m_excp_unmasked()
> to calculate the currently runable exception priority
> taking into account masks and active handlers.
> Use this in conjunction with the pending exception
> priority to determine if the pending exception
> can interrupt execution.

This function is used by code added in earlier patches in
this series, so this patch needs to be moved earlier in the
series, or those patches won't compile.

> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
> ---
>  target-arm/cpu.c | 26 +++++++-------------------
>  target-arm/cpu.h | 27 ++++++++++++++++++++++++++-
>  2 files changed, 33 insertions(+), 20 deletions(-)
>
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index be026bc..5d03117 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -173,6 +173,8 @@ static void arm_cpu_reset(CPUState *s)
>          uint32_t initial_pc; /* Loaded from 0x4 */
>          uint8_t *rom;
>
> +        env->v7m.exception_prio = env->v7m.pending_prio = 0x100;
> +
>          env->daif &= ~PSTATE_I;
>          rom = rom_ptr(0);
>          if (rom) {
> @@ -301,29 +303,15 @@ static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cs);
>      ARMCPU *cpu = ARM_CPU(cs);
> -    CPUARMState *env = &cpu->env;
>      bool ret = false;
>
> -
> -    if (interrupt_request & CPU_INTERRUPT_FIQ
> -        && !(env->daif & PSTATE_F)) {
> -        cs->exception_index = EXCP_FIQ;
> -        cc->do_interrupt(cs);
> -        ret = true;
> -    }
> -    /* 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 when the
> -     * CPU tries to execute code at the magic address.
> -     * This will cause the magic PC value to be pushed to
> -     * the stack if an interrupt occurred at the wrong time.
> -     * We avoid this by disabling interrupts when
> -     * pc contains a magic address.

This (removing this comment and the checks for the magic address)
seem to be part of a separate change [probably the one in
"armv7m: Undo armv7m.hack"] and shouldn't be in this patch.

> +    /* ARMv7-M interrupt masking works differently than -A or -R.
> +     * There is no FIQ/IRQ distinction.
> +     * Instead of masking interrupt sources, the I and F bits
> +     * (along with basepri) mask certain exception priority levels.
>       */
>      if (interrupt_request & CPU_INTERRUPT_HARD
> -        && !(env->daif & PSTATE_I)
> -        && (env->regs[15] < 0xfffffff0)) {
> +            && (armv7m_excp_unmasked(cpu) >= cpu->env.v7m.pending_prio)) {
>          cs->exception_index = EXCP_IRQ;
>          cc->do_interrupt(cs);
>          ret = true;
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index c193fbb..29d89ce 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1033,9 +1033,34 @@ void arm_cpu_list(FILE *f, fprintf_function cpu_fprintf);
>  uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx,
>                                   uint32_t cur_el, bool secure);
>
> +/* @returns highest (numerically lowest) unmasked exception priority
> + */
> +static inline
> +int armv7m_excp_unmasked(ARMCPU *cpu)

What this is really calculating is the current execution
priority (running priority) of the CPU, so I think a better
name would be armv7m_current_exec_priority() or
armv7m_current_priority() or armv7m_running_priority() or similar.

> +{
> +    CPUARMState *env = &cpu->env;
> +    int runnable;
> +
> +    /* find highest (numerically lowest) priority which could
> +     * run based on masks
> +     */
> +    if (env->daif&PSTATE_F) { /* FAULTMASK */

Style issue -- operands should have spaces around them.

> +        runnable = -2;

These all seem to be off by one: FAULTMASK sets the
running priority to -1, not -2, PRIMASK sets it to 0,
not -1, and so on.

> +    } else if (env->daif&PSTATE_I) { /* PRIMASK */
> +        runnable = -1;
> +    } else if (env->v7m.basepri > 0) {
> +        /* BASEPRI==1 -> runnable==-1 (same as PRIMASK==1) */

(applies to operands in comments too)

> +        runnable = env->v7m.basepri-2;

Where is this - 2 from? Also, BASEPRI values honour the
PRIGROUP setting. (Compare the ExecutionPriority pseudocode).

> +    } else {
> +        runnable = 0x100; /* lower than any possible priority */
> +    }
> +    /* consider priority of active handler */
> +    return MIN(runnable, env->v7m.exception_prio-1);

I don't think this -1 should be here.

> +}
> +
>  /* Interface between CPU and Interrupt controller.  */
>  void armv7m_nvic_set_pending(void *opaque, int irq);
> -int armv7m_nvic_acknowledge_irq(void *opaque);
> +void armv7m_nvic_acknowledge_irq(void *opaque);
>  void armv7m_nvic_complete_irq(void *opaque, int irq);
>
>  /* Interface for defining coprocessor registers.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 00/18] Fix exception handling and msr/mrs access

[Peter Maydell]

On 17 November 2015 at 17:07, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 9 November 2015 at 01:11, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
>> This series grew from a previous incorrect patch attempting to fix
>> some incorrect behavior.  After spending some time going through the
>> arch. ref. manual for v7-M I think I understand better how this should
>> work and have made a number of changes which actually improve the situation.
>>
>> These changes have not yet been cross checked against real hardware, and
>> I therefore don't consider them mergeable.  It's gotten big enough though
>> that I'd like to get some feedback.

> I'll reply to the various patches individually with comments.

I think I've now done that at least for the earlier patches.
There are probably some other finer details that I'll get to
in a later round of patch review but hopefully you have enough
to do some of the fixes and restructuring of this patchset for v2.

I think the most important thing here is getting the structure
of the changes into patches right so they're easy to review.
The general principles here are:
 * each patch should aim to be self-contained and to do one thing,
   not several things (for instance, avoid making several bug fixes
   in one patch, avoid putting "restructure/refactor code" and
   "add new feature" in the same patch, and so on)
 * at each point in the patch series QEMU needs to still compile
   and run (this is particularly important to allow bisection
   of bugs later on where people will want to be able to narrow
   down which commit introduced a bug)
If the structure of the patchset is right it should be fairly
easy to review your improvements against the architecture manual.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 00/18] Fix exception handling and msr/mrs access

[Michael Davidsaver]

On 11/20/2015 08:59 AM, Peter Maydell wrote:
> I think I've now done that at least for the earlier patches.
> There are probably some other finer details that I'll get to
> in a later round of patch review but hopefully you have enough
> to do some of the fixes and restructuring of this patchset for v2

Thanks for the comments.  I'll be addressing them individually and follow that with another patch set.

Re: [Qemu-devel] [PATCH 01/18] armv7m: MRS/MSR handle unprivileged access

[Michael Davidsaver]

On 11/17/2015 12:09 PM, Peter Maydell wrote:
> On 9 November 2015 at 01:11, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
>> The MRS and MSR instruction handling isn't checking
>> the current permission level.
>>
>> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
>> ---
>>  target-arm/helper.c | 79 +++++++++++++++++++++++++----------------------------
>>  1 file changed, 37 insertions(+), 42 deletions(-)
> 
> This patch looks good overall, but there's one style nit:
> 
>> +    case 0 ... 7: /* xPSR sub-fields */
>> +        mask = 0;
>> +        if ((reg&1) && el) {
> 
> you want spaces around operators, so "reg & 1" here and elsewhere.

Would be nice if checkpatch.pl caught these, but I understand that this would be quite difficult to do well.  I've tried to catch this with grep and sort through the false positives.  I think I got them all.

> It would also be good to mention in the commit message the
> other things this patch is fixing:
>  * privileged attempts to write EPSR should do nothing
>  * accessing an unknown special register now triggers a
>    guest-error warning rather than aborting QEMU

Will do.

Re: [Qemu-devel] [PATCH 03/18] armv7m: Complain about incorrect exception table entries.

[Michael Davidsaver]

On 11/17/2015 12:20 PM, Peter Maydell wrote:
> This one's not really correct, I'm afraid (though the spec-mandated
> behaviour is a bit subtle).

I've dropped this patch.

Re: [Qemu-devel] [PATCH 04/18] armv7m: Explicit error for bad vector table

[Michael Davidsaver]

On 11/17/2015 12:33 PM, Peter Maydell wrote:
> On 9 November 2015 at 01:11, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
>> Give an explicit error and abort when a load
>> from VECBASE fails.  Otherwise would likely
>> jump to 0, which for v7-m holds the reset stack
>> pointer address.
>>
>> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
>> ---
>>  target-arm/helper.c | 12 +++++++++++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 4178400..1d7ac43 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -5496,7 +5496,17 @@ 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);
>> +    {
>> +        MemTxResult result;
>> +        addr = address_space_ldl(cs->as,
>> +                                 env->v7m.vecbase + env->v7m.exception * 4,
>> +                                 MEMTXATTRS_UNSPECIFIED, &result);
>> +        if (result != MEMTX_OK) {
>> +            cpu_abort(cs, "Failed to read from exception vector table "
>> +                      "entry %08x\n",
>> +                      env->v7m.vecbase + env->v7m.exception * 4);
>> +        }
>> +    }
> 
> The behaviour on a failed vector table read is actually architecturally
> specified: we should take a nested exception (escalated to HardFault).
> If it happens while we're trying to take a HardFault in the first place
> then we go into Lockup (where the CPU sits around repeatedly trying
> to execute an instruction at 0xFFFFFFFE; it is technically possible
> to get back out of Lockup by taking an NMI or a system reset).
> 
> That said, trying to get nested exceptions and priority escalation
> right is fairly involved, and implementing lockup is both involved
> and an exercise in pointlessness. So I think this code is an
> improvement overall.

This is my thinking as well.  One point against it is that abort() is inconvenient when using '-gdb'.  I'm not sure if there is something else which could be done (cpu halt?).

> I would suggest some small changes, though:
> 
> (1) factor this out into its own function, something like:
> static uint32_t v7m_read_vector(CPUARMState *env, int excnum)
> so the calling code can just do
>    addr = v7m_read_vector(env, env->v7m.exception);
> (2) use a local variable for "env->v7m.vecbase + excnum * 4"
> rather than calculating it twice

Done.

Re: [Qemu-devel] [PATCH 05/18] armv7m: expand NVIC state

[Michael Davidsaver]

On 11/17/2015 01:10 PM, Peter Maydell wrote:
> On 9 November 2015 at 01:11, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
>> Expand the NVIC to fully support -M priorities and masking.
>> Doesn't use GIC code.
>>
>> Move some state to ARMCPU to allow calculation of exception masking.
>>
>> Add storage for PRIGROUP to configure group/sub-group split.
>> Track group and sub-group in separate fields for quick comparison.
>> Mix in vector # with sub-group as per tie breaking rules.
>>
>> NVIC now derives directly from SysBusDevice, and
>> struct NVICClass is eliminated.
>>
>> Also add DPRINTF() macro.
>>
>> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
> 
> This patch doesn't compile, because you've removed the definition of
> NVICClass, NVIC_CLASS, etc, but not their uses. A patchset needs to
> compile after every patch in it, not just at the end when all patches
> are applied. You'll need to rearrange your changes between patches
> a bit.

In the next rev. I've rearranged things so that each patches compiles.  At least according to 'git rebase -i -x make', so not a full rebuilt.

This does mean that the big block of changes to the NVIC are now almost entirely in one patch as I couldn't see how to split them up given that the nvic_state structure is changed so much.

Re: [Qemu-devel] [PATCH 01/18] armv7m: MRS/MSR handle unprivileged access

[Peter Maydell]

On 2 December 2015 at 22:51, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
>
>
> On 11/17/2015 12:09 PM, Peter Maydell wrote:
>> On 9 November 2015 at 01:11, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
>>> The MRS and MSR instruction handling isn't checking
>>> the current permission level.
>>>
>>> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
>>> ---
>>>  target-arm/helper.c | 79 +++++++++++++++++++++++++----------------------------
>>>  1 file changed, 37 insertions(+), 42 deletions(-)
>>
>> This patch looks good overall, but there's one style nit:
>>
>>> +    case 0 ... 7: /* xPSR sub-fields */
>>> +        mask = 0;
>>> +        if ((reg&1) && el) {
>>
>> you want spaces around operators, so "reg & 1" here and elsewhere.
>
> Would be nice if checkpatch.pl caught these, but I understand that
> this would be quite difficult to do well.  I've tried to catch this
> with grep and sort through the false positives.  I think I got them
> all.

It looks like this is a case where checkpatch.pl is applying
Linux kernel style (since that's where we borrowed it from)
which is looser than ours, or at least looser than what I think
our style is. There's a specific check:

       # << and >> may either have or not have spaces both sides
    } elsif ($op eq '<<' or $op eq '>>' or
             $op eq '&' or $op eq '^' or $op eq '|' or
             $op eq '+' or $op eq '-' or
             $op eq '*' or $op eq '/' or
             $op eq '%')

and those operators are only checked for "must have consistent
spacing both sides" rather than "must have spaces on both sides".

thanks
-- PMM

Re: [Qemu-devel] [PATCH 04/18] armv7m: Explicit error for bad vector table

[Peter Maydell]

On 2 December 2015 at 22:55, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> On 11/17/2015 12:33 PM, Peter Maydell wrote:
>> The behaviour on a failed vector table read is actually architecturally
>> specified: we should take a nested exception (escalated to HardFault).
>> If it happens while we're trying to take a HardFault in the first place
>> then we go into Lockup (where the CPU sits around repeatedly trying
>> to execute an instruction at 0xFFFFFFFE; it is technically possible
>> to get back out of Lockup by taking an NMI or a system reset).
>>
>> That said, trying to get nested exceptions and priority escalation
>> right is fairly involved, and implementing lockup is both involved
>> and an exercise in pointlessness. So I think this code is an
>> improvement overall.
>
> This is my thinking as well.  One point against it is that abort() is
> inconvenient when using '-gdb'.  I'm not sure if there is something
> else which could be done (cpu halt?).

I can't think of any immediately good suggestions. It is probably
worth at least commenting (a) places where we are not doing the
architectural behaviour (b) places which should go into Lockup.
Then we can find them later if we come up with a better idea...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 06/18] armv7m: new NVIC utility functions

[Michael Davidsaver]

On 11/20/2015 08:25 AM, Peter Maydell wrote:
> On 9 November 2015 at 01:11, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
>> Internal functions for operations previously done
>> by GIC internals.
>>
>> nvic_irq_update() recalculates highest pending/active
>> exceptions.
>>
>> armv7m_nvic_set_pending() include exception escalation
>> logic.
>>
>> armv7m_nvic_acknowledge_irq() and nvic_irq_update()
>> update ARMCPU fields.
> 
> Hi; I have a lot of review comments on this patch set, but that's
> really because v7M exception logic is pretty complicated and
> our current code is a long way away from correct. You might
> find it helpful to separate out "restructure the NVIC code
> into its own device" into separate patches from "and now add
> functionality like exception escalation", perhaps.

I'd certainly find this helpful :) I'm just not sure how to accomplish this and still make every patch compile.

>> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
>> ---
>>  hw/intc/armv7m_nvic.c | 250 +++++++++++++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 235 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
>> index 487a09a..ebb4d4e 100644
>> --- a/hw/intc/armv7m_nvic.c
>> +++ b/hw/intc/armv7m_nvic.c
>> @@ -140,36 +140,256 @@ static void systick_reset(nvic_state *s)
>>      timer_del(s->systick.timer);
>>  }
>>
>> -/* The external routines use the hardware vector numbering, ie. the first
>> -   IRQ is #16.  The internal GIC routines use #32 as the first IRQ.  */
>> +/* caller must call nvic_irq_update() after this */
>> +static
>> +void set_prio(nvic_state *s, unsigned irq, uint8_t prio)
>> +{
>> +    unsigned submask = (1<<(s->prigroup+1))-1;
>> +
>> +    assert(irq > 3); /* only use for configurable prios */
>> +    assert(irq < NVIC_MAX_VECTORS);
>> +
>> +    s->vectors[irq].raw_prio = prio;
>> +    s->vectors[irq].prio_group = (prio>>(s->prigroup+1));
>> +    s->vectors[irq].prio_sub = irq + (prio&submask)*NVIC_MAX_VECTORS;
> 
> Precalculating the group priority seems a bit odd, since it
> will change later anyway if the guest writes to PRIGROUP.

I've kept the pre-calculation as the alternative comparison code is no simpler when tie breaking with exception number is done.

>> +
>> +    DPRINTF(0, "Set %u priority grp %d sub %u\n", irq,
>> +            s->vectors[irq].prio_group, s->vectors[irq].prio_sub);
>> +}
>> +
>> +/* recompute highest pending */
>> +static
>> +void nvic_irq_update(nvic_state *s, int update_active)
>> +{
>> +    unsigned i;
>> +    int lvl;
>> +    CPUARMState *env = &s->cpu->env;
>> +    int16_t act_group = 0x100, pend_group = 0x100;
>> +    uint16_t act_sub = 0, pend_sub = 0;
>> +    uint16_t act_irq = 0, pend_irq = 0;
>> +
>> +    /* find highest priority */
>> +    for (i = 1; i < s->num_irq; i++) {
>> +        vec_info *I = &s->vectors[i];
> 
> Minor style issue: we prefer lower case for variable names,
> and CamelCase for structure type names (see CODING_STYLE).
> So this would be VecInfo *vec; or something similar.

Done.

>> +
>> +        DPRINTF(2, " VECT %d %d:%u\n", i, I->prio_group, I->prio_sub);
>> +
>> +        if (I->active && ((I->prio_group < act_group)
>> +                || (I->prio_group == act_group && I->prio_sub < act_sub)))
> 
> Because the group priority and sub priority are just two contiguous
> parts of the raw priority, you get the same effect here by just
> doing a comparison on the raw value: "I->raw_prio <  act_raw_prio".

Not quite since it's possible that I->raw_prio == act_raw_prio.  As I read the ARM this situation should be avoided by using the exception number to break the tie.  This way no two priorities are ever equal.  This is the reason that act_sub is "irq + (prio&submask)*NVIC_MAX_VECTORS".

> Note however that the running priority is actually only the
> group priority part (see the pseudocode ExecutionPriority function)
> and so you want something more like:
> 
>     highestpri = 0x100;
>     for (each vector) {
>         if (vector->active && vector->raw_prio < highestpri) {
>             highestpri = vector->raw_prio & prigroup_mask;
>             act_sub = vector->raw_prio & ~prigroup_mask; // if you need it
>         }
>     }
> 
> (this is why it's not worth precalculating the group and
> subpriorities -- it's cheap enough to do at this point.)
> 
>> +        {
>> +            act_group = I->prio_group;
>> +            act_sub = I->prio_sub;
>> +            act_irq = i;
>> +        }
>> +
>> +        if (I->enabled && I->pending && ((I->prio_group < pend_group)
>> +                || (I->prio_group == pend_group && I->prio_sub < pend_sub)))
>> +        {
>> +            pend_group = I->prio_group;
>> +            pend_sub = I->prio_sub;
>> +            pend_irq = i;
>> +        }
>> +    }
>> +
>> +    env->v7m.pending = pend_irq;
>> +    env->v7m.pending_prio = pend_group;
>> +
>> +    if (update_active) {
>> +        env->v7m.exception = act_irq;
>> +        env->v7m.exception_prio = act_group;
>> +    }
>> +
>> +    /* Raise NVIC output even if pend_group is masked.
>> +     * This is necessary as we get no notification
>> +     * when PRIMASK et al. are changed.
>> +     * As long as our output is high cpu_exec() will call
>> +     * into arm_v7m_cpu_exec_interrupt() frequently, which
>> +     * then tests to see if the pending exception
>> +     * is permitted.
>> +     */
> 
> This is bad -- we should instead arrange to update our idea
> of the current running priority when PRIMASK &c are updated.
> (Also it's not clear why we need to have an outbound qemu_irq
> just to tell the core there's a pending exception. I think
> we should just be able to call cpu_interrupt().)

I agree, but haven't done anything about it as yet (not sure I will).

>> +    lvl = pend_irq > 0;
>> +    DPRINTF(1, "highest pending %d %d:%u\n", pend_irq, pend_group, pend_sub);
>> +    DPRINTF(1, "highest active  %d %d:%u\n", act_irq, act_group, act_sub);
>> +
>> +    DPRINTF(0, "IRQ %c highest act %d pend %d\n",
>> +            lvl ? 'X' : '_', act_irq, pend_irq);
>> +
>> +    qemu_set_irq(s->excpout, lvl);
>> +}
>> +
>> +static
>> +void armv7m_nvic_clear_pending(void *opaque, int irq)
>> +{
>> +    nvic_state *s = (nvic_state *)opaque;
>> +    vec_info *I;
>> +
>> +    assert(irq >= 0);
>> +    assert(irq < NVIC_MAX_VECTORS);
>> +
>> +    I = &s->vectors[irq];
>> +    if (I->pending) {
>> +        I->pending = 0;
>> +        nvic_irq_update(s, 0);
>> +    }
>> +}
>> +
>>  void armv7m_nvic_set_pending(void *opaque, int irq)
>>  {
>>      nvic_state *s = (nvic_state *)opaque;
>> -    if (irq >= 16)
>> -        irq += 16;
>> -    gic_set_pending_private(&s->gic, 0, irq);
>> +    CPUARMState *env = &s->cpu->env;
>> +    vec_info *I;
>> +    int active = s->cpu->env.v7m.exception;
>> +
>> +    assert(irq > 0);
>> +    assert(irq < NVIC_MAX_VECTORS);
>> +
>> +    I = &s->vectors[irq];
>> +
>> +    if (irq < ARMV7M_EXCP_SYSTICK && irq != ARMV7M_EXCP_DEBUG) {
> 
> This will include NMI, which is wrong (NMI doesn't escalate
> to HardFault because it's already higher priority than HardFault).
> It also includes PendSV, which is classified as an interrupt
> (like SysTick) and so also doesn't get escalated.

Yup, I was mis-reading the part about using NMI to recover from the unrecoverable.

> Also worth noting in a comment somewhere that for QEMU
> BusFault is always synchronous and so we have no asynchronous
> faults. (Async faults don't escalate.)

Done.

>> +        int runnable = armv7m_excp_unmasked(s->cpu);
> 
> "running_prio" might be a better name for this variable, since
> it's the current execution (running) priority.

This was an intentional, but confusing, distinction I was making (runnable==running-1).  This goes away.

>> +        /* test for exception escalation for vectors other than:
>> +         * Debug (12), SysTick (15), and all external IRQs (>=16)
>> +         */
> 
> This isn't really getting the DebugMonitor fault case right:
> DebugMonitor exceptions caused by executing a BKPT insn must be
> escalated if the running priority is too low; other DebugMonitor
> exceptions are just ignored (*not* set Pending).

Fixed.

>> +        unsigned escalate = 0;
>> +        if (I->active) {
>> +            /* trying to pend an active fault (possibly nested).
>> +             * eg. UsageFault in UsageFault handler
>> +             */
>> +            escalate = 1;
>> +            DPRINTF(0, " Escalate, active\n");
> 
> You don't need to explicitly check for this case, because it
> will be caught by the "is the running priority too high" check
> (the current running priority must be at least as high as the
> priority of an active fault handler). If you look at the ARM ARM
> section on priority escalation you'll see that "undef in a
> UsageFault handler" is listed in the "examples of things that
> cause priority escalation" bullet list, not the "definition of
> what causes escalation" list.

Good point.  On re-reading this I think I understand better how priority changes take effect (immediately).  I've move this test last and made it a hw_error() since it should only be hit if some bug results in inconsistency between the NVIC and ARMCPU priorities fields.

>> +        } else if (!I->enabled) {
>> +            /* trying to pend a disabled fault
>> +             * eg. UsageFault while USGFAULTENA in SHCSR is clear.
>> +             */
>> +            escalate = 1;
>> +            DPRINTF(0, " Escalate, not enabled\n");
>> +        } else if (I->prio_group > runnable) {
> 
> This should be <= : (a) equal priority to current execution
> priority gets escalated and (b) prio values are "low numbers
> are higher priorities".

Fixed

>> +            /* trying to pend a fault which is not immediately
>> +             * runnable due to masking by PRIMASK, FAULTMASK, BASEPRI,
>> +             * or the priority of the active exception
>> +             */
>> +            DPRINTF(0, " Escalate, mask %d >= %d\n",
>> +                    I->prio_group, runnable);
> 
> The condition printed in the debug log doesn't match the
> condition tested by the code.

Fixed

>> +            escalate = 1;
>> +        }
>> +
>> +        if (escalate) {
>> +#ifdef DEBUG_NVIC
>> +            int oldirq = irq;
>> +#endif
>> +            if (runnable < -1) {
>> +                /* TODO: actual unrecoverable exception actions */
>> +                cpu_abort(&s->cpu->parent_obj,
>> +                          "%d in %d escalates to unrecoverable exception\n",
>> +                          irq, active);
>> +            } else {
>> +                irq = ARMV7M_EXCP_HARD;
>> +            }
>> +            I = &s->vectors[irq];
>> +
>> +            DPRINTF(0, "Escalate %d to %d\n", oldirq, irq);
> 
> Faults escalated to HardFault retain the ReturnAddress
> behaviour of the original fault (ie what return-address
> is saved to the stack as part of the exception stacking and
> so whether we resume execution on the faulting insn or the
> next one). So it's not sufficient to just say "treat it
> exactly as if it were a HardFault" like this.

I'm confused.  From my testing it seems like the PC has already been adjusted by this point and no further changes are needed.  Am I missing something?

>> +        }
>> +    }
>> +
>> +    I->pending = 1;
>> +    if (I->enabled && (I->prio_group < env->v7m.pending_prio)) {
>> +        env->v7m.pending_prio = I->prio_group;
>> +        env->v7m.pending = irq;
>> +        qemu_set_irq(s->excpout, irq > 0);
>> +    }
>> +    DPRINTF(0, "Pending %d at %d%s runnable %d\n",
>> +            irq, I->prio_group,
>> +            env->v7m.pending == irq ? " (highest)" : "",
>> +            armv7m_excp_unmasked(s->cpu));
>>  }
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH 09/18] armv7m: NVIC update vmstate

[Michael Davidsaver]

On 11/17/2015 12:58 PM, Peter Maydell wrote:
> On 9 November 2015 at 01:11, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
>> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
>> ---
>>  hw/intc/armv7m_nvic.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 62 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
>> index 3b10dee..c860b36 100644
>> --- a/hw/intc/armv7m_nvic.c
>> +++ b/hw/intc/armv7m_nvic.c
>> @@ -810,15 +810,75 @@ static const MemoryRegionOps nvic_sysreg_ops = {
>>      .endianness = DEVICE_NATIVE_ENDIAN,
>>  };
>>
>> +static
>> +int nvic_post_load(void *opaque, int version_id)
>> +{
>> +    nvic_state *s = opaque;
>> +    unsigned i;
>> +
>> +    /* evil hack to get ARMCPU* ahead of time */
>> +    assert(cpus.tqh_first);
>> +    assert(!CPU_NEXT(cpus.tqh_first));
>> +    s->cpu = ARM_CPU(cpus.tqh_first);
>> +    assert(s->cpu);
> 
> Why do we need to do this? By the time we get to loading
> state into the system, s->cpu should already have been set.

Just so, removed.

>> +
>> +    /* recalculate priorities */
>> +    for (i = 4; i < s->num_irq; i++) {
>> +        set_prio(s, i, s->vectors[i].raw_prio);
>> +    }
>> +
>> +    nvic_irq_update(s, highest_runnable_prio(s->cpu));
>> +
>> +    return 0;
>> +}
>> +
>> +static
>> +int vec_info_get(QEMUFile *f, void *pv, size_t size)
>> +{
>> +    vec_info *I = pv;
>> +    I->prio_sub = qemu_get_be16(f);
>> +    I->prio_group = qemu_get_byte(f);
>> +    I->raw_prio = qemu_get_ubyte(f);
>> +    I->enabled = qemu_get_ubyte(f);
>> +    I->pending = qemu_get_ubyte(f);
>> +    I->active = qemu_get_ubyte(f);
>> +    I->level = qemu_get_ubyte(f);
>> +    return 0;
>> +}
>> +
>> +static
>> +void vec_info_put(QEMUFile *f, void *pv, size_t size)
>> +{
>> +    vec_info *I = pv;
>> +    qemu_put_be16(f, I->prio_sub);
>> +    qemu_put_byte(f, I->prio_group);
>> +    qemu_put_ubyte(f, I->raw_prio);
>> +    qemu_put_ubyte(f, I->enabled);
>> +    qemu_put_ubyte(f, I->pending);
>> +    qemu_put_ubyte(f, I->active);
>> +    qemu_put_ubyte(f, I->level);
>> +}
>> +
>> +static const VMStateInfo vmstate_info = {
>> +    .name = "armv7m_nvic_info",
>> +    .get = vec_info_get,
>> +    .put = vec_info_put,
>> +};
> 
> I don't think there's any need to use get and put functions here:
> better to define a VMStateDescripton for the struct vec_info,
> and then you can just refer to that from the main vmstate_nvic
> description. (hw/audio/pl041.c has some examples of this.)

Fixed

>> +
>>  static const VMStateDescription vmstate_nvic = {
>>      .name = "armv7m_nvic",
>> -    .version_id = 1,
>> -    .minimum_version_id = 1,
>> +    .version_id = 2,
>> +    .minimum_version_id = 2,
>> +    .post_load = &nvic_post_load,
>>      .fields = (VMStateField[]) {
>> +        VMSTATE_ARRAY(vectors, nvic_state, NVIC_MAX_VECTORS, 0,
>> +                      vmstate_info, vec_info),
>> +        VMSTATE_UINT8(prigroup, nvic_state),
>>          VMSTATE_UINT32(systick.control, nvic_state),
>>          VMSTATE_UINT32(systick.reload, nvic_state),
>>          VMSTATE_INT64(systick.tick, nvic_state),
>>          VMSTATE_TIMER_PTR(systick.timer, nvic_state),
>> +        VMSTATE_UINT32(num_irq, nvic_state),
>>          VMSTATE_END_OF_LIST()
>>      }
>>  };
> 
> Ideally the VMState should be added in the same patches that
> add new fields or structures, rather than in its own patch
> at the end.

I'll try to do this.

Re: [Qemu-devel] [PATCH 11/18] armv7m: fix I and F flag handling

[Michael Davidsaver]

On 11/20/2015 08:47 AM, Peter Maydell wrote:
> On 9 November 2015 at 01:11, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
>> Despite having the same notation, these bits
>> have completely different meaning than -AR.
>>
>> Add armv7m_excp_unmasked()
>> to calculate the currently runable exception priority
>> taking into account masks and active handlers.
>> Use this in conjunction with the pending exception
>> priority to determine if the pending exception
>> can interrupt execution.
> 
> This function is used by code added in earlier patches in
> this series, so this patch needs to be moved earlier in the
> series, or those patches won't compile.

Should be fixed.

>> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
>> ---
>>  target-arm/cpu.c | 26 +++++++-------------------
>>  target-arm/cpu.h | 27 ++++++++++++++++++++++++++-
>>  2 files changed, 33 insertions(+), 20 deletions(-)
>>
>> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
>> index be026bc..5d03117 100644
>> --- a/target-arm/cpu.c
>> +++ b/target-arm/cpu.c
>> @@ -173,6 +173,8 @@ static void arm_cpu_reset(CPUState *s)
>>          uint32_t initial_pc; /* Loaded from 0x4 */
>>          uint8_t *rom;
>>
>> +        env->v7m.exception_prio = env->v7m.pending_prio = 0x100;
>> +
>>          env->daif &= ~PSTATE_I;
>>          rom = rom_ptr(0);
>>          if (rom) {
>> @@ -301,29 +303,15 @@ static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>>  {
>>      CPUClass *cc = CPU_GET_CLASS(cs);
>>      ARMCPU *cpu = ARM_CPU(cs);
>> -    CPUARMState *env = &cpu->env;
>>      bool ret = false;
>>
>> -
>> -    if (interrupt_request & CPU_INTERRUPT_FIQ
>> -        && !(env->daif & PSTATE_F)) {
>> -        cs->exception_index = EXCP_FIQ;
>> -        cc->do_interrupt(cs);
>> -        ret = true;
>> -    }
>> -    /* 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 when the
>> -     * CPU tries to execute code at the magic address.
>> -     * This will cause the magic PC value to be pushed to
>> -     * the stack if an interrupt occurred at the wrong time.
>> -     * We avoid this by disabling interrupts when
>> -     * pc contains a magic address.
> 
> This (removing this comment and the checks for the magic address)
> seem to be part of a separate change [probably the one in
> "armv7m: Undo armv7m.hack"] and shouldn't be in this patch.

Relocated.

>> +    /* ARMv7-M interrupt masking works differently than -A or -R.
>> +     * There is no FIQ/IRQ distinction.
>> +     * Instead of masking interrupt sources, the I and F bits
>> +     * (along with basepri) mask certain exception priority levels.
>>       */
>>      if (interrupt_request & CPU_INTERRUPT_HARD
>> -        && !(env->daif & PSTATE_I)
>> -        && (env->regs[15] < 0xfffffff0)) {
>> +            && (armv7m_excp_unmasked(cpu) >= cpu->env.v7m.pending_prio)) {
>>          cs->exception_index = EXCP_IRQ;
>>          cc->do_interrupt(cs);
>>          ret = true;
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index c193fbb..29d89ce 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -1033,9 +1033,34 @@ void arm_cpu_list(FILE *f, fprintf_function cpu_fprintf);
>>  uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx,
>>                                   uint32_t cur_el, bool secure);
>>
>> +/* @returns highest (numerically lowest) unmasked exception priority
>> + */
>> +static inline
>> +int armv7m_excp_unmasked(ARMCPU *cpu)
> 
> What this is really calculating is the current execution
> priority (running priority) of the CPU, so I think a better
> name would be armv7m_current_exec_priority() or
> armv7m_current_priority() or armv7m_running_priority() or similar.

Now armv7m_excp_running_prio()

>> +{
>> +    CPUARMState *env = &cpu->env;
>> +    int runnable;
>> +
>> +    /* find highest (numerically lowest) priority which could
>> +     * run based on masks
>> +     */
>> +    if (env->daif&PSTATE_F) { /* FAULTMASK */
> 
> Style issue -- operands should have spaces around them.
> 
>> +        runnable = -2;
> 
> These all seem to be off by one: FAULTMASK sets the
> running priority to -1, not -2, PRIMASK sets it to 0,
> not -1, and so on.

The off by one was due to my confusing runnable vs. running distinction, now gone.

>> +    } else if (env->daif&PSTATE_I) { /* PRIMASK */
>> +        runnable = -1;
>> +    } else if (env->v7m.basepri > 0) {
>> +        /* BASEPRI==1 -> runnable==-1 (same as PRIMASK==1) */
> 
> (applies to operands in comments too)
> 
>> +        runnable = env->v7m.basepri-2;
> 
> Where is this - 2 from? Also, BASEPRI values honour the
> PRIGROUP setting. (Compare the ExecutionPriority pseudocode).

The off by two for BASEPRI was my mis-reading the definition.

>> +    } else {
>> +        runnable = 0x100; /* lower than any possible priority */
>> +    }
>> +    /* consider priority of active handler */
>> +    return MIN(runnable, env->v7m.exception_prio-1);
> 
> I don't think this -1 should be here.

It is gone.

>> +}
>> +
>>  /* Interface between CPU and Interrupt controller.  */
>>  void armv7m_nvic_set_pending(void *opaque, int irq);
>> -int armv7m_nvic_acknowledge_irq(void *opaque);
>> +void armv7m_nvic_acknowledge_irq(void *opaque);
>>  void armv7m_nvic_complete_irq(void *opaque, int irq);
>>
>>  /* Interface for defining coprocessor registers.
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH 06/18] armv7m: new NVIC utility functions

[Peter Maydell]

On 2 December 2015 at 23:18, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> On 11/20/2015 08:25 AM, Peter Maydell wrote:
>> Hi; I have a lot of review comments on this patch set, but that's
>> really because v7M exception logic is pretty complicated and
>> our current code is a long way away from correct. You might
>> find it helpful to separate out "restructure the NVIC code
>> into its own device" into separate patches from "and now add
>> functionality like exception escalation", perhaps.
>
> I'd certainly find this helpful :) I'm just not sure how to accomplish
> this and still make every patch compile.

The idea would be to have a patch which is just moving (copying)
the old code into the new code structure, so you get the old behaviour
but in the new device. Then you have patches which change the old
behaviour to the desired new behaviour.

>>> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
>>> ---
>>>  hw/intc/armv7m_nvic.c | 250 +++++++++++++++++++++++++++++++++++++++++++++++---
>>>  1 file changed, 235 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
>>> index 487a09a..ebb4d4e 100644
>>> --- a/hw/intc/armv7m_nvic.c
>>> +++ b/hw/intc/armv7m_nvic.c
>>> @@ -140,36 +140,256 @@ static void systick_reset(nvic_state *s)
>>>      timer_del(s->systick.timer);
>>>  }
>>>
>>> -/* The external routines use the hardware vector numbering, ie. the first
>>> -   IRQ is #16.  The internal GIC routines use #32 as the first IRQ.  */
>>> +/* caller must call nvic_irq_update() after this */
>>> +static
>>> +void set_prio(nvic_state *s, unsigned irq, uint8_t prio)
>>> +{
>>> +    unsigned submask = (1<<(s->prigroup+1))-1;
>>> +
>>> +    assert(irq > 3); /* only use for configurable prios */
>>> +    assert(irq < NVIC_MAX_VECTORS);
>>> +
>>> +    s->vectors[irq].raw_prio = prio;
>>> +    s->vectors[irq].prio_group = (prio>>(s->prigroup+1));
>>> +    s->vectors[irq].prio_sub = irq + (prio&submask)*NVIC_MAX_VECTORS;
>>
>> Precalculating the group priority seems a bit odd, since it
>> will change later anyway if the guest writes to PRIGROUP.
>
> I've kept the pre-calculation as the alternative comparison code is
> no simpler when tie breaking with exception number is done.

Precalculation means you end up either (a) migrating state which
isn't really device state, or (b) having to re-calculate the
precalculations on migration load. (a) isn't really a good idea
since it turns internal details of the implementation into ABI
we might eventually have to support for backward compatibility
of migrations, and (b) is extra code.

>>> +
>>> +    DPRINTF(0, "Set %u priority grp %d sub %u\n", irq,
>>> +            s->vectors[irq].prio_group, s->vectors[irq].prio_sub);
>>> +}
>>> +
>>> +/* recompute highest pending */
>>> +static
>>> +void nvic_irq_update(nvic_state *s, int update_active)
>>> +{
>>> +    unsigned i;
>>> +    int lvl;
>>> +    CPUARMState *env = &s->cpu->env;
>>> +    int16_t act_group = 0x100, pend_group = 0x100;
>>> +    uint16_t act_sub = 0, pend_sub = 0;
>>> +    uint16_t act_irq = 0, pend_irq = 0;
>>> +
>>> +    /* find highest priority */
>>> +    for (i = 1; i < s->num_irq; i++) {
>>> +        vec_info *I = &s->vectors[i];
>>
>> Minor style issue: we prefer lower case for variable names,
>> and CamelCase for structure type names (see CODING_STYLE).
>> So this would be VecInfo *vec; or something similar.
>
> Done.
>
>>> +
>>> +        DPRINTF(2, " VECT %d %d:%u\n", i, I->prio_group, I->prio_sub);
>>> +
>>> +        if (I->active && ((I->prio_group < act_group)
>>> +                || (I->prio_group == act_group && I->prio_sub < act_sub)))
>>
>> Because the group priority and sub priority are just two contiguous
>> parts of the raw priority, you get the same effect here by just
>> doing a comparison on the raw value: "I->raw_prio <  act_raw_prio".
>
> Not quite since it's possible that I->raw_prio == act_raw_prio.  As
> I read the ARM this situation should be avoided by using the exception
> number to break the tie.  This way no two priorities are ever equal.
>  This is the reason that act_sub is "irq + (prio&submask)*NVIC_MAX_VECTORS".

You should use the exception number to break the tie, yes, but
I don't see why that requires you to encode the irq number into
anything, because you can arrange that you get that behaviour
by coding the loop suitably. The code I had isn't quite right,
though...

>> Note however that the running priority is actually only the
>> group priority part (see the pseudocode ExecutionPriority function)
>> and so you want something more like:
>>
>>     highestpri = 0x100;
>>     for (each vector) {
>>         if (vector->active && vector->raw_prio < highestpri) {
>>             highestpri = vector->raw_prio & prigroup_mask;
>>             act_sub = vector->raw_prio & ~prigroup_mask; // if you need it
>>         }
>>     }

...try:

   highestpri = current_running_priority;
   /* find highest priority active vector; this will correctly
    * handle group and subgroup priority fields, and in the case
    * of a tie we'll get the vector with the lowest exception
    * number, because the comparison is '<' and we start from 0.
    */
   for (each vector starting from 0) {
       if (vector->active && vector->raw_prio < highestpri) {
           highestpri = vector->raw_prio;
       }
   }
   if ((highestpri & prigroup_mask) < current_running_priority) {
       /* this interrupt should actually preempt */
   }

(I think we also have similar loop-through-all-interrupts logic in
the "return current running priority" function, though -- we should
only do the calculation in one place if we can.)

>>> +        unsigned escalate = 0;
>>> +        if (I->active) {
>>> +            /* trying to pend an active fault (possibly nested).
>>> +             * eg. UsageFault in UsageFault handler
>>> +             */
>>> +            escalate = 1;
>>> +            DPRINTF(0, " Escalate, active\n");
>>
>> You don't need to explicitly check for this case, because it
>> will be caught by the "is the running priority too high" check
>> (the current running priority must be at least as high as the
>> priority of an active fault handler). If you look at the ARM ARM
>> section on priority escalation you'll see that "undef in a
>> UsageFault handler" is listed in the "examples of things that
>> cause priority escalation" bullet list, not the "definition of
>> what causes escalation" list.
>
> Good point.  On re-reading this I think I understand better how
> priority changes take effect (immediately).  I've move this test
> last and made it a hw_error() since it should only be hit if
> some bug results in inconsistency between the NVIC and ARMCPU
> priorities fields.

It can happen for synchronous exceptions, like the example
case of UsageFault in a UsageFault handler (eg executing an
undefined insn in the handler).

>>> +            escalate = 1;
>>> +        }
>>> +
>>> +        if (escalate) {
>>> +#ifdef DEBUG_NVIC
>>> +            int oldirq = irq;
>>> +#endif
>>> +            if (runnable < -1) {
>>> +                /* TODO: actual unrecoverable exception actions */
>>> +                cpu_abort(&s->cpu->parent_obj,
>>> +                          "%d in %d escalates to unrecoverable exception\n",
>>> +                          irq, active);
>>> +            } else {
>>> +                irq = ARMV7M_EXCP_HARD;
>>> +            }
>>> +            I = &s->vectors[irq];
>>> +
>>> +            DPRINTF(0, "Escalate %d to %d\n", oldirq, irq);
>>
>> Faults escalated to HardFault retain the ReturnAddress
>> behaviour of the original fault (ie what return-address
>> is saved to the stack as part of the exception stacking and
>> so whether we resume execution on the faulting insn or the
>> next one). So it's not sufficient to just say "treat it
>> exactly as if it were a HardFault" like this.
>
> I'm confused.  From my testing it seems like the PC has already
> been adjusted by this point and no further changes are needed.
> Am I missing something?

You're right -- for arm_v7m_cpu_do_interrupt() we expect the
PC to have already been adjusted as required (this is unlike
the A/R profile do_interrupt function which does the adjustment
of the LR according to type of exception, which is why I thought
there was a problem.)

In that case we should just have a comment here that says
that since env->regs[15] has already been adjusted appropriately
for the original fault type we can just change the irq number
here and still get the correct ReturnAddress behaviour for
the original fault.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 00/18] Fix exception handling and msr/mrs access

[Peter Maydell]

On 9 November 2015 at 01:11, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> This series grew from a previous incorrect patch attempting to fix some
> incorrect behavior.  After spending some time going through the arch. ref.
> manual for v7-M I think I understand better how this should work and have
> made a number of changes which actually improve the situation.

Thanks for sending out v2. I've now reviewed all the patches in the
set up to the start of the MPU related ones. I think it's probably
better if we work on getting the NVIC related patches into a mergeable
state and then come back to the MPU as a second series -- the patchset
is already quite large.

thanks
-- PMM