Hi Ilias,

On Thu, 28 Dec 2023 at 22:14, Ilias Apalodimas ilias.apalodimas@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

Hi Heinrich,

On Wed, 10 Jan 2024 at 22:53, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

On 26.12.23 07:28, Masahisa Kojima wrote:

...

Current efibootmgr auto-generates the boot option for all disks and partitions installing EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, while EDK II reference implementation auto-generates the boot option for all devices installing EFI_BLOCK_IO_PROTOCOL with eliminating logical partitions.

This commit modifies the efibootmgr to get aligned to EDK II.

Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org

lib/efi_loader/efi_bootmgr.c | 94 +++++++++++++++++++++++++++--------- 1 file changed, 71 insertions(+), 23 deletions(-)

diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index 56d97f2382..747f75ee82 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -560,6 +560,50 @@ err: return ret; }

+/**

• • try_load_from_media() - load file from media
• • @file_path: file path
• • @handle: pointer to handle for newly installed image

Please, use the same description as in try_load_entry():

 on return handle for the newly installed image

OK.

...

• • If @file_path contains a file name, load the file.
• • If @file_path does not have a file name, search the architecture-specific
• • default file and load it.
• • TODO: If the FilePathList[0] device does not support
• • EFI_SIMPLE_FILE_SYSTEM_PROTOCOL but supports EFI_BLOCK_IO_PROTOCOL,
• • call EFI_BOOT_SERVICES.ConnectController()
• • TODO: FilePathList[0] device supports EFI_SIMPLE_FILE_SYSTEM_PROTOCOL
• • not based on EFI_BLOCK_IO_PROTOCOL
• • Return: status code
• */

+static efi_status_t try_load_from_media(struct efi_device_path *file_path,

•                                 efi_handle_t *handle)
  

%s/handle/handle_img/

OK.

or h_img

...

+{

• efi_handle_t h;
  

%s/h/handle_blkdev/

OK.

or h_blkdev

...

• efi_status_t ret = EFI_SUCCESS;
  

• struct efi_device_path *rem, *dp = NULL;
  

• struct efi_device_path *final_dp = file_path;
  

• h = efi_dp_find_obj(file_path, &efi_block_io_guid, &rem);
  

• if (h) {
  

•         if (rem->type == DEVICE_PATH_TYPE_END) {
  

•                 /* no file name present, try default file */
  

•                 ret = check_disk_has_default_file(h->dev, &dp);
  

check_disk_has_default_file() does not only check for a default file path , i.e. EFI/BOOT/BOO<ARCH>.EFI, but will check for a given file path, if provided. In a later patch we should rename this function to something less misleading.

OK. I will include a renaming patch in this series.

...

•                 if (ret != EFI_SUCCESS)
  

•                         return ret;
  

•                 final_dp = dp;
  

•         }
  

• }
  

• ret = EFI_CALL(efi_load_image(true, efi_root, final_dp, NULL, 0, handle));
  

• efi_free_pool(dp);
  

• return ret;
  

+}

• /**
  • 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

%s/efi_bootmgr_enumerate_boot_option/efi_bootmgr_enumerate_boot_options/

OK.

...

• @opt: pointer to the media boot option structure

• • @volume_handles: pointer to the efi handles

• • @handles: pointer to the efi handles

%s/efi/EFI/

"pointer to the efi handles" could be handles for anything.

%s/pointer to the efi handles/pointer to block device handles/

OK.

We should move the call to LocateBufferHandle into this function as the caller does not use handles.

But no_handles is required in the caller(efi_bootmgr_update_media_device_boot_option) to allocate an opt array. opt = calloc(count, sizeof(struct eficonfig_media_boot_option));

...

• @count: number of efi handle

On entry number of handles to block devices. On exit number of boot options.

OK.

...

• 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) {
                  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;
  

  }

@@ -1040,8 +1090,7 @@ efi_status_t efi_bootmgr_delete_boot_option(u16 boot_index) /**

• efi_bootmgr_update_media_device_boot_option() - generate the media device boot option

• • This function enumerates all devices supporting EFI_SIMPLE_FILE_SYSTEM_PROTOCOL
• • and generate the bootmenu entries.

• • This function enumerates all BlockIo devices and add the boot option for it.
  • This function also provide the BOOT#### variable maintenance for
  • the media device entries.
  • • Automatically create the BOOT#### variable for the newly detected device,

@@ -1057,13 +1106,13 @@ efi_status_t efi_bootmgr_update_media_device_boot_option(void) u32 i; efi_status_t ret; efi_uintn_t count;

• efi_handle_t *volume_handles = NULL;
  

• efi_handle_t *handles = NULL;
  struct eficonfig_media_boot_option *opt = NULL;
  
  ret = efi_locate_handle_buffer_int(BY_PROTOCOL,
  

•                                    &efi_simple_file_system_protocol_guid,
  

•                                    &efi_block_io_guid,
                                     NULL, &count,
  

•                                    (efi_handle_t **)&volume_handles);
  

•                                    (efi_handle_t **)&handles);
  

This is the call that should be moved to efi_bootmgr_enumerate_boot_options().

no_handles(count) is required to allocate an opt array. So let me keep efi_locate_handle_buffer_int() call here.

...

  if (ret != EFI_SUCCESS)
          goto out;

@@ -1073,8 +1122,7 @@ efi_status_t efi_bootmgr_update_media_device_boot_option(void) goto out; }

• /* enumerate all devices supporting EFI_SIMPLE_FILE_SYSTEM_PROTOCOL */
  

• ret = gg(opt, volume_handles, count);
  

• ret = efi_bootmgr_enumerate_boot_option(opt, handles, &count);
  

%s/, handles//

Let me keep it as is.

...

  if (ret != EFI_SUCCESS)
          goto out;

@@ -1122,7 +1170,7 @@ out: free(opt[i].lo); } free(opt);

• efi_free_pool(volume_handles);
  

• efi_free_pool(handles);
  

To be moved to efi_bootmgr_enumerate_boot_options().

Let me keep it as is.

Thanks, Masahisa Kojima

Best regards

Heinrich

...

  if (ret == EFI_NOT_FOUND)
          return EFI_SUCCESS;