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 <[email protected]> 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 <[email protected]> > --- 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
