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

[Alexander Graf]

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

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


Alex