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

[Stephen Warren <swarren@wwwdotorg.org>]

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.