On 4/12/19 10:38 AM, AKASHI Takahiro wrote:
> On Fri, Apr 12, 2019 at 07:53:25AM +0200, Heinrich Schuchardt wrote:
>> On 3/27/19 5:40 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.
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.aka...@linaro.org>
>>> ---
>>> cmd/bootefi.c | 199 +++++++++++++++++++---------------
>>> include/efi_loader.h | 5 +-
>>> lib/efi_loader/efi_bootmgr.c | 43 ++++----
>>> lib/efi_loader/efi_boottime.c | 1 +
>>> 4 files changed, 138 insertions(+), 110 deletions(-)
>>>
>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>> index 39a0712af6ad..17dccd718008 100644
>>> --- a/cmd/bootefi.c
>>> +++ b/cmd/bootefi.c
>>> @@ -224,88 +224,43 @@ 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;
>>> + efi_status_t ret;
>>>
>>> /* Transfer environment variable as load options */
>>> - set_load_options(loaded_image_info, "bootargs");
>>> -
>>> - /* Load the EFI payload */
>>> - ret = efi_load_pe(image_obj, efi, loaded_image_info);
>>> + ret = EFI_CALL(systab.boottime->open_protocol(
>>> + handle,
>>> + &efi_guid_loaded_image,
>>> + (void **)&loaded_image_info,
>>> + NULL, NULL,
>>> + EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL));
>>> 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;
>>> - }
>>> + goto err;
>>> + set_load_options(loaded_image_info, "bootargs");
>>>
>>> /* we don't support much: */
>>>
>>> env_set("efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported",
>>> "{ro,boot}(blob)0000000000000000");
>>>
>>> /* Call our payload! */
>>> + image_obj = container_of(handle, struct efi_loaded_image_obj, header);
>> This is not needed, if we remove the debug statement.
>>
>>> debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);
>> That line should go to StartImage() if needed. Please, drop it here.
>>
>>> - 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);
>>> + efi_free_pool(loaded_image_info);
>>
>> Wouldn't this be done by unload_image()?
>
> but we should not call unload_image(), efi_exit() does. Right?
> Yes, correct.
>>>
>>> +err:
>>> return ret;
>>> }
>>>
>>> @@ -319,8 +274,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 */
>>> @@ -340,19 +294,21 @@ static int do_efibootmgr(const char *fdt_opt)
>>> 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);
>>> +
>>> + EFI_CALL(efi_unload_image(handle));
>>
>> Only applications have to be unloaded, not drivers. Unloading is to be
>> handled by exit(), see UEFI spec.
>
> How do we know whether the loaded image is an application, or driver?
In efi_load_pe() we should evaluate the field opt->Subsystem describing
the "Windows Subsystem". I once started on this but then I saw that
cmd/bootefi.c needed to be cleaned up first:
https://github.com/xypron/u-boot-patches/blob/efi-next/0001-efi_loader-implement-UnloadImage.patch
>
>>>
>>> if (ret != EFI_SUCCESS)
>>> - return 1;
>>> + return CMD_RET_FAILURE;
>>>
>>> - return 0;
>>> + return CMD_RET_SUCCESS;
>>> }
>>>
>>> /*
>>> @@ -367,10 +323,14 @@ static int do_efibootmgr(const char *fdt_opt)
>>> */
>>> static int do_boot_efi(const char *image_opt, const char *fdt_opt)
>>> {
>>> - unsigned long addr;
>>> - char *saddr;
>>> + void *image_buf;
>>> + struct efi_device_path *device_path, *image_path;
>>> + struct efi_device_path *memdp = NULL, *file_path = NULL;
>>> + const char *saddr;
>>> + unsigned long addr, size;
>>> + const char *size_str;
>>> + efi_handle_t mem_handle = NULL, handle;
>>> efi_status_t ret;
>>> - unsigned long fdt_addr;
>>>
>>> /* Allow unaligned memory access */
>>> allow_unaligned();
>>> @@ -390,36 +350,96 @@ static int do_boot_efi(const char *image_opt, const
>>> char *fdt_opt)
>>> return CMD_RET_FAILURE;
>>>
>>> #ifdef CONFIG_CMD_BOOTEFI_HELLO
>>> - if (!strcmp(argv[1], "hello")) {
>>> - ulong size = __efi_helloworld_end - __efi_helloworld_begin;
>>> -
>>> + if (!strcmp(image_opt, "hello")) {
>>> 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
>>> {
>>> - saddr = argv[1];
>>> + saddr = image_opt;
>>> + size_str = env_get("filesize");
>>> + if (size_str)
>>> + size = simple_strtoul(size_str, NULL, 16);
>>> + else
>>> + size = 0;
>>>
>>> addr = simple_strtoul(saddr, NULL, 16);
>>> /* Check that a numeric value was passed */
>>> if (!addr && *saddr != '0')
>>> return CMD_RET_USAGE;
>>> +
>>> + image_buf = map_sysmem(addr, size);
>>> +
>>> + device_path = bootefi_device_path;
>>> + image_path = bootefi_image_path;
>>> + }
>>> +
>>> + 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:
>>> + */
>>> + printf("WARNING: using memory device/image path, this may
>>> confuse some payloads!\n");
>>> +
>>> + /* actual addresses filled in after efi_load_image() */
>>> + memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
>>> + (uint64_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, (void *)0x1 /* FIXME */,
>>> + file_path, image_buf, size, &handle));
>>> + if (ret != EFI_SUCCESS)
>>> + goto err_prepare;
>>> +
>>> 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);
>>> + ret = do_bootefi_exec(handle);
>>> + printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK);
>>
>> Why should we need all this duplicate code. Cant we move that to
>> do_bootefi_exec()?
>>
>>> +
>>> + EFI_CALL(efi_unload_image(handle));
>>
>> That is the task of efi_exit().
>
> Who should be blamed for reclaiming a handle which is created
> with load_image()?
> What if an application doesn't call efi_exit() by itself?
When the last protocol interface is deleted from a handle via
UninstallProtocolInterface() the handle is deleted.
Best regards
Heinrich
>
> -Takahiro Akashi
>
>
>> Best regards
>>
>> Heinrich
>>
>>> +err_prepare:
>>> + if (device_path)
>>> + efi_free_pool(file_path);
>>> +
>>> +err_add_protocol:
>>> + if (mem_handle)
>>> + efi_delete_handle(mem_handle);
>>> + if (memdp)
>>> + efi_free_pool(memdp);
>>>
>>> if (ret != EFI_SUCCESS)
>>> return CMD_RET_FAILURE;
>>> - else
>>> - return CMD_RET_SUCCESS;
>>> +
>>> + return CMD_RET_SUCCESS;
>>> }
>>>
>>> #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
>>> @@ -577,7 +597,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"
>>> @@ -602,6 +622,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 6dfa2fef3cf0..6c09a7f40d02 100644
>>> --- a/include/efi_loader.h
>>> +++ b/include/efi_loader.h
>>> @@ -410,8 +410,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);
>>>
>>> @@ -565,8 +563,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 */,
>>> + lo.file_path, NULL, 0,
>>> + handle));
>>> if (ret != EFI_SUCCESS)
>>> goto error;
>>>
>>> @@ -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 74da6b01054a..8e2fa0111591 100644
>>> --- a/lib/efi_loader/efi_boottime.c
>>> +++ b/lib/efi_loader/efi_boottime.c
>>> @@ -1610,6 +1610,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)
>>> {
>>>
>>
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
