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