[PATCH 01/27] linker_lists: Fix alignment issue
Simon Glass
sjg at chromium.org
Thu Dec 10 20:26:23 CET 2020
Hi Heinrich,
On Wed, 9 Dec 2020 at 11:22, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 12/1/20 4:58 PM, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Mon, 30 Nov 2020 at 15:56, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> >>
> >> On 11/30/20 9:11 PM, Simon Glass wrote:
> >>> +Marek Vasut who originally wrote it
> >>>
> >>> Hi Heinrich,
> >>>
> >>> On Sun, 29 Nov 2020 at 23:20, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> >>>>
> >>>> Am 30. November 2020 02:53:36 MEZ schrieb Simon Glass <sjg at chromium.org>:
> >>>>> The linker script uses alphabetic sorting to group the different linker
> >>>>> lists together. Each group has its own struct and potentially its own
> >>>>> alignment. But when the linker packs the structs together it cannot
> >>>>> ensure that a linker list starts on the expected alignment boundary.
> >>>>>
> >>>>> For example, if the first list has a struct size of 8 and we place 3 of
> >>>>> them in the image, that means that the next struct will start at offset
> >>>>> 0x18 from the start of the linker_list section. If the next struct has
> >>>>> a size of 16 then it will start at an 8-byte aligned offset, but not a
> >>>>> 16-byte aligned offset.
> >>>>>
> >>>>> With sandbox on x86_64, a reference to a linker list item using
> >>>>> ll_entry_get() can force alignment of that particular linker_list item,
> >>>>> if it is in the same file as the linker_list item is declared.
> >>>>>
> >>>>> Consider this example, where struct driver is 0x80 bytes:
> >>>>>
> >>>>> ll_entry_declare(struct driver, fred, driver)
> >>>>>
> >>>>> ...
> >>>>>
> >>>>> void *p = ll_entry_get(struct driver, fred, driver)
> >>>>>
> >>>>> If these two lines of code are in the same file, then the entry is
> >>>>> forced
> >>>>> to be aligned at the 'struct driver' alignment, which is 16 bytes. If
> >>>>> the
> >>>>> second line of code is in a different file, then no action is taken,
> >>>>> since
> >>>>> the compiler cannot update the alignment of the linker_list item.
> >>>>>
> >>>>> In the first case, an 8-byte 'fill' region is added:
> >>>>>
> >>>>> .u_boot_list_2_driver_2_testbus_drv
> >>>>> 0x0000000000270018 0x80 test/built-in.o
> >>>>> 0x0000000000270018
> >>>>> _u_boot_list_2_driver_2_testbus_drv
> >>>>> .u_boot_list_2_driver_2_testfdt1_drv
> >>>>> 0x0000000000270098 0x80 test/built-in.o
> >>>>> 0x0000000000270098
> >>>>> _u_boot_list_2_driver_2_testfdt1_drv
> >>>>> *fill* 0x0000000000270118 0x8
> >>>>> .u_boot_list_2_driver_2_testfdt_drv
> >>>>> 0x0000000000270120 0x80 test/built-in.o
> >>>>> 0x0000000000270120
> >>>>> _u_boot_list_2_driver_2_testfdt_drv
> >>>>> .u_boot_list_2_driver_2_testprobe_drv
> >>>>> 0x00000000002701a0 0x80 test/built-in.o
> >>>>> 0x00000000002701a0
> >>>>> _u_boot_list_2_driver_2_testprobe_drv
> >>>>>
> >>>>> With this, the linker_list no-longer works since items after
> >>>>> testfdt1_drv
> >>>>> are not at the expected address.
> >>>>>
> >>>>> Ideally we would have a way to tell gcc not to align structs in this
> >>>>> way.
> >>>>> It is not clear how we could do this, and in any case it would require
> >>>>> us
> >>>>> to adjust every struct used by the linker_list feature.
> >>>>>
> >>>>> One possible fix is to force each separate linker_list to start on the
> >>>>> largest possible boundary that can be required by the compiler. However
> >>>>> that does not seem to work on x86_64, which uses 16-byte alignment in
> >>>>> this
> >>>>> case but needs 32-byte alignment.
> >>>>>
> >>>>> So add a Kconfig option to handle this. Set the default value to 4 so
> >>>>> as to avoid changing platforms that don't need it.
> >>>>>
> >>>>> Update the ll_entry_start() accordingly.
> >>>>>
> >>>>> Signed-off-by: Simon Glass <sjg at chromium.org>
> >>>>> ---
> >>>>>
> >>>>> arch/Kconfig | 11 +++++++
> >>>>> doc/api/linker_lists.rst | 62 ++++++++++++++++++++++++++++++++++++++++
> >>>>> include/linker_lists.h | 3 +-
> >>>>> 3 files changed, 75 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/arch/Kconfig b/arch/Kconfig
> >>>>> index 3aa99e08fce..aa8664212f1 100644
> >>>>> --- a/arch/Kconfig
> >>>>> +++ b/arch/Kconfig
> >>>>> @@ -7,6 +7,17 @@ config HAVE_ARCH_IOREMAP
> >>>>> config NEEDS_MANUAL_RELOC
> >>>>> bool
> >>>>>
> >>>>> +config LINKER_LIST_ALIGN
> >>>>> + int
> >>>>> + default 32 if SANDBOX
> >>>>
> >>>> What is so special about the sandbox?
> >>>
> >>> I'm not too sure, actually. Also, 32 seems to be larger than
> >>> __BIGGEST_ALIGNMENT__ so it is confusing.
> >>>
> >>>> Just evaluate if the host is 64 bit and use 8 or 4 accordingly?
> >>>>
> >>>>> + default 8 if ARM64 || X86
> >>>>
> >>>> Shouldn't the default be 8 on all 64 bit platforms? And 4 on all 32 bit platforms?
> >>>
> >>> Possibly, but who knows? One way to really get to the bottom of this
> >>> is to have a test that checks that the alignment is what it should be.
> >>> I spent half a day diagnosing this but not that much time thinking of
> >>> the best solution. If you have time to dig into it please let me know.
> >>>
> >>> Regards,
> >>> Simon
> >>>
> >>
> >> If you change ll_entry_start() as below, the linker will complain
> >> "lib/efi_driver/efi_uclass.c:309:
> >> undefined reference to `bad_alignment'"
> >>
> >> If you change value 4 to 8, it stops complaining for qemu-x86_64_defconfig.
> >>
> >> #define ll_alignment(x) \
> >> (__builtin_offsetof(struct {char a; x b;}, b))
> >> void bad_alignment(void);
> >> #define ll_entry_start(_type, _list) \
> >> ({ \
> >> static char start[0] __aligned(4) __attribute__((unused, \
> >> section(".u_boot_list_2_"#_list"_1"))); \
> >> if (ll_alignment(_type) > 4) \
> >> bad_alignment(); \
> >> (_type *)&start; \
> >> })
> >>
> >> If the alignment is smaller than the limit, the compiler can remove the
> >> bad_alignment() call due to the optimization setting -Os (or -O2).
> >>
> >> For RISC-V 64bit you also need 8 byte alignment.
> >>
> >> I suggest that you replace 4 by sizeof(long) and run the change through
> >> Gitlab to validate that 8 bytes on 64-bit systems and 4 bytes on 32-bit
> >> systems are sufficient alignment.
> >>
> >> I did not check if any linker lists are accessed via other means then
> >> ll_entry_start().
> >
> > Thanks. You would think that would be enough, but somehow
> > ll_entry_get() seems to make things worse.
> >
> > I'm not sure how to do sizeof(long) in Kconfig
>
> Linux defines a symbol 64BIT in arch/ for all architectures, e.g.
>
> arch/x86/Kconfig:3:config 64BIT
> arch/x86/Kconfig-4- bool "64-bit kernel" if "$(ARCH)" = "x86"
> arch/x86/Kconfig-5- default "$(ARCH)" != "i386"
>
> U-Boot has it only for two architectures:
>
> arch/mips/Kconfig:501:config 64BIT
> arch/riscv/Kconfig:152:config 64BIT
Yes indeed.
But I haven't actually found documentation that explains what is going
on with alignment. It is hard to define rules when we don't know the
behaviour.
https://docs.adacore.com/live/wave/binutils-stable/html/ld/ld.html
Regards,
Simon
More information about the U-Boot
mailing list