[PATCH v3 1/3] riscv: Kconfig: Add SPL_ZERO_MEM_BEFORE_USE

[PATCH v3 1/3] riscv: Kconfig: Add SPL_ZERO_MEM_BEFORE_USE

[Shengyu Qu]

Add a Kconfig item to allow SPL to clear stack/GD/malloc area before
using them.

Signed-off-by: Bo Gan <ganboing@gmail.com>
Signed-off-by: Shengyu Qu <wiagn233@outlook.com>
---
 arch/riscv/Kconfig | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 867cbcbe74..6771d8d919 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -64,6 +64,14 @@ config SPL_SYS_DCACHE_OFF
 	help
 	  Do not enable data cache in SPL.
 
+config SPL_ZERO_MEM_BEFORE_USE
+	bool "Zero memory before use"
+	depends on SPL
+	default n
+	help
+	  Zero stack/GD/malloc area in SPL before using them, this is needed for
+	  Sifive core devices that uses L2 cache to store SPL.
+
 # board-specific options below
 source "board/AndesTech/ae350/Kconfig"
 source "board/emulation/qemu-riscv/Kconfig"
-- 
2.41.0

[PATCH v3 2/3] riscv: Add SPL_ZERO_MEM_BEFORE_USE implementation

[Shengyu Qu]

Add the actual support code for SPL_ZERO_MEM_BEFORE_USE and remove
existing Starfive JH7110's L2 LIM clean code, since existing code has
following issues:
 1. Each hart (in the middle of a function call) overwriting its own
    stack and other harts' stacks.
    (data-race and data-corruption)
 2. Lottery winner hart can be doing "board_init_f_init_reserve",
    while other harts are in the middle of zeroing L2 LIM.
    (data-race)

Signed-off-by: Bo Gan <ganboing@gmail.com>
Signed-off-by: Shengyu Qu <wiagn233@outlook.com>
---
Changes since v2:
 - Fix typo (ZERO_MEM_BEFORE_USE to SPL_ZERO_MEM_BEFORE_USE)
---
 arch/riscv/cpu/jh7110/spl.c | 25 -------------------------
 arch/riscv/cpu/start.S      | 12 ++++++++++++
 common/init/board_init.c    |  3 +++
 3 files changed, 15 insertions(+), 25 deletions(-)

diff --git a/arch/riscv/cpu/jh7110/spl.c b/arch/riscv/cpu/jh7110/spl.c
index 72adcefa0e..4047b10efe 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;
 
@@ -59,9 +58,6 @@ int spl_soc_init(void)
 
 void harts_early_init(void)
 {
-	ulong *ptr;
-	u8 *tmp;
-	ulong len, remain;
 	/*
 	 * Feature Disable CSR
 	 *
@@ -70,25 +66,4 @@ void harts_early_init(void)
 	 */
 	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;
-	}
 }
diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
index 59d58a5a57..930309d8d2 100644
--- a/arch/riscv/cpu/start.S
+++ b/arch/riscv/cpu/start.S
@@ -111,6 +111,18 @@ call_board_init_f:
  * It's essential before any function call, otherwise, we get data-race.
  */
 
+/* clear stack if necessary */
+#if CONFIG_IS_ENABLED(SPL_ZERO_MEM_BEFORE_USE)
+clear_stack:
+	li	t1, 1
+	slli	t1, t1, CONFIG_STACK_SIZE_SHIFT
+	sub	t1, sp, t1
+clear_stack_loop:
+	SREG	zero, 0(t1)		/* t1 is always 16 byte aligned */
+	addi	t1, t1, REGBYTES
+	blt	t1, sp, clear_stack_loop
+#endif
+
 call_board_init_f_0:
 	/* find top of reserve space */
 #if CONFIG_IS_ENABLED(SMP)
diff --git a/common/init/board_init.c b/common/init/board_init.c
index 96ffb79a98..51d9ec9a13 100644
--- a/common/init/board_init.c
+++ b/common/init/board_init.c
@@ -162,6 +162,9 @@ 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;
+#if CONFIG_IS_ENABLED(SPL_ZERO_MEM_BEFORE_USE)
+	memset((void *)base, '\0', CONFIG_VAL(SYS_MALLOC_F_LEN));
+#endif
 #endif
 
 	if (CONFIG_IS_ENABLED(SYS_REPORT_STACK_F_USAGE))
-- 
2.41.0

[PATCH v3 3/3] riscv: cpu: jh7110: Select SPL_ZERO_MEM_BEFORE_USE

[Shengyu Qu]

Add Kconfig item for Starfive JH7110 to select SPL_ZERO_MEM_BEFORE_USE.

Signed-off-by: Bo Gan <ganboing@gmail.com>
Signed-off-by: Shengyu Qu <wiagn233@outlook.com>
---
 arch/riscv/cpu/jh7110/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/riscv/cpu/jh7110/Kconfig b/arch/riscv/cpu/jh7110/Kconfig
index 4d9581165b..2e26d0731f 100644
--- a/arch/riscv/cpu/jh7110/Kconfig
+++ b/arch/riscv/cpu/jh7110/Kconfig
@@ -13,6 +13,7 @@ config STARFIVE_JH7110
 	select SUPPORT_SPL
 	select SPL_RAM if SPL
 	select SPL_STARFIVE_DDR
+	select SPL_ZERO_MEM_BEFORE_USE
 	select PINCTRL_STARFIVE_JH7110
 	imply MMC
 	imply MMC_BROKEN_CD
-- 
2.41.0

Re: [PATCH v3 1/3] riscv: Kconfig: Add SPL_ZERO_MEM_BEFORE_USE

[Leo Liang]

On Tue, Aug 08, 2023 at 08:39:55PM +0800, Shengyu Qu wrote:
> Add a Kconfig item to allow SPL to clear stack/GD/malloc area before
> using them.
> 
> Signed-off-by: Bo Gan <ganboing@gmail.com>
> Signed-off-by: Shengyu Qu <wiagn233@outlook.com>
> ---
>  arch/riscv/Kconfig | 8 ++++++++
>  1 file changed, 8 insertions(+)

Reviewed-by: Leo Yu-Chi Liang <ycliang@andestech.com>

Re: [PATCH v3 2/3] riscv: Add SPL_ZERO_MEM_BEFORE_USE implementation

[Leo Liang]

Hi Shengyu,

On Tue, Aug 08, 2023 at 08:39:56PM +0800, Shengyu Qu wrote:
> Add the actual support code for SPL_ZERO_MEM_BEFORE_USE and remove
> existing Starfive JH7110's L2 LIM clean code, since existing code has
> following issues:
>  1. Each hart (in the middle of a function call) overwriting its own
>     stack and other harts' stacks.
>     (data-race and data-corruption)
>  2. Lottery winner hart can be doing "board_init_f_init_reserve",
>     while other harts are in the middle of zeroing L2 LIM.
>     (data-race)
> 
> Signed-off-by: Bo Gan <ganboing@gmail.com>
> Signed-off-by: Shengyu Qu <wiagn233@outlook.com>
> ---
> Changes since v2:
>  - Fix typo (ZERO_MEM_BEFORE_USE to SPL_ZERO_MEM_BEFORE_USE)
> ---
>  arch/riscv/cpu/jh7110/spl.c | 25 -------------------------
>  arch/riscv/cpu/start.S      | 12 ++++++++++++
>  common/init/board_init.c    |  3 +++
>  3 files changed, 15 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/riscv/cpu/jh7110/spl.c b/arch/riscv/cpu/jh7110/spl.c
> index 72adcefa0e..4047b10efe 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;
>  
> @@ -59,9 +58,6 @@ int spl_soc_init(void)
>  
>  void harts_early_init(void)
>  {
> -	ulong *ptr;
> -	u8 *tmp;
> -	ulong len, remain;
>  	/*
>  	 * Feature Disable CSR
>  	 *
> @@ -70,25 +66,4 @@ void harts_early_init(void)
>  	 */
>  	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;
> -	}
>  }
> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
> index 59d58a5a57..930309d8d2 100644
> --- a/arch/riscv/cpu/start.S
> +++ b/arch/riscv/cpu/start.S
> @@ -111,6 +111,18 @@ call_board_init_f:
>   * It's essential before any function call, otherwise, we get data-race.
>   */
>  
> +/* clear stack if necessary */
> +#if CONFIG_IS_ENABLED(SPL_ZERO_MEM_BEFORE_USE)

I think what you have in v2 patch was the correct one.

Could you refer to include/linux/kconfig.h to check if 
this usage fits your expectation ?

> +clear_stack:
> +	li	t1, 1
> +	slli	t1, t1, CONFIG_STACK_SIZE_SHIFT
> +	sub	t1, sp, t1
> +clear_stack_loop:
> +	SREG	zero, 0(t1)		/* t1 is always 16 byte aligned */
> +	addi	t1, t1, REGBYTES
> +	blt	t1, sp, clear_stack_loop
> +#endif
> +
>  call_board_init_f_0:
>  	/* find top of reserve space */
>  #if CONFIG_IS_ENABLED(SMP)
> diff --git a/common/init/board_init.c b/common/init/board_init.c
> index 96ffb79a98..51d9ec9a13 100644
> --- a/common/init/board_init.c
> +++ b/common/init/board_init.c
> @@ -162,6 +162,9 @@ 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;
> +#if CONFIG_IS_ENABLED(SPL_ZERO_MEM_BEFORE_USE)

Ditto.

Best regards,
Leo
> +	memset((void *)base, '\0', CONFIG_VAL(SYS_MALLOC_F_LEN));
> +#endif
>  #endif
>  
>  	if (CONFIG_IS_ENABLED(SYS_REPORT_STACK_F_USAGE))
> -- 
> 2.41.0
> 

Re: [PATCH v3 3/3] riscv: cpu: jh7110: Select SPL_ZERO_MEM_BEFORE_USE

[Leo Liang]

On Tue, Aug 08, 2023 at 08:39:57PM +0800, Shengyu Qu wrote:
> Add Kconfig item for Starfive JH7110 to select SPL_ZERO_MEM_BEFORE_USE.
> 
> Signed-off-by: Bo Gan <ganboing@gmail.com>
> Signed-off-by: Shengyu Qu <wiagn233@outlook.com>
> ---
>  arch/riscv/cpu/jh7110/Kconfig | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Leo Yu-Chi Liang <ycliang@andestech.com>

Re: [PATCH v3 2/3] riscv: Add SPL_ZERO_MEM_BEFORE_USE implementation

[Shengyu Qu]

[-- Attachment #1.1.1: Type: text/plain, Size: 3980 bytes --]

Hi Leo,

Seems you are right. I'll send v4 to fix this. Thank you.

Best regards,

Shengyu

> Hi Shengyu,
>
> On Tue, Aug 08, 2023 at 08:39:56PM +0800, Shengyu Qu wrote:
>> Add the actual support code for SPL_ZERO_MEM_BEFORE_USE and remove
>> existing Starfive JH7110's L2 LIM clean code, since existing code has
>> following issues:
>>   1. Each hart (in the middle of a function call) overwriting its own
>>      stack and other harts' stacks.
>>      (data-race and data-corruption)
>>   2. Lottery winner hart can be doing "board_init_f_init_reserve",
>>      while other harts are in the middle of zeroing L2 LIM.
>>      (data-race)
>>
>> Signed-off-by: Bo Gan <ganboing@gmail.com>
>> Signed-off-by: Shengyu Qu <wiagn233@outlook.com>
>> ---
>> Changes since v2:
>>   - Fix typo (ZERO_MEM_BEFORE_USE to SPL_ZERO_MEM_BEFORE_USE)
>> ---
>>   arch/riscv/cpu/jh7110/spl.c | 25 -------------------------
>>   arch/riscv/cpu/start.S      | 12 ++++++++++++
>>   common/init/board_init.c    |  3 +++
>>   3 files changed, 15 insertions(+), 25 deletions(-)
>>
>> diff --git a/arch/riscv/cpu/jh7110/spl.c b/arch/riscv/cpu/jh7110/spl.c
>> index 72adcefa0e..4047b10efe 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;
>>   
>> @@ -59,9 +58,6 @@ int spl_soc_init(void)
>>   
>>   void harts_early_init(void)
>>   {
>> -	ulong *ptr;
>> -	u8 *tmp;
>> -	ulong len, remain;
>>   	/*
>>   	 * Feature Disable CSR
>>   	 *
>> @@ -70,25 +66,4 @@ void harts_early_init(void)
>>   	 */
>>   	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;
>> -	}
>>   }
>> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
>> index 59d58a5a57..930309d8d2 100644
>> --- a/arch/riscv/cpu/start.S
>> +++ b/arch/riscv/cpu/start.S
>> @@ -111,6 +111,18 @@ call_board_init_f:
>>    * It's essential before any function call, otherwise, we get data-race.
>>    */
>>   
>> +/* clear stack if necessary */
>> +#if CONFIG_IS_ENABLED(SPL_ZERO_MEM_BEFORE_USE)
> I think what you have in v2 patch was the correct one.
>
> Could you refer to include/linux/kconfig.h to check if
> this usage fits your expectation ?
>
>> +clear_stack:
>> +	li	t1, 1
>> +	slli	t1, t1, CONFIG_STACK_SIZE_SHIFT
>> +	sub	t1, sp, t1
>> +clear_stack_loop:
>> +	SREG	zero, 0(t1)		/* t1 is always 16 byte aligned */
>> +	addi	t1, t1, REGBYTES
>> +	blt	t1, sp, clear_stack_loop
>> +#endif
>> +
>>   call_board_init_f_0:
>>   	/* find top of reserve space */
>>   #if CONFIG_IS_ENABLED(SMP)
>> diff --git a/common/init/board_init.c b/common/init/board_init.c
>> index 96ffb79a98..51d9ec9a13 100644
>> --- a/common/init/board_init.c
>> +++ b/common/init/board_init.c
>> @@ -162,6 +162,9 @@ 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;
>> +#if CONFIG_IS_ENABLED(SPL_ZERO_MEM_BEFORE_USE)
> Ditto.
>
> Best regards,
> Leo
>> +	memset((void *)base, '\0', CONFIG_VAL(SYS_MALLOC_F_LEN));
>> +#endif
>>   #endif
>>   
>>   	if (CONFIG_IS_ENABLED(SYS_REPORT_STACK_F_USAGE))
>> -- 
>> 2.41.0
>>

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 6977 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]