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

[Alexander Graf]

On 18.07.19 19:33, Heinrich Schuchardt wrote:
> 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 at 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 at 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 at 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 at 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()?


The problem with 32bit ARM grub was that it simply never called 
StartImage(), because it did not execute UEFI kernels. Instead it tried 
to simulate the legacy boot path.

Or in other words: If there ever was a StartImage() call, we do not need 
to disable caches, as that means the kernel is called indirectly. Has 
that logic changed?


Alex