From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4CB4CC433F5 for ; Thu, 30 Sep 2021 11:13:35 +0000 (UTC) Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 2AC0861994 for ; Thu, 30 Sep 2021 11:13:34 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 2AC0861994 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=xs4all.nl Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id AC81D8169E; Thu, 30 Sep 2021 13:13:31 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=fail (p=none dis=none) header.from=xs4all.nl Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Received: by phobos.denx.de (Postfix, from userid 109) id A4AFF81918; Thu, 30 Sep 2021 13:13:28 +0200 (CEST) Received: from sibelius.xs4all.nl (sibelius.xs4all.nl [83.163.83.176]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 5726D80FA0 for ; Thu, 30 Sep 2021 13:13:24 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=fail (p=none dis=none) header.from=xs4all.nl Authentication-Results: phobos.denx.de; spf=fail smtp.mailfrom=mark.kettenis@xs4all.nl Received: from localhost (bloch.sibelius.xs4all.nl [local]) by bloch.sibelius.xs4all.nl (OpenSMTPD) with ESMTPA id 43199e6b; Thu, 30 Sep 2021 13:13:22 +0200 (CEST) Date: Thu, 30 Sep 2021 13:13:22 +0200 (CEST) From: Mark Kettenis To: Simon Glass Cc: xypron.glpk@gmx.de, bmeng.cn@gmail.com, ilias.apalodimas@linaro.org, u-boot@lists.denx.de, trini@konsulko.com In-Reply-To: (message from Simon Glass on Wed, 29 Sep 2021 22:08:49 -0600) Subject: Re: Driver model at UEFI runtime References: <2461183e-ae0a-d8b3-3822-6a06ee90950d@gmx.de> <420f2d03-2ccd-1568-4845-a267de2fae36@gmx.de> Message-ID: <56149888e96eedfc@bloch.sibelius.xs4all.nl> X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.34 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.2 at phobos.denx.de X-Virus-Status: Clean > From: Simon Glass > Date: Wed, 29 Sep 2021 22:08:49 -0600 Hi Simon, > Hi Heinrich, > > On Fri, 10 Sept 2021 at 08:19, Heinrich Schuchardt wrote: > > > > > > > > On 9/9/21 10:00 PM, Simon Glass wrote: > > > Hi Heinrich, > > > > > > On Thu, 9 Sept 2021 at 05:29, Heinrich Schuchardt wrote: > > >> > > >> Hello Simon, > > >> > > >> The EBBR specification requires that the UEFI SystemReset() runtime > > >> service is available to the operating system. > > >> > > >> Up to now this has been implemented by overriding function > > >> efi_reset_system() which is marked as __efi_runtime. > > >> > > >> Both ARM and RISC-V support generic interfaces for reset. PSCI for ARM > > >> and the System Reset Extension for SBI on RISC-V. This has kept the > > >> number of implementations low. The only exceptions are: > > >> > > >> * arch/arm/cpu/armv8/fsl-layerscape/cpu.c > > >> * arch/arm/mach-bcm283x/reset.c for the Raspberry PIs > > >> * arch/sandbox/cpu/start.c > > >> > > >> Bin has suggested in > > >> https://lists.denx.de/pipermail/u-boot/2021-September/459865.html to use > > >> reset drivers based on the driver model. > > >> > > >> Currently after ExitBootServices() the driver model does not exist anymore. > > >> > > >> When evaluating Bin's suggestion one has to keep in mind that after > > >> invoking ExitBootServices() most operating systems call > > >> SetVirtualAddressMap(). Due to the change of the address map all > > >> pointers used by U-Boot afterwards must be updated to match the new > > >> memory map. > > >> > > >> The impression that Ilias and I have is that keeping the driver model > > >> alive after SetVirtualAddressMap() would incur: > > >> > > >> * a high development effort > > >> * a significant code size increase > > >> * an enlarged attack surface > > >> > > >> For RISC-V it has been clarified in the platform specification that the > > >> SBI must implement the system reset extension. For ARMv8 using TF-A and > > >> PSCI is what ARM suggests. > > >> > > >> So for these architectures we do not expect a growth in the number of > > >> drivers needed. > > >> > > >> Ilias and my favorite would be keeping the design as is. > > >> > > >> What is your view on this? > > > > > > Not to dump on the original author but here again we are paying the > > > price for the shortcuts taken at the time and not since revisited. > > > > > > My original request then was to create a new build of U-Boot, since we > > > need to build (and load) the runtime stuff separately. Then we can > > > > Do you mean by new build something like TPL, SPL? > > I suppose, but we need to move it to PHASE instead, I think. BTW I > sent a series that shows how we can drop TPL_SPL_ once we complete the > CONFIG migration. > > > > > Tom is right in complaining that the UEFI implementation is getting too > > big for some boards. Duplicating a lot of binary code, e.g. the complete > > libfdt or everything needed for UEFI variables, does not look a viable > > option. The good thing about tagging functions as __efi_runtime is > > minimizing binary code duplication. > > That's true, but it is going to become impossible to maintain this > mess at some point. For example there is a duplicated reset driver and > the UEFI runtime specifically avoiding using driver model. Where does > it end?! It ends with whatever functionality we decide that the EFI runtime has to support after ExitBootServices() has been called. In EBBR all runtime services are optional at this stage, with the exception of SetVirtualAddressMap() and ConvertPointer(). So the only thing that you really need is a dummy implementation that returns EFI_UNSUPPORTED for everything else and a way to relocate that implementation to a virtual address chosen by the OS. So not even a reset implementation is needed. And then there is no reason to get DM involved. Of course the OS will need some way to reset/shutdown the machine. In principle this is supposed to be implemented in other firmware components (PSCI on ARM, SBI on RISC-V) and the UEFI implementation is supposed to just make the appropriate firmware call. In that case I'd say implementing ResetSystem() would be so trivial that involving DM makes very little sense. Unfortunately not all PSCI/SBI implementations actually implement this. But even then, as long as the hardware needed to reset the system is exposed in the DT passed to the OS, the OS can just take care of this itself. It's the capsule update stuff and EFI variables stuff that creates complications if you want to support those while the OS is running. But the former doesn't really work without the latter and I fear that implementing EFI variables is just not possible on the majority of the boards we support in U-Boot. Especially if you care about verified boot. You pretty much have to have something like OP-TEE running in a secure enclave to do this properly. There is a good reason why this was made optional in EBBR. > IMO EFI runtime is its own binary and we're going to have to accept > that at some point. Yes. I think so. Just like we do for the PSCI implementation that is provided for some of the boards that don't come with a TF-A implementation. The minimal implementation I suggest above would be tiny and would fit very well in this model. My suggestion would be that before ExitBootServices() gets called, runtime services would just call normal U-Boot code using DM and all. And when ExitBootServices() gets called we just switch in the standalone dummy implementation. Personally, I would really prefer this approach where the runtime code is as minimal as possible. It would be way easier to audit and convince myself as an OS developer that yes, if I make an EFI runtime call it isn't going to do nasty stuff behind my back. > > It would be possible to leave the whole U-Boot binary in memory when > > launching the operating system at the cost of loosing < 1MiB of RAM. > > This could eliminate the __efi_runtime tagging. > > Yes, but will people complain about the size? Yes. This would leave a ton of executable code in memory that could be exploited using a kernel ROP attack. > > The problematic stuff are the memory structures that we need to convey > > between the boottime and the runtime. It is here where pointers need to > > be updated. You cannot resolve this data side problem by duplicating code. > > > > The first thing we should work on is an easily parsable structure > > without pointers for conveying runtime device information. > > > > Something like a concatenation of structures with > > > > * length > > * driver id > > * private data > > > > might be sufficient. > > Well yes that's a whole other problem. I suppose we ultimately end up > running dm_init() again when we start up the runtime? :-( > > > > > > avoid all this mess and just use the normal U-Boot code (and driver > > > model). It also scales up to whatever else we want to do to the runtime > > > stuff in the future. > > > > > > This will be somewhat easier with the VPL series applied, and even > > > > VPL? Please, provide a link. > > http://patchwork.ozlabs.org/project/uboot/list/?series=263034 > > > > > When thinking of which drivers are needed at runtime it is restricted to > > the following: > > > > * reset driver. Some like SBI can be blindly called at runtime because > > configuration tells us that there is an SBI. For others, e.g GPIO, > > we need information from the runtime devicetree. For others we may > > want the result of probing at boottime to avoid code duplication. > > > > * tee driver: for managing variables at runtime it would be > > good to have access to non-volatile memory managed by the TEE. > > This has not been realized yet. > > > > All devices that are managed by the operating system must not be touched > > by the runtime firmware. > > OK. > > My eyes are glazing over at this point. As you say, EFI runtime as a > separate binary will further increase the size of EFI, but we will end > up there in the end as people need to call more U-Boot code. No one > ever claimed that EFI was svelte. > > Regards, > Simon >