[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