Skip to content

Conversation

@sylvioalves
Copy link
Contributor

When TLS is used, __tdata_start is PROVIDED by
"thread-local-storage.ld" using absolute address, which makes it land in wrong flash address. This causes risc-v startup code to fail during memcpy/memset.

This PR overrides __tdata_start to use ADDR, which will make sure it is placed in DROM region due to AT keyword.

Fixes #69708

@zephyrbot zephyrbot added the platform: ESP32 Espressif ESP32 label May 12, 2024
@zephyrbot zephyrbot requested a review from uLipe May 12, 2024 23:41
@keith-packard
Copy link
Contributor

This looks wrong to me -- LOADADDR(__tdata_start) is where the values for the initialized thread local data can be found in ROM while ADDR(__tdata_start) is a fake address used as a part of the thread local storage offset computations; it should match the reported base of the tdata section. TLS offsets are computed during linking by subtracting the fake address of the symbol from the base of the tdata section; those offsets are what are used to fetch values relative to the thread local storage base address (stored in the tp register on risc-v).

When initializing a newly allocated TLS block for a new thread, Zephyr copies bits from ROM into the parts of the new block corresponding to the initialized portion and then clears the portion corresponding to the zero-initialized portion.

Your patch seems like it will pull random data from RAM starting at the fake __tdata_start address instead of the correct values from ROM starting at LOADADDR(__tdata_start).

I'm betting we can't simply use memcpy from ROM on this chip and that we need to do something else to get the initialized values out of ROM and into each TLS block.

To check and make sure your fix works, you can try this version of samples/hello_world/src/main.c:

/*
 * Copyright (c) 2012-2014 Wind River Systems, Inc.
 *
 * SPDX-License-Identifier: Apache-2.0
 */

#include <stdio.h>

#define TLS_DATA_VALUE 42

__thread int tls_data_variable = TLS_DATA_VALUE;

int main(void)
{
	if (tls_data_variable == TLS_DATA_VALUE) {
		printf("SUCCESS\n");
	} else {
		printf("FAILURE: tls_data_variable is %d instead of %d\n", tls_data_variable, TLS_DATA_VALUE);
	}
	return 0;
}

@lgl88911
Copy link
Member

lgl88911 commented May 13, 2024

This looks wrong to me -- LOADADDR(__tdata_start) is where the values for the initialized thread local data can be found in ROM while ADDR(__tdata_start) is a fake address used as a part of the thread local storage offset computations; it should match the reported base of the tdata section. TLS offsets are computed during linking by subtracting the fake address of the symbol from the base of the tdata section; those offsets are what are used to fetch values relative to the thread local storage base address (stored in the tp register on risc-v).这在我看来是错误的 - LOADADDR(__tdata_start) 是可以在 ROM 中找到初始化线程本地数据的值的位置,而 ADDR(__tdata_start) 是用作线程本地存储的一部分的假地址偏移计算;它应该与 tdata 部分报告的基数匹配。 TLS 偏移量是在链接期间通过从 tdata 部分的基数中减去符号的假地址来计算的;这些偏移量用于获取相对于线程本地存储基地址(存储在 risc-v 上的 tp 寄存器中)的值。

When initializing a newly allocated TLS block for a new thread, Zephyr copies bits from ROM into the parts of the new block corresponding to the initialized portion and then clears the portion corresponding to the zero-initialized portion.当为新线程初始化新分配的 TLS 块时,Zephyr 将位从 ROM 复制到新块中与初始化部分相对应的部分,然后清除与零初始化部分相对应的部分。

Your patch seems like it will pull random data from RAM starting at the fake __tdata_start address instead of the correct values from ROM starting at LOADADDR(__tdata_start).您的补丁似乎会从假 __tdata_start 地址开始从 RAM 中提取随机数据,而不是从 LOADADDR(__tdata_start) 开始的 ROM 中提取正确的值。

I'm betting we can't simply use memcpy from ROM on this chip and that we need to do something else to get the initialized values out of ROM and into each TLS block.我敢打赌,我们不能简单地使用该芯片上 ROM 中的 memcpy ,我们需要做一些其他事情来将初始化值从 ROM 中取出并放入每个 TLS 块中。

To check and make sure your fix works, you can try this version of samples/hello_world/src/main.c:要检查并确保您的修复有效,您可以尝试此版本的示例/hello_world/src/main.c:

/*
 * Copyright (c) 2012-2014 Wind River Systems, Inc.
 *
 * SPDX-License-Identifier: Apache-2.0
 */

#include <stdio.h>

#define TLS_DATA_VALUE 42

__thread int tls_data_variable = TLS_DATA_VALUE;

int main(void)
{
	if (tls_data_variable == TLS_DATA_VALUE) {
		printf("SUCCESS\n");
	} else {
		printf("FAILURE: tls_data_variable is %d instead of %d\n", tls_data_variable, TLS_DATA_VALUE);
	}
	return 0;
}

For the example code, my test was SUCCESS. For esp32c3, after the bootloader bootstrapping, LOADADDR(__tdata_start) should have been mapped to ADDR(__tdata_start) by the Flash MMU, and the data therein can be accessed normally through ADDR(__tdata_start).

I (62) boot: ESP Simple boot
I (62) boot: compile time May 13 2024 09:29:38
I (62) boot: Multicore bootloader
I (62) spi_flash: detected chip: mxic
I (64) spi_flash: flash io: dio
W (67) spi_flash: Detected size(4096k) larger than the size in the binary image header(2048k). Using the size in the binary image header.
I (79) boot: chip revision: v0.3
I (82) boot.esp32c3: SPI Speed : 40MHz
I (86) boot.esp32c3: SPI Mode : SLOW READ
I (90) boot.esp32c3: SPI Flash Size : 4MB
I (94) boot: Enabling RNG early entropy source...
[esp32c3] [INF] DRAM: lma 0x00000020 vma 0x3fc87a30 len 0xb48 (2888)
[esp32c3] [INF] DRAM: lma 0x00000b70 vma 0x3fc88578 len 0xa44 (2628)
[esp32c3] [INF] IRAM: lma 0x000015bc vma 0x40380000 len 0x6b34 (27444)
[esp32c3] [INF] IRAM: lma 0x000080f8 vma 0x40386b34 len 0xeec (3820)
[esp32c3] [INF] padd: lma 0x00008ff8 vma 0x00000000 len 0x7000 (28672)
[esp32c3] [INF] IMAP: lma 0x00010000 vma 0x42010000 len 0x2fb8 (12216)
[esp32c3] [INF] padd: lma 0x00012fc0 vma 0x00000000 len 0xd038 (53304)
[esp32c3] [INF] DMAP: lma 0x00020000 vma 0x3c000000 len 0x7a8 (1960)
[esp32c3] [INF] Image with 8 segments
[esp32c3] [INF] DROM segment: paddr=00020000h, vaddr=3c000000h, size=007B0h ( 1968) map
[esp32c3] [INF] IROM segment: paddr=00010000h, vaddr=42010000h, size=02FB8h ( 12216) map
*** Booting Zephyr OS build v3.6.0-3667-gf4c067c6bcf0 ***
SUCCESS

@sylvioalves
Copy link
Contributor Author

@lgl88911 Thanks for testing and confirming it works. I've tested already before submitting this PR to confirm same scenario. @keith-packard I'll check similar entries as such as the __tdata_start to make sure this SoC is properly handling it. Anyway, let me know if you have any other comment or suggestion. Thanks!

uLipe
uLipe previously approved these changes May 20, 2024
@sylvioalves sylvioalves added this to the v3.7.0 milestone Jun 20, 2024
@sylvioalves sylvioalves requested a review from nashif June 20, 2024 14:32
When TLS is used, `__tdata_start` is PROVIDED by
"thread-local-storage.ld" using absolute address, which
makes it land in wrong flash address. This causes risc-v startup
code to fail during memcpy/memset.

This PR overrides `__tdata_start` to use ADDR, which will
make sure it is placed in DROM region due to AT keyword.

Signed-off-by: Sylvio Alves <[email protected]>
@nashif nashif merged commit b38d430 into zephyrproject-rtos:main Jun 20, 2024
sylvioalves added a commit to sylvioalves/zephyr that referenced this pull request Jun 24, 2024
Fix linking error due undefined tdata entry.
After zephyrproject-rtos#72642, tdata could be undefined due to
missing TLS check.

Fixed zephyrproject-rtos#74852

Signed-off-by: Sylvio Alves <[email protected]>
aescolar pushed a commit that referenced this pull request Jun 24, 2024
Fix linking error due undefined tdata entry.
After #72642, tdata could be undefined due to
missing TLS check.

Fixed #74852

Signed-off-by: Sylvio Alves <[email protected]>
Chenhongren pushed a commit to Chenhongren/zephyr that referenced this pull request Aug 26, 2024
Fix linking error due undefined tdata entry.
After zephyrproject-rtos#72642, tdata could be undefined due to
missing TLS check.

Fixed zephyrproject-rtos#74852

(cherry picked from commit ee1b13c)

Original-Signed-off-by: Sylvio Alves <[email protected]>
GitOrigin-RevId: ee1b13c
Change-Id: I489d954e14822ee44b2892b257afaab35724ee9c
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/zephyr/+/5653342
Reviewed-by: Ting Shen <[email protected]>
Tested-by: ChromeOS Prod (Robot) <[email protected]>
Tested-by: Ting Shen <[email protected]>
Commit-Queue: Ting Shen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

platform: ESP32 Espressif ESP32

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Linking picolibc's srand to esp32c3 causes startup fault

8 participants