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

[AKASHI Takahiro]

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