[U-Boot] [RFC v3 09/10] efi_loader: rework bootmgr/bootefi using load_image API
Alexander Graf
agraf at csgraf.de
Thu Apr 18 08:29:35 UTC 2019
On 16.04.19 18:20, Heinrich Schuchardt wrote:
> On 4/16/19 12:56 PM, Heinrich Schuchardt wrote:
>> On 4/16/19 6:24 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.akashi at linaro.org>
>>> ---
>>> cmd/bootefi.c | 197
>>> +++++++++++++++++++---------------
>>> include/efi_loader.h | 5 +-
>>> lib/efi_loader/efi_bootmgr.c | 43 ++++----
>>> lib/efi_loader/efi_boottime.c | 2 +
>>> 4 files changed, 134 insertions(+), 113 deletions(-)
>>>
>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>> index f14966961b23..97d49a53a44b 100644
>>> --- a/cmd/bootefi.c
>>> +++ b/cmd/bootefi.c
>>> @@ -222,88 +222,39 @@ 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");
>>
>> In set_load_options() an error could occur (out of memory). So I think
>> it should return a status.
>>
>>> -
>>> - /* 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));
>>
>> Shouldn't we move this to set_load_options()?
>>
>>> 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");
>>
>> Shouldn't this move to efi_setup.c? Probably we should also set
>> OsIndications. I would prefer using efi_set_variable(). I think moving
>> this could be done in a preparatory patch.
>>
>>>
>>> /* 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);
>>>
>>> +err:
>>> return ret;
>>> }
>>>
>>> @@ -317,8 +268,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 +290,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;
>>> }
>>>
>>> /*
>>> @@ -367,10 +317,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();
>>> @@ -392,36 +346,94 @@ 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;
>>> }
>>>
>>> - 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:
>>> + */
>>> + printf("WARNING: using memory device/image path, this may
>>> confuse some payloads!\n");
>>
>> The EFI spec says that either of SourceBuffer or DevicePath may be NULL
>> when calling LoadImage().
>>
>>> +
>>> + /* 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.
>>
>> GRUB will fail anyway because it cannot determine the disk from which it
>> was loaded. So why are we doing this?
>
> In Rob's original commit bf19273e81eb ("efi_loader: Add mem-mapped
> for fallback") the comment was:
> "This fixes 'bootefi hello' after devicepath refactoring."
>
> EFI Shell.efi starts fine even when we set device_path and image_path
> to NULL.
>
> When loading GRUB from disk or via tftp the device path and the image
> path are set, e.g. for tftp:
>
> => dhcp grubarm.efi
> => bootefi 0x40200000 $fdtcontroladdr
> Found 0 disks
> ## Starting EFI application at 40200000 ...
> device_path:
> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/MAC(525400123456,0x1)
> image_path: /grubarm.efi
> error: disk `,msdos2' not found.
> Entering rescue mode...
> grub rescue>
>
> Maybe GRUB would run if all necessary modules were compiled in to do a
> network boot. But how would it find grub.conf?
>
> So my feeling is that we are creating the dummy memory device path
> because:
>
> - Once `bootefi hello` which is our own test program had a problem.
> - GRUB has a bug: it hangs or crashes if the file device path is not
> set in LoadImage().
>
> I think the problem should not be fixed in U-Boot but in GRUB.
>
> I am CC-ing Leif due to all the attention he has paid both to U-Boot
> and to GRUB.
Please also keep in mind that you can not fix already released versions
of GRUB.
Alex
More information about the U-Boot
mailing list