subject:"Re\: \[U\-Boot\] \[PATCH 1\/6\] x86\: Add implementations of setjmp\(\) and longjmp\(\)"

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

2016-08-09 Thread Bin Meng

Hi Simon,

On Wed, Aug 10, 2016 at 2:16 AM, Simon Glass  wrote:
> Hi Bin,
>
> On 9 August 2016 at 00:49, Bin Meng  wrote:
>> Hi Simon,
>>
>> On Sun, Aug 7, 2016 at 7:23 AM, Simon Glass  wrote:
>>> Bring in these functions from Linux v4.4. They will be needed for EFI loader
>>> support.
>>>
>>> Signed-off-by: Simon Glass 
>>> ---
>>>
>>>  arch/x86/cpu/Makefile |  2 +-
>>>  arch/x86/cpu/setjmp.S | 71 
>>> +++
>>>  arch/x86/include/asm/setjmp.h | 24 +++
>>>  3 files changed, 96 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 000..24e7d8a
>>> --- /dev/null
>>> +++ b/arch/x86/cpu/setjmp.S
>>> @@ -0,0 +1,71 @@
>>> +/*
>>> + * Written by H. Peter Anvin 
>>> + * 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 
>>> +#include 
>>> +#include 
>>> +
>>> +#define _REGPARM
>>> +
>>> +/*
>>> + * The jmp_buf is assumed to contain the following, in order:
>>> + * %ebx
>>> + * %esp
>>> + * %ebp
>>> + * %esi
>>> + * %edi
>>> + * 
>>> + */
>>> +
>>> +   /*
>>> +* rdi - 32-bit code segment selector
>>> +* rsi - target address
>>> +* rdx - table address (0 if none)
>>> +*/
>>
>> The comment above looks irrelevant.
>
> OK, will drop.
>
>>
>>> +   .text
>>> +   .align 4
>>> +   .globl setjmp
>>> +   .type setjmp, @function
>>> +setjmp:
>>> +#ifdef _REGPARM
>>> +   movl %eax,%edx
>>
>> nits: needs space after ",". please fix this globally in this file.
>>
>>> +#else
>>> +   movl 4(%esp),%edx
>>> +#endif
>>> +   popl %ecx   # Return address, and adjust the 
>>> stack
>>
>> I believe # is deprecated. We should use /* */
>
> OK
>
>>
>>> +   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
>>> +
>>> +   .size setjmp,.-setjmp
>>
>> What is this .size for?
>
> Size of the function code. It helps with nm --size-sort I think.
>
>>
>>> +
>>> +   .text
>>
>> nits: not necessary
>>
>>> +   .align 4
>>> +   .globl longjmp
>>> +   .type longjmp, @function
>>> +longjmp:
>>> +#ifdef _REGPARM
>>> +   xchgl %eax,%edx
>>> +#else
>>> +   movl 4(%esp),%edx   # jmp_ptr address
>>> +   movl 8(%esp),%eax   # Return value
>>> +#endif
>>> +   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 000..deef67e
>>> --- /dev/null
>>> +++ b/arch/x86/include/asm/setjmp.h
>>> @@ -0,0 +1,24 @@
>>> +/*
>>> + * Written by H. Peter Anvin 
>>> + * 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);
>>
>> shouldn't it be void longjmp(struct jmp_buf_data *jmp_buf, int val)? I
>> am wondering how EFI loader could work with this longjmp()?
>
> efi_exit() seems to not need the extra parameter.

This does not look good to me. See efi_start_image(), there is a test
against return value of setjmp(), but longjmp() does not provide a
return value, which means the return value is undetermined.

if (setjmp(>exit_jmp)) {
/* We returned from the

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

2016-08-09 Thread Simon Glass

Hi Bin,

On 9 August 2016 at 00:49, Bin Meng  wrote:
> Hi Simon,
>
> On Sun, Aug 7, 2016 at 7:23 AM, Simon Glass  wrote:
>> Bring in these functions from Linux v4.4. They will be needed for EFI loader
>> support.
>>
>> Signed-off-by: Simon Glass 
>> ---
>>
>>  arch/x86/cpu/Makefile |  2 +-
>>  arch/x86/cpu/setjmp.S | 71 
>> +++
>>  arch/x86/include/asm/setjmp.h | 24 +++
>>  3 files changed, 96 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 000..24e7d8a
>> --- /dev/null
>> +++ b/arch/x86/cpu/setjmp.S
>> @@ -0,0 +1,71 @@
>> +/*
>> + * Written by H. Peter Anvin 
>> + * 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 
>> +#include 
>> +#include 
>> +
>> +#define _REGPARM
>> +
>> +/*
>> + * The jmp_buf is assumed to contain the following, in order:
>> + * %ebx
>> + * %esp
>> + * %ebp
>> + * %esi
>> + * %edi
>> + * 
>> + */
>> +
>> +   /*
>> +* rdi - 32-bit code segment selector
>> +* rsi - target address
>> +* rdx - table address (0 if none)
>> +*/
>
> The comment above looks irrelevant.

OK, will drop.

>
>> +   .text
>> +   .align 4
>> +   .globl setjmp
>> +   .type setjmp, @function
>> +setjmp:
>> +#ifdef _REGPARM
>> +   movl %eax,%edx
>
> nits: needs space after ",". please fix this globally in this file.
>
>> +#else
>> +   movl 4(%esp),%edx
>> +#endif
>> +   popl %ecx   # Return address, and adjust the 
>> stack
>
> I believe # is deprecated. We should use /* */

OK

>
>> +   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
>> +
>> +   .size setjmp,.-setjmp
>
> What is this .size for?

Size of the function code. It helps with nm --size-sort I think.

>
>> +
>> +   .text
>
> nits: not necessary
>
>> +   .align 4
>> +   .globl longjmp
>> +   .type longjmp, @function
>> +longjmp:
>> +#ifdef _REGPARM
>> +   xchgl %eax,%edx
>> +#else
>> +   movl 4(%esp),%edx   # jmp_ptr address
>> +   movl 8(%esp),%eax   # Return value
>> +#endif
>> +   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 000..deef67e
>> --- /dev/null
>> +++ b/arch/x86/include/asm/setjmp.h
>> @@ -0,0 +1,24 @@
>> +/*
>> + * Written by H. Peter Anvin 
>> + * 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);
>
> shouldn't it be void longjmp(struct jmp_buf_data *jmp_buf, int val)? I
> am wondering how EFI loader could work with this longjmp()?

efi_exit() seems to not need the extra parameter.

>
>> +
>> +#endif
>> --
>
> Regards,
> Bin

Regards,
SImon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2016-08-09 Thread Bin Meng

Hi Simon,

On Sun, Aug 7, 2016 at 7:23 AM, Simon Glass  wrote:
> Bring in these functions from Linux v4.4. They will be needed for EFI loader
> support.
>
> Signed-off-by: Simon Glass 
> ---
>
>  arch/x86/cpu/Makefile |  2 +-
>  arch/x86/cpu/setjmp.S | 71 
> +++
>  arch/x86/include/asm/setjmp.h | 24 +++
>  3 files changed, 96 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 000..24e7d8a
> --- /dev/null
> +++ b/arch/x86/cpu/setjmp.S
> @@ -0,0 +1,71 @@
> +/*
> + * Written by H. Peter Anvin 
> + * 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 
> +#include 
> +#include 
> +
> +#define _REGPARM
> +
> +/*
> + * The jmp_buf is assumed to contain the following, in order:
> + * %ebx
> + * %esp
> + * %ebp
> + * %esi
> + * %edi
> + * 
> + */
> +
> +   /*
> +* rdi - 32-bit code segment selector
> +* rsi - target address
> +* rdx - table address (0 if none)
> +*/

The comment above looks irrelevant.

> +   .text
> +   .align 4
> +   .globl setjmp
> +   .type setjmp, @function
> +setjmp:
> +#ifdef _REGPARM
> +   movl %eax,%edx

nits: needs space after ",". please fix this globally in this file.

> +#else
> +   movl 4(%esp),%edx
> +#endif
> +   popl %ecx   # Return address, and adjust the stack

I believe # is deprecated. We should use /* */

> +   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
> +
> +   .size setjmp,.-setjmp

What is this .size for?

> +
> +   .text

nits: not necessary

> +   .align 4
> +   .globl longjmp
> +   .type longjmp, @function
> +longjmp:
> +#ifdef _REGPARM
> +   xchgl %eax,%edx
> +#else
> +   movl 4(%esp),%edx   # jmp_ptr address
> +   movl 8(%esp),%eax   # Return value
> +#endif
> +   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 000..deef67e
> --- /dev/null
> +++ b/arch/x86/include/asm/setjmp.h
> @@ -0,0 +1,24 @@
> +/*
> + * Written by H. Peter Anvin 
> + * 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);

shouldn't it be void longjmp(struct jmp_buf_data *jmp_buf, int val)? I
am wondering how EFI loader could work with this longjmp()?

> +
> +#endif
> --

Regards,
Bin
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

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

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

3 matches

Site Navigation

Mail list logo

Footer information