[PATCH v3 1/2] cmd: eficonfig: add support for URI device path based boot options

[Sughosh Ganu]

On Tue, 1 Jul 2025 at 13:54, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>
> On Tue, 1 Jul 2025 at 13:21, Ilias Apalodimas
> <ilias.apalodimas at linaro.org> wrote:
> >
> > Hi Sughosh
> >
> > [...]
> >
> > > +
> > >  static struct eficonfig_item select_file_menu_items[] = {
> > >         {"Select File", eficonfig_process_select_file},
> > >         {"Clear", eficonfig_process_clear_file_selection},
> > > @@ -1042,16 +1101,30 @@ static struct eficonfig_item select_file_menu_items[] = {
> > >  efi_status_t eficonfig_process_show_file_option(void *data)
> > >  {
> > >         efi_status_t ret;
> > > +       unsigned int menu_count;
> > >         struct efimenu *efi_menu;
> > > +       struct eficonfig_item *menu_items;
> > > +       struct eficonfig_select_file_info *file_info = data;
> > > +
> > > +       menu_items = file_info->boot_file_option ? select_boot_file_menu_items :
> > > +               select_file_menu_items;
> >
> > Why do we need boot_file_option? Won't the len of the URI be 0 if it's a file?
>
> This is to determine if the menu shows the URI option, or not. The
> same function, eficonfig_process_show_file_option() gets called for
> selection of the boot file, initrd file and the fdt file. The URI
> option only needs to be shown in the first case and not the other two.
> The URI string will get populated at a later stage, only after the
> user has input the string, after selecting the menu option.

What you are suggesting can be done.  We need to check if the uri
pointer is non NULL instead, as the pointer will get allocated only
for the file_info member of struct eficonfig_boot_option. So it is
indeed possible to do away with the bool member being added. Thanks.

-sughosh

>
> >
> > [...]
> >
> > >
> > > +/**
> > > + * fill_dp_uri() - copy the URI string in the device path
> > > + * @dp:                pointer to the URI device path
> > > + * @uri_str:   URI string to be copied
> > > + *
> > > + * Copy the passed URI string to the URI device path. This
> > > + * requires utf8_utf16_strcpy() to copy the u16 string to
> > > + * the u8 array in the device path structure.
> > > + *
> > > + * Return: None
> > > + */
> > > +static void fill_dp_uri(struct efi_device_path *dp, u16 **uri_str)
> > > +{
> > > +       u16 *p = *uri_str;
> > > +       struct efi_device_path_uri *uridp;
> > > +
> > > +       uridp = (struct efi_device_path_uri *)dp;
> > > +
> > > +       utf8_utf16_strcpy(&p, uridp->uri);
> > > +}
> > > +
> > >  /**
> > >   * fill_file_info() - fill the file info from efi_device_path structure
> > >   *
> > > @@ -1392,10 +1486,13 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
> > >         size_t len;
> > >         efi_status_t ret;
> > >         char *tmp = NULL, *p;
> > > +       u16 *current_path = NULL;
> > >         struct efi_load_option lo = {0};
> > >         efi_uintn_t dp_size;
> > >         struct efi_device_path *dp = NULL;
> > >         efi_uintn_t size = load_option_size;
> > > +       struct efi_device_path *dp_volume = NULL;
> > > +       struct efi_device_path *uri_dp = NULL;
> > >         struct efi_device_path *device_dp = NULL;
> > >         struct efi_device_path *initrd_dp = NULL;
> > >         struct efi_device_path *fdt_dp = NULL;
> > > @@ -1464,6 +1561,14 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
> > >                 goto out;
> > >         }
> > >
> > > +       bo->file_info.uri = calloc(1, EFICONFIG_URI_MAX * sizeof(u16));
> > > +       if (!bo->file_info.uri) {
> > > +               ret =  EFI_OUT_OF_RESOURCES;
> > > +               goto out;
> > > +       }
> > > +
> > > +       bo->file_info.boot_file_option = true;
> > > +
> > >         /* copy the preset value */
> > >         if (load_option) {
> > >                 ret = efi_deserialize_load_option(&lo, load_option, &size);
> > > @@ -1481,7 +1586,10 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
> > >                 u16_strcpy(bo->description, lo.label);
> > >
> > >                 /* EFI image file path is a first instance */
> > > -               if (lo.file_path)
> > > +               if (lo.file_path && EFI_DP_TYPE(lo.file_path, MESSAGING_DEVICE,
> > > +                                               MSG_URI))
> > > +                       fill_dp_uri(lo.file_path, &bo->file_info.uri);
> > > +               else
> >
> > This else does something different now. Shouldn't it be else if (lo.file_path)>
>
> Yes. The else needs to be modified accordingly. A null lo.file_path
> does not cause a crash, but it is better to put that check to avoid
> unnecessary function calls. Thanks.
>
> -sughosh
>
> >
> > >                         fill_file_info(lo.file_path, &bo->file_info, device_dp);
> > >
> >
> > [...]
> >
> > Thanks
> > /Ilias