From mboxrd@z Thu Jan 1 00:00:00 1970 From: AKASHI Takahiro Date: Tue, 25 Dec 2018 17:44:53 +0900 Subject: [U-Boot] [PATCH v3 8/8] cmd: env: add "-e" option for handling UEFI variables In-Reply-To: <7e32d99f-967b-038a-e6b5-7357cb1fa972@suse.de> References: <20181218050510.20308-1-takahiro.akashi@linaro.org> <20181218050510.20308-9-takahiro.akashi@linaro.org> <20181219014915.GJ14562@linaro.org> <7e32d99f-967b-038a-e6b5-7357cb1fa972@suse.de> Message-ID: <20181225084451.GD14405@linaro.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Sun, Dec 23, 2018 at 02:56:40AM +0100, Alexander Graf wrote: > > > On 19.12.18 13:23, Heinrich Schuchardt wrote: > > On 12/19/18 2:49 AM, AKASHI Takahiro wrote: > >> Heinrich, > >> > >> On Tue, Dec 18, 2018 at 07:07:02AM +0100, Heinrich Schuchardt wrote: > >>> On 12/18/18 6:05 AM, AKASHI Takahiro wrote: > >>>> "env [print|set] -e" allows for handling uefi variables without > >>>> knowing details about mapping to corresponding u-boot variables. > >>>> > >>>> Signed-off-by: AKASHI Takahiro > >>> > >>> Hello Takahiro, > >>> > >>> in several patch series you are implementing multiple interactive > >>> commands that concern > >>> > >>> - handling of EFI variables > >>> - executing EFI binaries > >>> - managing boot sequence > >>> > >>> I very much appreciate your effort to provide an independent UEFI shell > >>> implementation. What I am worried about is that your current patches > >>> make it part of the monolithic U-Boot binary. > >> > >> First of all, in v3, CONFIG_CMD_EFISHELL was introduced after Alex's > >> comment on v2. So you can disable efishell command if you don't want it. > >> > >> Are you still worried? > >> > >>> This design has multiple drawbacks: > >>> > >>> The memory size available for U-Boot is very limited for many devices. > >>> We already had to disable EFI_LOADER for some boards due to this > >>> limitations. Hence we want to keep everything out of the U-Boot binary > >>> that does not serve the primary goal of loading and executing the next > >>> binary. > >> > >> I don't know your point here. If EFI_LOADER is disabled, efishell > >> will never be compiled in. > >> > >>> The UEFI forum has published a UEFI Shell specification which is very > >>> extensive. We still have a lot of deficiencies in U-Boot's UEFI API > >>> implementation. By merging in parts of an UEFI shell implementation our > >>> project looses focus. > >> > >> What is "our project?" What is "focus?" > >> I'm just asking as I want to share that information with you. > >> > >>> There is an EDK2 implementation of said > >>> specification. If we fix the remaining bugs of the EFI API > >>> implementation in U-Boot we could simply run the EDK2 shell which > >>> provides all that is needed for interactive work. > >>> > >>> With you monolithic approach your UEFI shell implementation can neither > >>> be used with other UEFI API implementations than U-Boot nor can it be > >>> tested against other API implementations. > >> > >> Let me explain my stance. > >> My efishell is basically something like a pursuit as well as > >> a debug/test tool which was and is still quite useful for me. > >> Without it, I would have completed (most of) my efi-related work so far. > >> So I believe that it will also be useful for other people who may want > >> to get involved and play with u-boot's efi environment. > > > > On SD-Cards U-Boot is installed between the MBR and the first partition. > > On other devices it is put into a very small ROM. Both ways the maximum > > size is rather limited. > > > > U-Boot provides all that is needed to load and execute an EFI binary. So > > you can put your efishell as file into the EFI partition like you would > > install the EDK2 shell. > > > > The only hardshift this approach brings is that you have to implement > > your own printf because UEFI does not offer formatted output. But this > > can be copied from lib/efi_selftest/efi_selftest_console.c. > > > > The same decision I took for booting from iSCSI. I did not try to put an > > iSCSI driver into U-Boot instead I use iPXE as an executable that is > > loaded from the EFI partition. > > > >> > >> I have never intended to fully implement a shell which is to be compliant > >> with UEFI specification while I'm trying to mimick some command > >> interfaces for convenience. UEFI shell, as you know, provides plenty > >> of "protocols" on which some UEFI applications, including UEFI SCT, > >> reply. I will never implement it with my efishell. > >> > >> I hope that my efishell is a quick and easy way of learning more about > >> u-boot's uefi environment. I will be even happier if more people > >> get involved there. > >> > >>> Due to these considerations I suggest that you build your UEFI shell > >>> implementation as a separate UEFI binary (like helloworld.efi). You may > >>> offer an embedding of the binary (like the bootefi hello command) into > >>> the finally linked U-Boot binary via a configuration variable. Please, > >>> put the shell implementation into a separate directory. You may want to > >>> designate yourself as maintainer (in file MAINTAINERS). > >> > >> Yeah, your suggestion is reasonable and I have thought of it before. > >> There are, however, several reasons that I haven't done so; particularly, > >> efishell is implemented not only with boottime services but also > >> other helper functions, say, from device path utilities. Exporting them > >> as libraries is possible but I don't think that it would be so valuable. > >> > >> Even if efishell is a separate application, it will not contribute to > >> reduce the total footprint if it is embedded along with u-boot binary. > > > > That is why CONFIG_CMD_BOOTEFI_HELLO - which embeds helloworld.efi into > > the U-Boot binary - is default no. Same I would do for efishell.efi. > > One big drawback with a separate binary is the missing command line > integration. It becomes quite awkward to execute efi debug commands > then, since you'll have to run them through a special bootefi subcommand. > > If you really want to have a "uefi shell", I think the sanest option is > to just provide a built-in copy of the edk2 uefi shell, similar to the > hello world binary. The big benefit of this patch set however, is not > that we get a shell - it's that we get quick and tiny debug > introspectability into efi_loader data structures. And my command can be used for simple testing. > I think the biggest problem here really is the name of the code. Why > don't we just call it "debugefi"? It would be default N except for debug > targets (just like bootefi_hello). > > That way when someone wants to just quickly introspect internal data > structures, they can. I also hope that if the name contains debug, > nobody will expect command line compatibility going forward, so we have > much more freedom to change internals (which is my biggest concern). > > So in my opinion, if you fix the 2 other comments from Heinrich and > rename everything from "efishell" to "debugefi" (so it aligns with > bootefi), we should be good. If Heinrich agrees, I will fix the name although I'm not a super fan of this new name :) Thanks, -Takahiro Akashi > > Alex