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 <takahiro.aka...@linaro.org> >>>>> >>>>> 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 ;). For normal operation, we need to come up with mechanisms that integrate much deeper into U-Boot's generic command structure. Alex _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
