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

[AKASHI Takahiro]

On Mon, Oct 07, 2019 at 06:45:08PM +0200, Heinrich Schuchardt wrote:
> On 10/7/19 5:43 PM, Tom Rini wrote:
> >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.
> >
> 
> With
> 
> https://lists.denx.de/pipermail/u-boot/2019-October/385626.html
> [PATCH 1/1] cmd: disable CMD_NVEDIT_EFI by default
> 
> the code modified by this patch is disabled by default.

So you have no reason to reject my patch, don't you?

-Takahiro Akashi

> Best regards
> 
> Heinrich