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

[Heinrich Schuchardt]

On 7/18/19 7:40 PM, Alexander Graf wrote:
>
> 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?

We always call StartImage() where we set efi_is_direct_boot = false.
Hence we never call cleanup_before_linux().

Regards

Heinrich