[U-Boot] [PATCH 8/9] efi_loader: rework bootmgr/bootefi using load_image API
AKASHI Takahiro
takahiro.akashi at linaro.org
Mon Apr 22 00:36:04 UTC 2019
On Sat, Apr 20, 2019 at 07:37:47PM +0200, Heinrich Schuchardt wrote:
> On 4/20/19 10:37 AM, Heinrich Schuchardt wrote:
> > On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
> >> In the current implementation, bootefi command and EFI boot manager
> >> don't use load_image API, instead, use more primitive and internal
> >> functions. This will introduce duplicated code and potentially
> >> unknown bugs as well as inconsistent behaviours.
> >>
> >> With this patch, do_efibootmgr() and do_boot_efi() are completely
> >> overhauled and re-implemented using load_image API.
>
> Hello Takahiro,
>
> with this patch applied iPXE cannot find a chained file on the EFI
> partition. Did you test booting GRUB with this patch?
No. I test my patch only with Shell.efi right now.
-Takahiro Akashi
> Best regards
>
> Heinrich
>
> EFI: Entry efi_load_image(0, 00000000b9f92020,
> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(1,MBR,0x800512bd,0x800,0x200000)/dtb,
> 0000000040080000, 21738, 00000000b9f33fc8)
> EFI: Exit: efi_load_image: 0
> EFI: Entry efi_start_image(00000000b9f95360, 0000000000000000,
> 0000000000000000)
> EFI: Call: efi_open_protocol(image_handle, &efi_guid_loaded_image,
> &info, NULL, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL)
> EFI: 0 returned by efi_open_protocol(image_handle,
> &efi_guid_loaded_image, &info, NULL, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL)
> EFI: Jumping into 0x00000000b8e7861c
> EFI: Call: image_obj->entry(image_handle, &systab)
> EFI: Entry efi_load_image(0, 00000000b9f95360, /, 00000000b8e9fbc0,
> 177, 00000000b9f33e38)
> efi_load_pe: Invalid DOS Signature
> EFI: Exit: efi_load_image: 1
> iPXE initialising devices...ok
> iPXE 1.0.0+ (ac4fb) -- Open Source Network Boot Firmware -- http://ipxe.org
> Features: DNS FTP HTTP HTTPS iSCSI NFS TFTP VLAN AoE EFI Menu
> iPXE script started
> Trying to chain file:/boot.ipxe
> Could not start download: No such file or directory
> (http://ipxe.org/2d4de08e)
> Could not find file:/boot.ipxe
> Opening shell
> iPXE>
>
>
> >>
> >> Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> >> ---
> >> cmd/bootefi.c | 187 ++++++++++++++++++----------------
> >> include/efi_loader.h | 5 +-
> >> lib/efi_loader/efi_bootmgr.c | 43 ++++----
> >> lib/efi_loader/efi_boottime.c | 2 +
> >> 4 files changed, 127 insertions(+), 110 deletions(-)
> >>
> >> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >> index e2ca68c4dc12..8c84fff590a7 100644
> >> --- a/cmd/bootefi.c
> >> +++ b/cmd/bootefi.c
> >> @@ -242,89 +242,36 @@ static efi_status_t efi_install_fdt(const char
> >> *fdt_opt)
> >> /**
> >> * do_bootefi_exec() - execute EFI binary
> >> *
> >> - * @efi: address of the binary
> >> - * @device_path: path of the device from which the binary was loaded
> >> - * @image_path: device path of the binary
> >> + * @handle: handle of loaded image
> >> * Return: status code
> >> *
> >> * Load the EFI binary into a newly assigned memory unwinding the
> >> relocation
> >> * information, install the loaded image protocol, and call the binary.
> >> */
> >> -static efi_status_t do_bootefi_exec(void *efi,
> >> - struct efi_device_path *device_path,
> >> - struct efi_device_path *image_path)
> >> +static efi_status_t do_bootefi_exec(efi_handle_t handle)
> >> {
> >> - efi_handle_t mem_handle = NULL;
> >> - struct efi_device_path *memdp = NULL;
> >> efi_status_t ret;
> >> - struct efi_loaded_image_obj *image_obj = NULL;
> >> - struct efi_loaded_image *loaded_image_info = NULL;
> >> -
> >> - /*
> >> - * Special case for efi payload not loaded from disk, such as
> >> - * 'bootefi hello' or for example payload loaded directly into
> >> - * memory via JTAG, etc:
> >> - */
> >> - if (!device_path && !image_path) {
> >> - printf("WARNING: using memory device/image path, this may
> >> confuse some payloads!\n");
> >> - /* actual addresses filled in after efi_load_pe() */
> >> - memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, 0, 0);
> >> - device_path = image_path = memdp;
> >> - /*
> >> - * Grub expects that the device path of the loaded image is
> >> - * installed on a handle.
> >> - */
> >> - ret = efi_create_handle(&mem_handle);
> >> - if (ret != EFI_SUCCESS)
> >> - return ret; /* TODO: leaks device_path */
> >> - ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
> >> - device_path);
> >> - if (ret != EFI_SUCCESS)
> >> - goto err_add_protocol;
> >> - } else {
> >> - assert(device_path && image_path);
> >> - }
> >> -
> >> - ret = efi_setup_loaded_image(device_path, image_path, &image_obj,
> >> - &loaded_image_info);
> >> - if (ret)
> >> - goto err_prepare;
> >>
> >> /* Transfer environment variable as load options */
> >> - ret = set_load_options((efi_handle_t)image_obj, "bootargs");
> >> - if (ret != EFI_SUCCESS)
> >> - goto err_prepare;
> >> -
> >> - /* Load the EFI payload */
> >> - ret = efi_load_pe(image_obj, efi, loaded_image_info);
> >> + ret = set_load_options(handle, "bootargs");
> >> if (ret != EFI_SUCCESS)
> >> - goto err_prepare;
> >> -
> >> - if (memdp) {
> >> - struct efi_device_path_memory *mdp = (void *)memdp;
> >> - mdp->memory_type = loaded_image_info->image_code_type;
> >> - mdp->start_address = (uintptr_t)loaded_image_info->image_base;
> >> - mdp->end_address = mdp->start_address +
> >> - loaded_image_info->image_size;
> >> - }
> >> + return ret;
> >>
> >> /* we don't support much: */
> >>
> >> env_set("efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported",
> >>
> >> "{ro,boot}(blob)0000000000000000");
> >>
> >> /* Call our payload! */
> >> - debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);
> >> - ret = EFI_CALL(efi_start_image(&image_obj->header, NULL, NULL));
> >> + ret = EFI_CALL(efi_start_image(handle, NULL, NULL));
> >>
> >> -err_prepare:
> >> - /* image has returned, loaded-image obj goes *poof*: */
> >> efi_restore_gd();
> >> - free(loaded_image_info->load_options);
> >> - efi_delete_handle(&image_obj->header);
> >>
> >> -err_add_protocol:
> >> - if (mem_handle)
> >> - efi_delete_handle(mem_handle);
> >> + /*
> >> + * FIXME: Who is responsible for
> >> + * free(loaded_image_info->load_options);
> >> + * Once efi_exit() is implemented correctly,
> >> + * handle itself doesn't exist here.
> >> + */
> >>
> >> return ret;
> >> }
> >> @@ -339,8 +286,7 @@ err_add_protocol:
> >> */
> >> static int do_efibootmgr(const char *fdt_opt)
> >> {
> >> - struct efi_device_path *device_path, *file_path;
> >> - void *addr;
> >> + efi_handle_t handle;
> >> efi_status_t ret;
> >>
> >> /* Allow unaligned memory access */
> >> @@ -362,19 +308,19 @@ static int do_efibootmgr(const char *fdt_opt)
> >> else if (ret != EFI_SUCCESS)
> >> return CMD_RET_FAILURE;
> >>
> >> - addr = efi_bootmgr_load(&device_path, &file_path);
> >> - if (!addr)
> >> - return 1;
> >> + ret = efi_bootmgr_load(&handle);
> >> + if (ret != EFI_SUCCESS) {
> >> + printf("EFI boot manager: Cannot load any image\n");
> >> + return CMD_RET_FAILURE;
> >> + }
> >>
> >> - printf("## Starting EFI application at %p ...\n", addr);
> >> - ret = do_bootefi_exec(addr, device_path, file_path);
> >> - printf("## Application terminated, r = %lu\n",
> >> - ret & ~EFI_ERROR_MASK);
> >> + ret = do_bootefi_exec(handle);
> >> + printf("## Application terminated, r = %lu\n", ret &
> >> ~EFI_ERROR_MASK);
> >>
> >> if (ret != EFI_SUCCESS)
> >> - return 1;
> >> + return CMD_RET_FAILURE;
> >>
> >> - return 0;
> >> + return CMD_RET_SUCCESS;
> >> }
> >>
> >> /*
> >> @@ -389,7 +335,12 @@ static int do_efibootmgr(const char *fdt_opt)
> >> */
> >> static int do_bootefi_image(const char *image_opt, const char *fdt_opt)
> >> {
> >> - unsigned long addr;
> >> + void *image_buf;
> >> + struct efi_device_path *device_path, *image_path;
> >> + struct efi_device_path *memdp = NULL, *file_path = NULL;
> >> + unsigned long addr, size;
> >> + const char *size_str;
> >> + efi_handle_t mem_handle = NULL, handle;
> >> efi_status_t ret;
> >>
> >> /* Allow unaligned memory access */
> >> @@ -414,33 +365,90 @@ static int do_bootefi_image(const char
> >> *image_opt, const char *fdt_opt)
> >> #ifdef CONFIG_CMD_BOOTEFI_HELLO
> >> if (!strcmp(image_opt, "hello")) {
> >> char *saddr;
> >> - ulong size = __efi_helloworld_end - __efi_helloworld_begin;
> >>
> >> saddr = env_get("loadaddr");
> >> + size = __efi_helloworld_end - __efi_helloworld_begin;
> >> +
> >> if (saddr)
> >> addr = simple_strtoul(saddr, NULL, 16);
> >> else
> >> addr = CONFIG_SYS_LOAD_ADDR;
> >> - memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size);
> >> +
> >> + image_buf = map_sysmem(addr, size);
> >> + memcpy(image_buf, __efi_helloworld_begin, size);
> >> +
> >> + device_path = NULL;
> >> + image_path = NULL;
> >> } else
> >> #endif
> >> {
> >> + size_str = env_get("filesize");
> >> + if (size_str)
> >> + size = simple_strtoul(size_str, NULL, 16);
> >> + else
> >> + size = 0;
> >> +
> >> addr = simple_strtoul(image_opt, NULL, 16);
> >> /* Check that a numeric value was passed */
> >> if (!addr && *image_opt != '0')
> >> return CMD_RET_USAGE;
> >> +
> >> + image_buf = map_sysmem(addr, size);
> >> +
> >> + device_path = bootefi_device_path;
> >> + image_path = bootefi_image_path;
> >> }
> >>
> >> - printf("## Starting EFI application at %08lx ...\n", addr);
> >> - ret = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path,
> >> - bootefi_image_path);
> >> - printf("## Application terminated, r = %lu\n",
> >> - ret & ~EFI_ERROR_MASK);
> >> + if (!device_path && !image_path) {
> >> + /*
> >> + * Special case for efi payload not loaded from disk,
> >> + * such as 'bootefi hello' or for example payload
> >> + * loaded directly into memory via JTAG, etc:
> >> + */
> >> + memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
> >> + (uintptr_t)image_buf, size);
> >> + file_path = memdp; /* append(memdp, NULL) */
> >> +
> >> + /*
> >> + * Make sure that device for device_path exist
> >> + * in load_image(). Otherwise, shell and grub will fail.
> >> + */
> >> + ret = efi_create_handle(&mem_handle);
> >> + if (ret != EFI_SUCCESS)
> >> + /* TODO: leaks device_path */
> >> + goto err_add_protocol;
> >> +
> >> + ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
> >> + memdp);
> >> + if (ret != EFI_SUCCESS)
> >> + goto err_add_protocol;
> >> + } else {
> >> + assert(device_path && image_path);
> >> + file_path = efi_dp_append(device_path, image_path);
> >> + }
> >> +
> >> + ret = EFI_CALL(efi_load_image(false, efi_root,
> >> + file_path, image_buf, size, &handle));
> >> + if (ret != EFI_SUCCESS)
> >> + goto err_prepare;
> >> +
> >> + ret = do_bootefi_exec(handle);
> >> + printf("## Application terminated, r = %lu\n", ret &
> >> ~EFI_ERROR_MASK);
> >> +
> >> +err_prepare:
> >> + if (device_path)
> >> + efi_free_pool(file_path);
> >> +
> >> +err_add_protocol:
> >> + if (mem_handle)
> >> + efi_delete_handle(mem_handle);
> >> + if (file_path) /* hence memdp */
> >> + efi_free_pool(file_path);
> >>
> >> if (ret != EFI_SUCCESS)
> >> return CMD_RET_FAILURE;
> >> - else
> >> - return CMD_RET_SUCCESS;
> >> +
> >> + return CMD_RET_SUCCESS;
> >> }
> >>
> >> #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
> >> @@ -598,7 +606,7 @@ static char bootefi_help_text[] =
> >> " Use environment variable efi_selftest to select a single
> >> test.\n"
> >> " Use 'setenv efi_selftest list' to enumerate all tests.\n"
> >> #endif
> >> - "bootefi bootmgr [fdt addr]\n"
> >> + "bootefi bootmgr [fdt address]\n"
> >> " - load and boot EFI payload based on BootOrder/BootXXXX
> >> variables.\n"
> >> "\n"
> >> " If specified, the device tree located at <fdt address> gets\n"
> >> @@ -623,6 +631,13 @@ void efi_set_bootdev(const char *dev, const char
> >> *devnr, const char *path)
> >> ret = efi_dp_from_name(dev, devnr, path, &device, &image);
> >> if (ret == EFI_SUCCESS) {
> >> bootefi_device_path = device;
> >> + if (image) {
> >> + /* FIXME: image should not contain device */
> >> + struct efi_device_path *image_tmp = image;
> >> +
> >> + efi_dp_split_file_path(image, &device, &image);
> >> + efi_free_pool(image_tmp);
> >> + }
> >> bootefi_image_path = image;
> >> } else {
> >> bootefi_device_path = NULL;
> >> diff --git a/include/efi_loader.h b/include/efi_loader.h
> >> index 93f7672aecbc..39ed8a6fa592 100644
> >> --- a/include/efi_loader.h
> >> +++ b/include/efi_loader.h
> >> @@ -412,8 +412,6 @@ efi_status_t efi_setup_loaded_image(struct
> >> efi_device_path *device_path,
> >> struct efi_device_path *file_path,
> >> struct efi_loaded_image_obj **handle_ptr,
> >> struct efi_loaded_image **info_ptr);
> >> -efi_status_t efi_load_image_from_path(struct efi_device_path *file_path,
> >> - void **buffer, efi_uintn_t *size);
> >> /* Print information about all loaded images */
> >> void efi_print_image_infos(void *pc);
> >>
> >> @@ -567,8 +565,7 @@ struct efi_load_option {
> >>
> >> void efi_deserialize_load_option(struct efi_load_option *lo, u8 *data);
> >> unsigned long efi_serialize_load_option(struct efi_load_option *lo,
> >> u8 **data);
> >> -void *efi_bootmgr_load(struct efi_device_path **device_path,
> >> - struct efi_device_path **file_path);
> >> +efi_status_t efi_bootmgr_load(efi_handle_t *handle);
> >>
> >> #else /* CONFIG_IS_ENABLED(EFI_LOADER) */
> >>
> >> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> >> index 4fccadc5483d..13ec79b2098b 100644
> >> --- a/lib/efi_loader/efi_bootmgr.c
> >> +++ b/lib/efi_loader/efi_bootmgr.c
> >> @@ -120,14 +120,14 @@ static void *get_var(u16 *name, const efi_guid_t
> >> *vendor,
> >> * if successful. This checks that the EFI_LOAD_OPTION is active
> >> (enabled)
> >> * and that the specified file to boot exists.
> >> */
> >> -static void *try_load_entry(uint16_t n, struct efi_device_path
> >> **device_path,
> >> - struct efi_device_path **file_path)
> >> +static efi_status_t try_load_entry(u16 n, efi_handle_t *handle)
> >> {
> >> struct efi_load_option lo;
> >> u16 varname[] = L"Boot0000";
> >> u16 hexmap[] = L"0123456789ABCDEF";
> >> - void *load_option, *image = NULL;
> >> + void *load_option;
> >> efi_uintn_t size;
> >> + efi_status_t ret;
> >>
> >> varname[4] = hexmap[(n & 0xf000) >> 12];
> >> varname[5] = hexmap[(n & 0x0f00) >> 8];
> >> @@ -136,19 +136,19 @@ static void *try_load_entry(uint16_t n, struct
> >> efi_device_path **device_path,
> >>
> >> load_option = get_var(varname, &efi_global_variable_guid, &size);
> >> if (!load_option)
> >> - return NULL;
> >> + return EFI_LOAD_ERROR;
> >>
> >> efi_deserialize_load_option(&lo, load_option);
> >>
> >> if (lo.attributes & LOAD_OPTION_ACTIVE) {
> >> u32 attributes;
> >> - efi_status_t ret;
> >>
> >> debug("%s: trying to load \"%ls\" from %pD\n",
> >> __func__, lo.label, lo.file_path);
> >>
> >> - ret = efi_load_image_from_path(lo.file_path, &image, &size);
> >> -
> >> + ret = EFI_CALL(efi_load_image(false, (void *)0x1 /* FIXME */,
> >
> > In cmd/booefi.c you use efi_root. So we shall do the same here.
> >
> > I will fix that. No need to resend the patch.
> >
> >
> >> + lo.file_path, NULL, 0,
> >> + handle));
> >> if (ret != EFI_SUCCESS)
> >> goto error;
> > TODO:
> > Now that you have the loaded image protocol you should update the load
> > options with the value in load_option. I guess you do not want to
> > overwrite it with the content of bootargs.
> >
> > A the problem was not introduced with this patch series:
> >
> > Reviewed-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
> >
> >>
> >> @@ -159,17 +159,22 @@ static void *try_load_entry(uint16_t n, struct
> >> efi_device_path **device_path,
> >> L"BootCurrent",
> >> (efi_guid_t *)&efi_global_variable_guid,
> >> attributes, size, &n));
> >> - if (ret != EFI_SUCCESS)
> >> + if (ret != EFI_SUCCESS) {
> >> + if (EFI_CALL(efi_unload_image(*handle))
> >> + != EFI_SUCCESS)
> >> + printf("Unloading image failed\n");
> >> goto error;
> >> + }
> >>
> >> printf("Booting: %ls\n", lo.label);
> >> - efi_dp_split_file_path(lo.file_path, device_path, file_path);
> >> + } else {
> >> + ret = EFI_LOAD_ERROR;
> >> }
> >>
> >> error:
> >> free(load_option);
> >>
> >> - return image;
> >> + return ret;
> >> }
> >>
> >> /*
> >> @@ -177,12 +182,10 @@ error:
> >> * EFI variable, the available load-options, finding and returning
> >> * the first one that can be loaded successfully.
> >> */
> >> -void *efi_bootmgr_load(struct efi_device_path **device_path,
> >> - struct efi_device_path **file_path)
> >> +efi_status_t efi_bootmgr_load(efi_handle_t *handle)
> >> {
> >> u16 bootnext, *bootorder;
> >> efi_uintn_t size;
> >> - void *image = NULL;
> >> int i, num;
> >> efi_status_t ret;
> >>
> >> @@ -209,10 +212,9 @@ void *efi_bootmgr_load(struct efi_device_path
> >> **device_path,
> >> /* load BootNext */
> >> if (ret == EFI_SUCCESS) {
> >> if (size == sizeof(u16)) {
> >> - image = try_load_entry(bootnext, device_path,
> >> - file_path);
> >> - if (image)
> >> - return image;
> >> + ret = try_load_entry(bootnext, handle);
> >> + if (ret == EFI_SUCCESS)
> >> + return ret;
> >> }
> >> } else {
> >> printf("Deleting BootNext failed\n");
> >> @@ -223,19 +225,20 @@ void *efi_bootmgr_load(struct efi_device_path
> >> **device_path,
> >> bootorder = get_var(L"BootOrder", &efi_global_variable_guid,
> >> &size);
> >> if (!bootorder) {
> >> printf("BootOrder not defined\n");
> >> + ret = EFI_NOT_FOUND;
> >> goto error;
> >> }
> >>
> >> num = size / sizeof(uint16_t);
> >> for (i = 0; i < num; i++) {
> >> debug("%s: trying to load Boot%04X\n", __func__, bootorder[i]);
> >> - image = try_load_entry(bootorder[i], device_path, file_path);
> >> - if (image)
> >> + ret = try_load_entry(bootorder[i], handle);
> >> + if (ret == EFI_SUCCESS)
> >> break;
> >> }
> >>
> >> free(bootorder);
> >>
> >> error:
> >> - return image;
> >> + return ret;
> >> }
> >> diff --git a/lib/efi_loader/efi_boottime.c
> >> b/lib/efi_loader/efi_boottime.c
> >> index abc295e392e9..19a58144cf4b 100644
> >> --- a/lib/efi_loader/efi_boottime.c
> >> +++ b/lib/efi_loader/efi_boottime.c
> >> @@ -1591,6 +1591,7 @@ failure:
> >> * @size: size of the loaded image
> >> * Return: status code
> >> */
> >> +static
> >> efi_status_t efi_load_image_from_path(struct efi_device_path
> >> *file_path,
> >> void **buffer, efi_uintn_t *size)
> >> {
> >> @@ -2664,6 +2665,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t
> >> image_handle,
> >> }
> >>
> >> current_image = image_handle;
> >> + EFI_PRINT("Jumping into 0x%p\n", image_obj->entry);
> >> ret = EFI_CALL(image_obj->entry(image_handle, &systab));
> >>
> >> /*
> >>
> >
> >
>
More information about the U-Boot
mailing list