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.
