| Message ID | 1401130573-7443-2-git-send-email-larry.bassel@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi Larry, On Mon, May 26, 2014 at 07:56:12PM +0100, Larry Bassel wrote: > To implement the context tracker properly on arm64, > a function call needs to be made after debugging and > interrupts are turned on, but before the lr is changed > to point to ret_to_user(). If the function call > is made after the lr is changed the function will not > return to the correct place. > > For similar reasons, defer the setting of x0 so that > it doesn't need to be saved around the function call > (save far_el1 in x26 temporarily instead). > > Signed-off-by: Larry Bassel <larry.bassel@linaro.org> [...] > @@ -476,23 +481,27 @@ el0_undef: > // enable interrupts before calling the main handler > enable_dbg_and_irq > mov x0, sp > + adr lr, ret_to_user > b do_undefinstr > el0_dbg: > /* > * Debug exception handling > */ > tbnz x24, #0, el0_inv // EL0 only > - mrs x0, far_el1 > + mrs x26, far_el1 > + mov x0, x26 > mov x1, x25 > mov x2, sp > bl do_debug_exception > enable_dbg > + mov x0, x26 > b ret_to_user Why have you added this mov instruction? Will -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 28 May 14 12:27, Will Deacon wrote: > Hi Larry, > > On Mon, May 26, 2014 at 07:56:12PM +0100, Larry Bassel wrote: > > To implement the context tracker properly on arm64, > > a function call needs to be made after debugging and > > interrupts are turned on, but before the lr is changed > > to point to ret_to_user(). If the function call > > is made after the lr is changed the function will not > > return to the correct place. > > > > For similar reasons, defer the setting of x0 so that > > it doesn't need to be saved around the function call > > (save far_el1 in x26 temporarily instead). > > > > Signed-off-by: Larry Bassel <larry.bassel@linaro.org> > > [...] > > > Why have you added this mov instruction? I believe (please correct me if I'm wrong) that it is necessary. Here is why: > > @@ -476,23 +481,27 @@ el0_undef: > > // enable interrupts before calling the main handler > > enable_dbg_and_irq > > mov x0, sp > > + adr lr, ret_to_user > > b do_undefinstr > > el0_dbg: > > /* > > * Debug exception handling > > */ > > tbnz x24, #0, el0_inv // EL0 only > > - mrs x0, far_el1 > > + mrs x26, far_el1 needed because do_debug_exception may clobber x0, so save far_el1 in x26 (as other parts of this patch do) > > + mov x0, x26 needed because far_el1 is expected to be in x0 here > > mov x1, x25 > > mov x2, sp > > bl do_debug_exception > > enable_dbg [call to ct_user_exit will go here in the next patch, this may re-clobber x0] > > + mov x0, x26 needed because far_el1 is expected to be in x0 here Since the purpose of this patch is to make calling a function possible in this code path, the "extra" mov instruction above is necessary and IMHO should be added in this patch and not in the next one whose purpose is to define the ct_user_* macros and add calls to them in the proper places. > > b ret_to_user > > Will Larry -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Wed, May 28, 2014 at 08:35:51PM +0100, Larry Bassel wrote: > On 28 May 14 12:27, Will Deacon wrote: > > On Mon, May 26, 2014 at 07:56:12PM +0100, Larry Bassel wrote: > > > To implement the context tracker properly on arm64, > > > a function call needs to be made after debugging and > > > interrupts are turned on, but before the lr is changed > > > to point to ret_to_user(). If the function call > > > is made after the lr is changed the function will not > > > return to the correct place. > > > > > > For similar reasons, defer the setting of x0 so that > > > it doesn't need to be saved around the function call > > > (save far_el1 in x26 temporarily instead). > > > > > > Signed-off-by: Larry Bassel <larry.bassel@linaro.org> > > > > [...] > > > > > > Why have you added this mov instruction? > > I believe (please correct me if I'm wrong) that it is necessary. > Here is why: > > > > @@ -476,23 +481,27 @@ el0_undef: > > > // enable interrupts before calling the main handler > > > enable_dbg_and_irq > > > mov x0, sp > > > + adr lr, ret_to_user > > > b do_undefinstr > > > el0_dbg: > > > /* > > > * Debug exception handling > > > */ > > > tbnz x24, #0, el0_inv // EL0 only > > > - mrs x0, far_el1 > > > + mrs x26, far_el1 > > needed because do_debug_exception may clobber x0, so save far_el1 > in x26 (as other parts of this patch do) Actually, do_debug_exception consumes the FAR as its first parameter, so you don't need to put this in x26 afaict. > > > + mov x0, x26 > > needed because far_el1 is expected to be in x0 here > > > > mov x1, x25 > > > mov x2, sp > > > bl do_debug_exception > > > enable_dbg > > [call to ct_user_exit will go here in the next patch, this may re-clobber x0] > > > > + mov x0, x26 > > needed because far_el1 is expected to be in x0 here Is it? ret_to_user doesn't care. Does ct_user_exit use the FAR? I don't think it does... Will -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S index e8b23a3..c6bc1a3 100644 --- a/arch/arm64/kernel/entry.S +++ b/arch/arm64/kernel/entry.S @@ -354,7 +354,6 @@ el0_sync: lsr x24, x25, #ESR_EL1_EC_SHIFT // exception class cmp x24, #ESR_EL1_EC_SVC64 // SVC in 64-bit state b.eq el0_svc - adr lr, ret_to_user cmp x24, #ESR_EL1_EC_DABT_EL0 // data abort in EL0 b.eq el0_da cmp x24, #ESR_EL1_EC_IABT_EL0 // instruction abort in EL0 @@ -383,7 +382,6 @@ el0_sync_compat: lsr x24, x25, #ESR_EL1_EC_SHIFT // exception class cmp x24, #ESR_EL1_EC_SVC32 // SVC in 32-bit state b.eq el0_svc_compat - adr lr, ret_to_user cmp x24, #ESR_EL1_EC_DABT_EL0 // data abort in EL0 b.eq el0_da cmp x24, #ESR_EL1_EC_IABT_EL0 // instruction abort in EL0 @@ -426,22 +424,25 @@ el0_da: /* * Data abort handling */ - mrs x0, far_el1 - bic x0, x0, #(0xff << 56) + mrs x26, far_el1 // enable interrupts before calling the main handler enable_dbg_and_irq + bic x0, x26, #(0xff << 56) mov x1, x25 mov x2, sp + adr lr, ret_to_user b do_mem_abort el0_ia: /* * Instruction abort handling */ - mrs x0, far_el1 + mrs x26, far_el1 // enable interrupts before calling the main handler enable_dbg_and_irq + mov x0, x26 orr x1, x25, #1 << 24 // use reserved ISS bit for instruction aborts mov x2, sp + adr lr, ret_to_user b do_mem_abort el0_fpsimd_acc: /* @@ -450,6 +451,7 @@ el0_fpsimd_acc: enable_dbg mov x0, x25 mov x1, sp + adr lr, ret_to_user b do_fpsimd_acc el0_fpsimd_exc: /* @@ -458,16 +460,19 @@ el0_fpsimd_exc: enable_dbg mov x0, x25 mov x1, sp + adr lr, ret_to_user b do_fpsimd_exc el0_sp_pc: /* * Stack or PC alignment exception handling */ - mrs x0, far_el1 + mrs x26, far_el1 // enable interrupts before calling the main handler enable_dbg_and_irq + mov x0, x26 mov x1, x25 mov x2, sp + adr lr, ret_to_user b do_sp_pc_abort el0_undef: /* @@ -476,23 +481,27 @@ el0_undef: // enable interrupts before calling the main handler enable_dbg_and_irq mov x0, sp + adr lr, ret_to_user b do_undefinstr el0_dbg: /* * Debug exception handling */ tbnz x24, #0, el0_inv // EL0 only - mrs x0, far_el1 + mrs x26, far_el1 + mov x0, x26 mov x1, x25 mov x2, sp bl do_debug_exception enable_dbg + mov x0, x26 b ret_to_user el0_inv: enable_dbg mov x0, sp mov x1, #BAD_SYNC mrs x2, esr_el1 + adr lr, ret_to_user b bad_mode ENDPROC(el0_sync)
To implement the context tracker properly on arm64, a function call needs to be made after debugging and interrupts are turned on, but before the lr is changed to point to ret_to_user(). If the function call is made after the lr is changed the function will not return to the correct place. For similar reasons, defer the setting of x0 so that it doesn't need to be saved around the function call (save far_el1 in x26 temporarily instead). Signed-off-by: Larry Bassel <larry.bassel@linaro.org> --- arch/arm64/kernel/entry.S | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-)