[U-Boot] [PATCH v2 3/4] efi_loader: use efi_start_image() for bootefi

[Mark Kettenis <mark.kettenis@xs4all.nl>]

> Date: Thu, 18 Jul 2019 16:00:16 +1000
> From: Jonathan Gray <jsg@jsg.id.au>
> 
> On Fri, Feb 08, 2019 at 07:46:49PM +0100, Heinrich Schuchardt wrote:
> > Remove the duplicate code in efi_do_enter() and use efi_start_image() to
> > start the image invoked by the bootefi command.
> > 
> > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > ---
> > v2
> > 	use EFI_CALL
> 
> This commit broke booting OpenBSD/armv7 kernels on mx6cuboxi with U-Boot
> releases after 2019.01.  2019.04 works if this commit is reverted.  With
> 2019.07 there are conflicts trying to revert it and it is still broken
> as released.
> 
> f69d63fae281ba98c3d063097cf4e95d17f3754d is the first bad commit
> commit f69d63fae281ba98c3d063097cf4e95d17f3754d
> Author: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Date:   Wed Dec 26 13:28:09 2018 +0100
> 
>     efi_loader: use efi_start_image() for bootefi
>     
>     Remove the duplicate code in efi_do_enter() and use efi_start_image() to
>     start the image invoked by the bootefi command.
>     
>     Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> 
>  cmd/bootefi.c                 | 22 +---------------------
>  include/efi_loader.h          |  4 ++++
>  lib/efi_loader/efi_boottime.c |  6 +++---
>  3 files changed, 8 insertions(+), 24 deletions(-)

Hi Jonathan,

With this commit the OpenBSD bootloader (an EFI application) still
boots, but the loaded OpenBSD kernel doesn't isn't it?

I bet the problem here is that efi_start_image() sets
efi_is_direct_boot to false, which means that efi_exit_caches() which
runs as a result of calling ExitBootServices() no longer
flushes/disables the caches on 32-bit ARM.

We have been here before.  It really isn't possible to flush/disable
the L2 cache on most 32-bit ARM implementations without SoC-specific
code.  And having such code in the early kernel bootstrap code isn't
really possible.

The comments in the code suggest that loading an EFI Linux kernel
directly from U-Boot somehow works without flushing the caches.  But I
don't understand how.  But I'm pretty sure this change break booting
non-EFI Linux kernels through grub on 32-bit ARM platforms as well.

The UEFI spec has the following text (UEFI 2.5 section 2.3.5 "AArch32
Platforms"):

  Implementations of boot services will enable architecturally
  manageable caches and TLBs i.e., those that can be managed directly
  using CP15 operations using mechanisms and procedures defined in the
  ARM Architecture Reference Manual. They should not enable caches
  requiring platform information to manage or invoke non-architectural
  cache/TLB lockdown mechanisms

This suggests that the real problem here is that U-Boot enables the L2
cache on i.MX6 which violates the spec.  But things will probably run
notably slower without the L2 cache enabled.  Maybe the answer is to
just flush/disable the L2 cache when ExitBootServices() is called?


> > ---
> >  cmd/bootefi.c                 | 22 +---------------------
> >  include/efi_loader.h          |  4 ++++
> >  lib/efi_loader/efi_boottime.c |  6 +++---
> >  3 files changed, 8 insertions(+), 24 deletions(-)
> > 
> > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > index 7f9913c0ee..a2d38256e9 100644
> > --- a/cmd/bootefi.c
> > +++ b/cmd/bootefi.c
> > @@ -133,20 +133,6 @@ done:
> >  	return ret;
> >  }
> >  
> > -static efi_status_t efi_do_enter(
> > -			efi_handle_t image_handle, struct efi_system_table *st,
> > -			EFIAPI efi_status_t (*entry)(
> > -				efi_handle_t image_handle,
> > -				struct efi_system_table *st))
> > -{
> > -	efi_status_t ret = EFI_LOAD_ERROR;
> > -
> > -	if (entry)
> > -		ret = entry(image_handle, st);
> > -	st->boottime->exit(image_handle, ret, 0, NULL);
> > -	return ret;
> > -}
> > -
> >  /*
> >   * efi_carve_out_dt_rsv() - Carve out DT reserved memory ranges
> >   *
> > @@ -315,13 +301,7 @@ static efi_status_t do_bootefi_exec(void *efi,
> >  
> >  	/* Call our payload! */
> >  	debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);
> > -
> > -	if (setjmp(&image_obj->exit_jmp)) {
> > -		ret = image_obj->exit_status;
> > -		goto err_prepare;
> > -	}
> > -
> > -	ret = efi_do_enter(&image_obj->header, &systab, image_obj->entry);
> > +	ret = EFI_CALL(efi_start_image(&image_obj->header, NULL, NULL));
> >  
> >  err_prepare:
> >  	/* image has returned, loaded-image obj goes *poof*: */
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index 3ce43f7a6f..512880ab8f 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -320,6 +320,10 @@ efi_status_t efi_create_handle(efi_handle_t *handle);
> >  void efi_delete_handle(efi_handle_t obj);
> >  /* Call this to validate a handle and find the EFI object for it */
> >  struct efi_object *efi_search_obj(const efi_handle_t handle);
> > +/* Start image */
> > +efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
> > +				    efi_uintn_t *exit_data_size,
> > +				    u16 **exit_data);
> >  /* Find a protocol on a handle */
> >  efi_status_t efi_search_protocol(const efi_handle_t handle,
> >  				 const efi_guid_t *protocol_guid,
> > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> > index 7a61a905f4..6c4e2f82ba 100644
> > --- a/lib/efi_loader/efi_boottime.c
> > +++ b/lib/efi_loader/efi_boottime.c
> > @@ -1770,9 +1770,9 @@ error:
> >   *
> >   * Return: status code
> >   */
> > -static efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
> > -					   efi_uintn_t *exit_data_size,
> > -					   u16 **exit_data)
> > +efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
> > +				    efi_uintn_t *exit_data_size,
> > +				    u16 **exit_data)
> >  {
> >  	struct efi_loaded_image_obj *image_obj =
> >  		(struct efi_loaded_image_obj *)image_handle;
> > -- 
> > 2.20.1
> > 
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot at lists.denx.de
> > https://lists.denx.de/listinfo/u-boot
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot