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

[Alexander Graf]

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.

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.


Alex

> 
> 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.
> 
> 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
> 
> Best regards
> 
> Heinrich
> 
>>
>> 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.


Thanks,

Alex

>> +	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);
>>  
>>
>