From mboxrd@z Thu Jan 1 00:00:00 1970 From: AKASHI Takahiro Date: Mon, 22 Apr 2019 09:36:04 +0900 Subject: [U-Boot] [PATCH 8/9] efi_loader: rework bootmgr/bootefi using load_image API In-Reply-To: <5e3a5bbe-3afd-4b67-6487-ad987e2e03c6@gmx.de> References: <20190419032236.8242-1-takahiro.akashi@linaro.org> <20190419032236.8242-9-takahiro.akashi@linaro.org> <797e7736-32dd-8bbf-457e-33d112fbe27d@gmx.de> <5e3a5bbe-3afd-4b67-6487-ad987e2e03c6@gmx.de> Message-ID: <20190422003602.GY7158@linaro.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de 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 > >> --- > >>   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 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 > > > >> > >> @@ -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)); > >> > >>       /* > >> > > > > >