[U-Boot] [PATCH v3] cmd: env: extend "env [set|print] -e" to manage UEFI variables

[AKASHI Takahiro]

On Sun, Oct 06, 2019 at 09:42:30PM -0400, Tom Rini wrote:
> On Mon, Oct 07, 2019 at 09:47:46AM +0900, AKASHI Takahiro wrote:
> > On Sat, Oct 05, 2019 at 08:53:39AM +0200, Heinrich Schuchardt wrote:
> > > On 10/4/19 3:20 AM, AKASHI Takahiro wrote:
> > > >With this patch, when setting UEFI variable with "env set -e" command,
> > > >we will be able to
> > > >- specify vendor guid with "-guid guid",
> > > >- specify variable attributes,  BOOTSERVICE_ACCESS, RUNTIME_ACCESS,
> > > >   respectively with "-bs" and "-rt",
> > > >- append a value instead of overwriting with "-a",
> > > >- use memory as variable's value instead of explicit values given
> > > >   at the command line with "-i address,size"
> > > >
> > > >If guid is not explicitly given, default value will be used.
> > > >
> > > >When "-at" is given, a variable should be authenticated with
> > > >appropriate signature database before setting or modifying its value.
> > > >(Authentication is not supported yet though.)
> > > 
> > > Didn't you remove this in v2?
> > 
> > It was an editorial error, which also existed in v2 commit message.
> > 
> > > >
> > > >Meanwhile, "env print -e," will be modified so that it will dump
> > > >a variable's value only if '-v' (verbose) is specified.
> > > >
> > > >Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> > > 
> > > This does not work as expected:
> > > 
> > > => setenv -e -guid 0123456789abcdef0123456789abcdef -bs ARGONAUT hello world
> > > => printenv -e
> > > OsIndicationsSupported:
> > >     EFI_GLOBAL_VARIABLE_GUID: BS|RT, DataSize = 0x8
> > > PlatformLang:
> > >     EFI_GLOBAL_VARIABLE_GUID: NV|BS|RT, DataSize = 0x6
> > > PlatformLangCodes:
> > >     EFI_GLOBAL_VARIABLE_GUID: BS|RT, DataSize = 0x6
> > > RuntimeServicesSupported:
> > >     EFI_GLOBAL_VARIABLE_GUID: BS|RT, DataSize = 0x2
> > > 
> > > Neither do I see an error nor is the variable set.
> > 
> > You have an incorrect guid format, so env_set_efi() should
> > return CMD_RET_USAGE, but mistakenly returns CMD_RET_FAILURE.
> > 
> > > The patch increases the size of u-boot.bin by 2216 bytes for qemu-arm64.
> > > Tom is always asking me how we can stop the size increase in the UEFI
> > > sub-system.
> > > 
> > > Why do we need this patch? This functionality is already available via
> > > the UEFI shell.
> > 
> > Such a question should have been made before initially "-e" option
> > has been introduced a long time ago.
> > 
> > * As far as we have "-e" option, this new patch is a natural extension.
> > * Why do we have to reply on external apps? We should have a minimum
> >   self-contained command set to manage UEFI as part of U-Boot.
> > * "-i" is used in secure boot pytest.
> 
> There is a difficult balance to find here.  On the one hand, there is
> the argument (that I find compelling) which is that we want EFI loader
> support enabled, by default, on architectures where there is EFI support
> as it provides a consistent interface between ${random board} and ${off
> the shelf SW stack}.  On the other hand, since this is a feature that's
> being enabled on a big majority of our platforms we need to be extremely
> wary of code size increases.
> 
> So while I do think that some defconfigs (qemu* for example, and
> sandbox) should be fine to enable everything on (especially sandbox as
> there's where coverity runs), most configs should only get 'default y'
> via Kconfig for things required to load the common EFI applications.

What is your definition of *common* EFI applications?
Do they include Shell.efi even on tight-resource platforms?

BTW, if you don't really like "env [set|print] -e" syntax,
we can move all the stuff to "efidebug" command.
This was the original place where I put them in my initial patch.

Thanks,
-Takahiro Akashi

> 
> -- 
> Tom