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

[Peter Maydell]

On 5 October 2017 at 17:04, Richard Henderson
<address@hidden> wrote:
> On 09/22/2017 10: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>
>> ---
>>  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.
>
> Exception return magic lr value does not appear to match "pushing".  Did you
> mean "poping" here?

No, because the code creates the exception magic LR value for an
exception entry, and then uses it to determine which SPSEL to use.
In the tailchained-exception case we use the magic LR that
the attempted exception-exit got when figuring out where we need
to push more registers as part of the tailchaining. The pseudocode
chooses to open-code the "find the right stack pointer" for that
codepath (in pseudocode function PushCalleeStack), whereas for this
QEMU code I opted to make the utility function more specific. That's
what this comment is trying to gesture at.

[The push-on-exception-entry code is in "target/arm: Add v8M support
to exception entry code"]

> Otherwise,
> Reviewed-by: Richard Henderson <address@hidden>

thanks
-- PMM