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

[Bin Meng]

Hi Alex,

On Mon, Sep 26, 2016 at 3:05 PM, Alexander Graf <agraf at suse.de> wrote:
>
>
> On 26.09.16 09:00, Bin Meng wrote:
>> Hi Simon,
>>
>> On Mon, Sep 26, 2016 at 5:27 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>
>>> ---
>>>
>>> Changes in v2:
>>> - Drop irrelevant comment
>>> - Add a comment about .size
>>> - Drop unnecessary .text directive
>>> - Make longjmp() always cause setjmp() to return 1
>>>
>>>  arch/x86/cpu/Makefile         |  2 +-
>>>  arch/x86/cpu/setjmp.S         | 66 +++++++++++++++++++++++++++++++++++++++++++
>>>  arch/x86/include/asm/setjmp.h | 24 ++++++++++++++++
>>>  3 files changed, 91 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..7443274
>>> --- /dev/null
>>> +++ b/arch/x86/cpu/setjmp.S
>>> @@ -0,0 +1,66 @@
>>> +/*
>>> + * 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>
>>> +
>>
>> I believe the above 3 includes are not needed.
>>
>>> +#define _REGPARM
>>> +
>>> +/*
>>> + * The jmp_buf is assumed to contain the following, in order:
>>> + *     %ebx
>>> + *     %esp
>>> + *     %ebp
>>> + *     %esi
>>> + *     %edi
>>> + *     <return address>
>>> + */
>>> +
>>> +       .text
>>> +       .align 4
>>> +       .globl setjmp
>>> +       .type setjmp, @function
>>> +setjmp:
>>> +#ifdef _REGPARM
>>> +       movl %eax, %edx
>>> +#else
>>> +       movl 4(%esp), %edx
>>> +#endif
>>> +       popl %ecx               /* Return address, and adjust the stack */
>>> +       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
>>> +
>>> +       /* Provide function size if needed */
>>> +       .size setjmp, .-setjmp
>>> +
>>> +       .align 4
>>> +       .globl longjmp
>>> +       .type longjmp, @function
>>> +longjmp:
>>> +#ifdef _REGPARM
>>> +       xchgl %eax, %edx
>>> +#else
>>> +       movl 4(%esp), %edx      /* jmp_ptr address */
>>> +#endif
>>> +       movl 1, %eax
>>
>> This should be $1.
>>
>> But I still think the setjmp/longjump codes in efi_loader is wrong. We
>> should have longjump() to pass the return value to setjmp().
>
> Why? Where's the difference?
>

longjump() does not have the setjmp() return value as the parameter,
which concerns me as it does not conform to the standard longjump()
implementation. This v2 hardcoded the return value to 1, which makes
the following logic work in efi_loader.

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

If we want such a simplified implementation in efi_loader, we should
probably rename these functions to efi_setjmp() and efi_longjump().

Regards,
Bin