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.gori...@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 <h...@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); > >> > >> > > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot