From mboxrd@z Thu Jan 1 00:00:00 1970 From: AKASHI Takahiro Date: Mon, 7 Jan 2019 16:47:13 +0900 Subject: [U-Boot] [PATCH v3 8/8] cmd: env: add "-e" option for handling UEFI variables In-Reply-To: <4da7abe8-2475-7a1f-0aa9-b49f67ea56dd@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> <20181225084451.GD14405@linaro.org> <4da7abe8-2475-7a1f-0aa9-b49f67ea56dd@suse.de> Message-ID: <20190107074711.GH9033@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 Heinrich, On Wed, Dec 26, 2018 at 10:20:32PM +0100, Alexander Graf wrote: > > > On 25.12.18 09:44, AKASHI Takahiro wrote: > > 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. > > Exactly, that would give us the best of both worlds. > > > > >> 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 :) > > Well, feel free to come up with a new one, but it definitely must have a > ring to it that it's a tiny, debug only feature that is not intended for > normal use ;). Do you have any idea/preference about this command's name? -Takahiro Akashi > For normal operation, we need to come up with mechanisms that integrate > much deeper into U-Boot's generic command structure. > > > Alex