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

Sun Oct 13 15:38:27 UTC 2019

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

setenv -e -guid 12345678123456781234567812345678 foo bar

neither sets a value nor gives an error. Please, fix this.

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

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

Best regards

Heinrich

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