From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heinrich Schuchardt Date: Fri, 19 Jul 2019 19:52:03 +0200 Subject: [U-Boot] [PATCH v2 3/4] efi_loader: use efi_start_image() for bootefi In-Reply-To: <20190719061405.GD66880@largo.jsg.id.au> References: <20190208184650.22765-1-xypron.glpk@gmx.de> <20190208184650.22765-4-xypron.glpk@gmx.de> <20190718060016.GA73593@largo.jsg.id.au> <5438c1db9647d8f4@bloch.sibelius.xs4all.nl> <20190718091644.GA18133@largo.jsg.id.au> <94f74740-6541-cdba-ff0d-34b2de4f3aca@csgraf.de> <5438c3eaeb24c83d@bloch.sibelius.xs4all.nl> <1cbc0442-6b04-7d78-b29b-2a70f440dd7d@gmx.de> <20190719061405.GD66880@largo.jsg.id.au> Message-ID: <7e8d03a8-e8a4-bd73-a329-cda5faf7e0a4@gmx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >>>> 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 >>>>>>>> >>>>>>>> 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 >>>>>>>>> --- >>>>>>>>> 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 >>>>>>>> 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 >>>>>>>> >>>>>>>> ?? 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