On 01/16/2015 02:39 AM, Ian Campbell wrote:
On Fri, 2015-01-16 at 09:52 +0100, Thierry Reding wrote:
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.
I started off with this but then removed it as redundant, but you are right that it makes it more obvious what is happening, and hence isn't really redundant at all. I'll add it back.
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.
Exactly, the FDT reservation needs to precisely match what the hardware is protecting, which has MB granularity on this platform.
So unless explicitly specified we'd need a way for platforms to be able to adjust the reserved region accordingly.
How about if CONFIG_ARMV7_SECURE_SIZE is set we reserve that amount, otherwise we reserve __secure_end - __secure_start, with the proposed SECURE_BASE_IS_IN_DRAM || !SECURE_BASE handling surrounding that?
IOW modifying Stephen's suggestion to something like:
#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 #if defined(CONFIG_ARMV7_SECURE_SIZE) size = CONFIG_ARMV7_SECURE_SIZE; #else size = __secure_end - __secure_start; #endif fdt_add_mem_rsv(fdt, base, size); #endif return fdt_psci(fdt); }
That sounds nice and orthogonal/flexible:-)
If we want to, that scheme is pretty easy to extend with a run-time hook to "round" the value of size at run-time, rather than hard-coding it in a config file, if we ever need that.