Re: [PATCH v3 4/5] eficonfig: use efi_get_next_variable_name_int()

[Heinrich Schuchardt]

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.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?

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