Hi Heinrich, On Wed, 9 Dec 2020 at 11:22, Heinrich Schuchardt <xypron.g...@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.g...@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.g...@gmx.de> > >>> wrote: > >>>> > >>>> Am 30. November 2020 02:53:36 MEZ schrieb Simon Glass > >>>> <s...@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 <s...@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