Message ID | 20230518064106.9082-1-yanhong.wang@starfivetech.com |
---|---|
State | Superseded |
Delegated to: | Andes |
Headers | show |
Series | [v1,1/2] arch: riscv: jh7110: Split the zeroing of L2 LIM on JH7110 | expand |
On 5/17/23 11:41 PM, Yanhong Wang wrote: > The per-hart stack,malloc space and global variable 'gd' sits between > __bss_end and L2_LIM_MEM_END.Zeroing this region could overwrite the > hart's stack, and other harts' stacks.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. > > So we should split the zeroing of L2 LIM into different places just > before the region is to be used. For stacks,let each hart clearing its > own stack, and for the malloc space, we can do so during malloc > initialization. The global variable 'gd' is initialized in the > board_init_f_init_reserve function. > > Signed-off-by: Yanhong Wang <yanhong.wang@starfivetech.com> > --- Hi Yanhong, Thanks for taking care of this. Would you mind refer my name if you want to directly copy-paste my previous response? > arch/riscv/cpu/jh7110/spl.c | 6 +++--- > arch/riscv/cpu/start.S | 14 ++++++++++++++ > common/init/board_init.c | 1 + > 3 files changed, 18 insertions(+), 3 deletions(-) > > diff --git a/arch/riscv/cpu/jh7110/spl.c b/arch/riscv/cpu/jh7110/spl.c > index 104f0fe949..6a48fba63d 100644 > --- a/arch/riscv/cpu/jh7110/spl.c > +++ b/arch/riscv/cpu/jh7110/spl.c > @@ -10,7 +10,6 @@ > #include <log.h> > > #define CSR_U74_FEATURE_DISABLE 0x7c1 > -#define L2_LIM_MEM_END 0x81FFFFFUL > > int spl_soc_init(void) > { > @@ -42,13 +41,14 @@ void harts_early_init(void) > csr_write(CSR_U74_FEATURE_DISABLE, 0); > > /* clear L2 LIM memory > - * set __bss_end to 0x81FFFFF region to zero > + * set __bss_end to stack end 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; > + len = CONFIG_SPL_STACK - CONFIG_VAL(SYS_MALLOC_F_LEN) - sizeof(*gd) - > + CONFIG_NR_CPUS * BIT(CONFIG_STACK_SIZE_SHIFT) - (ulong)&__bss_end; I don't understand what we are zeroing here. It seems that we are zeroing the hole between __bss_end and gd? Any reason in doing so? > 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 > + I think we should introduce another Kconfig option to indicate that stack should be zeroed before use, so we don't need board specific check, such as defined(CONFIG_STARFIVE_JH7110) > /* 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)) > Same as above.
Hi Yanhong and others, I've made up my own version and addressed my concerns in this patch: https://patchwork.ozlabs.org/project/uboot/patch/1684668616-358043-1-git-send-email-ganboing@gmail.com/ Some descriptions would be similar, as they are from my previous response to Heinrich: https://lists.denx.de/pipermail/u-boot/2023-May/518359.html Also please note that this patch is based on https://patchwork.ozlabs.org/project/uboot/patch/1684650044-313122-1-git-send-email-ganboing@gmail.com/ It addressed another issue with riscv stack initialization on all platforms. Please let me know if you have any suggestions. Thanks!
On 2023/5/21 19:42, Bo Gan wrote: > Hi Yanhong and others, > > I've made up my own version and addressed my concerns in this patch: > > https://patchwork.ozlabs.org/project/uboot/patch/1684668616-358043-1-git-send-email-ganboing@gmail.com/ > Hi Bo Gan, Sorry very much, when I submitted this patch, I didn't confirm that you had already submitted it, so the current situation has occurred. I will terminate the submission of this patch. > Some descriptions would be similar, as they are from my previous response to Heinrich: > https://lists.denx.de/pipermail/u-boot/2023-May/518359.html > The description of L2 LIM in the U74 datasheet is much the same as your previous response to Heinrich, so your previous response is referenced in the description section of this patch. This patch submission does not include a cover letter, and the content of the reference is not explained, I am very sorry. > Also please note that this patch is based on > https://patchwork.ozlabs.org/project/uboot/patch/1684650044-313122-1-git-send-email-ganboing@gmail.com/ > > It addressed another issue with riscv stack initialization on all platforms. > Please let me know if you have any suggestions. Thanks!
diff --git a/arch/riscv/cpu/jh7110/spl.c b/arch/riscv/cpu/jh7110/spl.c index 104f0fe949..6a48fba63d 100644 --- a/arch/riscv/cpu/jh7110/spl.c +++ b/arch/riscv/cpu/jh7110/spl.c @@ -10,7 +10,6 @@ #include <log.h> #define CSR_U74_FEATURE_DISABLE 0x7c1 -#define L2_LIM_MEM_END 0x81FFFFFUL int spl_soc_init(void) { @@ -42,13 +41,14 @@ void harts_early_init(void) csr_write(CSR_U74_FEATURE_DISABLE, 0); /* clear L2 LIM memory - * set __bss_end to 0x81FFFFF region to zero + * set __bss_end to stack end 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; + len = CONFIG_SPL_STACK - CONFIG_VAL(SYS_MALLOC_F_LEN) - sizeof(*gd) - + CONFIG_NR_CPUS * BIT(CONFIG_STACK_SIZE_SHIFT) - (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))
The per-hart stack,malloc space and global variable 'gd' sits between __bss_end and L2_LIM_MEM_END.Zeroing this region could overwrite the hart's stack, and other harts' stacks.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. So we should split the zeroing of L2 LIM into different places just before the region is to be used. For stacks,let each hart clearing its own stack, and for the malloc space, we can do so during malloc initialization. The global variable 'gd' is initialized in the board_init_f_init_reserve function. Signed-off-by: Yanhong Wang <yanhong.wang@starfivetech.com> --- arch/riscv/cpu/jh7110/spl.c | 6 +++--- arch/riscv/cpu/start.S | 14 ++++++++++++++ common/init/board_init.c | 1 + 3 files changed, 18 insertions(+), 3 deletions(-)