[PATCH] MIPS: Add emulation for fpureg-mem unaligned access

[PATCH] MIPS: Add emulation for fpureg-mem unaligned access

[Lluis Batlle i Rossell]

Reusing most of the code from lw,ld,sw,sd emulation,
I add the emulation for lwc1,ldc1,swc1,sdc1.

This avoids the direct SIGBUS sent to userspace processes that have
misaligned memory accesses.

I've tested the change in Loongson2F, with an own test program, and
WebKit 1.4.0, as both were killed by sigbus without this patch.

Signed-off: Lluis Batlle i Rossell <viric@viric.name>
---
 arch/mips/kernel/unaligned.c |   43 +++++++++++++++++++++++++++++-------------
 1 file changed, 30 insertions(+), 13 deletions(-)

diff --git a/arch/mips/kernel/unaligned.c b/arch/mips/kernel/unaligned.c
index 9c58bdf..4531e6c 100644
--- a/arch/mips/kernel/unaligned.c
+++ b/arch/mips/kernel/unaligned.c
@@ -85,6 +85,7 @@
 #include <asm/cop2.h>
 #include <asm/inst.h>
 #include <asm/uaccess.h>
+#include <asm/fpu.h>
 
 #define STR(x)  __STR(x)
 #define __STR(x)  #x
@@ -108,6 +109,7 @@ static void emulate_load_store_insn(struct pt_regs *regs,
 	union mips_instruction insn;
 	unsigned long value;
 	unsigned int res;
+	fpureg_t *fpuregs;
 
 	perf_sw_event(PERF_COUNT_SW_EMULATION_FAULTS, 1, regs, 0);
 
@@ -183,6 +185,7 @@ static void emulate_load_store_insn(struct pt_regs *regs,
 		break;
 
 	case lw_op:
+	case lwc1_op:
 		if (!access_ok(VERIFY_READ, addr, 4))
 			goto sigbus;
 
@@ -209,7 +212,12 @@ static void emulate_load_store_insn(struct pt_regs *regs,
 		if (res)
 			goto fault;
 		compute_return_epc(regs);
-		regs->regs[insn.i_format.rt] = value;
+		if (insn.i_format.opcode == lw_op) {
+			regs->regs[insn.i_format.rt] = value;
+		} else {
+			fpuregs = get_fpu_regs(current);
+			fpuregs[insn.i_format.rt] = value;
+		}
 		break;
 
 	case lhu_op:
@@ -291,6 +299,7 @@ static void emulate_load_store_insn(struct pt_regs *regs,
 		goto sigill;
 
 	case ld_op:
+	case ldc1_op:
 #ifdef CONFIG_64BIT
 		/*
 		 * A 32-bit kernel might be running on a 64-bit processor.  But
@@ -325,7 +334,12 @@ static void emulate_load_store_insn(struct pt_regs *regs,
 		if (res)
 			goto fault;
 		compute_return_epc(regs);
-		regs->regs[insn.i_format.rt] = value;
+		if (insn.i_format.opcode == ld_op) {
+			regs->regs[insn.i_format.rt] = value;
+		} else {
+			fpuregs = get_fpu_regs(current);
+			fpuregs[insn.i_format.rt] = value;
+		}
 		break;
 #endif /* CONFIG_64BIT */
 
@@ -370,10 +384,16 @@ static void emulate_load_store_insn(struct pt_regs *regs,
 		break;
 
 	case sw_op:
+	case swc1_op:
 		if (!access_ok(VERIFY_WRITE, addr, 4))
 			goto sigbus;
 
-		value = regs->regs[insn.i_format.rt];
+		if (insn.i_format.opcode == sw_op) {
+			value = regs->regs[insn.i_format.rt];
+		} else {
+			fpuregs = get_fpu_regs(current);
+			value = fpuregs[insn.i_format.rt];
+		}
 		__asm__ __volatile__ (
 #ifdef __BIG_ENDIAN
 			"1:\tswl\t%1,(%2)\n"
@@ -401,6 +421,7 @@ static void emulate_load_store_insn(struct pt_regs *regs,
 		break;
 
 	case sd_op:
+	case sdc1_op:
 #ifdef CONFIG_64BIT
 		/*
 		 * A 32-bit kernel might be running on a 64-bit processor.  But
@@ -412,7 +433,12 @@ static void emulate_load_store_insn(struct pt_regs *regs,
 		if (!access_ok(VERIFY_WRITE, addr, 8))
 			goto sigbus;
 
-		value = regs->regs[insn.i_format.rt];
+		if (insn.i_format.opcode == sd_op) {
+			value = regs->regs[insn.i_format.rt];
+		} else {
+			fpuregs = get_fpu_regs(current);
+			value = fpuregs[insn.i_format.rt];
+		}
 		__asm__ __volatile__ (
 #ifdef __BIG_ENDIAN
 			"1:\tsdl\t%1,(%2)\n"
@@ -443,15 +469,6 @@ static void emulate_load_store_insn(struct pt_regs *regs,
 		/* Cannot handle 64-bit instructions in 32-bit kernel */
 		goto sigill;
 
-	case lwc1_op:
-	case ldc1_op:
-	case swc1_op:
-	case sdc1_op:
-		/*
-		 * I herewith declare: this does not happen.  So send SIGBUS.
-		 */
-		goto sigbus;
-
 	/*
 	 * COP2 is available to implementor for application specific use.
 	 * It's up to applications to register a notifier chain and do
-- 
1.7.9.5

Re: [PATCH] MIPS: Add emulation for fpureg-mem unaligned access

[Jonas Gorski]

Hi,

some comments ...

On 16 June 2012 00:22, Lluis Batlle i Rossell <viric@viric.name> wrote:
> Reusing most of the code from lw,ld,sw,sd emulation,
> I add the emulation for lwc1,ldc1,swc1,sdc1.

What about lwxc1, ldxc1, swxc1 and sdxc1? These also require alignment.

> This avoids the direct SIGBUS sent to userspace processes that have
> misaligned memory accesses.
>
> I've tested the change in Loongson2F, with an own test program, and
> WebKit 1.4.0, as both were killed by sigbus without this patch.
>
> Signed-off: Lluis Batlle i Rossell <viric@viric.name>
> ---
>  arch/mips/kernel/unaligned.c |   43 +++++++++++++++++++++++++++++-------------
>  1 file changed, 30 insertions(+), 13 deletions(-)
>
> diff --git a/arch/mips/kernel/unaligned.c b/arch/mips/kernel/unaligned.c
> index 9c58bdf..4531e6c 100644
> --- a/arch/mips/kernel/unaligned.c
> +++ b/arch/mips/kernel/unaligned.c
> @@ -85,6 +85,7 @@
>  #include <asm/cop2.h>
>  #include <asm/inst.h>
>  #include <asm/uaccess.h>
> +#include <asm/fpu.h>
>
>  #define STR(x)  __STR(x)
>  #define __STR(x)  #x
> @@ -108,6 +109,7 @@ static void emulate_load_store_insn(struct pt_regs *regs,
>        union mips_instruction insn;
>        unsigned long value;
>        unsigned int res;
> +       fpureg_t *fpuregs;
>
>        perf_sw_event(PERF_COUNT_SW_EMULATION_FAULTS, 1, regs, 0);
>
> @@ -183,6 +185,7 @@ static void emulate_load_store_insn(struct pt_regs *regs,
>                break;
>
>        case lw_op:
> +       case lwc1_op:
>                if (!access_ok(VERIFY_READ, addr, 4))
>                        goto sigbus;
>
> @@ -209,7 +212,12 @@ static void emulate_load_store_insn(struct pt_regs *regs,
>                if (res)
>                        goto fault;
>                compute_return_epc(regs);
> -               regs->regs[insn.i_format.rt] = value;
> +               if (insn.i_format.opcode == lw_op) {
> +                       regs->regs[insn.i_format.rt] = value;
> +               } else {
> +                       fpuregs = get_fpu_regs(current);
> +                       fpuregs[insn.i_format.rt] = value;
> +               }
>                break;
>
>        case lhu_op:
> @@ -291,6 +299,7 @@ static void emulate_load_store_insn(struct pt_regs *regs,
>                goto sigill;
>
>        case ld_op:
> +       case ldc1_op:
>  #ifdef CONFIG_64BIT

From what I can tell, ldc1 is a valid MIPS32 instruction, so this
should probably be something like

        case ld_op:
#ifndef CONFIG_64BIT
                return sigill;
#endif
        case ldc1_op:
...

>                /*
>                 * A 32-bit kernel might be running on a 64-bit processor.  But
> @@ -325,7 +334,12 @@ static void emulate_load_store_insn(struct pt_regs *regs,
>                if (res)
>                        goto fault;
>                compute_return_epc(regs);
> -               regs->regs[insn.i_format.rt] = value;
> +               if (insn.i_format.opcode == ld_op) {
> +                       regs->regs[insn.i_format.rt] = value;
> +               } else {
> +                       fpuregs = get_fpu_regs(current);
> +                       fpuregs[insn.i_format.rt] = value;
> +               }
>                break;
>  #endif /* CONFIG_64BIT */
>
> @@ -370,10 +384,16 @@ static void emulate_load_store_insn(struct pt_regs *regs,
>                break;
>
>        case sw_op:
> +       case swc1_op:
>                if (!access_ok(VERIFY_WRITE, addr, 4))
>                        goto sigbus;
>
> -               value = regs->regs[insn.i_format.rt];
> +               if (insn.i_format.opcode == sw_op) {
> +                       value = regs->regs[insn.i_format.rt];
> +               } else {
> +                       fpuregs = get_fpu_regs(current);
> +                       value = fpuregs[insn.i_format.rt];
> +               }
>                __asm__ __volatile__ (
>  #ifdef __BIG_ENDIAN
>                        "1:\tswl\t%1,(%2)\n"
> @@ -401,6 +421,7 @@ static void emulate_load_store_insn(struct pt_regs *regs,
>                break;
>
>        case sd_op:
> +       case sdc1_op:
>  #ifdef CONFIG_64BIT

Same here.

>                /*
>                 * A 32-bit kernel might be running on a 64-bit processor.  But


Regards,
Jonas

Re: [PATCH] MIPS: Add emulation for fpureg-mem unaligned access

[Lluís Batlle i Rossell]

Hello,

thank you for the review.

On Sat, Jun 16, 2012 at 01:21:27PM +0200, Jonas Gorski wrote:
> On 16 June 2012 00:22, Lluis Batlle i Rossell <viric@viric.name> wrote:
> > Reusing most of the code from lw,ld,sw,sd emulation,
> > I add the emulation for lwc1,ldc1,swc1,sdc1.
> 
> What about lwxc1, ldxc1, swxc1 and sdxc1? These also require alignment.

Looking at gcc code, I could not find those instructions emmitted. I could write
some assembly tests cases though.

> >        case ld_op:
> > +       case ldc1_op:
> >  #ifdef CONFIG_64BIT
> 
> From what I can tell, ldc1 is a valid MIPS32 instruction, so this
> should probably be something like
> 
>         case ld_op:
> #ifndef CONFIG_64BIT
>                 return sigill;
> #endif

I agree! I'll repost with these fixes.

Regards,
Lluís

Re: [loongson-dev] Re: [PATCH] MIPS: Add emulation for fpureg-mem unaligned access

[Lluís Batlle i Rossell]

Hello,

On Sat, Jun 16, 2012 at 02:15:13PM +0200, Lluís Batlle i Rossell wrote:
> On Sat, Jun 16, 2012 at 01:21:27PM +0200, Jonas Gorski wrote:
> > On 16 June 2012 00:22, Lluis Batlle i Rossell <viric@viric.name> wrote:
> > > Reusing most of the code from lw,ld,sw,sd emulation,
> > > I add the emulation for lwc1,ldc1,swc1,sdc1.
> > 
> > What about lwxc1, ldxc1, swxc1 and sdxc1? These also require alignment.
> 
> Looking at gcc code, I could not find those instructions emmitted. I could write
> some assembly tests cases though.

I just undesrtood the Loongson2f only does MIPS III, and the *xc1
instructions are for MIPS IV, which I don't have, so I can't test.

I started to write the handling of cop1x_op, then a switch() of the coprocessor
operation, then I had to introduce a new instruction format not available in
inst.h, ... too much new lines I won't be able to test.

I'll repost only with the attention on MIPS II ldc1/sdc1. Btw, mips-iv.pdf says
in the LDC1 page that it's MIPS II, but Table B-5 mentions it as MIPS III. I
imagine the table is wrong, because it appears in Table B-25 too.

Regards,
Lluís.

Re: [PATCH] MIPS: Add emulation for fpureg-mem unaligned access

[Lluís Batlle i Rossell]

Hello again,

On Sat, Jun 16, 2012 at 02:15:13PM +0200, Lluís Batlle i Rossell wrote:
> > From what I can tell, ldc1 is a valid MIPS32 instruction, so this
> > should probably be something like
> > 
> >         case ld_op:
> > #ifndef CONFIG_64BIT
> >                 return sigill;
> > #endif
> 
> I agree! I'll repost with these fixes.

Well, I think I take my words back. Handling the ldc1/sdc1 cases in MIPS32 is
tricker than I thought first, because I can't use ldl/ldr or sdl/sdr there.
Given my ability with mips assembly, I leave the patch as is.

In 'patchwork' I had set the patch first to superseeded, but then I set it back
to New.

Regards,
Lluís.

Re: [PATCH] MIPS: Add emulation for fpureg-mem unaligned access

[Thomas Bogendoerfer]

On Sat, Jun 16, 2012 at 02:58:47PM +0200, Lluís Batlle i Rossell wrote:
> Hello again,
> 
> On Sat, Jun 16, 2012 at 02:15:13PM +0200, Lluís Batlle i Rossell wrote:
> > > From what I can tell, ldc1 is a valid MIPS32 instruction, so this
> > > should probably be something like
> > > 
> > >         case ld_op:
> > > #ifndef CONFIG_64BIT
> > >                 return sigill;
> > > #endif
> > 
> > I agree! I'll repost with these fixes.
> 
> Well, I think I take my words back. Handling the ldc1/sdc1 cases in MIPS32 is
> tricker than I thought first, because I can't use ldl/ldr or sdl/sdr there.
> Given my ability with mips assembly, I leave the patch as is.
> 
> In 'patchwork' I had set the patch first to superseeded, but then I set it back
> to New.

why is there a reason for this ? Unaligned FPU access shouts to me simply
broken code, go fix that. But maybe I'm wrong ?

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH] MIPS: Add emulation for fpureg-mem unaligned access

[Lluís Batlle i Rossell]

> In reply to Thomas Bogendoerfer - 2012-06-17 09:06:43
> On Sat, Jun 16, 2012 at 02:58:47PM +0200, Lluís Batlle i Rossell wrote:
> > Well, I think I take my words back. Handling the ldc1/sdc1 cases in MIPS32 is
> > tricker than I thought first, because I can't use ldl/ldr or sdl/sdr there.
> > Given my ability with mips assembly, I leave the patch as is.
> > 
> > In 'patchwork' I had set the patch first to superseeded, but then I set it
> > back
> > to New.
> 
> why is there a reason for this ? Unaligned FPU access shouts to me simply
> broken code, go fix that. But maybe I'm wrong ?
> 
> Thomas.

Hello Thomas,

sorry to answer this way; I was not subscribed to the list, and I noticed your
answer only today thanks to patchwork.

Right, the patch allows broken code to run further, instead of fail straight.
The crash can be still achieved disabling the emulation of unaligned accesses
completely, through debugfs, for example.

As Jonas reported, I think that maybe I should rework the patch for it to emit
sigbus instead of sigill on ldc1,ldc1 for mips32. Do I understand it right?

Regards,
Lluís.

Re: [PATCH] MIPS: Add emulation for fpureg-mem unaligned access

[Maciej W. Rozycki]

On Wed, 20 Jun 2012, Lluís Batlle i Rossell wrote:

> > > Well, I think I take my words back. Handling the ldc1/sdc1 cases in MIPS32 is
> > > tricker than I thought first, because I can't use ldl/ldr or sdl/sdr there.
> > > Given my ability with mips assembly, I leave the patch as is.

 I suggest that for 32-bit kernels you simply reuse the existing snippets 
from that function and handle ldc1/sdc1 with a pair of lwl/ldr or swl/swr 
pairs ordered as appropriate for the endianness selected -- that should be 
fairly easy.

 Also regardless of that, please make sure that your code handles the two 
possible settings of CP0 Status register's bit FR correctly, as the 32-bit 
halves of floating-point data are distributed differently across 
floating-point registers based on this bit's setting (check if an o32 and 
an n64 or n32 program gets these values right).

> > why is there a reason for this ? Unaligned FPU access shouts to me simply
> > broken code, go fix that. But maybe I'm wrong ?

 Since we're emulating these accesses at all I concur Lluís we should stay 
consistent across the whole instruction set.

> Right, the patch allows broken code to run further, instead of fail straight.
> The crash can be still achieved disabling the emulation of unaligned accesses
> completely, through debugfs, for example.

 sysmips(MIPS_FIXADE, 0) is another way.

> As Jonas reported, I think that maybe I should rework the patch for it to emit
> sigbus instead of sigill on ldc1,ldc1 for mips32. Do I understand it right?

 Have you checked your code against a non-FPU processor (or with the 
"nofpu" kernel option) too?

  Maciej

Re: [PATCH] MIPS: Add emulation for fpureg-mem unaligned access

[Lluís Batlle i Rossell]

Hello Maciej,

On Wed, Jul 11, 2012 at 01:05:04AM +0100, Maciej W. Rozycki wrote:
> On Wed, 20 Jun 2012, Lluís Batlle i Rossell wrote:
> 
> > > > Well, I think I take my words back. Handling the ldc1/sdc1 cases in MIPS32 is
> > > > tricker than I thought first, because I can't use ldl/ldr or sdl/sdr there.
> > > > Given my ability with mips assembly, I leave the patch as is.
>  I suggest that for 32-bit kernels you simply reuse the existing snippets 
> from that function and handle ldc1/sdc1 with a pair of lwl/ldr or swl/swr 
> pairs ordered as appropriate for the endianness selected -- that should be 
> fairly easy.

Hm I still don't understand well enough how to do that. Would I need to get some
aligned memory (a stack automatic variable for example), copy the double word
there with proper endianness, and then call again ldc1? (similar for sdc1)

>  Also regardless of that, please make sure that your code handles the two 
> possible settings of CP0 Status register's bit FR correctly, as the 32-bit 
> halves of floating-point data are distributed differently across 
> floating-point registers based on this bit's setting (check if an o32 and 
> an n64 or n32 program gets these values right).

Hm I'm failing to find in the mips-iv.pdf how to check that FR bit, although I
see it mentioned there. Sorry.

> > As Jonas reported, I think that maybe I should rework the patch for it to emit
> > sigbus instead of sigill on ldc1,ldc1 for mips32. Do I understand it right?
> 
>  Have you checked your code against a non-FPU processor (or with the 
> "nofpu" kernel option) too?

No. Would in that case the processor have the fpu disabled? I understand that
the code path is called only in a particular case of 'unaligned access'
exception.

Thank you for your comments.

Regards,
Lluís.

Re: [PATCH] MIPS: Add emulation for fpureg-mem unaligned access

[Maciej W. Rozycki]

Hi Lluís

> >  I suggest that for 32-bit kernels you simply reuse the existing snippets 
> > from that function and handle ldc1/sdc1 with a pair of lwl/ldr or swl/swr 
> > pairs ordered as appropriate for the endianness selected -- that should be 
> > fairly easy.
> 
> Hm I still don't understand well enough how to do that. Would I need to get some
> aligned memory (a stack automatic variable for example), copy the double word
> there with proper endianness, and then call again ldc1? (similar for sdc1)

 No need to copy anything to scratch space, you'd just handle the thing 
piecewise in 32-bit chunks, transferring one FPR first, followed with the 
other one -- this is exactly what LDC1/SDC1 logically do in the 32-bit 
mode anyway.  Of course FPR indices are swapped between endiannesses (or 
data in memory is swapped -- depending on how you look at it).

> >  Also regardless of that, please make sure that your code handles the two 
> > possible settings of CP0 Status register's bit FR correctly, as the 32-bit 
> > halves of floating-point data are distributed differently across 
> > floating-point registers based on this bit's setting (check if an o32 and 
> > an n64 or n32 program gets these values right).
> 
> Hm I'm failing to find in the mips-iv.pdf how to check that FR bit, although I
> see it mentioned there. Sorry.

 That'll be set in Linux's task status structure somewhere as the 
floating-point model is implied by the ABI (FR is clear for o32 and set 
for n32/n64) -- no need to poke at hardware.  Have a look at FP context 
switching code -- it has to take similar measures.  There may be some code 
that checks that in the FPU emulator as well.

> > > As Jonas reported, I think that maybe I should rework the patch for it to emit
> > > sigbus instead of sigill on ldc1,ldc1 for mips32. Do I understand it right?
> > 
> >  Have you checked your code against a non-FPU processor (or with the 
> > "nofpu" kernel option) too?
> 
> No. Would in that case the processor have the fpu disabled? I understand that
> the code path is called only in a particular case of 'unaligned access'
> exception.

 It may well possibly be, I'm not sure offhand, but unaligned access 
emulation just has to work the same for floating-point transfers 
regardless of whether the FPU has been enabled or is fully emulated.  
This just have to be verified.

 The MIPS/Linux user ABI specifies the presence of an FPU unconditionally 
and a missing or disabled unit is automatically emulated in software 
transparently (except for the performance loss of course).

  Maciej

Re: [PATCH] MIPS: Add emulation for fpureg-mem unaligned access

[Ralf Baechle]

On Sat, Jun 16, 2012 at 12:22:53AM +0200, Lluis Batlle i Rossell wrote:

> Reusing most of the code from lw,ld,sw,sd emulation,
> I add the emulation for lwc1,ldc1,swc1,sdc1.
> 
> This avoids the direct SIGBUS sent to userspace processes that have
> misaligned memory accesses.
> 
> I've tested the change in Loongson2F, with an own test program, and
> WebKit 1.4.0, as both were killed by sigbus without this patch.

A misaligned FPU access is a strong indication for broken, non-portable
software.  which means you're likely trying to fix the wrong issue.  It's
quite intentional that there is no unaligned handling for the FPU in the
kernel - and afaics there isn't for any other MIPS UNIX.

  Ralf

Re: [PATCH] MIPS: Add emulation for fpureg-mem unaligned access

[Lluís Batlle i Rossell]

On Tue, Jul 31, 2012 at 03:40:01PM +0200, Ralf Baechle wrote:
> On Sat, Jun 16, 2012 at 12:22:53AM +0200, Lluis Batlle i Rossell wrote:
> 
> > Reusing most of the code from lw,ld,sw,sd emulation,
> > I add the emulation for lwc1,ldc1,swc1,sdc1.
> > 
> > This avoids the direct SIGBUS sent to userspace processes that have
> > misaligned memory accesses.
> > 
> > I've tested the change in Loongson2F, with an own test program, and
> > WebKit 1.4.0, as both were killed by sigbus without this patch.
> 
> A misaligned FPU access is a strong indication for broken, non-portable
> software.  which means you're likely trying to fix the wrong issue.  It's
> quite intentional that there is no unaligned handling for the FPU in the
> kernel - and afaics there isn't for any other MIPS UNIX.

Ah, I had no idea it was intentional. 

Maybe there could be a cleaner declaration of that intention, though. The only
code there was "I herewith declare: this does not happen.  So send SIGBUS."

Thank you,
Lluís.

Re: [PATCH] MIPS: Add emulation for fpureg-mem unaligned access

[Ralf Baechle]

On Tue, Jul 31, 2012 at 04:07:23PM +0200, Lluís Batlle i Rossell wrote:

> Maybe there could be a cleaner declaration of that intention, though. The only
> code there was "I herewith declare: this does not happen.  So send SIGBUS."

To give you an idea, the emulation is on the order of a 1000 times slower
than the processing a properly aligned load in hardware.  And even where
hardware does unaligned handling such as on x86 there still is a performance
penalty though that would far less severe.

So given that proper alignment is always the right thing.  There are very
few cases were handling misalignment in software is justified, for example
the IP stack.  Even the checks if a packet is misaligned would cause a
performance penalty and it's (assuming sane networking hardware) a very
rare event.

lwl/lwr in the IP stack would be a bad tradeoff.  It's faster than the
unaligned exception handler but would slow down processing of every
correctly aligned packet.  So lwl/lwr are only a good choice where a high
fraction of misaligned packets is expected.

  Ralf