Hi Ilias, On Wed, May 8, 2024 at 9:47 PM Ilias Apalodimas <ilias.apalodi...@linaro.org> wrote: > > 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?
I'll do. > > > 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 Okay. > > > + * > > + * 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? Yes we have the try_load_from_media(), but it doesn't cover all cases. Maybe we need to expand it. > > > + 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. Okay. > > > + 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()? Yes it is BmExpandMediaDevicePath(). The device_is_present_and_system_part() check can be removed. > > > > + */ > > + 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