diff mbox

[05/10] armv7m: honour CCR.STACKALIGN on exception entry

Message ID 1485285380-10565-6-git-send-email-peter.maydell@linaro.org
State New
Headers show

Commit Message

Peter Maydell Jan. 24, 2017, 7:16 p.m. UTC 
From: Michael Davidsaver <mdavidsaver@gmail.com>

The CCR.STACKALIGN bit controls whether the CPU is supposed to force
8-alignment of the stack pointer on entry to the exception handler.

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
[PMM: commit message and comment tweaks]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Richard Henderson Jan. 24, 2017, 7:33 p.m. UTC | #1 
On 01/24/2017 11:16 AM, Peter Maydell wrote:
> The CCR.STACKALIGN bit controls whether the CPU is supposed to force
> 8-alignment of the stack pointer on entry to the exception handler.

8...

> +    /* Align stack pointer if the guest wants that */
> +    if ((env->regs[13] & 4) && (env->v7m.ccr & R_V7M_CCR_STKALIGN_MASK)) {

4...

>          env->regs[13] -= 4;

Not alignment.  "&= -4"?


r~
Peter Maydell Jan. 24, 2017, 7:45 p.m. UTC | #2 
On 24 January 2017 at 19:33, Richard Henderson <rth@twiddle.net> wrote:
> On 01/24/2017 11:16 AM, Peter Maydell wrote:
>> The CCR.STACKALIGN bit controls whether the CPU is supposed to force
>> 8-alignment of the stack pointer on entry to the exception handler.
>
> 8...
>
>> +    /* Align stack pointer if the guest wants that */
>> +    if ((env->regs[13] & 4) && (env->v7m.ccr & R_V7M_CCR_STKALIGN_MASK)) {
>
> 4...
>
>>          env->regs[13] -= 4;
>
> Not alignment.  "&= -4"?

We know SP is always at least a multiple of 4. If it's already
a multiple of 8 then (sp & 4) will be false and we leave sp alone.
Otherwise it's a multiple of 4 but not 8, and subtracting 4
makes it 8 aligned (and we set the saved-XPSR bit to indicate
that we need to undo that on exception exit).

You could maybe rephrase the code to look a bit closer to the
v7M ARM ARM pseudocode, but the way it's written now isn't wrong,
so since this patch is only trying to say "do this if STKALIGN
is set rather than all the time" just adjusting the if conditional
seemed the best thing to me.

(The pseudocode checks for "do we need to align" with
"SP<2> AND forcealign", and does the alignment by
ANDing with a mask constructed with "NOT(ZeroExtend(forcealign:'00',32))".
So we do the check the same way it does, but use a subtract
rather than an AND-NOT. (Since we know that bit 2 must be set
then subtracting 4 and masking that bit to 0 are the same thing.)

thanks
-- PMM
diff mbox

Patch

diff --git a/target/arm/helper.c b/target/arm/helper.c
index ce7e43b..7dc30f5 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6110,10 +6110,8 @@  void arm_v7m_cpu_do_interrupt(CPUState *cs)
         return; /* Never happens.  Keep compiler happy.  */
     }
 
-    /* Align stack pointer.  */
-    /* ??? Should only do this if Configuration Control Register
-       STACKALIGN bit is set.  */
-    if (env->regs[13] & 4) {
+    /* Align stack pointer if the guest wants that */
+    if ((env->regs[13] & 4) && (env->v7m.ccr & R_V7M_CCR_STKALIGN_MASK)) {
         env->regs[13] -= 4;
         xpsr |= 0x200;
     }