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

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

Heinrich,

Thank you for your review.

On Sun, Oct 13, 2019 at 05:38:27PM +0200, Heinrich Schuchardt wrote:
> On 10/8/19 2:05 AM, AKASHI Takahiro wrote:
> >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@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?
> 
> setenv -e -guid 12345678123456781234567812345678 foo bar
> 
> neither sets a value nor gives an error. Please, fix this.

Please make sure that you're now using v4.
I fixed the issue in v4 as I mentioned in its change log.

> Next I tried:
> => setenv -e -guid 12345678-1234-5678-1234-567812345678 foo bar
> ## Attributes (-bs or -rt) must be specified
> 
> So I followed the advice given by the command:
> 
> => setenv -e -guid 12345678-1234-5678-1234-567812345678 -rt foo bar
> ## Failed to set EFI variable (invalid parameter)
> 
> Who will know that he has to specify both -bs and -rt here?

UEFI specification clearly says that.

> As EFI_VARIABLE_RUNTIME_ACCESS always requires
> EFI_VARIABLE_BOOTSERVICE_ACCESS we do not need a -bs parameter. Just
> always set EFI_VARIABLE_BOOTSERVICE_ACCESS.

Please note that I always try to mimic UEFI's Shell interfaces
in my efi-related commands, either efidebug or env -e command, and
"-bs" and "-rt" option do exist there.

I admit, however, that the message is not quite friendly.
Will improve it.

> printenv -e does not show the content of the variable. This was working
> before the patch.

Please use "-v", which is also mentioned in the commit message.
I introduced this option as dumping some variables, like PK/KEK/db,
may overflow your screen.

Thanks,
-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> >
> >-Takahiro Akashi
> >
> >>Best regards
> >>
> >>Heinrich
> >
>