[U-Boot] [PATCH] x86: Add 64-bit setjmp/longjmp implementation

Tue Jun 5 17:56:38 UTC 2018

On Sun, Jun 03, 2018 at 02:06:47PM +0200, Alexander Graf wrote:
> On 02.06.18 20:44, Heinrich Schuchardt wrote:
> > On 05/31/2018 12:50 AM, Ivan Gorinov wrote:
> >> Add setjmp/longjmp functions for x86_64,
> >> based on existing 32-bit implementation.
> > 
> > Thank you for your patch. This addresses a gap that really should be closed.
> > 
> > Please, add a line to the commit message indicating the calling
> > conventions that are supported. This is important because we use
> > different calling conventions in different places.
> > 
> > One function where we need setjmp() is efi_start_image(). The function
> > is defined as EFIAPI. So we need an implementation supporting the ms_abi
> > calling convention.
> 
> I don't quite follow. The calling convention is declared in the C header
> (omitted means sysv, EFIAPI means ms). The compiler will adapt the call
> to the respective convention automatically. So if you call a sysv
> function from an EFIAPI function, gcc will push all registers that are
> potentially getting clobbered on the stack.

I can confirm that gcc does save %rsi and %rdi in EFIAPI functions before
using those two registers for setjmp() parameters. The other callee-saved
registers in ms_abi are also callee-saved in sysv and correctly handled by
the proposed implementation.

static efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
					   unsigned long *exit_data_size,
					   s16 **exit_data)
{
    5f03ce56:	57                   	push   %rdi
    5f03ce57:	56                   	push   %rsi

    ...

	/* call the image! */
	if (setjmp(&info->exit_jmp)) {
    5f03cecd:	48 8b 84 24 f0 00 00 	mov    0xf0(%rsp),%rax
    5f03ced4:	00 
    5f03ced5:	48 8d 78 78          	lea    0x78(%rax),%rdi
    5f03ced9:	e8 82 32 fc ff       	callq  5f000160 <setjmp>
    5f03cede:	85 c0                	test   %eax,%eax
    5f03cee0:	74 1b                	je     5f03cefd <efi_start_image+0xa7>

    ...

> So IIUC we really only need the sysv variant - which this patch
> implements, right? This also shouldn't need any mentioning, as it's the
> default throughout the code base, unless the function is annotated with
> the EFIAPI tag.

> > Another function where we use setjmp() is do_bootefi_exec() but this
> > function is defined as sysv_abi.
> > 
> > I guess the patch relates to the sysv_abi calling convention:
> > 
> > "System V Application Binary Interface: AMD64 Architecture Processor
> > Supplement (With LP64 and ILP32 Programming Models)"
> > https://software.intel.com/sites/default/files/article/402129/mpx-linux64-abi.pdf
> > 
> > This document contains the following sentence:
> > 
> > The control bits of the MXCSR register are callee-saved  (preserved
> > across calls), while the status bits are caller-saved (not preserved).
> > I cannot see that this is reflected in the patch.

Thank you for the link! I will add MXCSR to the callee-saved register set.

> > See also https://msdn.microsoft.com/en-us/library/36d3b75w.aspx
> > 
> > Your setjmp implemantion is not usable for the ms_abi that we need in
> > for efi_start_image(). Here you would have to save registers RBX, RBP,
> > RDI, RSI, RSP, R12, R13, R14, and R15.
> > 
> > As reference have a look at this implementation:
> > https://github.com/freebsd/freebsd/blob/master/lib/libc/amd64/gen/setjmp.S
> > 
> >>
> >> Signed-off-by: Ivan Gorinov <ivan.gorinov at intel.com>
> > 
> > 
> > 
> >> ---
> >>  arch/x86/cpu/x86_64/setjmp.S  | 56 +++++++++++++++++++++++++++++++++++++++++++
> >>  arch/x86/cpu/x86_64/setjmp.c  | 19 ---------------
> >>  arch/x86/include/asm/setjmp.h | 17 +++++++++++++
> >>  3 files changed, 73 insertions(+), 19 deletions(-)
> >>  create mode 100644 arch/x86/cpu/x86_64/setjmp.S
> >>  delete mode 100644 arch/x86/cpu/x86_64/setjmp.c
> >>
> >> diff --git a/arch/x86/cpu/x86_64/setjmp.S b/arch/x86/cpu/x86_64/setjmp.S
> >> new file mode 100644
> >> index 0000000..e10ee49
> >> --- /dev/null
> >> +++ b/arch/x86/cpu/x86_64/setjmp.S
> >> @@ -0,0 +1,56 @@
> >> +/*
> >> + * Based on arch/x86/cpu/i386/setjmp.S by H. Peter Anvin <hpa at zytor.com>
> >> + *
> >> + * SPDX-License-Identifier:	GPL-2.0
> >> + */
> >> +
> >> +/*
> >> + * The jmp_buf is assumed to contain the following, in order:
> >> + *	<return address>
> >> + *	%rsp
> >> + *	%rbp
> >> + *	%rbx
> >> + *	%r12
> >> + *	%r13
> >> + *	%r14
> >> + *	%r15
> >> + */
> >> +
> >> +.text
> >> +.align 8
> >> +.globl	setjmp
> >> +.type setjmp, @function
> >> +	
> >> +setjmp:
> 
> We have macros for asm provided functions (see arch/arm/lib/setjmp.S) to
> ensure that linker based section elimination works. Please make sure you
> use those.

OK. Shall I also add .pushsection/.popsection?
Can't find any example in arch/x86/

> >> +	pop	%rcx
> >> +	movq	%rcx, (%rdi)	/* Return address */
> >> +	movq	%rsp, 8 (%rdi)	/* Post-return %rsp! */
> >> +	movq	%rbp, 16 (%rdi)
> >> +	movq	%rbx, 24 (%rdi)
> >> +	movq	%r12, 32 (%rdi)
> >> +	movq	%r13, 40 (%rdi)
> >> +	movq	%r14, 48 (%rdi)
> >> +	movq	%r15, 56 (%rdi)
> >> +	xorq	%rax, %rax	/* Return value */
> >> +	jmpq	*%rcx
> >> +
> >> +/* Provide function size if needed */
> >> +.size setjmp, .-setjmp
> >> +
> >> +.align 8
> >> +.globl longjmp
> >> +.type longjmp, @function
> >> +
> >> +longjmp:
> >> +	movq	32 (%rdi), %r12
> >> +	movq	40 (%rdi), %r13
> >> +	movq	48 (%rdi), %r14
> >> +	movq	56 (%rdi), %r15
> >> +	movq	16 (%rdi), %rbp
> >> +	movq	24 (%rdi), %rbx
> >> +	movq	8 (%rdi), %rsp
> >> +	movq	(%rdi), %rcx
> >> +	movq	%rsi, %rdi
> >> +	jmpq	*%rcx
> >> +
> >> +	.size longjmp, .-longjmp
> >> diff --git a/arch/x86/cpu/x86_64/setjmp.c b/arch/x86/cpu/x86_64/setjmp.c
> >> deleted file mode 100644
> >> index 5d4a74a..0000000
> >> --- a/arch/x86/cpu/x86_64/setjmp.c
> >> +++ /dev/null
> >> @@ -1,19 +0,0 @@
> >> -// SPDX-License-Identifier: GPL-2.0+
> >> -/*
> >> - * Copyright (c) 2016 Google, Inc
> >> - */
> >> -
> >> -#include <common.h>
> >> -#include <asm/setjmp.h>
> >> -
> >> -int setjmp(struct jmp_buf_data *jmp_buf)
> >> -{
> >> -	printf("WARNING: setjmp() is not supported\n");
> >> -
> >> -	return 0;
> >> -}
> >> -
> >> -void longjmp(struct jmp_buf_data *jmp_buf, int val)
> >> -{
> >> -	printf("WARNING: longjmp() is not supported\n");
> >> -}
> >> diff --git a/arch/x86/include/asm/setjmp.h b/arch/x86/include/asm/setjmp.h
> >> index f25975f..49c36c1 100644
> >> --- a/arch/x86/include/asm/setjmp.h
> >> +++ b/arch/x86/include/asm/setjmp.h
> >> @@ -8,6 +8,21 @@
> >>  #ifndef __setjmp_h
> >>  #define __setjmp_h
> >>  
> >> +#ifdef CONFIG_X86_64
> >> +
> >> +struct jmp_buf_data {
> >> +	unsigned long __rip;
> >> +	unsigned long __rsp;
> >> +	unsigned long __rbp;
> >> +	unsigned long __rbx;
> >> +	unsigned long __r12;
> >> +	unsigned long __r13;
> >> +	unsigned long __r14;
> >> +	unsigned long __r15;
> >> +};
> >> +
> >> +#else
> >> +
> >>  struct jmp_buf_data {
> >>  	unsigned int __ebx;
> >>  	unsigned int __esp;
> >> @@ -17,6 +32,8 @@ struct jmp_buf_data {
> >>  	unsigned int __eip;
> >>  };
> >>  
> >> +#endif
> >> +
> >>  int setjmp(struct jmp_buf_data *jmp_buf);
> >>  void longjmp(struct jmp_buf_data *jmp_buf, int val);
> >>  
> >>
> > 