Message-ID: <50240D4D.80006@codeaurora.org>
Date: Thu, 09 Aug 2012 15:19:41 -0400
From: Christopher Covington <cov@codeaurora.org>
User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.13) Gecko/20101208 Thunderbird/3.1.7
MIME-Version: 1.0
To: Catalin Marinas <catalin.marinas@arm.com>
CC: Will Deacon <Will.Deacon@arm.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Arnd Bergmann <arnd@arndb.de>
Subject: Re: [09/36] AArch64: Exception handling
References: <1341608777-12982-10-git-send-email-catalin.marinas@arm.com> <5023EDE0.3070506@codeaurora.org> <20120809172315.GA12325@arm.com>
In-Reply-To: <20120809172315.GA12325@arm.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4924
Lines: 132

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 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/