[PATCH] ppc32: handle Book E debug exceptions on kernel stack

[PATCH] ppc32: handle Book E debug exceptions on kernel stack

[Dale Farnsworth]

From: Dale Farnsworth <dale@farnsworth.org>

On PPC Book E processsors, we currently handle debug
exceptions on the critical exception stack (debug stack
for E200).  This causes problems with the kgdb single
step handler, which calls smp_processor_id() and spin_lock(),
which reference current_thread_info(), which only works when
we are on the kernel stack.

We address this by switching to the kernel stack early while
handling debug exceptions.  Note that the entry values of r10
and r11 are still saved on the critical exception (or debug) stack.

Signed-off-by: Dale Farnsworth <dale@farnsworth.org>

---

This is low level code and needs careful review.
I've only tested it on mpc8548cds.

I didn't find the corresponding code in arch/powerpc,
so I assume Book E support hasn't been merged yet.

 arch/ppc/kernel/head_booke.h |   15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/ppc/kernel/head_booke.h b/arch/ppc/kernel/head_booke.h
index f3d274c..a5eec51 100644
--- a/arch/ppc/kernel/head_booke.h
+++ b/arch/ppc/kernel/head_booke.h
@@ -92,7 +92,8 @@
  * registers as the normal prolog above. Instead we use a portion of the
  * critical/machine check exception stack at low physical addresses.
  */
-#define EXC_LEVEL_EXCEPTION_PROLOG(exc_level, exc_level_srr0, exc_level_srr1) \
+#define EXC_LEVEL_EXCEPTION_PROLOG(exc_level, exc_level_srr0,		     \
+				   exc_level_srr1, kernel_sp_reg)	     \
 	mtspr	exc_level##_SPRG,r8;					     \
 	BOOKE_LOAD_EXC_LEVEL_STACK(exc_level);/* r8 points to the exc_level stack*/ \
 	stw	r10,GPR10-INT_FRAME_SIZE(r8);				     \
@@ -100,7 +101,7 @@
 	mfcr	r10;			/* save CR in r10 for now	   */\
 	mfspr	r11,exc_level_srr1;	/* check whether user or kernel    */\
 	andi.	r11,r11,MSR_PR;						     \
-	mr	r11,r8;							     \
+	mr	r11,kernel_sp_reg;					     \
 	mfspr	r8,exc_level##_SPRG;					     \
 	beq	1f;							     \
 	/* COMING FROM USER MODE */					     \
@@ -128,11 +129,13 @@
 	SAVE_2GPRS(7, r11)
 
 #define CRITICAL_EXCEPTION_PROLOG \
-		EXC_LEVEL_EXCEPTION_PROLOG(CRIT, SPRN_CSRR0, SPRN_CSRR1)
+		EXC_LEVEL_EXCEPTION_PROLOG(CRIT, SPRN_CSRR0, SPRN_CSRR1, r8)
+#define CRITICAL_EXCEPTION_ON_KERNEL_STACK_PROLOG \
+		EXC_LEVEL_EXCEPTION_PROLOG(CRIT, SPRN_CSRR0, SPRN_CSRR1, r1)
 #define DEBUG_EXCEPTION_PROLOG \
-		EXC_LEVEL_EXCEPTION_PROLOG(DEBUG, SPRN_DSRR0, SPRN_DSRR1)
+		EXC_LEVEL_EXCEPTION_PROLOG(DEBUG, SPRN_DSRR0, SPRN_DSRR1, r1)
 #define MCHECK_EXCEPTION_PROLOG \
-		EXC_LEVEL_EXCEPTION_PROLOG(MCHECK, SPRN_MCSRR0, SPRN_MCSRR1)
+		EXC_LEVEL_EXCEPTION_PROLOG(MCHECK, SPRN_MCSRR0, SPRN_MCSRR1, r8)
 
 /*
  * Exception vectors.
@@ -268,7 +271,7 @@ label:
 #else
 #define DEBUG_EXCEPTION							      \
 	START_EXCEPTION(Debug);						      \
-	CRITICAL_EXCEPTION_PROLOG;					      \
+	CRITICAL_EXCEPTION_ON_KERNEL_STACK_PROLOG;			      \
 									      \
 	/*								      \
 	 * If there is a single step or branch-taken exception in an	      \

Re: [PATCH] ppc32: handle Book E debug exceptions on kernel stack

[Kumar Gala]

Let me look at this a little, however, I'm amused by the 
smp_processor_id() usage on 8548.  Its a single core SoC, so any reason 
SMP is turned on in your kernel?

Not, that your patch, isn't needed for a SMP Book-E, just wondering.

- k 

On Tue, 14 Feb 2006, Dale Farnsworth wrote:

> From: Dale Farnsworth <dale@farnsworth.org>
> 
> On PPC Book E processsors, we currently handle debug
> exceptions on the critical exception stack (debug stack
> for E200).  This causes problems with the kgdb single
> step handler, which calls smp_processor_id() and spin_lock(),
> which reference current_thread_info(), which only works when
> we are on the kernel stack.
> 
> We address this by switching to the kernel stack early while
> handling debug exceptions.  Note that the entry values of r10
> and r11 are still saved on the critical exception (or debug) stack.
> 
> Signed-off-by: Dale Farnsworth <dale@farnsworth.org>
> 
> ---
> 
> This is low level code and needs careful review.
> I've only tested it on mpc8548cds.
> 
> I didn't find the corresponding code in arch/powerpc,
> so I assume Book E support hasn't been merged yet.
> 
>  arch/ppc/kernel/head_booke.h |   15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/ppc/kernel/head_booke.h b/arch/ppc/kernel/head_booke.h
> index f3d274c..a5eec51 100644
> --- a/arch/ppc/kernel/head_booke.h
> +++ b/arch/ppc/kernel/head_booke.h
> @@ -92,7 +92,8 @@
>   * registers as the normal prolog above. Instead we use a portion of the
>   * critical/machine check exception stack at low physical addresses.
>   */
> -#define EXC_LEVEL_EXCEPTION_PROLOG(exc_level, exc_level_srr0, exc_level_srr1) \
> +#define EXC_LEVEL_EXCEPTION_PROLOG(exc_level, exc_level_srr0,		     \
> +				   exc_level_srr1, kernel_sp_reg)	     \
>  	mtspr	exc_level##_SPRG,r8;					     \
>  	BOOKE_LOAD_EXC_LEVEL_STACK(exc_level);/* r8 points to the exc_level stack*/ \
>  	stw	r10,GPR10-INT_FRAME_SIZE(r8);				     \
> @@ -100,7 +101,7 @@
>  	mfcr	r10;			/* save CR in r10 for now	   */\
>  	mfspr	r11,exc_level_srr1;	/* check whether user or kernel    */\
>  	andi.	r11,r11,MSR_PR;						     \
> -	mr	r11,r8;							     \
> +	mr	r11,kernel_sp_reg;					     \
>  	mfspr	r8,exc_level##_SPRG;					     \
>  	beq	1f;							     \
>  	/* COMING FROM USER MODE */					     \
> @@ -128,11 +129,13 @@
>  	SAVE_2GPRS(7, r11)
>  
>  #define CRITICAL_EXCEPTION_PROLOG \
> -		EXC_LEVEL_EXCEPTION_PROLOG(CRIT, SPRN_CSRR0, SPRN_CSRR1)
> +		EXC_LEVEL_EXCEPTION_PROLOG(CRIT, SPRN_CSRR0, SPRN_CSRR1, r8)
> +#define CRITICAL_EXCEPTION_ON_KERNEL_STACK_PROLOG \
> +		EXC_LEVEL_EXCEPTION_PROLOG(CRIT, SPRN_CSRR0, SPRN_CSRR1, r1)
>  #define DEBUG_EXCEPTION_PROLOG \
> -		EXC_LEVEL_EXCEPTION_PROLOG(DEBUG, SPRN_DSRR0, SPRN_DSRR1)
> +		EXC_LEVEL_EXCEPTION_PROLOG(DEBUG, SPRN_DSRR0, SPRN_DSRR1, r1)
>  #define MCHECK_EXCEPTION_PROLOG \
> -		EXC_LEVEL_EXCEPTION_PROLOG(MCHECK, SPRN_MCSRR0, SPRN_MCSRR1)
> +		EXC_LEVEL_EXCEPTION_PROLOG(MCHECK, SPRN_MCSRR0, SPRN_MCSRR1, r8)
>  
>  /*
>   * Exception vectors.
> @@ -268,7 +271,7 @@ label:
>  #else
>  #define DEBUG_EXCEPTION							      \
>  	START_EXCEPTION(Debug);						      \
> -	CRITICAL_EXCEPTION_PROLOG;					      \
> +	CRITICAL_EXCEPTION_ON_KERNEL_STACK_PROLOG;			      \
>  									      \
>  	/*								      \
>  	 * If there is a single step or branch-taken exception in an	      \
> 

Re: [PATCH] ppc32: handle Book E debug exceptions on kernel stack

[Dale Farnsworth]

On Tue, Feb 14, 2006 at 03:57:58PM -0600, Kumar Gala wrote:
> Let me look at this a little, however, I'm amused by the 

Thanks.

> smp_processor_id() usage on 8548.  Its a single core SoC, so any reason 
> SMP is turned on in your kernel?

I haven't turned on SMP in my 8548 kernel.

The kgdb singlestep code calls smp_processor_id() even on UP.
It doesn't really need to call it on UP, but smp_processor_id()
still should work.  I would have preferred seeing #ifdef
CONFIG_SMP scattered throughout that code, but it's not my code.
Anyway, the smp_processor_id() calls happen to return the
correct value on UP, even on the critical exception stack,
since that stack area is initialized to 0.

The issue I actually hit is that for CONFIG_PREEMPT, the calls
to spin_lock() and spin_unlock() increment and decrement
current_thread_info()->preempt_count. This breaks things
when the preempt_count goes negative.

> Not, that your patch, isn't needed for a SMP Book-E, just wondering.

Yes, I'm looking forward to trying out a dual-core processor.

-Dale

> On Tue, 14 Feb 2006, Dale Farnsworth wrote:
> 
> > From: Dale Farnsworth <dale@farnsworth.org>
> > 
> > On PPC Book E processsors, we currently handle debug
> > exceptions on the critical exception stack (debug stack
> > for E200).  This causes problems with the kgdb single
> > step handler, which calls smp_processor_id() and spin_lock(),
> > which reference current_thread_info(), which only works when
> > we are on the kernel stack.
> > 
> > We address this by switching to the kernel stack early while
> > handling debug exceptions.  Note that the entry values of r10
> > and r11 are still saved on the critical exception (or debug) stack.
> > 
> > Signed-off-by: Dale Farnsworth <dale@farnsworth.org>

Re: [PATCH] ppc32: handle Book E debug exceptions on kernel stack

[Kumar Gala]

On Tue, 14 Feb 2006, Dale Farnsworth wrote:

> From: Dale Farnsworth <dale@farnsworth.org>
> 
> On PPC Book E processsors, we currently handle debug
> exceptions on the critical exception stack (debug stack
> for E200).  This causes problems with the kgdb single
> step handler, which calls smp_processor_id() and spin_lock(),
> which reference current_thread_info(), which only works when
> we are on the kernel stack.
> 
> We address this by switching to the kernel stack early while
> handling debug exceptions.  Note that the entry values of r10
> and r11 are still saved on the critical exception (or debug) stack.
> 
> Signed-off-by: Dale Farnsworth <dale@farnsworth.org>
> 

Paul,

We were wondering if you or David remember why a specific critical
exception stack was added in the 40x port from 2.4 to 2.6?

- kumar

> ---
> 
> This is low level code and needs careful review.
> I've only tested it on mpc8548cds.
> 
> I didn't find the corresponding code in arch/powerpc,
> so I assume Book E support hasn't been merged yet.
> 
>  arch/ppc/kernel/head_booke.h |   15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/ppc/kernel/head_booke.h b/arch/ppc/kernel/head_booke.h
> index f3d274c..a5eec51 100644
> --- a/arch/ppc/kernel/head_booke.h
> +++ b/arch/ppc/kernel/head_booke.h
> @@ -92,7 +92,8 @@
>   * registers as the normal prolog above. Instead we use a portion of the
>   * critical/machine check exception stack at low physical addresses.
>   */
> -#define EXC_LEVEL_EXCEPTION_PROLOG(exc_level, exc_level_srr0, exc_level_srr1) \
> +#define EXC_LEVEL_EXCEPTION_PROLOG(exc_level, exc_level_srr0,		     \
> +				   exc_level_srr1, kernel_sp_reg)	     \
>  	mtspr	exc_level##_SPRG,r8;					     \
>  	BOOKE_LOAD_EXC_LEVEL_STACK(exc_level);/* r8 points to the exc_level stack*/ \
>  	stw	r10,GPR10-INT_FRAME_SIZE(r8);				     \
> @@ -100,7 +101,7 @@
>  	mfcr	r10;			/* save CR in r10 for now	   */\
>  	mfspr	r11,exc_level_srr1;	/* check whether user or kernel    */\
>  	andi.	r11,r11,MSR_PR;						     \
> -	mr	r11,r8;							     \
> +	mr	r11,kernel_sp_reg;					     \
>  	mfspr	r8,exc_level##_SPRG;					     \
>  	beq	1f;							     \
>  	/* COMING FROM USER MODE */					     \
> @@ -128,11 +129,13 @@
>  	SAVE_2GPRS(7, r11)
>  
>  #define CRITICAL_EXCEPTION_PROLOG \
> -		EXC_LEVEL_EXCEPTION_PROLOG(CRIT, SPRN_CSRR0, SPRN_CSRR1)
> +		EXC_LEVEL_EXCEPTION_PROLOG(CRIT, SPRN_CSRR0, SPRN_CSRR1, r8)
> +#define CRITICAL_EXCEPTION_ON_KERNEL_STACK_PROLOG \
> +		EXC_LEVEL_EXCEPTION_PROLOG(CRIT, SPRN_CSRR0, SPRN_CSRR1, r1)
>  #define DEBUG_EXCEPTION_PROLOG \
> -		EXC_LEVEL_EXCEPTION_PROLOG(DEBUG, SPRN_DSRR0, SPRN_DSRR1)
> +		EXC_LEVEL_EXCEPTION_PROLOG(DEBUG, SPRN_DSRR0, SPRN_DSRR1, r1)
>  #define MCHECK_EXCEPTION_PROLOG \
> -		EXC_LEVEL_EXCEPTION_PROLOG(MCHECK, SPRN_MCSRR0, SPRN_MCSRR1)
> +		EXC_LEVEL_EXCEPTION_PROLOG(MCHECK, SPRN_MCSRR0, SPRN_MCSRR1, r8)
>  
>  /*
>   * Exception vectors.
> @@ -268,7 +271,7 @@ label:
>  #else
>  #define DEBUG_EXCEPTION							      \
>  	START_EXCEPTION(Debug);						      \
> -	CRITICAL_EXCEPTION_PROLOG;					      \
> +	CRITICAL_EXCEPTION_ON_KERNEL_STACK_PROLOG;			      \
>  									      \
>  	/*								      \
>  	 * If there is a single step or branch-taken exception in an	      \
> 

Re: [PATCH] ppc32: handle Book E debug exceptions on kernel stack

[Josh Boyer]

On Wed, 2006-02-22 at 22:19 -0600, Kumar Gala wrote:
> On Tue, 14 Feb 2006, Dale Farnsworth wrote:
> 
> > From: Dale Farnsworth <dale@farnsworth.org>
> > 
> > On PPC Book E processsors, we currently handle debug
> > exceptions on the critical exception stack (debug stack
> > for E200).  This causes problems with the kgdb single
> > step handler, which calls smp_processor_id() and spin_lock(),
> > which reference current_thread_info(), which only works when
> > we are on the kernel stack.
> > 
> > We address this by switching to the kernel stack early while
> > handling debug exceptions.  Note that the entry values of r10
> > and r11 are still saved on the critical exception (or debug) stack.
> > 
> > Signed-off-by: Dale Farnsworth <dale@farnsworth.org>
> > 
> 
> Paul,
> 
> We were wondering if you or David remember why a specific critical
> exception stack was added in the 40x port from 2.4 to 2.6?

I think Matt did that.  And if I remember correctly, it was to avoid
corruption if you were in the middle of handling a normal interrupt and
a critical interrupt came in.

josh

Re: [PATCH] ppc32: handle Book E debug exceptions on kernel stack

[Dale Farnsworth]

On Thu, Feb 23, 2006 at 10:54:02AM +0000, Josh Boyer wrote:
> On Wed, 2006-02-22 at 22:19 -0600, Kumar Gala wrote:
> > On Tue, 14 Feb 2006, Dale Farnsworth wrote:
> > 
> > > From: Dale Farnsworth <dale@farnsworth.org>
> > > 
> > > On PPC Book E processsors, we currently handle debug
> > > exceptions on the critical exception stack (debug stack
> > > for E200).  This causes problems with the kgdb single
> > > step handler, which calls smp_processor_id() and spin_lock(),
> > > which reference current_thread_info(), which only works when
> > > we are on the kernel stack.
> > > 
> > > We address this by switching to the kernel stack early while
> > > handling debug exceptions.  Note that the entry values of r10
> > > and r11 are still saved on the critical exception (or debug) stack.
> > > 
> > > Signed-off-by: Dale Farnsworth <dale@farnsworth.org>
> > 
> > Paul,
> > 
> > We were wondering if you or David remember why a specific critical
> > exception stack was added in the 40x port from 2.4 to 2.6?
> 
> I think Matt did that.  And if I remember correctly, it was to avoid
> corruption if you were in the middle of handling a normal interrupt and
> a critical interrupt came in.

Thanks Josh, Matt was one of the "we" who were wondering.  If indeed
you are remembering correctly, then I think we're safe to use the
normal kernel stack rather than the critical exception stack.

-Dale

Re: [PATCH] ppc32: handle Book E debug exceptions on kernel stack

[David Gibson]

On Wed, Feb 22, 2006 at 10:19:54PM -0600, Kumar Gala wrote:
> On Tue, 14 Feb 2006, Dale Farnsworth wrote:
> 
> > From: Dale Farnsworth <dale@farnsworth.org>
> > 
> > On PPC Book E processsors, we currently handle debug
> > exceptions on the critical exception stack (debug stack
> > for E200).  This causes problems with the kgdb single
> > step handler, which calls smp_processor_id() and spin_lock(),
> > which reference current_thread_info(), which only works when
> > we are on the kernel stack.
> > 
> > We address this by switching to the kernel stack early while
> > handling debug exceptions.  Note that the entry values of r10
> > and r11 are still saved on the critical exception (or debug) stack.
> > 
> > Signed-off-by: Dale Farnsworth <dale@farnsworth.org>
> > 
> 
> Paul,
> 
> We were wondering if you or David remember why a specific critical
> exception stack was added in the 40x port from 2.4 to 2.6?

I don't, in any detail.  But did it have to do with a possible race
with a critical exception in just the wrong part of the normal
exception exit path.  ISTR BenH was worried about something in that
area.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH] ppc32: handle Book E debug exceptions on kernel stack

[Dale Farnsworth]

On Fri, Feb 24, 2006 at 10:49:45AM +1100, David Gibson wrote:
> On Wed, Feb 22, 2006 at 10:19:54PM -0600, Kumar Gala wrote:
> > On Tue, 14 Feb 2006, Dale Farnsworth wrote:
> > 
> > > From: Dale Farnsworth <dale@farnsworth.org>
> > > 
> > > On PPC Book E processsors, we currently handle debug
> > > exceptions on the critical exception stack (debug stack
> > > for E200).  This causes problems with the kgdb single
> > > step handler, which calls smp_processor_id() and spin_lock(),
> > > which reference current_thread_info(), which only works when
> > > we are on the kernel stack.
> > > 
> > > We address this by switching to the kernel stack early while
> > > handling debug exceptions.  Note that the entry values of r10
> > > and r11 are still saved on the critical exception (or debug) stack.
> > > 
> > > Signed-off-by: Dale Farnsworth <dale@farnsworth.org>
> > > 
> > 
> > Paul,
> > 
> > We were wondering if you or David remember why a specific critical
> > exception stack was added in the 40x port from 2.4 to 2.6?
> 
> I don't, in any detail.  But did it have to do with a possible race
> with a critical exception in just the wrong part of the normal
> exception exit path.  ISTR BenH was worried about something in that
> area.

There's a race between a critical exception and the normal exception
entry path.  If a normal exception occurs in user mode and then a
critical exception occurs before the normal exception handler has set
r1 to the kernel stack area, we end up with r1 containing the user sp,
though the critical exception occurred in kernel mode.

I haven't yet come up with a way to detect this case and reliably use
the kernel stack on a critical exception.

-Dale

Re: [PATCH] ppc32: handle Book E debug exceptions on kernel stack

[Paul Mackerras]

Dale Farnsworth writes:

> There's a race between a critical exception and the normal exception
> entry path.  If a normal exception occurs in user mode and then a
> critical exception occurs before the normal exception handler has set
> r1 to the kernel stack area, we end up with r1 containing the user sp,
> though the critical exception occurred in kernel mode.

That's a separate problem from what we were discussing, which is why
program_check_exception is entered with interrupts disabled.

> I haven't yet come up with a way to detect this case and reliably use
> the kernel stack on a critical exception.

One way to solve this is to put the kernel stack pointer in some SPRGn
when exiting to usermode, and on critical exceptions, use that SPRGn
for the kernel stack if it is non-zero.  In the exception entry path
(for both normal and critical exceptions) put 0 in it once r1 has been
set up with a valid kernel stack pointer.  This also works if you have
to call firmware such as RTAS which runs in kernel mode but may put
arbitrary values in r1.

Paul.