All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 1/2] Boot var automatic management for removable medias
@ 2023-04-05  0:06 Raymond Mao
  2023-04-05  0:06 ` [PATCH v2 2/2] Load option with short device path for boot vars Raymond Mao
  2023-04-07 10:13 ` [PATCH v2 1/2] Boot var automatic management for removable medias Heinrich Schuchardt
  0 siblings, 2 replies; 6+ messages in thread
From: Raymond Mao @ 2023-04-05  0:06 UTC (permalink / raw)
  To: u-boot
  Cc: Raymond Mao, Heinrich Schuchardt, Ilias Apalodimas,
	Masahisa Kojima, Simon Glass

Changes for complying to EFI spec §3.5.1.1
'Removable Media Boot Behavior'.
Boot variables can be automatically generated during a removable
media is probed. At the same time, unused boot variables will be
detected and removed.
Related APIs are renamed and moved from cmd to lib for re-use
between eficonfig and bootmgr.

Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
---
Changes in v2
- Ignore EFI_NOT_FOUND returned from
  efi_bootmgr_update_media_device_boot_option which means no boot
  options scanned.

 cmd/bootmenu.c                    |   2 +-
 cmd/eficonfig.c                   | 408 +-----------------------------
 include/efi_config.h              |   5 -
 include/efi_loader.h              |  11 +
 lib/efi_loader/efi_bootmgr.c      | 380 ++++++++++++++++++++++++++++
 lib/efi_loader/efi_disk.c         |   7 +
 lib/efi_loader/efi_helper.c       |  25 ++
 lib/efi_loader/efi_variable.c     |   6 +-
 lib/efi_loader/efi_variable_tee.c |   3 +-
 9 files changed, 437 insertions(+), 410 deletions(-)

diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
index 6baeedc69f..01daddca7b 100644
--- a/cmd/bootmenu.c
+++ b/cmd/bootmenu.c
@@ -351,7 +351,7 @@ static struct bootmenu_data *bootmenu_create(int delay)
 		 * UEFI specification requires booting from removal media using
 		 * a architecture-specific default image name such as BOOTAA64.EFI.
 		 */
-		efi_ret = eficonfig_generate_media_device_boot_option();
+		efi_ret = efi_bootmgr_update_media_device_boot_option();
 		if (efi_ret != EFI_SUCCESS && efi_ret != EFI_NOT_FOUND)
 			goto cleanup;
 
diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
index 720f52b48b..82a80306f4 100644
--- a/cmd/eficonfig.c
+++ b/cmd/eficonfig.c
@@ -1134,43 +1134,6 @@ out:
 	return ret;
 }
 
-/**
- * eficonfig_get_unused_bootoption() - get unused "Boot####" index
- *
- * @buf:	pointer to the buffer to store boot option variable name
- * @buf_size:	buffer size
- * @index:	pointer to store the index in the BootOrder variable
- * Return:	status code
- */
-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;
-}
-
 /**
  * eficonfig_set_boot_option() - set boot option
  *
@@ -1208,46 +1171,6 @@ static efi_status_t eficonfig_set_boot_option(u16 *varname, struct efi_device_pa
 	return ret;
 }
 
-/**
- * eficonfig_append_bootorder() - append new boot option in BootOrder variable
- *
- * @index:	"Boot####" index to append to BootOrder variable
- * Return:	status code
- */
-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;
-}
-
 /**
  * create_boot_option_entry() - create boot option entry
  *
@@ -1619,7 +1542,7 @@ static efi_status_t eficonfig_process_add_boot_option(void *data)
 	if (!bo)
 		return EFI_OUT_OF_RESOURCES;
 
-	ret = eficonfig_get_unused_bootoption(varname, sizeof(varname), &bo->boot_index);
+	ret = efi_bootmgr_get_unused_bootoption(varname, sizeof(varname), &bo->boot_index);
 	if (ret != EFI_SUCCESS)
 		return ret;
 
@@ -1627,7 +1550,7 @@ static efi_status_t eficonfig_process_add_boot_option(void *data)
 	if (ret != EFI_SUCCESS)
 		goto out;
 
-	ret = eficonfig_append_bootorder((u16)bo->boot_index);
+	ret = efi_bootmgr_append_bootorder((u16)bo->boot_index);
 	if (ret != EFI_SUCCESS)
 		goto out;
 
@@ -1656,31 +1579,6 @@ static efi_status_t eficonfig_process_boot_selected(void *data)
 	return EFI_SUCCESS;
 }
 
-/**
- * search_bootorder() - search the boot option index in BootOrder
- *
- * @bootorder:	pointer to the BootOrder variable
- * @num:	number of BootOrder entry
- * @target:	target boot option index to search
- * @index:	pointer to store the index of BootOrder variable
- * Return:	true if exists, false otherwise
- */
-static bool search_bootorder(u16 *bootorder, efi_uintn_t num, u32 target, u32 *index)
-{
-	u32 i;
-
-	for (i = 0; i < num; i++) {
-		if (target == bootorder[i]) {
-			if (index)
-				*index = i;
-
-			return true;
-		}
-	}
-
-	return false;
-}
-
 /**
  * eficonfig_add_boot_selection_entry() - add boot option menu entry
  *
@@ -1805,7 +1703,7 @@ static efi_status_t eficonfig_show_boot_selection(unsigned int *selected)
 
 		if (efi_varname_is_load_option(var_name16, &index)) {
 			/* If the index is included in the BootOrder, skip it */
-			if (search_bootorder(bootorder, num, index, NULL))
+			if (efi_search_bootorder(bootorder, num, index, NULL))
 				continue;
 
 			ret = eficonfig_add_boot_selection_entry(efi_menu, index, selected);
@@ -2202,7 +2100,7 @@ static efi_status_t eficonfig_create_change_boot_order_entry(struct efimenu *efi
 
 		if (efi_varname_is_load_option(var_name16, &index)) {
 			/* If the index is included in the BootOrder, skip it */
-			if (search_bootorder(bootorder, num, index, NULL))
+			if (efi_search_bootorder(bootorder, num, index, NULL))
 				continue;
 
 			ret = eficonfig_add_change_boot_order_entry(efi_menu, index, false);
@@ -2304,50 +2202,6 @@ out:
 	return ret;
 }
 
-/**
- * delete_boot_option() - delete selected boot option
- *
- * @boot_index:	boot option index to delete
- * Return:	status code
- */
-static efi_status_t delete_boot_option(u16 boot_index)
-{
-	u16 *bootorder;
-	u16 varname[9];
-	efi_status_t ret;
-	unsigned int index;
-	efi_uintn_t num, size;
-
-	efi_create_indexed_name(varname, sizeof(varname),
-				"Boot", boot_index);
-	ret = efi_set_variable_int(varname, &efi_global_variable_guid,
-				   0, 0, NULL, false);
-	if (ret != EFI_SUCCESS) {
-		log_err("delete boot option(%ls) failed\n", varname);
-		return ret;
-	}
-
-	/* update BootOrder if necessary */
-	bootorder = efi_get_var(u"BootOrder", &efi_global_variable_guid, &size);
-	if (!bootorder)
-		return EFI_SUCCESS;
-
-	num = size / sizeof(u16);
-	if (!search_bootorder(bootorder, num, boot_index, &index))
-		return EFI_SUCCESS;
-
-	memmove(&bootorder[index], &bootorder[index + 1],
-		(num - index - 1) * sizeof(u16));
-	size -= sizeof(u16);
-	ret = efi_set_variable_int(u"BootOrder", &efi_global_variable_guid,
-				   EFI_VARIABLE_NON_VOLATILE |
-				   EFI_VARIABLE_BOOTSERVICE_ACCESS |
-				   EFI_VARIABLE_RUNTIME_ACCESS,
-				   size, bootorder, false);
-
-	return ret;
-}
-
 /**
  * eficonfig_process_delete_boot_option() - handler to delete boot option
  *
@@ -2362,7 +2216,7 @@ static efi_status_t eficonfig_process_delete_boot_option(void *data)
 	while (1) {
 		ret = eficonfig_show_boot_selection(&selected);
 		if (ret == EFI_SUCCESS)
-			ret = delete_boot_option(selected);
+			ret = efi_bootmgr_delete_boot_option(selected);
 
 		if (ret != EFI_SUCCESS)
 			break;
@@ -2374,256 +2228,6 @@ static efi_status_t eficonfig_process_delete_boot_option(void *data)
 	return ret;
 }
 
-/**
- * eficonfig_enumerate_boot_option() - enumerate the possible bootable media
- *
- * @opt:		pointer to the media boot option structure
- * @volume_handles:	pointer to the efi handles
- * @count:		number of efi handle
- * Return:		status code
- */
-efi_status_t eficonfig_enumerate_boot_option(struct eficonfig_media_boot_option *opt,
-					     efi_handle_t *volume_handles, efi_status_t count)
-{
-	u32 i;
-	struct efi_handler *handler;
-	efi_status_t ret = EFI_SUCCESS;
-
-	for (i = 0; i < count; i++) {
-		u16 *p;
-		u16 dev_name[BOOTMENU_DEVICE_NAME_MAX];
-		char *optional_data;
-		struct efi_load_option lo;
-		char buf[BOOTMENU_DEVICE_NAME_MAX];
-		struct efi_device_path *device_path;
-
-		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_disk_get_device_name(volume_handles[i], buf, BOOTMENU_DEVICE_NAME_MAX);
-		if (ret != EFI_SUCCESS)
-			continue;
-
-		p = dev_name;
-		utf8_utf16_strncpy(&p, buf, strlen(buf));
-
-		lo.label = dev_name;
-		lo.attributes = LOAD_OPTION_ACTIVE;
-		lo.file_path = device_path;
-		lo.file_path_length = efi_dp_size(device_path) + sizeof(END);
-		/*
-		 * Set the dedicated guid to optional_data, it is used to identify
-		 * the boot option that automatically generated by the bootmenu.
-		 * efi_serialize_load_option() expects optional_data is null-terminated
-		 * utf8 string, so set the "1234567" string to allocate enough space
-		 * to store guid, instead of realloc the load_option.
-		 */
-		lo.optional_data = "1234567";
-		opt[i].size = efi_serialize_load_option(&lo, (u8 **)&opt[i].lo);
-		if (!opt[i].size) {
-			ret = EFI_OUT_OF_RESOURCES;
-			goto out;
-		}
-		/* set the guid */
-		optional_data = (char *)opt[i].lo + (opt[i].size - u16_strsize(u"1234567"));
-		memcpy(optional_data, &efi_guid_bootmenu_auto_generated, sizeof(efi_guid_t));
-	}
-
-out:
-	return ret;
-}
-
-/**
- * eficonfig_delete_invalid_boot_option() - delete non-existing boot option
- *
- * @opt:		pointer to the media boot option structure
- * @count:		number of media boot option structure
- * Return:		status code
- */
-efi_status_t eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_option *opt,
-						  efi_status_t count)
-{
-	efi_uintn_t size;
-	void *load_option;
-	u32 i, list_size = 0;
-	struct efi_load_option lo;
-	u16 *var_name16 = NULL;
-	u16 varname[] = u"Boot####";
-	efi_status_t ret = EFI_SUCCESS;
-	u16 *delete_index_list = NULL, *p;
-	efi_uintn_t buf_size;
-
-	buf_size = 128;
-	var_name16 = malloc(buf_size);
-	if (!var_name16)
-		return EFI_OUT_OF_RESOURCES;
-
-	var_name16[0] = 0;
-	for (;;) {
-		int index;
-		efi_guid_t guid;
-		efi_uintn_t tmp;
-
-		ret = efi_next_variable_name(&buf_size, &var_name16, &guid);
-		if (ret == EFI_NOT_FOUND) {
-			/*
-			 * EFI_NOT_FOUND indicates we retrieved all EFI variables.
-			 * This should be treated as success.
-			 */
-			ret = EFI_SUCCESS;
-			break;
-		}
-		if (ret != EFI_SUCCESS)
-			goto out;
-
-		if (!efi_varname_is_load_option(var_name16, &index))
-			continue;
-
-		efi_create_indexed_name(varname, sizeof(varname), "Boot", index);
-		load_option = efi_get_var(varname, &efi_global_variable_guid, &size);
-		if (!load_option)
-			continue;
-
-		tmp = size;
-		ret = efi_deserialize_load_option(&lo, load_option, &size);
-		if (ret != EFI_SUCCESS)
-			goto next;
-
-		if (size >= sizeof(efi_guid_bootmenu_auto_generated) &&
-		    !guidcmp(lo.optional_data, &efi_guid_bootmenu_auto_generated)) {
-			for (i = 0; i < count; i++) {
-				if (opt[i].size == tmp &&
-				    memcmp(opt[i].lo, load_option, tmp) == 0) {
-					opt[i].exist = true;
-					break;
-				}
-			}
-
-			/*
-			 * The entire list of variables must be retrieved by
-			 * efi_get_next_variable_name_int() before deleting the invalid
-			 * boot option, just save the index here.
-			 */
-			if (i == count) {
-				p = realloc(delete_index_list, sizeof(u32) *
-					    (list_size + 1));
-				if (!p) {
-					ret = EFI_OUT_OF_RESOURCES;
-					goto out;
-				}
-				delete_index_list = p;
-				delete_index_list[list_size++] = index;
-			}
-		}
-next:
-		free(load_option);
-	}
-
-	/* delete all invalid boot options */
-	for (i = 0; i < list_size; i++) {
-		ret = delete_boot_option(delete_index_list[i]);
-		if (ret != EFI_SUCCESS)
-			goto out;
-	}
-
-out:
-	free(var_name16);
-	free(delete_index_list);
-
-	return ret;
-}
-
-/**
- * eficonfig_generate_media_device_boot_option() - generate the media device boot option
- *
- * This function enumerates all devices supporting EFI_SIMPLE_FILE_SYSTEM_PROTOCOL
- * and generate the bootmenu entries.
- * This function also provide the BOOT#### variable maintenance for
- * the media device entries.
- *   - Automatically create the BOOT#### variable for the newly detected device,
- *     this BOOT#### variable is distinguished by the special GUID
- *     stored in the EFI_LOAD_OPTION.optional_data
- *   - If the device is not attached to the system, the associated BOOT#### variable
- *     is automatically deleted.
- *
- * Return:	status code
- */
-efi_status_t eficonfig_generate_media_device_boot_option(void)
-{
-	u32 i;
-	efi_status_t ret;
-	efi_uintn_t count;
-	efi_handle_t *volume_handles = NULL;
-	struct eficonfig_media_boot_option *opt = NULL;
-
-	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)
-		return ret;
-
-	opt = calloc(count, sizeof(struct eficonfig_media_boot_option));
-	if (!opt)
-		goto out;
-
-	/* enumerate all devices supporting EFI_SIMPLE_FILE_SYSTEM_PROTOCOL */
-	ret = eficonfig_enumerate_boot_option(opt, volume_handles, count);
-	if (ret != EFI_SUCCESS)
-		goto out;
-
-	/*
-	 * System hardware configuration may vary depending on the user setup.
-	 * The boot option is automatically added by the bootmenu.
-	 * If the device is not attached to the system, the boot option needs
-	 * to be deleted.
-	 */
-	ret = eficonfig_delete_invalid_boot_option(opt, count);
-	if (ret != EFI_SUCCESS)
-		goto out;
-
-	/* add non-existent boot option */
-	for (i = 0; i < count; i++) {
-		u32 boot_index;
-		u16 var_name[9];
-
-		if (!opt[i].exist) {
-			ret = eficonfig_get_unused_bootoption(var_name, sizeof(var_name),
-							      &boot_index);
-			if (ret != EFI_SUCCESS)
-				goto out;
-
-			ret = efi_set_variable_int(var_name, &efi_global_variable_guid,
-						   EFI_VARIABLE_NON_VOLATILE |
-						   EFI_VARIABLE_BOOTSERVICE_ACCESS |
-						   EFI_VARIABLE_RUNTIME_ACCESS,
-						   opt[i].size, opt[i].lo, false);
-			if (ret != EFI_SUCCESS)
-				goto out;
-
-			ret = eficonfig_append_bootorder(boot_index);
-			if (ret != EFI_SUCCESS) {
-				efi_set_variable_int(var_name, &efi_global_variable_guid,
-						     0, 0, NULL, false);
-				goto out;
-			}
-		}
-	}
-
-out:
-	if (opt) {
-		for (i = 0; i < count; i++)
-			free(opt[i].lo);
-	}
-	free(opt);
-	efi_free_pool(volume_handles);
-
-	return ret;
-}
-
 /**
  * eficonfig_init() - do required initialization for eficonfig command
  *
@@ -2709,7 +2313,7 @@ static int do_eficonfig(struct cmd_tbl *cmdtp, int flag, int argc, char *const a
 	if (ret != EFI_SUCCESS)
 		return CMD_RET_FAILURE;
 
-	ret = eficonfig_generate_media_device_boot_option();
+	ret = efi_bootmgr_update_media_device_boot_option();
 	if (ret != EFI_SUCCESS && ret != EFI_NOT_FOUND)
 		return ret;
 
diff --git a/include/efi_config.h b/include/efi_config.h
index 01ce9b2b06..d7c1601137 100644
--- a/include/efi_config.h
+++ b/include/efi_config.h
@@ -105,11 +105,6 @@ efi_status_t eficonfig_process_common(struct efimenu *efi_menu,
 				      void (*item_data_print)(void *),
 				      char *(*item_choice)(void *));
 efi_status_t eficonfig_process_select_file(void *data);
-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 eficonfig_generate_media_device_boot_option(void);
-
 efi_status_t eficonfig_append_menu_entry(struct efimenu *efi_menu,
 					 char *title, eficonfig_entry_func func,
 					 void *data);
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 1542b4b625..31ca1f5d1d 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -516,6 +516,17 @@ extern struct list_head efi_register_notify_events;
 int efi_init_early(void);
 /* Initialize efi execution environment */
 efi_status_t efi_init_obj_list(void);
+/* Append new boot option in BootOrder variable */
+efi_status_t efi_bootmgr_append_bootorder(u16 index);
+/* Get unused "Boot####" index */
+efi_status_t efi_bootmgr_get_unused_bootoption(u16 *buf,
+					       efi_uintn_t buf_size, u32 *index);
+/* Generate the media device boot option */
+efi_status_t efi_bootmgr_update_media_device_boot_option(void);
+/* Delete selected boot option */
+efi_status_t efi_bootmgr_delete_boot_option(u16 boot_index);
+/* search the boot option index in BootOrder */
+bool efi_search_bootorder(u16 *bootorder, efi_uintn_t num, u32 target, u32 *index);
 /* Set up console modes */
 void efi_setup_console_size(void);
 /* Install device tree */
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index 4b24b41047..c329428973 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -347,3 +347,383 @@ efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options)
 error:
 	return ret;
 }
+
+/**
+ * efi_bootmgr_enumerate_boot_option() - enumerate the possible bootable media
+ *
+ * @opt:		pointer to the media boot option structure
+ * @volume_handles:	pointer to the efi handles
+ * @count:		number of efi handle
+ * Return:		status code
+ */
+static efi_status_t efi_bootmgr_enumerate_boot_option(struct eficonfig_media_boot_option *opt,
+						      efi_handle_t *volume_handles,
+						      efi_status_t count)
+{
+	u32 i;
+	struct efi_handler *handler;
+	efi_status_t ret = EFI_SUCCESS;
+
+	for (i = 0; i < count; i++) {
+		u16 *p;
+		u16 dev_name[BOOTMENU_DEVICE_NAME_MAX];
+		char *optional_data;
+		struct efi_load_option lo;
+		char buf[BOOTMENU_DEVICE_NAME_MAX];
+		struct efi_device_path *device_path;
+
+		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_disk_get_device_name(volume_handles[i], buf, BOOTMENU_DEVICE_NAME_MAX);
+		if (ret != EFI_SUCCESS)
+			continue;
+
+		p = dev_name;
+		utf8_utf16_strncpy(&p, buf, strlen(buf));
+
+		lo.label = dev_name;
+		lo.attributes = LOAD_OPTION_ACTIVE;
+		lo.file_path = device_path;
+		lo.file_path_length = efi_dp_size(device_path) + sizeof(END);
+		/*
+		 * Set the dedicated guid to optional_data, it is used to identify
+		 * the boot option that automatically generated by the bootmenu.
+		 * efi_serialize_load_option() expects optional_data is null-terminated
+		 * utf8 string, so set the "1234567" string to allocate enough space
+		 * to store guid, instead of realloc the load_option.
+		 */
+		lo.optional_data = "1234567";
+		opt[i].size = efi_serialize_load_option(&lo, (u8 **)&opt[i].lo);
+		if (!opt[i].size) {
+			ret = EFI_OUT_OF_RESOURCES;
+			goto out;
+		}
+		/* set the guid */
+		optional_data = (char *)opt[i].lo + (opt[i].size - u16_strsize(u"1234567"));
+		memcpy(optional_data, &efi_guid_bootmenu_auto_generated, sizeof(efi_guid_t));
+	}
+
+out:
+	return ret;
+}
+
+/**
+ * efi_bootmgr_delete_invalid_boot_option() - delete non-existing boot option
+ *
+ * @opt:		pointer to the media boot option structure
+ * @count:		number of media boot option structure
+ * Return:		status code
+ */
+static efi_status_t efi_bootmgr_delete_invalid_boot_option(struct eficonfig_media_boot_option *opt,
+							   efi_status_t count)
+{
+	efi_uintn_t size;
+	void *load_option;
+	u32 i, list_size = 0;
+	struct efi_load_option lo;
+	u16 *var_name16 = NULL;
+	u16 varname[] = u"Boot####";
+	efi_status_t ret = EFI_SUCCESS;
+	u16 *delete_index_list = NULL, *p;
+	efi_uintn_t buf_size;
+
+	buf_size = 128;
+	var_name16 = malloc(buf_size);
+	if (!var_name16)
+		return EFI_OUT_OF_RESOURCES;
+
+	var_name16[0] = 0;
+	for (;;) {
+		int index;
+		efi_guid_t guid;
+		efi_uintn_t tmp;
+
+		ret = efi_next_variable_name(&buf_size, &var_name16, &guid);
+		if (ret == EFI_NOT_FOUND) {
+			log_debug("last efi var\n");
+			/*
+			 * EFI_NOT_FOUND indicates we retrieved all EFI variables.
+			 * This should be treated as success.
+			 */
+			ret = EFI_SUCCESS;
+			break;
+		}
+
+		if (ret != EFI_SUCCESS)
+			goto out;
+
+		if (!efi_varname_is_load_option(var_name16, &index))
+			continue;
+
+		efi_create_indexed_name(varname, sizeof(varname), "Boot", index);
+		load_option = efi_get_var(varname, &efi_global_variable_guid, &size);
+		if (!load_option)
+			continue;
+
+		tmp = size;
+		ret = efi_deserialize_load_option(&lo, load_option, &size);
+		if (ret != EFI_SUCCESS)
+			goto next;
+
+		if (size >= sizeof(efi_guid_bootmenu_auto_generated) &&
+		    !guidcmp(lo.optional_data, &efi_guid_bootmenu_auto_generated)) {
+			for (i = 0; i < count; i++) {
+				if (opt[i].size == tmp &&
+				    memcmp(opt[i].lo, load_option, tmp) == 0) {
+					opt[i].exist = true;
+					break;
+				}
+			}
+
+			/*
+			 * The entire list of variables must be retrieved by
+			 * efi_get_next_variable_name_int() before deleting the invalid
+			 * boot option, just save the index here.
+			 */
+			if (i == count) {
+				log_debug("found index %d can be deleted\n", index);
+				p = realloc(delete_index_list, sizeof(u32) *
+					    (list_size + 1));
+				if (!p) {
+					ret = EFI_OUT_OF_RESOURCES;
+					goto out;
+				}
+				delete_index_list = p;
+				delete_index_list[list_size++] = index;
+			}
+		}
+next:
+		free(load_option);
+	}
+
+	/* delete all invalid boot options */
+	for (i = 0; i < list_size; i++) {
+		log_debug("deleting index %d\n", delete_index_list[i]);
+		ret = efi_bootmgr_delete_boot_option(delete_index_list[i]);
+		if (ret != EFI_SUCCESS)
+			goto out;
+	}
+
+out:
+	free(var_name16);
+	free(delete_index_list);
+
+	return ret;
+}
+
+/**
+ * efi_bootmgr_get_unused_bootoption() - get unused "Boot####" index
+ *
+ * @buf:	pointer to the buffer to store boot option variable name
+ * @buf_size:	buffer size
+ * @index:	pointer to store the index in the BootOrder variable
+ * Return:	status code
+ */
+efi_status_t efi_bootmgr_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;
+}
+
+/**
+ * efi_bootmgr_append_bootorder() - append new boot option in BootOrder variable
+ *
+ * @index:	"Boot####" index to append to BootOrder variable
+ * Return:	status code
+ */
+efi_status_t efi_bootmgr_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;
+}
+
+/**
+ * efi_bootmgr_delete_boot_option() - delete selected boot option
+ *
+ * @boot_index:	boot option index to delete
+ * Return:	status code
+ */
+efi_status_t efi_bootmgr_delete_boot_option(u16 boot_index)
+{
+	u16 *bootorder;
+	u16 varname[9];
+	efi_status_t ret;
+	unsigned int index;
+	efi_uintn_t num, size;
+
+	efi_create_indexed_name(varname, sizeof(varname),
+				"Boot", boot_index);
+	ret = efi_set_variable_int(varname, &efi_global_variable_guid,
+				   0, 0, NULL, false);
+	if (ret != EFI_SUCCESS) {
+		log_err("delete boot option(%ls) failed\n", varname);
+		return ret;
+	}
+
+	/* update BootOrder if necessary */
+	bootorder = efi_get_var(u"BootOrder", &efi_global_variable_guid, &size);
+	if (!bootorder)
+		return EFI_SUCCESS;
+
+	num = size / sizeof(u16);
+	if (!efi_search_bootorder(bootorder, num, boot_index, &index))
+		return EFI_SUCCESS;
+
+	memmove(&bootorder[index], &bootorder[index + 1],
+		(num - index - 1) * sizeof(u16));
+	size -= sizeof(u16);
+	ret = efi_set_variable_int(u"BootOrder", &efi_global_variable_guid,
+				   EFI_VARIABLE_NON_VOLATILE |
+				   EFI_VARIABLE_BOOTSERVICE_ACCESS |
+				   EFI_VARIABLE_RUNTIME_ACCESS,
+				   size, bootorder, false);
+
+	return ret;
+}
+
+/**
+ * efi_bootmgr_update_media_device_boot_option() - generate the media device boot option
+ *
+ * This function enumerates all devices supporting EFI_SIMPLE_FILE_SYSTEM_PROTOCOL
+ * and generate the bootmenu entries.
+ * This function also provide the BOOT#### variable maintenance for
+ * the media device entries.
+ *   - Automatically create the BOOT#### variable for the newly detected device,
+ *     this BOOT#### variable is distinguished by the special GUID
+ *     stored in the EFI_LOAD_OPTION.optional_data
+ *   - If the device is not attached to the system, the associated BOOT#### variable
+ *     is automatically deleted.
+ *
+ * Return:	status code
+ */
+efi_status_t efi_bootmgr_update_media_device_boot_option(void)
+{
+	u32 i;
+	efi_status_t ret;
+	efi_uintn_t count;
+	efi_handle_t *volume_handles = NULL;
+	struct eficonfig_media_boot_option *opt = NULL;
+
+	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)
+		return ret;
+
+	opt = calloc(count, sizeof(struct eficonfig_media_boot_option));
+	if (!opt)
+		goto out;
+
+	/* enumerate all devices supporting EFI_SIMPLE_FILE_SYSTEM_PROTOCOL */
+	ret = efi_bootmgr_enumerate_boot_option(opt, volume_handles, count);
+	if (ret != EFI_SUCCESS)
+		goto out;
+
+	/*
+	 * System hardware configuration may vary depending on the user setup.
+	 * The boot option is automatically added by the bootmenu.
+	 * If the device is not attached to the system, the boot option needs
+	 * to be deleted.
+	 */
+	ret = efi_bootmgr_delete_invalid_boot_option(opt, count);
+	if (ret != EFI_SUCCESS)
+		goto out;
+
+	/* add non-existent boot option */
+	for (i = 0; i < count; i++) {
+		u32 boot_index;
+		u16 var_name[9];
+
+		if (!opt[i].exist) {
+			ret = efi_bootmgr_get_unused_bootoption(var_name, sizeof(var_name),
+								&boot_index);
+			if (ret != EFI_SUCCESS)
+				goto out;
+
+			ret = efi_set_variable_int(var_name, &efi_global_variable_guid,
+						   EFI_VARIABLE_NON_VOLATILE |
+						   EFI_VARIABLE_BOOTSERVICE_ACCESS |
+						   EFI_VARIABLE_RUNTIME_ACCESS,
+						   opt[i].size, opt[i].lo, false);
+			if (ret != EFI_SUCCESS)
+				goto out;
+
+			ret = efi_bootmgr_append_bootorder(boot_index);
+			if (ret != EFI_SUCCESS) {
+				efi_set_variable_int(var_name, &efi_global_variable_guid,
+						     0, 0, NULL, false);
+				goto out;
+			}
+		}
+	}
+
+out:
+	if (opt) {
+		for (i = 0; i < count; i++) {
+			if (opt[i].lo)
+				free(opt[i].lo);
+		}
+		free(opt);
+	}
+	efi_free_pool(volume_handles);
+
+	return ret;
+}
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
index d2256713a8..1309e28134 100644
--- a/lib/efi_loader/efi_disk.c
+++ b/lib/efi_loader/efi_disk.c
@@ -687,6 +687,13 @@ int efi_disk_probe(void *ctx, struct event *event)
 			return -1;
 	}
 
+	/* only do the boot option management when UEFI sub-system is initialized */
+	if (efi_obj_list_initialized == EFI_SUCCESS) {
+		ret = efi_bootmgr_update_media_device_boot_option();
+		if (ret)
+			return -1;
+	}
+
 	return 0;
 }
 
diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
index 1f4ab2b419..cdfd16ea77 100644
--- a/lib/efi_loader/efi_helper.c
+++ b/lib/efi_loader/efi_helper.c
@@ -257,3 +257,28 @@ efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf, efi_guid_t *gu
 
 	return ret;
 }
+
+/**
+ * efi_search_bootorder() - search the boot option index in BootOrder
+ *
+ * @bootorder:	pointer to the BootOrder variable
+ * @num:	number of BootOrder entry
+ * @target:	target boot option index to search
+ * @index:	pointer to store the index of BootOrder variable
+ * Return:	true if exists, false otherwise
+ */
+bool efi_search_bootorder(u16 *bootorder, efi_uintn_t num, u32 target, u32 *index)
+{
+	u32 i;
+
+	for (i = 0; i < num; i++) {
+		if (target == bootorder[i]) {
+			if (index)
+				*index = i;
+
+			return true;
+		}
+	}
+
+	return false;
+}
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
index be95ed44e6..2f251553e1 100644
--- a/lib/efi_loader/efi_variable.c
+++ b/lib/efi_loader/efi_variable.c
@@ -476,6 +476,10 @@ efi_status_t efi_init_variables(void)
 			log_err("Invalid EFI variable seed\n");
 	}
 
+	ret = efi_init_secure_state();
+	if (ret != EFI_SUCCESS)
+		return ret;
 
-	return efi_init_secure_state();
+	/* update boot option management after variable service initialized */
+	return efi_bootmgr_update_media_device_boot_option();
 }
diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
index dfef18435d..a48d313ef0 100644
--- a/lib/efi_loader/efi_variable_tee.c
+++ b/lib/efi_loader/efi_variable_tee.c
@@ -748,5 +748,6 @@ efi_status_t efi_init_variables(void)
 	if (ret != EFI_SUCCESS)
 		return ret;
 
-	return EFI_SUCCESS;
+	/* update boot option management after variable service initialized */
+	return efi_bootmgr_update_media_device_boot_option();
 }
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2 2/2] Load option with short device path for boot vars
  2023-04-05  0:06 [PATCH v2 1/2] Boot var automatic management for removable medias Raymond Mao
@ 2023-04-05  0:06 ` Raymond Mao
  2023-04-07 10:13 ` [PATCH v2 1/2] Boot var automatic management for removable medias Heinrich Schuchardt
  1 sibling, 0 replies; 6+ messages in thread
From: Raymond Mao @ 2023-04-05  0:06 UTC (permalink / raw)
  To: u-boot
  Cc: Raymond Mao, Heinrich Schuchardt, Ilias Apalodimas,
	Masahisa Kojima, Simon Glass

The boot variables automatically generated for removable medias
should be with short form of device path without device nodes.
This is a requirement for the case that a removable media is
plugged into a different port but is still able to work with the
existing boot variables.

Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
---
Changes in v2
- Ignore EFI_NOT_FOUND returned from
  efi_bootmgr_update_media_device_boot_option which means no boot
  options scanned.

 cmd/bootmenu.c                    |  2 +-
 cmd/eficonfig.c                   |  2 +-
 include/efi_loader.h              |  2 +-
 lib/efi_loader/efi_bootmgr.c      | 16 +++++++++++++---
 lib/efi_loader/efi_disk.c         |  4 ++--
 lib/efi_loader/efi_variable.c     |  6 +++++-
 lib/efi_loader/efi_variable_tee.c |  6 +++++-
 7 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
index 01daddca7b..058b47c69a 100644
--- a/cmd/bootmenu.c
+++ b/cmd/bootmenu.c
@@ -351,7 +351,7 @@ static struct bootmenu_data *bootmenu_create(int delay)
 		 * UEFI specification requires booting from removal media using
 		 * a architecture-specific default image name such as BOOTAA64.EFI.
 		 */
-		efi_ret = efi_bootmgr_update_media_device_boot_option();
+		efi_ret = efi_bootmgr_update_media_device_boot_option(true);
 		if (efi_ret != EFI_SUCCESS && efi_ret != EFI_NOT_FOUND)
 			goto cleanup;
 
diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
index 82a80306f4..38371bfea6 100644
--- a/cmd/eficonfig.c
+++ b/cmd/eficonfig.c
@@ -2313,7 +2313,7 @@ static int do_eficonfig(struct cmd_tbl *cmdtp, int flag, int argc, char *const a
 	if (ret != EFI_SUCCESS)
 		return CMD_RET_FAILURE;
 
-	ret = efi_bootmgr_update_media_device_boot_option();
+	ret = efi_bootmgr_update_media_device_boot_option(true);
 	if (ret != EFI_SUCCESS && ret != EFI_NOT_FOUND)
 		return ret;
 
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 31ca1f5d1d..0a69f08d6c 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -522,7 +522,7 @@ efi_status_t efi_bootmgr_append_bootorder(u16 index);
 efi_status_t efi_bootmgr_get_unused_bootoption(u16 *buf,
 					       efi_uintn_t buf_size, u32 *index);
 /* Generate the media device boot option */
-efi_status_t efi_bootmgr_update_media_device_boot_option(void);
+efi_status_t efi_bootmgr_update_media_device_boot_option(bool short_path);
 /* Delete selected boot option */
 efi_status_t efi_bootmgr_delete_boot_option(u16 boot_index);
 /* search the boot option index in BootOrder */
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index c329428973..c4bc8b354f 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -354,11 +354,13 @@ error:
  * @opt:		pointer to the media boot option structure
  * @volume_handles:	pointer to the efi handles
  * @count:		number of efi handle
+ * @short_path:		use short form device path for matching
  * Return:		status code
  */
 static efi_status_t efi_bootmgr_enumerate_boot_option(struct eficonfig_media_boot_option *opt,
 						      efi_handle_t *volume_handles,
-						      efi_status_t count)
+						      efi_status_t count,
+						      bool short_path)
 {
 	u32 i;
 	struct efi_handler *handler;
@@ -387,6 +389,13 @@ static efi_status_t efi_bootmgr_enumerate_boot_option(struct eficonfig_media_boo
 		p = dev_name;
 		utf8_utf16_strncpy(&p, buf, strlen(buf));
 
+		/* use short form device path */
+		if (short_path) {
+			device_path = efi_dp_shorten(device_path);
+			if (!device_path)
+				continue;
+		}
+
 		lo.label = dev_name;
 		lo.attributes = LOAD_OPTION_ACTIVE;
 		lo.file_path = device_path;
@@ -651,9 +660,10 @@ efi_status_t efi_bootmgr_delete_boot_option(u16 boot_index)
  *   - If the device is not attached to the system, the associated BOOT#### variable
  *     is automatically deleted.
  *
+ * @short_path:	use short form device path for matching
  * Return:	status code
  */
-efi_status_t efi_bootmgr_update_media_device_boot_option(void)
+efi_status_t efi_bootmgr_update_media_device_boot_option(bool short_path)
 {
 	u32 i;
 	efi_status_t ret;
@@ -673,7 +683,7 @@ efi_status_t efi_bootmgr_update_media_device_boot_option(void)
 		goto out;
 
 	/* enumerate all devices supporting EFI_SIMPLE_FILE_SYSTEM_PROTOCOL */
-	ret = efi_bootmgr_enumerate_boot_option(opt, volume_handles, count);
+	ret = efi_bootmgr_enumerate_boot_option(opt, volume_handles, count, short_path);
 	if (ret != EFI_SUCCESS)
 		goto out;
 
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
index 1309e28134..7effffa8d5 100644
--- a/lib/efi_loader/efi_disk.c
+++ b/lib/efi_loader/efi_disk.c
@@ -689,8 +689,8 @@ int efi_disk_probe(void *ctx, struct event *event)
 
 	/* only do the boot option management when UEFI sub-system is initialized */
 	if (efi_obj_list_initialized == EFI_SUCCESS) {
-		ret = efi_bootmgr_update_media_device_boot_option();
-		if (ret)
+		ret = efi_bootmgr_update_media_device_boot_option(true);
+		if (ret != EFI_SUCCESS && ret != EFI_NOT_FOUND)
 			return -1;
 	}
 
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
index 2f251553e1..be3f40333d 100644
--- a/lib/efi_loader/efi_variable.c
+++ b/lib/efi_loader/efi_variable.c
@@ -481,5 +481,9 @@ efi_status_t efi_init_variables(void)
 		return ret;
 
 	/* update boot option management after variable service initialized */
-	return efi_bootmgr_update_media_device_boot_option();
+	ret = efi_bootmgr_update_media_device_boot_option(true);
+	if (ret != EFI_SUCCESS && ret != EFI_NOT_FOUND)
+		return ret;
+
+	return EFI_SUCCESS;
 }
diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
index a48d313ef0..481c4985ab 100644
--- a/lib/efi_loader/efi_variable_tee.c
+++ b/lib/efi_loader/efi_variable_tee.c
@@ -749,5 +749,9 @@ efi_status_t efi_init_variables(void)
 		return ret;
 
 	/* update boot option management after variable service initialized */
-	return efi_bootmgr_update_media_device_boot_option();
+	ret = efi_bootmgr_update_media_device_boot_option(true);
+	if (ret != EFI_SUCCESS && ret != EFI_NOT_FOUND)
+		return ret;
+
+	return EFI_SUCCESS;
 }
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/2] Boot var automatic management for removable medias
  2023-04-05  0:06 [PATCH v2 1/2] Boot var automatic management for removable medias Raymond Mao
  2023-04-05  0:06 ` [PATCH v2 2/2] Load option with short device path for boot vars Raymond Mao
@ 2023-04-07 10:13 ` Heinrich Schuchardt
  2023-04-13  8:19   ` AKASHI Takahiro
  1 sibling, 1 reply; 6+ messages in thread
From: Heinrich Schuchardt @ 2023-04-07 10:13 UTC (permalink / raw)
  To: Raymond Mao; +Cc: Ilias Apalodimas, Masahisa Kojima, Simon Glass, u-boot

On 4/5/23 02:06, Raymond Mao wrote:
> Changes for complying to EFI spec §3.5.1.1
> 'Removable Media Boot Behavior'.
> Boot variables can be automatically generated during a removable
> media is probed. At the same time, unused boot variables will be
> detected and removed.
> Related APIs are renamed and moved from cmd to lib for re-use
> between eficonfig and bootmgr.
>
> Signed-off-by: Raymond Mao <raymond.mao@linaro.org>

We should split the patch into moving and renaming functions and
individual patches for each changed functionality.

> ---
> Changes in v2
> - Ignore EFI_NOT_FOUND returned from
>    efi_bootmgr_update_media_device_boot_option which means no boot
>    options scanned.
>
>   cmd/bootmenu.c                    |   2 +-
>   cmd/eficonfig.c                   | 408 +-----------------------------
>   include/efi_config.h              |   5 -
>   include/efi_loader.h              |  11 +
>   lib/efi_loader/efi_bootmgr.c      | 380 ++++++++++++++++++++++++++++
>   lib/efi_loader/efi_disk.c         |   7 +
>   lib/efi_loader/efi_helper.c       |  25 ++
>   lib/efi_loader/efi_variable.c     |   6 +-
>   lib/efi_loader/efi_variable_tee.c |   3 +-
>   9 files changed, 437 insertions(+), 410 deletions(-)
>
> diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
> index 6baeedc69f..01daddca7b 100644
> --- a/cmd/bootmenu.c
> +++ b/cmd/bootmenu.c
> @@ -351,7 +351,7 @@ static struct bootmenu_data *bootmenu_create(int delay)
>   		 * UEFI specification requires booting from removal media using
>   		 * a architecture-specific default image name such as BOOTAA64.EFI.
>   		 */
> -		efi_ret = eficonfig_generate_media_device_boot_option();
> +		efi_ret = efi_bootmgr_update_media_device_boot_option();
>   		if (efi_ret != EFI_SUCCESS && efi_ret != EFI_NOT_FOUND)
>   			goto cleanup;
>
> diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> index 720f52b48b..82a80306f4 100644
> --- a/cmd/eficonfig.c
> +++ b/cmd/eficonfig.c
> @@ -1134,43 +1134,6 @@ out:
>   	return ret;
>   }
>
> -/**
> - * eficonfig_get_unused_bootoption() - get unused "Boot####" index
> - *
> - * @buf:	pointer to the buffer to store boot option variable name
> - * @buf_size:	buffer size
> - * @index:	pointer to store the index in the BootOrder variable
> - * Return:	status code
> - */
> -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;
> -}
> -
>   /**
>    * eficonfig_set_boot_option() - set boot option
>    *
> @@ -1208,46 +1171,6 @@ static efi_status_t eficonfig_set_boot_option(u16 *varname, struct efi_device_pa
>   	return ret;
>   }
>
> -/**
> - * eficonfig_append_bootorder() - append new boot option in BootOrder variable
> - *
> - * @index:	"Boot####" index to append to BootOrder variable
> - * Return:	status code
> - */
> -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;
> -}
> -
>   /**
>    * create_boot_option_entry() - create boot option entry
>    *
> @@ -1619,7 +1542,7 @@ static efi_status_t eficonfig_process_add_boot_option(void *data)
>   	if (!bo)
>   		return EFI_OUT_OF_RESOURCES;
>
> -	ret = eficonfig_get_unused_bootoption(varname, sizeof(varname), &bo->boot_index);
> +	ret = efi_bootmgr_get_unused_bootoption(varname, sizeof(varname), &bo->boot_index);
>   	if (ret != EFI_SUCCESS)
>   		return ret;
>
> @@ -1627,7 +1550,7 @@ static efi_status_t eficonfig_process_add_boot_option(void *data)
>   	if (ret != EFI_SUCCESS)
>   		goto out;
>
> -	ret = eficonfig_append_bootorder((u16)bo->boot_index);
> +	ret = efi_bootmgr_append_bootorder((u16)bo->boot_index);
>   	if (ret != EFI_SUCCESS)
>   		goto out;
>
> @@ -1656,31 +1579,6 @@ static efi_status_t eficonfig_process_boot_selected(void *data)
>   	return EFI_SUCCESS;
>   }
>
> -/**
> - * search_bootorder() - search the boot option index in BootOrder
> - *
> - * @bootorder:	pointer to the BootOrder variable
> - * @num:	number of BootOrder entry
> - * @target:	target boot option index to search
> - * @index:	pointer to store the index of BootOrder variable
> - * Return:	true if exists, false otherwise
> - */
> -static bool search_bootorder(u16 *bootorder, efi_uintn_t num, u32 target, u32 *index)
> -{
> -	u32 i;
> -
> -	for (i = 0; i < num; i++) {
> -		if (target == bootorder[i]) {
> -			if (index)
> -				*index = i;
> -
> -			return true;
> -		}
> -	}
> -
> -	return false;
> -}
> -
>   /**
>    * eficonfig_add_boot_selection_entry() - add boot option menu entry
>    *
> @@ -1805,7 +1703,7 @@ static efi_status_t eficonfig_show_boot_selection(unsigned int *selected)
>
>   		if (efi_varname_is_load_option(var_name16, &index)) {
>   			/* If the index is included in the BootOrder, skip it */
> -			if (search_bootorder(bootorder, num, index, NULL))
> +			if (efi_search_bootorder(bootorder, num, index, NULL))
>   				continue;
>
>   			ret = eficonfig_add_boot_selection_entry(efi_menu, index, selected);
> @@ -2202,7 +2100,7 @@ static efi_status_t eficonfig_create_change_boot_order_entry(struct efimenu *efi
>
>   		if (efi_varname_is_load_option(var_name16, &index)) {
>   			/* If the index is included in the BootOrder, skip it */
> -			if (search_bootorder(bootorder, num, index, NULL))
> +			if (efi_search_bootorder(bootorder, num, index, NULL))
>   				continue;
>
>   			ret = eficonfig_add_change_boot_order_entry(efi_menu, index, false);
> @@ -2304,50 +2202,6 @@ out:
>   	return ret;
>   }
>
> -/**
> - * delete_boot_option() - delete selected boot option
> - *
> - * @boot_index:	boot option index to delete
> - * Return:	status code
> - */
> -static efi_status_t delete_boot_option(u16 boot_index)
> -{
> -	u16 *bootorder;
> -	u16 varname[9];
> -	efi_status_t ret;
> -	unsigned int index;
> -	efi_uintn_t num, size;
> -
> -	efi_create_indexed_name(varname, sizeof(varname),
> -				"Boot", boot_index);
> -	ret = efi_set_variable_int(varname, &efi_global_variable_guid,
> -				   0, 0, NULL, false);
> -	if (ret != EFI_SUCCESS) {
> -		log_err("delete boot option(%ls) failed\n", varname);
> -		return ret;
> -	}
> -
> -	/* update BootOrder if necessary */
> -	bootorder = efi_get_var(u"BootOrder", &efi_global_variable_guid, &size);
> -	if (!bootorder)
> -		return EFI_SUCCESS;
> -
> -	num = size / sizeof(u16);
> -	if (!search_bootorder(bootorder, num, boot_index, &index))
> -		return EFI_SUCCESS;
> -
> -	memmove(&bootorder[index], &bootorder[index + 1],
> -		(num - index - 1) * sizeof(u16));
> -	size -= sizeof(u16);
> -	ret = efi_set_variable_int(u"BootOrder", &efi_global_variable_guid,
> -				   EFI_VARIABLE_NON_VOLATILE |
> -				   EFI_VARIABLE_BOOTSERVICE_ACCESS |
> -				   EFI_VARIABLE_RUNTIME_ACCESS,
> -				   size, bootorder, false);
> -
> -	return ret;
> -}
> -
>   /**
>    * eficonfig_process_delete_boot_option() - handler to delete boot option
>    *
> @@ -2362,7 +2216,7 @@ static efi_status_t eficonfig_process_delete_boot_option(void *data)
>   	while (1) {
>   		ret = eficonfig_show_boot_selection(&selected);
>   		if (ret == EFI_SUCCESS)
> -			ret = delete_boot_option(selected);
> +			ret = efi_bootmgr_delete_boot_option(selected);

Please, pass the number in the Boot#### variable and not an index in
BootOrder.

>
>   		if (ret != EFI_SUCCESS)
>   			break;
> @@ -2374,256 +2228,6 @@ static efi_status_t eficonfig_process_delete_boot_option(void *data)
>   	return ret;
>   }
>
> -/**
> - * eficonfig_enumerate_boot_option() - enumerate the possible bootable media
> - *
> - * @opt:		pointer to the media boot option structure
> - * @volume_handles:	pointer to the efi handles
> - * @count:		number of efi handle
> - * Return:		status code
> - */
> -efi_status_t eficonfig_enumerate_boot_option(struct eficonfig_media_boot_option *opt,
> -					     efi_handle_t *volume_handles, efi_status_t count)
> -{
> -	u32 i;
> -	struct efi_handler *handler;
> -	efi_status_t ret = EFI_SUCCESS;
> -
> -	for (i = 0; i < count; i++) {
> -		u16 *p;
> -		u16 dev_name[BOOTMENU_DEVICE_NAME_MAX];
> -		char *optional_data;
> -		struct efi_load_option lo;
> -		char buf[BOOTMENU_DEVICE_NAME_MAX];
> -		struct efi_device_path *device_path;
> -
> -		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_disk_get_device_name(volume_handles[i], buf, BOOTMENU_DEVICE_NAME_MAX);
> -		if (ret != EFI_SUCCESS)
> -			continue;
> -
> -		p = dev_name;
> -		utf8_utf16_strncpy(&p, buf, strlen(buf));
> -
> -		lo.label = dev_name;
> -		lo.attributes = LOAD_OPTION_ACTIVE;
> -		lo.file_path = device_path;
> -		lo.file_path_length = efi_dp_size(device_path) + sizeof(END);
> -		/*
> -		 * Set the dedicated guid to optional_data, it is used to identify
> -		 * the boot option that automatically generated by the bootmenu.
> -		 * efi_serialize_load_option() expects optional_data is null-terminated
> -		 * utf8 string, so set the "1234567" string to allocate enough space
> -		 * to store guid, instead of realloc the load_option.
> -		 */
> -		lo.optional_data = "1234567";
> -		opt[i].size = efi_serialize_load_option(&lo, (u8 **)&opt[i].lo);
> -		if (!opt[i].size) {
> -			ret = EFI_OUT_OF_RESOURCES;
> -			goto out;
> -		}
> -		/* set the guid */
> -		optional_data = (char *)opt[i].lo + (opt[i].size - u16_strsize(u"1234567"));
> -		memcpy(optional_data, &efi_guid_bootmenu_auto_generated, sizeof(efi_guid_t));
> -	}
> -
> -out:
> -	return ret;
> -}
> -
> -/**
> - * eficonfig_delete_invalid_boot_option() - delete non-existing boot option
> - *
> - * @opt:		pointer to the media boot option structure
> - * @count:		number of media boot option structure
> - * Return:		status code
> - */
> -efi_status_t eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_option *opt,
> -						  efi_status_t count)
> -{
> -	efi_uintn_t size;
> -	void *load_option;
> -	u32 i, list_size = 0;
> -	struct efi_load_option lo;
> -	u16 *var_name16 = NULL;
> -	u16 varname[] = u"Boot####";
> -	efi_status_t ret = EFI_SUCCESS;
> -	u16 *delete_index_list = NULL, *p;
> -	efi_uintn_t buf_size;
> -
> -	buf_size = 128;
> -	var_name16 = malloc(buf_size);
> -	if (!var_name16)
> -		return EFI_OUT_OF_RESOURCES;
> -
> -	var_name16[0] = 0;
> -	for (;;) {
> -		int index;
> -		efi_guid_t guid;
> -		efi_uintn_t tmp;
> -
> -		ret = efi_next_variable_name(&buf_size, &var_name16, &guid);
> -		if (ret == EFI_NOT_FOUND) {
> -			/*
> -			 * EFI_NOT_FOUND indicates we retrieved all EFI variables.
> -			 * This should be treated as success.
> -			 */
> -			ret = EFI_SUCCESS;
> -			break;
> -		}
> -		if (ret != EFI_SUCCESS)
> -			goto out;
> -
> -		if (!efi_varname_is_load_option(var_name16, &index))
> -			continue;
> -
> -		efi_create_indexed_name(varname, sizeof(varname), "Boot", index);
> -		load_option = efi_get_var(varname, &efi_global_variable_guid, &size);
> -		if (!load_option)
> -			continue;
> -
> -		tmp = size;
> -		ret = efi_deserialize_load_option(&lo, load_option, &size);
> -		if (ret != EFI_SUCCESS)
> -			goto next;
> -
> -		if (size >= sizeof(efi_guid_bootmenu_auto_generated) &&
> -		    !guidcmp(lo.optional_data, &efi_guid_bootmenu_auto_generated)) {
> -			for (i = 0; i < count; i++) {
> -				if (opt[i].size == tmp &&
> -				    memcmp(opt[i].lo, load_option, tmp) == 0) {
> -					opt[i].exist = true;
> -					break;
> -				}
> -			}
> -
> -			/*
> -			 * The entire list of variables must be retrieved by
> -			 * efi_get_next_variable_name_int() before deleting the invalid
> -			 * boot option, just save the index here.
> -			 */
> -			if (i == count) {
> -				p = realloc(delete_index_list, sizeof(u32) *
> -					    (list_size + 1));
> -				if (!p) {
> -					ret = EFI_OUT_OF_RESOURCES;
> -					goto out;
> -				}
> -				delete_index_list = p;
> -				delete_index_list[list_size++] = index;
> -			}
> -		}
> -next:
> -		free(load_option);
> -	}
> -
> -	/* delete all invalid boot options */
> -	for (i = 0; i < list_size; i++) {
> -		ret = delete_boot_option(delete_index_list[i]);
> -		if (ret != EFI_SUCCESS)
> -			goto out;
> -	}
> -
> -out:
> -	free(var_name16);
> -	free(delete_index_list);
> -
> -	return ret;
> -}
> -
> -/**
> - * eficonfig_generate_media_device_boot_option() - generate the media device boot option
> - *
> - * This function enumerates all devices supporting EFI_SIMPLE_FILE_SYSTEM_PROTOCOL
> - * and generate the bootmenu entries.
> - * This function also provide the BOOT#### variable maintenance for
> - * the media device entries.
> - *   - Automatically create the BOOT#### variable for the newly detected device,
> - *     this BOOT#### variable is distinguished by the special GUID
> - *     stored in the EFI_LOAD_OPTION.optional_data
> - *   - If the device is not attached to the system, the associated BOOT#### variable
> - *     is automatically deleted.
> - *
> - * Return:	status code
> - */
> -efi_status_t eficonfig_generate_media_device_boot_option(void)
> -{
> -	u32 i;
> -	efi_status_t ret;
> -	efi_uintn_t count;
> -	efi_handle_t *volume_handles = NULL;
> -	struct eficonfig_media_boot_option *opt = NULL;
> -
> -	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)
> -		return ret;
> -
> -	opt = calloc(count, sizeof(struct eficonfig_media_boot_option));
> -	if (!opt)
> -		goto out;
> -
> -	/* enumerate all devices supporting EFI_SIMPLE_FILE_SYSTEM_PROTOCOL */
> -	ret = eficonfig_enumerate_boot_option(opt, volume_handles, count);
> -	if (ret != EFI_SUCCESS)
> -		goto out;
> -
> -	/*
> -	 * System hardware configuration may vary depending on the user setup.
> -	 * The boot option is automatically added by the bootmenu.
> -	 * If the device is not attached to the system, the boot option needs
> -	 * to be deleted.
> -	 */
> -	ret = eficonfig_delete_invalid_boot_option(opt, count);
> -	if (ret != EFI_SUCCESS)
> -		goto out;
> -
> -	/* add non-existent boot option */
> -	for (i = 0; i < count; i++) {
> -		u32 boot_index;
> -		u16 var_name[9];
> -
> -		if (!opt[i].exist) {
> -			ret = eficonfig_get_unused_bootoption(var_name, sizeof(var_name),
> -							      &boot_index);
> -			if (ret != EFI_SUCCESS)
> -				goto out;
> -
> -			ret = efi_set_variable_int(var_name, &efi_global_variable_guid,
> -						   EFI_VARIABLE_NON_VOLATILE |
> -						   EFI_VARIABLE_BOOTSERVICE_ACCESS |
> -						   EFI_VARIABLE_RUNTIME_ACCESS,
> -						   opt[i].size, opt[i].lo, false);
> -			if (ret != EFI_SUCCESS)
> -				goto out;
> -
> -			ret = eficonfig_append_bootorder(boot_index);
> -			if (ret != EFI_SUCCESS) {
> -				efi_set_variable_int(var_name, &efi_global_variable_guid,
> -						     0, 0, NULL, false);
> -				goto out;
> -			}
> -		}
> -	}
> -
> -out:
> -	if (opt) {
> -		for (i = 0; i < count; i++)
> -			free(opt[i].lo);
> -	}
> -	free(opt);
> -	efi_free_pool(volume_handles);
> -
> -	return ret;
> -}
> -
>   /**
>    * eficonfig_init() - do required initialization for eficonfig command
>    *
> @@ -2709,7 +2313,7 @@ static int do_eficonfig(struct cmd_tbl *cmdtp, int flag, int argc, char *const a
>   	if (ret != EFI_SUCCESS)
>   		return CMD_RET_FAILURE;
>
> -	ret = eficonfig_generate_media_device_boot_option();
> +	ret = efi_bootmgr_update_media_device_boot_option();
>   	if (ret != EFI_SUCCESS && ret != EFI_NOT_FOUND)
>   		return ret;
>
> diff --git a/include/efi_config.h b/include/efi_config.h
> index 01ce9b2b06..d7c1601137 100644
> --- a/include/efi_config.h
> +++ b/include/efi_config.h
> @@ -105,11 +105,6 @@ efi_status_t eficonfig_process_common(struct efimenu *efi_menu,
>   				      void (*item_data_print)(void *),
>   				      char *(*item_choice)(void *));
>   efi_status_t eficonfig_process_select_file(void *data);
> -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 eficonfig_generate_media_device_boot_option(void);
> -
>   efi_status_t eficonfig_append_menu_entry(struct efimenu *efi_menu,
>   					 char *title, eficonfig_entry_func func,
>   					 void *data);
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 1542b4b625..31ca1f5d1d 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -516,6 +516,17 @@ extern struct list_head efi_register_notify_events;
>   int efi_init_early(void);
>   /* Initialize efi execution environment */
>   efi_status_t efi_init_obj_list(void);
> +/* Append new boot option in BootOrder variable */
> +efi_status_t efi_bootmgr_append_bootorder(u16 index);
> +/* Get unused "Boot####" index */
> +efi_status_t efi_bootmgr_get_unused_bootoption(u16 *buf,
> +					       efi_uintn_t buf_size, u32 *index);
> +/* Generate the media device boot option */
> +efi_status_t efi_bootmgr_update_media_device_boot_option(void);
> +/* Delete selected boot option */
> +efi_status_t efi_bootmgr_delete_boot_option(u16 boot_index);
> +/* search the boot option index in BootOrder */
> +bool efi_search_bootorder(u16 *bootorder, efi_uintn_t num, u32 target, u32 *index);
>   /* Set up console modes */
>   void efi_setup_console_size(void);
>   /* Install device tree */
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index 4b24b41047..c329428973 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -347,3 +347,383 @@ efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options)
>   error:
>   	return ret;
>   }
> +
> +/**
> + * efi_bootmgr_enumerate_boot_option() - enumerate the possible bootable media
> + *
> + * @opt:		pointer to the media boot option structure
> + * @volume_handles:	pointer to the efi handles
> + * @count:		number of efi handle
> + * Return:		status code
> + */
> +static efi_status_t efi_bootmgr_enumerate_boot_option(struct eficonfig_media_boot_option *opt,
> +						      efi_handle_t *volume_handles,
> +						      efi_status_t count)
> +{
> +	u32 i;
> +	struct efi_handler *handler;
> +	efi_status_t ret = EFI_SUCCESS;
> +
> +	for (i = 0; i < count; i++) {
> +		u16 *p;
> +		u16 dev_name[BOOTMENU_DEVICE_NAME_MAX];
> +		char *optional_data;
> +		struct efi_load_option lo;
> +		char buf[BOOTMENU_DEVICE_NAME_MAX];
> +		struct efi_device_path *device_path;
> +
> +		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_disk_get_device_name(volume_handles[i], buf, BOOTMENU_DEVICE_NAME_MAX);
> +		if (ret != EFI_SUCCESS)
> +			continue;
> +
> +		p = dev_name;
> +		utf8_utf16_strncpy(&p, buf, strlen(buf));
> +
> +		lo.label = dev_name;
> +		lo.attributes = LOAD_OPTION_ACTIVE;
> +		lo.file_path = device_path;
> +		lo.file_path_length = efi_dp_size(device_path) + sizeof(END);
> +		/*
> +		 * Set the dedicated guid to optional_data, it is used to identify
> +		 * the boot option that automatically generated by the bootmenu.
> +		 * efi_serialize_load_option() expects optional_data is null-terminated
> +		 * utf8 string, so set the "1234567" string to allocate enough space
> +		 * to store guid, instead of realloc the load_option.
> +		 */
> +		lo.optional_data = "1234567";
> +		opt[i].size = efi_serialize_load_option(&lo, (u8 **)&opt[i].lo);
> +		if (!opt[i].size) {
> +			ret = EFI_OUT_OF_RESOURCES;
> +			goto out;
> +		}
> +		/* set the guid */
> +		optional_data = (char *)opt[i].lo + (opt[i].size - u16_strsize(u"1234567"));
> +		memcpy(optional_data, &efi_guid_bootmenu_auto_generated, sizeof(efi_guid_t));
> +	}
> +
> +out:
> +	return ret;
> +}
> +
> +/**
> + * efi_bootmgr_delete_invalid_boot_option() - delete non-existing boot option

You can't delete what does not exist.

> + *
> + * @opt:		pointer to the media boot option structure
> + * @count:		number of media boot option structure

Why would you need such a parameter?

> + * Return:		status code
> + */
> +static efi_status_t efi_bootmgr_delete_invalid_boot_option(struct eficonfig_media_boot_option *opt,
> +							   efi_status_t count)

Why don't you pass in the boot option num

> +{
> +	efi_uintn_t size;
> +	void *load_option;
> +	u32 i, list_size = 0;
> +	struct efi_load_option lo;
> +	u16 *var_name16 = NULL;
> +	u16 varname[] = u"Boot####";

Why would you initialize the variable?

> +	efi_status_t ret = EFI_SUCCESS;
> +	u16 *delete_index_list = NULL, *p;
> +	efi_uintn_t buf_size;
> +
> +	buf_size = 128;
> +	var_name16 = malloc(buf_size);
> +	if (!var_name16)
> +		return EFI_OUT_OF_RESOURCES;
> +
> +	var_name16[0] = 0;
> +	for (;;) {
> +		int index;
> +		efi_guid_t guid;
> +		efi_uintn_t tmp;
> +
> +		ret = efi_next_variable_name(&buf_size, &var_name16, &guid);

Can't we simply put the boot option number into struct
eficonfig_media_boot_option and save 100 lines code?

> +		if (ret == EFI_NOT_FOUND) {
> +			log_debug("last efi var\n");
> +			/*
> +			 * EFI_NOT_FOUND indicates we retrieved all EFI variables.
> +			 * This should be treated as success.
> +			 */
> +			ret = EFI_SUCCESS;
> +			break;
> +		}
> +
> +		if (ret != EFI_SUCCESS)
> +			goto out;
> +
> +		if (!efi_varname_is_load_option(var_name16, &index))
> +			continue;
> +
> +		efi_create_indexed_name(varname, sizeof(varname), "Boot", index);
> +		load_option = efi_get_var(varname, &efi_global_variable_guid, &size);
> +		if (!load_option)
> +			continue;
> +
> +		tmp = size;
> +		ret = efi_deserialize_load_option(&lo, load_option, &size);
> +		if (ret != EFI_SUCCESS)
> +			goto next;
> +
> +		if (size >= sizeof(efi_guid_bootmenu_auto_generated) &&
> +		    !guidcmp(lo.optional_data, &efi_guid_bootmenu_auto_generated)) {
> +			for (i = 0; i < count; i++) {
> +				if (opt[i].size == tmp &&
> +				    memcmp(opt[i].lo, load_option, tmp) == 0) {
> +					opt[i].exist = true;
> +					break;
> +				}
> +			}
> +
> +			/*
> +			 * The entire list of variables must be retrieved by
> +			 * efi_get_next_variable_name_int() before deleting the invalid
> +			 * boot option, just save the index here.
> +			 */
> +			if (i == count) {
> +				log_debug("found index %d can be deleted\n", index);
> +				p = realloc(delete_index_list, sizeof(u32) *
> +					    (list_size + 1));
> +				if (!p) {
> +					ret = EFI_OUT_OF_RESOURCES;
> +					goto out;
> +				}
> +				delete_index_list = p;
> +				delete_index_list[list_size++] = index;
> +			}
> +		}
> +next:
> +		free(load_option);
> +	}
> +
> +	/* delete all invalid boot options */
> +	for (i = 0; i < list_size; i++) {
> +		log_debug("deleting index %d\n", delete_index_list[i]);
> +		ret = efi_bootmgr_delete_boot_option(delete_index_list[i]);
> +		if (ret != EFI_SUCCESS)
> +			goto out;
> +	}
> +
> +out:
> +	free(var_name16);
> +	free(delete_index_list);
> +
> +	return ret;
> +}
> +
> +/**
> + * efi_bootmgr_get_unused_bootoption() - get unused "Boot####" index
> + *
> + * @buf:	pointer to the buffer to store boot option variable name
> + * @buf_size:	buffer size
> + * @index:	pointer to store the index in the BootOrder variable
> + * Return:	status code
> + */
> +efi_status_t efi_bootmgr_get_unused_bootoption(u16 *buf, efi_uintn_t buf_size,
> +					       unsigned int *index)

This function should be static. The exported functions should not take
the inddex in the BootOrder variable as a parameter.

> +{
> +	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;
> +}
> +
> +/**
> + * efi_bootmgr_append_bootorder() - append new boot option in BootOrder variable
> + *
> + * @index:	"Boot####" index to append to BootOrder variable
> + * Return:	status code
> + */
> +efi_status_t efi_bootmgr_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;
> +}
> +
> +/**
> + * efi_bootmgr_delete_boot_option() - delete selected boot option
> + *
> + * @boot_index:	boot option index to delete
> + * Return:	status code
> + */
> +efi_status_t efi_bootmgr_delete_boot_option(u16 boot_index)
> +{
> +	u16 *bootorder;
> +	u16 varname[9];
> +	efi_status_t ret;
> +	unsigned int index;
> +	efi_uintn_t num, size;
> +
> +	efi_create_indexed_name(varname, sizeof(varname),
> +				"Boot", boot_index);
> +	ret = efi_set_variable_int(varname, &efi_global_variable_guid,
> +				   0, 0, NULL, false);
> +	if (ret != EFI_SUCCESS) {
> +		log_err("delete boot option(%ls) failed\n", varname);
> +		return ret;
> +	}
> +
> +	/* update BootOrder if necessary */
> +	bootorder = efi_get_var(u"BootOrder", &efi_global_variable_guid, &size);
> +	if (!bootorder)
> +		return EFI_SUCCESS;
> +
> +	num = size / sizeof(u16);
> +	if (!efi_search_bootorder(bootorder, num, boot_index, &index))
> +		return EFI_SUCCESS;
> +
> +	memmove(&bootorder[index], &bootorder[index + 1],
> +		(num - index - 1) * sizeof(u16));
> +	size -= sizeof(u16);
> +	ret = efi_set_variable_int(u"BootOrder", &efi_global_variable_guid,
> +				   EFI_VARIABLE_NON_VOLATILE |
> +				   EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +				   EFI_VARIABLE_RUNTIME_ACCESS,
> +				   size, bootorder, false);
> +
> +	return ret;
> +}
> +
> +/**
> + * efi_bootmgr_update_media_device_boot_option() - generate the media device boot option
> + *
> + * This function enumerates all devices supporting EFI_SIMPLE_FILE_SYSTEM_PROTOCOL
> + * and generate the bootmenu entries.
> + * This function also provide the BOOT#### variable maintenance for
> + * the media device entries.
> + *   - Automatically create the BOOT#### variable for the newly detected device,
> + *     this BOOT#### variable is distinguished by the special GUID
> + *     stored in the EFI_LOAD_OPTION.optional_data
> + *   - If the device is not attached to the system, the associated BOOT#### variable
> + *     is automatically deleted.
> + *
> + * Return:	status code
> + */
> +efi_status_t efi_bootmgr_update_media_device_boot_option(void)
> +{
> +	u32 i;
> +	efi_status_t ret;
> +	efi_uintn_t count;
> +	efi_handle_t *volume_handles = NULL;
> +	struct eficonfig_media_boot_option *opt = NULL;
> +
> +	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)
> +		return ret;
> +
> +	opt = calloc(count, sizeof(struct eficonfig_media_boot_option));
> +	if (!opt)
> +		goto out;
> +
> +	/* enumerate all devices supporting EFI_SIMPLE_FILE_SYSTEM_PROTOCOL */
> +	ret = efi_bootmgr_enumerate_boot_option(opt, volume_handles, count);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
> +	/*
> +	 * System hardware configuration may vary depending on the user setup.
> +	 * The boot option is automatically added by the bootmenu.
> +	 * If the device is not attached to the system, the boot option needs
> +	 * to be deleted.
> +	 */
> +	ret = efi_bootmgr_delete_invalid_boot_option(opt, count);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
> +	/* add non-existent boot option */
> +	for (i = 0; i < count; i++) {
> +		u32 boot_index;
> +		u16 var_name[9];
> +
> +		if (!opt[i].exist) {
> +			ret = efi_bootmgr_get_unused_bootoption(var_name, sizeof(var_name),
> +								&boot_index);
> +			if (ret != EFI_SUCCESS)
> +				goto out;
> +
> +			ret = efi_set_variable_int(var_name, &efi_global_variable_guid,
> +						   EFI_VARIABLE_NON_VOLATILE |
> +						   EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +						   EFI_VARIABLE_RUNTIME_ACCESS,
> +						   opt[i].size, opt[i].lo, false);
> +			if (ret != EFI_SUCCESS)
> +				goto out;
> +
> +			ret = efi_bootmgr_append_bootorder(boot_index);
> +			if (ret != EFI_SUCCESS) {
> +				efi_set_variable_int(var_name, &efi_global_variable_guid,
> +						     0, 0, NULL, false);
> +				goto out;
> +			}
> +		}
> +	}
> +
> +out:
> +	if (opt) {
> +		for (i = 0; i < count; i++) {
> +			if (opt[i].lo)
> +				free(opt[i].lo);
> +		}
> +		free(opt);
> +	}
> +	efi_free_pool(volume_handles);
> +
> +	return ret;
> +}
> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> index d2256713a8..1309e28134 100644
> --- a/lib/efi_loader/efi_disk.c
> +++ b/lib/efi_loader/efi_disk.c
> @@ -687,6 +687,13 @@ int efi_disk_probe(void *ctx, struct event *event)
>   			return -1;
>   	}
>
> +	/* only do the boot option management when UEFI sub-system is initialized */
> +	if (efi_obj_list_initialized == EFI_SUCCESS) {
> +		ret = efi_bootmgr_update_media_device_boot_option();

Why should we rescan all devices if a single device is added?

The only place where we need the list of removable media devices is when
executing the boot manager.

> +		if (ret)
> +			return -1;
> +	}
> +
>   	return 0;
>   }
>
> diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
> index 1f4ab2b419..cdfd16ea77 100644
> --- a/lib/efi_loader/efi_helper.c
> +++ b/lib/efi_loader/efi_helper.c
> @@ -257,3 +257,28 @@ efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf, efi_guid_t *gu
>
>   	return ret;
>   }
> +
> +/**
> + * efi_search_bootorder() - search the boot option index in BootOrder
> + *
> + * @bootorder:	pointer to the BootOrder variable
> + * @num:	number of BootOrder entry
> + * @target:	target boot option index to search
> + * @index:	pointer to store the index of BootOrder variable
> + * Return:	true if exists, false otherwise
> + */
> +bool efi_search_bootorder(u16 *bootorder, efi_uintn_t num, u32 target, u32 *index)

This function should be static. The exported function to delete a boot
option should not use the index in the boot order in their interface but
the number encoded in the variable name.

We can simplify the function:

Return -1 if the entry is not found and the index otherwise.

> +{
> +	u32 i;
> +
> +	for (i = 0; i < num; i++) {
> +		if (target == bootorder[i]) {
> +			if (index)
> +				*index = i;
> +
> +			return true;
> +		}
> +	}
> +
> +	return false;
> +}
> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> index be95ed44e6..2f251553e1 100644
> --- a/lib/efi_loader/efi_variable.c
> +++ b/lib/efi_loader/efi_variable.c
> @@ -476,6 +476,10 @@ efi_status_t efi_init_variables(void)
>   			log_err("Invalid EFI variable seed\n");
>   	}
>
> +	ret = efi_init_secure_state();
> +	if (ret != EFI_SUCCESS)
> +		return ret;
>
> -	return efi_init_secure_state();
> +	/* update boot option management after variable service initialized */
> +	return efi_bootmgr_update_media_device_boot_option();

The right place to call the function might be efi_init_obj_list().

Best regards

Heinrich

>   }
> diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> index dfef18435d..a48d313ef0 100644
> --- a/lib/efi_loader/efi_variable_tee.c
> +++ b/lib/efi_loader/efi_variable_tee.c
> @@ -748,5 +748,6 @@ efi_status_t efi_init_variables(void)
>   	if (ret != EFI_SUCCESS)
>   		return ret;
>
> -	return EFI_SUCCESS;
> +	/* update boot option management after variable service initialized */
> +	return efi_bootmgr_update_media_device_boot_option();
>   }


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/2] Boot var automatic management for removable medias
  2023-04-07 10:13 ` [PATCH v2 1/2] Boot var automatic management for removable medias Heinrich Schuchardt
@ 2023-04-13  8:19   ` AKASHI Takahiro
  2023-04-17 14:22     ` Raymond Mao
  0 siblings, 1 reply; 6+ messages in thread
From: AKASHI Takahiro @ 2023-04-13  8:19 UTC (permalink / raw)
  To: Heinrich Schuchardt
  Cc: Raymond Mao, Ilias Apalodimas, Masahisa Kojima, Simon Glass, u-boot

On Fri, Apr 07, 2023 at 12:13:36PM +0200, Heinrich Schuchardt wrote:
> On 4/5/23 02:06, Raymond Mao wrote:
> > Changes for complying to EFI spec §3.5.1.1
> > 'Removable Media Boot Behavior'.
> > Boot variables can be automatically generated during a removable
> > media is probed. At the same time, unused boot variables will be
> > detected and removed.
> > Related APIs are renamed and moved from cmd to lib for re-use
> > between eficonfig and bootmgr.
> > 
> > Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
> 
> We should split the patch into moving and renaming functions and
> individual patches for each changed functionality.
> 
> > ---
> > Changes in v2
> > - Ignore EFI_NOT_FOUND returned from
> >    efi_bootmgr_update_media_device_boot_option which means no boot
> >    options scanned.
> > 
> >   cmd/bootmenu.c                    |   2 +-
> >   cmd/eficonfig.c                   | 408 +-----------------------------
> >   include/efi_config.h              |   5 -
> >   include/efi_loader.h              |  11 +
> >   lib/efi_loader/efi_bootmgr.c      | 380 ++++++++++++++++++++++++++++
> >   lib/efi_loader/efi_disk.c         |   7 +
> >   lib/efi_loader/efi_helper.c       |  25 ++
> >   lib/efi_loader/efi_variable.c     |   6 +-
> >   lib/efi_loader/efi_variable_tee.c |   3 +-
> >   9 files changed, 437 insertions(+), 410 deletions(-)
> > 
> > diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
> > index 6baeedc69f..01daddca7b 100644
> > --- a/cmd/bootmenu.c
> > +++ b/cmd/bootmenu.c
> > @@ -351,7 +351,7 @@ static struct bootmenu_data *bootmenu_create(int delay)
> >   		 * UEFI specification requires booting from removal media using
> >   		 * a architecture-specific default image name such as BOOTAA64.EFI.
> >   		 */
> > -		efi_ret = eficonfig_generate_media_device_boot_option();
> > +		efi_ret = efi_bootmgr_update_media_device_boot_option();
> >   		if (efi_ret != EFI_SUCCESS && efi_ret != EFI_NOT_FOUND)
> >   			goto cleanup;
> > 
> > diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> > index 720f52b48b..82a80306f4 100644
> > --- a/cmd/eficonfig.c
> > +++ b/cmd/eficonfig.c
> > @@ -1134,43 +1134,6 @@ out:
> >   	return ret;
> >   }
> > 
> > -/**
> > - * eficonfig_get_unused_bootoption() - get unused "Boot####" index
> > - *
> > - * @buf:	pointer to the buffer to store boot option variable name
> > - * @buf_size:	buffer size
> > - * @index:	pointer to store the index in the BootOrder variable
> > - * Return:	status code
> > - */
> > -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;
> > -}
> > -
> >   /**
> >    * eficonfig_set_boot_option() - set boot option
> >    *
> > @@ -1208,46 +1171,6 @@ static efi_status_t eficonfig_set_boot_option(u16 *varname, struct efi_device_pa
> >   	return ret;
> >   }
> > 
> > -/**
> > - * eficonfig_append_bootorder() - append new boot option in BootOrder variable
> > - *
> > - * @index:	"Boot####" index to append to BootOrder variable
> > - * Return:	status code
> > - */
> > -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;
> > -}
> > -
> >   /**
> >    * create_boot_option_entry() - create boot option entry
> >    *
> > @@ -1619,7 +1542,7 @@ static efi_status_t eficonfig_process_add_boot_option(void *data)
> >   	if (!bo)
> >   		return EFI_OUT_OF_RESOURCES;
> > 
> > -	ret = eficonfig_get_unused_bootoption(varname, sizeof(varname), &bo->boot_index);
> > +	ret = efi_bootmgr_get_unused_bootoption(varname, sizeof(varname), &bo->boot_index);
> >   	if (ret != EFI_SUCCESS)
> >   		return ret;
> > 
> > @@ -1627,7 +1550,7 @@ static efi_status_t eficonfig_process_add_boot_option(void *data)
> >   	if (ret != EFI_SUCCESS)
> >   		goto out;
> > 
> > -	ret = eficonfig_append_bootorder((u16)bo->boot_index);
> > +	ret = efi_bootmgr_append_bootorder((u16)bo->boot_index);
> >   	if (ret != EFI_SUCCESS)
> >   		goto out;
> > 
> > @@ -1656,31 +1579,6 @@ static efi_status_t eficonfig_process_boot_selected(void *data)
> >   	return EFI_SUCCESS;
> >   }
> > 
> > -/**
> > - * search_bootorder() - search the boot option index in BootOrder
> > - *
> > - * @bootorder:	pointer to the BootOrder variable
> > - * @num:	number of BootOrder entry
> > - * @target:	target boot option index to search
> > - * @index:	pointer to store the index of BootOrder variable
> > - * Return:	true if exists, false otherwise
> > - */
> > -static bool search_bootorder(u16 *bootorder, efi_uintn_t num, u32 target, u32 *index)
> > -{
> > -	u32 i;
> > -
> > -	for (i = 0; i < num; i++) {
> > -		if (target == bootorder[i]) {
> > -			if (index)
> > -				*index = i;
> > -
> > -			return true;
> > -		}
> > -	}
> > -
> > -	return false;
> > -}
> > -
> >   /**
> >    * eficonfig_add_boot_selection_entry() - add boot option menu entry
> >    *
> > @@ -1805,7 +1703,7 @@ static efi_status_t eficonfig_show_boot_selection(unsigned int *selected)
> > 
> >   		if (efi_varname_is_load_option(var_name16, &index)) {
> >   			/* If the index is included in the BootOrder, skip it */
> > -			if (search_bootorder(bootorder, num, index, NULL))
> > +			if (efi_search_bootorder(bootorder, num, index, NULL))
> >   				continue;
> > 
> >   			ret = eficonfig_add_boot_selection_entry(efi_menu, index, selected);
> > @@ -2202,7 +2100,7 @@ static efi_status_t eficonfig_create_change_boot_order_entry(struct efimenu *efi
> > 
> >   		if (efi_varname_is_load_option(var_name16, &index)) {
> >   			/* If the index is included in the BootOrder, skip it */
> > -			if (search_bootorder(bootorder, num, index, NULL))
> > +			if (efi_search_bootorder(bootorder, num, index, NULL))
> >   				continue;
> > 
> >   			ret = eficonfig_add_change_boot_order_entry(efi_menu, index, false);
> > @@ -2304,50 +2202,6 @@ out:
> >   	return ret;
> >   }
> > 
> > -/**
> > - * delete_boot_option() - delete selected boot option
> > - *
> > - * @boot_index:	boot option index to delete
> > - * Return:	status code
> > - */
> > -static efi_status_t delete_boot_option(u16 boot_index)
> > -{
> > -	u16 *bootorder;
> > -	u16 varname[9];
> > -	efi_status_t ret;
> > -	unsigned int index;
> > -	efi_uintn_t num, size;
> > -
> > -	efi_create_indexed_name(varname, sizeof(varname),
> > -				"Boot", boot_index);
> > -	ret = efi_set_variable_int(varname, &efi_global_variable_guid,
> > -				   0, 0, NULL, false);
> > -	if (ret != EFI_SUCCESS) {
> > -		log_err("delete boot option(%ls) failed\n", varname);
> > -		return ret;
> > -	}
> > -
> > -	/* update BootOrder if necessary */
> > -	bootorder = efi_get_var(u"BootOrder", &efi_global_variable_guid, &size);
> > -	if (!bootorder)
> > -		return EFI_SUCCESS;
> > -
> > -	num = size / sizeof(u16);
> > -	if (!search_bootorder(bootorder, num, boot_index, &index))
> > -		return EFI_SUCCESS;
> > -
> > -	memmove(&bootorder[index], &bootorder[index + 1],
> > -		(num - index - 1) * sizeof(u16));
> > -	size -= sizeof(u16);
> > -	ret = efi_set_variable_int(u"BootOrder", &efi_global_variable_guid,
> > -				   EFI_VARIABLE_NON_VOLATILE |
> > -				   EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > -				   EFI_VARIABLE_RUNTIME_ACCESS,
> > -				   size, bootorder, false);
> > -
> > -	return ret;
> > -}
> > -
> >   /**
> >    * eficonfig_process_delete_boot_option() - handler to delete boot option
> >    *
> > @@ -2362,7 +2216,7 @@ static efi_status_t eficonfig_process_delete_boot_option(void *data)
> >   	while (1) {
> >   		ret = eficonfig_show_boot_selection(&selected);
> >   		if (ret == EFI_SUCCESS)
> > -			ret = delete_boot_option(selected);
> > +			ret = efi_bootmgr_delete_boot_option(selected);
> 
> Please, pass the number in the Boot#### variable and not an index in
> BootOrder.
> 
> > 
> >   		if (ret != EFI_SUCCESS)
> >   			break;
> > @@ -2374,256 +2228,6 @@ static efi_status_t eficonfig_process_delete_boot_option(void *data)
> >   	return ret;
> >   }
> > 
> > -/**
> > - * eficonfig_enumerate_boot_option() - enumerate the possible bootable media
> > - *
> > - * @opt:		pointer to the media boot option structure
> > - * @volume_handles:	pointer to the efi handles
> > - * @count:		number of efi handle
> > - * Return:		status code
> > - */
> > -efi_status_t eficonfig_enumerate_boot_option(struct eficonfig_media_boot_option *opt,
> > -					     efi_handle_t *volume_handles, efi_status_t count)
> > -{
> > -	u32 i;
> > -	struct efi_handler *handler;
> > -	efi_status_t ret = EFI_SUCCESS;
> > -
> > -	for (i = 0; i < count; i++) {
> > -		u16 *p;
> > -		u16 dev_name[BOOTMENU_DEVICE_NAME_MAX];
> > -		char *optional_data;
> > -		struct efi_load_option lo;
> > -		char buf[BOOTMENU_DEVICE_NAME_MAX];
> > -		struct efi_device_path *device_path;
> > -
> > -		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_disk_get_device_name(volume_handles[i], buf, BOOTMENU_DEVICE_NAME_MAX);
> > -		if (ret != EFI_SUCCESS)
> > -			continue;
> > -
> > -		p = dev_name;
> > -		utf8_utf16_strncpy(&p, buf, strlen(buf));
> > -
> > -		lo.label = dev_name;
> > -		lo.attributes = LOAD_OPTION_ACTIVE;
> > -		lo.file_path = device_path;
> > -		lo.file_path_length = efi_dp_size(device_path) + sizeof(END);
> > -		/*
> > -		 * Set the dedicated guid to optional_data, it is used to identify
> > -		 * the boot option that automatically generated by the bootmenu.
> > -		 * efi_serialize_load_option() expects optional_data is null-terminated
> > -		 * utf8 string, so set the "1234567" string to allocate enough space
> > -		 * to store guid, instead of realloc the load_option.
> > -		 */
> > -		lo.optional_data = "1234567";
> > -		opt[i].size = efi_serialize_load_option(&lo, (u8 **)&opt[i].lo);
> > -		if (!opt[i].size) {
> > -			ret = EFI_OUT_OF_RESOURCES;
> > -			goto out;
> > -		}
> > -		/* set the guid */
> > -		optional_data = (char *)opt[i].lo + (opt[i].size - u16_strsize(u"1234567"));
> > -		memcpy(optional_data, &efi_guid_bootmenu_auto_generated, sizeof(efi_guid_t));
> > -	}
> > -
> > -out:
> > -	return ret;
> > -}
> > -
> > -/**
> > - * eficonfig_delete_invalid_boot_option() - delete non-existing boot option
> > - *
> > - * @opt:		pointer to the media boot option structure
> > - * @count:		number of media boot option structure
> > - * Return:		status code
> > - */
> > -efi_status_t eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_option *opt,
> > -						  efi_status_t count)
> > -{
> > -	efi_uintn_t size;
> > -	void *load_option;
> > -	u32 i, list_size = 0;
> > -	struct efi_load_option lo;
> > -	u16 *var_name16 = NULL;
> > -	u16 varname[] = u"Boot####";
> > -	efi_status_t ret = EFI_SUCCESS;
> > -	u16 *delete_index_list = NULL, *p;
> > -	efi_uintn_t buf_size;
> > -
> > -	buf_size = 128;
> > -	var_name16 = malloc(buf_size);
> > -	if (!var_name16)
> > -		return EFI_OUT_OF_RESOURCES;
> > -
> > -	var_name16[0] = 0;
> > -	for (;;) {
> > -		int index;
> > -		efi_guid_t guid;
> > -		efi_uintn_t tmp;
> > -
> > -		ret = efi_next_variable_name(&buf_size, &var_name16, &guid);
> > -		if (ret == EFI_NOT_FOUND) {
> > -			/*
> > -			 * EFI_NOT_FOUND indicates we retrieved all EFI variables.
> > -			 * This should be treated as success.
> > -			 */
> > -			ret = EFI_SUCCESS;
> > -			break;
> > -		}
> > -		if (ret != EFI_SUCCESS)
> > -			goto out;
> > -
> > -		if (!efi_varname_is_load_option(var_name16, &index))
> > -			continue;
> > -
> > -		efi_create_indexed_name(varname, sizeof(varname), "Boot", index);
> > -		load_option = efi_get_var(varname, &efi_global_variable_guid, &size);
> > -		if (!load_option)
> > -			continue;
> > -
> > -		tmp = size;
> > -		ret = efi_deserialize_load_option(&lo, load_option, &size);
> > -		if (ret != EFI_SUCCESS)
> > -			goto next;
> > -
> > -		if (size >= sizeof(efi_guid_bootmenu_auto_generated) &&
> > -		    !guidcmp(lo.optional_data, &efi_guid_bootmenu_auto_generated)) {
> > -			for (i = 0; i < count; i++) {
> > -				if (opt[i].size == tmp &&
> > -				    memcmp(opt[i].lo, load_option, tmp) == 0) {
> > -					opt[i].exist = true;
> > -					break;
> > -				}
> > -			}
> > -
> > -			/*
> > -			 * The entire list of variables must be retrieved by
> > -			 * efi_get_next_variable_name_int() before deleting the invalid
> > -			 * boot option, just save the index here.
> > -			 */
> > -			if (i == count) {
> > -				p = realloc(delete_index_list, sizeof(u32) *
> > -					    (list_size + 1));
> > -				if (!p) {
> > -					ret = EFI_OUT_OF_RESOURCES;
> > -					goto out;
> > -				}
> > -				delete_index_list = p;
> > -				delete_index_list[list_size++] = index;
> > -			}
> > -		}
> > -next:
> > -		free(load_option);
> > -	}
> > -
> > -	/* delete all invalid boot options */
> > -	for (i = 0; i < list_size; i++) {
> > -		ret = delete_boot_option(delete_index_list[i]);
> > -		if (ret != EFI_SUCCESS)
> > -			goto out;
> > -	}
> > -
> > -out:
> > -	free(var_name16);
> > -	free(delete_index_list);
> > -
> > -	return ret;
> > -}
> > -
> > -/**
> > - * eficonfig_generate_media_device_boot_option() - generate the media device boot option
> > - *
> > - * This function enumerates all devices supporting EFI_SIMPLE_FILE_SYSTEM_PROTOCOL
> > - * and generate the bootmenu entries.
> > - * This function also provide the BOOT#### variable maintenance for
> > - * the media device entries.
> > - *   - Automatically create the BOOT#### variable for the newly detected device,
> > - *     this BOOT#### variable is distinguished by the special GUID
> > - *     stored in the EFI_LOAD_OPTION.optional_data
> > - *   - If the device is not attached to the system, the associated BOOT#### variable
> > - *     is automatically deleted.
> > - *
> > - * Return:	status code
> > - */
> > -efi_status_t eficonfig_generate_media_device_boot_option(void)
> > -{
> > -	u32 i;
> > -	efi_status_t ret;
> > -	efi_uintn_t count;
> > -	efi_handle_t *volume_handles = NULL;
> > -	struct eficonfig_media_boot_option *opt = NULL;
> > -
> > -	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)
> > -		return ret;
> > -
> > -	opt = calloc(count, sizeof(struct eficonfig_media_boot_option));
> > -	if (!opt)
> > -		goto out;
> > -
> > -	/* enumerate all devices supporting EFI_SIMPLE_FILE_SYSTEM_PROTOCOL */
> > -	ret = eficonfig_enumerate_boot_option(opt, volume_handles, count);
> > -	if (ret != EFI_SUCCESS)
> > -		goto out;
> > -
> > -	/*
> > -	 * System hardware configuration may vary depending on the user setup.
> > -	 * The boot option is automatically added by the bootmenu.
> > -	 * If the device is not attached to the system, the boot option needs
> > -	 * to be deleted.
> > -	 */
> > -	ret = eficonfig_delete_invalid_boot_option(opt, count);
> > -	if (ret != EFI_SUCCESS)
> > -		goto out;
> > -
> > -	/* add non-existent boot option */
> > -	for (i = 0; i < count; i++) {
> > -		u32 boot_index;
> > -		u16 var_name[9];
> > -
> > -		if (!opt[i].exist) {
> > -			ret = eficonfig_get_unused_bootoption(var_name, sizeof(var_name),
> > -							      &boot_index);
> > -			if (ret != EFI_SUCCESS)
> > -				goto out;
> > -
> > -			ret = efi_set_variable_int(var_name, &efi_global_variable_guid,
> > -						   EFI_VARIABLE_NON_VOLATILE |
> > -						   EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > -						   EFI_VARIABLE_RUNTIME_ACCESS,
> > -						   opt[i].size, opt[i].lo, false);
> > -			if (ret != EFI_SUCCESS)
> > -				goto out;
> > -
> > -			ret = eficonfig_append_bootorder(boot_index);
> > -			if (ret != EFI_SUCCESS) {
> > -				efi_set_variable_int(var_name, &efi_global_variable_guid,
> > -						     0, 0, NULL, false);
> > -				goto out;
> > -			}
> > -		}
> > -	}
> > -
> > -out:
> > -	if (opt) {
> > -		for (i = 0; i < count; i++)
> > -			free(opt[i].lo);
> > -	}
> > -	free(opt);
> > -	efi_free_pool(volume_handles);
> > -
> > -	return ret;
> > -}
> > -
> >   /**
> >    * eficonfig_init() - do required initialization for eficonfig command
> >    *
> > @@ -2709,7 +2313,7 @@ static int do_eficonfig(struct cmd_tbl *cmdtp, int flag, int argc, char *const a
> >   	if (ret != EFI_SUCCESS)
> >   		return CMD_RET_FAILURE;
> > 
> > -	ret = eficonfig_generate_media_device_boot_option();
> > +	ret = efi_bootmgr_update_media_device_boot_option();
> >   	if (ret != EFI_SUCCESS && ret != EFI_NOT_FOUND)
> >   		return ret;
> > 
> > diff --git a/include/efi_config.h b/include/efi_config.h
> > index 01ce9b2b06..d7c1601137 100644
> > --- a/include/efi_config.h
> > +++ b/include/efi_config.h
> > @@ -105,11 +105,6 @@ efi_status_t eficonfig_process_common(struct efimenu *efi_menu,
> >   				      void (*item_data_print)(void *),
> >   				      char *(*item_choice)(void *));
> >   efi_status_t eficonfig_process_select_file(void *data);
> > -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 eficonfig_generate_media_device_boot_option(void);
> > -
> >   efi_status_t eficonfig_append_menu_entry(struct efimenu *efi_menu,
> >   					 char *title, eficonfig_entry_func func,
> >   					 void *data);
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index 1542b4b625..31ca1f5d1d 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -516,6 +516,17 @@ extern struct list_head efi_register_notify_events;
> >   int efi_init_early(void);
> >   /* Initialize efi execution environment */
> >   efi_status_t efi_init_obj_list(void);
> > +/* Append new boot option in BootOrder variable */
> > +efi_status_t efi_bootmgr_append_bootorder(u16 index);
> > +/* Get unused "Boot####" index */
> > +efi_status_t efi_bootmgr_get_unused_bootoption(u16 *buf,
> > +					       efi_uintn_t buf_size, u32 *index);
> > +/* Generate the media device boot option */
> > +efi_status_t efi_bootmgr_update_media_device_boot_option(void);
> > +/* Delete selected boot option */
> > +efi_status_t efi_bootmgr_delete_boot_option(u16 boot_index);
> > +/* search the boot option index in BootOrder */
> > +bool efi_search_bootorder(u16 *bootorder, efi_uintn_t num, u32 target, u32 *index);
> >   /* Set up console modes */
> >   void efi_setup_console_size(void);
> >   /* Install device tree */
> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > index 4b24b41047..c329428973 100644
> > --- a/lib/efi_loader/efi_bootmgr.c
> > +++ b/lib/efi_loader/efi_bootmgr.c
> > @@ -347,3 +347,383 @@ efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options)
> >   error:
> >   	return ret;
> >   }
> > +
> > +/**
> > + * efi_bootmgr_enumerate_boot_option() - enumerate the possible bootable media
> > + *
> > + * @opt:		pointer to the media boot option structure
> > + * @volume_handles:	pointer to the efi handles
> > + * @count:		number of efi handle
> > + * Return:		status code
> > + */
> > +static efi_status_t efi_bootmgr_enumerate_boot_option(struct eficonfig_media_boot_option *opt,
> > +						      efi_handle_t *volume_handles,
> > +						      efi_status_t count)
> > +{
> > +	u32 i;
> > +	struct efi_handler *handler;
> > +	efi_status_t ret = EFI_SUCCESS;
> > +
> > +	for (i = 0; i < count; i++) {
> > +		u16 *p;
> > +		u16 dev_name[BOOTMENU_DEVICE_NAME_MAX];
> > +		char *optional_data;
> > +		struct efi_load_option lo;
> > +		char buf[BOOTMENU_DEVICE_NAME_MAX];
> > +		struct efi_device_path *device_path;
> > +
> > +		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_disk_get_device_name(volume_handles[i], buf, BOOTMENU_DEVICE_NAME_MAX);
> > +		if (ret != EFI_SUCCESS)
> > +			continue;
> > +
> > +		p = dev_name;
> > +		utf8_utf16_strncpy(&p, buf, strlen(buf));
> > +
> > +		lo.label = dev_name;
> > +		lo.attributes = LOAD_OPTION_ACTIVE;
> > +		lo.file_path = device_path;
> > +		lo.file_path_length = efi_dp_size(device_path) + sizeof(END);
> > +		/*
> > +		 * Set the dedicated guid to optional_data, it is used to identify
> > +		 * the boot option that automatically generated by the bootmenu.
> > +		 * efi_serialize_load_option() expects optional_data is null-terminated
> > +		 * utf8 string, so set the "1234567" string to allocate enough space
> > +		 * to store guid, instead of realloc the load_option.
> > +		 */
> > +		lo.optional_data = "1234567";
> > +		opt[i].size = efi_serialize_load_option(&lo, (u8 **)&opt[i].lo);
> > +		if (!opt[i].size) {
> > +			ret = EFI_OUT_OF_RESOURCES;
> > +			goto out;
> > +		}
> > +		/* set the guid */
> > +		optional_data = (char *)opt[i].lo + (opt[i].size - u16_strsize(u"1234567"));
> > +		memcpy(optional_data, &efi_guid_bootmenu_auto_generated, sizeof(efi_guid_t));
> > +	}
> > +
> > +out:
> > +	return ret;
> > +}
> > +
> > +/**
> > + * efi_bootmgr_delete_invalid_boot_option() - delete non-existing boot option
> 
> You can't delete what does not exist.
> 
> > + *
> > + * @opt:		pointer to the media boot option structure
> > + * @count:		number of media boot option structure
> 
> Why would you need such a parameter?
> 
> > + * Return:		status code
> > + */
> > +static efi_status_t efi_bootmgr_delete_invalid_boot_option(struct eficonfig_media_boot_option *opt,
> > +							   efi_status_t count)
> 
> Why don't you pass in the boot option num
> 
> > +{
> > +	efi_uintn_t size;
> > +	void *load_option;
> > +	u32 i, list_size = 0;
> > +	struct efi_load_option lo;
> > +	u16 *var_name16 = NULL;
> > +	u16 varname[] = u"Boot####";
> 
> Why would you initialize the variable?
> 
> > +	efi_status_t ret = EFI_SUCCESS;
> > +	u16 *delete_index_list = NULL, *p;
> > +	efi_uintn_t buf_size;
> > +
> > +	buf_size = 128;
> > +	var_name16 = malloc(buf_size);
> > +	if (!var_name16)
> > +		return EFI_OUT_OF_RESOURCES;
> > +
> > +	var_name16[0] = 0;
> > +	for (;;) {
> > +		int index;
> > +		efi_guid_t guid;
> > +		efi_uintn_t tmp;
> > +
> > +		ret = efi_next_variable_name(&buf_size, &var_name16, &guid);
> 
> Can't we simply put the boot option number into struct
> eficonfig_media_boot_option and save 100 lines code?
> 
> > +		if (ret == EFI_NOT_FOUND) {
> > +			log_debug("last efi var\n");
> > +			/*
> > +			 * EFI_NOT_FOUND indicates we retrieved all EFI variables.
> > +			 * This should be treated as success.
> > +			 */
> > +			ret = EFI_SUCCESS;
> > +			break;
> > +		}
> > +
> > +		if (ret != EFI_SUCCESS)
> > +			goto out;
> > +
> > +		if (!efi_varname_is_load_option(var_name16, &index))
> > +			continue;
> > +
> > +		efi_create_indexed_name(varname, sizeof(varname), "Boot", index);
> > +		load_option = efi_get_var(varname, &efi_global_variable_guid, &size);
> > +		if (!load_option)
> > +			continue;
> > +
> > +		tmp = size;
> > +		ret = efi_deserialize_load_option(&lo, load_option, &size);
> > +		if (ret != EFI_SUCCESS)
> > +			goto next;
> > +
> > +		if (size >= sizeof(efi_guid_bootmenu_auto_generated) &&
> > +		    !guidcmp(lo.optional_data, &efi_guid_bootmenu_auto_generated)) {
> > +			for (i = 0; i < count; i++) {
> > +				if (opt[i].size == tmp &&
> > +				    memcmp(opt[i].lo, load_option, tmp) == 0) {
> > +					opt[i].exist = true;
> > +					break;
> > +				}
> > +			}
> > +
> > +			/*
> > +			 * The entire list of variables must be retrieved by
> > +			 * efi_get_next_variable_name_int() before deleting the invalid
> > +			 * boot option, just save the index here.
> > +			 */
> > +			if (i == count) {
> > +				log_debug("found index %d can be deleted\n", index);
> > +				p = realloc(delete_index_list, sizeof(u32) *
> > +					    (list_size + 1));
> > +				if (!p) {
> > +					ret = EFI_OUT_OF_RESOURCES;
> > +					goto out;
> > +				}
> > +				delete_index_list = p;
> > +				delete_index_list[list_size++] = index;
> > +			}
> > +		}
> > +next:
> > +		free(load_option);
> > +	}
> > +
> > +	/* delete all invalid boot options */
> > +	for (i = 0; i < list_size; i++) {
> > +		log_debug("deleting index %d\n", delete_index_list[i]);
> > +		ret = efi_bootmgr_delete_boot_option(delete_index_list[i]);
> > +		if (ret != EFI_SUCCESS)
> > +			goto out;
> > +	}
> > +
> > +out:
> > +	free(var_name16);
> > +	free(delete_index_list);
> > +
> > +	return ret;
> > +}
> > +
> > +/**
> > + * efi_bootmgr_get_unused_bootoption() - get unused "Boot####" index
> > + *
> > + * @buf:	pointer to the buffer to store boot option variable name
> > + * @buf_size:	buffer size
> > + * @index:	pointer to store the index in the BootOrder variable
> > + * Return:	status code
> > + */
> > +efi_status_t efi_bootmgr_get_unused_bootoption(u16 *buf, efi_uintn_t buf_size,
> > +					       unsigned int *index)
> 
> This function should be static. The exported functions should not take
> the inddex in the BootOrder variable as a parameter.
> 
> > +{
> > +	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;
> > +}
> > +
> > +/**
> > + * efi_bootmgr_append_bootorder() - append new boot option in BootOrder variable
> > + *
> > + * @index:	"Boot####" index to append to BootOrder variable
> > + * Return:	status code
> > + */
> > +efi_status_t efi_bootmgr_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;
> > +}
> > +
> > +/**
> > + * efi_bootmgr_delete_boot_option() - delete selected boot option
> > + *
> > + * @boot_index:	boot option index to delete
> > + * Return:	status code
> > + */
> > +efi_status_t efi_bootmgr_delete_boot_option(u16 boot_index)
> > +{
> > +	u16 *bootorder;
> > +	u16 varname[9];
> > +	efi_status_t ret;
> > +	unsigned int index;
> > +	efi_uintn_t num, size;
> > +
> > +	efi_create_indexed_name(varname, sizeof(varname),
> > +				"Boot", boot_index);
> > +	ret = efi_set_variable_int(varname, &efi_global_variable_guid,
> > +				   0, 0, NULL, false);
> > +	if (ret != EFI_SUCCESS) {
> > +		log_err("delete boot option(%ls) failed\n", varname);
> > +		return ret;
> > +	}
> > +
> > +	/* update BootOrder if necessary */
> > +	bootorder = efi_get_var(u"BootOrder", &efi_global_variable_guid, &size);
> > +	if (!bootorder)
> > +		return EFI_SUCCESS;
> > +
> > +	num = size / sizeof(u16);
> > +	if (!efi_search_bootorder(bootorder, num, boot_index, &index))
> > +		return EFI_SUCCESS;
> > +
> > +	memmove(&bootorder[index], &bootorder[index + 1],
> > +		(num - index - 1) * sizeof(u16));
> > +	size -= sizeof(u16);
> > +	ret = efi_set_variable_int(u"BootOrder", &efi_global_variable_guid,
> > +				   EFI_VARIABLE_NON_VOLATILE |
> > +				   EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > +				   EFI_VARIABLE_RUNTIME_ACCESS,
> > +				   size, bootorder, false);
> > +
> > +	return ret;
> > +}
> > +
> > +/**
> > + * efi_bootmgr_update_media_device_boot_option() - generate the media device boot option
> > + *
> > + * This function enumerates all devices supporting EFI_SIMPLE_FILE_SYSTEM_PROTOCOL
> > + * and generate the bootmenu entries.
> > + * This function also provide the BOOT#### variable maintenance for
> > + * the media device entries.
> > + *   - Automatically create the BOOT#### variable for the newly detected device,
> > + *     this BOOT#### variable is distinguished by the special GUID
> > + *     stored in the EFI_LOAD_OPTION.optional_data
> > + *   - If the device is not attached to the system, the associated BOOT#### variable
> > + *     is automatically deleted.
> > + *
> > + * Return:	status code
> > + */
> > +efi_status_t efi_bootmgr_update_media_device_boot_option(void)
> > +{
> > +	u32 i;
> > +	efi_status_t ret;
> > +	efi_uintn_t count;
> > +	efi_handle_t *volume_handles = NULL;
> > +	struct eficonfig_media_boot_option *opt = NULL;
> > +
> > +	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)
> > +		return ret;
> > +
> > +	opt = calloc(count, sizeof(struct eficonfig_media_boot_option));
> > +	if (!opt)
> > +		goto out;
> > +
> > +	/* enumerate all devices supporting EFI_SIMPLE_FILE_SYSTEM_PROTOCOL */
> > +	ret = efi_bootmgr_enumerate_boot_option(opt, volume_handles, count);
> > +	if (ret != EFI_SUCCESS)
> > +		goto out;
> > +
> > +	/*
> > +	 * System hardware configuration may vary depending on the user setup.
> > +	 * The boot option is automatically added by the bootmenu.
> > +	 * If the device is not attached to the system, the boot option needs
> > +	 * to be deleted.
> > +	 */
> > +	ret = efi_bootmgr_delete_invalid_boot_option(opt, count);
> > +	if (ret != EFI_SUCCESS)
> > +		goto out;
> > +
> > +	/* add non-existent boot option */
> > +	for (i = 0; i < count; i++) {
> > +		u32 boot_index;
> > +		u16 var_name[9];
> > +
> > +		if (!opt[i].exist) {
> > +			ret = efi_bootmgr_get_unused_bootoption(var_name, sizeof(var_name),
> > +								&boot_index);
> > +			if (ret != EFI_SUCCESS)
> > +				goto out;
> > +
> > +			ret = efi_set_variable_int(var_name, &efi_global_variable_guid,
> > +						   EFI_VARIABLE_NON_VOLATILE |
> > +						   EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > +						   EFI_VARIABLE_RUNTIME_ACCESS,
> > +						   opt[i].size, opt[i].lo, false);
> > +			if (ret != EFI_SUCCESS)
> > +				goto out;
> > +
> > +			ret = efi_bootmgr_append_bootorder(boot_index);
> > +			if (ret != EFI_SUCCESS) {
> > +				efi_set_variable_int(var_name, &efi_global_variable_guid,
> > +						     0, 0, NULL, false);
> > +				goto out;
> > +			}
> > +		}
> > +	}
> > +
> > +out:
> > +	if (opt) {
> > +		for (i = 0; i < count; i++) {
> > +			if (opt[i].lo)
> > +				free(opt[i].lo);
> > +		}
> > +		free(opt);
> > +	}
> > +	efi_free_pool(volume_handles);
> > +
> > +	return ret;
> > +}
> > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> > index d2256713a8..1309e28134 100644
> > --- a/lib/efi_loader/efi_disk.c
> > +++ b/lib/efi_loader/efi_disk.c
> > @@ -687,6 +687,13 @@ int efi_disk_probe(void *ctx, struct event *event)
> >   			return -1;
> >   	}
> > 
> > +	/* only do the boot option management when UEFI sub-system is initialized */
> > +	if (efi_obj_list_initialized == EFI_SUCCESS) {
> > +		ret = efi_bootmgr_update_media_device_boot_option();
> 
> Why should we rescan all devices if a single device is added?

Then, don't forget to call the 'remove' function in efi_disk_remove()
(or _delete_raw() and _delete_part()).

> The only place where we need the list of removable media devices is when
> executing the boot manager.

Not sure what you mean, but we may want to confirm available boot options
with efidebug or eficonfig before invoking the boot manager.

-Takahiro Akashi

> 
> > +		if (ret)
> > +			return -1;
> > +	}
> > +
> >   	return 0;
> >   }
> > 
> > diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
> > index 1f4ab2b419..cdfd16ea77 100644
> > --- a/lib/efi_loader/efi_helper.c
> > +++ b/lib/efi_loader/efi_helper.c
> > @@ -257,3 +257,28 @@ efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf, efi_guid_t *gu
> > 
> >   	return ret;
> >   }
> > +
> > +/**
> > + * efi_search_bootorder() - search the boot option index in BootOrder
> > + *
> > + * @bootorder:	pointer to the BootOrder variable
> > + * @num:	number of BootOrder entry
> > + * @target:	target boot option index to search
> > + * @index:	pointer to store the index of BootOrder variable
> > + * Return:	true if exists, false otherwise
> > + */
> > +bool efi_search_bootorder(u16 *bootorder, efi_uintn_t num, u32 target, u32 *index)
> 
> This function should be static. The exported function to delete a boot
> option should not use the index in the boot order in their interface but
> the number encoded in the variable name.
> 
> We can simplify the function:
> 
> Return -1 if the entry is not found and the index otherwise.
> 
> > +{
> > +	u32 i;
> > +
> > +	for (i = 0; i < num; i++) {
> > +		if (target == bootorder[i]) {
> > +			if (index)
> > +				*index = i;
> > +
> > +			return true;
> > +		}
> > +	}
> > +
> > +	return false;
> > +}
> > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> > index be95ed44e6..2f251553e1 100644
> > --- a/lib/efi_loader/efi_variable.c
> > +++ b/lib/efi_loader/efi_variable.c
> > @@ -476,6 +476,10 @@ efi_status_t efi_init_variables(void)
> >   			log_err("Invalid EFI variable seed\n");
> >   	}
> > 
> > +	ret = efi_init_secure_state();
> > +	if (ret != EFI_SUCCESS)
> > +		return ret;
> > 
> > -	return efi_init_secure_state();
> > +	/* update boot option management after variable service initialized */
> > +	return efi_bootmgr_update_media_device_boot_option();
> 
> The right place to call the function might be efi_init_obj_list().
> 
> Best regards
> 
> Heinrich
> 
> >   }
> > diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> > index dfef18435d..a48d313ef0 100644
> > --- a/lib/efi_loader/efi_variable_tee.c
> > +++ b/lib/efi_loader/efi_variable_tee.c
> > @@ -748,5 +748,6 @@ efi_status_t efi_init_variables(void)
> >   	if (ret != EFI_SUCCESS)
> >   		return ret;
> > 
> > -	return EFI_SUCCESS;
> > +	/* update boot option management after variable service initialized */
> > +	return efi_bootmgr_update_media_device_boot_option();
> >   }
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/2] Boot var automatic management for removable medias
  2023-04-13  8:19   ` AKASHI Takahiro
@ 2023-04-17 14:22     ` Raymond Mao
  2023-04-18  0:50       ` AKASHI Takahiro
  0 siblings, 1 reply; 6+ messages in thread
From: Raymond Mao @ 2023-04-17 14:22 UTC (permalink / raw)
  To: AKASHI Takahiro, Heinrich Schuchardt, Raymond Mao,
	Ilias Apalodimas, Masahisa Kojima, Simon Glass, u-boot

On Thu, 13 Apr 2023 at 04:19, AKASHI Takahiro <takahiro.akashi@linaro.org>
wrote:

> On Fri, Apr 07, 2023 at 12:13:36PM +0200, Heinrich Schuchardt wrote:
> > On 4/5/23 02:06, Raymond Mao wrote:
> > > Changes for complying to EFI spec §3.5.1.1
> > > 'Removable Media Boot Behavior'.
> > > Boot variables can be automatically generated during a removable
> > > media is probed. At the same time, unused boot variables will be
> > > detected and removed.
> > > Related APIs are renamed and moved from cmd to lib for re-use
> > > between eficonfig and bootmgr.
> > >
> > > Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
> >
> > We should split the patch into moving and renaming functions and
> > individual patches for each changed functionality.
> >
> > > ---
> > > Changes in v2
> > > - Ignore EFI_NOT_FOUND returned from
> > >    efi_bootmgr_update_media_device_boot_option which means no boot
> > >    options scanned.
> > >
> > >   cmd/bootmenu.c                    |   2 +-
> > >   cmd/eficonfig.c                   | 408
> +-----------------------------
> > >   include/efi_config.h              |   5 -
> > >   include/efi_loader.h              |  11 +
> > >   lib/efi_loader/efi_bootmgr.c      | 380 ++++++++++++++++++++++++++++
> > >   lib/efi_loader/efi_disk.c         |   7 +
> > >   lib/efi_loader/efi_helper.c       |  25 ++
> > >   lib/efi_loader/efi_variable.c     |   6 +-
> > >   lib/efi_loader/efi_variable_tee.c |   3 +-
> > >   9 files changed, 437 insertions(+), 410 deletions(-)
> > >
> > > diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
> > > index 6baeedc69f..01daddca7b 100644
> > > --- a/cmd/bootmenu.c
> > > +++ b/cmd/bootmenu.c
> > > @@ -351,7 +351,7 @@ static struct bootmenu_data *bootmenu_create(int
> delay)
> > >              * UEFI specification requires booting from removal media
> using
> > >              * a architecture-specific default image name such as
> BOOTAA64.EFI.
> > >              */
> > > -           efi_ret = eficonfig_generate_media_device_boot_option();
> > > +           efi_ret = efi_bootmgr_update_media_device_boot_option();
> > >             if (efi_ret != EFI_SUCCESS && efi_ret != EFI_NOT_FOUND)
> > >                     goto cleanup;
> > >
> > > diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> > > index 720f52b48b..82a80306f4 100644
> > > --- a/cmd/eficonfig.c
> > > +++ b/cmd/eficonfig.c
> > > @@ -1134,43 +1134,6 @@ out:
> > >     return ret;
> > >   }
> > >
> > > -/**
> > > - * eficonfig_get_unused_bootoption() - get unused "Boot####" index
> > > - *
> > > - * @buf:   pointer to the buffer to store boot option variable name
> > > - * @buf_size:      buffer size
> > > - * @index: pointer to store the index in the BootOrder variable
> > > - * Return: status code
> > > - */
> > > -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;
> > > -}
> > > -
> > >   /**
> > >    * eficonfig_set_boot_option() - set boot option
> > >    *
> > > @@ -1208,46 +1171,6 @@ static efi_status_t
> eficonfig_set_boot_option(u16 *varname, struct efi_device_pa
> > >     return ret;
> > >   }
> > >
> > > -/**
> > > - * eficonfig_append_bootorder() - append new boot option in BootOrder
> variable
> > > - *
> > > - * @index: "Boot####" index to append to BootOrder variable
> > > - * Return: status code
> > > - */
> > > -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;
> > > -}
> > > -
> > >   /**
> > >    * create_boot_option_entry() - create boot option entry
> > >    *
> > > @@ -1619,7 +1542,7 @@ static efi_status_t
> eficonfig_process_add_boot_option(void *data)
> > >     if (!bo)
> > >             return EFI_OUT_OF_RESOURCES;
> > >
> > > -   ret = eficonfig_get_unused_bootoption(varname, sizeof(varname),
> &bo->boot_index);
> > > +   ret = efi_bootmgr_get_unused_bootoption(varname, sizeof(varname),
> &bo->boot_index);
> > >     if (ret != EFI_SUCCESS)
> > >             return ret;
> > >
> > > @@ -1627,7 +1550,7 @@ static efi_status_t
> eficonfig_process_add_boot_option(void *data)
> > >     if (ret != EFI_SUCCESS)
> > >             goto out;
> > >
> > > -   ret = eficonfig_append_bootorder((u16)bo->boot_index);
> > > +   ret = efi_bootmgr_append_bootorder((u16)bo->boot_index);
> > >     if (ret != EFI_SUCCESS)
> > >             goto out;
> > >
> > > @@ -1656,31 +1579,6 @@ static efi_status_t
> eficonfig_process_boot_selected(void *data)
> > >     return EFI_SUCCESS;
> > >   }
> > >
> > > -/**
> > > - * search_bootorder() - search the boot option index in BootOrder
> > > - *
> > > - * @bootorder:     pointer to the BootOrder variable
> > > - * @num:   number of BootOrder entry
> > > - * @target:        target boot option index to search
> > > - * @index: pointer to store the index of BootOrder variable
> > > - * Return: true if exists, false otherwise
> > > - */
> > > -static bool search_bootorder(u16 *bootorder, efi_uintn_t num, u32
> target, u32 *index)
> > > -{
> > > -   u32 i;
> > > -
> > > -   for (i = 0; i < num; i++) {
> > > -           if (target == bootorder[i]) {
> > > -                   if (index)
> > > -                           *index = i;
> > > -
> > > -                   return true;
> > > -           }
> > > -   }
> > > -
> > > -   return false;
> > > -}
> > > -
> > >   /**
> > >    * eficonfig_add_boot_selection_entry() - add boot option menu entry
> > >    *
> > > @@ -1805,7 +1703,7 @@ static efi_status_t
> eficonfig_show_boot_selection(unsigned int *selected)
> > >
> > >             if (efi_varname_is_load_option(var_name16, &index)) {
> > >                     /* If the index is included in the BootOrder, skip
> it */
> > > -                   if (search_bootorder(bootorder, num, index, NULL))
> > > +                   if (efi_search_bootorder(bootorder, num, index,
> NULL))
> > >                             continue;
> > >
> > >                     ret = eficonfig_add_boot_selection_entry(efi_menu,
> index, selected);
> > > @@ -2202,7 +2100,7 @@ static efi_status_t
> eficonfig_create_change_boot_order_entry(struct efimenu *efi
> > >
> > >             if (efi_varname_is_load_option(var_name16, &index)) {
> > >                     /* If the index is included in the BootOrder, skip
> it */
> > > -                   if (search_bootorder(bootorder, num, index, NULL))
> > > +                   if (efi_search_bootorder(bootorder, num, index,
> NULL))
> > >                             continue;
> > >
> > >                     ret =
> eficonfig_add_change_boot_order_entry(efi_menu, index, false);
> > > @@ -2304,50 +2202,6 @@ out:
> > >     return ret;
> > >   }
> > >
> > > -/**
> > > - * delete_boot_option() - delete selected boot option
> > > - *
> > > - * @boot_index:    boot option index to delete
> > > - * Return: status code
> > > - */
> > > -static efi_status_t delete_boot_option(u16 boot_index)
> > > -{
> > > -   u16 *bootorder;
> > > -   u16 varname[9];
> > > -   efi_status_t ret;
> > > -   unsigned int index;
> > > -   efi_uintn_t num, size;
> > > -
> > > -   efi_create_indexed_name(varname, sizeof(varname),
> > > -                           "Boot", boot_index);
> > > -   ret = efi_set_variable_int(varname, &efi_global_variable_guid,
> > > -                              0, 0, NULL, false);
> > > -   if (ret != EFI_SUCCESS) {
> > > -           log_err("delete boot option(%ls) failed\n", varname);
> > > -           return ret;
> > > -   }
> > > -
> > > -   /* update BootOrder if necessary */
> > > -   bootorder = efi_get_var(u"BootOrder", &efi_global_variable_guid,
> &size);
> > > -   if (!bootorder)
> > > -           return EFI_SUCCESS;
> > > -
> > > -   num = size / sizeof(u16);
> > > -   if (!search_bootorder(bootorder, num, boot_index, &index))
> > > -           return EFI_SUCCESS;
> > > -
> > > -   memmove(&bootorder[index], &bootorder[index + 1],
> > > -           (num - index - 1) * sizeof(u16));
> > > -   size -= sizeof(u16);
> > > -   ret = efi_set_variable_int(u"BootOrder", &efi_global_variable_guid,
> > > -                              EFI_VARIABLE_NON_VOLATILE |
> > > -                              EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > > -                              EFI_VARIABLE_RUNTIME_ACCESS,
> > > -                              size, bootorder, false);
> > > -
> > > -   return ret;
> > > -}
> > > -
> > >   /**
> > >    * eficonfig_process_delete_boot_option() - handler to delete boot
> option
> > >    *
> > > @@ -2362,7 +2216,7 @@ static efi_status_t
> eficonfig_process_delete_boot_option(void *data)
> > >     while (1) {
> > >             ret = eficonfig_show_boot_selection(&selected);
> > >             if (ret == EFI_SUCCESS)
> > > -                   ret = delete_boot_option(selected);
> > > +                   ret = efi_bootmgr_delete_boot_option(selected);
> >
> > Please, pass the number in the Boot#### variable and not an index in
> > BootOrder.
> >
> > >
> > >             if (ret != EFI_SUCCESS)
> > >                     break;
> > > @@ -2374,256 +2228,6 @@ static efi_status_t
> eficonfig_process_delete_boot_option(void *data)
> > >     return ret;
> > >   }
> > >
> > > -/**
> > > - * eficonfig_enumerate_boot_option() - enumerate the possible
> bootable media
> > > - *
> > > - * @opt:           pointer to the media boot option structure
> > > - * @volume_handles:        pointer to the efi handles
> > > - * @count:         number of efi handle
> > > - * Return:         status code
> > > - */
> > > -efi_status_t eficonfig_enumerate_boot_option(struct
> eficonfig_media_boot_option *opt,
> > > -                                        efi_handle_t *volume_handles,
> efi_status_t count)
> > > -{
> > > -   u32 i;
> > > -   struct efi_handler *handler;
> > > -   efi_status_t ret = EFI_SUCCESS;
> > > -
> > > -   for (i = 0; i < count; i++) {
> > > -           u16 *p;
> > > -           u16 dev_name[BOOTMENU_DEVICE_NAME_MAX];
> > > -           char *optional_data;
> > > -           struct efi_load_option lo;
> > > -           char buf[BOOTMENU_DEVICE_NAME_MAX];
> > > -           struct efi_device_path *device_path;
> > > -
> > > -           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_disk_get_device_name(volume_handles[i], buf,
> BOOTMENU_DEVICE_NAME_MAX);
> > > -           if (ret != EFI_SUCCESS)
> > > -                   continue;
> > > -
> > > -           p = dev_name;
> > > -           utf8_utf16_strncpy(&p, buf, strlen(buf));
> > > -
> > > -           lo.label = dev_name;
> > > -           lo.attributes = LOAD_OPTION_ACTIVE;
> > > -           lo.file_path = device_path;
> > > -           lo.file_path_length = efi_dp_size(device_path) +
> sizeof(END);
> > > -           /*
> > > -            * Set the dedicated guid to optional_data, it is used to
> identify
> > > -            * the boot option that automatically generated by the
> bootmenu.
> > > -            * efi_serialize_load_option() expects optional_data is
> null-terminated
> > > -            * utf8 string, so set the "1234567" string to allocate
> enough space
> > > -            * to store guid, instead of realloc the load_option.
> > > -            */
> > > -           lo.optional_data = "1234567";
> > > -           opt[i].size = efi_serialize_load_option(&lo, (u8
> **)&opt[i].lo);
> > > -           if (!opt[i].size) {
> > > -                   ret = EFI_OUT_OF_RESOURCES;
> > > -                   goto out;
> > > -           }
> > > -           /* set the guid */
> > > -           optional_data = (char *)opt[i].lo + (opt[i].size -
> u16_strsize(u"1234567"));
> > > -           memcpy(optional_data, &efi_guid_bootmenu_auto_generated,
> sizeof(efi_guid_t));
> > > -   }
> > > -
> > > -out:
> > > -   return ret;
> > > -}
> > > -
> > > -/**
> > > - * eficonfig_delete_invalid_boot_option() - delete non-existing boot
> option
> > > - *
> > > - * @opt:           pointer to the media boot option structure
> > > - * @count:         number of media boot option structure
> > > - * Return:         status code
> > > - */
> > > -efi_status_t eficonfig_delete_invalid_boot_option(struct
> eficonfig_media_boot_option *opt,
> > > -                                             efi_status_t count)
> > > -{
> > > -   efi_uintn_t size;
> > > -   void *load_option;
> > > -   u32 i, list_size = 0;
> > > -   struct efi_load_option lo;
> > > -   u16 *var_name16 = NULL;
> > > -   u16 varname[] = u"Boot####";
> > > -   efi_status_t ret = EFI_SUCCESS;
> > > -   u16 *delete_index_list = NULL, *p;
> > > -   efi_uintn_t buf_size;
> > > -
> > > -   buf_size = 128;
> > > -   var_name16 = malloc(buf_size);
> > > -   if (!var_name16)
> > > -           return EFI_OUT_OF_RESOURCES;
> > > -
> > > -   var_name16[0] = 0;
> > > -   for (;;) {
> > > -           int index;
> > > -           efi_guid_t guid;
> > > -           efi_uintn_t tmp;
> > > -
> > > -           ret = efi_next_variable_name(&buf_size, &var_name16,
> &guid);
> > > -           if (ret == EFI_NOT_FOUND) {
> > > -                   /*
> > > -                    * EFI_NOT_FOUND indicates we retrieved all EFI
> variables.
> > > -                    * This should be treated as success.
> > > -                    */
> > > -                   ret = EFI_SUCCESS;
> > > -                   break;
> > > -           }
> > > -           if (ret != EFI_SUCCESS)
> > > -                   goto out;
> > > -
> > > -           if (!efi_varname_is_load_option(var_name16, &index))
> > > -                   continue;
> > > -
> > > -           efi_create_indexed_name(varname, sizeof(varname), "Boot",
> index);
> > > -           load_option = efi_get_var(varname,
> &efi_global_variable_guid, &size);
> > > -           if (!load_option)
> > > -                   continue;
> > > -
> > > -           tmp = size;
> > > -           ret = efi_deserialize_load_option(&lo, load_option, &size);
> > > -           if (ret != EFI_SUCCESS)
> > > -                   goto next;
> > > -
> > > -           if (size >= sizeof(efi_guid_bootmenu_auto_generated) &&
> > > -               !guidcmp(lo.optional_data,
> &efi_guid_bootmenu_auto_generated)) {
> > > -                   for (i = 0; i < count; i++) {
> > > -                           if (opt[i].size == tmp &&
> > > -                               memcmp(opt[i].lo, load_option, tmp) ==
> 0) {
> > > -                                   opt[i].exist = true;
> > > -                                   break;
> > > -                           }
> > > -                   }
> > > -
> > > -                   /*
> > > -                    * The entire list of variables must be retrieved
> by
> > > -                    * efi_get_next_variable_name_int() before
> deleting the invalid
> > > -                    * boot option, just save the index here.
> > > -                    */
> > > -                   if (i == count) {
> > > -                           p = realloc(delete_index_list, sizeof(u32)
> *
> > > -                                       (list_size + 1));
> > > -                           if (!p) {
> > > -                                   ret = EFI_OUT_OF_RESOURCES;
> > > -                                   goto out;
> > > -                           }
> > > -                           delete_index_list = p;
> > > -                           delete_index_list[list_size++] = index;
> > > -                   }
> > > -           }
> > > -next:
> > > -           free(load_option);
> > > -   }
> > > -
> > > -   /* delete all invalid boot options */
> > > -   for (i = 0; i < list_size; i++) {
> > > -           ret = delete_boot_option(delete_index_list[i]);
> > > -           if (ret != EFI_SUCCESS)
> > > -                   goto out;
> > > -   }
> > > -
> > > -out:
> > > -   free(var_name16);
> > > -   free(delete_index_list);
> > > -
> > > -   return ret;
> > > -}
> > > -
> > > -/**
> > > - * eficonfig_generate_media_device_boot_option() - generate the media
> device boot option
> > > - *
> > > - * This function enumerates all devices supporting
> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL
> > > - * and generate the bootmenu entries.
> > > - * This function also provide the BOOT#### variable maintenance for
> > > - * the media device entries.
> > > - *   - Automatically create the BOOT#### variable for the newly
> detected device,
> > > - *     this BOOT#### variable is distinguished by the special GUID
> > > - *     stored in the EFI_LOAD_OPTION.optional_data
> > > - *   - If the device is not attached to the system, the associated
> BOOT#### variable
> > > - *     is automatically deleted.
> > > - *
> > > - * Return: status code
> > > - */
> > > -efi_status_t eficonfig_generate_media_device_boot_option(void)
> > > -{
> > > -   u32 i;
> > > -   efi_status_t ret;
> > > -   efi_uintn_t count;
> > > -   efi_handle_t *volume_handles = NULL;
> > > -   struct eficonfig_media_boot_option *opt = NULL;
> > > -
> > > -   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)
> > > -           return ret;
> > > -
> > > -   opt = calloc(count, sizeof(struct eficonfig_media_boot_option));
> > > -   if (!opt)
> > > -           goto out;
> > > -
> > > -   /* enumerate all devices supporting
> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL */
> > > -   ret = eficonfig_enumerate_boot_option(opt, volume_handles, count);
> > > -   if (ret != EFI_SUCCESS)
> > > -           goto out;
> > > -
> > > -   /*
> > > -    * System hardware configuration may vary depending on the user
> setup.
> > > -    * The boot option is automatically added by the bootmenu.
> > > -    * If the device is not attached to the system, the boot option
> needs
> > > -    * to be deleted.
> > > -    */
> > > -   ret = eficonfig_delete_invalid_boot_option(opt, count);
> > > -   if (ret != EFI_SUCCESS)
> > > -           goto out;
> > > -
> > > -   /* add non-existent boot option */
> > > -   for (i = 0; i < count; i++) {
> > > -           u32 boot_index;
> > > -           u16 var_name[9];
> > > -
> > > -           if (!opt[i].exist) {
> > > -                   ret = eficonfig_get_unused_bootoption(var_name,
> sizeof(var_name),
> > > -                                                         &boot_index);
> > > -                   if (ret != EFI_SUCCESS)
> > > -                           goto out;
> > > -
> > > -                   ret = efi_set_variable_int(var_name,
> &efi_global_variable_guid,
> > > -
> EFI_VARIABLE_NON_VOLATILE |
> > > -
> EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > > -
> EFI_VARIABLE_RUNTIME_ACCESS,
> > > -                                              opt[i].size, opt[i].lo,
> false);
> > > -                   if (ret != EFI_SUCCESS)
> > > -                           goto out;
> > > -
> > > -                   ret = eficonfig_append_bootorder(boot_index);
> > > -                   if (ret != EFI_SUCCESS) {
> > > -                           efi_set_variable_int(var_name,
> &efi_global_variable_guid,
> > > -                                                0, 0, NULL, false);
> > > -                           goto out;
> > > -                   }
> > > -           }
> > > -   }
> > > -
> > > -out:
> > > -   if (opt) {
> > > -           for (i = 0; i < count; i++)
> > > -                   free(opt[i].lo);
> > > -   }
> > > -   free(opt);
> > > -   efi_free_pool(volume_handles);
> > > -
> > > -   return ret;
> > > -}
> > > -
> > >   /**
> > >    * eficonfig_init() - do required initialization for eficonfig
> command
> > >    *
> > > @@ -2709,7 +2313,7 @@ static int do_eficonfig(struct cmd_tbl *cmdtp,
> int flag, int argc, char *const a
> > >     if (ret != EFI_SUCCESS)
> > >             return CMD_RET_FAILURE;
> > >
> > > -   ret = eficonfig_generate_media_device_boot_option();
> > > +   ret = efi_bootmgr_update_media_device_boot_option();
> > >     if (ret != EFI_SUCCESS && ret != EFI_NOT_FOUND)
> > >             return ret;
> > >
> > > diff --git a/include/efi_config.h b/include/efi_config.h
> > > index 01ce9b2b06..d7c1601137 100644
> > > --- a/include/efi_config.h
> > > +++ b/include/efi_config.h
> > > @@ -105,11 +105,6 @@ efi_status_t eficonfig_process_common(struct
> efimenu *efi_menu,
> > >                                   void (*item_data_print)(void *),
> > >                                   char *(*item_choice)(void *));
> > >   efi_status_t eficonfig_process_select_file(void *data);
> > > -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 eficonfig_generate_media_device_boot_option(void);
> > > -
> > >   efi_status_t eficonfig_append_menu_entry(struct efimenu *efi_menu,
> > >                                      char *title, eficonfig_entry_func
> func,
> > >                                      void *data);
> > > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > > index 1542b4b625..31ca1f5d1d 100644
> > > --- a/include/efi_loader.h
> > > +++ b/include/efi_loader.h
> > > @@ -516,6 +516,17 @@ extern struct list_head
> efi_register_notify_events;
> > >   int efi_init_early(void);
> > >   /* Initialize efi execution environment */
> > >   efi_status_t efi_init_obj_list(void);
> > > +/* Append new boot option in BootOrder variable */
> > > +efi_status_t efi_bootmgr_append_bootorder(u16 index);
> > > +/* Get unused "Boot####" index */
> > > +efi_status_t efi_bootmgr_get_unused_bootoption(u16 *buf,
> > > +                                          efi_uintn_t buf_size, u32
> *index);
> > > +/* Generate the media device boot option */
> > > +efi_status_t efi_bootmgr_update_media_device_boot_option(void);
> > > +/* Delete selected boot option */
> > > +efi_status_t efi_bootmgr_delete_boot_option(u16 boot_index);
> > > +/* search the boot option index in BootOrder */
> > > +bool efi_search_bootorder(u16 *bootorder, efi_uintn_t num, u32
> target, u32 *index);
> > >   /* Set up console modes */
> > >   void efi_setup_console_size(void);
> > >   /* Install device tree */
> > > diff --git a/lib/efi_loader/efi_bootmgr.c
> b/lib/efi_loader/efi_bootmgr.c
> > > index 4b24b41047..c329428973 100644
> > > --- a/lib/efi_loader/efi_bootmgr.c
> > > +++ b/lib/efi_loader/efi_bootmgr.c
> > > @@ -347,3 +347,383 @@ efi_status_t efi_bootmgr_load(efi_handle_t
> *handle, void **load_options)
> > >   error:
> > >     return ret;
> > >   }
> > > +
> > > +/**
> > > + * efi_bootmgr_enumerate_boot_option() - enumerate the possible
> bootable media
> > > + *
> > > + * @opt:           pointer to the media boot option structure
> > > + * @volume_handles:        pointer to the efi handles
> > > + * @count:         number of efi handle
> > > + * Return:         status code
> > > + */
> > > +static efi_status_t efi_bootmgr_enumerate_boot_option(struct
> eficonfig_media_boot_option *opt,
> > > +                                                 efi_handle_t
> *volume_handles,
> > > +                                                 efi_status_t count)
> > > +{
> > > +   u32 i;
> > > +   struct efi_handler *handler;
> > > +   efi_status_t ret = EFI_SUCCESS;
> > > +
> > > +   for (i = 0; i < count; i++) {
> > > +           u16 *p;
> > > +           u16 dev_name[BOOTMENU_DEVICE_NAME_MAX];
> > > +           char *optional_data;
> > > +           struct efi_load_option lo;
> > > +           char buf[BOOTMENU_DEVICE_NAME_MAX];
> > > +           struct efi_device_path *device_path;
> > > +
> > > +           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_disk_get_device_name(volume_handles[i], buf,
> BOOTMENU_DEVICE_NAME_MAX);
> > > +           if (ret != EFI_SUCCESS)
> > > +                   continue;
> > > +
> > > +           p = dev_name;
> > > +           utf8_utf16_strncpy(&p, buf, strlen(buf));
> > > +
> > > +           lo.label = dev_name;
> > > +           lo.attributes = LOAD_OPTION_ACTIVE;
> > > +           lo.file_path = device_path;
> > > +           lo.file_path_length = efi_dp_size(device_path) +
> sizeof(END);
> > > +           /*
> > > +            * Set the dedicated guid to optional_data, it is used to
> identify
> > > +            * the boot option that automatically generated by the
> bootmenu.
> > > +            * efi_serialize_load_option() expects optional_data is
> null-terminated
> > > +            * utf8 string, so set the "1234567" string to allocate
> enough space
> > > +            * to store guid, instead of realloc the load_option.
> > > +            */
> > > +           lo.optional_data = "1234567";
> > > +           opt[i].size = efi_serialize_load_option(&lo, (u8
> **)&opt[i].lo);
> > > +           if (!opt[i].size) {
> > > +                   ret = EFI_OUT_OF_RESOURCES;
> > > +                   goto out;
> > > +           }
> > > +           /* set the guid */
> > > +           optional_data = (char *)opt[i].lo + (opt[i].size -
> u16_strsize(u"1234567"));
> > > +           memcpy(optional_data, &efi_guid_bootmenu_auto_generated,
> sizeof(efi_guid_t));
> > > +   }
> > > +
> > > +out:
> > > +   return ret;
> > > +}
> > > +
> > > +/**
> > > + * efi_bootmgr_delete_invalid_boot_option() - delete non-existing
> boot option
> >
> > You can't delete what does not exist.

The functions (efi_bootmgr_delete_invalid_boot_option,
efi_bootmgr_delete_invalid_boot_option,
efi_bootmgr_get_unused_bootoption, efi_search_bootorder, etc.) have no
changes with this patch.
I just rename and move them from eficonfig to efibootmgr.
Do we target to refactor all of them with this patch?
- Raymond Mao

> >
> > > + *
> > > + * @opt:           pointer to the media boot option structure
> > > + * @count:         number of media boot option structure
> >
> > Why would you need such a parameter?
> >
> > > + * Return:         status code
> > > + */
> > > +static efi_status_t efi_bootmgr_delete_invalid_boot_option(struct
> eficonfig_media_boot_option *opt,
> > > +                                                      efi_status_t
> count)
> >
> > Why don't you pass in the boot option num
> >
> > > +{
> > > +   efi_uintn_t size;
> > > +   void *load_option;
> > > +   u32 i, list_size = 0;
> > > +   struct efi_load_option lo;
> > > +   u16 *var_name16 = NULL;
> > > +   u16 varname[] = u"Boot####";
> >
> > Why would you initialize the variable?
> >
> > > +   efi_status_t ret = EFI_SUCCESS;
> > > +   u16 *delete_index_list = NULL, *p;
> > > +   efi_uintn_t buf_size;
> > > +
> > > +   buf_size = 128;
> > > +   var_name16 = malloc(buf_size);
> > > +   if (!var_name16)
> > > +           return EFI_OUT_OF_RESOURCES;
> > > +
> > > +   var_name16[0] = 0;
> > > +   for (;;) {
> > > +           int index;
> > > +           efi_guid_t guid;
> > > +           efi_uintn_t tmp;
> > > +
> > > +           ret = efi_next_variable_name(&buf_size, &var_name16,
> &guid);
> >
> > Can't we simply put the boot option number into struct
> > eficonfig_media_boot_option and save 100 lines code?
> >
> > > +           if (ret == EFI_NOT_FOUND) {
> > > +                   log_debug("last efi var\n");
> > > +                   /*
> > > +                    * EFI_NOT_FOUND indicates we retrieved all EFI
> variables.
> > > +                    * This should be treated as success.
> > > +                    */
> > > +                   ret = EFI_SUCCESS;
> > > +                   break;
> > > +           }
> > > +
> > > +           if (ret != EFI_SUCCESS)
> > > +                   goto out;
> > > +
> > > +           if (!efi_varname_is_load_option(var_name16, &index))
> > > +                   continue;
> > > +
> > > +           efi_create_indexed_name(varname, sizeof(varname), "Boot",
> index);
> > > +           load_option = efi_get_var(varname,
> &efi_global_variable_guid, &size);
> > > +           if (!load_option)
> > > +                   continue;
> > > +
> > > +           tmp = size;
> > > +           ret = efi_deserialize_load_option(&lo, load_option, &size);
> > > +           if (ret != EFI_SUCCESS)
> > > +                   goto next;
> > > +
> > > +           if (size >= sizeof(efi_guid_bootmenu_auto_generated) &&
> > > +               !guidcmp(lo.optional_data,
> &efi_guid_bootmenu_auto_generated)) {
> > > +                   for (i = 0; i < count; i++) {
> > > +                           if (opt[i].size == tmp &&
> > > +                               memcmp(opt[i].lo, load_option, tmp) ==
> 0) {
> > > +                                   opt[i].exist = true;
> > > +                                   break;
> > > +                           }
> > > +                   }
> > > +
> > > +                   /*
> > > +                    * The entire list of variables must be retrieved
> by
> > > +                    * efi_get_next_variable_name_int() before
> deleting the invalid
> > > +                    * boot option, just save the index here.
> > > +                    */
> > > +                   if (i == count) {
> > > +                           log_debug("found index %d can be
> deleted\n", index);
> > > +                           p = realloc(delete_index_list, sizeof(u32)
> *
> > > +                                       (list_size + 1));
> > > +                           if (!p) {
> > > +                                   ret = EFI_OUT_OF_RESOURCES;
> > > +                                   goto out;
> > > +                           }
> > > +                           delete_index_list = p;
> > > +                           delete_index_list[list_size++] = index;
> > > +                   }
> > > +           }
> > > +next:
> > > +           free(load_option);
> > > +   }
> > > +
> > > +   /* delete all invalid boot options */
> > > +   for (i = 0; i < list_size; i++) {
> > > +           log_debug("deleting index %d\n", delete_index_list[i]);
> > > +           ret = efi_bootmgr_delete_boot_option(delete_index_list[i]);
> > > +           if (ret != EFI_SUCCESS)
> > > +                   goto out;
> > > +   }
> > > +
> > > +out:
> > > +   free(var_name16);
> > > +   free(delete_index_list);
> > > +
> > > +   return ret;
> > > +}
> > > +
> > > +/**
> > > + * efi_bootmgr_get_unused_bootoption() - get unused "Boot####" index
> > > + *
> > > + * @buf:   pointer to the buffer to store boot option variable name
> > > + * @buf_size:      buffer size
> > > + * @index: pointer to store the index in the BootOrder variable
> > > + * Return: status code
> > > + */
> > > +efi_status_t efi_bootmgr_get_unused_bootoption(u16 *buf, efi_uintn_t
> buf_size,
> > > +                                          unsigned int *index)
> >
> > This function should be static. The exported functions should not take
> > the inddex in the BootOrder variable as a parameter.
> >
> > > +{
> > > +   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;
> > > +}
> > > +
> > > +/**
> > > + * efi_bootmgr_append_bootorder() - append new boot option in
> BootOrder variable
> > > + *
> > > + * @index: "Boot####" index to append to BootOrder variable
> > > + * Return: status code
> > > + */
> > > +efi_status_t efi_bootmgr_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;
> > > +}
> > > +
> > > +/**
> > > + * efi_bootmgr_delete_boot_option() - delete selected boot option
> > > + *
> > > + * @boot_index:    boot option index to delete
> > > + * Return: status code
> > > + */
> > > +efi_status_t efi_bootmgr_delete_boot_option(u16 boot_index)
> > > +{
> > > +   u16 *bootorder;
> > > +   u16 varname[9];
> > > +   efi_status_t ret;
> > > +   unsigned int index;
> > > +   efi_uintn_t num, size;
> > > +
> > > +   efi_create_indexed_name(varname, sizeof(varname),
> > > +                           "Boot", boot_index);
> > > +   ret = efi_set_variable_int(varname, &efi_global_variable_guid,
> > > +                              0, 0, NULL, false);
> > > +   if (ret != EFI_SUCCESS) {
> > > +           log_err("delete boot option(%ls) failed\n", varname);
> > > +           return ret;
> > > +   }
> > > +
> > > +   /* update BootOrder if necessary */
> > > +   bootorder = efi_get_var(u"BootOrder", &efi_global_variable_guid,
> &size);
> > > +   if (!bootorder)
> > > +           return EFI_SUCCESS;
> > > +
> > > +   num = size / sizeof(u16);
> > > +   if (!efi_search_bootorder(bootorder, num, boot_index, &index))
> > > +           return EFI_SUCCESS;
> > > +
> > > +   memmove(&bootorder[index], &bootorder[index + 1],
> > > +           (num - index - 1) * sizeof(u16));
> > > +   size -= sizeof(u16);
> > > +   ret = efi_set_variable_int(u"BootOrder", &efi_global_variable_guid,
> > > +                              EFI_VARIABLE_NON_VOLATILE |
> > > +                              EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > > +                              EFI_VARIABLE_RUNTIME_ACCESS,
> > > +                              size, bootorder, false);
> > > +
> > > +   return ret;
> > > +}
> > > +
> > > +/**
> > > + * efi_bootmgr_update_media_device_boot_option() - generate the media
> device boot option
> > > + *
> > > + * This function enumerates all devices supporting
> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL
> > > + * and generate the bootmenu entries.
> > > + * This function also provide the BOOT#### variable maintenance for
> > > + * the media device entries.
> > > + *   - Automatically create the BOOT#### variable for the newly
> detected device,
> > > + *     this BOOT#### variable is distinguished by the special GUID
> > > + *     stored in the EFI_LOAD_OPTION.optional_data
> > > + *   - If the device is not attached to the system, the associated
> BOOT#### variable
> > > + *     is automatically deleted.
> > > + *
> > > + * Return: status code
> > > + */
> > > +efi_status_t efi_bootmgr_update_media_device_boot_option(void)
> > > +{
> > > +   u32 i;
> > > +   efi_status_t ret;
> > > +   efi_uintn_t count;
> > > +   efi_handle_t *volume_handles = NULL;
> > > +   struct eficonfig_media_boot_option *opt = NULL;
> > > +
> > > +   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)
> > > +           return ret;
> > > +
> > > +   opt = calloc(count, sizeof(struct eficonfig_media_boot_option));
> > > +   if (!opt)
> > > +           goto out;
> > > +
> > > +   /* enumerate all devices supporting
> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL */
> > > +   ret = efi_bootmgr_enumerate_boot_option(opt, volume_handles,
> count);
> > > +   if (ret != EFI_SUCCESS)
> > > +           goto out;
> > > +
> > > +   /*
> > > +    * System hardware configuration may vary depending on the user
> setup.
> > > +    * The boot option is automatically added by the bootmenu.
> > > +    * If the device is not attached to the system, the boot option
> needs
> > > +    * to be deleted.
> > > +    */
> > > +   ret = efi_bootmgr_delete_invalid_boot_option(opt, count);
> > > +   if (ret != EFI_SUCCESS)
> > > +           goto out;
> > > +
> > > +   /* add non-existent boot option */
> > > +   for (i = 0; i < count; i++) {
> > > +           u32 boot_index;
> > > +           u16 var_name[9];
> > > +
> > > +           if (!opt[i].exist) {
> > > +                   ret = efi_bootmgr_get_unused_bootoption(var_name,
> sizeof(var_name),
> > > +
>  &boot_index);
> > > +                   if (ret != EFI_SUCCESS)
> > > +                           goto out;
> > > +
> > > +                   ret = efi_set_variable_int(var_name,
> &efi_global_variable_guid,
> > > +
> EFI_VARIABLE_NON_VOLATILE |
> > > +
> EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > > +
> EFI_VARIABLE_RUNTIME_ACCESS,
> > > +                                              opt[i].size, opt[i].lo,
> false);
> > > +                   if (ret != EFI_SUCCESS)
> > > +                           goto out;
> > > +
> > > +                   ret = efi_bootmgr_append_bootorder(boot_index);
> > > +                   if (ret != EFI_SUCCESS) {
> > > +                           efi_set_variable_int(var_name,
> &efi_global_variable_guid,
> > > +                                                0, 0, NULL, false);
> > > +                           goto out;
> > > +                   }
> > > +           }
> > > +   }
> > > +
> > > +out:
> > > +   if (opt) {
> > > +           for (i = 0; i < count; i++) {
> > > +                   if (opt[i].lo)
> > > +                           free(opt[i].lo);
> > > +           }
> > > +           free(opt);
> > > +   }
> > > +   efi_free_pool(volume_handles);
> > > +
> > > +   return ret;
> > > +}
> > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> > > index d2256713a8..1309e28134 100644
> > > --- a/lib/efi_loader/efi_disk.c
> > > +++ b/lib/efi_loader/efi_disk.c
> > > @@ -687,6 +687,13 @@ int efi_disk_probe(void *ctx, struct event *event)
> > >                     return -1;
> > >     }
> > >
> > > +   /* only do the boot option management when UEFI sub-system is
> initialized */
> > > +   if (efi_obj_list_initialized == EFI_SUCCESS) {
> > > +           ret = efi_bootmgr_update_media_device_boot_option();
> >
> > Why should we rescan all devices if a single device is added?
>
> Then, don't forget to call the 'remove' function in efi_disk_remove()
> (or _delete_raw() and _delete_part()).

The disk remove event is not triggered when a removable disk is unplugged,
so adding the
calling of the 'remove' function in efi_disk_remove() cannot meet our
expectation.
Other than that, there is an issue with fTPM by doing that.
The efi_disk_remove() function was called to uninit the devices after ramfs
was mounted.
Calling the 'remove' function here gets a side effect that the optee-ftpm
doesn't close the
ta session properly and then gets an error when it tries to open it after
kernel starts.
Logs like below:
optee-ftpm optee-ta-bc50d971-d4c9-42c4-82cb-343fb7f37896: ftpm_tee_probe:
tee_client_open_session failed, err=ffff000d
optee-ftpm: probe of optee-ta-bc50d971-d4c9-42c4-82cb-343fb7f37896 failed
with error -22

- Raymond Mao


>
> The only place where we need the list of removable media devices is when
> > executing the boot manager.
>
> Not sure what you mean, but we may want to confirm available boot options
> with efidebug or eficonfig before invoking the boot manager.
>
> -Takahiro Akashi
>
> >
> > > +           if (ret)
> > > +                   return -1;
> > > +   }
> > > +
> > >     return 0;
> > >   }
> > >
> > > diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
> > > index 1f4ab2b419..cdfd16ea77 100644
> > > --- a/lib/efi_loader/efi_helper.c
> > > +++ b/lib/efi_loader/efi_helper.c
> > > @@ -257,3 +257,28 @@ efi_status_t efi_next_variable_name(efi_uintn_t
> *size, u16 **buf, efi_guid_t *gu
> > >
> > >     return ret;
> > >   }
> > > +
> > > +/**
> > > + * efi_search_bootorder() - search the boot option index in BootOrder
> > > + *
> > > + * @bootorder:     pointer to the BootOrder variable
> > > + * @num:   number of BootOrder entry
> > > + * @target:        target boot option index to search
> > > + * @index: pointer to store the index of BootOrder variable
> > > + * Return: true if exists, false otherwise
> > > + */
> > > +bool efi_search_bootorder(u16 *bootorder, efi_uintn_t num, u32
> target, u32 *index)
> >
> > This function should be static. The exported function to delete a boot
> > option should not use the index in the boot order in their interface but
> > the number encoded in the variable name.
> >
> > We can simplify the function:
> >
> > Return -1 if the entry is not found and the index otherwise.
> >
> > > +{
> > > +   u32 i;
> > > +
> > > +   for (i = 0; i < num; i++) {
> > > +           if (target == bootorder[i]) {
> > > +                   if (index)
> > > +                           *index = i;
> > > +
> > > +                   return true;
> > > +           }
> > > +   }
> > > +
> > > +   return false;
> > > +}
> > > diff --git a/lib/efi_loader/efi_variable.c
> b/lib/efi_loader/efi_variable.c
> > > index be95ed44e6..2f251553e1 100644
> > > --- a/lib/efi_loader/efi_variable.c
> > > +++ b/lib/efi_loader/efi_variable.c
> > > @@ -476,6 +476,10 @@ efi_status_t efi_init_variables(void)
> > >                     log_err("Invalid EFI variable seed\n");
> > >     }
> > >
> > > +   ret = efi_init_secure_state();
> > > +   if (ret != EFI_SUCCESS)
> > > +           return ret;
> > >
> > > -   return efi_init_secure_state();
> > > +   /* update boot option management after variable service
> initialized */
> > > +   return efi_bootmgr_update_media_device_boot_option();
> >
> > The right place to call the function might be efi_init_obj_list().
> >
> > Best regards
> >
> > Heinrich
> >
> > >   }
> > > diff --git a/lib/efi_loader/efi_variable_tee.c
> b/lib/efi_loader/efi_variable_tee.c
> > > index dfef18435d..a48d313ef0 100644
> > > --- a/lib/efi_loader/efi_variable_tee.c
> > > +++ b/lib/efi_loader/efi_variable_tee.c
> > > @@ -748,5 +748,6 @@ efi_status_t efi_init_variables(void)
> > >     if (ret != EFI_SUCCESS)
> > >             return ret;
> > >
> > > -   return EFI_SUCCESS;
> > > +   /* update boot option management after variable service
> initialized */
> > > +   return efi_bootmgr_update_media_device_boot_option();
> > >   }
> >
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/2] Boot var automatic management for removable medias
  2023-04-17 14:22     ` Raymond Mao
@ 2023-04-18  0:50       ` AKASHI Takahiro
  0 siblings, 0 replies; 6+ messages in thread
From: AKASHI Takahiro @ 2023-04-18  0:50 UTC (permalink / raw)
  To: Raymond Mao
  Cc: Heinrich Schuchardt, Ilias Apalodimas, Masahisa Kojima,
	Simon Glass, u-boot

On Mon, Apr 17, 2023 at 10:22:24AM -0400, Raymond Mao wrote:
> On Thu, 13 Apr 2023 at 04:19, AKASHI Takahiro <takahiro.akashi@linaro.org>
> wrote:
> 
> > On Fri, Apr 07, 2023 at 12:13:36PM +0200, Heinrich Schuchardt wrote:
> > > On 4/5/23 02:06, Raymond Mao wrote:
> > > > Changes for complying to EFI spec §3.5.1.1
> > > > 'Removable Media Boot Behavior'.
> > > > Boot variables can be automatically generated during a removable
> > > > media is probed. At the same time, unused boot variables will be
> > > > detected and removed.
> > > > Related APIs are renamed and moved from cmd to lib for re-use
> > > > between eficonfig and bootmgr.
> > > >
> > > > Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
> > >
> > > We should split the patch into moving and renaming functions and
> > > individual patches for each changed functionality.
> > >
> > > > ---
> > > > Changes in v2
> > > > - Ignore EFI_NOT_FOUND returned from
> > > >    efi_bootmgr_update_media_device_boot_option which means no boot
> > > >    options scanned.
> > > >
> > > >   cmd/bootmenu.c                    |   2 +-
> > > >   cmd/eficonfig.c                   | 408
> > +-----------------------------
> > > >   include/efi_config.h              |   5 -
> > > >   include/efi_loader.h              |  11 +
> > > >   lib/efi_loader/efi_bootmgr.c      | 380 ++++++++++++++++++++++++++++
> > > >   lib/efi_loader/efi_disk.c         |   7 +
> > > >   lib/efi_loader/efi_helper.c       |  25 ++
> > > >   lib/efi_loader/efi_variable.c     |   6 +-
> > > >   lib/efi_loader/efi_variable_tee.c |   3 +-
> > > >   9 files changed, 437 insertions(+), 410 deletions(-)
> > > >
> > > > diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
> > > > index 6baeedc69f..01daddca7b 100644
> > > > --- a/cmd/bootmenu.c
> > > > +++ b/cmd/bootmenu.c
> > > > @@ -351,7 +351,7 @@ static struct bootmenu_data *bootmenu_create(int
> > delay)
> > > >              * UEFI specification requires booting from removal media
> > using
> > > >              * a architecture-specific default image name such as
> > BOOTAA64.EFI.
> > > >              */
> > > > -           efi_ret = eficonfig_generate_media_device_boot_option();
> > > > +           efi_ret = efi_bootmgr_update_media_device_boot_option();
> > > >             if (efi_ret != EFI_SUCCESS && efi_ret != EFI_NOT_FOUND)
> > > >                     goto cleanup;
> > > >
> > > > diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> > > > index 720f52b48b..82a80306f4 100644
> > > > --- a/cmd/eficonfig.c
> > > > +++ b/cmd/eficonfig.c
> > > > @@ -1134,43 +1134,6 @@ out:
> > > >     return ret;
> > > >   }
> > > >
> > > > -/**
> > > > - * eficonfig_get_unused_bootoption() - get unused "Boot####" index
> > > > - *
> > > > - * @buf:   pointer to the buffer to store boot option variable name
> > > > - * @buf_size:      buffer size
> > > > - * @index: pointer to store the index in the BootOrder variable
> > > > - * Return: status code
> > > > - */
> > > > -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;
> > > > -}
> > > > -
> > > >   /**
> > > >    * eficonfig_set_boot_option() - set boot option
> > > >    *
> > > > @@ -1208,46 +1171,6 @@ static efi_status_t
> > eficonfig_set_boot_option(u16 *varname, struct efi_device_pa
> > > >     return ret;
> > > >   }
> > > >
> > > > -/**
> > > > - * eficonfig_append_bootorder() - append new boot option in BootOrder
> > variable
> > > > - *
> > > > - * @index: "Boot####" index to append to BootOrder variable
> > > > - * Return: status code
> > > > - */
> > > > -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;
> > > > -}
> > > > -
> > > >   /**
> > > >    * create_boot_option_entry() - create boot option entry
> > > >    *
> > > > @@ -1619,7 +1542,7 @@ static efi_status_t
> > eficonfig_process_add_boot_option(void *data)
> > > >     if (!bo)
> > > >             return EFI_OUT_OF_RESOURCES;
> > > >
> > > > -   ret = eficonfig_get_unused_bootoption(varname, sizeof(varname),
> > &bo->boot_index);
> > > > +   ret = efi_bootmgr_get_unused_bootoption(varname, sizeof(varname),
> > &bo->boot_index);
> > > >     if (ret != EFI_SUCCESS)
> > > >             return ret;
> > > >
> > > > @@ -1627,7 +1550,7 @@ static efi_status_t
> > eficonfig_process_add_boot_option(void *data)
> > > >     if (ret != EFI_SUCCESS)
> > > >             goto out;
> > > >
> > > > -   ret = eficonfig_append_bootorder((u16)bo->boot_index);
> > > > +   ret = efi_bootmgr_append_bootorder((u16)bo->boot_index);
> > > >     if (ret != EFI_SUCCESS)
> > > >             goto out;
> > > >
> > > > @@ -1656,31 +1579,6 @@ static efi_status_t
> > eficonfig_process_boot_selected(void *data)
> > > >     return EFI_SUCCESS;
> > > >   }
> > > >
> > > > -/**
> > > > - * search_bootorder() - search the boot option index in BootOrder
> > > > - *
> > > > - * @bootorder:     pointer to the BootOrder variable
> > > > - * @num:   number of BootOrder entry
> > > > - * @target:        target boot option index to search
> > > > - * @index: pointer to store the index of BootOrder variable
> > > > - * Return: true if exists, false otherwise
> > > > - */
> > > > -static bool search_bootorder(u16 *bootorder, efi_uintn_t num, u32
> > target, u32 *index)
> > > > -{
> > > > -   u32 i;
> > > > -
> > > > -   for (i = 0; i < num; i++) {
> > > > -           if (target == bootorder[i]) {
> > > > -                   if (index)
> > > > -                           *index = i;
> > > > -
> > > > -                   return true;
> > > > -           }
> > > > -   }
> > > > -
> > > > -   return false;
> > > > -}
> > > > -
> > > >   /**
> > > >    * eficonfig_add_boot_selection_entry() - add boot option menu entry
> > > >    *
> > > > @@ -1805,7 +1703,7 @@ static efi_status_t
> > eficonfig_show_boot_selection(unsigned int *selected)
> > > >
> > > >             if (efi_varname_is_load_option(var_name16, &index)) {
> > > >                     /* If the index is included in the BootOrder, skip
> > it */
> > > > -                   if (search_bootorder(bootorder, num, index, NULL))
> > > > +                   if (efi_search_bootorder(bootorder, num, index,
> > NULL))
> > > >                             continue;
> > > >
> > > >                     ret = eficonfig_add_boot_selection_entry(efi_menu,
> > index, selected);
> > > > @@ -2202,7 +2100,7 @@ static efi_status_t
> > eficonfig_create_change_boot_order_entry(struct efimenu *efi
> > > >
> > > >             if (efi_varname_is_load_option(var_name16, &index)) {
> > > >                     /* If the index is included in the BootOrder, skip
> > it */
> > > > -                   if (search_bootorder(bootorder, num, index, NULL))
> > > > +                   if (efi_search_bootorder(bootorder, num, index,
> > NULL))
> > > >                             continue;
> > > >
> > > >                     ret =
> > eficonfig_add_change_boot_order_entry(efi_menu, index, false);
> > > > @@ -2304,50 +2202,6 @@ out:
> > > >     return ret;
> > > >   }
> > > >
> > > > -/**
> > > > - * delete_boot_option() - delete selected boot option
> > > > - *
> > > > - * @boot_index:    boot option index to delete
> > > > - * Return: status code
> > > > - */
> > > > -static efi_status_t delete_boot_option(u16 boot_index)
> > > > -{
> > > > -   u16 *bootorder;
> > > > -   u16 varname[9];
> > > > -   efi_status_t ret;
> > > > -   unsigned int index;
> > > > -   efi_uintn_t num, size;
> > > > -
> > > > -   efi_create_indexed_name(varname, sizeof(varname),
> > > > -                           "Boot", boot_index);
> > > > -   ret = efi_set_variable_int(varname, &efi_global_variable_guid,
> > > > -                              0, 0, NULL, false);
> > > > -   if (ret != EFI_SUCCESS) {
> > > > -           log_err("delete boot option(%ls) failed\n", varname);
> > > > -           return ret;
> > > > -   }
> > > > -
> > > > -   /* update BootOrder if necessary */
> > > > -   bootorder = efi_get_var(u"BootOrder", &efi_global_variable_guid,
> > &size);
> > > > -   if (!bootorder)
> > > > -           return EFI_SUCCESS;
> > > > -
> > > > -   num = size / sizeof(u16);
> > > > -   if (!search_bootorder(bootorder, num, boot_index, &index))
> > > > -           return EFI_SUCCESS;
> > > > -
> > > > -   memmove(&bootorder[index], &bootorder[index + 1],
> > > > -           (num - index - 1) * sizeof(u16));
> > > > -   size -= sizeof(u16);
> > > > -   ret = efi_set_variable_int(u"BootOrder", &efi_global_variable_guid,
> > > > -                              EFI_VARIABLE_NON_VOLATILE |
> > > > -                              EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > > > -                              EFI_VARIABLE_RUNTIME_ACCESS,
> > > > -                              size, bootorder, false);
> > > > -
> > > > -   return ret;
> > > > -}
> > > > -
> > > >   /**
> > > >    * eficonfig_process_delete_boot_option() - handler to delete boot
> > option
> > > >    *
> > > > @@ -2362,7 +2216,7 @@ static efi_status_t
> > eficonfig_process_delete_boot_option(void *data)
> > > >     while (1) {
> > > >             ret = eficonfig_show_boot_selection(&selected);
> > > >             if (ret == EFI_SUCCESS)
> > > > -                   ret = delete_boot_option(selected);
> > > > +                   ret = efi_bootmgr_delete_boot_option(selected);
> > >
> > > Please, pass the number in the Boot#### variable and not an index in
> > > BootOrder.
> > >
> > > >
> > > >             if (ret != EFI_SUCCESS)
> > > >                     break;
> > > > @@ -2374,256 +2228,6 @@ static efi_status_t
> > eficonfig_process_delete_boot_option(void *data)
> > > >     return ret;
> > > >   }
> > > >
> > > > -/**
> > > > - * eficonfig_enumerate_boot_option() - enumerate the possible
> > bootable media
> > > > - *
> > > > - * @opt:           pointer to the media boot option structure
> > > > - * @volume_handles:        pointer to the efi handles
> > > > - * @count:         number of efi handle
> > > > - * Return:         status code
> > > > - */
> > > > -efi_status_t eficonfig_enumerate_boot_option(struct
> > eficonfig_media_boot_option *opt,
> > > > -                                        efi_handle_t *volume_handles,
> > efi_status_t count)
> > > > -{
> > > > -   u32 i;
> > > > -   struct efi_handler *handler;
> > > > -   efi_status_t ret = EFI_SUCCESS;
> > > > -
> > > > -   for (i = 0; i < count; i++) {
> > > > -           u16 *p;
> > > > -           u16 dev_name[BOOTMENU_DEVICE_NAME_MAX];
> > > > -           char *optional_data;
> > > > -           struct efi_load_option lo;
> > > > -           char buf[BOOTMENU_DEVICE_NAME_MAX];
> > > > -           struct efi_device_path *device_path;
> > > > -
> > > > -           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_disk_get_device_name(volume_handles[i], buf,
> > BOOTMENU_DEVICE_NAME_MAX);
> > > > -           if (ret != EFI_SUCCESS)
> > > > -                   continue;
> > > > -
> > > > -           p = dev_name;
> > > > -           utf8_utf16_strncpy(&p, buf, strlen(buf));
> > > > -
> > > > -           lo.label = dev_name;
> > > > -           lo.attributes = LOAD_OPTION_ACTIVE;
> > > > -           lo.file_path = device_path;
> > > > -           lo.file_path_length = efi_dp_size(device_path) +
> > sizeof(END);
> > > > -           /*
> > > > -            * Set the dedicated guid to optional_data, it is used to
> > identify
> > > > -            * the boot option that automatically generated by the
> > bootmenu.
> > > > -            * efi_serialize_load_option() expects optional_data is
> > null-terminated
> > > > -            * utf8 string, so set the "1234567" string to allocate
> > enough space
> > > > -            * to store guid, instead of realloc the load_option.
> > > > -            */
> > > > -           lo.optional_data = "1234567";
> > > > -           opt[i].size = efi_serialize_load_option(&lo, (u8
> > **)&opt[i].lo);
> > > > -           if (!opt[i].size) {
> > > > -                   ret = EFI_OUT_OF_RESOURCES;
> > > > -                   goto out;
> > > > -           }
> > > > -           /* set the guid */
> > > > -           optional_data = (char *)opt[i].lo + (opt[i].size -
> > u16_strsize(u"1234567"));
> > > > -           memcpy(optional_data, &efi_guid_bootmenu_auto_generated,
> > sizeof(efi_guid_t));
> > > > -   }
> > > > -
> > > > -out:
> > > > -   return ret;
> > > > -}
> > > > -
> > > > -/**
> > > > - * eficonfig_delete_invalid_boot_option() - delete non-existing boot
> > option
> > > > - *
> > > > - * @opt:           pointer to the media boot option structure
> > > > - * @count:         number of media boot option structure
> > > > - * Return:         status code
> > > > - */
> > > > -efi_status_t eficonfig_delete_invalid_boot_option(struct
> > eficonfig_media_boot_option *opt,
> > > > -                                             efi_status_t count)
> > > > -{
> > > > -   efi_uintn_t size;
> > > > -   void *load_option;
> > > > -   u32 i, list_size = 0;
> > > > -   struct efi_load_option lo;
> > > > -   u16 *var_name16 = NULL;
> > > > -   u16 varname[] = u"Boot####";
> > > > -   efi_status_t ret = EFI_SUCCESS;
> > > > -   u16 *delete_index_list = NULL, *p;
> > > > -   efi_uintn_t buf_size;
> > > > -
> > > > -   buf_size = 128;
> > > > -   var_name16 = malloc(buf_size);
> > > > -   if (!var_name16)
> > > > -           return EFI_OUT_OF_RESOURCES;
> > > > -
> > > > -   var_name16[0] = 0;
> > > > -   for (;;) {
> > > > -           int index;
> > > > -           efi_guid_t guid;
> > > > -           efi_uintn_t tmp;
> > > > -
> > > > -           ret = efi_next_variable_name(&buf_size, &var_name16,
> > &guid);
> > > > -           if (ret == EFI_NOT_FOUND) {
> > > > -                   /*
> > > > -                    * EFI_NOT_FOUND indicates we retrieved all EFI
> > variables.
> > > > -                    * This should be treated as success.
> > > > -                    */
> > > > -                   ret = EFI_SUCCESS;
> > > > -                   break;
> > > > -           }
> > > > -           if (ret != EFI_SUCCESS)
> > > > -                   goto out;
> > > > -
> > > > -           if (!efi_varname_is_load_option(var_name16, &index))
> > > > -                   continue;
> > > > -
> > > > -           efi_create_indexed_name(varname, sizeof(varname), "Boot",
> > index);
> > > > -           load_option = efi_get_var(varname,
> > &efi_global_variable_guid, &size);
> > > > -           if (!load_option)
> > > > -                   continue;
> > > > -
> > > > -           tmp = size;
> > > > -           ret = efi_deserialize_load_option(&lo, load_option, &size);
> > > > -           if (ret != EFI_SUCCESS)
> > > > -                   goto next;
> > > > -
> > > > -           if (size >= sizeof(efi_guid_bootmenu_auto_generated) &&
> > > > -               !guidcmp(lo.optional_data,
> > &efi_guid_bootmenu_auto_generated)) {
> > > > -                   for (i = 0; i < count; i++) {
> > > > -                           if (opt[i].size == tmp &&
> > > > -                               memcmp(opt[i].lo, load_option, tmp) ==
> > 0) {
> > > > -                                   opt[i].exist = true;
> > > > -                                   break;
> > > > -                           }
> > > > -                   }
> > > > -
> > > > -                   /*
> > > > -                    * The entire list of variables must be retrieved
> > by
> > > > -                    * efi_get_next_variable_name_int() before
> > deleting the invalid
> > > > -                    * boot option, just save the index here.
> > > > -                    */
> > > > -                   if (i == count) {
> > > > -                           p = realloc(delete_index_list, sizeof(u32)
> > *
> > > > -                                       (list_size + 1));
> > > > -                           if (!p) {
> > > > -                                   ret = EFI_OUT_OF_RESOURCES;
> > > > -                                   goto out;
> > > > -                           }
> > > > -                           delete_index_list = p;
> > > > -                           delete_index_list[list_size++] = index;
> > > > -                   }
> > > > -           }
> > > > -next:
> > > > -           free(load_option);
> > > > -   }
> > > > -
> > > > -   /* delete all invalid boot options */
> > > > -   for (i = 0; i < list_size; i++) {
> > > > -           ret = delete_boot_option(delete_index_list[i]);
> > > > -           if (ret != EFI_SUCCESS)
> > > > -                   goto out;
> > > > -   }
> > > > -
> > > > -out:
> > > > -   free(var_name16);
> > > > -   free(delete_index_list);
> > > > -
> > > > -   return ret;
> > > > -}
> > > > -
> > > > -/**
> > > > - * eficonfig_generate_media_device_boot_option() - generate the media
> > device boot option
> > > > - *
> > > > - * This function enumerates all devices supporting
> > EFI_SIMPLE_FILE_SYSTEM_PROTOCOL
> > > > - * and generate the bootmenu entries.
> > > > - * This function also provide the BOOT#### variable maintenance for
> > > > - * the media device entries.
> > > > - *   - Automatically create the BOOT#### variable for the newly
> > detected device,
> > > > - *     this BOOT#### variable is distinguished by the special GUID
> > > > - *     stored in the EFI_LOAD_OPTION.optional_data
> > > > - *   - If the device is not attached to the system, the associated
> > BOOT#### variable
> > > > - *     is automatically deleted.
> > > > - *
> > > > - * Return: status code
> > > > - */
> > > > -efi_status_t eficonfig_generate_media_device_boot_option(void)
> > > > -{
> > > > -   u32 i;
> > > > -   efi_status_t ret;
> > > > -   efi_uintn_t count;
> > > > -   efi_handle_t *volume_handles = NULL;
> > > > -   struct eficonfig_media_boot_option *opt = NULL;
> > > > -
> > > > -   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)
> > > > -           return ret;
> > > > -
> > > > -   opt = calloc(count, sizeof(struct eficonfig_media_boot_option));
> > > > -   if (!opt)
> > > > -           goto out;
> > > > -
> > > > -   /* enumerate all devices supporting
> > EFI_SIMPLE_FILE_SYSTEM_PROTOCOL */
> > > > -   ret = eficonfig_enumerate_boot_option(opt, volume_handles, count);
> > > > -   if (ret != EFI_SUCCESS)
> > > > -           goto out;
> > > > -
> > > > -   /*
> > > > -    * System hardware configuration may vary depending on the user
> > setup.
> > > > -    * The boot option is automatically added by the bootmenu.
> > > > -    * If the device is not attached to the system, the boot option
> > needs
> > > > -    * to be deleted.
> > > > -    */
> > > > -   ret = eficonfig_delete_invalid_boot_option(opt, count);
> > > > -   if (ret != EFI_SUCCESS)
> > > > -           goto out;
> > > > -
> > > > -   /* add non-existent boot option */
> > > > -   for (i = 0; i < count; i++) {
> > > > -           u32 boot_index;
> > > > -           u16 var_name[9];
> > > > -
> > > > -           if (!opt[i].exist) {
> > > > -                   ret = eficonfig_get_unused_bootoption(var_name,
> > sizeof(var_name),
> > > > -                                                         &boot_index);
> > > > -                   if (ret != EFI_SUCCESS)
> > > > -                           goto out;
> > > > -
> > > > -                   ret = efi_set_variable_int(var_name,
> > &efi_global_variable_guid,
> > > > -
> > EFI_VARIABLE_NON_VOLATILE |
> > > > -
> > EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > > > -
> > EFI_VARIABLE_RUNTIME_ACCESS,
> > > > -                                              opt[i].size, opt[i].lo,
> > false);
> > > > -                   if (ret != EFI_SUCCESS)
> > > > -                           goto out;
> > > > -
> > > > -                   ret = eficonfig_append_bootorder(boot_index);
> > > > -                   if (ret != EFI_SUCCESS) {
> > > > -                           efi_set_variable_int(var_name,
> > &efi_global_variable_guid,
> > > > -                                                0, 0, NULL, false);
> > > > -                           goto out;
> > > > -                   }
> > > > -           }
> > > > -   }
> > > > -
> > > > -out:
> > > > -   if (opt) {
> > > > -           for (i = 0; i < count; i++)
> > > > -                   free(opt[i].lo);
> > > > -   }
> > > > -   free(opt);
> > > > -   efi_free_pool(volume_handles);
> > > > -
> > > > -   return ret;
> > > > -}
> > > > -
> > > >   /**
> > > >    * eficonfig_init() - do required initialization for eficonfig
> > command
> > > >    *
> > > > @@ -2709,7 +2313,7 @@ static int do_eficonfig(struct cmd_tbl *cmdtp,
> > int flag, int argc, char *const a
> > > >     if (ret != EFI_SUCCESS)
> > > >             return CMD_RET_FAILURE;
> > > >
> > > > -   ret = eficonfig_generate_media_device_boot_option();
> > > > +   ret = efi_bootmgr_update_media_device_boot_option();
> > > >     if (ret != EFI_SUCCESS && ret != EFI_NOT_FOUND)
> > > >             return ret;
> > > >
> > > > diff --git a/include/efi_config.h b/include/efi_config.h
> > > > index 01ce9b2b06..d7c1601137 100644
> > > > --- a/include/efi_config.h
> > > > +++ b/include/efi_config.h
> > > > @@ -105,11 +105,6 @@ efi_status_t eficonfig_process_common(struct
> > efimenu *efi_menu,
> > > >                                   void (*item_data_print)(void *),
> > > >                                   char *(*item_choice)(void *));
> > > >   efi_status_t eficonfig_process_select_file(void *data);
> > > > -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 eficonfig_generate_media_device_boot_option(void);
> > > > -
> > > >   efi_status_t eficonfig_append_menu_entry(struct efimenu *efi_menu,
> > > >                                      char *title, eficonfig_entry_func
> > func,
> > > >                                      void *data);
> > > > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > > > index 1542b4b625..31ca1f5d1d 100644
> > > > --- a/include/efi_loader.h
> > > > +++ b/include/efi_loader.h
> > > > @@ -516,6 +516,17 @@ extern struct list_head
> > efi_register_notify_events;
> > > >   int efi_init_early(void);
> > > >   /* Initialize efi execution environment */
> > > >   efi_status_t efi_init_obj_list(void);
> > > > +/* Append new boot option in BootOrder variable */
> > > > +efi_status_t efi_bootmgr_append_bootorder(u16 index);
> > > > +/* Get unused "Boot####" index */
> > > > +efi_status_t efi_bootmgr_get_unused_bootoption(u16 *buf,
> > > > +                                          efi_uintn_t buf_size, u32
> > *index);
> > > > +/* Generate the media device boot option */
> > > > +efi_status_t efi_bootmgr_update_media_device_boot_option(void);
> > > > +/* Delete selected boot option */
> > > > +efi_status_t efi_bootmgr_delete_boot_option(u16 boot_index);
> > > > +/* search the boot option index in BootOrder */
> > > > +bool efi_search_bootorder(u16 *bootorder, efi_uintn_t num, u32
> > target, u32 *index);
> > > >   /* Set up console modes */
> > > >   void efi_setup_console_size(void);
> > > >   /* Install device tree */
> > > > diff --git a/lib/efi_loader/efi_bootmgr.c
> > b/lib/efi_loader/efi_bootmgr.c
> > > > index 4b24b41047..c329428973 100644
> > > > --- a/lib/efi_loader/efi_bootmgr.c
> > > > +++ b/lib/efi_loader/efi_bootmgr.c
> > > > @@ -347,3 +347,383 @@ efi_status_t efi_bootmgr_load(efi_handle_t
> > *handle, void **load_options)
> > > >   error:
> > > >     return ret;
> > > >   }
> > > > +
> > > > +/**
> > > > + * efi_bootmgr_enumerate_boot_option() - enumerate the possible
> > bootable media
> > > > + *
> > > > + * @opt:           pointer to the media boot option structure
> > > > + * @volume_handles:        pointer to the efi handles
> > > > + * @count:         number of efi handle
> > > > + * Return:         status code
> > > > + */
> > > > +static efi_status_t efi_bootmgr_enumerate_boot_option(struct
> > eficonfig_media_boot_option *opt,
> > > > +                                                 efi_handle_t
> > *volume_handles,
> > > > +                                                 efi_status_t count)
> > > > +{
> > > > +   u32 i;
> > > > +   struct efi_handler *handler;
> > > > +   efi_status_t ret = EFI_SUCCESS;
> > > > +
> > > > +   for (i = 0; i < count; i++) {
> > > > +           u16 *p;
> > > > +           u16 dev_name[BOOTMENU_DEVICE_NAME_MAX];
> > > > +           char *optional_data;
> > > > +           struct efi_load_option lo;
> > > > +           char buf[BOOTMENU_DEVICE_NAME_MAX];
> > > > +           struct efi_device_path *device_path;
> > > > +
> > > > +           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_disk_get_device_name(volume_handles[i], buf,
> > BOOTMENU_DEVICE_NAME_MAX);
> > > > +           if (ret != EFI_SUCCESS)
> > > > +                   continue;
> > > > +
> > > > +           p = dev_name;
> > > > +           utf8_utf16_strncpy(&p, buf, strlen(buf));
> > > > +
> > > > +           lo.label = dev_name;
> > > > +           lo.attributes = LOAD_OPTION_ACTIVE;
> > > > +           lo.file_path = device_path;
> > > > +           lo.file_path_length = efi_dp_size(device_path) +
> > sizeof(END);
> > > > +           /*
> > > > +            * Set the dedicated guid to optional_data, it is used to
> > identify
> > > > +            * the boot option that automatically generated by the
> > bootmenu.
> > > > +            * efi_serialize_load_option() expects optional_data is
> > null-terminated
> > > > +            * utf8 string, so set the "1234567" string to allocate
> > enough space
> > > > +            * to store guid, instead of realloc the load_option.
> > > > +            */
> > > > +           lo.optional_data = "1234567";
> > > > +           opt[i].size = efi_serialize_load_option(&lo, (u8
> > **)&opt[i].lo);
> > > > +           if (!opt[i].size) {
> > > > +                   ret = EFI_OUT_OF_RESOURCES;
> > > > +                   goto out;
> > > > +           }
> > > > +           /* set the guid */
> > > > +           optional_data = (char *)opt[i].lo + (opt[i].size -
> > u16_strsize(u"1234567"));
> > > > +           memcpy(optional_data, &efi_guid_bootmenu_auto_generated,
> > sizeof(efi_guid_t));
> > > > +   }
> > > > +
> > > > +out:
> > > > +   return ret;
> > > > +}
> > > > +
> > > > +/**
> > > > + * efi_bootmgr_delete_invalid_boot_option() - delete non-existing
> > boot option
> > >
> > > You can't delete what does not exist.
> 
> The functions (efi_bootmgr_delete_invalid_boot_option,
> efi_bootmgr_delete_invalid_boot_option,
> efi_bootmgr_get_unused_bootoption, efi_search_bootorder, etc.) have no
> changes with this patch.
> I just rename and move them from eficonfig to efibootmgr.
> Do we target to refactor all of them with this patch?
> - Raymond Mao
> 
> > >
> > > > + *
> > > > + * @opt:           pointer to the media boot option structure
> > > > + * @count:         number of media boot option structure
> > >
> > > Why would you need such a parameter?
> > >
> > > > + * Return:         status code
> > > > + */
> > > > +static efi_status_t efi_bootmgr_delete_invalid_boot_option(struct
> > eficonfig_media_boot_option *opt,
> > > > +                                                      efi_status_t
> > count)
> > >
> > > Why don't you pass in the boot option num
> > >
> > > > +{
> > > > +   efi_uintn_t size;
> > > > +   void *load_option;
> > > > +   u32 i, list_size = 0;
> > > > +   struct efi_load_option lo;
> > > > +   u16 *var_name16 = NULL;
> > > > +   u16 varname[] = u"Boot####";
> > >
> > > Why would you initialize the variable?
> > >
> > > > +   efi_status_t ret = EFI_SUCCESS;
> > > > +   u16 *delete_index_list = NULL, *p;
> > > > +   efi_uintn_t buf_size;
> > > > +
> > > > +   buf_size = 128;
> > > > +   var_name16 = malloc(buf_size);
> > > > +   if (!var_name16)
> > > > +           return EFI_OUT_OF_RESOURCES;
> > > > +
> > > > +   var_name16[0] = 0;
> > > > +   for (;;) {
> > > > +           int index;
> > > > +           efi_guid_t guid;
> > > > +           efi_uintn_t tmp;
> > > > +
> > > > +           ret = efi_next_variable_name(&buf_size, &var_name16,
> > &guid);
> > >
> > > Can't we simply put the boot option number into struct
> > > eficonfig_media_boot_option and save 100 lines code?
> > >
> > > > +           if (ret == EFI_NOT_FOUND) {
> > > > +                   log_debug("last efi var\n");
> > > > +                   /*
> > > > +                    * EFI_NOT_FOUND indicates we retrieved all EFI
> > variables.
> > > > +                    * This should be treated as success.
> > > > +                    */
> > > > +                   ret = EFI_SUCCESS;
> > > > +                   break;
> > > > +           }
> > > > +
> > > > +           if (ret != EFI_SUCCESS)
> > > > +                   goto out;
> > > > +
> > > > +           if (!efi_varname_is_load_option(var_name16, &index))
> > > > +                   continue;
> > > > +
> > > > +           efi_create_indexed_name(varname, sizeof(varname), "Boot",
> > index);
> > > > +           load_option = efi_get_var(varname,
> > &efi_global_variable_guid, &size);
> > > > +           if (!load_option)
> > > > +                   continue;
> > > > +
> > > > +           tmp = size;
> > > > +           ret = efi_deserialize_load_option(&lo, load_option, &size);
> > > > +           if (ret != EFI_SUCCESS)
> > > > +                   goto next;
> > > > +
> > > > +           if (size >= sizeof(efi_guid_bootmenu_auto_generated) &&
> > > > +               !guidcmp(lo.optional_data,
> > &efi_guid_bootmenu_auto_generated)) {
> > > > +                   for (i = 0; i < count; i++) {
> > > > +                           if (opt[i].size == tmp &&
> > > > +                               memcmp(opt[i].lo, load_option, tmp) ==
> > 0) {
> > > > +                                   opt[i].exist = true;
> > > > +                                   break;
> > > > +                           }
> > > > +                   }
> > > > +
> > > > +                   /*
> > > > +                    * The entire list of variables must be retrieved
> > by
> > > > +                    * efi_get_next_variable_name_int() before
> > deleting the invalid
> > > > +                    * boot option, just save the index here.
> > > > +                    */
> > > > +                   if (i == count) {
> > > > +                           log_debug("found index %d can be
> > deleted\n", index);
> > > > +                           p = realloc(delete_index_list, sizeof(u32)
> > *
> > > > +                                       (list_size + 1));
> > > > +                           if (!p) {
> > > > +                                   ret = EFI_OUT_OF_RESOURCES;
> > > > +                                   goto out;
> > > > +                           }
> > > > +                           delete_index_list = p;
> > > > +                           delete_index_list[list_size++] = index;
> > > > +                   }
> > > > +           }
> > > > +next:
> > > > +           free(load_option);
> > > > +   }
> > > > +
> > > > +   /* delete all invalid boot options */
> > > > +   for (i = 0; i < list_size; i++) {
> > > > +           log_debug("deleting index %d\n", delete_index_list[i]);
> > > > +           ret = efi_bootmgr_delete_boot_option(delete_index_list[i]);
> > > > +           if (ret != EFI_SUCCESS)
> > > > +                   goto out;
> > > > +   }
> > > > +
> > > > +out:
> > > > +   free(var_name16);
> > > > +   free(delete_index_list);
> > > > +
> > > > +   return ret;
> > > > +}
> > > > +
> > > > +/**
> > > > + * efi_bootmgr_get_unused_bootoption() - get unused "Boot####" index
> > > > + *
> > > > + * @buf:   pointer to the buffer to store boot option variable name
> > > > + * @buf_size:      buffer size
> > > > + * @index: pointer to store the index in the BootOrder variable
> > > > + * Return: status code
> > > > + */
> > > > +efi_status_t efi_bootmgr_get_unused_bootoption(u16 *buf, efi_uintn_t
> > buf_size,
> > > > +                                          unsigned int *index)
> > >
> > > This function should be static. The exported functions should not take
> > > the inddex in the BootOrder variable as a parameter.
> > >
> > > > +{
> > > > +   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;
> > > > +}
> > > > +
> > > > +/**
> > > > + * efi_bootmgr_append_bootorder() - append new boot option in
> > BootOrder variable
> > > > + *
> > > > + * @index: "Boot####" index to append to BootOrder variable
> > > > + * Return: status code
> > > > + */
> > > > +efi_status_t efi_bootmgr_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;
> > > > +}
> > > > +
> > > > +/**
> > > > + * efi_bootmgr_delete_boot_option() - delete selected boot option
> > > > + *
> > > > + * @boot_index:    boot option index to delete
> > > > + * Return: status code
> > > > + */
> > > > +efi_status_t efi_bootmgr_delete_boot_option(u16 boot_index)
> > > > +{
> > > > +   u16 *bootorder;
> > > > +   u16 varname[9];
> > > > +   efi_status_t ret;
> > > > +   unsigned int index;
> > > > +   efi_uintn_t num, size;
> > > > +
> > > > +   efi_create_indexed_name(varname, sizeof(varname),
> > > > +                           "Boot", boot_index);
> > > > +   ret = efi_set_variable_int(varname, &efi_global_variable_guid,
> > > > +                              0, 0, NULL, false);
> > > > +   if (ret != EFI_SUCCESS) {
> > > > +           log_err("delete boot option(%ls) failed\n", varname);
> > > > +           return ret;
> > > > +   }
> > > > +
> > > > +   /* update BootOrder if necessary */
> > > > +   bootorder = efi_get_var(u"BootOrder", &efi_global_variable_guid,
> > &size);
> > > > +   if (!bootorder)
> > > > +           return EFI_SUCCESS;
> > > > +
> > > > +   num = size / sizeof(u16);
> > > > +   if (!efi_search_bootorder(bootorder, num, boot_index, &index))
> > > > +           return EFI_SUCCESS;
> > > > +
> > > > +   memmove(&bootorder[index], &bootorder[index + 1],
> > > > +           (num - index - 1) * sizeof(u16));
> > > > +   size -= sizeof(u16);
> > > > +   ret = efi_set_variable_int(u"BootOrder", &efi_global_variable_guid,
> > > > +                              EFI_VARIABLE_NON_VOLATILE |
> > > > +                              EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > > > +                              EFI_VARIABLE_RUNTIME_ACCESS,
> > > > +                              size, bootorder, false);
> > > > +
> > > > +   return ret;
> > > > +}
> > > > +
> > > > +/**
> > > > + * efi_bootmgr_update_media_device_boot_option() - generate the media
> > device boot option
> > > > + *
> > > > + * This function enumerates all devices supporting
> > EFI_SIMPLE_FILE_SYSTEM_PROTOCOL
> > > > + * and generate the bootmenu entries.
> > > > + * This function also provide the BOOT#### variable maintenance for
> > > > + * the media device entries.
> > > > + *   - Automatically create the BOOT#### variable for the newly
> > detected device,
> > > > + *     this BOOT#### variable is distinguished by the special GUID
> > > > + *     stored in the EFI_LOAD_OPTION.optional_data
> > > > + *   - If the device is not attached to the system, the associated
> > BOOT#### variable
> > > > + *     is automatically deleted.
> > > > + *
> > > > + * Return: status code
> > > > + */
> > > > +efi_status_t efi_bootmgr_update_media_device_boot_option(void)
> > > > +{
> > > > +   u32 i;
> > > > +   efi_status_t ret;
> > > > +   efi_uintn_t count;
> > > > +   efi_handle_t *volume_handles = NULL;
> > > > +   struct eficonfig_media_boot_option *opt = NULL;
> > > > +
> > > > +   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)
> > > > +           return ret;
> > > > +
> > > > +   opt = calloc(count, sizeof(struct eficonfig_media_boot_option));
> > > > +   if (!opt)
> > > > +           goto out;
> > > > +
> > > > +   /* enumerate all devices supporting
> > EFI_SIMPLE_FILE_SYSTEM_PROTOCOL */
> > > > +   ret = efi_bootmgr_enumerate_boot_option(opt, volume_handles,
> > count);
> > > > +   if (ret != EFI_SUCCESS)
> > > > +           goto out;
> > > > +
> > > > +   /*
> > > > +    * System hardware configuration may vary depending on the user
> > setup.
> > > > +    * The boot option is automatically added by the bootmenu.
> > > > +    * If the device is not attached to the system, the boot option
> > needs
> > > > +    * to be deleted.
> > > > +    */
> > > > +   ret = efi_bootmgr_delete_invalid_boot_option(opt, count);
> > > > +   if (ret != EFI_SUCCESS)
> > > > +           goto out;
> > > > +
> > > > +   /* add non-existent boot option */
> > > > +   for (i = 0; i < count; i++) {
> > > > +           u32 boot_index;
> > > > +           u16 var_name[9];
> > > > +
> > > > +           if (!opt[i].exist) {
> > > > +                   ret = efi_bootmgr_get_unused_bootoption(var_name,
> > sizeof(var_name),
> > > > +
> >  &boot_index);
> > > > +                   if (ret != EFI_SUCCESS)
> > > > +                           goto out;
> > > > +
> > > > +                   ret = efi_set_variable_int(var_name,
> > &efi_global_variable_guid,
> > > > +
> > EFI_VARIABLE_NON_VOLATILE |
> > > > +
> > EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > > > +
> > EFI_VARIABLE_RUNTIME_ACCESS,
> > > > +                                              opt[i].size, opt[i].lo,
> > false);
> > > > +                   if (ret != EFI_SUCCESS)
> > > > +                           goto out;
> > > > +
> > > > +                   ret = efi_bootmgr_append_bootorder(boot_index);
> > > > +                   if (ret != EFI_SUCCESS) {
> > > > +                           efi_set_variable_int(var_name,
> > &efi_global_variable_guid,
> > > > +                                                0, 0, NULL, false);
> > > > +                           goto out;
> > > > +                   }
> > > > +           }
> > > > +   }
> > > > +
> > > > +out:
> > > > +   if (opt) {
> > > > +           for (i = 0; i < count; i++) {
> > > > +                   if (opt[i].lo)
> > > > +                           free(opt[i].lo);
> > > > +           }
> > > > +           free(opt);
> > > > +   }
> > > > +   efi_free_pool(volume_handles);
> > > > +
> > > > +   return ret;
> > > > +}
> > > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> > > > index d2256713a8..1309e28134 100644
> > > > --- a/lib/efi_loader/efi_disk.c
> > > > +++ b/lib/efi_loader/efi_disk.c
> > > > @@ -687,6 +687,13 @@ int efi_disk_probe(void *ctx, struct event *event)
> > > >                     return -1;
> > > >     }
> > > >
> > > > +   /* only do the boot option management when UEFI sub-system is
> > initialized */
> > > > +   if (efi_obj_list_initialized == EFI_SUCCESS) {
> > > > +           ret = efi_bootmgr_update_media_device_boot_option();
> > >
> > > Why should we rescan all devices if a single device is added?
> >
> > Then, don't forget to call the 'remove' function in efi_disk_remove()
> > (or _delete_raw() and _delete_part()).
> 
> The disk remove event is not triggered when a removable disk is unplugged,
> so adding the
> calling of the 'remove' function in efi_disk_remove() cannot meet our
> expectation.

Right, but it's not a fault of UEFI but U-Boot itself.
The purpose here is to synchronise the DM status and UEFI devices
(in this case, efi_disk) accordingly.
I still believe that adding a hook is a good practice.
(Note that a remove function may be called at any device *scan*
(or info) command.)

> Other than that, there is an issue with fTPM by doing that.
> The efi_disk_remove() function was called to uninit the devices after ramfs
> was mounted.
> Calling the 'remove' function here gets a side effect that the optee-ftpm
> doesn't close the
> ta session properly and then gets an error when it tries to open it after
> kernel starts.

Isn't it a bug?

-Takahiro Akashi

> Logs like below:
> optee-ftpm optee-ta-bc50d971-d4c9-42c4-82cb-343fb7f37896: ftpm_tee_probe:
> tee_client_open_session failed, err=ffff000d
> optee-ftpm: probe of optee-ta-bc50d971-d4c9-42c4-82cb-343fb7f37896 failed
> with error -22
> 
> - Raymond Mao
> 
> 
> >
> > The only place where we need the list of removable media devices is when
> > > executing the boot manager.
> >
> > Not sure what you mean, but we may want to confirm available boot options
> > with efidebug or eficonfig before invoking the boot manager.
> >
> > -Takahiro Akashi
> >
> > >
> > > > +           if (ret)
> > > > +                   return -1;
> > > > +   }
> > > > +
> > > >     return 0;
> > > >   }
> > > >
> > > > diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
> > > > index 1f4ab2b419..cdfd16ea77 100644
> > > > --- a/lib/efi_loader/efi_helper.c
> > > > +++ b/lib/efi_loader/efi_helper.c
> > > > @@ -257,3 +257,28 @@ efi_status_t efi_next_variable_name(efi_uintn_t
> > *size, u16 **buf, efi_guid_t *gu
> > > >
> > > >     return ret;
> > > >   }
> > > > +
> > > > +/**
> > > > + * efi_search_bootorder() - search the boot option index in BootOrder
> > > > + *
> > > > + * @bootorder:     pointer to the BootOrder variable
> > > > + * @num:   number of BootOrder entry
> > > > + * @target:        target boot option index to search
> > > > + * @index: pointer to store the index of BootOrder variable
> > > > + * Return: true if exists, false otherwise
> > > > + */
> > > > +bool efi_search_bootorder(u16 *bootorder, efi_uintn_t num, u32
> > target, u32 *index)
> > >
> > > This function should be static. The exported function to delete a boot
> > > option should not use the index in the boot order in their interface but
> > > the number encoded in the variable name.
> > >
> > > We can simplify the function:
> > >
> > > Return -1 if the entry is not found and the index otherwise.
> > >
> > > > +{
> > > > +   u32 i;
> > > > +
> > > > +   for (i = 0; i < num; i++) {
> > > > +           if (target == bootorder[i]) {
> > > > +                   if (index)
> > > > +                           *index = i;
> > > > +
> > > > +                   return true;
> > > > +           }
> > > > +   }
> > > > +
> > > > +   return false;
> > > > +}
> > > > diff --git a/lib/efi_loader/efi_variable.c
> > b/lib/efi_loader/efi_variable.c
> > > > index be95ed44e6..2f251553e1 100644
> > > > --- a/lib/efi_loader/efi_variable.c
> > > > +++ b/lib/efi_loader/efi_variable.c
> > > > @@ -476,6 +476,10 @@ efi_status_t efi_init_variables(void)
> > > >                     log_err("Invalid EFI variable seed\n");
> > > >     }
> > > >
> > > > +   ret = efi_init_secure_state();
> > > > +   if (ret != EFI_SUCCESS)
> > > > +           return ret;
> > > >
> > > > -   return efi_init_secure_state();
> > > > +   /* update boot option management after variable service
> > initialized */
> > > > +   return efi_bootmgr_update_media_device_boot_option();
> > >
> > > The right place to call the function might be efi_init_obj_list().
> > >
> > > Best regards
> > >
> > > Heinrich
> > >
> > > >   }
> > > > diff --git a/lib/efi_loader/efi_variable_tee.c
> > b/lib/efi_loader/efi_variable_tee.c
> > > > index dfef18435d..a48d313ef0 100644
> > > > --- a/lib/efi_loader/efi_variable_tee.c
> > > > +++ b/lib/efi_loader/efi_variable_tee.c
> > > > @@ -748,5 +748,6 @@ efi_status_t efi_init_variables(void)
> > > >     if (ret != EFI_SUCCESS)
> > > >             return ret;
> > > >
> > > > -   return EFI_SUCCESS;
> > > > +   /* update boot option management after variable service
> > initialized */
> > > > +   return efi_bootmgr_update_media_device_boot_option();
> > > >   }
> > >
> >

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-04-18  0:50 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-05  0:06 [PATCH v2 1/2] Boot var automatic management for removable medias Raymond Mao
2023-04-05  0:06 ` [PATCH v2 2/2] Load option with short device path for boot vars Raymond Mao
2023-04-07 10:13 ` [PATCH v2 1/2] Boot var automatic management for removable medias Heinrich Schuchardt
2023-04-13  8:19   ` AKASHI Takahiro
2023-04-17 14:22     ` Raymond Mao
2023-04-18  0:50       ` AKASHI Takahiro

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.