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

[Heinrich Schuchardt]

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.

Best regards

Heinrich