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

Fri Jul 19 17:52:03 UTC 2019

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?

Best regards

Heinrich