[U-Boot] [PATCH v3 2/3] efi_loader: ARM: run EFI payloads non-secure

[Mark Kettenis]

> From: Marc Zyngier <marc.zyngier at arm.com>
> Date: Fri, 15 Jun 2018 08:59:59 +0100
> 
> On 14/06/18 21:55, Mark Kettenis wrote:
> >> From: Marc Zyngier <marc.zyngier at arm.com>
> >> Date: Thu, 14 Jun 2018 12:54:53 +0100
> >>
> >> On 13/06/18 23:41, Mark Kettenis wrote:
> >>> If desired (and possible) switch into HYP mode or non-secure SVC mode
> >>> before calling the entry point of an EFI application.  This allows
> >>> U-Boot to provide a usable PSCI implementation and makes it possible
> >>> to boot kernels into hypervisor mode using an EFI bootloader.
> >>>
> >>> Based on diffs from Heinrich Schuchardt and Alexander Graf.
> >>>
> >>> Signed-off-by: Mark Kettenis <kettenis at openbsd.org>
> >>> ---
> >>>  cmd/bootefi.c | 32 ++++++++++++++++++++++++++++++++
> >>>  1 file changed, 32 insertions(+)
> >>>
> >>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >>> index 707d159bac..12a6b84ce6 100644
> >>> --- a/cmd/bootefi.c
> >>> +++ b/cmd/bootefi.c
> >>> @@ -20,6 +20,11 @@
> >>>  #include <asm-generic/unaligned.h>
> >>>  #include <linux/linkage.h>
> >>>  
> >>> +#ifdef CONFIG_ARMV7_NONSEC
> >>> +#include <asm/armv7.h>
> >>> +#include <asm/secure.h>
> >>> +#endif
> >>> +
> >>>  DECLARE_GLOBAL_DATA_PTR;
> >>>  
> >>>  #define OBJ_LIST_NOT_INITIALIZED 1
> >>> @@ -189,6 +194,18 @@ static efi_status_t efi_run_in_el2(EFIAPI efi_status_t (*entry)(
> >>>  }
> >>>  #endif
> >>>  
> >>> +#ifdef CONFIG_ARMV7_NONSEC
> >>> +static efi_status_t efi_run_in_hyp(EFIAPI efi_status_t (*entry)(
> >>> +			efi_handle_t image_handle, struct efi_system_table *st),
> >>> +			efi_handle_t image_handle, struct efi_system_table *st)
> >>> +{
> >>> +	/* Enable caches again */
> >>> +	dcache_enable();
> >>> +
> >>> +	return efi_do_enter(image_handle, st, entry);
> >>> +}
> >>> +#endif
> >>> +
> >>>  /* Carve out DT reserved memory ranges */
> >>>  static efi_status_t efi_carve_out_dt_rsv(void *fdt)
> >>>  {
> >>> @@ -338,6 +355,21 @@ static efi_status_t do_bootefi_exec(void *efi,
> >>>  	}
> >>>  #endif
> >>>  
> >>> +#ifdef CONFIG_ARMV7_NONSEC
> >>> +	if (armv7_boot_nonsec()) {
> >>> +		dcache_disable();	/* flush cache before switch to HYP */
> >>> +
> >>
> >> What is the rational for disabling/enabling caches across the transition
> >> to HYP? I'm sure there is a good reason, but I'd rather see it explained
> >> here.
> > 
> > Can't say I fully understan why.  But the AArch64 code does this as
> > well and if I don't flush the cache here the contents of efi_gd (which
> > gets initialized before the switch) sometimes gets lost.
> 
> I guess the following can happen:
> 
> - EL1 code (or SVC for AArch32) has its MMU enabled, and caches are on
> - Writes from EL1 are nicely sitting in the dcache
> - Enter EL2 (HYP) where the MMU is off, and thus the caches are too
> - The uncached accesses do not hit in the cache, and sh*t happens
> 
> dcache_disable also cleans to the PoC, making sure that everything is
> visible even when the MMU and caches are off. I have the strong feeling
> that dcache_enable is utterly useless as I don't think you install any
> page table at HYP (that code was never designed to run anything other
> than jumping into the kernel).

There actually is code in arch/arm/lib/cache-cp15.c to set up the page
tables for HYP and enable the MMU.  And it would run as part of the
dcache_enable() call if CONFIG_ARMV7_LPAE was defined.  But that isn't
set on the boards I'm looking at.  I'll have a go at enabling that option.

> It would make a lot more sense if you installed id-mapped page tables at
> HYP too in order to enable the caches, and geta  bit of performance back
> (otherwise anything you run at HYP will negatively compare to the speed
> of an anaemic snail stuck on sand).

On the Allwinner A20 it certainly does crawl; console output is really
slow.  Didn't notice it on the NXP i.MX7D board though.  Anyway,
thanks for the hint!