| Message ID | 20240508112401.155528-1-o451686892@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi_loader: capsule: detect possible ESP device path | expand |
On 5/8/24 13:24, Weizhao Ouyang 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. 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. Hello Wheizhoa, From the description I would not know how to reproduce the problem you face. Could you, please, provide an example (including disk image and capsule) for QEMU and describe expected and observed behavior. We should add a CI test case covering the observed bug. > > Fixes: f86fba8adbf3 ("efi_loader: auto-generate boot option for each blkio device") > Signed-off-by: Weizhao Ouyang <o451686892@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 > + * > + * 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 */ > + if (ret == EFI_SUCCESS && EFI_DP_TYPE(dp, END, END)) { > + 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 > + */ > + 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)); Which driver do you expect to connect here? > + > + /* 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, If you are looking for an ESP shouldn't you use the ESP GUID to find it? It is installed on the ESP in efi_status_t efi_disk_add_dev(). Best regards Heinrich > + 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; > + } > } > } > }
On Wed, May 08, 2024 at 07:24:01PM +0800, Weizhao Ouyang wrote: > 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 > + * > + * 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 */ > + if (ret == EFI_SUCCESS && EFI_DP_TYPE(dp, END, END)) { Flip this around. Always do failure handling. Never do success handling. full_path is always NULL. Just return that. Delete the variable. if (ret != EFI_SUCCESS || !EFI_DP_TYPE(dp, END, END)) return NULL; > + 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 > + */ > + if (ret == EFI_SUCCESS && EFI_DP_TYPE(temp_dp, END, END)) { "Always" was hyperbole. This success handling is fine. > + 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)); ret is not used. Delete. > + 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) { if (ret != EFI_SUCCESS) return NULL; > + buffer = memalign(block_io->media->io_align, block_io->media->block_size); > + if (buffer) { if (!buffer) return NULL; > + ret = EFI_CALL(block_io->read_blocks(block_io, > + block_io->media->media_id, > + 0, > + block_io->media->block_size, > + buffer)); Delete the unused "ret = " assignment. > + 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)) { You could combine these conditions: if (size <= temp_size && memcmp(temp_dp, boot_dev, size) == 0 && device_is_present_and_system_part(temp_dp)) { efi_free_pool(simple_file_system_handle); return 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; No need to initialize this to NULL, it disables static checker warnings for uninitialized variables so it can lead to bugs. > 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; Later, we free full_path but do we ever free the original boot_dev? > + goto found; > + } else { > + efi_free_pool(boot_dev); > + boot_dev = NULL; > + } Better to delete indenting where you can: full_path = get_esp_from_boot_option_file_path(boot_dev); if (full_path) { boot_dev = full_path; goto found; } efi_free_pool(boot_dev); boot_dev = NULL; regards, dan carpenter
On 5/8/24 14:04, Dan Carpenter wrote: > On Wed, May 08, 2024 at 07:24:01PM +0800, Weizhao Ouyang wrote: >> 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 >> + * >> + * 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 */ >> + if (ret == EFI_SUCCESS && EFI_DP_TYPE(dp, END, END)) { > > Flip this around. Always do failure handling. Never do success > handling. full_path is always NULL. Just return that. Delete > the variable. > > if (ret != EFI_SUCCESS || > !EFI_DP_TYPE(dp, END, END)) > return NULL; > > >> + 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 >> + */ >> + if (ret == EFI_SUCCESS && EFI_DP_TYPE(temp_dp, END, END)) { > > "Always" was hyperbole. This success handling is fine. > >> + 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)); > > ret is not used. Delete. Errors should be handled and not ignored. > >> + 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) { > > if (ret != EFI_SUCCESS) > return NULL; > >> + buffer = memalign(block_io->media->io_align, block_io->media->block_size); >> + if (buffer) { > > if (!buffer) > return NULL; > > >> + ret = EFI_CALL(block_io->read_blocks(block_io, >> + block_io->media->media_id, >> + 0, >> + block_io->media->block_size, >> + buffer)); > > Delete the unused "ret = " assignment. ditto Best regards Heinrich > >> + 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)) { > > You could combine these conditions: > > if (size <= temp_size && > memcmp(temp_dp, boot_dev, size) == 0 && > device_is_present_and_system_part(temp_dp)) { > efi_free_pool(simple_file_system_handle); > return 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; > > No need to initialize this to NULL, it disables static checker warnings > for uninitialized variables so it can lead to bugs. > >> 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; > > Later, we free full_path but do we ever free the original boot_dev? > >> + goto found; >> + } else { >> + efi_free_pool(boot_dev); >> + boot_dev = NULL; >> + } > > Better to delete indenting where you can: > > full_path = get_esp_from_boot_option_file_path(boot_dev); > if (full_path) { > boot_dev = full_path; > goto found; > } > efi_free_pool(boot_dev); > boot_dev = NULL; > > regards, > dan carpenter > >
On Wed, May 8, 2024 at 7:52 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 5/8/24 13:24, Weizhao Ouyang 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. 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. > > Hello Wheizhoa, > > From the description I would not know how to reproduce the problem you > face. > > Could you, please, provide an example (including disk image and capsule) > for QEMU and describe expected and observed behavior. > > We should add a CI test case covering the observed bug. Hi Heinrich, Here is a brief description, I'll add a testcase in the next patch. background: - there is an ESP in an mmc logical partition - in the edk2 shell payload, running "CapsuleApp.efi -OD" to perform the capsule on-disk update workflow: - currently, u-boot will auto-generate boot option and install with eliminating logical partitions. So we will have a boot variable Boot0000 that its file_path seems like: "/VenHw/eMMC(0)/eMMC(0)" - then we run the efi app in the edk shell payload. The CapsuleApp will find this boot entry, and it will detect that there is an ESP in this mmc device. According to the on-disk update flow, it will copy the capsule to the ESP, and add the Boot0000 to the BootNext. - After CapsuleApp.efi reset the device, u-boot will check OsIndications and perform the on-disk update flow. Then it will search for the capsule file from the boot entry indicated by the BootNext. Unfortunately, its file_path is "/VenHw/eMMC(0)/eMMC(0)", so the search will fail. - Using this patch, u-boot will expand the Boot0000 file path to detect its ESP, so the capsule file will be found in the expanded device path, eg: "/VenHw/eMMC(0)/eMMC(0)/HD(2)". BR, Weizhao > > > > > Fixes: f86fba8adbf3 ("efi_loader: auto-generate boot option for each blkio device") > > Signed-off-by: Weizhao Ouyang <o451686892@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 > > + * > > + * 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 */ > > + if (ret == EFI_SUCCESS && EFI_DP_TYPE(dp, END, END)) { > > + 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 > > + */ > > + 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)); > > Which driver do you expect to connect here? > > > + > > + /* 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, > > If you are looking for an ESP shouldn't you use the ESP GUID to find it? > It is installed on the ESP in efi_status_t efi_disk_add_dev(). > > Best regards > > Heinrich > > > + 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; > > + } > > } > > } > > } >
Hi Dan, Thanks for the suggestion, I'll fix them in the next patch. BR, Weizhao On Wed, May 8, 2024 at 8:04 PM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > On Wed, May 08, 2024 at 07:24:01PM +0800, Weizhao Ouyang wrote: > > 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 > > + * > > + * 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 */ > > + if (ret == EFI_SUCCESS && EFI_DP_TYPE(dp, END, END)) { > > Flip this around. Always do failure handling. Never do success > handling. full_path is always NULL. Just return that. Delete > the variable. > > if (ret != EFI_SUCCESS || > !EFI_DP_TYPE(dp, END, END)) > return NULL; > > > > + 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 > > + */ > > + if (ret == EFI_SUCCESS && EFI_DP_TYPE(temp_dp, END, END)) { > > "Always" was hyperbole. This success handling is fine. > > > + 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)); > > ret is not used. Delete. > > > + 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) { > > if (ret != EFI_SUCCESS) > return NULL; > > > + buffer = memalign(block_io->media->io_align, block_io->media->block_size); > > + if (buffer) { > > if (!buffer) > return NULL; > > > > + ret = EFI_CALL(block_io->read_blocks(block_io, > > + block_io->media->media_id, > > + 0, > > + block_io->media->block_size, > > + buffer)); > > Delete the unused "ret = " assignment. > > > + 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)) { > > You could combine these conditions: > > if (size <= temp_size && > memcmp(temp_dp, boot_dev, size) == 0 && > device_is_present_and_system_part(temp_dp)) { > efi_free_pool(simple_file_system_handle); > return 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; > > No need to initialize this to NULL, it disables static checker warnings > for uninitialized variables so it can lead to bugs. > > > 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; > > Later, we free full_path but do we ever free the original boot_dev? > > > + goto found; > > + } else { > > + efi_free_pool(boot_dev); > > + boot_dev = NULL; > > + } > > Better to delete indenting where you can: > > full_path = get_esp_from_boot_option_file_path(boot_dev); > if (full_path) { > boot_dev = full_path; > goto found; > } > efi_free_pool(boot_dev); > boot_dev = NULL; > > regards, > dan carpenter > >
Hi Weizhao, On Wed, 8 May 2024 at 14:24, Weizhao Ouyang <o451686892@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 <o451686892@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
On 5/8/24 14:59, Weizhao Ouyang wrote: > On Wed, May 8, 2024 at 7:52 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> >> On 5/8/24 13:24, Weizhao Ouyang 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. 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. >> >> Hello Wheizhoa, >> >> From the description I would not know how to reproduce the problem you >> face. >> >> Could you, please, provide an example (including disk image and capsule) >> for QEMU and describe expected and observed behavior. >> >> We should add a CI test case covering the observed bug. > > Hi Heinrich, > > Here is a brief description, I'll add a testcase in the next patch. > > background: > - there is an ESP in an mmc logical partition > - in the edk2 shell payload, running "CapsuleApp.efi -OD" to perform > the capsule on-disk update > > workflow: > - currently, u-boot will auto-generate boot option and install with > eliminating logical partitions. So we will have a boot variable > Boot0000 that its file_path seems like: "/VenHw/eMMC(0)/eMMC(0)" > - then we run the efi app in the edk shell payload. The CapsuleApp > will find this boot entry, and it will detect that there is an ESP > in this mmc device. According to the on-disk update flow, it will > copy the capsule to the ESP, and add the Boot0000 to the BootNext. Why do you need an EFI app? Shouldn't the operating system place the capsule file? What makes you expect that there is any ESP on whatever Boot0000 points to? You should evaluate BootCurrent if you want to know from which boot entry the current application was started from. > - After CapsuleApp.efi reset the device, u-boot will check > OsIndications and perform the on-disk update flow. Then it will U-Boot does not check OsIndications. Best regards Heirnich > search for the capsule file from the boot entry indicated by the > BootNext. Unfortunately, its file_path is "/VenHw/eMMC(0)/eMMC(0)", > so the search will fail. > - Using this patch, u-boot will expand the Boot0000 file path to > detect its ESP, so the capsule file will be found in the expanded > device path, eg: "/VenHw/eMMC(0)/eMMC(0)/HD(2)". > > BR, > Weizhao > >> >>> >>> Fixes: f86fba8adbf3 ("efi_loader: auto-generate boot option for each blkio device") >>> Signed-off-by: Weizhao Ouyang <o451686892@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 >>> + * >>> + * 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 */ >>> + if (ret == EFI_SUCCESS && EFI_DP_TYPE(dp, END, END)) { >>> + 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 >>> + */ >>> + 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)); >> >> Which driver do you expect to connect here? >> >>> + >>> + /* 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, >> >> If you are looking for an ESP shouldn't you use the ESP GUID to find it? >> It is installed on the ESP in efi_status_t efi_disk_add_dev(). >> >> Best regards >> >> Heinrich >> >>> + 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; >>> + } >>> } >>> } >>> } >>
> > > + * > > + * 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
Hi Ilias, On Wed, May 8, 2024 at 9:47 PM Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Weizhao, > > On Wed, 8 May 2024 at 14:24, Weizhao Ouyang <o451686892@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 <o451686892@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
On Wed, May 8, 2024 at 9:53 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 5/8/24 14:59, Weizhao Ouyang wrote: > > On Wed, May 8, 2024 at 7:52 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > >> > >> On 5/8/24 13:24, Weizhao Ouyang 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. 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. > >> > >> Hello Wheizhoa, > >> > >> From the description I would not know how to reproduce the problem you > >> face. > >> > >> Could you, please, provide an example (including disk image and capsule) > >> for QEMU and describe expected and observed behavior. > >> > >> We should add a CI test case covering the observed bug. > > > > Hi Heinrich, > > > > Here is a brief description, I'll add a testcase in the next patch. > > > > background: > > - there is an ESP in an mmc logical partition > > - in the edk2 shell payload, running "CapsuleApp.efi -OD" to perform > > the capsule on-disk update > > > > workflow: > > - currently, u-boot will auto-generate boot option and install with > > eliminating logical partitions. So we will have a boot variable > > Boot0000 that its file_path seems like: "/VenHw/eMMC(0)/eMMC(0)" > > - then we run the efi app in the edk shell payload. The CapsuleApp > > will find this boot entry, and it will detect that there is an ESP > > in this mmc device. According to the on-disk update flow, it will > > copy the capsule to the ESP, and add the Boot0000 to the BootNext. > > Why do you need an EFI app? Shouldn't the operating system place the > capsule file? > > What makes you expect that there is any ESP on whatever Boot0000 points > to? You should evaluate BootCurrent if you want to know from which boot > entry the current application was started from. CapsuleApp[1] is a shell application that can manually trigger capsule update process. As one of its flow, it will install the capsule to the ESP and update the BootNext. The purpose of this patch is to fix the broken update process and align with edk2. > > > - After CapsuleApp.efi reset the device, u-boot will check > > OsIndications and perform the on-disk update flow. Then it will > > U-Boot does not check OsIndications. I mean the check_run_capsules(). [1] https://github.com/tianocore/edk2/tree/master/MdeModulePkg/Application/CapsuleApp BR, Weizhao > > Best regards > > Heirnich > > > search for the capsule file from the boot entry indicated by the > > BootNext. Unfortunately, its file_path is "/VenHw/eMMC(0)/eMMC(0)", > > so the search will fail. > > - Using this patch, u-boot will expand the Boot0000 file path to > > detect its ESP, so the capsule file will be found in the expanded > > device path, eg: "/VenHw/eMMC(0)/eMMC(0)/HD(2)". > > > > BR, > > Weizhao > > > >> > >>> > >>> Fixes: f86fba8adbf3 ("efi_loader: auto-generate boot option for each blkio device") > >>> Signed-off-by: Weizhao Ouyang <o451686892@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 > >>> + * > >>> + * 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 */ > >>> + if (ret == EFI_SUCCESS && EFI_DP_TYPE(dp, END, END)) { > >>> + 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 > >>> + */ > >>> + 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)); > >> > >> Which driver do you expect to connect here? > >> > >>> + > >>> + /* 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, > >> > >> If you are looking for an ESP shouldn't you use the ESP GUID to find it? > >> It is installed on the ESP in efi_status_t efi_disk_add_dev(). > >> > >> Best regards > >> > >> Heinrich > >> > >>> + 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; > >>> + } > >>> } > >>> } > >>> } > >> >
On Wed, May 8, 2024 at 9:56 PM Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > > > > > + * > > > + * 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. Yeah, I'll summarize them to the try_load_from_media(). > Also I don't think we need the Fixes tag Okay. BR, Weizhao > > 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
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 + * + * 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 */ + if (ret == EFI_SUCCESS && EFI_DP_TYPE(dp, END, END)) { + 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 + */ + 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; + } } } }
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. 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 <o451686892@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(-)