[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 <sjg at chromium.org> wrote:
> Hi,
>
> On 9 August 2018 at 23:45, Bin Meng <bmeng.cn at gmail.com> wrote:
>>
>> Hi Alex,
>>
>> On Wed, Aug 8, 2018 at 1:16 AM, Alexander Graf <agraf at suse.de> wrote:
>> >
>> >
>> >> Am 07.08.2018 um 18:12 schrieb Simon Glass <sjg at chromium.org>:
>> >>
>> >> Hi Alex,
>> >>
>> >>> On 11 June 2018 at 23:48, Alexander Graf <agraf at suse.de> 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 <agraf at suse.de>
>> >>> ---
>> >>> 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 <http://gnu.org/licenses/gpl.html>
>> >> 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:
>> >> <http://www.gnu.org/software/gdb/bugs/>.
>> >> Find the GDB manual and other documentation resources online at:
>> >> <http://www.gnu.org/software/gdb/documentation/>.
>> >> 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.
>> >> 0x0000555555571520 in open at plt ()
>> >> (gdb) up
>> >> #1  0x0000555555571e9a 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 = 0x7ffff7ff0000 "/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.

Regards,
Bin