Re: [RFC PATCH v3 2/2] bootmenu: add UEFI and disto_boot entries

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

[...]

> > > +                     command = calloc(1, command_size);
> > > +                     if (!command) {
> > > +                             free(entry->title);
> > > +                             free(load_option);
> > > +                             free(entry);
> > > +                             goto cleanup;
> > > +                     }
> > > +                     snprintf(command, command_size, "bootefi bootindex %X", bootorder[j]);
> > > +                     entry->command = command;
> > > +                     sprintf(entry->key, "%d", i);
> > > +                     entry->num = i;
> > > +                     entry->menu = menu;
> > > +                     entry->type = BOOT_TYPE_UEFI;
> > > +                     entry->bootorder = bootorder[j];
> > > +                     entry->next = NULL;
> > > +
> > > +                     if (!iter)
> > > +                             menu->first = entry;
> > > +                     else
> > > +                             iter->next = entry;
> > > +
> > > +                     iter = entry;
> > > +                     ++i;
> > > +             }
> > > +
> > > +             if (i == MAX_COUNT - 1)
> > > +                     break;
> > > +     }
> > > +     free(bootorder);
> > > +}
> > > +
> > > +bootmgr:
> >
> > Why do we need an entire menu entry if the bootorder is not defined?
> > Currently there's no logic in the efibootmgr to look for the standard
> > filenames in the ESP (eg bootarm.efi, bootaa64.efi etc) if no boot option
> > is defined.  Instead this is implement in distro_bootcmd.
> 
> In this version, newly added command "bootefi bootindex XXXX" is called
> if the user selects one of the "Boot####" option, and "bootefi bootindex"
> does not handle "BootNext". That's why I think the menu entry simply
> call "bootefi bootmgr" is required.
> Anyway, I will modify to set "BootNext" and call "bootefi bootmgr" if
> the user selects a "Boot####" option.
> 
> I think we need to consider the case that "BootNext" is already set
> when bootmenu comes up.
> In this case, bootmenu must automatically try to boot the load option
> of "BootNext" without any user interaction.

Good point.  Especially if we fix setvariable at runtime for specific
boards, capsuleupdates will need this

> 
> >
> > I was thinking of something along the lines of:
> > 1. bootmenu comes up
> > 2. We read all the Boot#### variables that are defined and add them on the
> >    menu
> > 2a. If the user doesn't explicitly choose a boot option we run 'bootefi bootmgr'
> > 2b. If the user does select a different option we set BootNext and still
> > run 'bootefi bootmgr'
> > 3. If there's not Boot#### option defined,  we should in the future add the
> >    functionality of searching for bootaa64.efi etc in ESP and still just
> >    launch the efi bootmgr
> 
> Thank you very much for summarizing.
> I would like to update as follows.
> 
> 1. bootmenu comes up
> 2. If the BootNext is already defined, try to boot with BootNext
> without showing bootmenu
> 3. We read the BootOrder, then read Boot#### with the order specified
> by BootOrder and add it on the menu (UEFI spec requires to honor the
> priorities set in BootOrder variable)
> 3a. If the user doesn't explicitly choose a boot option we run 'bootefi bootmgr'
> 3b. If the user does select a different option we set BootNext and still
> run 'bootefi bootmgr'
> 4. If there's not Boot#### option defined,  we should in the future add the
>    functionality of searching for bootaa64.efi etc in ESP and still just
>    launch the efi bootmgr
> 4a. If "bootefi bootmgr" returns, run U-Boot "bootcmd" environment variable.
> 

Yep, that looks correct

> >
> > > +     /* Add UEFI Boot Manager entry if available */
> > > +     if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR)) {

[...]

> > > +                     free(entry);
> > > +                     goto cleanup;
> > > +             }
> >
> > Any idea of how we'll tackle that?  We could export the efibootmgr
> > functions that deal with this and use them on the edit menu ?
> 
> I guess you are referring the RFC patch[*1] I sent before?
> If yes, yes I will reuse them.

Yes

> Or do you mean the "efidebug" command?
> 
> [*1] https://lore.kernel.org/u-boot/20220225013257.15674-5-masahisa.kojima@linaro.org/
> 
> >
> > > +
> > > +             sprintf(entry->key, "%d", i);
> > > +
> > > +             entry->num = i;
> > > +             entry->menu = menu;
> > > +             entry->type = BOOT_TYPE_NONE;
> > > +             entry->next = NULL;
> > > +
> > > +             if (!iter)
> > > +                     menu->first = entry;
> > > +             else
> > > +                     iter->next = entry;
> > > +
> > > +             iter = entry;
> > > +             ++i;
> > >       }
> > >
> > >       /* Add U-Boot console entry at the end */
> > > @@ -353,7 +602,7 @@ static struct bootmenu_data *bootmenu_create(int delay)
> > >               if (!entry)
> > >                       goto cleanup;
> > >
> > > -             entry->title = strdup("U-Boot console");
> > > +             entry->title = u16_strdup(u"U-Boot console");
> >
> > Can we add an option to prohibit the user from going back to the console?
> 
> Yes, I will add this option.
> 

[...]

Thanks
/Ilias