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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
