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

[AKASHI Takahiro]

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 at 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.

> 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