Patchwork [v5,1/6] powerpc/book3e: load critical/machine/debug exception stack

login
register
mail settings
Submitter Tiejun Chen
Date June 20, 2013, 10:28 a.m.
Message ID <1371724110-8250-2-git-send-email-tiejun.chen@windriver.com>
Download mbox | patch
Permalink /patch/252859/
State Changes Requested
Headers show

Comments

Tiejun Chen - June 20, 2013, 10:28 a.m.
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.

Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
---
 arch/powerpc/kernel/exceptions-64e.S |   49 +++++++++++++++++++++++++++++++---
 1 file changed, 46 insertions(+), 3 deletions(-)
Scott Wood - Oct. 18, 2013, 10:37 p.m.
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@windriver.com>
> ---
>  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
Scott Wood - Oct. 18, 2013, 11:55 p.m.
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.
> 
> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
> ---
>  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);

Where does r2 come from here, where does it get used, and why do we need
tovirt() on book3e?

-Scott
Tiejun Chen - Oct. 23, 2013, 9:26 a.m.
On 10/19/2013 06:37 AM, Scott Wood wrote:
> 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"?

Yes.

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

Here I don't fix anything, and I just want to do the same thing as 32-bit.

>
>> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
>> ---
>>   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?

Okay, I'd like to move forward the c code, please see next version.

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

Looks this is redundant so this will be left out.

Thanks,

Tiejun
Tiejun Chen - Oct. 23, 2013, 9:28 a.m.
On 10/19/2013 07:55 AM, Scott Wood wrote:
> 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.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
>> ---
>>   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);
>
> Where does r2 come from here, where does it get used, and why do we need
> tovirt() on book3e?
>

As I remember this should be covered when we boot that capture kernel in 
kexec/kdump case.

Now this is also gone away after move forward the c code.

Thanks,

Tiejun

Patch

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
+
 /* 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;					\
+1:
 #define SPRN_CRIT_SRR0	SPRN_CSRR0
 #define SPRN_CRIT_SRR1	SPRN_CSRR1
 
 #define DBG_SET_KSTACK						            \
+	andi.	r10,r11,MSR_PR;							\
+	bne	1f;								\
+	BOOK3E_LOAD_EXC_LEVEL_STACK(DBG);					\
 	ld	r1,PACA_DBG_STACK(r13);					    \
-	subi	r1,r1,SPECIAL_EXC_FRAME_SIZE;
+	subi	r1,r1,SPECIAL_EXC_FRAME_SIZE;					\
+1:
 #define SPRN_DBG_SRR0	SPRN_DSRR0
 #define SPRN_DBG_SRR1	SPRN_DSRR1
 
 #define MC_SET_KSTACK						            \
+	andi.	r10,r11,MSR_PR;							\
+	bne	1f;								\
+	BOOK3E_LOAD_EXC_LEVEL_STACK(MC);					\
 	ld	r1,PACA_MC_STACK(r13);					    \
-	subi	r1,r1,SPECIAL_EXC_FRAME_SIZE;
+	subi	r1,r1,SPECIAL_EXC_FRAME_SIZE;					\
+1:
 #define SPRN_MC_SRR0	SPRN_MCSRR0
 #define SPRN_MC_SRR1	SPRN_MCSRR1