Re: [PATCH] efi_loader: capsule: detect possible ESP device path

...

• • Get a possible efi system partition by expanding a boot option
• • file path.
• • @boot_dev The device path pointing to a boot option
• • Return: The full ESP device path or NULL if fail
• */

+static struct efi_device_path *get_esp_from_boot_option_file_path(struct efi_device_path *boot_dev) +{

•   efi_status_t ret = EFI_SUCCESS;
  

•   efi_handle_t handle;
  

•   struct efi_device_path *dp = boot_dev;
  

•   struct efi_device_path *full_path = NULL;
  

•   ret = EFI_CALL(efi_locate_device_path(&efi_simple_file_system_protocol_guid,
  

•                                         &dp,
  

•                                         &handle));
  

•   if (ret != EFI_SUCCESS)
  

•           ret = EFI_CALL(efi_locate_device_path(&efi_block_io_guid, &dp, &handle));
  

•   /* Expand Media Device Path */
  

I don't see where the device path is expanded in this patch. We already have try_load_from_media() which tries to do something similar. Is anything missing from there?

Looking a bit closer, we do lack calling ConnectController there, but all this should be added to try_load_from_media() not in a different path. Also I don't think we need the Fixes tag

Thanks /Ilias

...

•   if (ret == EFI_SUCCESS && EFI_DP_TYPE(dp, END, END)) {
  

Can you invert the logic here and return immediately? (if ret != EFI_SUCCESS ...etc ) return full_path;

The indentation will go away.

...

•           struct efi_device_path *temp_dp;
  

•           struct efi_block_io *block_io;
  

•           void *buffer;
  

•           efi_handle_t *simple_file_system_handle;
  

•           efi_uintn_t number_handles, index;
  

•           u32 size;
  

•           u32 temp_size;
  

•           temp_dp = boot_dev;
  

•           ret = EFI_CALL(efi_locate_device_path(&efi_simple_file_system_protocol_guid,
  

•                                                 &temp_dp,
  

•                                                 &handle));
  

•           /*
  

•            * For device path pointing to simple file system, it only expands to one
  

•            * full path
  

Why do we have multiple calls to efi_locate_device_path()? Aren't the arguments identical? Which part of edk2 did you read? Is it BmExpandMediaDevicePath()?

...

•            */
  

•           if (ret == EFI_SUCCESS && EFI_DP_TYPE(temp_dp, END, END)) {
  

•                   if (device_is_present_and_system_part(temp_dp))
  

•                           return temp_dp;
  

•           }
  

•           /*
  

•            * For device path only pointing to the removable device handle, try to
  

•            * search all its children handles
  

•            */
  

•           ret = EFI_CALL(efi_locate_device_path(&efi_block_io_guid, &temp_dp, &handle));
  

•           EFI_CALL(efi_connect_controller(handle, NULL, NULL, true));
  

•           /* Align with edk2, issue a dummy read to the device to check the device change */
  

•           ret = EFI_CALL(efi_handle_protocol(handle, &efi_block_io_guid, (void **)&block_io));
  

•           if (ret == EFI_SUCCESS) {
  

•                   buffer = memalign(block_io->media->io_align, block_io->media->block_size);
  

•                   if (buffer) {
  

•                           ret = EFI_CALL(block_io->read_blocks(block_io,
  

•                                                                block_io->media->media_id,
  

•                                                                0,
  

•                                                                block_io->media->block_size,
  

•                                                                buffer));
  

•                           free(buffer);
  

•                   } else {
  

•                           return full_path;
  

•                   }
  

•           } else {
  

•                   return full_path;
  

•           }
  

•           /* detect the default boot file from removable media */
  

•           size = efi_dp_size(boot_dev) - sizeof(struct efi_device_path);
  

•           EFI_CALL(efi_locate_handle_buffer(BY_PROTOCOL,
  

•                                             &efi_simple_file_system_protocol_guid,
  

•                                             NULL,
  

•                                             &number_handles,
  

•                                             &simple_file_system_handle));
  

•           for (index = 0; index < number_handles; index++) {
  

•                   EFI_CALL(efi_handle_protocol(simple_file_system_handle[index],
  

•                                                &efi_guid_device_path,
  

•                                                (void **)&temp_dp));
  

•                   log_debug("Search ESP %pD\n", temp_dp);
  

•                   temp_size = efi_dp_size(temp_dp) - sizeof(struct efi_device_path);
  

•                   if (size <= temp_size && !memcmp(temp_dp, boot_dev, size)) {
  

•                           if (device_is_present_and_system_part(temp_dp)) {
  

•                                   efi_free_pool(simple_file_system_handle);
  

•                                   return temp_dp;
  

•                           }
  

•                   }
  

•           }
  

•           if (simple_file_system_handle)
  

•                   efi_free_pool(simple_file_system_handle);
  

•   }
  

•   return full_path;
  

+}

/**

• find_boot_device - identify the boot device

@@ -938,6 +1037,7 @@ static efi_status_t find_boot_device(void) int i, num; struct efi_simple_file_system_protocol *volume; struct efi_device_path *boot_dev = NULL;

•   struct efi_device_path *full_path = NULL;
    efi_status_t ret;
  
    /* find active boot device in BootNext */
  

@@ -961,8 +1061,14 @@ static efi_status_t find_boot_device(void) if (device_is_present_and_system_part(boot_dev)) { goto found; } else {

•                           efi_free_pool(boot_dev);
  

•                           boot_dev = NULL;
  

•                           full_path = get_esp_from_boot_option_file_path(boot_dev);
  

•                           if (full_path) {
  

•                                   boot_dev = full_path;
  

•                                   goto found;
  

•                           } else {
  

•                                   efi_free_pool(boot_dev);
  

•                                   boot_dev = NULL;
  

•                           }
                    }
            }
    }
  

-- 2.40.1

Thanks /Ilias