[U-Boot] [PATCH 1/6] x86: Add implementations of setjmp() and longjmp()

[Bin Meng]

Hi Simon,

On Wed, Aug 10, 2016 at 2:16 AM, Simon Glass <sjg at chromium.org> wrote:
> Hi Bin,
>
> On 9 August 2016 at 00:49, Bin Meng <bmeng.cn at gmail.com> wrote:
>> Hi Simon,
>>
>> On Sun, Aug 7, 2016 at 7:23 AM, Simon Glass <sjg at chromium.org> wrote:
>>> Bring in these functions from Linux v4.4. They will be needed for EFI loader
>>> support.
>>>
>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>> ---
>>>
>>>  arch/x86/cpu/Makefile         |  2 +-
>>>  arch/x86/cpu/setjmp.S         | 71 +++++++++++++++++++++++++++++++++++++++++++
>>>  arch/x86/include/asm/setjmp.h | 24 +++++++++++++++
>>>  3 files changed, 96 insertions(+), 1 deletion(-)
>>>  create mode 100644 arch/x86/cpu/setjmp.S
>>>  create mode 100644 arch/x86/include/asm/setjmp.h
>>>
>>> diff --git a/arch/x86/cpu/Makefile b/arch/x86/cpu/Makefile
>>> index 2667e0b..f5b8c9e 100644
>>> --- a/arch/x86/cpu/Makefile
>>> +++ b/arch/x86/cpu/Makefile
>>> @@ -10,7 +10,7 @@
>>>
>>>  extra-y        = start.o
>>>  obj-$(CONFIG_X86_RESET_VECTOR) += resetvec.o start16.o
>>> -obj-y  += interrupts.o cpu.o cpu_x86.o call64.o
>>> +obj-y  += interrupts.o cpu.o cpu_x86.o call64.o setjmp.o
>>>
>>>  AFLAGS_REMOVE_call32.o := -mregparm=3 \
>>>         $(if $(CONFIG_EFI_STUB_64BIT),-march=i386 -m32)
>>> diff --git a/arch/x86/cpu/setjmp.S b/arch/x86/cpu/setjmp.S
>>> new file mode 100644
>>> index 0000000..24e7d8a
>>> --- /dev/null
>>> +++ b/arch/x86/cpu/setjmp.S
>>> @@ -0,0 +1,71 @@
>>> +/*
>>> + * Written by H. Peter Anvin <hpa at zytor.com>
>>> + * Brought in from Linux v4.4 and modified for U-Boot
>>> + * From Linux arch/um/sys-i386/setjmp.S
>>> + *
>>> + * SPDX-License-Identifier:    GPL-2.0
>>> + */
>>> +
>>> +#include <asm/global_data.h>
>>> +#include <asm/msr-index.h>
>>> +#include <asm/processor-flags.h>
>>> +
>>> +#define _REGPARM
>>> +
>>> +/*
>>> + * The jmp_buf is assumed to contain the following, in order:
>>> + *     %ebx
>>> + *     %esp
>>> + *     %ebp
>>> + *     %esi
>>> + *     %edi
>>> + *     <return address>
>>> + */
>>> +
>>> +       /*
>>> +        * rdi - 32-bit code segment selector
>>> +        * rsi - target address
>>> +        * rdx - table address (0 if none)
>>> +        */
>>
>> The comment above looks irrelevant.
>
> OK, will drop.
>
>>
>>> +       .text
>>> +       .align 4
>>> +       .globl setjmp
>>> +       .type setjmp, @function
>>> +setjmp:
>>> +#ifdef _REGPARM
>>> +       movl %eax,%edx
>>
>> nits: needs space after ",". please fix this globally in this file.
>>
>>> +#else
>>> +       movl 4(%esp),%edx
>>> +#endif
>>> +       popl %ecx                       # Return address, and adjust the stack
>>
>> I believe # is deprecated. We should use /* */
>
> OK
>
>>
>>> +       xorl %eax,%eax                  # Return value
>>> +       movl %ebx,(%edx)
>>> +       movl %esp,4(%edx)               # Post-return %esp!
>>> +       pushl %ecx                      # Make the call/return stack happy
>>> +       movl %ebp,8(%edx)
>>> +       movl %esi,12(%edx)
>>> +       movl %edi,16(%edx)
>>> +       movl %ecx,20(%edx)              # Return address
>>> +       ret
>>> +
>>> +       .size setjmp,.-setjmp
>>
>> What is this .size for?
>
> Size of the function code. It helps with nm --size-sort I think.
>
>>
>>> +
>>> +       .text
>>
>> nits: not necessary
>>
>>> +       .align 4
>>> +       .globl longjmp
>>> +       .type longjmp, @function
>>> +longjmp:
>>> +#ifdef _REGPARM
>>> +       xchgl %eax,%edx
>>> +#else
>>> +       movl 4(%esp),%edx               # jmp_ptr address
>>> +       movl 8(%esp),%eax               # Return value
>>> +#endif
>>> +       movl (%edx),%ebx
>>> +       movl 4(%edx),%esp
>>> +       movl 8(%edx),%ebp
>>> +       movl 12(%edx),%esi
>>> +       movl 16(%edx),%edi
>>> +       jmp *20(%edx)
>>> +
>>> +       .size longjmp,.-longjmp
>>> diff --git a/arch/x86/include/asm/setjmp.h b/arch/x86/include/asm/setjmp.h
>>> new file mode 100644
>>> index 0000000..deef67e
>>> --- /dev/null
>>> +++ b/arch/x86/include/asm/setjmp.h
>>> @@ -0,0 +1,24 @@
>>> +/*
>>> + * Written by H. Peter Anvin <hpa at zytor.com>
>>> + * Brought in from Linux v4.4 and modified for U-Boot
>>> + * From Linux arch/um/sys-i386/setjmp.S
>>> + *
>>> + * SPDX-License-Identifier:    GPL-2.0
>>> + */
>>> +
>>> +#ifndef __setjmp_h
>>> +#define __setjmp_h
>>> +
>>> +struct jmp_buf_data {
>>> +       unsigned int __ebx;
>>> +       unsigned int __esp;
>>> +       unsigned int __ebp;
>>> +       unsigned int __esi;
>>> +       unsigned int __edi;
>>> +       unsigned int __eip;
>>> +};
>>> +
>>> +int setjmp(struct jmp_buf_data *jmp_buf);
>>> +void longjmp(struct jmp_buf_data *jmp_buf);
>>
>> shouldn't it be void longjmp(struct jmp_buf_data *jmp_buf, int val)? I
>> am wondering how EFI loader could work with this longjmp()?
>
> efi_exit() seems to not need the extra parameter.

This does not look good to me. See efi_start_image(), there is a test
against return value of setjmp(), but longjmp() does not provide a
return value, which means the return value is undetermined.

if (setjmp(&info->exit_jmp)) {
    /* We returned from the child image */
    return EFI_EXIT(info->exit_status);
}

Regards,
Bin