[PATCH v8 2/9] eficonfig: menu-driven addition of UEFI boot option

Heinrich Schuchardt xypron.glpk at gmx.de
Tue Jul 12 08:25:10 CEST 2022


On 7/12/22 03:55, Takahiro Akashi wrote:
> On Sun, Jul 10, 2022 at 11:03:43AM +0200, Heinrich Schuchardt wrote:
>> On 6/19/22 06:56, Masahisa Kojima wrote:
>>> This commit add the "eficonfig" command.
>>> The "eficonfig" command implements the menu-driven UEFI boot option
>>> maintenance feature. This commit implements the addition of
>>> new boot option. User can select the block device volume having
>>> efi_simple_file_system_protocol and select the file corresponding
>>> to the Boot#### variable. User can also enter the description and
>>> optional_data of the BOOT#### variable in utf8.
>>>
>>> This commit adds "include/efi_config.h", it contains the common
>>> definition to be used from other menus such as UEFI Secure Boot
>>> key management.
>>>
>>> Signed-off-by: Masahisa Kojima <masahisa.kojima at linaro.org>
>>> ---
>>> Changes in v8:
>>> - command name is change from "efimenu" to "eficonfig"
>>> - function and struct prefixes is changed to "eficonfig"
>>> - fix menu header string
>>>
>>> Changes in v7:
>>> - add "efimenu" command and uefi variable maintenance code
>>>     moved into cmd/efimenu.c
>>> - create include/efimenu.h to define the common definition for
>>>     the other menu such as UEFI Secure Boot key management
>>> - update boot option edit UI, user can select description, file,
>>>     and optional_data to edit in the same menu like following.
>>>
>>>     ** Edit Boot Option **
>>>
>>>        Description: debian
>>>        File: virtio 0:1/EFI\debian\grubaa64.efi
>>>        Optional Data: test
>>>        Save
>>>        Quit
>>>
>>> - remove exit parameter from efimenu_process_common()
>>> - menu title type is changed from u16 to char
>>> - efimenu_process_common() add menu title string
>>> - reduce printf/puts function call for displaying the menu
>>> - efi_console_get_u16_string() accept 0 length to allow
>>>     optional_data is empty
>>> - efi_console_get_u16_string() the "size" parameter name is changes to "count"
>>> - efimenu is now designed to maintain the UEFI variables, remove autoboot related code
>>> - remove one empty line before "Quit" entry
>>> - efimenu_init() processes only the first time
>>>
>>> Changes in v6:
>>> - fix typos
>>> - modify volume name to match U-Boot syntax
>>> - compile in CONFIG_EFI_LOADER=n and CONFIG_CMD_BOOTEFI_BOOTMGR=n
>>> - simplify u16_strncmp() usage
>>> - support "a\b.efi" file path, use link list to handle filepath
>>> - modify length check condition
>>> - UEFI related menu items only appears with CONFIG_AUTOBOOT_MENU_SHOW=y
>>>
>>> Changes in v5:
>>> - remove forward declarations
>>> - add const qualifier for menu items
>>> - fix the possible unaligned access for directory info access
>>> - split into three commit 1)add boot option 2) delete boot option 3)change boot order
>>>     This commit is 1)add boot option.
>>> - fix file name buffer allocation size, it should be EFI_BOOTMENU_FILE_PATH_MAX * sizeof(u16)
>>> - fix wrong size checking for file selection
>>>
>>> Chanes in v4:
>>> - UEFI boot option maintenance menu is integrated into bootmenu
>>> - display the simplified volume name(e.g. usb0:1, nvme1:2) for the
>>>     volume selection
>>> - instead of extending lib/efi_loader/efi_bootmgr.c, newly create
>>>     lib/efi_loader/efi_bootmenu_maintenance.c and implement boot
>>>     variable maintenance into it.
>>>
>>> Changes in RFC v3:
>>>    not included in v3 series
>>>
>>> Changes in RFC v2:
>>> - enable utf8 user input for boot option name
>>> - create lib/efi_loader/efi_console.c::efi_console_get_u16_string() for
>>>     utf8 user input handling
>>> - use u16_strlcat instead of u16_strcat
>>> - remove the EFI_CALLs, and newly create or expose the following
>>>     xxx_int() functions.
>>>       efi_locate_handle_buffer_int(), efi_open_volume_int(),
>>>       efi_file_open_int(), efi_file_close_int(), efi_file_read_int() and
>>>       efi_file_setpos_int().
>>>     Note that EFI_CALLs still exist for EFI_DEVICE_PATH_TO_TEXT_PROTOCOL
>>>     and EFI_SIMPLE_TEXT_INPUT/OUTPUT_PROTOCOL
>>> - use efi_search_protocol() instead of calling locate_protocol() to get
>>>     the device_path_to_text_protocol interface.
>>> - remove unnecessary puts(ANSI_CLEAR_LINE), this patch is still depends on
>>>     puts(ANSI_CLEAR_CONSOLE)
>>> - skip SetVariable() if the bootorder is not changed
>>>
>>>    cmd/Kconfig                   |    7 +
>>>    cmd/Makefile                  |    1 +
>>>    cmd/eficonfig.c               | 1270 +++++++++++++++++++++++++++++++++
>>>    include/efi_config.h          |   91 +++
>>>    include/efi_loader.h          |   40 ++
>>>    lib/efi_loader/efi_boottime.c |   52 +-
>>>    lib/efi_loader/efi_console.c  |   78 ++
>>>    lib/efi_loader/efi_disk.c     |   11 +
>>>    lib/efi_loader/efi_file.c     |   75 +-
>>>    9 files changed, 1578 insertions(+), 47 deletions(-)
>>>    create mode 100644 cmd/eficonfig.c
>>>    create mode 100644 include/efi_config.h
>>>
>>> diff --git a/cmd/Kconfig b/cmd/Kconfig
>>> index 09193b61b9..bb7f1d0463 100644
>>> --- a/cmd/Kconfig
>>> +++ b/cmd/Kconfig
>>> @@ -1870,6 +1870,13 @@ config CMD_EFIDEBUG
>>>    	  particularly for managing boot parameters as  well as examining
>>>    	  various EFI status for debugging.
>>>
>>> +config CMD_EFICONFIG
>>> +	bool "eficonfig - provide menu-driven uefi variables maintenance interface"
>>> +	depends on CMD_BOOTEFI_BOOTMGR
>>> +	help
>>> +	  Enable the 'eficonfig' command which provides the menu-driven UEFI
>>> +	  variable maintenance interface.
>>> +
>>>    config CMD_EXCEPTION
>>>    	bool "exception - raise exception"
>>>    	depends on ARM || RISCV || SANDBOX || X86
>>> diff --git a/cmd/Makefile b/cmd/Makefile
>>> index 5e43a1e022..0afa687e94 100644
>>> --- a/cmd/Makefile
>>> +++ b/cmd/Makefile
>>> @@ -63,6 +63,7 @@ obj-$(CONFIG_ENV_IS_IN_EEPROM) += eeprom.o
>>>    obj-$(CONFIG_CMD_EEPROM) += eeprom.o
>>>    obj-$(CONFIG_EFI) += efi.o
>>>    obj-$(CONFIG_CMD_EFIDEBUG) += efidebug.o
>>> +obj-$(CONFIG_CMD_EFICONFIG) += eficonfig.o
>>>    obj-$(CONFIG_CMD_ELF) += elf.o
>>>    obj-$(CONFIG_CMD_EROFS) += erofs.o
>>>    obj-$(CONFIG_HUSH_PARSER) += exit.o
>>> diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
>>> new file mode 100644
>>> index 0000000000..20747db115
>>> --- /dev/null
>>> +++ b/cmd/eficonfig.c
>>> @@ -0,0 +1,1270 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + *  Menu-driven UEFI Variable maintenance
>>> + *
>>> + *  Copyright (c) 2022 Masahisa Kojima, Linaro Limited
>>> + */
>>> +
>>> +#include <ansi.h>
>>> +#include <common.h>
>>> +#include <charset.h>
>>> +#include <efi_loader.h>
>>> +#include <efi_config.h>
>>> +#include <efi_variable.h>
>>> +#include <log.h>
>>> +#include <malloc.h>
>>> +#include <menu.h>
>>> +#include <watchdog.h>
>>> +#include <asm/unaligned.h>
>>> +#include <linux/delay.h>
>>> +
>>> +static struct efi_simple_text_input_protocol *cin;
>>> +static struct efi_simple_text_output_protocol *cout;
>>> +
>>> +#define EFICONFIG_DESCRIPTION_MAX 32
>>> +#define EFICONFIG_OPTIONAL_DATA_MAX 64
>>> +#define EFICONFIG_EDIT_BOOT_OPTION_MENU_ENTRY 5
>>> +
>>> +#define EFICONFIG_AUTO_GENERATED_ENTRY_GUID \
>>> +	EFI_GUID(0x38c1acc1, 0x9fc0, 0x41f0, \
>>> +		 0xb9, 0x01, 0xfa, 0x74, 0xd6, 0xd6, 0xe4, 0xde)
>>> +const efi_guid_t efi_guid_bootmenu_auto_generated =
>>> +		EFICONFIG_AUTO_GENERATED_ENTRY_GUID;
>>> +
>>> +struct eficonfig_boot_selection_data {
>>> +	u16 bootorder_index;
>>> +	int *selected;
>>> +};
>>> +
>>> +struct eficonfig_filepath_info {
>>> +	u16 *name;
>>> +	struct list_head list;
>>> +};
>>> +
>>> +/**
>>> + * struct eficonfig_boot_option - structure to be used for uefi boot option update
>>> + *
>>> + * @file_info:		user selected file info
>>> + * @boot_index:		index of the uefi BootOrder variable
>>> + * @description:	pointer to the description string
>>> + * @optional_data:	pointer to the optional_data
>>> + * @edit_completed:	flag indicates edit complete
>>> + */
>>> +struct eficonfig_boot_option {
>>> +	struct eficonfig_select_file_info file_info;
>>> +	unsigned int boot_index;
>>> +	u16 *description;
>>> +	u16 *optional_data;
>>> +	bool edit_completed;
>>> +};
>>> +
>>> +struct eficonfig_volume_entry_data {
>>> +	struct eficonfig_select_file_info *file_info;
>>> +	struct efi_simple_file_system_protocol *v;
>>> +	struct efi_device_path *dp;
>>> +};
>>> +
>>> +struct eficonfig_file_entry_data {
>>> +	struct eficonfig_select_file_info *file_info;
>>> +	bool is_directory;
>>> +	u16 *file_name;
>>> +};
>>> +
>>> +/**
>>> + * eficonfig_print_msg() - print message
>>> + *
>>> + * display the message to the user, user proceeds the screen
>>> + * with ENTER key press.
>>> + *
>>> + * @items:		pointer to the structure of each menu entry
>>> + * @count:		the number of menu entry
>>> + * @menu_header:	pointer to the menu header string
>>> + * Return:	status code
>>> + */
>>> +void eficonfig_print_msg(char *msg)
>>
>> None of you patches adds this function to an include. So it should be
>> static.
>>
>>> +{
>>> +	char c;
>>> +
>>> +	puts(ANSI_CURSOR_HIDE);
>>> +	puts(ANSI_CLEAR_CONSOLE);
>>> +	printf(ANSI_CURSOR_POSITION, 3, 4);
>>> +	printf(msg);
>>> +
>>> +	/* Flush input */
>>> +	while (tstc())
>>> +		getchar();
>>> +
>>> +	printf("\n\n  Press ENTER to continue");
>>
>> We want to minimize the size of the U-Boot binary.
>>
>> Isn't the following single function call good enough?
>>
>> printf(ANSI_CURSOR_HIDE ANSI_CLEAR_CONSOLE ANSI_CURSOR_POSITION msg
>>         "\n\n  Press ENTER to continue", 3, 4);
>>
>>> +	while (1) {
>>> +		while (!tstc()) {
>>> +			WATCHDOG_RESET();
>>
>> If there is no user to hit a key, why should be stop the watchdog reset?
>>
>>> +			mdelay(10);
>>
>> There is no benefit of calling mdelay here.
>>
>>> +		}
>>> +		c = getchar();
>>> +		if (c == '\r')
>>> +			break;
>>> +	}
>>> +}
>>
>> We should replace the whole loop by:
>>
>> getchar();
>>
>>> +
>>
>>
>> Please, provide Sphinx style function descriptions for all functions.
>>
>>
>>> +static void eficonfig_print_entry(void *data)
>>> +{
>>> +	struct eficonfig_entry *entry = data;
>>> +	int reverse = (entry->efi_menu->active == entry->num);
>>> +
>>> +	/* TODO: support scroll or page for many entries */
>>> +
>>> +	/*
>>> +	 * Move cursor to line where the entry will be drawn (entry->count)
>>> +	 * First 3 lines(menu header) + one empty line
>>> +	 */
>>> +	printf(ANSI_CURSOR_POSITION "     ", entry->num + 4, 1);
>>> +
>>> +	if (reverse)
>>> +		puts(ANSI_COLOR_REVERSE);
>>> +
>>> +	printf("%s", entry->title);
>>> +
>>> +	if (reverse)
>>> +		puts(ANSI_COLOR_RESET);
>>> +}
>>> +
>>> +static void eficonfig_display_statusline(struct menu *m)
>>> +{
>>> +	struct eficonfig_entry *entry;
>>> +
>>> +	if (menu_default_choice(m, (void *)&entry) < 0)
>>> +		return;
>>> +
>>> +	printf(ANSI_CURSOR_POSITION
>>> +	      "\n%s\n"
>>> +	       ANSI_CURSOR_POSITION
>>> +	       "\n"
>>> +	       "  Press UP/DOWN to move, ENTER to select, ESC/CTRL+C to quit",
>>> +	       0, 1, entry->efi_menu->menu_header, entry->efi_menu->count + 5, 1);
>>> +}
>>> +
>>> +static char *eficonfig_choice_entry(void *data)
>>> +{
>>> +	int i;
>>> +	int esc = 0;
>>> +	struct eficonfig_entry *iter;
>>> +	enum bootmenu_key key = KEY_NONE;
>>> +	struct efimenu *efi_menu = data;
>>> +
>>> +	while (1) {
>>> +		bootmenu_loop((struct bootmenu_data *)efi_menu, &key, &esc);
>>> +
>>> +		switch (key) {
>>> +		case KEY_UP:
>>> +			if (efi_menu->active > 0)
>>> +				--efi_menu->active;
>>> +			/* no menu key selected, regenerate menu */
>>> +			return NULL;
>>> +		case KEY_DOWN:
>>> +			if (efi_menu->active < efi_menu->count - 1)
>>> +				++efi_menu->active;
>>> +			/* no menu key selected, regenerate menu */
>>> +			return NULL;
>>> +		case KEY_SELECT:
>>> +			iter = efi_menu->first;
>>> +			for (i = 0; i < efi_menu->active; ++i)
>>> +				iter = iter->next;
>>> +			return iter->key;
>>> +		case KEY_QUIT:
>>> +			/* Quit by choosing the last entry */
>>> +			iter = efi_menu->first;
>>> +			while (iter->next)
>>> +				iter = iter->next;
>>> +			return iter->key;
>>> +		default:
>>> +			break;
>>> +		}
>>> +	}
>>> +
>>> +	/* never happens */
>>> +	debug("eficonfig: this should not happen");
>>
>> This comment and message is wrong.
>> key = KEY_NONE is an expected value for key.
>>
>>> +	return NULL;
>>> +}
>>> +
>>> +static void eficonfig_destroy(struct efimenu *efi_menu)
>>> +{
>>> +	struct eficonfig_entry *next;
>>> +	struct eficonfig_entry *iter = efi_menu->first;
>>> +
>>> +	while (iter) {
>>> +		next = iter->next;
>>> +		free(iter);
>>> +		iter = next;
>>> +	}
>>> +	free(efi_menu->menu_header);
>>> +	free(efi_menu);
>>> +}
>>> +
>>> +/**
>>> + * eficonfig_process_quit() - callback function for "Quit" entry
>>> + *
>>> + * @data:	pointer to the data
>>> + * Return:	status code
>>> + */
>>> +efi_status_t eficonfig_process_quit(void *data)
>>> +{
>>> +	return EFI_ABORTED;
>>> +}
>>> +
>>> +/**
>>> + * eficonfig_process_common() - main handler for uefi menu
>>
>> %s/uefi/UEFI/
>>
>>> + *
>>> + * Construct the structures required to show the menu, then handle
>>> + * the user input intracting with u-boot menu functions.
>>> + *
>>> + * @items:		pointer to the structure of each menu entry
>>> + * @count:		the number of menu entry
>>> + * @menu_header:	pointer to the menu header string
>>> + * Return:	status code
>>> + */
>>> +efi_status_t eficonfig_process_common(const struct eficonfig_item *items, int count,
>>> +				      char *menu_header)
>>> +{
>>> +	u32 i;
>>> +	efi_status_t ret;
>>> +	struct menu *menu;
>>> +	void *choice = NULL;
>>> +	struct eficonfig_entry *entry;
>>> +	struct efimenu *efi_menu;
>>> +	struct eficonfig_entry *iter = NULL;
>>> +
>>> +	if (count > EFICONFIG_ENTRY_NUM_MAX)
>>> +		return EFI_OUT_OF_RESOURCES;
>>> +
>>> +	efi_menu = calloc(1, sizeof(struct efimenu));
>>> +	if (!efi_menu)
>>> +		return EFI_OUT_OF_RESOURCES;
>>> +
>>> +	efi_menu->delay = -1;
>>> +	efi_menu->active = 0;
>>> +	efi_menu->first = NULL;
>>> +
>>> +	if (menu_header) {
>>> +		efi_menu->menu_header = strdup(menu_header);
>>> +		if (!efi_menu->menu_header) {
>>> +			free(efi_menu);
>>> +			return EFI_OUT_OF_RESOURCES;
>>> +		}
>>> +	}
>>> +
>>> +	for (i = 0; i < count; i++) {
>>> +		entry = calloc(1, sizeof(struct eficonfig_entry));
>>> +		if (!entry) {
>>> +			ret = EFI_LOAD_ERROR;
>>> +			goto out;
>>> +		}
>>> +
>>> +		entry->num = i;
>>> +		entry->title = items->title;
>>> +		sprintf(entry->key, "%d", i);
>>> +		entry->efi_menu = efi_menu;
>>> +		entry->func = items->func;
>>> +		entry->data = items->data;
>>> +		entry->next = NULL;
>>> +
>>> +		if (!iter)
>>> +			efi_menu->first = entry;
>>> +		else
>>> +			iter->next = entry;
>>> +
>>> +		iter = entry;
>>> +		items++;
>>> +	}
>>> +	efi_menu->count = count;
>>> +
>>> +	menu = menu_create(NULL, 0, 1, eficonfig_display_statusline,
>>> +			   eficonfig_print_entry, eficonfig_choice_entry,
>>> +			   efi_menu);
>>> +	if (!menu) {
>>> +		ret = EFI_INVALID_PARAMETER;
>>> +		goto out;
>>> +	}
>>> +
>>> +	for (entry = efi_menu->first; entry; entry = entry->next) {
>>> +		if (!menu_item_add(menu, entry->key, entry)) {
>>> +			ret = EFI_INVALID_PARAMETER;
>>> +			goto out;
>>> +		}
>>> +	}
>>> +
>>> +	menu_default_set(menu, efi_menu->first->key);
>>> +
>>> +	puts(ANSI_CURSOR_HIDE);
>>> +	puts(ANSI_CLEAR_CONSOLE);
>>> +	printf(ANSI_CURSOR_POSITION, 1, 1);
>>
>> Please, use a single function call.
>>
>>> +
>>> +	if (menu_get_choice(menu, &choice)) {
>>> +		entry = choice;
>>> +		if (entry->func)
>>> +			ret = entry->func(entry->data);
>>> +	}
>>> +
>>> +out:
>>> +	menu_destroy(menu);
>>> +	eficonfig_destroy(efi_menu);
>>> +
>>> +	puts(ANSI_CLEAR_CONSOLE);
>>> +	printf(ANSI_CURSOR_POSITION, 1, 1);
>>> +	puts(ANSI_CURSOR_SHOW);
>>
>> ditto
>>
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static efi_status_t eficonfig_volume_selected(void *data)
>>> +{
>>> +	struct eficonfig_volume_entry_data *info = data;
>>> +
>>> +	if (info) {
>>> +		info->file_info->current_volume = info->v;
>>> +		info->file_info->dp_volume = info->dp;
>>> +	}
>>> +
>>> +	return EFI_SUCCESS;
>>> +}
>>> +
>>> +static efi_status_t eficonfig_file_selected(void *data)
>>> +{
>>> +	struct eficonfig_file_entry_data *info = data;
>>> +
>>> +	if (!info)
>>> +		return EFI_INVALID_PARAMETER;
>>> +
>>> +	if (u16_strcmp(info->file_name, u".") == 0 &&
>>> +	    u16_strlen(info->file_name) == 1) {
>>> +		/* stay current path */
>>> +	} else if (u16_strcmp(info->file_name, u"..") == 0 &&
>>> +		   u16_strlen(info->file_name) == 2) {
>>> +		struct eficonfig_filepath_info *iter;
>>> +		struct list_head *pos, *n;
>>> +		int is_last;
>>> +
>>> +		memset(info->file_info->current_path, 0, EFICONFIG_FILE_PATH_BUF_SIZE);
>>> +		list_for_each_safe(pos, n, &info->file_info->filepath_list) {
>>> +			iter = list_entry(pos, struct eficonfig_filepath_info, list);
>>> +
>>> +			is_last = list_is_last(&iter->list, &info->file_info->filepath_list);
>>> +			if (is_last) {
>>> +				list_del(&iter->list);
>>> +				free(iter->name);
>>> +				free(iter);
>>> +				break;
>>> +			}
>>> +			u16_strlcat(info->file_info->current_path, iter->name,
>>> +				    EFICONFIG_FILE_PATH_MAX);
>>> +			u16_strlcat(info->file_info->current_path, u"\\",
>>> +				    EFICONFIG_FILE_PATH_MAX);
>>> +		}
>>> +	} else {
>>> +		size_t new_len;
>>> +		struct eficonfig_filepath_info *filepath;
>>> +
>>> +		new_len = u16_strlen(info->file_info->current_path) +
>>> +				     u16_strlen(info->file_name);
>>> +		if (new_len >= EFICONFIG_FILE_PATH_MAX) {
>>> +			eficonfig_print_msg("File path is too long!");
>>> +			return EFI_INVALID_PARAMETER;
>>> +		}
>>> +		u16_strlcat(info->file_info->current_path, info->file_name,
>>> +			    EFICONFIG_FILE_PATH_MAX);
>>> +
>>> +		filepath = calloc(1, sizeof(struct eficonfig_filepath_info));
>>> +		if (!filepath)
>>> +			return EFI_OUT_OF_RESOURCES;
>>> +
>>> +		filepath->name = u16_strdup(info->file_name);
>>> +		if (!filepath->name) {
>>> +			free(filepath);
>>> +			return EFI_OUT_OF_RESOURCES;
>>> +		}
>>> +		list_add_tail(&filepath->list, &info->file_info->filepath_list);
>>> +
>>> +		if (info->is_directory) {
>>> +			/*
>>> +			 * Remainig buffer should have enough space to contain u"\\" and
>>> +			 * at least one character for file name
>>> +			 */
>>> +			if (new_len + 2 >= EFICONFIG_FILE_PATH_MAX) {
>>> +				eficonfig_print_msg("Directory path is too long!");
>>> +				return EFI_INVALID_PARAMETER;
>>> +			}
>>> +			u16_strlcat(info->file_info->current_path, u"\\",
>>> +				    EFICONFIG_FILE_PATH_MAX);
>>> +		} else {
>>> +			info->file_info->file_selected = true;
>>> +		}
>>> +	}
>>> +	return EFI_SUCCESS;
>>> +}
>>> +
>>> +static efi_status_t eficonfig_select_volume(struct eficonfig_select_file_info *file_info)
>>> +{
>>> +	u32 i;
>>> +	efi_status_t ret;
>>> +	efi_uintn_t count;
>>> +	struct efi_handler *handler;
>>> +	struct efi_device_path *device_path;
>>> +	efi_handle_t *volume_handles = NULL;
>>> +	struct eficonfig_item *menu_item, *iter;
>>> +	struct efi_simple_file_system_protocol *v;
>>> +
>>> +	ret = efi_locate_handle_buffer_int(BY_PROTOCOL, &efi_simple_file_system_protocol_guid,
>>> +					   NULL, &count, (efi_handle_t **)&volume_handles);
>>> +	if (ret != EFI_SUCCESS) {
>>> +		eficonfig_print_msg("No block device found!");
>>> +		return ret;
>>> +	}
>>> +
>>> +	menu_item = calloc(count + 1, sizeof(struct eficonfig_item));
>>> +	if (!menu_item) {
>>> +		efi_free_pool(volume_handles);
>>> +		return EFI_OUT_OF_RESOURCES;
>>> +	}
>>> +
>>> +	iter = menu_item;
>>> +	for (i = 0; i < count; i++) {
>>> +		char *devname;
>>> +		struct efi_block_io *block_io;
>>> +		struct eficonfig_volume_entry_data *info;
>>> +
>>> +		ret = efi_search_protocol(volume_handles[i],
>>> +					  &efi_simple_file_system_protocol_guid, &handler);
>>> +		if (ret != EFI_SUCCESS)
>>> +			continue;
>>> +		ret = efi_protocol_open(handler, (void **)&v, efi_root, NULL,
>>> +					EFI_OPEN_PROTOCOL_GET_PROTOCOL);
>>> +		if (ret != EFI_SUCCESS)
>>> +			continue;
>>> +
>>> +		ret = efi_search_protocol(volume_handles[i], &efi_guid_device_path, &handler);
>>> +		if (ret != EFI_SUCCESS)
>>> +			continue;
>>> +		ret = efi_protocol_open(handler, (void **)&device_path,
>>> +					efi_root, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL);
>>> +		if (ret != EFI_SUCCESS)
>>> +			continue;
>>> +
>>> +		ret = efi_search_protocol(volume_handles[i], &efi_block_io_guid, &handler);
>>> +		if (ret != EFI_SUCCESS)
>>> +			continue;
>>> +		ret = efi_protocol_open(handler, (void **)&block_io,
>>> +					efi_root, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL);
>>> +		if (ret != EFI_SUCCESS)
>>> +			continue;
>>> +
>>> +		info = calloc(1, sizeof(struct eficonfig_volume_entry_data));
>>> +		if (!info) {
>>> +			ret = EFI_OUT_OF_RESOURCES;
>>> +			goto out;
>>> +		}
>>> +
>>> +		devname = calloc(1, BOOTMENU_DEVICE_NAME_MAX);
>>> +		if (!devname) {
>>> +			free(info);
>>> +			ret = EFI_OUT_OF_RESOURCES;
>>> +			goto out;
>>> +		}
>>> +		efi_disk_get_device_name(block_io, devname, BOOTMENU_DEVICE_NAME_MAX);
>>> +
>>> +		info->v = v;
>>> +		info->dp = device_path;
>>> +		info->file_info = file_info;
>>> +		iter->title = devname;
>>> +		iter->func = eficonfig_volume_selected;
>>> +		iter->data = info;
>>> +		iter++;
>>> +	}
>>> +
>>> +	iter->title = strdup("Quit");
>>> +	iter->func = eficonfig_process_quit;
>>> +	iter->data = NULL;
>>> +	count += 1;
>>> +
>>> +	ret = eficonfig_process_common(menu_item, count, "  ** Select Volume **");
>>> +
>>> +out:
>>> +	iter = menu_item;
>>> +	for (i = 0; i < count; i++, iter++) {
>>> +		free(iter->data);
>>> +		free(iter->title);
>>> +	}
>>> +
>>> +	free(menu_item);
>>> +
>>> +	efi_free_pool(volume_handles);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static efi_status_t eficonfig_select_file(struct eficonfig_select_file_info *file_info,
>>> +					  struct efi_file_handle *root)
>>> +{
>>> +	u32 i;
>>> +	u32 count = 0;
>>> +	efi_uintn_t len;
>>> +	efi_status_t ret;
>>> +	struct efi_file_handle *f;
>>> +	struct efi_file_info *buf;
>>> +	struct eficonfig_item *menu_item, *iter;
>>> +
>>> +	buf = calloc(1, sizeof(struct efi_file_info) + EFICONFIG_FILE_PATH_BUF_SIZE);
>>> +	if (!buf)
>>> +		return EFI_OUT_OF_RESOURCES;
>>> +
>>> +	while (!file_info->file_selected) {
>>> +		count = 0;
>>> +
>>> +		ret = efi_file_open_int(root, &f, file_info->current_path, EFI_FILE_MODE_READ, 0);
>>> +		if (ret != EFI_SUCCESS) {
>>> +			/* TODO: need to fileter out non-FAT partition? */
>>
>> %s/fileter/filter/
>>
>>> +			eficonfig_print_msg("Reading volume failed! Please make sure the selected\n"
>>> +					    "   volume is FAT12/FAT16/FAT32 partition.");
>>
>> Who cares if the file is on FAT, ext4, or any other file system else if
>> U-Boot can read it?
>
> UEFI specification and distro's does.
>>From technical viewpoint, there is no reason that we need to impose a restriction on file system type.
> But, from the viewpoint of goal of UEFI, we should have a very common file system, that is FAT,
> on UEFI system partition.
> UEFI specification says:
>
> ---8<---
> 1.5 UEFI Design Overview
> ...
> system partition. The System partition defines a partition and file system that are designed to
> allow safe sharing between multiple vendors, and for different purposes. The ability to include
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> a separate, sharable system partition presents an opportunity to increase platform value-add
> without significantly growing the need for nonvolatile platform memory.
>
> 13.3.1.3 Directory Structure
> An EFI system partition that is present on a hard disk must contain an EFI defined directory in the root
> directory. This directory is named EFI. All OS loaders and applications will be stored in subdirectories
> below EFI.
> --->8---

I agree that the firmware must support the FAT file system and the ESP
should be FAT formatted. But the UEFI specifcation does neither forbid
support for further file-systems nor launching EFI binaries located
outside of the ESP.

U-Boot has always allowed running EFI binaries from non-FAT partitions.
The efidebug command allows to define boot options for such binaries. I
cannot see why the menu driven definition of boot options should deviate.

Best regards

Heinrich

>
> I believe that FAT file system is the only file system to meet the goal.
>
> Furthermore, for instance, installing "grub" package fails on Debian
> if there is no FAT file system available as the system partition.
>
> That said, I don't think we'd better refer to the file system in an error message:)
>
> -Takahiro Akashi
>
>>> +			ret = EFI_ABORTED;
>>> +			goto out;
>>> +		}
>>> +
>>> +		/* calculate directory information total count */
>>
>> /* Count the number of directory entries */
>>
>>> +		for (;;) {
>>> +			len = sizeof(struct efi_file_info) + EFICONFIG_FILE_PATH_BUF_SIZE;
>>> +			ret = efi_file_read_int(f, &len, buf);
>>> +			if (ret != EFI_SUCCESS || len == 0)
>>> +				break;
>>> +
>>> +			count++;
>>> +		}
>>> +
>>> +		menu_item = calloc(count + 1, sizeof(struct eficonfig_item));
>>> +		if (!menu_item) {
>>> +			efi_file_close_int(f);
>>> +			ret = EFI_OUT_OF_RESOURCES;
>>> +			goto out;
>>> +		}
>>> +
>>> +		/* read directory and construct menu structure */
>>> +		efi_file_setpos_int(f, 0);
>>> +		iter = menu_item;
>>> +		for (i = 0; i < count; i++) {
>>> +			char *name, *p;
>>> +			int name_len;
>>> +			struct eficonfig_file_entry_data *info;
>>> +
>>> +			len = sizeof(struct efi_file_info) + EFICONFIG_FILE_PATH_BUF_SIZE;
>>> +			ret = efi_file_read_int(f, &len, buf);
>>> +			if (ret != EFI_SUCCESS || len == 0)
>>> +				goto err;
>>> +
>>> +			info = calloc(1, sizeof(struct eficonfig_file_entry_data));
>>> +			if (!info) {
>>> +				ret = EFI_OUT_OF_RESOURCES;
>>> +				goto err;
>>> +			}
>>> +
>>> +			if (buf->attribute & EFI_FILE_DIRECTORY) {
>>
>> Should we filter out '.' and '..'?
>>
>>> +				/* append u'/' at the end of directory name */
>>> +				name_len = utf16_utf8_strlen(buf->file_name) + 2;
>>> +				name = calloc(1, name_len);
>>> +				if (!name) {
>>> +					ret = EFI_OUT_OF_RESOURCES;
>>> +					goto err;
>>> +				}
>>> +				p = name;
>>> +				utf16_utf8_strcpy(&p, buf->file_name);
>>> +				name[u16_strlen(buf->file_name)] = u'/';
>>> +
>>> +				info->is_directory = true;
>>> +			} else {
>>> +				name_len = utf16_utf8_strlen(buf->file_name) + 1;
>>> +				name = calloc(1, name_len);
>>> +				if (!name) {
>>> +					ret = EFI_OUT_OF_RESOURCES;
>>> +					goto err;
>>> +				}
>>> +				p = name;
>>> +				utf16_utf8_strcpy(&p, buf->file_name);
>>> +			}
>>> +
>>> +			info->file_name = u16_strdup(buf->file_name);
>>> +			info->file_info = file_info;
>>> +			iter->title = name;
>>> +			iter->func = eficonfig_file_selected;
>>> +			iter->data = info;
>>> +			iter++;
>>> +		}
>>> +
>>> +		/* add "Quit" entry */
>>> +		iter->title = "Quit";
>>> +		iter->func = eficonfig_process_quit;
>>> +		iter->data = NULL;
>>> +		count += 1;
>>
>> We want to reduce code size. Start the function with
>>
>> unsigned int count = 1;
>>
>>> +
>>> +		ret = eficonfig_process_common(menu_item, count, "  ** Select File **");
>>> +err:
>>> +		efi_file_close_int(f);
>>> +		iter = menu_item;
>>> +		for (i = 0; i < count - 1; i++, iter++) {
>>> +			free(((struct eficonfig_file_entry_data *)(iter->data))->file_name);
>>> +			free(iter->title);
>>> +			free(iter->data);
>>> +		}
>>> +
>>> +		free(menu_item);
>>> +
>>> +		if (ret != EFI_SUCCESS)
>>> +			break;
>>> +	}
>>> +
>>> +out:
>>> +	free(buf);
>>> +	return ret;
>>> +}
>>> +
>>> +static efi_status_t eficonfig_boot_add_enter_description(void *data)
>>> +{
>>> +	u16 *tmp;
>>> +	efi_status_t ret;
>>> +	struct eficonfig_boot_option *bo = data;
>>> +
>>> +	printf(ANSI_CLEAR_CONSOLE
>>> +	       ANSI_CURSOR_POSITION
>>> +	       "\n  ** Edit Description **\n"
>>> +	       "\n"
>>> +	       "  enter description: "
>>> +	       ANSI_CURSOR_POSITION
>>> +	       "  Press ENTER to complete, ESC/CTRL+C to quit",
>>> +	       0, 1, 8, 1);
>>> +
>>> +	tmp = calloc(1, EFICONFIG_DESCRIPTION_MAX * sizeof(u16));
>>> +	if (!tmp)
>>> +		return EFI_OUT_OF_RESOURCES;
>>> +
>>> +	ret = efi_console_get_u16_string(cin, cout, tmp,
>>> +					 EFICONFIG_DESCRIPTION_MAX, NULL, 4, 22);
>>> +	if (ret == EFI_SUCCESS)
>>> +		u16_strcpy(bo->description, tmp);
>>> +
>>> +	free(tmp);
>>> +
>>> +	/* to stay the parent menu */
>>> +	ret = (ret == EFI_ABORTED) ? EFI_NOT_READY : ret;
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static efi_status_t eficonfig_boot_add_optional_data(void *data)
>>> +{
>>> +	u16 *tmp;
>>> +	efi_status_t ret;
>>> +	struct eficonfig_boot_option *bo = data;
>>> +
>>> +	printf(ANSI_CLEAR_CONSOLE
>>> +	       ANSI_CURSOR_POSITION
>>> +	       "\n  ** Edit Optional Data **\n"
>>> +	       "\n"
>>> +	       "  enter optional data:"
>>> +	       ANSI_CURSOR_POSITION
>>> +	       "  Press ENTER to complete, ESC/CTRL+C to quit",
>>> +	       0, 1, 8, 1);
>>> +
>>> +	tmp = calloc(1, EFICONFIG_OPTIONAL_DATA_MAX * sizeof(u16));
>>> +	ret = efi_console_get_u16_string(cin, cout, tmp,
>>> +					 EFICONFIG_OPTIONAL_DATA_MAX, NULL, 4, 24);
>>> +
>>> +	if (ret == EFI_SUCCESS)
>>> +		u16_strcpy(bo->optional_data, tmp);
>>> +
>>> +	free(tmp);
>>> +
>>> +	/* to stay the parent menu */
>>> +	ret = (ret == EFI_ABORTED) ? EFI_NOT_READY : ret;
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static efi_status_t eficonfig_boot_edit_save(void *data)
>>> +{
>>> +	struct eficonfig_boot_option *bo = data;
>>> +
>>> +	if (u16_strlen(bo->description) == 0) {
>>> +		eficonfig_print_msg("Boot Description is empty!");
>>> +		bo->edit_completed = false;
>>> +		return EFI_NOT_READY;
>>> +	}
>>> +	if (u16_strlen(bo->file_info.current_path) == 0) {
>>> +		eficonfig_print_msg("File is not selected!");
>>> +		bo->edit_completed = false;
>>> +		return EFI_NOT_READY;
>>> +	}
>>> +
>>> +	bo->edit_completed = true;
>>> +
>>> +	return EFI_SUCCESS;
>>> +}
>>> +
>>> +static efi_status_t eficonfig_boot_edit_quit(void *data)
>>> +{
>>> +	return EFI_ABORTED;
>>> +}
>>> +
>>> +/**
>>> + * eficonfig_select_file_handler() - handle user file selection
>>> + *
>>> + * @data:	pointer to the data
>>> + * Return:	status code
>>> + */
>>> +efi_status_t eficonfig_select_file_handler(void *data)
>>> +{
>>> +	size_t len;
>>> +	efi_status_t ret;
>>> +	struct list_head *pos, *n;
>>> +	struct efi_file_handle *root;
>>> +	struct eficonfig_filepath_info *item;
>>> +	struct eficonfig_select_file_info *file_info = data;
>>> +	struct eficonfig_select_file_info *tmp = NULL;
>>> +
>>> +	tmp = calloc(1, sizeof(struct eficonfig_select_file_info));
>>> +	if (!tmp)
>>> +		return EFI_OUT_OF_RESOURCES;
>>> +
>>> +	tmp->current_path = calloc(1, EFICONFIG_FILE_PATH_BUF_SIZE);
>>> +	if (!tmp->current_path) {
>>> +		free(tmp);
>>> +		return EFI_OUT_OF_RESOURCES;
>>> +	}
>>> +	INIT_LIST_HEAD(&tmp->filepath_list);
>>> +
>>> +	while (!tmp->file_selected) {
>>> +		tmp->current_volume = NULL;
>>> +		memset(tmp->current_path, 0, EFICONFIG_FILE_PATH_BUF_SIZE);
>>> +
>>> +		ret = eficonfig_select_volume(tmp);
>>> +		if (ret != EFI_SUCCESS)
>>> +			goto out;
>>> +
>>> +		if (!tmp->current_volume)
>>> +			return EFI_INVALID_PARAMETER;
>>> +
>>> +		ret = efi_open_volume_int(tmp->current_volume, &root);
>>> +		if (ret != EFI_SUCCESS)
>>> +			goto out;
>>> +
>>> +		ret = eficonfig_select_file(tmp, root);
>>> +		if (ret == EFI_ABORTED)
>>> +			continue;
>>> +		if (ret != EFI_SUCCESS)
>>> +			goto out;
>>> +	}
>>> +
>>> +out:
>>> +	if (ret == EFI_SUCCESS) {
>>> +		len = u16_strlen(tmp->current_path);
>>> +		len = (len >= EFICONFIG_FILE_PATH_MAX) ? (EFICONFIG_FILE_PATH_MAX - 1) : len;
>>> +		memcpy(file_info->current_path, tmp->current_path, len * sizeof(u16));
>>> +		file_info->current_path[len] = u'\0';
>>> +		file_info->current_volume = tmp->current_volume;
>>> +		file_info->dp_volume = tmp->dp_volume;
>>> +	}
>>> +
>>> +	list_for_each_safe(pos, n, &tmp->filepath_list) {
>>> +		item = list_entry(pos, struct eficonfig_filepath_info, list);
>>> +		list_del(&item->list);
>>> +		free(item->name);
>>> +		free(item);
>>> +	}
>>> +	free(tmp->current_path);
>>> +	free(tmp);
>>> +
>>> +	/* to stay the parent menu */
>>> +	ret = (ret == EFI_ABORTED) ? EFI_NOT_READY : ret;
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +efi_status_t eficonfig_get_unused_bootoption(u16 *buf, efi_uintn_t buf_size,
>>> +					     unsigned int *index)
>>> +{
>>> +	u32 i;
>>> +	efi_status_t ret;
>>> +	efi_uintn_t size;
>>> +
>>> +	if (buf_size < u16_strsize(u"Boot####"))
>>> +		return EFI_BUFFER_TOO_SMALL;
>>> +
>>> +	for (i = 0; i <= 0xFFFF; i++) {
>>> +		size = 0;
>>> +		efi_create_indexed_name(buf, buf_size, "Boot", i);
>>> +		ret = efi_get_variable_int(buf, &efi_global_variable_guid,
>>> +					   NULL, &size, NULL, NULL);
>>> +		if (ret == EFI_BUFFER_TOO_SMALL)
>>> +			continue;
>>> +		else
>>> +			break;
>>> +	}
>>> +
>>> +	if (i > 0xFFFF)
>>> +		return EFI_OUT_OF_RESOURCES;
>>> +
>>> +	*index = i;
>>> +
>>> +	return EFI_SUCCESS;
>>> +}
>>> +
>>> +static efi_status_t eficonfig_set_boot_option(u16 *varname, struct efi_device_path *dp,
>>> +					      u16 *label, char *optional_data)
>>> +{
>>> +	void *p = NULL;
>>> +	efi_status_t ret;
>>> +	efi_uintn_t size;
>>> +	struct efi_load_option lo;
>>> +
>>> +	lo.file_path = dp;
>>> +	lo.file_path_length = efi_dp_size(dp) + sizeof(END);
>>> +	lo.attributes = LOAD_OPTION_ACTIVE;
>>> +	lo.optional_data = optional_data;
>>> +	lo.label = label;
>>> +
>>> +	size = efi_serialize_load_option(&lo, (u8 **)&p);
>>> +	if (!size)
>>> +		return EFI_INVALID_PARAMETER;
>>> +
>>> +	ret = efi_set_variable_int(varname, &efi_global_variable_guid,
>>> +				   EFI_VARIABLE_NON_VOLATILE |
>>> +				   EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>> +				   EFI_VARIABLE_RUNTIME_ACCESS,
>>> +				   size, p, false);
>>> +	free(p);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +efi_status_t eficonfig_append_bootorder(u16 index)
>>> +{
>>> +	u16 *bootorder;
>>> +	efi_status_t ret;
>>> +	u16 *new_bootorder = NULL;
>>> +	efi_uintn_t last, size, new_size;
>>> +
>>> +	/* append new boot option */
>>> +	bootorder = efi_get_var(u"BootOrder", &efi_global_variable_guid, &size);
>>> +	last = size / sizeof(u16);
>>> +	new_size = size + sizeof(u16);
>>> +	new_bootorder = calloc(1, new_size);
>>> +	if (!new_bootorder) {
>>> +		ret = EFI_OUT_OF_RESOURCES;
>>> +		goto out;
>>> +	}
>>> +	memcpy(new_bootorder, bootorder, size);
>>> +	new_bootorder[last] = index;
>>> +
>>> +	ret = efi_set_variable_int(u"BootOrder", &efi_global_variable_guid,
>>> +				   EFI_VARIABLE_NON_VOLATILE |
>>> +				   EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>> +				   EFI_VARIABLE_RUNTIME_ACCESS,
>>> +				   new_size, new_bootorder, false);
>>> +	if (ret != EFI_SUCCESS)
>>> +		goto out;
>>> +
>>> +out:
>>> +	free(bootorder);
>>> +	free(new_bootorder);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static efi_status_t eficonfig_convert_dp_to_device_name(struct efi_device_path *dp,
>>> +							char *buf, int size)
>>
>> This function seems to be generic enough to live in lib/efi_loader/ as a
>> library function.
>>
>>> +{
>>> +	u32 i;
>>> +	efi_status_t ret;
>>> +	struct efi_handler *handler;
>>> +	efi_uintn_t count, dp_size, iter_dp_size;
>>> +	efi_handle_t *volume_handles = NULL;
>>> +	struct efi_device_path *iter_dp;
>>> +
>>> +	if (!dp || !buf || !size)
>>> +		return EFI_INVALID_PARAMETER;
>>> +
>>> +	ret = efi_locate_handle_buffer_int(BY_PROTOCOL, &efi_simple_file_system_protocol_guid,
>>> +					   NULL, &count, (efi_handle_t **)&volume_handles);
>>
>> Please, use efi_dp_find_obj() to retrieve the handle.
>>
>>> +	if (ret != EFI_SUCCESS)
>>> +		return ret;
>>> +
>>> +	dp_size = efi_dp_size(dp);
>>> +
>>> +	for (i = 0; i < count; i++) {
>>> +		struct efi_block_io *block_io;
>>> +
>>> +		ret = efi_search_protocol(volume_handles[i],
>>> +					  &efi_simple_file_system_protocol_guid, &handler);
>>> +		if (ret != EFI_SUCCESS)
>>> +			continue;
>>> +
>>> +		ret = efi_search_protocol(volume_handles[i], &efi_guid_device_path, &handler);
>>> +		if (ret != EFI_SUCCESS)
>>> +			continue;
>>> +		ret = efi_protocol_open(handler, (void **)&iter_dp,
>>> +					efi_root, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL);
>>> +		if (ret != EFI_SUCCESS)
>>> +			continue;
>>> +
>>> +		ret = efi_search_protocol(volume_handles[i], &efi_block_io_guid, &handler);
>>> +		if (ret != EFI_SUCCESS)
>>> +			continue;
>>> +		ret = efi_protocol_open(handler, (void **)&block_io,
>>> +					efi_root, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL);
>>> +		if (ret != EFI_SUCCESS)
>>> +			continue;
>>> +
>>> +		iter_dp_size = efi_dp_size(iter_dp);
>>> +		if (dp_size == iter_dp_size) {
>>> +			if (memcmp(dp, iter_dp, dp_size) == 0) {
>>> +				efi_disk_get_device_name(block_io, buf, size);
>>> +				return EFI_SUCCESS;
>>> +			}
>>> +		}
>>> +	}
>>> +
>>> +	return EFI_NOT_FOUND;
>>> +}
>>> +
>>> +static efi_status_t create_boot_option_entry(void *data, char *title, u16 *val,
>>> +					     eficonfig_entry_func func, struct eficonfig_item *iter)
>>> +{
>>> +	u32 len;
>>> +	char  *p;
>>> +
>>> +	len = strlen(title) + 1;
>>> +	if (val)
>>> +		len += utf16_utf8_strlen(val);
>>> +	iter->title = calloc(1, len);
>>> +	if (!iter->title)
>>> +		return EFI_OUT_OF_RESOURCES;
>>> +
>>> +	strcpy(iter->title, title);
>>> +	if (val) {
>>> +		p = iter->title + strlen(title);
>>> +		utf16_utf8_strcpy(&p, val);
>>> +	}
>>> +
>>> +	iter->func = func;
>>> +	iter->data = data;
>>> +
>>> +	return EFI_SUCCESS;
>>> +}
>>> +
>>> +/**
>>> + * eficonfig_show_boot_option() - prepare menu entry for editing boot option
>>> + *
>>> + * Construct the structures to create edit boot option menu
>>> + *
>>> + * @bo:		pointer to the boot option
>>> + * @header_str:	pointer to the header string
>>> + * Return:	status code
>>> + */
>>> +static efi_status_t eficonfig_show_boot_option(struct eficonfig_boot_option *bo,
>>> +					       char *header_str)
>>> +{
>>> +	u32 i, len;
>>> +	u16 *file_name, *p;
>>> +	struct eficonfig_item *menu_item, *iter;
>>> +	efi_status_t ret = EFI_OUT_OF_RESOURCES;
>>> +	char devname[BOOTMENU_DEVICE_NAME_MAX] = {0};
>>> +
>>> +	menu_item = calloc(EFICONFIG_EDIT_BOOT_OPTION_MENU_ENTRY, sizeof(struct eficonfig_item));
>>> +	if (!menu_item)
>>> +		goto out;
>>> +
>>> +	iter = menu_item;
>>> +
>>> +	ret = create_boot_option_entry(bo, "Description: ", bo->description,
>>> +				       eficonfig_boot_add_enter_description, iter++);
>>> +	if (ret != EFI_SUCCESS)
>>> +		goto out;
>>> +
>>> +	/* file name */
>>> +	eficonfig_convert_dp_to_device_name(bo->file_info.dp_volume, devname,
>>> +					    BOOTMENU_DEVICE_NAME_MAX);
>>> +	/*
>>> +	 * efi_convert_device_path_to_text() automatically adds u'/' at the beginning of
>>> +	 * file name, add manually u'/' at the last of device name if there is no u'/'
>>
>> There is nothing manual here.
>>
>> %s/add manually u'\/' at the last of device name/append u'\/'/
>>
>>> +	 * at bo->file_info.current_path[0].
>>> +	 */
>>> +	if (bo->file_info.current_path[0] != u'\0' && bo->file_info.current_path[0] != u'/')
>>> +		strlcat(devname, "/", BOOTMENU_DEVICE_NAME_MAX);
>>> +
>>> +	len = strlen(devname);
>>> +	len += utf16_utf8_strlen(bo->file_info.current_path) + 1;
>>> +	file_name = calloc(1, len * sizeof(u16));
>>> +	if (!file_name)
>>> +		goto out;
>>> +
>>> +	p = file_name;
>>> +	utf8_utf16_strcpy(&p, devname);
>>> +	u16_strlcat(file_name, bo->file_info.current_path, len);
>>> +	ret = create_boot_option_entry(&bo->file_info, "File: ", file_name,
>>> +				       eficonfig_select_file_handler, iter++);
>>> +	if (ret != EFI_SUCCESS)
>>> +		goto out;
>>> +
>>> +	ret = create_boot_option_entry(bo, "Optional Data: ", bo->optional_data,
>>> +				       eficonfig_boot_add_optional_data, iter++);
>>> +	if (ret != EFI_SUCCESS)
>>> +		goto out;
>>> +
>>> +	ret = create_boot_option_entry(bo, "Save", NULL,
>>> +				       eficonfig_boot_edit_save, iter++);
>>> +	if (ret != EFI_SUCCESS)
>>> +		goto out;
>>> +
>>> +	ret = create_boot_option_entry(bo, "Quit", NULL,
>>> +				       eficonfig_boot_edit_quit, iter++);
>>> +	if (ret != EFI_SUCCESS)
>>> +		goto out;
>>> +
>>> +	ret = eficonfig_process_common(menu_item, EFICONFIG_EDIT_BOOT_OPTION_MENU_ENTRY,
>>> +				       header_str);
>>> +
>>> +out:
>>> +	iter = menu_item;
>>> +	for (i = 0; i < EFICONFIG_EDIT_BOOT_OPTION_MENU_ENTRY; i++) {
>>> +		free(iter->title);
>>> +		iter++;
>>> +	}
>>> +
>>> +	free(file_name);
>>> +	free(menu_item);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +/**
>>> + * eficonfig_edit_boot_option() - prepare boot option structure for editing
>>> + *
>>> + * Construct the boot option structure and copy the existing value
>>> + *
>>> + * @varname:		pointer to the uefi variable name
>>> + * @bo:			pointer to the boot option
>>> + * @description:	pointer to the description
>>> + * @optional_data:	pointer to the optional_data
>>> + * @optional_data_size:	optional_data_size
>>> + * @dp:			pointer to the device path
>>> + * @header_str:		pointer to the header string
>>> + * Return	:	status code
>>> + */
>>> +static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_boot_option *bo,
>>> +					       u16 *description, const u8 *optional_data,
>>> +					       efi_uintn_t optional_data_size,
>>> +					       struct efi_device_path *dp,
>>> +					       char *header_str)
>>> +{
>>> +	size_t len;
>>> +	char *buf = NULL;
>>> +	efi_status_t ret;
>>> +	char *iter = NULL;
>>> +	char *tmp = NULL, *p;
>>> +	efi_uintn_t dp_size, fp_size;
>>> +	struct efi_device_path *device_dp = NULL;
>>> +	struct efi_device_path_file_path *fp;
>>> +
>>> +	bo->file_info.current_path = calloc(1, EFICONFIG_FILE_PATH_BUF_SIZE);
>>> +	if (!bo->file_info.current_path) {
>>> +		ret =  EFI_OUT_OF_RESOURCES;
>>> +		goto out;
>>> +	}
>>> +
>>> +	bo->description = calloc(1, EFICONFIG_DESCRIPTION_MAX * sizeof(u16));
>>> +	if (!bo->description) {
>>> +		ret =  EFI_OUT_OF_RESOURCES;
>>> +		goto out;
>>> +	}
>>> +
>>> +	bo->optional_data = calloc(1, EFICONFIG_OPTIONAL_DATA_MAX * sizeof(u16));
>>> +	if (!bo->optional_data) {
>>> +		ret =  EFI_OUT_OF_RESOURCES;
>>> +		goto out;
>>> +	}
>>> +
>>> +	if (description && u16_strlen(description) >= EFICONFIG_DESCRIPTION_MAX) {
>>> +		ret = EFI_INVALID_PARAMETER;
>>> +		goto out;
>>> +	}
>>> +	if (description)
>>> +		u16_strcpy(bo->description, description);
>>> +
>>> +	if (dp) {
>>> +		u16 *file_str;
>>> +		struct efi_device_path *file_dp = NULL;
>>> +
>>> +		efi_dp_split_file_path(dp, &device_dp, &file_dp);
>>> +		bo->file_info.dp_volume = device_dp;
>>> +		file_str = efi_dp_str(file_dp);
>>> +		u16_strcpy(bo->file_info.current_path, file_str);
>>> +		efi_free_pool(file_dp);
>>> +		efi_free_pool(file_str);
>>> +	}
>>> +
>>> +	if (optional_data && optional_data_size >= EFICONFIG_OPTIONAL_DATA_MAX * sizeof(u16)) {
>>> +		ret = EFI_INVALID_PARAMETER;
>>> +		goto out;
>>> +	}
>>> +	if (optional_data && optional_data_size > 0)
>>> +		memcpy(bo->optional_data, optional_data, optional_data_size);
>>> +
>>> +	while (1) {
>>> +		ret = eficonfig_show_boot_option(bo, header_str);
>>> +		if (ret == EFI_SUCCESS && bo->edit_completed)
>>> +			break;
>>> +		if (ret == EFI_NOT_READY)
>>> +			continue;
>>> +		if (ret != EFI_SUCCESS)
>>> +			goto out;
>>> +	}
>>> +
>>> +	dp_size = efi_dp_size(bo->file_info.dp_volume);
>>> +	fp_size = sizeof(struct efi_device_path) +
>>> +		  ((u16_strlen(bo->file_info.current_path) + 1) * sizeof(u16));
>>> +	buf = calloc(1, dp_size + fp_size + sizeof(END));
>>> +	if (!buf)
>>> +		goto out;
>>> +
>>> +	iter = buf;
>>> +	memcpy(iter, bo->file_info.dp_volume, dp_size);
>>> +	iter += dp_size;
>>> +
>>> +	fp = (struct efi_device_path_file_path *)iter;
>>> +	fp->dp.type = DEVICE_PATH_TYPE_MEDIA_DEVICE;
>>> +	fp->dp.sub_type = DEVICE_PATH_SUB_TYPE_FILE_PATH;
>>> +	fp->dp.length = (u16)fp_size;
>>> +	u16_strcpy(fp->str, bo->file_info.current_path);
>>> +	iter += fp_size;
>>> +	*((struct efi_device_path *)iter) = END;
>>> +
>>> +	len = utf16_utf8_strlen(bo->optional_data) + 1;
>>> +	tmp = calloc(1, len);
>>> +	if (!tmp)
>>> +		goto out;
>>> +	p = tmp;
>>> +	utf16_utf8_strncpy(&p, bo->optional_data, u16_strlen(bo->optional_data));
>>> +
>>> +	ret = eficonfig_set_boot_option(varname, (struct efi_device_path *)buf,
>>> +					bo->description, tmp);
>>> +	if (ret != EFI_SUCCESS)
>>> +		goto out;
>>> +
>>> +out:
>>> +	free(tmp);
>>> +	free(buf);
>>> +	free(bo->optional_data);
>>> +	free(bo->description);
>>> +	free(bo->file_info.current_path);
>>> +	efi_free_pool(device_dp);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static efi_status_t eficonfig_process_add_boot_option(void *data)
>>> +{
>>> +	u16 varname[9];
>>> +	efi_status_t ret;
>>> +	struct eficonfig_boot_option *bo = NULL;
>>> +
>>> +	bo = calloc(1, sizeof(struct eficonfig_boot_option));
>>> +	if (!bo)
>>> +		return EFI_OUT_OF_RESOURCES;
>>> +
>>> +	ret = eficonfig_get_unused_bootoption(varname, sizeof(varname), &bo->boot_index);
>>> +	if (ret != EFI_SUCCESS)
>>> +		return ret;
>>> +
>>> +	ret = eficonfig_edit_boot_option(varname, bo, NULL, NULL, 0, NULL,
>>> +					 "  ** Add Boot Option ** ");
>>> +	if (ret != EFI_SUCCESS)
>>> +		goto out;
>>> +
>>> +	ret = eficonfig_append_bootorder((u16)bo->boot_index);
>>> +	if (ret != EFI_SUCCESS)
>>> +		goto out;
>>> +
>>> +out:
>>> +	free(bo);
>>> +
>>> +	/* to stay the parent menu */
>>> +	ret = (ret == EFI_ABORTED) ? EFI_SUCCESS : ret;
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static efi_status_t eficonfig_init(void)
>>> +{
>>> +	efi_status_t ret;
>>> +	static bool init;
>>> +	struct efi_handler *handler;
>>> +
>>> +	if (!init) {
>>> +		ret = efi_search_protocol(efi_root, &efi_guid_text_input_protocol, &handler);
>>> +		if (ret != EFI_SUCCESS)
>>> +			return ret;
>>> +
>>> +		ret = efi_protocol_open(handler, (void **)&cin, efi_root, NULL,
>>> +					EFI_OPEN_PROTOCOL_GET_PROTOCOL);
>>> +		if (ret != EFI_SUCCESS)
>>> +			return ret;
>>> +
>>> +		ret = efi_search_protocol(efi_root, &efi_guid_text_output_protocol, &handler);
>>> +		if (ret != EFI_SUCCESS)
>>> +			return ret;
>>> +
>>> +		ret = efi_protocol_open(handler, (void **)&cout, efi_root, NULL,
>>> +					EFI_OPEN_PROTOCOL_GET_PROTOCOL);
>>> +		if (ret != EFI_SUCCESS)
>>> +			return ret;
>>> +	}
>>> +
>>> +	init = true;
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static const struct eficonfig_item maintenance_menu_items[] = {
>>> +	{"Add Boot Option", eficonfig_process_add_boot_option},
>>> +	{"Quit", eficonfig_process_quit},
>>> +};
>>> +
>>> +int do_eficonfig(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>>> +{
>>> +	efi_status_t ret;
>>> +
>>> +	if (argc > 1)
>>> +		return CMD_RET_USAGE;
>>> +
>>> +	ret = efi_init_obj_list();
>>
>> We should add efi_init_obj_list() to init_sequence_r[] in a future patch
>> to avoid calling it it in many different places.
>>
>>> +	if (ret != EFI_SUCCESS) {
>>> +		log_err("Error: Cannot initialize UEFI sub-system, r = %lu\n",
>>> +			ret & ~EFI_ERROR_MASK);
>>> +
>>> +		return CMD_RET_FAILURE;
>>> +	}
>>> +
>>> +	ret = eficonfig_init();
>>> +	if (ret != EFI_SUCCESS)
>>> +		return CMD_RET_FAILURE;
>>> +
>>> +	while (1) {
>>> +		ret = eficonfig_process_common(maintenance_menu_items,
>>> +					       ARRAY_SIZE(maintenance_menu_items),
>>> +					     "  ** UEFI Maintenance Menu **");
>>> +		if (ret == EFI_ABORTED)
>>> +			break;
>>> +	}
>>> +
>>> +	return CMD_RET_SUCCESS;
>>> +}
>>> +
>>> +U_BOOT_CMD(
>>> +	eficonfig, 1, 0, do_eficonfig,
>>> +	"provide menu-driven UEFI variable maintenance interface",
>>> +	""
>>> +);
>>> diff --git a/include/efi_config.h b/include/efi_config.h
>>> new file mode 100644
>>> index 0000000000..1b48e47c48
>>> --- /dev/null
>>> +++ b/include/efi_config.h
>>> @@ -0,0 +1,91 @@
>>> +/* SPDX-License-Identifier: GPL-2.0+ */
>>> +/*
>>> + *  Menu-driven UEFI Variable maintenance
>>> + *
>>> + *  Copyright (c) 2022 Masahisa Kojima, Linaro Limited
>>> + */
>>> +
>>> +#ifndef _EFI_CONFIG_H
>>> +#define _EFI_CONFIG_H
>>> +
>>> +#define EFICONFIG_ENTRY_NUM_MAX 99
>>> +#define EFICONFIG_FILE_PATH_MAX 512
>>> +#define EFICONFIG_FILE_PATH_BUF_SIZE (EFICONFIG_FILE_PATH_MAX * sizeof(u16))
>>> +
>>> +typedef efi_status_t (*eficonfig_entry_func)(void *data);
>>> +
>>> +/**
>>> + * struct eficonfig_entry - menu entry structure
>>> + *
>>> + * @num:	menu entry index
>>> + * @title:	title of entry
>>> + * @key:	unique key
>>> + * @efi_menu:	pointer to the menu structure
>>> + * @next:	pointer to the next entry
>>> + * @func:	callback function to be called when this entry is selected
>>> + * @data:	data to be passed to the callback function
>>> + */
>>> +struct eficonfig_entry {
>>> +	u32 num;
>>> +	char *title;
>>> +	char key[3];
>>> +	struct efimenu *efi_menu;
>>> +	struct eficonfig_entry *next;
>>> +	eficonfig_entry_func func;
>>> +	void *data;
>>> +};
>>> +
>>> +/**
>>> + * struct efimenu - efi menu structure
>>> + *
>>> + * @delay:		delay for autoboot
>>> + * @active:		active menu entry index
>>> + * @count:		total count of menu entry
>>> + * @menu_header:	menu header string
>>> + * @first:		pointer to the first menu entry
>>> + */
>>> +struct efimenu {
>>> +	int delay;
>>> +	int active;
>>> +	int count;
>>> +	char *menu_header;
>>> +	struct eficonfig_entry *first;
>>> +};
>>> +
>>> +/**
>>> + * struct eficonfig_item - structure to construct eficonfig_entry
>>> + *
>>> + * @title:	title of entry
>>> + * @func:	callback function to be called when this entry is selected
>>> + * @data:	data to be passed to the callback function
>>> + */
>>> +struct eficonfig_item {
>>> +	char *title;
>>> +	eficonfig_entry_func func;
>>> +	void *data;
>>> +};
>>> +
>>> +/**
>>> + * struct eficonfig_select_file_info - structure to be used for file selection
>>> + *
>>> + * @current_volume:	pointer to the efi_simple_file_system_protocol
>>> + * @dp_volume:		pointer to device path of the selected device
>>> + * @current_path:	pointer to the selected file path string
>>> + * @file_selectred:	flag indicates file selecting status
>>> + * @filepath_list:	list_head structure for file path list
>>> + */
>>> +struct eficonfig_select_file_info {
>>> +	struct efi_simple_file_system_protocol *current_volume;
>>> +	struct efi_device_path *dp_volume;
>>> +	u16 *current_path;
>>> +	struct list_head filepath_list;
>>> +	bool file_selected;
>>> +};
>>> +
>>> +void eficonfig_print_msg(char *msg);
>>> +efi_status_t eficonfig_process_quit(void *data);
>>> +efi_status_t eficonfig_process_common(const struct eficonfig_item *items, int count,
>>> +				      char *menu_header);
>>> +efi_status_t eficonfig_select_file_handler(void *data);
>>> +
>>> +#endif
>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>> index c6df29993c..365ce9493e 100644
>>> --- a/include/efi_loader.h
>>> +++ b/include/efi_loader.h
>>> @@ -226,6 +226,9 @@ const char *__efi_nesting_dec(void);
>>>    #define EFI_CACHELINE_SIZE 128
>>>    #endif
>>>
>>> +/* max bootmenu title size for volume selection */
>>> +#define BOOTMENU_DEVICE_NAME_MAX 16
>>> +
>>>    /* Key identifying current memory map */
>>>    extern efi_uintn_t efi_memory_map_key;
>>>
>>> @@ -249,6 +252,9 @@ extern const struct efi_hii_string_protocol efi_hii_string;
>>>
>>>    uint16_t *efi_dp_str(struct efi_device_path *dp);
>>>
>>> +/* GUID for the auto generated boot menu entry */
>>> +extern const efi_guid_t efi_guid_bootmenu_auto_generated;
>>> +
>>>    /* GUID of the U-Boot root node */
>>>    extern const efi_guid_t efi_u_boot_guid;
>>>    #ifdef CONFIG_SANDBOX
>>> @@ -314,6 +320,9 @@ extern const efi_guid_t efi_guid_firmware_management_protocol;
>>>    extern const efi_guid_t efi_esrt_guid;
>>>    /* GUID of the SMBIOS table */
>>>    extern const efi_guid_t smbios_guid;
>>> +/*GUID of console */
>>> +extern const efi_guid_t efi_guid_text_input_protocol;
>>> +extern const efi_guid_t efi_guid_text_output_protocol;
>>>
>>>    extern char __efi_runtime_start[], __efi_runtime_stop[];
>>>    extern char __efi_runtime_rel_start[], __efi_runtime_rel_stop[];
>>> @@ -883,6 +892,8 @@ efi_status_t efi_set_load_options(efi_handle_t handle,
>>>    				  void *load_options);
>>>    efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options);
>>>
>>> +efi_status_t efi_bootmenu_show_maintenance_menu(void);
>>> +
>>>    /**
>>>     * struct efi_image_regions - A list of memory regions
>>>     *
>>> @@ -1054,4 +1065,33 @@ efi_status_t efi_esrt_populate(void);
>>>    efi_status_t efi_load_capsule_drivers(void);
>>>
>>>    efi_status_t platform_get_eventlog(struct udevice *dev, u64 *addr, u32 *sz);
>>> +
>>> +efi_status_t efi_locate_handle_buffer_int(enum efi_locate_search_type search_type,
>>> +					  const efi_guid_t *protocol, void *search_key,
>>> +					  efi_uintn_t *no_handles, efi_handle_t **buffer);
>>> +
>>> +efi_status_t efi_open_volume_int(struct efi_simple_file_system_protocol *this,
>>> +				 struct efi_file_handle **root);
>>> +efi_status_t efi_file_open_int(struct efi_file_handle *this,
>>> +			       struct efi_file_handle **new_handle,
>>> +			       u16 *file_name, u64 open_mode,
>>> +			       u64 attributes);
>>> +efi_status_t efi_file_close_int(struct efi_file_handle *file);
>>> +efi_status_t efi_file_read_int(struct efi_file_handle *this,
>>> +			       efi_uintn_t *buffer_size, void *buffer);
>>> +efi_status_t efi_file_setpos_int(struct efi_file_handle *file, u64 pos);
>>> +
>>> +typedef efi_status_t (*efi_console_filter_func)(struct efi_input_key *key);
>>> +efi_status_t efi_console_get_u16_string
>>> +		(struct efi_simple_text_input_protocol *cin,
>>> +		 struct efi_simple_text_output_protocol *cout,
>>> +		 u16 *buf, efi_uintn_t count, efi_console_filter_func filer_func,
>>> +		 int row, int col);
>>> +
>>> +efi_status_t eficonfig_get_unused_bootoption(u16 *buf,
>>> +					     efi_uintn_t buf_size, u32 *index);
>>> +efi_status_t eficonfig_append_bootorder(u16 index);
>>> +
>>> +efi_status_t efi_disk_get_device_name(struct efi_block_io *this, char *buf, int size);
>>> +
>>>    #endif /* _EFI_LOADER_H */
>>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>>> index 4da64b5d29..1233418e77 100644
>>> --- a/lib/efi_loader/efi_boottime.c
>>> +++ b/lib/efi_loader/efi_boottime.c
>>> @@ -2453,6 +2453,35 @@ static efi_status_t EFIAPI efi_protocols_per_handle(
>>>    	return EFI_EXIT(EFI_SUCCESS);
>>>    }
>>>
>>> +efi_status_t efi_locate_handle_buffer_int(enum efi_locate_search_type search_type,
>>> +					  const efi_guid_t *protocol, void *search_key,
>>> +					  efi_uintn_t *no_handles, efi_handle_t **buffer)
>>> +{
>>> +	efi_status_t r;
>>> +	efi_uintn_t buffer_size = 0;
>>> +
>>> +	if (!no_handles || !buffer) {
>>> +		r = EFI_INVALID_PARAMETER;
>>> +		goto out;
>>> +	}
>>> +	*no_handles = 0;
>>> +	*buffer = NULL;
>>> +	r = efi_locate_handle(search_type, protocol, search_key, &buffer_size,
>>> +			      *buffer);
>>> +	if (r != EFI_BUFFER_TOO_SMALL)
>>> +		goto out;
>>> +	r = efi_allocate_pool(EFI_BOOT_SERVICES_DATA, buffer_size,
>>> +			      (void **)buffer);
>>> +	if (r != EFI_SUCCESS)
>>> +		goto out;
>>> +	r = efi_locate_handle(search_type, protocol, search_key, &buffer_size,
>>> +			      *buffer);
>>> +	if (r == EFI_SUCCESS)
>>> +		*no_handles = buffer_size / sizeof(efi_handle_t);
>>> +out:
>>> +	return r;
>>> +}
>>> +
>>>    /**
>>>     * efi_locate_handle_buffer() - locate handles implementing a protocol
>>>     * @search_type: selection criterion
>>> @@ -2474,30 +2503,13 @@ efi_status_t EFIAPI efi_locate_handle_buffer(
>>>    			efi_uintn_t *no_handles, efi_handle_t **buffer)
>>>    {
>>>    	efi_status_t r;
>>> -	efi_uintn_t buffer_size = 0;
>>>
>>>    	EFI_ENTRY("%d, %pUs, %p, %p, %p", search_type, protocol, search_key,
>>>    		  no_handles, buffer);
>>>
>>> -	if (!no_handles || !buffer) {
>>> -		r = EFI_INVALID_PARAMETER;
>>> -		goto out;
>>> -	}
>>> -	*no_handles = 0;
>>> -	*buffer = NULL;
>>> -	r = efi_locate_handle(search_type, protocol, search_key, &buffer_size,
>>> -			      *buffer);
>>> -	if (r != EFI_BUFFER_TOO_SMALL)
>>> -		goto out;
>>> -	r = efi_allocate_pool(EFI_BOOT_SERVICES_DATA, buffer_size,
>>> -			      (void **)buffer);
>>> -	if (r != EFI_SUCCESS)
>>> -		goto out;
>>> -	r = efi_locate_handle(search_type, protocol, search_key, &buffer_size,
>>> -			      *buffer);
>>> -	if (r == EFI_SUCCESS)
>>> -		*no_handles = buffer_size / sizeof(efi_handle_t);
>>> -out:
>>> +	r = efi_locate_handle_buffer_int(search_type, protocol, search_key,
>>> +					 no_handles, buffer);
>>> +
>>>    	return EFI_EXIT(r);
>>>    }
>>>
>>> diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
>>> index 60a3fc85ac..ca8e38b8eb 100644
>>> --- a/lib/efi_loader/efi_console.c
>>> +++ b/lib/efi_loader/efi_console.c
>>> @@ -5,6 +5,7 @@
>>>     *  Copyright (c) 2016 Alexander Graf
>>>     */
>>>
>>> +#include <ansi.h>
>>>    #include <common.h>
>>>    #include <charset.h>
>>>    #include <malloc.h>
>>> @@ -1312,3 +1313,80 @@ out_of_memory:
>>>    	printf("ERROR: Out of memory\n");
>>>    	return r;
>>>    }
>>> +
>>> +/**
>>> + * efi_console_get_u16_string() - get user input string
>>> + *
>>> + * @cin:		protocol interface to EFI_SIMPLE_TEXT_INPUT_PROTOCOL
>>> + * @cout:		protocol interface to EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL
>>> + * @buf:		buffer to store user input string in UTF16
>>> + * @count:		number of u16 string including NULL terminator that buf has
>>> + * @filter_func:	callback to filter user input
>>> + * @row:		row number to locate user input form
>>> + * @col:		column number to locate user input form
>>> + * Return:		status code
>>> + */
>>> +efi_status_t efi_console_get_u16_string(struct efi_simple_text_input_protocol *cin,
>>> +					struct efi_simple_text_output_protocol *cout,
>>> +					u16 *buf, efi_uintn_t count,
>>> +					efi_console_filter_func filter_func,
>>> +					int row, int col)
>>> +{
>>> +	efi_status_t ret;
>>> +	efi_uintn_t len = 0;
>>> +	struct efi_input_key key;
>>> +
>>> +	printf(ANSI_CURSOR_POSITION, row, col);
>>> +	puts(ANSI_CLEAR_LINE_TO_END);
>>> +	puts(ANSI_CURSOR_SHOW);
>>
>> One printf() statement is enough and reduces code size:
>>
>> printf(ANSI_CURSOR_POSITION ANSI_CLEAR_LINE_TO_END ANSI_CURSOR_SHOW,
>>         row col);
>>
>>> +
>>> +	ret = EFI_CALL(cin->reset(cin, false));
>>> +	if (ret != EFI_SUCCESS)
>>> +		return ret;
>>> +
>>> +	for (;;) {
>>> +		do {
>>> +			ret = EFI_CALL(cin->read_key_stroke(cin, &key));
>>> +			mdelay(10);
>>> +		} while (ret == EFI_NOT_READY);
>>> +
>>> +		if (key.unicode_char == u'\b') {
>>> +			if (len > 0)
>>> +				buf[--len] = u'\0';
>>> +
>>> +			printf(ANSI_CURSOR_POSITION, row, col);
>>> +			ret = EFI_CALL(cout->output_string(cout, buf));
>>
>> Please, use %ls to print u16 strings:
>>
>> printf("ANSI_CURSOR_POSITION "%ls" ANSI_CLEAR_LINE_TO_END,
>>         row, col, buf);
>>
>>> +			if (ret != EFI_SUCCESS)
>>> +				return ret;
>>> +
>>> +			puts(ANSI_CLEAR_LINE_TO_END);
>>> +			continue;
>>> +		} else if (key.unicode_char == u'\r') {
>>> +			buf[len] = u'\0';
>>> +			return EFI_SUCCESS;
>>> +		} else if (key.unicode_char == 0x3 || key.scan_code == 23) {
>>> +			return EFI_ABORTED;
>>> +		} else if (key.unicode_char < 0x20) {
>>> +			/* ignore control codes other than Ctrl+C, '\r' and '\b' */
>>> +			continue;
>>> +		} else if (key.scan_code != 0) {
>>> +			/* only accept single ESC press for cancel */
>>> +			continue;
>>> +		}
>>> +
>>> +		if (filter_func) {
>>> +			if (filter_func(&key) != EFI_SUCCESS)
>>> +				continue;
>>> +		}
>>> +
>>> +		if (len >= (count - 1))
>>> +			continue;
>>> +
>>> +		buf[len] = key.unicode_char;
>>> +		len++;
>>> +		printf(ANSI_CURSOR_POSITION, row, col);
>>> +		ret = EFI_CALL(cout->output_string(cout, buf));
>>
>> Please use %ls:
>>
>> printf(ANSI_CURSOR_POSITION "%ls", row, col, buf);
>>
>>> +		if (ret != EFI_SUCCESS)
>>> +			return ret;
>>> +	}
>>> +}
>>> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
>>> index 1e82f52dc0..efc2f20ff0 100644
>>> --- a/lib/efi_loader/efi_disk.c
>>> +++ b/lib/efi_loader/efi_disk.c
>>> @@ -778,3 +778,14 @@ efi_status_t efi_disk_init(void)
>>>
>>>    	return EFI_SUCCESS;
>>>    }
>>> +
>>> +efi_status_t efi_disk_get_device_name(struct efi_block_io *this, char *buf, int size)
>>> +{
>>> +	struct efi_disk_obj *diskobj;
>>> +
>>> +	diskobj = container_of(this, struct efi_disk_obj, ops);
>>
>> This will fail if the EFI_BLOCK_IO_PROTOCOL is provided by a driver
>> loaded via the bootefi command.
>>
>> Handles can only be created by U-Boot in contrast to protocol
>> interfaces. What you need is the udevice as a field in struct efi_object
>> instead of struct efi_disk_obj. See Takahiro's comment in
>> lib/efi_loader/efi_disk.c:49:
>>
>> struct udevice *dev; /* TODO: move it to efi_object */
>>
>> Best regards
>>
>> Heinrich
>>
>>> +
>>> +	snprintf(buf, size, "%s %d:%d", diskobj->ifname, diskobj->dev_index, diskobj->part);
>>> +
>>> +	return EFI_SUCCESS;
>>> +}
>>> diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
>>> index 7a7077e6d0..c96a7f7ca3 100644
>>> --- a/lib/efi_loader/efi_file.c
>>> +++ b/lib/efi_loader/efi_file.c
>>> @@ -246,10 +246,10 @@ error:
>>>    	return NULL;
>>>    }
>>>
>>> -static efi_status_t efi_file_open_int(struct efi_file_handle *this,
>>> -				      struct efi_file_handle **new_handle,
>>> -				      u16 *file_name, u64 open_mode,
>>> -				      u64 attributes)
>>> +efi_status_t efi_file_open_int(struct efi_file_handle *this,
>>> +			       struct efi_file_handle **new_handle,
>>> +			       u16 *file_name, u64 open_mode,
>>> +			       u64 attributes)
>>>    {
>>>    	struct file_handle *fh = to_fh(this);
>>>    	efi_status_t ret;
>>> @@ -369,11 +369,17 @@ static efi_status_t file_close(struct file_handle *fh)
>>>    	return EFI_SUCCESS;
>>>    }
>>>
>>> -static efi_status_t EFIAPI efi_file_close(struct efi_file_handle *file)
>>> +efi_status_t efi_file_close_int(struct efi_file_handle *file)
>>>    {
>>>    	struct file_handle *fh = to_fh(file);
>>> +
>>> +	return file_close(fh);
>>> +}
>>> +
>>> +static efi_status_t EFIAPI efi_file_close(struct efi_file_handle *file)
>>> +{
>>>    	EFI_ENTRY("%p", file);
>>> -	return EFI_EXIT(file_close(fh));
>>> +	return EFI_EXIT(efi_file_close_int(file));
>>>    }
>>>
>>>    static efi_status_t EFIAPI efi_file_delete(struct efi_file_handle *file)
>>> @@ -562,8 +568,8 @@ static efi_status_t dir_read(struct file_handle *fh, u64 *buffer_size,
>>>    	return EFI_SUCCESS;
>>>    }
>>>
>>> -static efi_status_t efi_file_read_int(struct efi_file_handle *this,
>>> -				      efi_uintn_t *buffer_size, void *buffer)
>>> +efi_status_t efi_file_read_int(struct efi_file_handle *this,
>>> +			       efi_uintn_t *buffer_size, void *buffer)
>>>    {
>>>    	struct file_handle *fh = to_fh(this);
>>>    	efi_status_t ret = EFI_SUCCESS;
>>> @@ -773,24 +779,11 @@ out:
>>>    	return EFI_EXIT(ret);
>>>    }
>>>
>>> -/**
>>> - * efi_file_setpos() - set current position in file
>>> - *
>>> - * This function implements the SetPosition service of the EFI file protocol.
>>> - * See the UEFI spec for details.
>>> - *
>>> - * @file:	file handle
>>> - * @pos:	new file position
>>> - * Return:	status code
>>> - */
>>> -static efi_status_t EFIAPI efi_file_setpos(struct efi_file_handle *file,
>>> -					   u64 pos)
>>> +efi_status_t efi_file_setpos_int(struct efi_file_handle *file, u64 pos)
>>>    {
>>>    	struct file_handle *fh = to_fh(file);
>>>    	efi_status_t ret = EFI_SUCCESS;
>>>
>>> -	EFI_ENTRY("%p, %llu", file, pos);
>>> -
>>>    	if (fh->isdir) {
>>>    		if (pos != 0) {
>>>    			ret = EFI_UNSUPPORTED;
>>> @@ -812,6 +805,28 @@ static efi_status_t EFIAPI efi_file_setpos(struct efi_file_handle *file,
>>>    	fh->offset = pos;
>>>
>>>    error:
>>> +	return ret;
>>> +}
>>> +
>>> +/**
>>> + * efi_file_setpos() - set current position in file
>>> + *
>>> + * This function implements the SetPosition service of the EFI file protocol.
>>> + * See the UEFI spec for details.
>>> + *
>>> + * @file:	file handle
>>> + * @pos:	new file position
>>> + * Return:	status code
>>> + */
>>> +static efi_status_t EFIAPI efi_file_setpos(struct efi_file_handle *file,
>>> +					   u64 pos)
>>> +{
>>> +	efi_status_t ret = EFI_SUCCESS;
>>> +
>>> +	EFI_ENTRY("%p, %llu", file, pos);
>>> +
>>> +	ret = efi_file_setpos_int(file, pos);
>>> +
>>>    	return EFI_EXIT(ret);
>>>    }
>>>
>>> @@ -1138,17 +1153,23 @@ struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp)
>>>    	return f;
>>>    }
>>>
>>> +efi_status_t efi_open_volume_int(struct efi_simple_file_system_protocol *this,
>>> +				 struct efi_file_handle **root)
>>> +{
>>> +	struct file_system *fs = to_fs(this);
>>> +
>>> +	*root = file_open(fs, NULL, NULL, 0, 0);
>>> +
>>> +	return EFI_SUCCESS;
>>> +}
>>> +
>>>    static efi_status_t EFIAPI
>>>    efi_open_volume(struct efi_simple_file_system_protocol *this,
>>>    		struct efi_file_handle **root)
>>>    {
>>> -	struct file_system *fs = to_fs(this);
>>> -
>>>    	EFI_ENTRY("%p, %p", this, root);
>>>
>>> -	*root = file_open(fs, NULL, NULL, 0, 0);
>>> -
>>> -	return EFI_EXIT(EFI_SUCCESS);
>>> +	return EFI_EXIT(efi_open_volume_int(this, root));
>>>    }
>>>
>>>    struct efi_simple_file_system_protocol *
>>



More information about the U-Boot mailing list