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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
