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? 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? Best regards Heinrich >+ default 4 >+ 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..7a37db52ba8 100644 >--- a/doc/api/linker_lists.rst >+++ b/doc/api/linker_lists.rst >@@ -96,5 +96,67 @@ 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; \ > })