[PATCH] MIPS: Fix exception entry when CONFIG_EVA enabled

[PATCH] MIPS: Fix exception entry when CONFIG_EVA enabled

[Matt Redfearn]

Commit 9fef68686317b ("MIPS: Make SAVE_SOME more standard") made several
changes to the order in which registers are saved in the SAVE_SOME
macro, used by exception handlers to save the processor state. In
particular, it removed the
move   k1, sp
in the delay slot of the branch testing if the processor is already in
kernel mode. This is replaced later in the macro by a
move   k0, sp
When CONFIG_EVA is disabled, this instruction actually appears in the
delay slot of the branch. However, when CONFIG_EVA is enabled, instead
the RPS workaround of
MFC0	k0, CP0_ENTRYHI
appears in the delay slot. This results in k0 not containing the stack
pointer, but some unrelated value, which is then saved to the kernel
stack. On exit from the exception, this bogus value is restored to the
stack pointer, resulting in an OOPS.

Fix this by moving the save of SP in k0 explicitly in the delay slot of
the branch, outside of the CONFIG_EVA section, restoring the expected
instruction ordering when CONFIG_EVA is active.

Fixes: 9fef68686317b ("MIPS: Make SAVE_SOME more standard")
Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
Reported-by: Vladimir Kondratiev <vladimir.kondratiev@intel.com>

---

Note that some of our compiler people are dubious about putting frame
related instructions in conditionally executed blocks of code. In this
case, presuming that we only care about unwinding the kernel stack, then
we only care about the case in which the branch is taken, and k0 always
contains the SP to be saved. There is also a question about putting
frame related instructions in branch delay slots. Again, in this case,
we think it's OK to use them since the only path that ought to be
unwound will be the "branch taken" route where we are already on the
kernel stack.
Not having access to a CFI based kernel stack unwinder makes this change
difficult to verify, but since the same construct already existed when
CONFIG_EVA is disabled, I don't think this change is likely to break the
unwinder, and fixes exception entry when CONFIG_EVA is enabled.

Thanks,
Matt

---
 arch/mips/include/asm/stackframe.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/mips/include/asm/stackframe.h b/arch/mips/include/asm/stackframe.h
index 5d3563c55e0c..2161357cc68f 100644
--- a/arch/mips/include/asm/stackframe.h
+++ b/arch/mips/include/asm/stackframe.h
@@ -199,6 +199,10 @@
 		sll	k0, 3		/* extract cu0 bit */
 		.set	noreorder
 		bltz	k0, 8f
+		 move	k0, sp
+		.if \docfi
+		.cfi_register sp, k0
+		.endif
 #ifdef CONFIG_EVA
 		/*
 		 * Flush interAptiv's Return Prediction Stack (RPS) by writing
@@ -225,10 +229,6 @@
 		MTC0	k0, CP0_ENTRYHI
 #endif
 		.set	reorder
-		 move	k0, sp
-		.if \docfi
-		.cfi_register sp, k0
-		.endif
 		/* Called from user mode, new stack. */
 		get_saved_sp docfi=\docfi tosp=1
 8:
-- 
2.7.4

Re: [PATCH] MIPS: Fix exception entry when CONFIG_EVA enabled

[Corey Minyard]

On 10/11/2017 03:59 AM, Matt Redfearn wrote:
> Commit 9fef68686317b ("MIPS: Make SAVE_SOME more standard") made several
> changes to the order in which registers are saved in the SAVE_SOME
> macro, used by exception handlers to save the processor state. In
> particular, it removed the
> move   k1, sp
> in the delay slot of the branch testing if the processor is already in
> kernel mode. This is replaced later in the macro by a
> move   k0, sp
> When CONFIG_EVA is disabled, this instruction actually appears in the
> delay slot of the branch. However, when CONFIG_EVA is enabled, instead
> the RPS workaround of
> MFC0	k0, CP0_ENTRYHI
> appears in the delay slot. This results in k0 not containing the stack
> pointer, but some unrelated value, which is then saved to the kernel
> stack. On exit from the exception, this bogus value is restored to the
> stack pointer, resulting in an OOPS.
>
> Fix this by moving the save of SP in k0 explicitly in the delay slot of
> the branch, outside of the CONFIG_EVA section, restoring the expected
> instruction ordering when CONFIG_EVA is active.
>
> Fixes: 9fef68686317b ("MIPS: Make SAVE_SOME more standard")
> Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
> Reported-by: Vladimir Kondratiev <vladimir.kondratiev@intel.com>

I looked this over pretty carefully and it looks correct to me.  It 
makes no difference
in the instructions generated by the non-EVA case.  I shouldn't have 
missed this :(.

Reviewed-by: Corey Minyard <cminyard@mvista.com>

>
> ---
>
> Note that some of our compiler people are dubious about putting frame
> related instructions in conditionally executed blocks of code. In this
> case, presuming that we only care about unwinding the kernel stack, then
> we only care about the case in which the branch is taken, and k0 always
> contains the SP to be saved. There is also a question about putting
> frame related instructions in branch delay slots. Again, in this case,
> we think it's OK to use them since the only path that ought to be
> unwound will be the "branch taken" route where we are already on the
> kernel stack.

Since the compiler can put frame-related instructions in delay slots (see
aee16625b19 MIPS: Fix issues in backtraces), it's probably ok.  I have 
tested
this before with kernel dumps and gdb, and gdb had no issues with this.

That said, this is a tricky case.  But looking at the generated unwinding
info, it seems to do the right thing.

> Not having access to a CFI based kernel stack unwinder makes this change
> difficult to verify, but since the same construct already existed when
> CONFIG_EVA is disabled, I don't think this change is likely to break the
> unwinder, and fixes exception entry when CONFIG_EVA is enabled.

Agreed.  Thanks for fixing this.

-corey

> Thanks,
> Matt
>
> ---
>   arch/mips/include/asm/stackframe.h | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/mips/include/asm/stackframe.h b/arch/mips/include/asm/stackframe.h
> index 5d3563c55e0c..2161357cc68f 100644
> --- a/arch/mips/include/asm/stackframe.h
> +++ b/arch/mips/include/asm/stackframe.h
> @@ -199,6 +199,10 @@
>   		sll	k0, 3		/* extract cu0 bit */
>   		.set	noreorder
>   		bltz	k0, 8f
> +		 move	k0, sp
> +		.if \docfi
> +		.cfi_register sp, k0
> +		.endif
>   #ifdef CONFIG_EVA
>   		/*
>   		 * Flush interAptiv's Return Prediction Stack (RPS) by writing
> @@ -225,10 +229,6 @@
>   		MTC0	k0, CP0_ENTRYHI
>   #endif
>   		.set	reorder
> -		 move	k0, sp
> -		.if \docfi
> -		.cfi_register sp, k0
> -		.endif
>   		/* Called from user mode, new stack. */
>   		get_saved_sp docfi=\docfi tosp=1
>   8:

Re: [PATCH] MIPS: Fix exception entry when CONFIG_EVA enabled

[Maciej W. Rozycki]

On Wed, 11 Oct 2017, Matt Redfearn wrote:

> diff --git a/arch/mips/include/asm/stackframe.h b/arch/mips/include/asm/stackframe.h
> index 5d3563c55e0c..2161357cc68f 100644
> --- a/arch/mips/include/asm/stackframe.h
> +++ b/arch/mips/include/asm/stackframe.h
> @@ -199,6 +199,10 @@
>  		sll	k0, 3		/* extract cu0 bit */
>  		.set	noreorder
>  		bltz	k0, 8f
> +		 move	k0, sp
> +		.if \docfi
> +		.cfi_register sp, k0
> +		.endif

 Using $k1 for the Status.CU0 check would let us get rid of the 
`noreorder' block, making this code less fragile at no run-time cost, 
i.e.:

		mfc0	k1, CP0_STATUS
		sll	k1, 3
		move	k0, sp
		bltz	k1, 8f

(unfortunately I can't see a way to usefully fill the coprocessor move 
delay slot automatically scheduled by GAS here between MFC0 and SLL for 
processors that require it).

  Maciej

Re: [PATCH] MIPS: Fix exception entry when CONFIG_EVA enabled

[James Hogan]

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

On Wed, Oct 11, 2017 at 08:12:31AM -0500, Corey Minyard wrote:
> On 10/11/2017 03:59 AM, Matt Redfearn wrote:
> > Commit 9fef68686317b ("MIPS: Make SAVE_SOME more standard") made several
> > changes to the order in which registers are saved in the SAVE_SOME
> > macro, used by exception handlers to save the processor state. In
> > particular, it removed the
> > move   k1, sp
> > in the delay slot of the branch testing if the processor is already in
> > kernel mode. This is replaced later in the macro by a
> > move   k0, sp
> > When CONFIG_EVA is disabled, this instruction actually appears in the
> > delay slot of the branch. However, when CONFIG_EVA is enabled, instead
> > the RPS workaround of
> > MFC0	k0, CP0_ENTRYHI
> > appears in the delay slot. This results in k0 not containing the stack
> > pointer, but some unrelated value, which is then saved to the kernel
> > stack. On exit from the exception, this bogus value is restored to the
> > stack pointer, resulting in an OOPS.
> >
> > Fix this by moving the save of SP in k0 explicitly in the delay slot of
> > the branch, outside of the CONFIG_EVA section, restoring the expected
> > instruction ordering when CONFIG_EVA is active.
> >
> > Fixes: 9fef68686317b ("MIPS: Make SAVE_SOME more standard")
> > Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
> > Reported-by: Vladimir Kondratiev <vladimir.kondratiev@intel.com>
> 
> I looked this over pretty carefully and it looks correct to me.  It 
> makes no difference
> in the instructions generated by the non-EVA case.  I shouldn't have 
> missed this :(.
> 
> Reviewed-by: Corey Minyard <cminyard@mvista.com>

Yeh, having stared at it for a little while it looks correct to me too.

Reviewed-by: James Hogan <jhogan@kernel.org>

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] MIPS: Fix exception entry when CONFIG_EVA enabled

[Maciej W. Rozycki]

On Tue, 31 Oct 2017, James Hogan wrote:

> > I looked this over pretty carefully and it looks correct to me.  It 
> > makes no difference
> > in the instructions generated by the non-EVA case.  I shouldn't have 
> > missed this :(.
> > 
> > Reviewed-by: Corey Minyard <cminyard@mvista.com>
> 
> Yeh, having stared at it for a little while it looks correct to me too.
> 
> Reviewed-by: James Hogan <jhogan@kernel.org>

 How about getting rid of the `noreorder' mode and the manually scheduled 
delay slot here altogether though, as I outlined?

  Maciej

Re: [PATCH] MIPS: Fix exception entry when CONFIG_EVA enabled

[Matt Redfearn]

On 13/11/17 10:47, Maciej W. Rozycki wrote:
> On Tue, 31 Oct 2017, James Hogan wrote:
> 
>>> I looked this over pretty carefully and it looks correct to me.  It
>>> makes no difference
>>> in the instructions generated by the non-EVA case.  I shouldn't have
>>> missed this :(.
>>>
>>> Reviewed-by: Corey Minyard <cminyard@mvista.com>
>>
>> Yeh, having stared at it for a little while it looks correct to me too.
>>
>> Reviewed-by: James Hogan <jhogan@kernel.org>
> 
>   How about getting rid of the `noreorder' mode and the manually scheduled
> delay slot here altogether though, as I outlined?
> 
>    Maciej
> 

Hi Maciej,

I like the change you propose, however I can't coax GAS to reorder the 
instructions appropriately. With this patch on top of 4.14:

--- a/arch/mips/include/asm/stackframe.h
+++ b/arch/mips/include/asm/stackframe.h
@@ -195,14 +195,16 @@
                 .set    push
                 .set    noat
                 .set    reorder
-               mfc0    k0, CP0_STATUS
-               sll     k0, 3           /* extract cu0 bit */
-               .set    noreorder
-               bltz    k0, 8f
-                move   k0, sp
+               mfc0    k1, CP0_STATUS
+               sll     k1, 3           /* extract cu0 bit */
+
+               move    k0, sp
                 .if \docfi
                 .cfi_register sp, k0
                 .endif
+
+               bltz    k1, 8f
+
  #ifdef CONFIG_EVA
                 /*
                  * Flush interAptiv's Return Prediction Stack (RPS) by 
writing
@@ -228,7 +230,6 @@
                 MFC0    k0, CP0_ENTRYHI
                 MTC0    k0, CP0_ENTRYHI
  #endif
-               .set    reorder
                 /* Called from user mode, new stack. */
                 get_saved_sp docfi=\docfi tosp=1
  8:


The generated assembly is:

80405d00 <handle_int>:
80405d00:       401b6000        mfc0    k1,c0_status
80405d04:       001bd8c0        sll     k1,k1,0x3
80405d08:       03a0d025        move    k0,sp
80405d0c:       07600007        bltz    k1,80405d2c <handle_int+0x2c>
80405d10:       00000000        nop
80405d14:       401a2000        mfc0    k0,c0_context

Apparently GAS has not been able to reorder the move into the branch 
delay slot for some reason. Any ideas?

Thanks,
Matt

Re: [PATCH] MIPS: Fix exception entry when CONFIG_EVA enabled

[Maciej W. Rozycki]

On Wed, 15 Nov 2017, Matt Redfearn wrote:

> I like the change you propose, however I can't coax GAS to reorder the
> instructions appropriately. With this patch on top of 4.14:
> 
> --- a/arch/mips/include/asm/stackframe.h
> +++ b/arch/mips/include/asm/stackframe.h
> @@ -195,14 +195,16 @@
>                 .set    push
>                 .set    noat
>                 .set    reorder
> -               mfc0    k0, CP0_STATUS
> -               sll     k0, 3           /* extract cu0 bit */
> -               .set    noreorder
> -               bltz    k0, 8f
> -                move   k0, sp
> +               mfc0    k1, CP0_STATUS
> +               sll     k1, 3           /* extract cu0 bit */
> +
> +               move    k0, sp
>                 .if \docfi
>                 .cfi_register sp, k0
>                 .endif
> +
> +               bltz    k1, 8f
> +
>  #ifdef CONFIG_EVA
>                 /*
>                  * Flush interAptiv's Return Prediction Stack (RPS) by writing
> @@ -228,7 +230,6 @@
>                 MFC0    k0, CP0_ENTRYHI
>                 MTC0    k0, CP0_ENTRYHI
>  #endif
> -               .set    reorder
>                 /* Called from user mode, new stack. */
>                 get_saved_sp docfi=\docfi tosp=1
>  8:
> 
> 
> The generated assembly is:
> 
> 80405d00 <handle_int>:
> 80405d00:       401b6000        mfc0    k1,c0_status
> 80405d04:       001bd8c0        sll     k1,k1,0x3
> 80405d08:       03a0d025        move    k0,sp
> 80405d0c:       07600007        bltz    k1,80405d2c <handle_int+0x2c>
> 80405d10:       00000000        nop
> 80405d14:       401a2000        mfc0    k0,c0_context
> 
> Apparently GAS has not been able to reorder the move into the branch delay
> slot for some reason. Any ideas?

 It could be the `.cfi_register' pseudo-op acting as a scheduling barrier.  
I think it can be moved further down, beyond the branch, because until 
clobbered later on $sp still holds the original value, so using either 
register for frame access or the value itself will yield the same result.

 Can you send me .i output from the offending source along with GCC 
options used to make .o output (use `V=1' with `make' if needed)?  I'll 
check if my hypothesis is right or find the actual cause otherwise.

  Maciej

Re: [PATCH] MIPS: Fix exception entry when CONFIG_EVA enabled

[Maciej W. Rozycki]

Hi Matt,

On Wed, 15 Nov 2017, Maciej W. Rozycki wrote:

>  Can you send me .i output from the offending source along with GCC 
> options used to make .o output (use `V=1' with `make' if needed)?  I'll 
> check if my hypothesis is right or find the actual cause otherwise.

 Thanks for the pieces requested.

 I can see what is going on here: the problem is the source is built with 
no optimisation enabled.  Consequently GAS does not schedule delay slots 
itself.  I wasn't aware we had this problem with kernel build flags -- it 
certainly affects more than just this piece of code.

 For GAS to schedule delay slots it has to be passed -O2 by the GCC 
driver.  The driver in turn will pass -O2 to GAS whenever at least -O has 
been used.  By default -O1 is passed, which only enables the removal of 
unnecessary NOPs, i.e. those that have been inserted precautionarily for 
execution hazard avoidance in the assembly pass and in the end turned out 
unnecessary and can be optionally removed in the relaxation pass.  NB 
GAS's default is actually -O2, i.e. when invoked directly, however this is 
overridden by the GCC driver as described.

 My recommendation would be using just the same CFLAGS for assembly 
sources that are used for C language sources; the GCC driver will silently 
filter out options that are irrelevant and interpret all the options that 
do have relevance.  This may be important for various `-f<foo>' and 
`-m<bar>' options which may sometimes affect assembly generation and, for 
that matter, multilib selection.

 This also means all CFLAGS should be passed to the linker as well, 
because again, they may affect linker options.

 So if you stick say `-O2' with your compiler invocation, for the purpose 
of this consideration possibly just like this:

$ make AFLAGS_genex.o=-O2 arch/mips/kernel/genex.o

then you should get the delay slot in question (and any other ones) 
scheduled.  Indeed with this build recipe applied I can already see a 
small code size reduction in `genex.o' even without your change:

$ size arch/mips/kernel/genex-?.o
   text	   data	    bss	    dec	    hex	filename
   8236	     48	      0	   8284	   205c	arch/mips/kernel/genex-0.o
   8204	     48	      0	   8252	   203c	arch/mips/kernel/genex-1.o
$

 HTH,

  Maciej