From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heinrich Schuchardt Date: Thu, 11 Mar 2021 13:50:34 +0100 Subject: [PATCH 4/6] efi_loader: Replace config option for initrd loading In-Reply-To: References: <20210305222303.2866284-1-ilias.apalodimas@linaro.org> <20210305222303.2866284-4-ilias.apalodimas@linaro.org> <0934e763-e275-3ee5-e481-370463dbd258@gmx.de> Message-ID: <19026159-5aba-bc88-d99e-eb044f730c07@gmx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 11.03.21 13:30, Ilias Apalodimas wrote: > On Thu, Mar 11, 2021 at 01:23:05PM +0100, Heinrich Schuchardt wrote: >> On 05.03.21 23:23, Ilias Apalodimas wrote: >>> Up to now we install EFI_LOAD_FILE2_PROTOCOL to load an initrd >>> unconditionally. Although we correctly return various EFI exit codes >>> depending on the file status (i.e EFI_NO_MEDIA, EFI_NOT_FOUND etc), the >>> kernel loader, only falls back to the cmdline interpreted initrd if the >>> protocol is not installed. >>> >>> This creates a problem for EFI installers, since they won't be able to >>> load their own initrd and continue the installation. It also makes the >>> feature hard to use, since we can either have a single initrd or we have >>> to recompile u-boot if the filename changes. >>> >>> So let's introduce a different logic that will decouple the initrd >>> path from the config option we currently have. >>> When defining a UEFI BootXXXX we can use the filepathlist and store >>> a file path pointing to our initrd. Specifically the EFI spec describes: >>> >>> "The first element of the array is a device path that describes the device >>> and location of the Image for this load option. Other device paths may >>> optionally exist in the FilePathList, but their usage is OSV specific" >>> >>> When the EFI application is launched through the bootmgr, we'll try to >>> interpret the extra device path. If that points to a file that exists on >>> our disk, we'll now install the load_file2 and the efi-stub will be able >>> to use it. >>> >>> This opens up another path using U-Boot and defines a new boot flow. >>> A user will be able to control the kernel/initrd pairs without explicit >>> cmdline args or GRUB. >>> >>> Signed-off-by: Ilias Apalodimas >>> --- >>> cmd/bootefi.c | 3 + >>> include/efi_loader.h | 1 + >>> lib/efi_loader/Kconfig | 15 +-- >>> lib/efi_loader/efi_bootmgr.c | 3 + >>> lib/efi_loader/efi_load_initrd.c | 209 +++++++++++++++++++++---------- >>> 5 files changed, 152 insertions(+), 79 deletions(-) >>> >>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c >>> index 271b385edea6..cba81ffe75e4 100644 >>> --- a/cmd/bootefi.c >>> +++ b/cmd/bootefi.c >>> @@ -358,6 +358,9 @@ static efi_status_t do_bootefi_exec(efi_handle_t handle, void *load_options) >>> >>> free(load_options); >>> >>> + if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD)) >>> + efi_initrd_deregister(); >>> + >>> return ret; >>> } >>> >>> diff --git a/include/efi_loader.h b/include/efi_loader.h >>> index eb11a8c7d4b1..0d4f5d9acc9f 100644 >>> --- a/include/efi_loader.h >>> +++ b/include/efi_loader.h >>> @@ -431,6 +431,7 @@ efi_status_t efi_net_register(void); >>> /* Called by bootefi to make the watchdog available */ >>> efi_status_t efi_watchdog_register(void); >>> efi_status_t efi_initrd_register(void); >>> +void efi_initrd_deregister(void); >>> /* Called by bootefi to make SMBIOS tables available */ >>> /** >>> * efi_acpi_register() - write out ACPI tables >>> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig >>> index e729f727df11..029f0e515f57 100644 >>> --- a/lib/efi_loader/Kconfig >>> +++ b/lib/efi_loader/Kconfig >>> @@ -317,16 +317,11 @@ config EFI_LOAD_FILE2_INITRD >>> bool "EFI_FILE_LOAD2_PROTOCOL for Linux initial ramdisk" >>> default n >>> help >>> - Expose a EFI_FILE_LOAD2_PROTOCOL that the Linux UEFI stub can >>> - use to load the initial ramdisk. Once this is enabled using >>> - initrd= will stop working. >>> - >>> -config EFI_INITRD_FILESPEC >>> - string "initramfs path" >>> - default "host 0:1 initrd" >>> - depends on EFI_LOAD_FILE2_INITRD >>> - help >>> - Full path of the initramfs file, e.g. mmc 0:2 initramfs.cpio.gz. >>> + Linux v5.7 and later can make use of this option. If the boot option >>> + selected by the UEFI boot manager specifies an existing file to be used >>> + as initial RAM disk, a Linux specific Load File2 protocol will be >>> + installed and Linux 5.7+ will ignore any initrd= command line >>> + argument. >>> >>> config EFI_SECURE_BOOT >>> bool "Enable EFI secure boot support" >>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c >>> index 25f5cebfdb67..d1baa8c71a4d 100644 >>> --- a/lib/efi_loader/efi_bootmgr.c >>> +++ b/lib/efi_loader/efi_bootmgr.c >>> @@ -118,6 +118,9 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle, >>> ret = efi_set_variable_int(L"BootCurrent", >>> &efi_global_variable_guid, >>> attributes, sizeof(n), &n, false); >> >> Why would you continue if BootCurrent is not set? > > Because the bitops below is just trying to simplify the code > reading, since both must call efi_unload_image(). Calling > efi_initrd_register() without a BootCurrent is guaranteed to fail as well. > >> >>> + /* try to register load file2 for initrd's */ >>> + if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD)) >>> + ret |= efi_initrd_register(); >> >> ret is not a boolean. So |= does not make sense to me here. > > Uhm? It's an unsigned long though and you can hav bitops properly? EFI_DEVICE_ERROR | EFI_INVALID_PARAMETER => EFI_ACCESS_DENIED That does not make much sense. Best regards Heinrich > >> >>> if (ret != EFI_SUCCESS) { >>> if (EFI_CALL(efi_unload_image(*handle)) >>> != EFI_SUCCESS) >>> diff --git a/lib/efi_loader/efi_load_initrd.c b/lib/efi_loader/efi_load_initrd.c >>> index b9ee8839054f..b11c5ee293fe 100644 >>> --- a/lib/efi_loader/efi_load_initrd.c >>> +++ b/lib/efi_loader/efi_load_initrd.c >>> @@ -5,7 +5,9 @@ >>> >>> #include >>> #include >>> +#include >>> #include >>> +#include >>> #include >>> #include >>> #include >>> @@ -39,41 +41,71 @@ static const struct efi_initrd_dp dp = { >>> } >>> }; >>> >>> +static efi_handle_t efi_initrd_handle; >>> + >>> /** >>> - * get_file_size() - retrieve the size of initramfs, set efi status on error >>> + * get_initrd_fp() - Get initrd device path from a FilePathList device path >>> * >>> - * @dev: device to read from, e.g. "mmc" >>> - * @part: device partition, e.g. "0:1" >>> - * @file: name of file >>> - * @status: EFI exit code in case of failure >>> + * @initrd_fp: the final initrd filepath >>> * >>> - * Return: size of file >>> + * Return: status code >>> */ >>> -static loff_t get_file_size(const char *dev, const char *part, const char *file, >>> - efi_status_t *status) >>> +static efi_status_t get_initrd_fp(struct efi_device_path **initrd_fp) >>> { >>> - loff_t sz = 0; >>> - int ret; >>> + const efi_guid_t lf2_initrd_guid = EFI_INITRD_MEDIA_GUID; >>> + struct efi_device_path *cur = NULL; >>> + struct efi_device_path *dp = NULL; >>> + struct efi_device_path *tmp_dp; >>> + efi_uintn_t ret; >>> + efi_uintn_t size; >>> >>> - ret = fs_set_blk_dev(dev, part, FS_TYPE_ANY); >>> - if (ret) { >>> - *status = EFI_NO_MEDIA; >> gg/> + /* >>> + * if bootmgr is setup with and initrd, the device path will be >>> + * in the FilePathList[] of our load options in Boot####. >>> + * The first device path of the multi instance device path will >>> + * start with a VenMedia and the initrds will follow. >>> + * >>> + * If the device path is not found return EFI_INVALID_PARAMETER. >>> + * We can then use this specific return value and not install the >>> + * protocol, while allowing the boot to continue >>> + */ >>> + dp = efi_get_dp_from_boot(lf2_initrd_guid); >>> + if (!dp) { >>> + ret = EFI_INVALID_PARAMETER; >>> goto out; >>> } >>> >>> - ret = fs_size(file, &sz); >>> - if (ret) { >>> - sz = 0; >>> - *status = EFI_NOT_FOUND; >>> + tmp_dp = dp; >>> + cur = efi_dp_get_next_instance(&tmp_dp, &size); >>> + if (!cur) { >>> + ret = EFI_OUT_OF_RESOURCES; >>> goto out; >>> } >>> >>> + /* >>> + * We don't care if the file path is eventually NULL here. The >>> + * callers will try to load a file from it and eventually fail >>> + * but let's be explicit with our return values >>> + */ >>> + if (!tmp_dp) { >>> + *initrd_fp = NULL; >>> + ret = EFI_NOT_FOUND; >>> + } else { >>> + *initrd_fp = efi_dp_dup(tmp_dp); >>> + if (*initrd_fp) >>> + ret = EFI_SUCCESS; >>> + else >>> + ret = EFI_OUT_OF_RESOURCES; >>> + } >>> + >>> out: >>> - return sz; >>> + efi_free_pool(cur); >>> + efi_free_pool(dp); >>> + return ret; >>> } >>> >>> /** >>> - * efi_load_file2initrd() - load initial RAM disk >>> + * efi_load_file2_initrd() - load initial RAM disk >>> * >>> * This function implements the LoadFile service of the EFI_LOAD_FILE2_PROTOCOL >>> * in order to load an initial RAM disk requested by the Linux kernel stub. >>> @@ -93,98 +125,131 @@ efi_load_file2_initrd(struct efi_load_file_protocol *this, >>> struct efi_device_path *file_path, bool boot_policy, >>> efi_uintn_t *buffer_size, void *buffer) >>> { >>> - char *filespec; >>> - efi_status_t status = EFI_NOT_FOUND; >>> - loff_t file_sz = 0, read_sz = 0; >>> - char *dev, *part, *file; >>> - char *pos; >>> - int ret; >>> + struct efi_device_path *initrd_fp = NULL; >>> + struct efi_file_info *info = NULL; >>> + efi_status_t ret = EFI_NOT_FOUND; >>> + struct efi_file_handle *f; >>> + efi_uintn_t bs; >>> >>> EFI_ENTRY("%p, %p, %d, %p, %p", this, file_path, boot_policy, >>> buffer_size, buffer); >>> >>> - filespec = strdup(CONFIG_EFI_INITRD_FILESPEC); >>> - if (!filespec) >>> - goto out; >>> - pos = filespec; >>> - >>> if (!this || this != &efi_lf2_protocol || >>> !buffer_size) { >>> - status = EFI_INVALID_PARAMETER; >>> + ret = EFI_INVALID_PARAMETER; >>> goto out; >>> } >>> >>> if (file_path->type != dp.end.type || >>> file_path->sub_type != dp.end.sub_type) { >>> - status = EFI_INVALID_PARAMETER; >>> + ret = EFI_INVALID_PARAMETER; >>> goto out; >>> } >>> >>> if (boot_policy) { >>> - status = EFI_UNSUPPORTED; >>> + ret = EFI_UNSUPPORTED; >>> goto out; >>> } >>> >>> - /* >>> - * expect a string with three space separated parts: >>> - * >>> - * * a block device type, e.g. "mmc" >>> - * * a device and partition identifier, e.g. "0:1" >>> - * * a file path on the block device, e.g. "/boot/initrd.cpio.gz" >>> - */ >>> - dev = strsep(&pos, " "); >>> - if (!dev) >>> + ret = get_initrd_fp(&initrd_fp); >>> + if (ret != EFI_SUCCESS) >>> goto out; >>> - part = strsep(&pos, " "); >>> - if (!part) >>> + >>> + /* Open file */ >>> + f = efi_file_from_path(initrd_fp); >>> + if (!f) { >>> + ret = EFI_NOT_FOUND; >>> goto out; >>> - file = strsep(&pos, " "); >>> - if (!file) >>> + } >>> + >>> + /* Get file size */ >>> + bs = 0; >>> + EFI_CALL(ret = f->getinfo(f, (efi_guid_t *)&efi_file_info_guid, >>> + &bs, info)); >> >> Please, use >> >> ret = EFI_CALL(f-> ...); >> >> througout the code. >> >> In efi_load_image_from_file() and efi_capsule_read_file() we also >> retrieve the file size. We should factor out a function efi_get_file_size(). >> >>> + if (ret != EFI_BUFFER_TOO_SMALL) { >>> + ret = EFI_DEVICE_ERROR; >>> goto out; >>> + } >>> >>> - file_sz = get_file_size(dev, part, file, &status); >>> - if (!file_sz) >>> + info = malloc(bs); >>> + EFI_CALL(ret = f->getinfo(f, (efi_guid_t *)&efi_file_info_guid, &bs, >>> + info)); >>> + if (ret != EFI_SUCCESS) >>> goto out; >>> >>> - if (!buffer || *buffer_size < file_sz) { >>> - status = EFI_BUFFER_TOO_SMALL; >>> - *buffer_size = file_sz; >>> + bs = info->file_size; >>> + if (!buffer || *buffer_size < bs) { >>> + ret = EFI_BUFFER_TOO_SMALL; >>> + *buffer_size = bs; >>> } else { >>> - ret = fs_set_blk_dev(dev, part, FS_TYPE_ANY); >>> - if (ret) { >>> - status = EFI_NO_MEDIA; >>> - goto out; >>> - } >>> - >>> - ret = fs_read(file, map_to_sysmem(buffer), 0, *buffer_size, >>> - &read_sz); >>> - if (ret || read_sz != file_sz) >>> - goto out; >>> - *buffer_size = read_sz; >>> - >>> - status = EFI_SUCCESS; >>> + EFI_CALL(ret = f->read(f, &bs, (void *)(uintptr_t)buffer)); >>> + *buffer_size = bs; >>> } >>> >>> out: >>> - free(filespec); >>> - return EFI_EXIT(status); >>> + free(info); >>> + efi_free_pool(initrd_fp); >>> + EFI_CALL(f->close(f)); >>> + return EFI_EXIT(ret); >>> +} >>> + >>> +/** >>> + * check_initrd() - Determine if the file defined as an initrd in Boot#### >>> + * load_options device path is present >>> + * >>> + * Return: status code >>> + */ >>> +static efi_status_t check_initrd(void) >>> +{ >>> + struct efi_device_path *initrd_fp = NULL; >>> + struct efi_file_handle *f; >>> + efi_status_t ret; >>> + >>> + ret = get_initrd_fp(&initrd_fp); >>> + if (ret != EFI_SUCCESS) >>> + goto out; >>> + >>> + /* >>> + * If the file is not found, but the file path is set, return an error >>> + * and trigger the bootmgr fallback >>> + */ >>> + f = efi_file_from_path(initrd_fp); >>> + if (!f) { >> >> This deserves a log_warning(). >> >>> + ret = EFI_NOT_FOUND; >>> + goto out; >>> + } >>> + >>> + EFI_CALL(f->close(f)); >>> + >>> +out: >>> + efi_free_pool(initrd_fp); >>> + return ret; >>> } >>> >>> /** >>> * efi_initrd_register() - create handle for loading initial RAM disk >>> * >>> * This function creates a new handle and installs a Linux specific vendor >>> - * device path and an EFI_LOAD_FILE_2_PROTOCOL. Linux uses the device path >>> + * device path and an EFI_LOAD_FILE2_PROTOCOL. Linux uses the device path >>> * to identify the handle and then calls the LoadFile service of the >>> - * EFI_LOAD_FILE_2_PROTOCOL to read the initial RAM disk. >>> + * EFI_LOAD_FILE2_PROTOCOL to read the initial RAM disk. >>> * >>> * Return: status code >>> */ >>> efi_status_t efi_initrd_register(void) >>> { >>> - efi_handle_t efi_initrd_handle = NULL; >>> efi_status_t ret; >>> >>> + /* >>> + * Allow the user to continue if Boot#### file path is not set for >>> + * an initrd >>> + */ >>> + ret = check_initrd(); >>> + if (ret == EFI_INVALID_PARAMETER) >>> + return EFI_SUCCESS; >>> + if (ret != EFI_SUCCESS) >>> + return ret; >>> + >>> ret = EFI_CALL(efi_install_multiple_protocol_interfaces >>> (&efi_initrd_handle, >>> /* initramfs */ >>> @@ -196,3 +261,9 @@ efi_status_t efi_initrd_register(void) >>> >>> return ret; >>> } >>> + >>> +void efi_initrd_deregister(void) >> >> Missing description of an exported function. >> >> Best regards >> >> Heinrich >> >>> +{ >>> + efi_delete_handle(efi_initrd_handle); >>> + efi_initrd_handle = NULL; >>> +} >>> >>