Hi Catalin and Will,
On 01/-10/-28163 02:59 PM, Catalin Marinas wrote:
> The patch contains the exception entry code (kernel/entry.S), pt_regs
> structure and related accessors, undefined instruction trapping and
> stack tracing.
>
> AArch64 Linux kernel (including kernel threads) runs in EL1 mode using
> the SP1 stack. The vectors don't have a fixed address, only alignment
> (2^11) requirements.
[...]
> diff --git a/arch/aarch64/kernel/entry.S b/arch/aarch64/kernel/entry.S
> new file mode 100644
> index 0000000..8ad3995
> --- /dev/null
> +++ b/arch/aarch64/kernel/entry.S
> @@ -0,0 +1,696 @@
> +/*
> + * Low-level exception handling code
[...]
> +/*
> + * Exception vectors.
> + */
> + .macro ventry label
> + .align 7
> + b \label
> + .endm
> +
> + .align 11
> +ENTRY(vectors)
> + ventry el1_sync_invalid // Synchronous EL1t
> + ventry el1_irq_invalid // IRQ EL1t
> + ventry el1_fiq_invalid // FIQ EL1t
> + ventry el1_error_invalid // Error EL1t
> +
> + ventry el1_sync // Synchronous EL1h
> + ventry el1_irq // IRQ EL1h
> + ventry el1_fiq_invalid // FIQ EL1h
> + ventry el1_error_invalid // Error EL1h
> +
> + ventry el0_sync // Synchronous 64-bit EL0
> + ventry el0_irq // IRQ 64-bit EL0
> + ventry el0_fiq_invalid // FIQ 64-bit EL0
> + ventry el0_error_invalid // Error 64-bit EL0
> +
> +#ifdef CONFIG_AARCH32_EMULATION
> + ventry el0_sync_compat // Synchronous 32-bit EL0
> + ventry el0_irq_compat // IRQ 32-bit EL0
> + ventry el0_fiq_invalid_compat // FIQ 32-bit EL0
> + ventry el0_error_invalid_compat // Error 32-bit EL0
> +#else
> + ventry el0_sync_invalid // Synchronous 32-bit EL0
> + ventry el0_irq_invalid // IRQ 32-bit EL0
> + ventry el0_fiq_invalid // FIQ 32-bit EL0
> + ventry el0_error_invalid // Error 32-bit EL0
> +#endif
> +END(vectors)
> +
> +/*
> + * Invalid mode handlers
> + */
> + .macro inv_entry, el, reason, regsize = 64
> + kernel_entry el, \regsize
> + mov x0, sp
> + mov x1, #\reason
> + mrs x2, esr_el1
> + b bad_mode
> + .endm
The code seems to indicate that the invalid mode handlers have different alignment requirements than the valid mode handlers, which puzzles me.
> +
> +el0_sync_invalid:
> + inv_entry 0, BAD_SYNC
> +ENDPROC(el0_sync_invalid)
Plain labels, the ENTRY macro, the END macro and the ENDPROC macro are used variously throughout this file, and I wonder if a greater amount of consistency might be attainable. The description of the ENDPROC macro in include/linux/linkage.h makes me think its use might not be completely warranted in blocks of assembly that don't end with a return instruction.
> +el0_irq_invalid:
> + inv_entry 0, BAD_IRQ
> +ENDPROC(el0_irq_invalid)
> +
> +el0_fiq_invalid:
> + inv_entry 0, BAD_FIQ
> +ENDPROC(el0_fiq_invalid)
> +
> +el0_error_invalid:
> + inv_entry 0, BAD_ERROR
> +ENDPROC(el0_error_invalid)
> +
> +#ifdef CONFIG_AARCH32_EMULATION
> +el0_fiq_invalid_compat:
> + inv_entry 0, BAD_FIQ, 32
> +ENDPROC(el0_fiq_invalid_compat)
> +
> +el0_error_invalid_compat:
> + inv_entry 0, BAD_ERROR, 32
> +ENDPROC(el0_error_invalid_compat)
> +#endif
> +
> +el1_sync_invalid:
> + inv_entry 1, BAD_SYNC
> +ENDPROC(el1_sync_invalid)
> +
> +el1_irq_invalid:
> + inv_entry 1, BAD_IRQ
> +ENDPROC(el1_irq_invalid)
> +
> +el1_fiq_invalid:
> + inv_entry 1, BAD_FIQ
> +ENDPROC(el1_fiq_invalid)
> +
> +el1_error_invalid:
> + inv_entry 1, BAD_ERROR
> +ENDPROC(el1_error_invalid)
> +
> +/*
> + * EL1 mode handlers.
> + */
> + .align 6
> +el1_sync:
> + kernel_entry 1
> + mrs x1, esr_el1 // read the syndrome register
> + lsr x24, x1, #26 // exception class
> + cmp x24, #0x25 // data abort in EL1
> + b.eq el1_da
> + cmp x24, #0x18 // configurable trap
> + b.eq el1_undef
> + cmp x24, #0x26 // stack alignment exception
> + b.eq el1_sp_pc
> + cmp x24, #0x22 // pc alignment exception
> + b.eq el1_sp_pc
> + cmp x24, #0x00 // unknown exception in EL1
> + b.eq el1_undef
> + cmp x24, #0x30 // debug exception in EL1
> + b.ge el1_dbg
> + b el1_inv
> +el1_da:
> + /*
> + * Data abort handling
> + */
> + mrs x0, far_el1
> + enable_dbg_if_not_stepping x2
> + // re-enable interrupts if they were enabled in the aborted context
> + tbnz x23, #7, 1f // PSR_I_BIT
> + enable_irq
> +1:
> + mov x2, sp // struct pt_regs
> + bl do_mem_abort
> +
> + // disable interrupts before pulling preserved data off the stack
> + disable_irq
> + kernel_exit 1
> +el1_sp_pc:
> + /*
> + *Stack or PC alignment exception handling
> + */
> + mrs x0, far_el1
> + mov x1, x25
> + mov x2, sp
> + b do_sp_pc_abort
> +el1_undef:
> + /*
> + *Undefined instruction
> + */
> + mov x0, sp
> + b do_undefinstr
> +el1_dbg:
> + /*
> + * Debug exception handling
> + */
> + tbz x24, #0, el1_inv // EL1 only
> + mrs x0, far_el1
> + mov x2, sp // struct pt_regs
> + bl do_debug_exception
> +
> + kernel_exit 1
> +el1_inv:
> + // TODO: add support for undefined instructions in kernel mode
> + mov x0, sp
> + mov x1, #BAD_SYNC
> + mrs x2, esr_el1
> + b bad_mode
> +ENDPROC(el1_sync)
> +
> + .align 6
> +el1_irq:
> + kernel_entry 1
> + enable_dbg_if_not_stepping x0
> +#ifdef CONFIG_TRACE_IRQFLAGS
> + bl trace_hardirqs_off
> +#endif
> +#ifdef CONFIG_PREEMPT
> + get_thread_info tsk
> + ldr x24, [tsk, #TI_PREEMPT] // get preempt count
> + add x0, x24, #1 // increment it
> + str x0, [tsk, #TI_PREEMPT]
> +#endif
> + irq_handler
> +#ifdef CONFIG_PREEMPT
> + str x24, [tsk, #TI_PREEMPT] // restore preempt count
> + cbnz x24, 1f // preempt count != 0
> + ldr x0, [tsk, #TI_FLAGS] // get flags
> + tbz x0, #TIF_NEED_RESCHED, 1f // needs rescheduling?
> + bl el1_preempt
> +1:
> +#endif
> +#ifdef CONFIG_TRACE_IRQFLAGS
> + bl trace_hardirqs_on
> +#endif
> + kernel_exit 1
> +ENDPROC(el1_irq)
> +
> +#ifdef CONFIG_PREEMPT
> +el1_preempt:
> + mov x24, lr
> +1: enable_dbg
> + bl preempt_schedule_irq // irq en/disable is done inside
> + ldr x0, [tsk, #TI_FLAGS] // get new tasks TI_FLAGS
> + tbnz x0, #TIF_NEED_RESCHED, 1b // needs rescheduling?
> + ret x24
> +#endif
> +
> +/*
> + * EL0 mode handlers.
> + */
> + .align 6
> +el0_sync:
> + kernel_entry 0
> + mrs x25, esr_el1 // read the syndrome register
> + lsr x24, x25, #26 // exception class
> + cmp x24, #0x15 // SVC in 64-bit state
> + b.eq el0_svc
> + adr lr, ret_from_exception
> + cmp x24, #0x24 // data abort in EL0
> + b.eq el0_da
> + cmp x24, #0x20 // instruction abort in EL0
> + b.eq el0_ia
> + cmp x24, #0x07 // FP/ASIMD access
> + b.eq el0_fpsimd_acc
> + cmp x24, #0x2c // FP/ASIMD exception
> + b.eq el0_fpsimd_exc
> + cmp x24, #0x18 // configurable trap
> + b.eq el0_undef
> + cmp x24, #0x26 // stack alignment exception
> + b.eq el0_sp_pc
> + cmp x24, #0x22 // pc alignment exception
> + b.eq el0_sp_pc
> + cmp x24, #0x00 // unknown exception in EL0
> + b.eq el0_undef
> + cmp x24, #0x30 // debug exception in EL0
> + b.ge el0_dbg
> + b el0_inv
> +
> +#ifdef CONFIG_AARCH32_EMULATION
> + .align 6
> +el0_sync_compat:
> + kernel_entry 0, 32
> + mrs x25, esr_el1 // read the syndrome register
> + lsr x24, x25, #26 // exception class
> + cmp x24, #0x11 // SVC in 32-bit state
> + b.eq el0_svc_compat
> + adr lr, ret_from_exception
> + cmp x24, #0x24 // data abort in EL0
> + b.eq el0_da
> + cmp x24, #0x20 // instruction abort in EL0
> + b.eq el0_ia
> + cmp x24, #0x07 // FP/ASIMD access
> + b.eq el0_fpsimd_acc
> + cmp x24, #0x28 // FP/ASIMD exception
> + b.eq el0_fpsimd_exc
> + cmp x24, #0x00 // unknown exception in EL0
> + b.eq el0_undef
> + cmp x24, #0x30 // debug exception in EL0
> + b.ge el0_dbg
> + b el0_inv
> +el0_svc_compat:
> + /*
> + * AArch32 syscall handling
> + */
> + adr stbl, compat_sys_call_table // load compat syscall table pointer
> + uxtw scno, w7 // syscall number in w7 (r7)
> + mov sc_nr, #__NR_compat_syscalls
> + b el0_svc_naked
> +
> + .align 6
> +el0_irq_compat:
> + kernel_entry 0, 32
> + b el0_irq_naked
> +#endif
> +
> +el0_da:
> + /*
> + * Data abort handling
> + */
> + mrs x0, far_el1
> + disable_step x1
> + isb
> + enable_dbg
> + // enable interrupts before calling the main handler
> + enable_irq
> + mov x1, x25
> + mov x2, sp
> + b do_mem_abort
> +el0_ia:
> + /*
> + * Instruction abort handling
> + */
> + mrs x0, far_el1
> + disable_step x1
> + isb
> + enable_dbg
> + // enable interrupts before calling the main handler
> + enable_irq
> + orr x1, x25, #1 << 24 // use reserved ISS bit for instruction aborts
> + mov x2, sp
> + b do_mem_abort
> +el0_fpsimd_acc:
> + /*
> + * Floating Point or Advanced SIMD access
> + */
> + mov x0, x25
> + mov x1, sp
> + b do_fpsimd_acc
> +el0_fpsimd_exc:
> + /*
> + * Floating Point or Advanced SIMD exception
> + */
> + mov x0, x25
> + mov x1, sp
> + b do_fpsimd_exc
> +el0_sp_pc:
> + /*
> + * Stack or PC alignment exception handling
> + */
> + mrs x0, far_el1
> + disable_step x1
> + isb
> + enable_dbg
> + // enable interrupts before calling the main handler
> + enable_irq
> + mov x1, x25
> + mov x2, sp
> + b do_sp_pc_abort
> +el0_undef:
> + /*
> + *Undefined instruction
> + */
> + mov x0, sp
> + b do_undefinstr
> +el0_dbg:
> + /*
> + * Debug exception handling
> + */
> + tbnz x24, #0, el0_inv // EL0 only
> + mrs x0, far_el1
> + disable_step x1
> + mov x1, x25
> + mov x2, sp
> + b do_debug_exception
> +el0_inv:
> + mov x0, sp
> + mov x1, #BAD_SYNC
> + mrs x2, esr_el1
> + b bad_mode
> +ENDPROC(el0_sync)
> +
> + .align 6
> +el0_irq:
> + kernel_entry 0
> +el0_irq_naked:
> + disable_step x1
> + isb
> + enable_dbg
> +#ifdef CONFIG_TRACE_IRQFLAGS
> + bl trace_hardirqs_off
> +#endif
> + get_thread_info tsk
> +#ifdef CONFIG_PREEMPT
> + ldr x24, [tsk, #TI_PREEMPT] // get preempt count
> + add x23, x24, #1 // increment it
> + str x23, [tsk, #TI_PREEMPT]
> +#endif
> + irq_handler
> +#ifdef CONFIG_PREEMPT
> + ldr x0, [tsk, #TI_PREEMPT]
> + str x24, [tsk, #TI_PREEMPT]
> + cmp x0, x23
> + b.eq 1f
> + mov x1, #0
> + str x1, [x1] // BUG
It looks like the error handling here isn't quite complete.
> +1:
> +#endif
> +#ifdef CONFIG_TRACE_IRQFLAGS
> + bl trace_hardirqs_on
> +#endif
> + b ret_to_user
> +ENDPROC(el0_irq)
[...]
Regards,
Christopher
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Hi Christopher,
On Thu, Aug 09, 2012 at 06:05:36PM +0100, Christopher Covington wrote:
> On 01/-10/-28163 02:59 PM, Catalin Marinas wrote:
> > +/*
> > + * Exception vectors.
> > + */
> > + .macro ventry label
> > + .align 7
> > + b \label
> > + .endm
> > +
> > + .align 11
> > +ENTRY(vectors)
> > + ventry el1_sync_invalid // Synchronous EL1t
> > + ventry el1_irq_invalid // IRQ EL1t
> > + ventry el1_fiq_invalid // FIQ EL1t
> > + ventry el1_error_invalid // Error EL1t
> > +
> > + ventry el1_sync // Synchronous EL1h
> > + ventry el1_irq // IRQ EL1h
> > + ventry el1_fiq_invalid // FIQ EL1h
> > + ventry el1_error_invalid // Error EL1h
> > +
> > + ventry el0_sync // Synchronous 64-bit EL0
> > + ventry el0_irq // IRQ 64-bit EL0
> > + ventry el0_fiq_invalid // FIQ 64-bit EL0
> > + ventry el0_error_invalid // Error 64-bit EL0
> > +
> > +#ifdef CONFIG_AARCH32_EMULATION
> > + ventry el0_sync_compat // Synchronous 32-bit EL0
> > + ventry el0_irq_compat // IRQ 32-bit EL0
> > + ventry el0_fiq_invalid_compat // FIQ 32-bit EL0
> > + ventry el0_error_invalid_compat // Error 32-bit EL0
> > +#else
> > + ventry el0_sync_invalid // Synchronous 32-bit EL0
> > + ventry el0_irq_invalid // IRQ 32-bit EL0
> > + ventry el0_fiq_invalid // FIQ 32-bit EL0
> > + ventry el0_error_invalid // Error 32-bit EL0
> > +#endif
> > +END(vectors)
> > +
> > +/*
> > + * Invalid mode handlers
> > + */
> > + .macro inv_entry, el, reason, regsize = 64
> > + kernel_entry el, \regsize
> > + mov x0, sp
> > + mov x1, #\reason
> > + mrs x2, esr_el1
> > + b bad_mode
> > + .endm
>
> The code seems to indicate that the invalid mode handlers have
> different alignment requirements than the valid mode handlers, which
> puzzles me.
I don't see any difference. The whole vector must be 2K aligned while
each individual entry is found every 128 bytes (to allow for more
instructions, though we only use a branch).
The inv_entry macro (as the kernel_entry one) is used in code being
branched to from the vector and not inside the vector.
> > +el0_sync_invalid:
> > + inv_entry 0, BAD_SYNC
> > +ENDPROC(el0_sync_invalid)
>
> Plain labels, the ENTRY macro, the END macro and the ENDPROC macro are
> used variously throughout this file, and I wonder if a greater amount
> of consistency might be attainable. The description of the ENDPROC
> macro in include/linux/linkage.h makes me think its use might not be
> completely warranted in blocks of assembly that don't end with a
> return instruction.
We use ENTRY only when we want to export the symbol as it contains the
.globl directive. The ENDPROC is used to mark a function and it's in
general useful for debugging information it generates.
> > + .align 6
> > +el0_irq:
> > + kernel_entry 0
> > +el0_irq_naked:
> > + disable_step x1
> > + isb
> > + enable_dbg
> > +#ifdef CONFIG_TRACE_IRQFLAGS
> > + bl trace_hardirqs_off
> > +#endif
> > + get_thread_info tsk
> > +#ifdef CONFIG_PREEMPT
> > + ldr x24, [tsk, #TI_PREEMPT] // get preempt count
> > + add x23, x24, #1 // increment it
> > + str x23, [tsk, #TI_PREEMPT]
> > +#endif
> > + irq_handler
> > +#ifdef CONFIG_PREEMPT
> > + ldr x0, [tsk, #TI_PREEMPT]
> > + str x24, [tsk, #TI_PREEMPT]
> > + cmp x0, x23
> > + b.eq 1f
> > + mov x1, #0
> > + str x1, [x1] // BUG
>
> It looks like the error handling here isn't quite complete.
We trigger a bug by storing to 0 and the kernel will panic, giving the
full trace. I don't think we can do more in terms of error handling
here.
Regards.
--
Catalin
Hi Catalin,
Thanks for your response.
On 08/09/2012 01:23 PM, Catalin Marinas wrote:
> Hi Christopher,
>
> On Thu, Aug 09, 2012 at 06:05:36PM +0100, Christopher Covington wrote:
>> On 01/-10/-28163 02:59 PM, Catalin Marinas wrote:
>>> +/*
>>> + * Exception vectors.
>>> + */
>>> + .macro ventry label
>>> + .align 7
>>> + b \label
>>> + .endm
>>> +
>>> + .align 11
>>> +ENTRY(vectors)
>>> + ventry el1_sync_invalid // Synchronous EL1t
>>> + ventry el1_irq_invalid // IRQ EL1t
>>> + ventry el1_fiq_invalid // FIQ EL1t
>>> + ventry el1_error_invalid // Error EL1t
>>> +
>>> + ventry el1_sync // Synchronous EL1h
>>> + ventry el1_irq // IRQ EL1h
>>> + ventry el1_fiq_invalid // FIQ EL1h
>>> + ventry el1_error_invalid // Error EL1h
>>> +
>>> + ventry el0_sync // Synchronous 64-bit EL0
>>> + ventry el0_irq // IRQ 64-bit EL0
>>> + ventry el0_fiq_invalid // FIQ 64-bit EL0
>>> + ventry el0_error_invalid // Error 64-bit EL0
>>> +
>>> +#ifdef CONFIG_AARCH32_EMULATION
>>> + ventry el0_sync_compat // Synchronous 32-bit EL0
>>> + ventry el0_irq_compat // IRQ 32-bit EL0
>>> + ventry el0_fiq_invalid_compat // FIQ 32-bit EL0
>>> + ventry el0_error_invalid_compat // Error 32-bit EL0
>>> +#else
>>> + ventry el0_sync_invalid // Synchronous 32-bit EL0
>>> + ventry el0_irq_invalid // IRQ 32-bit EL0
>>> + ventry el0_fiq_invalid // FIQ 32-bit EL0
>>> + ventry el0_error_invalid // Error 32-bit EL0
>>> +#endif
>>> +END(vectors)
>>> +
>>> +/*
>>> + * Invalid mode handlers
>>> + */
>>> + .macro inv_entry, el, reason, regsize = 64
>>> + kernel_entry el, \regsize
>>> + mov x0, sp
>>> + mov x1, #\reason
>>> + mrs x2, esr_el1
>>> + b bad_mode
>>> + .endm
>>
>> The code seems to indicate that the invalid mode handlers have
>> different alignment requirements than the valid mode handlers, which
>> puzzles me.
>
> I don't see any difference. The whole vector must be 2K aligned while
> each individual entry is found every 128 bytes (to allow for more
> instructions, though we only use a branch).
>
> The inv_entry macro (as the kernel_entry one) is used in code being
> branched to from the vector and not inside the vector.
Sorry to not be clearer. I meant this in relation to the handlers that the vectors branch to, rather than the vectors themselves. For example, an .align 6 directive is used for el0_irq, but not for el0_irq_invalid.
>>> +el0_sync_invalid:
>>> + inv_entry 0, BAD_SYNC
>>> +ENDPROC(el0_sync_invalid)
>>
>> Plain labels, the ENTRY macro, the END macro and the ENDPROC macro are
>> used variously throughout this file, and I wonder if a greater amount
>> of consistency might be attainable. The description of the ENDPROC
>> macro in include/linux/linkage.h makes me think its use might not be
>> completely warranted in blocks of assembly that don't end with a
>> return instruction.
>
> We use ENTRY only when we want to export the symbol as it contains the
> .globl directive. The ENDPROC is used to mark a function and it's in
> general useful for debugging information it generates.
Does code that has no returning path, such as el0_sync_invalid, fully qualify as a function? On the flip side, it appears to me that el1_preempt does qualify and should get an ENDPROC.
>>> + .align 6
>>> +el0_irq:
>>> + kernel_entry 0
>>> +el0_irq_naked:
>>> + disable_step x1
>>> + isb
>>> + enable_dbg
>>> +#ifdef CONFIG_TRACE_IRQFLAGS
>>> + bl trace_hardirqs_off
>>> +#endif
>>> + get_thread_info tsk
>>> +#ifdef CONFIG_PREEMPT
>>> + ldr x24, [tsk, #TI_PREEMPT] // get preempt count
>>> + add x23, x24, #1 // increment it
>>> + str x23, [tsk, #TI_PREEMPT]
>>> +#endif
>>> + irq_handler
>>> +#ifdef CONFIG_PREEMPT
>>> + ldr x0, [tsk, #TI_PREEMPT]
>>> + str x24, [tsk, #TI_PREEMPT]
>>> + cmp x0, x23
>>> + b.eq 1f
>>> + mov x1, #0
>>> + str x1, [x1] // BUG
>>
>> It looks like the error handling here isn't quite complete.
>
> We trigger a bug by storing to 0 and the kernel will panic, giving the
> full trace. I don't think we can do more in terms of error handling
> here.
The approach is concise and clever. However, I think it sacrifices clarity to some extent. I worry that the top of the stack trace will be populated with extraneous data fault handling routines. Even if branching to do_mem_abort was ideal, I feel like getting there by way of the address translation hardware and yet another exception vector adds a number of unnecessary variables to that particular state transition. Perhaps branching to a wrapper around panic(...) would handle the error in a more obvious manner?
Thanks,
Christopher
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum