On Tue, 6 Dec 2022 at 23:12, Ilias Apalodimas <ilias.apalodi...@linaro.org> wrote: > > On Sat, Dec 03, 2022 at 09:56:20AM +0900, Masahisa Kojima wrote: > > On Fri, 2 Dec 2022 at 16:36, Ilias Apalodimas > > <ilias.apalodi...@linaro.org> wrote: > > > > > > On Fri, Dec 02, 2022 at 01:59:36PM +0900, Masahisa Kojima wrote: > > > > eficonfig command reads all possible UEFI load options > > > > from 0x0000 to 0xFFFF to construct the menu. This takes too much > > > > time in some environment. > > > > This commit uses efi_get_next_variable_name_int() to read all > > > > existing UEFI load options to significantlly reduce the count of > > > > efi_get_var() call. > > > > > > > > Signed-off-by: Masahisa Kojima <masahisa.koj...@linaro.org> > > > > --- > > > > No update since v2 > > > > > > > > v1->v2: > > > > - totaly change the implemention, remove new Kconfig introduced in v1. > > > > - use efi_get_next_variable_name_int() to read the all existing > > > > UEFI variables, then enumerate the "Boot####" variables > > > > - this commit does not provide the common function to enumerate > > > > all "Boot####" variables. I think common function does not > > > > simplify the implementation, because caller of > > > > efi_get_next_variable_name_int() > > > > needs to remember original buffer size, new buffer size and buffer > > > > address > > > > and it is a bit complicated when we consider to create common > > > > function. > > > > > > > > cmd/eficonfig.c | 141 +++++++++++++++++++++++++++++++++++++++--------- > > > > 1 file changed, 117 insertions(+), 24 deletions(-) > > > > > > > > diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c > > > > index 88d507d04c..394ae67cce 100644 > > > > --- a/cmd/eficonfig.c > > > > +++ b/cmd/eficonfig.c > > > > @@ -1683,7 +1683,8 @@ static efi_status_t > > > > eficonfig_show_boot_selection(unsigned int *selected) > > > > u32 i; > > > > u16 *bootorder; > > > > efi_status_t ret; > > > > - efi_uintn_t num, size; > > > > + u16 *var_name16 = NULL, *p; > > > > + efi_uintn_t num, size, buf_size; > > > > struct efimenu *efi_menu; > > > > struct list_head *pos, *n; > > > > struct eficonfig_entry *entry; > > > > @@ -1707,14 +1708,43 @@ static efi_status_t > > > > eficonfig_show_boot_selection(unsigned int *selected) > > > > } > > > > > > > > /* list the remaining load option not included in the BootOrder */ > > > > - for (i = 0; i <= 0xFFFF; i++) { > > > > - /* If the index is included in the BootOrder, skip it */ > > > > - if (search_bootorder(bootorder, num, i, NULL)) > > > > - continue; > > > > + buf_size = 128; > > > > + var_name16 = malloc(buf_size); > > > > + if (!var_name16) > > > > + return EFI_OUT_OF_RESOURCES; > > > > > > > > - ret = eficonfig_add_boot_selection_entry(efi_menu, i, > > > > selected); > > > > - if (ret != EFI_SUCCESS) > > > > - goto out; > > > > + var_name16[0] = 0; > > > > > > Is it worth using calloc instead of malloc and get rid of this? > > > > > > > + for (;;) { > > > > + int index; > > > > + efi_guid_t guid; > > > > + > > > > + size = buf_size; > > > > + ret = efi_get_next_variable_name_int(&size, var_name16, > > > > &guid); > > > > + if (ret == EFI_NOT_FOUND) > > > > + break; > > > > + if (ret == EFI_BUFFER_TOO_SMALL) { > > > > + buf_size = size; > > > > + p = realloc(var_name16, buf_size); > > > > + if (!p) { > > > > + free(var_name16); > > > > + return EFI_OUT_OF_RESOURCES; > > > > + } > > > > + var_name16 = p; > > > > + ret = efi_get_next_variable_name_int(&size, > > > > var_name16, &guid); > > > > + } > > > > + if (ret != EFI_SUCCESS) { > > > > + free(var_name16); > > > > + return ret; > > > > + } > > > > + 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)) > > > > + continue; > > > > + > > > > + ret = > > > > eficonfig_add_boot_selection_entry(efi_menu, index, selected); > > > > + if (ret != EFI_SUCCESS) > > > > + goto out; > > > > + } > > > > > > > > if (efi_menu->count >= EFICONFIG_ENTRY_NUM_MAX - 1) > > > > break; > > > > @@ -1732,6 +1762,8 @@ out: > > > > } > > > > eficonfig_destroy(efi_menu); > > > > > > > > + free(var_name16); > > > > + > > > > return ret; > > > > } > > > > > > > > @@ -1994,6 +2026,8 @@ static efi_status_t > > > > eficonfig_create_change_boot_order_entry(struct efimenu *efi > > > > u32 i; > > > > char *title; > > > > efi_status_t ret; > > > > + u16 *var_name16 = NULL, *p; > > > > + efi_uintn_t size, buf_size; > > > > > > > > /* list the load option in the order of BootOrder variable */ > > > > for (i = 0; i < num; i++) { > > > > @@ -2006,17 +2040,45 @@ static efi_status_t > > > > eficonfig_create_change_boot_order_entry(struct efimenu *efi > > > > } > > > > > > > > /* list the remaining load option not included in the BootOrder */ > > > > - for (i = 0; i < 0xFFFF; i++) { > > > > + 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; > > > > + > > > > if (efi_menu->count >= EFICONFIG_ENTRY_NUM_MAX - 2) > > > > break; > > > > > > > > - /* If the index is included in the BootOrder, skip it */ > > > > - if (search_bootorder(bootorder, num, i, NULL)) > > > > - continue; > > > > - > > > > - ret = eficonfig_add_change_boot_order_entry(efi_menu, i, > > > > false); > > > > + size = buf_size; > > > > + ret = efi_get_next_variable_name_int(&size, var_name16, > > > > &guid); > > > > + if (ret == EFI_NOT_FOUND) > > > > + break; > > > > + if (ret == EFI_BUFFER_TOO_SMALL) { > > > > + buf_size = size; > > > > + p = realloc(var_name16, buf_size); > > > > + if (!p) { > > > > + ret = EFI_OUT_OF_RESOURCES; > > > > + goto out; > > > > + } > > > > + var_name16 = p; > > > > + ret = efi_get_next_variable_name_int(&size, > > > > var_name16, &guid); > > > > + } > > > > if (ret != EFI_SUCCESS) > > > > goto out; > > > > + > > > > + 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)) > > > > + continue; > > > > + > > > > + ret = > > > > eficonfig_add_change_boot_order_entry(efi_menu, index, false); > > > > + if (ret != EFI_SUCCESS) > > > > + goto out; > > > > + } > > > > } > > > > > > > > /* add "Save" and "Quit" entries */ > > > > @@ -2035,9 +2097,9 @@ static efi_status_t > > > > eficonfig_create_change_boot_order_entry(struct efimenu *efi > > > > goto out; > > > > > > > > efi_menu->active = 0; > > > > - > > > > - return EFI_SUCCESS; > > > > out: > > > > + free(var_name16); > > > > + > > > > return ret; > > > > } > > > > > > > > @@ -2270,17 +2332,48 @@ out: > > > > efi_status_t eficonfig_delete_invalid_boot_option(struct > > > > eficonfig_media_boot_option *opt, > > > > efi_status_t count) > > > > { > > > > - u32 i, j; > > > > + u32 i; > > > > efi_uintn_t size; > > > > void *load_option; > > > > struct efi_load_option lo; > > > > + u16 *var_name16 = NULL, *p; > > > > u16 varname[] = u"Boot####"; > > > > efi_status_t ret = EFI_SUCCESS; > > > > + efi_uintn_t varname_size, buf_size; > > > > > > > > - for (i = 0; i <= 0xFFFF; i++) { > > > > + 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; > > > > > > > > - efi_create_indexed_name(varname, sizeof(varname), "Boot", > > > > i); > > > > + varname_size = buf_size; > > > > + ret = efi_get_next_variable_name_int(&varname_size, > > > > var_name16, &guid); > > > > + if (ret == EFI_NOT_FOUND) > > > > + break; > > > > + if (ret == EFI_BUFFER_TOO_SMALL) { > > > > + buf_size = varname_size; > > > > + p = realloc(var_name16, buf_size); > > > > + if (!p) { > > > > + free(var_name16); > > > > + return EFI_OUT_OF_RESOURCES; > > > > + } > > > > + var_name16 = p; > > > > + ret = > > > > efi_get_next_variable_name_int(&varname_size, var_name16, &guid); > > > > + } > > > > + if (ret != EFI_SUCCESS) { > > > > + free(var_name16); > > > > + return ret; > > > > + } > > > > + 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; > > > > @@ -2292,15 +2385,15 @@ efi_status_t > > > > eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_op > > > > > > > > if (size >= sizeof(efi_guid_bootmenu_auto_generated)) { > > > > if (guidcmp(lo.optional_data, > > > > &efi_guid_bootmenu_auto_generated) == 0) { > > > > - for (j = 0; j < count; j++) { > > > > - if (opt[j].size == tmp && > > > > - memcmp(opt[j].lo, > > > > load_option, tmp) == 0) { > > > > - opt[j].exist = true; > > > > + for (i = 0; i < count; i++) { > > > > + if (opt[i].size == tmp && > > > > + memcmp(opt[i].lo, > > > > load_option, tmp) == 0) { > > > > + opt[i].exist = true; > > > > break; > > > > } > > > > } > > > > > > > > - if (j == count) { > > > > + if (i == count) { > > > > ret = delete_boot_option(i); > > > > if (ret != EFI_SUCCESS) { > > > > free(load_option); > > > > -- > > > > 2.17.1 > > > > > > > > > > Overall this looks correct and a good idea. > > > The sequence of alloc -> efi_get_next_variable_name_int -> realloc -> > > > efi_get_next_variable_name_int seems common and repeated though. > > > Can we factor that out on a common function? > > > > I tried to factor out these sequences into a common function, but I > > could not find > > proper common function interface. > > Even if we factor out some of these sequences, the caller still should > > take care the cases > > of ret == EFI_NOT_FOUND and ret != EFI_SUCCESS. > > We can factor out the EFI_BUFFER_TOO_SMALL and realloc sequence, but I > > think it will not > > make the caller simpler. > > I think mean the same thing here, but let me make sure. > This snippet seems like it could be it's own function, no? > > ret = efi_get_next_variable_name_int(&size, var_name16, &guid); > if (ret == EFI_NOT_FOUND) > break; > if (ret == EFI_BUFFER_TOO_SMALL) { > buf_size = size; > p = realloc(var_name16, buf_size); > if (!p) { > ret = EFI_OUT_OF_RESOURCES; > goto out; > } > var_name16 = p; > ret = efi_get_next_variable_name_int(&size, var_name16, &guid); > }
OK, I will create a common function. This patch was already merged, I will send a new patch. Thanks, Masahisa Kojima > > Regards > /Ilias > > > > Thanks, > > Masahisa Kojima > > > > > > > > Thanks > > > /Ilias