| Message ID | 20201129185331.1.Ie3fa227cbe539fa6742fa1e647093557a2b9b8ee@changeid |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | dm: Change the way sequence numbers are implemented | expand |
Am 30. November 2020 02:53:36 MEZ schrieb Simon Glass <sjg@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@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; \ > })
+Marek Vasut who originally wrote it Hi Heinrich, On Sun, 29 Nov 2020 at 23:20, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > Am 30. November 2020 02:53:36 MEZ schrieb Simon Glass <sjg@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@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
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.glpk@gmx.de> wrote: >> >> Am 30. November 2020 02:53:36 MEZ schrieb Simon Glass <sjg@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@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(). Best regards Heinrich
Hi Heinrich, On Mon, 30 Nov 2020 at 15:56, Heinrich Schuchardt <xypron.glpk@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.glpk@gmx.de> wrote: > >> > >> Am 30. November 2020 02:53:36 MEZ schrieb Simon Glass <sjg@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@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 I think we need a test, as you say, but it needs to go one step further from what you have here, I think. Regards, Simon
On 12/1/20 4:58 PM, Simon Glass wrote: > Hi Heinrich, > > On Mon, 30 Nov 2020 at 15:56, Heinrich Schuchardt <xypron.glpk@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.glpk@gmx.de> wrote: >>>> >>>> Am 30. November 2020 02:53:36 MEZ schrieb Simon Glass <sjg@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@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 Best regards Heinrich > > I think we need a test, as you say, but it needs to go one step > further from what you have here, I think. > > Regards, > Simon >
Hi Heinrich, On Wed, 9 Dec 2020 at 11:22, Heinrich Schuchardt <xypron.glpk@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.glpk@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.glpk@gmx.de> wrote: > >>>> > >>>> Am 30. November 2020 02:53:36 MEZ schrieb Simon Glass <sjg@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@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
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 + default 8 if ARM64 || X86 + 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; \ })
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@chromium.org> --- arch/Kconfig | 11 +++++++ doc/api/linker_lists.rst | 62 ++++++++++++++++++++++++++++++++++++++++ include/linker_lists.h | 3 +- 3 files changed, 75 insertions(+), 1 deletion(-)