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

Tue Aug 21 19:26:53 UTC 2018

On 21.08.18 19:30, Simon Glass wrote:
> Hi Alex,
> 
> On 20 August 2018 at 06:23, Alexander Graf <agraf at suse.de> wrote:
>>
>> On 08/17/2018 02:49 PM, Simon Glass 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 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?
> 
> I would like to revert the sandbox changes at least. I don't want to
> enable -ffunction-sections, for example.

Could you please explain why? In general I always thought the sandbox
target was meant as debugging aid which allows you to find and debug
bugs more easily.

I would assume that chances for breakage are higher with function and
data sections, because the linker could remove code it considers dead?
So for a debugging target, I would think it makes sense to have it
enabled rather than disabled.

Alex