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

Fri Jul 19 06:14:05 UTC 2019

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