On 2023/5/12 13:50, Bo Gan wrote:
> On 3/28/23 8:42 PM, Yanhong Wang wrote:
>> +void harts_early_init(void)
>> +{
>> + ulong *ptr;
>> + u8 *tmp;
>> + ulong len, remain;
>> + /*
>> + * Feature Disable CSR
>> + *
>> + * Clear feature disable CSR to '0' to turn on all features for
>> + * each core. This operation must be in M-mode.
>> + */
>> + if (CONFIG_IS_ENABLED(RISCV_MMODE))
>> + csr_write(CSR_U74_FEATURE_DISABLE, 0);
>> +
>> + /* clear L2 LIM memory
>> + * set __bss_end to 0x81FFFFF region to zero
>> + * The L2 Cache Controller supports ECC. ECC is applied to SRAM.
>> + * If it is not cleared, the ECC part is invalid, and an ECC error
>> + * will be reported when reading data.
>> + */
>> + ptr = (ulong *)&__bss_end;
>> + len = L2_LIM_MEM_END - (ulong)&__bss_end;
>> + remain = len % sizeof(ulong);
>> + len /= sizeof(ulong);
>> +
>> + while (len--)
>> + *ptr++ = 0;
>> +
>> + /* clear the remain bytes */
>> + if (remain) {
>> + tmp = (u8 *)ptr;
>> + while (remain--)
>> + *tmp++ = 0;
>> + }
>> +}
> Hi Yanhong, I know this is already merged, but it looks wrong to
> me.`harts_early_init`
> will be called by all harts in SPL. The per-hart stack sits between __bss_end
> and L2_LIM_MEM_END.
> Zeroing this region could overwrite the hart's stack, and other harts'
> stacks. The current
> implementation works likely because harts_early_init doesn't use any stack
> space, but it's up to
> the compiler and we can't guarantee that. If it were to save and restore `ra`
> register, then we
> would crash in function epilogue. Also, we are having data-races here,
> because harts are writing
> over each other's stack.
>
> My advice is that we should split the zeroing of L2 LIM into different places
> just before the
> region is to be used. For stacks, we can let each hart clearing its own
> stack, and for the malloc
> space, we can do so during malloc initialization. Doing so also gives us the
> benefit of catching
> the read of uninitialized data. In this approach, the L2_LIM_MEM_END macro is
> not needed anymore.
Hi，Bo Gan， I agree with you, there is some problem with the initialization of
the L2 LIM,
so, as you suggested, we should split the zeroing of L2 LIM into different
places.
Something like：
diff --git a/arch/riscv/cpu/jh7110/spl.c b/arch/riscv/cpu/jh7110/spl.c
index 72adcefa0e..574ffc3d33 100644
--- a/arch/riscv/cpu/jh7110/spl.c
+++ b/arch/riscv/cpu/jh7110/spl.c
@@ -13,7 +13,6 @@
#include <init.h>
#define CSR_U74_FEATURE_DISABLE 0x7c1
-#define L2_LIM_MEM_END 0x81FFFFFUL
DECLARE_GLOBAL_DATA_PTR;
@@ -61,7 +60,7 @@ void harts_early_init(void)
{
ulong *ptr;
u8 *tmp;
- ulong len, remain;
+ ulong len, remain, init_end;
/*
* Feature Disable CSR
*
@@ -77,8 +76,10 @@ void harts_early_init(void)
* If it is not cleared, the ECC part is invalid, and an ECC error
* will be reported when reading data.
*/
+ init_end = CONFIG_SPL_STACK -CONFIG_VAL(SYS_MALLOC_F_LEN) - sizeof(*gd)
+ - CONFIG_NR_CPUS * BIT(CONFIG_STACK_SIZE_SHIFT);
ptr = (ulong *)&__bss_end;
- len = L2_LIM_MEM_END - (ulong)&__bss_end;
+ len = init_end - (ulong)&__bss_end;
remain = len % sizeof(ulong);
len /= sizeof(ulong);
diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
index dad22bfea8..46da9ec503 100644
--- a/arch/riscv/cpu/start.S
+++ b/arch/riscv/cpu/start.S
@@ -118,6 +118,20 @@ call_board_init_f_0:
mv sp, a0
#endif
+#if defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_STACK) && \
+ defined(CONFIG_STARFIVE_JH7110)
+
+ /* Set the stack region to zero */
+ li t0, 1
+ slli t1, t0, CONFIG_STACK_SIZE_SHIFT
+ mv t0, sp
+ sub t1, t0, t1
+clear_stack:
+ SREG zero, 0(t1)
+ addi t1, t1, REGBYTES
+ blt t1, t0, clear_stack
+#endif
+
/* Configure proprietary settings and customized CSRs of harts */
call_harts_early_init:
jal harts_early_init
diff --git a/common/init/board_init.c b/common/init/board_init.c
index 96ffb79a98..46e4e4abc7 100644
--- a/common/init/board_init.c
+++ b/common/init/board_init.c
@@ -162,6 +162,7 @@ void board_init_f_init_reserve(ulong base)
#if CONFIG_VAL(SYS_MALLOC_F_LEN)
/* go down one 'early malloc arena' */
gd->malloc_base = base;
+ memset((void *)base, 0, CONFIG_VAL(SYS_MALLOC_F_LEN));
#endif
if (CONFIG_IS_ENABLED(SYS_REPORT_STACK_F_USAGE))
