| Message ID | 1474838857-25045-1-git-send-email-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Bin Meng |
| Headers | show |
Hi Simon, On Mon, Sep 26, 2016 at 5:27 AM, Simon Glass <sjg@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@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@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(). > + 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@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); > + > +#endif > -- Regards, Bin
On 26.09.16 09:00, Bin Meng wrote: > Hi Simon, > > On Mon, Sep 26, 2016 at 5:27 AM, Simon Glass <sjg@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@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@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? Alex
Hi Alex, On Mon, Sep 26, 2016 at 3:05 PM, Alexander Graf <agraf@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@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@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@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
On 26.09.16 09:21, Bin Meng wrote: > Hi Alex, > > On Mon, Sep 26, 2016 at 3:05 PM, Alexander Graf <agraf@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@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@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@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. Alex
Hi Alex, On 26 September 2016 at 01:28, Alexander Graf <agraf@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@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@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@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@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
On Tue, Sep 27, 2016 at 8:34 AM, Simon Glass <sjg@chromium.org> wrote: > Hi Alex, > > On 26 September 2016 at 01:28, Alexander Graf <agraf@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@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@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@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@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
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@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> + */ + + .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 + 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@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); + +#endif
Bring in these functions from Linux v4.4. They will be needed for EFI loader support. Signed-off-by: Simon Glass <sjg@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