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

Message ID 20180613224108.13372-3-kettenis@openbsd.org
State Superseded
Delegated to: Alexander Graf
Headers show
Series
  • efi_loader: ARM: add support for ARMV7_NONSEC=y
Related show

Commit Message

Mark Kettenis June 13, 2018, 10:41 p.m.
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@openbsd.org>
---
 cmd/bootefi.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

Comments

Marc Zyngier June 14, 2018, 11:54 a.m. | #1
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@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.

> +		armv7_init_nonsec();
> +		secure_ram_addr(_do_nonsec_entry)(efi_run_in_hyp,
> +				(uintptr_t)entry,
> +				(uintptr_t)loaded_image_info_obj.handle,
> +				(uintptr_t)&systab);
> +
> +		/* Should never reach here, efi exits with longjmp */
> +		while (1) { }
> +	}
> +#endif
> +
>  	ret = efi_do_enter(loaded_image_info_obj.handle, &systab, entry);
>  
>  exit:
> 

Thanks,

	M.
Mark Kettenis June 14, 2018, 8:55 p.m. | #2
> From: Marc Zyngier <marc.zyngier@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@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.

> > +		armv7_init_nonsec();
> > +		secure_ram_addr(_do_nonsec_entry)(efi_run_in_hyp,
> > +				(uintptr_t)entry,
> > +				(uintptr_t)loaded_image_info_obj.handle,
> > +				(uintptr_t)&systab);
> > +
> > +		/* Should never reach here, efi exits with longjmp */
> > +		while (1) { }
> > +	}
> > +#endif
> > +
> >  	ret = efi_do_enter(loaded_image_info_obj.handle, &systab, entry);
> >  
> >  exit:
> >
Marc Zyngier June 15, 2018, 7:59 a.m. | #3
On 14/06/18 21:55, Mark Kettenis wrote:
>> From: Marc Zyngier <marc.zyngier@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@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).

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).

Thanks,

	M.
Mark Kettenis June 15, 2018, 12:51 p.m. | #4
> From: Marc Zyngier <marc.zyngier@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@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@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!
Marc Zyngier June 15, 2018, 1:20 p.m. | #5
On 15/06/18 13:51, Mark Kettenis wrote:
>> From: Marc Zyngier <marc.zyngier@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@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@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.

Yeah, you most definitely want to have that one, and LPAE is the only
thing that makes sense if you have a virtualization-capable CPU.

> 
>> 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!

No worries.

	M.

Patch

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 */
+
+		armv7_init_nonsec();
+		secure_ram_addr(_do_nonsec_entry)(efi_run_in_hyp,
+				(uintptr_t)entry,
+				(uintptr_t)loaded_image_info_obj.handle,
+				(uintptr_t)&systab);
+
+		/* Should never reach here, efi exits with longjmp */
+		while (1) { }
+	}
+#endif
+
 	ret = efi_do_enter(loaded_image_info_obj.handle, &systab, entry);
 
 exit: