[PATCH 01/27] linker_lists: Fix alignment issue

[Simon Glass]

+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