efi/libstub/x86: two more tweaks for the EFI stub startup code

PATCH In review Ard Biesheuvel 3 participants 7 messages Jan 8, 2020

Subsystems (2)

x86 EFI

lore

Share & tools

lore

Ard Biesheuvel

author maintainer

2020-01-08 07:45 UTC #0 lore raw

cover efi/libstub/x86: two more tweaks for the EFI stub startup code

Two final cleanups for the x86 startup code, one that helps the compiler generate better code, by annotating a helper function with the 'const' function attribute, and one that fixes the misalignment of the stack in mixed mode.

There are no known issues regarding entering the 32-bit firmware from the 64-bit kernel with the stack misaligned, and the 32-bit kernel does so all the time, but it is better to comply with the UEFI spec.

Ard Biesheuvel (2):
efi/libstub/x86: use const attribute for efi_is_64bit() efi/libstub/x86: use mandatory 16-byte stack alignment in mixed mode

 arch/x86/boot/compressed/eboot.c        | 14 +++---
 arch/x86/boot/compressed/efi_thunk_64.S | 46 ++++++--------------
 arch/x86/boot/compressed/head_64.S      |  7 ++-
 arch/x86/include/asm/efi.h              |  2 +-
 4 files changed, 23 insertions(+), 46 deletions(-)

Signature · 1 line

2.20.1

Ard Biesheuvel

author maintainer

2020-01-08 07:45 UTC #1 lore raw

PATCH 1/2 efi/libstub/x86: use const attribute for efi_is_64bit()

Reshuffle the x86 stub code a bit so that we can tag the efi_is_64bit() function with the 'const' attribute, which permits the compiler to optimize away any redundant calls. Since we have two different entry points for 32 and 64 bit firmware in the startup code, this also simplifies the C code since we'll enter it with the efi_is64 variable already set.

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

---
 arch/x86/boot/compressed/eboot.c   | 14 ++++++--------
 arch/x86/boot/compressed/head_64.S |  7 +++----
 arch/x86/include/asm/efi.h         |  2 +-
 3 files changed, 10 insertions(+), 13 deletions(-)

arch/x86/boot/compressed/eboot.c +6 -8

index 4afd29eb5b34..ab3a40283db7 100644
@@ -21,16 +21,18 @@
 #include "eboot.h"
 
 static efi_system_table_t *sys_table;
-static bool efi_is64 = IS_ENABLED(CONFIG_X86_64);
+extern const bool efi_is64;
 
 __pure efi_system_table_t *efi_system_table(void)
 {
 	return sys_table;
 }
 
-__pure bool efi_is_64bit(void)
+__attribute_const__ bool efi_is_64bit(void)
 {
-	return efi_is64;
+	if (IS_ENABLED(CONFIG_EFI_MIXED))
+		return efi_is64;
+	return IS_ENABLED(CONFIG_X64_64);
 }
 
 static efi_status_t

@@ -710,8 +712,7 @@ static efi_status_t exit_boot(struct boot_params *boot_params, void *handle)
  */
 struct boot_params *efi_main(efi_handle_t handle,
 			     efi_system_table_t *sys_table_arg,
-			     struct boot_params *boot_params,
-			     bool is64)
+			     struct boot_params *boot_params)
 {
 	struct desc_ptr *gdt = NULL;
 	struct setup_header *hdr = &boot_params->hdr;

@@ -721,9 +722,6 @@ struct boot_params *efi_main(efi_handle_t handle,
 
 	sys_table = sys_table_arg;
 
-	if (IS_ENABLED(CONFIG_EFI_MIXED))
-		efi_is64 = is64;
-
 	/* Check if we were booted by the EFI firmware */
 	if (sys_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
 		goto fail;

arch/x86/boot/compressed/head_64.S +3 -4

index 44a6bb6964b5..1f1f6c8139b3 100644
@@ -211,10 +211,9 @@ SYM_FUNC_START(startup_32)
 	movl	efi32_boot_args(%ebp), %edi
 	cmp	$0, %edi
 	jz	1f
-	leal	handover_entry(%ebp), %eax
+	leal	efi64_stub_entry(%ebp), %eax
 	movl	%esi, %edx
 	movl	efi32_boot_args+4(%ebp), %esi
-	movl	$0x0, %ecx
 1:
 #endif
 	pushl	%eax

@@ -242,6 +241,7 @@ SYM_FUNC_START(efi32_stub_entry)
 	movl	%ecx, efi32_boot_args(%ebp)
 	movl	%edx, efi32_boot_args+4(%ebp)
 	sgdtl	efi32_boot_gdt(%ebp)
+	movb	$0, efi_is64(%ebp)
 
 	/* Disable paging */
 	movl	%cr0, %eax

@@ -452,8 +452,6 @@ SYM_CODE_END(startup_64)
 	.org 0x390
 SYM_FUNC_START(efi64_stub_entry)
 SYM_FUNC_START_ALIAS(efi_stub_entry)
-	movq	$1, %rcx
-SYM_INNER_LABEL(handover_entry, SYM_L_LOCAL)
 	and	$~0xf, %rsp			/* realign the stack */
 	call	efi_main
 	movq	%rax,%rsi

@@ -632,6 +630,7 @@ SYM_DATA_END_LABEL(gdt, SYM_L_LOCAL, gdt_end)
 
 #ifdef CONFIG_EFI_MIXED
 SYM_DATA_LOCAL(efi32_boot_args, .long 0, 0)
+SYM_DATA(efi_is64, .byte 1)
 #endif
 
 /*

arch/x86/include/asm/efi.h +1 -1

index 9ce697a621cc..86169a24b0d8 100644
@@ -223,7 +223,7 @@ efi_status_t efi_set_virtual_address_map(unsigned long memory_map_size,
 
 /* arch specific definitions used by the stub code */
 
-__pure bool efi_is_64bit(void);
+__attribute_const__ bool efi_is_64bit(void);
 
 static inline bool efi_is_native(void)
 {

Signature · 1 line

2.20.1

Arvind Sankar

2020-01-08 15:23 UTC #2 lore raw

Re: [PATCH 1/2] efi/libstub/x86: use const attribute for efi_is_64bit()

Quoted 19 lines · Ard Biesheuvel

Reshuffle the x86 stub code a bit so that we can tag the efi_is_64bit() function with the 'const' attribute, which permits the compiler to optimize away any redundant calls. Since we have two different entry points for 32 and 64 bit firmware in the startup code, this also simplifies the C code since we'll enter it with the efi_is64 variable already set.
Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/x86/boot/compressed/eboot.c   | 14 ++++++--------
 arch/x86/boot/compressed/head_64.S |  7 +++----
 arch/x86/include/asm/efi.h         |  2 +-
 3 files changed, 10 insertions(+), 13 deletions(-)
arch/x86/boot/compressed/eboot.c +0 -0
index 4afd29eb5b34..ab3a40283db7 100644

arch/x86/boot/compressed/eboot.c line 25

Quoted 6 lines

arch/x86/boot/compressed/eboot.c +1 -1

@@ -21,16 +21,18 @@
 #include "eboot.h"
 
 static efi_system_table_t *sys_table;
-static bool efi_is64 = IS_ENABLED(CONFIG_X86_64);
+extern const bool efi_is64;

Didn't we need to declare this with hidden visibility? Or use the #pragma GCC visibility push(hidden)?

Ard Biesheuvel

2020-01-08 15:25 UTC #3 lore raw

Re: [PATCH 1/2] efi/libstub/x86: use const attribute for efi_is_64bit()

Quoted 23 lines · Arvind Sankar

Quoted 21 lines · Ard Biesheuvel
Reshuffle the x86 stub code a bit so that we can tag the efi_is_64bit() function with the 'const' attribute, which permits the compiler to optimize away any redundant calls. Since we have two different entry points for 32 and 64 bit firmware in the startup code, this also simplifies the C code since we'll enter it with the efi_is64 variable already set.
Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/x86/boot/compressed/eboot.c   | 14 ++++++--------
 arch/x86/boot/compressed/head_64.S |  7 +++----
 arch/x86/include/asm/efi.h         |  2 +-
 3 files changed, 10 insertions(+), 13 deletions(-)
arch/x86/boot/compressed/eboot.c +0 -0
index 4afd29eb5b34..ab3a40283db7 100644
@@ -21,16 +21,18 @@
 #include "eboot.h"
 

Quoted 7 lines

arch/x86/boot/compressed/eboot.c line 25
Quoted 4 lines
arch/x86/boot/compressed/eboot.c +1 -1
 static efi_system_table_t *sys_table;
-static bool efi_is64 = IS_ENABLED(CONFIG_X86_64);
+extern const bool efi_is64;
 
Didn't we need to declare this with hidden visibility? Or use the #pragma GCC visibility push(hidden)?

Yes. So this patch depends on

Ard Biesheuvel

2020-01-08 15:25 UTC #4 lore raw

Re: [PATCH 1/2] efi/libstub/x86: use const attribute for efi_is_64bit()

Quoted 33 lines · Ard Biesheuvel

Quoted 23 lines · Arvind Sankar
Quoted 21 lines · Ard Biesheuvel
Reshuffle the x86 stub code a bit so that we can tag the efi_is_64bit() function with the 'const' attribute, which permits the compiler to optimize away any redundant calls. Since we have two different entry points for 32 and 64 bit firmware in the startup code, this also simplifies the C code since we'll enter it with the efi_is64 variable already set.
Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/x86/boot/compressed/eboot.c   | 14 ++++++--------
 arch/x86/boot/compressed/head_64.S |  7 +++----
 arch/x86/include/asm/efi.h         |  2 +-
 3 files changed, 10 insertions(+), 13 deletions(-)
arch/x86/boot/compressed/eboot.c +0 -0
index 4afd29eb5b34..ab3a40283db7 100644
@@ -21,16 +21,18 @@
 #include "eboot.h"
 
Quoted 7 lines
arch/x86/boot/compressed/eboot.c line 25
Quoted 4 lines
arch/x86/boot/compressed/eboot.c +1 -1
 static efi_system_table_t *sys_table;
-static bool efi_is64 = IS_ENABLED(CONFIG_X86_64);
+extern const bool efi_is64;
 
Didn't we need to declare this with hidden visibility? Or use the #pragma GCC visibility push(hidden)?

Quoted 1 line

Yes. So this patch depends on

https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git/commit/?h=next&id=c71339946177f235aa1f750b2dc556ede3288c23

which is part of the open PR to the TIP maintainers.

Arvind Sankar

2020-01-08 15:27 UTC #5 lore raw

Re: [PATCH 1/2] efi/libstub/x86: use const attribute for efi_is_64bit()

Quoted 34 lines · Ard Biesheuvel

Quoted 33 lines · Ard Biesheuvel
Quoted 30 lines · Arvind Sankar
Quoted 19 lines · Ard Biesheuvel
Reshuffle the x86 stub code a bit so that we can tag the efi_is_64bit() function with the 'const' attribute, which permits the compiler to optimize away any redundant calls. Since we have two different entry points for 32 and 64 bit firmware in the startup code, this also simplifies the C code since we'll enter it with the efi_is64 variable already set.
Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/x86/boot/compressed/eboot.c   | 14 ++++++--------
 arch/x86/boot/compressed/head_64.S |  7 +++----
 arch/x86/include/asm/efi.h         |  2 +-
 3 files changed, 10 insertions(+), 13 deletions(-)
arch/x86/boot/compressed/eboot.c +0 -0
index 4afd29eb5b34..ab3a40283db7 100644
arch/x86/boot/compressed/eboot.c line 25
Quoted 6 lines
arch/x86/boot/compressed/eboot.c +1 -1
@@ -21,16 +21,18 @@
 #include "eboot.h"
 
 static efi_system_table_t *sys_table;
-static bool efi_is64 = IS_ENABLED(CONFIG_X86_64);
+extern const bool efi_is64;
 
Didn't we need to declare this with hidden visibility? Or use the #pragma GCC visibility push(hidden)?

Quoted 5 lines

Quoted 1 line
Yes. So this patch depends on
https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git/commit/?h=next&id=c71339946177f235aa1f750b2dc556ede3288c23
which is part of the open PR to the TIP maintainers.

Ah sorry.

Ard Biesheuvel

author maintainer

2020-01-08 07:45 UTC #6 lore raw

PATCH 2/2 efi/libstub/x86: use mandatory 16-byte stack alignment in mixed mode

In reply to Ard Biesheuvel

Reduce the stack frame of the EFI stub's mixed mode thunk routine by 8 bytes, by moving the GDT and return addresses to EBP and EBX, which we need to preserve anyway, since their top halves will be cleared by the call into 32-bit firmware code. Doing so results in the UEFI code being entered with a 16 byte aligned stack, as mandated by the UEFI spec, fixing the last occurrence in the 64-bit kernel where we violate this requirement.

Also, move the saved GDT from a global variable to an unused part of the stack frame, and touch up some other parts of the code.

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

---
 arch/x86/boot/compressed/efi_thunk_64.S | 46 ++++++--------------
 1 file changed, 13 insertions(+), 33 deletions(-)

arch/x86/boot/compressed/efi_thunk_64.S +13 ~~-33~~

index d040ff5458e5..8fb7f6799c52 100644
@@ -27,12 +27,9 @@ SYM_FUNC_START(__efi64_thunk)
 	push	%rbp
 	push	%rbx
 
-	subq	$8, %rsp
-	leaq	1f(%rip), %rax
-	movl	%eax, 4(%rsp)
-	leaq	efi_gdt64(%rip), %rax
-	movl	%eax, (%rsp)
-	movl	%eax, 2(%rax)		/* Fixup the gdt base address */
+	leaq	1f(%rip), %rbp
+	leaq	efi_gdt64(%rip), %rbx
+	movl	%ebx, 2(%rbx)		/* Fixup the gdt base address */
 
 	movl	%ds, %eax
 	push	%rax

@@ -48,12 +45,10 @@ SYM_FUNC_START(__efi64_thunk)
 	movl	%esi, 0x0(%rsp)
 	movl	%edx, 0x4(%rsp)
 	movl	%ecx, 0x8(%rsp)
-	movq	%r8, %rsi
-	movl	%esi, 0xc(%rsp)
-	movq	%r9, %rsi
-	movl	%esi,  0x10(%rsp)
+	movl	%r8d, 0xc(%rsp)
+	movl	%r9d, 0x10(%rsp)
 
-	sgdt	save_gdt(%rip)
+	sgdt	0x14(%rsp)
 
 	/*
 	 * Switch to gdt with 32-bit segments. This is the firmware GDT

@@ -68,11 +63,10 @@ SYM_FUNC_START(__efi64_thunk)
 	pushq	%rax
 	lretq
 
-1:	addq	$32, %rsp
+1:	lgdt	0x14(%rsp)
+	addq	$32, %rsp
 	movq	%rdi, %rax
 
-	lgdt	save_gdt(%rip)
-
 	pop	%rbx
 	movl	%ebx, %ss
 	pop	%rbx

@@ -83,15 +77,9 @@ SYM_FUNC_START(__efi64_thunk)
 	/*
 	 * Convert 32-bit status code into 64-bit.
 	 */
-	test	%rax, %rax
-	jz	1f
-	movl	%eax, %ecx
-	andl	$0x0fffffff, %ecx
-	andl	$0xf0000000, %eax
-	shl	$32, %rax
-	or	%rcx, %rax
-1:
-	addq	$8, %rsp
+	roll	$1, %eax
+	rorq	$1, %rax
+
 	pop	%rbx
 	pop	%rbp
 	ret

@@ -135,9 +123,7 @@ SYM_FUNC_START_LOCAL(efi_enter32)
 	 */
 	cli
 
-	movl	56(%esp), %eax
-	movl	%eax, 2(%eax)
-	lgdtl	(%eax)
+	lgdtl	(%ebx)
 
 	movl	%cr4, %eax
 	btsl	$(X86_CR4_PAE_BIT), %eax

@@ -154,9 +140,8 @@ SYM_FUNC_START_LOCAL(efi_enter32)
 	xorl	%eax, %eax
 	lldt	%ax
 
-	movl	60(%esp), %eax
 	pushl	$__KERNEL_CS
-	pushl	%eax
+	pushl	%ebp
 
 	/* Enable paging */
 	movl	%cr0, %eax

@@ -172,11 +157,6 @@ SYM_DATA_START(efi32_boot_gdt)
 	.quad	0
 SYM_DATA_END(efi32_boot_gdt)
 
-SYM_DATA_START_LOCAL(save_gdt)
-	.word	0
-	.quad	0
-SYM_DATA_END(save_gdt)
-
 SYM_DATA_START(efi_gdt64)
 	.word	efi_gdt64_end - efi_gdt64
 	.long	0			/* Filled out by user */

Signature · 1 line

2.20.1