[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 <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.

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


Alex