[PATCH v8 0/4] Boot variables management for removable media

[PATCH v8 0/4] Boot variables management for removable media

[Raymond Mao]

Major changes:
1.Rename and move bootorder and bootoption apis from cmd to lib
  for re-use between eficonfig and bootmgr
2.Fix incorrect return code of boot option update
  Correct the return code for out-of-memory and no boot option found
3.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.
4.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.

Raymond Mao (4):
  Move bootorder and bootoption apis to lib
  Fix incorrect return code of boot option update
  Boot var automatic management for removable medias
  Load option with short device path for boot vars

 cmd/bootmenu.c               |   4 +-
 cmd/eficonfig.c              | 410 +----------------------------------
 include/efi_config.h         |   5 -
 include/efi_loader.h         |  11 +
 lib/efi_loader/efi_bootmgr.c | 385 ++++++++++++++++++++++++++++++++
 lib/efi_loader/efi_disk.c    |   7 +
 lib/efi_loader/efi_helper.c  |  25 +++
 lib/efi_loader/efi_setup.c   |   5 +
 8 files changed, 442 insertions(+), 410 deletions(-)

-- 
2.25.1

[PATCH v8 1/4] Move bootorder and bootoption apis to lib

[Raymond Mao]

Rename and move bootorder and bootoption apis from cmd to lib
for re-use between eficonfig and bootmgr
Fix 'unexpected indentation' when 'make htmldocs' after functions
are moved

Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
---
Changes in v3
- Split the patch into moving and renaming functions and
  individual patches for each changed functionality
Changes in v6
- Revert v3 changes in function
  efi_bootmgr_update_media_device_boot_option and
  efi_bootmgr_delete_invalid_boot_option
Changes in v8
- Fix 'unexpected indentation' when 'make htmldocs'

 cmd/bootmenu.c               |   2 +-
 cmd/eficonfig.c              | 408 +----------------------------------
 include/efi_config.h         |   5 -
 include/efi_loader.h         |  11 +
 lib/efi_loader/efi_bootmgr.c | 375 ++++++++++++++++++++++++++++++++
 lib/efi_loader/efi_helper.c  |  25 +++
 6 files changed, 418 insertions(+), 408 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..97d5b8dc2b 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -347,3 +347,378 @@ 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) {
+			/*
+			 * 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 = 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++)
+			free(opt[i].lo);
+	}
+	free(opt);
+	efi_free_pool(volume_handles);
+
+	return ret;
+}
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;
+}
-- 
2.25.1

Re: [PATCH v8 1/4] Move bootorder and bootoption apis to lib

[Ilias Apalodimas]

On Tue, Jun 06, 2023 at 09:37:18AM -0700, Raymond Mao wrote:
> Rename and move bootorder and bootoption apis from cmd to lib
> for re-use between eficonfig and bootmgr
> Fix 'unexpected indentation' when 'make htmldocs' after functions
> are moved
>
> Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
> ---
> Changes in v3
> - Split the patch into moving and renaming functions and
>   individual patches for each changed functionality
> Changes in v6
> - Revert v3 changes in function
>   efi_bootmgr_update_media_device_boot_option and
>   efi_bootmgr_delete_invalid_boot_option
> Changes in v8
> - Fix 'unexpected indentation' when 'make htmldocs'
>
>  cmd/bootmenu.c               |   2 +-
>  cmd/eficonfig.c              | 408 +----------------------------------
>  include/efi_config.h         |   5 -
>  include/efi_loader.h         |  11 +
>  lib/efi_loader/efi_bootmgr.c | 375 ++++++++++++++++++++++++++++++++
>  lib/efi_loader/efi_helper.c  |  25 +++
>  6 files changed, 418 insertions(+), 408 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..97d5b8dc2b 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -347,3 +347,378 @@ 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) {
> +			/*
> +			 * 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 = 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++)
> +			free(opt[i].lo);
> +	}
> +	free(opt);
> +	efi_free_pool(volume_handles);
> +
> +	return ret;
> +}
> 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;
> +}
> --
> 2.25.1
>

One nitpick when moving around efi_bootmgr_get_unused_bootoption().  The
unused boot option number has nothing to do with the boot manager. I think
moving it to lib/efi_loader/efi_helper.c is a better option.  In any case
we can do it in a future patch since this one is moves code around
from eficonfig to the efibootmgr.

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

[PATCH v8 2/4] Fix incorrect return code of boot option update

[Raymond Mao]

Correct the return code for out-of-memory and no boot option found

Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
Changes in v7
- new patch file created

 cmd/bootmenu.c               | 2 +-
 cmd/eficonfig.c              | 2 +-
 lib/efi_loader/efi_bootmgr.c | 8 ++++++--
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
index 01daddca7b..987b16889f 100644
--- a/cmd/bootmenu.c
+++ b/cmd/bootmenu.c
@@ -352,7 +352,7 @@ static struct bootmenu_data *bootmenu_create(int delay)
 		 * a architecture-specific default image name such as BOOTAA64.EFI.
 		 */
 		efi_ret = efi_bootmgr_update_media_device_boot_option();
-		if (efi_ret != EFI_SUCCESS && efi_ret != EFI_NOT_FOUND)
+		if (efi_ret != EFI_SUCCESS)
 			goto cleanup;
 
 		ret = prepare_uefi_bootorder_entry(menu, &iter, &i);
diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
index 82a80306f4..e6e8a0a488 100644
--- a/cmd/eficonfig.c
+++ b/cmd/eficonfig.c
@@ -2314,7 +2314,7 @@ static int do_eficonfig(struct cmd_tbl *cmdtp, int flag, int argc, char *const a
 		return CMD_RET_FAILURE;
 
 	ret = efi_bootmgr_update_media_device_boot_option();
-	if (ret != EFI_SUCCESS && ret != EFI_NOT_FOUND)
+	if (ret != EFI_SUCCESS)
 		return ret;
 
 	while (1) {
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index 97d5b8dc2b..28e800499c 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -663,11 +663,13 @@ efi_status_t efi_bootmgr_update_media_device_boot_option(void)
 					   NULL, &count,
 					   (efi_handle_t **)&volume_handles);
 	if (ret != EFI_SUCCESS)
-		return ret;
+		goto out;
 
 	opt = calloc(count, sizeof(struct eficonfig_media_boot_option));
-	if (!opt)
+	if (!opt) {
+		ret = EFI_OUT_OF_RESOURCES;
 		goto out;
+	}
 
 	/* enumerate all devices supporting EFI_SIMPLE_FILE_SYSTEM_PROTOCOL */
 	ret = efi_bootmgr_enumerate_boot_option(opt, volume_handles, count);
@@ -720,5 +722,7 @@ out:
 	free(opt);
 	efi_free_pool(volume_handles);
 
+	if (ret == EFI_NOT_FOUND)
+		return EFI_SUCCESS;
 	return ret;
 }
-- 
2.25.1

Re: [PATCH v8 2/4] Fix incorrect return code of boot option update

[Ilias Apalodimas]

Hi Raymond

On Tue, 6 Jun 2023 at 19:37, Raymond Mao <raymond.mao@linaro.org> wrote:
>
> Correct the return code for out-of-memory and no boot option found
>
> Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> Changes in v7
> - new patch file created
>
>  cmd/bootmenu.c               | 2 +-
>  cmd/eficonfig.c              | 2 +-
>  lib/efi_loader/efi_bootmgr.c | 8 ++++++--
>  3 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
> index 01daddca7b..987b16889f 100644
> --- a/cmd/bootmenu.c
> +++ b/cmd/bootmenu.c
> @@ -352,7 +352,7 @@ static struct bootmenu_data *bootmenu_create(int delay)
>                  * a architecture-specific default image name such as BOOTAA64.EFI.
>                  */
>                 efi_ret = efi_bootmgr_update_media_device_boot_option();
> -               if (efi_ret != EFI_SUCCESS && efi_ret != EFI_NOT_FOUND)
> +               if (efi_ret != EFI_SUCCESS)
>                         goto cleanup;
>
>                 ret = prepare_uefi_bootorder_entry(menu, &iter, &i);
> diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> index 82a80306f4..e6e8a0a488 100644
> --- a/cmd/eficonfig.c
> +++ b/cmd/eficonfig.c
> @@ -2314,7 +2314,7 @@ static int do_eficonfig(struct cmd_tbl *cmdtp, int flag, int argc, char *const a
>                 return CMD_RET_FAILURE;
>
>         ret = efi_bootmgr_update_media_device_boot_option();
> -       if (ret != EFI_SUCCESS && ret != EFI_NOT_FOUND)
> +       if (ret != EFI_SUCCESS)
>                 return ret;
>
>         while (1) {
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index 97d5b8dc2b..28e800499c 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -663,11 +663,13 @@ efi_status_t efi_bootmgr_update_media_device_boot_option(void)
>                                            NULL, &count,
>                                            (efi_handle_t **)&volume_handles);
>         if (ret != EFI_SUCCESS)
> -               return ret;
> +               goto out;

I missed that in my original review, but I think this change is wrong.
If you follow the goto out tag now instead of returning directly
there's a chance efi_locate_handle_buffer_int() will return
EFI_NOT_FOUND.  The goto out tag will convert that to EFI_SUCCESS
although it's an actual error.

Thanks
/Ilias
>
>         opt = calloc(count, sizeof(struct eficonfig_media_boot_option));
> -       if (!opt)
> +       if (!opt) {
> +               ret = EFI_OUT_OF_RESOURCES;
>                 goto out;
> +       }
>
>         /* enumerate all devices supporting EFI_SIMPLE_FILE_SYSTEM_PROTOCOL */
>         ret = efi_bootmgr_enumerate_boot_option(opt, volume_handles, count);
> @@ -720,5 +722,7 @@ out:
>         free(opt);
>         efi_free_pool(volume_handles);
>
> +       if (ret == EFI_NOT_FOUND)
> +               return EFI_SUCCESS;
>         return ret;
>  }
> --
> 2.25.1
>

Re: [PATCH v8 2/4] Fix incorrect return code of boot option update

[Ilias Apalodimas]

On Tue, 6 Jun 2023 at 22:23, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Raymond
>
> On Tue, 6 Jun 2023 at 19:37, Raymond Mao <raymond.mao@linaro.org> wrote:
> >
> > Correct the return code for out-of-memory and no boot option found
> >
> > Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
> > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> > Changes in v7
> > - new patch file created
> >
> >  cmd/bootmenu.c               | 2 +-
> >  cmd/eficonfig.c              | 2 +-
> >  lib/efi_loader/efi_bootmgr.c | 8 ++++++--
> >  3 files changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
> > index 01daddca7b..987b16889f 100644
> > --- a/cmd/bootmenu.c
> > +++ b/cmd/bootmenu.c
> > @@ -352,7 +352,7 @@ static struct bootmenu_data *bootmenu_create(int delay)
> >                  * a architecture-specific default image name such as BOOTAA64.EFI.
> >                  */
> >                 efi_ret = efi_bootmgr_update_media_device_boot_option();
> > -               if (efi_ret != EFI_SUCCESS && efi_ret != EFI_NOT_FOUND)
> > +               if (efi_ret != EFI_SUCCESS)
> >                         goto cleanup;
> >
> >                 ret = prepare_uefi_bootorder_entry(menu, &iter, &i);
> > diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> > index 82a80306f4..e6e8a0a488 100644
> > --- a/cmd/eficonfig.c
> > +++ b/cmd/eficonfig.c
> > @@ -2314,7 +2314,7 @@ static int do_eficonfig(struct cmd_tbl *cmdtp, int flag, int argc, char *const a
> >                 return CMD_RET_FAILURE;
> >
> >         ret = efi_bootmgr_update_media_device_boot_option();
> > -       if (ret != EFI_SUCCESS && ret != EFI_NOT_FOUND)
> > +       if (ret != EFI_SUCCESS)
> >                 return ret;
> >
> >         while (1) {
> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > index 97d5b8dc2b..28e800499c 100644
> > --- a/lib/efi_loader/efi_bootmgr.c
> > +++ b/lib/efi_loader/efi_bootmgr.c
> > @@ -663,11 +663,13 @@ efi_status_t efi_bootmgr_update_media_device_boot_option(void)
> >                                            NULL, &count,
> >                                            (efi_handle_t **)&volume_handles);
> >         if (ret != EFI_SUCCESS)
> > -               return ret;
> > +               goto out;
>
> I missed that in my original review, but I think this change is wrong.
> If you follow the goto out tag now instead of returning directly
> there's a chance efi_locate_handle_buffer_int() will return
> EFI_NOT_FOUND.  The goto out tag will convert that to EFI_SUCCESS
> although it's an actual error.

OTOH we were ignoring EFI_NOT_FOUND already outside this function.
So the overall functionality isn't affected

Thanks
/Ilias
>
> Thanks
> /Ilias
> >
> >         opt = calloc(count, sizeof(struct eficonfig_media_boot_option));
> > -       if (!opt)
> > +       if (!opt) {
> > +               ret = EFI_OUT_OF_RESOURCES;
> >                 goto out;
> > +       }
> >
> >         /* enumerate all devices supporting EFI_SIMPLE_FILE_SYSTEM_PROTOCOL */
> >         ret = efi_bootmgr_enumerate_boot_option(opt, volume_handles, count);
> > @@ -720,5 +722,7 @@ out:
> >         free(opt);
> >         efi_free_pool(volume_handles);
> >
> > +       if (ret == EFI_NOT_FOUND)
> > +               return EFI_SUCCESS;
> >         return ret;
> >  }
> > --
> > 2.25.1
> >

Re: [PATCH v8 2/4] Fix incorrect return code of boot option update

[Raymond Mao]

Hi Ilias,

> > --- a/lib/efi_loader/efi_bootmgr.c
> > +++ b/lib/efi_loader/efi_bootmgr.c
> > @@ -663,11 +663,13 @@ efi_status_t efi_bootmgr_update_media_device_boot_option(void)
> >                                            NULL, &count,
> >                                            (efi_handle_t **)&volume_handles);
> >         if (ret != EFI_SUCCESS)
> > -               return ret;
> > +               goto out;
>
> I missed that in my original review, but I think this change is wrong.
> If you follow the goto out tag now instead of returning directly
> there's a chance efi_locate_handle_buffer_int() will return
> EFI_NOT_FOUND.  The goto out tag will convert that to EFI_SUCCESS
> although it's an actual error.
This is what we intend to do. When there are no removable medias plug-in,
efi_locate_handle_buffer_int() will return EFI_NOT_FOUND.
And if we don't convert it to EFI_SUCCESS here,
efi_bootmgr_update_media_device_boot_option()
will return EFI_NOT_FOUND and then efi_init_obj_list() fails.
This leads to the efi init failure and it will just prompt 'Error:
Cannot initialize UEFI sub-system'
when you run any efidebug commands.

Regards,
Raymond


On Tue, 6 Jun 2023 at 15:24, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Raymond
>
> On Tue, 6 Jun 2023 at 19:37, Raymond Mao <raymond.mao@linaro.org> wrote:
> >
> > Correct the return code for out-of-memory and no boot option found
> >
> > Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
> > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> > Changes in v7
> > - new patch file created
> >
> >  cmd/bootmenu.c               | 2 +-
> >  cmd/eficonfig.c              | 2 +-
> >  lib/efi_loader/efi_bootmgr.c | 8 ++++++--
> >  3 files changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
> > index 01daddca7b..987b16889f 100644
> > --- a/cmd/bootmenu.c
> > +++ b/cmd/bootmenu.c
> > @@ -352,7 +352,7 @@ static struct bootmenu_data *bootmenu_create(int delay)
> >                  * a architecture-specific default image name such as BOOTAA64.EFI.
> >                  */
> >                 efi_ret = efi_bootmgr_update_media_device_boot_option();
> > -               if (efi_ret != EFI_SUCCESS && efi_ret != EFI_NOT_FOUND)
> > +               if (efi_ret != EFI_SUCCESS)
> >                         goto cleanup;
> >
> >                 ret = prepare_uefi_bootorder_entry(menu, &iter, &i);
> > diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> > index 82a80306f4..e6e8a0a488 100644
> > --- a/cmd/eficonfig.c
> > +++ b/cmd/eficonfig.c
> > @@ -2314,7 +2314,7 @@ static int do_eficonfig(struct cmd_tbl *cmdtp, int flag, int argc, char *const a
> >                 return CMD_RET_FAILURE;
> >
> >         ret = efi_bootmgr_update_media_device_boot_option();
> > -       if (ret != EFI_SUCCESS && ret != EFI_NOT_FOUND)
> > +       if (ret != EFI_SUCCESS)
> >                 return ret;
> >
> >         while (1) {
> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > index 97d5b8dc2b..28e800499c 100644
> > --- a/lib/efi_loader/efi_bootmgr.c
> > +++ b/lib/efi_loader/efi_bootmgr.c
> > @@ -663,11 +663,13 @@ efi_status_t efi_bootmgr_update_media_device_boot_option(void)
> >                                            NULL, &count,
> >                                            (efi_handle_t **)&volume_handles);
> >         if (ret != EFI_SUCCESS)
> > -               return ret;
> > +               goto out;
>
> I missed that in my original review, but I think this change is wrong.
> If you follow the goto out tag now instead of returning directly
> there's a chance efi_locate_handle_buffer_int() will return
> EFI_NOT_FOUND.  The goto out tag will convert that to EFI_SUCCESS
> although it's an actual error.
>
> Thanks
> /Ilias
> >
> >         opt = calloc(count, sizeof(struct eficonfig_media_boot_option));
> > -       if (!opt)
> > +       if (!opt) {
> > +               ret = EFI_OUT_OF_RESOURCES;
> >                 goto out;
> > +       }
> >
> >         /* enumerate all devices supporting EFI_SIMPLE_FILE_SYSTEM_PROTOCOL */
> >         ret = efi_bootmgr_enumerate_boot_option(opt, volume_handles, count);
> > @@ -720,5 +722,7 @@ out:
> >         free(opt);
> >         efi_free_pool(volume_handles);
> >
> > +       if (ret == EFI_NOT_FOUND)
> > +               return EFI_SUCCESS;
> >         return ret;
> >  }
> > --
> > 2.25.1
> >

Re: [PATCH v8 2/4] Fix incorrect return code of boot option update

[Heinrich Schuchardt]

On 6/6/23 21:43, Raymond Mao wrote:
> Hi Ilias,
>
>>> --- a/lib/efi_loader/efi_bootmgr.c
>>> +++ b/lib/efi_loader/efi_bootmgr.c
>>> @@ -663,11 +663,13 @@ efi_status_t efi_bootmgr_update_media_device_boot_option(void)
>>>                                             NULL, &count,
>>>                                             (efi_handle_t **)&volume_handles);
>>>          if (ret != EFI_SUCCESS)
>>> -               return ret;
>>> +               goto out;
>>
>> I missed that in my original review, but I think this change is wrong.
>> If you follow the goto out tag now instead of returning directly
>> there's a chance efi_locate_handle_buffer_int() will return
>> EFI_NOT_FOUND.  The goto out tag will convert that to EFI_SUCCESS
>> although it's an actual error.
> This is what we intend to do. When there are no removable medias plug-in,
> efi_locate_handle_buffer_int() will return EFI_NOT_FOUND.
> And if we don't convert it to EFI_SUCCESS here,
> efi_bootmgr_update_media_device_boot_option()
> will return EFI_NOT_FOUND and then efi_init_obj_list() fails.
> This leads to the efi init failure and it will just prompt 'Error:
> Cannot initialize UEFI sub-system'
> when you run any efidebug commands.
>
> Regards,
> Raymond
>
>
> On Tue, 6 Jun 2023 at 15:24, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
>>
>> Hi Raymond
>>
>> On Tue, 6 Jun 2023 at 19:37, Raymond Mao <raymond.mao@linaro.org> wrote:
>>>
>>> Correct the return code for out-of-memory and no boot option found
>>>
>>> Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
>>> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>>> ---
>>> Changes in v7
>>> - new patch file created
>>>
>>>   cmd/bootmenu.c               | 2 +-
>>>   cmd/eficonfig.c              | 2 +-
>>>   lib/efi_loader/efi_bootmgr.c | 8 ++++++--
>>>   3 files changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
>>> index 01daddca7b..987b16889f 100644
>>> --- a/cmd/bootmenu.c
>>> +++ b/cmd/bootmenu.c
>>> @@ -352,7 +352,7 @@ static struct bootmenu_data *bootmenu_create(int delay)
>>>                   * a architecture-specific default image name such as BOOTAA64.EFI.
>>>                   */
>>>                  efi_ret = efi_bootmgr_update_media_device_boot_option();
>>> -               if (efi_ret != EFI_SUCCESS && efi_ret != EFI_NOT_FOUND)
>>> +               if (efi_ret != EFI_SUCCESS)
>>>                          goto cleanup;
>>>
>>>                  ret = prepare_uefi_bootorder_entry(menu, &iter, &i);
>>> diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
>>> index 82a80306f4..e6e8a0a488 100644
>>> --- a/cmd/eficonfig.c
>>> +++ b/cmd/eficonfig.c
>>> @@ -2314,7 +2314,7 @@ static int do_eficonfig(struct cmd_tbl *cmdtp, int flag, int argc, char *const a
>>>                  return CMD_RET_FAILURE;
>>>
>>>          ret = efi_bootmgr_update_media_device_boot_option();
>>> -       if (ret != EFI_SUCCESS && ret != EFI_NOT_FOUND)
>>> +       if (ret != EFI_SUCCESS)
>>>                  return ret;
>>>
>>>          while (1) {
>>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
>>> index 97d5b8dc2b..28e800499c 100644
>>> --- a/lib/efi_loader/efi_bootmgr.c
>>> +++ b/lib/efi_loader/efi_bootmgr.c
>>> @@ -663,11 +663,13 @@ efi_status_t efi_bootmgr_update_media_device_boot_option(void)
>>>                                             NULL, &count,
>>>                                             (efi_handle_t **)&volume_handles);
>>>          if (ret != EFI_SUCCESS)
>>> -               return ret;
>>> +               goto out;
>>
>> I missed that in my original review, but I think this change is wrong.
>> If you follow the goto out tag now instead of returning directly
>> there's a chance efi_locate_handle_buffer_int() will return
>> EFI_NOT_FOUND.  The goto out tag will convert that to EFI_SUCCESS
>> although it's an actual error.

It was due to my reply
https://lore.kernel.org/u-boot/e951264b-f952-3e83-dacc-bbe2fa37715f@gmx.de/
that Raymond moved the EFI_NOT_FOUND handling into this function. As
said non-availability of a file system on removable media should not
stop the initialization of the EFI sub-system or the probing of block
devices.

Why is efi_bootmgr_update_media_device_boot_option() not invoked when
block devices are removed?

Best regards

Heinrich

>>
>> Thanks
>> /Ilias
>>>
>>>          opt = calloc(count, sizeof(struct eficonfig_media_boot_option));
>>> -       if (!opt)
>>> +       if (!opt) {
>>> +               ret = EFI_OUT_OF_RESOURCES;
>>>                  goto out;
>>> +       }
>>>
>>>          /* enumerate all devices supporting EFI_SIMPLE_FILE_SYSTEM_PROTOCOL */
>>>          ret = efi_bootmgr_enumerate_boot_option(opt, volume_handles, count);
>>> @@ -720,5 +722,7 @@ out:
>>>          free(opt);
>>>          efi_free_pool(volume_handles);
>>>
>>> +       if (ret == EFI_NOT_FOUND)
>>> +               return EFI_SUCCESS;
>>>          return ret;
>>>   }
>>> --
>>> 2.25.1
>>>

Re: [PATCH v8 2/4] Fix incorrect return code of boot option update

[Ilias Apalodimas]

Hi Heinrich,


On Tue, 6 Jun 2023 at 22:56, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 6/6/23 21:43, Raymond Mao wrote:
> > Hi Ilias,
> >
> >>> --- a/lib/efi_loader/efi_bootmgr.c
> >>> +++ b/lib/efi_loader/efi_bootmgr.c
> >>> @@ -663,11 +663,13 @@ efi_status_t efi_bootmgr_update_media_device_boot_option(void)
> >>>                                             NULL, &count,
> >>>                                             (efi_handle_t **)&volume_handles);
> >>>          if (ret != EFI_SUCCESS)
> >>> -               return ret;
> >>> +               goto out;
> >>
> >> I missed that in my original review, but I think this change is wrong.
> >> If you follow the goto out tag now instead of returning directly
> >> there's a chance efi_locate_handle_buffer_int() will return
> >> EFI_NOT_FOUND.  The goto out tag will convert that to EFI_SUCCESS
> >> although it's an actual error.
> > This is what we intend to do. When there are no removable medias plug-in,
> > efi_locate_handle_buffer_int() will return EFI_NOT_FOUND.
> > And if we don't convert it to EFI_SUCCESS here,
> > efi_bootmgr_update_media_device_boot_option()
> > will return EFI_NOT_FOUND and then efi_init_obj_list() fails.
> > This leads to the efi init failure and it will just prompt 'Error:
> > Cannot initialize UEFI sub-system'
> > when you run any efidebug commands.
> >
> > Regards,
> > Raymond
> >
> >
> > On Tue, 6 Jun 2023 at 15:24, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> >>
> >> Hi Raymond
> >>
> >> On Tue, 6 Jun 2023 at 19:37, Raymond Mao <raymond.mao@linaro.org> wrote:
> >>>
> >>> Correct the return code for out-of-memory and no boot option found
> >>>
> >>> Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
> >>> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >>> ---
> >>> Changes in v7
> >>> - new patch file created
> >>>
> >>>   cmd/bootmenu.c               | 2 +-
> >>>   cmd/eficonfig.c              | 2 +-
> >>>   lib/efi_loader/efi_bootmgr.c | 8 ++++++--
> >>>   3 files changed, 8 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
> >>> index 01daddca7b..987b16889f 100644
> >>> --- a/cmd/bootmenu.c
> >>> +++ b/cmd/bootmenu.c
> >>> @@ -352,7 +352,7 @@ static struct bootmenu_data *bootmenu_create(int delay)
> >>>                   * a architecture-specific default image name such as BOOTAA64.EFI.
> >>>                   */
> >>>                  efi_ret = efi_bootmgr_update_media_device_boot_option();
> >>> -               if (efi_ret != EFI_SUCCESS && efi_ret != EFI_NOT_FOUND)
> >>> +               if (efi_ret != EFI_SUCCESS)
> >>>                          goto cleanup;
> >>>
> >>>                  ret = prepare_uefi_bootorder_entry(menu, &iter, &i);
> >>> diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> >>> index 82a80306f4..e6e8a0a488 100644
> >>> --- a/cmd/eficonfig.c
> >>> +++ b/cmd/eficonfig.c
> >>> @@ -2314,7 +2314,7 @@ static int do_eficonfig(struct cmd_tbl *cmdtp, int flag, int argc, char *const a
> >>>                  return CMD_RET_FAILURE;
> >>>
> >>>          ret = efi_bootmgr_update_media_device_boot_option();
> >>> -       if (ret != EFI_SUCCESS && ret != EFI_NOT_FOUND)
> >>> +       if (ret != EFI_SUCCESS)
> >>>                  return ret;
> >>>
> >>>          while (1) {
> >>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> >>> index 97d5b8dc2b..28e800499c 100644
> >>> --- a/lib/efi_loader/efi_bootmgr.c
> >>> +++ b/lib/efi_loader/efi_bootmgr.c
> >>> @@ -663,11 +663,13 @@ efi_status_t efi_bootmgr_update_media_device_boot_option(void)
> >>>                                             NULL, &count,
> >>>                                             (efi_handle_t **)&volume_handles);
> >>>          if (ret != EFI_SUCCESS)
> >>> -               return ret;
> >>> +               goto out;
> >>
> >> I missed that in my original review, but I think this change is wrong.
> >> If you follow the goto out tag now instead of returning directly
> >> there's a chance efi_locate_handle_buffer_int() will return
> >> EFI_NOT_FOUND.  The goto out tag will convert that to EFI_SUCCESS
> >> although it's an actual error.
>
> It was due to my reply
> https://lore.kernel.org/u-boot/e951264b-f952-3e83-dacc-bbe2fa37715f@gmx.de/
> that Raymond moved the EFI_NOT_FOUND handling into this function. As
> said non-availability of a file system on removable media should not
> stop the initialization of the EFI sub-system or the probing of block
> devices.

Yea, I had a similar comment on an earlier version.  EFI_NOT_FOUND was
ignored by the caller in the past, so the current change is fine.

>
> Why is efi_bootmgr_update_media_device_boot_option() not invoked when
> block devices are removed?

_remove() is being called before we exit to the OS, so we would end up
constantly adding/removing boot options and writing to flashes, which
is something we want to avoid.  The downside is that the device
removal and the boot options that might have been added, will be
removed on the next boot.  It's still a problem, but I don't think
it's that important at the moment.
Thanks
/Ilias
>
> Best regards
>
> Heinrich
>
> >>
> >> Thanks
> >> /Ilias
> >>>
> >>>          opt = calloc(count, sizeof(struct eficonfig_media_boot_option));
> >>> -       if (!opt)
> >>> +       if (!opt) {
> >>> +               ret = EFI_OUT_OF_RESOURCES;
> >>>                  goto out;
> >>> +       }
> >>>
> >>>          /* enumerate all devices supporting EFI_SIMPLE_FILE_SYSTEM_PROTOCOL */
> >>>          ret = efi_bootmgr_enumerate_boot_option(opt, volume_handles, count);
> >>> @@ -720,5 +722,7 @@ out:
> >>>          free(opt);
> >>>          efi_free_pool(volume_handles);
> >>>
> >>> +       if (ret == EFI_NOT_FOUND)
> >>> +               return EFI_SUCCESS;
> >>>          return ret;
> >>>   }
> >>> --
> >>> 2.25.1
> >>>
>

Re: [PATCH v8 2/4] Fix incorrect return code of boot option update

[Raymond Mao]

Hi Heinrich,

The reason that we don't add calling to
efi_bootmgr_update_media_device_boot_option() under efi_disk_remove()
is because efi_disk_remove() is always being called during exiting U-Boot and
booting an OS. - it doesn't make sense that the boot options we added is
removed there.
I added a text in the commit message of 'v8 3/4' patch for reference as
suggested by Ilias.

Regards,
Raymond

On Tue, 6 Jun 2023 at 15:56, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 6/6/23 21:43, Raymond Mao wrote:
> > Hi Ilias,
> >
> >>> --- a/lib/efi_loader/efi_bootmgr.c
> >>> +++ b/lib/efi_loader/efi_bootmgr.c
> >>> @@ -663,11 +663,13 @@ efi_status_t efi_bootmgr_update_media_device_boot_option(void)
> >>>                                             NULL, &count,
> >>>                                             (efi_handle_t **)&volume_handles);
> >>>          if (ret != EFI_SUCCESS)
> >>> -               return ret;
> >>> +               goto out;
> >>
> >> I missed that in my original review, but I think this change is wrong.
> >> If you follow the goto out tag now instead of returning directly
> >> there's a chance efi_locate_handle_buffer_int() will return
> >> EFI_NOT_FOUND.  The goto out tag will convert that to EFI_SUCCESS
> >> although it's an actual error.
> > This is what we intend to do. When there are no removable medias plug-in,
> > efi_locate_handle_buffer_int() will return EFI_NOT_FOUND.
> > And if we don't convert it to EFI_SUCCESS here,
> > efi_bootmgr_update_media_device_boot_option()
> > will return EFI_NOT_FOUND and then efi_init_obj_list() fails.
> > This leads to the efi init failure and it will just prompt 'Error:
> > Cannot initialize UEFI sub-system'
> > when you run any efidebug commands.
> >
> > Regards,
> > Raymond
> >
> >
> > On Tue, 6 Jun 2023 at 15:24, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> >>
> >> Hi Raymond
> >>
> >> On Tue, 6 Jun 2023 at 19:37, Raymond Mao <raymond.mao@linaro.org> wrote:
> >>>
> >>> Correct the return code for out-of-memory and no boot option found
> >>>
> >>> Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
> >>> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >>> ---
> >>> Changes in v7
> >>> - new patch file created
> >>>
> >>>   cmd/bootmenu.c               | 2 +-
> >>>   cmd/eficonfig.c              | 2 +-
> >>>   lib/efi_loader/efi_bootmgr.c | 8 ++++++--
> >>>   3 files changed, 8 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
> >>> index 01daddca7b..987b16889f 100644
> >>> --- a/cmd/bootmenu.c
> >>> +++ b/cmd/bootmenu.c
> >>> @@ -352,7 +352,7 @@ static struct bootmenu_data *bootmenu_create(int delay)
> >>>                   * a architecture-specific default image name such as BOOTAA64.EFI.
> >>>                   */
> >>>                  efi_ret = efi_bootmgr_update_media_device_boot_option();
> >>> -               if (efi_ret != EFI_SUCCESS && efi_ret != EFI_NOT_FOUND)
> >>> +               if (efi_ret != EFI_SUCCESS)
> >>>                          goto cleanup;
> >>>
> >>>                  ret = prepare_uefi_bootorder_entry(menu, &iter, &i);
> >>> diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> >>> index 82a80306f4..e6e8a0a488 100644
> >>> --- a/cmd/eficonfig.c
> >>> +++ b/cmd/eficonfig.c
> >>> @@ -2314,7 +2314,7 @@ static int do_eficonfig(struct cmd_tbl *cmdtp, int flag, int argc, char *const a
> >>>                  return CMD_RET_FAILURE;
> >>>
> >>>          ret = efi_bootmgr_update_media_device_boot_option();
> >>> -       if (ret != EFI_SUCCESS && ret != EFI_NOT_FOUND)
> >>> +       if (ret != EFI_SUCCESS)
> >>>                  return ret;
> >>>
> >>>          while (1) {
> >>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> >>> index 97d5b8dc2b..28e800499c 100644
> >>> --- a/lib/efi_loader/efi_bootmgr.c
> >>> +++ b/lib/efi_loader/efi_bootmgr.c
> >>> @@ -663,11 +663,13 @@ efi_status_t efi_bootmgr_update_media_device_boot_option(void)
> >>>                                             NULL, &count,
> >>>                                             (efi_handle_t **)&volume_handles);
> >>>          if (ret != EFI_SUCCESS)
> >>> -               return ret;
> >>> +               goto out;
> >>
> >> I missed that in my original review, but I think this change is wrong.
> >> If you follow the goto out tag now instead of returning directly
> >> there's a chance efi_locate_handle_buffer_int() will return
> >> EFI_NOT_FOUND.  The goto out tag will convert that to EFI_SUCCESS
> >> although it's an actual error.
>
> It was due to my reply
> https://lore.kernel.org/u-boot/e951264b-f952-3e83-dacc-bbe2fa37715f@gmx.de/
> that Raymond moved the EFI_NOT_FOUND handling into this function. As
> said non-availability of a file system on removable media should not
> stop the initialization of the EFI sub-system or the probing of block
> devices.
>
> Why is efi_bootmgr_update_media_device_boot_option() not invoked when
> block devices are removed?
>
> Best regards
>
> Heinrich
>
> >>
> >> Thanks
> >> /Ilias
> >>>
> >>>          opt = calloc(count, sizeof(struct eficonfig_media_boot_option));
> >>> -       if (!opt)
> >>> +       if (!opt) {
> >>> +               ret = EFI_OUT_OF_RESOURCES;
> >>>                  goto out;
> >>> +       }
> >>>
> >>>          /* enumerate all devices supporting EFI_SIMPLE_FILE_SYSTEM_PROTOCOL */
> >>>          ret = efi_bootmgr_enumerate_boot_option(opt, volume_handles, count);
> >>> @@ -720,5 +722,7 @@ out:
> >>>          free(opt);
> >>>          efi_free_pool(volume_handles);
> >>>
> >>> +       if (ret == EFI_NOT_FOUND)
> >>> +               return EFI_SUCCESS;
> >>>          return ret;
> >>>   }
> >>> --
> >>> 2.25.1
> >>>
>

[PATCH v8 3/4] Boot var automatic management for removable medias

[Raymond Mao]

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.
Please note that currently the function 'efi_disk_remove' is being
invoked during exiting U-Boot and booting an OS - that is why we
don't add the boot variables management inside 'efi_disk_remove'.

Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
Changes in v3
- Split the patch into moving and renaming functions and
  individual patches for each changed functionality
Changes in v5
- Move function call of efi_bootmgr_update_media_device_boot_option()
  from efi_init_variables() to efi_init_obj_list()
Changes in v6
- Revert unrelated changes
Changes in v7
- adapt the return code of function
  efi_bootmgr_update_media_device_boot_option()
Changes in v8
- add a note in the commit message for future reference

 lib/efi_loader/efi_disk.c  | 7 +++++++
 lib/efi_loader/efi_setup.c | 5 +++++
 2 files changed, 12 insertions(+)

diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
index d2256713a8..d9b48ecf64 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 != EFI_SUCCESS)
+			return -1;
+	}
+
 	return 0;
 }
 
diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
index 58d4e13402..877f3878d6 100644
--- a/lib/efi_loader/efi_setup.c
+++ b/lib/efi_loader/efi_setup.c
@@ -245,6 +245,11 @@ efi_status_t efi_init_obj_list(void)
 	if (ret != EFI_SUCCESS)
 		goto out;
 
+	/* update boot option after variable service initialized */
+	ret = efi_bootmgr_update_media_device_boot_option();
+	if (ret != EFI_SUCCESS)
+		goto out;
+
 	/* Define supported languages */
 	ret = efi_init_platform_lang();
 	if (ret != EFI_SUCCESS)
-- 
2.25.1

Re: [PATCH v8 3/4] Boot var automatic management for removable medias

[AKASHI Takahiro]

On Tue, Jun 06, 2023 at 09:37:20AM -0700, 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.
> Please note that currently the function 'efi_disk_remove' is being
> invoked during exiting U-Boot and booting an OS - that is why we
> don't add the boot variables management inside 'efi_disk_remove'.

It's not accurate.
For instance, a user may run "scsi scan" or "usb reset" to re-enumerate
devices added (or removed). They will invoke "remove" against pre-existing
devices before invoking a "probe" routine.

So the text above doesn't justify not implementing "remove" hook.

-Takahiro Akashi

> Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> Changes in v3
> - Split the patch into moving and renaming functions and
>   individual patches for each changed functionality
> Changes in v5
> - Move function call of efi_bootmgr_update_media_device_boot_option()
>   from efi_init_variables() to efi_init_obj_list()
> Changes in v6
> - Revert unrelated changes
> Changes in v7
> - adapt the return code of function
>   efi_bootmgr_update_media_device_boot_option()
> Changes in v8
> - add a note in the commit message for future reference
> 
>  lib/efi_loader/efi_disk.c  | 7 +++++++
>  lib/efi_loader/efi_setup.c | 5 +++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> index d2256713a8..d9b48ecf64 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 != EFI_SUCCESS)
> +			return -1;
> +	}
> +
>  	return 0;
>  }
>  
> diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> index 58d4e13402..877f3878d6 100644
> --- a/lib/efi_loader/efi_setup.c
> +++ b/lib/efi_loader/efi_setup.c
> @@ -245,6 +245,11 @@ efi_status_t efi_init_obj_list(void)
>  	if (ret != EFI_SUCCESS)
>  		goto out;
>  
> +	/* update boot option after variable service initialized */
> +	ret = efi_bootmgr_update_media_device_boot_option();
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
>  	/* Define supported languages */
>  	ret = efi_init_platform_lang();
>  	if (ret != EFI_SUCCESS)
> -- 
> 2.25.1
> 

Re: [PATCH v8 3/4] Boot var automatic management for removable medias

[Ilias Apalodimas]

On Wed, 7 Jun 2023 at 03:41, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
>
> On Tue, Jun 06, 2023 at 09:37:20AM -0700, 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.
> > Please note that currently the function 'efi_disk_remove' is being
> > invoked during exiting U-Boot and booting an OS - that is why we
> > don't add the boot variables management inside 'efi_disk_remove'.
>
> It's not accurate.
> For instance, a user may run "scsi scan" or "usb reset" to re-enumerate
> devices added (or removed). They will invoke "remove" against pre-existing
> devices before invoking a "probe" routine.
>
> So the text above doesn't justify not implementing "remove" hook.

I am not sure I am following you here.  Yes resetting the device will
call the remove callback and it's a current problem of the existing
implementation, but why is the commit message inaccurate?  Won't
exiting u-boot to jump to the OS call all the _remove() functions?

Thanks
/Ilias
>
> -Takahiro Akashi
>
> > Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
> > Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> > Changes in v3
> > - Split the patch into moving and renaming functions and
> >   individual patches for each changed functionality
> > Changes in v5
> > - Move function call of efi_bootmgr_update_media_device_boot_option()
> >   from efi_init_variables() to efi_init_obj_list()
> > Changes in v6
> > - Revert unrelated changes
> > Changes in v7
> > - adapt the return code of function
> >   efi_bootmgr_update_media_device_boot_option()
> > Changes in v8
> > - add a note in the commit message for future reference
> >
> >  lib/efi_loader/efi_disk.c  | 7 +++++++
> >  lib/efi_loader/efi_setup.c | 5 +++++
> >  2 files changed, 12 insertions(+)
> >
> > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> > index d2256713a8..d9b48ecf64 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 != EFI_SUCCESS)
> > +                     return -1;
> > +     }
> > +
> >       return 0;
> >  }
> >
> > diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> > index 58d4e13402..877f3878d6 100644
> > --- a/lib/efi_loader/efi_setup.c
> > +++ b/lib/efi_loader/efi_setup.c
> > @@ -245,6 +245,11 @@ efi_status_t efi_init_obj_list(void)
> >       if (ret != EFI_SUCCESS)
> >               goto out;
> >
> > +     /* update boot option after variable service initialized */
> > +     ret = efi_bootmgr_update_media_device_boot_option();
> > +     if (ret != EFI_SUCCESS)
> > +             goto out;
> > +
> >       /* Define supported languages */
> >       ret = efi_init_platform_lang();
> >       if (ret != EFI_SUCCESS)
> > --
> > 2.25.1
> >

Re: [PATCH v8 3/4] Boot var automatic management for removable medias

[AKASHI Takahiro]

On Wed, Jun 07, 2023 at 09:38:30AM +0300, Ilias Apalodimas wrote:
> On Wed, 7 Jun 2023 at 03:41, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> >
> > On Tue, Jun 06, 2023 at 09:37:20AM -0700, 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.
> > > Please note that currently the function 'efi_disk_remove' is being
> > > invoked during exiting U-Boot and booting an OS - that is why we
> > > don't add the boot variables management inside 'efi_disk_remove'.
> >
> > It's not accurate.
> > For instance, a user may run "scsi scan" or "usb reset" to re-enumerate
> > devices added (or removed). They will invoke "remove" against pre-existing
> > devices before invoking a "probe" routine.
> >
> > So the text above doesn't justify not implementing "remove" hook.
> 
> I am not sure I am following you here.  Yes resetting the device will
> call the remove callback and it's a current problem of the existing
> implementation,

I think it is a design decision.

> but why is the commit message inaccurate?  Won't
> exiting u-boot to jump to the OS call all the _remove() functions?

Exiting U-Boot is irrelevant to maintaining boot options.

-Takahiro Akashi

> Thanks
> /Ilias
> >
> > -Takahiro Akashi
> >
> > > Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
> > > Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > ---
> > > Changes in v3
> > > - Split the patch into moving and renaming functions and
> > >   individual patches for each changed functionality
> > > Changes in v5
> > > - Move function call of efi_bootmgr_update_media_device_boot_option()
> > >   from efi_init_variables() to efi_init_obj_list()
> > > Changes in v6
> > > - Revert unrelated changes
> > > Changes in v7
> > > - adapt the return code of function
> > >   efi_bootmgr_update_media_device_boot_option()
> > > Changes in v8
> > > - add a note in the commit message for future reference
> > >
> > >  lib/efi_loader/efi_disk.c  | 7 +++++++
> > >  lib/efi_loader/efi_setup.c | 5 +++++
> > >  2 files changed, 12 insertions(+)
> > >
> > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> > > index d2256713a8..d9b48ecf64 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 != EFI_SUCCESS)
> > > +                     return -1;
> > > +     }
> > > +
> > >       return 0;
> > >  }
> > >
> > > diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> > > index 58d4e13402..877f3878d6 100644
> > > --- a/lib/efi_loader/efi_setup.c
> > > +++ b/lib/efi_loader/efi_setup.c
> > > @@ -245,6 +245,11 @@ efi_status_t efi_init_obj_list(void)
> > >       if (ret != EFI_SUCCESS)
> > >               goto out;
> > >
> > > +     /* update boot option after variable service initialized */
> > > +     ret = efi_bootmgr_update_media_device_boot_option();
> > > +     if (ret != EFI_SUCCESS)
> > > +             goto out;
> > > +
> > >       /* Define supported languages */
> > >       ret = efi_init_platform_lang();
> > >       if (ret != EFI_SUCCESS)
> > > --
> > > 2.25.1
> > >

Re: [PATCH v8 3/4] Boot var automatic management for removable medias

[Ilias Apalodimas]

On Wed, 7 Jun 2023 at 09:52, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
>
> On Wed, Jun 07, 2023 at 09:38:30AM +0300, Ilias Apalodimas wrote:
> > On Wed, 7 Jun 2023 at 03:41, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > >
> > > On Tue, Jun 06, 2023 at 09:37:20AM -0700, 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.
> > > > Please note that currently the function 'efi_disk_remove' is being
> > > > invoked during exiting U-Boot and booting an OS - that is why we
> > > > don't add the boot variables management inside 'efi_disk_remove'.
> > >
> > > It's not accurate.
> > > For instance, a user may run "scsi scan" or "usb reset" to re-enumerate
> > > devices added (or removed). They will invoke "remove" against pre-existing
> > > devices before invoking a "probe" routine.
> > >
> > > So the text above doesn't justify not implementing "remove" hook.
> >
> > I am not sure I am following you here.  Yes resetting the device will
> > call the remove callback and it's a current problem of the existing
> > implementation,
>
> I think it is a design decision.

It's not.  Ideally, we should call the _remove() callbacks and
reconfigure the boot options correctly if a device gets unplugged or
the subsystem is restarted. However, at least in my opinion, the
problem isn't that big at the moment to prevent the patches from
getting merged.
If you restart the subsystem the same boot options will be rescanned
and the device will be able to boot.  The corner case here is scanning
a few boot options and then *removing* the device.  But in that case,
the device wasn't going to boot anyway from the removed media.

>
> > but why is the commit message inaccurate?  Won't
> > exiting u-boot to jump to the OS call all the _remove() functions?
>
> Exiting U-Boot is irrelevant to maintaining boot options.

In theory yes,  but the reality is that you can't remove the boot
options and rewrite flashes of embedded boards, with a limited
lifetime, every time you boot your OS.  So adding it on the _remove
callback *now* is a bad idea and that's what the commit message is
trying to hint.  We could in the future add an extra flag to the
remove callback which only gets set to false when we are exiting
u-boot

Thanks
/Ilias
>
> -Takahiro Akashi
>
> > Thanks
> > /Ilias
> > >
> > > -Takahiro Akashi
> > >
> > > > Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
> > > > Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > ---
> > > > Changes in v3
> > > > - Split the patch into moving and renaming functions and
> > > >   individual patches for each changed functionality
> > > > Changes in v5
> > > > - Move function call of efi_bootmgr_update_media_device_boot_option()
> > > >   from efi_init_variables() to efi_init_obj_list()
> > > > Changes in v6
> > > > - Revert unrelated changes
> > > > Changes in v7
> > > > - adapt the return code of function
> > > >   efi_bootmgr_update_media_device_boot_option()
> > > > Changes in v8
> > > > - add a note in the commit message for future reference
> > > >
> > > >  lib/efi_loader/efi_disk.c  | 7 +++++++
> > > >  lib/efi_loader/efi_setup.c | 5 +++++
> > > >  2 files changed, 12 insertions(+)
> > > >
> > > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> > > > index d2256713a8..d9b48ecf64 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 != EFI_SUCCESS)
> > > > +                     return -1;
> > > > +     }
> > > > +
> > > >       return 0;
> > > >  }
> > > >
> > > > diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> > > > index 58d4e13402..877f3878d6 100644
> > > > --- a/lib/efi_loader/efi_setup.c
> > > > +++ b/lib/efi_loader/efi_setup.c
> > > > @@ -245,6 +245,11 @@ efi_status_t efi_init_obj_list(void)
> > > >       if (ret != EFI_SUCCESS)
> > > >               goto out;
> > > >
> > > > +     /* update boot option after variable service initialized */
> > > > +     ret = efi_bootmgr_update_media_device_boot_option();
> > > > +     if (ret != EFI_SUCCESS)
> > > > +             goto out;
> > > > +
> > > >       /* Define supported languages */
> > > >       ret = efi_init_platform_lang();
> > > >       if (ret != EFI_SUCCESS)
> > > > --
> > > > 2.25.1
> > > >

Re: [PATCH v8 3/4] Boot var automatic management for removable medias

[AKASHI Takahiro]

On Wed, Jun 07, 2023 at 10:00:37AM +0300, Ilias Apalodimas wrote:
> On Wed, 7 Jun 2023 at 09:52, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> >
> > On Wed, Jun 07, 2023 at 09:38:30AM +0300, Ilias Apalodimas wrote:
> > > On Wed, 7 Jun 2023 at 03:41, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > > >
> > > > On Tue, Jun 06, 2023 at 09:37:20AM -0700, 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.
> > > > > Please note that currently the function 'efi_disk_remove' is being
> > > > > invoked during exiting U-Boot and booting an OS - that is why we
> > > > > don't add the boot variables management inside 'efi_disk_remove'.
> > > >
> > > > It's not accurate.
> > > > For instance, a user may run "scsi scan" or "usb reset" to re-enumerate
> > > > devices added (or removed). They will invoke "remove" against pre-existing
> > > > devices before invoking a "probe" routine.
> > > >
> > > > So the text above doesn't justify not implementing "remove" hook.
> > >
> > > I am not sure I am following you here.  Yes resetting the device will
> > > call the remove callback and it's a current problem of the existing
> > > implementation,
> >
> > I think it is a design decision.
> 
> It's not.  Ideally, we should call the _remove() callbacks and
> reconfigure the boot options correctly if a device gets unplugged or
> the subsystem is restarted. However, at least in my opinion, the
> problem isn't that big at the moment to prevent the patches from
> getting merged.
> If you restart the subsystem the same boot options will be rescanned
> and the device will be able to boot.  The corner case here is scanning
> a few boot options and then *removing* the device.  But in that case,
> the device wasn't going to boot anyway from the removed media.

First of all, we should distinguish two cases when you mention
the *existing* issues in Raymond's current implementation:
1. Some device is added or removed when we are still on U-Boot
2. When we are existing U-Boot

In the above text, you mention case(1) which is not covered by
his original commit message. It is one reason why I think that the commit
message is not accurate.

> >
> > > but why is the commit message inaccurate?  Won't
> > > exiting u-boot to jump to the OS call all the _remove() functions?
> >
> > Exiting U-Boot is irrelevant to maintaining boot options.
> 
> In theory yes,  but the reality is that you can't remove the boot
> options and rewrite flashes of embedded boards, with a limited
> lifetime, every time you boot your OS.  So adding it on the _remove

Here you have much more explanation than the original message
has for case(2). It is another reason why the commit message is
not accurate (or I should say 'not enough'). Without those texts,
most people won't understand what the commit message tries to say.

-Takahiro Akashi

> We could in the future add an extra flag to the
> callback *now* is a bad idea and that's what the commit message is
> trying to hint.
> remove callback which only gets set to false when we are exiting
> u-boot

> Thanks
> /Ilias
> >
> > -Takahiro Akashi
> >
> > > Thanks
> > > /Ilias
> > > >
> > > > -Takahiro Akashi
> > > >
> > > > > Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
> > > > > Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > > > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > > ---
> > > > > Changes in v3
> > > > > - Split the patch into moving and renaming functions and
> > > > >   individual patches for each changed functionality
> > > > > Changes in v5
> > > > > - Move function call of efi_bootmgr_update_media_device_boot_option()
> > > > >   from efi_init_variables() to efi_init_obj_list()
> > > > > Changes in v6
> > > > > - Revert unrelated changes
> > > > > Changes in v7
> > > > > - adapt the return code of function
> > > > >   efi_bootmgr_update_media_device_boot_option()
> > > > > Changes in v8
> > > > > - add a note in the commit message for future reference
> > > > >
> > > > >  lib/efi_loader/efi_disk.c  | 7 +++++++
> > > > >  lib/efi_loader/efi_setup.c | 5 +++++
> > > > >  2 files changed, 12 insertions(+)
> > > > >
> > > > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> > > > > index d2256713a8..d9b48ecf64 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 != EFI_SUCCESS)
> > > > > +                     return -1;
> > > > > +     }
> > > > > +
> > > > >       return 0;
> > > > >  }
> > > > >
> > > > > diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> > > > > index 58d4e13402..877f3878d6 100644
> > > > > --- a/lib/efi_loader/efi_setup.c
> > > > > +++ b/lib/efi_loader/efi_setup.c
> > > > > @@ -245,6 +245,11 @@ efi_status_t efi_init_obj_list(void)
> > > > >       if (ret != EFI_SUCCESS)
> > > > >               goto out;
> > > > >
> > > > > +     /* update boot option after variable service initialized */
> > > > > +     ret = efi_bootmgr_update_media_device_boot_option();
> > > > > +     if (ret != EFI_SUCCESS)
> > > > > +             goto out;
> > > > > +
> > > > >       /* Define supported languages */
> > > > >       ret = efi_init_platform_lang();
> > > > >       if (ret != EFI_SUCCESS)
> > > > > --
> > > > > 2.25.1
> > > > >

Re: [PATCH v8 3/4] Boot var automatic management for removable medias

[Ilias Apalodimas]

On Wed, 7 Jun 2023 at 10:27, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
>
> On Wed, Jun 07, 2023 at 10:00:37AM +0300, Ilias Apalodimas wrote:
> > On Wed, 7 Jun 2023 at 09:52, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > >
> > > On Wed, Jun 07, 2023 at 09:38:30AM +0300, Ilias Apalodimas wrote:
> > > > On Wed, 7 Jun 2023 at 03:41, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > > > >
> > > > > On Tue, Jun 06, 2023 at 09:37:20AM -0700, 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.
> > > > > > Please note that currently the function 'efi_disk_remove' is being
> > > > > > invoked during exiting U-Boot and booting an OS - that is why we
> > > > > > don't add the boot variables management inside 'efi_disk_remove'.
> > > > >
> > > > > It's not accurate.
> > > > > For instance, a user may run "scsi scan" or "usb reset" to re-enumerate
> > > > > devices added (or removed). They will invoke "remove" against pre-existing
> > > > > devices before invoking a "probe" routine.
> > > > >
> > > > > So the text above doesn't justify not implementing "remove" hook.
> > > >
> > > > I am not sure I am following you here.  Yes resetting the device will
> > > > call the remove callback and it's a current problem of the existing
> > > > implementation,
> > >
> > > I think it is a design decision.
> >
> > It's not.  Ideally, we should call the _remove() callbacks and
> > reconfigure the boot options correctly if a device gets unplugged or
> > the subsystem is restarted. However, at least in my opinion, the
> > problem isn't that big at the moment to prevent the patches from
> > getting merged.
> > If you restart the subsystem the same boot options will be rescanned
> > and the device will be able to boot.  The corner case here is scanning
> > a few boot options and then *removing* the device.  But in that case,
> > the device wasn't going to boot anyway from the removed media.
>
> First of all, we should distinguish two cases when you mention
> the *existing* issues in Raymond's current implementation:
> 1. Some device is added or removed when we are still on U-Boot
> 2. When we are existing U-Boot
>
> In the above text, you mention case(1) which is not covered by
> his original commit message. It is one reason why I think that the commit
> message is not accurate.
>
> > >
> > > > but why is the commit message inaccurate?  Won't
> > > > exiting u-boot to jump to the OS call all the _remove() functions?
> > >
> > > Exiting U-Boot is irrelevant to maintaining boot options.
> >
> > In theory yes,  but the reality is that you can't remove the boot
> > options and rewrite flashes of embedded boards, with a limited
> > lifetime, every time you boot your OS.  So adding it on the _remove
>
> Here you have much more explanation than the original message
> has for case(2). It is another reason why the commit message is
> not accurate (or I should say 'not enough'). Without those texts,
> most people won't understand what the commit message tries to say.
>

Fair enough.  Raymond can you resend the series and add
- a more detailed explanation of why we are not removing the options
in the _remove() callback currently
- a comment in the function that explains the same thing for future reference?

Thanks
/Ilias
> -Takahiro Akashi
>
> > We could in the future add an extra flag to the
> > callback *now* is a bad idea and that's what the commit message is
> > trying to hint.
> > remove callback which only gets set to false when we are exiting
> > u-boot
>
> > Thanks
> > /Ilias
> > >
> > > -Takahiro Akashi
> > >
> > > > Thanks
> > > > /Ilias
> > > > >
> > > > > -Takahiro Akashi
> > > > >
> > > > > > Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
> > > > > > Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > > > > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > > > ---
> > > > > > Changes in v3
> > > > > > - Split the patch into moving and renaming functions and
> > > > > >   individual patches for each changed functionality
> > > > > > Changes in v5
> > > > > > - Move function call of efi_bootmgr_update_media_device_boot_option()
> > > > > >   from efi_init_variables() to efi_init_obj_list()
> > > > > > Changes in v6
> > > > > > - Revert unrelated changes
> > > > > > Changes in v7
> > > > > > - adapt the return code of function
> > > > > >   efi_bootmgr_update_media_device_boot_option()
> > > > > > Changes in v8
> > > > > > - add a note in the commit message for future reference
> > > > > >
> > > > > >  lib/efi_loader/efi_disk.c  | 7 +++++++
> > > > > >  lib/efi_loader/efi_setup.c | 5 +++++
> > > > > >  2 files changed, 12 insertions(+)
> > > > > >
> > > > > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> > > > > > index d2256713a8..d9b48ecf64 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 != EFI_SUCCESS)
> > > > > > +                     return -1;
> > > > > > +     }
> > > > > > +
> > > > > >       return 0;
> > > > > >  }
> > > > > >
> > > > > > diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> > > > > > index 58d4e13402..877f3878d6 100644
> > > > > > --- a/lib/efi_loader/efi_setup.c
> > > > > > +++ b/lib/efi_loader/efi_setup.c
> > > > > > @@ -245,6 +245,11 @@ efi_status_t efi_init_obj_list(void)
> > > > > >       if (ret != EFI_SUCCESS)
> > > > > >               goto out;
> > > > > >
> > > > > > +     /* update boot option after variable service initialized */
> > > > > > +     ret = efi_bootmgr_update_media_device_boot_option();
> > > > > > +     if (ret != EFI_SUCCESS)
> > > > > > +             goto out;
> > > > > > +
> > > > > >       /* Define supported languages */
> > > > > >       ret = efi_init_platform_lang();
> > > > > >       if (ret != EFI_SUCCESS)
> > > > > > --
> > > > > > 2.25.1
> > > > > >

Re: [PATCH v8 3/4] Boot var automatic management for removable medias

[Raymond Mao]

Hi Ilias,

I will amend the note in the commit message and add a TODO into the
function efi_disk_remove() according to our discussion in this email
thread.

Raymond

On Wed, 7 Jun 2023 at 03:45, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Wed, 7 Jun 2023 at 10:27, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> >
> > On Wed, Jun 07, 2023 at 10:00:37AM +0300, Ilias Apalodimas wrote:
> > > On Wed, 7 Jun 2023 at 09:52, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > > >
> > > > On Wed, Jun 07, 2023 at 09:38:30AM +0300, Ilias Apalodimas wrote:
> > > > > On Wed, 7 Jun 2023 at 03:41, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > > > > >
> > > > > > On Tue, Jun 06, 2023 at 09:37:20AM -0700, 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.
> > > > > > > Please note that currently the function 'efi_disk_remove' is being
> > > > > > > invoked during exiting U-Boot and booting an OS - that is why we
> > > > > > > don't add the boot variables management inside 'efi_disk_remove'.
> > > > > >
> > > > > > It's not accurate.
> > > > > > For instance, a user may run "scsi scan" or "usb reset" to re-enumerate
> > > > > > devices added (or removed). They will invoke "remove" against pre-existing
> > > > > > devices before invoking a "probe" routine.
> > > > > >
> > > > > > So the text above doesn't justify not implementing "remove" hook.
> > > > >
> > > > > I am not sure I am following you here.  Yes resetting the device will
> > > > > call the remove callback and it's a current problem of the existing
> > > > > implementation,
> > > >
> > > > I think it is a design decision.
> > >
> > > It's not.  Ideally, we should call the _remove() callbacks and
> > > reconfigure the boot options correctly if a device gets unplugged or
> > > the subsystem is restarted. However, at least in my opinion, the
> > > problem isn't that big at the moment to prevent the patches from
> > > getting merged.
> > > If you restart the subsystem the same boot options will be rescanned
> > > and the device will be able to boot.  The corner case here is scanning
> > > a few boot options and then *removing* the device.  But in that case,
> > > the device wasn't going to boot anyway from the removed media.
> >
> > First of all, we should distinguish two cases when you mention
> > the *existing* issues in Raymond's current implementation:
> > 1. Some device is added or removed when we are still on U-Boot
> > 2. When we are existing U-Boot
> >
> > In the above text, you mention case(1) which is not covered by
> > his original commit message. It is one reason why I think that the commit
> > message is not accurate.
> >
> > > >
> > > > > but why is the commit message inaccurate?  Won't
> > > > > exiting u-boot to jump to the OS call all the _remove() functions?
> > > >
> > > > Exiting U-Boot is irrelevant to maintaining boot options.
> > >
> > > In theory yes,  but the reality is that you can't remove the boot
> > > options and rewrite flashes of embedded boards, with a limited
> > > lifetime, every time you boot your OS.  So adding it on the _remove
> >
> > Here you have much more explanation than the original message
> > has for case(2). It is another reason why the commit message is
> > not accurate (or I should say 'not enough'). Without those texts,
> > most people won't understand what the commit message tries to say.
> >
>
> Fair enough.  Raymond can you resend the series and add
> - a more detailed explanation of why we are not removing the options
> in the _remove() callback currently
> - a comment in the function that explains the same thing for future reference?
>
> Thanks
> /Ilias
> > -Takahiro Akashi
> >
> > > We could in the future add an extra flag to the
> > > callback *now* is a bad idea and that's what the commit message is
> > > trying to hint.
> > > remove callback which only gets set to false when we are exiting
> > > u-boot
> >
> > > Thanks
> > > /Ilias
> > > >
> > > > -Takahiro Akashi
> > > >
> > > > > Thanks
> > > > > /Ilias
> > > > > >
> > > > > > -Takahiro Akashi
> > > > > >
> > > > > > > Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
> > > > > > > Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > > > > > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > > > > ---
> > > > > > > Changes in v3
> > > > > > > - Split the patch into moving and renaming functions and
> > > > > > >   individual patches for each changed functionality
> > > > > > > Changes in v5
> > > > > > > - Move function call of efi_bootmgr_update_media_device_boot_option()
> > > > > > >   from efi_init_variables() to efi_init_obj_list()
> > > > > > > Changes in v6
> > > > > > > - Revert unrelated changes
> > > > > > > Changes in v7
> > > > > > > - adapt the return code of function
> > > > > > >   efi_bootmgr_update_media_device_boot_option()
> > > > > > > Changes in v8
> > > > > > > - add a note in the commit message for future reference
> > > > > > >
> > > > > > >  lib/efi_loader/efi_disk.c  | 7 +++++++
> > > > > > >  lib/efi_loader/efi_setup.c | 5 +++++
> > > > > > >  2 files changed, 12 insertions(+)
> > > > > > >
> > > > > > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> > > > > > > index d2256713a8..d9b48ecf64 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 != EFI_SUCCESS)
> > > > > > > +                     return -1;
> > > > > > > +     }
> > > > > > > +
> > > > > > >       return 0;
> > > > > > >  }
> > > > > > >
> > > > > > > diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> > > > > > > index 58d4e13402..877f3878d6 100644
> > > > > > > --- a/lib/efi_loader/efi_setup.c
> > > > > > > +++ b/lib/efi_loader/efi_setup.c
> > > > > > > @@ -245,6 +245,11 @@ efi_status_t efi_init_obj_list(void)
> > > > > > >       if (ret != EFI_SUCCESS)
> > > > > > >               goto out;
> > > > > > >
> > > > > > > +     /* update boot option after variable service initialized */
> > > > > > > +     ret = efi_bootmgr_update_media_device_boot_option();
> > > > > > > +     if (ret != EFI_SUCCESS)
> > > > > > > +             goto out;
> > > > > > > +
> > > > > > >       /* Define supported languages */
> > > > > > >       ret = efi_init_platform_lang();
> > > > > > >       if (ret != EFI_SUCCESS)
> > > > > > > --
> > > > > > > 2.25.1
> > > > > > >

[PATCH v8 4/4] Load option with short device path for boot vars

[Raymond Mao]

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>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
Changes in v2
- Ignore EFI_NOT_FOUND returned from
  efi_bootmgr_update_media_device_boot_option which means no boot
  options scanned.
Changes in v3
- Split the patch into moving and renaming functions and
  individual patches for each changed functionality
Changes in v4
- Revert v2 change of introducing a bool parameter when updating
  the boot option. Use short-form of device path by default
Changes in v5
- Add warning log when a short-form device path doesn't exist
Changes in v6
- Revert v5 change and use original device path if short-form does
  not exist

 lib/efi_loader/efi_bootmgr.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index 28e800499c..398b0f89e2 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -371,6 +371,7 @@ static efi_status_t efi_bootmgr_enumerate_boot_option(struct eficonfig_media_boo
 		struct efi_load_option lo;
 		char buf[BOOTMENU_DEVICE_NAME_MAX];
 		struct efi_device_path *device_path;
+		struct efi_device_path *short_dp;
 
 		ret = efi_search_protocol(volume_handles[i], &efi_guid_device_path, &handler);
 		if (ret != EFI_SUCCESS)
@@ -387,6 +388,11 @@ static efi_status_t efi_bootmgr_enumerate_boot_option(struct eficonfig_media_boo
 		p = dev_name;
 		utf8_utf16_strncpy(&p, buf, strlen(buf));
 
+		/* prefer to short form device path */
+		short_dp = efi_dp_shorten(device_path);
+		if (short_dp)
+			device_path = short_dp;
+
 		lo.label = dev_name;
 		lo.attributes = LOAD_OPTION_ACTIVE;
 		lo.file_path = device_path;
-- 
2.25.1