Re: [PATCH v11 03/29] linker_lists: set LINKER_LIST_ALIGN to 8 for CPU_MIPS64

[Jerome Forissier <jerome.forissier@linaro.org>]

On 10/4/24 20:28, Tom Rini wrote:
> On Fri, Oct 04, 2024 at 02:55:58PM +0200, Jerome Forissier wrote:
>>
>>
>> On 10/4/24 14:04, Heinrich Schuchardt wrote:
>>> On 03.10.24 17:22, Jerome Forissier wrote:
>>>> Note: Patch posted separately [0].
>>>>
>>>> [0] https://patchwork.ozlabs.org/project/uboot/patch/20241003142030.1610222-1-jerome.forissier@linaro.org/
>>>>
>>>> CPU_MIPS64 needs 8-byte alignment on the linker lists, otherwise an
>>>> exception may occur. Fixes an issue found on malta64 with QEMU:
>>>>
>>>>   Breakpoint 1, lists_driver_lookup_name (name=0xffffffffbe043578 "root_driver") at /home/uboot/u-boot/drivers/core/lists.c:31
>>>>   31                      if (!strcmp(name, entry->name))
>>>>   [...]
>>>>   ld      a1,0(s0)
>>>>
>>>>   (gdb) p/x &entry->name
>>>>   0xffffffffbe04b0d4
>>>>   (gdb) p/x $s0
>>>>   0xffffffffbe04b0d4
>>>>
>>>>   $ grep __u_boot_list /tmp/malta64/u-boot.objdump
>>>>   4 __u_boot_list 000018e0  ffffffffbe04a4d4  ffffffffbe04a4d4  0004a584  2**2
>>>>
>>>> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
>>>> ---
>>>>   arch/Kconfig | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/Kconfig b/arch/Kconfig
>>>> index 8f1f4667012..8f4df849801 100644
>>>> --- a/arch/Kconfig
>>>> +++ b/arch/Kconfig
>>>> @@ -45,7 +45,7 @@ config SYS_CACHELINE_SIZE
>>>>   config LINKER_LIST_ALIGN
>>>>       int
>>>>       default 32 if SANDBOX
>>>> -    default 8 if ARM64 || X86
>>>> +    default 8 if ARM64 || X86 || CPU_MIPS64
>>>
>>> Shouldn't we set 8 byte alignment on all 64bit architectures including
>>> riscv64?
>>>
>>>     default 8 if 64BIT
>>
>> Makes sense.
>>
>>>
>>> @Simon
>>> I would not know why 32bit X86 should need 8 byte alignment.
>>> I am a bit astonished that you chose 32 byte alignment for the Sandbox.
>>> Is there any justification to use more than 4 on the 32bit Sandbox and
>>> more than 8 on the 64bit Sandbox? If yes, we should document it via a
>>> comment.
>>
>> I tried running what CI does manually (.azure_pipelines.yml) with
>> "default 32 if SANDBOX" removed, and I got SIGSEGV with
>> TEST_PY_BD=sandbox. OTOH, TEST_PY_BD=sandbox64 worked fine but it might
>> have been a fluke.
> 
> I think we can leave sandbox as a future cleanup, and perhaps file an
> issue at https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues/ so
> it doesn't get lost.

https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues/29

Thanks,
-- 
Jerome