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

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

On 7/18/19 11:16 AM, Jonathan Gray wrote:
> On Thu, Jul 18, 2019 at 10:39:57AM +0200, Mark Kettenis wrote:
>>> 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?
>
> Yes, when it fails the last thing on serial is:
>
> ## Starting EFI application at 12000000 ...
>>> OpenBSD/armv7 BOOTARM 1.3
> boot>
> booting sd0a:/bsd: 4572484+689312+238360+561608 [298268+120+314400+278666]=0x0
>
>>
>> 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.
>
> Indeed, removing 'efi_is_direct_boot = false;' from
> efi_start_image() allows me to boot multiuser on cubox with 2019.07.
>
>>
>> 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

Thanks for your investigation.

To which spec are you relating?

UEFI 2.8 spec, chapter 2.3.5 AArch32 Platforms:

The core will be configured as follows (common across all processor
architecture revisions): Instruction and Data caches enabled

GRUB is invalidating the cache before starting Linux by calling
grub_arch_sync_caches ((void *) linux_addr, linux_size);.

I think the BSD bootloader will also have to clear caches after loading
the main program.

@Alex:
When should we call clean_up_before_linux()?
In the first StartImage() call?
Always in ExitBootServices()?

Best regards

Heinrich

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