[U-Boot] [PATCH v3 8/8] cmd: env: add "-e" option for handling UEFI variables

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

On Mon, Jan 07, 2019 at 04:47:13PM +0900, AKASHI Takahiro wrote:
> 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 <takahiro.akashi@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 ;).
> 
> Do you have any idea/preference about this command's name?

I prefer efidebug/efidbg or efitool so that we can use a shorthand
name, efi, at command line in most cases.

-Takahiro Akashi


> -Takahiro Akashi
> 
> > For normal operation, we need to come up with mechanisms that integrate
> > much deeper into U-Boot's generic command structure.
> > 
> > 
> > Alex