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