Hi Alex, On 26 September 2016 at 01:28, Alexander Graf <ag...@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 <ag...@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 <s...@chromium.org> wrote: >>>>> Bring in these functions from Linux v4.4. They will be needed for EFI >>>>> loader >>>>> support. >>>>> >>>>> Signed-off-by: Simon Glass <s...@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 <h...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot