From patchwork Tue Aug 6 03:35:47 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Roy Franz X-Patchwork-Id: 2839147 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 6EBD39F479 for ; Tue, 6 Aug 2013 03:36:20 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 15F5A20179 for ; Tue, 6 Aug 2013 03:36:19 +0000 (UTC) Received: from casper.infradead.org (casper.infradead.org [85.118.1.10]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 885E720177 for ; Tue, 6 Aug 2013 03:36:17 +0000 (UTC) Received: from merlin.infradead.org ([2001:4978:20e::2]) by casper.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1V6Y4E-0008BT-VL; Tue, 06 Aug 2013 03:36:15 +0000 Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1V6Y4C-0000R8-Iz; Tue, 06 Aug 2013 03:36:12 +0000 Received: from mail-vb0-f41.google.com ([209.85.212.41]) by merlin.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1V6Y49-0000QR-9u for linux-arm-kernel@lists.infradead.org; Tue, 06 Aug 2013 03:36:10 +0000 Received: by mail-vb0-f41.google.com with SMTP id g17so3663831vbg.28 for ; Mon, 05 Aug 2013 20:35:47 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:x-gm-message-state; bh=eSbwBK2b0y98+9fcnIUiw/F09somO0PNP7zxMUexAmI=; b=JNGXOzBEuH027V9X6d3awTBxIU80HEndAIqddQsZ+pB4zU6MhlVk5iDh8Z8mdgc7Q0 EX3LTj1aj5xWF31HdqE5x4n4Vf9XHt+anJdMb/Je8haVjmW8Q7OapGu9LDmVQkEgkFz9 QVJuRWq+miijbkdVLcBFUp85N5mMKZhqk2kI1gMx/DjMtv9rvtbMvE8Yh/4mVNPgNfSg Ry/D/MpRul5C8rRhIHcVch3NYhIFrUrEYr9KAVB9gnVrdpDfWERghaQNV/SHL+zCbD3p 8GLsIWmlocyULffrpjOOa5agMOJDmDQFBnj05/xNa0RG1+FqHBdw7dnZ87MHnJuIROfh zAmA== MIME-Version: 1.0 X-Received: by 10.52.75.162 with SMTP id d2mr5754077vdw.86.1375760147779; Mon, 05 Aug 2013 20:35:47 -0700 (PDT) Received: by 10.221.6.74 with HTTP; Mon, 5 Aug 2013 20:35:47 -0700 (PDT) In-Reply-To: <20130805141149.GB2755@localhost.localdomain> References: <1375478948-22562-1-git-send-email-roy.franz@linaro.org> <1375478948-22562-7-git-send-email-roy.franz@linaro.org> <20130805141149.GB2755@localhost.localdomain> Date: Mon, 5 Aug 2013 20:35:47 -0700 Message-ID: Subject: Re: [PATCH 6/7] Add EFI stub for ARM From: Roy Franz To: Dave Martin X-Gm-Message-State: ALoCoQlXc8QcYfmsiAkA2SmocYfGmGIej4S7HWutGVoCHDW/tQvcf4NgySfzhyx4UsSEkJpWWiiE X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20130805_233609_433805_83BB5EE7 X-CRM114-Status: GOOD ( 33.29 ) X-Spam-Score: -2.6 (--) Cc: linux-efi@vger.kernel.org, Russell King - ARM Linux , linux-kernel@vger.kernel.org, Leif Lindholm , matt.fleming@intel.com, "linux-arm-kernel@lists.infradead.org" X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP >> diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S >> index 75189f1..4c70b9e 100644 >> --- a/arch/arm/boot/compressed/head.S >> +++ b/arch/arm/boot/compressed/head.S >> @@ -122,19 +122,106 @@ >> .arm @ Always enter in ARM state >> start: >> .type start,#function >> - .rept 7 >> +#ifdef CONFIG_EFI_STUB >> + @ Magic MSDOS signature for PE/COFF + ADD opcode >> + .word 0x62805a4d > > What about BE32? > > In that case, the instruction is a coprocessor load, that loads from a > random address to a coprocessor that almost certainly doesn't exist. > This will probably fault. > > Since BE32 is only for older platforms ( solvable, it might be sensible to make the EFI stub support depend on > !CPU_ENDIAN_BE32. > >> +#else >> + mov r0, r0 >> +#endif >> + .rept 5 > > You reduced the .rept count by 2, but only inserted one extra word, > perhaps because of the extra, but buggy, insertion below. > >> mov r0, r0 >> .endr >> ARM( mov r0, r0 ) >> ARM( b 1f ) >> THUMB( adr r12, BSYM(1f) ) >> THUMB( bx r12 ) > > Can't you just replace 1f with zimage_continue directly in the above > lines, instead of the subsequent extra branch: > >> + THUMB( .thumb ) >> +1: >> + b zimage_continue > > This also avoids having two labels both called '1'. > > I believe the magic word is expected to be in a predictable offset, > but the size of the preceding branch is unpredictable in Thumb > (you could use b.w, or possibly remove the branch altogether, as > explained above). > >> .word 0x016f2818 @ Magic numbers to help the loader >> .word start @ absolute load/run zImage address >> .word _edata @ zImage end address >> + >> +#ifdef CONFIG_EFI_STUB >> + @ Portions of the MSDOS file header must be at offset >> + @ 0x3c from the start of the file. All PE/COFF headers >> + @ are kept contiguous for simplicity. >> +#include "efi-header.S" >> + >> +efi_stub_entry: >> + .text >> + @ The EFI stub entry point is not at a fixed address, however >> + @ this address must be set in the PE/COFF header. >> + @ EFI entry point is in A32 mode, switch to T32 if configured. >> + .arm >> + ARM( mov r0, r0 ) >> + ARM( b 1f ) > > Those above two instructions are effectively just no-op padding. Do you > need them at all? > >> + THUMB( adr r12, BSYM(1f) ) >> + THUMB( bx r12 ) >> THUMB( .thumb ) >> 1: >> + @ Save lr on stack for possible return to EFI firmware. >> + @ Don't care about fp, but need 64 bit alignment.... >> + stmfd sp!, {fp, lr} >> + >> + @ Save args to EFI app across got fixup call >> + stmfd sp!, {r0, r1} >> + ldmfd sp!, {r0, r1} >> + >> + @ allocate space on stack for return of new entry point of >> + @ zImage, as EFI stub may copy the kernel. Pass address >> + @ of space in r2 - EFI stub will fill in the pointer. >> + >> + sub sp, #8 @ we only need 4 bytes, >> + @ but keep stack 8 byte aligned. >> + mov r2, sp >> + @ Pass our actual runtime start address in pointer data >> + adr r11, LC0 @ address of LC0 at run time >> + ldr r12, [r11, #0] @ address of LC0 at link time >> + >> + sub r3, r11, r12 @ calculate the delta offset >> + str r3, [r2, #0] >> + bl efi_entry >> + >> + @ get new zImage entry address from stack, put into r3 >> + ldr r3, [sp, #0] >> + add sp, #8 @ restore stack >> + >> + @ Check for error return from EFI stub (0xFFFFFFFF) >> + ldr r1, =0xffffffff >> + cmp r0, r1 >> + beq efi_load_fail >> + >> + >> + @ Save return values of efi_entry >> + stmfd sp!, {r0, r3} >> + bl cache_clean_flush >> + bl cache_off >> + ldmfd sp!, {r0, r3} >> + >> + @ put DTB address in r2, it was returned by EFI entry >> + mov r2, r0 >> + ldr r1, =0xffffffff @ DTB machine type >> + mov r0, #0 @ r0 is 0 >> + >> + @ Branch to (possibly) relocated zImage entry that is in r3 >> + bx r3 >> + >> +efi_load_fail: >> + @ We need to exit THUMB mode here, to return to EFI firmware. > > If you lose these 4 lines: > >> + THUMB( adr r12, BSYM(1f) ) >> + THUMB( bx r12 ) >> +1: >> + .arm > > ... > >> + @ Return EFI_LOAD_ERROR to EFI firmware on error. >> + ldr r0, =0x80000001 > > and replace these > >> + ldmfd sp!, {fp, lr} >> + mov pc, lr > > with: > > ldmfd sp!, {fp, pc} > > then the Thumb-ness on return will be determined by whatever is loaded > into pc. > > There's no special need to switch back to ARM before the final return, > provided that "mov pc," is not used (that will never switch from > Thumb to ARM). > > > Cheers > ---Dave > >> + THUMB( .thumb ) > > Then you can also remove the above line. > >> +#endif >> + >> +zimage_continue: >> mrs r9, cpsr >> #ifdef CONFIG_ARM_VIRT_EXT >> bl __hyp_stub_install @ get into SVC mode, reversibly >> @@ -167,7 +254,6 @@ not_angel: >> * by the linker here, but it should preserve r7, r8, and r9. >> */ >> >> - .text >> >> #ifdef CONFIG_AUTO_ZRELADDR >> @ determine final kernel image address >> -- Hi Dave, Thanks for your suggestions - they do make the code cleaner. I think I have addressed all your suggestions in the patch below. In addition, I noticed that I had moved the start of the .text section to within the #ifdef CONFIG_EFI_STUB, so I have fixed that. Also, the previous return to the EFI stub was broken in the THUMB case - it was staying in thumb mode - this is now fixed with a cleaner return as you suggested. Updated patch for head.S below. I'll send out an updated patch series in a few days after I get more feedback. Thanks, Roy diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S index 75189f1..491e752 100644 --- a/arch/arm/boot/compressed/head.S +++ b/arch/arm/boot/compressed/head.S @@ -120,21 +120,100 @@ */ .align .arm @ Always enter in ARM state + .text start: .type start,#function - .rept 7 +#ifdef CONFIG_EFI_STUB + @ Magic MSDOS signature for PE/COFF + ADD opcode + .word 0x62805a4d +#else + mov r0, r0 +#endif + .rept 5 mov r0, r0 .endr - ARM( mov r0, r0 ) - ARM( b 1f ) - THUMB( adr r12, BSYM(1f) ) - THUMB( bx r12 ) + + @ zimage_continue will be in ARM or thumb mode as configured + THUMB( adrl r12, BSYM(zimage_continue)) + ARM( adrl r12, zimage_continue) + bx r12 + THUMB( .thumb ) .word 0x016f2818 @ Magic numbers to help the loader .word start @ absolute load/run zImage address .word _edata @ zImage end address + +#ifdef CONFIG_EFI_STUB + @ Portions of the MSDOS file header must be at offset + @ 0x3c from the start of the file. All PE/COFF headers + @ are kept contiguous for simplicity. +#include "efi-header.S" + +efi_stub_entry: + @ The EFI stub entry point is not at a fixed address, however + @ this address must be set in the PE/COFF header. + @ EFI entry point is in A32 mode, switch to T32 if configured. + THUMB( .arm ) + THUMB( adr r12, BSYM(1f) ) + THUMB( bx r12 ) THUMB( .thumb ) 1: + @ Save lr on stack for possible return to EFI firmware. + @ Don't care about fp, but need 64 bit alignment.... + stmfd sp!, {fp, lr} + + @ Save args to EFI app across got fixup call + stmfd sp!, {r0, r1} + ldmfd sp!, {r0, r1} + + @ allocate space on stack for return of new entry point of + @ zImage, as EFI stub may copy the kernel. Pass address + @ of space in r2 - EFI stub will fill in the pointer. + + sub sp, #8 @ we only need 4 bytes, + @ but keep stack 8 byte aligned. + mov r2, sp + @ Pass our actual runtime start address in pointer data + adr r11, LC0 @ address of LC0 at run time + ldr r12, [r11, #0] @ address of LC0 at link time + + sub r3, r11, r12 @ calculate the delta offset + str r3, [r2, #0] + bl efi_entry + + @ get new zImage entry address from stack, put into r3 + ldr r3, [sp, #0] + add sp, #8 @ restore stack + + @ Check for error return from EFI stub (0xFFFFFFFF) + ldr r1, =0xffffffff + cmp r0, r1 + beq efi_load_fail + + + @ Save return values of efi_entry + stmfd sp!, {r0, r3} + bl cache_clean_flush + bl cache_off + ldmfd sp!, {r0, r3} + + @ put DTB address in r2, it was returned by EFI entry + mov r2, r0 + ldr r1, =0xffffffff @ DTB machine type + mov r0, #0 @ r0 is 0 + + @ Branch to (possibly) relocated zImage entry that is in r3 + bx r3 + +efi_load_fail: + @ Return EFI_LOAD_ERROR to EFI firmware on error. + @ Switch back to ARM mode for EFI is done based on + @ return address on stack + ldr r0, =0x80000001 + ldmfd sp!, {fp, pc} +#endif + +zimage_continue: mrs r9, cpsr #ifdef CONFIG_ARM_VIRT_EXT bl __hyp_stub_install @ get into SVC mode, reversibly @@ -167,7 +246,6 @@ not_angel: * by the linker here, but it should preserve r7, r8, and r9. */ - .text #ifdef CONFIG_AUTO_ZRELADDR @ determine final kernel image address