From mboxrd@z Thu Jan 1 00:00:00 1970 From: AKASHI Takahiro Date: Tue, 8 Oct 2019 09:05:28 +0900 Subject: [U-Boot] [PATCH v3] cmd: env: extend "env [set|print] -e" to manage UEFI variables In-Reply-To: References: <20191004012033.6216-1-takahiro.akashi@linaro.org> <20191007004745.GQ18778@linaro.org> <20191007014230.GD15735@bill-the-cat> <20191007050225.GX18778@linaro.org> <20191007154305.GJ6716@bill-the-cat> Message-ID: <20191008000526.GY18778@linaro.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > >>>>> > >>>>>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