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