[U-Boot] [PATCH v2] ARM: bootm: Allow booting in secure mode on hyp capable systems

[Ian Campbell]

On Thu, 2014-10-23 at 10:22 +0200, Hans de Goede wrote:
> Hi Ian,
> 
> On 10/22/2014 08:55 PM, Ian Campbell wrote:
> > On Wed, 2014-10-22 at 15:45 +0200, Hans de Goede wrote:
> >> 	if (!fake) {
> >>  #if defined(CONFIG_ARMV7_NONSEC) || defined(CONFIG_ARMV7_VIRT)
> >> -		armv7_init_nonsec();
> >> -		secure_ram_addr(_do_nonsec_entry)(kernel_entry,
> >> -						  0, machid, r2);
> >> -#else
> >> -		kernel_entry(0, machid, r2);
> >> +		if (boot_nonsec()) {
> >> +			armv7_init_nonsec();
> >> +			secure_ram_addr(_do_nonsec_entry)(kernel_entry,
> >> +							  0, machid, r2);
> >> +		}
> >>  #endif
> >> +		kernel_entry(0, machid, r2);
> > 
> > There's a subtle different here, which is that this final kernel_entry
> > call used to be in the #else clause, and so emitted for the NONSEC ||
> > VIRT case. So if the _do_nonsec_entry call were to fail (not currently
> > possible) and return you'd end up trying again via the sec path.
> > 
> > I'm not sure that's a bad thing, but it is a difference so it'd be good
> > to know it was a deliberate choice (or not).
> 
> I was under the assumption that do_nonsec_entry would never fail, and would
> not return, which is why I wrote this code the way I wrote it.

AFAICT in practice it can't fail today, but if it were somehow modified
in the future to do so this would expose some slightly surprising
behaviour.

>  I'm not sure
> if retrying in secure mode meets the principle of least surprise, so I guess
> the #if .. #endif block should probably get an "else" added before the #endif,
> do you agree?

Yes.

BTW, if you put the #ifdef around boot_nonsec() instead and make the
#else case #define boot_nonsec() (0) then does that end up looking
cleaner here at the caller? Maybe that causes knockons with the
prototypes for the unused functions in that case, in which case I doubt
it is worth it.

Ian.