efi/x86: clean up and simplify runtime call wrappers

The efi_call() wrapper used to invoke EFI runtime services serves a number of purposes:
- realign the stack to 16 bytes
- preserve FP register state
- translate from SysV to MS calling convention.

Preserving the FP register state is redundant in most cases, since efi_call() is almost always used from within the scope of a pair of kernel_fpu_begin()/_end() calls, with the exception of the early call to SetVirtualAddressMap() and the SGI UV support code. So let's add a pair of kernel_fpu_begin()/_end() calls there as well, and remove the unnecessary code from the assembly implementation of efi_call(), and only keep the pieces that deal with the stack alignment and the ABI translation.

The efi_call() wrapper used to invoke EFI runtime services serves a number of purposes:
- realign the stack to 16 bytes
- preserve FP register state
- translate from SysV to MS calling convention.

Preserving the FP register state is redundant in most cases, since efi_call() is almost always used from within the scope of a pair of kernel_fpu_begin()/_end() calls, with the exception of the early call to SetVirtualAddressMap() and the SGI UV support code. So let's add a pair of kernel_fpu_begin()/_end() calls there as well, and remove the unnecessary code from the assembly implementation of efi_call(), and only keep the pieces that deal with the stack alignment and the ABI translation.

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

---
 arch/x86/platform/efi/efi_64.c      |  4 +++
 arch/x86/platform/efi/efi_stub_64.S | 36 ++------------------
 arch/x86/platform/uv/bios_uv.c      |  7 ++--
 3 files changed, 11 insertions(+), 36 deletions(-)

arch/x86/platform/efi/efi_64.c +2 -0

index 03c2ed3c645c..3690df1d31c6 100644
@@ -84,6 +84,7 @@ pgd_t * __init efi_call_phys_prolog(void)
 
 	if (!efi_enabled(EFI_OLD_MEMMAP)) {
 		efi_switch_mm(&efi_mm);
+		kernel_fpu_begin();
 		return efi_mm.pgd;
 	}
 

@@ -141,6 +142,7 @@ pgd_t * __init efi_call_phys_prolog(void)
 	}
 
 	__flush_tlb_all();
+	kernel_fpu_begin();
 	return save_pgd;
 out:
 	efi_call_phys_epilog(save_pgd);

Quoted 43 lines · Ard Biesheuvel

The efi_call() wrapper used to invoke EFI runtime services serves a number of purposes:
- realign the stack to 16 bytes
- preserve FP register state
- translate from SysV to MS calling convention.

Preserving the FP register state is redundant in most cases, since efi_call() is almost always used from within the scope of a pair of kernel_fpu_begin()/_end() calls, with the exception of the early call to SetVirtualAddressMap() and the SGI UV support code. So let's add a pair of kernel_fpu_begin()/_end() calls there as well, and remove the unnecessary code from the assembly implementation of efi_call(), and only keep the pieces that deal with the stack alignment and the ABI translation.

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

---
 arch/x86/platform/efi/efi_64.c      |  4 +++
 arch/x86/platform/efi/efi_stub_64.S | 36 ++------------------
 arch/x86/platform/uv/bios_uv.c      |  7 ++--
 3 files changed, 11 insertions(+), 36 deletions(-)

arch/x86/platform/efi/efi_64.c +2 -0

index 03c2ed3c645c..3690df1d31c6 100644
@@ -84,6 +84,7 @@ pgd_t * __init efi_call_phys_prolog(void)
 
 	if (!efi_enabled(EFI_OLD_MEMMAP)) {
 		efi_switch_mm(&efi_mm);
+		kernel_fpu_begin();
 		return efi_mm.pgd;
 	}
 

@@ -141,6 +142,7 @@ pgd_t * __init efi_call_phys_prolog(void)
 	}
 
 	__flush_tlb_all();
+	kernel_fpu_begin();
 	return save_pgd;
 out:
 	efi_call_phys_epilog(save_pgd);

arch/x86/platform/efi/efi_64.c void __init efi_call_phys_epilog(pgd_t *save_pgd) · line 167

Quoted 8 lines

arch/x86/platform/efi/efi_64.c +2 -0

@@ -158,6 +160,8 @@ void __init efi_call_phys_epilog(pgd_t *save_pgd)
 	p4d_t *p4d;
 	pud_t *pud;
 
+	kernel_fpu_end();
+
 	if (!efi_enabled(EFI_OLD_MEMMAP)) {
 		efi_switch_mm(efi_scratch.prev_mm);
 		return;

Does kernel_fpu_begin/kernel_fpu_end need to be outside the efi_switch_mm?

If there's an error in efi_call_phys_prolog during the old memmap code, it will call efi_call_phys_epilog without having called kernel_fpu_begin, which will cause an unbalanced kernel_fpu_end. Looks like the next step will be a panic anyway though.

Quoted 55 lines · Arvind Sankar

Quoted 43 lines · Ard Biesheuvel

The efi_call() wrapper used to invoke EFI runtime services serves a number of purposes:
- realign the stack to 16 bytes
- preserve FP register state
- translate from SysV to MS calling convention.

Preserving the FP register state is redundant in most cases, since efi_call() is almost always used from within the scope of a pair of kernel_fpu_begin()/_end() calls, with the exception of the early call to SetVirtualAddressMap() and the SGI UV support code. So let's add a pair of kernel_fpu_begin()/_end() calls there as well, and remove the unnecessary code from the assembly implementation of efi_call(), and only keep the pieces that deal with the stack alignment and the ABI translation.

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

---
 arch/x86/platform/efi/efi_64.c      |  4 +++
 arch/x86/platform/efi/efi_stub_64.S | 36 ++------------------
 arch/x86/platform/uv/bios_uv.c      |  7 ++--
 3 files changed, 11 insertions(+), 36 deletions(-)

arch/x86/platform/efi/efi_64.c +2 -0

index 03c2ed3c645c..3690df1d31c6 100644
@@ -84,6 +84,7 @@ pgd_t * __init efi_call_phys_prolog(void)
 
    if (!efi_enabled(EFI_OLD_MEMMAP)) {
            efi_switch_mm(&efi_mm);
+           kernel_fpu_begin();
            return efi_mm.pgd;
    }
 

@@ -141,6 +142,7 @@ pgd_t * __init efi_call_phys_prolog(void)
    }
 
    __flush_tlb_all();
+   kernel_fpu_begin();
    return save_pgd;
 out:
    efi_call_phys_epilog(save_pgd);

arch/x86/platform/efi/efi_64.c void __init efi_call_phys_epilog(pgd_t *save_pgd) · line 167

Quoted 8 lines

arch/x86/platform/efi/efi_64.c +2 -0

@@ -158,6 +160,8 @@ void __init efi_call_phys_epilog(pgd_t *save_pgd)
    p4d_t *p4d;
    pud_t *pud;
 
+   kernel_fpu_end();
+
    if (!efi_enabled(EFI_OLD_MEMMAP)) {
            efi_switch_mm(efi_scratch.prev_mm);
            return;

Does kernel_fpu_begin/kernel_fpu_end need to be outside the efi_switch_mm?

Quoted 4 lines

If there's an error in efi_call_phys_prolog during the old memmap code, it will call efi_call_phys_epilog without having called kernel_fpu_begin, which will cause an unbalanced kernel_fpu_end. Looks like the next step will be a panic anyway though.

Do we even need to save/restore the fpu state at this point in boot? The mixed-mode code path doesn't appear to be saving/restoring the XMM registers during SetVirtualAddressMap.

Quoted 57 lines · Arvind Sankar

Quoted 57 lines · Arvind Sankar

Quoted 43 lines · Ard Biesheuvel

The efi_call() wrapper used to invoke EFI runtime services serves a number of purposes:
- realign the stack to 16 bytes
- preserve FP register state
- translate from SysV to MS calling convention.

Preserving the FP register state is redundant in most cases, since efi_call() is almost always used from within the scope of a pair of kernel_fpu_begin()/_end() calls, with the exception of the early call to SetVirtualAddressMap() and the SGI UV support code. So let's add a pair of kernel_fpu_begin()/_end() calls there as well, and remove the unnecessary code from the assembly implementation of efi_call(), and only keep the pieces that deal with the stack alignment and the ABI translation.

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

---

arch/x86/platform/efi/efi_64.c      |  4 +++
arch/x86/platform/efi/efi_stub_64.S | 36 ++------------------
arch/x86/platform/uv/bios_uv.c      |  7 ++--
3 files changed, 11 insertions(+), 36 deletions(-)

arch/x86/platform/efi/efi_64.c +2 -0

index 03c2ed3c645c..3690df1d31c6 100644
@@ -84,6 +84,7 @@ pgd_t * __init efi_call_phys_prolog(void)
 
   if (!efi_enabled(EFI_OLD_MEMMAP)) {
           efi_switch_mm(&efi_mm);
+           kernel_fpu_begin();
           return efi_mm.pgd;
   }
 

@@ -141,6 +142,7 @@ pgd_t * __init efi_call_phys_prolog(void)
   }
 
   __flush_tlb_all();
+   kernel_fpu_begin();
   return save_pgd;

out:

   efi_call_phys_epilog(save_pgd);

arch/x86/platform/efi/efi_64.c void __init efi_call_phys_epilog(pgd_t *save_pgd) · line 167

Quoted 8 lines

arch/x86/platform/efi/efi_64.c +2 -0

@@ -158,6 +160,8 @@ void __init efi_call_phys_epilog(pgd_t *save_pgd)
   p4d_t *p4d;
   pud_t *pud;
 
+   kernel_fpu_end();
+
   if (!efi_enabled(EFI_OLD_MEMMAP)) {
           efi_switch_mm(efi_scratch.prev_mm);
           return;

Does kernel_fpu_begin/kernel_fpu_end need to be outside the efi_switch_mm?

Quoted 8 lines

Quoted 4 lines

If there's an error in efi_call_phys_prolog during the old memmap code, it will call efi_call_phys_epilog without having called kernel_fpu_begin, which will cause an unbalanced kernel_fpu_end. Looks like the next step will be a panic anyway though.

Do we even need to save/restore the fpu state at this point in boot? The mixed-mode code path doesn't appear to be saving/restoring the XMM registers during SetVirtualAddressMap.

That is an excellent question, and I was hoping Andy or Ingo could shed some light on that.

I tested without and it booted fine, and it does seem to me that there should be very little to preserve given how early this call happens (from efi_enter_virtual_mode() which gets called from start_kernel())

Quoted 21 lines

On Dec 28, 2019, at 2:13 AM, Ard Biesheuvel
<[email protected]> wrote:

On Fri, 27 Dec 2019 at 19:08, Arvind Sankar

Quoted 5 lines · <[email protected]> wrote:

Quoted 5 lines · Arvind Sankar

Does kernel_fpu_begin/kernel_fpu_end need to be outside the efi_switch_mm?

Quoted 8 lines

Quoted 4 lines

If there's an error in efi_call_phys_prolog during the old memmap code, it will call efi_call_phys_epilog without having called kernel_fpu_begin, which will cause an unbalanced kernel_fpu_end. Looks like the next step will be a panic anyway though.

Do we even need to save/restore the fpu state at this point in boot? The mixed-mode code path doesn't appear to be saving/restoring the XMM registers during SetVirtualAddressMap.

Quoted 7 lines

That is an excellent question, and I was hoping Andy or Ingo could shed some light on that.

I tested without and it booted fine, and it does seem to me that there should be very little to preserve given how early this call happens (from efi_enter_virtual_mode() which gets called from start_kernel())

Unless you’re somehow calling it from interrupt context, I doubt saving FP regs is needed. Certainly kernel_fpu_begin() doesn’t do anything that matters if we’re in the (misnamed) init task. If you’re calling it really really early, there’s a different potential issue:
FP might not be fully initialized. We could have CR0.TS still set, or we might not have all the various “OS supports such-and-such regs” flags set.

Does the UEFI spec explicitly state what FP state can be used by the EFI functions?

Quoted 5 lines

Does the UEFI spec explicitly state what FP state can be used by the EFI functions?

For 32-bit, it requires the following for boot services and runtime services:

* Direction flag in EFLAGs clear

* 4 KiB, or more, of available stack space

* The stack must be 16-byte aligned

* Floating-point control word must be initialized to 0x027F (all exceptions masked, double- precision, round-to-nearest)

* Multimedia-extensions control word (if supported) must be initialized to 0x1F80 (all exceptions masked, round-to-nearest, flush to zero for masked underflow)

* CR0.EM must be zero
* CR0.TS must be zero

We don't actually align the stack for 32-bit mode before calling, do we?

No FP registers are clobbered except the floating point status register.

For 64-bit:
* Direction flag in EFLAGs clear
* 4 KiB, or more, of available stack space

* The stack must be 16-byte aligned

* Floating-point control word must be initialized to 0x037F (all exceptions masked, double- extended-precision, round-to-nearest)
* Multimedia-extensions control word (if supported) must be initialized to 0x1F80 (all exceptions masked, round-to-nearest, flush to zero for masked underflow)

* CR0.EM must be zero
* CR0.TS must be zero

xmm0-5 (and the FP status register) may be clobbered, the rest of the FP registers are preserved.

Quoted 2 lines

* The stack must be 16-byte aligned

Nope. The asm needs to do this for runtime services. The kernel runs with 8-byte stack alignment.

Quoted 5 lines · Andy Lutomirski

Quoted 2 lines

* The stack must be 16-byte aligned

Nope. The asm needs to do this for runtime services. The kernel runs with 8-byte stack alignment.

32-bit code is actually only 4-byte aligned in the kernel proper, right?

Currently, only native 64-bit calls always respect the 16-byte alignment requirement, by aligning explicitly in the asm stubs, or after the cleanup patches, via the efi bootloader running with 16-byte stack alignment.

I think mixed mode might actually be aligned via the asm stub in the kernel proper, though it doesn't look like it is in the bootloader portion.

Quoted 12 lines · Arvind Sankar

Quoted 5 lines · Andy Lutomirski

Quoted 2 lines

* The stack must be 16-byte aligned

Nope. The asm needs to do this for runtime services. The kernel runs with 8-byte stack alignment.

32-bit code is actually only 4-byte aligned in the kernel proper, right?

Right. By “8” I meant “long”. Sorry.

Quoted 6 lines

Currently, only native 64-bit calls always respect the 16-byte alignment requirement, by aligning explicitly in the asm stubs, or after the cleanup patches, via the efi bootloader running with 16-byte stack alignment.

Quoted 3 lines

I think mixed mode might actually be aligned via the asm stub in the kernel proper, though it doesn't look like it is in the bootloader portion.

The underlying problem is that gcc doesn’t give us a way to do CALL from asm while preserving more than a single word of alignment. This forces us to compile the kernel proper with reduced alignment. (Also, the generated code is better with reduced alignment.)

Quoted 25 lines · Andy Lutomirski

Quoted 5 lines

Quoted 4 lines · Arvind Sankar

Quoted 7 lines

Quoted 5 lines · Andy Lutomirski

Quoted 2 lines

* The stack must be 16-byte aligned

Nope. The asm needs to do this for runtime services. The kernel runs with 8-byte stack alignment.

32-bit code is actually only 4-byte aligned in the kernel proper, right?

Right. By “8” I meant “long”. Sorry.

Quoted 9 lines

Currently, only native 64-bit calls always respect the 16-byte alignment requirement, by aligning explicitly in the asm stubs, or after the cleanup patches, via the efi bootloader running with 16-byte stack alignment.

I think mixed mode might actually be aligned via the asm stub in the kernel proper, though it doesn't look like it is in the bootloader portion.

Quoted 1 line

The underlying problem is that gcc doesn’t give us a way to do CALL from asm while preserving more than a single word of alignment. This forces us to compile the kernel proper with reduced alignment. (Also, the generated code is better with reduced alignment.)

At runtime, the 64-bit kernel always uses a 16 byte aligned stack when calling into EFI (32 or 64 bit), either by aligning the stack pointer, or by switching to a special stack.

Quoted 31 lines · Ard Biesheuvel

Quoted 25 lines · Andy Lutomirski

Quoted 5 lines

Quoted 4 lines · Arvind Sankar

Quoted 7 lines

Quoted 5 lines · Andy Lutomirski

Quoted 2 lines

* The stack must be 16-byte aligned

Nope. The asm needs to do this for runtime services. The kernel runs with 8-byte stack alignment.

32-bit code is actually only 4-byte aligned in the kernel proper, right?

Right. By “8” I meant “long”. Sorry.

Quoted 9 lines

Currently, only native 64-bit calls always respect the 16-byte alignment requirement, by aligning explicitly in the asm stubs, or after the cleanup patches, via the efi bootloader running with 16-byte stack alignment.

I think mixed mode might actually be aligned via the asm stub in the kernel proper, though it doesn't look like it is in the bootloader portion.

Quoted 5 lines

Quoted 1 line

The underlying problem is that gcc doesn’t give us a way to do CALL from asm while preserving more than a single word of alignment. This forces us to compile the kernel proper with reduced alignment. (Also, the generated code is better with reduced alignment.)

At runtime, the 64-bit kernel always uses a 16 byte aligned stack when calling into EFI (32 or 64 bit), either by aligning the stack pointer, or by switching to a special stack.

Can you point me at the stack switching code? Stack switches always make me nervous due to interactions with other things, especially NMIs.

Calling 32-bit EFI runtime services from a 64-bit OS involves switching back to the flat mapping with a stack carved out of memory that is 32-bit addressable.

There is no need to actually execute the 64-bit part of this routine from the flat mapping as well, as long as the entry and return address fit in 32 bits. There is also no need to preserve part of the calling context in global variables: we can simply preserve the old stack pointer in %r11 across the call into 32-bit firmware, and use either stack to preserve other values.

arch/x86/platform/efi/efi_thunk_64.S +0 -0

index 3189f1394701..7357808d3ae8 100644

Quoted 5 lines · Ard Biesheuvel

Calling 32-bit EFI runtime services from a 64-bit OS involves switching back to the flat mapping with a stack carved out of memory that is 32-bit addressable.

Quoted 7 lines

There is no need to actually execute the 64-bit part of this routine from the flat mapping as well, as long as the entry and return address fit in 32 bits. There is also no need to preserve part of the calling context in global variables: we can simply preserve the old stack pointer in %r11 across the call into 32-bit firmware, and use either stack to preserve other values.

The %r11 trick makes me a little bit nervous. I can imagine a 32-bit firmware implementation clobbering r11 by one of a few means: SMM bugs (unlikely -- this would probably kill the system even outside of an EFI call) or, more likely, if some code module is actualy 64-bit. Maybe we shouldn't be worried about this. More comments below.

Quoted 5 lines

arch/x86/platform/efi/efi_thunk_64.S +0 -0

index 3189f1394701..7357808d3ae8 100644

arch/x86/platform/efi/efi_thunk_64.S line 36

Quoted 9 lines

arch/x86/platform/efi/efi_thunk_64.S +6 -0

@@ -28,11 +28,17 @@
 SYM_FUNC_START(efi64_thunk)
        push    %rbp
        push    %rbx
+       movl    %ds, %ebx
+       push    %rbx
+       movl    %es, %ebx
+       push    %rbx
+       movl    %ss, %ebx
+       push    %rbx

I realize that you haven't actually changed any of the below, but this code has issues.

You don't actually need to save %ss. Loading KERNEL_DS is fine. 0 would almost be fine, except that AMD CPUs have some oddities and the fallout would be subtle and annoying to debug.

The kernel does not strictly guarantee that the selectors in DS and ES are always valid. They're fairly likely to be valid when running syscalls, but code like this should not bet on it. And the EFI thunk is missing exception handlers when it reloads them. So the right thing to do is probably to get rid of all the segment handling in the asm for everything except CS and to move it into C, like:

unsigned short ds, es;

/* DS and ES contain user values. We need to save them. */ savesegment(ds, ds);
savesegment(es, es);

/* The 32-bit EFI code needs a valid DS, ES, and SS. There's no need to save the old SS: __KERNEL_DS is always acceptable. */ loadsegment(ss, __KERNEL_DS);
loadsegment(ds, __KERNEL_DS);
loadsegment(es, __KERNEL_DS);

__s = efi64_thunk(...);

loadsegment(ds, ds);
loadsegment(es, es);

Want to make that change?

Calling 32-bit EFI runtime services from a 64-bit OS involves switching back to the flat mapping with a stack carved out of memory that is 32-bit addressable.

There is no need to actually execute the 64-bit part of this routine from the flat mapping as well, as long as the entry and return address fit in 32 bits. There is also no need to preserve part of the calling context in global variables: we can simply preserve the old stack pointer in %r11 across the call into 32-bit firmware, and use either stack to preserve other values.

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

---
 arch/x86/platform/efi/efi_thunk_64.S | 106 ++++++--------------
 1 file changed, 29 insertions(+), 77 deletions(-)

arch/x86/platform/efi/efi_thunk_64.S +6 -2

index 3189f1394701..7357808d3ae8 100644
+	/*
+	 * Convert 32-bit status code into 64-bit.
+	 */
+1:	btrl	$31, %eax
+	jb	3f
+2:	mov	%r11, %rsp
 	pop	%rbx
 	movl	%ebx, %ss
 	pop	%rbx
 	movl	%ebx, %es
 	pop	%rbx
 	movl	%ebx, %ds
-
-	/*

Quoted 44 lines · Ard Biesheuvel

Calling 32-bit EFI runtime services from a 64-bit OS involves switching back to the flat mapping with a stack carved out of memory that is 32-bit addressable.

There is no need to actually execute the 64-bit part of this routine from the flat mapping as well, as long as the entry and return address fit in 32 bits. There is also no need to preserve part of the calling context in global variables: we can simply preserve the old stack pointer in %r11 across the call into 32-bit firmware, and use either stack to preserve other values.

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

---
 arch/x86/platform/efi/efi_thunk_64.S | 106 ++++++--------------
 1 file changed, 29 insertions(+), 77 deletions(-)

arch/x86/platform/efi/efi_thunk_64.S +6 -11

index 3189f1394701..7357808d3ae8 100644
+     /*
+      * Convert 32-bit status code into 64-bit.
+      */
+1:   btrl    $31, %eax
+     jb      3f
+2:   mov     %r11, %rsp
      pop     %rbx
      movl    %ebx, %ss
      pop     %rbx
      movl    %ebx, %es
      pop     %rbx
      movl    %ebx, %ds
-
-     /*
-      * 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

Quoted 1 line

-1:

Is it worth optimizing the conversion? The entire high nibble is significant according to the spec. It probably doesn't matter except in one potential case: according to the spec, transitioning secure boot status to setup mode by deleting the platform key is allowed to return EFI_WARN_RESET_REQUIRED and AFAICT this can take place after ExitBootServices?

Separately, it might be worth considering moving the status translation into C instead of asm for the version in arch/x86/boot/compressed/efi_thunk_64.S -- there are (at least) three protocols that have methods that don't return efi_status_t:
DEVICE_PATH_UTILITIES, DEVICE_PATH_TO_TEXT and DEVICE_PATH_FROM_TEXT. If we ever want to use them (eg for debugging), it might be worth having a thunk that doesn't mangle the return value.

In theory, yes. But all this code does is move the top bit from bit 31 to 63, which should be fine to convert any return code you may receive from a runtime service.