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

[Alexander Graf]

On 05.06.18 19:56, Ivan Gorinov wrote:
> 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/

The ENTRY/ENDPROC macros are architecture agnostic, so you should just
be able to use them exactly like in the arm example above.


Alex