From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Warren Date: Mon, 19 Jan 2015 10:17:31 -0700 Subject: [U-Boot] [PATCH v1 3/4] jetson-tk1: Add PSCI configuration options and reserve secure code In-Reply-To: <1421401190.19839.22.camel@hellion.org.uk> References: <1421178290.11796.159.camel@hellion.org.uk> <1421178360-23778-3-git-send-email-ijc@hellion.org.uk> <54B85450.1030504@wwwdotorg.org> <20150116085224.GA9170@ulmo.nvidia.com> <1421401190.19839.22.camel@hellion.org.uk> Message-ID: <54BD3C2B.3040202@wwwdotorg.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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.