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

Weizhao Ouyang o451686892 at gmail.com
Wed May 8 16:04:48 CEST 2024


Hi Ilias,

On Wed, May 8, 2024 at 9:47 PM Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Hi Weizhao,
>
> On Wed, 8 May 2024 at 14:24, Weizhao Ouyang <o451686892 at 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 at 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


More information about the U-Boot mailing list