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

Masahisa Kojima masahisa.kojima at linaro.org
Wed Dec 7 08:19:55 CET 2022


On Tue, 6 Dec 2022 at 23:12, Ilias Apalodimas
<ilias.apalodimas at 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.apalodimas at 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.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?
> > >
> > > > +     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


More information about the U-Boot mailing list