[PATCH 01/27] linker_lists: Fix alignment issue

[Heinrich Schuchardt]

On 12/1/20 4:58 PM, Simon Glass wrote:
> Hi Heinrich,
>
> On Mon, 30 Nov 2020 at 15:56, Heinrich Schuchardt <xypron.glpk at 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 at gmx.de> wrote:
>>>>
>>>> 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?
>>>
>>> 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
>