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

[Heinrich Schuchardt]

On 19.10.23 17:19, 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.
> 

Even after factoring out the boot functionality I would not know why we 
should have two separate symbols. I am fine with a rename which makes it 
clear that this symbol is about a library functionality.

Best regards

Heinrich