On Thu, Jan 15, 2015 at 04:59:12PM -0700, Stephen Warren wrote:
On 01/13/2015 12:45 PM, Ian Campbell wrote:
The secure world code is relocated to the MB just below the top of 4G, we reserve it in the FDT (by setting CONFIG_ARMV7_SECURE_RESERVE_SIZE) but it is not protected in h/w. See next patch.
diff --git a/include/configs/jetson-tk1.h b/include/configs/jetson-tk1.h
+#define CONFIG_ARMV7_PSCI 1 +/* Reserve top 1M for secure RAM */ +#define CONFIG_ARMV7_SECURE_BASE 0xfff00000 +#define CONFIG_ARMV7_SECURE_RESERVE_SIZE 0x00100000
I /think/ the assumption in the existing code is that CONFIG_ARMV7_SECURE_BASE is the base of some out-of-DRAM secure memory, and hence that's why arch/arm/cpu/armv7/virt-dt.c() only reserves memory if that symbol is *not* set? That seems like rather a confusing semantic given the variable name. Introducing a new define that looks like it's simply the size of that region but actually changes the reservation semantics makes the situation worse for me.
Wouldn't it be better to have:
CONFIG_ARMV7_SECURE_BASE defines where the secure code is copied to.
CONFIG_ARMV7_SECURE_BASE_IS_IN_DRAM defines the obvious; whether the secure base is in DRAM or not.
That define would default to unset and you'd get the current behaviour.
If that define was set, then CONFIG_ARMV7_SECURE_BASE through CONFIG_ARMV7_SECURE_BASE + (__secure_end - __secure_start) would be reserved in RAM?
That way, armv7_update_dt would be more like:
int armv7_update_dt(void *fdt) { #if defined(CONFIG_ARMV7_SECURE_BASE_IS_IN_DRAM) || \ !defined(CONFIG_ARMV7_SECURE_BASE) /* secure code lives in RAM, keep it alive */ #if defined(CONFIG_ARMV7_SECURE_BASE) base = CONFIG_ARMV7_SECURE_BASE; #else base = __secure_start; #endif fdt_add_mem_rsv(fdt, base, __secure_end - __secure_start); #endif
return fdt_psci(fdt);}
As I understand it, one of the purposes of the RESERVE_SIZE is that hardware may not allow regions of arbitrary size to be reserved. On Tegra for example I think the restriction is that memory can only be secured on 1 MiB boundaries.
So unless explicitly specified we'd need a way for platforms to be able to adjust the reserved region accordingly.
Thierry