[PATCH 0/3] efi/x86: clean up and simplify runtime call wrappers

[PATCH 0/3] efi/x86: clean up and simplify runtime call wrappers

[Ard Biesheuvel]

There are three different ways the x86 kernel can call into EFI firmware
at runtime, (native 64 bit, native 32 bit or mixed mode), and for each
of them, we have a special wrapper routine written in assembler that
deals with the peculiarities of ABI translation, 1:1 mapping of memory
etc.

The 64-bit version can be simplified, by getting rid of the FP register
preserve/restore, which is redundant in most cases.

The 32-bit version is only used to call the SetVirtualAddressMap EFI
service, so with that taken into account, we can simplify it a lot as well.

The mixed mode version can be simplified too, by using the stack instead
of global variables to store context while the EFI call is in progress.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>

Ard Biesheuvel (3):
  efi/x86: simplify 64-bit EFI firmware call wrapper
  efi/x86: simplify i386 efi_call_phys() firmware call wrapper
  efi/x86: simplify mixed mode call wrapper

 arch/x86/include/asm/efi.h           |   3 +-
 arch/x86/platform/efi/efi_64.c       |   4 +
 arch/x86/platform/efi/efi_stub_32.S  | 106 +++-----------------
 arch/x86/platform/efi/efi_stub_64.S  |  36 +------
 arch/x86/platform/efi/efi_thunk_64.S | 106 ++++++--------------
 arch/x86/platform/uv/bios_uv.c       |   7 +-
 6 files changed, 58 insertions(+), 204 deletions(-)

-- 
2.17.1

[PATCH 1/3] efi/x86: simplify 64-bit EFI firmware call wrapper

[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 <ardb@kernel.org>
---
 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(-)

diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 03c2ed3c645c..3690df1d31c6 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -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);
@@ -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;
diff --git a/arch/x86/platform/efi/efi_stub_64.S b/arch/x86/platform/efi/efi_stub_64.S
index b1d2313fe3bf..3e44d55ac730 100644
--- a/arch/x86/platform/efi/efi_stub_64.S
+++ b/arch/x86/platform/efi/efi_stub_64.S
@@ -8,41 +8,11 @@
  */
 
 #include <linux/linkage.h>
-#include <asm/segment.h>
-#include <asm/msr.h>
-#include <asm/processor-flags.h>
-#include <asm/page_types.h>
-
-#define SAVE_XMM			\
-	mov %rsp, %rax;			\
-	subq $0x70, %rsp;		\
-	and $~0xf, %rsp;		\
-	mov %rax, (%rsp);		\
-	mov %cr0, %rax;			\
-	clts;				\
-	mov %rax, 0x8(%rsp);		\
-	movaps %xmm0, 0x60(%rsp);	\
-	movaps %xmm1, 0x50(%rsp);	\
-	movaps %xmm2, 0x40(%rsp);	\
-	movaps %xmm3, 0x30(%rsp);	\
-	movaps %xmm4, 0x20(%rsp);	\
-	movaps %xmm5, 0x10(%rsp)
-
-#define RESTORE_XMM			\
-	movaps 0x60(%rsp), %xmm0;	\
-	movaps 0x50(%rsp), %xmm1;	\
-	movaps 0x40(%rsp), %xmm2;	\
-	movaps 0x30(%rsp), %xmm3;	\
-	movaps 0x20(%rsp), %xmm4;	\
-	movaps 0x10(%rsp), %xmm5;	\
-	mov 0x8(%rsp), %rsi;		\
-	mov %rsi, %cr0;			\
-	mov (%rsp), %rsp
 
 SYM_FUNC_START(efi_call)
 	pushq %rbp
 	movq %rsp, %rbp
-	SAVE_XMM
+	and $~0xf, %rsp
 	mov 16(%rbp), %rax
 	subq $48, %rsp
 	mov %r9, 32(%rsp)
@@ -51,8 +21,6 @@ SYM_FUNC_START(efi_call)
 	mov %rcx, %r8
 	mov %rsi, %rcx
 	call *%rdi
-	addq $48, %rsp
-	RESTORE_XMM
-	popq %rbp
+	leave
 	ret
 SYM_FUNC_END(efi_call)
diff --git a/arch/x86/platform/uv/bios_uv.c b/arch/x86/platform/uv/bios_uv.c
index ece9cb9c1189..5c0e2eb5d87c 100644
--- a/arch/x86/platform/uv/bios_uv.c
+++ b/arch/x86/platform/uv/bios_uv.c
@@ -34,10 +34,13 @@ static s64 __uv_bios_call(enum uv_bios_cmd which, u64 a1, u64 a2, u64 a3,
 	 * If EFI_OLD_MEMMAP is set, we need to fall back to using our old EFI
 	 * callback method, which uses efi_call() directly, with the kernel page tables:
 	 */
-	if (unlikely(efi_enabled(EFI_OLD_MEMMAP)))
+	if (unlikely(efi_enabled(EFI_OLD_MEMMAP))) {
+		kernel_fpu_begin();
 		ret = efi_call((void *)__va(tab->function), (u64)which, a1, a2, a3, a4, a5);
-	else
+		kernel_fpu_end();
+	} else {
 		ret = efi_call_virt_pointer(tab, function, (u64)which, a1, a2, a3, a4, a5);
+	}
 
 	return ret;
 }
-- 
2.17.1

Re: [PATCH 1/3] efi/x86: simplify 64-bit EFI firmware call wrapper

[Andy Lutomirski]

On Thu, Dec 26, 2019 at 7:16 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> 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.

FWIW, you also removed the CR0 save/restore.  This ought to be fine,
but it might be worth mentioning in the changelog.  (At one point,
CR0.TS was dynamic on Linux, but these days it's always cleared, so
that part was presumably just a historical wart.)

Reviewed-by: Andy Lutomirski <luto@kernel.org>

--Andy

Re: [PATCH 1/3] efi/x86: simplify 64-bit EFI firmware call wrapper

[Arvind Sankar]

On Thu, Dec 26, 2019 at 04:14:05PM +0100, Ard Biesheuvel wrote:
> 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 <ardb@kernel.org>
> ---
>  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(-)
> 
> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> index 03c2ed3c645c..3690df1d31c6 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -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);
> @@ -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.

Re: [PATCH 1/3] efi/x86: simplify 64-bit EFI firmware call wrapper

[Arvind Sankar]

On Fri, Dec 27, 2019 at 12:51:56PM -0500, Arvind Sankar wrote:
> On Thu, Dec 26, 2019 at 04:14:05PM +0100, Ard Biesheuvel wrote:
> > 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 <ardb@kernel.org>
> > ---
> >  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(-)
> > 
> > diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> > index 03c2ed3c645c..3690df1d31c6 100644
> > --- a/arch/x86/platform/efi/efi_64.c
> > +++ b/arch/x86/platform/efi/efi_64.c
> > @@ -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);
> > @@ -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.

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.

Re: [PATCH 1/3] efi/x86: simplify 64-bit EFI firmware call wrapper

[Ard Biesheuvel]

On Fri, 27 Dec 2019 at 19:08, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Fri, Dec 27, 2019 at 12:51:56PM -0500, Arvind Sankar wrote:
> > On Thu, Dec 26, 2019 at 04:14:05PM +0100, Ard Biesheuvel wrote:
> > > 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 <ardb@kernel.org>
> > > ---
> > >  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(-)
> > >
> > > diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> > > index 03c2ed3c645c..3690df1d31c6 100644
> > > --- a/arch/x86/platform/efi/efi_64.c
> > > +++ b/arch/x86/platform/efi/efi_64.c
> > > @@ -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);
> > > @@ -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.
>
> 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())

Re: [PATCH 1/3] efi/x86: simplify 64-bit EFI firmware call wrapper

[Andy Lutomirski]

> On Dec 28, 2019, at 2:13 AM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> 
> On Fri, 27 Dec 2019 at 19:08, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>> 
>>> On Fri, Dec 27, 2019 at 12:51:56PM -0500, Arvind Sankar wrote:
>>> On Thu, Dec 26, 2019 at 04:14:05PM +0100, Ard Biesheuvel wrote:
>>>> 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 <ardb@kernel.org>
>>>> ---
>>>> 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(-)
>>>> 
>>>> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
>>>> index 03c2ed3c645c..3690df1d31c6 100644
>>>> --- a/arch/x86/platform/efi/efi_64.c
>>>> +++ b/arch/x86/platform/efi/efi_64.c
>>>> @@ -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);
>>>> @@ -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.
>> 
>> 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())


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?

Re: [PATCH 1/3] efi/x86: simplify 64-bit EFI firmware call wrapper

[Arvind Sankar]

On Sat, Dec 28, 2019 at 11:25:49AM +0800, Andy Lutomirski wrote:
> 
> 
> > On Dec 28, 2019, at 2:13 AM, Ard Biesheuvel
> > <ard.biesheuvel@linaro.org> wrote:
> > 
> > On Fri, 27 Dec 2019 at 19:08, Arvind Sankar
> > <nivedita@alum.mit.edu> wrote:
> >> 
> >>> On Fri, Dec 27, 2019 at 12:51:56PM -0500, Arvind Sankar wrote:
> >>> 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.
> >> 
> >> 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())
> 
> 
> 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?

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.

Re: [PATCH 1/3] efi/x86: simplify 64-bit EFI firmware call wrapper

[Andy Lutomirski]

> On Dec 28, 2019, at 12:44 PM, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> 

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

Check.

> * 4 KiB, or more, of available stack space

There ought to be.

> * 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.

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

Ingo, surely kernel_fpu_begin() does this.  But I can’t find any code that does this. Please tell me I’m just missing it because I’m on my phone.

Presumably the code ought to optimize it by saving regs, then checking for unexpected values, then doing FNINIT and STMXCSR if the state is bad.

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

Ditto.

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

Check, on modern kernels anyway.

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

I don’t believe this for a minute :).

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

Check.

> * The stack must be 16-byte aligned

Nope. We need to align it. Does the EFI runtime service code do this?

> * 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)

See above.

> * 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.

Seems reasonable.

Re: [PATCH 1/3] efi/x86: simplify 64-bit EFI firmware call wrapper

[Arvind Sankar]

On Sat, Dec 28, 2019 at 01:29:00PM +0800, Andy Lutomirski wrote:
> 
> > * 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.

Re: [PATCH 1/3] efi/x86: simplify 64-bit EFI firmware call wrapper

[Andy Lutomirski]

> On Dec 28, 2019, at 2:35 PM, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> 
> On Sat, Dec 28, 2019 at 01:29:00PM +0800, Andy Lutomirski wrote:
>> 
>>> * 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.

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

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.)

Re: [PATCH 1/3] efi/x86: simplify 64-bit EFI firmware call wrapper

[Ard Biesheuvel]

On Sat, 28 Dec 2019 at 08:03, Andy Lutomirski <luto@amacapital.net> wrote:
>
>
>
> > On Dec 28, 2019, at 2:35 PM, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > On Sat, Dec 28, 2019 at 01:29:00PM +0800, Andy Lutomirski wrote:
> >>
> >>> * 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.
>
> >
> > 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.
>
> 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. On 32-bit kernels, the EFI calls
are simply indirect calls generated by the compiler, so there we may
enter with a misaligned stack pointer.

The EFI stub+decompressor are not built with
-mpreferred-stack-boundary=3, and so as long as we ensure that we
enter the C code with the proper alignment, the EFI calls will see the
correct alignment as well. We currently only do this for native 64-bit
boot, though, as 32-bit EFI firmware doesn't seem to require 16-byte
alignment in practice.

Re: [PATCH 1/3] efi/x86: simplify 64-bit EFI firmware call wrapper

[Andy Lutomirski]

> On Dec 28, 2019, at 4:51 PM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> 
> On Sat, 28 Dec 2019 at 08:03, Andy Lutomirski <luto@amacapital.net> wrote:
>> 
>> 
>> 
>>>> On Dec 28, 2019, at 2:35 PM, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>>> 
>>> On Sat, Dec 28, 2019 at 01:29:00PM +0800, Andy Lutomirski wrote:
>>>> 
>>>>> * 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.
>> 
>>> 
>>> 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.
>> 
>> 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.

Re: [PATCH 1/3] efi/x86: simplify 64-bit EFI firmware call wrapper

[Ard Biesheuvel]

On Sat, 28 Dec 2019 at 10:00, Andy Lutomirski <luto@amacapital.net> wrote:
>
>
>
> > On Dec 28, 2019, at 4:51 PM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >
> > On Sat, 28 Dec 2019 at 08:03, Andy Lutomirski <luto@amacapital.net> wrote:
> >>
> >>
> >>
> >>>> On Dec 28, 2019, at 2:35 PM, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >>>
> >>> On Sat, Dec 28, 2019 at 01:29:00PM +0800, Andy Lutomirski wrote:
> >>>>
> >>>>> * 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.
> >>
> >>>
> >>> 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.
> >>
> >> 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.

It's in patch 3/3 in this series.

[PATCH 2/3] efi/x86: simplify i386 efi_call_phys() firmware call wrapper

[Ard Biesheuvel]

The variadic efi_call_phys() wrapper that exists on i386 was
originally created to call into any EFI firmware runtime service,
but in practice, we only use it once, to call SetVirtualAddressMap()
during early boot.
The flexibility provided by the variadic nature also makes it
type unsafe, and makes the assembler code more complicated than
needed, since it has to deal with an unknown number of arguments
living on the stack.

So let's make the prototype of efi_call_phys() non-variadic, and
clean up the assembler code and make it deal with a fixed number
of arguments.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/include/asm/efi.h          |   3 +-
 arch/x86/platform/efi/efi_stub_32.S | 106 +++-----------------
 2 files changed, 18 insertions(+), 91 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index a873bd06cfdd..fb213ee3870b 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -36,7 +36,8 @@
 
 #ifdef CONFIG_X86_32
 
-extern asmlinkage unsigned long efi_call_phys(void *, ...);
+extern asmlinkage
+unsigned long efi_call_phys(void *, unsigned long, unsigned long, u32, void *);
 
 #define arch_efi_call_virt_setup()					\
 ({									\
diff --git a/arch/x86/platform/efi/efi_stub_32.S b/arch/x86/platform/efi/efi_stub_32.S
index eed8b5b441f8..d21292ca93b4 100644
--- a/arch/x86/platform/efi/efi_stub_32.S
+++ b/arch/x86/platform/efi/efi_stub_32.S
@@ -7,118 +7,44 @@
  */
 
 #include <linux/linkage.h>
+#include <linux/init.h>
 #include <asm/page_types.h>
 
-/*
- * efi_call_phys(void *, ...) is a function with variable parameters.
- * All the callers of this function assure that all the parameters are 4-bytes.
- */
-
-/*
- * In gcc calling convention, EBX, ESP, EBP, ESI and EDI are all callee save.
- * So we'd better save all of them at the beginning of this function and restore
- * at the end no matter how many we use, because we can not assure EFI runtime
- * service functions will comply with gcc calling convention, too.
- */
-
-.text
+	__INIT
 SYM_FUNC_START(efi_call_phys)
-	/*
-	 * 0. The function can only be called in Linux kernel. So CS has been
-	 * set to 0x0010, DS and SS have been set to 0x0018. In EFI, I found
-	 * the values of these registers are the same. And, the corresponding
-	 * GDT entries are identical. So I will do nothing about segment reg
-	 * and GDT, but change GDT base register in prolog and epilog.
-	 */
+	movl	4(%esp), %eax
+	push	20(%esp)
+	push	20(%esp)
+	push	20(%esp)
+	push	20(%esp)
 
 	/*
-	 * 1. Now I am running with EIP = <physical address> + PAGE_OFFSET.
-	 * But to make it smoothly switch from virtual mode to flat mode.
-	 * The mapping of lower virtual memory has been created in prolog and
-	 * epilog.
+	 * Switch to the flat mapped alias of this routine, by jumping to the
+	 * address of label '1' after subtracting PAGE_OFFSET from it.
 	 */
 	movl	$1f, %edx
 	subl	$__PAGE_OFFSET, %edx
 	jmp	*%edx
 1:
 
-	/*
-	 * 2. Now on the top of stack is the return
-	 * address in the caller of efi_call_phys(), then parameter 1,
-	 * parameter 2, ..., param n. To make things easy, we save the return
-	 * address of efi_call_phys in a global variable.
-	 */
-	popl	%edx
-	movl	%edx, saved_return_addr
-	/* get the function pointer into ECX*/
-	popl	%ecx
-	movl	%ecx, efi_rt_function_ptr
-	movl	$2f, %edx
-	subl	$__PAGE_OFFSET, %edx
-	pushl	%edx
-
-	/*
-	 * 3. Clear PG bit in %CR0.
-	 */
+	/* disable paging */
 	movl	%cr0, %edx
 	andl	$0x7fffffff, %edx
 	movl	%edx, %cr0
-	jmp	1f
-1:
 
-	/*
-	 * 4. Adjust stack pointer.
-	 */
+	/* convert the stack pointer to a flat mapped address */
 	subl	$__PAGE_OFFSET, %esp
 
-	/*
-	 * 5. Call the physical function.
-	 */
-	jmp	*%ecx
+	/* call the EFI routine */
+	call	*%eax
 
-2:
-	/*
-	 * 6. After EFI runtime service returns, control will return to
-	 * following instruction. We'd better readjust stack pointer first.
-	 */
-	addl	$__PAGE_OFFSET, %esp
+	/* convert ESP back to a kernel VA, and pop the outgoing args */
+	addl	$__PAGE_OFFSET + 16, %esp
 
-	/*
-	 * 7. Restore PG bit
-	 */
+	/* re-enable paging */
 	movl	%cr0, %edx
 	orl	$0x80000000, %edx
 	movl	%edx, %cr0
-	jmp	1f
-1:
-	/*
-	 * 8. Now restore the virtual mode from flat mode by
-	 * adding EIP with PAGE_OFFSET.
-	 */
-	movl	$1f, %edx
-	jmp	*%edx
-1:
-
-	/*
-	 * 9. Balance the stack. And because EAX contain the return value,
-	 * we'd better not clobber it.
-	 */
-	leal	efi_rt_function_ptr, %edx
-	movl	(%edx), %ecx
-	pushl	%ecx
 
-	/*
-	 * 10. Push the saved return address onto the stack and return.
-	 */
-	leal	saved_return_addr, %edx
-	movl	(%edx), %ecx
-	pushl	%ecx
 	ret
 SYM_FUNC_END(efi_call_phys)
-.previous
-
-.data
-saved_return_addr:
-	.long 0
-efi_rt_function_ptr:
-	.long 0
-- 
2.17.1

[PATCH 3/3] efi/x86: simplify mixed mode call wrapper

[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 <ardb@kernel.org>
---
 arch/x86/platform/efi/efi_thunk_64.S | 106 ++++++--------------
 1 file changed, 29 insertions(+), 77 deletions(-)

diff --git a/arch/x86/platform/efi/efi_thunk_64.S b/arch/x86/platform/efi/efi_thunk_64.S
index 3189f1394701..7357808d3ae8 100644
--- a/arch/x86/platform/efi/efi_thunk_64.S
+++ b/arch/x86/platform/efi/efi_thunk_64.S
@@ -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
 
 	/*
 	 * Switch to 1:1 mapped 32-bit stack pointer.
 	 */
-	movq	%rsp, efi_saved_sp(%rip)
+	movq	%rsp, %r11
 	movq	efi_scratch(%rip), %rsp
 
 	/*
@@ -41,87 +47,41 @@ SYM_FUNC_START(efi64_thunk)
 	movq	$__START_KERNEL_map, %rax
 	subq	phys_base(%rip), %rax
 
-	/*
-	 * Push some physical addresses onto the stack. This is easier
-	 * to do now in a code64 section while the assembler can address
-	 * 64-bit values. Note that all the addresses on the stack are
-	 * 32-bit.
-	 */
-	subq	$16, %rsp
-	leaq	efi_exit32(%rip), %rbx
-	subq	%rax, %rbx
-	movl	%ebx, 8(%rsp)
-
-	leaq	__efi64_thunk(%rip), %rbx
-	subq	%rax, %rbx
-	call	*%rbx
-
-	movq	efi_saved_sp(%rip), %rsp
-	pop	%rbx
-	pop	%rbp
-	retq
-SYM_FUNC_END(efi64_thunk)
-
-/*
- * We run this function from the 1:1 mapping.
- *
- * This function must be invoked with a 1:1 mapped stack.
- */
-SYM_FUNC_START_LOCAL(__efi64_thunk)
-	movl	%ds, %eax
-	push	%rax
-	movl	%es, %eax
-	push	%rax
-	movl	%ss, %eax
-	push	%rax
-
-	subq	$32, %rsp
+	subq	$24, %rsp
 	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)
 	leaq	1f(%rip), %rbx
-	movq	%rbx, func_rt_ptr(%rip)
+	subq	%rax, %rbx
+	movl	%ebx, 0x14(%rsp)
 
 	/* Switch to 32-bit descriptor */
 	pushq	$__KERNEL32_CS
-	leaq	efi_enter32(%rip), %rax
-	pushq	%rax
+	leaq	efi_enter32(%rip), %rbx
+	subq	%rax, %rbx
+	pushq	%rbx
 	lretq
 
-1:	addq	$32, %rsp
-
+	/*
+	 * 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
-1:
-	ret
-SYM_FUNC_END(__efi64_thunk)
-
-SYM_FUNC_START_LOCAL(efi_exit32)
-	movq	func_rt_ptr(%rip), %rax
-	push	%rax
-	mov	%rdi, %rax
-	ret
-SYM_FUNC_END(efi_exit32)
+	pop	%rbx
+	pop	%rbp
+	retq
+3:	btsq	$63, %rax
+	jmp	2b
+SYM_FUNC_END(efi64_thunk)
 
 	.code32
 /*
@@ -137,17 +97,9 @@ SYM_FUNC_START_LOCAL(efi_enter32)
 
 	call	*%edi
 
-	/* We must preserve return value */
-	movl	%eax, %edi
-
-	movl	72(%esp), %eax
+	movl	20(%esp), %edi
 	pushl	$__KERNEL_CS
-	pushl	%eax
+	pushl	%edi
 
 	lret
 SYM_FUNC_END(efi_enter32)
-
-	.data
-	.balign	8
-func_rt_ptr:		.quad 0
-efi_saved_sp:		.quad 0
-- 
2.17.1

Re: [PATCH 3/3] efi/x86: simplify mixed mode call wrapper

[Andy Lutomirski]

On Thu, Dec 26, 2019 at 7:16 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> 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.

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.

> diff --git a/arch/x86/platform/efi/efi_thunk_64.S b/arch/x86/platform/efi/efi_thunk_64.S
> index 3189f1394701..7357808d3ae8 100644
> --- a/arch/x86/platform/efi/efi_thunk_64.S
> +++ b/arch/x86/platform/efi/efi_thunk_64.S
> @@ -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?

Re: [PATCH 3/3] efi/x86: simplify mixed mode call wrapper

[Ard Biesheuvel]

On Fri, 27 Dec 2019 at 03:56, Andy Lutomirski <luto@kernel.org> wrote:
>
> On Thu, Dec 26, 2019 at 7:16 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > 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.
>
> 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.
>

I'll put it on the stack instead, just to be safe.

> > diff --git a/arch/x86/platform/efi/efi_thunk_64.S b/arch/x86/platform/efi/efi_thunk_64.S
> > index 3189f1394701..7357808d3ae8 100644
> > --- a/arch/x86/platform/efi/efi_thunk_64.S
> > +++ b/arch/x86/platform/efi/efi_thunk_64.S
> > @@ -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?

Sure.

Re: [PATCH 3/3] efi/x86: simplify mixed mode call wrapper

[Arvind Sankar]

On Thu, Dec 26, 2019 at 04:14:07PM +0100, Ard Biesheuvel wrote:
> 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 <ardb@kernel.org>
> ---
>  arch/x86/platform/efi/efi_thunk_64.S | 106 ++++++--------------
>  1 file changed, 29 insertions(+), 77 deletions(-)
> 
> diff --git a/arch/x86/platform/efi/efi_thunk_64.S b/arch/x86/platform/efi/efi_thunk_64.S
> index 3189f1394701..7357808d3ae8 100644
> --- a/arch/x86/platform/efi/efi_thunk_64.S
> +++ b/arch/x86/platform/efi/efi_thunk_64.S
> +	/*
> +	 * 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
> -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.

Re: [PATCH 3/3] efi/x86: simplify mixed mode call wrapper

[Ard Biesheuvel]

On Fri, 27 Dec 2019 at 05:34, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Thu, Dec 26, 2019 at 04:14:07PM +0100, Ard Biesheuvel wrote:
> > 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 <ardb@kernel.org>
> > ---
> >  arch/x86/platform/efi/efi_thunk_64.S | 106 ++++++--------------
> >  1 file changed, 29 insertions(+), 77 deletions(-)
> >
> > diff --git a/arch/x86/platform/efi/efi_thunk_64.S b/arch/x86/platform/efi/efi_thunk_64.S
> > index 3189f1394701..7357808d3ae8 100644
> > --- a/arch/x86/platform/efi/efi_thunk_64.S
> > +++ b/arch/x86/platform/efi/efi_thunk_64.S
> > +     /*
> > +      * 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
> > -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?
>

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.

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

Good point. I'll put that on my todo list.

Thanks,
Ard.

Re: [PATCH 3/3] efi/x86: simplify mixed mode call wrapper

[Arvind Sankar]

On Fri, Dec 27, 2019 at 09:05:46AM +0100, Ard Biesheuvel wrote:
> 
> 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.
> 

Oh sorry, I misread the table in the spec and thought the warnings have
high nibble set to 0x1.