[PATCH v3 4/5] eficonfig: use efi_get_next_variable_name_int()
Heinrich Schuchardt
xypron.glpk at gmx.de
Fri Dec 2 17:59:37 CET 2022
On 12/2/22 08:35, Ilias Apalodimas 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.kojima at 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?
It doesn't change the binary code size as var_name16 is already in a
register.
Best regards
Heinrich
>
>> + 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?
>
> Thanks
> /Ilias
More information about the U-Boot
mailing list