[Qemu-devel] [PATCH v2 00/26] armv7m: exception handling, MPU, and more

[Qemu-devel] [PATCH v2 00/26] armv7m: exception handling, MPU, and more

[Michael Davidsaver]

All,

Second revision of ARMv7-M exception handling patchset, which now adds MPU support (as well as can be done).

Parts of this series are informed by the previous work of Alex Zuepke.

This time I have access to a EK-TM4C1294XL eval board (cortex-m4f), and have done some cross-checks using test programs of my own[1].  There are differences in some corner cases, but mostly agreement (certainly better than before).

This series attempts to fix the exception masking and priority escalation behavour, and in the process removes the dependence of the NVIC on the shared GIC code.  It also fills out some of the missing fault reporting registers.

The CCR register is also added, mainly to implement the STACKALIGN bit.  I've also made an attempt on USERSETMPEND.  BFHFNMIGN, DIV_0_TRP, UNALIGN_TRP remain unimplemnted.

Additional checks are added to do_v7m_exception_exit() to capture the faults which should arise when the guest stack gets out of sync with the CPU's state.

In cross-checking with a real device I also found that the behavour of CONTROL bit 1 wasn't as spec'd in exception handlers.

I also "discovered" that many v7-M targets don't implement all 8 bits of the priority registers (the TM4C1294 only has 3).  So I've added a Property to allow this to be set appropriately.

This led me to make some changes to allow board code to actually set this Property, which entailed splitting armv7m_init() in two parts, and associated changes to the stellaris and stm32f205 boards.

I'm not completely happy with the level of compatibility with the MPU.  For my own purposes it's good enough as I can make special builds.  However, the additional alignment and size restrictions imposed by the common TLB and 1024 page size will probably break most unmodified guests using the MPU.  I can't see any way around this short of changes to the TLB code, or a seperate build with TARGET_PAGE_BITS==5.  I'm not inclined to undertake either.

Should this part of the series be dropped?

Michael


[1] https://github.com/mdavidsaver/baremetal/tree/qemutest


Michael Davidsaver (26):
  armv7m: MRS/MSR handle unprivileged access
  armv7m: Undo armv7m.hack
  armv7m: Explicit error for bad vector table
  armv7m: additional cpu state for exception handling
  armv7m: add armv7m_excp_running_prio()
  armv7m: fix I and F flag handling
  armv7m: simpler/faster exception start
  armv7m: rewrite NVIC
  armv7m: implement CFSR, HFSR, BFAR, and MMFAR
  armv7m: auto-clear FAULTMASK
  arm: gic: Remove references to NVIC
  armv7m: check exception return consistency
  armv7m: implement CCR
  armv7m: prevent unprivileged write to STIR
  armv7m: add MPU to cortex-m3 and cortex-m4
  armv7m: add some mpu debugging prints
  armv7m: mpu background miss is perm fault
  armv7m: update base region policy
  armv7m: mpu not allowed to map exception return codes
  armv7m: observable initial register state
  armv7m: CONTROL<1> handling
  armv7m: priority field mask
  qom: add cpu_generic_init_unrealized()
  armv7m: split armv7m_init in two parts
  armv7m: remove extra cpu_reset()
  armv7m: decide whether faults are MemManage or BusFault

 hw/arm/armv7m.c          |   60 ++-
 hw/arm/stellaris.c       |    7 +-
 hw/arm/stm32f205_soc.c   |    6 +-
 hw/intc/arm_gic.c        |   14 +-
 hw/intc/arm_gic_common.c |   23 +-
 hw/intc/armv7m_nvic.c    | 1037 +++++++++++++++++++++++++++++++++++++---------
 hw/intc/gic_internal.h   |    7 +-
 include/hw/arm/arm.h     |    4 +-
 include/qom/cpu.h        |   12 +
 qom/cpu.c                |   23 +-
 target-arm/cpu-qom.h     |    6 +
 target-arm/cpu.c         |   61 ++-
 target-arm/cpu.h         |   17 +-
 target-arm/helper.c      |  346 ++++++++++++----
 target-arm/machine.c     |   11 +-
 15 files changed, 1252 insertions(+), 382 deletions(-)

-- 
2.1.4

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

[Michael Davidsaver]

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

Prevent privlaged from changing writing EPSR fields.
Access to unknown/undefined special registers not
fatal (read 0, write ignored) w/ guest error message.
---
 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 afc4163..2c631e3 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -7375,23 +7375,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 */
@@ -7403,40 +7412,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)
@@ -7477,8 +7472,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

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

[Peter Maydell]

On 3 December 2015 at 00:18, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> The MRS and MSR instruction handling isn't checking
> the current permission level.
>
> Prevent privlaged from changing writing EPSR fields.
> Access to unknown/undefined special registers not
> fatal (read 0, write ignored) w/ guest error message.

This patch and all the others in this series seem to be
missing your Signed-off-by: line.

(scripts/checkpatch.pl will catch this, among other things.)

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

When you send out this patch in future versions of this series,
please include my Reviewed-by: tag in the commit message
after your signed-off-by line; that way I know I don't have
to recheck it. (If a patch changes much after somebody's
reviewed it you should drop the reviewed-by line and say why
in the cover letter.)

thanks
-- PMM

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

[Peter Maydell]

On 17 December 2015 at 13:10, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 3 December 2015 at 00:18, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
>> The MRS and MSR instruction handling isn't checking
>> the current permission level.
>>
>> Prevent privlaged from changing writing EPSR fields.
>> Access to unknown/undefined special registers not
>> fatal (read 0, write ignored) w/ guest error message.
>
> This patch and all the others in this series seem to be
> missing your Signed-off-by: line.

Michael has confirmed to me that this was just an oversight.
Cleanup of the NVIC has finally reached the top of my todo
list, so I'm planning to take this patchset and finish
the process of fixing it up to go into master.

thanks
-- PMM

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

[Michael Davidsaver]

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

On 01/12/2017 09:14 AM, Peter Maydell wrote:
> ...
> Michael has confirmed to me that this was just an oversight.

This was me not knowing the process.  (just to confirm this publicly)



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Michael Davidsaver]

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

The unassigned handler will signal other faults as either
prefetch or data exceptions, with the FSR code 0x8 to
distinguish them from memory translation faults (0xd).
Future code will make use of this distinction when
deciding to raise BusFault or MemManage exceptions.
---
 hw/arm/armv7m.c  |  8 --------
 target-arm/cpu.c | 32 +++++++++++++++++++++-----------
 2 files changed, 21 insertions(+), 19 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..728854f 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -280,6 +280,25 @@ 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;
+
+    env->exception.vaddress = addr;
+    env->exception.fsr = is_write ? 0x808 : 0x8; /* Precise External Abort */
+    if (env->v7m.exception != 0 && addr >= 0xfffffff0 && !is_write) {
+        cpu->exception_index = EXCP_EXCEPTION_EXIT;
+    } else if (is_exec) {
+        cpu->exception_index = EXCP_PREFETCH_ABORT;
+    } else {
+        cpu->exception_index = EXCP_DATA_ABORT;
+    }
+    cpu_loop_exit(cpu);
+}
+
 static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 {
     CPUClass *cc = CPU_GET_CLASS(cs);
@@ -294,19 +313,9 @@ static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
         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.
-     */
     if (interrupt_request & CPU_INTERRUPT_HARD
         && !(env->daif & PSTATE_I)
-        && (env->regs[15] < 0xfffffff0)) {
+            ) {
         cs->exception_index = EXCP_IRQ;
         cc->do_interrupt(cs);
         ret = true;
@@ -909,6 +918,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

Re: [Qemu-devel] [PATCH v2 02/26] armv7m: Undo armv7m.hack

[Peter Maydell]

On 3 December 2015 at 00:18, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> Add CPU unassigned access handler in place of special
> MemoryRegion to catch exception returns.
>
> The unassigned handler will signal other faults as either
> prefetch or data exceptions, with the FSR code 0x8 to
> distinguish them from memory translation faults (0xd).
> Future code will make use of this distinction when
> deciding to raise BusFault or MemManage exceptions.

This patch breaks my Stellaris test image -- instead of starting
it just sits there with a black screen.

I've put a copy of that test image up at
  http://people.linaro.org/~peter.maydell/stellaris.tgz
You can run it with path/to/stellaris/runme path/to/qemu-system-arm .

> ---
>  hw/arm/armv7m.c  |  8 --------
>  target-arm/cpu.c | 32 +++++++++++++++++++++-----------
>  2 files changed, 21 insertions(+), 19 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..728854f 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -280,6 +280,25 @@ 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;
> +
> +    env->exception.vaddress = addr;
> +    env->exception.fsr = is_write ? 0x808 : 0x8; /* Precise External Abort */
> +    if (env->v7m.exception != 0 && addr >= 0xfffffff0 && !is_write) {
> +        cpu->exception_index = EXCP_EXCEPTION_EXIT;

We could use a comment here (a) explaining what we're doing and (b)
mentioning that this isn't architecturally correct -- ideally we should
catch these exception exits on execution of the jump insn, not by
letting the jump execute and then trapping when we actually try to
execute at the magic addresses.

> +    } else if (is_exec) {
> +        cpu->exception_index = EXCP_PREFETCH_ABORT;
> +    } else {
> +        cpu->exception_index = EXCP_DATA_ABORT;
> +    }
> +    cpu_loop_exit(cpu);
> +}
> +
>  static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cs);
> @@ -294,19 +313,9 @@ static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>          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.
> -     */
>      if (interrupt_request & CPU_INTERRUPT_HARD
>          && !(env->daif & PSTATE_I)
> -        && (env->regs[15] < 0xfffffff0)) {
> +            ) {

Can we really drop this change? The thing it's guarding against
(interrupt comes in while the PC is this not-really-an-address
value) can still happen whether we catch the attempt to execute
in translate.c or via the unassigned-access hook.

(This isn't what's causing my test image to not run, though.)

>          cs->exception_index = EXCP_IRQ;
>          cc->do_interrupt(cs);
>          ret = true;
> @@ -909,6 +918,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;
>  }

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 02/26] armv7m: Undo armv7m.hack

[Michael Davidsaver]

On 12/17/2015 10:38 AM, Peter Maydell wrote:
> On 3 December 2015 at 00:18, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
>> Add CPU unassigned access handler in place of special
>> MemoryRegion to catch exception returns.
>>
>> The unassigned handler will signal other faults as either
>> prefetch or data exceptions, with the FSR code 0x8 to
>> distinguish them from memory translation faults (0xd).
>> Future code will make use of this distinction when
>> deciding to raise BusFault or MemManage exceptions.
> This patch breaks my Stellaris test image -- instead of starting
> it just sits there with a black screen.
>
> I've put a copy of that test image up at
>   http://people.linaro.org/~peter.maydell/stellaris.tgz
> You can run it with path/to/stellaris/runme path/to/qemu-system-arm .

There were several issues.  Two bugs (wrong IRQ enabled and systick not
enabled) and a "feature" (access to unimplemented registers for a PWM
controller is now a BusFault).

As a workaround for the "feature" I add a low priority MemoryRegion from
0x40000000 -> 0x40ffffff which completes all reads with zero and logs. 
Please advise on how this should be handled.

With these changes both test programs appear to run correctly, although
the http server example has painfully slow load times and seems to hit
an out of memory condition if I look at it wrong.  Is this expected? 
(and the blub on the buttons page about "xml technology" is priceless)

I can see that the http server example spends some time attempting MII
operations on the NIC.  As these aren't modeled it spins and eventually
gives up.

> ...
> We could use a comment here (a) explaining what we're doing and (b)
> mentioning that this isn't architecturally correct -- ideally we
> should catch these exception exits on execution of the jump insn, not
> by letting the jump execute and then trapping when we actually try to
> execute at the magic addresses. 

Will do.

>> ...
>> @@ -294,19 +313,9 @@ static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>>          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.
>> -     */
>>      if (interrupt_request & CPU_INTERRUPT_HARD
>>          && !(env->daif & PSTATE_I)
>> -        && (env->regs[15] < 0xfffffff0)) {
>> +            ) {
> Can we really drop this change? The thing it's guarding against
> (interrupt comes in while the PC is this not-really-an-address
> value) can still happen whether we catch the attempt to execute
> in translate.c or via the unassigned-access hook.

I don't think the M-profile case in gen_intermediate_code() in
translate.c can ever be reached without first hitting the unassigned
memory handler.  Before the code can be translated, the page containing
it must be loaded.  Such loads no longer succeed.

Put more literally, gen_intermediate_code() is only called from
tb_gen_code() where it comes after a call to get_page_addr_code(),
wherein the unassigned handler calls cpu_loop_exit().

I've replaced the M case for EXCP_EXCEPTION_EXIT in
gen_intermediate_code() with an assert.  So far it hasn't failed.

Re: [Qemu-devel] [PATCH v2 02/26] armv7m: Undo armv7m.hack

[Peter Maydell]

On 27 December 2015 at 20:22, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> On 12/17/2015 10:38 AM, Peter Maydell wrote:
>> On 3 December 2015 at 00:18, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
>>> Add CPU unassigned access handler in place of special
>>> MemoryRegion to catch exception returns.
>>>
>>> The unassigned handler will signal other faults as either
>>> prefetch or data exceptions, with the FSR code 0x8 to
>>> distinguish them from memory translation faults (0xd).
>>> Future code will make use of this distinction when
>>> deciding to raise BusFault or MemManage exceptions.
>> This patch breaks my Stellaris test image -- instead of starting
>> it just sits there with a black screen.
>>
>> I've put a copy of that test image up at
>>   http://people.linaro.org/~peter.maydell/stellaris.tgz
>> You can run it with path/to/stellaris/runme path/to/qemu-system-arm .
>
> There were several issues.  Two bugs (wrong IRQ enabled and systick not
> enabled) and a "feature" (access to unimplemented registers for a PWM
> controller is now a BusFault).
>
> As a workaround for the "feature" I add a low priority MemoryRegion from
> 0x40000000 -> 0x40ffffff which completes all reads with zero and logs.
> Please advise on how this should be handled.

We should probably at least identify what particular devices are
supposed to be here and put in dummy versions, rather than just
having a single memory region which does RAZ/WI.

> With these changes both test programs appear to run correctly, although
> the http server example has painfully slow load times and seems to hit
> an out of memory condition if I look at it wrong.  Is this expected?
> (and the blub on the buttons page about "xml technology" is priceless)

I don't run the HTTP example often. The basic requirement is
"should not get any worse as a result of the patchset". Problems
that were already there before need not be addressed.

>>> @@ -294,19 +313,9 @@ static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>>>          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.
>>> -     */
>>>      if (interrupt_request & CPU_INTERRUPT_HARD
>>>          && !(env->daif & PSTATE_I)
>>> -        && (env->regs[15] < 0xfffffff0)) {
>>> +            ) {
>> Can we really drop this change? The thing it's guarding against
>> (interrupt comes in while the PC is this not-really-an-address
>> value) can still happen whether we catch the attempt to execute
>> in translate.c or via the unassigned-access hook.
>
> I don't think the M-profile case in gen_intermediate_code() in
> translate.c can ever be reached without first hitting the unassigned
> memory handler.  Before the code can be translated, the page containing
> it must be loaded.  Such loads no longer succeed.

Yes, but the code you've deleted here may be called after
we have set the PC to a magic value but before we have tried
to do the address load for it:
 1 translation block A (with the pop or ldm) sets PC to
   0xfffffffx, and execution leaves this TB and returns to
   the top level loop
 2 normally, we would then try to execute at the magic address,
   which would result in our trying to translate a TB for that
   address, which immediately causes us to run the code in the
   unassigned-access hook. That will cause the PC to be set
   to the appropriate value for having returned from the
   interrupt handler
 3 however, it is possible that an interrupt has been raised
   which means that between steps 1 and 2 we will say "actually,
   need to take an interrupt now". Since between steps 1 and 2
   the value in env->regs[15] is the magic 0xfffffffx value,
   we will end up stacking the magic value as part of the
   interrupt entry process. This is wrong, and the reason for
   the condition above is to avoid this problem.

Changing the handling of "PC == magic value" from translate.c
to the unassigned-access hook does not close the window where
env->regs[15] is a value the guest should not see as an
interrupted PC.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 02/26] armv7m: Undo armv7m.hack

[Michael Davidsaver]

On 12/17/2015 10:38 AM, Peter Maydell wrote:
> We could use a comment here (a) explaining what we're doing and (b)
> mentioning that this isn't architecturally correct -- ideally we should
> catch these exception exits on execution of the jump insn, not by
> letting the jump execute and then trapping when we actually try to
> execute at the magic addresses.

I had an instructive little digression to investigate doing things the
"right way" (in tcg).  I can see how it would be done by adding a
conditional every time the PC could be updated.  To me the unassigned
handler trick/hack seems simpler (less likely to add a bug) and avoids
emitting more code for every ldm/pop instruction.

Re: [Qemu-devel] [PATCH v2 02/26] armv7m: Undo armv7m.hack

[Peter Maydell]

On 28 December 2015 at 01:55, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> On 12/17/2015 10:38 AM, Peter Maydell wrote:
>> We could use a comment here (a) explaining what we're doing and (b)
>> mentioning that this isn't architecturally correct -- ideally we should
>> catch these exception exits on execution of the jump insn, not by
>> letting the jump execute and then trapping when we actually try to
>> execute at the magic addresses.
>
> I had an instructive little digression to investigate doing things the
> "right way" (in tcg).  I can see how it would be done by adding a
> conditional every time the PC could be updated.  To me the unassigned
> handler trick/hack seems simpler (less likely to add a bug) and avoids
> emitting more code for every ldm/pop instruction.

Yes, it's faster, which is why we do it this way. It is however
not what the hardware does (in a way which is visible to guest code
which is specifically looking for the difference), which is why it's
worth commenting on.

thanks
-- PMM

[Qemu-devel] [PATCH v2 03/26] 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.
---
 target-arm/helper.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 2c631e3..7b76f32 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -5414,6 +5414,25 @@ static void do_v7m_exception_exit(CPUARMState *env)
        pointer.  */
 }
 
+static
+uint32_t arm_v7m_load_vector(ARMCPU *cpu)
+
+{
+    CPUState *cs = &cpu->parent_obj;
+    CPUARMState *env = &cpu->env;
+    MemTxResult result;
+    hwaddr vec = env->v7m.vecbase + env->v7m.exception * 4;
+    uint32_t addr;
+
+    addr = address_space_ldl(cs->as, vec,
+                             MEMTXATTRS_UNSPECIFIED, &result);
+    if (result != MEMTX_OK) {
+        cpu_abort(cs, "Failed to read from exception vector table "
+                  "entry %08x\n", (unsigned)vec);
+    }
+    return addr;
+}
+
 void arm_v7m_cpu_do_interrupt(CPUState *cs)
 {
     ARMCPU *cpu = ARM_CPU(cs);
@@ -5495,7 +5514,7 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
     /* Clear IT bits */
     env->condexec_bits = 0;
     env->regs[14] = lr;
-    addr = ldl_phys(cs->as, env->v7m.vecbase + env->v7m.exception * 4);
+    addr = arm_v7m_load_vector(cpu);
     env->regs[15] = addr & 0xfffffffe;
     env->thumb = addr & 1;
 }
-- 
2.1.4

Re: [Qemu-devel] [PATCH v2 03/26] armv7m: Explicit error for bad vector table

[Peter Maydell]

On 3 December 2015 at 00:18, 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.
> ---
>  target-arm/helper.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 2c631e3..7b76f32 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -5414,6 +5414,25 @@ static void do_v7m_exception_exit(CPUARMState *env)
>         pointer.  */
>  }
>
> +static
> +uint32_t arm_v7m_load_vector(ARMCPU *cpu)
> +
> +{
> +    CPUState *cs = &cpu->parent_obj;

This isn't the right way to cast to the base class of a QOM object.
You want:
   CPUState *cs = CPU(cpu);

> +    CPUARMState *env = &cpu->env;
> +    MemTxResult result;
> +    hwaddr vec = env->v7m.vecbase + env->v7m.exception * 4;
> +    uint32_t addr;
> +
> +    addr = address_space_ldl(cs->as, vec,
> +                             MEMTXATTRS_UNSPECIFIED, &result);
> +    if (result != MEMTX_OK) {

We could use a comment here:
   /* Architecturally this should cause a HardFault setting HSFR.VECTTBL,
    * which would then be immediately followed by our failing to load
    * the entry vector for that HardFault, which is a Lockup case.
    * Since we don't model Lockup, we just report this guest error
    * via cpu_abort().
    */

> +        cpu_abort(cs, "Failed to read from exception vector table "
> +                  "entry %08x\n", (unsigned)vec);
> +    }
> +    return addr;
> +}
> +
>  void arm_v7m_cpu_do_interrupt(CPUState *cs)
>  {
>      ARMCPU *cpu = ARM_CPU(cs);
> @@ -5495,7 +5514,7 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>      /* Clear IT bits */
>      env->condexec_bits = 0;
>      env->regs[14] = lr;
> -    addr = ldl_phys(cs->as, env->v7m.vecbase + env->v7m.exception * 4);
> +    addr = arm_v7m_load_vector(cpu);
>      env->regs[15] = addr & 0xfffffffe;
>      env->thumb = addr & 1;
>  }

The rest of this patch looks OK though.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 03/26] armv7m: Explicit error for bad vector table

[Michael Davidsaver]

On 12/17/2015 08:25 AM, Peter Maydell wrote:
> On 3 December 2015 at 00:18, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
>> ...
>> +static
>> +uint32_t arm_v7m_load_vector(ARMCPU *cpu)
>> +
>> +{
>> +    CPUState *cs = &cpu->parent_obj;
> This isn't the right way to cast to the base class of a QOM object.
> You want:
>    CPUState *cs = CPU(cpu);

from cpu.h

> /* Since this macro is used a lot in hot code paths and in conjunction
> with
>  * FooCPU *foo_env_get_cpu(), we deviate from usual QOM practice by using
>  * an unchecked cast.
>  */
> #define CPU(obj) ((CPUState *)(obj))

Given the present definition of CPU() this change seems like a step
backwards in terms of safety as mis-use won't be caught at compile or
runtime.  I'll change it anyway.


>
>> +    CPUARMState *env = &cpu->env;
>> +    MemTxResult result;
>> +    hwaddr vec = env->v7m.vecbase + env->v7m.exception * 4;
>> +    uint32_t addr;
>> +
>> +    addr = address_space_ldl(cs->as, vec,
>> +                             MEMTXATTRS_UNSPECIFIED, &result);
>> +    if (result != MEMTX_OK) {
> We could use a comment here:
>    /* Architecturally this should cause a HardFault setting HSFR.VECTTBL,
>     * which would then be immediately followed by our failing to load
>     * the entry vector for that HardFault, which is a Lockup case.
>     * Since we don't model Lockup, we just report this guest error
>     * via cpu_abort().
>     */

Added.

Re: [Qemu-devel] [PATCH v2 03/26] armv7m: Explicit error for bad vector table

[Peter Maydell]

On 27 December 2015 at 20:43, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> On 12/17/2015 08:25 AM, Peter Maydell wrote:
>> On 3 December 2015 at 00:18, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
>>> ...
>>> +static
>>> +uint32_t arm_v7m_load_vector(ARMCPU *cpu)
>>> +
>>> +{
>>> +    CPUState *cs = &cpu->parent_obj;
>> This isn't the right way to cast to the base class of a QOM object.
>> You want:
>>    CPUState *cs = CPU(cpu);
>
> from cpu.h
>
>> /* Since this macro is used a lot in hot code paths and in conjunction
>> with
>>  * FooCPU *foo_env_get_cpu(), we deviate from usual QOM practice by using
>>  * an unchecked cast.
>>  */
>> #define CPU(obj) ((CPUState *)(obj))
>
> Given the present definition of CPU() this change seems like a step
> backwards in terms of safety as mis-use won't be caught at compile or
> runtime.  I'll change it anyway.

The idea is that all code should use the QOM cast macros.
At the moment we have a special case for CPU() because it's
a hot path; in future we might be able to improve the speed of
the cast checking to the point where we can reinstate it.

thanks
-- PMM

[Qemu-devel] [PATCH v2 04/26] armv7m: additional cpu state for exception handling

[Michael Davidsaver]

Track priorities and highest active and pending
exception.  Also the highest pending exception
for faster exception handler entry.

The pending exception information will be re-calculated
on load, so no additional vmstate tracking is needed.
---
 target-arm/cpu.c | 2 ++
 target-arm/cpu.h | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 728854f..5348028 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) {
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 v2 05/26] armv7m: add armv7m_excp_running_prio()

[Michael Davidsaver]

Implements v7m exception priority algorithm
using FAULTMASK, PRIMASK, BASEPRI, and the highest
priority active exception.

The number returned is the current execution priority
which may be in the range [-2,0x7f] when an exception is active
or 0x100 when no exception is active.
---
 hw/intc/armv7m_nvic.c | 25 +++++++++++++++++++++++++
 target-arm/cpu.h      |  1 +
 2 files changed, 26 insertions(+)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 6fc167e..0145ca7 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -18,6 +18,8 @@
 
 typedef struct {
     GICState gic;
+    uint8_t prigroup;
+
     struct {
         uint32_t control;
         uint32_t reload;
@@ -116,6 +118,29 @@ static void systick_reset(nvic_state *s)
     timer_del(s->systick.timer);
 }
 
+/* @returns the active (running) exception priority.
+ *    only a higher (numerically lower) priority can preempt.
+ */
+int armv7m_excp_running_prio(ARMCPU *cpu)
+{
+    CPUARMState *env = &cpu->env;
+    nvic_state *s = env->nvic;
+    int running;
+
+    if (env->daif & PSTATE_F) { /* FAULTMASK */
+        running = -1;
+    } else if (env->daif & PSTATE_I) { /* PRIMASK */
+        running = 0;
+    } else if (env->v7m.basepri > 0) {
+        /* BASEPRI==1 -> masks [1,255] (not same as PRIMASK==1) */
+        running = env->v7m.basepri >> (s->prigroup+1);
+    } else {
+        running = 0x100; /* lower than any possible priority */
+    }
+    /* consider priority of active handler */
+    return MIN(running, env->v7m.exception_prio);
+}
+
 /* The external routines use the hardware vector numbering, ie. the first
    IRQ is #16.  The internal GIC routines use #32 as the first IRQ.  */
 void armv7m_nvic_set_pending(void *opaque, int irq)
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index c193fbb..e2d9e75 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -1034,6 +1034,7 @@ uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx,
                                  uint32_t cur_el, bool secure);
 
 /* Interface between CPU and Interrupt controller.  */
+int armv7m_excp_running_prio(ARMCPU *cpu);
 void armv7m_nvic_set_pending(void *opaque, int irq);
 int armv7m_nvic_acknowledge_irq(void *opaque);
 void armv7m_nvic_complete_irq(void *opaque, int irq);
-- 
2.1.4

Re: [Qemu-devel] [PATCH v2 05/26] armv7m: add armv7m_excp_running_prio()

[Peter Maydell]

On 3 December 2015 at 00:18, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> Implements v7m exception priority algorithm
> using FAULTMASK, PRIMASK, BASEPRI, and the highest
> priority active exception.
>
> The number returned is the current execution priority
> which may be in the range [-2,0x7f] when an exception is active
> or 0x100 when no exception is active.
> ---
>  hw/intc/armv7m_nvic.c | 25 +++++++++++++++++++++++++
>  target-arm/cpu.h      |  1 +
>  2 files changed, 26 insertions(+)
>
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 6fc167e..0145ca7 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -18,6 +18,8 @@
>
>  typedef struct {
>      GICState gic;
> +    uint8_t prigroup;
> +
>      struct {
>          uint32_t control;
>          uint32_t reload;
> @@ -116,6 +118,29 @@ static void systick_reset(nvic_state *s)
>      timer_del(s->systick.timer);
>  }
>
> +/* @returns the active (running) exception priority.
> + *    only a higher (numerically lower) priority can preempt.
> + */
> +int armv7m_excp_running_prio(ARMCPU *cpu)
> +{
> +    CPUARMState *env = &cpu->env;
> +    nvic_state *s = env->nvic;
> +    int running;
> +
> +    if (env->daif & PSTATE_F) { /* FAULTMASK */
> +        running = -1;
> +    } else if (env->daif & PSTATE_I) { /* PRIMASK */
> +        running = 0;
> +    } else if (env->v7m.basepri > 0) {
> +        /* BASEPRI==1 -> masks [1,255] (not same as PRIMASK==1) */
> +        running = env->v7m.basepri >> (s->prigroup+1);

This isn't right -- the effect of PRIGROUP is that we mask
out the lower (subgroup) bits, but the upper group bits stay
where they are rather than shifting down.

So you want env->v7m.basepri & ~((1 << (s->prigroup + 1)) - 1);

(the same mask you need to get the group priority for
an interrupt).

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

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 05/26] armv7m: add armv7m_excp_running_prio()

[Michael Davidsaver]

On 12/17/2015 09:36 AM, Peter Maydell wrote:
> On 3 December 2015 at 00:18, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
>> Implements v7m exception priority algorithm
>> using FAULTMASK, PRIMASK, BASEPRI, and the highest
>> priority active exception.
>>
>> The number returned is the current execution priority
>> which may be in the range [-2,0x7f] when an exception is active
>> or 0x100 when no exception is active.
>> ---
>>  hw/intc/armv7m_nvic.c | 25 +++++++++++++++++++++++++
>>  target-arm/cpu.h      |  1 +
>>  2 files changed, 26 insertions(+)
>>
>> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
>> index 6fc167e..0145ca7 100644
>> --- a/hw/intc/armv7m_nvic.c
>> +++ b/hw/intc/armv7m_nvic.c
>> @@ -18,6 +18,8 @@
>>
>>  typedef struct {
>>      GICState gic;
>> +    uint8_t prigroup;
>> +
>>      struct {
>>          uint32_t control;
>>          uint32_t reload;
>> @@ -116,6 +118,29 @@ static void systick_reset(nvic_state *s)
>>      timer_del(s->systick.timer);
>>  }
>>
>> +/* @returns the active (running) exception priority.
>> + *    only a higher (numerically lower) priority can preempt.
>> + */
>> +int armv7m_excp_running_prio(ARMCPU *cpu)
>> +{
>> +    CPUARMState *env = &cpu->env;
>> +    nvic_state *s = env->nvic;
>> +    int running;
>> +
>> +    if (env->daif & PSTATE_F) { /* FAULTMASK */
>> +        running = -1;
>> +    } else if (env->daif & PSTATE_I) { /* PRIMASK */
>> +        running = 0;
>> +    } else if (env->v7m.basepri > 0) {
>> +        /* BASEPRI==1 -> masks [1,255] (not same as PRIMASK==1) */
>> +        running = env->v7m.basepri >> (s->prigroup+1);
> This isn't right -- the effect of PRIGROUP is that we mask
> out the lower (subgroup) bits, but the upper group bits stay
> where they are rather than shifting down.
>
> So you want env->v7m.basepri & ~((1 << (s->prigroup + 1)) - 1);
>
> (the same mask you need to get the group priority for
> an interrupt).

I don't know about "right", but it is consistent with how the
.prio_group field is now handled in the nvic.  So I think the final
behavior is as specified.

There is no functional reason that I do this.  I just think it makes the
DPRINTF messages easier to interpret.  If you feel strongly I can change
this.

Re: [Qemu-devel] [PATCH v2 05/26] armv7m: add armv7m_excp_running_prio()

[Peter Maydell]

On 27 December 2015 at 20:56, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> On 12/17/2015 09:36 AM, Peter Maydell wrote:
>> On 3 December 2015 at 00:18, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
>>> Implements v7m exception priority algorithm
>>> using FAULTMASK, PRIMASK, BASEPRI, and the highest
>>> priority active exception.
>>>
>>> The number returned is the current execution priority
>>> which may be in the range [-2,0x7f] when an exception is active
>>> or 0x100 when no exception is active.
>>> ---
>>>  hw/intc/armv7m_nvic.c | 25 +++++++++++++++++++++++++
>>>  target-arm/cpu.h      |  1 +
>>>  2 files changed, 26 insertions(+)
>>>
>>> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
>>> index 6fc167e..0145ca7 100644
>>> --- a/hw/intc/armv7m_nvic.c
>>> +++ b/hw/intc/armv7m_nvic.c
>>> @@ -18,6 +18,8 @@
>>>
>>>  typedef struct {
>>>      GICState gic;
>>> +    uint8_t prigroup;
>>> +
>>>      struct {
>>>          uint32_t control;
>>>          uint32_t reload;
>>> @@ -116,6 +118,29 @@ static void systick_reset(nvic_state *s)
>>>      timer_del(s->systick.timer);
>>>  }
>>>
>>> +/* @returns the active (running) exception priority.
>>> + *    only a higher (numerically lower) priority can preempt.
>>> + */
>>> +int armv7m_excp_running_prio(ARMCPU *cpu)
>>> +{
>>> +    CPUARMState *env = &cpu->env;
>>> +    nvic_state *s = env->nvic;
>>> +    int running;
>>> +
>>> +    if (env->daif & PSTATE_F) { /* FAULTMASK */
>>> +        running = -1;
>>> +    } else if (env->daif & PSTATE_I) { /* PRIMASK */
>>> +        running = 0;
>>> +    } else if (env->v7m.basepri > 0) {
>>> +        /* BASEPRI==1 -> masks [1,255] (not same as PRIMASK==1) */
>>> +        running = env->v7m.basepri >> (s->prigroup+1);
>> This isn't right -- the effect of PRIGROUP is that we mask
>> out the lower (subgroup) bits, but the upper group bits stay
>> where they are rather than shifting down.
>>
>> So you want env->v7m.basepri & ~((1 << (s->prigroup + 1)) - 1);
>>
>> (the same mask you need to get the group priority for
>> an interrupt).
>
> I don't know about "right", but it is consistent with how the
> .prio_group field is now handled in the nvic.  So I think the final
> behavior is as specified.

The v7M ARM ARM is the authoritative source for "what is the
right behaviour", and the pseudocode is I think fairly clear
about how BASEPRI affects the running priority. If the current
NVIC code is handling BASEPRI wrongly that's a bug in the NVIC
code.

> There is no functional reason that I do this.  I just think it makes the
> DPRINTF messages easier to interpret.  If you feel strongly I can change
> this.

You need to get the right value here because otherwise we won't
be implementing the spec correctly.

thanks
-- PMM

[Qemu-devel] [PATCH v2 06/26] armv7m: fix I and F flag handling

[Michael Davidsaver]

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

Use armv7m_excp_running_prio() and the highest
pending exception priority to determine
if the pending exception can interrupt preempt.
---
 target-arm/cpu.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 5348028..6e3b251 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -305,19 +305,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 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)
-            ) {
+            && (armv7m_excp_running_prio(cpu) > cpu->env.v7m.pending_prio)) {
         cs->exception_index = EXCP_IRQ;
         cc->do_interrupt(cs);
         ret = true;
-- 
2.1.4

Re: [Qemu-devel] [PATCH v2 06/26] armv7m: fix I and F flag handling

[Peter Maydell]

On 3 December 2015 at 00:18, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> Despite having the same notation, these bits
> have completely different meaning than -AR.
>
> Use armv7m_excp_running_prio() and the highest
> pending exception priority to determine
> if the pending exception can interrupt preempt.
> ---
>  target-arm/cpu.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)

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

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 06/26] armv7m: fix I and F flag handling

[Peter Maydell]

On 17 December 2015 at 14:39, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 3 December 2015 at 00:18, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
>> Despite having the same notation, these bits
>> have completely different meaning than -AR.
>>
>> Use armv7m_excp_running_prio() and the highest
>> pending exception priority to determine
>> if the pending exception can interrupt preempt.
>> ---
>>  target-arm/cpu.c | 16 ++++++----------
>>  1 file changed, 6 insertions(+), 10 deletions(-)
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

...except this breaks the build for linux-user:

  LINK  arm-linux-user/qemu-arm
target-arm/cpu.o: In function `arm_v7m_cpu_exec_interrupt':
/home/petmay01/linaro/qemu-from-laptop/qemu/target-arm/cpu.c:316:
undefined reference to `armv7m_excp_running_prio'

because the function you're calling here is in armv7m_nvic.c,
which isn't compiled into the linux-user binary.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 06/26] armv7m: fix I and F flag handling

[Michael Davidsaver]

On 12/17/2015 10:18 AM, Peter Maydell wrote:
> On 17 December 2015 at 14:39, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 3 December 2015 at 00:18, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
>>> Despite having the same notation, these bits
>>> have completely different meaning than -AR.
>>>
>>> Use armv7m_excp_running_prio() and the highest
>>> pending exception priority to determine
>>> if the pending exception can interrupt preempt.
>>> ---
>>>  target-arm/cpu.c | 16 ++++++----------
>>>  1 file changed, 6 insertions(+), 10 deletions(-)
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> ...except this breaks the build for linux-user:
>
>   LINK  arm-linux-user/qemu-arm
> target-arm/cpu.o: In function `arm_v7m_cpu_exec_interrupt':
> /home/petmay01/linaro/qemu-from-laptop/qemu/target-arm/cpu.c:316:
> undefined reference to `armv7m_excp_running_prio'
>
> because the function you're calling here is in armv7m_nvic.c,
> which isn't compiled into the linux-user binary.

Is there any reason to include the armv7m code in linux-user at all?

Re: [Qemu-devel] [Qemu-arm] [PATCH v2 06/26] armv7m: fix I and F flag handling

[Peter Maydell]

On 28 December 2015 at 01:59, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> On 12/17/2015 10:18 AM, Peter Maydell wrote:
>> because the function you're calling here is in armv7m_nvic.c,
>> which isn't compiled into the linux-user binary.
>
> Is there any reason to include the armv7m code in linux-user at all?

It's not a useful cpu for actual linux binaries, but
I believe there is some use of the linux-user target with
armv7m to test unprivileged bare-metal code, for instance
the gcc test suite. (This uses semihosting for test case
output I think.)

thanks
-- PMM

[Qemu-devel] [PATCH v2 07/26] armv7m: simpler/faster exception start

[Michael Davidsaver]

No need to bounce through EXCP_IRQ handling
for non-IRQ exceptions.  just update CPU
state directly.
---
 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 7b76f32..4490b74 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -5451,23 +5451,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;
@@ -5484,7 +5482,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);
@@ -5494,6 +5491,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

Re: [Qemu-devel] [PATCH v2 07/26] armv7m: simpler/faster exception start

[Peter Maydell]

On 3 December 2015 at 00:18, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> No need to bounce through EXCP_IRQ handling
> for non-IRQ exceptions.  just update CPU
> state directly.
> ---
>  target-arm/helper.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)

This commit takes my stellaris test image from "broken and sits
there with a black screen" to "aborts on startup":

Peripheral reset not implemented
Peripheral reset not implemented
qemu: hardware error: Interrupt but no vector

CPU #0:
R00=40028000 R01=00000003 R02=00000000 R03=00000000
R04=40028000 R05=00000003 R06=00000000 R07=00000000
R08=00000000 R09=00000000 R10=00000000 R11=00000000
R12=40005000 R13=20006620 R14=fffffff9 R15=20006668
PSR=60000153 -ZC- A svc32
FPSCR: 00000000
Aborted (core dumped)

thanks
-- PMM

[Qemu-devel] [PATCH v2 08/26] armv7m: rewrite NVIC

[Michael Davidsaver]

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

Use 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.

Internal functions for operations previously done
by GIC internals.

nvic_irq_update() recalculates highest pending exception.
Update ARMCPU state.

armv7m_nvic_set_pending() include exception escalation logic.

Replace use of GIC state/functions with new NVIC.

Fix RETTOBASE.  The polarity is reversed, and it should include
internal exceptions.  Should be set when # of active exceptions == 1.
---
 hw/intc/armv7m_nvic.c | 805 ++++++++++++++++++++++++++++++++++++++------------
 target-arm/cpu.h      |   4 +-
 2 files changed, 622 insertions(+), 187 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 0145ca7..ca9bd4c 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -13,11 +13,48 @@
 #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 VecInfo {
+    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 VecInfo VecInfo;
+
+struct NVICState {
+    /*< private >*/
+    SysBusDevice parent_obj;
+    /*< public >*/
+
+    ARMCPU *cpu; /* NVIC is so closely tied to the CPU, just keep a ref */
+
+    VecInfo vectors[NVIC_MAX_VECTORS];
+
     uint8_t prigroup;
 
     struct {
@@ -26,34 +63,19 @@ typedef struct {
         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 NVICState NVICState;
 
 #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)
+    OBJECT_CHECK(NVICState, (obj), TYPE_NVIC)
 
 static const uint8_t nvic_id[] = {
     0x00, 0xb0, 0x1b, 0x00, 0x0d, 0xe0, 0x05, 0xb1
@@ -70,7 +92,7 @@ static const uint8_t nvic_id[] = {
 int system_clock_scale;
 
 /* Conversion factor from qemu timer to SysTick frequencies.  */
-static inline int64_t systick_scale(nvic_state *s)
+static inline int64_t systick_scale(NVICState *s)
 {
     if (s->systick.control & SYSTICK_CLKSOURCE)
         return system_clock_scale;
@@ -78,7 +100,7 @@ static inline int64_t systick_scale(nvic_state *s)
         return 1000;
 }
 
-static void systick_reload(nvic_state *s, int reset)
+static void systick_reload(NVICState *s, int reset)
 {
     /* The Cortex-M3 Devices Generic User Guide says that "When the
      * ENABLE bit is set to 1, the counter loads the RELOAD value from the
@@ -97,7 +119,7 @@ static void systick_reload(nvic_state *s, int reset)
 
 static void systick_timer_tick(void * opaque)
 {
-    nvic_state *s = (nvic_state *)opaque;
+    NVICState *s = (NVICState *)opaque;
     s->systick.control |= SYSTICK_COUNTFLAG;
     if (s->systick.control & SYSTICK_TICKINT) {
         /* Trigger the interrupt.  */
@@ -110,7 +132,7 @@ static void systick_timer_tick(void * opaque)
     }
 }
 
-static void systick_reset(nvic_state *s)
+static void systick_reset(NVICState *s)
 {
     s->systick.control = 0;
     s->systick.reload = 0;
@@ -118,13 +140,120 @@ static void systick_reset(nvic_state *s)
     timer_del(s->systick.timer);
 }
 
+/* caller must call nvic_irq_update() after this */
+static
+void set_prio(NVICState *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 (prigroup = %u)\n", irq,
+            s->vectors[irq].prio_group, s->vectors[irq].prio_sub,
+            (unsigned)s->prigroup);
+}
+
+/* recompute highest pending */
+static
+void nvic_irq_update(NVICState *s)
+{
+    unsigned i;
+    int lvl;
+    CPUARMState *env = &s->cpu->env;
+    int16_t pend_group = 0x100;
+    uint16_t pend_sub = 0, pend_irq = 0;
+
+    /* find highest priority */
+    for (i = 1; i < s->num_irq; i++) {
+        VecInfo *vec = &s->vectors[i];
+
+        DPRINTF(2, " VECT %d %d:%u\n", i, vec->prio_group, vec->prio_sub);
+
+        if (vec->enabled && vec->pending && ((vec->prio_group < pend_group)
+                || (vec->prio_group == pend_group
+                    && vec->prio_sub < pend_sub)))
+        {
+            pend_group = vec->prio_group;
+            pend_sub = vec->prio_sub;
+            pend_irq = i;
+        }
+    }
+
+    env->v7m.pending = pend_irq;
+    env->v7m.pending_prio = pend_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(0, "IRQ %c highest pending %d %d:%u\n",
+            lvl ? 'X' : '_',
+            pend_irq, pend_group, pend_sub);
+
+    qemu_set_irq(s->excpout, lvl);
+}
+
+static
+void armv7m_nvic_clear_pending(void *opaque, int irq)
+{
+    NVICState *s = (NVICState *)opaque;
+    VecInfo *vec;
+
+    assert(irq >= 0);
+    assert(irq < NVIC_MAX_VECTORS);
+
+    vec = &s->vectors[irq];
+    if (vec->pending) {
+        vec->pending = 0;
+        nvic_irq_update(s);
+    }
+}
+
+int armv7m_nvic_get_active_prio(void *opaque)
+{
+    NVICState *s = (NVICState *)opaque;
+    unsigned i;
+    int16_t group = 0x100;
+    uint16_t sub = 0xff;
+
+    /* don't consider env->v7m.exception
+     * as we are called while it is inconsistent
+     */
+
+    for (i = 1; i < s->num_irq; i++) {
+        VecInfo *vec = &s->vectors[i];
+        if (!vec->active) {
+            continue;
+        }
+        if (vec->prio_group < group ||
+                (vec->prio_group == group &&
+                 vec->prio_sub < sub))
+        {
+            group = vec->prio_group;
+            sub = vec->prio_sub;
+        }
+    }
+
+    return group;
+}
+
 /* @returns the active (running) exception priority.
  *    only a higher (numerically lower) priority can preempt.
  */
 int armv7m_excp_running_prio(ARMCPU *cpu)
 {
     CPUARMState *env = &cpu->env;
-    nvic_state *s = env->nvic;
+    NVICState *s = env->nvic;
     int running;
 
     if (env->daif & PSTATE_F) { /* FAULTMASK */
@@ -141,47 +270,191 @@ int armv7m_excp_running_prio(ARMCPU *cpu)
     return MIN(running, env->v7m.exception_prio);
 }
 
-/* The external routines use the hardware vector numbering, ie. the first
-   IRQ is #16.  The internal GIC routines use #32 as the first IRQ.  */
 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);
+    NVICState *s = (NVICState *)opaque;
+    CPUARMState *env = &s->cpu->env;
+    VecInfo *vec;
+    int active = s->cpu->env.v7m.exception;
+
+    assert(irq > 1); /* don't pend reset */
+    assert(irq < s->num_irq);
+
+    vec = &s->vectors[irq];
+
+    if (irq < ARMV7M_EXCP_PENDSV
+            && irq != ARMV7M_EXCP_DEBUG
+            && irq != ARMV7M_EXCP_NMI)
+    {
+        int running = armv7m_excp_running_prio(s->cpu);
+        /* test for exception escalation for vectors other than:
+         * NMI (2), Debug (12), PendSV (14), SysTick (15),
+         * and all external IRQs (>=16).
+         * This assumes that all such exceptions are precise (sync.)
+         * and that we don't simulate imprecise (async.) faults.
+         * Some Debug exceptions should be escalated, however
+         * this exception is presently unused.
+         */
+        unsigned escalate = 0;
+        if (vec->prio_group >= running) {
+            /* Trying to pend a fault which is not immediately
+             * runnable due to masking by PRIMASK, FAULTMASK, BASEPRI,
+             * or the priority of an active exception
+             */
+            DPRINTF(0, " Escalate, insufficient priority %d >= %d\n",
+                    vec->prio_group, running);
+            escalate = 1;
+
+        } else if (!vec->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 (vec->active) {
+            /* This case should only be reached if some logic error
+             * has caused env->exception_prio to get out of sync with
+             * the active exception priorities.
+             */
+            hw_error("exception priorities are out of sync\n");
+        }
+
+        if (escalate) {
+#ifdef DEBUG_NVIC
+            int oldirq = irq;
+#endif
+            if (running < 0) {
+                /* TODO: actual unrecoverable exception actions */
+                cpu_abort(&s->cpu->parent_obj,
+                          "%d in %d escalates to unrecoverable exception\n",
+                          irq, active);
+            }
+            irq = ARMV7M_EXCP_HARD;
+            vec = &s->vectors[irq];
+
+            DPRINTF(0, "Escalate %d to HardFault\n", oldirq);
+        }
+    }
+
+    vec->pending = 1;
+    if (vec->enabled && (vec->prio_group < env->v7m.pending_prio)) {
+        env->v7m.pending_prio = vec->prio_group;
+        env->v7m.pending = irq;
+        qemu_set_irq(s->excpout, irq > 0);
+    }
+    DPRINTF(0, "Pending %d at %d%s running %d\n",
+            irq, vec->prio_group,
+            env->v7m.pending == irq ? " (highest)" : "",
+            armv7m_excp_running_prio(s->cpu));
+}
+
+bool armv7m_nvic_is_active(void *opaque, int irq)
+{
+    NVICState *s = (NVICState *)opaque;
+
+    assert(irq > 0 && irq < s->num_irq);
+    return s->vectors[irq].active;
 }
 
 /* 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;
+    NVICState *s = (NVICState *)opaque;
+    CPUARMState *env = &s->cpu->env;
+    const int pending = env->v7m.pending;
+    const int running = armv7m_excp_running_prio(s->cpu);
+    VecInfo *vec;
 
-    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);
+    vec = &s->vectors[pending];
+
+    assert(vec->enabled);
+
+    assert(env->v7m.pending_prio == vec->prio_group);
+    if (env->v7m.pending_prio >= running) {
+        hw_error("Interrupt ack. while masked %d >= %d",
+                 env->v7m.pending_prio, running);
+    }
+
+    DPRINTF(0, "ACT %d at %d\n", pending, vec->prio_group);
+
+    assert(vec->pending);
+    vec->active = 1;
+    vec->pending = 0;
+
+    env->v7m.exception = env->v7m.pending;
+    env->v7m.exception_prio = env->v7m.pending_prio;
+
+    nvic_irq_update(s); /* recalc pending */
+
+    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);
+    NVICState *s = (NVICState *)opaque;
+    VecInfo *vec;
+
+    assert(irq > 0);
+    assert(irq < NVIC_MAX_VECTORS);
+
+    vec = &s->vectors[irq];
+
+    vec->active = 0;
+    vec->pending = vec->level;
+    assert(!vec->level || irq >= 16);
+
+    nvic_irq_update(s);
+    DPRINTF(0, "EOI %d\n", irq);
+}
+
+/* Only called for external interrupt (vector>=16) */
+static
+void set_irq_level(void *opaque, int n, int level)
+{
+    NVICState *s = opaque;
+    VecInfo *vec;
+
+    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.
+     */
+    vec = &s->vectors[n];
+    vec->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)
+static uint32_t nvic_readl(NVICState *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;
@@ -207,45 +480,50 @@ 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);
-        /* ISRPENDING and RETTOBASE */
-        for (irq = 32; irq < s->num_irq; irq++) {
-            if (s->gic.irq_state[irq].pending) {
+        val |= (cpu->env.v7m.pending & 0xff) << 12;
+        /* 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;
             }
-            if (irq != cpu->env.v7m.exception && s->gic.irq_state[irq].active) {
-                val |= (1 << 11);
+        }
+        /* RETTOBASE - Set if only one handler is active */
+    {
+        unsigned nhand = 0;
+        for (irq = 1; irq < s->num_irq; irq++) {
+            if (s->vectors[irq].active) {
+                nhand++;
+                if (nhand == 2) {
+                    break;
+                }
             }
         }
+        val |= nhand == 1 ? (1<<11) : 0;
+    }
         /* 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);
+        }
+        /* ISRPREEMPT not implemented */
         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;
@@ -254,20 +532,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.  */
@@ -314,9 +592,9 @@ static uint32_t nvic_readl(nvic_state *s, uint32_t offset)
     }
 }
 
-static void nvic_writel(nvic_state *s, uint32_t offset, uint32_t value)
+static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value)
 {
-    ARMCPU *cpu;
+    ARMCPU *cpu = s->cpu;
     uint32_t oldval;
     switch (offset) {
     case 0x10: /* SysTick Control and Status.  */
@@ -358,18 +636,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.  */
@@ -378,13 +653,24 @@ 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) {
-                qemu_log_mask(LOG_UNIMP, "PRIGROUP unimplemented\n");
+                unsigned i;
+                s->prigroup = (value>>8) & 0xf;
+                /* recalculate priorities 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);
+                cpu->env.v7m.exception_prio = armv7m_nvic_get_active_prio(s);
             }
         }
         break;
@@ -396,9 +682,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.  */
@@ -410,8 +699,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:
@@ -423,54 +712,159 @@ static void nvic_writel(nvic_state *s, uint32_t offset, uint32_t value)
 static uint64_t nvic_sysreg_read(void *opaque, hwaddr addr,
                                  unsigned size)
 {
-    nvic_state *s = (nvic_state *)opaque;
+    NVICState *s = (NVICState *)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,
                               uint64_t value, unsigned size)
 {
-    nvic_state *s = (nvic_state *)opaque;
+    NVICState *s = (NVICState *)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);
+        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);
+        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);
+        s->cpu->env.v7m.exception_prio = armv7m_nvic_get_active_prio(s);
         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);
+        s->cpu->env.v7m.exception_prio = armv7m_nvic_get_active_prio(s);
         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 = {
@@ -479,79 +873,123 @@ static const MemoryRegionOps nvic_sysreg_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static const VMStateDescription vmstate_nvic = {
-    .name = "armv7m_nvic",
+static
+int nvic_post_load(void *opaque, int version_id)
+{
+    NVICState *s = opaque;
+    unsigned i;
+
+    /* recalculate priorities */
+    for (i = 4; i < s->num_irq; i++) {
+        set_prio(s, i, s->vectors[i].raw_prio);
+    }
+
+    nvic_irq_update(s);
+    s->cpu->env.v7m.exception_prio = armv7m_nvic_get_active_prio(s);
+
+    return 0;
+}
+
+static const VMStateDescription vmstate_VecInfo = {
+    .name = "armv7m_nvic_info",
     .version_id = 1,
     .minimum_version_id = 1,
     .fields = (VMStateField[]) {
-        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_UINT16(prio_sub, VecInfo),
+        VMSTATE_INT8(prio_group, VecInfo),
+        VMSTATE_UINT8(raw_prio, VecInfo),
+        VMSTATE_UINT8(enabled, VecInfo),
+        VMSTATE_UINT8(pending, VecInfo),
+        VMSTATE_UINT8(active, VecInfo),
+        VMSTATE_UINT8(level, VecInfo),
         VMSTATE_END_OF_LIST()
     }
 };
 
+static const VMStateDescription vmstate_nvic = {
+    .name = "armv7m_nvic",
+    .version_id = 2,
+    .minimum_version_id = 2,
+    .post_load = &nvic_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_STRUCT_ARRAY(vectors, NVICState, NVIC_MAX_VECTORS, 1,
+                             vmstate_VecInfo, VecInfo),
+        VMSTATE_UINT8(prigroup, NVICState),
+        VMSTATE_UINT32(systick.control, NVICState),
+        VMSTATE_UINT32(systick.reload, NVICState),
+        VMSTATE_INT64(systick.tick, NVICState),
+        VMSTATE_TIMER_PTR(systick.timer, NVICState),
+        VMSTATE_UINT32(num_irq, NVICState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static Property props_nvic[] = {
+    DEFINE_PROP_UINT32("num-irq", NVICState, 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.
+    NVICState *s = NVIC(dev);
+
+    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;
+
+    /* strictly speaking the reset handler should be enabled.
+     * However, we don't simulate soft resets through the NVIC,
+     * and the reset vector should never be pended.
+     * So don't enabled to catch logic errors.
+    s->vectors[ARMV7M_EXCP_RESET].enabled = 1;
      */
-    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;
+
     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);
+    NVICState *s = NVIC(dev);
+
+    s->cpu = ARM_CPU(first_cpu);
+
+    if (s->num_irq > NVIC_MAX_IRQ) {
+        error_setg(errp, TYPE_NVIC " num-irq too large");
+        return;
+
+    } else if (s->num_irq & 0x1f) {
+        error_setg(errp, TYPE_NVIC " num-irq must be a multiple of 32");
         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);
 }
 
@@ -563,36 +1001,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;
+    NVICState *nvic = NVIC(obj);
+    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),
+    .instance_size = sizeof(NVICState),
     .class_init    = armv7m_nvic_class_init,
-    .class_size    = sizeof(NVICClass),
+    .class_size    = sizeof(SysBusDeviceClass),
 };
 
 static void armv7m_nvic_register_types(void)
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index e2d9e75..4b7f78e 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -1036,7 +1036,9 @@ uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx,
 /* Interface between CPU and Interrupt controller.  */
 int armv7m_excp_running_prio(ARMCPU *cpu);
 void armv7m_nvic_set_pending(void *opaque, int irq);
-int armv7m_nvic_acknowledge_irq(void *opaque);
+bool armv7m_nvic_is_active(void *opaque, int irq);
+int armv7m_nvic_get_active_prio(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

Re: [Qemu-devel] [PATCH v2 08/26] armv7m: rewrite NVIC

[Peter Maydell]

On 3 December 2015 at 00:18, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> Expand the NVIC to fully support -M priorities and masking.
> Doesn't use GIC code.
>
> Use 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.
>
> Internal functions for operations previously done
> by GIC internals.
>
> nvic_irq_update() recalculates highest pending exception.
> Update ARMCPU state.
>
> armv7m_nvic_set_pending() include exception escalation logic.
>
> Replace use of GIC state/functions with new NVIC.
>
> Fix RETTOBASE.  The polarity is reversed, and it should include
> internal exceptions.  Should be set when # of active exceptions == 1.
> ---
>  hw/intc/armv7m_nvic.c | 805 ++++++++++++++++++++++++++++++++++++++------------
>  target-arm/cpu.h      |   4 +-
>  2 files changed, 622 insertions(+), 187 deletions(-)

This patch is always going to be huge, but I think we can
trim it down a little by pulling a few things out:

 * do the rename nvic_state to NVICState as a separate
   patch before this one
 * adding CPU* to NVICState and avoiding use of current_cpu
   can go in its own patch
 * fixing RETTOBASE should be its own patch
 * [the main 'rewrite nvic' patch goes here in the sequence]
 * leave the "support read/write of AIRCR.PRIGROUP" parts
   to a patch that goes after the rewrite-nvic patch
 * changing the LOG_UNIMP to LOG_GUEST_ERROR in AIRCR
   writes can also go in a patch of its own

I've written some more detailed comments below; other than those
I think this looks OK, but I can't test it at this point.

> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 0145ca7..ca9bd4c 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -13,11 +13,48 @@
>  #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 VecInfo {
> +    uint16_t prio_sub; /* sub-group priority*512 + exception# */
> +    int8_t prio_group; /* group priority [-2, 0x7f] */

I still don't think we need to store group and subpriority
separately. See more detailed comments below.

> +    uint8_t raw_prio; /* value writen by guest */

"written"

> +    uint8_t enabled;
> +    uint8_t pending;
> +    uint8_t active;
> +    uint8_t level;
> +    /* exceptions <=15 never set level */
> +};
> +typedef struct VecInfo VecInfo;
> +
> +struct NVICState {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +    /*< public >*/
> +
> +    ARMCPU *cpu; /* NVIC is so closely tied to the CPU, just keep a ref */
> +
> +    VecInfo vectors[NVIC_MAX_VECTORS];
> +
>      uint8_t prigroup;
>
>      struct {
> @@ -26,34 +63,19 @@ typedef struct {
>          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 NVICState NVICState;
>
>  #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)
> +    OBJECT_CHECK(NVICState, (obj), TYPE_NVIC)
>
>  static const uint8_t nvic_id[] = {
>      0x00, 0xb0, 0x1b, 0x00, 0x0d, 0xe0, 0x05, 0xb1
> @@ -70,7 +92,7 @@ static const uint8_t nvic_id[] = {
>  int system_clock_scale;
>
>  /* Conversion factor from qemu timer to SysTick frequencies.  */
> -static inline int64_t systick_scale(nvic_state *s)
> +static inline int64_t systick_scale(NVICState *s)
>  {
>      if (s->systick.control & SYSTICK_CLKSOURCE)
>          return system_clock_scale;
> @@ -78,7 +100,7 @@ static inline int64_t systick_scale(nvic_state *s)
>          return 1000;
>  }
>
> -static void systick_reload(nvic_state *s, int reset)
> +static void systick_reload(NVICState *s, int reset)
>  {
>      /* The Cortex-M3 Devices Generic User Guide says that "When the
>       * ENABLE bit is set to 1, the counter loads the RELOAD value from the
> @@ -97,7 +119,7 @@ static void systick_reload(nvic_state *s, int reset)
>
>  static void systick_timer_tick(void * opaque)
>  {
> -    nvic_state *s = (nvic_state *)opaque;
> +    NVICState *s = (NVICState *)opaque;
>      s->systick.control |= SYSTICK_COUNTFLAG;
>      if (s->systick.control & SYSTICK_TICKINT) {
>          /* Trigger the interrupt.  */
> @@ -110,7 +132,7 @@ static void systick_timer_tick(void * opaque)
>      }
>  }
>
> -static void systick_reset(nvic_state *s)
> +static void systick_reset(NVICState *s)
>  {
>      s->systick.control = 0;
>      s->systick.reload = 0;
> @@ -118,13 +140,120 @@ static void systick_reset(nvic_state *s)
>      timer_del(s->systick.timer);
>  }
>
> +/* caller must call nvic_irq_update() after this */
> +static

No newline after 'static', please.

> +void set_prio(NVICState *s, unsigned irq, uint8_t prio)
> +{
> +    unsigned submask = (1<<(s->prigroup+1))-1;

Spaces around operators.

> +
> +    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 (prigroup = %u)\n", irq,
> +            s->vectors[irq].prio_group, s->vectors[irq].prio_sub,
> +            (unsigned)s->prigroup);
> +}
> +
> +/* recompute highest pending */
> +static
> +void nvic_irq_update(NVICState *s)
> +{
> +    unsigned i;
> +    int lvl;
> +    CPUARMState *env = &s->cpu->env;
> +    int16_t pend_group = 0x100;
> +    uint16_t pend_sub = 0, pend_irq = 0;
> +
> +    /* find highest priority */
> +    for (i = 1; i < s->num_irq; i++) {
> +        VecInfo *vec = &s->vectors[i];
> +
> +        DPRINTF(2, " VECT %d %d:%u\n", i, vec->prio_group, vec->prio_sub);
> +
> +        if (vec->enabled && vec->pending && ((vec->prio_group < pend_group)
> +                || (vec->prio_group == pend_group
> +                    && vec->prio_sub < pend_sub)))

You don't need to do this comparison with separate group and
subgroup tests. Just compare raw priorities.

> +        {
> +            pend_group = vec->prio_group;
> +            pend_sub = vec->prio_sub;
> +            pend_irq = i;
> +        }
> +    }
> +
> +    env->v7m.pending = pend_irq;
> +    env->v7m.pending_prio = pend_group;

You want to set v7m.pending_prio to the raw priority, not
the group priority:
 * in the check against armv7m_excp_running_prio()
   in cpu.c it doesn't matter which we use (since the running
   prio will always be a group-level priority)
 * in the check in armv7m_nvic_set_pending() we must look
   at the raw priority, to ensure that when pending an irq which
   has the same group priority as the current pending irq
   we end up with the pending irq being set appropriately
   depending on the subpriority values of the two interrupts
 * in armv7m_nvic_acknowledge_irq() you can just mask to
   get the group priority when you need it.

> +
> +    /* 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(0, "IRQ %c highest pending %d %d:%u\n",
> +            lvl ? 'X' : '_',
> +            pend_irq, pend_group, pend_sub);
> +
> +    qemu_set_irq(s->excpout, lvl);
> +}
> +
> +static
> +void armv7m_nvic_clear_pending(void *opaque, int irq)
> +{
> +    NVICState *s = (NVICState *)opaque;
> +    VecInfo *vec;
> +
> +    assert(irq >= 0);
> +    assert(irq < NVIC_MAX_VECTORS);

We seem to be inconsistent whether we check for irq < NVIC_MAX_VECTORS
or < s->num_irq in these assertions.

> +
> +    vec = &s->vectors[irq];
> +    if (vec->pending) {
> +        vec->pending = 0;
> +        nvic_irq_update(s);
> +    }
> +}
> +
> +int armv7m_nvic_get_active_prio(void *opaque)
> +{
> +    NVICState *s = (NVICState *)opaque;
> +    unsigned i;
> +    int16_t group = 0x100;
> +    uint16_t sub = 0xff;
> +
> +    /* don't consider env->v7m.exception
> +     * as we are called while it is inconsistent
> +     */
> +
> +    for (i = 1; i < s->num_irq; i++) {
> +        VecInfo *vec = &s->vectors[i];
> +        if (!vec->active) {
> +            continue;
> +        }
> +        if (vec->prio_group < group ||
> +                (vec->prio_group == group &&
> +                 vec->prio_sub < sub))

This doesn't need to consider group and subpriority
separately either...

> +        {
> +            group = vec->prio_group;
> +            sub = vec->prio_sub;
> +        }
> +    }
> +

...so you can just mask out to get the group priority
here rather than precalculate it for every irq.

> +    return group;
> +}
> +
>  /* @returns the active (running) exception priority.
>   *    only a higher (numerically lower) priority can preempt.
>   */
>  int armv7m_excp_running_prio(ARMCPU *cpu)
>  {
>      CPUARMState *env = &cpu->env;
> -    nvic_state *s = env->nvic;
> +    NVICState *s = env->nvic;
>      int running;
>
>      if (env->daif & PSTATE_F) { /* FAULTMASK */
> @@ -141,47 +270,191 @@ int armv7m_excp_running_prio(ARMCPU *cpu)
>      return MIN(running, env->v7m.exception_prio);
>  }
>
> -/* The external routines use the hardware vector numbering, ie. the first
> -   IRQ is #16.  The internal GIC routines use #32 as the first IRQ.  */
>  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);
> +    NVICState *s = (NVICState *)opaque;
> +    CPUARMState *env = &s->cpu->env;
> +    VecInfo *vec;
> +    int active = s->cpu->env.v7m.exception;
> +
> +    assert(irq > 1); /* don't pend reset */
> +    assert(irq < s->num_irq);
> +
> +    vec = &s->vectors[irq];
> +
> +    if (irq < ARMV7M_EXCP_PENDSV
> +            && irq != ARMV7M_EXCP_DEBUG
> +            && irq != ARMV7M_EXCP_NMI)
> +    {
> +        int running = armv7m_excp_running_prio(s->cpu);
> +        /* test for exception escalation for vectors other than:
> +         * NMI (2), Debug (12), PendSV (14), SysTick (15),
> +         * and all external IRQs (>=16).
> +         * This assumes that all such exceptions are precise (sync.)
> +         * and that we don't simulate imprecise (async.) faults.
> +         * Some Debug exceptions should be escalated, however
> +         * this exception is presently unused.
> +         */
> +        unsigned escalate = 0;
> +        if (vec->prio_group >= running) {

You can use the raw priority in this comparison (using prio_group
isn't incorrect but there's no need to use it).

> +            /* Trying to pend a fault which is not immediately
> +             * runnable due to masking by PRIMASK, FAULTMASK, BASEPRI,
> +             * or the priority of an active exception
> +             */
> +            DPRINTF(0, " Escalate, insufficient priority %d >= %d\n",
> +                    vec->prio_group, running);
> +            escalate = 1;
> +
> +        } else if (!vec->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 (vec->active) {
> +            /* This case should only be reached if some logic error
> +             * has caused env->exception_prio to get out of sync with
> +             * the active exception priorities.
> +             */
> +            hw_error("exception priorities are out of sync\n");
> +        }
> +
> +        if (escalate) {
> +#ifdef DEBUG_NVIC
> +            int oldirq = irq;
> +#endif

You can avoid the need for this ifdef if you just move the DPRINTF
up to here, before the irq variable gets changed.

> +            if (running < 0) {
> +                /* TODO: actual unrecoverable exception actions */
> +                cpu_abort(&s->cpu->parent_obj,
> +                          "%d in %d escalates to unrecoverable exception\n",
> +                          irq, active);
> +            }
> +            irq = ARMV7M_EXCP_HARD;
> +            vec = &s->vectors[irq];
> +
> +            DPRINTF(0, "Escalate %d to HardFault\n", oldirq);
> +        }
> +    }
> +
> +    vec->pending = 1;
> +    if (vec->enabled && (vec->prio_group < env->v7m.pending_prio)) {

This is the comparison that must use the raw priority so we
sort the currently-pending irq correctly against this new one.

> +        env->v7m.pending_prio = vec->prio_group;
> +        env->v7m.pending = irq;
> +        qemu_set_irq(s->excpout, irq > 0);
> +    }
> +    DPRINTF(0, "Pending %d at %d%s running %d\n",
> +            irq, vec->prio_group,
> +            env->v7m.pending == irq ? " (highest)" : "",
> +            armv7m_excp_running_prio(s->cpu));
> +}
> +
> +bool armv7m_nvic_is_active(void *opaque, int irq)
> +{
> +    NVICState *s = (NVICState *)opaque;
> +
> +    assert(irq > 0 && irq < s->num_irq);
> +    return s->vectors[irq].active;
>  }
>
>  /* 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;
> +    NVICState *s = (NVICState *)opaque;
> +    CPUARMState *env = &s->cpu->env;
> +    const int pending = env->v7m.pending;
> +    const int running = armv7m_excp_running_prio(s->cpu);
> +    VecInfo *vec;
>
> -    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);
> +    vec = &s->vectors[pending];
> +
> +    assert(vec->enabled);
> +
> +    assert(env->v7m.pending_prio == vec->prio_group);
> +    if (env->v7m.pending_prio >= running) {
> +        hw_error("Interrupt ack. while masked %d >= %d",
> +                 env->v7m.pending_prio, running);
> +    }
> +
> +    DPRINTF(0, "ACT %d at %d\n", pending, vec->prio_group);
> +
> +    assert(vec->pending);
> +    vec->active = 1;
> +    vec->pending = 0;
> +
> +    env->v7m.exception = env->v7m.pending;

If you switch to storing the full raw prio in pending_prio
then you need to mask it to get just the group prio to
put into v7m.exception_prio here.

> +    env->v7m.exception_prio = env->v7m.pending_prio;
> +
> +    nvic_irq_update(s); /* recalc pending */
> +
> +    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);
> +    NVICState *s = (NVICState *)opaque;
> +    VecInfo *vec;
> +
> +    assert(irq > 0);
> +    assert(irq < NVIC_MAX_VECTORS);
> +
> +    vec = &s->vectors[irq];
> +
> +    vec->active = 0;
> +    vec->pending = vec->level;
> +    assert(!vec->level || irq >= 16);
> +
> +    nvic_irq_update(s);
> +    DPRINTF(0, "EOI %d\n", irq);
> +}
> +
> +/* Only called for external interrupt (vector>=16) */
> +static
> +void set_irq_level(void *opaque, int n, int level)
> +{
> +    NVICState *s = opaque;
> +    VecInfo *vec;
> +
> +    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.
> +     */
> +    vec = &s->vectors[n];
> +    vec->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)
> +static uint32_t nvic_readl(NVICState *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;
> @@ -207,45 +480,50 @@ 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);
> -        /* ISRPENDING and RETTOBASE */
> -        for (irq = 32; irq < s->num_irq; irq++) {
> -            if (s->gic.irq_state[irq].pending) {
> +        val |= (cpu->env.v7m.pending & 0xff) << 12;
> +        /* ISRPENDING - Set it any externel IRQ pending (vector>=16) */

"external", and I think the "it" is unnecessary.

> +        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) {
> -                val |= (1 << 11);
> +        }
> +        /* RETTOBASE - Set if only one handler is active */
> +    {
> +        unsigned nhand = 0;
> +        for (irq = 1; irq < s->num_irq; irq++) {
> +            if (s->vectors[irq].active) {
> +                nhand++;
> +                if (nhand == 2) {
> +                    break;
> +                }
>              }
>          }
> +        val |= nhand == 1 ? (1<<11) : 0;
> +    }
>          /* 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);
> +        }
> +        /* ISRPREEMPT not implemented */
>          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);

Spaces around operators, please.

>      case 0xd10: /* System Control.  */
>          /* TODO: Implement SLEEPONEXIT.  */
>          return 0;
> @@ -254,20 +532,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);

(I did consider suggesting having a separate patch before this that
turned these direct checks on active/pending/enabled into function
calls, which would then mean this patch would only need to change
the implementation of those calls rather than every callsite.
That would make this patch smaller, but in the end I decided it
wasn't really worthwhile.)

>          return val;
>      case 0xd28: /* Configurable Fault Status.  */
>          /* TODO: Implement Fault Status.  */
> @@ -314,9 +592,9 @@ static uint32_t nvic_readl(nvic_state *s, uint32_t offset)
>      }
>  }
>
> -static void nvic_writel(nvic_state *s, uint32_t offset, uint32_t value)
> +static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value)
>  {
> -    ARMCPU *cpu;
> +    ARMCPU *cpu = s->cpu;
>      uint32_t oldval;
>      switch (offset) {
>      case 0x10: /* SysTick Control and Status.  */
> @@ -358,18 +636,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.  */
> @@ -378,13 +653,24 @@ 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) {
> -                qemu_log_mask(LOG_UNIMP, "PRIGROUP unimplemented\n");
> +                unsigned i;
> +                s->prigroup = (value>>8) & 0xf;
> +                /* recalculate priorities 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);
> +                cpu->env.v7m.exception_prio = armv7m_nvic_get_active_prio(s);
>              }
>          }
>          break;
> @@ -396,9 +682,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.  */
> @@ -410,8 +699,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:
> @@ -423,54 +712,159 @@ static void nvic_writel(nvic_state *s, uint32_t offset, uint32_t value)
>  static uint64_t nvic_sysreg_read(void *opaque, hwaddr addr,
>                                   unsigned size)
>  {
> -    nvic_state *s = (nvic_state *)opaque;
> +    NVICState *s = (NVICState *)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,
>                                uint64_t value, unsigned size)
>  {
> -    nvic_state *s = (nvic_state *)opaque;
> +    NVICState *s = (NVICState *)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);
> +        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);
> +        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);
> +        s->cpu->env.v7m.exception_prio = armv7m_nvic_get_active_prio(s);
>          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);
> +        s->cpu->env.v7m.exception_prio = armv7m_nvic_get_active_prio(s);
>          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 = {
> @@ -479,79 +873,123 @@ static const MemoryRegionOps nvic_sysreg_ops = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>
> -static const VMStateDescription vmstate_nvic = {
> -    .name = "armv7m_nvic",
> +static
> +int nvic_post_load(void *opaque, int version_id)
> +{
> +    NVICState *s = opaque;
> +    unsigned i;
> +
> +    /* recalculate priorities */
> +    for (i = 4; i < s->num_irq; i++) {
> +        set_prio(s, i, s->vectors[i].raw_prio);
> +    }
> +
> +    nvic_irq_update(s);
> +    s->cpu->env.v7m.exception_prio = armv7m_nvic_get_active_prio(s);
> +
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_VecInfo = {
> +    .name = "armv7m_nvic_info",
>      .version_id = 1,
>      .minimum_version_id = 1,
>      .fields = (VMStateField[]) {
> -        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_UINT16(prio_sub, VecInfo),
> +        VMSTATE_INT8(prio_group, VecInfo),
> +        VMSTATE_UINT8(raw_prio, VecInfo),
> +        VMSTATE_UINT8(enabled, VecInfo),
> +        VMSTATE_UINT8(pending, VecInfo),
> +        VMSTATE_UINT8(active, VecInfo),
> +        VMSTATE_UINT8(level, VecInfo),
>          VMSTATE_END_OF_LIST()
>      }
>  };
>
> +static const VMStateDescription vmstate_nvic = {
> +    .name = "armv7m_nvic",
> +    .version_id = 2,
> +    .minimum_version_id = 2,
> +    .post_load = &nvic_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_STRUCT_ARRAY(vectors, NVICState, NVIC_MAX_VECTORS, 1,
> +                             vmstate_VecInfo, VecInfo),
> +        VMSTATE_UINT8(prigroup, NVICState),
> +        VMSTATE_UINT32(systick.control, NVICState),
> +        VMSTATE_UINT32(systick.reload, NVICState),
> +        VMSTATE_INT64(systick.tick, NVICState),
> +        VMSTATE_TIMER_PTR(systick.timer, NVICState),
> +        VMSTATE_UINT32(num_irq, NVICState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static Property props_nvic[] = {
> +    DEFINE_PROP_UINT32("num-irq", NVICState, 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.
> +    NVICState *s = NVIC(dev);
> +
> +    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;
> +
> +    /* strictly speaking the reset handler should be enabled.
> +     * However, we don't simulate soft resets through the NVIC,
> +     * and the reset vector should never be pended.
> +     * So don't enabled to catch logic errors.

"So leave it disabled"

> +    s->vectors[ARMV7M_EXCP_RESET].enabled = 1;

Just remove this line; it's confusing and looks like an error
to have an unindented line of code in the middle of a comment.

>       */
> -    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;
> +
>      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);
> +    NVICState *s = NVIC(dev);
> +
> +    s->cpu = ARM_CPU(first_cpu);
> +
> +    if (s->num_irq > NVIC_MAX_IRQ) {
> +        error_setg(errp, TYPE_NVIC " num-irq too large");
> +        return;
> +
> +    } else if (s->num_irq & 0x1f) {
> +        error_setg(errp, TYPE_NVIC " num-irq must be a multiple of 32");
>          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);
>  }
>
> @@ -563,36 +1001,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;
> +    NVICState *nvic = NVIC(obj);
> +    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),
> +    .instance_size = sizeof(NVICState),
>      .class_init    = armv7m_nvic_class_init,
> -    .class_size    = sizeof(NVICClass),
> +    .class_size    = sizeof(SysBusDeviceClass),
>  };
>
>  static void armv7m_nvic_register_types(void)
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index e2d9e75..4b7f78e 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1036,7 +1036,9 @@ uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx,
>  /* Interface between CPU and Interrupt controller.  */
>  int armv7m_excp_running_prio(ARMCPU *cpu);
>  void armv7m_nvic_set_pending(void *opaque, int irq);
> -int armv7m_nvic_acknowledge_irq(void *opaque);
> +bool armv7m_nvic_is_active(void *opaque, int irq);
> +int armv7m_nvic_get_active_prio(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
>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 08/26] armv7m: rewrite NVIC

[Christopher Friedt]

On Wed, Dec 2, 2015 at 7:18 PM, Michael Davidsaver
<mdavidsaver@gmail.com> wrote:
> Expand the NVIC to fully support -M priorities and masking.

Wow - this whole patch series is quite impressive. I was just about to
start doing a MemManage implementation when I saw this and some
previous posts on the topic.

Michael, I've previously written an emulator that covers v6M & v7M
(unfortunately no DSP or floating point - yet). Do you have a branch
in github that I could track?

I'm sure I could be of some use for feedback and to help get your
patchset accepted.

Thanks,

C

Re: [Qemu-devel] [PATCH v2 08/26] armv7m: rewrite NVIC

[Christopher Friedt]

On Sat, Dec 19, 2015 at 2:08 PM, Christopher Friedt
<chrisfriedt@gmail.com> wrote:
> On Wed, Dec 2, 2015 at 7:18 PM, Michael Davidsaver
> <mdavidsaver@gmail.com> wrote:
>> Expand the NVIC to fully support -M priorities and masking.
>
> Wow - this whole patch series is quite impressive. I was just about to
> start doing a MemManage implementation when I saw this and some
> previous posts on the topic.
>
> Michael, I've previously written an emulator that covers v6M & v7M
> (unfortunately no DSP or floating point - yet). Do you have a branch
> in github that I could track?
>
> I'm sure I could be of some use for feedback and to help get your
> patchset accepted.

I think I found your github tree already: https://github.com/mdavidsaver/qemu

[Qemu-devel] [PATCH v2 09/26] armv7m: implement CFSR, HFSR, BFAR, and MMFAR

[Michael Davidsaver]

Add the Configurable, HardFault, BusFault and MemManage Status registers.
Note undefined instructions, violations, and escalations.

No BusFaults are raised at this point.
---
 hw/intc/armv7m_nvic.c | 28 ++++++++++++++++++++++------
 target-arm/cpu.h      |  4 ++++
 target-arm/helper.c   |  3 +++
 target-arm/machine.c  |  8 ++++++--
 4 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index ca9bd4c..619c320 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -333,6 +333,7 @@ void armv7m_nvic_set_pending(void *opaque, int irq)
             irq = ARMV7M_EXCP_HARD;
             vec = &s->vectors[irq];
 
+            s->cpu->env.v7m.hfsr |= 1<<30; /* FORCED */
             DPRINTF(0, "Escalate %d to HardFault\n", oldirq);
         }
     }
@@ -548,16 +549,20 @@ static uint32_t nvic_readl(NVICState *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.  */
+        qemu_log_mask(LOG_UNIMP, "Debug Fault status register unimplemented\n");
+        return 0;
+    case 0xd34: /* MMFAR MemManage Fault Address */
+        return cpu->env.v7m.mmfar;
     case 0xd38: /* Bus Fault Address.  */
+        return cpu->env.v7m.bfar;
     case 0xd3c: /* Aux Fault Status.  */
         /* TODO: Implement fault status registers.  */
-        qemu_log_mask(LOG_UNIMP, "Fault status registers unimplemented\n");
+        qemu_log_mask(LOG_UNIMP,
+                      "Aux Fault status registers unimplemented\n");
         return 0;
     case 0xd40: /* PFR0.  */
         return 0x00000030;
@@ -690,13 +695,24 @@ static void nvic_writel(NVICState *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.  */
+        qemu_log_mask(LOG_UNIMP,
+                      "NVIC: debug fault status register unimplemented\n");
+        break;
     case 0xd34: /* Mem Manage Address.  */
+        cpu->env.v7m.mmfar = value;
+        return;
     case 0xd38: /* Bus Fault Address.  */
+        cpu->env.v7m.bfar = value;
+        return;
     case 0xd3c: /* Aux Fault Status.  */
         qemu_log_mask(LOG_UNIMP,
-                      "NVIC: fault status registers unimplemented\n");
+                      "NVIC: Aux fault status registers unimplemented\n");
         break;
     case 0xf00: /* Software Triggered Interrupt Register */
         if ((value & 0x1ff) < NVIC_MAX_IRQ) {
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 4b7f78e..4262efc 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -396,6 +396,10 @@ typedef struct CPUARMState {
         uint32_t vecbase;
         uint32_t basepri;
         uint32_t control;
+        uint32_t cfsr; /* Configurable Fault Status */
+        uint32_t hfsr; /* HardFault Status */
+        uint32_t mmfar; /* MemManage Fault Address */
+        uint32_t bfar; /* BusFault Address */
         int current_sp;
         int exception;
         int exception_prio;
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 4490b74..d1ca011 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -5454,6 +5454,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.  */
@@ -5465,6 +5466,8 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
          * should set the MMFAR, etc from exception.fsr and exception.vaddress.
          */
         armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_MEM);
+        env->v7m.mmfar = env->exception.vaddress;
+        env->v7m.cfsr = (1<<1)|(1<<7); /* DACCVIOL and MMARVALID */
         break;
     case EXCP_BKPT:
         if (semihosting_enabled()) {
diff --git a/target-arm/machine.c b/target-arm/machine.c
index 36a0d15..14a4882 100644
--- a/target-arm/machine.c
+++ b/target-arm/machine.c
@@ -92,14 +92,18 @@ 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_UINT32(env.v7m.mmfar, ARMCPU),
+        VMSTATE_UINT32(env.v7m.bfar, ARMCPU),
         VMSTATE_INT32(env.v7m.current_sp, ARMCPU),
         VMSTATE_INT32(env.v7m.exception, ARMCPU),
         VMSTATE_END_OF_LIST()
-- 
2.1.4

Re: [Qemu-devel] [PATCH v2 09/26] armv7m: implement CFSR, HFSR, BFAR, and MMFAR

[Peter Maydell]

On 3 December 2015 at 00:18, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> Add the Configurable, HardFault, BusFault and MemManage Status registers.
> Note undefined instructions, violations, and escalations.
>
> No BusFaults are raised at this point.
> ---
>  hw/intc/armv7m_nvic.c | 28 ++++++++++++++++++++++------
>  target-arm/cpu.h      |  4 ++++
>  target-arm/helper.c   |  3 +++
>  target-arm/machine.c  |  8 ++++++--
>  4 files changed, 35 insertions(+), 8 deletions(-)
>
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index ca9bd4c..619c320 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -333,6 +333,7 @@ void armv7m_nvic_set_pending(void *opaque, int irq)
>              irq = ARMV7M_EXCP_HARD;
>              vec = &s->vectors[irq];
>
> +            s->cpu->env.v7m.hfsr |= 1<<30; /* FORCED */

I suggest defining some constants for the various FSR bits, eg

#define HFSR_FORCED (1U << 30)
#define HFSR_DEBUGEVT (1U << 31)

etc.

Then you don't need a comment every time you set a bit to say
what it means...

target-arm/cpu.h is probably the best place for these.

>              DPRINTF(0, "Escalate %d to HardFault\n", oldirq);
>          }
>      }
> @@ -548,16 +549,20 @@ static uint32_t nvic_readl(NVICState *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.  */
> +        qemu_log_mask(LOG_UNIMP, "Debug Fault status register unimplemented\n");
> +        return 0;
> +    case 0xd34: /* MMFAR MemManage Fault Address */
> +        return cpu->env.v7m.mmfar;
>      case 0xd38: /* Bus Fault Address.  */
> +        return cpu->env.v7m.bfar;
>      case 0xd3c: /* Aux Fault Status.  */
>          /* TODO: Implement fault status registers.  */
> -        qemu_log_mask(LOG_UNIMP, "Fault status registers unimplemented\n");
> +        qemu_log_mask(LOG_UNIMP,
> +                      "Aux Fault status registers unimplemented\n");
>          return 0;
>      case 0xd40: /* PFR0.  */
>          return 0x00000030;
> @@ -690,13 +695,24 @@ static void nvic_writel(NVICState *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.  */
> +        qemu_log_mask(LOG_UNIMP,
> +                      "NVIC: debug fault status register unimplemented\n");
> +        break;
>      case 0xd34: /* Mem Manage Address.  */
> +        cpu->env.v7m.mmfar = value;
> +        return;
>      case 0xd38: /* Bus Fault Address.  */
> +        cpu->env.v7m.bfar = value;
> +        return;
>      case 0xd3c: /* Aux Fault Status.  */
>          qemu_log_mask(LOG_UNIMP,
> -                      "NVIC: fault status registers unimplemented\n");
> +                      "NVIC: Aux fault status registers unimplemented\n");
>          break;
>      case 0xf00: /* Software Triggered Interrupt Register */
>          if ((value & 0x1ff) < NVIC_MAX_IRQ) {
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 4b7f78e..4262efc 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -396,6 +396,10 @@ typedef struct CPUARMState {
>          uint32_t vecbase;
>          uint32_t basepri;
>          uint32_t control;
> +        uint32_t cfsr; /* Configurable Fault Status */
> +        uint32_t hfsr; /* HardFault Status */
> +        uint32_t mmfar; /* MemManage Fault Address */
> +        uint32_t bfar; /* BusFault Address */
>          int current_sp;
>          int exception;
>          int exception_prio;
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 4490b74..d1ca011 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -5454,6 +5454,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 */

We get here for other things than UNDEFINSTR (eg NOCP and maybe
also DIVBYZERO and INVSTATE). You need to figure out which before you
can set the right CFSR bit. If you're lucky then exception.syndrome
will have sufficient information; if not then we may need to sort out
something else (probably borrowing syndrome to pass v7m specific
info as needed).

>          break;
>      case EXCP_SWI:
>          /* The PC already points to the next instruction.  */
> @@ -5465,6 +5466,8 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>           * should set the MMFAR, etc from exception.fsr and exception.vaddress.
>           */
>          armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_MEM);
> +        env->v7m.mmfar = env->exception.vaddress;
> +        env->v7m.cfsr = (1<<1)|(1<<7); /* DACCVIOL and MMARVALID */

This change is fixing the TODO comment which you can just see the
tail end of in the context, so you should remove that comment.

You should decode exception.fsr properly to distinguish whether
this is a BusFault or a MemManage fault and what subtype.
(Also whether this is EXCP_DATA_ABORT or EXCP_PREFETCH_ABORT
will tell you whether it's IACCVIOL or DACCVIOL.)

>          break;
>      case EXCP_BKPT:
>          if (semihosting_enabled()) {
> diff --git a/target-arm/machine.c b/target-arm/machine.c
> index 36a0d15..14a4882 100644
> --- a/target-arm/machine.c
> +++ b/target-arm/machine.c
> @@ -92,14 +92,18 @@ 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_UINT32(env.v7m.mmfar, ARMCPU),
> +        VMSTATE_UINT32(env.v7m.bfar, ARMCPU),
>          VMSTATE_INT32(env.v7m.current_sp, ARMCPU),
>          VMSTATE_INT32(env.v7m.exception, ARMCPU),
>          VMSTATE_END_OF_LIST()
> --
> 2.1.4

thanks
-- PMM

[Qemu-devel] [PATCH v2 10/26] armv7m: auto-clear FAULTMASK

[Michael Davidsaver]

on return from all exceptions other than NMI
---
 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 d1ca011..b6ec761 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

Re: [Qemu-devel] [PATCH v2 10/26] armv7m: auto-clear FAULTMASK

[Peter Maydell]

On 3 December 2015 at 00:18, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> on return from all exceptions other than NMI
> ---
>  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 d1ca011..b6ec761 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);
> +    }

Should the "clear PSTATE_F if this isn't NMI" check be inside
the "if exception != 0" if() ? It's not clear to me when this
function is called with exception equal to 0...

thanks
-- PMM

[Qemu-devel] [PATCH v2 11/26] 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.
---
 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 13e297d..2b09cd9 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -182,7 +182,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);
@@ -333,7 +333,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.
          */
@@ -512,7 +512,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)
@@ -670,7 +670,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;
         }
 
@@ -881,7 +881,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 {
@@ -899,7 +899,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);
@@ -910,7 +910,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

Re: [Qemu-devel] [PATCH v2 11/26] arm: gic: Remove references to NVIC

[Peter Maydell]

On 3 December 2015 at 00:18, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> armv7m_nvic.c no longer relies on the GIC.
> Remove REV_NVIC and conditionals which use it.
> ---
>  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

[Qemu-devel] [PATCH v2 12/26] 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.
---
 hw/intc/armv7m_nvic.c |  7 +++-
 target-arm/cpu.h      |  2 +-
 target-arm/helper.c   | 95 ++++++++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 94 insertions(+), 10 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 619c320..7d261ae 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -396,7 +396,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)
 {
     NVICState *s = (NVICState *)opaque;
     VecInfo *vec;
@@ -406,12 +406,17 @@ void armv7m_nvic_complete_irq(void *opaque, int irq)
 
     vec = &s->vectors[irq];
 
+    if (!vec->active) {
+        return true;
+    }
+
     vec->active = 0;
     vec->pending = vec->level;
     assert(!vec->level || irq >= 16);
 
     nvic_irq_update(s);
     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 4262efc..b98ef89 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -1043,7 +1043,7 @@ void armv7m_nvic_set_pending(void *opaque, int irq);
 bool armv7m_nvic_is_active(void *opaque, int irq);
 int armv7m_nvic_get_active_prio(void *opaque);
 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 b6ec761..f7e496d 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;
 
-    type = env->regs[15];
+    if (env->v7m.exception == 0) {
+        hw_error("Return from exception w/o active exception.  Logic error.");
+    }
+
     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 = -42; /* spoil, will be unstacked below */
+    env->v7m.exception_prio = armv7m_nvic_get_active_prio(env->nvic);
+
+    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_prio == 0x100) {
+            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_prio != 0x100) {
+            /* 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,14 +5456,46 @@ static void do_v7m_exception_exit(CPUARMState *env)
     }
     xpsr = v7m_pop(env);
     xpsr_write(env, xpsr, 0xfffffdff);
+
+    assert(env->v7m.exception!=-42);
+
     /* 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.  */
+
+    if (!ufault) {
+        /* consistency check between NVIC and guest stack */
+        if (env->v7m.exception == 0 && env->v7m.exception_prio != 0x100) {
+            ufault = 1;
+            qemu_log_mask(LOG_GUEST_ERROR, "Can't Unstacked to thread mode "
+                          "with active exception\n");
+            env->v7m.exception_prio = 0x100;
+
+        } else if (env->v7m.exception != 0 &&
+                   !armv7m_nvic_is_active(env->nvic, env->v7m.exception))
+        {
+            ufault = 1;
+            qemu_log_mask(LOG_GUEST_ERROR, "Unstacked exception %d is not "
+                          "active\n", env->v7m.exception);
+        } else  if (env->v7m.exception != 0
+                    && env->v7m.exception_prio == 0x100) {
+            hw_error("logic error at exception exit\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));
 }
 
 static
-- 
2.1.4

Re: [Qemu-devel] [PATCH v2 12/26] armv7m: check exception return consistency

[Peter Maydell]

On 3 December 2015 at 00:18, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> 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.
> ---
>  hw/intc/armv7m_nvic.c |  7 +++-
>  target-arm/cpu.h      |  2 +-
>  target-arm/helper.c   | 95 ++++++++++++++++++++++++++++++++++++++++++++++-----
>  3 files changed, 94 insertions(+), 10 deletions(-)
>
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 619c320..7d261ae 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -396,7 +396,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)
>  {
>      NVICState *s = (NVICState *)opaque;
>      VecInfo *vec;
> @@ -406,12 +406,17 @@ void armv7m_nvic_complete_irq(void *opaque, int irq)
>
>      vec = &s->vectors[irq];
>
> +    if (!vec->active) {
> +        return true;
> +    }
> +
>      vec->active = 0;
>      vec->pending = vec->level;
>      assert(!vec->level || irq >= 16);
>
>      nvic_irq_update(s);
>      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 4262efc..b98ef89 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1043,7 +1043,7 @@ void armv7m_nvic_set_pending(void *opaque, int irq);
>  bool armv7m_nvic_is_active(void *opaque, int irq);
>  int armv7m_nvic_get_active_prio(void *opaque);
>  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);

A brief comment here describing what the return value means would be nice.

>
>  /* 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 b6ec761..f7e496d 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;
>
> -    type = env->regs[15];
> +    if (env->v7m.exception == 0) {
> +        hw_error("Return from exception w/o active exception.  Logic error.");

Should this just be an assert(), or can the guest provoke this?

> +    }
> +
>      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 = -42; /* spoil, will be unstacked below */

I don't really like this. If you're going to do it please at least
use a symbolic constant rather than hardcoding -42.

> +    env->v7m.exception_prio = armv7m_nvic_get_active_prio(env->nvic);
> +
> +    type = env->regs[15] & 0xf;
> +    /* QEMU seems to clear the LSB at some point. */
> +    type |= 1;

This is a rather vague comment. The LSB of regs[15] is always
clear because Thumb PCs are 2-aligned. We don't ever store the
"Thumb mode" bit in regs[15] (and the hardware doesn't either).

> +
> +    switch (type) {
> +    case 0x1: /* Return to Handler mode */
> +        if (env->v7m.exception_prio == 0x100) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "Requesting return from exception "
> +                          "to Handler mode not allowed at base level of "
> +                          "activation");
> +            ufault = 1;

What is this test? None of the listed integrity checks in section
B1.5.8 of the v7M ARM ARM or the ExceptionReturn pseudocode cehck
the priority of the currently executing exception.

> +        }
> +        break;
> +    case 0x9: /* Return to Thread mode w/ Main stack */
> +    case 0xd: /* Return to Thread mode w/ Process stack */
> +        if (env->v7m.exception_prio != 0x100) {
> +            /* 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);

The error message and the comment talk about NONBASETHRDENA, but
the code isn't testing it. Also, you have nothing corresponding
to what the pseudocode NestedActivation variable is tracking
(ie is there a single active exception currently).

> +            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?
> +     */

There is a distinction if SP is currently pointing at an invalid
address -- if you try to unstack and stack again then that will
fault and the guest can see the difference. This is a corner case
that I'm happy with us just marking as TODO for the moment though.
(Also I think there may be a difference if we have a pending NMI
at this point, but I haven't thought that through.)

> +
>      /* Switch to the target stack.  */
>      switch_v7m_sp(env, (type & 4) != 0);
>      /* Pop registers.  */
> @@ -5409,14 +5456,46 @@ static void do_v7m_exception_exit(CPUARMState *env)
>      }
>      xpsr = v7m_pop(env);
>      xpsr_write(env, xpsr, 0xfffffdff);
> +
> +    assert(env->v7m.exception!=-42);
> +
>      /* 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.  */
> +
> +    if (!ufault) {
> +        /* consistency check between NVIC and guest stack */
> +        if (env->v7m.exception == 0 && env->v7m.exception_prio != 0x100) {
> +            ufault = 1;
> +            qemu_log_mask(LOG_GUEST_ERROR, "Can't Unstacked to thread mode "
> +                          "with active exception\n");
> +            env->v7m.exception_prio = 0x100;
> +
> +        } else if (env->v7m.exception != 0 &&
> +                   !armv7m_nvic_is_active(env->nvic, env->v7m.exception))
> +        {
> +            ufault = 1;
> +            qemu_log_mask(LOG_GUEST_ERROR, "Unstacked exception %d is not "
> +                          "active\n", env->v7m.exception);
> +        } else  if (env->v7m.exception != 0
> +                    && env->v7m.exception_prio == 0x100) {
> +            hw_error("logic error at exception exit\n");
> +        }
> +        /* ARM calls for PushStack() here, which should happen
> +         * went we return with a pending exception

"when"

> +         */
> +    }
> +
> +    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));
>  }
>
>  static

thanks
-- PMM

[Qemu-devel] [PATCH v2 13/26] armv7m: implement CCR

[Michael Davidsaver]

Implement Configuration and Control register.
Handle STACKALIGN and USERSETMPEND bits.
---
 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 7d261ae..0f9ca6a 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -534,8 +534,7 @@ static uint32_t nvic_readl(NVICState *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);
@@ -685,9 +684,17 @@ static void nvic_writel(NVICState *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 b98ef89..4e1b8cf 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 */
         uint32_t mmfar; /* MemManage Fault Address */
diff --git a/target-arm/helper.c b/target-arm/helper.c
index f7e496d..17d1ca0 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_prio != 0x100) {
+        if ((env->v7m.exception_prio != 0x100) && !(env->v7m.ccr & 1)) {
             /* Attempt to return to Thread mode
              * from nested handler while NONBASETHRDENA not set.
              */
@@ -5582,10 +5582,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->regs[13] & 4) && (env->v7m.ccr & (1<<9))) {
         env->regs[13] -= 4;
         xpsr |= 0x200;
     }
diff --git a/target-arm/machine.c b/target-arm/machine.c
index 14a4882..7aee41e 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_UINT32(env.v7m.mmfar, ARMCPU),
-- 
2.1.4

Re: [Qemu-devel] [PATCH v2 13/26] armv7m: implement CCR

[Peter Maydell]

On 3 December 2015 at 00:18, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> Implement Configuration and Control register.
> Handle STACKALIGN and USERSETMPEND bits.

The patch doesn't seem to do anything with USERSETMPEND,
though it does do something with NONBASETHRDENA.

> ---
>  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 7d261ae..0f9ca6a 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -534,8 +534,7 @@ static uint32_t nvic_readl(NVICState *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);
> @@ -685,9 +684,17 @@ static void nvic_writel(NVICState *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;

There's not much point in clearing out the bits we don't implement;
we may as well let them read-as-written.

> +        }
> +        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 b98ef89..4e1b8cf 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 */
>          uint32_t mmfar; /* MemManage Fault Address */
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index f7e496d..17d1ca0 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_prio != 0x100) {
> +        if ((env->v7m.exception_prio != 0x100) && !(env->v7m.ccr & 1)) {

A #define for CCR_NONBASETHRDENA would be nice (and for
CCR_STKALIGN).

>              /* Attempt to return to Thread mode
>               * from nested handler while NONBASETHRDENA not set.
>               */
> @@ -5582,10 +5582,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->regs[13] & 4) && (env->v7m.ccr & (1<<9))) {

Spaces around operators.

>          env->regs[13] -= 4;
>          xpsr |= 0x200;
>      }
> diff --git a/target-arm/machine.c b/target-arm/machine.c
> index 14a4882..7aee41e 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_UINT32(env.v7m.mmfar, ARMCPU),

Changing the vmstate implies that we should be bumping its
version number.

thanks
-- PMM

[Qemu-devel] [PATCH v2 14/26] armv7m: prevent unprivileged write to STIR

[Michael Davidsaver]

Prevent unprivileged from writing to the
Software Triggered Interrupt register
---
 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 0f9ca6a..5731146 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -727,7 +727,9 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value)
                       "NVIC: Aux 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 v2 14/26] armv7m: prevent unprivileged write to STIR

[Peter Maydell]

On 3 December 2015 at 00:18, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> Prevent unprivileged from writing to the

"unprivileged writes to the"

> Software Triggered Interrupt register

".".

> ---
>  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 0f9ca6a..5731146 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -727,7 +727,9 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value)
>                        "NVIC: Aux 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 */

You'll want to use the #define of CCR_USERSETMPEND that you'll
add to the previous patch. If you do that then the comment is
no longer really needed as the code itself is fairly straightforward.
(If you want to keep it then "privileged".)

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

thanks
-- PMM

[Qemu-devel] [PATCH v2 15/26] armv7m: add MPU to cortex-m3 and cortex-m4

[Michael Davidsaver]

The M series MPU is almost the same as the already
implemented R series MPU.  So use the M series
and translate.

Primary difference is that a real v7-M MPU is has
much relaxed alignment and size requirements for MPU
regions (32 bytes) compared with the 1K page size
of the QEMU TLB which is shared with all ARM targets.

Add MPU feature flag to cortex-m3 and -m4.

The v7-R MPU registers don't have a place for HFNMIENA,
so add another v7m. field.
---
 hw/arm/stellaris.c    |  11 ++++
 hw/intc/armv7m_nvic.c | 163 ++++++++++++++++++++++++++++++++++++++++++++++++--
 target-arm/cpu.c      |   2 +
 target-arm/cpu.h      |   1 +
 target-arm/helper.c   |   6 ++
 target-arm/machine.c  |   1 +
 6 files changed, 179 insertions(+), 5 deletions(-)

diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index 0114e0a..7e56f02 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -1255,6 +1255,17 @@ static void stellaris_init(const char *kernel_filename, const char *cpu_model,
     qdev_connect_gpio_out_named(nvic, "SYSRESETREQ", 0,
                                 qemu_allocate_irq(&do_sys_reset, NULL, 0));
 
+    {
+        /* hack to change the number of MPU regions.
+         * Less of a hack than messing with cpu_model string.
+         * Safe as long as the number is being reduced.
+         */
+        ARMCPU *cpu = ARM_CPU(qemu_get_cpu(0));
+        assert(cpu->pmsav7_dregion>=8);
+        cpu->pmsav7_dregion = 8;
+    }
+
+
     if (board->dc1 & (1 << 16)) {
         dev = sysbus_create_varargs(TYPE_STELLARIS_ADC, 0x40038000,
                                     qdev_get_gpio_in(nvic, 14),
diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 5731146..2b42d9d 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -595,8 +595,67 @@ static uint32_t nvic_readl(NVICState *s, uint32_t offset)
     case 0xd70: /* ISAR4.  */
         return 0x01310102;
     /* TODO: Implement debug registers.  */
+    case 0xd90: /* MPU_TYPE */
+        return cpu->has_mpu ? (cpu->pmsav7_dregion<<8) : 0;
+        break;
+    case 0xd94: /* MPU_CTRL */
+        val = 0;
+        /* We only use sctlr_el[1] since v7m has only two ELs unpriv. (0)
+         * and priv. (1).  The "controlling" EL is always priv.
+         */
+        if (cpu->env.cp15.sctlr_el[1] & SCTLR_M) {
+            val |= 1; /* ENABLE */
+        }
+        if (cpu->env.v7m.mpu_hfnmiena) {
+            val |= 2; /* HFNMIENA */
+        }
+        if (cpu->env.cp15.sctlr_el[1] & SCTLR_BR) {
+            val |= 4; /* PRIVDEFENA */
+        }
+        return val;
+    case 0xd98: /* MPU_RNR */
+        return cpu->env.cp15.c6_rgnr;
+    case 0xd9c: /* MPU_RBAR */
+    case 0xda4: /* MPU_RBAR_A1 */
+    case 0xdac: /* MPU_RBAR_A2 */
+    case 0xdb4: /* MPU_RBAR_A3 */
+    {
+        uint32_t range;
+        if (offset == 0xd9c) {
+            range = cpu->env.cp15.c6_rgnr;
+        } else {
+            range = (offset - 0xda4)/8;
+        }
+
+        if (range >= cpu->pmsav7_dregion) {
+            return 0;
+        } else {
+            return (cpu->env.pmsav7.drbar[range] & (0x1f)) | (range & 0xf);
+        }
+    }
+    case 0xda0: /* MPU_RASR */
+    case 0xda8: /* MPU_RASR_A1 */
+    case 0xdb0: /* MPU_RASR_A2 */
+    case 0xdb8: /* MPU_RASR_A3 */
+    {
+        uint32_t range;
+
+        if (offset == 0xda0) {
+            range = cpu->env.cp15.c6_rgnr;
+        } else {
+            range = (offset - 0xda8)/8;
+        }
+
+        if (range >= cpu->pmsav7_dregion) {
+            return 0;
+        } else {
+            return ((cpu->env.pmsav7.dracr[range] & 0xffff)<<16)
+                    | (cpu->env.pmsav7.drsr[range] & 0xffff);
+        }
+    }
     default:
-        qemu_log_mask(LOG_GUEST_ERROR, "NVIC: Bad read offset 0x%x\n", offset);
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "NVIC: Bad read offset 0x%x\n", offset);
         return 0;
     }
 }
@@ -726,11 +785,105 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value)
         qemu_log_mask(LOG_UNIMP,
                       "NVIC: Aux fault status registers unimplemented\n");
         break;
+    case 0xd90: /* MPU_TYPE (0xe000ed90) */
+        return; /* RO */
+    case 0xd94: /* MPU_CTRL */
+    {
+        if ((value & 3) == 2) {
+            qemu_log_mask(LOG_GUEST_ERROR, "MPU_CTRL: HFNMIENA and !ENABLE is "
+                          "UNPREDICTABLE\n");
+            /* we choice to ignore HFNMIENA when the MPU
+             * is not enabled.
+             */
+            value &= ~2;
+        }
+        if (value & 1) {
+            cpu->env.cp15.sctlr_el[1] |= SCTLR_M;
+        } else {
+            cpu->env.cp15.sctlr_el[1] &= ~SCTLR_M;
+        }
+        cpu->env.v7m.mpu_hfnmiena = !!(value & 2);
+        if (value & 4) {
+            cpu->env.cp15.sctlr_el[1] |= SCTLR_BR;
+        } else {
+            cpu->env.cp15.sctlr_el[1] &= ~SCTLR_BR;
+        }
+        tlb_flush(CPU(cpu), 1);
+    }
+        break;
+    case 0xd98: /* MPU_RNR */
+        if (value >= cpu->pmsav7_dregion) {
+            qemu_log_mask(LOG_GUEST_ERROR, "MPU region out of range %u/%u\n",
+                          (unsigned)value, (unsigned)cpu->pmsav7_dregion);
+        } else {
+            cpu->env.cp15.c6_rgnr = value;
+            DPRINTF(0, "MPU -> RGNR = %u\n", (unsigned)value);
+        }
+        tlb_flush(CPU(cpu), 1); /* necessary? */
+        break;
+    case 0xd9c: /* MPU_RBAR */
+    case 0xda4: /* MPU_RBAR_A1 */
+    case 0xdac: /* MPU_RBAR_A2 */
+    case 0xdb4: /* MPU_RBAR_A3 */
+    {
+        uint32_t range;
+        uint32_t base = value;
+
+        if (offset == 0xd9c) {
+            range = cpu->env.cp15.c6_rgnr;
+        } else {
+            range = (offset - 0xda4)/8;
+        }
+
+        if (value & (1<<4)) {
+            range = value & 0xf;
+
+            if (range >= cpu->pmsav7_dregion) {
+                qemu_log_mask(LOG_GUEST_ERROR,
+                              "MPU region out of range %u/%u\n",
+                              (unsigned)range,
+                              (unsigned)cpu->pmsav7_dregion);
+                return;
+            }
+            cpu->env.cp15.c6_rgnr = range;
+            base &= ~0x1f;
+
+        } else if (range >= cpu->pmsav7_dregion) {
+            return;
+        }
+
+        cpu->env.pmsav7.drbar[range] = base & ~0x3;
+        DPRINTF(0, "MPU -> DRBAR[%u] = %08x\n", range,
+                cpu->env.pmsav7.drbar[range]);
+    }
+        tlb_flush(CPU(cpu), 1);
+        break;
+    case 0xda0: /* MPU_RASR */
+    case 0xda8: /* MPU_RASR_A1 */
+    case 0xdb0: /* MPU_RASR_A2 */
+    case 0xdb8: /* MPU_RASR_A3 */
+    {
+        uint32_t range;
+
+        if (offset == 0xda0) {
+            range = cpu->env.cp15.c6_rgnr;
+        } else {
+            range = (offset-0xda8)/8;
+        }
+
+        cpu->env.pmsav7.drsr[range] = value & 0xff3f;
+        cpu->env.pmsav7.dracr[range] = (value>>16) & 0x173f;
+        DPRINTF(0, "MPU -> DRSR[%u] = %08x DRACR[%u] = %08x\n",
+                range, cpu->env.pmsav7.drsr[range],
+                range, cpu->env.pmsav7.dracr[range]);
+    }
+        tlb_flush(CPU(cpu), 1);
+        break;
     case 0xf00: /* Software Triggered Interrupt Register */
         /* 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);
+            armv7m_nvic_set_pending(s, (value & 0x1ff)+16);
         }
         break;
     default:
@@ -841,7 +994,7 @@ static void nvic_sysreg_write(void *opaque, hwaddr addr,
         offset = offset-0x180+16; /* vector # */
 
         for (i = 0, end = size*8; i < end && offset+i < s->num_irq; i++) {
-            if (value&(1<<i)) {
+            if (value & (1<<i)) {
                 s->vectors[offset+i].enabled = setval;
             }
         }
@@ -858,7 +1011,7 @@ static void nvic_sysreg_write(void *opaque, hwaddr addr,
         offset = offset-0x280+16; /* vector # */
 
         for (i = 0, end = size*8; i < end && offset+i < s->num_irq; i++) {
-            if (value&(1<<i)) {
+            if (value & (1<<i)) {
                 s->vectors[offset+i].pending = setval;
             }
         }
@@ -870,7 +1023,7 @@ static void nvic_sysreg_write(void *opaque, hwaddr addr,
         offset = offset-0x400+16; /* vector # */
 
         for (i = 0; i < size; i++) {
-            set_prio(s, offset+i, (value>>(i*8))&0xff);
+            set_prio(s, offset+i, (value>>(i*8)) & 0xff);
         }
         nvic_irq_update(s);
         s->cpu->env.v7m.exception_prio = armv7m_nvic_get_active_prio(s);
diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 6e3b251..1fa1f96 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -896,6 +896,7 @@ static void cortex_m3_initfn(Object *obj)
     ARMCPU *cpu = ARM_CPU(obj);
     set_feature(&cpu->env, ARM_FEATURE_V7);
     set_feature(&cpu->env, ARM_FEATURE_M);
+    set_feature(&cpu->env, ARM_FEATURE_MPU);
     cpu->midr = 0x410fc231;
 }
 
@@ -905,6 +906,7 @@ static void cortex_m4_initfn(Object *obj)
 
     set_feature(&cpu->env, ARM_FEATURE_V7);
     set_feature(&cpu->env, ARM_FEATURE_M);
+    set_feature(&cpu->env, ARM_FEATURE_MPU);
     set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP);
     cpu->midr = 0x410fc240; /* r0p0 */
 }
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 4e1b8cf..b93f8ae 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -401,6 +401,7 @@ typedef struct CPUARMState {
         uint32_t hfsr; /* HardFault Status */
         uint32_t mmfar; /* MemManage Fault Address */
         uint32_t bfar; /* BusFault Address */
+        unsigned mpu_hfnmiena; /* MPU_CTRL not mappable into SCTLR */
         int current_sp;
         int exception;
         int exception_prio;
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 17d1ca0..589aa54 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -6060,6 +6060,12 @@ static inline bool regime_translation_disabled(CPUARMState *env,
     if (mmu_idx == ARMMMUIdx_S2NS) {
         return (env->cp15.hcr_el2 & HCR_VM) == 0;
     }
+    if (IS_M(env) && !env->v7m.mpu_hfnmiena &&
+            ((env->v7m.exception > 0 && env->v7m.exception <= 3)
+             || (env->daif & PSTATE_F)))
+    {
+        return 1;
+    }
     return (regime_sctlr(env, mmu_idx) & SCTLR_M) == 0;
 }
 
diff --git a/target-arm/machine.c b/target-arm/machine.c
index 7aee41e..8852410 100644
--- a/target-arm/machine.c
+++ b/target-arm/machine.c
@@ -106,6 +106,7 @@ static const VMStateDescription vmstate_m = {
         VMSTATE_UINT32(env.v7m.mmfar, ARMCPU),
         VMSTATE_UINT32(env.v7m.bfar, ARMCPU),
         VMSTATE_INT32(env.v7m.current_sp, ARMCPU),
+        VMSTATE_UINT32(env.v7m.mpu_hfnmiena, ARMCPU),
         VMSTATE_INT32(env.v7m.exception, ARMCPU),
         VMSTATE_END_OF_LIST()
     }
-- 
2.1.4

[Qemu-devel] [PATCH v2 16/26] armv7m: add some mpu debugging prints

[Michael Davidsaver]

Provide some more "-d mmu" related to the MPU translation
process as an aid in debugging guest MPU configurations.
Helpful since our MPU resolution is limited to the ARM7-AR
page size.
---
 target-arm/helper.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 589aa54..da99825 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -7110,7 +7110,7 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
 
             if (base & rmask) {
                 qemu_log_mask(LOG_GUEST_ERROR, "DRBAR %" PRIx32 " misaligned "
-                              "to DRSR region size, mask = %" PRIx32,
+                              "to DRSR region size, mask = %" PRIx32 "\n",
                               base, rmask);
                 continue;
             }
@@ -7148,9 +7148,9 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
                 }
             }
             if (rsize < TARGET_PAGE_BITS) {
-                qemu_log_mask(LOG_UNIMP, "No support for MPU (sub)region"
+                qemu_log_mask(LOG_UNIMP, "No support for MPU[%u] (sub)region "
                               "alignment of %" PRIu32 " bits. Minimum is %d\n",
-                              rsize, TARGET_PAGE_BITS);
+                              n, rsize, TARGET_PAGE_BITS);
                 continue;
             }
             if (srdis) {
@@ -7164,11 +7164,14 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
                 (is_user || !(regime_sctlr(env, mmu_idx) & SCTLR_BR))) {
                 /* background fault */
                 *fsr = 0;
+
+                qemu_log_mask(CPU_LOG_MMU, "Miss MPU\n");
                 return true;
             }
             get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
         } else { /* a MPU hit! */
             uint32_t ap = extract32(env->pmsav7.dracr[n], 8, 3);
+            qemu_log_mask(CPU_LOG_MMU, "Hit MPU %u AP %08x\n", n, (unsigned)ap);
 
             if (is_user) { /* User mode AP bit decoding */
                 switch (ap) {
@@ -7382,9 +7385,15 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,
      */
     if (arm_feature(env, ARM_FEATURE_MPU) &&
         arm_feature(env, ARM_FEATURE_V7)) {
+        bool ret;
         *page_size = TARGET_PAGE_SIZE;
-        return get_phys_addr_pmsav7(env, address, access_type, mmu_idx,
-                                    phys_ptr, prot, fsr);
+        ret = get_phys_addr_pmsav7(env, address, access_type, mmu_idx,
+                                   phys_ptr, prot, fsr);
+        qemu_log_mask(CPU_LOG_MMU, "TLB %08x mmu_idx=%u AC %u -> AC %u %s\n",
+                      (unsigned)address, mmu_idx, 1<<access_type, *prot,
+                      ret ? "Miss" : "Hit");
+
+        return ret;
     }
 
     if (regime_translation_disabled(env, mmu_idx)) {
-- 
2.1.4

[Qemu-devel] [PATCH v2 17/26] armv7m: mpu background miss is perm fault

[Michael Davidsaver]

Set an appropriate FSR code when an access does
not match any MPU region, including the background/default.
---
 target-arm/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index da99825..e73f7a6 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -7163,7 +7163,7 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
             if (cpu->pmsav7_dregion &&
                 (is_user || !(regime_sctlr(env, mmu_idx) & SCTLR_BR))) {
                 /* background fault */
-                *fsr = 0;
+                *fsr = 0x00d; /* Permission fault */
 
                 qemu_log_mask(CPU_LOG_MMU, "Miss MPU\n");
                 return true;
-- 
2.1.4

[Qemu-devel] [PATCH v2 18/26] armv7m: update base region policy

[Michael Davidsaver]

Update MPU background policy as per ARM.
Main changes are preventing writes to ROM
and no-exec for device regions.
---
 target-arm/helper.c | 35 +++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index e73f7a6..e42f6d0 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -7062,16 +7062,35 @@ static inline void get_phys_addr_pmsav7_default(CPUARMState *env,
                                                 ARMMMUIdx mmu_idx,
                                                 int32_t address, int *prot)
 {
-    *prot = PAGE_READ | PAGE_WRITE;
-    switch (address) {
-    case 0xF0000000 ... 0xFFFFFFFF:
-        if (regime_sctlr(env, mmu_idx) & SCTLR_V) { /* hivecs execing is ok */
+    if (!IS_M(env)) {
+        *prot = PAGE_READ | PAGE_WRITE;
+        switch (address) {
+        case 0xF0000000 ... 0xFFFFFFFF:
+            if (regime_sctlr(env, mmu_idx) & SCTLR_V) {
+                /* hivecs execing is ok */
+                *prot |= PAGE_EXEC;
+            }
+            break;
+        case 0x00000000 ... 0x7FFFFFFF:
             *prot |= PAGE_EXEC;
+            break;
+        }
+    } else {
+        /* ARM specfies XN (PAGE_EXEC) but leaves R/W to implementation.
+         * Mark ROM as read only since writes would otherwise be ignored.
+         */
+        switch (address) {
+        case 0 ... 0x1fffffff: /* ROM */
+            *prot = PAGE_READ | PAGE_EXEC;
+            break;
+        case 0x20000000 ... 0x3fffffff:  /* SRAM */
+        case 0x60000000 ... 0x7fffffff: /* RAM */
+        case 0x80000000 ... 0x9fffffff: /* RAM */
+            *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
+            break;
+        default: /* Peripheral, 2x Device, and System */
+            *prot = PAGE_READ | PAGE_WRITE;
         }
-        break;
-    case 0x00000000 ... 0x7FFFFFFF:
-        *prot |= PAGE_EXEC;
-        break;
     }
 
 }
-- 
2.1.4

[Qemu-devel] [PATCH v2 19/26] armv7m: mpu not allowed to map exception return codes

[Michael Davidsaver]

Always pass these through to be caught be by the
unassigned handler.
---
 target-arm/helper.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index e42f6d0..a5adf2d 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -7106,6 +7106,15 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
     *phys_ptr = address;
     *prot = 0;
 
+    /* Magic exception codes returns always pass through the MPU
+     * to be trapped later in arm_v7m_unassigned_access()
+     */
+    if (IS_M(env) && env->v7m.exception != 0 && address >= 0xfffffff0) {
+        *prot = PAGE_EXEC;
+        *fsr = 0;
+        return false;
+    }
+
     if (regime_translation_disabled(env, mmu_idx)) { /* MPU disabled */
         get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
     } else { /* MPU enabled */
-- 
2.1.4

[Qemu-devel] [PATCH v2 20/26] armv7m: observable initial register state

[Michael Davidsaver]

At least for TI TM4C1294.
LR==-1
XPSR==0
PRIMASK, FAULTMASK, and BASEPRI all cleared
so exception handlers are unmasked.
STKALIGN set.
---
 target-arm/cpu.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 1fa1f96..8b85888 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -175,7 +175,10 @@ static void arm_cpu_reset(CPUState *s)
 
         env->v7m.exception_prio = env->v7m.pending_prio = 0x100;
 
-        env->daif &= ~PSTATE_I;
+        env->v7m.ccr = 1<<9; /* STKALIGN */
+
+        env->daif &= ~(PSTATE_I|PSTATE_F);
+        env->ZF = 1;
         rom = rom_ptr(0);
         if (rom) {
             /* Address zero is covered by ROM which hasn't yet been
@@ -194,6 +197,7 @@ static void arm_cpu_reset(CPUState *s)
         }
 
         env->regs[13] = initial_msp & 0xFFFFFFFC;
+        env->regs[14] = 0xffffffff;
         env->regs[15] = initial_pc & ~1;
         env->thumb = initial_pc & 1;
     }
-- 
2.1.4

[Qemu-devel] [PATCH v2 21/26] armv7m: CONTROL<1> handling

[Michael Davidsaver]

The ARM states that this bit indicates the stack
being used, which in handler mode is always MSP.
Further CONTROL<1>==1 in handler mode is reserved.

With the TM4C always CONTROL<1>==0 in handler mode
which inconveniently prevents the handler from
knowing which stack thread mode was using...

This bit is a direct indication of which stack pointer
register is "aliased" to r13, so easiest to eliminate
the now redundant current_sp field.
---
 target-arm/cpu.h     |  1 -
 target-arm/helper.c  | 20 ++++++++++----------
 target-arm/machine.c |  1 -
 3 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index b93f8ae..01c9cdb 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -402,7 +402,6 @@ typedef struct CPUARMState {
         uint32_t mmfar; /* MemManage Fault Address */
         uint32_t bfar; /* BusFault Address */
         unsigned mpu_hfnmiena; /* MPU_CTRL not mappable into SCTLR */
-        int current_sp;
         int exception;
         int exception_prio;
         unsigned pending;
diff --git a/target-arm/helper.c b/target-arm/helper.c
index a5adf2d..2661da4 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -5362,14 +5362,14 @@ static uint32_t v7m_pop(CPUARMState *env)
 }
 
 /* Switch to V7M main or process stack pointer.  */
-static void switch_v7m_sp(CPUARMState *env, int process)
+static void switch_v7m_sp(CPUARMState *env, bool process)
 {
     uint32_t tmp;
-    if (env->v7m.current_sp != process) {
+    if (!!(env->v7m.control & 2) != process) {
         tmp = env->v7m.other_sp;
         env->v7m.other_sp = env->regs[13];
         env->regs[13] = tmp;
-        env->v7m.current_sp = process;
+        env->v7m.control = (env->v7m.control & ~2) | (process ? 2 : 0);
     }
 }
 
@@ -5457,7 +5457,7 @@ static void do_v7m_exception_exit(CPUARMState *env)
     xpsr = v7m_pop(env);
     xpsr_write(env, xpsr, 0xfffffdff);
 
-    assert(env->v7m.exception!=-42);
+    assert(env->v7m.exception != -42);
 
     /* Undo stack alignment.  */
     if (xpsr & 0x200)
@@ -5528,7 +5528,7 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
     arm_log_exception(cs->exception_index);
 
     lr = 0xfffffff1;
-    if (env->v7m.current_sp)
+    if (env->v7m.control & 2)
         lr |= 4;
     if (env->v7m.exception == 0)
         lr |= 8;
@@ -7550,9 +7550,9 @@ uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t reg)
 
     switch (reg) {
     case 8: /* MSP */
-        return env->v7m.current_sp ? env->v7m.other_sp : env->regs[13];
+        return env->v7m.control & 2 ? env->v7m.other_sp : env->regs[13];
     case 9: /* PSP */
-        return env->v7m.current_sp ? env->regs[13] : env->v7m.other_sp;
+        return env->v7m.control & 2 ? env->regs[13] : env->v7m.other_sp;
     case 16: /* PRIMASK */
         return (env->daif & PSTATE_I) != 0;
     case 17: /* BASEPRI */
@@ -7582,13 +7582,13 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t reg, uint32_t val)
         }
         break;
     case 8: /* MSP */
-        if (env->v7m.current_sp)
+        if (env->v7m.control & 2)
             env->v7m.other_sp = val;
         else
             env->regs[13] = val;
         break;
     case 9: /* PSP */
-        if (env->v7m.current_sp)
+        if (env->v7m.control & 2)
             env->regs[13] = val;
         else
             env->v7m.other_sp = val;
@@ -7616,8 +7616,8 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t reg, uint32_t val)
         }
         break;
     case 20: /* CONTROL */
-        env->v7m.control = val & 3;
         switch_v7m_sp(env, (val & 2) != 0);
+        env->v7m.control = (env->v7m.control & ~1) | (val & 1);
         break;
     default:
         qemu_log_mask(LOG_GUEST_ERROR, "Attempt to write unknown special"
diff --git a/target-arm/machine.c b/target-arm/machine.c
index 8852410..dab1626 100644
--- a/target-arm/machine.c
+++ b/target-arm/machine.c
@@ -105,7 +105,6 @@ static const VMStateDescription vmstate_m = {
         VMSTATE_UINT32(env.v7m.hfsr, ARMCPU),
         VMSTATE_UINT32(env.v7m.mmfar, ARMCPU),
         VMSTATE_UINT32(env.v7m.bfar, ARMCPU),
-        VMSTATE_INT32(env.v7m.current_sp, ARMCPU),
         VMSTATE_UINT32(env.v7m.mpu_hfnmiena, ARMCPU),
         VMSTATE_INT32(env.v7m.exception, ARMCPU),
         VMSTATE_END_OF_LIST()
-- 
2.1.4

[Qemu-devel] [PATCH v2 22/26] armv7m: priority field mask

[Michael Davidsaver]

Many v7m CPUs don't implement all of the 8 bits
of the priority fields.  Typically, only the
top N bits are available.  Existing practice
implies that writes to unimplemented
bits will be ignore, and read as zero.

This allows a guest to discover the implemented
bits by writing 0xff to (eg. basepri).
The value read back will have only the available
bits set.
---
 hw/intc/armv7m_nvic.c | 2 ++
 target-arm/cpu-qom.h  | 6 ++++++
 target-arm/cpu.c      | 7 +++++++
 target-arm/helper.c   | 6 ++++--
 4 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 2b42d9d..e2410a3 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -149,6 +149,8 @@ void set_prio(NVICState *s, unsigned irq, uint8_t prio)
     assert(irq > 3); /* only use for configurable prios */
     assert(irq < NVIC_MAX_VECTORS);
 
+    prio &= s->cpu->v7m_priority_mask;
+
     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;
diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
index 25fb1ce..79cf591 100644
--- a/target-arm/cpu-qom.h
+++ b/target-arm/cpu-qom.h
@@ -108,6 +108,12 @@ typedef struct ARMCPU {
     /* PMSAv7 MPU number of supported regions */
     uint32_t pmsav7_dregion;
 
+    /* Some v7-M targets don't impliment the full 8 bits
+     * of the priority fields.  Writes to unimplimented
+     * bits are treated as zero (guest can discover mask).
+     */
+    uint8_t v7m_priority_mask;
+
     /* PSCI conduit used to invoke PSCI methods
      * 0 - disabled, 1 - smc, 2 - hvc
      */
diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 8b85888..27cf482 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -527,6 +527,9 @@ static Property arm_cpu_has_mpu_property =
 static Property arm_cpu_pmsav7_dregion_property =
             DEFINE_PROP_UINT32("pmsav7-dregion", ARMCPU, pmsav7_dregion, 16);
 
+static Property armv7m_priority_mask_property =
+        DEFINE_PROP_UINT8("priority-mask", ARMCPU, v7m_priority_mask, 0xff);
+
 static void arm_cpu_post_init(Object *obj)
 {
     ARMCPU *cpu = ARM_CPU(obj);
@@ -565,6 +568,10 @@ static void arm_cpu_post_init(Object *obj)
         }
     }
 
+    if (IS_M(&cpu->env) && arm_feature(&cpu->env, ARM_FEATURE_V7)) {
+        qdev_property_add_static(DEVICE(obj), &armv7m_priority_mask_property,
+                                 &error_abort);
+    }
 }
 
 static void arm_cpu_finalizefn(Object *obj)
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 2661da4..c890b3a 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -7569,6 +7569,8 @@ uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t reg)
 
 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;
@@ -7601,10 +7603,10 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t reg, uint32_t val)
         }
         break;
     case 17: /* BASEPRI */
-        env->v7m.basepri = val & 0xff;
+        env->v7m.basepri = val & cpu->v7m_priority_mask;
         break;
     case 18: /* BASEPRI_MAX */
-        val &= 0xff;
+        val &= cpu->v7m_priority_mask;
         if (val != 0 && (val < env->v7m.basepri || env->v7m.basepri == 0))
             env->v7m.basepri = val;
         break;
-- 
2.1.4

[Qemu-devel] [PATCH v2 23/26] qom: add cpu_generic_init_unrealized()

[Michael Davidsaver]

cpu_generic_init() without realized=true.
Gives board code an opportunity to change
CPU properties.
---
 include/qom/cpu.h | 12 ++++++++++++
 qom/cpu.c         | 23 +++++++++++++++++------
 2 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 51a1323..9093500 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -489,6 +489,18 @@ ObjectClass *cpu_class_by_name(const char *typename, const char *cpu_model);
 CPUState *cpu_generic_init(const char *typename, const char *cpu_model);
 
 /**
+ * cpu_generic_init_unrealized:
+ * @typename: The CPU base type.
+ * @cpu_model: The model string including optional parameters.
+ *
+ * Instantiates a CPU, processes optional parameters but does not realize it.
+ *
+ * Returns: A #CPUState or %NULL if an error occurred.
+ */
+CPUState *cpu_generic_init_unrealized(const char *typename,
+                                      const char *cpu_model);
+
+/**
  * cpu_has_work:
  * @cpu: The vCPU to check.
  *
diff --git a/qom/cpu.c b/qom/cpu.c
index fb80d13..f622fc2 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -42,6 +42,23 @@ bool cpu_exists(int64_t id)
 
 CPUState *cpu_generic_init(const char *typename, const char *cpu_model)
 {
+    CPUState *cpu = cpu_generic_init_unrealized(typename, cpu_model);
+    if (cpu) {
+        Error *err = NULL;
+        object_property_set_bool(OBJECT(cpu), true, "realized", &err);
+
+        if (err != NULL) {
+            error_report_err(err);
+            object_unref(OBJECT(cpu));
+            return NULL;
+        }
+    }
+    return cpu;
+}
+
+CPUState *cpu_generic_init_unrealized(const char *typename,
+                                      const char *cpu_model)
+{
     char *str, *name, *featurestr;
     CPUState *cpu;
     ObjectClass *oc;
@@ -63,13 +80,7 @@ CPUState *cpu_generic_init(const char *typename, const char *cpu_model)
     featurestr = strtok(NULL, ",");
     cc->parse_features(cpu, featurestr, &err);
     g_free(str);
-    if (err != NULL) {
-        goto out;
-    }
-
-    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
 
-out:
     if (err != NULL) {
         error_report_err(err);
         object_unref(OBJECT(cpu));
-- 
2.1.4

[Qemu-devel] [PATCH v2 24/26] armv7m: split armv7m_init in two parts

[Michael Davidsaver]

Separate init and realize phases to allow
board code the opportunity to set properties
on the cpu and nvic.

Assign names for cpu, nvic, and bitband regions.

update stellaris and stm32 board code accordingly.
---
 hw/arm/armv7m.c        | 42 +++++++++++++++++++++++++++---------------
 hw/arm/stellaris.c     | 18 +++++-------------
 hw/arm/stm32f205_soc.c |  6 ++++--
 include/hw/arm/arm.h   |  4 ++--
 4 files changed, 38 insertions(+), 32 deletions(-)

diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index 68146de..fb805fe 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -144,11 +144,15 @@ static void armv7m_bitband_init(void)
 
     dev = qdev_create(NULL, TYPE_BITBAND);
     qdev_prop_set_uint32(dev, "base", 0x20000000);
+    object_property_add_child(qdev_get_machine(), "bitband22",
+                              &dev->parent_obj, &error_fatal);
     qdev_init_nofail(dev);
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0x22000000);
 
     dev = qdev_create(NULL, TYPE_BITBAND);
     qdev_prop_set_uint32(dev, "base", 0x40000000);
+    object_property_add_child(qdev_get_machine(), "bitband42",
+                              &dev->parent_obj, &error_fatal);
     qdev_init_nofail(dev);
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0x42000000);
 }
@@ -162,39 +166,48 @@ static void armv7m_reset(void *opaque)
     cpu_reset(CPU(cpu));
 }
 
-/* Init CPU and memory for a v7-M based board.
-   mem_size is in bytes.
-   Returns the NVIC array.  */
-
-DeviceState *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
-                      const char *kernel_filename, const char *cpu_model)
+void armv7m_init(const char *cpu_model)
 {
     ARMCPU *cpu;
     CPUARMState *env;
     DeviceState *nvic;
-    int image_size;
-    uint64_t entry;
-    uint64_t lowaddr;
-    int big_endian;
 
     if (cpu_model == NULL) {
-	cpu_model = "cortex-m3";
+        cpu_model = "cortex-m3";
     }
-    cpu = cpu_arm_init(cpu_model);
+    cpu = ARM_CPU(cpu_generic_init_unrealized(TYPE_ARM_CPU, cpu_model));
     if (cpu == NULL) {
         fprintf(stderr, "Unable to find CPU definition\n");
         exit(1);
     }
     env = &cpu->env;
 
+    object_property_add_child(qdev_get_machine(), "cpu[*]", OBJECT(cpu),
+                              &error_fatal);
+
     armv7m_bitband_init();
 
     nvic = qdev_create(NULL, "armv7m_nvic");
-    qdev_prop_set_uint32(nvic, "num-irq", num_irq);
+    object_property_add_child(qdev_get_machine(), "nvic", &nvic->parent_obj,
+                              &error_fatal);
     env->nvic = nvic;
-    qdev_init_nofail(nvic);
+
     sysbus_connect_irq(SYS_BUS_DEVICE(nvic), 0,
                        qdev_get_gpio_in(DEVICE(cpu), ARM_CPU_IRQ));
+}
+
+
+void armv7m_realize(int mem_size, const char *kernel_filename)
+{
+    ARMCPU *cpu = ARM_CPU(first_cpu);
+    DeviceState *nvic = DEVICE(object_resolve_path("/machine/nvic", NULL));
+    int image_size;
+    uint64_t entry;
+    uint64_t lowaddr;
+    int big_endian;
+
+    qdev_init_nofail(DEVICE(cpu));
+    qdev_init_nofail(nvic);
 
 #ifdef TARGET_WORDS_BIGENDIAN
     big_endian = 1;
@@ -221,7 +234,6 @@ DeviceState *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
     }
 
     qemu_register_reset(armv7m_reset, cpu);
-    return nvic;
 }
 
 static Property bitband_properties[] = {
diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index 7e56f02..3f12b57 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -1249,23 +1249,15 @@ static void stellaris_init(const char *kernel_filename, const char *cpu_model,
     vmstate_register_ram_global(sram);
     memory_region_add_subregion(system_memory, 0x20000000, sram);
 
-    nvic = armv7m_init(system_memory, flash_size, NUM_IRQ_LINES,
-                      kernel_filename, cpu_model);
+    armv7m_init(cpu_model);
+    qdev_prop_set_uint32(&first_cpu->parent_obj, "pmsav7-dregion", 8);
+    nvic = DEVICE(object_resolve_path("/machine/nvic", NULL));
+    qdev_prop_set_uint32(nvic, "num-irq", NUM_IRQ_LINES);
+    armv7m_realize(flash_size, kernel_filename);
 
     qdev_connect_gpio_out_named(nvic, "SYSRESETREQ", 0,
                                 qemu_allocate_irq(&do_sys_reset, NULL, 0));
 
-    {
-        /* hack to change the number of MPU regions.
-         * Less of a hack than messing with cpu_model string.
-         * Safe as long as the number is being reduced.
-         */
-        ARMCPU *cpu = ARM_CPU(qemu_get_cpu(0));
-        assert(cpu->pmsav7_dregion>=8);
-        cpu->pmsav7_dregion = 8;
-    }
-
-
     if (board->dc1 & (1 << 16)) {
         dev = sysbus_create_varargs(TYPE_STELLARIS_ADC, 0x40038000,
                                     qdev_get_gpio_in(nvic, 14),
diff --git a/hw/arm/stm32f205_soc.c b/hw/arm/stm32f205_soc.c
index 3f99340..01ae1e7 100644
--- a/hw/arm/stm32f205_soc.c
+++ b/hw/arm/stm32f205_soc.c
@@ -87,8 +87,10 @@ static void stm32f205_soc_realize(DeviceState *dev_soc, Error **errp)
     vmstate_register_ram_global(sram);
     memory_region_add_subregion(system_memory, SRAM_BASE_ADDRESS, sram);
 
-    nvic = armv7m_init(get_system_memory(), FLASH_SIZE, 96,
-                       s->kernel_filename, s->cpu_model);
+    armv7m_init(s->cpu_model);
+    nvic = DEVICE(object_resolve_path("/machine/nvic", NULL));
+    qdev_prop_set_uint32(nvic, "num-irq", 96);
+    armv7m_realize(FLASH_SIZE, s->kernel_filename);
 
     /* System configuration controller */
     syscfgdev = DEVICE(&s->syscfg);
diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
index c26b0e3..2d81ff1 100644
--- a/include/hw/arm/arm.h
+++ b/include/hw/arm/arm.h
@@ -17,8 +17,8 @@
 #include "cpu.h"
 
 /* armv7m.c */
-DeviceState *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
-                      const char *kernel_filename, const char *cpu_model);
+void armv7m_init(const char *cpu_model);
+void armv7m_realize(int mem_size, const char *kernel_filename);
 
 /*
  * struct used as a parameter of the arm_load_kernel machine init
-- 
2.1.4

[Qemu-devel] [PATCH v2 25/26] armv7m: remove extra cpu_reset()

[Michael Davidsaver]

cpu_reset() is called as a side-effect
of realizing the CPU.
arm_cpu_reset() calls rom_ptr(0), which
expects to find the image mapped.
This was happening way before load_*()
and was worked around with a second call to cpu_reset().
Now wait to realize until after the image is mapped.
---
 hw/arm/armv7m.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index fb805fe..41b9596 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -159,13 +159,6 @@ static void armv7m_bitband_init(void)
 
 /* Board init.  */
 
-static void armv7m_reset(void *opaque)
-{
-    ARMCPU *cpu = opaque;
-
-    cpu_reset(CPU(cpu));
-}
-
 void armv7m_init(const char *cpu_model)
 {
     ARMCPU *cpu;
@@ -206,9 +199,6 @@ void armv7m_realize(int mem_size, const char *kernel_filename)
     uint64_t lowaddr;
     int big_endian;
 
-    qdev_init_nofail(DEVICE(cpu));
-    qdev_init_nofail(nvic);
-
 #ifdef TARGET_WORDS_BIGENDIAN
     big_endian = 1;
 #else
@@ -233,7 +223,11 @@ void armv7m_realize(int mem_size, const char *kernel_filename)
         }
     }
 
-    qemu_register_reset(armv7m_reset, cpu);
+    /* Realizing cpu calls cpu_reset(), which must have rom image
+     * already mapped to find the correct entry point.
+     */
+    qdev_init_nofail(DEVICE(cpu));
+    qdev_init_nofail(nvic);
 }
 
 static Property bitband_properties[] = {
-- 
2.1.4

[Qemu-devel] [PATCH v2 26/26] armv7m: decide whether faults are MemManage or BusFault

[Michael Davidsaver]

General logic is that operations stopped by the MPU are MemManage,
and those which go through the MPU and are caught by the unassigned
handle are BusFault.
---
 target-arm/helper.c | 35 +++++++++++++++++++++++++++++------
 1 file changed, 29 insertions(+), 6 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index c890b3a..630d5c9 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -5546,12 +5546,35 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
         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);
-        env->v7m.mmfar = env->exception.vaddress;
-        env->v7m.cfsr = (1<<1)|(1<<7); /* DACCVIOL and MMARVALID */
+        switch (env->exception.fsr & 0xf) {
+        case 0x8: /* External Abort */
+            switch (cs->exception_index) {
+            case EXCP_PREFETCH_ABORT:
+                env->v7m.cfsr |= (1<<(8+1)); /* PRECISERR */
+                break;
+            case EXCP_DATA_ABORT:
+                env->v7m.cfsr |= (1<<(8+0)); /* IBUSERR */
+                break;
+            }
+            armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_BUS);
+            env->v7m.bfar = env->exception.vaddress;
+            env->v7m.cfsr |= (1<<(8+7)); /* BFARVALID */
+            break;
+        case 0xd: /* Permission fault */
+        default:
+            switch (cs->exception_index) {
+            case EXCP_PREFETCH_ABORT:
+                env->v7m.cfsr |= (1<<0); /* IACCVIOL */
+                break;
+            case EXCP_DATA_ABORT:
+                env->v7m.cfsr |= (1<<1); /* DACCVIOL */
+                break;
+            }
+            armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_MEM);
+            env->v7m.mmfar = env->exception.vaddress;
+            env->v7m.cfsr |= (1<<7); /* MMARVALID */
+            break;
+        }
         break;
     case EXCP_BKPT:
         if (semihosting_enabled()) {
-- 
2.1.4

Re: [Qemu-devel] [PATCH v2 00/26] armv7m: exception handling, MPU, and more

[Peter Maydell]

On 3 December 2015 at 00:18, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> All,
>
> Second revision of ARMv7-M exception handling patchset, which now adds
> MPU support (as well as can be done).

I just sent this as a followup to the v1 cover letter by mistake;
here it is again:

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.

We do want MPU support for our M profile models, but I think it'll
be faster to concentrate on one thing at a time.

thanks
-- PMM