[Qemu-devel] [PATCH 0/7] target/arm: Implement M profile derived exceptions

[Qemu-devel] [PATCH 0/7] target/arm: Implement M profile derived exceptions

[Peter Maydell]

In the Arm M-profile architecture, the process of taking or
returning from an exception can itself cause an exception
(for instance if there is an MPU permissions fault when
writing or reading the exception stack frame). This is called
a derived exception. Currently we don't implement this at
all in QEMU, instead just doing direct physical address
loads and stores which bypass MPU/SAU checks.

This patchset changes all the loads and stores we do on
exception entry and exit so that they do the MPU and SAU checks
and handle failures in the architecturally required way.

A note for reviewers: the way I've structured handling of
derived exceptions on exception entry diverges a bit from the
structure of the v8M Arm ARM pseudocode. In the pseudocode,
derived exceptions cause the attempt to process the original
exception to be abandoned (either without calling TakeException,
or bailing out of TakeException partway through). Then at the top
level the pseudocode calls DerivedLateArrival to prioritize the
derived exception and call TakeException from there.
For the QEMU implementation, I chose to let the NVIC do the
prioritization and continue forward, so that the call to
v7m_exception_taken() will then take either the original or the
erived exception. The effect is the same, but this structure works
better for QEMU, because we don't have a convenient top level place
to do the abandon-and-retry logic.

(The motivation for filling in this missing bit of functionality
is that the Zephyr RTOS would like to implement stack-overrun
checking on v7M cores using the MPU, so noticing permissions
failures when exceptions frames are written to the stack is
important.)

Peter Maydell (7):
  target/arm: Add armv7m_nvic_set_pending_derived()
  target/arm: Split "get pending exception info" from "acknowledge it"
  target/arm: Add ignore_stackfaults argument to v7m_exception_taken()
  target/arm: Make v7M exception entry stack push check MPU
  target/arm: Make v7m_push_callee_stack() honour MPU
  target/arm: Make exception vector loads honour the SAU
  target/arm: Handle exceptions during exception stack pop

 target/arm/cpu.h      |  32 ++++-
 hw/intc/armv7m_nvic.c |  98 +++++++++++--
 target/arm/helper.c   | 392 ++++++++++++++++++++++++++++++++++++++++----------
 hw/intc/trace-events  |   5 +-
 4 files changed, 435 insertions(+), 92 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/7] target/arm: Add armv7m_nvic_set_pending_derived()

[Peter Maydell]

In order to support derived exceptions (exceptions generated in
the course of trying to take an exception), we need to be able
to handle prioritizing whether to take the original exception
or the derived exception.

We do this by introducing a new function
armv7m_nvic_set_pending_derived() which the exception-taking code in
helper.c will call when a derived exception occurs.  Derived
exceptions are dealt with mostly like normal pending exceptions, so
we share the implementation with the armv7m_nvic_set_pending()
function.

Note that the way we structure this is significantly different
from the v8M Arm ARM pseudocode: that does all the prioritization
logic in the DerivedLateArrival() function, whereas we choose to
let the existing "identify highest priority exception" logic
do the prioritization for us. The effect is the same, though.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.h      | 13 ++++++++++
 hw/intc/armv7m_nvic.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++--
 hw/intc/trace-events  |  2 +-
 3 files changed, 80 insertions(+), 3 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index d2bb59e..9ed03e6 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1506,6 +1506,19 @@ static inline bool armv7m_nvic_can_take_pending_exception(void *opaque)
  */
 void armv7m_nvic_set_pending(void *opaque, int irq, bool secure);
 /**
+ * armv7m_nvic_set_pending_derived: mark this derived exception as pending
+ * @opaque: the NVIC
+ * @irq: the exception number to mark pending
+ * @secure: false for non-banked exceptions or for the nonsecure
+ * version of a banked exception, true for the secure version of a banked
+ * exception.
+ *
+ * Similar to armv7m_nvic_set_pending(), but specifically for derived
+ * exceptions (exceptions generated in the course of trying to take
+ * a different exception).
+ */
+void armv7m_nvic_set_pending_derived(void *opaque, int irq, bool secure);
+/**
  * armv7m_nvic_acknowledge_irq: make highest priority pending exception active
  * @opaque: the NVIC
  *
diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 8ca6cee..b4a6e7c 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -503,8 +503,25 @@ static void armv7m_nvic_clear_pending(void *opaque, int irq, bool secure)
     }
 }
 
-void armv7m_nvic_set_pending(void *opaque, int irq, bool secure)
+static void do_armv7m_nvic_set_pending(void *opaque, int irq, bool secure,
+                                       bool derived)
 {
+    /* Pend an exception, including possibly escalating it to HardFault.
+     *
+     * This function handles both "normal" pending of interrupts and
+     * exceptions, and also derived exceptions (ones which occur as
+     * a result of trying to take some other exception).
+     *
+     * If derived == true, the caller guarantees that we are part way through
+     * trying to take an exception (but have not yet called
+     * armv7m_nvic_acknowledge_irq() to make it active), and so:
+     *  - s->vectpending is the "original exception" we were trying to take
+     *  - irq is the "derived exception"
+     *  - nvic_exec_prio(s) gives the priority before exception entry
+     * Here we handle the prioritization logic which the pseudocode puts
+     * in the DerivedLateArrival() function.
+     */
+
     NVICState *s = (NVICState *)opaque;
     bool banked = exc_is_banked(irq);
     VecInfo *vec;
@@ -514,7 +531,44 @@ void armv7m_nvic_set_pending(void *opaque, int irq, bool secure)
 
     vec = (banked && secure) ? &s->sec_vectors[irq] : &s->vectors[irq];
 
-    trace_nvic_set_pending(irq, secure, vec->enabled, vec->prio);
+    trace_nvic_set_pending(irq, secure, derived, vec->enabled, vec->prio);
+
+    if (derived) {
+        /* Derived exceptions are always synchronous. */
+        assert(irq >= ARMV7M_EXCP_HARD && irq < ARMV7M_EXCP_PENDSV);
+
+        if (irq == ARMV7M_EXCP_DEBUG &&
+            exc_group_prio(s, vec->prio, secure) >= nvic_exec_prio(s)) {
+            /* DebugMonitorFault, but its priority is lower than the
+             * preempted exception priority: just ignore it.
+             */
+            return;
+        }
+
+        if (irq == ARMV7M_EXCP_HARD && vec->prio >= s->vectpending_prio) {
+            /* If this is a terminal exception (one which means we cannot
+             * take the original exception, like a failure to read its
+             * vector table entry), then we must take the derived exception.
+             * If the derived exception can't take priority over the
+             * original exception, then we go into Lockup.
+             *
+             * For QEMU, we rely on the fact that a derived exception is
+             * terminal if and only if it's reported to us as HardFault,
+             * which saves having to have an extra argument is_terminal
+             * that we'd only use in one place.
+             */
+            cpu_abort(&s->cpu->parent_obj,
+                      "Lockup: can't take terminal derived exception "
+                      "(original exception priority %d)\n",
+                      s->vectpending_prio);
+        }
+        /* We now continue with the same code as for a normal pending
+         * exception, which will cause us to pend the derived exception.
+         * We'll then take either the original or the derived exception
+         * based on which is higher priority by the usual mechanism
+         * for selecting the highest priority pending interrupt.
+         */
+    }
 
     if (irq >= ARMV7M_EXCP_HARD && irq < ARMV7M_EXCP_PENDSV) {
         /* If a synchronous exception is pending then it may be
@@ -585,6 +639,16 @@ void armv7m_nvic_set_pending(void *opaque, int irq, bool secure)
     }
 }
 
+void armv7m_nvic_set_pending(void *opaque, int irq, bool secure)
+{
+    do_armv7m_nvic_set_pending(opaque, irq, secure, false);
+}
+
+void armv7m_nvic_set_pending_derived(void *opaque, int irq, bool secure)
+{
+    do_armv7m_nvic_set_pending(opaque, irq, secure, true);
+}
+
 /* Make pending IRQ active.  */
 bool armv7m_nvic_acknowledge_irq(void *opaque)
 {
diff --git a/hw/intc/trace-events b/hw/intc/trace-events
index be76918..09e87d1 100644
--- a/hw/intc/trace-events
+++ b/hw/intc/trace-events
@@ -177,7 +177,7 @@ nvic_set_prio(int irq, bool secure, uint8_t prio) "NVIC set irq %d secure-bank %
 nvic_irq_update(int vectpending, int pendprio, int exception_prio, int level) "NVIC vectpending %d pending prio %d exception_prio %d: setting irq line to %d"
 nvic_escalate_prio(int irq, int irqprio, int runprio) "NVIC escalating irq %d to HardFault: insufficient priority %d >= %d"
 nvic_escalate_disabled(int irq) "NVIC escalating irq %d to HardFault: disabled"
-nvic_set_pending(int irq, bool secure, int en, int prio) "NVIC set pending irq %d secure-bank %d (enabled: %d priority %d)"
+nvic_set_pending(int irq, bool secure, bool derived, int en, int prio) "NVIC set pending irq %d secure-bank %d derived %d (enabled: %d priority %d)"
 nvic_clear_pending(int irq, bool secure, int en, int prio) "NVIC clear pending irq %d secure-bank %d (enabled: %d priority %d)"
 nvic_set_pending_level(int irq) "NVIC set pending: irq %d higher prio than vectpending: setting irq line to 1"
 nvic_acknowledge_irq(int irq, int prio, bool targets_secure) "NVIC acknowledge IRQ: %d now active (prio %d targets_secure %d)"
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/7] target/arm: Add armv7m_nvic_set_pending_derived()

[Richard Henderson]

On 01/30/2018 07:02 AM, Peter Maydell wrote:
> In order to support derived exceptions (exceptions generated in
> the course of trying to take an exception), we need to be able
> to handle prioritizing whether to take the original exception
> or the derived exception.
> 
> We do this by introducing a new function
> armv7m_nvic_set_pending_derived() which the exception-taking code in
> helper.c will call when a derived exception occurs.  Derived
> exceptions are dealt with mostly like normal pending exceptions, so
> we share the implementation with the armv7m_nvic_set_pending()
> function.
> 
> Note that the way we structure this is significantly different
> from the v8M Arm ARM pseudocode: that does all the prioritization
> logic in the DerivedLateArrival() function, whereas we choose to
> let the existing "identify highest priority exception" logic
> do the prioritization for us. The effect is the same, though.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/cpu.h      | 13 ++++++++++
>  hw/intc/armv7m_nvic.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++--
>  hw/intc/trace-events  |  2 +-
>  3 files changed, 80 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH 2/7] target/arm: Split "get pending exception info" from "acknowledge it"

[Peter Maydell]

Currently armv7m_nvic_acknowledge_irq() does three things:
 * make the current highest priority pending interrupt active
 * return a bool indicating whether that interrupt is targeting
   Secure or NonSecure state
 * implicitly tell the caller which is the highest priority
   pending interrupt by setting env->v7m.exception

We need to split these jobs, because v7m_exception_taken()
needs to know whether the pending interrupt targets Secure so
it can choose to stack callee-saves registers or not, but it
must not make the interrupt active until after it has done
that stacking, in case the stacking causes a derived exception.
Similarly, it needs to know the number of the pending interrupt
so it can read the correct vector table entry before the
interrupt is made active, because vector table reads might
also cause a derived exception.

Create a new armv7m_nvic_get_pending_irq_info() function which simply
returns information about the highest priority pending interrupt, and
use it to rearrange the v7m_exception_taken() code so we don't
acknowledge the exception until we've done all the things which could
possibly cause a derived exception.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.h      | 19 ++++++++++++++++---
 hw/intc/armv7m_nvic.c | 30 +++++++++++++++++++++++-------
 target/arm/helper.c   | 16 ++++++++++++----
 hw/intc/trace-events  |  3 ++-
 4 files changed, 53 insertions(+), 15 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 9ed03e6..f21f68e 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1519,16 +1519,29 @@ void armv7m_nvic_set_pending(void *opaque, int irq, bool secure);
  */
 void armv7m_nvic_set_pending_derived(void *opaque, int irq, bool secure);
 /**
+ * armv7m_nvic_get_pending_irq_info: return highest priority pending
+ *    exception, and whether it targets Secure state
+ * @opaque: the NVIC
+ * @pirq: set to pending exception number
+ * @ptargets_secure: set to whether pending exception targets Secure
+ *
+ * This function writes the number of the highest priority pending
+ * exception (the one which would be made active by
+ * armv7m_nvic_acknowledge_irq()) to @pirq, and sets @ptargets_secure
+ * to true if the current highest priority pending exception should
+ * be taken to Secure state, false for NS.
+ */
+void armv7m_nvic_get_pending_irq_info(void *opaque, int *pirq,
+                                      bool *ptargets_secure);
+/**
  * armv7m_nvic_acknowledge_irq: make highest priority pending exception active
  * @opaque: the NVIC
  *
  * Move the current highest priority pending exception from the pending
  * state to the active state, and update v7m.exception to indicate that
  * it is the exception currently being handled.
- *
- * Returns: true if exception should be taken to Secure state, false for NS
  */
-bool armv7m_nvic_acknowledge_irq(void *opaque);
+void armv7m_nvic_acknowledge_irq(void *opaque);
 /**
  * armv7m_nvic_complete_irq: complete specified interrupt or exception
  * @opaque: the NVIC
diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index b4a6e7c..360889d 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -650,24 +650,20 @@ void armv7m_nvic_set_pending_derived(void *opaque, int irq, bool secure)
 }
 
 /* Make pending IRQ active.  */
-bool armv7m_nvic_acknowledge_irq(void *opaque)
+void armv7m_nvic_acknowledge_irq(void *opaque)
 {
     NVICState *s = (NVICState *)opaque;
     CPUARMState *env = &s->cpu->env;
     const int pending = s->vectpending;
     const int running = nvic_exec_prio(s);
     VecInfo *vec;
-    bool targets_secure;
 
     assert(pending > ARMV7M_EXCP_RESET && pending < s->num_irq);
 
     if (s->vectpending_is_s_banked) {
         vec = &s->sec_vectors[pending];
-        targets_secure = true;
     } else {
         vec = &s->vectors[pending];
-        targets_secure = !exc_is_banked(s->vectpending) &&
-            exc_targets_secure(s, s->vectpending);
     }
 
     assert(vec->enabled);
@@ -675,7 +671,7 @@ bool armv7m_nvic_acknowledge_irq(void *opaque)
 
     assert(s->vectpending_prio < running);
 
-    trace_nvic_acknowledge_irq(pending, s->vectpending_prio, targets_secure);
+    trace_nvic_acknowledge_irq(pending, s->vectpending_prio);
 
     vec->active = 1;
     vec->pending = 0;
@@ -683,8 +679,28 @@ bool armv7m_nvic_acknowledge_irq(void *opaque)
     write_v7m_exception(env, s->vectpending);
 
     nvic_irq_update(s);
+}
+
+void armv7m_nvic_get_pending_irq_info(void *opaque,
+                                      int *pirq, bool *ptargets_secure)
+{
+    NVICState *s = (NVICState *)opaque;
+    const int pending = s->vectpending;
+    bool targets_secure;
+
+    assert(pending > ARMV7M_EXCP_RESET && pending < s->num_irq);
+
+    if (s->vectpending_is_s_banked) {
+        targets_secure = true;
+    } else {
+        targets_secure = !exc_is_banked(pending) &&
+            exc_targets_secure(s, pending);
+    }
+
+    trace_nvic_get_pending_irq_info(pending, targets_secure);
 
-    return targets_secure;
+    *ptargets_secure = targets_secure;
+    *pirq = pending;
 }
 
 int armv7m_nvic_complete_irq(void *opaque, int irq, bool secure)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index bfce096..6062f38 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6395,12 +6395,12 @@ static uint32_t *get_v7m_sp_ptr(CPUARMState *env, bool secure, bool threadmode,
     }
 }
 
-static uint32_t arm_v7m_load_vector(ARMCPU *cpu, bool targets_secure)
+static uint32_t arm_v7m_load_vector(ARMCPU *cpu, int exc, bool targets_secure)
 {
     CPUState *cs = CPU(cpu);
     CPUARMState *env = &cpu->env;
     MemTxResult result;
-    hwaddr vec = env->v7m.vecbase[targets_secure] + env->v7m.exception * 4;
+    hwaddr vec = env->v7m.vecbase[targets_secure] + exc * 4;
     uint32_t addr;
 
     addr = address_space_ldl(cs->as, vec,
@@ -6462,8 +6462,9 @@ static void v7m_exception_taken(ARMCPU *cpu, uint32_t lr, bool dotailchain)
     CPUARMState *env = &cpu->env;
     uint32_t addr;
     bool targets_secure;
+    int exc;
 
-    targets_secure = armv7m_nvic_acknowledge_irq(env->nvic);
+    armv7m_nvic_get_pending_irq_info(env->nvic, &exc, &targets_secure);
 
     if (arm_feature(env, ARM_FEATURE_V8)) {
         if (arm_feature(env, ARM_FEATURE_M_SECURITY) &&
@@ -6531,6 +6532,14 @@ static void v7m_exception_taken(ARMCPU *cpu, uint32_t lr, bool dotailchain)
         }
     }
 
+    addr = arm_v7m_load_vector(cpu, exc, targets_secure);
+
+    /* Now we've done everything that might cause a derived exception
+     * we can go ahead and activate whichever exception we're going to
+     * take (which might now be the derived exception).
+     */
+    armv7m_nvic_acknowledge_irq(env->nvic);
+
     /* Switch to target security state -- must do this before writing SPSEL */
     switch_v7m_security_state(env, targets_secure);
     write_v7m_control_spsel(env, 0);
@@ -6538,7 +6547,6 @@ static void v7m_exception_taken(ARMCPU *cpu, uint32_t lr, bool dotailchain)
     /* Clear IT bits */
     env->condexec_bits = 0;
     env->regs[14] = lr;
-    addr = arm_v7m_load_vector(cpu, targets_secure);
     env->regs[15] = addr & 0xfffffffe;
     env->thumb = addr & 1;
 }
diff --git a/hw/intc/trace-events b/hw/intc/trace-events
index 09e87d1..4092d28 100644
--- a/hw/intc/trace-events
+++ b/hw/intc/trace-events
@@ -180,7 +180,8 @@ nvic_escalate_disabled(int irq) "NVIC escalating irq %d to HardFault: disabled"
 nvic_set_pending(int irq, bool secure, bool derived, int en, int prio) "NVIC set pending irq %d secure-bank %d derived %d (enabled: %d priority %d)"
 nvic_clear_pending(int irq, bool secure, int en, int prio) "NVIC clear pending irq %d secure-bank %d (enabled: %d priority %d)"
 nvic_set_pending_level(int irq) "NVIC set pending: irq %d higher prio than vectpending: setting irq line to 1"
-nvic_acknowledge_irq(int irq, int prio, bool targets_secure) "NVIC acknowledge IRQ: %d now active (prio %d targets_secure %d)"
+nvic_acknowledge_irq(int irq, int prio) "NVIC acknowledge IRQ: %d now active (prio %d)"
+nvic_get_pending_irq_info(int irq, bool secure) "NVIC next IRQ %d: targets_secure: %d"
 nvic_complete_irq(int irq, bool secure) "NVIC complete IRQ %d (secure %d)"
 nvic_set_irq_level(int irq, int level) "NVIC external irq %d level set to %d"
 nvic_sysreg_read(uint64_t addr, uint32_t value, unsigned size) "NVIC sysreg read addr 0x%" PRIx64 " data 0x%" PRIx32 " size %u"
-- 
2.7.4

Re: [Qemu-devel] [PATCH 2/7] target/arm: Split "get pending exception info" from "acknowledge it"

[Richard Henderson]

On 01/30/2018 07:02 AM, Peter Maydell wrote:
> Currently armv7m_nvic_acknowledge_irq() does three things:
>  * make the current highest priority pending interrupt active
>  * return a bool indicating whether that interrupt is targeting
>    Secure or NonSecure state
>  * implicitly tell the caller which is the highest priority
>    pending interrupt by setting env->v7m.exception
> 
> We need to split these jobs, because v7m_exception_taken()
> needs to know whether the pending interrupt targets Secure so
> it can choose to stack callee-saves registers or not, but it
> must not make the interrupt active until after it has done
> that stacking, in case the stacking causes a derived exception.
> Similarly, it needs to know the number of the pending interrupt
> so it can read the correct vector table entry before the
> interrupt is made active, because vector table reads might
> also cause a derived exception.
> 
> Create a new armv7m_nvic_get_pending_irq_info() function which simply
> returns information about the highest priority pending interrupt, and
> use it to rearrange the v7m_exception_taken() code so we don't
> acknowledge the exception until we've done all the things which could
> possibly cause a derived exception.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/cpu.h      | 19 ++++++++++++++++---
>  hw/intc/armv7m_nvic.c | 30 +++++++++++++++++++++++-------
>  target/arm/helper.c   | 16 ++++++++++++----
>  hw/intc/trace-events  |  3 ++-
>  4 files changed, 53 insertions(+), 15 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [Qemu-arm] [PATCH 2/7] target/arm: Split "get pending exception info" from "acknowledge it"

[Philippe Mathieu-Daudé]

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

On 01/30/2018 12:02 PM, Peter Maydell wrote:
> Currently armv7m_nvic_acknowledge_irq() does three things:
>  * make the current highest priority pending interrupt active
>  * return a bool indicating whether that interrupt is targeting
>    Secure or NonSecure state
>  * implicitly tell the caller which is the highest priority
>    pending interrupt by setting env->v7m.exception
> 
> We need to split these jobs, because v7m_exception_taken()
> needs to know whether the pending interrupt targets Secure so
> it can choose to stack callee-saves registers or not, but it
> must not make the interrupt active until after it has done
> that stacking, in case the stacking causes a derived exception.
> Similarly, it needs to know the number of the pending interrupt
> so it can read the correct vector table entry before the
> interrupt is made active, because vector table reads might
> also cause a derived exception.
> 
> Create a new armv7m_nvic_get_pending_irq_info() function which simply
> returns information about the highest priority pending interrupt, and
> use it to rearrange the v7m_exception_taken() code so we don't
> acknowledge the exception until we've done all the things which could
> possibly cause a derived exception.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/cpu.h      | 19 ++++++++++++++++---
>  hw/intc/armv7m_nvic.c | 30 +++++++++++++++++++++++-------
>  target/arm/helper.c   | 16 ++++++++++++----
>  hw/intc/trace-events  |  3 ++-
>  4 files changed, 53 insertions(+), 15 deletions(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 9ed03e6..f21f68e 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1519,16 +1519,29 @@ void armv7m_nvic_set_pending(void *opaque, int irq, bool secure);
>   */
>  void armv7m_nvic_set_pending_derived(void *opaque, int irq, bool secure);
>  /**
> + * armv7m_nvic_get_pending_irq_info: return highest priority pending
> + *    exception, and whether it targets Secure state
> + * @opaque: the NVIC
> + * @pirq: set to pending exception number
> + * @ptargets_secure: set to whether pending exception targets Secure
> + *
> + * This function writes the number of the highest priority pending
> + * exception (the one which would be made active by
> + * armv7m_nvic_acknowledge_irq()) to @pirq, and sets @ptargets_secure
> + * to true if the current highest priority pending exception should
> + * be taken to Secure state, false for NS.
> + */
> +void armv7m_nvic_get_pending_irq_info(void *opaque, int *pirq,
> +                                      bool *ptargets_secure);
> +/**
>   * armv7m_nvic_acknowledge_irq: make highest priority pending exception active
>   * @opaque: the NVIC
>   *
>   * Move the current highest priority pending exception from the pending
>   * state to the active state, and update v7m.exception to indicate that
>   * it is the exception currently being handled.
> - *
> - * Returns: true if exception should be taken to Secure state, false for NS
>   */
> -bool armv7m_nvic_acknowledge_irq(void *opaque);
> +void armv7m_nvic_acknowledge_irq(void *opaque);
>  /**
>   * armv7m_nvic_complete_irq: complete specified interrupt or exception
>   * @opaque: the NVIC
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index b4a6e7c..360889d 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -650,24 +650,20 @@ void armv7m_nvic_set_pending_derived(void *opaque, int irq, bool secure)
>  }
>  
>  /* Make pending IRQ active.  */
> -bool armv7m_nvic_acknowledge_irq(void *opaque)
> +void armv7m_nvic_acknowledge_irq(void *opaque)
>  {
>      NVICState *s = (NVICState *)opaque;
>      CPUARMState *env = &s->cpu->env;
>      const int pending = s->vectpending;
>      const int running = nvic_exec_prio(s);
>      VecInfo *vec;
> -    bool targets_secure;
>  
>      assert(pending > ARMV7M_EXCP_RESET && pending < s->num_irq);
>  
>      if (s->vectpending_is_s_banked) {
>          vec = &s->sec_vectors[pending];
> -        targets_secure = true;
>      } else {
>          vec = &s->vectors[pending];
> -        targets_secure = !exc_is_banked(s->vectpending) &&
> -            exc_targets_secure(s, s->vectpending);
>      }
>  
>      assert(vec->enabled);
> @@ -675,7 +671,7 @@ bool armv7m_nvic_acknowledge_irq(void *opaque)
>  
>      assert(s->vectpending_prio < running);
>  
> -    trace_nvic_acknowledge_irq(pending, s->vectpending_prio, targets_secure);
> +    trace_nvic_acknowledge_irq(pending, s->vectpending_prio);
>  
>      vec->active = 1;
>      vec->pending = 0;
> @@ -683,8 +679,28 @@ bool armv7m_nvic_acknowledge_irq(void *opaque)
>      write_v7m_exception(env, s->vectpending);
>  
>      nvic_irq_update(s);
> +}
> +
> +void armv7m_nvic_get_pending_irq_info(void *opaque,
> +                                      int *pirq, bool *ptargets_secure)
> +{
> +    NVICState *s = (NVICState *)opaque;
> +    const int pending = s->vectpending;
> +    bool targets_secure;
> +
> +    assert(pending > ARMV7M_EXCP_RESET && pending < s->num_irq);
> +
> +    if (s->vectpending_is_s_banked) {
> +        targets_secure = true;
> +    } else {
> +        targets_secure = !exc_is_banked(pending) &&
> +            exc_targets_secure(s, pending);
> +    }
> +
> +    trace_nvic_get_pending_irq_info(pending, targets_secure);
>  
> -    return targets_secure;

maybe:

assert(ptargets_secure && pirq);

(function is externally callable)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> +    *ptargets_secure = targets_secure;
> +    *pirq = pending;
>  }
>  
>  int armv7m_nvic_complete_irq(void *opaque, int irq, bool secure)
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index bfce096..6062f38 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6395,12 +6395,12 @@ static uint32_t *get_v7m_sp_ptr(CPUARMState *env, bool secure, bool threadmode,
>      }
>  }
>  
> -static uint32_t arm_v7m_load_vector(ARMCPU *cpu, bool targets_secure)
> +static uint32_t arm_v7m_load_vector(ARMCPU *cpu, int exc, bool targets_secure)
>  {
>      CPUState *cs = CPU(cpu);
>      CPUARMState *env = &cpu->env;
>      MemTxResult result;
> -    hwaddr vec = env->v7m.vecbase[targets_secure] + env->v7m.exception * 4;
> +    hwaddr vec = env->v7m.vecbase[targets_secure] + exc * 4;
>      uint32_t addr;
>  
>      addr = address_space_ldl(cs->as, vec,
> @@ -6462,8 +6462,9 @@ static void v7m_exception_taken(ARMCPU *cpu, uint32_t lr, bool dotailchain)
>      CPUARMState *env = &cpu->env;
>      uint32_t addr;
>      bool targets_secure;
> +    int exc;
>  
> -    targets_secure = armv7m_nvic_acknowledge_irq(env->nvic);
> +    armv7m_nvic_get_pending_irq_info(env->nvic, &exc, &targets_secure);
>  
>      if (arm_feature(env, ARM_FEATURE_V8)) {
>          if (arm_feature(env, ARM_FEATURE_M_SECURITY) &&
> @@ -6531,6 +6532,14 @@ static void v7m_exception_taken(ARMCPU *cpu, uint32_t lr, bool dotailchain)
>          }
>      }
>  
> +    addr = arm_v7m_load_vector(cpu, exc, targets_secure);
> +
> +    /* Now we've done everything that might cause a derived exception
> +     * we can go ahead and activate whichever exception we're going to
> +     * take (which might now be the derived exception).
> +     */
> +    armv7m_nvic_acknowledge_irq(env->nvic);
> +
>      /* Switch to target security state -- must do this before writing SPSEL */
>      switch_v7m_security_state(env, targets_secure);
>      write_v7m_control_spsel(env, 0);
> @@ -6538,7 +6547,6 @@ static void v7m_exception_taken(ARMCPU *cpu, uint32_t lr, bool dotailchain)
>      /* Clear IT bits */
>      env->condexec_bits = 0;
>      env->regs[14] = lr;
> -    addr = arm_v7m_load_vector(cpu, targets_secure);
>      env->regs[15] = addr & 0xfffffffe;
>      env->thumb = addr & 1;
>  }
> diff --git a/hw/intc/trace-events b/hw/intc/trace-events
> index 09e87d1..4092d28 100644
> --- a/hw/intc/trace-events
> +++ b/hw/intc/trace-events
> @@ -180,7 +180,8 @@ nvic_escalate_disabled(int irq) "NVIC escalating irq %d to HardFault: disabled"
>  nvic_set_pending(int irq, bool secure, bool derived, int en, int prio) "NVIC set pending irq %d secure-bank %d derived %d (enabled: %d priority %d)"
>  nvic_clear_pending(int irq, bool secure, int en, int prio) "NVIC clear pending irq %d secure-bank %d (enabled: %d priority %d)"
>  nvic_set_pending_level(int irq) "NVIC set pending: irq %d higher prio than vectpending: setting irq line to 1"
> -nvic_acknowledge_irq(int irq, int prio, bool targets_secure) "NVIC acknowledge IRQ: %d now active (prio %d targets_secure %d)"
> +nvic_acknowledge_irq(int irq, int prio) "NVIC acknowledge IRQ: %d now active (prio %d)"
> +nvic_get_pending_irq_info(int irq, bool secure) "NVIC next IRQ %d: targets_secure: %d"
>  nvic_complete_irq(int irq, bool secure) "NVIC complete IRQ %d (secure %d)"
>  nvic_set_irq_level(int irq, int level) "NVIC external irq %d level set to %d"
>  nvic_sysreg_read(uint64_t addr, uint32_t value, unsigned size) "NVIC sysreg read addr 0x%" PRIx64 " data 0x%" PRIx32 " size %u"
> 


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

Re: [Qemu-devel] [Qemu-arm] [PATCH 2/7] target/arm: Split "get pending exception info" from "acknowledge it"

[Peter Maydell]

On 5 February 2018 at 23:44, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> maybe:
>
> assert(ptargets_secure && pirq);

This is one of those areas where people have different opinions,
but personally I think the point of assertions is to turn
bugs that manifest later and in confusing ways into bugs
that manifest early and clearly. In this case, if either of
those pointers is NULL then we'll immediately segfault in
this function, and the stacktrace will point directly at
the buggy code, so the assert doesn't gain anything.

thanks
-- PMM

[Qemu-devel] [PATCH 3/7] target/arm: Add ignore_stackfaults argument to v7m_exception_taken()

[Peter Maydell]

In the v8M architecture, if the process of taking an exception
results in a further exception this is called a derived exception
(for example, an MPU exception when writing the exception frame to
memory). If the derived exception happens while pushing the initial
stack frame, we must ignore any subsequent possible exception
pushing the callee-saves registers.

In preparation for making the stack writes check for exceptions,
add a return value from v7m_push_stack() and a new parameter to
v7m_exception_taken(), so that the former can tell the latter that
it needs to ignore failures to write to the stack. We also plumb
the argument through to v7m_push_callee_stack(), which is where
the code to ignore the failures will be.

(Note that the v8M ARM pseudocode structures this slightly differently:
derived exceptions cause the attempt to process the original
exception to be abandoned; then at the top level it calls
DerivedLateArrival to prioritize the derived exception and call
TakeException from there. We choose to let the NVIC do the prioritization
and continue forward with a call to TakeException which will then
take either the original or the derived exception. The effect is
the same, but this structure works better for QEMU because we don't
have a convenient top level place to do the abandon-and-retry logic.)

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

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 6062f38..c713eea 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6419,7 +6419,8 @@ static uint32_t arm_v7m_load_vector(ARMCPU *cpu, int exc, bool targets_secure)
     return addr;
 }
 
-static void v7m_push_callee_stack(ARMCPU *cpu, uint32_t lr, bool dotailchain)
+static void v7m_push_callee_stack(ARMCPU *cpu, uint32_t lr, bool dotailchain,
+                                  bool ignore_faults)
 {
     /* For v8M, push the callee-saves register part of the stack frame.
      * Compare the v8M pseudocode PushCalleeStack().
@@ -6453,7 +6454,8 @@ static void v7m_push_callee_stack(ARMCPU *cpu, uint32_t lr, bool dotailchain)
     *frame_sp_p = frameptr;
 }
 
-static void v7m_exception_taken(ARMCPU *cpu, uint32_t lr, bool dotailchain)
+static void v7m_exception_taken(ARMCPU *cpu, uint32_t lr, bool dotailchain,
+                                bool ignore_stackfaults)
 {
     /* Do the "take the exception" parts of exception entry,
      * but not the pushing of state to the stack. This is
@@ -6490,7 +6492,8 @@ static void v7m_exception_taken(ARMCPU *cpu, uint32_t lr, bool dotailchain)
                  */
                 if (lr & R_V7M_EXCRET_DCRS_MASK &&
                     !(dotailchain && (lr & R_V7M_EXCRET_ES_MASK))) {
-                    v7m_push_callee_stack(cpu, lr, dotailchain);
+                    v7m_push_callee_stack(cpu, lr, dotailchain,
+                                          ignore_stackfaults);
                 }
                 lr |= R_V7M_EXCRET_DCRS_MASK;
             }
@@ -6551,10 +6554,13 @@ static void v7m_exception_taken(ARMCPU *cpu, uint32_t lr, bool dotailchain)
     env->thumb = addr & 1;
 }
 
-static void v7m_push_stack(ARMCPU *cpu)
+static bool v7m_push_stack(ARMCPU *cpu)
 {
     /* Do the "set up stack frame" part of exception entry,
      * similar to pseudocode PushStack().
+     * Return true if we generate a derived exception (and so
+     * should ignore further stack faults trying to process
+     * that derived exception.)
      */
     CPUARMState *env = &cpu->env;
     uint32_t xpsr = xpsr_read(env);
@@ -6574,6 +6580,8 @@ static void v7m_push_stack(ARMCPU *cpu)
     v7m_push(env, env->regs[2]);
     v7m_push(env, env->regs[1]);
     v7m_push(env, env->regs[0]);
+
+    return false;
 }
 
 static void do_v7m_exception_exit(ARMCPU *cpu)
@@ -6719,7 +6727,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
     if (sfault) {
         env->v7m.sfsr |= R_V7M_SFSR_INVER_MASK;
         armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_SECURE, false);
-        v7m_exception_taken(cpu, excret, true);
+        v7m_exception_taken(cpu, excret, true, false);
         qemu_log_mask(CPU_LOG_INT, "...taking SecureFault on existing "
                       "stackframe: failed EXC_RETURN.ES validity check\n");
         return;
@@ -6731,7 +6739,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
          */
         env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_INVPC_MASK;
         armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE, env->v7m.secure);
-        v7m_exception_taken(cpu, excret, true);
+        v7m_exception_taken(cpu, excret, true, false);
         qemu_log_mask(CPU_LOG_INT, "...taking UsageFault on existing "
                       "stackframe: failed exception return integrity check\n");
         return;
@@ -6779,7 +6787,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
                 /* Take a SecureFault on the current stack */
                 env->v7m.sfsr |= R_V7M_SFSR_INVIS_MASK;
                 armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_SECURE, false);
-                v7m_exception_taken(cpu, excret, true);
+                v7m_exception_taken(cpu, excret, true, false);
                 qemu_log_mask(CPU_LOG_INT, "...taking SecureFault on existing "
                               "stackframe: failed exception return integrity "
                               "signature check\n");
@@ -6844,7 +6852,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
                 armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE,
                                         env->v7m.secure);
                 env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_INVPC_MASK;
-                v7m_exception_taken(cpu, excret, true);
+                v7m_exception_taken(cpu, excret, true, false);
                 qemu_log_mask(CPU_LOG_INT, "...taking UsageFault on existing "
                               "stackframe: failed exception return integrity "
                               "check\n");
@@ -6877,11 +6885,13 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
         /* Take an INVPC UsageFault by pushing the stack again;
          * we know we're v7M so this is never a Secure UsageFault.
          */
+        bool ignore_stackfaults;
+
         assert(!arm_feature(env, ARM_FEATURE_V8));
         armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE, false);
         env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_INVPC_MASK;
-        v7m_push_stack(cpu);
-        v7m_exception_taken(cpu, excret, false);
+        ignore_stackfaults = v7m_push_stack(cpu);
+        v7m_exception_taken(cpu, excret, false, ignore_stackfaults);
         qemu_log_mask(CPU_LOG_INT, "...taking UsageFault on new stackframe: "
                       "failed exception return integrity check\n");
         return;
@@ -7122,6 +7132,7 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
     uint32_t lr;
+    bool ignore_stackfaults;
 
     arm_log_exception(cs->exception_index);
 
@@ -7296,8 +7307,8 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
         lr |= R_V7M_EXCRET_MODE_MASK;
     }
 
-    v7m_push_stack(cpu);
-    v7m_exception_taken(cpu, lr, false);
+    ignore_stackfaults = v7m_push_stack(cpu);
+    v7m_exception_taken(cpu, lr, false, ignore_stackfaults);
     qemu_log_mask(CPU_LOG_INT, "... as %d\n", env->v7m.exception);
 }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH 3/7] target/arm: Add ignore_stackfaults argument to v7m_exception_taken()

[Richard Henderson]

On 01/30/2018 07:02 AM, Peter Maydell wrote:
> In the v8M architecture, if the process of taking an exception
> results in a further exception this is called a derived exception
> (for example, an MPU exception when writing the exception frame to
> memory). If the derived exception happens while pushing the initial
> stack frame, we must ignore any subsequent possible exception
> pushing the callee-saves registers.
> 
> In preparation for making the stack writes check for exceptions,
> add a return value from v7m_push_stack() and a new parameter to
> v7m_exception_taken(), so that the former can tell the latter that
> it needs to ignore failures to write to the stack. We also plumb
> the argument through to v7m_push_callee_stack(), which is where
> the code to ignore the failures will be.
> 
> (Note that the v8M ARM pseudocode structures this slightly differently:
> derived exceptions cause the attempt to process the original
> exception to be abandoned; then at the top level it calls
> DerivedLateArrival to prioritize the derived exception and call
> TakeException from there. We choose to let the NVIC do the prioritization
> and continue forward with a call to TakeException which will then
> take either the original or the derived exception. The effect is
> the same, but this structure works better for QEMU because we don't
> have a convenient top level place to do the abandon-and-retry logic.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 35 +++++++++++++++++++++++------------
>  1 file changed, 23 insertions(+), 12 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH 4/7] target/arm: Make v7M exception entry stack push check MPU

[Peter Maydell]

The memory writes done to push registers on the stack
on exception entry in M profile CPUs are supposed to
go via MPU permissions checks, which may cause us to
take a derived exception instead of the original one of
the MPU lookup fails. We were implementing these as
always-succeeds direct writes to physical memory.
Rewrite v7m_push_stack() to do the necessary checks.

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

diff --git a/target/arm/helper.c b/target/arm/helper.c
index c713eea..007e760 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6161,12 +6161,66 @@ uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx,
     return target_el;
 }
 
-static void v7m_push(CPUARMState *env, uint32_t val)
+static bool v7m_stack_write(ARMCPU *cpu, uint32_t addr, uint32_t value,
+                            ARMMMUIdx mmu_idx, bool ignfault)
 {
-    CPUState *cs = CPU(arm_env_get_cpu(env));
+    CPUState *cs = CPU(cpu);
+    CPUARMState *env = &cpu->env;
+    MemTxAttrs attrs = {};
+    MemTxResult txres;
+    target_ulong page_size;
+    hwaddr physaddr;
+    int prot;
+    ARMMMUFaultInfo fi;
+    bool secure = mmu_idx & ARM_MMU_IDX_M_S;
+    int exc;
+    bool exc_secure;
 
-    env->regs[13] -= 4;
-    stl_phys(cs->as, env->regs[13], val);
+    if (get_phys_addr(env, addr, MMU_DATA_STORE, mmu_idx, &physaddr,
+                      &attrs, &prot, &page_size, &fi, NULL)) {
+        /* MPU/SAU lookup failed */
+        if (fi.type == ARMFault_QEMU_SFault) {
+            qemu_log_mask(CPU_LOG_INT,
+                          "...SecureFault with SFSR.AUVIOL during stacking\n");
+            env->v7m.sfsr |= R_V7M_SFSR_AUVIOL_MASK | R_V7M_SFSR_SFARVALID_MASK;
+            env->v7m.sfar = addr;
+            exc = ARMV7M_EXCP_SECURE;
+            exc_secure = true;
+        } else {
+            qemu_log_mask(CPU_LOG_INT, "...MemManageFault with CFSR.MSTKERR\n");
+            env->v7m.cfsr[secure] |= R_V7M_CFSR_MSTKERR_MASK;
+            exc = ARMV7M_EXCP_MEM;
+            exc_secure = secure;
+        }
+        goto pend_fault;
+    }
+    address_space_stl_le(arm_addressspace(cs, attrs), physaddr, value,
+                         attrs, &txres);
+    if (txres != MEMTX_OK) {
+        /* BusFault trying to write the data */
+        qemu_log_mask(CPU_LOG_INT, "...BusFault with BFSR.STKERR\n");
+        env->v7m.cfsr[M_REG_NS] |= R_V7M_CFSR_STKERR_MASK;
+        exc = ARMV7M_EXCP_BUS;
+        exc_secure = false;
+        goto pend_fault;
+    }
+    return true;
+
+pend_fault:
+    /* By pending the exception at this point we are making
+     * the IMPDEF choice "overridden exceptions pended" (see the
+     * MergeExcInfo() pseudocode). The other choice would be to not
+     * pend them now and then make a choice about which to throw away
+     * later if we have two derived exceptions.
+     * The only case when we must not pend the exception but instead
+     * throw it away is if we are doing the push of the callee registers
+     * and we've already generated a derived exception. Even in this
+     * case we will still update the fault status registers.
+     */
+    if (!ignfault) {
+        armv7m_nvic_set_pending_derived(env->nvic, exc, exc_secure);
+    }
+    return false;
 }
 
 /* Return true if we're using the process stack pointer (not the MSP) */
@@ -6562,26 +6616,43 @@ static bool v7m_push_stack(ARMCPU *cpu)
      * should ignore further stack faults trying to process
      * that derived exception.)
      */
+    bool stacked_ok;
     CPUARMState *env = &cpu->env;
     uint32_t xpsr = xpsr_read(env);
+    uint32_t frameptr = env->regs[13];
+    ARMMMUIdx mmu_idx = core_to_arm_mmu_idx(env, cpu_mmu_index(env, false));
 
     /* Align stack pointer if the guest wants that */
-    if ((env->regs[13] & 4) &&
+    if ((frameptr & 4) &&
         (env->v7m.ccr[env->v7m.secure] & R_V7M_CCR_STKALIGN_MASK)) {
-        env->regs[13] -= 4;
+        frameptr -= 4;
         xpsr |= XPSR_SPREALIGN;
     }
-    /* Switch to the handler mode.  */
-    v7m_push(env, xpsr);
-    v7m_push(env, env->regs[15]);
-    v7m_push(env, env->regs[14]);
-    v7m_push(env, env->regs[12]);
-    v7m_push(env, env->regs[3]);
-    v7m_push(env, env->regs[2]);
-    v7m_push(env, env->regs[1]);
-    v7m_push(env, env->regs[0]);
 
-    return false;
+    frameptr -= 0x20;
+
+    /* Write as much of the stack frame as we can. If we fail a stack
+     * write this will result in a derived exception being pended
+     * (which may be taken in preference to the one we started with
+     * if it has higher priority).
+     */
+    stacked_ok =
+        v7m_stack_write(cpu, frameptr, env->regs[0], mmu_idx, false) &&
+        v7m_stack_write(cpu, frameptr + 4, env->regs[1], mmu_idx, false) &&
+        v7m_stack_write(cpu, frameptr + 8, env->regs[2], mmu_idx, false) &&
+        v7m_stack_write(cpu, frameptr + 12, env->regs[3], mmu_idx, false) &&
+        v7m_stack_write(cpu, frameptr + 16, env->regs[12], mmu_idx, false) &&
+        v7m_stack_write(cpu, frameptr + 20, env->regs[14], mmu_idx, false) &&
+        v7m_stack_write(cpu, frameptr + 24, env->regs[15], mmu_idx, false) &&
+        v7m_stack_write(cpu, frameptr + 28, xpsr, mmu_idx, false);
+
+    /* Update SP regardless of whether any of the stack accesses failed.
+     * When we implement v8M stack limit checking then this attempt to
+     * update SP might also fail and result in a derived exception.
+     */
+    env->regs[13] = frameptr;
+
+    return !stacked_ok;
 }
 
 static void do_v7m_exception_exit(ARMCPU *cpu)
-- 
2.7.4

Re: [Qemu-devel] [PATCH 4/7] target/arm: Make v7M exception entry stack push check MPU

[Richard Henderson]

On 01/30/2018 07:02 AM, Peter Maydell wrote:
> +    if (get_phys_addr(env, addr, MMU_DATA_STORE, mmu_idx, &physaddr,
> +                      &attrs, &prot, &page_size, &fi, NULL)) {

Given that you know you're going to call this at least 8 times in a row,
probably with all operations on the same page, it is worth querying the qemu
tlb first, and when that fails but the lookup succeeds, installing the result
into the tlb?

I know we don't have a primitive for that within exec/cputlb.h, but perhaps we
should.  Perhaps a more generic form of probe_write...

That said, this isn't wrong so

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 4/7] target/arm: Make v7M exception entry stack push check MPU

[Peter Maydell]

On 3 February 2018 at 21:21, Richard Henderson
<richard.henderson@linaro.org> wrote:
> On 01/30/2018 07:02 AM, Peter Maydell wrote:
>> +    if (get_phys_addr(env, addr, MMU_DATA_STORE, mmu_idx, &physaddr,
>> +                      &attrs, &prot, &page_size, &fi, NULL)) {
>
> Given that you know you're going to call this at least 8 times in a row,
> probably with all operations on the same page, it is worth querying the qemu
> tlb first, and when that fails but the lookup succeeds, installing the result
> into the tlb?

get_phys_addr and address_space_stl_le don't touch the TLB though?
We could in theory do a get_phys_addr() once per-page rather than
doing it all 8 times though, yes.

But yes, I figured I'd write the straightforward code -- M class
CPU h/w all runs much slower than the host CPU anyway so we
probably have some overhead to play with. If anybody ever
complains maybe we can profile it :-)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 4/7] target/arm: Make v7M exception entry stack push check MPU

[Richard Henderson]

On 02/03/2018 01:30 PM, Peter Maydell wrote:
> On 3 February 2018 at 21:21, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>> On 01/30/2018 07:02 AM, Peter Maydell wrote:
>>> +    if (get_phys_addr(env, addr, MMU_DATA_STORE, mmu_idx, &physaddr,
>>> +                      &attrs, &prot, &page_size, &fi, NULL)) {
>>
>> Given that you know you're going to call this at least 8 times in a row,
>> probably with all operations on the same page, it is worth querying the qemu
>> tlb first, and when that fails but the lookup succeeds, installing the result
>> into the tlb?
> 
> get_phys_addr and address_space_stl_le don't touch the TLB though?

No, get_phys_addr is the primitive that tlb_fill uses.

> We could in theory do a get_phys_addr() once per-page rather than
> doing it all 8 times though, yes.

After I sent that first message I thought perhaps interacting with the proper
TLB is too/overly complicated.  We'd get nearly the same results just having a
local 1-entry cache.  Something like

    pagecache = -1;
    stacked_ok =
        v7m_stack_write(cpu, frameptr, env->regs[0],
                        &pagecache, mmu_idx, false) &&
        v7m_stack_write(cpu, frameptr + 4, env->regs[1],
                        &pagecache, mmu_idx, false) &&
        ...


r~

Re: [Qemu-devel] [Qemu-arm] [PATCH 4/7] target/arm: Make v7M exception entry stack push check MPU

[Peter Maydell]

On 30 January 2018 at 15:02, Peter Maydell <peter.maydell@linaro.org> wrote:
> -static void v7m_push(CPUARMState *env, uint32_t val)
> +static bool v7m_stack_write(ARMCPU *cpu, uint32_t addr, uint32_t value,
> +                            ARMMMUIdx mmu_idx, bool ignfault)
>  {
> -    CPUState *cs = CPU(arm_env_get_cpu(env));
> +    CPUState *cs = CPU(cpu);
> +    CPUARMState *env = &cpu->env;
> +    MemTxAttrs attrs = {};
> +    MemTxResult txres;
> +    target_ulong page_size;
> +    hwaddr physaddr;
> +    int prot;
> +    ARMMMUFaultInfo fi;
> +    bool secure = mmu_idx & ARM_MMU_IDX_M_S;
> +    int exc;
> +    bool exc_secure;
>
> -    env->regs[13] -= 4;
> -    stl_phys(cs->as, env->regs[13], val);
> +    if (get_phys_addr(env, addr, MMU_DATA_STORE, mmu_idx, &physaddr,
> +                      &attrs, &prot, &page_size, &fi, NULL)) {
> +        /* MPU/SAU lookup failed */
> +        if (fi.type == ARMFault_QEMU_SFault) {
> +            qemu_log_mask(CPU_LOG_INT,
> +                          "...SecureFault with SFSR.AUVIOL during stacking\n");
> +            env->v7m.sfsr |= R_V7M_SFSR_AUVIOL_MASK | R_V7M_SFSR_SFARVALID_MASK;
> +            env->v7m.sfar = addr;
> +            exc = ARMV7M_EXCP_SECURE;
> +            exc_secure = true;

I noticed on Friday that this code isn't quite right -- it should be
   exc_secure = false;

This is because SecureFault is not banked, and for non-banked exceptions
we want to always pass false to armv7m_nvic_set_pending{,_derived}().
Otherwise that function will assert().

v7m_stack_read() in a later patch has the same bug.

thanks
-- PMM

[Qemu-devel] [PATCH 5/7] target/arm: Make v7m_push_callee_stack() honour MPU

[Peter Maydell]

Make v7m_push_callee_stack() honour the MPU by using the
new v7m_stack_write() function. We return a flag to indicate
whether the pushes failed, which we can then use in
v7m_exception_taken() to cause us to handle the derived
exception correctly.

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

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 007e760..de0031b 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6473,7 +6473,7 @@ static uint32_t arm_v7m_load_vector(ARMCPU *cpu, int exc, bool targets_secure)
     return addr;
 }
 
-static void v7m_push_callee_stack(ARMCPU *cpu, uint32_t lr, bool dotailchain,
+static bool v7m_push_callee_stack(ARMCPU *cpu, uint32_t lr, bool dotailchain,
                                   bool ignore_faults)
 {
     /* For v8M, push the callee-saves register part of the stack frame.
@@ -6481,31 +6481,55 @@ static void v7m_push_callee_stack(ARMCPU *cpu, uint32_t lr, bool dotailchain,
      * In the tailchaining case this may not be the current stack.
      */
     CPUARMState *env = &cpu->env;
-    CPUState *cs = CPU(cpu);
     uint32_t *frame_sp_p;
     uint32_t frameptr;
+    ARMMMUIdx mmu_idx;
+    bool stacked_ok;
 
     if (dotailchain) {
-        frame_sp_p = get_v7m_sp_ptr(env, true,
-                                    lr & R_V7M_EXCRET_MODE_MASK,
+        bool mode = lr & R_V7M_EXCRET_MODE_MASK;
+        bool priv = !(env->v7m.control[M_REG_S] & R_V7M_CONTROL_NPRIV_MASK) ||
+            !mode;
+
+        mmu_idx = arm_v7m_mmu_idx_for_secstate_and_priv(env, M_REG_S, priv);
+        frame_sp_p = get_v7m_sp_ptr(env, M_REG_S, mode,
                                     lr & R_V7M_EXCRET_SPSEL_MASK);
     } else {
+        mmu_idx = core_to_arm_mmu_idx(env, cpu_mmu_index(env, false));
         frame_sp_p = &env->regs[13];
     }
 
     frameptr = *frame_sp_p - 0x28;
 
-    stl_phys(cs->as, frameptr, 0xfefa125b);
-    stl_phys(cs->as, frameptr + 0x8, env->regs[4]);
-    stl_phys(cs->as, frameptr + 0xc, env->regs[5]);
-    stl_phys(cs->as, frameptr + 0x10, env->regs[6]);
-    stl_phys(cs->as, frameptr + 0x14, env->regs[7]);
-    stl_phys(cs->as, frameptr + 0x18, env->regs[8]);
-    stl_phys(cs->as, frameptr + 0x1c, env->regs[9]);
-    stl_phys(cs->as, frameptr + 0x20, env->regs[10]);
-    stl_phys(cs->as, frameptr + 0x24, env->regs[11]);
+    /* Write as much of the stack frame as we can. A write failure may
+     * cause us to pend a derived exception.
+     */
+    stacked_ok =
+        v7m_stack_write(cpu, frameptr, 0xfefa125b, mmu_idx, ignore_faults) &&
+        v7m_stack_write(cpu, frameptr + 0x8, env->regs[4], mmu_idx,
+                        ignore_faults) &&
+        v7m_stack_write(cpu, frameptr + 0xc, env->regs[5], mmu_idx,
+                        ignore_faults) &&
+        v7m_stack_write(cpu, frameptr + 0x10, env->regs[6], mmu_idx,
+                        ignore_faults) &&
+        v7m_stack_write(cpu, frameptr + 0x14, env->regs[7], mmu_idx,
+                        ignore_faults) &&
+        v7m_stack_write(cpu, frameptr + 0x18, env->regs[8], mmu_idx,
+                        ignore_faults) &&
+        v7m_stack_write(cpu, frameptr + 0x1c, env->regs[9], mmu_idx,
+                        ignore_faults) &&
+        v7m_stack_write(cpu, frameptr + 0x20, env->regs[10], mmu_idx,
+                        ignore_faults) &&
+        v7m_stack_write(cpu, frameptr + 0x24, env->regs[11], mmu_idx,
+                        ignore_faults);
 
+    /* Update SP regardless of whether any of the stack accesses failed.
+     * When we implement v8M stack limit checking then this attempt to
+     * update SP might also fail and result in a derived exception.
+     */
     *frame_sp_p = frameptr;
+
+    return !stacked_ok;
 }
 
 static void v7m_exception_taken(ARMCPU *cpu, uint32_t lr, bool dotailchain,
@@ -6519,6 +6543,7 @@ static void v7m_exception_taken(ARMCPU *cpu, uint32_t lr, bool dotailchain,
     uint32_t addr;
     bool targets_secure;
     int exc;
+    bool push_failed = false;
 
     armv7m_nvic_get_pending_irq_info(env->nvic, &exc, &targets_secure);
 
@@ -6546,8 +6571,8 @@ static void v7m_exception_taken(ARMCPU *cpu, uint32_t lr, bool dotailchain,
                  */
                 if (lr & R_V7M_EXCRET_DCRS_MASK &&
                     !(dotailchain && (lr & R_V7M_EXCRET_ES_MASK))) {
-                    v7m_push_callee_stack(cpu, lr, dotailchain,
-                                          ignore_stackfaults);
+                    push_failed = v7m_push_callee_stack(cpu, lr, dotailchain,
+                                                        ignore_stackfaults);
                 }
                 lr |= R_V7M_EXCRET_DCRS_MASK;
             }
@@ -6589,6 +6614,15 @@ static void v7m_exception_taken(ARMCPU *cpu, uint32_t lr, bool dotailchain,
         }
     }
 
+    if (push_failed && !ignore_stackfaults) {
+        /* Derived exception on callee-saves register stacking:
+         * we might now want to take a different exception which
+         * targets a different security state, so try again from the top.
+         */
+        v7m_exception_taken(cpu, lr, true, true);
+        return;
+    }
+
     addr = arm_v7m_load_vector(cpu, exc, targets_secure);
 
     /* Now we've done everything that might cause a derived exception
-- 
2.7.4

Re: [Qemu-devel] [PATCH 5/7] target/arm: Make v7m_push_callee_stack() honour MPU

[Richard Henderson]

On 01/30/2018 07:02 AM, Peter Maydell wrote:
> Make v7m_push_callee_stack() honour the MPU by using the
> new v7m_stack_write() function. We return a flag to indicate
> whether the pushes failed, which we can then use in
> v7m_exception_taken() to cause us to handle the derived
> exception correctly.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 64 ++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 49 insertions(+), 15 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [Qemu-arm] [PATCH 5/7] target/arm: Make v7m_push_callee_stack() honour MPU

[Philippe Mathieu-Daudé]

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

On 01/30/2018 12:02 PM, Peter Maydell wrote:
> Make v7m_push_callee_stack() honour the MPU by using the
> new v7m_stack_write() function. We return a flag to indicate
> whether the pushes failed, which we can then use in
> v7m_exception_taken() to cause us to handle the derived
> exception correctly.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 64 ++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 49 insertions(+), 15 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 007e760..de0031b 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6473,7 +6473,7 @@ static uint32_t arm_v7m_load_vector(ARMCPU *cpu, int exc, bool targets_secure)
>      return addr;
>  }
>  
> -static void v7m_push_callee_stack(ARMCPU *cpu, uint32_t lr, bool dotailchain,
> +static bool v7m_push_callee_stack(ARMCPU *cpu, uint32_t lr, bool dotailchain,
>                                    bool ignore_faults)
>  {
>      /* For v8M, push the callee-saves register part of the stack frame.
> @@ -6481,31 +6481,55 @@ static void v7m_push_callee_stack(ARMCPU *cpu, uint32_t lr, bool dotailchain,
>       * In the tailchaining case this may not be the current stack.
>       */
>      CPUARMState *env = &cpu->env;
> -    CPUState *cs = CPU(cpu);
>      uint32_t *frame_sp_p;
>      uint32_t frameptr;
> +    ARMMMUIdx mmu_idx;
> +    bool stacked_ok;
>  
>      if (dotailchain) {
> -        frame_sp_p = get_v7m_sp_ptr(env, true,
> -                                    lr & R_V7M_EXCRET_MODE_MASK,
> +        bool mode = lr & R_V7M_EXCRET_MODE_MASK;
> +        bool priv = !(env->v7m.control[M_REG_S] & R_V7M_CONTROL_NPRIV_MASK) ||
> +            !mode;
> +
> +        mmu_idx = arm_v7m_mmu_idx_for_secstate_and_priv(env, M_REG_S, priv);
> +        frame_sp_p = get_v7m_sp_ptr(env, M_REG_S, mode,
>                                      lr & R_V7M_EXCRET_SPSEL_MASK);
>      } else {
> +        mmu_idx = core_to_arm_mmu_idx(env, cpu_mmu_index(env, false));
>          frame_sp_p = &env->regs[13];
>      }
>  
>      frameptr = *frame_sp_p - 0x28;
>  
> -    stl_phys(cs->as, frameptr, 0xfefa125b);
> -    stl_phys(cs->as, frameptr + 0x8, env->regs[4]);
> -    stl_phys(cs->as, frameptr + 0xc, env->regs[5]);
> -    stl_phys(cs->as, frameptr + 0x10, env->regs[6]);
> -    stl_phys(cs->as, frameptr + 0x14, env->regs[7]);
> -    stl_phys(cs->as, frameptr + 0x18, env->regs[8]);
> -    stl_phys(cs->as, frameptr + 0x1c, env->regs[9]);
> -    stl_phys(cs->as, frameptr + 0x20, env->regs[10]);
> -    stl_phys(cs->as, frameptr + 0x24, env->regs[11]);
> +    /* Write as much of the stack frame as we can. A write failure may
> +     * cause us to pend a derived exception.
> +     */
> +    stacked_ok =
> +        v7m_stack_write(cpu, frameptr, 0xfefa125b, mmu_idx, ignore_faults) &&
> +        v7m_stack_write(cpu, frameptr + 0x8, env->regs[4], mmu_idx,

nice!

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> +                        ignore_faults) &&
> +        v7m_stack_write(cpu, frameptr + 0xc, env->regs[5], mmu_idx,
> +                        ignore_faults) &&
> +        v7m_stack_write(cpu, frameptr + 0x10, env->regs[6], mmu_idx,
> +                        ignore_faults) &&
> +        v7m_stack_write(cpu, frameptr + 0x14, env->regs[7], mmu_idx,
> +                        ignore_faults) &&
> +        v7m_stack_write(cpu, frameptr + 0x18, env->regs[8], mmu_idx,
> +                        ignore_faults) &&
> +        v7m_stack_write(cpu, frameptr + 0x1c, env->regs[9], mmu_idx,
> +                        ignore_faults) &&
> +        v7m_stack_write(cpu, frameptr + 0x20, env->regs[10], mmu_idx,
> +                        ignore_faults) &&
> +        v7m_stack_write(cpu, frameptr + 0x24, env->regs[11], mmu_idx,
> +                        ignore_faults);
>  
> +    /* Update SP regardless of whether any of the stack accesses failed.
> +     * When we implement v8M stack limit checking then this attempt to
> +     * update SP might also fail and result in a derived exception.
> +     */
>      *frame_sp_p = frameptr;
> +
> +    return !stacked_ok;
>  }
>  
>  static void v7m_exception_taken(ARMCPU *cpu, uint32_t lr, bool dotailchain,
> @@ -6519,6 +6543,7 @@ static void v7m_exception_taken(ARMCPU *cpu, uint32_t lr, bool dotailchain,
>      uint32_t addr;
>      bool targets_secure;
>      int exc;
> +    bool push_failed = false;
>  
>      armv7m_nvic_get_pending_irq_info(env->nvic, &exc, &targets_secure);
>  
> @@ -6546,8 +6571,8 @@ static void v7m_exception_taken(ARMCPU *cpu, uint32_t lr, bool dotailchain,
>                   */
>                  if (lr & R_V7M_EXCRET_DCRS_MASK &&
>                      !(dotailchain && (lr & R_V7M_EXCRET_ES_MASK))) {
> -                    v7m_push_callee_stack(cpu, lr, dotailchain,
> -                                          ignore_stackfaults);
> +                    push_failed = v7m_push_callee_stack(cpu, lr, dotailchain,
> +                                                        ignore_stackfaults);
>                  }
>                  lr |= R_V7M_EXCRET_DCRS_MASK;
>              }
> @@ -6589,6 +6614,15 @@ static void v7m_exception_taken(ARMCPU *cpu, uint32_t lr, bool dotailchain,
>          }
>      }
>  
> +    if (push_failed && !ignore_stackfaults) {
> +        /* Derived exception on callee-saves register stacking:
> +         * we might now want to take a different exception which
> +         * targets a different security state, so try again from the top.
> +         */
> +        v7m_exception_taken(cpu, lr, true, true);
> +        return;
> +    }
> +
>      addr = arm_v7m_load_vector(cpu, exc, targets_secure);
>  
>      /* Now we've done everything that might cause a derived exception
> 


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

[Qemu-devel] [PATCH 6/7] target/arm: Make exception vector loads honour the SAU

[Peter Maydell]

Make the load of the exception vector from the vector table honour
the SAU and any bus error on the load (possibly provoking a derived
exception), rather than simply aborting if the load fails.

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

diff --git a/target/arm/helper.c b/target/arm/helper.c
index de0031b..6931a9d 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6449,28 +6449,63 @@ static uint32_t *get_v7m_sp_ptr(CPUARMState *env, bool secure, bool threadmode,
     }
 }
 
-static uint32_t arm_v7m_load_vector(ARMCPU *cpu, int exc, bool targets_secure)
+static bool arm_v7m_load_vector(ARMCPU *cpu, int exc, bool targets_secure,
+                                uint32_t *pvec)
 {
     CPUState *cs = CPU(cpu);
     CPUARMState *env = &cpu->env;
     MemTxResult result;
-    hwaddr vec = env->v7m.vecbase[targets_secure] + exc * 4;
-    uint32_t addr;
+    uint32_t addr = env->v7m.vecbase[targets_secure] + exc * 4;
+    uint32_t vector_entry;
+    MemTxAttrs attrs = {};
+    ARMMMUIdx mmu_idx;
+    bool exc_secure;
+
+    mmu_idx = arm_v7m_mmu_idx_for_secstate_and_priv(env, targets_secure, true);
 
-    addr = address_space_ldl(cs->as, vec,
-                             MEMTXATTRS_UNSPECIFIED, &result);
+    /* We don't do a get_phys_addr() here because the rules for vector
+     * loads are special: they always use the default memory map, and
+     * the default memory map permits reads from all addresses.
+     * Since there's no easy way to pass through to pmsav8_mpu_lookup()
+     * that we want this special case which would always say "yes",
+     * we just do the SAU lookup here followed by a direct physical load.
+     */
+    attrs.secure = targets_secure;
+    attrs.user = false;
+
+    if (arm_feature(env, ARM_FEATURE_M_SECURITY)) {
+        V8M_SAttributes sattrs = {};
+
+        v8m_security_lookup(env, addr, MMU_DATA_LOAD, mmu_idx, &sattrs);
+        if (sattrs.ns) {
+            attrs.secure = false;
+        } else if (!targets_secure) {
+            /* NS access to S memory */
+            goto load_fail;
+        }
+    }
+
+    vector_entry = address_space_ldl(arm_addressspace(cs, attrs), addr,
+                                     attrs, &result);
     if (result != MEMTX_OK) {
-        /* Architecturally this should cause a HardFault setting HSFR.VECTTBL,
-         * which would then be immediately followed by our failing to load
-         * the entry vector for that HardFault, which is a Lockup case.
-         * Since we don't model Lockup, we just report this guest error
-         * via cpu_abort().
-         */
-        cpu_abort(cs, "Failed to read from %s exception vector table "
-                  "entry %08x\n", targets_secure ? "secure" : "nonsecure",
-                  (unsigned)vec);
+        goto load_fail;
     }
-    return addr;
+    *pvec = vector_entry;
+    return true;
+
+load_fail:
+    /* All vector table fetch fails are reported as HardFault, with
+     * HFSR.VECTTBL and .FORCED set. (FORCED is set because
+     * technically the underlying exception is a MemManage or BusFault
+     * that is escalated to HardFault.) This is a terminal exception,
+     * so we will either take the HardFault immediately or else enter
+     * lockup (the latter case is handled in armv7m_nvic_set_pending_derived()).
+     */
+    exc_secure = targets_secure ||
+        !(cpu->env.v7m.aircr & R_V7M_AIRCR_BFHFNMINS_MASK);
+    env->v7m.hfsr |= R_V7M_HFSR_VECTTBL_MASK | R_V7M_HFSR_FORCED_MASK;
+    armv7m_nvic_set_pending_derived(env->nvic, ARMV7M_EXCP_HARD, exc_secure);
+    return false;
 }
 
 static bool v7m_push_callee_stack(ARMCPU *cpu, uint32_t lr, bool dotailchain,
@@ -6623,7 +6658,11 @@ static void v7m_exception_taken(ARMCPU *cpu, uint32_t lr, bool dotailchain,
         return;
     }
 
-    addr = arm_v7m_load_vector(cpu, exc, targets_secure);
+    if (!arm_v7m_load_vector(cpu, exc, targets_secure, &addr)) {
+        /* Vector load failed: derived exception */
+        v7m_exception_taken(cpu, lr, true, true);
+        return;
+    }
 
     /* Now we've done everything that might cause a derived exception
      * we can go ahead and activate whichever exception we're going to
-- 
2.7.4

Re: [Qemu-devel] [PATCH 6/7] target/arm: Make exception vector loads honour the SAU

[Richard Henderson]

On 01/30/2018 07:02 AM, Peter Maydell wrote:
> Make the load of the exception vector from the vector table honour
> the SAU and any bus error on the load (possibly provoking a derived
> exception), rather than simply aborting if the load fails.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 71 +++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 55 insertions(+), 16 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH 7/7] target/arm: Handle exceptions during exception stack pop

[Peter Maydell]

Handle possible MPU faults, SAU faults or bus errors when
popping register state off the stack during exception return.

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

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 6931a9d..3332565 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6223,6 +6223,67 @@ pend_fault:
     return false;
 }
 
+static bool v7m_stack_read(ARMCPU *cpu, uint32_t *dest, uint32_t addr,
+                           ARMMMUIdx mmu_idx)
+{
+    CPUState *cs = CPU(cpu);
+    CPUARMState *env = &cpu->env;
+    MemTxAttrs attrs = {};
+    MemTxResult txres;
+    target_ulong page_size;
+    hwaddr physaddr;
+    int prot;
+    ARMMMUFaultInfo fi;
+    bool secure = mmu_idx & ARM_MMU_IDX_M_S;
+    int exc;
+    bool exc_secure;
+    uint32_t value;
+
+    if (get_phys_addr(env, addr, MMU_DATA_LOAD, mmu_idx, &physaddr,
+                      &attrs, &prot, &page_size, &fi, NULL)) {
+        /* MPU/SAU lookup failed */
+        if (fi.type == ARMFault_QEMU_SFault) {
+            qemu_log_mask(CPU_LOG_INT,
+                          "...SecureFault with SFSR.AUVIOL during unstack\n");
+            env->v7m.sfsr |= R_V7M_SFSR_AUVIOL_MASK | R_V7M_SFSR_SFARVALID_MASK;
+            env->v7m.sfar = addr;
+            exc = ARMV7M_EXCP_SECURE;
+            exc_secure = true;
+        } else {
+            qemu_log_mask(CPU_LOG_INT,
+                          "...MemManageFault with CFSR.MUNSTKERR\n");
+            env->v7m.cfsr[secure] |= R_V7M_CFSR_MUNSTKERR_MASK;
+            exc = ARMV7M_EXCP_MEM;
+            exc_secure = secure;
+        }
+        goto pend_fault;
+    }
+
+    value = address_space_ldl(arm_addressspace(cs, attrs), physaddr,
+                              attrs, &txres);
+    if (txres != MEMTX_OK) {
+        /* BusFault trying to read the data */
+        qemu_log_mask(CPU_LOG_INT, "...BusFault with BFSR.UNSTKERR\n");
+        env->v7m.cfsr[M_REG_NS] |= R_V7M_CFSR_UNSTKERR_MASK;
+        exc = ARMV7M_EXCP_BUS;
+        exc_secure = false;
+        goto pend_fault;
+    }
+
+    *dest = value;
+    return true;
+
+pend_fault:
+    /* By pending the exception at this point we are making
+     * the IMPDEF choice "overridden exceptions pended" (see the
+     * MergeExcInfo() pseudocode). The other choice would be to not
+     * pend them now and then make a choice about which to throw away
+     * later if we have two derived exceptions.
+     */
+    armv7m_nvic_set_pending(env->nvic, exc, exc_secure);
+    return false;
+}
+
 /* Return true if we're using the process stack pointer (not the MSP) */
 static bool v7m_using_psp(CPUARMState *env)
 {
@@ -6912,6 +6973,11 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
                                               !return_to_handler,
                                               return_to_sp_process);
         uint32_t frameptr = *frame_sp_p;
+        bool pop_ok = true;
+        ARMMMUIdx mmu_idx;
+
+        mmu_idx = arm_v7m_mmu_idx_for_secstate_and_priv(env, return_to_secure,
+                                                        !return_to_handler);
 
         if (!QEMU_IS_ALIGNED(frameptr, 8) &&
             arm_feature(env, ARM_FEATURE_V8)) {
@@ -6938,29 +7004,38 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
                 return;
             }
 
-            env->regs[4] = ldl_phys(cs->as, frameptr + 0x8);
-            env->regs[5] = ldl_phys(cs->as, frameptr + 0xc);
-            env->regs[6] = ldl_phys(cs->as, frameptr + 0x10);
-            env->regs[7] = ldl_phys(cs->as, frameptr + 0x14);
-            env->regs[8] = ldl_phys(cs->as, frameptr + 0x18);
-            env->regs[9] = ldl_phys(cs->as, frameptr + 0x1c);
-            env->regs[10] = ldl_phys(cs->as, frameptr + 0x20);
-            env->regs[11] = ldl_phys(cs->as, frameptr + 0x24);
+            pop_ok =
+                v7m_stack_read(cpu, &env->regs[4], frameptr + 0x8, mmu_idx) &&
+                v7m_stack_read(cpu, &env->regs[4], frameptr + 0x8, mmu_idx) &&
+                v7m_stack_read(cpu, &env->regs[5], frameptr + 0xc, mmu_idx) &&
+                v7m_stack_read(cpu, &env->regs[6], frameptr + 0x10, mmu_idx) &&
+                v7m_stack_read(cpu, &env->regs[7], frameptr + 0x14, mmu_idx) &&
+                v7m_stack_read(cpu, &env->regs[8], frameptr + 0x18, mmu_idx) &&
+                v7m_stack_read(cpu, &env->regs[9], frameptr + 0x1c, mmu_idx) &&
+                v7m_stack_read(cpu, &env->regs[10], frameptr + 0x20, mmu_idx) &&
+                v7m_stack_read(cpu, &env->regs[11], frameptr + 0x24, mmu_idx);
 
             frameptr += 0x28;
         }
 
-        /* Pop registers. TODO: make these accesses use the correct
-         * attributes and address space (S/NS, priv/unpriv) and handle
-         * memory transaction failures.
-         */
-        env->regs[0] = ldl_phys(cs->as, frameptr);
-        env->regs[1] = ldl_phys(cs->as, frameptr + 0x4);
-        env->regs[2] = ldl_phys(cs->as, frameptr + 0x8);
-        env->regs[3] = ldl_phys(cs->as, frameptr + 0xc);
-        env->regs[12] = ldl_phys(cs->as, frameptr + 0x10);
-        env->regs[14] = ldl_phys(cs->as, frameptr + 0x14);
-        env->regs[15] = ldl_phys(cs->as, frameptr + 0x18);
+        /* Pop registers */
+        pop_ok = pop_ok &&
+            v7m_stack_read(cpu, &env->regs[0], frameptr, mmu_idx) &&
+            v7m_stack_read(cpu, &env->regs[1], frameptr + 0x4, mmu_idx) &&
+            v7m_stack_read(cpu, &env->regs[2], frameptr + 0x8, mmu_idx) &&
+            v7m_stack_read(cpu, &env->regs[3], frameptr + 0xc, mmu_idx) &&
+            v7m_stack_read(cpu, &env->regs[12], frameptr + 0x10, mmu_idx) &&
+            v7m_stack_read(cpu, &env->regs[14], frameptr + 0x14, mmu_idx) &&
+            v7m_stack_read(cpu, &env->regs[15], frameptr + 0x18, mmu_idx) &&
+            v7m_stack_read(cpu, &xpsr, frameptr + 0x1c, mmu_idx);
+
+        if (!pop_ok) {
+            /* v7m_stack_read() pended a fault, so take it (as a tail
+             * chained exception on the same stack frame)
+             */
+            v7m_exception_taken(cpu, excret, true, false);
+            return;
+        }
 
         /* Returning from an exception with a PC with bit 0 set is defined
          * behaviour on v8M (bit 0 is ignored), but for v7M it was specified
@@ -6979,8 +7054,6 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
             }
         }
 
-        xpsr = ldl_phys(cs->as, frameptr + 0x1c);
-
         if (arm_feature(env, ARM_FEATURE_V8)) {
             /* For v8M we have to check whether the xPSR exception field
              * matches the EXCRET value for return to handler/thread
-- 
2.7.4

Re: [Qemu-devel] [PATCH 7/7] target/arm: Handle exceptions during exception stack pop

[Richard Henderson]

On 01/30/2018 07:02 AM, Peter Maydell wrote:
> Handle possible MPU faults, SAU faults or bus errors when
> popping register state off the stack during exception return.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 115 ++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 94 insertions(+), 21 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~