Re: [v5][PATCH 1/6] powerpc/book3e: load critical/machine/debugexception stack

From: Scott Wood
Date: Fri Oct 18 2013 - 18:37:22 EST


On Thu, 2013-06-20 at 18:28 +0800, Tiejun Chen wrote:
> We always alloc critical/machine/debug check exceptions. This is
> different from the normal exception. So we should load these exception
> stack properly like we did for booke.

This is "booke". Do you mean like "like we did for 32-bit"?

And the code is already trying to load the special stack; it just
happens that it's loading from a different location than the C code
placed the stack addresses. The changelog should point out the specific
thing that is being fixed.

> Signed-off-by: Tiejun Chen <tiejun.chen@xxxxxxxxxxxxx>
> ---
> arch/powerpc/kernel/exceptions-64e.S | 49 +++++++++++++++++++++++++++++++---
> 1 file changed, 46 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
> index 4b23119..4d8e57f 100644
> --- a/arch/powerpc/kernel/exceptions-64e.S
> +++ b/arch/powerpc/kernel/exceptions-64e.S
> @@ -36,6 +36,37 @@
> */
> #define SPECIAL_EXC_FRAME_SIZE INT_FRAME_SIZE
>
> +/* only on book3e */
> +#define DBG_STACK_BASE dbgirq_ctx
> +#define MC_STACK_BASE mcheckirq_ctx
> +#define CRIT_STACK_BASE critirq_ctx
> +
> +#ifdef CONFIG_RELOCATABLE
> +#define LOAD_STACK_BASE(reg, level) \
> + tovirt(r2,r2); \
> + LOAD_REG_ADDR(reg, level##_STACK_BASE);
> +#else
> +#define LOAD_STACK_BASE(reg, level) \
> + LOAD_REG_IMMEDIATE(reg, level##_STACK_BASE);
> +#endif
> +
> +#ifdef CONFIG_SMP
> +#define BOOK3E_LOAD_EXC_LEVEL_STACK(level) \
> + mfspr r14,SPRN_PIR; \
> + slwi r14,r14,3; \
> + LOAD_STACK_BASE(r10, level); \
> + add r10,r10,r14; \
> + ld r10,0(r10); \
> + addi r10,r10,THREAD_SIZE; \
> + std r10,PACA_##level##_STACK(r13);
> +#else
> +#define BOOK3E_LOAD_EXC_LEVEL_STACK(level) \
> + LOAD_STACK_BASE(r10, level); \
> + ld r10,0(r10); \
> + addi r10,r10,THREAD_SIZE; \
> + std r10,PACA_##level##_STACK(r13);
> +#endif

It looks like you're loading the stack from *irq_ctx, storing it in
PACA_*_stack, and then (immediately after this in the caller) loading it
back from PACA_*_STACK. Why not just load it from *irq_ctx and get rid
of PACA_*_STACK altogether -- or change the C code to initialize the
addresses in the PACA instead, and get ird of *irq_ctx on 64-bit?

> /* Exception prolog code for all exceptions */
> #define EXCEPTION_PROLOG(n, intnum, type, addition) \
> mtspr SPRN_SPRG_##type##_SCRATCH,r13; /* get spare registers */ \
> @@ -68,20 +99,32 @@
> #define SPRN_GDBELL_SRR1 SPRN_GSRR1
>
> #define CRIT_SET_KSTACK \
> + andi. r10,r11,MSR_PR; \
> + bne 1f; \
> + BOOK3E_LOAD_EXC_LEVEL_STACK(CRIT); \
> ld r1,PACA_CRIT_STACK(r13); \
> - subi r1,r1,SPECIAL_EXC_FRAME_SIZE;
> + subi r1,r1,SPECIAL_EXC_FRAME_SIZE; \

The caller will already check MSR_PR and override this if coming from
userspace; why do you need to check again here?

-Scott



--
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/