lib: open-amp: initialize the resource_table.cm_trace.da at runtime #96554
Conversation
When build the openamp_rsc_table with the ram-console snippet, it will
report build error: "initializer element is not constant". This patch
fix it by moving the initialization of the resource_table.cm_trace.da
to runtime.
build error logs:
zephyr/lib/open-amp/resource_table.c:77:17: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
77 | (uint32_t)ram_console_buf, CONFIG_RAM_CONSOLE_BUFFER_SIZE, 0,
| ^
zephyr/lib/open-amp/resource_table.c:77:17: error: initializer element is not constant
zephyr/lib/open-amp/resource_table.c:77:17: note: (near initialization for 'resource_table.cm_trace.da')
Signed-off-by: Hou Zhiqiang <[email protected]>
Signed-off-by: Jiafei Pan <[email protected]>
|
|
@Zhiqiang-Hou I've enabled CONFIG_RAM_CONSOLE and I cannot reproduce your error. |
Try this build: got the following build error: |
Yes, I can reproduce the error with this setup.
|
It is just used for trigger this error, which was found on arm64 platforms, actually any sample with RAM Console and resource table configs selected can trigger this error on arm64 platforms. |
So, is there something special about arm64 platforms? What exactly is it? |
| .cm_trace = { \ | ||
| RSC_TRACE, \ | ||
| (uint32_t)ram_console_buf, CONFIG_RAM_CONSOLE_BUFFER_SIZE, 0,\ | ||
| 0, /* Will be initialized at runtime */ \ |
There was a problem hiding this comment.
IMO, a simple solution, with small changes is:
diff --git a/subsys/ipc/open-amp/resource_table.h b/subsys/ipc/open-amp/resource_table.h
index 4e8f2c00d00..70476b1812f 100644
--- a/subsys/ipc/open-amp/resource_table.h
+++ b/subsys/ipc/open-amp/resource_table.h
@@ -85,7 +85,7 @@ struct fw_resource_table {
#define CM_TRACE_ENTRY \
.cm_trace = { \
RSC_TRACE, \
- (uint32_t)ram_console_buf, CONFIG_RAM_CONSOLE_BUFFER_SIZE, 0,\
+ DT_REG_ADDR(DT_CHOSEN(zephyr_ram_console)), CONFIG_RAM_CONSOLE_BUFFER_SIZE, 0,\
"Zephyr_log", \
},
#else
This way:
DT_REG_ADDR(DT_CHOSEN(zephyr_ram_console))will resolve to the compile-time constant from snippet;- no runtime initialization needed;
- no pointer-to-integer casting issues;
- the resource table gets the correct fixed address that matches where the RAM console buffer is actually located;
There was a problem hiding this comment.
There are 2 config options that we have to take into account:
- CONFIG_RAM_CONSOLE_BUFFER_SECTION=y: the ram_console buffer will be located into a dedicated section.
- CONFIG_RAM_CONSOLE_BUFFER_SECTION=n: the ram_console buffer address will be determined during compiling.
Your solution is only for the option 1. Do you have any idea for option 2.
There was a problem hiding this comment.
@iuliana-prodan and @arnopo I think we can use the runtime-initialization solution, it can handle both of the 2 options, and won't lead Linux crash. Any comments?
There was a problem hiding this comment.
There are 2 config options that we have to take into account:
- CONFIG_RAM_CONSOLE_BUFFER_SECTION=y: the ram_console buffer will be located into a dedicated section.
- CONFIG_RAM_CONSOLE_BUFFER_SECTION=n: the ram_console buffer address will be determined during compiling.
Your solution is only for the option 1. Do you have any idea for option 2.
I've tested the above solution on the Xtensa and ARM32 architectures, and their toolchains are more permissive, allowing undefined macros to default to 0.
To fix also ARM64, we should do something like:
diff --git a/subsys/ipc/open-amp/resource_table.h b/subsys/ipc/open-amp/resource_table.h
index 4e8f2c00d00..5c70df4f74a 100644
--- a/subsys/ipc/open-amp/resource_table.h
+++ b/subsys/ipc/open-amp/resource_table.h
@@ -82,14 +82,23 @@ struct fw_resource_table {
#endif
#if defined(CONFIG_RAM_CONSOLE)
- #define CM_TRACE_ENTRY \
- .cm_trace = { \
- RSC_TRACE, \
- (uint32_t)ram_console_buf, CONFIG_RAM_CONSOLE_BUFFER_SIZE, 0,\
- "Zephyr_log", \
- },
+ #if DT_HAS_CHOSEN(zephyr_ram_console)
+ #define CM_TRACE_ENTRY \
+ .cm_trace = { \
+ RSC_TRACE, \
+ DT_REG_ADDR(DT_CHOSEN(zephyr_ram_console)), CONFIG_RAM_CONSOLE_BUFFER_SIZE, 0,\
+ "Zephyr_log", \
+ },
+ #else
+ #define CM_TRACE_ENTRY \
+ .cm_trace = { \
+ RSC_TRACE, \
+ 0, CONFIG_RAM_CONSOLE_BUFFER_SIZE, 0, \
+ "Zephyr_log", \
+ },
+ #endif
#else
- #define CM_TRACE_ENTRY
+ #define CM_TRACE_ENTRY
#endif
#define RESOURCE_TABLE_INIT \
With this change, if CONFIG_RAM_CONSOLE=y is set, the ram_console_buf will be placed in .bss. If a snippet defines the zephyr,ram-console node, the buffer will be located in the specified memory region.
Here’s the location of ram_console_buf from three different builds:
west build -d imx95_a55 -p always -b imx95_evk/mimx9596/a55 samples/hello_world/ -DCONFIG_OPENAMP_RSC_TABLE=y -DCONFIG_RAM_CONSOLE=y
west build -d imx93_a55 -p always -b imx93_evk/mimx9352/a55 samples/hello_world/ -DCONFIG_OPENAMP_RSC_TABLE=y -DCONFIG_RAM_CONSOLE=y
west build -d imx93_a55_snippet -p always -S ram-console -b imx93_evk/mimx9352/a55 samples/hello_world/ -DCONFIG_OPENAMP_RSC_TABLE=y -DCONFIG_RAM_CONSOLE=y
~zephyrproject/zephyr$ grep -rwn ram_console_buf imx9*/zephyr/zephyr.map
imx93_a55_snippet/zephyr/zephyr.map:4634: 0x00000000d0100000 ram_console_buf
imx93_a55/zephyr/zephyr.map:3965: .bss.ram_console_buf
imx93_a55/zephyr/zephyr.map:3967: 0x00000000d004c340 ram_console_buf
imx95_a55/zephyr/zephyr.map:4149: .bss.ram_console_buf
imx95_a55/zephyr/zephyr.map:4151: 0x00000000d0052c94 ram_console_buf
~zephyrproject/zephyr$
Can you please check this and let me know if it works for you?
There was a problem hiding this comment.
+ #else
+ #define CM_TRACE_ENTRY \
+ .cm_trace = { \
+ RSC_TRACE, \
+ 0, CONFIG_RAM_CONSOLE_BUFFER_SIZE, 0, \
+ "Zephyr_log", \
+ },
+ #endif
In this case, the cm_trace.da is initialized with '0', so still need runtime initialization to assign it the ram_console_buf[] address?
There was a problem hiding this comment.
Yes, the feature is disabled even though the buffer exists.
However, since we don’t have a snippet that defines the address of the ram_console_buf, I assumed this was the expected behavior.
Am I mistaken?
There was a problem hiding this comment.
This changes the functionality against the current code. The current code initializes the cm_trace.da no matter the ram_console_buf locates in dedicated section or the address arragned by toolchain, this change only handle the former. I think we also need to handle the latter, since it affects all architectures/platforms.
| .cm_trace = { \ | ||
| RSC_TRACE, \ | ||
| (uint32_t)ram_console_buf, CONFIG_RAM_CONSOLE_BUFFER_SIZE, 0,\ | ||
| 0, /* Will be initialized at runtime */ \ |
There was a problem hiding this comment.
Do you check impact in case of firmware that are loaded with a static const table? I do not verify by a test but seems to me that this update can lead to a Linux crash.
Indeed initialise the address at runtime is to late, the resource table is parsed by the Linux remoteproc driver before loading the remoteproc firmware (https://elixir.bootlin.com/linux/v6.17/source/drivers/remoteproc/remoteproc_core.c#L571)
There was a problem hiding this comment.
Thanks for your comments!
It is not too late and won't cause Linux crash, as the cm_trace entry need to be dump through the Linux debugfs to get the ram_console buffer address after using the remoteproc boot up Zephyr, and then dump the logs from ram_console buffer.
There was a problem hiding this comment.
Please tell me if I'm wrong but the Linux kernel calls rproc_handle_trace before loading the and booting up the Zephyr
With your patch trace->trace_mem.da = 0;
The rproc_debug_trace structure is then give as parameter of https://elixir.bootlin.com/linux/v6.17/C/ident/rproc_create_trace_file
Then when you call the debugfs rproc_trace_read your DA value is 0.
Do you load and start the firmware with the Linux remoteproc driver or just use the attach mechanism?
I will try to find time end of this week to test your patch on Stm32mp157F-DK board to confirm ( or not) my hypothesis...
There was a problem hiding this comment.
I had a run on imx95evk, start the Zephyr on a CA55 core and cat the resource table immediately, and can get the correct ram_console_buf[] address.
The Linux logs:
root@imx95-15x15-lpddr4x-evk:/lib/firmware# echo rpmsg_str_echo_ca55_RTOS0_RAM_CONSOLE-0xd0100000.elf > /sys/devices/platform/remoteproc-ca55-4/remoteproc/remoteproc3/firmware
root@imx95-15x15-lpddr4x-evk:/lib/firmware# echo start > /sys/devices/platform/remoteproc-ca55-4/remoteproc/remoteproc3/state;cat /sys/kernel/debug/remoteproc/remoteproc3/resource_table
[ 1637.130732] remoteproc remoteproc3: powering up remoteproc-ca55-4
[ 1637.132837] remoteproc remoteproc3: Booting fw image rpmsg_str_echo_ca55_RTOS0_RAM_CONSOLE-0xd0100000.elf, size 1002904
[ 1637.225101] psci: CPU4 killed (polled 0 ms)
[ 1637.282067] rproc-virtio rproc-virtio.2.auto: assigned reserved memory node vdev0buffer@c0200000
[ 1637.284033] virtio_rpmsg_bus virtio0: rpmsg host is online
[ 1637.284181] rproc-virtio rproc-virtio.2.auto: registered virtio0 (type 7)
[ 1637.284200] remoteproc remoteproc3: remote processor remoteproc-ca55-4 is now up
Entry 0 is of typ[ 1637.285838] virtio_rpmsg_bus virtio0: creating channel rpmsg-virtual-tty-channel-1 addr 0x3
e vdev
[ 1637.286357] imx_rpmsg_tty virtio0.rpmsg-virtual-tty-channel-1.-1.3: new channel: 0x400 -> 0x3!
[ 1637.287403] Install rpmsg tty driver!
ID 7[ 1637.290148] virtio_rpmsg_bus virtio0: creating channel rpmsg-virtual-tty-channel-1 addr 0x4
[ 1637.290616] imx_rpmsg_tty virtio0.rpmsg-virtual-tty-channel-1.-1.4: new channel: 0x401 -> 0x4!
[ 1637.291172] Install rpmsg tty driver!
[ 1637.291471] virtio_rpmsg_bus virtio0: creating channel rpmsg-virtual-tty-channel-1 addr 0x5
[ 1637.291904] imx_rpmsg_tty virtio0.rpmsg-virtual-tty-channel-1.-1.5: new channel: 0x402 -> 0x5!
[ 1637.292640] Install rpmsg tty driver!
Notify ID 0
Device features 0x1
Guest features 0x1
Config length 0x0
Status 0x7
Number of vrings 2
Reserved (should be zero) [0][0]
Vring 0
Device Address 0xc0100000
Alignment 16
Number of buffers 8
Notify ID 0
Physical Address 0x0
Vring 1
Device Address 0xc0108000
Alignment 16
Number of buffers 8
Notify ID 1
Physical Address 0x0
Entry 1 is of type trace
Device Address 0xd0100000 // got the correct ram_console_buf address
Length 0x1000 Bytes
Reserved (should be zero) [0]
Name Zephyr_log
There was a problem hiding this comment.
What Linux are you using? Have you made any changes to the remoteproc subsystem?
Also, how are you configuring the A55 core as a remote processor - are you using any form of virtualization?
i.MX95 has 6 ARM Cortex-A55.
There was a problem hiding this comment.
@iuliana-prodan and @arnopo
I had a investigation into why this compilation error occurs on ARM64 platforms but not on ARM32 platforms. The root cause lies in a violation of the programming language C standard regarding static initialization.
The initialization of cm_trace.da (a uint32_t type) violates the following requirement:
(uint32_t)ram_console_buf // this expression is not a constant expressions (Obviously not a string literals either)
-
C23 Standard 6.7.11 Initialization, Clause 5:
"All the expressions in an initializer for an object that has static or thread storage duration or is
declared with the constexpr storage-class specifier shall be constant expressions or string literals." -
C23 Standard 6.6 Constant Expressions, Clause 8:
"An integer constant expression shall have integer type and shall only have operands that are
integer constants, named and compound literal constants of integer type, character constants,
sizeof expressions whose results are integer constants, alignof expressions, and floating, named,
or compound literal constants of arithmetic type that are the immediate operands of casts. Cast
operators in an integer constant expression shall only convert arithmetic types to integer types,
except as part of an operand to the typeof operators, sizeof operator, or alignof operator."
The operand ram_console_buf in (uint32_t)ram_console_buf is not among the allowed operand types for integer constant expressions.
- C23 Standard 6.6 Constant Expressions, Clause 11
"An address constant is a null pointer, a pointer to an lvalue designating an object of static storage
duration, or a pointer to a function designator; it shall be created explicitly using the unary &
operator or an integer constant cast to pointer type, or implicitly using an expression of array or
function type."
While ram_console_buf (as an array) is indeed an address constant through implicit conversion, the expression (uint32_t)ram_console_buf represents a pointer-to-integer conversion, which is not defined as a constant expression in the standard.
- C23 Standard 6.6 Constant Expressions, Clause 14
"An implementation may accept other forms of constant expressions; however, it is implementation-defined whether they are an integer constant expression."
This case seems fell into the 6.6 Constant Expressions, Clause 14:
GCC's implementation appears to allow pointer-to-integer conversions as extensions only when no actual size conversion is required.
The current expression (uint32_t)ram_console_buf doesn't trigger the "initializer element is not constant" on ARM32 platforms (the uint32_t has the same size as a pointer var, no size change in pointer-to-integer conversion); and triggers on ARM64 platforms (the size of uint32_t is different from a pointer var on ARM64).
When change to (uint64_t)ram_console_buf, it doesn't trigger this build error on AMR64; and triggers on ARM32.
So, the current expression (uint32_t)ram_console_buf is not always treated as 'constant expression', the current code is not portable. Correct me if I'm wrong.
And any idea to fix this violation?
There was a problem hiding this comment.
@Zhiqiang-Hou, could you please share the west command you are using to reproduce the issue?
There was a problem hiding this comment.
@iuliana-prodan and @arnopo I had a investigation into why this compilation error occurs on ARM64 platforms but not on ARM32 platforms. The root cause lies in a violation of the programming language C standard regarding static initialization.
The initialization of cm_trace.da (a uint32_t type) violates the following requirement:
(uint32_t)ram_console_buf // this expression is not a constant expressions (Obviously not a string literals either)
- C23 Standard 6.7.11 Initialization, Clause 5:
"All the expressions in an initializer for an object that has static or thread storage duration or is
declared with the constexpr storage-class specifier shall be constant expressions or string literals."- C23 Standard 6.6 Constant Expressions, Clause 8:
"An integer constant expression shall have integer type and shall only have operands that are
integer constants, named and compound literal constants of integer type, character constants,
sizeof expressions whose results are integer constants, alignof expressions, and floating, named,
or compound literal constants of arithmetic type that are the immediate operands of casts. Cast
operators in an integer constant expression shall only convert arithmetic types to integer types,
except as part of an operand to the typeof operators, sizeof operator, or alignof operator."The operand
ram_console_bufin(uint32_t)ram_console_bufis not among the allowed operand types for integer constant expressions.
- C23 Standard 6.6 Constant Expressions, Clause 11
"An address constant is a null pointer, a pointer to an lvalue designating an object of static storage
duration, or a pointer to a function designator; it shall be created explicitly using the unary &
operator or an integer constant cast to pointer type, or implicitly using an expression of array or
function type."While
ram_console_buf(as an array) is indeed an address constant through implicit conversion, the expression (uint32_t)ram_console_buf represents a pointer-to-integer conversion, which is not defined as a constant expression in the standard.
- C23 Standard 6.6 Constant Expressions, Clause 14
"An implementation may accept other forms of constant expressions; however, it is implementation-defined whether they are an integer constant expression."This case seems fell into the 6.6 Constant Expressions, Clause 14: GCC's implementation appears to allow pointer-to-integer conversions as extensions only when no actual size conversion is required.
The current expression
(uint32_t)ram_console_bufdoesn't trigger the "initializer element is not constant" on ARM32 platforms (the uint32_t has the same size as a pointer var, no size change in pointer-to-integer conversion); and triggers on ARM64 platforms (the size of uint32_t is different from a pointer var on ARM64).When change to
(uint64_t)ram_console_buf, it doesn't trigger this build error on AMR64; and triggers on ARM32.So, the current expression
(uint32_t)ram_console_bufis not always treated as 'constant expression', the current code is not portable. Correct me if I'm wrong.And any idea to fix this violation?
@Zhiqiang-Hou @arnopo I propose this solution:
diff --git a/subsys/ipc/open-amp/resource_table.c b/subsys/ipc/open-amp/resource_table.c
index 65a47897a39..8997bf6ffb9 100644
--- a/subsys/ipc/open-amp/resource_table.c
+++ b/subsys/ipc/open-amp/resource_table.c
@@ -43,6 +43,10 @@ static struct fw_resource_table __resource resource_table = RESOURCE_TABLE_INIT;
void rsc_table_get(void **table_ptr, int *length)
{
+#if defined(CONFIG_RAM_CONSOLE) && !DT_HAS_CHOSEN(zephyr_ram_console)
+ // Update the address before returning the table
+ resource_table.cm_trace.da = (uint32_t)(uintptr_t)ram_console_buf;
+#endif
*length = sizeof(resource_table);
#ifdef CONFIG_OPENAMP_COPY_RSC_TABLE
*table_ptr = (void *)RSC_TABLE_ADDR;
diff --git a/subsys/ipc/open-amp/resource_table.h b/subsys/ipc/open-amp/resource_table.h
index 4e8f2c00d00..691bae48281 100644
--- a/subsys/ipc/open-amp/resource_table.h
+++ b/subsys/ipc/open-amp/resource_table.h
@@ -82,12 +82,21 @@ struct fw_resource_table {
#endif
#if defined(CONFIG_RAM_CONSOLE)
- #define CM_TRACE_ENTRY \
- .cm_trace = { \
- RSC_TRACE, \
- (uint32_t)ram_console_buf, CONFIG_RAM_CONSOLE_BUFFER_SIZE, 0,\
- "Zephyr_log", \
- },
+ #if DT_HAS_CHOSEN(zephyr_ram_console)
+ #define CM_TRACE_ENTRY \
+ .cm_trace = { \
+ RSC_TRACE, \
+ DT_REG_ADDR(DT_CHOSEN(zephyr_ram_console)), CONFIG_RAM_CONSOLE_BUFFER_SIZE, 0,\
+ "Zephyr_log", \
+ },
+ #else
+ #define CM_TRACE_ENTRY \
+ .cm_trace = { \
+ RSC_TRACE, \
+ 0, CONFIG_RAM_CONSOLE_BUFFER_SIZE, 0, \
+ "Zephyr_log", \
+ },
+ #endif
#else
#define CM_TRACE_ENTRY
#endif
In this case, if we have the zephyr_ram_console node, DT_REG_ADDR(DT_CHOSEN(zephyr_ram_console)) will resolve to the compile-time constant from snippet and no runtime initialization needed.
Otherwise, we use runtime initialization to update the ram_console_buf address before returning the resource table.
I've verified on the DSP (Xtensa arch) from i.MX8M Plus.
Can you, please, check?
There was a problem hiding this comment.
@Zhiqiang-Hou, could you please share the west command you are using to reproduce the issue?
west build -p always -b imx95_evk/mimx9596/a55 samples/hello_world/ -DCONFIG_OPENAMP_RSC_TABLE=y -DCONFIG_RAM_CONSOLE=y
There was a problem hiding this comment.
@Zhiqiang-Hou @arnopo I propose this solution:
diff --git a/subsys/ipc/open-amp/resource_table.c b/subsys/ipc/open-amp/resource_table.c index 65a47897a39..8997bf6ffb9 100644 --- a/subsys/ipc/open-amp/resource_table.c +++ b/subsys/ipc/open-amp/resource_table.c @@ -43,6 +43,10 @@ static struct fw_resource_table __resource resource_table = RESOURCE_TABLE_INIT; void rsc_table_get(void **table_ptr, int *length) { +#if defined(CONFIG_RAM_CONSOLE) && !DT_HAS_CHOSEN(zephyr_ram_console) + // Update the address before returning the table + resource_table.cm_trace.da = (uint32_t)(uintptr_t)ram_console_buf; +#endif *length = sizeof(resource_table); #ifdef CONFIG_OPENAMP_COPY_RSC_TABLE *table_ptr = (void *)RSC_TABLE_ADDR; diff --git a/subsys/ipc/open-amp/resource_table.h b/subsys/ipc/open-amp/resource_table.h index 4e8f2c00d00..691bae48281 100644 --- a/subsys/ipc/open-amp/resource_table.h +++ b/subsys/ipc/open-amp/resource_table.h @@ -82,12 +82,21 @@ struct fw_resource_table { #endif #if defined(CONFIG_RAM_CONSOLE) - #define CM_TRACE_ENTRY \ - .cm_trace = { \ - RSC_TRACE, \ - (uint32_t)ram_console_buf, CONFIG_RAM_CONSOLE_BUFFER_SIZE, 0,\ - "Zephyr_log", \ - }, + #if DT_HAS_CHOSEN(zephyr_ram_console) + #define CM_TRACE_ENTRY \ + .cm_trace = { \ + RSC_TRACE, \ + DT_REG_ADDR(DT_CHOSEN(zephyr_ram_console)), CONFIG_RAM_CONSOLE_BUFFER_SIZE, 0,\ + "Zephyr_log", \ + }, + #else + #define CM_TRACE_ENTRY \ + .cm_trace = { \ + RSC_TRACE, \ + 0, CONFIG_RAM_CONSOLE_BUFFER_SIZE, 0, \ + "Zephyr_log", \ + }, + #endif #else #define CM_TRACE_ENTRY #endifIn this case, if we have the
zephyr_ram_consolenode,DT_REG_ADDR(DT_CHOSEN(zephyr_ram_console)) will resolve to the compile-time constant from snippet and no runtime initialization needed. Otherwise, we use runtime initialization to update theram_console_bufaddress before returning the resource table. I've verified on the DSP (Xtensa arch) from i.MX8M Plus.Can you, please, check?
Here it looks to me that the issue is related to 64-bit arch build that does not allow to cast a 64-bit address to 32-bit address in a static way.
In such case I would use the CONFIG_ARM64 with a comment explaining that the cast is not possible.
An alternative would be to create a new entry that support the 64-bit addressing but this would request a patch in Linux also.
It's not related to ram_console snippet, the following build command can also trigger this build error. |
It is a valid use case as the trace can be used to print the hello world message if the RAM console is used ( no serial console on the remote processor).
|
|
Hi, @Zhiqiang-Hou , please fix compliance issue and rebase to fix confliction. |
|
5 participants
When build the openamp_rsc_table with the ram-console snippet, it will report build error: "initializer element is not constant". This patch fix it by moving the initialization of the resource_table.cm_trace.da to runtime.
build error logs:
zephyr/lib/open-amp/resource_table.c:77:17: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
77 | (uint32_t)ram_console_buf, CONFIG_RAM_CONSOLE_BUFFER_SIZE, 0,
| ^
zephyr/lib/open-amp/resource_table.c:77:17: error: initializer element is not constant
zephyr/lib/open-amp/resource_table.c:77:17: note: (near initialization for 'resource_table.cm_trace.da')