Skip to contentSkip to conversation

efi/libstub/arm: make efi_entry() an ordinary PE/COFF entrypoint

PATCH In review Ard Biesheuvel 2 participants 3 messages Feb 17, 2020 – Feb 18, 2020

Subsystems (3)
Arm (32-bit) arm64 EFI
lore
Share & tools
lore

Ard Biesheuvel

author maintainer
#0 lore raw

PATCH 1/1 efi/libstub/arm: make efi_entry() an ordinary PE/COFF entrypoint

Expose efi_entry() as the PE/COFF entrypoint directly, instead of jumping into a wrapper that fiddles with stack buffers and other stuff that the compiler is much better at. The only reason this code exists is to obtain a pointer to the base of the image, but we can get the same value from the loaded_image protocol, which we already need for other reasons anyway.

Update the return type as well, to make it consistent with what is required for a PE/COFF executable entrypoint.

Signed-off-by Ard Biesheuvel <[email protected]>

---
Apologies to Atish for making him track a moving target, but this is another bit of cleanup work that I would like to merge before taking the RISC-V changes on top.

On RISC-V, I would expect efi_enter_kernel() to be implemented as a C routine that finds the hart id in the DT (which is passed as an argument), and pass it to the startup code of the kernel proper.

 arch/arm/boot/compressed/efi-header.S     |  2 +-
 arch/arm/boot/compressed/head.S           | 44 +++-----------
 arch/arm64/kernel/efi-entry.S             | 64 +++-----------------
 arch/arm64/kernel/efi-header.S            |  2 +-
 drivers/firmware/efi/libstub/arm-stub.c   | 37 +++++------
 drivers/firmware/efi/libstub/arm32-stub.c |  1 +
 drivers/firmware/efi/libstub/arm64-stub.c |  3 +-
 7 files changed, 42 insertions(+), 111 deletions(-)
arch/arm/boot/compressed/efi-header.S +1 -1
index a5983588f96b..e38fbda02b93 100644
@@ -60,7 +60,7 @@ optional_header:
 		.long	__pecoff_code_size		@ SizeOfCode
 		.long	__pecoff_data_size		@ SizeOfInitializedData
 		.long	0				@ SizeOfUninitializedData
-		.long	efi_stub_entry - start		@ AddressOfEntryPoint
+		.long	efi_entry - start		@ AddressOfEntryPoint
 		.long	start_offset			@ BaseOfCode
 		.long	__pecoff_data_start - start	@ BaseOfData
arch/arm/boot/compressed/head.S +8 -36

58 lines not on this page · raw message

arch/arm64/kernel/efi-entry.S +10 -54

85 lines not on this page · raw message

arch/arm64/kernel/efi-header.S +1 -1
index a7cfacce3e15..40c704c5d3a5 100644
@@ -27,7 +27,7 @@ optional_header:
 	.long	__initdata_begin - efi_header_end	// SizeOfCode
 	.long	__pecoff_data_size			// SizeOfInitializedData
 	.long	0					// SizeOfUninitializedData
-	.long	__efistub_entry - _head			// AddressOfEntryPoint
+	.long	__efistub_efi_entry - _head		// AddressOfEntryPoint
 	.long	efi_header_end - _head			// BaseOfCode
 
 extra_header_fields:
drivers/firmware/efi/libstub/arm-stub.c +20 -17
index 56e475c1aa55..439c094f7bf2 100644
@@ -133,17 +133,20 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
 				 unsigned long *reserve_size,
 				 unsigned long dram_base,
 				 efi_loaded_image_t *image);
+
+void __noreturn efi_enter_kernel(unsigned long entrypoint, unsigned long fdt);
+
 /*
  * EFI entry point for the arm/arm64 EFI stubs.  This is the entrypoint
  * that is described in the PE/COFF header.  Most of the code is the same
  * for both archictectures, with the arch-specific code provided in the
  * handle_kernel_image() function.
  */
-unsigned long efi_entry(void *handle, efi_system_table_t *sys_table_arg,
-			       unsigned long *image_addr)
+efi_status_t efi_entry(efi_handle_t handle, efi_system_table_t *sys_table_arg)
 {
 	efi_loaded_image_t *image;
 	efi_status_t status;
+	unsigned long image_addr;
 	unsigned long image_size = 0;
 	unsigned long dram_base;
 	/* addr/point and size pairs for memory management*/

@@ -153,7 +156,6 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table_arg,
 	unsigned long fdt_size = 0;
 	char *cmdline_ptr = NULL;
 	int cmdline_size = 0;
-	unsigned long new_fdt_addr;
 	efi_guid_t loaded_image_proto = LOADED_IMAGE_PROTOCOL_GUID;
 	unsigned long reserve_addr = 0;
 	unsigned long reserve_size = 0;

@@ -165,8 +167,10 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table_arg,
 	sys_table = sys_table_arg;
 
 	/* Check if we were booted by the EFI firmware */
-	if (sys_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
+	if (sys_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE) {
+		status = EFI_INVALID_PARAMETER;
 		goto fail;
+	}
 
 	status = check_platform_features();
 	if (status != EFI_SUCCESS)

@@ -187,6 +191,7 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table_arg,
 	dram_base = get_dram_base();
 	if (dram_base == EFI_ERROR) {
 		pr_efi_err("Failed to find DRAM base\n");
+		status = EFI_LOAD_ERROR;
 		goto fail;
 	}
 

@@ -198,6 +203,7 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table_arg,
 	cmdline_ptr = efi_convert_cmdline(image, &cmdline_size, ULONG_MAX);
 	if (!cmdline_ptr) {
 		pr_efi_err("getting command line via LOADED_IMAGE_PROTOCOL\n");
+		status = EFI_OUT_OF_RESOURCES;
 		goto fail;
 	}
 

@@ -213,7 +219,7 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table_arg,
 
 	si = setup_graphics();
 
-	status = handle_kernel_image(image_addr, &image_size,
+	status = handle_kernel_image(&image_addr, &image_size,
 				     &reserve_addr,
 				     &reserve_size,
 				     dram_base, image);

@@ -260,7 +266,7 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table_arg,
 		pr_efi("Generating empty DTB\n");
 
 	if (!noinitrd()) {
-		max_addr = efi_get_max_initrd_addr(dram_base, *image_addr);
+		max_addr = efi_get_max_initrd_addr(dram_base, image_addr);
 		status = efi_load_initrd_dev_path(&initrd_addr, &initrd_size,
 						  max_addr);
 		if (status == EFI_SUCCESS) {

@@ -311,33 +317,30 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table_arg,
 
 	install_memreserve_table();
 
-	new_fdt_addr = fdt_addr;
 	status = allocate_new_fdt_and_exit_boot(handle,
-				&new_fdt_addr, efi_get_max_fdt_addr(dram_base),
+				&fdt_addr, efi_get_max_fdt_addr(dram_base),
 				initrd_addr, initrd_size, cmdline_ptr,
 				fdt_addr, fdt_size);
+	if (status != EFI_SUCCESS)
+		goto fail_free_initrd;
 
-	/*
-	 * If all went well, we need to return the FDT address to the
-	 * calling function so it can be passed to kernel as part of
-	 * the kernel boot protocol.
-	 */
-	if (status == EFI_SUCCESS)
-		return new_fdt_addr;
+	efi_enter_kernel(image_addr, fdt_addr);
+	/* not reached */
 
+fail_free_initrd:
 	pr_efi_err("Failed to update FDT and exit boot services\n");
 
 	efi_free(initrd_size, initrd_addr);
 	efi_free(fdt_size, fdt_addr);
 
 fail_free_image:
-	efi_free(image_size, *image_addr);
+	efi_free(image_size, image_addr);
 	efi_free(reserve_size, reserve_addr);
 fail_free_cmdline:
 	free_screen_info(si);
 	efi_free(cmdline_size, (unsigned long)cmdline_ptr);
 fail:
-	return EFI_ERROR;
+	return status;
 }
 
 /*
drivers/firmware/efi/libstub/arm32-stub.c +1 -0
index 7b2a6382b647..113298b8ea65 100644
@@ -227,6 +227,7 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
 	 * Relocate the zImage, so that it appears in the lowest 128 MB
 	 * memory window.
 	 */
+	*image_addr = image->image_base;
 	*image_size = image->image_size;
 	status = efi_relocate_kernel(image_addr, *image_size, *image_size,
 				     kernel_base + MAX_UNCOMP_KERNEL_SIZE, 0, 0);
drivers/firmware/efi/libstub/arm64-stub.c +1 -2
index 719d03a64329..9254cd8ab2d3 100644
@@ -43,7 +43,6 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
 {
 	efi_status_t status;
 	unsigned long kernel_size, kernel_memsize = 0;
-	void *old_image_addr = (void *)*image_addr;
 	unsigned long preferred_offset;
 	u64 phys_seed = 0;
 

@@ -141,7 +140,7 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
 		}
 		*image_addr = *reserve_addr + TEXT_OFFSET;
 	}
-	memcpy((void *)*image_addr, old_image_addr, kernel_size);
+	memcpy((void *)*image_addr, image->image_base, kernel_size);
 
 	return EFI_SUCCESS;
 }
Signature · 1 line
2.17.1
#1 lore raw
Quoted 10 lines · Ard Biesheuvel

Expose efi_entry() as the PE/COFF entrypoint directly, instead of jumping into a wrapper that fiddles with stack buffers and other stuff that the compiler is much better at. The only reason this code exists is to obtain a pointer to the base of the image, but we can get the same value from the loaded_image protocol, which we already need for other reasons anyway.

Update the return type as well, to make it consistent with what is required for a PE/COFF executable entrypoint.

Quoted 8 lines
Signed-off-by Ard Biesheuvel <[email protected]>

---
Apologies to Atish for making him track a moving target, but this is another
bit of cleanup work that I would like to merge before taking the RISC-V
changes on top.

And I thought I am done rebasing my patches ;). Jokes apart, this is actually much cleaner approach in retrieving image base and parsing DT compared to what I currently have for RISC-V.

Quoted 6 lines

On RISC-V, I would expect efi_enter_kernel() to be implemented as a C routine
that finds the hart id in the DT (which is passed as an argument), and pass
it to the startup code of the kernel proper.

Yes. I probably won't need efi-entry.S for RISC-V at all which simplifies the RISC-V implementation. Yay!!

Is there a tree with this patch ? I tried to find it in your kernel.org repo but couldn't find it. As there are 3 efi related patch set in flight, I am not sure about the order in which they should be applied.

Quoted 402 lines
 arch/arm/boot/compressed/efi-header.S     |  2 +-
 arch/arm/boot/compressed/head.S           | 44 +++-----------
 arch/arm64/kernel/efi-entry.S             | 64 +++-----------------
 arch/arm64/kernel/efi-header.S            |  2 +-
 drivers/firmware/efi/libstub/arm-stub.c   | 37 +++++------
 drivers/firmware/efi/libstub/arm32-stub.c |  1 +
 drivers/firmware/efi/libstub/arm64-stub.c |  3 +-
 7 files changed, 42 insertions(+), 111 deletions(-)
arch/arm/boot/compressed/efi-header.S +0 -0
index a5983588f96b..e38fbda02b93 100644
@@ -60,7 +60,7 @@ optional_header:
 		.long	__pecoff_code_size		@ SizeOfCode
 		.long	__pecoff_data_size		@

SizeOfInitializedData
.long 0 @
SizeOfUninitializedData

arch/arm/boot/compressed/efi-header.S +0 -1
-		.long	efi_stub_entry - start		@

AddressOfEntryPoint

arch/arm/boot/compressed/efi-header.S +1 -0
+		.long	efi_entry - start		@

AddressOfEntryPoint

arch/arm/boot/compressed/efi-header.S +0 -0
 		.long	start_offset			@ BaseOfCode
 		.long	__pecoff_data_start - start	@ BaseOfData
arch/arm/boot/compressed/head.S +0 -5
index 088b0a060876..941cd28a13c3 100644
@@ -1437,50 +1437,22 @@ __enter_kernel:
 reloc_code_end:
 
 #ifdef CONFIG_EFI_STUB
-		.align	2
-_start:		.long	start - .
-
-ENTRY(efi_stub_entry)
-		@ allocate space on stack for passing current zImage

address

arch/arm/boot/compressed/head.S +0 -2
-		@ and for the EFI stub to return of new entry point of
-		@ zImage, as EFI stub may copy the kernel. Pointer

address

arch/arm/boot/compressed/head.S +0 -1
-		@ is passed in r2. r0 and r1 are passed through from

the

arch/arm/boot/compressed/head.S +0 -6
-		@ EFI firmware to efi_entry
-		adr	ip, _start
-		ldr	r3, [ip]
-		add	r3, r3, ip
-		stmfd	sp!, {r3, lr}
-		mov	r2, sp			@ pass zImage address

in r2

arch/arm/boot/compressed/head.S +0 -3
-		bl	efi_entry
-
-		@ Check for error return from EFI stub. r0 has FDT

address

arch/arm/boot/compressed/head.S +3 -6
-		@ or error code.
-		cmn	r0, #1
-		beq	efi_load_fail
-
-		@ Preserve return value of efi_entry() in r4
-		mov	r4, r0
+ENTRY(efi_enter_kernel)
+		mov	r4, r0			@ preserve entrypoint
+		mov	r5, r1			@ preserve DT pointer
 		bl	cache_clean_flush
 		bl	cache_off
 
 		@ Set parameters for booting zImage according to boot

protocol

arch/arm/boot/compressed/head.S +2 -7
-		@ put FDT address in r2, it was returned by efi_entry()
-		@ r1 is the machine type, and r0 needs to be 0
 		mov	r0, #0
 		mov	r1, #0xFFFFFFFF
-		mov	r2, r4
+		mov	r2, r5
 
-		@ Branch to (possibly) relocated zImage that is in [sp]
-		ldr	lr, [sp]
+		@ Branch to (possibly) relocated zImage @ [r4]
 		ldr	ip, =start_offset
-		add	lr, lr, ip
-		mov	pc, lr				@ no mode

switch

arch/arm/boot/compressed/head.S +3 -6
-
-efi_load_fail:
-		@ Return EFI_LOAD_ERROR to EFI firmware on error.
-		ldr	r0, =0x80000001
-		ldmfd	sp!, {ip, pc}
-ENDPROC(efi_stub_entry)
+		add	r4, r4, ip
+		mov	pc, r4			@ no mode switch
+ENDPROC(efi_enter_kernel)
 #endif
 
 		.align
arch/arm64/kernel/efi- renamed from arch/arm64/kernel/efi-entry.S +0 -0

entry.S
index 304d5b02ca67..49e3b1ad1b36 100644

arch/arm64/kernel/efi-entry.S +0 -10
@@ -10,55 +10,18 @@
 
 #include <asm/assembler.h>
 
-#define EFI_LOAD_ERROR 0x8000000000000001
-
 	__INIT
 
-	/*
-	 * We arrive here from the EFI boot manager with:
-	 *
-	 *    * CPU in little-endian mode
-	 *    * MMU on with identity-mapped RAM
-	 *    * Icache and Dcache on
-	 *
-	 * We will most likely be running from some place other than

where

arch/arm64/kernel/efi-entry.S +0 -1
-	 * we want to be. The kernel image wants to be placed at

TEXT_OFFSET

arch/arm64/kernel/efi-entry.S +1 -26
-	 * from start of RAM.
-	 */
-ENTRY(entry)
-	/*
-	 * Create a stack frame to save FP/LR with extra space
-	 * for image_addr variable passed to efi_entry().
-	 */
-	stp	x29, x30, [sp, #-32]!
-	mov	x29, sp
-
-	/*
-	 * Call efi_entry to do the real work.
-	 * x0 and x1 are already set up by firmware. Current runtime
-	 * address of image is calculated and passed via *image_addr.
-	 *
-	 * unsigned long efi_entry(void *handle,
-	 *                         efi_system_table_t *sys_table,
-	 *                         unsigned long *image_addr) ;
-	 */
-	adr_l	x8, _text
-	add	x2, sp, 16
-	str	x8, [x2]
-	bl	efi_entry
-	cmn	x0, #1
-	b.eq	efi_load_fail
-
+ENTRY(efi_enter_kernel)
 	/*
 	 * efi_entry() will have copied the kernel image if necessary

and we

arch/arm64/kernel/efi-entry.S +0 -1
-	 * return here with device tree address in x0 and the kernel

entry

arch/arm64/kernel/efi-entry.S +0 -1
-	 * point stored at *image_addr. Save those values in registers

which

arch/arm64/kernel/efi-entry.S +1 -1
-	 * are callee preserved.
+	 * end up here with device tree address in x1 and the kernel

entry

arch/arm64/kernel/efi-entry.S +4 -3
+	 * point stored in x0. Save those values in registers which are
+	 * callee preserved.
 	 */
-	mov	x20, x0		// DTB address
-	ldr	x0, [sp, #16]	// relocated _text address
+	mov	x20, x1			// DTB address
 	ldr	w21, =stext_offset
-	add	x21, x0, x21
+	add	x21, x0, x21		// relocated Image address
 
 	/*
 	 * Calculate size of the kernel Image (same for original and

copy).

arch/arm64/kernel/efi-entry.S +0 -0
@@ -80,9 +43,8 @@ ENTRY(entry)
 	 * entries for the VA range of the current image, so no

maintenance is

arch/arm64/kernel/efi-entry.S +4 -11
 	 * necessary.
 	 */
-	adr	x0, entry
-	adr	x1, entry_end
-	sub	x1, x1, x0
+	adr	x0, efi_enter_kernel
+	mov	x1, efi_enter_kernel_size
 	bl	__flush_dcache_area
 
 	/* Turn off Dcache and MMU */

@@ -110,11 +72,5 @@ ENTRY(entry)
 	mov	x2, xzr
 	mov	x3, xzr
 	br	x21
-
-efi_load_fail:
-	mov	x0, #EFI_LOAD_ERROR
-	ldp	x29, x30, [sp], #32
-	ret
-
-entry_end:
-ENDPROC(entry)
+ENDPROC(efi_enter_kernel)
+	.set	efi_enter_kernel_size, . - efi_enter_kernel
arch/arm64/kernel/efi- renamed from arch/arm64/kernel/efi-header.S +0 -0

header.S
index a7cfacce3e15..40c704c5d3a5 100644

arch/arm64/kernel/efi-header.S +0 -0
@@ -27,7 +27,7 @@ optional_header:
 	.long	__initdata_begin - efi_header_end	// SizeOfCode
 	.long	__pecoff_data_size			//

SizeOfInitializedData
.long 0 //
SizeOfUninitializedData

arch/arm64/kernel/efi-header.S +0 -1
-	.long	__efistub_entry - _head			//

AddressOfEntryPoint

arch/arm64/kernel/efi-header.S +1 -0
+	.long	__efistub_efi_entry - _head		//

AddressOfEntryPoint

arch/arm64/kernel/efi-header.S +0 -0
 	.long	efi_header_end - _head			// BaseOfCode
 
 extra_header_fields:
drivers/firmware/efi/libstub/arm-stub.c +3 -0
index 56e475c1aa55..439c094f7bf2 100644
@@ -133,17 +133,20 @@ efi_status_t handle_kernel_image(unsigned long
 *image_addr,
 				 unsigned long *reserve_size,
 				 unsigned long dram_base,
 				 efi_loaded_image_t *image);
+
+void __noreturn efi_enter_kernel(unsigned long entrypoint, unsigned
 long fdt);
+
 /*
  * EFI entry point for the arm/arm64 EFI stubs.  This is the

entrypoint
* that is described in the PE/COFF header. Most of the code is the
same
* for both archictectures, with the arch-specific code provided in
the
* handle_kernel_image() function.
*/
-unsigned long efi_entry(void *handle, efi_system_table_t *sys_table_arg,

drivers/firmware/efi/libstub/arm-stub.c +1 -1
-			       unsigned long *image_addr)
+efi_status_t efi_entry(efi_handle_t handle, efi_system_table_t

*sys_table_arg)
{

drivers/firmware/efi/libstub/arm-stub.c +1 -1
 	efi_loaded_image_t *image;
 	efi_status_t status;
+	unsigned long image_addr;
 	unsigned long image_size = 0;
 	unsigned long dram_base;
 	/* addr/point and size pairs for memory management*/

@@ -153,7 +156,6 @@ unsigned long efi_entry(void *handle,
 efi_system_table_t *sys_table_arg,
 	unsigned long fdt_size = 0;
 	char *cmdline_ptr = NULL;
 	int cmdline_size = 0;
-	unsigned long new_fdt_addr;
 	efi_guid_t loaded_image_proto = LOADED_IMAGE_PROTOCOL_GUID;
 	unsigned long reserve_addr = 0;
drivers/firmware/efi/libstub/arm-stub.c +3 -1
 	unsigned long reserve_size = 0;

@@ -165,8 +167,10 @@ unsigned long efi_entry(void *handle,
 efi_system_table_t *sys_table_arg,
 	sys_table = sys_table_arg;
 
 	/* Check if we were booted by the EFI firmware */
-	if (sys_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
+	if (sys_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE) {
+		status = EFI_INVALID_PARAMETER;
 		goto fail;
+	}
 
 	status = check_platform_features();
drivers/firmware/efi/libstub/arm-stub.c +2 -0
 	if (status != EFI_SUCCESS)

@@ -187,6 +191,7 @@ unsigned long efi_entry(void *handle,
 efi_system_table_t *sys_table_arg,
 	dram_base = get_dram_base();
 	if (dram_base == EFI_ERROR) {
 		pr_efi_err("Failed to find DRAM base\n");
+		status = EFI_LOAD_ERROR;
 		goto fail;
 	}

@@ -198,6 +203,7 @@ unsigned long efi_entry(void *handle,
 efi_system_table_t *sys_table_arg,
 	cmdline_ptr = efi_convert_cmdline(image, &cmdline_size,
 ULONG_MAX);
 	if (!cmdline_ptr) {
 		pr_efi_err("getting command line via
 LOADED_IMAGE_PROTOCOL\n");
+		status = EFI_OUT_OF_RESOURCES;
drivers/firmware/efi/libstub/arm-stub.c +1 -1
 		goto fail;
 	}

@@ -213,7 +219,7 @@ unsigned long efi_entry(void *handle,
 efi_system_table_t *sys_table_arg,
 
 	si = setup_graphics();
 
-	status = handle_kernel_image(image_addr, &image_size,
+	status = handle_kernel_image(&image_addr, &image_size,
 				     &reserve_addr,
 				     &reserve_size,
drivers/firmware/efi/libstub/arm-stub.c +1 -1
 				     dram_base, image);

@@ -260,7 +266,7 @@ unsigned long efi_entry(void *handle,
 efi_system_table_t *sys_table_arg,
 		pr_efi("Generating empty DTB\n");
 
 	if (!noinitrd()) {
-		max_addr = efi_get_max_initrd_addr(dram_base,
 *image_addr);
+		max_addr = efi_get_max_initrd_addr(dram_base,
 image_addr);

status = efi_load_initrd_dev_path(&initrd_addr,
&initrd_size,

drivers/firmware/efi/libstub/arm-stub.c +8 -11
 						  max_addr);
 		if (status == EFI_SUCCESS) {

@@ -311,33 +317,30 @@ unsigned long efi_entry(void *handle,
 efi_system_table_t *sys_table_arg,
 
 	install_memreserve_table();
 
-	new_fdt_addr = fdt_addr;
 	status = allocate_new_fdt_and_exit_boot(handle,
-				&new_fdt_addr,
 efi_get_max_fdt_addr(dram_base),
+				&fdt_addr,
 efi_get_max_fdt_addr(dram_base),
 				initrd_addr, initrd_size, cmdline_ptr,
 				fdt_addr, fdt_size);
+	if (status != EFI_SUCCESS)
+		goto fail_free_initrd;
 
-	/*
-	 * If all went well, we need to return the FDT address to the
-	 * calling function so it can be passed to kernel as part of
-	 * the kernel boot protocol.
-	 */
-	if (status == EFI_SUCCESS)
-		return new_fdt_addr;
+	efi_enter_kernel(image_addr, fdt_addr);
+	/* not reached */
 
+fail_free_initrd:
 	pr_efi_err("Failed to update FDT and exit boot services\n");
 
 	efi_free(initrd_size, initrd_addr);
 	efi_free(fdt_size, fdt_addr);
 
 fail_free_image:
-	efi_free(image_size, *image_addr);
+	efi_free(image_size, image_addr);
 	efi_free(reserve_size, reserve_addr);
 fail_free_cmdline:
 	free_screen_info(si);
 	efi_free(cmdline_size, (unsigned long)cmdline_ptr);
 fail:
-	return EFI_ERROR;
+	return status;

}

/*

drivers/firmware/efi/libstub/arm32-stub.c +1 -0
index 7b2a6382b647..113298b8ea65 100644
@@ -227,6 +227,7 @@ efi_status_t handle_kernel_image(unsigned long
 *image_addr,
 	 * Relocate the zImage, so that it appears in the lowest 128 MB
 	 * memory window.
 	 */
+	*image_addr = image->image_base;
 	*image_size = image->image_size;
 	status = efi_relocate_kernel(image_addr, *image_size,

*image_size,
kernel_base +
MAX_UNCOMP_KERNEL_SIZE, 0, 0);

drivers/firmware/efi/libstub/arm64-stub.c +1 -2
index 719d03a64329..9254cd8ab2d3 100644
@@ -43,7 +43,6 @@ efi_status_t handle_kernel_image(unsigned long
 *image_addr,
 {
 	efi_status_t status;
 	unsigned long kernel_size, kernel_memsize = 0;
-	void *old_image_addr = (void *)*image_addr;
 	unsigned long preferred_offset;
 	u64 phys_seed = 0;

@@ -141,7 +140,7 @@ efi_status_t handle_kernel_image(unsigned long
 *image_addr,
 		}
 		*image_addr = *reserve_addr + TEXT_OFFSET;
 	}
-	memcpy((void *)*image_addr, old_image_addr, kernel_size);
+	memcpy((void *)*image_addr, image->image_base, kernel_size);
 
 	return EFI_SUCCESS;

}

Signature · 2 lines
Regards,
Atish
author maintainer
#2 lore raw
Quoted 33 lines · Atish Patra
Quoted 10 lines · Ard Biesheuvel

Expose efi_entry() as the PE/COFF entrypoint directly, instead of jumping into a wrapper that fiddles with stack buffers and other stuff that the compiler is much better at. The only reason this code exists is to obtain a pointer to the base of the image, but we can get the same value from the loaded_image protocol, which we already need for other reasons anyway.

Update the return type as well, to make it consistent with what is required for a PE/COFF executable entrypoint.

Quoted 8 lines
Signed-off-by Ard Biesheuvel <[email protected]>

---
Apologies to Atish for making him track a moving target, but this is another
bit of cleanup work that I would like to merge before taking the RISC-V
changes on top.

And I thought I am done rebasing my patches ;). Jokes apart, this is actually much cleaner approach in retrieving image base and parsing DT compared to what I currently have for RISC-V.

Quoted 6 lines

On RISC-V, I would expect efi_enter_kernel() to be implemented as a C routine
that finds the hart id in the DT (which is passed as an argument), and pass
it to the startup code of the kernel proper.

Quoted 3 lines

Yes. I probably won't need efi-entry.S for RISC-V at all which simplifies the RISC-V implementation. Yay!!

Glad to hear that.

Quoted 5 lines

Is there a tree with this patch ? I tried to find it in your kernel.org repo but couldn't find it. As there are 3 efi related patch set in flight, I am not sure about the order in which they should be applied.

Please rebase onto

https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git/log/?h=next

Quoted 406 lines
Quoted 401 lines
 arch/arm/boot/compressed/efi-header.S     |  2 +-
 arch/arm/boot/compressed/head.S           | 44 +++-----------
 arch/arm64/kernel/efi-entry.S             | 64 +++-----------------
 arch/arm64/kernel/efi-header.S            |  2 +-
 drivers/firmware/efi/libstub/arm-stub.c   | 37 +++++------
 drivers/firmware/efi/libstub/arm32-stub.c |  1 +
 drivers/firmware/efi/libstub/arm64-stub.c |  3 +-
 7 files changed, 42 insertions(+), 111 deletions(-)
arch/arm/boot/compressed/efi-header.S +0 -0
index a5983588f96b..e38fbda02b93 100644
@@ -60,7 +60,7 @@ optional_header:
              .long   __pecoff_code_size              @ SizeOfCode
              .long   __pecoff_data_size              @

SizeOfInitializedData
.long 0 @
SizeOfUninitializedData
- .long efi_stub_entry - start @
AddressOfEntryPoint
+ .long efi_entry - start @
AddressOfEntryPoint

              .long   start_offset                    @ BaseOfCode
              .long   __pecoff_data_start - start     @ BaseOfData
arch/arm/boot/compressed/head.S +0 -5
index 088b0a060876..941cd28a13c3 100644
@@ -1437,50 +1437,22 @@ __enter_kernel:
 reloc_code_end:
 
 #ifdef CONFIG_EFI_STUB
-             .align  2
-_start:              .long   start - .
-
-ENTRY(efi_stub_entry)
-             @ allocate space on stack for passing current zImage

address

arch/arm/boot/compressed/head.S +0 -2
-             @ and for the EFI stub to return of new entry point of
-             @ zImage, as EFI stub may copy the kernel. Pointer

address
- @ is passed in r2. r0 and r1 are passed through from
the

arch/arm/boot/compressed/head.S +0 -6
-             @ EFI firmware to efi_entry
-             adr     ip, _start
-             ldr     r3, [ip]
-             add     r3, r3, ip
-             stmfd   sp!, {r3, lr}
-             mov     r2, sp                  @ pass zImage address

in r2

arch/arm/boot/compressed/head.S +0 -3
-             bl      efi_entry
-
-             @ Check for error return from EFI stub. r0 has FDT

address

arch/arm/boot/compressed/head.S +3 -6
-             @ or error code.
-             cmn     r0, #1
-             beq     efi_load_fail
-
-             @ Preserve return value of efi_entry() in r4
-             mov     r4, r0
+ENTRY(efi_enter_kernel)
+             mov     r4, r0                  @ preserve entrypoint
+             mov     r5, r1                  @ preserve DT pointer
              bl      cache_clean_flush
              bl      cache_off
 
              @ Set parameters for booting zImage according to boot

protocol

arch/arm/boot/compressed/head.S +2 -7
-             @ put FDT address in r2, it was returned by efi_entry()
-             @ r1 is the machine type, and r0 needs to be 0
              mov     r0, #0
              mov     r1, #0xFFFFFFFF
-             mov     r2, r4
+             mov     r2, r5
 
-             @ Branch to (possibly) relocated zImage that is in [sp]
-             ldr     lr, [sp]
+             @ Branch to (possibly) relocated zImage @ [r4]
              ldr     ip, =start_offset
-             add     lr, lr, ip
-             mov     pc, lr                          @ no mode

switch

arch/arm/boot/compressed/head.S +3 -6
-
-efi_load_fail:
-             @ Return EFI_LOAD_ERROR to EFI firmware on error.
-             ldr     r0, =0x80000001
-             ldmfd   sp!, {ip, pc}
-ENDPROC(efi_stub_entry)
+             add     r4, r4, ip
+             mov     pc, r4                  @ no mode switch
+ENDPROC(efi_enter_kernel)
 #endif
 
              .align
arch/arm64/kernel/efi- renamed from arch/arm64/kernel/efi-entry.S +0 -0

entry.S
index 304d5b02ca67..49e3b1ad1b36 100644

arch/arm64/kernel/efi-entry.S +0 -10
@@ -10,55 +10,18 @@
 
 #include <asm/assembler.h>
 
-#define EFI_LOAD_ERROR 0x8000000000000001
-
      __INIT
 
-     /*
-      * We arrive here from the EFI boot manager with:
-      *
-      *    * CPU in little-endian mode
-      *    * MMU on with identity-mapped RAM
-      *    * Icache and Dcache on
-      *
-      * We will most likely be running from some place other than

where
- * we want to be. The kernel image wants to be placed at
TEXT_OFFSET

arch/arm64/kernel/efi-entry.S +1 -26
-      * from start of RAM.
-      */
-ENTRY(entry)
-     /*
-      * Create a stack frame to save FP/LR with extra space
-      * for image_addr variable passed to efi_entry().
-      */
-     stp     x29, x30, [sp, #-32]!
-     mov     x29, sp
-
-     /*
-      * Call efi_entry to do the real work.
-      * x0 and x1 are already set up by firmware. Current runtime
-      * address of image is calculated and passed via *image_addr.
-      *
-      * unsigned long efi_entry(void *handle,
-      *                         efi_system_table_t *sys_table,
-      *                         unsigned long *image_addr) ;
-      */
-     adr_l   x8, _text
-     add     x2, sp, 16
-     str     x8, [x2]
-     bl      efi_entry
-     cmn     x0, #1
-     b.eq    efi_load_fail
-
+ENTRY(efi_enter_kernel)
      /*
       * efi_entry() will have copied the kernel image if necessary

and we
- * return here with device tree address in x0 and the kernel
entry
- * point stored at *image_addr. Save those values in registers
which

arch/arm64/kernel/efi-entry.S +1 -1
-      * are callee preserved.
+      * end up here with device tree address in x1 and the kernel

entry

arch/arm64/kernel/efi-entry.S +4 -3
+      * point stored in x0. Save those values in registers which are
+      * callee preserved.
       */
-     mov     x20, x0         // DTB address
-     ldr     x0, [sp, #16]   // relocated _text address
+     mov     x20, x1                 // DTB address
      ldr     w21, =stext_offset
-     add     x21, x0, x21
+     add     x21, x0, x21            // relocated Image address
 
      /*
       * Calculate size of the kernel Image (same for original and

copy).

arch/arm64/kernel/efi-entry.S +0 -0
@@ -80,9 +43,8 @@ ENTRY(entry)
       * entries for the VA range of the current image, so no

maintenance is
* necessary.
*/

arch/arm64/kernel/efi-entry.S +4 -11
-     adr     x0, entry
-     adr     x1, entry_end
-     sub     x1, x1, x0
+     adr     x0, efi_enter_kernel
+     mov     x1, efi_enter_kernel_size
      bl      __flush_dcache_area
 
      /* Turn off Dcache and MMU */

@@ -110,11 +72,5 @@ ENTRY(entry)
      mov     x2, xzr
      mov     x3, xzr
      br      x21
-
-efi_load_fail:
-     mov     x0, #EFI_LOAD_ERROR
-     ldp     x29, x30, [sp], #32
-     ret
-
-entry_end:
-ENDPROC(entry)
+ENDPROC(efi_enter_kernel)
+     .set    efi_enter_kernel_size, . - efi_enter_kernel
arch/arm64/kernel/efi- renamed from arch/arm64/kernel/efi-header.S +0 -0

header.S
index a7cfacce3e15..40c704c5d3a5 100644

arch/arm64/kernel/efi-header.S +0 -0
@@ -27,7 +27,7 @@ optional_header:
      .long   __initdata_begin - efi_header_end       // SizeOfCode
      .long   __pecoff_data_size                      //

SizeOfInitializedData
.long 0 //
SizeOfUninitializedData
- .long __efistub_entry - _head //
AddressOfEntryPoint
+ .long __efistub_efi_entry - _head //
AddressOfEntryPoint
.long efi_header_end - _head // BaseOfCode

extra_header_fields:

drivers/firmware/efi/libstub/arm-stub.c +3 -0
index 56e475c1aa55..439c094f7bf2 100644
@@ -133,17 +133,20 @@ efi_status_t handle_kernel_image(unsigned long
 *image_addr,
                               unsigned long *reserve_size,
                               unsigned long dram_base,
                               efi_loaded_image_t *image);
+
+void __noreturn efi_enter_kernel(unsigned long entrypoint, unsigned
 long fdt);
+
 /*
  * EFI entry point for the arm/arm64 EFI stubs.  This is the

entrypoint
* that is described in the PE/COFF header. Most of the code is the
same
* for both archictectures, with the arch-specific code provided in
the
* handle_kernel_image() function.
*/
-unsigned long efi_entry(void *handle, efi_system_table_t *sys_table_arg,

drivers/firmware/efi/libstub/arm-stub.c +1 -1
-                            unsigned long *image_addr)
+efi_status_t efi_entry(efi_handle_t handle, efi_system_table_t

*sys_table_arg)

 {
      efi_loaded_image_t *image;
      efi_status_t status;
drivers/firmware/efi/libstub/arm-stub.c +1 -1
+     unsigned long image_addr;
      unsigned long image_size = 0;
      unsigned long dram_base;
      /* addr/point and size pairs for memory management*/

@@ -153,7 +156,6 @@ unsigned long efi_entry(void *handle,
 efi_system_table_t *sys_table_arg,
      unsigned long fdt_size = 0;
      char *cmdline_ptr = NULL;
      int cmdline_size = 0;
-     unsigned long new_fdt_addr;
      efi_guid_t loaded_image_proto = LOADED_IMAGE_PROTOCOL_GUID;
      unsigned long reserve_addr = 0;

      unsigned long reserve_size = 0;
drivers/firmware/efi/libstub/arm-stub.c +3 -1
@@ -165,8 +167,10 @@ unsigned long efi_entry(void *handle,
 efi_system_table_t *sys_table_arg,
      sys_table = sys_table_arg;
 
      /* Check if we were booted by the EFI firmware */
-     if (sys_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
+     if (sys_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE) {
+             status = EFI_INVALID_PARAMETER;
              goto fail;
+     }
 
      status = check_platform_features();

      if (status != EFI_SUCCESS)
drivers/firmware/efi/libstub/arm-stub.c +2 -0
@@ -187,6 +191,7 @@ unsigned long efi_entry(void *handle,
 efi_system_table_t *sys_table_arg,
      dram_base = get_dram_base();
      if (dram_base == EFI_ERROR) {
              pr_efi_err("Failed to find DRAM base\n");
+             status = EFI_LOAD_ERROR;
              goto fail;
      }

@@ -198,6 +203,7 @@ unsigned long efi_entry(void *handle,
 efi_system_table_t *sys_table_arg,
      cmdline_ptr = efi_convert_cmdline(image, &cmdline_size,
 ULONG_MAX);
      if (!cmdline_ptr) {
              pr_efi_err("getting command line via
 LOADED_IMAGE_PROTOCOL\n");
+             status = EFI_OUT_OF_RESOURCES;

              goto fail;
      }
drivers/firmware/efi/libstub/arm-stub.c +1 -1
@@ -213,7 +219,7 @@ unsigned long efi_entry(void *handle,
 efi_system_table_t *sys_table_arg,
 
      si = setup_graphics();
 
-     status = handle_kernel_image(image_addr, &image_size,
+     status = handle_kernel_image(&image_addr, &image_size,
                                   &reserve_addr,
                                   &reserve_size,

                                   dram_base, image);
drivers/firmware/efi/libstub/arm-stub.c +1 -1
@@ -260,7 +266,7 @@ unsigned long efi_entry(void *handle,
 efi_system_table_t *sys_table_arg,
              pr_efi("Generating empty DTB\n");
 
      if (!noinitrd()) {
-             max_addr = efi_get_max_initrd_addr(dram_base,
 *image_addr);
+             max_addr = efi_get_max_initrd_addr(dram_base,
 image_addr);

status = efi_load_initrd_dev_path(&initrd_addr,
&initrd_size,

                                                max_addr);
              if (status == EFI_SUCCESS) {
drivers/firmware/efi/libstub/arm-stub.c +8 -11
@@ -311,33 +317,30 @@ unsigned long efi_entry(void *handle,
 efi_system_table_t *sys_table_arg,
 
      install_memreserve_table();
 
-     new_fdt_addr = fdt_addr;
      status = allocate_new_fdt_and_exit_boot(handle,
-                             &new_fdt_addr,
 efi_get_max_fdt_addr(dram_base),
+                             &fdt_addr,
 efi_get_max_fdt_addr(dram_base),
                              initrd_addr, initrd_size, cmdline_ptr,
                              fdt_addr, fdt_size);
+     if (status != EFI_SUCCESS)
+             goto fail_free_initrd;
 
-     /*
-      * If all went well, we need to return the FDT address to the
-      * calling function so it can be passed to kernel as part of
-      * the kernel boot protocol.
-      */
-     if (status == EFI_SUCCESS)
-             return new_fdt_addr;
+     efi_enter_kernel(image_addr, fdt_addr);
+     /* not reached */
 
+fail_free_initrd:
      pr_efi_err("Failed to update FDT and exit boot services\n");
 
      efi_free(initrd_size, initrd_addr);
      efi_free(fdt_size, fdt_addr);
 
 fail_free_image:
-     efi_free(image_size, *image_addr);
+     efi_free(image_size, image_addr);
      efi_free(reserve_size, reserve_addr);
 fail_free_cmdline:
      free_screen_info(si);
      efi_free(cmdline_size, (unsigned long)cmdline_ptr);
 fail:
-     return EFI_ERROR;
+     return status;

}

/*

drivers/firmware/efi/libstub/arm32-stub.c +1 -0
index 7b2a6382b647..113298b8ea65 100644
@@ -227,6 +227,7 @@ efi_status_t handle_kernel_image(unsigned long
 *image_addr,
       * Relocate the zImage, so that it appears in the lowest 128 MB
       * memory window.
       */
+     *image_addr = image->image_base;
      *image_size = image->image_size;
      status = efi_relocate_kernel(image_addr, *image_size,

*image_size,
kernel_base +
MAX_UNCOMP_KERNEL_SIZE, 0, 0);

drivers/firmware/efi/libstub/arm64-stub.c +1 -2
index 719d03a64329..9254cd8ab2d3 100644
@@ -43,7 +43,6 @@ efi_status_t handle_kernel_image(unsigned long
 *image_addr,
 {
      efi_status_t status;
      unsigned long kernel_size, kernel_memsize = 0;
-     void *old_image_addr = (void *)*image_addr;
      unsigned long preferred_offset;
      u64 phys_seed = 0;

@@ -141,7 +140,7 @@ efi_status_t handle_kernel_image(unsigned long
 *image_addr,
              }
              *image_addr = *reserve_addr + TEXT_OFFSET;
      }
-     memcpy((void *)*image_addr, old_image_addr, kernel_size);
+     memcpy((void *)*image_addr, image->image_base, kernel_size);
 
      return EFI_SUCCESS;
Quoted 1 line

}

--
Regards,
Atish