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

[Siarhei Siamashka]

On Wed, 15 Oct 2014 17:40:38 +0100
Marc Zyngier <marc.zyngier at arm.com> wrote:

> On Wed, Oct 15 2014 at 03:05:24 PM, Siarhei Siamashka <siarhei.siamashka at gmail.com> wrote:
> > On Wed, 15 Oct 2014 13:42:33 +0100
> > Marc Zyngier <marc.zyngier at arm.com> wrote:
> >
> >> On Wed, Oct 15 2014 at 11:40:24 AM, Siarhei Siamashka
> >> <siarhei.siamashka at gmail.com> wrote:
> >> > On Wed, 15 Oct 2014 11:31:44 +0100
> >> > Marc Zyngier <marc.zyngier at arm.com> wrote:
> >> >
> >> >> On Wed, Oct 15 2014 at 11:25:10 AM, Siarhei Siamashka
> >> >> <siarhei.siamashka at gmail.com> wrote:
> >> >> > On Wed, 15 Oct 2014 12:13:05 +0200
> >> >> > Hans de Goede <hdegoede at redhat.com> wrote:
> >> >> >
> >> >> >> Older Linux kernels will not properly boot in hype mode, add support for a
> >> >> >> bootm_boot_mode environment variable, which when set to "sec" will cause
> >> >> >> u-boot to boot in secure mode even when build with non-sec (and hyp) support.
> >> >> >> 
> >> >> >> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> >> >> >> ---
> >> >> >>  arch/arm/lib/bootm.c | 23 ++++++++++++++++++-----
> >> >> >>  1 file changed, 18 insertions(+), 5 deletions(-)
> >> >> >> 
> >> >> >> diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
> >> >> >> index 39fe7a1..037fc8d 100644
> >> >> >> --- a/arch/arm/lib/bootm.c
> >> >> >> +++ b/arch/arm/lib/bootm.c
> >> >> >> @@ -235,6 +235,18 @@ static void boot_prep_linux(bootm_headers_t *images)
> >> >> >>  	}
> >> >> >>  }
> >> >> >>  
> >> >> >> +#if defined(CONFIG_ARMV7_NONSEC) || defined(CONFIG_ARMV7_VIRT)
> >> >> >> +static bool boot_nonsec(void)
> >> >> >> +{
> >> >> >> +	char *s = getenv("bootm_boot_mode");
> >> >> >> +
> >> >> >> +	if (s && !strcmp(s, "sec"))
> >> >> >> +		return false;
> >> >> >> +
> >> >> >> +	return true;
> >> >> >> +}
> >> >> >> +#endif
> >> >> >> +
> >> >> >>  /* Subcommand: GO */
> >> >> >>  static void boot_jump_linux(bootm_headers_t *images, int flag)
> >> >> >>  {
> >> >> >> @@ -283,12 +295,13 @@ static void boot_jump_linux(bootm_headers_t *images, int flag)
> >> >> >>  
> >> >> >>  	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);
> >> >> >>  	}
> >> >> >>  #endif
> >> >> >>  }
> >> >> >
> >> >> > BTW, I'm currently achieving the same result, but without the need to do
> >> >> > extra environment configuration gymnastics for the end users:
> >> >> >     https://github.com/ssvb/u-boot-sunxi-dram/commit/e914abe551e712a59a8fc1ac9a4b8490e8e7c866
> >> >> 
> >> >> And thus preventing kernels with appended DT (not provided by u-boot),
> >> >> but still using HYP/non-sec/PSCI from booting.
> >> >> 
> >> >> Sorry, but using DT as a way to distinguish between all these cases is
> >> >> just wrong.
> >> >> 
> >> >> Thanks,
> >> >
> >> > Do you see the HACK prefix in the summary of that patch?
> >> 
> >> I do. But the fact that you even mention it here makes me think that you
> >> believe it is a better approach that Hans'.
> >
> > The need to meddle with the environment variables makes Hans' approach
> > immensely less useful for the end users.
> >
> > In your Ivory Tower, you may think that you have resolved the problem.
> > But in reality, u-boot will still have to be patched before deploying
> > it to the end users in order to prevent them from running into troubles.
> 
> My position in a nice ivory tower allows me to see further than you seem
> to do, and makes me prefer robust solutions, as opposed to random hacks
> that impact unsuspecting users by modifying the way things gets booted
> based on decisions made by someone who may or may not have any idea of
> what said users are trying to achieve.

Basically, your reasoning boils down to "I'm a cool guy, so I know
it better than you". And you just quoted only the irrelevant part,
skipping the rest. Not sure if participating in this kind of debate
makes any sense for me.

> Hans' solution is simple, robust, and doesn't randomly change
> behaviour. That's definitely good enough for me.

To sum it up:

1) I'm in favour of having a way to toggle between booting in secure and
   non-secure mode
2) I'm neutral to the idea of adding this new environment variable and
   have no objections. Especially if it is also useful on non-sunxi
   hardware.
3) I still think that u-boot has enough information at runtime to make
   a reliable decision (on sunxi hardware) without any need for the
   extra environment variable, but we just agree to disagree.

And since the patch from Hans is moving things in the right direction

Acked-by: Siarhei Siamashka <siarhei.siamashka at gmail.com>

Have a nice day :-)

-- 
Best regards,
Siarhei Siamashka