[PATCH] ret_from_irq adjustment

[PATCH] ret_from_irq adjustment

[Atsushi Nemoto]

Make sure that RA on top of interrupt stack is an address of
ret_from_irq, so that dump_stack etc. can trace info interrupted
context.

Also this patch fixes except_vec_vi_handler and __smtc_ipi_vector
which seems broken.

Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>

 dec/int-handler.S |   11 ++++-------
 kernel/entry.S    |   14 +++++++++-----
 kernel/genex.S    |    8 +++-----
 kernel/smtc-asm.S |    9 +++------
 4 files changed, 19 insertions(+), 23 deletions(-)

diff --git a/arch/mips/dec/int-handler.S b/arch/mips/dec/int-handler.S
index 55d60d5..31dd47d 100644
--- a/arch/mips/dec/int-handler.S
+++ b/arch/mips/dec/int-handler.S
@@ -266,10 +266,8 @@ #endif
 handle_it:
 		LONG_L	s0, TI_REGS($28)
 		LONG_S	sp, TI_REGS($28)
-		jal	do_IRQ
-		LONG_S	s0, TI_REGS($28)
-
-		j	ret_from_irq
+		PTR_LA	ra, ret_from_irq
+		j	do_IRQ
 		 nop
 
 #ifdef CONFIG_32BIT
@@ -279,9 +277,8 @@ fpu:
 #endif
 
 spurious:
-		jal	spurious_interrupt
-		 nop
-		j	ret_from_irq
+		PTR_LA	ra, _ret_from_irq
+		j	spurious_interrupt
 		 nop
 		END(plat_irq_dispatch)
 
diff --git a/arch/mips/kernel/entry.S b/arch/mips/kernel/entry.S
index e93e43e..417c08a 100644
--- a/arch/mips/kernel/entry.S
+++ b/arch/mips/kernel/entry.S
@@ -20,10 +20,7 @@ #ifdef CONFIG_MIPS_MT_SMTC
 #include <asm/mipsmtregs.h>
 #endif
 
-#ifdef CONFIG_PREEMPT
-	.macro	preempt_stop
-	.endm
-#else
+#ifndef CONFIG_PREEMPT
 	.macro	preempt_stop
 	local_irq_disable
 	.endm
@@ -32,9 +29,16 @@ #endif
 
 	.text
 	.align	5
+FEXPORT(ret_from_irq)
+	LONG_S	s0, TI_REGS($28)
+#ifdef CONFIG_PREEMPT
+FEXPORT(ret_from_exception)
+#else
+	b	_ret_from_irq
 FEXPORT(ret_from_exception)
 	preempt_stop
-FEXPORT(ret_from_irq)
+#endif
+FEXPORT(_ret_from_irq)
 	LONG_L	t0, PT_STATUS(sp)		# returning to kernel mode?
 	andi	t0, t0, KU_USER
 	beqz	t0, resume_kernel
diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
index 50ed772..5baca16 100644
--- a/arch/mips/kernel/genex.S
+++ b/arch/mips/kernel/genex.S
@@ -133,9 +133,8 @@ NESTED(handle_int, PT_SIZE, sp)
 
 	LONG_L	s0, TI_REGS($28)
 	LONG_S	sp, TI_REGS($28)
-	jal	plat_irq_dispatch
-	LONG_S	s0, TI_REGS($28)
-	j	ret_from_irq
+	PTR_LA	ra, ret_from_irq
+	j	plat_irq_dispatch
 	END(handle_int)
 
 	__INIT
@@ -224,9 +223,8 @@ #endif /* CONFIG_MIPS_MT_SMTC */
 
 	LONG_L	s0, TI_REGS($28)
 	LONG_S	sp, TI_REGS($28)
-	jalr	v0
-	LONG_S	s0, TI_REGS($28)
 	PTR_LA	ra, ret_from_irq
+	jr	v0
 	END(except_vec_vi_handler)
 
 /*
diff --git a/arch/mips/kernel/smtc-asm.S b/arch/mips/kernel/smtc-asm.S
index 76cb31d..1cb9441 100644
--- a/arch/mips/kernel/smtc-asm.S
+++ b/arch/mips/kernel/smtc-asm.S
@@ -97,15 +97,12 @@ FEXPORT(__smtc_ipi_vector)
 	SAVE_ALL
 	CLI
 	TRACE_IRQS_OFF
-	move	a0,sp
 	/* Function to be invoked passed stack pad slot 5 */
 	lw	t0,PT_PADSLOT5(sp)
 	/* Argument from sender passed in stack pad slot 4 */
-	lw	a1,PT_PADSLOT4(sp)
-	jalr	t0
-	nop
-	j	ret_from_irq
-	nop
+	lw	a0,PT_PADSLOT4(sp)
+	PTR_LA	ra, _ret_from_irq
+	jr	t0
 
 /*
  * Called from idle loop to provoke processing of queued IPIs

Re: [PATCH] ret_from_irq adjustment

[Kevin D. Kissell]

While setting up ra "by hand" and transferring control via the jr
is a reasonable optimization, you're otherwise breaking things for SMTC.
While the comments are misleading (they accurately described an earlier
version of the code), the function being called here is ipi_decode(), which
 needs a pt_regs * in the first argument (hence the copy of the sp), and 
the pointer to the IPI message descriptor in the second.

Do you have access to a 34K to test changes to SMTC?  I'd have
expected this one to have been pretty quickly fatal.

            Regards,

            Kevin K.

> diff --git a/arch/mips/kernel/smtc-asm.S b/arch/mips/kernel/smtc-asm.S
> index 76cb31d..1cb9441 100644
> --- a/arch/mips/kernel/smtc-asm.S
> +++ b/arch/mips/kernel/smtc-asm.S
> @@ -97,15 +97,12 @@ FEXPORT(__smtc_ipi_vector)
>   SAVE_ALL
>   CLI
>   TRACE_IRQS_OFF
> - move a0,sp
>   /* Function to be invoked passed stack pad slot 5 */
>   lw t0,PT_PADSLOT5(sp)
>   /* Argument from sender passed in stack pad slot 4 */
> - lw a1,PT_PADSLOT4(sp)
> - jalr t0
> - nop
> - j ret_from_irq
> - nop
> + lw a0,PT_PADSLOT4(sp)
> + PTR_LA ra, _ret_from_irq
> + jr t0
>  
>  /*
>   * Called from idle loop to provoke processing of queued IPIs
> 
> 

Re: [PATCH] ret_from_irq adjustment

[Kevin D. Kissell]

While setting up ra "by hand" and transferring control via the jr
is a reasonable optimization, you're otherwise breaking things for SMTC.
While the comments are misleading (they accurately described an earlier
version of the code), the function being called here is ipi_decode(), which
 needs a pt_regs * in the first argument (hence the copy of the sp), and 
the pointer to the IPI message descriptor in the second.

Do you have access to a 34K to test changes to SMTC?  I'd have
expected this one to have been pretty quickly fatal.

            Regards,

            Kevin K.

> diff --git a/arch/mips/kernel/smtc-asm.S b/arch/mips/kernel/smtc-asm.S
> index 76cb31d..1cb9441 100644
> --- a/arch/mips/kernel/smtc-asm.S
> +++ b/arch/mips/kernel/smtc-asm.S
> @@ -97,15 +97,12 @@ FEXPORT(__smtc_ipi_vector)
>   SAVE_ALL
>   CLI
>   TRACE_IRQS_OFF
> - move a0,sp
>   /* Function to be invoked passed stack pad slot 5 */
>   lw t0,PT_PADSLOT5(sp)
>   /* Argument from sender passed in stack pad slot 4 */
> - lw a1,PT_PADSLOT4(sp)
> - jalr t0
> - nop
> - j ret_from_irq
> - nop
> + lw a0,PT_PADSLOT4(sp)
> + PTR_LA ra, _ret_from_irq
> + jr t0
>  
>  /*
>   * Called from idle loop to provoke processing of queued IPIs
> 
> 

Re: [PATCH] ret_from_irq adjustment

[Ralf Baechle]

On Sun, Oct 08, 2006 at 08:26:44PM +0200, Kevin D. Kissell wrote:

> While setting up ra "by hand" and transferring control via the jr
> is a reasonable optimization, you're otherwise breaking things for SMTC.
> While the comments are misleading (they accurately described an earlier
> version of the code), the function being called here is ipi_decode(), which
>  needs a pt_regs * in the first argument (hence the copy of the sp), and 
> the pointer to the IPI message descriptor in the second.
> 
> Do you have access to a 34K to test changes to SMTC?  I'd have
> expected this one to have been pretty quickly fatal.

The shakeup of the code by the recent series of pt_regs related cleanups
is pretty massive.  As of last night I only had uniprocessor support
working again.  VSMP and SMTC were broken; actual multi-core CPU not
tested yet.

  Ralf

Re: [PATCH] ret_from_irq adjustment

[Kevin D. Kissell]

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

I attach a text file (inline cut-and-paste produces Windows whitespace
which apparently is unacceptable) of a patch which (a) implements the
ret_from_irq optimization that Atsushi wanted to do to the SMTC code, 
only without breaking it.  I also reorganized and re-commented the code to
be easier to maintain in the future, and in an unrelated matter (b) fixes
a bug in arch/mips/kernel/smtc.c when there are 64 or more TLB
entry pairs on a 34K core.  This TLB patch has been in the internal
MIPS repository forever, but for some reason has never made it
out onto linux-mips.org.

The resulting kernel boots and runs (with a 64-entry TLB).

Note that these patches are relative to the 2.6.17 semi-stable
tree, and not the latest hackfest, so the renaming of ret_from_irq
to _ret_from_irq had not been done, and is not reflected in the patch.

            Regards,

            Kevin K.

----- Original Message ----- 
From: "Kevin D. Kissell" <KevinK@mips.com>
To: <linux-mips@linux-mips.org>; "Atsushi Nemoto" <anemo@mba.ocn.ne.jp>
Cc: <ralf@linux-mips.org>
Sent: Sunday, October 08, 2006 8:26 PM
Subject: Re: [PATCH] ret_from_irq adjustment


> While setting up ra "by hand" and transferring control via the jr
> is a reasonable optimization, you're otherwise breaking things for SMTC.
> While the comments are misleading (they accurately described an earlier
> version of the code), the function being called here is ipi_decode(), which
>  needs a pt_regs * in the first argument (hence the copy of the sp), and 
> the pointer to the IPI message descriptor in the second.
> 
> Do you have access to a 34K to test changes to SMTC?  I'd have
> expected this one to have been pretty quickly fatal.
> 
>             Regards,
> 
>             Kevin K.
> 
> > diff --git a/arch/mips/kernel/smtc-asm.S b/arch/mips/kernel/smtc-asm.S
> > index 76cb31d..1cb9441 100644
> > --- a/arch/mips/kernel/smtc-asm.S
> > +++ b/arch/mips/kernel/smtc-asm.S
> > @@ -97,15 +97,12 @@ FEXPORT(__smtc_ipi_vector)
> >   SAVE_ALL
> >   CLI
> >   TRACE_IRQS_OFF
> > - move a0,sp
> >   /* Function to be invoked passed stack pad slot 5 */
> >   lw t0,PT_PADSLOT5(sp)
> >   /* Argument from sender passed in stack pad slot 4 */
> > - lw a1,PT_PADSLOT4(sp)
> > - jalr t0
> > - nop
> > - j ret_from_irq
> > - nop
> > + lw a0,PT_PADSLOT4(sp)
> > + PTR_LA ra, _ret_from_irq
> > + jr t0
> >  
> >  /*
> >   * Called from idle loop to provoke processing of queued IPIs
> > 
> > 
> 
> 

[-- Attachment #2: smtcpatch.gitdiff --]
[-- Type: application/octet-stream, Size: 1269 bytes --]

diff --git a/arch/mips/kernel/smtc-asm.S b/arch/mips/kernel/smtc-asm.S
index 72c6d98..a1709de 100644
--- a/arch/mips/kernel/smtc-asm.S
+++ b/arch/mips/kernel/smtc-asm.S
@@ -96,15 +96,14 @@ FEXPORT(__smtc_ipi_vector)
 	/* Save all will redundantly recompute the SP, but use it for now */
 	SAVE_ALL
 	CLI
-	move	a0,sp
 	/* Function to be invoked passed stack pad slot 5 */
 	lw	t0,PT_PADSLOT5(sp)
-	/* Argument from sender passed in stack pad slot 4 */
+	/* First argument is pointer to pt_regs on kernel stack */
+	move	a0,sp
+	/* Additional argument from sender passed in stack pad slot 4 */
 	lw	a1,PT_PADSLOT4(sp)
-	jalr	t0
-	nop
-	j	ret_from_irq
-	nop
+	PTR_LA	ra,ret_from_irq
+	jr	t0
 
 /*
  * Called from idle loop to provoke processing of queued IPIs
diff --git a/arch/mips/kernel/smtc.c b/arch/mips/kernel/smtc.c
index 2e8e52c..1657d15 100644
--- a/arch/mips/kernel/smtc.c
+++ b/arch/mips/kernel/smtc.c
@@ -269,7 +269,8 @@ void smtc_configure_tlb(void)
 		 * of their initialization in smtc_cpu_setup().
 		 */
 
-		tlbsiz = tlbsiz & 0x3f;	/* MIPS32 limits TLB indices to 64 */
+		/* MIPS32 limits TLB indices to 64 */
+		if (tlbsiz > 64) tlbsiz = 64;
 		cpu_data[0].tlbsize = tlbsiz;
 		smtc_status |= SMTC_TLB_SHARED;
 

Re: [PATCH] ret_from_irq adjustment

[Kevin D. Kissell]

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

I attach a text file (inline cut-and-paste produces Windows whitespace
which apparently is unacceptable) of a patch which (a) implements the
ret_from_irq optimization that Atsushi wanted to do to the SMTC code, 
only without breaking it.  I also reorganized and re-commented the code to
be easier to maintain in the future, and in an unrelated matter (b) fixes
a bug in arch/mips/kernel/smtc.c when there are 64 or more TLB
entry pairs on a 34K core.  This TLB patch has been in the internal
MIPS repository forever, but for some reason has never made it
out onto linux-mips.org.

The resulting kernel boots and runs (with a 64-entry TLB).

Note that these patches are relative to the 2.6.17 semi-stable
tree, and not the latest hackfest, so the renaming of ret_from_irq
to _ret_from_irq had not been done, and is not reflected in the patch.

            Regards,

            Kevin K.

----- Original Message ----- 
From: "Kevin D. Kissell" <KevinK@mips.com>
To: <linux-mips@linux-mips.org>; "Atsushi Nemoto" <anemo@mba.ocn.ne.jp>
Cc: <ralf@linux-mips.org>
Sent: Sunday, October 08, 2006 8:26 PM
Subject: Re: [PATCH] ret_from_irq adjustment


> While setting up ra "by hand" and transferring control via the jr
> is a reasonable optimization, you're otherwise breaking things for SMTC.
> While the comments are misleading (they accurately described an earlier
> version of the code), the function being called here is ipi_decode(), which
>  needs a pt_regs * in the first argument (hence the copy of the sp), and 
> the pointer to the IPI message descriptor in the second.
> 
> Do you have access to a 34K to test changes to SMTC?  I'd have
> expected this one to have been pretty quickly fatal.
> 
>             Regards,
> 
>             Kevin K.
> 
> > diff --git a/arch/mips/kernel/smtc-asm.S b/arch/mips/kernel/smtc-asm.S
> > index 76cb31d..1cb9441 100644
> > --- a/arch/mips/kernel/smtc-asm.S
> > +++ b/arch/mips/kernel/smtc-asm.S
> > @@ -97,15 +97,12 @@ FEXPORT(__smtc_ipi_vector)
> >   SAVE_ALL
> >   CLI
> >   TRACE_IRQS_OFF
> > - move a0,sp
> >   /* Function to be invoked passed stack pad slot 5 */
> >   lw t0,PT_PADSLOT5(sp)
> >   /* Argument from sender passed in stack pad slot 4 */
> > - lw a1,PT_PADSLOT4(sp)
> > - jalr t0
> > - nop
> > - j ret_from_irq
> > - nop
> > + lw a0,PT_PADSLOT4(sp)
> > + PTR_LA ra, _ret_from_irq
> > + jr t0
> >  
> >  /*
> >   * Called from idle loop to provoke processing of queued IPIs
> > 
> > 
> 
> 

[-- Attachment #2: smtcpatch.gitdiff --]
[-- Type: application/octet-stream, Size: 1269 bytes --]

diff --git a/arch/mips/kernel/smtc-asm.S b/arch/mips/kernel/smtc-asm.S
index 72c6d98..a1709de 100644
--- a/arch/mips/kernel/smtc-asm.S
+++ b/arch/mips/kernel/smtc-asm.S
@@ -96,15 +96,14 @@ FEXPORT(__smtc_ipi_vector)
 	/* Save all will redundantly recompute the SP, but use it for now */
 	SAVE_ALL
 	CLI
-	move	a0,sp
 	/* Function to be invoked passed stack pad slot 5 */
 	lw	t0,PT_PADSLOT5(sp)
-	/* Argument from sender passed in stack pad slot 4 */
+	/* First argument is pointer to pt_regs on kernel stack */
+	move	a0,sp
+	/* Additional argument from sender passed in stack pad slot 4 */
 	lw	a1,PT_PADSLOT4(sp)
-	jalr	t0
-	nop
-	j	ret_from_irq
-	nop
+	PTR_LA	ra,ret_from_irq
+	jr	t0
 
 /*
  * Called from idle loop to provoke processing of queued IPIs
diff --git a/arch/mips/kernel/smtc.c b/arch/mips/kernel/smtc.c
index 2e8e52c..1657d15 100644
--- a/arch/mips/kernel/smtc.c
+++ b/arch/mips/kernel/smtc.c
@@ -269,7 +269,8 @@ void smtc_configure_tlb(void)
 		 * of their initialization in smtc_cpu_setup().
 		 */
 
-		tlbsiz = tlbsiz & 0x3f;	/* MIPS32 limits TLB indices to 64 */
+		/* MIPS32 limits TLB indices to 64 */
+		if (tlbsiz > 64) tlbsiz = 64;
 		cpu_data[0].tlbsize = tlbsiz;
 		smtc_status |= SMTC_TLB_SHARED;
 

Re: [PATCH] ret_from_irq adjustment

[Ralf Baechle]

On Sun, Oct 08, 2006 at 08:26:44PM +0200, Kevin D. Kissell wrote:

> While setting up ra "by hand" and transferring control via the jr
> is a reasonable optimization, you're otherwise breaking things for SMTC.
> While the comments are misleading (they accurately described an earlier
> version of the code), the function being called here is ipi_decode(), which
>  needs a pt_regs * in the first argument (hence the copy of the sp), and 
> the pointer to the IPI message descriptor in the second.
> 
> Do you have access to a 34K to test changes to SMTC?  I'd have
> expected this one to have been pretty quickly fatal.

Second reply after a closer look at the patch.

ipi_decode() has lost it's pt_regs argument like most of the interrupt
related functions, so Atushi's patch was right.  Any interrupt handler
that wants to get a pointer to the register frame can do so by calling
get_irq_regs().

The cleanup did actually work so well I'm tempted to use the same
strategy also for the CU and RI exception handlers which would make the
FPU exception handler look a whole lot more friendly.

So with Atsushi's patch applied VSMP and SMTC with only two TCs activated
are working again.  It still crashes with 5 TCs enabled:

Cpu 1
$ 0   : 00000000 18102000 00000000 8041ed44
$ 4   : 00000000 00000000 8041ec88 00000000
$ 8   : 00000000 18001c00 8010de78 80430000
$12   : 80420000 fffffffb ffffffff 0000000a
$16   : 00000000 00000001 8041ec04 8041ec08
$20   : 803b0000 8041ed40 80380000 18102000
$24   : 00000000 810c3b11
$28   : 810c2000 810c3b58 00000100 80108bdc
Hi    : 00000009
Lo    : fbe7d600
epc   : 80132b74 profile_tick+0x20/0xb4     Not tainted
ra    : 80108bdc local_timer_interrupt+0x10/0x30
Status: 1100a603    KERNEL EXL IE

  Ralf

Re: [PATCH] ret_from_irq adjustment

[Atsushi Nemoto]

On Mon, 9 Oct 2006 14:53:33 +0100, Ralf Baechle <ralf@linux-mips.org> wrote:
> ipi_decode() has lost it's pt_regs argument like most of the interrupt
> related functions, so Atushi's patch was right.  Any interrupt handler
> that wants to get a pointer to the register frame can do so by calling
> get_irq_regs().

Yes, excuse me for a terse description.

> So with Atsushi's patch applied VSMP and SMTC with only two TCs activated
> are working again.  It still crashes with 5 TCs enabled:
> 
> Cpu 1
> $ 0   : 00000000 18102000 00000000 8041ed44
> $ 4   : 00000000 00000000 8041ec88 00000000
> $ 8   : 00000000 18001c00 8010de78 80430000
> $12   : 80420000 fffffffb ffffffff 0000000a
> $16   : 00000000 00000001 8041ec04 8041ec08
> $20   : 803b0000 8041ed40 80380000 18102000
> $24   : 00000000 810c3b11
> $28   : 810c2000 810c3b58 00000100 80108bdc
> Hi    : 00000009
> Lo    : fbe7d600
> epc   : 80132b74 profile_tick+0x20/0xb4     Not tainted
> ra    : 80108bdc local_timer_interrupt+0x10/0x30
> Status: 1100a603    KERNEL EXL IE

Hmm, this would be because local_timer_interrupt was called from
ipi_decode().  Is this a proper fix?

diff --git a/arch/mips/kernel/smtc-asm.S b/arch/mips/kernel/smtc-asm.S
index 1cb9441..20938a4 100644
--- a/arch/mips/kernel/smtc-asm.S
+++ b/arch/mips/kernel/smtc-asm.S
@@ -101,7 +101,9 @@ FEXPORT(__smtc_ipi_vector)
 	lw	t0,PT_PADSLOT5(sp)
 	/* Argument from sender passed in stack pad slot 4 */
 	lw	a0,PT_PADSLOT4(sp)
-	PTR_LA	ra, _ret_from_irq
+	LONG_L	s0, TI_REGS($28)
+	LONG_S	sp, TI_REGS($28)
+	PTR_LA	ra, ret_from_irq
 	jr	t0
 
 /*