From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heinrich Schuchardt Date: Thu, 18 Jul 2019 19:33:53 +0200 Subject: [U-Boot] [PATCH v2 3/4] efi_loader: use efi_start_image() for bootefi In-Reply-To: <20190718091644.GA18133@largo.jsg.id.au> References: <20190208184650.22765-1-xypron.glpk@gmx.de> <20190208184650.22765-4-xypron.glpk@gmx.de> <20190718060016.GA73593@largo.jsg.id.au> <5438c1db9647d8f4@bloch.sibelius.xs4all.nl> <20190718091644.GA18133@largo.jsg.id.au> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.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 >>> >>> 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 >>>> --- >>>> 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 >>> 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 >>> >>> 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 >