[PATCH v2 1/2] efi_loader: auto-generate boot option for each blkio device

[Masahisa Kojima]

Hi Ilias,

On Thu, 28 Dec 2023 at 22:14, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Kojima-san
>
> [...]
>
> >  /**
> >   * try_load_entry() - try to load image for boot option
> >   *
> > @@ -594,7 +638,6 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle,
> >       }
> >
> >       if (lo.attributes & LOAD_OPTION_ACTIVE) {
> > -             struct efi_device_path *file_path;
> >               u32 attributes;
> >
> >               log_debug("trying to load \"%ls\" from %pD\n", lo.label,
> > @@ -611,10 +654,7 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle,
> >                       else
> >                               ret = EFI_LOAD_ERROR;
> >               } else {
> > -                     file_path = expand_media_path(lo.file_path);
> > -                     ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
> > -                                                   NULL, 0, handle));
> > -                     efi_free_pool(file_path);
> > +                     ret = try_load_from_media(lo.file_path, handle);
> >               }
> >               if (ret != EFI_SUCCESS) {
> >                       log_warning("Loading %ls '%ls' failed\n",
> > @@ -748,19 +788,19 @@ error:
> >   * efi_bootmgr_enumerate_boot_option() - enumerate the possible bootable media
> >   *
> >   * @opt:             pointer to the media boot option structure
> > - * @volume_handles:  pointer to the efi handles
> > + * @handles:         pointer to the efi handles
> >   * @count:           number of efi handle
> >   * Return:           status code
> >   */
> >  static efi_status_t efi_bootmgr_enumerate_boot_option(struct eficonfig_media_boot_option *opt,
> > -                                                   efi_handle_t *volume_handles,
> > -                                                   efi_status_t count)
> > +                                                   efi_handle_t *handles,
> > +                                                   efi_uintn_t *count)
> >  {
> > -     u32 i;
> > +     u32 i, num = 0;
> >       struct efi_handler *handler;
> >       efi_status_t ret = EFI_SUCCESS;
> >
> > -     for (i = 0; i < count; i++) {
> > +     for (i = 0; i < *count; i++) {
> >               u16 *p;
> >               u16 dev_name[BOOTMENU_DEVICE_NAME_MAX];
> >               char *optional_data;
> > @@ -768,8 +808,15 @@ static efi_status_t efi_bootmgr_enumerate_boot_option(struct eficonfig_media_boo
> >               char buf[BOOTMENU_DEVICE_NAME_MAX];
> >               struct efi_device_path *device_path;
> >               struct efi_device_path *short_dp;
> > +             struct efi_block_io *blkio;
> > +
> > +             ret = efi_search_protocol(handles[i], &efi_block_io_guid, &handler);
> > +             blkio = handler->protocol_interface;
> >
> > -             ret = efi_search_protocol(volume_handles[i], &efi_guid_device_path, &handler);
> > +             if (blkio->media->logical_partition)
> > +                     continue;
> > +
> > +             ret = efi_search_protocol(handles[i], &efi_guid_device_path, &handler);
> >               if (ret != EFI_SUCCESS)
> >                       continue;
> >               ret = efi_protocol_open(handler, (void **)&device_path,
> > @@ -777,7 +824,7 @@ static efi_status_t efi_bootmgr_enumerate_boot_option(struct eficonfig_media_boo
> >               if (ret != EFI_SUCCESS)
> >                       continue;
> >
> > -             ret = efi_disk_get_device_name(volume_handles[i], buf, BOOTMENU_DEVICE_NAME_MAX);
> > +             ret = efi_disk_get_device_name(handles[i], buf, BOOTMENU_DEVICE_NAME_MAX);
> >               if (ret != EFI_SUCCESS)
> >                       continue;
> >
> > @@ -801,17 +848,20 @@ static efi_status_t efi_bootmgr_enumerate_boot_option(struct eficonfig_media_boo
> >                * to store guid, instead of realloc the load_option.
> >                */
> >               lo.optional_data = "1234567";
> > -             opt[i].size = efi_serialize_load_option(&lo, (u8 **)&opt[i].lo);
> > -             if (!opt[i].size) {
> > +             opt[num].size = efi_serialize_load_option(&lo, (u8 **)&opt[num].lo);
> > +             if (!opt[num].size) {
>
> I am a bit lost here. Why are we using 'num' instead of 'i'? The opt
> variable contains all media boot options with a block_io_protocol right?
> Aren't we going to copy the wrong load options in optional_data if we don't use
> 'i'?

opt[] array is a container of valid boot options with a block_io_protocol
to be auto-generated by efi bootmgr.
handles[] array contains all block_io_protocol efi handles.

With this patch, we ignore the logical partition, opt[] array does not contain
the boot options of logical partitions.
So 'num' instead of 'i' is correct here.

Thanks,
Masahisa Kojima

>
> >                       ret = EFI_OUT_OF_RESOURCES;
> >                       goto out;
> >               }
> >               /* set the guid */
> > -             optional_data = (char *)opt[i].lo + (opt[i].size - u16_strsize(u"1234567"));
> > +             optional_data = (char *)opt[num].lo + (opt[num].size - u16_strsize(u"1234567"));
> >               memcpy(optional_data, &efi_guid_bootmenu_auto_generated, sizeof(efi_guid_t));
> > +             num++;
> >       }
> >
> >  out:
> > +     *count = num;
> > +
> >       return ret;
> >  }
> >
>
> [...]
>
> Thanks
> /Ilias