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

Tue Jan 8 08:58:27 UTC 2019

On 08.01.19 08:29, AKASHI Takahiro wrote:
> 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 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?
> 
> I prefer efidebug/efidbg or efitool so that we can use a shorthand
> name, efi, at command line in most cases.

That definitely works for me as well, yes.

Alex