[PATCH v4] disk: efi_loader: Improve disk image detection in efi_bootmgr

[Ilias Apalodimas]

Hi Javier

Generally speaking this looks ok now. A few nits below and I think we
can pull it

On Thu, 4 Sept 2025 at 00:44, Javier Tia <javier.tia at linaro.org> wrote:
>
> Enhances the process for identifying disk images within the EFI boot
> manager. Utilize part_driver_lookup_type() to verify the validity of a
> downloaded file as a disk image, rather than depending on file
> extensions.
>
> part_driver_lookup_type() is now used in the prepare_loaded_image()
> function in the EFI boot manager to detect partitions on a block device
> created from a downloaded image. This allows the boot manager to boot
> from any disk image that can be recognized by a partition driver, not
> just ISO and IMG images.
>
> Update prepare_loaded_image() to create the ramdisk block device
> internally, obtain the blk_desc and use part_driver_lookup_type() to
> detect a valid partition table.
>
> In try_load_from_uri_path(), try prepare_loaded_image() first to detect
> disk images, and fall back to PE-COFF detection only if that fails.
>
> Include part.h in the EFI loader and export part_driver_lookup_type and
> add its prototype and documentation in include/part.h. This makes the
> partition-driver lookup available to other code paths that need to
> detect partition tables on a block descriptor.
>
> Signed-off-by: Javier Tia <javier.tia at linaro.org>
> ---

We usually add a changelog across patches here, to make the reviewers
life easier.

>  disk/part.c                  |  2 +-
>  include/part.h               | 18 ++++++++++
>  lib/efi_loader/efi_bootmgr.c | 64 +++++++++++++++++++++++++-----------
>  3 files changed, 63 insertions(+), 21 deletions(-)
>
> diff --git a/disk/part.c b/disk/part.c
> index 66e2b3a7219..f0c6866e210 100644
> --- a/disk/part.c
> +++ b/disk/part.c
> @@ -63,7 +63,7 @@ static struct part_driver *part_driver_get_type(int part_type)
>   * @dev_desc: Device descriptor
>   * Return: Driver found, or NULL if none
>   */

[...]


>  static efi_status_t prepare_loaded_image(u16 *label, ulong addr, ulong size,
>                                          struct efi_device_path **dp,
> @@ -355,15 +359,36 @@ static efi_status_t prepare_loaded_image(u16 *label, ulong addr, ulong size,
>         u64 pages;
>         efi_status_t ret;
>         struct udevice *ramdisk_blk;
> +       struct blk_desc *desc;
> +       struct part_driver *part_drv;
>
> +       /* Create the ramdisk block device internally */
>         ramdisk_blk = mount_image(label, addr, size);
> -       if (!ramdisk_blk)
> -               return EFI_LOAD_ERROR;
> +       if (!ramdisk_blk) {
> +               log_err("Failed to create ramdisk block device\n");
> +               return EFI_DEVICE_ERROR;
> +       }
> +
> +       /* Get the block descriptor and detect partitions */
> +       desc = dev_get_uclass_plat(ramdisk_blk);
> +       if (!desc) {
> +               log_err("Failed to get block descriptor\n");
> +               ret = EFI_DEVICE_ERROR;
> +               goto err_cleanup_blkdev;

Don't rename 'err' to 'err_cleanup_blkdev'. Keep the patches simnple
and change one thing at a time.
The only thing this change does is make the review harder.

> +       }
> +
> +       /* Use part_driver_lookup_type for comprehensive partition detection */
> +       part_drv = part_driver_lookup_type(desc);
> +       if (!part_drv) {
> +               log_err("Image is not a valid disk image\n");
> +               ret = EFI_INVALID_PARAMETER;
> +               goto err_cleanup_blkdev;
> +       }
>
>         ret = fill_default_file_path(ramdisk_blk, dp);
>         if (ret != EFI_SUCCESS) {
>                 log_info("Cannot boot from downloaded image\n");
> -               goto err;
> +               goto err_cleanup_blkdev;
>         }
>
>         /*
> @@ -379,17 +404,17 @@ static efi_status_t prepare_loaded_image(u16 *label, ulong addr, ulong size,
>                                     false, true);
>         if (ret != EFI_SUCCESS) {
>                 log_err("Failed to reserve memory\n");
> -               goto err;
> +               goto err_cleanup_blkdev;
>         }
>
>         *blk = ramdisk_blk;
>
>         return EFI_SUCCESS;
>
> -err:
> +err_cleanup_blkdev:
> +       /* Clean up the created block device on error */
>         if (blkmap_destroy(ramdisk_blk->parent))
> -               log_err("Destroying blkmap failed\n");
> -
> +               log_err("Destroying blkmap failed during cleanup\n");

Same here. If you want to change these send them on a different patch.
r.g one that renames the error tag and changes some error messages

>         return ret;
>  }
>
> @@ -407,7 +432,7 @@ static efi_status_t efi_bootmgr_release_uridp(struct uridp_context *ctx)
>         if (!ctx)
>                 return ret;
>
> -       /* cleanup for iso or img image */

[...]

>
> +       /* First, try to treat the image as a disk image */
> +       ret = prepare_loaded_image(lo_label, image_addr, image_size,
> +                                  &loaded_dp, &blk);
> +       if (ret == EFI_SUCCESS) {
> +               /* Image is a disk image, set up for disk boot */
>                 source_buffer = NULL;
>                 source_size = 0;
>         } else if (efi_check_pe((void *)image_addr, image_size, NULL) == EFI_SUCCESS) {
> +               /* Image is a PE-COFF executable */

We don't need the comment, it's obvious from the else if check

>                 /*
>                  * loaded_dp must exist until efi application returns,
>                  * will be freed in return_to_efibootmgr event callback.
> @@ -545,7 +568,8 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
>                 source_buffer = (void *)image_addr;
>                 source_size = image_size;
>         } else {
> -               log_err("Error: file type is not supported\n");
> +               /* Neither disk image nor PE-COFF */
> +               log_err("Error: downloaded image is not supported\n");

Same here. Don't change ther error message in this patch.

>                 ret = EFI_UNSUPPORTED;
>                 goto err;
>         }
> --
> 2.51.0
>

I suggest just resending a single patch removing the changes on prints
and error tags.
If we think the messages need to change after that you can do it on a follow up

Cheers
/Ilias