Hi Weizhao,
On Wed, 8 May 2024 at 14:24, Weizhao Ouyang <o451686...@gmail.com> wrote:
>
> When using CapsuleApp to execute on-disk update, it will choose the
> first boot option as BootNext entry to perform the capsule update after
> a reboot.
This is not entirely accurate. The capsule update on-disk mechanism
will look in the ESP pointed to by BootNext or the highest priority
BootOrder.
But the problem you are describing isn't tied only to capsules. IIUC
if you have a logical partition with bootXXX.efi the current code will
ignore it as well and the automatic selection of the boot option won't
work.
Can you adjust the commit message on v2 and mention the scanning as an
issue with the capsule on disk being the obvious side-effect?
> But auto-generate boot option will ignore the logical
> partition which might be an ESP, thus it could not find the capsule
> file.
>
> Align with the EDK II, detect the possible ESP device path by expanding
> the media path.
>
> Fixes: f86fba8adbf3 ("efi_loader: auto-generate boot option for each blkio
> device")
> Signed-off-by: Weizhao Ouyang <o451686...@gmail.com>
> ---
> include/efi_loader.h | 6 ++
> lib/efi_loader/efi_boottime.c | 15 ++---
> lib/efi_loader/efi_capsule.c | 110 +++++++++++++++++++++++++++++++++-
> 3 files changed, 118 insertions(+), 13 deletions(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 9600941aa327..9d78fa936701 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -683,6 +683,12 @@ efi_status_t efi_protocol_open(struct efi_handler
> *handler,
> void **protocol_interface, void *agent_handle,
> void *controller_handle, uint32_t attributes);
>
> +/* Connect drivers to controller */
> +efi_status_t EFIAPI efi_connect_controller(efi_handle_t controller_handle,
> + efi_handle_t *driver_image_handle,
> + struct efi_device_path
> *remain_device_path,
> + bool recursive);
> +
> /* Install multiple protocol interfaces */
> efi_status_t EFIAPI
> efi_install_multiple_protocol_interfaces(efi_handle_t *handle, ...);
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 1951291747cd..2c86d78208b2 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -103,12 +103,6 @@ static efi_status_t EFIAPI efi_disconnect_controller(
> efi_handle_t driver_image_handle,
> efi_handle_t child_handle);
>
> -static
> -efi_status_t EFIAPI efi_connect_controller(efi_handle_t controller_handle,
> - efi_handle_t *driver_image_handle,
> - struct efi_device_path
> *remain_device_path,
> - bool recursive);
> -
> /* Called on every callback entry */
> int __efi_entry_check(void)
> {
> @@ -3670,11 +3664,10 @@ static efi_status_t efi_connect_single_controller(
> *
> * Return: status code
> */
> -static efi_status_t EFIAPI efi_connect_controller(
> - efi_handle_t controller_handle,
> - efi_handle_t *driver_image_handle,
> - struct efi_device_path *remain_device_path,
> - bool recursive)
> +efi_status_t EFIAPI efi_connect_controller(efi_handle_t controller_handle,
> + efi_handle_t *driver_image_handle,
> + struct efi_device_path
> *remain_device_path,
> + bool recursive)
> {
> efi_status_t r;
> efi_status_t ret = EFI_NOT_FOUND;
> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> index de0d49ebebda..919e3cba071b 100644
> --- a/lib/efi_loader/efi_capsule.c
> +++ b/lib/efi_loader/efi_capsule.c
> @@ -922,6 +922,105 @@ static bool device_is_present_and_system_part(struct
> efi_device_path *dp)
> return true;
> }
>
> +/**
> + * get_esp_from_boot_option_file_path - get the expand device path
expanded
> + *
> + * 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?
> + 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
