[U-Boot] [PATCH 0/6] efi: make efi and bootmgr more usable

[Alexander Graf]

On 18.10.18 07:24, AKASHI Takahiro wrote:
> On Wed, Oct 17, 2018 at 10:06:58AM +0200, Alexander Graf wrote:
>>
>>
>> On 17.10.18 09:32, AKASHI Takahiro wrote:
>>> This patch set is a collection of patches to enhance efi user interfaces
>>> /commands. It will help improve user experience on efi boot and make it
>>> more usable without edk2's shell utility.
>>
>> That's amazing, thanks a lot :)
>>
>>> Patch#1 to #4 are for efishell.
>>> Patch#5 and #6 are for bootmgr.
>>>
>>> Let's see how it works:
>>> => efishell boot add 1 SHELL mmc 0:1 /Shell.efi ""
>>> => efishell boot add 2 HELLO mmc 0:1 /hello.efi ""
>>> => efishell boot dump
>>
>> IMHO those 3 belong semantically to the "bootmgr" command. I can see how
>> "bootefi bootmgr add 1 SHELL ..." is terrible UX. But then again it's
>> not too much more to type than "efishell boot", right? ;)
>>
>> So at the end of the day, these should probably be
>>
>>  => bootefi bootmgr add 1 SHELL mmc 0:1 /Shell.efi ""
>>  => bootefi bootmgr add 2 HELLO mmc 0:1 /hello.efi ""
>>  => bootefi bootmgr dump
> 
> To be frank, I hesitate to agree with you for several reasons.
> * Boot manager is a sort of boot loader application while adding/showing
>   BootXXXX variables can be part of more generic system utility.
>   (My interface here mimics uefi shell's bcfg command with slightly
>    different syntax.)

If you look at it from a U-Boot perspective, the only consumer of
BootXXXX variables will realistically be bootmgr. So I don't quite see
how setting BootXXXX variables doesn't belong to the same command line
syntax?

> * In future, we may want to have another sub command, "driver," to support
>   driver loading, namely DriverOrder/DriverXXXX.

Hm. Who would read that order? When would it get parsed? Would that be
part of the script (and thus a command again) or would we want to have
them get loaded at an earlier stage?

My current stance would be that driver loading would happen very similar
to application loading: Using a command from a script / the shell. So
the same command could again be responsible for changing the order and
adding/removing driver entries.

> * Anyhow, we need another command for "setvar"( and "dumpvar").

I would much much rather like to integrate with existing U-Boot commands
(setenv, printenv, print) than invent new commands. The UEFI bits in
U-Boot shouldn't be alien - they should just seamlessly integrate :).

> 
>>> Boot0001:
>>> 	attributes: A-- (0x00000001)
>>> 	label: SHELL
>>> 	file_path: /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(1,MBR,0x086246ba,0x800,0x40000)/\\Shell.efi
>>> 	data: 
>>> Boot0002:
>>> 	attributes: A-- (0x00000001)
>>> 	label: HELLO
>>> 	file_path: /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(1,MBR,0x086246ba,0x800,0x40000)/\\hello.efi
>>> 	data: 
>>>
>>> => efishell boot order 1 2
>>> => efishell boot order
>>
>> Same thing here :).
>>
>>>  1: Boot0001: SHELL
>>>  2: Boot0002: HELLO
>>>
>>> => bootefi bootmgr -2
>>> WARNING: booting without device tree
>>> Booting: HELLO
>>> ## Starting EFI application at 000000007db8b040 ...
>>> Hello, world!
>>> ## Application terminated, r = 0
>>>
>>> => efishell setvar PlatformLang en        <--- important!
>>
>> That one is slightly more complicated. How about we introduce a -e flag
>> to all the env operations? Then this would become
>>
>>  => setenv -e PlatformLang en
>>
>> and you could print only EFI variables using
>>
>>  => printenv -e
>>
>> maybe one day we could then also just implement partial variable storage
>> for UEFI variables:
>>
>>  => saveenv -e
>>
>> which we could then reuse in the ExitBootServices() call to persist EFI
>> variables?
> 
> I didn't get your point. Can you please elaborate it?

Currently we have:

=> help printenv
printenv - print environment variables

Usage:
printenv [-a]
    - print [all] values of all environment variables
printenv name ...
    - print value of environment variable 'name'
=> help setenv
setenv - set environment variables

Usage:
setenv [-f] name value ...
    - [forcibly] set environment variable 'name' to 'value ...'
setenv [-f] name
    - [forcibly] delete environment variable 'name'


What if we add UEFI variable awareness to those commands? So instead of

  => efishell setvar PlatformLang en

you would get

  => setenv -e PlatformLang en

The same would apply for printenv. Something like

  => printenv -e

would just show UEFI variables with potential additional decoding?


Or was your question on saveenv? The saveenv command is used to make the
variable storage persistent. One problem we have with variable storage
is that you want to only persist variables that are

  a) marked to persist
  b) UEFI variables

So for example if you're in the middle of a script and you want to
change the boot order, you want to say "please make the UEFI variable
store persistent now", but you definitely do now want to make the U-Boot
variable store persistent, because your variables are in the middle of
your script execution with helper variables.

That same problem also occurs with booting. When calling
ExitBootServices() we ideally want to persist the UEFI variable storage
(well, the non-temporary variables in it). But for that we currently
don't have any logic implemented to differentiate between UEFI variables
and U-Boot variables on save.

> 
>>> => efishell bootmgr -1 or efishell bootmgr
>>>
>>>    (shell ...)
>>>
>>> # The only drawback is that it can be confusing to type
>>>   "bootefi ..." and "efi(shell) boot ..." :)
>>
>> Yes :).
> 
> The compromise I can imagine is that efishell be also aliased
> to bootefi so that we can do:
>  => efi(shell) boot add 1 ...
>  => efi(shell) bootmgr -1 ( in my current syntax)
> Yet we still maintain an old/boot*-style interface:
>  => bootefi bootmgr or <appl addr>

In a nutshell, I want to have UEFI commands be first class U-Boot
commands :). The whole fact that we have "bootefi bootmgr" is bad. It
shouldn't have been a bootefi subcommand really :/.

Now the really big question is what an alternative *good* naming scheme
is. And there I'm not sure yet...


Alex