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

[Bin Meng]

On Tue, Sep 27, 2016 at 8:34 AM, Simon Glass <sjg at chromium.org> wrote:
> 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.

Yes, we should fix the longjump() declaration as well as the ARM codes
first. Then rework this x86 implementation to handle return value in
the assembly codes.

Regards,
Bin