[PATCH v3 01/28] linker_lists: Fix alignment issue
Simon Glass
sjg at chromium.org
Wed Apr 21 09:14:36 CEST 2021
Hi Heinrich,
On Thu, 15 Apr 2021 at 19:39, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 17.12.20 05:20, Simon Glass wrote:
> > 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>
> > ---
> >
> > (no changes since v1)
> >
> > arch/Kconfig | 11 ++++++++
> > doc/api/linker_lists.rst | 59 ++++++++++++++++++++++++++++++++++++++++
> > include/linker_lists.h | 3 +-
> > 3 files changed, 72 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/Kconfig b/arch/Kconfig
> > index e8f9a9e1b77..27843cd79c4 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
>
> On which host architecture would the sandbox actually require 32 bytes?
x86_64, for example.
>
> Please, use an alignment based on the bitness of the generated binary to
> get relevant test results.
>
> > + default 8 if ARM64 || X86
>
> Except for jmp_buf the maximum alignment requirement on i386 is 4 and on
> amd64 it is 8.
>
> > + default 4
Please do check the above commit message. This is a real problem and
causes U-Boot to crash. If you have a better idea how to fix it, I am
all ears, as I don't like this either.
>
> This defies the alignment requirements of 64 bit systems like RV64, mips64.
>
> The code must work on *all* architectures.
The status quo is 4 as you can see, so I am only actually changing
sandbox. I have not seen this problem on other archs, although I
wouldn't rule it out.
Regards,
Simon
>
> > + help
> > + Force the each linker list to be aligned to this boundary. This
> > + is required if ll_entry_get() is used, since otherwise the linker
> > + may add padding into the table, thus breaking it.
> > + See linker_lists.rst for full details.
> > +
> > choice
> > prompt "Architecture select"
> > default SANDBOX
> > diff --git a/doc/api/linker_lists.rst b/doc/api/linker_lists.rst
> > index 72f514e0ac0..7063fdc8314 100644
> > --- a/doc/api/linker_lists.rst
> > +++ b/doc/api/linker_lists.rst
> > @@ -96,5 +96,64 @@ defined for the whole list and each sub-list:
> > %u_boot_list_2_drivers_2_pci_3
> > %u_boot_list_2_drivers_3
> >
> > +Alignment issues
> > +----------------
> > +
> > +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.
> > +
> > +The simplest fix seems to be to force each separate linker_list to start
> > +on the largest possible boundary that can be required by the compiler. This
> > +is the purpose of CONFIG_LINKER_LIST_ALIGN
> > +
> > +
> > .. kernel-doc:: include/linker_lists.h
> > :internal:
> > diff --git a/include/linker_lists.h b/include/linker_lists.h
> > index d775d041e04..fd98ecd297c 100644
> > --- a/include/linker_lists.h
> > +++ b/include/linker_lists.h
> > @@ -124,7 +124,8 @@
> > */
> > #define ll_entry_start(_type, _list) \
> > ({ \
> > - static char start[0] __aligned(4) __attribute__((unused, \
> > + static char start[0] __aligned(CONFIG_LINKER_LIST_ALIGN) \
> > + __attribute__((unused, \
> > section(".u_boot_list_2_"#_list"_1"))); \
> > (_type *)&start; \
> > })
> >
>
More information about the U-Boot
mailing list