[PATCH v1 1/2] arch: riscv: jh7110: Split the zeroing of L2 LIM on JH7110

[Bo Gan]

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 at 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.