On Wed, May 8, 2024 at 9:56 PM Ilias Apalodimas
<ilias.apalodi...@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
