[U-Boot] [RFC 1/1] efi_loader: refactor switch to hypervisor mode

[Alexander Graf]

On 25.12.18 09:26, Heinrich Schuchardt wrote:
> Refactor the switch from supervisor to hypervisor to a new function called at
> the beginning of do_bootefi().

Why?

> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
> ---
> With this patch I am just moving around the switch from supervisor to
> hypervisor mode within the EFI subsystem. Similar switching also occurs
> in all other boot commands (cf. arch/arm/lib/bootm.c).
> 
> Why are we running the U-Boot console in supervisor mode at all? Wouldn't
> it be advisable for security reasons to switch to hypervisor mode before
> entering the console?
> 
> Best regards
> 
> Heinrich
> ---
>  cmd/bootefi.c | 109 ++++++++++++++++++++++----------------------------
>  1 file changed, 47 insertions(+), 62 deletions(-)
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 1aaf5319e13..277d4863953 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -31,9 +31,9 @@ DECLARE_GLOBAL_DATA_PTR;
>  #define OBJ_LIST_NOT_INITIALIZED 1
>  
>  static efi_status_t efi_obj_list_initialized = OBJ_LIST_NOT_INITIALIZED;
> -
>  static struct efi_device_path *bootefi_image_path;
>  static struct efi_device_path *bootefi_device_path;
> +struct jmp_buf_data hyp_jmp;
>  
>  /* Initialize and populate EFI object list */
>  efi_status_t efi_init_obj_list(void)
> @@ -122,6 +122,50 @@ void __weak allow_unaligned(void)
>  {
>  }
>  
> +/**
> + * entry_hyp() - entry point when switching to hypervisor mode
> + */
> +static void entry_hyp(void)

Potentially unused function depending on config settings?

> +{
> +	dcache_enable();
> +	debug("Reached HYP mode\n");

HYP is a 32bit ARM term. AArch64 does not know what HYP is - there it's EL2.

Also keep in mind that this is not 100% ARM specific. We might see
something along the lines of this logic on RISC-V eventually too.

> +	longjmp(&hyp_jmp, 1);
> +}
> +
> +/**
> + * leave_supervisor() - switch to hypervisor mode
> + */
> +static void leave_supervisor(void)

That's not necessarily what this function does. It may switch from
EL1->EL2 or from EL3->EL2.

> +{
> +#ifdef CONFIG_ARMV7_NONSEC
> +	static bool is_nonsec;
> +
> +	if (armv7_boot_nonsec() && !is_nonsec) {
> +		if (setjmp(&hyp_jmp))
> +			return;
> +		dcache_disable();	/* flush cache before switch to HYP */
> +		armv7_init_nonsec();
> +		is_nonsec = true;
> +		secure_ram_addr(_do_nonsec_entry)(entry_hyp, 0, 0, 0);
> +	}
> +#endif
> +
> +#ifdef CONFIG_ARM64
> +	/* On AArch64 we need to make sure we call our payload in < EL3 */
> +	if (current_el() == 3) {
> +		if (setjmp(&hyp_jmp))
> +			return;
> +		smp_kick_all_cpus();
> +		dcache_disable();	/* flush cache before switch to EL2 */
> +
> +		/* Move into EL2 and keep running there */
> +		armv8_switch_to_el2(0, 0, 0, 0, (uintptr_t)entry_hyp,
> +				    ES_TO_AARCH64);
> +	}
> +#endif
> +	return;
> +}
> +
>  /*
>   * Set the load options of an image from an environment variable.
>   *
> @@ -233,34 +277,6 @@ static efi_status_t efi_do_enter(
>  	return ret;
>  }
>  
> -#ifdef CONFIG_ARM64
> -static efi_status_t efi_run_in_el2(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
> -
> -#ifdef CONFIG_ARMV7_NONSEC
> -static bool is_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();
> -
> -	is_nonsec = true;
> -
> -	return efi_do_enter(image_handle, st, entry);
> -}
> -#endif
> -
>  /*
>   * efi_carve_out_dt_rsv() - Carve out DT reserved memory ranges
>   *
> @@ -440,39 +456,6 @@ static efi_status_t do_bootefi_exec(void *efi,
>  		goto err_prepare;
>  	}
>  
> -#ifdef CONFIG_ARM64
> -	/* On AArch64 we need to make sure we call our payload in < EL3 */
> -	if (current_el() == 3) {
> -		smp_kick_all_cpus();
> -		dcache_disable();	/* flush cache before switch to EL2 */
> -
> -		/* Move into EL2 and keep running there */
> -		armv8_switch_to_el2((ulong)entry,
> -				    (ulong)&image_obj->header,
> -				    (ulong)&systab, 0, (ulong)efi_run_in_el2,
> -				    ES_TO_AARCH64);
> -
> -		/* Should never reach here, efi exits with longjmp */
> -		while (1) { }
> -	}
> -#endif
> -
> -#ifdef CONFIG_ARMV7_NONSEC
> -	if (armv7_boot_nonsec() && !is_nonsec) {
> -		dcache_disable();	/* flush cache before switch to HYP */
> -
> -		armv7_init_nonsec();
> -		secure_ram_addr(_do_nonsec_entry)(
> -					efi_run_in_hyp,
> -					(uintptr_t)entry,
> -					(uintptr_t)&image_obj->header,
> -					(uintptr_t)&systab);
> -
> -		/* Should never reach here, efi exits with longjmp */
> -		while (1) { }
> -	}
> -#endif
> -
>  	ret = efi_do_enter(&image_obj->header, &systab, entry);
>  
>  err_prepare:
> @@ -558,6 +541,8 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  	/* Allow unaligned memory access */
>  	allow_unaligned();
>  
> +	leave_supervisor();

This means you're potentially executing object initialization code in a
mode it wasn't meant to be run in. Is that a smart thing to do? It's
definitely more than just refactoring.

If you just find the do_bootefi_exec() function too hard to read,
refactoring it into calling an arch_efi_do_enter() call is probably
smarter. Then aarch64 and 32bit arm can just provide their own "get be
into target mode" functions within their own arch directories.


Alex

> +
>  	/* Initialize EFI drivers */
>  	r = efi_init_obj_list();
>  	if (r != EFI_SUCCESS) {
>