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

[Ian Campbell]

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);
        }

Ian.