diff mbox series

[18/20] target/arm: Implement BLXNS

Message ID 1506092407-26985-19-git-send-email-peter.maydell@linaro.org
State Superseded
Headers show
Series ARM v8M: exception entry, exit and security | expand

Commit Message

Peter Maydell Sept. 22, 2017, 3 p.m. UTC 
Implement the BLXNS instruction, which allows secure code to
call non-secure code.

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

---
 target/arm/helper.h    |  1 +
 target/arm/internals.h |  1 +
 target/arm/helper.c    | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++
 target/arm/translate.c | 17 +++++++++++++--
 4 files changed, 76 insertions(+), 2 deletions(-)

-- 
2.7.4

Comments

Philippe Mathieu-Daudé Oct. 5, 2017, 1:07 p.m. UTC | #1 
On 09/22/2017 12:00 PM, Peter Maydell wrote:
> Implement the BLXNS instruction, which allows secure code to

> call non-secure code.

> 

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


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


> ---

>  target/arm/helper.h    |  1 +

>  target/arm/internals.h |  1 +

>  target/arm/helper.c    | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++

>  target/arm/translate.c | 17 +++++++++++++--

>  4 files changed, 76 insertions(+), 2 deletions(-)

> 

> diff --git a/target/arm/helper.h b/target/arm/helper.h

> index 64afbac..2cf6f74 100644

> --- a/target/arm/helper.h

> +++ b/target/arm/helper.h

> @@ -64,6 +64,7 @@ DEF_HELPER_3(v7m_msr, void, env, i32, i32)

>  DEF_HELPER_2(v7m_mrs, i32, env, i32)

>  

>  DEF_HELPER_2(v7m_bxns, void, env, i32)

> +DEF_HELPER_2(v7m_blxns, 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)

> diff --git a/target/arm/internals.h b/target/arm/internals.h

> index fd9a7e8..1746737 100644

> --- a/target/arm/internals.h

> +++ b/target/arm/internals.h

> @@ -60,6 +60,7 @@ static inline bool excp_is_internal(int excp)

>  FIELD(V7M_CONTROL, NPRIV, 0, 1)

>  FIELD(V7M_CONTROL, SPSEL, 1, 1)

>  FIELD(V7M_CONTROL, FPCA, 2, 1)

> +FIELD(V7M_CONTROL, SFPA, 3, 1)

>  

>  /* Bit definitions for v7M exception return payload */

>  FIELD(V7M_EXCRET, ES, 0, 1)

> diff --git a/target/arm/helper.c b/target/arm/helper.c

> index 8df819d..30dc2a9 100644

> --- a/target/arm/helper.c

> +++ b/target/arm/helper.c

> @@ -5890,6 +5890,12 @@ void HELPER(v7m_bxns)(CPUARMState *env, uint32_t dest)

>      g_assert_not_reached();

>  }

>  

> +void HELPER(v7m_blxns)(CPUARMState *env, uint32_t dest)

> +{

> +    /* translate.c should never generate calls here in user-only mode */

> +    g_assert_not_reached();

> +}

> +

>  void switch_mode(CPUARMState *env, int mode)

>  {

>      ARMCPU *cpu = arm_env_get_cpu(env);

> @@ -6182,6 +6188,59 @@ void HELPER(v7m_bxns)(CPUARMState *env, uint32_t dest)

>      env->regs[15] = dest & ~1;

>  }

>  

> +void HELPER(v7m_blxns)(CPUARMState *env, uint32_t dest)

> +{

> +    /* Handle v7M BLXNS:

> +     *  - bit 0 of the destination address is the target security state

> +     */

> +

> +    /* At this point regs[15] is the address just after the BLXNS */

> +    uint32_t nextinst = env->regs[15] | 1;

> +    uint32_t sp = env->regs[13] - 8;

> +    uint32_t saved_psr;

> +

> +    /* translate.c will have made BLXNS UNDEF unless we're secure */

> +    assert(env->v7m.secure);

> +

> +    if (dest & 1) {

> +        /* target is Secure, so this is just a normal BLX,

> +         * except that the low bit doesn't indicate Thumb/not.

> +         */

> +        env->regs[14] = nextinst;

> +        env->thumb = 1;

> +        env->regs[15] = dest & ~1;

> +        return;

> +    }

> +

> +    /* Target is non-secure: first push a stack frame */

> +    if (!QEMU_IS_ALIGNED(sp, 8)) {

> +        qemu_log_mask(LOG_GUEST_ERROR,

> +                      "BLXNS with misaligned SP is UNPREDICTABLE\n");

> +    }

> +

> +    saved_psr = env->v7m.exception;

> +    if (env->v7m.control[M_REG_S] & R_V7M_CONTROL_SFPA_MASK) {

> +        saved_psr |= XPSR_SFPA;

> +    }

> +

> +    /* Note that these stores can throw exceptions on MPU faults */

> +    cpu_stl_data(env, sp, nextinst);

> +    cpu_stl_data(env, sp + 4, saved_psr);

> +

> +    env->regs[13] = sp;

> +    env->regs[14] = 0xfeffffff;

> +    if (arm_v7m_is_handler_mode(env)) {

> +        /* Write a dummy value to IPSR, to avoid leaking the current secure

> +         * exception number to non-secure code. This is guaranteed not

> +         * to cause write_v7m_exception() to actually change stacks.

> +         */

> +        write_v7m_exception(env, 1);

> +    }

> +    switch_v7m_security_state(env, dest & 1);

> +    env->thumb = 1;

> +    env->regs[15] = dest & ~1;

> +}

> +

>  static uint32_t *get_v7m_sp_ptr(CPUARMState *env, bool secure, bool threadmode,

>                                  bool spsel)

>  {

> diff --git a/target/arm/translate.c b/target/arm/translate.c

> index ab1a12a..53694bb 100644

> --- a/target/arm/translate.c

> +++ b/target/arm/translate.c

> @@ -1013,6 +1013,20 @@ static inline void gen_bxns(DisasContext *s, int rm)

>      s->base.is_jmp = DISAS_EXIT;

>  }

>  

> +static inline void gen_blxns(DisasContext *s, int rm)

> +{

> +    TCGv_i32 var = load_reg(s, rm);

> +

> +    /* We don't need to sync condexec state, for the same reason as blxns.

> +     * We do however need to set the PC, because the blxns helper reads it.

> +     * The blxns helper may throw an exception.

> +     */

> +    gen_set_pc_im(s, s->pc);

> +    gen_helper_v7m_blxns(cpu_env, var);

> +    tcg_temp_free_i32(var);

> +    s->base.is_jmp = DISAS_EXIT;

> +}

> +

>  /* Variant of store_reg which uses branch&exchange logic when storing

>     to r15 in ARM architecture v7 and above. The source must be a temporary

>     and will be marked as dead. */

> @@ -11221,8 +11235,7 @@ static void disas_thumb_insn(CPUARMState *env, DisasContext *s)

>                          goto undef;

>                      }

>                      if (link) {

> -                        /* BLXNS: not yet implemented */

> -                        goto undef;

> +                        gen_blxns(s, rm);

>                      } else {

>                          gen_bxns(s, rm);

>                      }

>
Richard Henderson Oct. 5, 2017, 6:56 p.m. UTC | #2 
On 09/22/2017 11:00 AM, Peter Maydell wrote:
> +void HELPER(v7m_blxns)(CPUARMState *env, uint32_t dest)

> +{

...
> +    if (dest & 1) {

> +        /* target is Secure, so this is just a normal BLX,

> +         * except that the low bit doesn't indicate Thumb/not.

> +         */

> +        env->regs[14] = nextinst;

> +        env->thumb = 1;

> +        env->regs[15] = dest & ~1;

> +        return;

> +    }

...
> +    switch_v7m_security_state(env, dest & 1);

> +    env->thumb = 1;

> +    env->regs[15] = dest & ~1;


dest & 1 is known to be 0.

> +static inline void gen_blxns(DisasContext *s, int rm)

> +{

> +    TCGv_i32 var = load_reg(s, rm);

> +

> +    /* We don't need to sync condexec state, for the same reason as blxns.


s/blxns/bxns/ ?

Otherwise,

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



r~
Peter Maydell Oct. 5, 2017, 7:40 p.m. UTC | #3 
On 5 October 2017 at 19:56, Richard Henderson
<richard.henderson@linaro.org> wrote:
> On 09/22/2017 11:00 AM, Peter Maydell wrote:

>> +void HELPER(v7m_blxns)(CPUARMState *env, uint32_t dest)

>> +{

> ...

>> +    if (dest & 1) {

>> +        /* target is Secure, so this is just a normal BLX,

>> +         * except that the low bit doesn't indicate Thumb/not.

>> +         */

>> +        env->regs[14] = nextinst;

>> +        env->thumb = 1;

>> +        env->regs[15] = dest & ~1;

>> +        return;

>> +    }

> ...

>> +    switch_v7m_security_state(env, dest & 1);

>> +    env->thumb = 1;

>> +    env->regs[15] = dest & ~1;

>

> dest & 1 is known to be 0.


Yes. I liked the symmetry with the tail end of the v7m_bxns helper,
which is conceptually doing the same thing, and assumed the
compiler would be smart enough not to generate unnecessary code.

>> +static inline void gen_blxns(DisasContext *s, int rm)

>> +{

>> +    TCGv_i32 var = load_reg(s, rm);

>> +

>> +    /* We don't need to sync condexec state, for the same reason as blxns.

>

> s/blxns/bxns/ ?


Yes.

thanks
-- PMM
diff mbox series

Patch

diff --git a/target/arm/helper.h b/target/arm/helper.h
index 64afbac..2cf6f74 100644
--- a/target/arm/helper.h
+++ b/target/arm/helper.h
@@ -64,6 +64,7 @@  DEF_HELPER_3(v7m_msr, void, env, i32, i32)
 DEF_HELPER_2(v7m_mrs, i32, env, i32)
 
 DEF_HELPER_2(v7m_bxns, void, env, i32)
+DEF_HELPER_2(v7m_blxns, 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)
diff --git a/target/arm/internals.h b/target/arm/internals.h
index fd9a7e8..1746737 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -60,6 +60,7 @@  static inline bool excp_is_internal(int excp)
 FIELD(V7M_CONTROL, NPRIV, 0, 1)
 FIELD(V7M_CONTROL, SPSEL, 1, 1)
 FIELD(V7M_CONTROL, FPCA, 2, 1)
+FIELD(V7M_CONTROL, SFPA, 3, 1)
 
 /* Bit definitions for v7M exception return payload */
 FIELD(V7M_EXCRET, ES, 0, 1)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 8df819d..30dc2a9 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5890,6 +5890,12 @@  void HELPER(v7m_bxns)(CPUARMState *env, uint32_t dest)
     g_assert_not_reached();
 }
 
+void HELPER(v7m_blxns)(CPUARMState *env, uint32_t dest)
+{
+    /* translate.c should never generate calls here in user-only mode */
+    g_assert_not_reached();
+}
+
 void switch_mode(CPUARMState *env, int mode)
 {
     ARMCPU *cpu = arm_env_get_cpu(env);
@@ -6182,6 +6188,59 @@  void HELPER(v7m_bxns)(CPUARMState *env, uint32_t dest)
     env->regs[15] = dest & ~1;
 }
 
+void HELPER(v7m_blxns)(CPUARMState *env, uint32_t dest)
+{
+    /* Handle v7M BLXNS:
+     *  - bit 0 of the destination address is the target security state
+     */
+
+    /* At this point regs[15] is the address just after the BLXNS */
+    uint32_t nextinst = env->regs[15] | 1;
+    uint32_t sp = env->regs[13] - 8;
+    uint32_t saved_psr;
+
+    /* translate.c will have made BLXNS UNDEF unless we're secure */
+    assert(env->v7m.secure);
+
+    if (dest & 1) {
+        /* target is Secure, so this is just a normal BLX,
+         * except that the low bit doesn't indicate Thumb/not.
+         */
+        env->regs[14] = nextinst;
+        env->thumb = 1;
+        env->regs[15] = dest & ~1;
+        return;
+    }
+
+    /* Target is non-secure: first push a stack frame */
+    if (!QEMU_IS_ALIGNED(sp, 8)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "BLXNS with misaligned SP is UNPREDICTABLE\n");
+    }
+
+    saved_psr = env->v7m.exception;
+    if (env->v7m.control[M_REG_S] & R_V7M_CONTROL_SFPA_MASK) {
+        saved_psr |= XPSR_SFPA;
+    }
+
+    /* Note that these stores can throw exceptions on MPU faults */
+    cpu_stl_data(env, sp, nextinst);
+    cpu_stl_data(env, sp + 4, saved_psr);
+
+    env->regs[13] = sp;
+    env->regs[14] = 0xfeffffff;
+    if (arm_v7m_is_handler_mode(env)) {
+        /* Write a dummy value to IPSR, to avoid leaking the current secure
+         * exception number to non-secure code. This is guaranteed not
+         * to cause write_v7m_exception() to actually change stacks.
+         */
+        write_v7m_exception(env, 1);
+    }
+    switch_v7m_security_state(env, dest & 1);
+    env->thumb = 1;
+    env->regs[15] = dest & ~1;
+}
+
 static uint32_t *get_v7m_sp_ptr(CPUARMState *env, bool secure, bool threadmode,
                                 bool spsel)
 {
diff --git a/target/arm/translate.c b/target/arm/translate.c
index ab1a12a..53694bb 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -1013,6 +1013,20 @@  static inline void gen_bxns(DisasContext *s, int rm)
     s->base.is_jmp = DISAS_EXIT;
 }
 
+static inline void gen_blxns(DisasContext *s, int rm)
+{
+    TCGv_i32 var = load_reg(s, rm);
+
+    /* We don't need to sync condexec state, for the same reason as blxns.
+     * We do however need to set the PC, because the blxns helper reads it.
+     * The blxns helper may throw an exception.
+     */
+    gen_set_pc_im(s, s->pc);
+    gen_helper_v7m_blxns(cpu_env, var);
+    tcg_temp_free_i32(var);
+    s->base.is_jmp = DISAS_EXIT;
+}
+
 /* Variant of store_reg which uses branch&exchange logic when storing
    to r15 in ARM architecture v7 and above. The source must be a temporary
    and will be marked as dead. */
@@ -11221,8 +11235,7 @@  static void disas_thumb_insn(CPUARMState *env, DisasContext *s)
                         goto undef;
                     }
                     if (link) {
-                        /* BLXNS: not yet implemented */
-                        goto undef;
+                        gen_blxns(s, rm);
                     } else {
                         gen_bxns(s, rm);
                     }