lkml.org 
[lkml]   [2017]   [Oct]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] MIPS: Fix exception entry when CONFIG_EVA enabled
From
Date
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:


\
 
 \ /
  Last update: 2017-10-11 15:13    [W:0.071 / U:0.688 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site