Re: [PATCH 2/8] xtensa: keep exception/interrupt stack continuous

[Chris Zankel]

Hi Max,

Not such a big fan about these changes as they introduce additional
load and stores and a branch.

Copy spill area:
- is this only for debugging? Could the debugger identify the 'kernel
exception' frame and handle it appropriately?
- if we need it for debugging/perf, maybe move these line under a
kernel option (could be enabled by default)
- since we know the kernel stack frame size, why can't we just load
from that offset (instead of the l32i and add). Also, can't we use
s32e (need to look up the spec again).

Branch:
- is there any logical change in the code or are these only to avoid using a0?
- if for a0, couldn't we load a0 just before the 'xsr a3, ps' and keep
the code as before?

Thanks,
-Chris

On Mon, Jul 6, 2015 at 6:32 AM, Max Filippov <jcmvbkbc@xxxxxxxxx> wrote:
> Restore original a0 in the kernel exception stack frame. This way it
> looks like the frame that got interrupt/exception did alloca (copy a0 and
> a1 potentially spilled under old stack to the new location as well) to
> save registers and then did a call to handler. The point where
> interrupt/exception was taken is not in the stack chain, only in pt_regs
> (call4 from that address can be simulated to keep it in the stack trace).
>
> Signed-off-by: Max Filippov <jcmvbkbc@xxxxxxxxx>
> ---
>  arch/xtensa/kernel/entry.S | 60 +++++++++++++++++++++++++++-------------------
>  1 file changed, 35 insertions(+), 25 deletions(-)
>
> diff --git a/arch/xtensa/kernel/entry.S b/arch/xtensa/kernel/entry.S
> index 82bbfa5..ab6a857 100644
> --- a/arch/xtensa/kernel/entry.S
> +++ b/arch/xtensa/kernel/entry.S
> @@ -218,7 +218,7 @@ _user_exception:
>         /* We are back to the original stack pointer (a1) */
>
>  2:     /* Now, jump to the common exception handler. */
> -
> +       movi    a0, 0                   # terminate user stack trace with 0
>         j       common_exception
>
>  ENDPROC(user_exception)
> @@ -264,6 +264,19 @@ ENTRY(kernel_exception)
>         .globl _kernel_exception
>  _kernel_exception:
>
> +       /* Copy spill slots of a0 and a1 to imitate movsp
> +        * in order to keep exception stack continuous
> +        */
> +       l32i    a0, a1, PT_AREG1
> +       addi    a2, a1, -16
> +       addi    a0, a0, -16
> +       l32i    a3, a0, 0
> +       l32i    a0, a0, 4
> +       s32i    a3, a2, 0
> +       s32i    a0, a2, 4
> +
> +       l32i    a0, a1, PT_AREG0        # restore saved a0
> +
>         /* Save SAR and turn off single stepping */
>
>         movi    a2, 0
> @@ -338,52 +351,50 @@ common_exception:
>         xsr     a2, lcount
>         s32i    a2, a1, PT_LCOUNT
>
> -       /* It is now save to restore the EXC_TABLE_FIXUP variable. */
> +       /* It is now safe to restore the EXC_TABLE_FIXUP variable. */
>
> -       rsr     a0, exccause
>         movi    a3, 0
>         rsr     a2, excsave1
> -       s32i    a0, a1, PT_EXCCAUSE
>         s32i    a3, a2, EXC_TABLE_FIXUP
>
> -       /* All unrecoverable states are saved on stack, now, and a1 is valid,
> -        * so we can allow exceptions and interrupts (*) again.
> -        * Set PS(EXCM = 0, UM = 0, RING = 0, OWB = 0, WOE = 1, INTLEVEL = X)
> +       /* Save remaining unrecoverable states (exccause, ps) on stack.
> +        * Now we can allow exceptions again. In case we've got an interrupt
> +        * PS.INTLEVEL is set to LOCKLEVEL disabling furhter interrupts,
> +        * otherwise it's left unchanged.
>          *
> -        * (*) We only allow interrupts if they were previously enabled and
> -        *     we're not handling an IRQ
> +        * Set PS(EXCM = 0, UM = 0, RING = 0, OWB = 0, WOE = 1, INTLEVEL = X)
>          */
>
>         rsr     a3, ps
> -       addi    a0, a0, -EXCCAUSE_LEVEL1_INTERRUPT
> -       movi    a2, LOCKLEVEL
> +       movi    a2, (1 << PS_WOE_BIT)
>         extui   a3, a3, PS_INTLEVEL_SHIFT, PS_INTLEVEL_WIDTH
> -                                       # a3 = PS.INTLEVEL
> -       moveqz  a3, a2, a0              # a3 = LOCKLEVEL iff interrupt
> -       movi    a2, 1 << PS_WOE_BIT
>         or      a3, a3, a2
> -       rsr     a0, exccause
> -       xsr     a3, ps
>
> +       rsr     a2, exccause
> +       bnei    a2, EXCCAUSE_LEVEL1_INTERRUPT, 1f
> +       movi    a3, (1 << PS_WOE_BIT) | LOCKLEVEL
> +1:
> +       xsr     a3, ps
> +       s32i    a2, a1, PT_EXCCAUSE
>         s32i    a3, a1, PT_PS           # save ps
>
>         /* Save lbeg, lend */
>
> -       rsr     a2, lbeg
> +       rsr     a4, lbeg
>         rsr     a3, lend
> -       s32i    a2, a1, PT_LBEG
> +       s32i    a4, a1, PT_LBEG
>         s32i    a3, a1, PT_LEND
>
>         /* Save SCOMPARE1 */
>
>  #if XCHAL_HAVE_S32C1I
> -       rsr     a2, scompare1
> -       s32i    a2, a1, PT_SCOMPARE1
> +       rsr     a3, scompare1
> +       s32i    a3, a1, PT_SCOMPARE1
>  #endif
>
>         /* Save optional registers. */
>
> -       save_xtregs_opt a1 a2 a4 a5 a6 a7 PT_XTREGS_OPT
> +       save_xtregs_opt a1 a3 a4 a5 a6 a7 PT_XTREGS_OPT
>
>  #ifdef CONFIG_TRACE_IRQFLAGS
>         l32i    a4, a1, PT_DEPC
> @@ -391,8 +402,7 @@ common_exception:
>          * while PS.EXCM was set, i.e. interrupts disabled.
>          */
>         bgeui   a4, VALID_DOUBLE_EXCEPTION_ADDRESS, 1f
> -       l32i    a4, a1, PT_EXCCAUSE
> -       bnei    a4, EXCCAUSE_LEVEL1_INTERRUPT, 1f
> +       bnei    a2, EXCCAUSE_LEVEL1_INTERRUPT, 1f
>         /* We came here with an interrupt means interrupts were enabled
>          * and we've just disabled them.
>          */
> @@ -407,8 +417,8 @@ common_exception:
>
>         rsr     a4, excsave1
>         mov     a6, a1                  # pass stack frame
> -       mov     a7, a0                  # pass EXCCAUSE
> -       addx4   a4, a0, a4
> +       mov     a7, a2                  # pass EXCCAUSE
> +       addx4   a4, a2, a4
>         l32i    a4, a4, EXC_TABLE_DEFAULT               # load handler
>
>         /* Call the second-level handler */
> --
> 1.8.1.4
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/