[U-Boot] [PATCH v1 3/4] jetson-tk1: Add PSCI configuration options and reserve secure code

[Thierry Reding <treding@nvidia.com>]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150116/428f8694/attachment.pgp>