powerpc: save trap number in bad_stack

PATCH Superseded Olof Johansson 7 participants 15 messages Apr 16, 2007 – Apr 23, 2007 PowerPC

Share & tools

Olof Johansson

author

2007-04-16 06:05 UTC #0 lore raw

PATCH 1/1 powerpc: save trap number in bad_stack

Save the trap number in the case of getting a bad stack in an exception handler. It is sometimes useful to know what exception it was that caused this to happen. Without this, no trap is reported.

Signed-off-by Olof Johansson <[email protected]>

arch/powerpc/kernel/asm-offsets.c +1 -0

Index: linux-2.6/arch/powerpc/kernel/asm-offsets.c
===================================================================
@@ -139,6 +139,7 @@ int main(void)
 	DEFINE(PACA_SYSTEM_TIME, offsetof(struct paca_struct, system_time));
 	DEFINE(PACA_SLBSHADOWPTR, offsetof(struct paca_struct, slb_shadow_ptr));
 	DEFINE(PACA_DATA_OFFSET, offsetof(struct paca_struct, data_offset));
+	DEFINE(PACA_TRAP_SAVE, offsetof(struct paca_struct, trap_save));
 
 	DEFINE(SLBSHADOW_STACKVSID,
 	       offsetof(struct slb_shadow, save_area[SLB_NUM_BOLTED - 1].vsid));

include/asm-powerpc/paca.h +2 -0

Index: linux-2.6/include/asm-powerpc/paca.h
===================================================================
@@ -81,6 +81,8 @@ struct paca_struct {
 	u64 exslb[10];		/* used for SLB/segment table misses
  				 * on the linear mapping */
 
+	u64 trap_save;			/* Used when bad stack is encountered */
+
 	mm_context_t context;
 	u16 vmalloc_sllp;
 	u16 slb_cache[SLB_CACHE_ENTRIES];

arch/powerpc/kernel/head_64.S +8 -2

Index: linux-2.6/arch/powerpc/kernel/head_64.S
===================================================================
@@ -278,8 +278,12 @@ exception_marker:
 	beq-	1f;							   \
 	ld	r1,PACAKSAVE(r13);	/* kernel stack to use		*/ \
 1:	cmpdi	cr1,r1,0;		/* check if r1 is in userspace	*/ \
-	bge-	cr1,bad_stack;		/* abort if it is		*/ \
-	std	r9,_CCR(r1);		/* save CR in stackframe	*/ \
+	bge-	cr1,2f;			/* abort if it is		*/ \
+	b	3f;							   \
+2:	li	r1,(n);			/* will be reloaded later	*/ \
+	std	r1,PACA_TRAP_SAVE(r13);					   \
+	b	bad_stack;						   \
+3:	std	r9,_CCR(r1);		/* save CR in stackframe	*/ \
 	std	r11,_NIP(r1);		/* save SRR0 in stackframe	*/ \
 	std	r12,_MSR(r1);		/* save SRR1 in stackframe	*/ \
 	std	r10,0(r1);		/* make stack chain pointer	*/ \

@@ -940,6 +944,8 @@ bad_stack:
 	SAVE_2GPRS(7,r1)
 	SAVE_10GPRS(12,r1)
 	SAVE_10GPRS(22,r1)
+	ld	r12,PACA_TRAP_SAVE(r13)
+	std	r12,_TRAP(r1)
 	addi	r11,r1,INT_FRAME_SIZE
 	std	r11,0(r1)
 	li	r12,0

Benjamin Herrenschmidt

2007-04-19 23:18 UTC #1 lore raw

Quoted 24 lines · Olof Johansson

Save the trap number in the case of getting a bad stack in an exception handler. It is sometimes useful to know what exception it was that caused this to happen. Without this, no trap is reported.
Signed-off-by Olof Johansson <[email protected]>
Index: linux-2.6/arch/powerpc/kernel/asm-offsets.c
===================================================================
arch/powerpc/kernel/asm-offsets.c +1 -0
@@ -139,6 +139,7 @@ int main(void)
 	DEFINE(PACA_SYSTEM_TIME, offsetof(struct paca_struct, system_time));
 	DEFINE(PACA_SLBSHADOWPTR, offsetof(struct paca_struct, slb_shadow_ptr));
 	DEFINE(PACA_DATA_OFFSET, offsetof(struct paca_struct, data_offset));
+	DEFINE(PACA_TRAP_SAVE, offsetof(struct paca_struct, trap_save));
 
 	DEFINE(SLBSHADOW_STACKVSID,
 	       offsetof(struct slb_shadow, save_area[SLB_NUM_BOLTED - 1].vsid));
Index: linux-2.6/include/asm-powerpc/paca.h
===================================================================

include/asm-powerpc/paca.h struct paca_struct { · line 84

Quoted 4 lines

include/asm-powerpc/paca.h +1 -0

@@ -81,6 +81,8 @@ struct paca_struct {
 	u64 exslb[10];		/* used for SLB/segment table misses
  				 * on the linear mapping */
 
+	u64 trap_save;			/* Used when bad stack is encountered */

Can you put it elsewhere in the PACA (around less frequently used bits) ? This is a pretty hot area of the PACA...

Ben.

Michael Ellerman

2007-04-20 02:18 UTC #2 lore raw

Quoted 28 lines · Benjamin Herrenschmidt

Quoted 27 lines · Olof Johansson
Save the trap number in the case of getting a bad stack in an exception handler. It is sometimes useful to know what exception it was that caused this to happen. Without this, no trap is reported.
Signed-off-by Olof Johansson <[email protected]>
Index: linux-2.6/arch/powerpc/kernel/asm-offsets.c
===================================================================
arch/powerpc/kernel/asm-offsets.c +1 -0
@@ -139,6 +139,7 @@ int main(void)
 	DEFINE(PACA_SYSTEM_TIME, offsetof(struct paca_struct, system_time));
 	DEFINE(PACA_SLBSHADOWPTR, offsetof(struct paca_struct, slb_shadow_ptr));
 	DEFINE(PACA_DATA_OFFSET, offsetof(struct paca_struct, data_offset));
+	DEFINE(PACA_TRAP_SAVE, offsetof(struct paca_struct, trap_save));
 
 	DEFINE(SLBSHADOW_STACKVSID,
 	       offsetof(struct slb_shadow, save_area[SLB_NUM_BOLTED - 1].vsid));
Index: linux-2.6/include/asm-powerpc/paca.h
===================================================================
include/asm-powerpc/paca.h +0 -0
@@ -81,6 +81,8 @@ struct paca_struct {
 	u64 exslb[10];		/* used for SLB/segment table misses
  				 * on the linear mapping */
 

Quoted 4 lines

include/asm-powerpc/paca.h struct paca_struct { · line 84
Quoted 1 line
include/asm-powerpc/paca.h +1 -0
+	u64 trap_save;			/* Used when bad stack is encountered */
Can you put it elsewhere in the PACA (around less frequently used bits) ? This is a pretty hot area of the PACA...

It's only set in the bad_stack case though.

You should put it after cpu_start, there's a bit hole there, and trap_save is readonly during normal operation, so it shouldn't be a performance hit.

cheers

Signature · 8 lines

Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

Segher Boessenkool

2007-04-20 08:51 UTC #3 lore raw

Quoted 10 lines

Quoted 4 lines
include/asm-powerpc/paca.h struct paca_struct { · line 84
Quoted 1 line
include/asm-powerpc/paca.h +1 -0
+	u64 trap_save;			/* Used when bad stack is encountered */
Can you put it elsewhere in the PACA (around less frequently used bits) ? This is a pretty hot area of the PACA...
It's only set in the bad_stack case though.
You should put it after cpu_start, there's a bit hole there, and trap_save is readonly during normal operation, so it shouldn't be a performance hit.

That's not a performance hit compared to the current state of the code, sure; but that "hot slot" could be used for a hotter variable, instead.

Segher

Olof Johansson

author

2007-04-22 23:15 UTC #4 lore raw

In reply to Benjamin Herrenschmidt

Quoted 2 lines · Benjamin Herrenschmidt

Can you put it elsewhere in the PACA (around less frequently used bits) ? This is a pretty hot area of the PACA...

Good point, I didn't think much about layout when I added it. There's actually room right before if it's reduced in size (which is easy to do). See patch posted separately.

-Olof

Olof Johansson

author

2007-04-22 23:19 UTC #5 lore raw

PATCH v2 1/1 powerpc: save trap number in bad_stack

In reply to Olof Johansson

Save the trap number in the case of getting a bad stack in an exception handler. It is sometimes useful to know what exception it was that caused this to happen. Without this, no trap is reported.

Signed-off-by Olof Johansson <[email protected]>

---

Changes since last time: Reduce to a u16 and move it.

arch/powerpc/kernel/asm-offsets.c +1 -0

Index: powerpc/arch/powerpc/kernel/asm-offsets.c
===================================================================
@@ -139,6 +139,7 @@ int main(void)
 	DEFINE(PACA_SYSTEM_TIME, offsetof(struct paca_struct, system_time));
 	DEFINE(PACA_SLBSHADOWPTR, offsetof(struct paca_struct, slb_shadow_ptr));
 	DEFINE(PACA_DATA_OFFSET, offsetof(struct paca_struct, data_offset));
+	DEFINE(PACA_TRAP_SAVE, offsetof(struct paca_struct, trap_save));
 
 	DEFINE(SLBSHADOW_STACKVSID,
 	       offsetof(struct slb_shadow, save_area[SLB_NUM_BOLTED - 1].vsid));

include/asm-powerpc/paca.h +1 -0

Index: powerpc/include/asm-powerpc/paca.h
===================================================================
@@ -68,6 +68,7 @@ struct paca_struct {
 	void *emergency_sp;		/* pointer to emergency stack */
 	u64 data_offset;		/* per cpu data offset */
 	s16 hw_cpu_id;			/* Physical processor number */
+	u16 trap_save;			/* Used when bad stack is encountered */
 	u8 cpu_start;			/* At startup, processor spins until */
 					/* this becomes non-zero. */
 	struct slb_shadow *slb_shadow_ptr;

arch/powerpc/kernel/head_64.S +8 -2

Index: powerpc/arch/powerpc/kernel/head_64.S
===================================================================
@@ -278,8 +278,12 @@ exception_marker:
 	beq-	1f;							   \
 	ld	r1,PACAKSAVE(r13);	/* kernel stack to use		*/ \
 1:	cmpdi	cr1,r1,0;		/* check if r1 is in userspace	*/ \
-	bge-	cr1,bad_stack;		/* abort if it is		*/ \
-	std	r9,_CCR(r1);		/* save CR in stackframe	*/ \
+	bge-	cr1,2f;			/* abort if it is		*/ \
+	b	3f;							   \
+2:	li	r1,(n);			/* will be reloaded later	*/ \
+	stw	r1,PACA_TRAP_SAVE(r13);					   \
+	b	bad_stack;						   \
+3:	std	r9,_CCR(r1);		/* save CR in stackframe	*/ \
 	std	r11,_NIP(r1);		/* save SRR0 in stackframe	*/ \
 	std	r12,_MSR(r1);		/* save SRR1 in stackframe	*/ \
 	std	r10,0(r1);		/* make stack chain pointer	*/ \

@@ -940,6 +944,8 @@ bad_stack:
 	SAVE_2GPRS(7,r1)
 	SAVE_10GPRS(12,r1)
 	SAVE_10GPRS(22,r1)
+	lwz	r12,PACA_TRAP_SAVE(r13)
+	std	r12,_TRAP(r1)
 	addi	r11,r1,INT_FRAME_SIZE
 	std	r11,0(r1)
 	li	r12,0

Olof Johansson

author

2007-04-23 11:30 UTC #7 lore raw

Quoted 1 line · Gabriel Paubert

stW to an u16? stH seems safer!

Yes, thanks. my bad.

-Olof

Olof Johansson

author

2007-04-23 12:00 UTC #8 lore raw

PATCH v3 1/1 powerpc: save trap number in bad_stack

In reply to Olof Johansson

Save the trap number in the case of getting a bad stack in an exception handler. It is sometimes useful to know what exception it was that caused this to happen. Without this, no trap is reported.

Signed-off-by Olof Johansson <[email protected]>

arch/powerpc/kernel/asm-offsets.c +1 -0

Index: powerpc/arch/powerpc/kernel/asm-offsets.c
===================================================================
@@ -139,6 +139,7 @@ int main(void)
 	DEFINE(PACA_SYSTEM_TIME, offsetof(struct paca_struct, system_time));
 	DEFINE(PACA_SLBSHADOWPTR, offsetof(struct paca_struct, slb_shadow_ptr));
 	DEFINE(PACA_DATA_OFFSET, offsetof(struct paca_struct, data_offset));
+	DEFINE(PACA_TRAP_SAVE, offsetof(struct paca_struct, trap_save));
 
 	DEFINE(SLBSHADOW_STACKVSID,
 	       offsetof(struct slb_shadow, save_area[SLB_NUM_BOLTED - 1].vsid));

include/asm-powerpc/paca.h +1 -0

Index: powerpc/include/asm-powerpc/paca.h
===================================================================
@@ -68,6 +68,7 @@ struct paca_struct {
 	void *emergency_sp;		/* pointer to emergency stack */
 	u64 data_offset;		/* per cpu data offset */
 	s16 hw_cpu_id;			/* Physical processor number */
+	u16 trap_save;			/* Used when bad stack is encountered */
 	u8 cpu_start;			/* At startup, processor spins until */
 					/* this becomes non-zero. */
 	struct slb_shadow *slb_shadow_ptr;

arch/powerpc/kernel/head_64.S +8 -2

Index: powerpc/arch/powerpc/kernel/head_64.S
===================================================================
@@ -278,8 +278,12 @@ exception_marker:
 	beq-	1f;							   \
 	ld	r1,PACAKSAVE(r13);	/* kernel stack to use		*/ \
 1:	cmpdi	cr1,r1,0;		/* check if r1 is in userspace	*/ \
-	bge-	cr1,bad_stack;		/* abort if it is		*/ \
-	std	r9,_CCR(r1);		/* save CR in stackframe	*/ \
+	bge-	cr1,2f;			/* abort if it is		*/ \
+	b	3f;							   \
+2:	li	r1,(n);			/* will be reloaded later	*/ \
+	sth	r1,PACA_TRAP_SAVE(r13);					   \
+	b	bad_stack;						   \
+3:	std	r9,_CCR(r1);		/* save CR in stackframe	*/ \
 	std	r11,_NIP(r1);		/* save SRR0 in stackframe	*/ \
 	std	r12,_MSR(r1);		/* save SRR1 in stackframe	*/ \
 	std	r10,0(r1);		/* make stack chain pointer	*/ \

@@ -940,6 +944,8 @@ bad_stack:
 	SAVE_2GPRS(7,r1)
 	SAVE_10GPRS(12,r1)
 	SAVE_10GPRS(22,r1)
+	lhz	r12,PACA_TRAP_SAVE(r13)
+	std	r12,_TRAP(r1)
 	addi	r11,r1,INT_FRAME_SIZE
 	std	r11,0(r1)
 	li	r12,0

Olof Johansson

author

2007-04-23 15:11 UTC #9 lore raw

PATCH v4 1/1 powerpc: save trap number in bad_stack

Save the trap number in the case of getting a bad stack in an exception handler. It is sometimes useful to know what exception it was that caused this to happen. Without this, no trap is reported.

Signed-off-by Olof Johansson <[email protected]>

---

Moved to a hole in a later cacheline instead of in the read-only line.

arch/powerpc/kernel/asm-offsets.c +1 -0

Index: linux-2.6/arch/powerpc/kernel/asm-offsets.c
===================================================================
@@ -140,6 +140,7 @@ int main(void)
 	DEFINE(PACA_SYSTEM_TIME, offsetof(struct paca_struct, system_time));
 	DEFINE(PACA_SLBSHADOWPTR, offsetof(struct paca_struct, slb_shadow_ptr));
 	DEFINE(PACA_DATA_OFFSET, offsetof(struct paca_struct, data_offset));
+	DEFINE(PACA_TRAP_SAVE, offsetof(struct paca_struct, trap_save));
 
 	DEFINE(SLBSHADOW_STACKVSID,
 	       offsetof(struct slb_shadow, save_area[SLB_NUM_BOLTED - 1].vsid));

include/asm-powerpc/paca.h +1 -0

Index: linux-2.6/include/asm-powerpc/paca.h
===================================================================
@@ -93,6 +93,7 @@ struct paca_struct {
 	u64 stab_rr;			/* stab/slb round-robin counter */
 	u64 saved_r1;			/* r1 save for RTAS calls */
 	u64 saved_msr;			/* MSR saved here by enter_rtas */
+	u16 trap_save;			/* Used when bad stack is encountered */
 	u8 soft_enabled;		/* irq soft-enable flag */
 	u8 hard_enabled;		/* set if irqs are enabled in MSR */
 	u8 io_sync;			/* writel() needs spin_unlock sync */

arch/powerpc/kernel/head_64.S +8 -2

Index: linux-2.6/arch/powerpc/kernel/head_64.S
===================================================================
@@ -278,8 +278,12 @@ exception_marker:
 	beq-	1f;							   \
 	ld	r1,PACAKSAVE(r13);	/* kernel stack to use		*/ \
 1:	cmpdi	cr1,r1,0;		/* check if r1 is in userspace	*/ \
-	bge-	cr1,bad_stack;		/* abort if it is		*/ \
-	std	r9,_CCR(r1);		/* save CR in stackframe	*/ \
+	bge-	cr1,2f;			/* abort if it is		*/ \
+	b	3f;							   \
+2:	li	r1,(n);			/* will be reloaded later	*/ \
+	sth	r1,PACA_TRAP_SAVE(r13);					   \
+	b	bad_stack;						   \
+3:	std	r9,_CCR(r1);		/* save CR in stackframe	*/ \
 	std	r11,_NIP(r1);		/* save SRR0 in stackframe	*/ \
 	std	r12,_MSR(r1);		/* save SRR1 in stackframe	*/ \
 	std	r10,0(r1);		/* make stack chain pointer	*/ \

@@ -940,6 +944,8 @@ bad_stack:
 	SAVE_2GPRS(7,r1)
 	SAVE_10GPRS(12,r1)
 	SAVE_10GPRS(22,r1)
+	lhz	r12,PACA_TRAP_SAVE(r13)
+	std	r12,_TRAP(r1)
 	addi	r11,r1,INT_FRAME_SIZE
 	std	r11,0(r1)
 	li	r12,0

Stephen Rothwell

2007-04-23 15:34 UTC #10 lore raw

Quoted 7 lines · On Mon, 23 Apr 2007 10:11:55 -0500 [email protected] (Olof Johansson) wrote:

Save the trap number in the case of getting a bad stack in an exception handler. It is sometimes useful to know what exception it was that caused this to happen. Without this, no trap is reported.
Signed-off-by Olof Johansson <[email protected]>

I'm happy this time :-)

--
Cheers,
Stephen Rothwell [email protected]
http://www.canb.auug.org.au/~sfr/

Stephen Rothwell

2007-04-23 13:22 UTC #11 lore raw

In reply to Olof Johansson

include/asm-powerpc/paca.h struct paca_struct { · line 71

Quoted 8 lines · On Sun, 22 Apr 2007 18:38:21 -0500 [email protected] (Olof Johansson) wrote:

include/asm-powerpc/paca.h +1 -0

@@ -68,6 +68,7 @@ struct paca_struct {
 	void *emergency_sp;		/* pointer to emergency stack */
 	u64 data_offset;		/* per cpu data offset */
 	s16 hw_cpu_id;			/* Physical processor number */
+	u16 trap_save;			/* Used when bad stack is encountered */

Please read the comment at the top of the struct:

/*
* Because hw_cpu_id, unlike other paca fields, is accessed
* routinely from other CPUs (from the IRQ code), we stick to
* read-only (after boot) fields in the first cacheline to
* avoid cacheline bouncing.
*/

So please move this out of the first cache line.

--
Cheers,
Stephen Rothwell [email protected]
http://www.canb.auug.org.au/~sfr/

Michael Ellerman

2007-04-23 13:33 UTC #12 lore raw

Quoted 12 lines · Stephen Rothwell

include/asm-powerpc/paca.h struct paca_struct { · line 71
Quoted 8 lines · On Sun, 22 Apr 2007 18:38:21 -0500 [email protected] (Olof Johansson) wrote:
include/asm-powerpc/paca.h +1 -0
@@ -68,6 +68,7 @@ struct paca_struct {
 	void *emergency_sp;		/* pointer to emergency stack */
 	u64 data_offset;		/* per cpu data offset */
 	s16 hw_cpu_id;			/* Physical processor number */
+	u16 trap_save;			/* Used when bad stack is encountered */
Please read the comment at the top of the struct:

Quoted 8 lines

/*
* Because hw_cpu_id, unlike other paca fields, is accessed
* routinely from other CPUs (from the IRQ code), we stick to
* read-only (after boot) fields in the first cacheline to
* avoid cacheline bouncing.
*/
So please move this out of the first cache line.

The only time it's written to is when we detect a bad stack pointer, after which we promptly panic, so it's essentially read-only (in fact unused) during normal operation. Plus there's a massive gap there in paca_struct before the start of the second cacheline.

cheers

Signature · 8 lines

Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

Olof Johansson

author

2007-04-23 13:35 UTC #13 lore raw

In reply to Stephen Rothwell

Quoted 12 lines · Stephen Rothwell

include/asm-powerpc/paca.h struct paca_struct { · line 71
Quoted 8 lines · On Sun, 22 Apr 2007 18:38:21 -0500 [email protected] (Olof Johansson) wrote:
include/asm-powerpc/paca.h +1 -0
@@ -68,6 +68,7 @@ struct paca_struct {
 	void *emergency_sp;		/* pointer to emergency stack */
 	u64 data_offset;		/* per cpu data offset */
 	s16 hw_cpu_id;			/* Physical processor number */
+	u16 trap_save;			/* Used when bad stack is encountered */
Please read the comment at the top of the struct:

Quoted 8 lines

/*
* Because hw_cpu_id, unlike other paca fields, is accessed
* routinely from other CPUs (from the IRQ code), we stick to
* read-only (after boot) fields in the first cacheline to
* avoid cacheline bouncing.
*/
So please move this out of the first cache line.

Never thought I'd say this: Too many comments! I missed that one. :)

The variable should be written very rarely, so there should be no performance impact for a normal system. Still, it's a valid point and for consistency it should be moved.

There's room right before the u8 variables further down, I'll move it there.

-Olof

David Gibson

2007-04-23 13:39 UTC #14 lore raw

In reply to Stephen Rothwell

Quoted 12 lines · Stephen Rothwell

include/asm-powerpc/paca.h struct paca_struct { · line 71
Quoted 8 lines · On Sun, 22 Apr 2007 18:38:21 -0500 [email protected] (Olof Johansson) wrote:
include/asm-powerpc/paca.h +1 -0
@@ -68,6 +68,7 @@ struct paca_struct {
 	void *emergency_sp;		/* pointer to emergency stack */
 	u64 data_offset;		/* per cpu data offset */
 	s16 hw_cpu_id;			/* Physical processor number */
+	u16 trap_save;			/* Used when bad stack is encountered */
Please read the comment at the top of the struct:

Quoted 8 lines

/*
* Because hw_cpu_id, unlike other paca fields, is accessed
* routinely from other CPUs (from the IRQ code), we stick to
* read-only (after boot) fields in the first cacheline to
* avoid cacheline bouncing.
*/
So please move this out of the first cache line.

Afaict, Olof's patch only writes to that field when we're about to enter the bad_stack path. In that case we're sufficiently screwed that I don't think some extra cacheline bouncing matters.

Signature · 4 lines

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