From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Wed, 21 Apr 2021 19:14:36 +1200 Subject: [PATCH v3 01/28] linker_lists: Fix alignment issue In-Reply-To: <9e8647d4-ce8d-2b10-0f6c-23d011f614a8@gmx.de> References: <20201217042034.411902-1-sjg@chromium.org> <20201216212001.v3.1.Ie3fa227cbe539fa6742fa1e647093557a2b9b8ee@changeid> <9e8647d4-ce8d-2b10-0f6c-23d011f614a8@gmx.de> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Heinrich, On Thu, 15 Apr 2021 at 19:39, Heinrich Schuchardt 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 > > --- > > > > (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; \ > > }) > > >