[PATCH v2] powerpc: Add irqtrace support for 32-bit powerpc

[PATCH v2] powerpc: Add irqtrace support for 32-bit powerpc

[Dale Farnsworth]

Add the low level irq tracing hooks for 32-bit powerpc needed
to enable full lockdep functionality.

Dale Farnsworth <dale@farnsworth.org>
---

This patch applies on top of Benjamin Herrenschmidt's lockdep patch for
64-bit powerpc. (http://patchwork.ozlabs.org/linuxppc/patch?id=17673)

This version takes care to save and restore *all* the volatile registers
around the call to trace_hardirqs_off in transfer_to_handler, and should
fix the anomalies seen by Johannes Berg.  I've tested on 7447- and 8548-
based boards.

 arch/powerpc/Kconfig           |    1 -
 arch/powerpc/kernel/entry_32.S |   90 ++++++++++++++++++++++++++++++++++++++-
 arch/powerpc/kernel/setup_32.c |    2 +
 include/asm-powerpc/hw_irq.h   |   20 ++++----
 include/asm-powerpc/system.h   |    3 +-
 5 files changed, 100 insertions(+), 16 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 6dbf123..99d8e18 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -55,7 +55,6 @@ config STACKTRACE_SUPPORT
 
 config TRACE_IRQFLAGS_SUPPORT
 	bool
-	depends on PPC64
 	default y
 
 config LOCKDEP_SUPPORT
diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
index 69a91bd..bd3ce0f 100644
--- a/arch/powerpc/kernel/entry_32.S
+++ b/arch/powerpc/kernel/entry_32.S
@@ -144,6 +144,47 @@ transfer_to_handler:
 	.globl transfer_to_handler_cont
 transfer_to_handler_cont:
 3:
+#ifdef CONFIG_TRACE_IRQFLAGS
+	lis	r11,reenable_mmu@h
+	ori	r11,r11,reenable_mmu@l
+	mtspr	SPRN_SRR0,r11
+	mtspr	SPRN_SRR1,r10
+	SYNC
+	RFI
+reenable_mmu:				/* re-enable mmu so we can */
+	mflr	r9			/* call C code, if necessary */
+	mfmsr	r10
+	lwz	r11,_MSR(r1)
+	xor	r10,r10,r11
+	andi.	r10,r10,MSR_EE		/* Did EE change? */
+	beq	1f
+	stwu	r1,-48(r1)		/* Yes, it must have been cleared */
+	stw	r9,52(r1)
+	stw	r0,16(r1)
+	stw	r3,20(r1)
+	stw	r4,24(r1)
+	stw	r5,28(r1)
+	stw	r6,32(r1)
+	stw	r7,36(r1)
+	stw	r8,40(r1)
+	bl	trace_hardirqs_off
+	lwz	r0,16(r1)
+	lwz	r3,20(r1)
+	lwz	r4,24(r1)
+	lwz	r5,28(r1)
+	lwz	r6,32(r1)
+	lwz	r7,36(r1)
+	lwz	r8,40(r1)
+	lwz	r9,52(r1)
+	addi	r1,r1,48
+1:
+	tovirt(r9,r9)
+	lwz	r11,0(r9)		/* virtual address of handler */
+	lwz	r9,4(r9)		/* where to go when done */
+	mtctr	r11
+	mtlr	r9
+	bctr				/* jump to handler */
+#else /* CONFIG_TRACE_IRQFLAGS */
 	mflr	r9
 	lwz	r11,0(r9)		/* virtual address of handler */
 	lwz	r9,4(r9)		/* where to go when done */
@@ -152,6 +193,7 @@ transfer_to_handler_cont:
 	mtlr	r9
 	SYNC
 	RFI				/* jump to handler, enable MMU */
+#endif /* CONFIG_TRACE_IRQFLAGS */
 
 #ifdef CONFIG_6xx
 4:	rlwinm	r12,r12,0,~_TLF_NAPPING
@@ -220,12 +262,20 @@ ret_from_syscall:
 #ifdef SHOW_SYSCALLS
 	bl	do_show_syscall_exit
 #endif
-	mr	r6,r3
 	rlwinm	r12,r1,0,0,(31-THREAD_SHIFT)	/* current_thread_info() */
 	/* disable interrupts so current_thread_info()->flags can't change */
 	LOAD_MSR_KERNEL(r10,MSR_KERNEL)	/* doesn't include MSR_EE */
 	SYNC
 	MTMSRD(r10)
+#ifdef CONFIG_TRACE_IRQFLAGS
+	stwu	r1,-16(r1)
+	stw	r3,12(r1)
+	bl      trace_hardirqs_off
+	lwz	r3,12(r1)
+	addi	r1,r1,16
+	LOAD_MSR_KERNEL(r10,MSR_KERNEL)
+#endif
+	mr	r6,r3
 	lwz	r9,TI_FLAGS(r12)
 	li	r8,-_LAST_ERRNO
 	andi.	r0,r9,(_TIF_SYSCALL_T_OR_A|_TIF_SINGLESTEP|_TIF_USER_WORK_MASK|_TIF_PERSYSCALL_MASK)
@@ -237,6 +287,13 @@ ret_from_syscall:
 	oris	r11,r11,0x1000	/* Set SO bit in CR */
 	stw	r11,_CCR(r1)
 syscall_exit_cont:
+#ifdef CONFIG_TRACE_IRQFLAGS
+	stwu	r1,-16(r1)
+	stw	r3,12(r1)
+	bl      trace_hardirqs_on
+	lwz	r3,12(r1)
+	addi	r1,r1,16
+#endif
 #if defined(CONFIG_4xx) || defined(CONFIG_BOOKE)
 	/* If the process has its own DBCR0 value, load it up.  The single
 	   step bit tells us that dbcr0 should be loaded. */
@@ -337,7 +394,10 @@ syscall_exit_work:
 4:	/* Anything which requires enabling interrupts? */
 	andi.	r0,r9,(_TIF_SYSCALL_T_OR_A|_TIF_SINGLESTEP)
 	beq	ret_from_except
-
+#ifdef CONFIG_TRACE_IRQFLAGS
+	bl      trace_hardirqs_on
+	LOAD_MSR_KERNEL(r10,MSR_KERNEL)
+#endif
 	/* Re-enable interrupts */
 	ori	r10,r10,MSR_EE
 	SYNC
@@ -646,13 +706,22 @@ ret_from_except_full:
 
 	.globl	ret_from_except
 ret_from_except:
+#ifdef CONFIG_TRACE_IRQFLAGS
+	mfmsr	r3
+#endif
 	/* Hard-disable interrupts so that current_thread_info()->flags
 	 * can't change between when we test it and when we return
 	 * from the interrupt. */
 	LOAD_MSR_KERNEL(r10,MSR_KERNEL)
 	SYNC			/* Some chip revs have problems here... */
 	MTMSRD(r10)		/* disable interrupts */
-
+#ifdef CONFIG_TRACE_IRQFLAGS
+	andi.	r3,r3,MSR_EE
+	beq	1f			/* verified */
+	bl	trace_hardirqs_off
+	LOAD_MSR_KERNEL(r10,MSR_KERNEL)
+1:
+#endif
 	lwz	r3,_MSR(r1)	/* Returning to user mode? */
 	andi.	r0,r3,MSR_PR
 	beq	resume_kernel
@@ -709,6 +778,9 @@ restore:
 	stw	r6,icache_44x_need_flush@l(r4)
 1:
 #endif  /* CONFIG_44x */
+#ifdef CONFIG_TRACE_IRQFLAGS
+	bl	trace_hardirqs_on
+#endif
 	lwz	r0,GPR0(r1)
 	lwz	r2,GPR2(r1)
 	REST_4GPRS(3, r1)
@@ -900,6 +972,10 @@ do_work:			/* r10 contains MSR_KERNEL here */
 	beq	do_user_signal
 
 do_resched:			/* r10 contains MSR_KERNEL here */
+#ifdef CONFIG_TRACE_IRQFLAGS
+	bl	trace_hardirqs_on
+	LOAD_MSR_KERNEL(r10,MSR_KERNEL)
+#endif
 	ori	r10,r10,MSR_EE
 	SYNC
 	MTMSRD(r10)		/* hard-enable interrupts */
@@ -908,6 +984,10 @@ recheck:
 	LOAD_MSR_KERNEL(r10,MSR_KERNEL)
 	SYNC
 	MTMSRD(r10)		/* disable interrupts */
+#ifdef CONFIG_TRACE_IRQFLAGS
+	bl	trace_hardirqs_off
+	LOAD_MSR_KERNEL(r10,MSR_KERNEL)
+#endif
 	rlwinm	r9,r1,0,0,(31-THREAD_SHIFT)
 	lwz	r9,TI_FLAGS(r9)
 	andi.	r0,r9,_TIF_NEED_RESCHED
@@ -915,6 +995,10 @@ recheck:
 	andi.	r0,r9,_TIF_SIGPENDING|_TIF_RESTORE_SIGMASK
 	beq	restore_user
 do_user_signal:			/* r10 contains MSR_KERNEL here */
+#ifdef CONFIG_TRACE_IRQFLAGS
+	bl	trace_hardirqs_on
+	LOAD_MSR_KERNEL(r10,MSR_KERNEL)
+#endif
 	ori	r10,r10,MSR_EE
 	SYNC
 	MTMSRD(r10)		/* hard-enable interrupts */
diff --git a/arch/powerpc/kernel/setup_32.c b/arch/powerpc/kernel/setup_32.c
index cd870a8..725dd18 100644
--- a/arch/powerpc/kernel/setup_32.c
+++ b/arch/powerpc/kernel/setup_32.c
@@ -120,6 +120,8 @@ unsigned long __init early_init(unsigned long dt_ptr)
  */
 void __init machine_init(unsigned long dt_ptr, unsigned long phys)
 {
+	lockdep_init();
+
 	/* Enable early debugging if any specified (see udbg.h) */
 	udbg_early_init();
 
diff --git a/include/asm-powerpc/hw_irq.h b/include/asm-powerpc/hw_irq.h
index ad8c9f7..a5f347a 100644
--- a/include/asm-powerpc/hw_irq.h
+++ b/include/asm-powerpc/hw_irq.h
@@ -63,13 +63,13 @@ extern void iseries_handle_interrupts(void);
 
 #if defined(CONFIG_BOOKE)
 #define SET_MSR_EE(x)	mtmsr(x)
-#define local_irq_restore(flags)	__asm__ __volatile__("wrtee %0" : : "r" (flags) : "memory")
+#define raw_local_irq_restore(flags)	__asm__ __volatile__("wrtee %0" : : "r" (flags) : "memory")
 #else
 #define SET_MSR_EE(x)	mtmsr(x)
-#define local_irq_restore(flags)	mtmsr(flags)
+#define raw_local_irq_restore(flags)	mtmsr(flags)
 #endif
 
-static inline void local_irq_disable(void)
+static inline void raw_local_irq_disable(void)
 {
 #ifdef CONFIG_BOOKE
 	__asm__ __volatile__("wrteei 0": : :"memory");
@@ -81,7 +81,7 @@ static inline void local_irq_disable(void)
 #endif
 }
 
-static inline void local_irq_enable(void)
+static inline void raw_local_irq_enable(void)
 {
 #ifdef CONFIG_BOOKE
 	__asm__ __volatile__("wrteei 1": : :"memory");
@@ -93,7 +93,7 @@ static inline void local_irq_enable(void)
 #endif
 }
 
-static inline void local_irq_save_ptr(unsigned long *flags)
+static inline void raw_local_irq_save_ptr(unsigned long *flags)
 {
 	unsigned long msr;
 	msr = mfmsr();
@@ -106,12 +106,12 @@ static inline void local_irq_save_ptr(unsigned long *flags)
 	__asm__ __volatile__("": : :"memory");
 }
 
-#define local_save_flags(flags)	((flags) = mfmsr())
-#define local_irq_save(flags)	local_irq_save_ptr(&flags)
-#define irqs_disabled()		((mfmsr() & MSR_EE) == 0)
+#define raw_local_save_flags(flags)	((flags) = mfmsr())
+#define raw_local_irq_save(flags)	raw_local_irq_save_ptr(&flags)
+#define raw_irqs_disabled()		((mfmsr() & MSR_EE) == 0)
+#define raw_irqs_disabled_flags(flags)	(((flags) & MSR_EE) == 0)
 
-#define hard_irq_enable()	local_irq_enable()
-#define hard_irq_disable()	local_irq_disable()
+#define hard_irq_disable()		raw_local_irq_disable()
 
 #endif /* CONFIG_PPC64 */
 
diff --git a/include/asm-powerpc/system.h b/include/asm-powerpc/system.h
index 29552ff..e040a48 100644
--- a/include/asm-powerpc/system.h
+++ b/include/asm-powerpc/system.h
@@ -5,8 +5,7 @@
 #define _ASM_POWERPC_SYSTEM_H
 
 #include <linux/kernel.h>
-
-#include <asm/hw_irq.h>
+#include <linux/irqflags.h>
 
 /*
  * Memory barrier.
-- 
1.5.4

Re: [PATCH v2] powerpc: Add irqtrace support for 32-bit powerpc

[Benjamin Herrenschmidt]

>  config LOCKDEP_SUPPORT
> diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
> index 69a91bd..bd3ce0f 100644
> --- a/arch/powerpc/kernel/entry_32.S
> +++ b/arch/powerpc/kernel/entry_32.S
> @@ -144,6 +144,47 @@ transfer_to_handler:
>  	.globl transfer_to_handler_cont
>  transfer_to_handler_cont:
>  3:
> +#ifdef CONFIG_TRACE_IRQFLAGS
> +	lis	r11,reenable_mmu@h
> +	ori	r11,r11,reenable_mmu@l
> +	mtspr	SPRN_SRR0,r11
> +	mtspr	SPRN_SRR1,r10
> +	SYNC
> +	RFI

I don't think we need that on 4xx/BookE when using AS0 (that is also
true of the existing transfer_to_handler_cont, could be improved there.

> +reenable_mmu:				/* re-enable mmu so we can */
> +	mflr	r9			/* call C code, if necessary */
> +	mfmsr	r10
> +	lwz	r11,_MSR(r1)
> +	xor	r10,r10,r11
> +	andi.	r10,r10,MSR_EE		/* Did EE change? */
> +	beq	1f
> +	stwu	r1,-48(r1)		/* Yes, it must have been cleared */
> +	stw	r9,52(r1)
> +	stw	r0,16(r1)
> +	stw	r3,20(r1)
> +	stw	r4,24(r1)
> +	stw	r5,28(r1)
> +	stw	r6,32(r1)
> +	stw	r7,36(r1)
> +	stw	r8,40(r1)
> +	bl	trace_hardirqs_off
> +	lwz	r0,16(r1)
> +	lwz	r3,20(r1)
> +	lwz	r4,24(r1)
> +	lwz	r5,28(r1)
> +	lwz	r6,32(r1)
> +	lwz	r7,36(r1)
> +	lwz	r8,40(r1)
> +	lwz	r9,52(r1)
> +	addi	r1,r1,48

Why do yo save all the volatile regs ? They should have been saved on
the stack already by the exception prolog (the ptregs on the stack).

Also, only the system call really cares about -restoring- them. Maybe
you could stick that in an ifdef CONFIG_TRACE_IRQFLAGS section in
DoSyscall pulling them back off the ptregs in the stackframe. 

> +	tovirt(r9,r9)
> +	lwz	r11,0(r9)		/* virtual address of handler */
> +	lwz	r9,4(r9)		/* where to go when done */
> +	mtctr	r11
> +	mtlr	r9
> +	bctr				/* jump to handler */
> +#else /* CONFIG_TRACE_IRQFLAGS */
>  	mflr	r9
>  	lwz	r11,0(r9)		/* virtual address of handler */
>  	lwz	r9,4(r9)		/* where to go when done */
> @@ -152,6 +193,7 @@ transfer_to_handler_cont:
>  	mtlr	r9
>  	SYNC
>  	RFI				/* jump to handler, enable MMU */
> +#endif /* CONFIG_TRACE_IRQFLAGS */
>  
>  #ifdef CONFIG_6xx
>  4:	rlwinm	r12,r12,0,~_TLF_NAPPING
> @@ -220,12 +262,20 @@ ret_from_syscall:
>  #ifdef SHOW_SYSCALLS
>  	bl	do_show_syscall_exit
>  #endif
> -	mr	r6,r3
>  	rlwinm	r12,r1,0,0,(31-THREAD_SHIFT)	/* current_thread_info() */
>  	/* disable interrupts so current_thread_info()->flags can't change */
>  	LOAD_MSR_KERNEL(r10,MSR_KERNEL)	/* doesn't include MSR_EE */
>  	SYNC
>  	MTMSRD(r10)
> +#ifdef CONFIG_TRACE_IRQFLAGS
> +	stwu	r1,-16(r1)
> +	stw	r3,12(r1)
> +	bl      trace_hardirqs_off
> +	lwz	r3,12(r1)
> +	addi	r1,r1,16
> +	LOAD_MSR_KERNEL(r10,MSR_KERNEL)
> +#endif

You may get away with pre-storing r3 in RESULT(r1), I'll have to double
check on monday... the 32 bits syscall exit code is a bit scary :-)

I'm pretty sure there's no need to allocate a stackframe. At worst,
there must be some ptregs field in the existing one that can be used.

That's all for today, I'll have another close look on monday.

Cheers,
Ben.

Re: [PATCH v2] powerpc: Add irqtrace support for 32-bit powerpc

[Dale Farnsworth]

On Sat, Apr 05, 2008 at 09:07:31AM +1100, Benjamin Herrenschmidt wrote:
> > diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
> > index 69a91bd..bd3ce0f 100644
> > --- a/arch/powerpc/kernel/entry_32.S
> > +++ b/arch/powerpc/kernel/entry_32.S
> > @@ -144,6 +144,47 @@ transfer_to_handler:
> >  	.globl transfer_to_handler_cont
> >  transfer_to_handler_cont:
> >  3:
> > +#ifdef CONFIG_TRACE_IRQFLAGS
> > +	lis	r11,reenable_mmu@h
> > +	ori	r11,r11,reenable_mmu@l
> > +	mtspr	SPRN_SRR0,r11
> > +	mtspr	SPRN_SRR1,r10
> > +	SYNC
> > +	RFI
> 
> I don't think we need that on 4xx/BookE when using AS0 (that is also
> true of the existing transfer_to_handler_cont, could be improved there.

Right, it's not needed on 4xx/BookE, but I didn't think it worth
optimizing at this point, since it will split the code into 4xx/BookE
and classic versions. Let's get it working solid first.

> > +reenable_mmu:				/* re-enable mmu so we can */
> > +	mflr	r9			/* call C code, if necessary */
> > +	mfmsr	r10
> > +	lwz	r11,_MSR(r1)
> > +	xor	r10,r10,r11
> > +	andi.	r10,r10,MSR_EE		/* Did EE change? */
> > +	beq	1f
> > +	stwu	r1,-48(r1)		/* Yes, it must have been cleared */
> > +	stw	r9,52(r1)
> > +	stw	r0,16(r1)
> > +	stw	r3,20(r1)
> > +	stw	r4,24(r1)
> > +	stw	r5,28(r1)
> > +	stw	r6,32(r1)
> > +	stw	r7,36(r1)
> > +	stw	r8,40(r1)
> > +	bl	trace_hardirqs_off
> > +	lwz	r0,16(r1)
> > +	lwz	r3,20(r1)
> > +	lwz	r4,24(r1)
> > +	lwz	r5,28(r1)
> > +	lwz	r6,32(r1)
> > +	lwz	r7,36(r1)
> > +	lwz	r8,40(r1)
> > +	lwz	r9,52(r1)
> > +	addi	r1,r1,48
> 
> Why do yo save all the volatile regs ? They should have been saved on
> the stack already by the exception prolog (the ptregs on the stack).

That's what I originally thought and had in my first version.
However, in the BookE case, we must save at least r3, r4, and r5.
(See data_access: in head_fsl_booke.S.)  It isn't clear what the
rules are, and I didn't want to set a trap for when a handler is
added that uses a fourth argument.

If you think it's worth it, I could test a version that saves
r3, r4, and r5 and restores the others from ptregs.

> Also, only the system call really cares about -restoring- them. Maybe
> you could stick that in an ifdef CONFIG_TRACE_IRQFLAGS section in
> DoSyscall pulling them back off the ptregs in the stackframe. 

Another optimization that I'm not convinced is worth the trouble
for this tracing code.  I'll try to take a look at it though.
As you say below, it's scary code.

> > +	tovirt(r9,r9)
> > +	lwz	r11,0(r9)		/* virtual address of handler */
> > +	lwz	r9,4(r9)		/* where to go when done */
> > +	mtctr	r11
> > +	mtlr	r9
> > +	bctr				/* jump to handler */
> > +#else /* CONFIG_TRACE_IRQFLAGS */
> >  	mflr	r9
> >  	lwz	r11,0(r9)		/* virtual address of handler */
> >  	lwz	r9,4(r9)		/* where to go when done */
> > @@ -152,6 +193,7 @@ transfer_to_handler_cont:
> >  	mtlr	r9
> >  	SYNC
> >  	RFI				/* jump to handler, enable MMU */
> > +#endif /* CONFIG_TRACE_IRQFLAGS */
> >  
> >  #ifdef CONFIG_6xx
> >  4:	rlwinm	r12,r12,0,~_TLF_NAPPING
> > @@ -220,12 +262,20 @@ ret_from_syscall:
> >  #ifdef SHOW_SYSCALLS
> >  	bl	do_show_syscall_exit
> >  #endif
> > -	mr	r6,r3
> >  	rlwinm	r12,r1,0,0,(31-THREAD_SHIFT)	/* current_thread_info() */
> >  	/* disable interrupts so current_thread_info()->flags can't change */
> >  	LOAD_MSR_KERNEL(r10,MSR_KERNEL)	/* doesn't include MSR_EE */
> >  	SYNC
> >  	MTMSRD(r10)
> > +#ifdef CONFIG_TRACE_IRQFLAGS
> > +	stwu	r1,-16(r1)
> > +	stw	r3,12(r1)
> > +	bl      trace_hardirqs_off
> > +	lwz	r3,12(r1)
> > +	addi	r1,r1,16
> > +	LOAD_MSR_KERNEL(r10,MSR_KERNEL)
> > +#endif
> 
> You may get away with pre-storing r3 in RESULT(r1), I'll have to double
> check on monday... the 32 bits syscall exit code is a bit scary :-)

It sure is.  :-)

> I'm pretty sure there's no need to allocate a stackframe. At worst,
> there must be some ptregs field in the existing one that can be used.
> 
> That's all for today, I'll have another close look on monday.

Thanks.

-Dale

Re: [PATCH v2] powerpc: Add irqtrace support for 32-bit powerpc

[Benjamin Herrenschmidt]

> Right, it's not needed on 4xx/BookE, but I didn't think it worth
> optimizing at this point, since it will split the code into 4xx/BookE
> and classic versions. Let's get it working solid first.

Yup, it's just that I spotted it while reading the code.

> That's what I originally thought and had in my first version.
> However, in the BookE case, we must save at least r3, r4, and r5.
> (See data_access: in head_fsl_booke.S.)  It isn't clear what the
> rules are, and I didn't want to set a trap for when a handler is
> added that uses a fourth argument.

Ok, this definitely is worth some rework around the edges. For now, I
suppose keeping it stable will do.

> If you think it's worth it, I could test a version that saves
> r3, r4, and r5 and restores the others from ptregs.

Don't bother for now. I'll see if we can do things differently later.

> > Also, only the system call really cares about -restoring- them. Maybe
> > you could stick that in an ifdef CONFIG_TRACE_IRQFLAGS section in
> > DoSyscall pulling them back off the ptregs in the stackframe. 
> 
> Another optimization that I'm not convinced is worth the trouble
> for this tracing code.  I'll try to take a look at it though.
> As you say below, it's scary code.

Yup. The RESTOREALL case doesn't write the result to the PT_REGS but I'm
not yet sure if that's a big issue to do it regardless or not.

Ben.

Re: [PATCH v2] powerpc: Add irqtrace support for 32-bit powerpc

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 462 bytes --]


> This version takes care to save and restore *all* the volatile registers
> around the call to trace_hardirqs_off in transfer_to_handler, and should
> fix the anomalies seen by Johannes Berg.  I've tested on 7447- and 8548-
> based boards.

Still has problems on my machine. Maybe to do with non-maskable PMU
interrupts? (saw a patch about something like that float by, don't
really have any idea whether it's related to this work or not)

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH v2] powerpc: Add irqtrace support for 32-bit powerpc

[Benjamin Herrenschmidt]

On Sat, 2008-04-05 at 01:31 +0200, Johannes Berg wrote:
> > This version takes care to save and restore *all* the volatile registers
> > around the call to trace_hardirqs_off in transfer_to_handler, and should
> > fix the anomalies seen by Johannes Berg.  I've tested on 7447- and 8548-
> > based boards.
> 
> Still has problems on my machine. Maybe to do with non-maskable PMU
> interrupts? (saw a patch about something like that float by, don't
> really have any idea whether it's related to this work or not)

Or more likely he missed some clobber somewhere, it took me a while to
get that right on ppc64 (well... hopefully it -is- right by now :-)

Ben.

Re: [PATCH v2] powerpc: Add irqtrace support for 32-bit powerpc

[Dale Farnsworth]

On Sat, Apr 05, 2008 at 04:14:56PM +1100, Benjamin Herrenschmidt wrote:
> 
> On Sat, 2008-04-05 at 01:31 +0200, Johannes Berg wrote:
> > > This version takes care to save and restore *all* the volatile registers
> > > around the call to trace_hardirqs_off in transfer_to_handler, and should
> > > fix the anomalies seen by Johannes Berg.  I've tested on 7447- and 8548-
> > > based boards.
> > 
> > Still has problems on my machine. Maybe to do with non-maskable PMU
> > interrupts? (saw a patch about something like that float by, don't
> > really have any idea whether it's related to this work or not)
> 
> Or more likely he missed some clobber somewhere, it took me a while to
> get that right on ppc64 (well... hopefully it -is- right by now :-)

Yep, likely.  I'd appreciate help finding it.  I'm at a loss.  :-)

-Dale

Re: [PATCH v2] powerpc: Add irqtrace support for 32-bit powerpc

[Benjamin Herrenschmidt]

I think I found one:

 .../...

> -	mr	r6,r3
>  	rlwinm	r12,r1,0,0,(31-THREAD_SHIFT)	/* current_thread_info() */
>  	/* disable interrupts so current_thread_info()->flags can't change */
>  	LOAD_MSR_KERNEL(r10,MSR_KERNEL)	/* doesn't include MSR_EE */
>  	SYNC
>  	MTMSRD(r10)
> +#ifdef CONFIG_TRACE_IRQFLAGS
> +	stwu	r1,-16(r1)
> +	stw	r3,12(r1)
> +	bl      trace_hardirqs_off
> +	lwz	r3,12(r1)
> +	addi	r1,r1,16
> +	LOAD_MSR_KERNEL(r10,MSR_KERNEL)
> +#endif

Here, r12 is clobbered, though it's used two lines later:

> +	mr	r6,r3
>  	lwz	r9,TI_FLAGS(r12)

Here.

You can probably just move the rlwinm down as you moved the mr.

Note that I've been wondering wether we should attempt to trace all
those IRQ state change internally to the exception code. I've looked at
not doing it, which simplifies things a bit.

Unfortunately, that will make us occasionally trace redundant
enable/disable (which isn't a big problem per-se, just counters).

The idea is that I only kept the trace of disable in transfer_to_handler
and I modified the enable tracing in restore: moved it lower down, and
made it test for _MSR(r1):MSR_EE. I added a trace_irq_off just before
the preempt_schedule_irq() as well.

Anyway, let me know what you think.

Cheers,
Ben.

Re: [PATCH v2] powerpc: Add irqtrace support for 32-bit powerpc

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 456 bytes --]


> Note that I've been wondering wether we should attempt to trace all
> those IRQ state change internally to the exception code. I've looked at
> not doing it, which simplifies things a bit.
> 
> Unfortunately, that will make us occasionally trace redundant
> enable/disable (which isn't a big problem per-se, just counters).

We already have a huge number of redundant enable/disable, about 40% of
all events on both 32 and 64-bit.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH v2] powerpc: Add irqtrace support for 32-bit powerpc

[Dale Farnsworth]

On Mon, Apr 07, 2008 at 02:49:51PM +1000, Benjamin Herrenschmidt wrote:
> I think I found one:
> 
>  .../...
> 
> > -	mr	r6,r3
> >  	rlwinm	r12,r1,0,0,(31-THREAD_SHIFT)	/* current_thread_info() */
> >  	/* disable interrupts so current_thread_info()->flags can't change */
> >  	LOAD_MSR_KERNEL(r10,MSR_KERNEL)	/* doesn't include MSR_EE */
> >  	SYNC
> >  	MTMSRD(r10)
> > +#ifdef CONFIG_TRACE_IRQFLAGS
> > +	stwu	r1,-16(r1)
> > +	stw	r3,12(r1)
> > +	bl      trace_hardirqs_off
> > +	lwz	r3,12(r1)
> > +	addi	r1,r1,16
> > +	LOAD_MSR_KERNEL(r10,MSR_KERNEL)
> > +#endif
> 
> Here, r12 is clobbered, though it's used two lines later:

Ah yes. r12 is volatile, isn't it.  :-)  Good catch.

> > +	mr	r6,r3
> >  	lwz	r9,TI_FLAGS(r12)
> 
> Here.
> 
> You can probably just move the rlwinm down as you moved the mr.

I'll follow up with a revised patch that does exactly that.

> Note that I've been wondering wether we should attempt to trace all
> those IRQ state change internally to the exception code. I've looked at
> not doing it, which simplifies things a bit.
> 
> Unfortunately, that will make us occasionally trace redundant
> enable/disable (which isn't a big problem per-se, just counters).
> 
> The idea is that I only kept the trace of disable in transfer_to_handler
> and I modified the enable tracing in restore: moved it lower down, and
> made it test for _MSR(r1):MSR_EE. I added a trace_irq_off just before
> the preempt_schedule_irq() as well.
> 
> Anyway, let me know what you think.

The main purpose I did this code was to measure
interrupt-disabled intervals.  There, the redundant trace calls aren't
really an issue.

It all depends on how important you view the counters.  I think that
if we're going to go to the trouble to maintain counters, we should
make them accurate.  I admit that it took me quite a while to get the
counts correct. :-)  I think having an accurate measure of the redundant
interrupt disable/enables is useful.  And, after doing the code, I'd
prefer to see it stay.  :-)

Thanks,
-Dale

Re: [PATCH v2] powerpc: Add irqtrace support for 32-bit powerpc

[Dale Farnsworth]

On Mon, Apr 07, 2008 at 03:10:09PM +0200, Johannes Berg wrote:
> > Note that I've been wondering wether we should attempt to trace all
> > those IRQ state change internally to the exception code. I've looked at
> > not doing it, which simplifies things a bit.
> > 
> > Unfortunately, that will make us occasionally trace redundant
> > enable/disable (which isn't a big problem per-se, just counters).
> 
> We already have a huge number of redundant enable/disable, about 40% of
> all events on both 32 and 64-bit.

Yes, but there is a big difference between the large number of actual
redundant enables and disables we now have, and introducing miscounts
of them in the measurement code.  That we have so many might mean that
we shouldn't bother to count them at all, but I don't think it's an
excuse for not counting them accurately.

-Dale

[PATCH v3] powerpc: Add irqtrace support for 32-bit powerpc

[Dale Farnsworth]

Add the low level irq tracing hooks for 32-bit powerpc needed
to enable full lockdep functionality.

Dale Farnsworth <dale@farnsworth.org>
---
This version fixes the clobbering of r12 by the call to
trace_hardirqs_off() that was pointed out by BenH.

Johannes, I'd appreciate your trying this version if/when
you get the chance.

 arch/powerpc/Kconfig           |    1 -
 arch/powerpc/kernel/entry_32.S |   92 ++++++++++++++++++++++++++++++++++++++--
 arch/powerpc/kernel/setup_32.c |    2 +
 include/asm-powerpc/hw_irq.h   |   20 ++++----
 include/asm-powerpc/system.h   |    3 +-
 5 files changed, 101 insertions(+), 17 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 6dbf123..99d8e18 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -55,7 +55,6 @@ config STACKTRACE_SUPPORT
 
 config TRACE_IRQFLAGS_SUPPORT
 	bool
-	depends on PPC64
 	default y
 
 config LOCKDEP_SUPPORT
diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
index 69a91bd..ad30d38 100644
--- a/arch/powerpc/kernel/entry_32.S
+++ b/arch/powerpc/kernel/entry_32.S
@@ -144,6 +144,47 @@ transfer_to_handler:
 	.globl transfer_to_handler_cont
 transfer_to_handler_cont:
 3:
+#ifdef CONFIG_TRACE_IRQFLAGS
+	lis	r11,reenable_mmu@h
+	ori	r11,r11,reenable_mmu@l
+	mtspr	SPRN_SRR0,r11
+	mtspr	SPRN_SRR1,r10
+	SYNC
+	RFI
+reenable_mmu:				/* re-enable mmu so we can */
+	mflr	r9			/* call C code, if necessary */
+	mfmsr	r10
+	lwz	r11,_MSR(r1)
+	xor	r10,r10,r11
+	andi.	r10,r10,MSR_EE		/* Did EE change? */
+	beq	1f
+	stwu	r1,-48(r1)		/* Yes, it must have been cleared */
+	stw	r9,52(r1)
+	stw	r0,16(r1)
+	stw	r3,20(r1)
+	stw	r4,24(r1)
+	stw	r5,28(r1)
+	stw	r6,32(r1)
+	stw	r7,36(r1)
+	stw	r8,40(r1)
+	bl	trace_hardirqs_off
+	lwz	r0,16(r1)
+	lwz	r3,20(r1)
+	lwz	r4,24(r1)
+	lwz	r5,28(r1)
+	lwz	r6,32(r1)
+	lwz	r7,36(r1)
+	lwz	r8,40(r1)
+	lwz	r9,52(r1)
+	addi	r1,r1,48
+1:
+	tovirt(r9,r9)
+	lwz	r11,0(r9)		/* virtual address of handler */
+	lwz	r9,4(r9)		/* where to go when done */
+	mtctr	r11
+	mtlr	r9
+	bctr				/* jump to handler */
+#else /* CONFIG_TRACE_IRQFLAGS */
 	mflr	r9
 	lwz	r11,0(r9)		/* virtual address of handler */
 	lwz	r9,4(r9)		/* where to go when done */
@@ -152,6 +193,7 @@ transfer_to_handler_cont:
 	mtlr	r9
 	SYNC
 	RFI				/* jump to handler, enable MMU */
+#endif /* CONFIG_TRACE_IRQFLAGS */
 
 #ifdef CONFIG_6xx
 4:	rlwinm	r12,r12,0,~_TLF_NAPPING
@@ -220,12 +262,20 @@ ret_from_syscall:
 #ifdef SHOW_SYSCALLS
 	bl	do_show_syscall_exit
 #endif
-	mr	r6,r3
-	rlwinm	r12,r1,0,0,(31-THREAD_SHIFT)	/* current_thread_info() */
 	/* disable interrupts so current_thread_info()->flags can't change */
 	LOAD_MSR_KERNEL(r10,MSR_KERNEL)	/* doesn't include MSR_EE */
 	SYNC
 	MTMSRD(r10)
+#ifdef CONFIG_TRACE_IRQFLAGS
+	stwu	r1,-16(r1)
+	stw	r3,12(r1)
+	bl      trace_hardirqs_off
+	lwz	r3,12(r1)
+	addi	r1,r1,16
+	LOAD_MSR_KERNEL(r10,MSR_KERNEL)
+#endif
+	mr	r6,r3
+	rlwinm	r12,r1,0,0,(31-THREAD_SHIFT)	/* current_thread_info() */
 	lwz	r9,TI_FLAGS(r12)
 	li	r8,-_LAST_ERRNO
 	andi.	r0,r9,(_TIF_SYSCALL_T_OR_A|_TIF_SINGLESTEP|_TIF_USER_WORK_MASK|_TIF_PERSYSCALL_MASK)
@@ -237,6 +287,13 @@ ret_from_syscall:
 	oris	r11,r11,0x1000	/* Set SO bit in CR */
 	stw	r11,_CCR(r1)
 syscall_exit_cont:
+#ifdef CONFIG_TRACE_IRQFLAGS
+	stwu	r1,-16(r1)
+	stw	r3,12(r1)
+	bl      trace_hardirqs_on
+	lwz	r3,12(r1)
+	addi	r1,r1,16
+#endif
 #if defined(CONFIG_4xx) || defined(CONFIG_BOOKE)
 	/* If the process has its own DBCR0 value, load it up.  The single
 	   step bit tells us that dbcr0 should be loaded. */
@@ -337,7 +394,10 @@ syscall_exit_work:
 4:	/* Anything which requires enabling interrupts? */
 	andi.	r0,r9,(_TIF_SYSCALL_T_OR_A|_TIF_SINGLESTEP)
 	beq	ret_from_except
-
+#ifdef CONFIG_TRACE_IRQFLAGS
+	bl      trace_hardirqs_on
+	LOAD_MSR_KERNEL(r10,MSR_KERNEL)
+#endif
 	/* Re-enable interrupts */
 	ori	r10,r10,MSR_EE
 	SYNC
@@ -646,13 +706,22 @@ ret_from_except_full:
 
 	.globl	ret_from_except
 ret_from_except:
+#ifdef CONFIG_TRACE_IRQFLAGS
+	mfmsr	r3
+#endif
 	/* Hard-disable interrupts so that current_thread_info()->flags
 	 * can't change between when we test it and when we return
 	 * from the interrupt. */
 	LOAD_MSR_KERNEL(r10,MSR_KERNEL)
 	SYNC			/* Some chip revs have problems here... */
 	MTMSRD(r10)		/* disable interrupts */
-
+#ifdef CONFIG_TRACE_IRQFLAGS
+	andi.	r3,r3,MSR_EE
+	beq	1f
+	bl	trace_hardirqs_off
+	LOAD_MSR_KERNEL(r10,MSR_KERNEL)
+1:
+#endif
 	lwz	r3,_MSR(r1)	/* Returning to user mode? */
 	andi.	r0,r3,MSR_PR
 	beq	resume_kernel
@@ -709,6 +778,9 @@ restore:
 	stw	r6,icache_44x_need_flush@l(r4)
 1:
 #endif  /* CONFIG_44x */
+#ifdef CONFIG_TRACE_IRQFLAGS
+	bl	trace_hardirqs_on
+#endif
 	lwz	r0,GPR0(r1)
 	lwz	r2,GPR2(r1)
 	REST_4GPRS(3, r1)
@@ -900,6 +972,10 @@ do_work:			/* r10 contains MSR_KERNEL here */
 	beq	do_user_signal
 
 do_resched:			/* r10 contains MSR_KERNEL here */
+#ifdef CONFIG_TRACE_IRQFLAGS
+	bl	trace_hardirqs_on
+	LOAD_MSR_KERNEL(r10,MSR_KERNEL)
+#endif
 	ori	r10,r10,MSR_EE
 	SYNC
 	MTMSRD(r10)		/* hard-enable interrupts */
@@ -908,6 +984,10 @@ recheck:
 	LOAD_MSR_KERNEL(r10,MSR_KERNEL)
 	SYNC
 	MTMSRD(r10)		/* disable interrupts */
+#ifdef CONFIG_TRACE_IRQFLAGS
+	bl	trace_hardirqs_off
+	LOAD_MSR_KERNEL(r10,MSR_KERNEL)
+#endif
 	rlwinm	r9,r1,0,0,(31-THREAD_SHIFT)
 	lwz	r9,TI_FLAGS(r9)
 	andi.	r0,r9,_TIF_NEED_RESCHED
@@ -915,6 +995,10 @@ recheck:
 	andi.	r0,r9,_TIF_SIGPENDING|_TIF_RESTORE_SIGMASK
 	beq	restore_user
 do_user_signal:			/* r10 contains MSR_KERNEL here */
+#ifdef CONFIG_TRACE_IRQFLAGS
+	bl	trace_hardirqs_on
+	LOAD_MSR_KERNEL(r10,MSR_KERNEL)
+#endif
 	ori	r10,r10,MSR_EE
 	SYNC
 	MTMSRD(r10)		/* hard-enable interrupts */
diff --git a/arch/powerpc/kernel/setup_32.c b/arch/powerpc/kernel/setup_32.c
index cd870a8..725dd18 100644
--- a/arch/powerpc/kernel/setup_32.c
+++ b/arch/powerpc/kernel/setup_32.c
@@ -120,6 +120,8 @@ unsigned long __init early_init(unsigned long dt_ptr)
  */
 void __init machine_init(unsigned long dt_ptr, unsigned long phys)
 {
+	lockdep_init();
+
 	/* Enable early debugging if any specified (see udbg.h) */
 	udbg_early_init();
 
diff --git a/include/asm-powerpc/hw_irq.h b/include/asm-powerpc/hw_irq.h
index ad8c9f7..a5f347a 100644
--- a/include/asm-powerpc/hw_irq.h
+++ b/include/asm-powerpc/hw_irq.h
@@ -63,13 +63,13 @@ extern void iseries_handle_interrupts(void);
 
 #if defined(CONFIG_BOOKE)
 #define SET_MSR_EE(x)	mtmsr(x)
-#define local_irq_restore(flags)	__asm__ __volatile__("wrtee %0" : : "r" (flags) : "memory")
+#define raw_local_irq_restore(flags)	__asm__ __volatile__("wrtee %0" : : "r" (flags) : "memory")
 #else
 #define SET_MSR_EE(x)	mtmsr(x)
-#define local_irq_restore(flags)	mtmsr(flags)
+#define raw_local_irq_restore(flags)	mtmsr(flags)
 #endif
 
-static inline void local_irq_disable(void)
+static inline void raw_local_irq_disable(void)
 {
 #ifdef CONFIG_BOOKE
 	__asm__ __volatile__("wrteei 0": : :"memory");
@@ -81,7 +81,7 @@ static inline void local_irq_disable(void)
 #endif
 }
 
-static inline void local_irq_enable(void)
+static inline void raw_local_irq_enable(void)
 {
 #ifdef CONFIG_BOOKE
 	__asm__ __volatile__("wrteei 1": : :"memory");
@@ -93,7 +93,7 @@ static inline void local_irq_enable(void)
 #endif
 }
 
-static inline void local_irq_save_ptr(unsigned long *flags)
+static inline void raw_local_irq_save_ptr(unsigned long *flags)
 {
 	unsigned long msr;
 	msr = mfmsr();
@@ -106,12 +106,12 @@ static inline void local_irq_save_ptr(unsigned long *flags)
 	__asm__ __volatile__("": : :"memory");
 }
 
-#define local_save_flags(flags)	((flags) = mfmsr())
-#define local_irq_save(flags)	local_irq_save_ptr(&flags)
-#define irqs_disabled()		((mfmsr() & MSR_EE) == 0)
+#define raw_local_save_flags(flags)	((flags) = mfmsr())
+#define raw_local_irq_save(flags)	raw_local_irq_save_ptr(&flags)
+#define raw_irqs_disabled()		((mfmsr() & MSR_EE) == 0)
+#define raw_irqs_disabled_flags(flags)	(((flags) & MSR_EE) == 0)
 
-#define hard_irq_enable()	local_irq_enable()
-#define hard_irq_disable()	local_irq_disable()
+#define hard_irq_disable()		raw_local_irq_disable()
 
 #endif /* CONFIG_PPC64 */
 
diff --git a/include/asm-powerpc/system.h b/include/asm-powerpc/system.h
index 29552ff..e040a48 100644
--- a/include/asm-powerpc/system.h
+++ b/include/asm-powerpc/system.h
@@ -5,8 +5,7 @@
 #define _ASM_POWERPC_SYSTEM_H
 
 #include <linux/kernel.h>
-
-#include <asm/hw_irq.h>
+#include <linux/irqflags.h>
 
 /*
  * Memory barrier.
-- 
1.5.4

Re: [PATCH v3] powerpc: Add irqtrace support for 32-bit powerpc

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 581 bytes --]


On Mon, 2008-04-07 at 10:14 -0700, Dale Farnsworth wrote:
> Add the low level irq tracing hooks for 32-bit powerpc needed
> to enable full lockdep functionality.
> 
> Dale Farnsworth <dale@farnsworth.org>
> ---
> This version fixes the clobbering of r12 by the call to
> trace_hardirqs_off() that was pointed out by BenH.
> 
> Johannes, I'd appreciate your trying this version if/when
> you get the chance.

Thanks Dale, this version seems to work. I'll stress it a bit more
later, but so far it has survived *much* longer than both previous
versions.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH v3] powerpc: Add irqtrace support for 32-bit powerpc

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 586 bytes --]


> > This version fixes the clobbering of r12 by the call to
> > trace_hardirqs_off() that was pointed out by BenH.
> > 
> > Johannes, I'd appreciate your trying this version if/when
> > you get the chance.
> 
> Thanks Dale, this version seems to work. I'll stress it a bit more
> later, but so far it has survived *much* longer than both previous
> versions.

Hmm. Bad news. I got a crash in console_callback() again where
apparently r28 had a bogus value. That, however, doesn't make sense, so
maybe that register value was calculated from another register.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH v3] powerpc: Add irqtrace support for 32-bit powerpc

[Kumar Gala]

On Apr 8, 2008, at 4:36 PM, Johannes Berg wrote:
>
>>> This version fixes the clobbering of r12 by the call to
>>> trace_hardirqs_off() that was pointed out by BenH.
>>>
>>> Johannes, I'd appreciate your trying this version if/when
>>> you get the chance.
>>
>> Thanks Dale, this version seems to work. I'll stress it a bit more
>> later, but so far it has survived *much* longer than both previous
>> versions.
>
> Hmm. Bad news. I got a crash in console_callback() again where
> apparently r28 had a bogus value. That, however, doesn't make sense,  
> so
> maybe that register value was calculated from another register.

did we ever figure this out?

- k

Re: [PATCH v3] powerpc: Add irqtrace support for 32-bit powerpc

[Benjamin Herrenschmidt]

On Tue, 2008-04-08 at 23:36 +0200, Johannes Berg wrote:
> > > This version fixes the clobbering of r12 by the call to
> > > trace_hardirqs_off() that was pointed out by BenH.
> > > 
> > > Johannes, I'd appreciate your trying this version if/when
> > > you get the chance.
> > 
> > Thanks Dale, this version seems to work. I'll stress it a bit more
> > later, but so far it has survived *much* longer than both previous
> > versions.
> 
> Hmm. Bad news. I got a crash in console_callback() again where
> apparently r28 had a bogus value. That, however, doesn't make sense, so
> maybe that register value was calculated from another register.

Possibly. How hard was it to reproduce ? Could it be a bug unrelated to
lockdep ?

Ben.

Re: [PATCH v3] powerpc: Add irqtrace support for 32-bit powerpc

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 529 bytes --]

On Wed, 2008-04-23 at 14:40 +1000, Benjamin Herrenschmidt wrote:

> > Hmm. Bad news. I got a crash in console_callback() again where
> > apparently r28 had a bogus value. That, however, doesn't make sense, so
> > maybe that register value was calculated from another register.
> 
> Possibly. How hard was it to reproduce ? 

It just happened after maybe half an hour or an hour uptime.

> Could it be a bug unrelated to lockdep ?

Unlikely, I've been running the same kernel for days without that patch.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]