[v4 11/24] efi: Rearrange the Kconfig for CMD_BOOTEFI_BOOTMGR

[Tom Rini]

On Fri, Oct 20, 2023 at 09:21:00PM +0900, AKASHI Takahiro wrote:
> On Thu, Oct 19, 2023 at 11:19:33AM -0400, Tom Rini wrote:
> > On Thu, Oct 19, 2023 at 05:16:28PM +0200, Heinrich Schuchardt wrote:
> > > On 19.10.23 17:00, Tom Rini wrote:
> > > > From: Simon Glass <sjg at chromium.org>
> > > > 
> > > > The command should not be used to enable library functionality. Add a
> > > > new BOOTEFI_BOOTMGR Kconfig for that. Adjust the conditions so that the
> > > > same code is built.
> > > > 
> > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > Suggested-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> > > > ---
> > > > Cc: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
> > > > Cc: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> > > > Changes in v4:
> > > > - Integrate AKASHI Takahiro's feedback from v3
> > > > - Reword the help text on CMD_BOOTEFI_BOOTMGR slightly
> > > > ---
> > > >   cmd/Kconfig             | 11 ++++++++++-
> > > >   lib/efi_loader/Kconfig  |  6 +++---
> > > >   lib/efi_loader/Makefile |  2 +-
> > > >   3 files changed, 14 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > > > index 16e5cb8f0633..872cb49150cc 100644
> > > > --- a/cmd/Kconfig
> > > > +++ b/cmd/Kconfig
> > > > @@ -379,6 +379,15 @@ config CMD_BOOTEFI
> > > >   	help
> > > >   	  Boot an EFI image from memory.
> > > > +config CMD_BOOTEFI_BOOTMGR
> > > > +	bool "UEFI Boot Manager command"
> > > > +	depends on BOOTEFI_BOOTMGR && CMD_BOOTEFI
> > > > +	default y
> > > > +	help
> > > > +	  Select this option to enable the 'bootmgr' subcommand of 'bootefi'.
> > > > +	  This subcommand will allow you to select the UEFI binary to be booted
> > > > +	  via UEFI variables Boot####, BootOrder, and BootNext.
> > > > +
> > > >   config CMD_BOOTEFI_HELLO_COMPILE
> > > >   	bool "Compile a standard EFI hello world binary for testing"
> > > >   	depends on CMD_BOOTEFI && !CPU_V7M
> > > > @@ -2110,7 +2119,7 @@ config CMD_EFIDEBUG
> > > >   config CMD_EFICONFIG
> > > >   	bool "eficonfig - provide menu-driven uefi variables maintenance interface"
> > > >   	default y if !HAS_BOARD_SIZE_LIMIT
> > > > -	depends on CMD_BOOTEFI_BOOTMGR
> > > > +	depends on BOOTEFI_BOOTMGR
> > > >   	select MENU
> > > >   	help
> > > >   	  Enable the 'eficonfig' command which provides the menu-driven UEFI
> > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > > index d20aaab6dba4..13cad6342c36 100644
> > > > --- a/lib/efi_loader/Kconfig
> > > > +++ b/lib/efi_loader/Kconfig
> > > > @@ -32,14 +32,14 @@ config EFI_LOADER
> > > >   if EFI_LOADER
> > > > -config CMD_BOOTEFI_BOOTMGR
> > > > +config BOOTEFI_BOOTMGR
> > > >   	bool "UEFI Boot Manager"
> > > >   	default y
> > > >   	select BOOTMETH_GLOBAL if BOOTSTD
> > > >   	help
> > > >   	  Select this option if you want to select the UEFI binary to be booted
> > > > -	  via UEFI variables Boot####, BootOrder, and BootNext. This enables the
> > > > -	  'bootefi bootmgr' command.
> > > > +	  via UEFI variables Boot####, BootOrder, and BootNext. You should also
> > > > +	  normally enable CMD_BOOTEFI_BOOTMGR so that the command is available.
> > > >   choice
> > > >   	prompt "Store for non-volatile UEFI variables"
> > > > diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> > > > index 8d31fc61c601..0a2cb6e3c476 100644
> > > > --- a/lib/efi_loader/Makefile
> > > > +++ b/lib/efi_loader/Makefile
> > > > @@ -42,7 +42,7 @@ targets += initrddump.o
> > > >   endif
> > > >   obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o
> > > > -obj-$(CONFIG_CMD_BOOTEFI_BOOTMGR) += efi_bootmgr.o
> > > > +obj-$(CONFIG_BOOTEFI_BOOTMGR) += efi_bootmgr.o
> > > >   obj-y += efi_boottime.o
> > > >   obj-y += efi_helper.o
> > > >   obj-$(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) += efi_capsule.o
> > > 
> > > This patch looks wrong.
> > > 
> > > Symbol CONFIG_CMD_BOOTEFI_BOOTMGR is used in a lot of places where it is not
> > > related to the 'bootefi bootmgr' subcommand.
> > > 
> > > I see no benefit in two separate symbols. If you want to rename the symbol,
> > > please, replace *all* occurrences:
> > > 
> > > %s/CONFIG_CMD_BOOTEFI_BOOTMGR/CONFIG_BOOTEFI_BOOTMGR/
> > 
> > Yes, there's work on the EFI_LOADER side of things to support the use
> > case of "boot to menu" (or, "boot to efi bootmgr") of which this is the
> > starting point. The follow-up work that I'm hoping you or someone else
> > with more EFI_LOADER experience will pick up is splitting cmd/bootefi.c
> > such that we can call in to starting an EFI payload (or bootmgr) without
> > the command line.
> 
> I will be able to take care unless Heinrich wants to do by himself.
> # Test would be another issue now that "bootefi" may handle various
>   boot scenarios.

At the very high level, this is solved for "bootm" today, and I _think_
booti/bootz also but I'm less sure of that.  The non-command side logic
is in boot/bootm.c rather than cmd/bootm.c and so we exercise the core
still via the normal tests. I don't know if the best answer is
boot/bootefi.c or lib/efi_loader/something.c. Tests of new functionality
will require some thought tho, yes.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20231020/0a8c8318/attachment.sig>