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

[Corey Minyard <cminyard@mvista.com>]

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: