[PATCH] MIPS: Trap exception handling fixes

[PATCH] MIPS: Trap exception handling fixes

[Maciej W. Rozycki]

2a0b24f56c2492b932f1aed617ae80fb23500d21 broke Trap exception handling in 
the standard MIPS mode.  Additionally the microMIPS-mode trap code mask is 
wrong, as it's a 4-bit field.  Here's a fix.

Signed-off-by: Maciej W. Rozycki <macro@codesourcery.com>
---
Ralf, please apply.  Also the mention of: "A few NOP instructions are used 
to maintain the correct alignment[...]" in the commit referred makes me 
feel scared -- there is that .align pseudo-op, you know...  Maciej

linux-mips-do-tr.diff

 arch/mips/kernel/traps.c |   28 +++++++++++++++-------------
 1 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
index e3be670..a75ae40 100644
--- a/arch/mips/kernel/traps.c
+++ b/arch/mips/kernel/traps.c
@@ -897,22 +897,24 @@ out_sigsegv:
 
 asmlinkage void do_tr(struct pt_regs *regs)
 {
-	unsigned int opcode, tcode = 0;
+	u32 opcode, tcode = 0;
 	u16 instr[2];
-	unsigned long epc = exception_epc(regs);
+	unsigned long epc = msk_isa16_mode(exception_epc(regs));
 
-	if ((__get_user(instr[0], (u16 __user *)msk_isa16_mode(epc))) ||
-		(__get_user(instr[1], (u16 __user *)msk_isa16_mode(epc + 2))))
+	if (get_isa16_mode(regs->cp0_epc)) {
+		if (__get_user(instr[0], (u16 __user *)(epc + 0)) ||
+		    __get_user(instr[1], (u16 __user *)(epc + 2)))
 			goto out_sigsegv;
-	opcode = (instr[0] << 16) | instr[1];
-
-	/* Immediate versions don't provide a code.  */
-	if (!(opcode & OPCODE)) {
-		if (get_isa16_mode(regs->cp0_epc))
-			/* microMIPS */
-			tcode = (opcode >> 12) & 0x1f;
-		else
-			tcode = ((opcode >> 6) & ((1 << 10) - 1));
+		opcode = (instr[0] << 16) | instr[1];
+		/* Immediate versions don't provide a code.  */
+		if (!(opcode & OPCODE))
+			tcode = (opcode >> 12) & ((1 << 4) - 1);
+	} else {
+		if (__get_user(opcode, (u32 __user *)epc))
+			goto out_sigsegv;
+		/* Immediate versions don't provide a code.  */
+		if (!(opcode & OPCODE))
+			tcode = (opcode >> 6) & ((1 << 10) - 1);
 	}
 
 	do_trap_or_bp(regs, tcode, "Trap");

[PATCH] MIPS: Trap exception handling fixes

[Maciej W. Rozycki]

2a0b24f56c2492b932f1aed617ae80fb23500d21 broke Trap exception handling in 
the standard MIPS mode.  Additionally the microMIPS-mode trap code mask is 
wrong, as it's a 4-bit field.  Here's a fix.

Signed-off-by: Maciej W. Rozycki <macro@codesourcery.com>
---
Ralf, please apply.  Also the mention of: "A few NOP instructions are used 
to maintain the correct alignment[...]" in the commit referred makes me 
feel scared -- there is that .align pseudo-op, you know...  Maciej

linux-mips-do-tr.diff

 arch/mips/kernel/traps.c |   28 +++++++++++++++-------------
 1 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
index e3be670..a75ae40 100644
--- a/arch/mips/kernel/traps.c
+++ b/arch/mips/kernel/traps.c
@@ -897,22 +897,24 @@ out_sigsegv:
 
 asmlinkage void do_tr(struct pt_regs *regs)
 {
-	unsigned int opcode, tcode = 0;
+	u32 opcode, tcode = 0;
 	u16 instr[2];
-	unsigned long epc = exception_epc(regs);
+	unsigned long epc = msk_isa16_mode(exception_epc(regs));
 
-	if ((__get_user(instr[0], (u16 __user *)msk_isa16_mode(epc))) ||
-		(__get_user(instr[1], (u16 __user *)msk_isa16_mode(epc + 2))))
+	if (get_isa16_mode(regs->cp0_epc)) {
+		if (__get_user(instr[0], (u16 __user *)(epc + 0)) ||
+		    __get_user(instr[1], (u16 __user *)(epc + 2)))
 			goto out_sigsegv;
-	opcode = (instr[0] << 16) | instr[1];
-
-	/* Immediate versions don't provide a code.  */
-	if (!(opcode & OPCODE)) {
-		if (get_isa16_mode(regs->cp0_epc))
-			/* microMIPS */
-			tcode = (opcode >> 12) & 0x1f;
-		else
-			tcode = ((opcode >> 6) & ((1 << 10) - 1));
+		opcode = (instr[0] << 16) | instr[1];
+		/* Immediate versions don't provide a code.  */
+		if (!(opcode & OPCODE))
+			tcode = (opcode >> 12) & ((1 << 4) - 1);
+	} else {
+		if (__get_user(opcode, (u32 __user *)epc))
+			goto out_sigsegv;
+		/* Immediate versions don't provide a code.  */
+		if (!(opcode & OPCODE))
+			tcode = (opcode >> 6) & ((1 << 10) - 1);
 	}
 
 	do_trap_or_bp(regs, tcode, "Trap");

Re: [PATCH] MIPS: Trap exception handling fixes

[Ralf Baechle]

On Thu, May 23, 2013 at 04:31:23PM +0100, Maciej W. Rozycki wrote:

> 2a0b24f56c2492b932f1aed617ae80fb23500d21 broke Trap exception handling in 
> the standard MIPS mode.  Additionally the microMIPS-mode trap code mask is 
> wrong, as it's a 4-bit field.  Here's a fix.
> 
> Signed-off-by: Maciej W. Rozycki <macro@codesourcery.com>
> ---
> Ralf, please apply.  Also the mention of: "A few NOP instructions are used 
> to maintain the correct alignment[...]" in the commit referred makes me 
> feel scared -- there is that .align pseudo-op, you know...  Maciej

Thanks, applied.

Seems that whole bloody patchset was painfully unripe :-(

  Ralf

Re: [PATCH] MIPS: Trap exception handling fixes

[Maciej W. Rozycki]

On Thu, 23 May 2013, Ralf Baechle wrote:

> Seems that whole bloody patchset was painfully unripe :-(

 Sigh, I meant to at least skim over the patches to review them, but these 
days I can hardly find time for smaller changes even.  This particular 
issue was found in GCC regression testing.  I think at least running the 
GCC and glibc test suites across the three ABIs and both endiannesses each 
to make sure MIPS16/microMIPS support didn't regress standard MIPS support 
was due.  This would have avoided this problem.  It looks to me do_bp 
would benefit from some polishing too.

  Maciej

Re: [PATCH] MIPS: Trap exception handling fixes

[Maciej W. Rozycki]

On Thu, 23 May 2013, Ralf Baechle wrote:

> Seems that whole bloody patchset was painfully unripe :-(

 Sigh, I meant to at least skim over the patches to review them, but these 
days I can hardly find time for smaller changes even.  This particular 
issue was found in GCC regression testing.  I think at least running the 
GCC and glibc test suites across the three ABIs and both endiannesses each 
to make sure MIPS16/microMIPS support didn't regress standard MIPS support 
was due.  This would have avoided this problem.  It looks to me do_bp 
would benefit from some polishing too.

  Maciej

Re: [PATCH] MIPS: Trap exception handling fixes

[Maciej W. Rozycki]

On Thu, 23 May 2013, Maciej W. Rozycki wrote:

> It looks to me do_bp would benefit from some polishing too.

 To make myself more clear here -- there are two microMIPS BREAK encodings 
of which do_bp assumes (without checking!) the 32-bit one, that is 
actually unlikely to be ever present in compiler-generated code (so how 
was this change validated?), and also the MIPS16 path is special-cased 
(makes a separate call to do_trap_or_bp), which I find error-prone in 
long-term maintenance.

  Maciej

Re: [PATCH] MIPS: Trap exception handling fixes

[Maciej W. Rozycki]

On Thu, 23 May 2013, Maciej W. Rozycki wrote:

> It looks to me do_bp would benefit from some polishing too.

 To make myself more clear here -- there are two microMIPS BREAK encodings 
of which do_bp assumes (without checking!) the 32-bit one, that is 
actually unlikely to be ever present in compiler-generated code (so how 
was this change validated?), and also the MIPS16 path is special-cased 
(makes a separate call to do_trap_or_bp), which I find error-prone in 
long-term maintenance.

  Maciej

Re: [PATCH] MIPS: Trap exception handling fixes

[David Daney]

On 05/23/2013 09:54 AM, Maciej W. Rozycki wrote:
> On Thu, 23 May 2013, Maciej W. Rozycki wrote:
>
>> It looks to me do_bp would benefit from some polishing too.
>
>   To make myself more clear here -- there are two microMIPS BREAK encodings
> of which do_bp assumes (without checking!) the 32-bit one, that is
> actually unlikely to be ever present in compiler-generated code (so how
> was this change validated?),

Probably sjhill should answer that, as they are his patches.

David Daney


> and also the MIPS16 path is special-cased
> (makes a separate call to do_trap_or_bp), which I find error-prone in
> long-term maintenance.
>
>    Maciej
>
>
>

RE: [PATCH] MIPS: Trap exception handling fixes

[Steven J. Hill]

I am working on a patch to fix the microMIPS BREAK encoding. This was already discovered when we were regression testing for our 'linux-mti-3.8' branch release. With regards to the MIPS16e path, I will need to take a closer look.

RE: [PATCH] MIPS: Trap exception handling fixes

[Maciej W. Rozycki]

On Fri, 24 May 2013, Steven J. Hill wrote:

> I am working on a patch to fix the microMIPS BREAK encoding. This was 
> already discovered when we were regression testing for our 
> 'linux-mti-3.8' branch release. With regards to the MIPS16e path, I will 
> need to take a closer look.

 Great, thanks!

  Maciej