[Qemu-devel] [PATCH 00/13] target/arm: Implement v8M stack limit checks

[Qemu-devel] [PATCH 00/13] target/arm: Implement v8M stack limit checks

[Peter Maydell]

This patchset implements the v8M stack limit checking
feature, which is the last missing piece of the v8M
architectural support.

Note that the stack limit triggers when the SP value
is changed to something below the limit, not when
a load or store is performed below the limit. It's
also done only for certain instructions that update
SP, not for every possible way to change SP. For
loads and stores which do writeback to SP there are
also some rules about what parts of the load/store
are permitted to happen if the check triggers -- we
keep things simple by taking the approach of doing
the check first so that no accesses are done.

We take a straightforward approach to implementing
the checks: generating a call to a helper function
which does the comparison and might raise an exception.
This obviously imposes some overhead for the common
case where the limit isn't being breached, but
generating code for a compare-and-conditionally-call
seemed too tricky to insert into the existing code...

thanks
-- PMM

Peter Maydell (13):
  target/arm: Define new TBFLAG for v8M stack checking
  target/arm: Define new EXCP type for v8M stack overflows
  target/arm: Move v7m_using_psp() to internals.h
  target/arm: Add v8M stack checks on ADD/SUB/MOV of SP
  target/arm: Add some comments in Thumb decode
  target/arm: Add v8M stack checks on exception entry
  target/arm: Add v8M stack limit checks on NS function calls
  target/arm: Add v8M stack checks for LDRD/STRD (imm)
  target/arm: Add v8M stack checks for Thumb2 LDM/STM
  target/arm: Add v8M stack checks for T32 load/store single
  target/arm: Add v8M stack checks for Thumb push/pop
  target/arm: Add v8M stack checks for VLDM/VSTM
  target/arm: Add v8M stack checks for MSR to SP_NS

 target/arm/cpu.h       |   9 ++
 target/arm/helper.h    |   2 +
 target/arm/internals.h |  38 ++++++++
 target/arm/translate.h |   1 +
 target/arm/helper.c    |  99 ++++++++++++++++-----
 target/arm/op_helper.c |  23 ++++-
 target/arm/translate.c | 198 +++++++++++++++++++++++++++++++++++++----
 7 files changed, 330 insertions(+), 40 deletions(-)

-- 
2.19.0

[Qemu-devel] [PATCH 01/13] target/arm: Define new TBFLAG for v8M stack checking

[Peter Maydell]

The Arm v8M architecture includes hardware stack limit checking.
When certain instructions update the stack pointer, if the new
value of SP is below the limit set in the associated limit register
then an exception is taken. Add a TB flag that tracks whether
the limit-checking code needs to be emitted.

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

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 65c0fa0a659..d2c1d005ed7 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1336,8 +1336,10 @@ FIELD(V7M_CCR, UNALIGN_TRP, 3, 1)
 FIELD(V7M_CCR, DIV_0_TRP, 4, 1)
 FIELD(V7M_CCR, BFHFNMIGN, 8, 1)
 FIELD(V7M_CCR, STKALIGN, 9, 1)
+FIELD(V7M_CCR, STKOFHFNMIGN, 10, 1)
 FIELD(V7M_CCR, DC, 16, 1)
 FIELD(V7M_CCR, IC, 17, 1)
+FIELD(V7M_CCR, BP, 18, 1)
 
 /* V7M SCR bits */
 FIELD(V7M_SCR, SLEEPONEXIT, 1, 1)
@@ -2842,6 +2844,9 @@ static inline bool arm_cpu_data_is_big_endian(CPUARMState *env)
 /* For M profile only, Handler (ie not Thread) mode */
 #define ARM_TBFLAG_HANDLER_SHIFT    21
 #define ARM_TBFLAG_HANDLER_MASK     (1 << ARM_TBFLAG_HANDLER_SHIFT)
+/* For M profile only, whether we should generate stack-limit checks */
+#define ARM_TBFLAG_STACKCHECK_SHIFT 22
+#define ARM_TBFLAG_STACKCHECK_MASK  (1 << ARM_TBFLAG_STACKCHECK_SHIFT)
 
 /* Bit usage when in AArch64 state */
 #define ARM_TBFLAG_TBI0_SHIFT 0        /* TBI0 for EL0/1 or TBI for EL2/3 */
@@ -2884,6 +2889,8 @@ static inline bool arm_cpu_data_is_big_endian(CPUARMState *env)
     (((F) & ARM_TBFLAG_BE_DATA_MASK) >> ARM_TBFLAG_BE_DATA_SHIFT)
 #define ARM_TBFLAG_HANDLER(F) \
     (((F) & ARM_TBFLAG_HANDLER_MASK) >> ARM_TBFLAG_HANDLER_SHIFT)
+#define ARM_TBFLAG_STACKCHECK(F) \
+    (((F) & ARM_TBFLAG_STACKCHECK_MASK) >> ARM_TBFLAG_STACKCHECK_SHIFT)
 #define ARM_TBFLAG_TBI0(F) \
     (((F) & ARM_TBFLAG_TBI0_MASK) >> ARM_TBFLAG_TBI0_SHIFT)
 #define ARM_TBFLAG_TBI1(F) \
diff --git a/target/arm/translate.h b/target/arm/translate.h
index 45f04244be8..c1b65f3efb0 100644
--- a/target/arm/translate.h
+++ b/target/arm/translate.h
@@ -38,6 +38,7 @@ typedef struct DisasContext {
     int vec_stride;
     bool v7m_handler_mode;
     bool v8m_secure; /* true if v8M and we're in Secure mode */
+    bool v8m_stackcheck; /* true if we need to perform v8M stack limit checks */
     /* Immediate value in AArch32 SVC insn; must be set if is_jmp == DISAS_SWI
      * so that top level loop can generate correct syndrome information.
      */
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 5e721a65272..6ed8631dbee 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -12667,6 +12667,16 @@ void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
         flags |= ARM_TBFLAG_HANDLER_MASK;
     }
 
+    /* v8M always applies stack limit checks unless CCR.STKOFHFNMIGN is
+     * suppressing them because the requested execution priority is less than 0.
+     */
+    if (arm_feature(env, ARM_FEATURE_V8) &&
+        arm_feature(env, ARM_FEATURE_M) &&
+        !((mmu_idx  & ARM_MMU_IDX_M_NEGPRI) &&
+          (env->v7m.ccr[env->v7m.secure] & R_V7M_CCR_STKOFHFNMIGN_MASK))) {
+        flags |= ARM_TBFLAG_STACKCHECK_MASK;
+    }
+
     *pflags = flags;
     *cs_base = 0;
 }
diff --git a/target/arm/translate.c b/target/arm/translate.c
index c6a5d2ac444..751d5811cee 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -12451,6 +12451,7 @@ static void arm_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
     dc->v7m_handler_mode = ARM_TBFLAG_HANDLER(dc->base.tb->flags);
     dc->v8m_secure = arm_feature(env, ARM_FEATURE_M_SECURITY) &&
         regime_is_secure(env, dc->mmu_idx);
+    dc->v8m_stackcheck = ARM_TBFLAG_STACKCHECK(dc->base.tb->flags);
     dc->cp_regs = cpu->cp_regs;
     dc->features = env->features;
 
-- 
2.19.0

Re: [Qemu-devel] [PATCH 01/13] target/arm: Define new TBFLAG for v8M stack checking

[Richard Henderson]

On 10/2/18 11:35 AM, Peter Maydell wrote:
> The Arm v8M architecture includes hardware stack limit checking.
> When certain instructions update the stack pointer, if the new
> value of SP is below the limit set in the associated limit register
> then an exception is taken. Add a TB flag that tracks whether
> the limit-checking code needs to be emitted.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/cpu.h       |  7 +++++++
>  target/arm/translate.h |  1 +
>  target/arm/helper.c    | 10 ++++++++++
>  target/arm/translate.c |  1 +
>  4 files changed, 19 insertions(+)

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

r~

Re: [Qemu-devel] [PATCH 01/13] target/arm: Define new TBFLAG for v8M stack checking

[Philippe Mathieu-Daudé]

Hi Peter,

On 02/10/2018 18:35, Peter Maydell wrote:
> The Arm v8M architecture includes hardware stack limit checking.
> When certain instructions update the stack pointer, if the new
> value of SP is below the limit set in the associated limit register
> then an exception is taken. Add a TB flag that tracks whether
> the limit-checking code needs to be emitted.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/cpu.h       |  7 +++++++
>  target/arm/translate.h |  1 +
>  target/arm/helper.c    | 10 ++++++++++
>  target/arm/translate.c |  1 +
>  4 files changed, 19 insertions(+)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 65c0fa0a659..d2c1d005ed7 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1336,8 +1336,10 @@ FIELD(V7M_CCR, UNALIGN_TRP, 3, 1)
>  FIELD(V7M_CCR, DIV_0_TRP, 4, 1)
>  FIELD(V7M_CCR, BFHFNMIGN, 8, 1)
>  FIELD(V7M_CCR, STKALIGN, 9, 1)
> +FIELD(V7M_CCR, STKOFHFNMIGN, 10, 1)
>  FIELD(V7M_CCR, DC, 16, 1)
>  FIELD(V7M_CCR, IC, 17, 1)
> +FIELD(V7M_CCR, BP, 18, 1)
>  
>  /* V7M SCR bits */
>  FIELD(V7M_SCR, SLEEPONEXIT, 1, 1)
> @@ -2842,6 +2844,9 @@ static inline bool arm_cpu_data_is_big_endian(CPUARMState *env)
>  /* For M profile only, Handler (ie not Thread) mode */
>  #define ARM_TBFLAG_HANDLER_SHIFT    21
>  #define ARM_TBFLAG_HANDLER_MASK     (1 << ARM_TBFLAG_HANDLER_SHIFT)
> +/* For M profile only, whether we should generate stack-limit checks */
> +#define ARM_TBFLAG_STACKCHECK_SHIFT 22
> +#define ARM_TBFLAG_STACKCHECK_MASK  (1 << ARM_TBFLAG_STACKCHECK_SHIFT)
>  
>  /* Bit usage when in AArch64 state */
>  #define ARM_TBFLAG_TBI0_SHIFT 0        /* TBI0 for EL0/1 or TBI for EL2/3 */
> @@ -2884,6 +2889,8 @@ static inline bool arm_cpu_data_is_big_endian(CPUARMState *env)
>      (((F) & ARM_TBFLAG_BE_DATA_MASK) >> ARM_TBFLAG_BE_DATA_SHIFT)
>  #define ARM_TBFLAG_HANDLER(F) \
>      (((F) & ARM_TBFLAG_HANDLER_MASK) >> ARM_TBFLAG_HANDLER_SHIFT)
> +#define ARM_TBFLAG_STACKCHECK(F) \
> +    (((F) & ARM_TBFLAG_STACKCHECK_MASK) >> ARM_TBFLAG_STACKCHECK_SHIFT)
>  #define ARM_TBFLAG_TBI0(F) \
>      (((F) & ARM_TBFLAG_TBI0_MASK) >> ARM_TBFLAG_TBI0_SHIFT)
>  #define ARM_TBFLAG_TBI1(F) \
> diff --git a/target/arm/translate.h b/target/arm/translate.h
> index 45f04244be8..c1b65f3efb0 100644
> --- a/target/arm/translate.h
> +++ b/target/arm/translate.h
> @@ -38,6 +38,7 @@ typedef struct DisasContext {
>      int vec_stride;
>      bool v7m_handler_mode;
>      bool v8m_secure; /* true if v8M and we're in Secure mode */
> +    bool v8m_stackcheck; /* true if we need to perform v8M stack limit checks */

OK

>      /* Immediate value in AArch32 SVC insn; must be set if is_jmp == DISAS_SWI
>       * so that top level loop can generate correct syndrome information.
>       */
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 5e721a65272..6ed8631dbee 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -12667,6 +12667,16 @@ void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
>          flags |= ARM_TBFLAG_HANDLER_MASK;
>      }
>  
> +    /* v8M always applies stack limit checks unless CCR.STKOFHFNMIGN is
> +     * suppressing them because the requested execution priority is less than 0.
> +     */
> +    if (arm_feature(env, ARM_FEATURE_V8) &&
> +        arm_feature(env, ARM_FEATURE_M) &&
> +        !((mmu_idx  & ARM_MMU_IDX_M_NEGPRI) &&
> +          (env->v7m.ccr[env->v7m.secure] & R_V7M_CCR_STKOFHFNMIGN_MASK))) {

Specs:

    CCR.STKOFHFNMIGN controls whether stack limit violations
    are IGNORED while executing at a requested execution priority
    that is negative.

    The possible values of this bit are:
    0 Stack limit faults not ignored.
    1 Stack limit faults at requested priorities of less than 0 ignored.

OK, the '!((' notation was hard to read, hopefully the 2 indent spaces
in the 2nd line helped...

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> +        flags |= ARM_TBFLAG_STACKCHECK_MASK;
> +    }
> +
>      *pflags = flags;
>      *cs_base = 0;
>  }
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index c6a5d2ac444..751d5811cee 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -12451,6 +12451,7 @@ static void arm_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>      dc->v7m_handler_mode = ARM_TBFLAG_HANDLER(dc->base.tb->flags);
>      dc->v8m_secure = arm_feature(env, ARM_FEATURE_M_SECURITY) &&
>          regime_is_secure(env, dc->mmu_idx);
> +    dc->v8m_stackcheck = ARM_TBFLAG_STACKCHECK(dc->base.tb->flags);
>      dc->cp_regs = cpu->cp_regs;
>      dc->features = env->features;
>  
> 

[Qemu-devel] [PATCH 02/13] target/arm: Define new EXCP type for v8M stack overflows

[Peter Maydell]

Define EXCP_STKOF, and arrange for it to cause us to take
a UsageFault with CFSR.STKOF set.

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

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index d2c1d005ed7..318792823b9 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -56,6 +56,7 @@
 #define EXCP_SEMIHOST       16   /* semihosting call */
 #define EXCP_NOCP           17   /* v7M NOCP UsageFault */
 #define EXCP_INVSTATE       18   /* v7M INVSTATE UsageFault */
+#define EXCP_STKOF          19   /* v8M STKOF UsageFault */
 /* NB: add new EXCP_ defines to the array in arm_log_exception() too */
 
 #define ARMV7M_EXCP_RESET   1
@@ -1380,6 +1381,7 @@ FIELD(V7M_CFSR, UNDEFINSTR, 16 + 0, 1)
 FIELD(V7M_CFSR, INVSTATE, 16 + 1, 1)
 FIELD(V7M_CFSR, INVPC, 16 + 2, 1)
 FIELD(V7M_CFSR, NOCP, 16 + 3, 1)
+FIELD(V7M_CFSR, STKOF, 16 + 4, 1)
 FIELD(V7M_CFSR, UNALIGNED, 16 + 8, 1)
 FIELD(V7M_CFSR, DIVBYZERO, 16 + 9, 1)
 
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 6ed8631dbee..c303dc453f1 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -7511,6 +7511,7 @@ static void arm_log_exception(int idx)
             [EXCP_SEMIHOST] = "Semihosting call",
             [EXCP_NOCP] = "v7M NOCP UsageFault",
             [EXCP_INVSTATE] = "v7M INVSTATE UsageFault",
+            [EXCP_STKOF] = "v8M STKOF UsageFault",
         };
 
         if (idx >= 0 && idx < ARRAY_SIZE(excnames)) {
@@ -7666,6 +7667,10 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
         armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE, env->v7m.secure);
         env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_INVSTATE_MASK;
         break;
+    case EXCP_STKOF:
+        armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE, env->v7m.secure);
+        env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_STKOF_MASK;
+        break;
     case EXCP_SWI:
         /* The PC already points to the next instruction.  */
         armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_SVC, env->v7m.secure);
-- 
2.19.0

Re: [Qemu-devel] [PATCH 02/13] target/arm: Define new EXCP type for v8M stack overflows

[Philippe Mathieu-Daudé]

On 02/10/2018 18:35, Peter Maydell wrote:
> Define EXCP_STKOF, and arrange for it to cause us to take
> a UsageFault with CFSR.STKOF set.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  target/arm/cpu.h    | 2 ++
>  target/arm/helper.c | 5 +++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index d2c1d005ed7..318792823b9 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -56,6 +56,7 @@
>  #define EXCP_SEMIHOST       16   /* semihosting call */
>  #define EXCP_NOCP           17   /* v7M NOCP UsageFault */
>  #define EXCP_INVSTATE       18   /* v7M INVSTATE UsageFault */
> +#define EXCP_STKOF          19   /* v8M STKOF UsageFault */
>  /* NB: add new EXCP_ defines to the array in arm_log_exception() too */
>  
>  #define ARMV7M_EXCP_RESET   1
> @@ -1380,6 +1381,7 @@ FIELD(V7M_CFSR, UNDEFINSTR, 16 + 0, 1)
>  FIELD(V7M_CFSR, INVSTATE, 16 + 1, 1)
>  FIELD(V7M_CFSR, INVPC, 16 + 2, 1)
>  FIELD(V7M_CFSR, NOCP, 16 + 3, 1)
> +FIELD(V7M_CFSR, STKOF, 16 + 4, 1)
>  FIELD(V7M_CFSR, UNALIGNED, 16 + 8, 1)
>  FIELD(V7M_CFSR, DIVBYZERO, 16 + 9, 1)
>  
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 6ed8631dbee..c303dc453f1 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -7511,6 +7511,7 @@ static void arm_log_exception(int idx)
>              [EXCP_SEMIHOST] = "Semihosting call",
>              [EXCP_NOCP] = "v7M NOCP UsageFault",
>              [EXCP_INVSTATE] = "v7M INVSTATE UsageFault",
> +            [EXCP_STKOF] = "v8M STKOF UsageFault",
>          };
>  
>          if (idx >= 0 && idx < ARRAY_SIZE(excnames)) {
> @@ -7666,6 +7667,10 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>          armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE, env->v7m.secure);
>          env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_INVSTATE_MASK;
>          break;
> +    case EXCP_STKOF:
> +        armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE, env->v7m.secure);
> +        env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_STKOF_MASK;
> +        break;
>      case EXCP_SWI:
>          /* The PC already points to the next instruction.  */
>          armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_SVC, env->v7m.secure);
> 

Re: [Qemu-devel] [PATCH 02/13] target/arm: Define new EXCP type for v8M stack overflows

[Richard Henderson]

On 10/2/18 11:35 AM, Peter Maydell wrote:
> Define EXCP_STKOF, and arrange for it to cause us to take
> a UsageFault with CFSR.STKOF set.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/cpu.h    | 2 ++
>  target/arm/helper.c | 5 +++++
>  2 files changed, 7 insertions(+)

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

r~

[Qemu-devel] [PATCH 03/13] target/arm: Move v7m_using_psp() to internals.h

[Peter Maydell]

We're going to want v7m_using_psp() in op_helper.c in the
next patch, so move it from helper.c to internals.h.

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

diff --git a/target/arm/internals.h b/target/arm/internals.h
index dc9357766c9..bc4c01ccd92 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -796,4 +796,19 @@ static inline uint32_t arm_debug_exception_fsr(CPUARMState *env)
     }
 }
 
+/**
+ * v7m_using_psp: Return true if using process stack pointer
+ * Return true if the CPU is currently using the process stack
+ * pointer, or false if it is using the main stack pointer.
+ */
+static inline bool v7m_using_psp(CPUARMState *env)
+{
+    /* Handler mode always uses the main stack; for thread mode
+     * the CONTROL.SPSEL bit determines the answer.
+     * Note that in v7M it is not possible to be in Handler mode with
+     * CONTROL.SPSEL non-zero, but in v8M it is, so we must check both.
+     */
+    return !arm_v7m_is_handler_mode(env) &&
+        env->v7m.control[env->v7m.secure] & R_V7M_CONTROL_SPSEL_MASK;
+}
 #endif
diff --git a/target/arm/helper.c b/target/arm/helper.c
index c303dc453f1..ef8c244fb84 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6554,18 +6554,6 @@ pend_fault:
     return false;
 }
 
-/* Return true if we're using the process stack pointer (not the MSP) */
-static bool v7m_using_psp(CPUARMState *env)
-{
-    /* Handler mode always uses the main stack; for thread mode
-     * the CONTROL.SPSEL bit determines the answer.
-     * Note that in v7M it is not possible to be in Handler mode with
-     * CONTROL.SPSEL non-zero, but in v8M it is, so we must check both.
-     */
-    return !arm_v7m_is_handler_mode(env) &&
-        env->v7m.control[env->v7m.secure] & R_V7M_CONTROL_SPSEL_MASK;
-}
-
 /* Write to v7M CONTROL.SPSEL bit for the specified security bank.
  * This may change the current stack pointer between Main and Process
  * stack pointers if it is done for the CONTROL register for the current
-- 
2.19.0

Re: [Qemu-devel] [PATCH 03/13] target/arm: Move v7m_using_psp() to internals.h

[Philippe Mathieu-Daudé]

On 02/10/2018 18:35, Peter Maydell wrote:
> We're going to want v7m_using_psp() in op_helper.c in the
> next patch, so move it from helper.c to internals.h.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  target/arm/internals.h | 15 +++++++++++++++
>  target/arm/helper.c    | 12 ------------
>  2 files changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index dc9357766c9..bc4c01ccd92 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -796,4 +796,19 @@ static inline uint32_t arm_debug_exception_fsr(CPUARMState *env)
>      }
>  }
>  
> +/**
> + * v7m_using_psp: Return true if using process stack pointer
> + * Return true if the CPU is currently using the process stack
> + * pointer, or false if it is using the main stack pointer.
> + */
> +static inline bool v7m_using_psp(CPUARMState *env)
> +{
> +    /* Handler mode always uses the main stack; for thread mode
> +     * the CONTROL.SPSEL bit determines the answer.
> +     * Note that in v7M it is not possible to be in Handler mode with
> +     * CONTROL.SPSEL non-zero, but in v8M it is, so we must check both.
> +     */
> +    return !arm_v7m_is_handler_mode(env) &&
> +        env->v7m.control[env->v7m.secure] & R_V7M_CONTROL_SPSEL_MASK;
> +}
>  #endif
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index c303dc453f1..ef8c244fb84 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6554,18 +6554,6 @@ pend_fault:
>      return false;
>  }
>  
> -/* Return true if we're using the process stack pointer (not the MSP) */
> -static bool v7m_using_psp(CPUARMState *env)
> -{
> -    /* Handler mode always uses the main stack; for thread mode
> -     * the CONTROL.SPSEL bit determines the answer.
> -     * Note that in v7M it is not possible to be in Handler mode with
> -     * CONTROL.SPSEL non-zero, but in v8M it is, so we must check both.
> -     */
> -    return !arm_v7m_is_handler_mode(env) &&
> -        env->v7m.control[env->v7m.secure] & R_V7M_CONTROL_SPSEL_MASK;
> -}
> -
>  /* Write to v7M CONTROL.SPSEL bit for the specified security bank.
>   * This may change the current stack pointer between Main and Process
>   * stack pointers if it is done for the CONTROL register for the current
> 

Re: [Qemu-devel] [PATCH 03/13] target/arm: Move v7m_using_psp() to internals.h

[Richard Henderson]

On 10/2/18 11:35 AM, Peter Maydell wrote:
> We're going to want v7m_using_psp() in op_helper.c in the
> next patch, so move it from helper.c to internals.h.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/internals.h | 15 +++++++++++++++
>  target/arm/helper.c    | 12 ------------
>  2 files changed, 15 insertions(+), 12 deletions(-)

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

r~

[Qemu-devel] [PATCH 04/13] target/arm: Add v8M stack checks on ADD/SUB/MOV of SP

[Peter Maydell]

Add code to insert calls to a helper function to do the stack
limit checking when we handle these forms of instruction
that write to SP:
 * ADD (SP plus immediate)
 * ADD (SP plus register)
 * SUB (SP minus immediate)
 * SUB (SP minus register)
 * MOV (register)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.h    |  2 ++
 target/arm/internals.h | 14 ++++++++
 target/arm/op_helper.c | 19 ++++++++++
 target/arm/translate.c | 80 +++++++++++++++++++++++++++++++++++++-----
 4 files changed, 106 insertions(+), 9 deletions(-)

diff --git a/target/arm/helper.h b/target/arm/helper.h
index 59e8c3bd1b9..8c9590091b0 100644
--- a/target/arm/helper.h
+++ b/target/arm/helper.h
@@ -69,6 +69,8 @@ DEF_HELPER_2(v7m_blxns, void, env, i32)
 
 DEF_HELPER_3(v7m_tt, i32, env, i32, i32)
 
+DEF_HELPER_2(v8m_stackcheck, void, env, i32)
+
 DEF_HELPER_4(access_check_cp_reg, void, env, ptr, i32, i32)
 DEF_HELPER_3(set_cp_reg, void, env, ptr, i32)
 DEF_HELPER_2(get_cp_reg, i32, env, ptr)
diff --git a/target/arm/internals.h b/target/arm/internals.h
index bc4c01ccd92..966a8131623 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -811,4 +811,18 @@ static inline bool v7m_using_psp(CPUARMState *env)
     return !arm_v7m_is_handler_mode(env) &&
         env->v7m.control[env->v7m.secure] & R_V7M_CONTROL_SPSEL_MASK;
 }
+
+/**
+ * v7m_sp_limit: Return SP limit for current CPU state
+ * Return the SP limit value for the current CPU security state
+ * and stack pointer.
+ */
+static inline uint32_t v7m_sp_limit(CPUARMState *env)
+{
+    if (v7m_using_psp(env)) {
+        return env->v7m.psplim[env->v7m.secure];
+    } else {
+        return env->v7m.msplim[env->v7m.secure];
+    }
+}
 #endif
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index 952b8d122b7..38f885b290f 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -238,6 +238,25 @@ void arm_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
 
 #endif /* !defined(CONFIG_USER_ONLY) */
 
+void HELPER(v8m_stackcheck)(CPUARMState *env, uint32_t newvalue)
+{
+    /*
+     * Perform the v8M stack limit check for SP updates from translated code,
+     * raising an exception if the limit is breached.
+     */
+    if (newvalue < v7m_sp_limit(env)) {
+        CPUState *cs = CPU(arm_env_get_cpu(env));
+
+        /*
+         * Stack limit exceptions are a rare case, so rather than syncing
+         * PC/condbits before the call, we use cpu_restore_state() to
+         * get them right before raising the exception.
+         */
+        cpu_restore_state(cs, GETPC(), true);
+        raise_exception(env, EXCP_STKOF, 0, 1);
+    }
+}
+
 uint32_t HELPER(add_setq)(CPUARMState *env, uint32_t a, uint32_t b)
 {
     uint32_t res = a + b;
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 751d5811cee..25a8fe672f5 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -239,6 +239,23 @@ static void store_reg(DisasContext *s, int reg, TCGv_i32 var)
     tcg_temp_free_i32(var);
 }
 
+/*
+ * Variant of store_reg which applies v8M stack-limit checks before updating
+ * SP. If the check fails this will result in an exception being taken.
+ * We disable the stack checks for CONFIG_USER_ONLY because we have
+ * no idea what the stack limits should be in that case.
+ * If stack checking is not being done this just acts like store_reg().
+ */
+static void store_sp_checked(DisasContext *s, TCGv_i32 var)
+{
+#ifndef CONFIG_USER_ONLY
+    if (s->v8m_stackcheck) {
+        gen_helper_v8m_stackcheck(cpu_env, var);
+    }
+#endif
+    store_reg(s, 13, var);
+}
+
 /* Value extensions.  */
 #define gen_uxtb(var) tcg_gen_ext8u_i32(var, var)
 #define gen_uxth(var) tcg_gen_ext16u_i32(var, var)
@@ -10583,7 +10600,13 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
             if (gen_thumb2_data_op(s, op, conds, 0, tmp, tmp2))
                 goto illegal_op;
             tcg_temp_free_i32(tmp2);
-            if (rd != 15) {
+            if (rd == 13 &&
+                ((op == 2 && rn == 15) ||
+                 (op == 8 && rn == 13) ||
+                 (op == 13 && rn == 13))) {
+                /* MOV SP, ... or ADD SP, SP, ... or SUB SP, SP, ... */
+                store_sp_checked(s, tmp);
+            } else if (rd != 15) {
                 store_reg(s, rd, tmp);
             } else {
                 tcg_temp_free_i32(tmp);
@@ -11267,8 +11290,15 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
                 gen_jmp(s, s->pc + offset);
             }
         } else {
-            /* Data processing immediate.  */
+            /*
+             * 0b1111_0xxx_xxxx_0xxx_xxxx_xxxx
+             *  - Data-processing (modified immediate, plain binary immediate)
+             */
             if (insn & (1 << 25)) {
+                /*
+                 * 0b1111_0x1x_xxxx_0xxx_xxxx_xxxx
+                 *  - Data-processing (plain binary immediate)
+                 */
                 if (insn & (1 << 24)) {
                     if (insn & (1 << 20))
                         goto illegal_op;
@@ -11364,6 +11394,7 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
                             tmp = tcg_temp_new_i32();
                             tcg_gen_movi_i32(tmp, imm);
                         }
+                        store_reg(s, rd, tmp);
                     } else {
                         /* Add/sub 12-bit immediate.  */
                         if (rn == 15) {
@@ -11374,17 +11405,27 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
                                 offset += imm;
                             tmp = tcg_temp_new_i32();
                             tcg_gen_movi_i32(tmp, offset);
+                            store_reg(s, rd, tmp);
                         } else {
                             tmp = load_reg(s, rn);
                             if (insn & (1 << 23))
                                 tcg_gen_subi_i32(tmp, tmp, imm);
                             else
                                 tcg_gen_addi_i32(tmp, tmp, imm);
+                            if (rn == 13 && rd == 13) {
+                                /* ADD SP, SP, imm or SUB SP, SP, imm */
+                                store_sp_checked(s, tmp);
+                            } else {
+                                store_reg(s, rd, tmp);
+                            }
                         }
                     }
-                    store_reg(s, rd, tmp);
                 }
             } else {
+                /*
+                 * 0b1111_0x0x_xxxx_0xxx_xxxx_xxxx
+                 *  - Data-processing (modified immediate)
+                 */
                 int shifter_out = 0;
                 /* modified 12-bit immediate.  */
                 shift = ((insn & 0x04000000) >> 23) | ((insn & 0x7000) >> 12);
@@ -11426,7 +11467,11 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
                     goto illegal_op;
                 tcg_temp_free_i32(tmp2);
                 rd = (insn >> 8) & 0xf;
-                if (rd != 15) {
+                if (rd == 13 && rn == 13
+                    && (op == 8 || op == 13)) {
+                    /* ADD(S) SP, SP, imm or SUB(S) SP, SP, imm */
+                    store_sp_checked(s, tmp);
+                } else if (rd != 15) {
                     store_reg(s, rd, tmp);
                 } else {
                     tcg_temp_free_i32(tmp);
@@ -11732,7 +11777,12 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
                 tmp2 = load_reg(s, rm);
                 tcg_gen_add_i32(tmp, tmp, tmp2);
                 tcg_temp_free_i32(tmp2);
-                store_reg(s, rd, tmp);
+                if (rd == 13) {
+                    /* ADD SP, SP, reg */
+                    store_sp_checked(s, tmp);
+                } else {
+                    store_reg(s, rd, tmp);
+                }
                 break;
             case 1: /* cmp */
                 tmp = load_reg(s, rd);
@@ -11743,7 +11793,12 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
                 break;
             case 2: /* mov/cpy */
                 tmp = load_reg(s, rm);
-                store_reg(s, rd, tmp);
+                if (rd == 13) {
+                    /* MOV SP, reg */
+                    store_sp_checked(s, tmp);
+                } else {
+                    store_reg(s, rd, tmp);
+                }
                 break;
             case 3:
             {
@@ -12071,7 +12126,10 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
         break;
 
     case 10:
-        /* add to high reg */
+        /*
+         * 0b1010_xxxx_xxxx_xxxx
+         *  - Add PC/SP (immediate)
+         */
         rd = (insn >> 8) & 7;
         if (insn & (1 << 11)) {
             /* SP */
@@ -12091,13 +12149,17 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
         op = (insn >> 8) & 0xf;
         switch (op) {
         case 0:
-            /* adjust stack pointer */
+            /*
+             * 0b1011_0000_xxxx_xxxx
+             *  - ADD (SP plus immediate)
+             *  - SUB (SP minus immediate)
+             */
             tmp = load_reg(s, 13);
             val = (insn & 0x7f) * 4;
             if (insn & (1 << 7))
                 val = -(int32_t)val;
             tcg_gen_addi_i32(tmp, tmp, val);
-            store_reg(s, 13, tmp);
+            store_sp_checked(s, tmp);
             break;
 
         case 2: /* sign/zero extend.  */
-- 
2.19.0

Re: [Qemu-devel] [PATCH 04/13] target/arm: Add v8M stack checks on ADD/SUB/MOV of SP

[Richard Henderson]

On 10/2/18 11:35 AM, Peter Maydell wrote:
> Add code to insert calls to a helper function to do the stack
> limit checking when we handle these forms of instruction
> that write to SP:
>  * ADD (SP plus immediate)
>  * ADD (SP plus register)
>  * SUB (SP minus immediate)
>  * SUB (SP minus register)
>  * MOV (register)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.h    |  2 ++
>  target/arm/internals.h | 14 ++++++++
>  target/arm/op_helper.c | 19 ++++++++++
>  target/arm/translate.c | 80 +++++++++++++++++++++++++++++++++++++-----
>  4 files changed, 106 insertions(+), 9 deletions(-)

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

r~

[Qemu-devel] [PATCH 05/13] target/arm: Add some comments in Thumb decode

[Peter Maydell]

Add some comments to the Thumb decoder indicating what bits
of the instruction have been decoded at various points in
the code.

This is not an exhaustive set of comments; we're gradually
adding comments as we work with particular bits of the code.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Specifically, I figured these out as I was going through looking
for the insns which write SP. These comments turn out not to
be relevant to those instructions, but I don't want to throw
them away.
---
 target/arm/translate.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/target/arm/translate.c b/target/arm/translate.c
index 25a8fe672f5..fcb33b8a503 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -10623,6 +10623,10 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
             tmp2 = load_reg(s, rm);
             if ((insn & 0x70) != 0)
                 goto illegal_op;
+            /*
+             * 0b1111_1010_0xxx_xxxx_1111_xxxx_0000_xxxx:
+             *  - MOV, MOVS (register-shifted register), flagsetting
+             */
             op = (insn >> 21) & 3;
             logic_cc = (insn & (1 << 20)) != 0;
             gen_arm_shift_reg(tmp, op, tmp2, logic_cc);
@@ -11674,7 +11678,11 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
         rd = insn & 7;
         op = (insn >> 11) & 3;
         if (op == 3) {
-            /* add/subtract */
+            /*
+             * 0b0001_1xxx_xxxx_xxxx
+             *  - Add, subtract (three low registers)
+             *  - Add, subtract (two low registers and immediate)
+             */
             rn = (insn >> 3) & 7;
             tmp = load_reg(s, rn);
             if (insn & (1 << 10)) {
@@ -11711,7 +11719,10 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
         }
         break;
     case 2: case 3:
-        /* arithmetic large immediate */
+        /*
+         * 0b001x_xxxx_xxxx_xxxx
+         *  - Add, subtract, compare, move (one low register and immediate)
+         */
         op = (insn >> 11) & 3;
         rd = (insn >> 8) & 0x7;
         if (op == 0) { /* mov */
@@ -11848,7 +11859,10 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
             break;
         }
 
-        /* data processing register */
+        /*
+         * 0b0100_00xx_xxxx_xxxx
+         *  - Data-processing (two low registers)
+         */
         rd = insn & 7;
         rm = (insn >> 3) & 7;
         op = (insn >> 6) & 0xf;
-- 
2.19.0

Re: [Qemu-devel] [PATCH 05/13] target/arm: Add some comments in Thumb decode

[Philippe Mathieu-Daudé]

On 02/10/2018 18:35, Peter Maydell wrote:
> Add some comments to the Thumb decoder indicating what bits
> of the instruction have been decoded at various points in
> the code.
> 
> This is not an exhaustive set of comments; we're gradually
> adding comments as we work with particular bits of the code.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Specifically, I figured these out as I was going through looking
> for the insns which write SP. These comments turn out not to
> be relevant to those instructions, but I don't want to throw
> them away.
> ---
>  target/arm/translate.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 25a8fe672f5..fcb33b8a503 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -10623,6 +10623,10 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
>              tmp2 = load_reg(s, rm);
>              if ((insn & 0x70) != 0)
>                  goto illegal_op;
> +            /*
> +             * 0b1111_1010_0xxx_xxxx_1111_xxxx_0000_xxxx:
> +             *  - MOV, MOVS (register-shifted register), flagsetting

T2, OK

> +             */
>              op = (insn >> 21) & 3;
>              logic_cc = (insn & (1 << 20)) != 0;
>              gen_arm_shift_reg(tmp, op, tmp2, logic_cc);
> @@ -11674,7 +11678,11 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
>          rd = insn & 7;
>          op = (insn >> 11) & 3;
>          if (op == 3) {
> -            /* add/subtract */
> +            /*
> +             * 0b0001_1xxx_xxxx_xxxx
> +             *  - Add, subtract (three low registers)
> +             *  - Add, subtract (two low registers and immediate)

T1, OK

> +             */
>              rn = (insn >> 3) & 7;
>              tmp = load_reg(s, rn);
>              if (insn & (1 << 10)) {
> @@ -11711,7 +11719,10 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
>          }
>          break;
>      case 2: case 3:
> -        /* arithmetic large immediate */
> +        /*
> +         * 0b001x_xxxx_xxxx_xxxx
> +         *  - Add, subtract, compare, move (one low register and immediate)

T2, OK

> +         */
>          op = (insn >> 11) & 3;
>          rd = (insn >> 8) & 0x7;
>          if (op == 0) { /* mov */
> @@ -11848,7 +11859,10 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
>              break;
>          }
>  
> -        /* data processing register */
> +        /*
> +         * 0b0100_00xx_xxxx_xxxx
> +         *  - Data-processing (two low registers)

T1, OK

> +         */
>          rd = insn & 7;
>          rm = (insn >> 3) & 7;
>          op = (insn >> 6) & 0xf;
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [Qemu-devel] [PATCH 05/13] target/arm: Add some comments in Thumb decode

[Richard Henderson]

On 10/2/18 11:35 AM, Peter Maydell wrote:
> Add some comments to the Thumb decoder indicating what bits
> of the instruction have been decoded at various points in
> the code.
> 
> This is not an exhaustive set of comments; we're gradually
> adding comments as we work with particular bits of the code.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Specifically, I figured these out as I was going through looking
> for the insns which write SP. These comments turn out not to
> be relevant to those instructions, but I don't want to throw
> them away.
> ---
>  target/arm/translate.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)

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

r~

[Qemu-devel] [PATCH 06/13] target/arm: Add v8M stack checks on exception entry

[Peter Maydell]

Add checks for breaches of the v8M stack limit when the
stack pointer is decremented to push the exception frame
for exception entry.

Note that the exception-entry case is unique in that the
stack pointer is updated to be the limit value if the limit
is hit (per rule R_ZLZG).

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

diff --git a/target/arm/helper.c b/target/arm/helper.c
index ef8c244fb84..a10dff01a90 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6839,6 +6839,8 @@ static bool v7m_push_callee_stack(ARMCPU *cpu, uint32_t lr, bool dotailchain,
     uint32_t frameptr;
     ARMMMUIdx mmu_idx;
     bool stacked_ok;
+    uint32_t limit;
+    bool want_psp;
 
     if (dotailchain) {
         bool mode = lr & R_V7M_EXCRET_MODE_MASK;
@@ -6848,12 +6850,34 @@ static bool v7m_push_callee_stack(ARMCPU *cpu, uint32_t lr, bool dotailchain,
         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);
+        want_psp = mode && (lr & R_V7M_EXCRET_SPSEL_MASK);
+        if (want_psp) {
+            limit = env->v7m.psplim[M_REG_S];
+        } else {
+            limit = env->v7m.msplim[M_REG_S];
+        }
     } else {
         mmu_idx = core_to_arm_mmu_idx(env, cpu_mmu_index(env, false));
         frame_sp_p = &env->regs[13];
+        limit = v7m_sp_limit(env);
     }
 
     frameptr = *frame_sp_p - 0x28;
+    if (frameptr < limit) {
+        /*
+         * Stack limit failure: set SP to the limit value, and generate
+         * STKOF UsageFault. Stack pushes below the limit must not be
+         * performed. It is IMPDEF whether pushes above the limit are
+         * performed; we choose not to.
+         */
+        qemu_log_mask(CPU_LOG_INT,
+                      "...STKOF during callee-saves register stacking\n");
+        env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_STKOF_MASK;
+        armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE,
+                                env->v7m.secure);
+        *frame_sp_p = limit;
+        return true;
+    }
 
     /* Write as much of the stack frame as we can. A write failure may
      * cause us to pend a derived exception.
@@ -6877,10 +6901,7 @@ static bool v7m_push_callee_stack(ARMCPU *cpu, uint32_t lr, bool dotailchain,
         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.
-     */
+    /* Update SP regardless of whether any of the stack accesses failed. */
     *frame_sp_p = frameptr;
 
     return !stacked_ok;
@@ -7028,6 +7049,26 @@ static bool v7m_push_stack(ARMCPU *cpu)
 
     frameptr -= 0x20;
 
+    if (arm_feature(env, ARM_FEATURE_V8)) {
+        uint32_t limit = v7m_sp_limit(env);
+
+        if (frameptr < limit) {
+            /*
+             * Stack limit failure: set SP to the limit value, and generate
+             * STKOF UsageFault. Stack pushes below the limit must not be
+             * performed. It is IMPDEF whether pushes above the limit are
+             * performed; we choose not to.
+             */
+            qemu_log_mask(CPU_LOG_INT,
+                          "...STKOF during stacking\n");
+            env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_STKOF_MASK;
+            armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE,
+                                    env->v7m.secure);
+            env->regs[13] = limit;
+            return true;
+        }
+    }
+
     /* 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
@@ -7043,10 +7084,7 @@ static bool v7m_push_stack(ARMCPU *cpu)
         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.
-     */
+    /* Update SP regardless of whether any of the stack accesses failed. */
     env->regs[13] = frameptr;
 
     return !stacked_ok;
-- 
2.19.0

Re: [Qemu-devel] [PATCH 06/13] target/arm: Add v8M stack checks on exception entry

[Philippe Mathieu-Daudé]

On 02/10/2018 18:35, Peter Maydell wrote:
> Add checks for breaches of the v8M stack limit when the
> stack pointer is decremented to push the exception frame
> for exception entry.
> 
> Note that the exception-entry case is unique in that the
> stack pointer is updated to be the limit value if the limit
> is hit (per rule R_ZLZG).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  target/arm/helper.c | 54 ++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 46 insertions(+), 8 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index ef8c244fb84..a10dff01a90 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6839,6 +6839,8 @@ static bool v7m_push_callee_stack(ARMCPU *cpu, uint32_t lr, bool dotailchain,
>      uint32_t frameptr;
>      ARMMMUIdx mmu_idx;
>      bool stacked_ok;
> +    uint32_t limit;
> +    bool want_psp;
>  
>      if (dotailchain) {
>          bool mode = lr & R_V7M_EXCRET_MODE_MASK;
> @@ -6848,12 +6850,34 @@ static bool v7m_push_callee_stack(ARMCPU *cpu, uint32_t lr, bool dotailchain,
>          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);
> +        want_psp = mode && (lr & R_V7M_EXCRET_SPSEL_MASK);
> +        if (want_psp) {
> +            limit = env->v7m.psplim[M_REG_S];
> +        } else {
> +            limit = env->v7m.msplim[M_REG_S];
> +        }
>      } else {
>          mmu_idx = core_to_arm_mmu_idx(env, cpu_mmu_index(env, false));
>          frame_sp_p = &env->regs[13];
> +        limit = v7m_sp_limit(env);
>      }
>  
>      frameptr = *frame_sp_p - 0x28;
> +    if (frameptr < limit) {
> +        /*
> +         * Stack limit failure: set SP to the limit value, and generate
> +         * STKOF UsageFault. Stack pushes below the limit must not be
> +         * performed. It is IMPDEF whether pushes above the limit are
> +         * performed; we choose not to.
> +         */
> +        qemu_log_mask(CPU_LOG_INT,
> +                      "...STKOF during callee-saves register stacking\n");
> +        env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_STKOF_MASK;
> +        armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE,
> +                                env->v7m.secure);
> +        *frame_sp_p = limit;
> +        return true;
> +    }
>  
>      /* Write as much of the stack frame as we can. A write failure may
>       * cause us to pend a derived exception.
> @@ -6877,10 +6901,7 @@ static bool v7m_push_callee_stack(ARMCPU *cpu, uint32_t lr, bool dotailchain,
>          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.
> -     */
> +    /* Update SP regardless of whether any of the stack accesses failed. */
>      *frame_sp_p = frameptr;
>  
>      return !stacked_ok;
> @@ -7028,6 +7049,26 @@ static bool v7m_push_stack(ARMCPU *cpu)
>  
>      frameptr -= 0x20;
>  
> +    if (arm_feature(env, ARM_FEATURE_V8)) {
> +        uint32_t limit = v7m_sp_limit(env);
> +
> +        if (frameptr < limit) {
> +            /*
> +             * Stack limit failure: set SP to the limit value, and generate
> +             * STKOF UsageFault. Stack pushes below the limit must not be
> +             * performed. It is IMPDEF whether pushes above the limit are
> +             * performed; we choose not to.
> +             */
> +            qemu_log_mask(CPU_LOG_INT,
> +                          "...STKOF during stacking\n");
> +            env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_STKOF_MASK;
> +            armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE,
> +                                    env->v7m.secure);
> +            env->regs[13] = limit;
> +            return true;
> +        }
> +    }
> +
>      /* 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
> @@ -7043,10 +7084,7 @@ static bool v7m_push_stack(ARMCPU *cpu)
>          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.
> -     */
> +    /* Update SP regardless of whether any of the stack accesses failed. */
>      env->regs[13] = frameptr;
>  
>      return !stacked_ok;
> 

Re: [Qemu-devel] [PATCH 06/13] target/arm: Add v8M stack checks on exception entry

[Richard Henderson]

On 10/2/18 11:35 AM, Peter Maydell wrote:
> Add checks for breaches of the v8M stack limit when the
> stack pointer is decremented to push the exception frame
> for exception entry.
> 
> Note that the exception-entry case is unique in that the
> stack pointer is updated to be the limit value if the limit
> is hit (per rule R_ZLZG).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 54 ++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 46 insertions(+), 8 deletions(-)

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

r~

[Qemu-devel] [PATCH 07/13] target/arm: Add v8M stack limit checks on NS function calls

[Peter Maydell]

Check the v8M stack limits when pushing the frame for a
non-secure function call via BLXNS.

In order to be able to generate the exception we need to
promote raise_exception() from being local to op_helper.c
so we can call it from helper.c.

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

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 966a8131623..aa124a06a9d 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -94,6 +94,15 @@ FIELD(V7M_EXCRET, RES1, 7, 25) /* including the must-be-1 prefix */
 #define M_FAKE_FSR_NSC_EXEC 0xf /* NS executing in S&NSC memory */
 #define M_FAKE_FSR_SFAULT 0xe /* SecureFault INVTRAN, INVEP or AUVIOL */
 
+/**
+ * raise_exception: Raise the specified exception.
+ * Raise a guest exception with the specified value, syndrome register
+ * and target exception level. This should be called from helper functions,
+ * and never returns because we will longjump back up to the CPU main loop.
+ */
+void QEMU_NORETURN raise_exception(CPUARMState *env, uint32_t excp,
+                                   uint32_t syndrome, uint32_t target_el);
+
 /*
  * For AArch64, map a given EL to an index in the banked_spsr array.
  * Note that this mapping and the AArch32 mapping defined in bank_number()
diff --git a/target/arm/helper.c b/target/arm/helper.c
index a10dff01a90..074f7616272 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6710,6 +6710,10 @@ void HELPER(v7m_blxns)(CPUARMState *env, uint32_t dest)
                       "BLXNS with misaligned SP is UNPREDICTABLE\n");
     }
 
+    if (sp < v7m_sp_limit(env)) {
+        raise_exception(env, EXCP_STKOF, 0, 1);
+    }
+
     saved_psr = env->v7m.exception;
     if (env->v7m.control[M_REG_S] & R_V7M_CONTROL_SFPA_MASK) {
         saved_psr |= XPSR_SFPA;
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index 38f885b290f..de0d3984ea4 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -28,8 +28,8 @@
 #define SIGNBIT (uint32_t)0x80000000
 #define SIGNBIT64 ((uint64_t)1 << 63)
 
-static void raise_exception(CPUARMState *env, uint32_t excp,
-                            uint32_t syndrome, uint32_t target_el)
+void raise_exception(CPUARMState *env, uint32_t excp,
+                     uint32_t syndrome, uint32_t target_el)
 {
     CPUState *cs = CPU(arm_env_get_cpu(env));
 
-- 
2.19.0

Re: [Qemu-devel] [PATCH 07/13] target/arm: Add v8M stack limit checks on NS function calls

[Philippe Mathieu-Daudé]

On 02/10/2018 18:35, Peter Maydell wrote:
> Check the v8M stack limits when pushing the frame for a
> non-secure function call via BLXNS.
> 
> In order to be able to generate the exception we need to
> promote raise_exception() from being local to op_helper.c
> so we can call it from helper.c.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  target/arm/internals.h | 9 +++++++++
>  target/arm/helper.c    | 4 ++++
>  target/arm/op_helper.c | 4 ++--
>  3 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 966a8131623..aa124a06a9d 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -94,6 +94,15 @@ FIELD(V7M_EXCRET, RES1, 7, 25) /* including the must-be-1 prefix */
>  #define M_FAKE_FSR_NSC_EXEC 0xf /* NS executing in S&NSC memory */
>  #define M_FAKE_FSR_SFAULT 0xe /* SecureFault INVTRAN, INVEP or AUVIOL */
>  
> +/**
> + * raise_exception: Raise the specified exception.
> + * Raise a guest exception with the specified value, syndrome register
> + * and target exception level. This should be called from helper functions,
> + * and never returns because we will longjump back up to the CPU main loop.
> + */
> +void QEMU_NORETURN raise_exception(CPUARMState *env, uint32_t excp,
> +                                   uint32_t syndrome, uint32_t target_el);
> +
>  /*
>   * For AArch64, map a given EL to an index in the banked_spsr array.
>   * Note that this mapping and the AArch32 mapping defined in bank_number()
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index a10dff01a90..074f7616272 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6710,6 +6710,10 @@ void HELPER(v7m_blxns)(CPUARMState *env, uint32_t dest)
>                        "BLXNS with misaligned SP is UNPREDICTABLE\n");
>      }
>  
> +    if (sp < v7m_sp_limit(env)) {
> +        raise_exception(env, EXCP_STKOF, 0, 1);
> +    }
> +
>      saved_psr = env->v7m.exception;
>      if (env->v7m.control[M_REG_S] & R_V7M_CONTROL_SFPA_MASK) {
>          saved_psr |= XPSR_SFPA;
> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> index 38f885b290f..de0d3984ea4 100644
> --- a/target/arm/op_helper.c
> +++ b/target/arm/op_helper.c
> @@ -28,8 +28,8 @@
>  #define SIGNBIT (uint32_t)0x80000000
>  #define SIGNBIT64 ((uint64_t)1 << 63)
>  
> -static void raise_exception(CPUARMState *env, uint32_t excp,
> -                            uint32_t syndrome, uint32_t target_el)
> +void raise_exception(CPUARMState *env, uint32_t excp,
> +                     uint32_t syndrome, uint32_t target_el)
>  {
>      CPUState *cs = CPU(arm_env_get_cpu(env));
>  
> 

Re: [Qemu-devel] [PATCH 07/13] target/arm: Add v8M stack limit checks on NS function calls

[Richard Henderson]

On 10/2/18 11:35 AM, Peter Maydell wrote:
> Check the v8M stack limits when pushing the frame for a
> non-secure function call via BLXNS.
> 
> In order to be able to generate the exception we need to
> promote raise_exception() from being local to op_helper.c
> so we can call it from helper.c.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/internals.h | 9 +++++++++
>  target/arm/helper.c    | 4 ++++
>  target/arm/op_helper.c | 4 ++--
>  3 files changed, 15 insertions(+), 2 deletions(-)

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

r~

[Qemu-devel] [PATCH 08/13] target/arm: Add v8M stack checks for LDRD/STRD (imm)

[Peter Maydell]

Add the v8M stack checks for:
 * LDRD (immediate)
 * STRD (immediate)

Loads and stores are more complicated than ADD/SUB/MOV, because we
must ensure that memory accesses below the stack limit are not
performed, so we can't simply do the check when we actually update
SP.

For these instructions, if the stack limit check triggers
we must not:
 * perform any memory access below the SP limit
 * update PC, SP or the load/store base register
but it is IMPDEF whether we:
 * perform any accesses above or equal to the SP limit
 * update destination registers for loads

For QEMU we choose to always check the limit before doing any other
part of the load or store, so we won't update any registers or
perform any memory accesses.

It is UNKNOWN whether the limit check triggers for a load or store
where the initial SP value is below the limit and one of the stores
would be below the limit, but the writeback moves SP to above the
limit.  For QEMU we choose to trigger the check in this situation.

Note that limit checks happen only for loads and stores which update
SP via writeback; they do not happen for loads and stores which
simply use SP as a base register.

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

diff --git a/target/arm/translate.c b/target/arm/translate.c
index fcb33b8a503..c16d6075d94 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -10278,6 +10278,8 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
                  * 0b1111_1001_x11x_xxxx_xxxx_xxxx_xxxx_xxxx
                  *  - load/store dual (pre-indexed)
                  */
+                bool wback = extract32(insn, 21, 1);
+
                 if (rn == 15) {
                     if (insn & (1 << 21)) {
                         /* UNPREDICTABLE */
@@ -10289,8 +10291,29 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
                     addr = load_reg(s, rn);
                 }
                 offset = (insn & 0xff) * 4;
-                if ((insn & (1 << 23)) == 0)
+                if ((insn & (1 << 23)) == 0) {
                     offset = -offset;
+                }
+
+                if (s->v8m_stackcheck && rn == 13 && wback) {
+                    /*
+                     * Here 'addr' is the current SP; if offset is +ve we're
+                     * moving SP up, else down. It is UNKNOWN whether the limit
+                     * check triggers when SP starts below the limit and ends
+                     * up above it; check whichever of the current and final
+                     * SP is lower, so QEMU will trigger in that situation.
+                     */
+                    if ((int32_t)offset < 0) {
+                        TCGv_i32 newsp = tcg_temp_new_i32();
+
+                        tcg_gen_addi_i32(newsp, addr, offset);
+                        gen_helper_v8m_stackcheck(cpu_env, newsp);
+                        tcg_temp_free_i32(newsp);
+                    } else {
+                        gen_helper_v8m_stackcheck(cpu_env, addr);
+                    }
+                }
+
                 if (insn & (1 << 24)) {
                     tcg_gen_addi_i32(addr, addr, offset);
                     offset = 0;
@@ -10314,7 +10337,7 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
                     gen_aa32_st32(s, tmp, addr, get_mem_index(s));
                     tcg_temp_free_i32(tmp);
                 }
-                if (insn & (1 << 21)) {
+                if (wback) {
                     /* Base writeback.  */
                     tcg_gen_addi_i32(addr, addr, offset - 4);
                     store_reg(s, rn, addr);
-- 
2.19.0

Re: [Qemu-devel] [PATCH 08/13] target/arm: Add v8M stack checks for LDRD/STRD (imm)

[Philippe Mathieu-Daudé]

On 02/10/2018 18:35, Peter Maydell wrote:
> Add the v8M stack checks for:
>  * LDRD (immediate)
>  * STRD (immediate)
> 
> Loads and stores are more complicated than ADD/SUB/MOV, because we
> must ensure that memory accesses below the stack limit are not
> performed, so we can't simply do the check when we actually update
> SP.
> 
> For these instructions, if the stack limit check triggers
> we must not:
>  * perform any memory access below the SP limit
>  * update PC, SP or the load/store base register
> but it is IMPDEF whether we:
>  * perform any accesses above or equal to the SP limit
>  * update destination registers for loads
> 
> For QEMU we choose to always check the limit before doing any other
> part of the load or store, so we won't update any registers or
> perform any memory accesses.
> 
> It is UNKNOWN whether the limit check triggers for a load or store
> where the initial SP value is below the limit and one of the stores
> would be below the limit, but the writeback moves SP to above the
> limit.  For QEMU we choose to trigger the check in this situation.
> 
> Note that limit checks happen only for loads and stores which update
> SP via writeback; they do not happen for loads and stores which
> simply use SP as a base register.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  target/arm/translate.c | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index fcb33b8a503..c16d6075d94 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -10278,6 +10278,8 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
>                   * 0b1111_1001_x11x_xxxx_xxxx_xxxx_xxxx_xxxx
>                   *  - load/store dual (pre-indexed)
>                   */
> +                bool wback = extract32(insn, 21, 1);
> +
>                  if (rn == 15) {
>                      if (insn & (1 << 21)) {
>                          /* UNPREDICTABLE */
> @@ -10289,8 +10291,29 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
>                      addr = load_reg(s, rn);
>                  }
>                  offset = (insn & 0xff) * 4;
> -                if ((insn & (1 << 23)) == 0)
> +                if ((insn & (1 << 23)) == 0) {
>                      offset = -offset;
> +                }
> +
> +                if (s->v8m_stackcheck && rn == 13 && wback) {
> +                    /*
> +                     * Here 'addr' is the current SP; if offset is +ve we're
> +                     * moving SP up, else down. It is UNKNOWN whether the limit
> +                     * check triggers when SP starts below the limit and ends
> +                     * up above it; check whichever of the current and final
> +                     * SP is lower, so QEMU will trigger in that situation.
> +                     */
> +                    if ((int32_t)offset < 0) {
> +                        TCGv_i32 newsp = tcg_temp_new_i32();
> +
> +                        tcg_gen_addi_i32(newsp, addr, offset);
> +                        gen_helper_v8m_stackcheck(cpu_env, newsp);
> +                        tcg_temp_free_i32(newsp);
> +                    } else {
> +                        gen_helper_v8m_stackcheck(cpu_env, addr);
> +                    }
> +                }
> +
>                  if (insn & (1 << 24)) {
>                      tcg_gen_addi_i32(addr, addr, offset);
>                      offset = 0;
> @@ -10314,7 +10337,7 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
>                      gen_aa32_st32(s, tmp, addr, get_mem_index(s));
>                      tcg_temp_free_i32(tmp);
>                  }
> -                if (insn & (1 << 21)) {
> +                if (wback) {
>                      /* Base writeback.  */
>                      tcg_gen_addi_i32(addr, addr, offset - 4);
>                      store_reg(s, rn, addr);
> 

Re: [Qemu-devel] [PATCH 08/13] target/arm: Add v8M stack checks for LDRD/STRD (imm)

[Richard Henderson]

On 10/2/18 11:35 AM, Peter Maydell wrote:
> Add the v8M stack checks for:
>  * LDRD (immediate)
>  * STRD (immediate)
> 
> Loads and stores are more complicated than ADD/SUB/MOV, because we
> must ensure that memory accesses below the stack limit are not
> performed, so we can't simply do the check when we actually update
> SP.
> 
> For these instructions, if the stack limit check triggers
> we must not:
>  * perform any memory access below the SP limit
>  * update PC, SP or the load/store base register
> but it is IMPDEF whether we:
>  * perform any accesses above or equal to the SP limit
>  * update destination registers for loads
> 
> For QEMU we choose to always check the limit before doing any other
> part of the load or store, so we won't update any registers or
> perform any memory accesses.
> 
> It is UNKNOWN whether the limit check triggers for a load or store
> where the initial SP value is below the limit and one of the stores
> would be below the limit, but the writeback moves SP to above the
> limit.  For QEMU we choose to trigger the check in this situation.
> 
> Note that limit checks happen only for loads and stores which update
> SP via writeback; they do not happen for loads and stores which
> simply use SP as a base register.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/translate.c | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)

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

r~

[Qemu-devel] [PATCH 09/13] target/arm: Add v8M stack checks for Thumb2 LDM/STM

[Peter Maydell]

Add the v8M stack checks for:
 * LDM (T2 encoding)
 * STM (T2 encoding)

This includes the 32-bit encodings of the instructions listed
in v8M ARM ARM rule R_YVWT as
 * LDM, LDMIA, LDMFD
 * LDMDB, LDMEA
 * POP (multiple registers)
 * PUSH (muliple registers)
 * STM, STMIA, STMEA
 * STMDB, STMFD

We perform the stack limit before doing any other part
of the load or store.

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

diff --git a/target/arm/translate.c b/target/arm/translate.c
index c16d6075d94..3fb378a492d 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -10524,6 +10524,7 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
             } else {
                 int i, loaded_base = 0;
                 TCGv_i32 loaded_var;
+                bool wback = extract32(insn, 21, 1);
                 /* Load/store multiple.  */
                 addr = load_reg(s, rn);
                 offset = 0;
@@ -10531,10 +10532,26 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
                     if (insn & (1 << i))
                         offset += 4;
                 }
+
                 if (insn & (1 << 24)) {
                     tcg_gen_addi_i32(addr, addr, -offset);
                 }
 
+                if (s->v8m_stackcheck && rn == 13 && wback) {
+                    /*
+                     * If the writeback is incrementing SP rather than
+                     * decrementing it, and the initial SP is below the
+                     * stack limit but the final written-back SP would
+                     * be above, then then we must not perform any memory
+                     * accesses, but it is IMPDEF whether we generate
+                     * an exception. We choose to do so in this case.
+                     * At this point 'addr' is the lowest address, so
+                     * either the original SP (if incrementing) or our
+                     * final SP (if decrementing), so that's what we check.
+                     */
+                    gen_helper_v8m_stackcheck(cpu_env, addr);
+                }
+
                 loaded_var = NULL;
                 for (i = 0; i < 16; i++) {
                     if ((insn & (1 << i)) == 0)
@@ -10562,7 +10579,7 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
                 if (loaded_base) {
                     store_reg(s, rn, loaded_var);
                 }
-                if (insn & (1 << 21)) {
+                if (wback) {
                     /* Base register writeback.  */
                     if (insn & (1 << 24)) {
                         tcg_gen_addi_i32(addr, addr, -offset);
-- 
2.19.0

Re: [Qemu-devel] [PATCH 09/13] target/arm: Add v8M stack checks for Thumb2 LDM/STM

[Philippe Mathieu-Daudé]

On 02/10/2018 18:35, Peter Maydell wrote:
> Add the v8M stack checks for:
>  * LDM (T2 encoding)
>  * STM (T2 encoding)
> 
> This includes the 32-bit encodings of the instructions listed
> in v8M ARM ARM rule R_YVWT as
>  * LDM, LDMIA, LDMFD
>  * LDMDB, LDMEA
>  * POP (multiple registers)
>  * PUSH (muliple registers)
>  * STM, STMIA, STMEA
>  * STMDB, STMFD
> 
> We perform the stack limit before doing any other part
> of the load or store.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  target/arm/translate.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index c16d6075d94..3fb378a492d 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -10524,6 +10524,7 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
>              } else {
>                  int i, loaded_base = 0;
>                  TCGv_i32 loaded_var;
> +                bool wback = extract32(insn, 21, 1);
>                  /* Load/store multiple.  */
>                  addr = load_reg(s, rn);
>                  offset = 0;
> @@ -10531,10 +10532,26 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
>                      if (insn & (1 << i))
>                          offset += 4;
>                  }
> +
>                  if (insn & (1 << 24)) {
>                      tcg_gen_addi_i32(addr, addr, -offset);
>                  }
>  
> +                if (s->v8m_stackcheck && rn == 13 && wback) {
> +                    /*
> +                     * If the writeback is incrementing SP rather than
> +                     * decrementing it, and the initial SP is below the
> +                     * stack limit but the final written-back SP would
> +                     * be above, then then we must not perform any memory
> +                     * accesses, but it is IMPDEF whether we generate
> +                     * an exception. We choose to do so in this case.
> +                     * At this point 'addr' is the lowest address, so
> +                     * either the original SP (if incrementing) or our
> +                     * final SP (if decrementing), so that's what we check.
> +                     */
> +                    gen_helper_v8m_stackcheck(cpu_env, addr);
> +                }
> +
>                  loaded_var = NULL;
>                  for (i = 0; i < 16; i++) {
>                      if ((insn & (1 << i)) == 0)
> @@ -10562,7 +10579,7 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
>                  if (loaded_base) {
>                      store_reg(s, rn, loaded_var);
>                  }
> -                if (insn & (1 << 21)) {
> +                if (wback) {
>                      /* Base register writeback.  */
>                      if (insn & (1 << 24)) {
>                          tcg_gen_addi_i32(addr, addr, -offset);
> 

Re: [Qemu-devel] [PATCH 09/13] target/arm: Add v8M stack checks for Thumb2 LDM/STM

[Richard Henderson]

On 10/2/18 11:35 AM, Peter Maydell wrote:
> Add the v8M stack checks for:
>  * LDM (T2 encoding)
>  * STM (T2 encoding)
> 
> This includes the 32-bit encodings of the instructions listed
> in v8M ARM ARM rule R_YVWT as
>  * LDM, LDMIA, LDMFD
>  * LDMDB, LDMEA
>  * POP (multiple registers)
>  * PUSH (muliple registers)
>  * STM, STMIA, STMEA
>  * STMDB, STMFD
> 
> We perform the stack limit before doing any other part
> of the load or store.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/translate.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)

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

r~

[Qemu-devel] [PATCH 10/13] target/arm: Add v8M stack checks for T32 load/store single

[Peter Maydell]

Add v8M stack checks for the instructions in the T32
"load/store single" encoding class: these are the
"immediate pre-indexed" and "immediate, post-indexed"
LDR and STR instructions.

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

diff --git a/target/arm/translate.c b/target/arm/translate.c
index 3fb378a492d..65df8d6975c 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -11624,7 +11624,6 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
                     imm = -imm;
                     /* Fall through.  */
                 case 0xf: /* Pre-increment.  */
-                    tcg_gen_addi_i32(addr, addr, imm);
                     writeback = 1;
                     break;
                 default:
@@ -11636,6 +11635,28 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
 
         issinfo = writeback ? ISSInvalid : rs;
 
+        if (s->v8m_stackcheck && rn == 13 && writeback) {
+            /*
+             * Stackcheck. Here we know 'addr' is the current SP;
+             * if imm is +ve we're moving SP up, else down. It is
+             * UNKNOWN whether the limit check triggers when SP starts
+             * below the limit and ends up above it; we chose to do so.
+             */
+            if ((int32_t)imm < 0) {
+                TCGv_i32 newsp = tcg_temp_new_i32();
+
+                tcg_gen_addi_i32(newsp, addr, imm);
+                gen_helper_v8m_stackcheck(cpu_env, newsp);
+                tcg_temp_free_i32(newsp);
+            } else {
+                gen_helper_v8m_stackcheck(cpu_env, addr);
+            }
+        }
+
+        if (writeback && !postinc) {
+            tcg_gen_addi_i32(addr, addr, imm);
+        }
+
         if (insn & (1 << 20)) {
             /* Load.  */
             tmp = tcg_temp_new_i32();
-- 
2.19.0

Re: [Qemu-devel] [PATCH 10/13] target/arm: Add v8M stack checks for T32 load/store single

[Philippe Mathieu-Daudé]

On 02/10/2018 18:35, Peter Maydell wrote:
> Add v8M stack checks for the instructions in the T32
> "load/store single" encoding class: these are the
> "immediate pre-indexed" and "immediate, post-indexed"
> LDR and STR instructions.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/translate.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 3fb378a492d..65df8d6975c 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -11624,7 +11624,6 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
>                      imm = -imm;
>                      /* Fall through.  */
>                  case 0xf: /* Pre-increment.  */
> -                    tcg_gen_addi_i32(addr, addr, imm);
>                      writeback = 1;
>                      break;
>                  default:
> @@ -11636,6 +11635,28 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
>  
>          issinfo = writeback ? ISSInvalid : rs;
>  
> +        if (s->v8m_stackcheck && rn == 13 && writeback) {
> +            /*
> +             * Stackcheck. Here we know 'addr' is the current SP;
> +             * if imm is +ve we're moving SP up, else down. It is

TIL +ve is a short form for positive, and -ve for negative.

> +             * UNKNOWN whether the limit check triggers when SP starts
> +             * below the limit and ends up above it; we chose to do so.
> +             */
> +            if ((int32_t)imm < 0) {
> +                TCGv_i32 newsp = tcg_temp_new_i32();
> +
> +                tcg_gen_addi_i32(newsp, addr, imm);
> +                gen_helper_v8m_stackcheck(cpu_env, newsp);
> +                tcg_temp_free_i32(newsp);
> +            } else {
> +                gen_helper_v8m_stackcheck(cpu_env, addr);
> +            }
> +        }
> +
> +        if (writeback && !postinc) {
> +            tcg_gen_addi_i32(addr, addr, imm);
> +        }
> +
>          if (insn & (1 << 20)) {
>              /* Load.  */
>              tmp = tcg_temp_new_i32();
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [Qemu-devel] [PATCH 10/13] target/arm: Add v8M stack checks for T32 load/store single

[Richard Henderson]

On 10/2/18 11:35 AM, Peter Maydell wrote:
> Add v8M stack checks for the instructions in the T32
> "load/store single" encoding class: these are the
> "immediate pre-indexed" and "immediate, post-indexed"
> LDR and STR instructions.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/translate.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)

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

r~

[Qemu-devel] [PATCH 11/13] target/arm: Add v8M stack checks for Thumb push/pop

[Peter Maydell]

Add v8M stack checks for the 16-bit Thumb push/pop
encodings: STMDB, STMFD, LDM, LDMIA, LDMFD.

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

diff --git a/target/arm/translate.c b/target/arm/translate.c
index 65df8d6975c..ef64d2559de 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -12251,7 +12251,10 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
             store_reg(s, rd, tmp);
             break;
         case 4: case 5: case 0xc: case 0xd:
-            /* push/pop */
+            /*
+             * 0b1011_x10x_xxxx_xxxx
+             *  - push/pop
+             */
             addr = load_reg(s, 13);
             if (insn & (1 << 8))
                 offset = 4;
@@ -12264,6 +12267,17 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
             if ((insn & (1 << 11)) == 0) {
                 tcg_gen_addi_i32(addr, addr, -offset);
             }
+
+            if (s->v8m_stackcheck) {
+                /*
+                 * Here 'addr' is the lower of "old SP" and "new SP";
+                 * if this is a pop that starts below the limit and ends
+                 * above it, it is UNKNOWN whether the limit check triggers;
+                 * we choose to trigger.
+                 */
+                gen_helper_v8m_stackcheck(cpu_env, addr);
+            }
+
             for (i = 0; i < 8; i++) {
                 if (insn & (1 << i)) {
                     if (insn & (1 << 11)) {
-- 
2.19.0

Re: [Qemu-devel] [PATCH 11/13] target/arm: Add v8M stack checks for Thumb push/pop

[Philippe Mathieu-Daudé]

On 02/10/2018 18:35, Peter Maydell wrote:
> Add v8M stack checks for the 16-bit Thumb push/pop
> encodings: STMDB, STMFD, LDM, LDMIA, LDMFD.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  target/arm/translate.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 65df8d6975c..ef64d2559de 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -12251,7 +12251,10 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
>              store_reg(s, rd, tmp);
>              break;
>          case 4: case 5: case 0xc: case 0xd:
> -            /* push/pop */
> +            /*
> +             * 0b1011_x10x_xxxx_xxxx
> +             *  - push/pop
> +             */
>              addr = load_reg(s, 13);
>              if (insn & (1 << 8))
>                  offset = 4;
> @@ -12264,6 +12267,17 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
>              if ((insn & (1 << 11)) == 0) {
>                  tcg_gen_addi_i32(addr, addr, -offset);
>              }
> +
> +            if (s->v8m_stackcheck) {
> +                /*
> +                 * Here 'addr' is the lower of "old SP" and "new SP";
> +                 * if this is a pop that starts below the limit and ends
> +                 * above it, it is UNKNOWN whether the limit check triggers;
> +                 * we choose to trigger.
> +                 */
> +                gen_helper_v8m_stackcheck(cpu_env, addr);
> +            }
> +
>              for (i = 0; i < 8; i++) {
>                  if (insn & (1 << i)) {
>                      if (insn & (1 << 11)) {
> 

Re: [Qemu-devel] [PATCH 11/13] target/arm: Add v8M stack checks for Thumb push/pop

[Richard Henderson]

On 10/2/18 11:35 AM, Peter Maydell wrote:
> Add v8M stack checks for the 16-bit Thumb push/pop
> encodings: STMDB, STMFD, LDM, LDMIA, LDMFD.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/translate.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)

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

r~

[Qemu-devel] [PATCH 12/13] target/arm: Add v8M stack checks for VLDM/VSTM

[Peter Maydell]

Add the v8M stack checks for the VLDM/VSTM
(aka VPUSH/VPOP) instructions. This code is currently
unreachable because we haven't yet implemented M profile
floating point support, but since the change is simple,
we add it now because otherwise we're likely to forget to
do it later.

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

diff --git a/target/arm/translate.c b/target/arm/translate.c
index ef64d2559de..2d3a1be518b 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -4229,6 +4229,18 @@ static int disas_vfp_insn(DisasContext *s, uint32_t insn)
                 if (insn & (1 << 24)) /* pre-decrement */
                     tcg_gen_addi_i32(addr, addr, -((insn & 0xff) << 2));
 
+                if (rn == 13 && w && s->v8m_stackcheck) {
+                    /*
+                     * Here 'addr' is the lowest address we will store to,
+                     * and is either the old SP (if post-increment) or
+                     * the new SP (if pre-decrement). For post-increment
+                     * where the old value is below the limit and the new
+                     * value is above, it is UNKNOWN whether the limit check
+                     * triggers; we choose to trigger.
+                     */
+                    gen_helper_v8m_stackcheck(cpu_env, addr);
+                }
+
                 if (dp)
                     offset = 8;
                 else
-- 
2.19.0

Re: [Qemu-devel] [PATCH 12/13] target/arm: Add v8M stack checks for VLDM/VSTM

[Philippe Mathieu-Daudé]

On 02/10/2018 18:35, Peter Maydell wrote:
> Add the v8M stack checks for the VLDM/VSTM
> (aka VPUSH/VPOP) instructions. This code is currently
> unreachable because we haven't yet implemented M profile
> floating point support, but since the change is simple,
> we add it now because otherwise we're likely to forget to
> do it later.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/translate.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index ef64d2559de..2d3a1be518b 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -4229,6 +4229,18 @@ static int disas_vfp_insn(DisasContext *s, uint32_t insn)
>                  if (insn & (1 << 24)) /* pre-decrement */
>                      tcg_gen_addi_i32(addr, addr, -((insn & 0xff) << 2));
>  
> +                if (rn == 13 && w && s->v8m_stackcheck) {

Following previous patches style:

                   if (s->v8m_stackcheck && rn == 13 && w) {

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> +                    /*
> +                     * Here 'addr' is the lowest address we will store to,
> +                     * and is either the old SP (if post-increment) or
> +                     * the new SP (if pre-decrement). For post-increment
> +                     * where the old value is below the limit and the new
> +                     * value is above, it is UNKNOWN whether the limit check
> +                     * triggers; we choose to trigger.
> +                     */
> +                    gen_helper_v8m_stackcheck(cpu_env, addr);
> +                }
> +
>                  if (dp)
>                      offset = 8;
>                  else
> 

Re: [Qemu-devel] [PATCH 12/13] target/arm: Add v8M stack checks for VLDM/VSTM

[Richard Henderson]

On 10/2/18 11:35 AM, Peter Maydell wrote:
> Add the v8M stack checks for the VLDM/VSTM
> (aka VPUSH/VPOP) instructions. This code is currently
> unreachable because we haven't yet implemented M profile
> floating point support, but since the change is simple,
> we add it now because otherwise we're likely to forget to
> do it later.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/translate.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)

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

r~

Re: [Qemu-devel] [PATCH 12/13] target/arm: Add v8M stack checks for VLDM/VSTM

[Richard Henderson]

On 10/2/18 11:35 AM, Peter Maydell wrote:
> Add the v8M stack checks for the VLDM/VSTM
> (aka VPUSH/VPOP) instructions. This code is currently
> unreachable because we haven't yet implemented M profile
> floating point support, but since the change is simple,
> we add it now because otherwise we're likely to forget to
> do it later.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/translate.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)

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

r~

[Qemu-devel] [PATCH 13/13] target/arm: Add v8M stack checks for MSR to SP_NS

[Peter Maydell]

Updating the NS stack pointer via MSR to SP_NS should include
a check whether the new SP value is below the stack limit.
No other kinds of update to the various stack pointer and
limit registers via MSR should perform a check.

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

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 074f7616272..712828674fa 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -10963,11 +10963,23 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val)
              * currently in handler mode or not, using the NS CONTROL.SPSEL.
              */
             bool spsel = env->v7m.control[M_REG_NS] & R_V7M_CONTROL_SPSEL_MASK;
+            bool is_psp = !arm_v7m_is_handler_mode(env) && spsel;
+            uint32_t limit;
 
             if (!env->v7m.secure) {
                 return;
             }
-            if (!arm_v7m_is_handler_mode(env) && spsel) {
+
+            limit = is_psp ? env->v7m.psplim[false] : env->v7m.msplim[false];
+
+            if (val < limit) {
+                CPUState *cs = CPU(arm_env_get_cpu(env));
+
+                cpu_restore_state(cs, GETPC(), true);
+                raise_exception(env, EXCP_STKOF, 0, 1);
+            }
+
+            if (is_psp) {
                 env->v7m.other_ss_psp = val;
             } else {
                 env->v7m.other_ss_msp = val;
-- 
2.19.0

Re: [Qemu-devel] [PATCH 13/13] target/arm: Add v8M stack checks for MSR to SP_NS

[Philippe Mathieu-Daudé]

On 02/10/2018 18:35, Peter Maydell wrote:
> Updating the NS stack pointer via MSR to SP_NS should include
> a check whether the new SP value is below the stack limit.
> No other kinds of update to the various stack pointer and
> limit registers via MSR should perform a check.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  target/arm/helper.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 074f7616272..712828674fa 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -10963,11 +10963,23 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val)
>               * currently in handler mode or not, using the NS CONTROL.SPSEL.
>               */
>              bool spsel = env->v7m.control[M_REG_NS] & R_V7M_CONTROL_SPSEL_MASK;
> +            bool is_psp = !arm_v7m_is_handler_mode(env) && spsel;
> +            uint32_t limit;
>  
>              if (!env->v7m.secure) {
>                  return;
>              }
> -            if (!arm_v7m_is_handler_mode(env) && spsel) {
> +
> +            limit = is_psp ? env->v7m.psplim[false] : env->v7m.msplim[false];
> +
> +            if (val < limit) {
> +                CPUState *cs = CPU(arm_env_get_cpu(env));
> +
> +                cpu_restore_state(cs, GETPC(), true);
> +                raise_exception(env, EXCP_STKOF, 0, 1);
> +            }
> +
> +            if (is_psp) {
>                  env->v7m.other_ss_psp = val;
>              } else {
>                  env->v7m.other_ss_msp = val;
> 

Re: [Qemu-devel] [PATCH 13/13] target/arm: Add v8M stack checks for MSR to SP_NS

[Richard Henderson]

On 10/2/18 11:35 AM, Peter Maydell wrote:
> Updating the NS stack pointer via MSR to SP_NS should include
> a check whether the new SP value is below the stack limit.
> No other kinds of update to the various stack pointer and
> limit registers via MSR should perform a check.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)

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

r~