[PATCH] MIPS: Print correct PC in trace dump after NMI exception

[PATCH] MIPS: Print correct PC in trace dump after NMI exception

[Markos Chandras]

From: Leonid Yegoshin <Leonid.Yegoshin@imgtec.com>

An NMI exception delivered from YAMON delivers the PC in ErrorPC
instead of EPC. It's also necessary to clear the Status.BEV
bit for the page fault exception handler to work properly.

Signed-off-by: Leonid Yegoshin <Leonid.Yegoshin@imgtec.com>
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/kernel/genex.S | 15 ++++++++++++---
 arch/mips/kernel/traps.c | 13 +++++++++++--
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
index 31fa856..6a6bea2 100644
--- a/arch/mips/kernel/genex.S
+++ b/arch/mips/kernel/genex.S
@@ -374,12 +374,21 @@ NESTED(except_vec_nmi, 0, sp)
 NESTED(nmi_handler, PT_SIZE, sp)
 	.set	push
 	.set	noat
+	/*
+	 * Clear ERL - restore segment mapping
+	 * Clear BEV - required for page fault exception handler to work
+	 */
+	mfc0	k0, CP0_STATUS
+	ori     k0, k0, ST0_EXL
+	lui     k1, %hi(~ST0_BEV)
+	ori     k1, k1, %lo(~ST0_ERL)
+	and     k0, k0, k1
+	mtc0    k0, CP0_STATUS
+	ehb
 	SAVE_ALL
 	move	a0, sp
 	jal	nmi_exception_handler
-	RESTORE_ALL
-	.set	mips3
-	eret
+	/* nmi_exception_handler never returns */
 	.set	pop
 	END(nmi_handler)
 
diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
index 28d1a20..709c154 100644
--- a/arch/mips/kernel/traps.c
+++ b/arch/mips/kernel/traps.c
@@ -330,6 +330,7 @@ void show_regs(struct pt_regs *regs)
 void show_registers(struct pt_regs *regs)
 {
 	const int field = 2 * sizeof(unsigned long);
+	mm_segment_t old_fs = get_fs();
 
 	__show_regs(regs);
 	print_modules();
@@ -344,9 +345,13 @@ void show_registers(struct pt_regs *regs)
 			printk("*HwTLS: %0*lx\n", field, tls);
 	}
 
+	if (!user_mode(regs))
+		/* Necessary for getting the correct stack content */
+		set_fs(KERNEL_DS);
 	show_stacktrace(current, regs);
 	show_code((unsigned int __user *) regs->cp0_epc);
 	printk("\n");
+	set_fs(old_fs);
 }
 
 static int regs_to_trapnr(struct pt_regs *regs)
@@ -1506,10 +1511,14 @@ int register_nmi_notifier(struct notifier_block *nb)
 
 void __noreturn nmi_exception_handler(struct pt_regs *regs)
 {
+	char str[100];
+
 	raw_notifier_call_chain(&nmi_chain, 0, regs);
 	bust_spinlocks(1);
-	printk("NMI taken!!!!\n");
-	die("NMI", regs);
+	snprintf(str, 100, "CPU%d NMI taken, CP0_EPC=%lx\n",
+		 smp_processor_id(), regs->cp0_epc);
+	regs->cp0_epc = read_c0_errorepc();
+	die(str, regs);
 }
 
 #define VECTORSPACING 0x100	/* for EI/VI mode */
-- 
1.8.3.2

[PATCH] MIPS: Print correct PC in trace dump after NMI exception

[Markos Chandras]

From: Leonid Yegoshin <Leonid.Yegoshin@imgtec.com>

An NMI exception delivered from YAMON delivers the PC in ErrorPC
instead of EPC. It's also necessary to clear the Status.BEV
bit for the page fault exception handler to work properly.

Signed-off-by: Leonid Yegoshin <Leonid.Yegoshin@imgtec.com>
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/kernel/genex.S | 15 ++++++++++++---
 arch/mips/kernel/traps.c | 13 +++++++++++--
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
index 31fa856..6a6bea2 100644
--- a/arch/mips/kernel/genex.S
+++ b/arch/mips/kernel/genex.S
@@ -374,12 +374,21 @@ NESTED(except_vec_nmi, 0, sp)
 NESTED(nmi_handler, PT_SIZE, sp)
 	.set	push
 	.set	noat
+	/*
+	 * Clear ERL - restore segment mapping
+	 * Clear BEV - required for page fault exception handler to work
+	 */
+	mfc0	k0, CP0_STATUS
+	ori     k0, k0, ST0_EXL
+	lui     k1, %hi(~ST0_BEV)
+	ori     k1, k1, %lo(~ST0_ERL)
+	and     k0, k0, k1
+	mtc0    k0, CP0_STATUS
+	ehb
 	SAVE_ALL
 	move	a0, sp
 	jal	nmi_exception_handler
-	RESTORE_ALL
-	.set	mips3
-	eret
+	/* nmi_exception_handler never returns */
 	.set	pop
 	END(nmi_handler)
 
diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
index 28d1a20..709c154 100644
--- a/arch/mips/kernel/traps.c
+++ b/arch/mips/kernel/traps.c
@@ -330,6 +330,7 @@ void show_regs(struct pt_regs *regs)
 void show_registers(struct pt_regs *regs)
 {
 	const int field = 2 * sizeof(unsigned long);
+	mm_segment_t old_fs = get_fs();
 
 	__show_regs(regs);
 	print_modules();
@@ -344,9 +345,13 @@ void show_registers(struct pt_regs *regs)
 			printk("*HwTLS: %0*lx\n", field, tls);
 	}
 
+	if (!user_mode(regs))
+		/* Necessary for getting the correct stack content */
+		set_fs(KERNEL_DS);
 	show_stacktrace(current, regs);
 	show_code((unsigned int __user *) regs->cp0_epc);
 	printk("\n");
+	set_fs(old_fs);
 }
 
 static int regs_to_trapnr(struct pt_regs *regs)
@@ -1506,10 +1511,14 @@ int register_nmi_notifier(struct notifier_block *nb)
 
 void __noreturn nmi_exception_handler(struct pt_regs *regs)
 {
+	char str[100];
+
 	raw_notifier_call_chain(&nmi_chain, 0, regs);
 	bust_spinlocks(1);
-	printk("NMI taken!!!!\n");
-	die("NMI", regs);
+	snprintf(str, 100, "CPU%d NMI taken, CP0_EPC=%lx\n",
+		 smp_processor_id(), regs->cp0_epc);
+	regs->cp0_epc = read_c0_errorepc();
+	die(str, regs);
 }
 
 #define VECTORSPACING 0x100	/* for EI/VI mode */
-- 
1.8.3.2

RE: [PATCH] MIPS: Print correct PC in trace dump after NMI exception

[thomas.langer]

Hello Markos,

Markos Chandras wrote on 2013-10-08:
> From: Leonid Yegoshin <Leonid.Yegoshin@imgtec.com>
> 
> An NMI exception delivered from YAMON delivers the PC in ErrorPC
> instead of EPC. It's also necessary to clear the Status.BEV
> bit for the page fault exception handler to work properly.
> 
> Signed-off-by: Leonid Yegoshin <Leonid.Yegoshin@imgtec.com>
> Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
> ---


> @@ -1506,10 +1511,14 @@ int register_nmi_notifier(struct notifier_block
> *nb)
> 
>  void __noreturn nmi_exception_handler(struct pt_regs *regs)
>  {
> +	char str[100];
> +
>  	raw_notifier_call_chain(&nmi_chain, 0, regs);
>  	bust_spinlocks(1);
> -	printk("NMI taken!!!!\n");
> -	die("NMI", regs);
> +	snprintf(str, 100, "CPU%d NMI taken, CP0_EPC=%lx\n",
> +		 smp_processor_id(), regs->cp0_epc);
> +	regs->cp0_epc = read_c0_errorepc();

If this is a YAMON specific fix, why is it done in a common file?

> +	die(str, regs);
>  }
>  
>  #define VECTORSPACING 0x100	/* for EI/VI mode */

Best Regards,
Thomas

RE: [PATCH] MIPS: Print correct PC in trace dump after NMI exception

[thomas.langer]

Hello Markos,

Markos Chandras wrote on 2013-10-08:
> From: Leonid Yegoshin <Leonid.Yegoshin@imgtec.com>
> 
> An NMI exception delivered from YAMON delivers the PC in ErrorPC
> instead of EPC. It's also necessary to clear the Status.BEV
> bit for the page fault exception handler to work properly.
> 
> Signed-off-by: Leonid Yegoshin <Leonid.Yegoshin@imgtec.com>
> Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
> ---


> @@ -1506,10 +1511,14 @@ int register_nmi_notifier(struct notifier_block
> *nb)
> 
>  void __noreturn nmi_exception_handler(struct pt_regs *regs)
>  {
> +	char str[100];
> +
>  	raw_notifier_call_chain(&nmi_chain, 0, regs);
>  	bust_spinlocks(1);
> -	printk("NMI taken!!!!\n");
> -	die("NMI", regs);
> +	snprintf(str, 100, "CPU%d NMI taken, CP0_EPC=%lx\n",
> +		 smp_processor_id(), regs->cp0_epc);
> +	regs->cp0_epc = read_c0_errorepc();

If this is a YAMON specific fix, why is it done in a common file?

> +	die(str, regs);
>  }
>  
>  #define VECTORSPACING 0x100	/* for EI/VI mode */

Best Regards,
Thomas

Re: [PATCH] MIPS: Print correct PC in trace dump after NMI exception

[Markos Chandras]

On 10/08/13 12:48, thomas.langer@lantiq.com wrote:
> Hello Markos,
>
> If this is a YAMON specific fix, why is it done in a common file?
>
Hi Thomas,

I can see how the commit message can be a bit misleading. However, it's 
not YAMON specific. The NMI was just delivered by YAMON because we patch 
the default NMI location in YAMON (see mti-malta/malta-init.c).
NMI, Reset and Cache Error exceptions use ErrorEPC instead of EPC so 
this patch is platform independent.

Re: [PATCH] MIPS: Print correct PC in trace dump after NMI exception

[Markos Chandras]

On 10/08/13 12:48, thomas.langer@lantiq.com wrote:
> Hello Markos,
>
> If this is a YAMON specific fix, why is it done in a common file?
>
Hi Thomas,

I can see how the commit message can be a bit misleading. However, it's 
not YAMON specific. The NMI was just delivered by YAMON because we patch 
the default NMI location in YAMON (see mti-malta/malta-init.c).
NMI, Reset and Cache Error exceptions use ErrorEPC instead of EPC so 
this patch is platform independent.

Re: [PATCH] MIPS: Print correct PC in trace dump after NMI exception

[Ralf Baechle]

On Tue, Oct 08, 2013 at 11:48:54AM +0000, thomas.langer@lantiq.com wrote:

> >  void __noreturn nmi_exception_handler(struct pt_regs *regs)
> >  {
> > +	char str[100];
> > +
> >  	raw_notifier_call_chain(&nmi_chain, 0, regs);
> >  	bust_spinlocks(1);
> > -	printk("NMI taken!!!!\n");
> > -	die("NMI", regs);
> > +	snprintf(str, 100, "CPU%d NMI taken, CP0_EPC=%lx\n",
> > +		 smp_processor_id(), regs->cp0_epc);
> > +	regs->cp0_epc = read_c0_errorepc();
> 
> If this is a YAMON specific fix, why is it done in a common file?

The installation of an NMI handler is platform specific - this handler
however in all its simplicity is generic - or at least trying to.

The NMI on MIPS is notoriously hard to use.  The vectors is pointing to
the boot ROM so firmware first gets its grubby hands on a fresh NMI and
on most systems it'll do the firmware equivalent of a panic or reset
the system outright.  If that's still working - it's about the worst
tested functionality of firmware ...

  Ralf

RE: [PATCH] MIPS: Print correct PC in trace dump after NMI exception

[thomas.langer]

Hello Ralf,

Ralf Baechle wrote on 2013-10-08:

> On Tue, Oct 08, 2013 at 11:48:54AM +0000, thomas.langer@lantiq.com wrote:
> 
>>>  void __noreturn nmi_exception_handler(struct pt_regs *regs)
>>>  {
>>> +	char str[100];
>>> +
>>>  	raw_notifier_call_chain(&nmi_chain, 0, regs);
>>>  	bust_spinlocks(1);
>>> -	printk("NMI taken!!!!\n");
>>> -	die("NMI", regs);
>>> +	snprintf(str, 100, "CPU%d NMI taken, CP0_EPC=%lx\n",
>>> +		 smp_processor_id(), regs->cp0_epc);
>>> +	regs->cp0_epc = read_c0_errorepc();
>> 
>> If this is a YAMON specific fix, why is it done in a common file?
> 
> The installation of an NMI handler is platform specific - this handler
> however in all its simplicity is generic - or at least trying to.
> 
> The NMI on MIPS is notoriously hard to use.  The vectors is pointing to
> the boot ROM so firmware first gets its grubby hands on a fresh NMI and
> on most systems it'll do the firmware equivalent of a panic or reset
> the system outright.  If that's still working - it's about the worst
> tested functionality of firmware ...

I know, I am working on a chip which has working wrapper implemented in its bootrom:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/mips/lantiq/falcon/prom.c#n90
Therefore I was triggered by the keyword NMI ;-)

So if this has nothing to do with YAMON and is some generic NMI specific fix:
Acked-By: Thomas Langer <thomas.langer@lantiq.com>

> 
>   Ralf

Best Regards,
Thomas

RE: [PATCH] MIPS: Print correct PC in trace dump after NMI exception

[thomas.langer]

Hello Ralf,

Ralf Baechle wrote on 2013-10-08:

> On Tue, Oct 08, 2013 at 11:48:54AM +0000, thomas.langer@lantiq.com wrote:
> 
>>>  void __noreturn nmi_exception_handler(struct pt_regs *regs)
>>>  {
>>> +	char str[100];
>>> +
>>>  	raw_notifier_call_chain(&nmi_chain, 0, regs);
>>>  	bust_spinlocks(1);
>>> -	printk("NMI taken!!!!\n");
>>> -	die("NMI", regs);
>>> +	snprintf(str, 100, "CPU%d NMI taken, CP0_EPC=%lx\n",
>>> +		 smp_processor_id(), regs->cp0_epc);
>>> +	regs->cp0_epc = read_c0_errorepc();
>> 
>> If this is a YAMON specific fix, why is it done in a common file?
> 
> The installation of an NMI handler is platform specific - this handler
> however in all its simplicity is generic - or at least trying to.
> 
> The NMI on MIPS is notoriously hard to use.  The vectors is pointing to
> the boot ROM so firmware first gets its grubby hands on a fresh NMI and
> on most systems it'll do the firmware equivalent of a panic or reset
> the system outright.  If that's still working - it's about the worst
> tested functionality of firmware ...

I know, I am working on a chip which has working wrapper implemented in its bootrom:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/mips/lantiq/falcon/prom.c#n90
Therefore I was triggered by the keyword NMI ;-)

So if this has nothing to do with YAMON and is some generic NMI specific fix:
Acked-By: Thomas Langer <thomas.langer@lantiq.com>

> 
>   Ralf

Best Regards,
Thomas

Re: [PATCH] MIPS: Print correct PC in trace dump after NMI exception

[Ralf Baechle]

On Tue, Oct 08, 2013 at 12:39:31PM +0100, Markos Chandras wrote:

> +	/*
> +	 * Clear ERL - restore segment mapping
> +	 * Clear BEV - required for page fault exception handler to work
> +	 */
> +	mfc0	k0, CP0_STATUS
> +	ori     k0, k0, ST0_EXL
> +	lui     k1, %hi(~ST0_BEV)
> +	ori     k1, k1, %lo(~ST0_ERL)

Why not:

	li	k1 ~(ST0_BEV | ST0_ERL)

If you were afraid gas might use $1 expanding this macro instruction - no,
it won't.  A belt & suspenders approach might be to drop in a ".set noat";
it would make the assembler throw an error if should ever see the need to
use $1.

  Ralf

Re: [PATCH] MIPS: Print correct PC in trace dump after NMI exception

[Markos Chandras]

On 10/08/13 13:15, Ralf Baechle wrote:
> On Tue, Oct 08, 2013 at 12:39:31PM +0100, Markos Chandras wrote:
>
>> +	/*
>> +	 * Clear ERL - restore segment mapping
>> +	 * Clear BEV - required for page fault exception handler to work
>> +	 */
>> +	mfc0	k0, CP0_STATUS
>> +	ori     k0, k0, ST0_EXL
>> +	lui     k1, %hi(~ST0_BEV)
>> +	ori     k1, k1, %lo(~ST0_ERL)
>
> Why not:
>
> 	li	k1 ~(ST0_BEV | ST0_ERL)

I have no answer to that :) Looks good to me.
>
> If you were afraid gas might use $1 expanding this macro instruction - no,
> it won't.  A belt & suspenders approach might be to drop in a ".set noat";
> it would make the assembler throw an error if should ever see the need to
> use $1.
>
yeah i don't think the assembler would pick $1 in this case but we could 
add ".set noat" just to be safe i suppose.

Thanks for the review. Could you fix these problems for me or should i 
submit a new patch?

Re: [PATCH] MIPS: Print correct PC in trace dump after NMI exception

[Markos Chandras]

On 10/08/13 13:15, Ralf Baechle wrote:
> On Tue, Oct 08, 2013 at 12:39:31PM +0100, Markos Chandras wrote:
>
>> +	/*
>> +	 * Clear ERL - restore segment mapping
>> +	 * Clear BEV - required for page fault exception handler to work
>> +	 */
>> +	mfc0	k0, CP0_STATUS
>> +	ori     k0, k0, ST0_EXL
>> +	lui     k1, %hi(~ST0_BEV)
>> +	ori     k1, k1, %lo(~ST0_ERL)
>
> Why not:
>
> 	li	k1 ~(ST0_BEV | ST0_ERL)

I have no answer to that :) Looks good to me.
>
> If you were afraid gas might use $1 expanding this macro instruction - no,
> it won't.  A belt & suspenders approach might be to drop in a ".set noat";
> it would make the assembler throw an error if should ever see the need to
> use $1.
>
yeah i don't think the assembler would pick $1 in this case but we could 
add ".set noat" just to be safe i suppose.

Thanks for the review. Could you fix these problems for me or should i 
submit a new patch?

Re: [PATCH] MIPS: Print correct PC in trace dump after NMI exception

[Ralf Baechle]

On Tue, Oct 08, 2013 at 01:31:28PM +0100, Markos Chandras wrote:

> >If you were afraid gas might use $1 expanding this macro instruction - no,
> >it won't.  A belt & suspenders approach might be to drop in a ".set noat";
> >it would make the assembler throw an error if should ever see the need to
> >use $1.
> >
> yeah i don't think the assembler would pick $1 in this case but we
> could add ".set noat" just to be safe i suppose.
> 
> Thanks for the review. Could you fix these problems for me or should
> i submit a new patch?

It's trivial enough so I'm going to do it.

The NMI handler btw. was already wrapped with .set push; .set noat; ...
.set pop.

  Ralf

Re: [PATCH] MIPS: Print correct PC in trace dump after NMI exception

[Markos Chandras]

markos

On 10/08/13 13:38, Ralf Baechle wrote:
> On Tue, Oct 08, 2013 at 01:31:28PM +0100, Markos Chandras wrote:
>
>>> If you were afraid gas might use $1 expanding this macro instruction - no,
>>> it won't.  A belt & suspenders approach might be to drop in a ".set noat";
>>> it would make the assembler throw an error if should ever see the need to
>>> use $1.
>>>
>> yeah i don't think the assembler would pick $1 in this case but we
>> could add ".set noat" just to be safe i suppose.
>>
>> Thanks for the review. Could you fix these problems for me or should
>> i submit a new patch?
>
> It's trivial enough so I'm going to do it.
>
> The NMI handler btw. was already wrapped with .set push; .set noat; ...
> .set pop.
>
>    Ralf
>
yeah i just noticed. Thank you!

Re: [PATCH] MIPS: Print correct PC in trace dump after NMI exception

[Markos Chandras]

markos

On 10/08/13 13:38, Ralf Baechle wrote:
> On Tue, Oct 08, 2013 at 01:31:28PM +0100, Markos Chandras wrote:
>
>>> If you were afraid gas might use $1 expanding this macro instruction - no,
>>> it won't.  A belt & suspenders approach might be to drop in a ".set noat";
>>> it would make the assembler throw an error if should ever see the need to
>>> use $1.
>>>
>> yeah i don't think the assembler would pick $1 in this case but we
>> could add ".set noat" just to be safe i suppose.
>>
>> Thanks for the review. Could you fix these problems for me or should
>> i submit a new patch?
>
> It's trivial enough so I'm going to do it.
>
> The NMI handler btw. was already wrapped with .set push; .set noat; ...
> .set pop.
>
>    Ralf
>
yeah i just noticed. Thank you!

Re: [PATCH] MIPS: Print correct PC in trace dump after NMI exception

[Ralf Baechle]

On Tue, Oct 08, 2013 at 01:43:40PM +0100, Markos Chandras wrote:

> markos
> 
> On 10/08/13 13:38, Ralf Baechle wrote:
> >On Tue, Oct 08, 2013 at 01:31:28PM +0100, Markos Chandras wrote:
> >
> >>>If you were afraid gas might use $1 expanding this macro instruction - no,
> >>>it won't.  A belt & suspenders approach might be to drop in a ".set noat";
> >>>it would make the assembler throw an error if should ever see the need to
> >>>use $1.
> >>>
> >>yeah i don't think the assembler would pick $1 in this case but we
> >>could add ".set noat" just to be safe i suppose.
> >>
> >>Thanks for the review. Could you fix these problems for me or should
> >>i submit a new patch?
> >
> >It's trivial enough so I'm going to do it.
> >
> >The NMI handler btw. was already wrapped with .set push; .set noat; ...
> >.set pop.
> >
> >   Ralf
> >
> yeah i just noticed. Thank you!

Signature at the top, content at the bottom?  Sigh ...

:-)

I've pushed a fresh upstream-sfr with this patch applied.  This will be
the last change for the day as I want to concentrate on other code.

  Ralf