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

[Hans de Goede]

Hi,

On 10/23/2014 11:30 AM, Ian Campbell wrote:
> 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.

The problem there is that do_nonsec_entry is not defined in that case, so
we really need an #ifdef there.

Regards,

Hans