| Message ID | 1699de5384a71c0d0ad535cdc48cc2b95087719d.1727968902.git.jerome.forissier@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce the lwIP network stack | expand |
On Thu, 3 Oct 2024 at 18:23, Jerome Forissier <jerome.forissier@linaro.org> 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 > default 4 > help > Force the each linker list to be aligned to this boundary. This > -- > 2.40.1 > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
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 @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. Best regards Heinrich > default 4 > help > Force the each linker list to be aligned to this boundary. This
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. Thanks,
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.
Hi Heinrich, On Fri, 4 Oct 2024 at 06:16, Heinrich Schuchardt <xypron.glpk@gmx.de> 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 > > @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. It was really long so I put it in the commit message. Please check 'git blame' as there is often stuff there: linker_lists: Fix alignment issue 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. Regards, Simon
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,
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 default 4 help Force the each linker list to be aligned to this boundary. This
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(-)