[PATCH 01/27] linker_lists: Fix alignment issue

[Heinrich Schuchardt]

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?
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;						\
> })