Re: [Qemu-arm] [Qemu-devel] [PATCH 02/20] target/arm: Don't switch to target stack early in v7M exception return

[Philippe Mathieu-Daudé]

On 09/22/2017 11:59 AM, Peter Maydell wrote:
> Currently our M profile exception return code switches to the
> target stack pointer relatively early in the process, before
> it tries to pop the exception frame off the stack. This is
> awkward for v8M for two reasons:
>  * in v8M the process vs main stack pointer is not selected
>    purely by the value of CONTROL.SPSEL, so updating SPSEL
>    and relying on that to switch to the right stack pointer
>    won't work
>  * the stack we should be reading the stack frame from and
>    the stack we will eventually switch to might not be the
>    same if the guest is doing strange things
> 
> Change our exception return code to use a 'frame pointer'
> to read the exception frame rather than assuming that we
> can switch the live stack pointer this early.
> 
> Signed-off-by: Peter Maydell <address@hidden>

Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

> ---
>  target/arm/helper.c | 127 
> +++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 95 insertions(+), 32 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 8be78ea..f13b99d 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6040,16 +6040,6 @@ static void v7m_push(CPUARMState *env, uint32_t val)
>      stl_phys(cs->as, env->regs[13], val);
>  }
>  
> -static uint32_t v7m_pop(CPUARMState *env)
> -{
> -    CPUState *cs = CPU(arm_env_get_cpu(env));
> -    uint32_t val;
> -
> -    val = ldl_phys(cs->as, env->regs[13]);
> -    env->regs[13] += 4;
> -    return val;
> -}
> -
>  /* Return true if we're using the process stack pointer (not the MSP) */
>  static bool v7m_using_psp(CPUARMState *env)
>  {
> @@ -6141,6 +6131,40 @@ void HELPER(v7m_bxns)(CPUARMState *env, uint32_t dest)
>      env->regs[15] = dest & ~1;
>  }
>  
> +static uint32_t *get_v7m_sp_ptr(CPUARMState *env, bool secure, bool 
> threadmode,
> +                                bool spsel)
> +{
> +    /* Return a pointer to the location where we currently store the
> +     * stack pointer for the requested security state and thread mode.
> +     * This pointer will become invalid if the CPU state is updated
> +     * such that the stack pointers are switched around (eg changing
> +     * the SPSEL control bit).
> +     * Compare the v8M ARM ARM pseudocode LookUpSP_with_security_mode().
> +     * Unlike that pseudocode, we require the caller to pass us in the
> +     * SPSEL control bit value; this is because we also use this
> +     * function in handling of pushing of the callee-saves registers
> +     * part of the v8M stack frame, and in that case the SPSEL bit
> +     * comes from the exception return magic LR value.
> +     */
> +    bool want_psp = threadmode && spsel;
> +
> +    if (secure == env->v7m.secure) {
> +        /* Currently switch_v7m_sp switches SP as it updates SPSEL,
> +         * so the SP we want is always in regs[13].
> +         * When we decouple SPSEL from the actually selected SP
> +         * we need to check want_psp against v7m_using_psp()
> +         * to see whether we need regs[13] or v7m.other_sp.
> +         */
> +        return &env->regs[13];
> +    } else {
> +        if (want_psp) {
> +            return &env->v7m.other_ss_psp;
> +        } else {
> +            return &env->v7m.other_ss_msp;
> +        }
> +    }
> +}
> +
>  static uint32_t arm_v7m_load_vector(ARMCPU *cpu)
>  {
>      CPUState *cs = CPU(cpu);
> @@ -6212,6 +6236,7 @@ static void v7m_push_stack(ARMCPU *cpu)
>  static void do_v7m_exception_exit(ARMCPU *cpu)
>  {
>      CPUARMState *env = &cpu->env;
> +    CPUState *cs = CPU(cpu);
>      uint32_t excret;
>      uint32_t xpsr;
>      bool ufault = false;
> @@ -6219,6 +6244,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
>      bool return_to_handler = false;
>      bool rettobase = false;
>      bool exc_secure = false;
> +    bool return_to_secure;
>  
>      /* We can only get here from an EXCP_EXCEPTION_EXIT, and
>       * gen_bx_excret() enforces the architectural rule
> @@ -6286,6 +6312,9 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
>          g_assert_not_reached();
>      }
>  
> +    return_to_secure = arm_feature(env, ARM_FEATURE_M_SECURITY) &&
> +        (excret & R_V7M_EXCRET_S_MASK);
> +
>      switch (excret & 0xf) {
>      case 1: /* Return to Handler */
>          return_to_handler = true;
> @@ -6315,32 +6344,66 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
>          return;
>      }
>  
> -    /* Switch to the target stack.  */
> +    /* Set CONTROL.SPSEL from excret.SPSEL. For QEMU this currently
> +     * causes us to switch the active SP, but we will change this
> +     * later to not do that so we can support v8M.
> +     */
>      switch_v7m_sp(env, return_to_sp_process);
> -    /* Pop registers.  */
> -    env->regs[0] = v7m_pop(env);
> -    env->regs[1] = v7m_pop(env);
> -    env->regs[2] = v7m_pop(env);
> -    env->regs[3] = v7m_pop(env);
> -    env->regs[12] = v7m_pop(env);
> -    env->regs[14] = v7m_pop(env);
> -    env->regs[15] = v7m_pop(env);
> -    if (env->regs[15] & 1) {
> -        qemu_log_mask(LOG_GUEST_ERROR,
> -                      "M profile return from interrupt with misaligned "
> -                      "PC is UNPREDICTABLE\n");
> -        /* Actual hardware seems to ignore the lsbit, and there are several
> -         * RTOSes out there which incorrectly assume the r15 in the stack
> -         * frame should be a Thumb-style "lsbit indicates ARM/Thumb" value.
> +
> +    {
> +        /* The stack pointer we should be reading the exception frame from
> +         * depends on bits in the magic exception return type value (and
> +         * for v8M isn't necessarily the stack pointer we will eventually
> +         * end up resuming execution with). Get a pointer to the location
> +         * in the CPU state struct where the SP we need is currently being
> +         * stored; we will use and modify it in place.
> +         * We use this limited C variable scope so we don't accidentally
> +         * use 'frame_sp_p' after we do something that makes it invalid.
> +         */
> +        uint32_t *frame_sp_p = get_v7m_sp_ptr(env,
> +                                              return_to_secure,
> +                                              !return_to_handler,
> +                                              return_to_sp_process);
> +        uint32_t frameptr = *frame_sp_p;
> +
> +        /* Pop registers. TODO: make these accesses use the correct
> +         * attributes and address space (S/NS, priv/unpriv) and handle
> +         * memory transaction failures.
>           */
> -        env->regs[15] &= ~1U;
> +        env->regs[0] = ldl_phys(cs->as, frameptr);
> +        env->regs[1] = ldl_phys(cs->as, frameptr + 0x4);
> +        env->regs[2] = ldl_phys(cs->as, frameptr + 0x8);
> +        env->regs[3] = ldl_phys(cs->as, frameptr + 0xc);
> +        env->regs[12] = ldl_phys(cs->as, frameptr + 0x10);
> +        env->regs[14] = ldl_phys(cs->as, frameptr + 0x14);
> +        env->regs[15] = ldl_phys(cs->as, frameptr + 0x18);
> +        if (env->regs[15] & 1) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "M profile return from interrupt with misaligned "
> +                          "PC is UNPREDICTABLE\n");
> +            /* Actual hardware seems to ignore the lsbit, and there are 
> several
> +             * RTOSes out there which incorrectly assume the r15 in the stack
> +             * frame should be a Thumb-style "lsbit indicates ARM/Thumb" 
> value.
> +             */
> +            env->regs[15] &= ~1U;
> +        }
> +        xpsr = ldl_phys(cs->as, frameptr + 0x1c);
> +
> +        /* Commit to consuming the stack frame */
> +        frameptr += 0x20;
> +        /* Undo stack alignment (the SPREALIGN bit indicates that the 
> original
> +         * pre-exception SP was not 8-aligned and we added a padding word to
> +         * align it, so we undo this by ORing in the bit that increases it
> +         * from the current 8-aligned value to the 8-unaligned value. 
> (Adding 4
> +         * would work too but a logical OR is how the pseudocode specifies 
> it.)
> +         */
> +        if (xpsr & XPSR_SPREALIGN) {
> +            frameptr |= 4;
> +        }
> +        *frame_sp_p = frameptr;
>      }
> -    xpsr = v7m_pop(env);
> +    /* This xpsr_write() will invalidate frame_sp_p as it may switch stack */
>      xpsr_write(env, xpsr, ~XPSR_SPREALIGN);
> -    /* Undo stack alignment.  */
> -    if (xpsr & XPSR_SPREALIGN) {
> -        env->regs[13] |= 4;
> -    }
>  
>      /* The restored xPSR exception field will be zero if we're
>       * resuming in Thread mode. If that doesn't match what the
>