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

[Simon Glass]

Hi Alex,

On 26 September 2016 at 01:28, Alexander Graf <agraf at suse.de> wrote:
>
>
> On 26.09.16 09:21, Bin Meng wrote:
>> 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().
>
> Ah, I see. The second parameter to longjmp() should be the the return
> value after the jump - which the code above actually implements. And
> then it clobbers it with a 1.
>
> I guess that's my fault, because I skipped the second argument bit in my
> longjmp header definition. Please just add it to the longjmp prototype
> and explicitly pass "1" as return value in (don't forget to adapt the
> prototypes in arch/arm/include/asm/setjmp.h). I can fix up the arm code
> later to actually pass the argument.

Would you mind doing the fix-up patch? I don't think we should change
the prototype separate from the ARM code.

Regards,
Simon