From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Date: Thu, 23 Oct 2014 10:30:56 +0100 Subject: [U-Boot] [PATCH v2] ARM: bootm: Allow booting in secure mode on hyp capable systems In-Reply-To: <5448BAC8.1000104@redhat.com> References: <1413985502-19257-1-git-send-email-hdegoede@redhat.com> <1414004103.20604.72.camel@hellion.org.uk> <5448BAC8.1000104@redhat.com> Message-ID: <1414056656.19198.26.camel@hellion.org.uk> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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.