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

Fri Jul 19 18:36:19 UTC 2019

On 7/19/19 7:52 PM, Heinrich Schuchardt wrote:
> On 7/19/19 8:14 AM, Jonathan Gray wrote:
>> On Fri, Jul 19, 2019 at 07:43:17AM +0200, Heinrich Schuchardt wrote:
>>> On 7/19/19 1:07 AM, Mark Kettenis wrote:
>>>>> From: Alexander Graf <agraf at csgraf.de>
>>>>> Date: Thu, 18 Jul 2019 20:50:39 +0200
>>>>>
>>>>> 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.
>>>>>
>>>>>
>>>>> Heinrich asked me to clarify again in the mail thread.
>>>>>
>>>>> The UEFI spec says that caches on 32bit ARM are supposed to be
>>>>> *enabled*
>>>>> always. That means throughout boot time, as well as after
>>>>> ExitBootServices(). So the behavior with efi_is_direct_boot = false is
>>>>> the expected behavior according to the spec.
>>>>>
>>>>> The reason we have the hack above in the tree is that grub had a nasty
>>>>> deficiency on 32bit ARM where it called into the Linux kernel using
>>>>> the
>>>>> legacy boot protocol which again expected caches to be disabled.
>>>>>
>>>>> The problem you're facing here sounds to me like a bug in the OpenBSD
>>>>> loader that resembles the bug in grub. Instead of calling into a BSD
>>>>> kernel code path that expects caches to already be enabled, it expects
>>>>> to come up with caches disabled.
>>>>
>>>> I don't think we assume that the kernel is called with the
>>>> architecturally defined caches disabled.  As far as I know, OpenBSD
>>>> works fine with a recent U-Boot on Cortex-A7 systems that don't have
>>>> an non-architectural L2 cache.
>>>>
>>>>> You *could* solve this generically by adding functionality akin to
>>>>> clean_up_before_linux() inside the BSD loader. The downside of that is
>>>>> that the code would be platform specific, as L2 caches used to be
>>>>> enabled/disabled via MMIOs and not via architected registers in the
>>>>> old
>>>>> days.
>>>>
>>>> Messing with the non-architectural L2 cache is someting we defenitely
>>>> don't want to do in the OpenBSD bootloader and kernel bootstrap code.
>>>> We have a single bootloader/kernel that works on all supported armv7
>>>> systems!
>>>>
>>>> I quoted UEFI 2.5 upthread, but 2.8 retains the same text in section
>>>> 2.3.5:
>>>>
>>>>     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
>>>>
>>>> So I'd say the problem here is that U-Boot passes control to the EFI
>>>> payload with the L2 cache enabled.
>>>>
>>>
>>> Hello Mark,
>>>
>>> your last observation concerned an i.MX6 board. Does this processor have
>>> any cache that falls under the cited paragraph? A link to the processor
>>> documentation would be helpful.
>>>
>>> Best regards
>>>
>>> Heinrich
>>
>> i.MX6 has a PL310 L2 cache which has it's own TRM from Arm.
>> NXP now requires an account to download IMX6DQRM.pdf sadly.
>>
>> http://infocenter.arm.com/help/topic/com.arm.doc.ddi0246c/DDI0246C_l2cc_pl310_r2p0_trm.pdf
>>
>> "The cache controller is configured using memory-mapped registers,
>> rather than using CP15 instructions"
>>
>> u-boot/include/configs/mx6_common.h:#define CONFIG_SYS_L2_PL310
>>
>
> mx6cuboxi_defconfig has CONFIG_SYS_L2CACHE_OFF=N.
>
> Doesn't this mean that we conform to the UEFI specification?

Jonathan can you, please, test booting OpenBSD with
CONFIG_SYS_L2CACHE_OFF=Y.

Best regards

Heinrich