Re: [U-Boot] [PATCH] efi_loader: Rename sections to allow for implicit data

[Simon Glass]

Hi Tom,

On 23 August 2018 at 11:55, Tom Rini  wrote:
> On Thu, Aug 23, 2018 at 06:41:56PM +0200, Andreas Färber wrote:
>> Am 23.08.2018 um 18:31 schrieb Simon Glass:
>> > On 23 August 2018 at 07:58, Tom Rini  wrote:
>> >> OK, now I'm trying to see what we need to do next.  Regardless of Alex's
>> >> current EFI PR, we have 7e21fbca26d1 ("efi_loader: Rename sections to
>> >> allow for implicit data") in master.  Looking at the PR, I see that it
>> >> does turn on -fdata-sections on x86, and that Bin reviewed/tested it.
>> >> So I'm going to push the PR there shortly.
>> >>
>> >> If, for sandbox you want to backout --gc-sections now (the commit
>> >> message does spell out that we need -ffunction/data-sections and what
>> >> for) and re-introduce that for the next release please post the patches
>> >> and I'll grab the backout one and apply it (or post it then PR it if
>> >> you'd rather, that's fine too).
>> >
>> > I'd like to back out all of the compilel/link flag changes:
>> >
>> > Here is my patch:
>> >
>> > http://patchwork.ozlabs.org/patch/954875/
>>
>> If you go down that path, please align the subject of that commit with
>> what it does, e.g., s/Revert/Partially revert/. Otherwise it is terribly
>> confusing for users that don't care about sandbox.

Yes I agree, sorry. I did a full revert and then changed it because I
figured the only real problem I had was with sandbox.

>
> With a slight subject reword, should I grab this patch now then?

That's fine with me.

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] efi_loader: Rename sections to allow for implicit data

[Tom Rini]

On Thu, Aug 23, 2018 at 06:41:56PM +0200, Andreas Färber wrote:
> Am 23.08.2018 um 18:31 schrieb Simon Glass:
> > On 23 August 2018 at 07:58, Tom Rini  wrote:
> >> OK, now I'm trying to see what we need to do next.  Regardless of Alex's
> >> current EFI PR, we have 7e21fbca26d1 ("efi_loader: Rename sections to
> >> allow for implicit data") in master.  Looking at the PR, I see that it
> >> does turn on -fdata-sections on x86, and that Bin reviewed/tested it.
> >> So I'm going to push the PR there shortly.
> >>
> >> If, for sandbox you want to backout --gc-sections now (the commit
> >> message does spell out that we need -ffunction/data-sections and what
> >> for) and re-introduce that for the next release please post the patches
> >> and I'll grab the backout one and apply it (or post it then PR it if
> >> you'd rather, that's fine too).
> > 
> > I'd like to back out all of the compilel/link flag changes:
> > 
> > Here is my patch:
> > 
> > http://patchwork.ozlabs.org/patch/954875/
> 
> If you go down that path, please align the subject of that commit with
> what it does, e.g., s/Revert/Partially revert/. Otherwise it is terribly
> confusing for users that don't care about sandbox.

With a slight subject reword, should I grab this patch now then?

-- 
Tom


signature.asc
Description: PGP signature
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] efi_loader: Rename sections to allow for implicit data

[Andreas Färber]

Am 23.08.2018 um 18:31 schrieb Simon Glass:
> On 23 August 2018 at 07:58, Tom Rini  wrote:
>> OK, now I'm trying to see what we need to do next.  Regardless of Alex's
>> current EFI PR, we have 7e21fbca26d1 ("efi_loader: Rename sections to
>> allow for implicit data") in master.  Looking at the PR, I see that it
>> does turn on -fdata-sections on x86, and that Bin reviewed/tested it.
>> So I'm going to push the PR there shortly.
>>
>> If, for sandbox you want to backout --gc-sections now (the commit
>> message does spell out that we need -ffunction/data-sections and what
>> for) and re-introduce that for the next release please post the patches
>> and I'll grab the backout one and apply it (or post it then PR it if
>> you'd rather, that's fine too).
> 
> I'd like to back out all of the compilel/link flag changes:
> 
> Here is my patch:
> 
> http://patchwork.ozlabs.org/patch/954875/

If you go down that path, please align the subject of that commit with
what it does, e.g., s/Revert/Partially revert/. Otherwise it is terribly
confusing for users that don't care about sandbox.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] efi_loader: Rename sections to allow for implicit data

[Simon Glass]

Hi Tom.

On 23 August 2018 at 07:58, Tom Rini  wrote:
> On Thu, Aug 23, 2018 at 04:45:29AM -0600, Simon Glass wrote:
>> Hi Tom,
>>
>> On 21 August 2018 at 17:11, Tom Rini  wrote:
>> > On Tue, Aug 21, 2018 at 04:29:49PM -0600, Simon Glass wrote:
>> >> Hi Alex,
>> >>
>> >> On 21 August 2018 at 13:26, Alexander Graf  wrote:
>> >> >
>> >> >
>> >> > On 21.08.18 19:30, Simon Glass wrote:
>> >> >> Hi Alex,
>> >> >>
>> >> >> On 20 August 2018 at 06:23, Alexander Graf  wrote:
>> >> >>>
>> >> >>> On 08/17/2018 02:49 PM, Simon Glass wrote:
>> >> 
>> >>  Hi,
>> >> 
>> >>  On 9 August 2018 at 23:45, Bin Meng  wrote:
>> >> >
>> >> > Hi Alex,
>> >> >
>> >> > On Wed, Aug 8, 2018 at 1:16 AM, Alexander Graf  
>> >> > wrote:
>> >> >>
>> >> >>
>> >> >>> Am 07.08.2018 um 18:12 schrieb Simon Glass :
>> >> >>>
>> >> >>> Hi Alex,
>> >> >>>
>> >>  On 11 June 2018 at 23:48, Alexander Graf  wrote:
>> >>  Some times gcc may generate data that is then used within code 
>> >>  that may
>> >>  be part of an efi runtime section. That data could be jump 
>> >>  tables,
>> >>  constants or strings.
>> >> 
>> >>  In order to make sure we catch these, we need to ensure that gcc 
>> >>  emits
>> >>  them into a section that we can relocate together with all the 
>> >>  other
>> >>  efi runtime bits. This only works if the -ffunction-sections and
>> >>  -fdata-sections flags are passed and the efi runtime functions 
>> >>  are
>> >>  in a section that starts with ".text".
>> >> 
>> >>  Up to now we had all efi runtime bits in sections that did not
>> >>  interfere with the normal section naming scheme, but this forces
>> >>  us to do so. Hence we need to move the efi_loader 
>> >>  text/data/rodata
>> >>  sections before the global *(.text*) catch-all section.
>> >> 
>> >>  With this patch in place, we should hopefully have an easier time
>> >>  to extend the efi runtime functionality in the future.
>> >> 
>> >>  Signed-off-by: Alexander Graf 
>> >>  ---
>> >>  arch/arm/config.mk|  4 ++--
>> >>  arch/arm/cpu/armv8/u-boot.lds | 24 
>> >>  +
>> >>  arch/arm/cpu/u-boot.lds   | 36 
>> >>  ++-
>> >>  arch/arm/mach-zynq/u-boot.lds | 36 
>> >>  ++-
>> >>  arch/riscv/cpu/ax25/u-boot.lds| 26 
>> >>  +-
>> >>  arch/sandbox/config.mk|  3 +++
>> >>  arch/sandbox/cpu/u-boot.lds   |  9 
>> >>  arch/x86/config.mk|  2 +-
>> >>  arch/x86/cpu/u-boot.lds   | 32 
>> >>  ++-
>> >>  board/qualcomm/dragonboard410c/u-boot.lds | 17 +--
>> >>  board/qualcomm/dragonboard820c/u-boot.lds | 24 
>> >>  +
>> >>  board/ti/am335x/u-boot.lds| 36 
>> >>  ++-
>> >>  include/efi_loader.h  |  4 ++--
>> >>  13 files changed, 154 insertions(+), 99 deletions(-)
>> >> 
>> >> >>> I missed this at the time, probably thinking the subject made it 
>> >> >>> sound
>> >> >>> innocuous. There is no 'sandbox:' tag.
>> >> >>>
>> >> >>> This seems to break sandbox in a pretty strange way:
>> >> >>>
>> >> >>> gdb --args /tmp/crosfw/sandbox/u-boot -D
>> >> >>> GNU gdb (Debian 7.12-6) 7.12.0.20161007-git
>> >> >>> Copyright (C) 2016 Free Software Foundation, Inc.
>> >> >>> License GPLv3+: GNU GPL version 3 or later 
>> >> >>> 
>> >> >>> This is free software: you are free to change and redistribute it.
>> >> >>> There is NO WARRANTY, to the extent permitted by law.  Type "show 
>> >> >>> copying"
>> >> >>> and "show warranty" for details.
>> >> >>> This GDB was configured as "x86_64-linux-gnu".
>> >> >>> Type "show configuration" for configuration details.
>> >> >>> For bug reporting instructions, please see:
>> >> >>> .
>> >> >>> Find the GDB manual and other documentation resources online at:
>> >> >>> .
>> >> >>> For help, type "help".
>> >> >>> Type "apropos word" to search for commands related to "word"...
>> >> >>> Reading symbols from /tmp/crosfw/sandbox/u-boot...done.
>> >> >>> (gdb) r
>> >> >>> Starting program: /tmp/crosfw/sandbox/u-boot -D
>> >> >>> [Thread debugging using libthread_db enabled]
>> >> >>> Using host libthread_db library 
>> >> >>> 

Re: [U-Boot] [PATCH] efi_loader: Rename sections to allow for implicit data

[Tom Rini]

On Thu, Aug 23, 2018 at 04:45:29AM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On 21 August 2018 at 17:11, Tom Rini  wrote:
> > On Tue, Aug 21, 2018 at 04:29:49PM -0600, Simon Glass wrote:
> >> Hi Alex,
> >>
> >> On 21 August 2018 at 13:26, Alexander Graf  wrote:
> >> >
> >> >
> >> > On 21.08.18 19:30, Simon Glass wrote:
> >> >> Hi Alex,
> >> >>
> >> >> On 20 August 2018 at 06:23, Alexander Graf  wrote:
> >> >>>
> >> >>> On 08/17/2018 02:49 PM, Simon Glass wrote:
> >> 
> >>  Hi,
> >> 
> >>  On 9 August 2018 at 23:45, Bin Meng  wrote:
> >> >
> >> > Hi Alex,
> >> >
> >> > On Wed, Aug 8, 2018 at 1:16 AM, Alexander Graf  wrote:
> >> >>
> >> >>
> >> >>> Am 07.08.2018 um 18:12 schrieb Simon Glass :
> >> >>>
> >> >>> Hi Alex,
> >> >>>
> >>  On 11 June 2018 at 23:48, Alexander Graf  wrote:
> >>  Some times gcc may generate data that is then used within code 
> >>  that may
> >>  be part of an efi runtime section. That data could be jump tables,
> >>  constants or strings.
> >> 
> >>  In order to make sure we catch these, we need to ensure that gcc 
> >>  emits
> >>  them into a section that we can relocate together with all the 
> >>  other
> >>  efi runtime bits. This only works if the -ffunction-sections and
> >>  -fdata-sections flags are passed and the efi runtime functions are
> >>  in a section that starts with ".text".
> >> 
> >>  Up to now we had all efi runtime bits in sections that did not
> >>  interfere with the normal section naming scheme, but this forces
> >>  us to do so. Hence we need to move the efi_loader text/data/rodata
> >>  sections before the global *(.text*) catch-all section.
> >> 
> >>  With this patch in place, we should hopefully have an easier time
> >>  to extend the efi runtime functionality in the future.
> >> 
> >>  Signed-off-by: Alexander Graf 
> >>  ---
> >>  arch/arm/config.mk|  4 ++--
> >>  arch/arm/cpu/armv8/u-boot.lds | 24 
> >>  +
> >>  arch/arm/cpu/u-boot.lds   | 36 
> >>  ++-
> >>  arch/arm/mach-zynq/u-boot.lds | 36 
> >>  ++-
> >>  arch/riscv/cpu/ax25/u-boot.lds| 26 
> >>  +-
> >>  arch/sandbox/config.mk|  3 +++
> >>  arch/sandbox/cpu/u-boot.lds   |  9 
> >>  arch/x86/config.mk|  2 +-
> >>  arch/x86/cpu/u-boot.lds   | 32 
> >>  ++-
> >>  board/qualcomm/dragonboard410c/u-boot.lds | 17 +--
> >>  board/qualcomm/dragonboard820c/u-boot.lds | 24 
> >>  +
> >>  board/ti/am335x/u-boot.lds| 36 
> >>  ++-
> >>  include/efi_loader.h  |  4 ++--
> >>  13 files changed, 154 insertions(+), 99 deletions(-)
> >> 
> >> >>> I missed this at the time, probably thinking the subject made it 
> >> >>> sound
> >> >>> innocuous. There is no 'sandbox:' tag.
> >> >>>
> >> >>> This seems to break sandbox in a pretty strange way:
> >> >>>
> >> >>> gdb --args /tmp/crosfw/sandbox/u-boot -D
> >> >>> GNU gdb (Debian 7.12-6) 7.12.0.20161007-git
> >> >>> Copyright (C) 2016 Free Software Foundation, Inc.
> >> >>> License GPLv3+: GNU GPL version 3 or later 
> >> >>> 
> >> >>> This is free software: you are free to change and redistribute it.
> >> >>> There is NO WARRANTY, to the extent permitted by law.  Type "show 
> >> >>> copying"
> >> >>> and "show warranty" for details.
> >> >>> This GDB was configured as "x86_64-linux-gnu".
> >> >>> Type "show configuration" for configuration details.
> >> >>> For bug reporting instructions, please see:
> >> >>> .
> >> >>> Find the GDB manual and other documentation resources online at:
> >> >>> .
> >> >>> For help, type "help".
> >> >>> Type "apropos word" to search for commands related to "word"...
> >> >>> Reading symbols from /tmp/crosfw/sandbox/u-boot...done.
> >> >>> (gdb) r
> >> >>> Starting program: /tmp/crosfw/sandbox/u-boot -D
> >> >>> [Thread debugging using libthread_db enabled]
> >> >>> Using host libthread_db library 
> >> >>> "/lib/x86_64-linux-gnu/libthread_db.so.1".
> >> >>>
> >> >>> Program received signal SIGSEGV, Segmentation fault.
> >> >>> 0x55571520 in open@plt ()
> >> >>> (gdb) up
> >> >>> #1  

Re: [U-Boot] [PATCH] efi_loader: Rename sections to allow for implicit data

[Simon Glass]

Hi Tom,

On 21 August 2018 at 17:11, Tom Rini  wrote:
> On Tue, Aug 21, 2018 at 04:29:49PM -0600, Simon Glass wrote:
>> Hi Alex,
>>
>> On 21 August 2018 at 13:26, Alexander Graf  wrote:
>> >
>> >
>> > On 21.08.18 19:30, Simon Glass wrote:
>> >> Hi Alex,
>> >>
>> >> On 20 August 2018 at 06:23, Alexander Graf  wrote:
>> >>>
>> >>> On 08/17/2018 02:49 PM, Simon Glass wrote:
>> 
>>  Hi,
>> 
>>  On 9 August 2018 at 23:45, Bin Meng  wrote:
>> >
>> > Hi Alex,
>> >
>> > On Wed, Aug 8, 2018 at 1:16 AM, Alexander Graf  wrote:
>> >>
>> >>
>> >>> Am 07.08.2018 um 18:12 schrieb Simon Glass :
>> >>>
>> >>> Hi Alex,
>> >>>
>>  On 11 June 2018 at 23:48, Alexander Graf  wrote:
>>  Some times gcc may generate data that is then used within code that 
>>  may
>>  be part of an efi runtime section. That data could be jump tables,
>>  constants or strings.
>> 
>>  In order to make sure we catch these, we need to ensure that gcc 
>>  emits
>>  them into a section that we can relocate together with all the other
>>  efi runtime bits. This only works if the -ffunction-sections and
>>  -fdata-sections flags are passed and the efi runtime functions are
>>  in a section that starts with ".text".
>> 
>>  Up to now we had all efi runtime bits in sections that did not
>>  interfere with the normal section naming scheme, but this forces
>>  us to do so. Hence we need to move the efi_loader text/data/rodata
>>  sections before the global *(.text*) catch-all section.
>> 
>>  With this patch in place, we should hopefully have an easier time
>>  to extend the efi runtime functionality in the future.
>> 
>>  Signed-off-by: Alexander Graf 
>>  ---
>>  arch/arm/config.mk|  4 ++--
>>  arch/arm/cpu/armv8/u-boot.lds | 24 +
>>  arch/arm/cpu/u-boot.lds   | 36 
>>  ++-
>>  arch/arm/mach-zynq/u-boot.lds | 36 
>>  ++-
>>  arch/riscv/cpu/ax25/u-boot.lds| 26 
>>  +-
>>  arch/sandbox/config.mk|  3 +++
>>  arch/sandbox/cpu/u-boot.lds   |  9 
>>  arch/x86/config.mk|  2 +-
>>  arch/x86/cpu/u-boot.lds   | 32 
>>  ++-
>>  board/qualcomm/dragonboard410c/u-boot.lds | 17 +--
>>  board/qualcomm/dragonboard820c/u-boot.lds | 24 +
>>  board/ti/am335x/u-boot.lds| 36 
>>  ++-
>>  include/efi_loader.h  |  4 ++--
>>  13 files changed, 154 insertions(+), 99 deletions(-)
>> 
>> >>> I missed this at the time, probably thinking the subject made it 
>> >>> sound
>> >>> innocuous. There is no 'sandbox:' tag.
>> >>>
>> >>> This seems to break sandbox in a pretty strange way:
>> >>>
>> >>> gdb --args /tmp/crosfw/sandbox/u-boot -D
>> >>> GNU gdb (Debian 7.12-6) 7.12.0.20161007-git
>> >>> Copyright (C) 2016 Free Software Foundation, Inc.
>> >>> License GPLv3+: GNU GPL version 3 or later 
>> >>> 
>> >>> This is free software: you are free to change and redistribute it.
>> >>> There is NO WARRANTY, to the extent permitted by law.  Type "show 
>> >>> copying"
>> >>> and "show warranty" for details.
>> >>> This GDB was configured as "x86_64-linux-gnu".
>> >>> Type "show configuration" for configuration details.
>> >>> For bug reporting instructions, please see:
>> >>> .
>> >>> Find the GDB manual and other documentation resources online at:
>> >>> .
>> >>> For help, type "help".
>> >>> Type "apropos word" to search for commands related to "word"...
>> >>> Reading symbols from /tmp/crosfw/sandbox/u-boot...done.
>> >>> (gdb) r
>> >>> Starting program: /tmp/crosfw/sandbox/u-boot -D
>> >>> [Thread debugging using libthread_db enabled]
>> >>> Using host libthread_db library 
>> >>> "/lib/x86_64-linux-gnu/libthread_db.so.1".
>> >>>
>> >>> Program received signal SIGSEGV, Segmentation fault.
>> >>> 0x55571520 in open@plt ()
>> >>> (gdb) up
>> >>> #1  0x55571e9a in sandbox_read_fdt_from_file ()
>> >>> at 
>> >>> /home/sjg/c/src/third_party/u-boot/files/arch/sandbox/cpu/cpu.c:264
>> >>> 264 fd = os_open(fname, OS_O_RDONLY);
>> >>> (gdb) print fname
>> >>> $1 = 0x77ff "/tmp/crosfw/sandbox/u-boot.dtb"
>> >>> (gdb) q
>> >>>
>> 

Re: [U-Boot] [PATCH] efi_loader: Rename sections to allow for implicit data

[Tom Rini]

On Tue, Aug 21, 2018 at 04:29:49PM -0600, Simon Glass wrote:
> Hi Alex,
> 
> On 21 August 2018 at 13:26, Alexander Graf  wrote:
> >
> >
> > On 21.08.18 19:30, Simon Glass wrote:
> >> Hi Alex,
> >>
> >> On 20 August 2018 at 06:23, Alexander Graf  wrote:
> >>>
> >>> On 08/17/2018 02:49 PM, Simon Glass wrote:
> 
>  Hi,
> 
>  On 9 August 2018 at 23:45, Bin Meng  wrote:
> >
> > Hi Alex,
> >
> > On Wed, Aug 8, 2018 at 1:16 AM, Alexander Graf  wrote:
> >>
> >>
> >>> Am 07.08.2018 um 18:12 schrieb Simon Glass :
> >>>
> >>> Hi Alex,
> >>>
>  On 11 June 2018 at 23:48, Alexander Graf  wrote:
>  Some times gcc may generate data that is then used within code that 
>  may
>  be part of an efi runtime section. That data could be jump tables,
>  constants or strings.
> 
>  In order to make sure we catch these, we need to ensure that gcc 
>  emits
>  them into a section that we can relocate together with all the other
>  efi runtime bits. This only works if the -ffunction-sections and
>  -fdata-sections flags are passed and the efi runtime functions are
>  in a section that starts with ".text".
> 
>  Up to now we had all efi runtime bits in sections that did not
>  interfere with the normal section naming scheme, but this forces
>  us to do so. Hence we need to move the efi_loader text/data/rodata
>  sections before the global *(.text*) catch-all section.
> 
>  With this patch in place, we should hopefully have an easier time
>  to extend the efi runtime functionality in the future.
> 
>  Signed-off-by: Alexander Graf 
>  ---
>  arch/arm/config.mk|  4 ++--
>  arch/arm/cpu/armv8/u-boot.lds | 24 +
>  arch/arm/cpu/u-boot.lds   | 36 
>  ++-
>  arch/arm/mach-zynq/u-boot.lds | 36 
>  ++-
>  arch/riscv/cpu/ax25/u-boot.lds| 26 +-
>  arch/sandbox/config.mk|  3 +++
>  arch/sandbox/cpu/u-boot.lds   |  9 
>  arch/x86/config.mk|  2 +-
>  arch/x86/cpu/u-boot.lds   | 32 
>  ++-
>  board/qualcomm/dragonboard410c/u-boot.lds | 17 +--
>  board/qualcomm/dragonboard820c/u-boot.lds | 24 +
>  board/ti/am335x/u-boot.lds| 36 
>  ++-
>  include/efi_loader.h  |  4 ++--
>  13 files changed, 154 insertions(+), 99 deletions(-)
> 
> >>> I missed this at the time, probably thinking the subject made it sound
> >>> innocuous. There is no 'sandbox:' tag.
> >>>
> >>> This seems to break sandbox in a pretty strange way:
> >>>
> >>> gdb --args /tmp/crosfw/sandbox/u-boot -D
> >>> GNU gdb (Debian 7.12-6) 7.12.0.20161007-git
> >>> Copyright (C) 2016 Free Software Foundation, Inc.
> >>> License GPLv3+: GNU GPL version 3 or later 
> >>> 
> >>> This is free software: you are free to change and redistribute it.
> >>> There is NO WARRANTY, to the extent permitted by law.  Type "show 
> >>> copying"
> >>> and "show warranty" for details.
> >>> This GDB was configured as "x86_64-linux-gnu".
> >>> Type "show configuration" for configuration details.
> >>> For bug reporting instructions, please see:
> >>> .
> >>> Find the GDB manual and other documentation resources online at:
> >>> .
> >>> For help, type "help".
> >>> Type "apropos word" to search for commands related to "word"...
> >>> Reading symbols from /tmp/crosfw/sandbox/u-boot...done.
> >>> (gdb) r
> >>> Starting program: /tmp/crosfw/sandbox/u-boot -D
> >>> [Thread debugging using libthread_db enabled]
> >>> Using host libthread_db library 
> >>> "/lib/x86_64-linux-gnu/libthread_db.so.1".
> >>>
> >>> Program received signal SIGSEGV, Segmentation fault.
> >>> 0x55571520 in open@plt ()
> >>> (gdb) up
> >>> #1  0x55571e9a in sandbox_read_fdt_from_file ()
> >>> at 
> >>> /home/sjg/c/src/third_party/u-boot/files/arch/sandbox/cpu/cpu.c:264
> >>> 264 fd = os_open(fname, OS_O_RDONLY);
> >>> (gdb) print fname
> >>> $1 = 0x77ff "/tmp/crosfw/sandbox/u-boot.dtb"
> >>> (gdb) q
> >>>
> >>>
> >>> Also the commit message suggests that this patch changes sandbox to
> >>> use --gc-sections, which is not obvious from the subject. I think that
> >>> should be a 

Re: [U-Boot] [PATCH] efi_loader: Rename sections to allow for implicit data

[Simon Glass]

Hi Alex,

On 21 August 2018 at 13:26, Alexander Graf  wrote:
>
>
> On 21.08.18 19:30, Simon Glass wrote:
>> Hi Alex,
>>
>> On 20 August 2018 at 06:23, Alexander Graf  wrote:
>>>
>>> On 08/17/2018 02:49 PM, Simon Glass wrote:

 Hi,

 On 9 August 2018 at 23:45, Bin Meng  wrote:
>
> Hi Alex,
>
> On Wed, Aug 8, 2018 at 1:16 AM, Alexander Graf  wrote:
>>
>>
>>> Am 07.08.2018 um 18:12 schrieb Simon Glass :
>>>
>>> Hi Alex,
>>>
 On 11 June 2018 at 23:48, Alexander Graf  wrote:
 Some times gcc may generate data that is then used within code that may
 be part of an efi runtime section. That data could be jump tables,
 constants or strings.

 In order to make sure we catch these, we need to ensure that gcc emits
 them into a section that we can relocate together with all the other
 efi runtime bits. This only works if the -ffunction-sections and
 -fdata-sections flags are passed and the efi runtime functions are
 in a section that starts with ".text".

 Up to now we had all efi runtime bits in sections that did not
 interfere with the normal section naming scheme, but this forces
 us to do so. Hence we need to move the efi_loader text/data/rodata
 sections before the global *(.text*) catch-all section.

 With this patch in place, we should hopefully have an easier time
 to extend the efi runtime functionality in the future.

 Signed-off-by: Alexander Graf 
 ---
 arch/arm/config.mk|  4 ++--
 arch/arm/cpu/armv8/u-boot.lds | 24 +
 arch/arm/cpu/u-boot.lds   | 36 
 ++-
 arch/arm/mach-zynq/u-boot.lds | 36 
 ++-
 arch/riscv/cpu/ax25/u-boot.lds| 26 +-
 arch/sandbox/config.mk|  3 +++
 arch/sandbox/cpu/u-boot.lds   |  9 
 arch/x86/config.mk|  2 +-
 arch/x86/cpu/u-boot.lds   | 32 
 ++-
 board/qualcomm/dragonboard410c/u-boot.lds | 17 +--
 board/qualcomm/dragonboard820c/u-boot.lds | 24 +
 board/ti/am335x/u-boot.lds| 36 
 ++-
 include/efi_loader.h  |  4 ++--
 13 files changed, 154 insertions(+), 99 deletions(-)

>>> I missed this at the time, probably thinking the subject made it sound
>>> innocuous. There is no 'sandbox:' tag.
>>>
>>> This seems to break sandbox in a pretty strange way:
>>>
>>> gdb --args /tmp/crosfw/sandbox/u-boot -D
>>> GNU gdb (Debian 7.12-6) 7.12.0.20161007-git
>>> Copyright (C) 2016 Free Software Foundation, Inc.
>>> License GPLv3+: GNU GPL version 3 or later 
>>> 
>>> This is free software: you are free to change and redistribute it.
>>> There is NO WARRANTY, to the extent permitted by law.  Type "show 
>>> copying"
>>> and "show warranty" for details.
>>> This GDB was configured as "x86_64-linux-gnu".
>>> Type "show configuration" for configuration details.
>>> For bug reporting instructions, please see:
>>> .
>>> Find the GDB manual and other documentation resources online at:
>>> .
>>> For help, type "help".
>>> Type "apropos word" to search for commands related to "word"...
>>> Reading symbols from /tmp/crosfw/sandbox/u-boot...done.
>>> (gdb) r
>>> Starting program: /tmp/crosfw/sandbox/u-boot -D
>>> [Thread debugging using libthread_db enabled]
>>> Using host libthread_db library 
>>> "/lib/x86_64-linux-gnu/libthread_db.so.1".
>>>
>>> Program received signal SIGSEGV, Segmentation fault.
>>> 0x55571520 in open@plt ()
>>> (gdb) up
>>> #1  0x55571e9a in sandbox_read_fdt_from_file ()
>>> at 
>>> /home/sjg/c/src/third_party/u-boot/files/arch/sandbox/cpu/cpu.c:264
>>> 264 fd = os_open(fname, OS_O_RDONLY);
>>> (gdb) print fname
>>> $1 = 0x77ff "/tmp/crosfw/sandbox/u-boot.dtb"
>>> (gdb) q
>>>
>>>
>>> Also the commit message suggests that this patch changes sandbox to
>>> use --gc-sections, which is not obvious from the subject. I think that
>>> should be a separate commit and in fact it should really be separate
>>> commits for each arch, I think. That might help people notice it...
>>>
>>> I only noticed now since the EFI pull request has landed.
>>
>> Can you try my bss patch really quick? Maybe we're just overwriting gd.
>>
>> 

Re: [U-Boot] [PATCH] efi_loader: Rename sections to allow for implicit data

[Tom Rini]

On Tue, Aug 21, 2018 at 09:26:53PM +0200, Alexander Graf wrote:
> 
> 
> On 21.08.18 19:30, Simon Glass wrote:
> > Hi Alex,
> > 
> > On 20 August 2018 at 06:23, Alexander Graf  wrote:
> >>
> >> On 08/17/2018 02:49 PM, Simon Glass wrote:
> >>>
> >>> Hi,
> >>>
> >>> On 9 August 2018 at 23:45, Bin Meng  wrote:
> 
>  Hi Alex,
> 
>  On Wed, Aug 8, 2018 at 1:16 AM, Alexander Graf  wrote:
> >
> >
> >> Am 07.08.2018 um 18:12 schrieb Simon Glass :
> >>
> >> Hi Alex,
> >>
> >>> On 11 June 2018 at 23:48, Alexander Graf  wrote:
> >>> Some times gcc may generate data that is then used within code that 
> >>> may
> >>> be part of an efi runtime section. That data could be jump tables,
> >>> constants or strings.
> >>>
> >>> In order to make sure we catch these, we need to ensure that gcc emits
> >>> them into a section that we can relocate together with all the other
> >>> efi runtime bits. This only works if the -ffunction-sections and
> >>> -fdata-sections flags are passed and the efi runtime functions are
> >>> in a section that starts with ".text".
> >>>
> >>> Up to now we had all efi runtime bits in sections that did not
> >>> interfere with the normal section naming scheme, but this forces
> >>> us to do so. Hence we need to move the efi_loader text/data/rodata
> >>> sections before the global *(.text*) catch-all section.
> >>>
> >>> With this patch in place, we should hopefully have an easier time
> >>> to extend the efi runtime functionality in the future.
> >>>
> >>> Signed-off-by: Alexander Graf 
> >>> ---
> >>> arch/arm/config.mk|  4 ++--
> >>> arch/arm/cpu/armv8/u-boot.lds | 24 +
> >>> arch/arm/cpu/u-boot.lds   | 36 
> >>> ++-
> >>> arch/arm/mach-zynq/u-boot.lds | 36 
> >>> ++-
> >>> arch/riscv/cpu/ax25/u-boot.lds| 26 +-
> >>> arch/sandbox/config.mk|  3 +++
> >>> arch/sandbox/cpu/u-boot.lds   |  9 
> >>> arch/x86/config.mk|  2 +-
> >>> arch/x86/cpu/u-boot.lds   | 32 
> >>> ++-
> >>> board/qualcomm/dragonboard410c/u-boot.lds | 17 +--
> >>> board/qualcomm/dragonboard820c/u-boot.lds | 24 +
> >>> board/ti/am335x/u-boot.lds| 36 
> >>> ++-
> >>> include/efi_loader.h  |  4 ++--
> >>> 13 files changed, 154 insertions(+), 99 deletions(-)
> >>>
> >> I missed this at the time, probably thinking the subject made it sound
> >> innocuous. There is no 'sandbox:' tag.
> >>
> >> This seems to break sandbox in a pretty strange way:
> >>
> >> gdb --args /tmp/crosfw/sandbox/u-boot -D
> >> GNU gdb (Debian 7.12-6) 7.12.0.20161007-git
> >> Copyright (C) 2016 Free Software Foundation, Inc.
> >> License GPLv3+: GNU GPL version 3 or later 
> >> 
> >> This is free software: you are free to change and redistribute it.
> >> There is NO WARRANTY, to the extent permitted by law.  Type "show 
> >> copying"
> >> and "show warranty" for details.
> >> This GDB was configured as "x86_64-linux-gnu".
> >> Type "show configuration" for configuration details.
> >> For bug reporting instructions, please see:
> >> .
> >> Find the GDB manual and other documentation resources online at:
> >> .
> >> For help, type "help".
> >> Type "apropos word" to search for commands related to "word"...
> >> Reading symbols from /tmp/crosfw/sandbox/u-boot...done.
> >> (gdb) r
> >> Starting program: /tmp/crosfw/sandbox/u-boot -D
> >> [Thread debugging using libthread_db enabled]
> >> Using host libthread_db library 
> >> "/lib/x86_64-linux-gnu/libthread_db.so.1".
> >>
> >> Program received signal SIGSEGV, Segmentation fault.
> >> 0x55571520 in open@plt ()
> >> (gdb) up
> >> #1  0x55571e9a in sandbox_read_fdt_from_file ()
> >> at 
> >> /home/sjg/c/src/third_party/u-boot/files/arch/sandbox/cpu/cpu.c:264
> >> 264 fd = os_open(fname, OS_O_RDONLY);
> >> (gdb) print fname
> >> $1 = 0x77ff "/tmp/crosfw/sandbox/u-boot.dtb"
> >> (gdb) q
> >>
> >>
> >> Also the commit message suggests that this patch changes sandbox to
> >> use --gc-sections, which is not obvious from the subject. I think that
> >> should be a separate commit and in fact it should really be separate
> >> commits for each arch, I think. That might help people notice it...
> >>
> >> I only noticed now since the EFI 

Re: [U-Boot] [PATCH] efi_loader: Rename sections to allow for implicit data

[Alexander Graf]

On 21.08.18 19:30, Simon Glass wrote:
> Hi Alex,
> 
> On 20 August 2018 at 06:23, Alexander Graf  wrote:
>>
>> On 08/17/2018 02:49 PM, Simon Glass wrote:
>>>
>>> Hi,
>>>
>>> On 9 August 2018 at 23:45, Bin Meng  wrote:

 Hi Alex,

 On Wed, Aug 8, 2018 at 1:16 AM, Alexander Graf  wrote:
>
>
>> Am 07.08.2018 um 18:12 schrieb Simon Glass :
>>
>> Hi Alex,
>>
>>> On 11 June 2018 at 23:48, Alexander Graf  wrote:
>>> Some times gcc may generate data that is then used within code that may
>>> be part of an efi runtime section. That data could be jump tables,
>>> constants or strings.
>>>
>>> In order to make sure we catch these, we need to ensure that gcc emits
>>> them into a section that we can relocate together with all the other
>>> efi runtime bits. This only works if the -ffunction-sections and
>>> -fdata-sections flags are passed and the efi runtime functions are
>>> in a section that starts with ".text".
>>>
>>> Up to now we had all efi runtime bits in sections that did not
>>> interfere with the normal section naming scheme, but this forces
>>> us to do so. Hence we need to move the efi_loader text/data/rodata
>>> sections before the global *(.text*) catch-all section.
>>>
>>> With this patch in place, we should hopefully have an easier time
>>> to extend the efi runtime functionality in the future.
>>>
>>> Signed-off-by: Alexander Graf 
>>> ---
>>> arch/arm/config.mk|  4 ++--
>>> arch/arm/cpu/armv8/u-boot.lds | 24 +
>>> arch/arm/cpu/u-boot.lds   | 36 
>>> ++-
>>> arch/arm/mach-zynq/u-boot.lds | 36 
>>> ++-
>>> arch/riscv/cpu/ax25/u-boot.lds| 26 +-
>>> arch/sandbox/config.mk|  3 +++
>>> arch/sandbox/cpu/u-boot.lds   |  9 
>>> arch/x86/config.mk|  2 +-
>>> arch/x86/cpu/u-boot.lds   | 32 
>>> ++-
>>> board/qualcomm/dragonboard410c/u-boot.lds | 17 +--
>>> board/qualcomm/dragonboard820c/u-boot.lds | 24 +
>>> board/ti/am335x/u-boot.lds| 36 
>>> ++-
>>> include/efi_loader.h  |  4 ++--
>>> 13 files changed, 154 insertions(+), 99 deletions(-)
>>>
>> I missed this at the time, probably thinking the subject made it sound
>> innocuous. There is no 'sandbox:' tag.
>>
>> This seems to break sandbox in a pretty strange way:
>>
>> gdb --args /tmp/crosfw/sandbox/u-boot -D
>> GNU gdb (Debian 7.12-6) 7.12.0.20161007-git
>> Copyright (C) 2016 Free Software Foundation, Inc.
>> License GPLv3+: GNU GPL version 3 or later 
>> 
>> This is free software: you are free to change and redistribute it.
>> There is NO WARRANTY, to the extent permitted by law.  Type "show 
>> copying"
>> and "show warranty" for details.
>> This GDB was configured as "x86_64-linux-gnu".
>> Type "show configuration" for configuration details.
>> For bug reporting instructions, please see:
>> .
>> Find the GDB manual and other documentation resources online at:
>> .
>> For help, type "help".
>> Type "apropos word" to search for commands related to "word"...
>> Reading symbols from /tmp/crosfw/sandbox/u-boot...done.
>> (gdb) r
>> Starting program: /tmp/crosfw/sandbox/u-boot -D
>> [Thread debugging using libthread_db enabled]
>> Using host libthread_db library 
>> "/lib/x86_64-linux-gnu/libthread_db.so.1".
>>
>> Program received signal SIGSEGV, Segmentation fault.
>> 0x55571520 in open@plt ()
>> (gdb) up
>> #1  0x55571e9a in sandbox_read_fdt_from_file ()
>> at 
>> /home/sjg/c/src/third_party/u-boot/files/arch/sandbox/cpu/cpu.c:264
>> 264 fd = os_open(fname, OS_O_RDONLY);
>> (gdb) print fname
>> $1 = 0x77ff "/tmp/crosfw/sandbox/u-boot.dtb"
>> (gdb) q
>>
>>
>> Also the commit message suggests that this patch changes sandbox to
>> use --gc-sections, which is not obvious from the subject. I think that
>> should be a separate commit and in fact it should really be separate
>> commits for each arch, I think. That might help people notice it...
>>
>> I only noticed now since the EFI pull request has landed.
>
> Can you try my bss patch really quick? Maybe we're just overwriting gd.
>
> Alex
>
 This patch breaks efi-x86_app_defconfig. The EFI application no longer
 boots. I was testing on top of u-boot/master.

 If I do:

 diff 

Re: [U-Boot] [PATCH] efi_loader: Rename sections to allow for implicit data

[Simon Glass]

Hi Alex,

On 20 August 2018 at 06:23, Alexander Graf  wrote:
>
> On 08/17/2018 02:49 PM, Simon Glass wrote:
>>
>> Hi,
>>
>> On 9 August 2018 at 23:45, Bin Meng  wrote:
>>>
>>> Hi Alex,
>>>
>>> On Wed, Aug 8, 2018 at 1:16 AM, Alexander Graf  wrote:


> Am 07.08.2018 um 18:12 schrieb Simon Glass :
>
> Hi Alex,
>
>> On 11 June 2018 at 23:48, Alexander Graf  wrote:
>> Some times gcc may generate data that is then used within code that may
>> be part of an efi runtime section. That data could be jump tables,
>> constants or strings.
>>
>> In order to make sure we catch these, we need to ensure that gcc emits
>> them into a section that we can relocate together with all the other
>> efi runtime bits. This only works if the -ffunction-sections and
>> -fdata-sections flags are passed and the efi runtime functions are
>> in a section that starts with ".text".
>>
>> Up to now we had all efi runtime bits in sections that did not
>> interfere with the normal section naming scheme, but this forces
>> us to do so. Hence we need to move the efi_loader text/data/rodata
>> sections before the global *(.text*) catch-all section.
>>
>> With this patch in place, we should hopefully have an easier time
>> to extend the efi runtime functionality in the future.
>>
>> Signed-off-by: Alexander Graf 
>> ---
>> arch/arm/config.mk|  4 ++--
>> arch/arm/cpu/armv8/u-boot.lds | 24 +
>> arch/arm/cpu/u-boot.lds   | 36 
>> ++-
>> arch/arm/mach-zynq/u-boot.lds | 36 
>> ++-
>> arch/riscv/cpu/ax25/u-boot.lds| 26 +-
>> arch/sandbox/config.mk|  3 +++
>> arch/sandbox/cpu/u-boot.lds   |  9 
>> arch/x86/config.mk|  2 +-
>> arch/x86/cpu/u-boot.lds   | 32 
>> ++-
>> board/qualcomm/dragonboard410c/u-boot.lds | 17 +--
>> board/qualcomm/dragonboard820c/u-boot.lds | 24 +
>> board/ti/am335x/u-boot.lds| 36 
>> ++-
>> include/efi_loader.h  |  4 ++--
>> 13 files changed, 154 insertions(+), 99 deletions(-)
>>
> I missed this at the time, probably thinking the subject made it sound
> innocuous. There is no 'sandbox:' tag.
>
> This seems to break sandbox in a pretty strange way:
>
> gdb --args /tmp/crosfw/sandbox/u-boot -D
> GNU gdb (Debian 7.12-6) 7.12.0.20161007-git
> Copyright (C) 2016 Free Software Foundation, Inc.
> License GPLv3+: GNU GPL version 3 or later 
> 
> This is free software: you are free to change and redistribute it.
> There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
> and "show warranty" for details.
> This GDB was configured as "x86_64-linux-gnu".
> Type "show configuration" for configuration details.
> For bug reporting instructions, please see:
> .
> Find the GDB manual and other documentation resources online at:
> .
> For help, type "help".
> Type "apropos word" to search for commands related to "word"...
> Reading symbols from /tmp/crosfw/sandbox/u-boot...done.
> (gdb) r
> Starting program: /tmp/crosfw/sandbox/u-boot -D
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>
> Program received signal SIGSEGV, Segmentation fault.
> 0x55571520 in open@plt ()
> (gdb) up
> #1  0x55571e9a in sandbox_read_fdt_from_file ()
> at /home/sjg/c/src/third_party/u-boot/files/arch/sandbox/cpu/cpu.c:264
> 264 fd = os_open(fname, OS_O_RDONLY);
> (gdb) print fname
> $1 = 0x77ff "/tmp/crosfw/sandbox/u-boot.dtb"
> (gdb) q
>
>
> Also the commit message suggests that this patch changes sandbox to
> use --gc-sections, which is not obvious from the subject. I think that
> should be a separate commit and in fact it should really be separate
> commits for each arch, I think. That might help people notice it...
>
> I only noticed now since the EFI pull request has landed.

 Can you try my bss patch really quick? Maybe we're just overwriting gd.

 Alex

>>> This patch breaks efi-x86_app_defconfig. The EFI application no longer
>>> boots. I was testing on top of u-boot/master.
>>>
>>> If I do:
>>>
>>> diff --git a/arch/x86/config.mk b/arch/x86/config.mk
>>> index 586e11a..fc119ec 100644
>>> --- a/arch/x86/config.mk
>>> +++ b/arch/x86/config.mk
>>> @@ -24,7 +24,6 @@ endif
>>>   ifeq 

Re: [U-Boot] [PATCH] efi_loader: Rename sections to allow for implicit data

[Alexander Graf]

On 08/17/2018 02:49 PM, Simon Glass wrote:

Hi,

On 9 August 2018 at 23:45, Bin Meng  wrote:

Hi Alex,

On Wed, Aug 8, 2018 at 1:16 AM, Alexander Graf  wrote:



Am 07.08.2018 um 18:12 schrieb Simon Glass :

Hi Alex,


On 11 June 2018 at 23:48, Alexander Graf  wrote:
Some times gcc may generate data that is then used within code that may
be part of an efi runtime section. That data could be jump tables,
constants or strings.

In order to make sure we catch these, we need to ensure that gcc emits
them into a section that we can relocate together with all the other
efi runtime bits. This only works if the -ffunction-sections and
-fdata-sections flags are passed and the efi runtime functions are
in a section that starts with ".text".

Up to now we had all efi runtime bits in sections that did not
interfere with the normal section naming scheme, but this forces
us to do so. Hence we need to move the efi_loader text/data/rodata
sections before the global *(.text*) catch-all section.

With this patch in place, we should hopefully have an easier time
to extend the efi runtime functionality in the future.

Signed-off-by: Alexander Graf 
---
arch/arm/config.mk|  4 ++--
arch/arm/cpu/armv8/u-boot.lds | 24 +
arch/arm/cpu/u-boot.lds   | 36 ++-
arch/arm/mach-zynq/u-boot.lds | 36 ++-
arch/riscv/cpu/ax25/u-boot.lds| 26 +-
arch/sandbox/config.mk|  3 +++
arch/sandbox/cpu/u-boot.lds   |  9 
arch/x86/config.mk|  2 +-
arch/x86/cpu/u-boot.lds   | 32 ++-
board/qualcomm/dragonboard410c/u-boot.lds | 17 +--
board/qualcomm/dragonboard820c/u-boot.lds | 24 +
board/ti/am335x/u-boot.lds| 36 ++-
include/efi_loader.h  |  4 ++--
13 files changed, 154 insertions(+), 99 deletions(-)


I missed this at the time, probably thinking the subject made it sound
innocuous. There is no 'sandbox:' tag.

This seems to break sandbox in a pretty strange way:

gdb --args /tmp/crosfw/sandbox/u-boot -D
GNU gdb (Debian 7.12-6) 7.12.0.20161007-git
Copyright (C) 2016 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later 
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
.
Find the GDB manual and other documentation resources online at:
.
For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from /tmp/crosfw/sandbox/u-boot...done.
(gdb) r
Starting program: /tmp/crosfw/sandbox/u-boot -D
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

Program received signal SIGSEGV, Segmentation fault.
0x55571520 in open@plt ()
(gdb) up
#1  0x55571e9a in sandbox_read_fdt_from_file ()
at /home/sjg/c/src/third_party/u-boot/files/arch/sandbox/cpu/cpu.c:264
264 fd = os_open(fname, OS_O_RDONLY);
(gdb) print fname
$1 = 0x77ff "/tmp/crosfw/sandbox/u-boot.dtb"
(gdb) q


Also the commit message suggests that this patch changes sandbox to
use --gc-sections, which is not obvious from the subject. I think that
should be a separate commit and in fact it should really be separate
commits for each arch, I think. That might help people notice it...

I only noticed now since the EFI pull request has landed.

Can you try my bss patch really quick? Maybe we're just overwriting gd.

Alex


This patch breaks efi-x86_app_defconfig. The EFI application no longer
boots. I was testing on top of u-boot/master.

If I do:

diff --git a/arch/x86/config.mk b/arch/x86/config.mk
index 586e11a..fc119ec 100644
--- a/arch/x86/config.mk
+++ b/arch/x86/config.mk
@@ -24,7 +24,6 @@ endif
  ifeq ($(IS_32BIT),y)
  PLATFORM_CPPFLAGS += -march=i386 -m32
  # TODO: These break on x86_64; need to debug further
-PLATFORM_RELFLAGS += -fdata-sections
  else
  PLATFORM_CPPFLAGS += $(if $(CONFIG_SPL_BUILD),,-fpic) -fno-common -m64
  endif

Then it boots again. Can you please take a look?

Regards,
Bin

Please can we revert the offending patch quickly for the release? I am
not comfortable with the sandbox changes either (data-sections, etc.).
I can not reproduce the sandbox breakage (and travis doesn't seem to 
either, otherwise it would be broken for everyone, no?). Can you give me 
some guidelines on how to reproduce the failures for you and I'll just 
fix it?



Alex

Re: [U-Boot] [PATCH] efi_loader: Rename sections to allow for implicit data

[Alexander Graf]

On 08/20/2018 03:43 AM, Bin Meng wrote:

On Fri, Aug 17, 2018 at 8:49 PM, Simon Glass  wrote:

Hi,

On 9 August 2018 at 23:45, Bin Meng  wrote:

Hi Alex,

On Wed, Aug 8, 2018 at 1:16 AM, Alexander Graf  wrote:



Am 07.08.2018 um 18:12 schrieb Simon Glass :

Hi Alex,


On 11 June 2018 at 23:48, Alexander Graf  wrote:
Some times gcc may generate data that is then used within code that may
be part of an efi runtime section. That data could be jump tables,
constants or strings.

In order to make sure we catch these, we need to ensure that gcc emits
them into a section that we can relocate together with all the other
efi runtime bits. This only works if the -ffunction-sections and
-fdata-sections flags are passed and the efi runtime functions are
in a section that starts with ".text".

Up to now we had all efi runtime bits in sections that did not
interfere with the normal section naming scheme, but this forces
us to do so. Hence we need to move the efi_loader text/data/rodata
sections before the global *(.text*) catch-all section.

With this patch in place, we should hopefully have an easier time
to extend the efi runtime functionality in the future.

Signed-off-by: Alexander Graf 
---
arch/arm/config.mk|  4 ++--
arch/arm/cpu/armv8/u-boot.lds | 24 +
arch/arm/cpu/u-boot.lds   | 36 ++-
arch/arm/mach-zynq/u-boot.lds | 36 ++-
arch/riscv/cpu/ax25/u-boot.lds| 26 +-
arch/sandbox/config.mk|  3 +++
arch/sandbox/cpu/u-boot.lds   |  9 
arch/x86/config.mk|  2 +-
arch/x86/cpu/u-boot.lds   | 32 ++-
board/qualcomm/dragonboard410c/u-boot.lds | 17 +--
board/qualcomm/dragonboard820c/u-boot.lds | 24 +
board/ti/am335x/u-boot.lds| 36 ++-
include/efi_loader.h  |  4 ++--
13 files changed, 154 insertions(+), 99 deletions(-)


I missed this at the time, probably thinking the subject made it sound
innocuous. There is no 'sandbox:' tag.

This seems to break sandbox in a pretty strange way:

gdb --args /tmp/crosfw/sandbox/u-boot -D
GNU gdb (Debian 7.12-6) 7.12.0.20161007-git
Copyright (C) 2016 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later 
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
.
Find the GDB manual and other documentation resources online at:
.
For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from /tmp/crosfw/sandbox/u-boot...done.
(gdb) r
Starting program: /tmp/crosfw/sandbox/u-boot -D
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

Program received signal SIGSEGV, Segmentation fault.
0x55571520 in open@plt ()
(gdb) up
#1  0x55571e9a in sandbox_read_fdt_from_file ()
at /home/sjg/c/src/third_party/u-boot/files/arch/sandbox/cpu/cpu.c:264
264 fd = os_open(fname, OS_O_RDONLY);
(gdb) print fname
$1 = 0x77ff "/tmp/crosfw/sandbox/u-boot.dtb"
(gdb) q


Also the commit message suggests that this patch changes sandbox to
use --gc-sections, which is not obvious from the subject. I think that
should be a separate commit and in fact it should really be separate
commits for each arch, I think. That might help people notice it...

I only noticed now since the EFI pull request has landed.

Can you try my bss patch really quick? Maybe we're just overwriting gd.

Alex


This patch breaks efi-x86_app_defconfig. The EFI application no longer
boots. I was testing on top of u-boot/master.

If I do:

diff --git a/arch/x86/config.mk b/arch/x86/config.mk
index 586e11a..fc119ec 100644
--- a/arch/x86/config.mk
+++ b/arch/x86/config.mk
@@ -24,7 +24,6 @@ endif
  ifeq ($(IS_32BIT),y)
  PLATFORM_CPPFLAGS += -march=i386 -m32
  # TODO: These break on x86_64; need to debug further
-PLATFORM_RELFLAGS += -fdata-sections
  else
  PLATFORM_CPPFLAGS += $(if $(CONFIG_SPL_BUILD),,-fpic) -fno-common -m64
  endif

Then it boots again. Can you please take a look?

Regards,
Bin

Please can we revert the offending patch quickly for the release? I am
not comfortable with the sandbox changes either (data-sections, etc.).


I agree since it's getting close to the release date.


I have a fix ready for you. Will send it out soon. Basically the linker 
scripts only included .bss, not .bss*.



Alex

Re: [U-Boot] [PATCH] efi_loader: Rename sections to allow for implicit data

[Bin Meng]

On Fri, Aug 17, 2018 at 8:49 PM, Simon Glass  wrote:
> Hi,
>
> On 9 August 2018 at 23:45, Bin Meng  wrote:
>>
>> Hi Alex,
>>
>> On Wed, Aug 8, 2018 at 1:16 AM, Alexander Graf  wrote:
>> >
>> >
>> >> Am 07.08.2018 um 18:12 schrieb Simon Glass :
>> >>
>> >> Hi Alex,
>> >>
>> >>> On 11 June 2018 at 23:48, Alexander Graf  wrote:
>> >>> Some times gcc may generate data that is then used within code that may
>> >>> be part of an efi runtime section. That data could be jump tables,
>> >>> constants or strings.
>> >>>
>> >>> In order to make sure we catch these, we need to ensure that gcc emits
>> >>> them into a section that we can relocate together with all the other
>> >>> efi runtime bits. This only works if the -ffunction-sections and
>> >>> -fdata-sections flags are passed and the efi runtime functions are
>> >>> in a section that starts with ".text".
>> >>>
>> >>> Up to now we had all efi runtime bits in sections that did not
>> >>> interfere with the normal section naming scheme, but this forces
>> >>> us to do so. Hence we need to move the efi_loader text/data/rodata
>> >>> sections before the global *(.text*) catch-all section.
>> >>>
>> >>> With this patch in place, we should hopefully have an easier time
>> >>> to extend the efi runtime functionality in the future.
>> >>>
>> >>> Signed-off-by: Alexander Graf 
>> >>> ---
>> >>> arch/arm/config.mk|  4 ++--
>> >>> arch/arm/cpu/armv8/u-boot.lds | 24 +
>> >>> arch/arm/cpu/u-boot.lds   | 36 
>> >>> ++-
>> >>> arch/arm/mach-zynq/u-boot.lds | 36 
>> >>> ++-
>> >>> arch/riscv/cpu/ax25/u-boot.lds| 26 +-
>> >>> arch/sandbox/config.mk|  3 +++
>> >>> arch/sandbox/cpu/u-boot.lds   |  9 
>> >>> arch/x86/config.mk|  2 +-
>> >>> arch/x86/cpu/u-boot.lds   | 32 
>> >>> ++-
>> >>> board/qualcomm/dragonboard410c/u-boot.lds | 17 +--
>> >>> board/qualcomm/dragonboard820c/u-boot.lds | 24 +
>> >>> board/ti/am335x/u-boot.lds| 36 
>> >>> ++-
>> >>> include/efi_loader.h  |  4 ++--
>> >>> 13 files changed, 154 insertions(+), 99 deletions(-)
>> >>>
>> >>
>> >> I missed this at the time, probably thinking the subject made it sound
>> >> innocuous. There is no 'sandbox:' tag.
>> >>
>> >> This seems to break sandbox in a pretty strange way:
>> >>
>> >> gdb --args /tmp/crosfw/sandbox/u-boot -D
>> >> GNU gdb (Debian 7.12-6) 7.12.0.20161007-git
>> >> Copyright (C) 2016 Free Software Foundation, Inc.
>> >> License GPLv3+: GNU GPL version 3 or later 
>> >> 
>> >> This is free software: you are free to change and redistribute it.
>> >> There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
>> >> and "show warranty" for details.
>> >> This GDB was configured as "x86_64-linux-gnu".
>> >> Type "show configuration" for configuration details.
>> >> For bug reporting instructions, please see:
>> >> .
>> >> Find the GDB manual and other documentation resources online at:
>> >> .
>> >> For help, type "help".
>> >> Type "apropos word" to search for commands related to "word"...
>> >> Reading symbols from /tmp/crosfw/sandbox/u-boot...done.
>> >> (gdb) r
>> >> Starting program: /tmp/crosfw/sandbox/u-boot -D
>> >> [Thread debugging using libthread_db enabled]
>> >> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>> >>
>> >> Program received signal SIGSEGV, Segmentation fault.
>> >> 0x55571520 in open@plt ()
>> >> (gdb) up
>> >> #1  0x55571e9a in sandbox_read_fdt_from_file ()
>> >>at /home/sjg/c/src/third_party/u-boot/files/arch/sandbox/cpu/cpu.c:264
>> >> 264 fd = os_open(fname, OS_O_RDONLY);
>> >> (gdb) print fname
>> >> $1 = 0x77ff "/tmp/crosfw/sandbox/u-boot.dtb"
>> >> (gdb) q
>> >>
>> >>
>> >> Also the commit message suggests that this patch changes sandbox to
>> >> use --gc-sections, which is not obvious from the subject. I think that
>> >> should be a separate commit and in fact it should really be separate
>> >> commits for each arch, I think. That might help people notice it...
>> >>
>> >> I only noticed now since the EFI pull request has landed.
>> >
>> > Can you try my bss patch really quick? Maybe we're just overwriting gd.
>> >
>> > Alex
>> >
>>
>> This patch breaks efi-x86_app_defconfig. The EFI application no longer
>> boots. I was testing on top of u-boot/master.
>>
>> If I do:
>>
>> diff --git a/arch/x86/config.mk b/arch/x86/config.mk
>> index 586e11a..fc119ec 100644
>> --- a/arch/x86/config.mk
>> +++ b/arch/x86/config.mk
>> @@ -24,7 +24,6 @@ endif
>>  ifeq ($(IS_32BIT),y)
>>  PLATFORM_CPPFLAGS += -march=i386 -m32
>>  # TODO: 

Re: [U-Boot] [PATCH] efi_loader: Rename sections to allow for implicit data

[Simon Glass]

Hi,

On 9 August 2018 at 23:45, Bin Meng  wrote:
>
> Hi Alex,
>
> On Wed, Aug 8, 2018 at 1:16 AM, Alexander Graf  wrote:
> >
> >
> >> Am 07.08.2018 um 18:12 schrieb Simon Glass :
> >>
> >> Hi Alex,
> >>
> >>> On 11 June 2018 at 23:48, Alexander Graf  wrote:
> >>> Some times gcc may generate data that is then used within code that may
> >>> be part of an efi runtime section. That data could be jump tables,
> >>> constants or strings.
> >>>
> >>> In order to make sure we catch these, we need to ensure that gcc emits
> >>> them into a section that we can relocate together with all the other
> >>> efi runtime bits. This only works if the -ffunction-sections and
> >>> -fdata-sections flags are passed and the efi runtime functions are
> >>> in a section that starts with ".text".
> >>>
> >>> Up to now we had all efi runtime bits in sections that did not
> >>> interfere with the normal section naming scheme, but this forces
> >>> us to do so. Hence we need to move the efi_loader text/data/rodata
> >>> sections before the global *(.text*) catch-all section.
> >>>
> >>> With this patch in place, we should hopefully have an easier time
> >>> to extend the efi runtime functionality in the future.
> >>>
> >>> Signed-off-by: Alexander Graf 
> >>> ---
> >>> arch/arm/config.mk|  4 ++--
> >>> arch/arm/cpu/armv8/u-boot.lds | 24 +
> >>> arch/arm/cpu/u-boot.lds   | 36 
> >>> ++-
> >>> arch/arm/mach-zynq/u-boot.lds | 36 
> >>> ++-
> >>> arch/riscv/cpu/ax25/u-boot.lds| 26 +-
> >>> arch/sandbox/config.mk|  3 +++
> >>> arch/sandbox/cpu/u-boot.lds   |  9 
> >>> arch/x86/config.mk|  2 +-
> >>> arch/x86/cpu/u-boot.lds   | 32 ++-
> >>> board/qualcomm/dragonboard410c/u-boot.lds | 17 +--
> >>> board/qualcomm/dragonboard820c/u-boot.lds | 24 +
> >>> board/ti/am335x/u-boot.lds| 36 
> >>> ++-
> >>> include/efi_loader.h  |  4 ++--
> >>> 13 files changed, 154 insertions(+), 99 deletions(-)
> >>>
> >>
> >> I missed this at the time, probably thinking the subject made it sound
> >> innocuous. There is no 'sandbox:' tag.
> >>
> >> This seems to break sandbox in a pretty strange way:
> >>
> >> gdb --args /tmp/crosfw/sandbox/u-boot -D
> >> GNU gdb (Debian 7.12-6) 7.12.0.20161007-git
> >> Copyright (C) 2016 Free Software Foundation, Inc.
> >> License GPLv3+: GNU GPL version 3 or later 
> >> 
> >> This is free software: you are free to change and redistribute it.
> >> There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
> >> and "show warranty" for details.
> >> This GDB was configured as "x86_64-linux-gnu".
> >> Type "show configuration" for configuration details.
> >> For bug reporting instructions, please see:
> >> .
> >> Find the GDB manual and other documentation resources online at:
> >> .
> >> For help, type "help".
> >> Type "apropos word" to search for commands related to "word"...
> >> Reading symbols from /tmp/crosfw/sandbox/u-boot...done.
> >> (gdb) r
> >> Starting program: /tmp/crosfw/sandbox/u-boot -D
> >> [Thread debugging using libthread_db enabled]
> >> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
> >>
> >> Program received signal SIGSEGV, Segmentation fault.
> >> 0x55571520 in open@plt ()
> >> (gdb) up
> >> #1  0x55571e9a in sandbox_read_fdt_from_file ()
> >>at /home/sjg/c/src/third_party/u-boot/files/arch/sandbox/cpu/cpu.c:264
> >> 264 fd = os_open(fname, OS_O_RDONLY);
> >> (gdb) print fname
> >> $1 = 0x77ff "/tmp/crosfw/sandbox/u-boot.dtb"
> >> (gdb) q
> >>
> >>
> >> Also the commit message suggests that this patch changes sandbox to
> >> use --gc-sections, which is not obvious from the subject. I think that
> >> should be a separate commit and in fact it should really be separate
> >> commits for each arch, I think. That might help people notice it...
> >>
> >> I only noticed now since the EFI pull request has landed.
> >
> > Can you try my bss patch really quick? Maybe we're just overwriting gd.
> >
> > Alex
> >
>
> This patch breaks efi-x86_app_defconfig. The EFI application no longer
> boots. I was testing on top of u-boot/master.
>
> If I do:
>
> diff --git a/arch/x86/config.mk b/arch/x86/config.mk
> index 586e11a..fc119ec 100644
> --- a/arch/x86/config.mk
> +++ b/arch/x86/config.mk
> @@ -24,7 +24,6 @@ endif
>  ifeq ($(IS_32BIT),y)
>  PLATFORM_CPPFLAGS += -march=i386 -m32
>  # TODO: These break on x86_64; need to debug further
> -PLATFORM_RELFLAGS += -fdata-sections
>  else
>  PLATFORM_CPPFLAGS += $(if $(CONFIG_SPL_BUILD),,-fpic) -fno-common -m64
>  endif
>
> 

Re: [U-Boot] [PATCH] efi_loader: Rename sections to allow for implicit data

[Bin Meng]

Hi Alex,

On Wed, Aug 8, 2018 at 1:16 AM, Alexander Graf  wrote:
>
>
>> Am 07.08.2018 um 18:12 schrieb Simon Glass :
>>
>> Hi Alex,
>>
>>> On 11 June 2018 at 23:48, Alexander Graf  wrote:
>>> Some times gcc may generate data that is then used within code that may
>>> be part of an efi runtime section. That data could be jump tables,
>>> constants or strings.
>>>
>>> In order to make sure we catch these, we need to ensure that gcc emits
>>> them into a section that we can relocate together with all the other
>>> efi runtime bits. This only works if the -ffunction-sections and
>>> -fdata-sections flags are passed and the efi runtime functions are
>>> in a section that starts with ".text".
>>>
>>> Up to now we had all efi runtime bits in sections that did not
>>> interfere with the normal section naming scheme, but this forces
>>> us to do so. Hence we need to move the efi_loader text/data/rodata
>>> sections before the global *(.text*) catch-all section.
>>>
>>> With this patch in place, we should hopefully have an easier time
>>> to extend the efi runtime functionality in the future.
>>>
>>> Signed-off-by: Alexander Graf 
>>> ---
>>> arch/arm/config.mk|  4 ++--
>>> arch/arm/cpu/armv8/u-boot.lds | 24 +
>>> arch/arm/cpu/u-boot.lds   | 36 
>>> ++-
>>> arch/arm/mach-zynq/u-boot.lds | 36 
>>> ++-
>>> arch/riscv/cpu/ax25/u-boot.lds| 26 +-
>>> arch/sandbox/config.mk|  3 +++
>>> arch/sandbox/cpu/u-boot.lds   |  9 
>>> arch/x86/config.mk|  2 +-
>>> arch/x86/cpu/u-boot.lds   | 32 ++-
>>> board/qualcomm/dragonboard410c/u-boot.lds | 17 +--
>>> board/qualcomm/dragonboard820c/u-boot.lds | 24 +
>>> board/ti/am335x/u-boot.lds| 36 
>>> ++-
>>> include/efi_loader.h  |  4 ++--
>>> 13 files changed, 154 insertions(+), 99 deletions(-)
>>>
>>
>> I missed this at the time, probably thinking the subject made it sound
>> innocuous. There is no 'sandbox:' tag.
>>
>> This seems to break sandbox in a pretty strange way:
>>
>> gdb --args /tmp/crosfw/sandbox/u-boot -D
>> GNU gdb (Debian 7.12-6) 7.12.0.20161007-git
>> Copyright (C) 2016 Free Software Foundation, Inc.
>> License GPLv3+: GNU GPL version 3 or later 
>> This is free software: you are free to change and redistribute it.
>> There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
>> and "show warranty" for details.
>> This GDB was configured as "x86_64-linux-gnu".
>> Type "show configuration" for configuration details.
>> For bug reporting instructions, please see:
>> .
>> Find the GDB manual and other documentation resources online at:
>> .
>> For help, type "help".
>> Type "apropos word" to search for commands related to "word"...
>> Reading symbols from /tmp/crosfw/sandbox/u-boot...done.
>> (gdb) r
>> Starting program: /tmp/crosfw/sandbox/u-boot -D
>> [Thread debugging using libthread_db enabled]
>> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>>
>> Program received signal SIGSEGV, Segmentation fault.
>> 0x55571520 in open@plt ()
>> (gdb) up
>> #1  0x55571e9a in sandbox_read_fdt_from_file ()
>>at /home/sjg/c/src/third_party/u-boot/files/arch/sandbox/cpu/cpu.c:264
>> 264 fd = os_open(fname, OS_O_RDONLY);
>> (gdb) print fname
>> $1 = 0x77ff "/tmp/crosfw/sandbox/u-boot.dtb"
>> (gdb) q
>>
>>
>> Also the commit message suggests that this patch changes sandbox to
>> use --gc-sections, which is not obvious from the subject. I think that
>> should be a separate commit and in fact it should really be separate
>> commits for each arch, I think. That might help people notice it...
>>
>> I only noticed now since the EFI pull request has landed.
>
> Can you try my bss patch really quick? Maybe we're just overwriting gd.
>
> Alex
>

This patch breaks efi-x86_app_defconfig. The EFI application no longer
boots. I was testing on top of u-boot/master.

If I do:

diff --git a/arch/x86/config.mk b/arch/x86/config.mk
index 586e11a..fc119ec 100644
--- a/arch/x86/config.mk
+++ b/arch/x86/config.mk
@@ -24,7 +24,6 @@ endif
 ifeq ($(IS_32BIT),y)
 PLATFORM_CPPFLAGS += -march=i386 -m32
 # TODO: These break on x86_64; need to debug further
-PLATFORM_RELFLAGS += -fdata-sections
 else
 PLATFORM_CPPFLAGS += $(if $(CONFIG_SPL_BUILD),,-fpic) -fno-common -m64
 endif

Then it boots again. Can you please take a look?

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

Re: [U-Boot] [PATCH] efi_loader: Rename sections to allow for implicit data

[Alexander Graf]

> Am 07.08.2018 um 18:12 schrieb Simon Glass :
> 
> Hi Alex,
> 
>> On 11 June 2018 at 23:48, Alexander Graf  wrote:
>> Some times gcc may generate data that is then used within code that may
>> be part of an efi runtime section. That data could be jump tables,
>> constants or strings.
>> 
>> In order to make sure we catch these, we need to ensure that gcc emits
>> them into a section that we can relocate together with all the other
>> efi runtime bits. This only works if the -ffunction-sections and
>> -fdata-sections flags are passed and the efi runtime functions are
>> in a section that starts with ".text".
>> 
>> Up to now we had all efi runtime bits in sections that did not
>> interfere with the normal section naming scheme, but this forces
>> us to do so. Hence we need to move the efi_loader text/data/rodata
>> sections before the global *(.text*) catch-all section.
>> 
>> With this patch in place, we should hopefully have an easier time
>> to extend the efi runtime functionality in the future.
>> 
>> Signed-off-by: Alexander Graf 
>> ---
>> arch/arm/config.mk|  4 ++--
>> arch/arm/cpu/armv8/u-boot.lds | 24 +
>> arch/arm/cpu/u-boot.lds   | 36 
>> ++-
>> arch/arm/mach-zynq/u-boot.lds | 36 
>> ++-
>> arch/riscv/cpu/ax25/u-boot.lds| 26 +-
>> arch/sandbox/config.mk|  3 +++
>> arch/sandbox/cpu/u-boot.lds   |  9 
>> arch/x86/config.mk|  2 +-
>> arch/x86/cpu/u-boot.lds   | 32 ++-
>> board/qualcomm/dragonboard410c/u-boot.lds | 17 +--
>> board/qualcomm/dragonboard820c/u-boot.lds | 24 +
>> board/ti/am335x/u-boot.lds| 36 
>> ++-
>> include/efi_loader.h  |  4 ++--
>> 13 files changed, 154 insertions(+), 99 deletions(-)
>> 
> 
> I missed this at the time, probably thinking the subject made it sound
> innocuous. There is no 'sandbox:' tag.
> 
> This seems to break sandbox in a pretty strange way:
> 
> gdb --args /tmp/crosfw/sandbox/u-boot -D
> GNU gdb (Debian 7.12-6) 7.12.0.20161007-git
> Copyright (C) 2016 Free Software Foundation, Inc.
> License GPLv3+: GNU GPL version 3 or later 
> This is free software: you are free to change and redistribute it.
> There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
> and "show warranty" for details.
> This GDB was configured as "x86_64-linux-gnu".
> Type "show configuration" for configuration details.
> For bug reporting instructions, please see:
> .
> Find the GDB manual and other documentation resources online at:
> .
> For help, type "help".
> Type "apropos word" to search for commands related to "word"...
> Reading symbols from /tmp/crosfw/sandbox/u-boot...done.
> (gdb) r
> Starting program: /tmp/crosfw/sandbox/u-boot -D
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
> 
> Program received signal SIGSEGV, Segmentation fault.
> 0x55571520 in open@plt ()
> (gdb) up
> #1  0x55571e9a in sandbox_read_fdt_from_file ()
>at /home/sjg/c/src/third_party/u-boot/files/arch/sandbox/cpu/cpu.c:264
> 264 fd = os_open(fname, OS_O_RDONLY);
> (gdb) print fname
> $1 = 0x77ff "/tmp/crosfw/sandbox/u-boot.dtb"
> (gdb) q
> 
> 
> Also the commit message suggests that this patch changes sandbox to
> use --gc-sections, which is not obvious from the subject. I think that
> should be a separate commit and in fact it should really be separate
> commits for each arch, I think. That might help people notice it...
> 
> I only noticed now since the EFI pull request has landed.

Can you try my bss patch really quick? Maybe we're just overwriting gd.

Alex

> 
> Hopefully there is no impact on all the x86 builds.
> 
> Regards,
> Simon

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] efi_loader: Rename sections to allow for implicit data

[Simon Glass]

Hi Alex,

On 11 June 2018 at 23:48, Alexander Graf  wrote:
> Some times gcc may generate data that is then used within code that may
> be part of an efi runtime section. That data could be jump tables,
> constants or strings.
>
> In order to make sure we catch these, we need to ensure that gcc emits
> them into a section that we can relocate together with all the other
> efi runtime bits. This only works if the -ffunction-sections and
> -fdata-sections flags are passed and the efi runtime functions are
> in a section that starts with ".text".
>
> Up to now we had all efi runtime bits in sections that did not
> interfere with the normal section naming scheme, but this forces
> us to do so. Hence we need to move the efi_loader text/data/rodata
> sections before the global *(.text*) catch-all section.
>
> With this patch in place, we should hopefully have an easier time
> to extend the efi runtime functionality in the future.
>
> Signed-off-by: Alexander Graf 
> ---
>  arch/arm/config.mk|  4 ++--
>  arch/arm/cpu/armv8/u-boot.lds | 24 +
>  arch/arm/cpu/u-boot.lds   | 36 
> ++-
>  arch/arm/mach-zynq/u-boot.lds | 36 
> ++-
>  arch/riscv/cpu/ax25/u-boot.lds| 26 +-
>  arch/sandbox/config.mk|  3 +++
>  arch/sandbox/cpu/u-boot.lds   |  9 
>  arch/x86/config.mk|  2 +-
>  arch/x86/cpu/u-boot.lds   | 32 ++-
>  board/qualcomm/dragonboard410c/u-boot.lds | 17 +--
>  board/qualcomm/dragonboard820c/u-boot.lds | 24 +
>  board/ti/am335x/u-boot.lds| 36 
> ++-
>  include/efi_loader.h  |  4 ++--
>  13 files changed, 154 insertions(+), 99 deletions(-)
>

I missed this at the time, probably thinking the subject made it sound
innocuous. There is no 'sandbox:' tag.

This seems to break sandbox in a pretty strange way:

 gdb --args /tmp/crosfw/sandbox/u-boot -D
GNU gdb (Debian 7.12-6) 7.12.0.20161007-git
Copyright (C) 2016 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later 
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
.
Find the GDB manual and other documentation resources online at:
.
For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from /tmp/crosfw/sandbox/u-boot...done.
(gdb) r
Starting program: /tmp/crosfw/sandbox/u-boot -D
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

Program received signal SIGSEGV, Segmentation fault.
0x55571520 in open@plt ()
(gdb) up
#1  0x55571e9a in sandbox_read_fdt_from_file ()
at /home/sjg/c/src/third_party/u-boot/files/arch/sandbox/cpu/cpu.c:264
264 fd = os_open(fname, OS_O_RDONLY);
(gdb) print fname
$1 = 0x77ff "/tmp/crosfw/sandbox/u-boot.dtb"
(gdb) q


Also the commit message suggests that this patch changes sandbox to
use --gc-sections, which is not obvious from the subject. I think that
should be a separate commit and in fact it should really be separate
commits for each arch, I think. That might help people notice it...

I only noticed now since the EFI pull request has landed.

Hopefully there is no impact on all the x86 builds.

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH] efi_loader: Rename sections to allow for implicit data

[Alexander Graf]

Some times gcc may generate data that is then used within code that may
be part of an efi runtime section. That data could be jump tables,
constants or strings.

In order to make sure we catch these, we need to ensure that gcc emits
them into a section that we can relocate together with all the other
efi runtime bits. This only works if the -ffunction-sections and
-fdata-sections flags are passed and the efi runtime functions are
in a section that starts with ".text".

Up to now we had all efi runtime bits in sections that did not
interfere with the normal section naming scheme, but this forces
us to do so. Hence we need to move the efi_loader text/data/rodata
sections before the global *(.text*) catch-all section.

With this patch in place, we should hopefully have an easier time
to extend the efi runtime functionality in the future.

Signed-off-by: Alexander Graf 
---
 arch/arm/config.mk|  4 ++--
 arch/arm/cpu/armv8/u-boot.lds | 24 +
 arch/arm/cpu/u-boot.lds   | 36 ++-
 arch/arm/mach-zynq/u-boot.lds | 36 ++-
 arch/riscv/cpu/ax25/u-boot.lds| 26 +-
 arch/sandbox/config.mk|  3 +++
 arch/sandbox/cpu/u-boot.lds   |  9 
 arch/x86/config.mk|  2 +-
 arch/x86/cpu/u-boot.lds   | 32 ++-
 board/qualcomm/dragonboard410c/u-boot.lds | 17 +--
 board/qualcomm/dragonboard820c/u-boot.lds | 24 +
 board/ti/am335x/u-boot.lds| 36 ++-
 include/efi_loader.h  |  4 ++--
 13 files changed, 154 insertions(+), 99 deletions(-)

diff --git a/arch/arm/config.mk b/arch/arm/config.mk
index efafc69d1b..f25603109e 100644
--- a/arch/arm/config.mk
+++ b/arch/arm/config.mk
@@ -134,11 +134,11 @@ endif
 ifdef CONFIG_ARM64
 OBJCOPYFLAGS += -j .text -j .secure_text -j .secure_data -j .rodata -j .data \
-j .u_boot_list -j .rela.dyn -j .got -j .got.plt \
-   -j .binman_sym_table
+   -j .binman_sym_table -j .text_rest
 else
 OBJCOPYFLAGS += -j .text -j .secure_text -j .secure_data -j .rodata -j .hash \
-j .data -j .got -j .got.plt -j .u_boot_list -j .rel.dyn \
-   -j .binman_sym_table
+   -j .binman_sym_table -j .text_rest
 endif
 
 # if a dtb section exists we always have to include it
diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
index eb926b3c14..53de80f745 100644
--- a/arch/arm/cpu/armv8/u-boot.lds
+++ b/arch/arm/cpu/armv8/u-boot.lds
@@ -25,6 +25,19 @@ SECTIONS
{
*(.__image_copy_start)
CPUDIR/start.o (.text*)
+   }
+
+   /* This needs to come before *(.text*) */
+   .efi_runtime : {
+__efi_runtime_start = .;
+   *(.text.efi_runtime*)
+   *(.rodata.efi_runtime*)
+   *(.data.efi_runtime*)
+__efi_runtime_stop = .;
+   }
+
+   .text_rest :
+   {
*(.text*)
}
 
@@ -98,17 +111,10 @@ SECTIONS
 
. = ALIGN(8);
 
-   .efi_runtime : {
-__efi_runtime_start = .;
-   *(efi_runtime_text)
-   *(efi_runtime_data)
-__efi_runtime_stop = .;
-   }
-
.efi_runtime_rel : {
 __efi_runtime_rel_start = .;
-   *(.relaefi_runtime_text)
-   *(.relaefi_runtime_data)
+   *(.rel*.efi_runtime)
+   *(.rel*.efi_runtime.*)
 __efi_runtime_rel_stop = .;
}
 
diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
index 4157374d21..834dc99554 100644
--- a/arch/arm/cpu/u-boot.lds
+++ b/arch/arm/cpu/u-boot.lds
@@ -43,6 +43,25 @@ SECTIONS
*(.__image_copy_start)
*(.vectors)
CPUDIR/start.o (.text*)
+   }
+
+   /* This needs to come before *(.text*) */
+   .__efi_runtime_start : {
+   *(.__efi_runtime_start)
+   }
+
+   .efi_runtime : {
+   *(.text.efi_runtime*)
+   *(.rodata.efi_runtime*)
+   *(.data.efi_runtime*)
+   }
+
+   .__efi_runtime_stop : {
+   *(.__efi_runtime_stop)
+   }
+
+   .text_rest :
+   {
*(.text*)
}
 
@@ -136,27 +155,14 @@ SECTIONS
 
. = ALIGN(4);
 
-   .__efi_runtime_start : {
-   *(.__efi_runtime_start)
-   }
-
-   .efi_runtime : {
-   *(efi_runtime_text)
-   *(efi_runtime_data)
-   }
-
-   .__efi_runtime_stop : {
-   *(.__efi_runtime_stop)
-   }
-
.efi_runtime_rel_start :
{
*(.__efi_runtime_rel_start)
}
 
.efi_runtime_rel : {
-   *(.relefi_runtime_text)
-