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

[Tom Rini]

On Mon, Oct 07, 2019 at 02:02:26PM +0900, AKASHI Takahiro wrote:
> 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?

I'm happy to get more feedback on this but my first pass is GRUB2 and
*BSD loaders.  Shell, SCT, etc are not.  Specific platforms can enable
whatever is needed there (even outside of qemu*/sandbox).  And focusing
on "tight resource platforms" is the wrong thing to focus on.  Even on
platforms where we have tons of space no one wants a loader that is
larger than it needs to be.  The bigger it is the longer it takes to
read and relocate and get on with the business of running the system.

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

I don't have a strong preference where this goes and will leave that to
Heinrich but I do have preferences about binary size (and boot time
increases) even when they seem small as they add up over time.  Thanks
for your understanding here, I do appreciate the time you've been
investing here.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20191007/24ccf2fa/attachment.sig>