[PATCH 01/27] linker_lists: Fix alignment issue

[Heinrich Schuchardt]

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().

Best regards

Heinrich