[PATCH 01/27] linker_lists: Fix alignment issue

[Simon Glass]

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

I think we need a test, as you say, but it needs to go one step
further from what you have here, I think.

Regards,
Simon