[PATCH 0/5] efi/x86 cleanups and one bugfix

[PATCH 0/5] efi/x86 cleanups and one bugfix

[Arvind Sankar]

First 3 patches are misc. beautifications to the new compat PE entry
code.

Next patch stops EFI stub using code32_start field to communicate the
address of startup_32, instead returning it directly to efi_stub_entry.

Last patch is a bugfix for x86/boot/head code to use unsigned
comparisons on addresses rather than signed.

Based on tip:efi/core

Arvind Sankar (5):
  efi/x86: Annotate the LOADED_IMAGE_PROTOCOL_GUID with SYM_DATA
  efi/x86: Respect 32-bit ABI in efi32_pe_entry
  efi/x86: Make efi32_pe_entry more readable
  efi/x86: Avoid using code32_start
  x86/boot: Use unsigned comparison for addresses

 arch/x86/boot/compressed/head_32.S      |  5 +-
 arch/x86/boot/compressed/head_64.S      | 70 ++++++++++++++++++-------
 arch/x86/kernel/asm-offsets.c           |  1 -
 drivers/firmware/efi/libstub/x86-stub.c | 10 ++--
 4 files changed, 57 insertions(+), 29 deletions(-)

-- 
2.24.1

[PATCH 1/5] efi/x86: Annotate the LOADED_IMAGE_PROTOCOL_GUID with SYM_DATA

[Arvind Sankar]

Use SYM_DATA* macro to annotate this constant, and explicitly align it
to 4-byte boundary. Use lower-case for hexadecimal data.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 arch/x86/boot/compressed/head_64.S | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index fcf8feaa57ea..8105e8348607 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -672,7 +672,7 @@ SYM_FUNC_START(efi32_pe_entry)
 	/* Get the loaded image protocol pointer from the image handle */
 	subl	$12, %esp			// space for the loaded image pointer
 	pushl	%esp				// pass its address
-	leal	4f(%ebp), %eax
+	leal	loaded_image_proto(%ebp), %eax
 	pushl	%eax				// pass the GUID address
 	pushl	28(%esp)			// pass the image handle
 
@@ -695,9 +695,12 @@ SYM_FUNC_END(efi32_pe_entry)
 
 	.section ".rodata"
 	/* EFI loaded image protocol GUID */
-4:	.long	0x5B1B31A1
+	.balign 4
+SYM_DATA_START_LOCAL(loaded_image_proto)
+	.long	0x5b1b31a1
 	.word	0x9562, 0x11d2
-	.byte	0x8E, 0x3F, 0x00, 0xA0, 0xC9, 0x69, 0x72, 0x3B
+	.byte	0x8e, 0x3f, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b
+SYM_DATA_END(loaded_image_proto)
 #endif
 
 /*
-- 
2.24.1

[PATCH 2/5] efi/x86: Respect 32-bit ABI in efi32_pe_entry

[Arvind Sankar]

verify_cpu clobbers BX and DI. In case we have to return error, we need
to preserve them to respect 32-bit calling convention.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 arch/x86/boot/compressed/head_64.S | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index 8105e8348607..920daf62dac2 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -660,7 +660,11 @@ SYM_DATA(efi_is64, .byte 1)
 SYM_FUNC_START(efi32_pe_entry)
 	pushl	%ebp
 
+	pushl	%ebx
+	pushl	%edi
 	call	verify_cpu			// check for long mode support
+	popl	%edi
+	popl	%ebx
 	testl	%eax, %eax
 	movl	$0x80000003, %eax		// EFI_UNSUPPORTED
 	jnz	3f
-- 
2.24.1

[PATCH 3/5] efi/x86: Make efi32_pe_entry more readable

[Arvind Sankar]

Setup a proper frame pointer in efi32_pe_entry so that it's easier to
calculate offsets for arguments.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 arch/x86/boot/compressed/head_64.S | 57 +++++++++++++++++++++---------
 1 file changed, 40 insertions(+), 17 deletions(-)

diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index 920daf62dac2..fabbd4c2e9f2 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -658,42 +658,65 @@ SYM_DATA(efi_is64, .byte 1)
 	.text
 	.code32
 SYM_FUNC_START(efi32_pe_entry)
+/*
+ * efi_status_t efi32_pe_entry(efi_handle_t image_handle,
+ *			       efi_system_table_32_t *sys_table)
+ */
+
 	pushl	%ebp
+	movl	%esp, %ebp
+	pushl	%eax				// dummy push to allocate loaded_image
 
-	pushl	%ebx
+	pushl	%ebx				// save callee-save registers
 	pushl	%edi
+
 	call	verify_cpu			// check for long mode support
-	popl	%edi
-	popl	%ebx
 	testl	%eax, %eax
 	movl	$0x80000003, %eax		// EFI_UNSUPPORTED
-	jnz	3f
+	jnz	2f
 
 	call	1f
-1:	pop	%ebp
-	subl	$1b, %ebp
+1:	pop	%ebx
+	subl	$1b, %ebx
 
 	/* Get the loaded image protocol pointer from the image handle */
-	subl	$12, %esp			// space for the loaded image pointer
-	pushl	%esp				// pass its address
-	leal	loaded_image_proto(%ebp), %eax
+	leal	-4(%ebp), %eax
+	pushl	%eax				// &loaded_image
+	leal	loaded_image_proto(%ebx), %eax
 	pushl	%eax				// pass the GUID address
-	pushl	28(%esp)			// pass the image handle
+	pushl	8(%ebp)				// pass the image handle
 
-	movl	36(%esp), %eax			// sys_table
+	/*
+	 * Note the alignment of the stack frame.
+	 *   sys_table
+	 *   handle             <-- 16-byte aligned on entry by ABI
+	 *   return address
+	 *   frame pointer
+	 *   loaded_image       <-- local variable
+	 *   saved %ebx		<-- 16-byte aligned here
+	 *   saved %edi
+	 *   &loaded_image
+	 *   &loaded_image_proto
+	 *   handle             <-- 16-byte aligned for call to handle_protocol
+	 */
+
+	movl	12(%ebp), %eax			// sys_table
 	movl	ST32_boottime(%eax), %eax	// sys_table->boottime
 	call	*BS32_handle_protocol(%eax)	// sys_table->boottime->handle_protocol
-	cmp	$0, %eax
+	addl	$12, %esp			// restore argument space
+	testl	%eax, %eax
 	jnz	2f
 
-	movl	32(%esp), %ecx			// image_handle
-	movl	36(%esp), %edx			// sys_table
-	movl	12(%esp), %esi			// loaded_image
+	movl	8(%ebp), %ecx			// image_handle
+	movl	12(%ebp), %edx			// sys_table
+	movl	-4(%ebp), %esi			// loaded_image
 	movl	LI32_image_base(%esi), %esi	// loaded_image->image_base
+	movl	%ebx, %ebp			// startup_32 for efi32_pe_stub_entry
 	jmp	efi32_pe_stub_entry
 
-2:	addl	$24, %esp
-3:	popl	%ebp
+2:	popl	%edi				// restore callee-save registers
+	popl	%ebx
+	leave
 	ret
 SYM_FUNC_END(efi32_pe_entry)
 
-- 
2.24.1

[PATCH 4/5] efi/x86: Avoid using code32_start

[Arvind Sankar]

code32_start is meant for 16-bit real-mode bootloaders to inform the
kernel where the 32-bit protected mode code starts. Nothing in the
protected mode kernel except the EFI stub uses it.

efi_main currently returns boot_params, with code32_start set inside it
to tell efi_stub_entry where startup_32 is located. Since it was invoked
by efi_stub_entry in the first place, boot_params is already known.
Return the address of startup_32 instead.

This will allow a 64-bit kernel to live above 4Gb, for example, and it's
cleaner.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 arch/x86/boot/compressed/head_32.S      |  3 +--
 arch/x86/boot/compressed/head_64.S      |  4 ++--
 arch/x86/kernel/asm-offsets.c           |  1 -
 drivers/firmware/efi/libstub/x86-stub.c | 10 +++++-----
 4 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/arch/x86/boot/compressed/head_32.S b/arch/x86/boot/compressed/head_32.S
index 2f8138b71ea9..e013bdc1237b 100644
--- a/arch/x86/boot/compressed/head_32.S
+++ b/arch/x86/boot/compressed/head_32.S
@@ -156,9 +156,8 @@ SYM_FUNC_END(startup_32)
 SYM_FUNC_START(efi32_stub_entry)
 SYM_FUNC_START_ALIAS(efi_stub_entry)
 	add	$0x4, %esp
+	movl	8(%esp), %esi	/* save boot_params pointer */
 	call	efi_main
-	movl	%eax, %esi
-	movl	BP_code32_start(%esi), %eax
 	leal	startup_32(%eax), %eax
 	jmp	*%eax
 SYM_FUNC_END(efi32_stub_entry)
diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index fabbd4c2e9f2..6a4ff919008c 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -472,9 +472,9 @@ SYM_CODE_END(startup_64)
 SYM_FUNC_START(efi64_stub_entry)
 SYM_FUNC_START_ALIAS(efi_stub_entry)
 	and	$~0xf, %rsp			/* realign the stack */
+	movq	%rdx, %rbx			/* save boot_params pointer */
 	call	efi_main
-	movq	%rax,%rsi
-	movl	BP_code32_start(%esi), %eax
+	movq	%rbx,%rsi
 	leaq	startup_64(%rax), %rax
 	jmp	*%rax
 SYM_FUNC_END(efi64_stub_entry)
diff --git a/arch/x86/kernel/asm-offsets.c b/arch/x86/kernel/asm-offsets.c
index 5c7ee3df4d0b..3ca07ad552ae 100644
--- a/arch/x86/kernel/asm-offsets.c
+++ b/arch/x86/kernel/asm-offsets.c
@@ -88,7 +88,6 @@ static void __used common(void)
 	OFFSET(BP_kernel_alignment, boot_params, hdr.kernel_alignment);
 	OFFSET(BP_init_size, boot_params, hdr.init_size);
 	OFFSET(BP_pref_address, boot_params, hdr.pref_address);
-	OFFSET(BP_code32_start, boot_params, hdr.code32_start);
 
 	BLANK();
 	DEFINE(PTREGS_SIZE, sizeof(struct pt_regs));
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index 9db98839d7b4..7f3e97c2aad3 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -703,10 +703,11 @@ static efi_status_t exit_boot(struct boot_params *boot_params, void *handle)
 }
 
 /*
- * On success we return a pointer to a boot_params structure, and NULL
- * on failure.
+ * On success, we return the address of startup_32, which has potentially been
+ * relocated by efi_relocate_kernel.
+ * On failure, we exit to the firmware via efi_exit instead of returning.
  */
-struct boot_params *efi_main(efi_handle_t handle,
+unsigned long efi_main(efi_handle_t handle,
 			     efi_system_table_t *sys_table_arg,
 			     struct boot_params *boot_params)
 {
@@ -736,7 +737,6 @@ struct boot_params *efi_main(efi_handle_t handle,
 			goto fail;
 		}
 	}
-	hdr->code32_start = (u32)bzimage_addr;
 
 	/*
 	 * efi_pe_entry() may have been called before efi_main(), in which
@@ -799,7 +799,7 @@ struct boot_params *efi_main(efi_handle_t handle,
 		goto fail;
 	}
 
-	return boot_params;
+	return bzimage_addr;
 fail:
 	efi_printk("efi_main() failed!\n");
 
-- 
2.24.1

[PATCH 5/5] x86/boot: Use unsigned comparison for addresses

[Arvind Sankar]

The load address is compared with LOAD_PHYSICAL_ADDR using a signed
comparison currently (using jge instruction).

When loading a 64-bit kernel using the new efi32_pe_entry point added by
commit 97aa276579b2 ("efi/x86: Add true mixed mode entry point into
.compat section") using qemu with -m 3072, the firmware actually loads
us above 2Gb, resulting in a very early crash.

Use jae instruction to perform unsigned comparison instead, as physical
addresses should be considered as unsigned.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 arch/x86/boot/compressed/head_32.S | 2 +-
 arch/x86/boot/compressed/head_64.S | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/boot/compressed/head_32.S b/arch/x86/boot/compressed/head_32.S
index e013bdc1237b..46bbe7ab4adf 100644
--- a/arch/x86/boot/compressed/head_32.S
+++ b/arch/x86/boot/compressed/head_32.S
@@ -105,7 +105,7 @@ SYM_FUNC_START(startup_32)
 	notl	%eax
 	andl    %eax, %ebx
 	cmpl	$LOAD_PHYSICAL_ADDR, %ebx
-	jge	1f
+	jae	1f
 #endif
 	movl	$LOAD_PHYSICAL_ADDR, %ebx
 1:
diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index 6a4ff919008c..5d8338a693ce 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -105,7 +105,7 @@ SYM_FUNC_START(startup_32)
 	notl	%eax
 	andl	%eax, %ebx
 	cmpl	$LOAD_PHYSICAL_ADDR, %ebx
-	jge	1f
+	jae	1f
 #endif
 	movl	$LOAD_PHYSICAL_ADDR, %ebx
 1:
@@ -305,7 +305,7 @@ SYM_CODE_START(startup_64)
 	notq	%rax
 	andq	%rax, %rbp
 	cmpq	$LOAD_PHYSICAL_ADDR, %rbp
-	jge	1f
+	jae	1f
 #endif
 	movq	$LOAD_PHYSICAL_ADDR, %rbp
 1:
-- 
2.24.1

Re: [PATCH 3/5] efi/x86: Make efi32_pe_entry more readable

[Ard Biesheuvel]

On Mon, 2 Mar 2020 at 00:04, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> Setup a proper frame pointer in efi32_pe_entry so that it's easier to
> calculate offsets for arguments.
>
> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> ---
>  arch/x86/boot/compressed/head_64.S | 57 +++++++++++++++++++++---------
>  1 file changed, 40 insertions(+), 17 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
> index 920daf62dac2..fabbd4c2e9f2 100644
> --- a/arch/x86/boot/compressed/head_64.S
> +++ b/arch/x86/boot/compressed/head_64.S
> @@ -658,42 +658,65 @@ SYM_DATA(efi_is64, .byte 1)
>         .text
>         .code32
>  SYM_FUNC_START(efi32_pe_entry)
> +/*
> + * efi_status_t efi32_pe_entry(efi_handle_t image_handle,
> + *                            efi_system_table_32_t *sys_table)
> + */
> +
>         pushl   %ebp
> +       movl    %esp, %ebp
> +       pushl   %eax                            // dummy push to allocate loaded_image
>
> -       pushl   %ebx
> +       pushl   %ebx                            // save callee-save registers
>         pushl   %edi
> +
>         call    verify_cpu                      // check for long mode support
> -       popl    %edi
> -       popl    %ebx
>         testl   %eax, %eax
>         movl    $0x80000003, %eax               // EFI_UNSUPPORTED
> -       jnz     3f
> +       jnz     2f
>
>         call    1f
> -1:     pop     %ebp
> -       subl    $1b, %ebp
> +1:     pop     %ebx
> +       subl    $1b, %ebx
>
>         /* Get the loaded image protocol pointer from the image handle */
> -       subl    $12, %esp                       // space for the loaded image pointer
> -       pushl   %esp                            // pass its address
> -       leal    loaded_image_proto(%ebp), %eax
> +       leal    -4(%ebp), %eax
> +       pushl   %eax                            // &loaded_image
> +       leal    loaded_image_proto(%ebx), %eax
>         pushl   %eax                            // pass the GUID address
> -       pushl   28(%esp)                        // pass the image handle
> +       pushl   8(%ebp)                         // pass the image handle
>
> -       movl    36(%esp), %eax                  // sys_table
> +       /*
> +        * Note the alignment of the stack frame.
> +        *   sys_table
> +        *   handle             <-- 16-byte aligned on entry by ABI
> +        *   return address
> +        *   frame pointer
> +        *   loaded_image       <-- local variable
> +        *   saved %ebx         <-- 16-byte aligned here
> +        *   saved %edi
> +        *   &loaded_image
> +        *   &loaded_image_proto
> +        *   handle             <-- 16-byte aligned for call to handle_protocol
> +        */
> +
> +       movl    12(%ebp), %eax                  // sys_table
>         movl    ST32_boottime(%eax), %eax       // sys_table->boottime
>         call    *BS32_handle_protocol(%eax)     // sys_table->boottime->handle_protocol
> -       cmp     $0, %eax
> +       addl    $12, %esp                       // restore argument space
> +       testl   %eax, %eax
>         jnz     2f
>
> -       movl    32(%esp), %ecx                  // image_handle
> -       movl    36(%esp), %edx                  // sys_table
> -       movl    12(%esp), %esi                  // loaded_image
> +       movl    8(%ebp), %ecx                   // image_handle
> +       movl    12(%ebp), %edx                  // sys_table
> +       movl    -4(%ebp), %esi                  // loaded_image
>         movl    LI32_image_base(%esi), %esi     // loaded_image->image_base
> +       movl    %ebx, %ebp                      // startup_32 for efi32_pe_stub_entry

The code that follows efi32_pe_stub_entry still expects the runtime
displacement in %ebp, so we'll need to pass that in another way here.

>         jmp     efi32_pe_stub_entry
>
> -2:     addl    $24, %esp
> -3:     popl    %ebp
> +2:     popl    %edi                            // restore callee-save registers
> +       popl    %ebx
> +       leave
>         ret
>  SYM_FUNC_END(efi32_pe_entry)
>
> --
> 2.24.1
>

Re: [PATCH 0/5] efi/x86 cleanups and one bugfix

[Ard Biesheuvel]

On Mon, 2 Mar 2020 at 00:04, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> First 3 patches are misc. beautifications to the new compat PE entry
> code.
>
> Next patch stops EFI stub using code32_start field to communicate the
> address of startup_32, instead returning it directly to efi_stub_entry.
>
> Last patch is a bugfix for x86/boot/head code to use unsigned
> comparisons on addresses rather than signed.
>
> Based on tip:efi/core
>
> Arvind Sankar (5):
>   efi/x86: Annotate the LOADED_IMAGE_PROTOCOL_GUID with SYM_DATA
>   efi/x86: Respect 32-bit ABI in efi32_pe_entry
>   efi/x86: Make efi32_pe_entry more readable
>   efi/x86: Avoid using code32_start
>   x86/boot: Use unsigned comparison for addresses
>

Thanks Arvind. This looks really good, as usual. I had one question,
the rest looks good to go.

Re: [PATCH 3/5] efi/x86: Make efi32_pe_entry more readable

[Arvind Sankar]

On Mon, Mar 02, 2020 at 08:49:17AM +0100, Ard Biesheuvel wrote:
> On Mon, 2 Mar 2020 at 00:04, Arvind Sankar <nivedita@alum.mit.edu> wrote:
...
> >         call    1f
> > -1:     pop     %ebp
> > -       subl    $1b, %ebp
> > +1:     pop     %ebx
> > +       subl    $1b, %ebx
...
> >
> > +       movl    %ebx, %ebp                      // startup_32 for efi32_pe_stub_entry
> 
> The code that follows efi32_pe_stub_entry still expects the runtime
> displacement in %ebp, so we'll need to pass that in another way here.
> 
> >         jmp     efi32_pe_stub_entry

Didn't follow -- what do you mean by runtime displacement?

efi32_pe_stub_entry expects the runtime address of startup_32 to be in
%ebp, but with the changes for keeping the frame pointer in %ebp, I
changed the runtime address to be in %ebx instead. Hence I added that
movl %ebx, %ebp to put it in %ebp just before calling efi32_pe_stub_entry.
That should be fine, no?

Re: [PATCH 3/5] efi/x86: Make efi32_pe_entry more readable

[Ard Biesheuvel]

On Mon, 2 Mar 2020 at 17:54, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Mon, Mar 02, 2020 at 08:49:17AM +0100, Ard Biesheuvel wrote:
> > On Mon, 2 Mar 2020 at 00:04, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> ...
> > >         call    1f
> > > -1:     pop     %ebp
> > > -       subl    $1b, %ebp
> > > +1:     pop     %ebx
> > > +       subl    $1b, %ebx
> ...
> > >
> > > +       movl    %ebx, %ebp                      // startup_32 for efi32_pe_stub_entry
> >
> > The code that follows efi32_pe_stub_entry still expects the runtime
> > displacement in %ebp, so we'll need to pass that in another way here.
> >
> > >         jmp     efi32_pe_stub_entry
>
> Didn't follow -- what do you mean by runtime displacement?
>
> efi32_pe_stub_entry expects the runtime address of startup_32 to be in
> %ebp, but with the changes for keeping the frame pointer in %ebp, I
> changed the runtime address to be in %ebx instead. Hence I added that
> movl %ebx, %ebp to put it in %ebp just before calling efi32_pe_stub_entry.
> That should be fine, no?

But how does that work with:

SYM_INNER_LABEL(efi32_pe_stub_entry, SYM_L_LOCAL)
    movl %ecx, efi32_boot_args(%ebp)
    movl %edx, efi32_boot_args+4(%ebp)
    movb $0, efi_is64(%ebp)


?

Re: [PATCH 3/5] efi/x86: Make efi32_pe_entry more readable

[Arvind Sankar]

On Mon, Mar 02, 2020 at 05:57:04PM +0100, Ard Biesheuvel wrote:
> On Mon, 2 Mar 2020 at 17:54, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > On Mon, Mar 02, 2020 at 08:49:17AM +0100, Ard Biesheuvel wrote:
> > > On Mon, 2 Mar 2020 at 00:04, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > ...
> > > >         call    1f
> > > > -1:     pop     %ebp
> > > > -       subl    $1b, %ebp
> > > > +1:     pop     %ebx
> > > > +       subl    $1b, %ebx
> > ...
> > > >
> > > > +       movl    %ebx, %ebp                      // startup_32 for efi32_pe_stub_entry
> > >
> > > The code that follows efi32_pe_stub_entry still expects the runtime
> > > displacement in %ebp, so we'll need to pass that in another way here.
> > >
> > > >         jmp     efi32_pe_stub_entry
> >
> > Didn't follow -- what do you mean by runtime displacement?
> >
> > efi32_pe_stub_entry expects the runtime address of startup_32 to be in
> > %ebp, but with the changes for keeping the frame pointer in %ebp, I
> > changed the runtime address to be in %ebx instead. Hence I added that
> > movl %ebx, %ebp to put it in %ebp just before calling efi32_pe_stub_entry.
> > That should be fine, no?
> 
> But how does that work with:
> 
> SYM_INNER_LABEL(efi32_pe_stub_entry, SYM_L_LOCAL)
>     movl %ecx, efi32_boot_args(%ebp)
>     movl %edx, efi32_boot_args+4(%ebp)
>     movb $0, efi_is64(%ebp)
> 
> 
> ?

Why wouldn't it work? Before this change, efi32_pe_entry set %ebp to
startup_32 (via the call/pop/sub sequence), so efi32_pe_stub_entry was
entered with %ebp == startup_32.

After this change, the call/pop/sub sequence puts startup_32 into %ebx,
and then I copy it into %ebp just before branching to efi32_pe_stub_entry.
So everything should continue to work the same way as before?

Re: [PATCH 3/5] efi/x86: Make efi32_pe_entry more readable

[Ard Biesheuvel]

On Mon, 2 Mar 2020 at 18:02, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Mon, Mar 02, 2020 at 05:57:04PM +0100, Ard Biesheuvel wrote:
> > On Mon, 2 Mar 2020 at 17:54, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > >
> > > On Mon, Mar 02, 2020 at 08:49:17AM +0100, Ard Biesheuvel wrote:
> > > > On Mon, 2 Mar 2020 at 00:04, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > ...
> > > > >         call    1f
> > > > > -1:     pop     %ebp
> > > > > -       subl    $1b, %ebp
> > > > > +1:     pop     %ebx
> > > > > +       subl    $1b, %ebx
> > > ...
> > > > >
> > > > > +       movl    %ebx, %ebp                      // startup_32 for efi32_pe_stub_entry
> > > >
> > > > The code that follows efi32_pe_stub_entry still expects the runtime
> > > > displacement in %ebp, so we'll need to pass that in another way here.
> > > >
> > > > >         jmp     efi32_pe_stub_entry
> > >
> > > Didn't follow -- what do you mean by runtime displacement?
> > >
> > > efi32_pe_stub_entry expects the runtime address of startup_32 to be in
> > > %ebp, but with the changes for keeping the frame pointer in %ebp, I
> > > changed the runtime address to be in %ebx instead. Hence I added that
> > > movl %ebx, %ebp to put it in %ebp just before calling efi32_pe_stub_entry.
> > > That should be fine, no?
> >
> > But how does that work with:
> >
> > SYM_INNER_LABEL(efi32_pe_stub_entry, SYM_L_LOCAL)
> >     movl %ecx, efi32_boot_args(%ebp)
> >     movl %edx, efi32_boot_args+4(%ebp)
> >     movb $0, efi_is64(%ebp)
> >
> >
> > ?
>
> Why wouldn't it work? Before this change, efi32_pe_entry set %ebp to
> startup_32 (via the call/pop/sub sequence), so efi32_pe_stub_entry was
> entered with %ebp == startup_32.
>
> After this change, the call/pop/sub sequence puts startup_32 into %ebx,
> and then I copy it into %ebp just before branching to efi32_pe_stub_entry.
> So everything should continue to work the same way as before?

Ah ok, so the sequence

   call 1f
1: pop %ebp
   subl $1b, %ebp

happens to produce the value of startup_32, which is obvious now that
I think of it, but it wasn't before.