| Message ID | 20210113111149.64567-3-ilias.apalodimas@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Change logic of EFI LoadFile2 protocol for initrd loading | expand |
On 13.01.21 12:11, 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 <ilias.apalodimas@linaro.org> > --- > cmd/bootefi.c | 3 + > include/efi_loader.h | 1 + > lib/efi_loader/Kconfig | 13 +-- > lib/efi_loader/efi_bootmgr.c | 3 + > lib/efi_loader/efi_load_initrd.c | 154 ++++++++++++++++--------------- > 5 files changed, 91 insertions(+), 83 deletions(-) > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c > index fdf909f8da2c..053927d5d986 100644 > --- a/cmd/bootefi.c > +++ b/cmd/bootefi.c > @@ -357,6 +357,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 4719fa93f06d..5d2e161963c3 100644 > --- a/include/efi_loader.h > +++ b/include/efi_loader.h > @@ -432,6 +432,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 fdf245dea30b..597a3ee86c88 100644 > --- a/lib/efi_loader/Kconfig > +++ b/lib/efi_loader/Kconfig > @@ -307,14 +307,11 @@ config EFI_LOAD_FILE2_INITRD > 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=<ramdisk> 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. > + initrd=<ramdisk> will stop working. The protocol will only be How about "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=<ramdisk> command line argument." > + installed if bootmgr is used and the file is found on the defined > + path. A boot entry of Boot0001 will try to match Initrd0001 and use This does not match the implementation. > + it. Initrd EFI variable format should be '<dev> <part> <filename>' > + i.e 'mmc 0:1 boot/initrd' "The efidebug command can be used to specify the file with the initial RAM disk." > > 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 0fe503a7f376..aa5d521535ee 100644 > --- a/lib/efi_loader/efi_bootmgr.c > +++ b/lib/efi_loader/efi_bootmgr.c > @@ -222,6 +222,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); > + /* try to register load file2 for initrd's */ > + if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD)) > + ret |= efi_initrd_register(); > 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..bb0c5e63b65c 100644 > --- a/lib/efi_loader/efi_load_initrd.c > +++ b/lib/efi_loader/efi_load_initrd.c > @@ -5,7 +5,9 @@ > > #include <common.h> > #include <efi_loader.h> > +#include <efi_helper.h> > #include <efi_load_initrd.h> > +#include <efi_variable.h> > #include <fs.h> > #include <malloc.h> > #include <mapmem.h> > @@ -39,41 +41,10 @@ static const struct efi_initrd_dp dp = { > } > }; > > -/** > - * get_file_size() - retrieve the size of initramfs, set efi status on error > - * > - * @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 > - * > - * Return: size of file > - */ > -static loff_t get_file_size(const char *dev, const char *part, const char *file, > - efi_status_t *status) > -{ > - loff_t sz = 0; > - int ret; > - > - ret = fs_set_blk_dev(dev, part, FS_TYPE_ANY); > - if (ret) { > - *status = EFI_NO_MEDIA; > - goto out; > - } > - > - ret = fs_size(file, &sz); > - if (ret) { > - sz = 0; > - *status = EFI_NOT_FOUND; > - goto out; > - } > - > -out: > - return sz; > -} > +static efi_handle_t efi_initrd_handle; > > /** > - * 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,21 +64,15 @@ 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; In all our other coding this variable is called ret. I would prefer to do the same here too. Best regards Heinrich > - loff_t file_sz = 0, read_sz = 0; > - char *dev, *part, *file; > - char *pos; > - int ret; > + struct efi_device_path *initrd_path = NULL; > + struct efi_file_info *info = NULL; > + 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; > @@ -125,51 +90,82 @@ efi_load_file2_initrd(struct efi_load_file_protocol *this, > 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) > + initrd_path = efi_get_fp_from_boot(INITRD_DP); > + if (!initrd_path) { > + status = EFI_NOT_FOUND; > goto out; > - part = strsep(&pos, " "); > - if (!part) > + } > + > + /* Open file */ > + f = efi_file_from_path(initrd_path); > + if (!f) { > + status = EFI_NOT_FOUND; > goto out; > - file = strsep(&pos, " "); > - if (!file) > + } > + > + /* Get file size */ > + bs = 0; > + EFI_CALL(status = f->getinfo(f, (efi_guid_t *)&efi_file_info_guid, > + &bs, info)); > + if (status != EFI_BUFFER_TOO_SMALL) { > + status = EFI_DEVICE_ERROR; > goto out; > + } > > - file_sz = get_file_size(dev, part, file, &status); > - if (!file_sz) > + info = malloc(bs); > + EFI_CALL(status = f->getinfo(f, (efi_guid_t *)&efi_file_info_guid, &bs, > + info)); > + if (status != EFI_SUCCESS) > goto out; > > - if (!buffer || *buffer_size < file_sz) { > + bs = info->file_size; > + //efi_load_image_from_file > + if (!buffer || *buffer_size < bs) { > status = EFI_BUFFER_TOO_SMALL; > - *buffer_size = file_sz; > + *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(status = f->read(f, &bs, (void *)(uintptr_t)buffer)); > + *buffer_size = bs; > } > > out: > - free(filespec); > + free(info); > + efi_free_pool(initrd_path); > + EFI_CALL(f->close(f)); > return EFI_EXIT(status); > } > > +/** > + * 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 *file_path; > + struct efi_file_handle *f; > + > + /* > + * if bootmgr is setup with and initrd, that will be the second > + * device path instance in our load options located in Boot#### > + */ > + file_path = efi_get_fp_from_boot(INITRD_DP); > + if (!file_path) > + return EFI_NOT_FOUND; > + > + f = efi_file_from_path(file_path); > + if (!f) { > + efi_free_pool(file_path); > + return EFI_NOT_FOUND; > + } > + > + EFI_CALL(f->close(f)); > + efi_free_pool(file_path); > + > + return EFI_SUCCESS; > +} > + > /** > * efi_initrd_register() - create handle for loading initial RAM disk > * > @@ -182,9 +178,12 @@ out: > */ > efi_status_t efi_initrd_register(void) > { > - efi_handle_t efi_initrd_handle = NULL; > efi_status_t ret; > > + ret = check_initrd(); > + if (ret != EFI_SUCCESS) > + return ret; > + > ret = EFI_CALL(efi_install_multiple_protocol_interfaces > (&efi_initrd_handle, > /* initramfs */ > @@ -196,3 +195,8 @@ efi_status_t efi_initrd_register(void) > > return ret; > } > + > +void efi_initrd_deregister(void) > +{ > + efi_delete_handle(efi_initrd_handle); > +} >
> > + initrd=<ramdisk> will stop working. The protocol will only be [...] > > How about > > "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=<ramdisk> command line > argument." > > > + installed if bootmgr is used and the file is found on the defined > > + path. A boot entry of Boot0001 will try to match Initrd0001 and use > > This does not match the implementation. > > > + it. Initrd EFI variable format should be '<dev> <part> <filename>' > > + i.e 'mmc 0:1 boot/initrd' > > "The efidebug command can be used to specify the file with the initial > RAM disk." Seems like I missed some text while amending the description. Your proposal sounds good I'll update it. [...] > > In all our other coding this variable is called ret. > I would prefer to do the same here too. No problem Cheers /Ilias > > Best regards >
Ilias, On Wed, Jan 13, 2021 at 01:11:48PM +0200, 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" I wonder what "OSV specific" does and should mean. Apparently, using a "array of device paths" is U-Boot specific for now and any distro who wants to use U-Boot as an EFI boot loader needs to (at least, preferably) take care of this. It would be sad that the installation process cannot be EFI-implementation agnostic in terms of EFI's purpose. So do you intend to propose your idea as a common practice to linux community? > 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 <ilias.apalodimas@linaro.org> > --- > cmd/bootefi.c | 3 + > include/efi_loader.h | 1 + > lib/efi_loader/Kconfig | 13 +-- > lib/efi_loader/efi_bootmgr.c | 3 + > lib/efi_loader/efi_load_initrd.c | 154 ++++++++++++++++--------------- > 5 files changed, 91 insertions(+), 83 deletions(-) > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c > index fdf909f8da2c..053927d5d986 100644 > --- a/cmd/bootefi.c > +++ b/cmd/bootefi.c > @@ -357,6 +357,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(); > + I understand why you want do "deregister" the initrd handle, but the handle for the loaded image is still valid at this point. So it looks inconsistent from the viewpoint of API's. > return ret; > } > > diff --git a/include/efi_loader.h b/include/efi_loader.h > index 4719fa93f06d..5d2e161963c3 100644 > --- a/include/efi_loader.h > +++ b/include/efi_loader.h > @@ -432,6 +432,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 fdf245dea30b..597a3ee86c88 100644 > --- a/lib/efi_loader/Kconfig > +++ b/lib/efi_loader/Kconfig > @@ -307,14 +307,11 @@ config EFI_LOAD_FILE2_INITRD > 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=<ramdisk> 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. > + initrd=<ramdisk> will stop working. The protocol will only be > + installed if bootmgr is used and the file is found on the defined > + path. A boot entry of Boot0001 will try to match Initrd0001 and use > + it. Initrd EFI variable format should be '<dev> <part> <filename>' > + i.e 'mmc 0:1 boot/initrd' > > 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 0fe503a7f376..aa5d521535ee 100644 > --- a/lib/efi_loader/efi_bootmgr.c > +++ b/lib/efi_loader/efi_bootmgr.c > @@ -222,6 +222,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); > + /* try to register load file2 for initrd's */ > + if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD)) > + ret |= efi_initrd_register(); I think that, even if the config(EFI_LOAD_FILE@_INITRD) is turned on, we should be allowed to boot linux without initrd if we want. In addition, we may want to boot non-linux images by using U-boot with the config enabled. > 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..bb0c5e63b65c 100644 > --- a/lib/efi_loader/efi_load_initrd.c > +++ b/lib/efi_loader/efi_load_initrd.c > @@ -5,7 +5,9 @@ > > #include <common.h> > #include <efi_loader.h> > +#include <efi_helper.h> > #include <efi_load_initrd.h> > +#include <efi_variable.h> > #include <fs.h> > #include <malloc.h> > #include <mapmem.h> > @@ -39,41 +41,10 @@ static const struct efi_initrd_dp dp = { > } > }; > > -/** > - * get_file_size() - retrieve the size of initramfs, set efi status on error > - * > - * @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 > - * > - * Return: size of file > - */ > -static loff_t get_file_size(const char *dev, const char *part, const char *file, > - efi_status_t *status) > -{ > - loff_t sz = 0; > - int ret; > - > - ret = fs_set_blk_dev(dev, part, FS_TYPE_ANY); > - if (ret) { > - *status = EFI_NO_MEDIA; > - goto out; > - } > - > - ret = fs_size(file, &sz); > - if (ret) { > - sz = 0; > - *status = EFI_NOT_FOUND; > - goto out; > - } > - > -out: > - return sz; > -} > +static efi_handle_t efi_initrd_handle; > > /** > - * 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,21 +64,15 @@ 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_path = NULL; > + struct efi_file_info *info = NULL; > + 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; > @@ -125,51 +90,82 @@ efi_load_file2_initrd(struct efi_load_file_protocol *this, > 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) > + initrd_path = efi_get_fp_from_boot(INITRD_DP); > + if (!initrd_path) { > + status = EFI_NOT_FOUND; > goto out; > - part = strsep(&pos, " "); > - if (!part) > + } > + > + /* Open file */ > + f = efi_file_from_path(initrd_path); > + if (!f) { > + status = EFI_NOT_FOUND; > goto out; The logic here (efi_get_fp_from_boot, then efi_file_from_path) is also seen in check_initrd(). Pls refactor the code. -Takahiro Akashi > - file = strsep(&pos, " "); > - if (!file) > + } > + > + /* Get file size */ > + bs = 0; > + EFI_CALL(status = f->getinfo(f, (efi_guid_t *)&efi_file_info_guid, > + &bs, info)); > + if (status != EFI_BUFFER_TOO_SMALL) { > + status = EFI_DEVICE_ERROR; > goto out; > + } > > - file_sz = get_file_size(dev, part, file, &status); > - if (!file_sz) > + info = malloc(bs); > + EFI_CALL(status = f->getinfo(f, (efi_guid_t *)&efi_file_info_guid, &bs, > + info)); > + if (status != EFI_SUCCESS) > goto out; > > - if (!buffer || *buffer_size < file_sz) { > + bs = info->file_size; > + //efi_load_image_from_file > + if (!buffer || *buffer_size < bs) { > status = EFI_BUFFER_TOO_SMALL; > - *buffer_size = file_sz; > + *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(status = f->read(f, &bs, (void *)(uintptr_t)buffer)); > + *buffer_size = bs; > } > > out: > - free(filespec); > + free(info); > + efi_free_pool(initrd_path); > + EFI_CALL(f->close(f)); > return EFI_EXIT(status); > } > > +/** > + * 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 *file_path; > + struct efi_file_handle *f; > + > + /* > + * if bootmgr is setup with and initrd, that will be the second > + * device path instance in our load options located in Boot#### > + */ > + file_path = efi_get_fp_from_boot(INITRD_DP); > + if (!file_path) > + return EFI_NOT_FOUND; > + > + f = efi_file_from_path(file_path); > + if (!f) { > + efi_free_pool(file_path); > + return EFI_NOT_FOUND; > + } > + > + EFI_CALL(f->close(f)); > + efi_free_pool(file_path); > + > + return EFI_SUCCESS; > +} > + > /** > * efi_initrd_register() - create handle for loading initial RAM disk > * > @@ -182,9 +178,12 @@ out: > */ > efi_status_t efi_initrd_register(void) > { > - efi_handle_t efi_initrd_handle = NULL; > efi_status_t ret; > > + ret = check_initrd(); > + if (ret != EFI_SUCCESS) > + return ret; > + > ret = EFI_CALL(efi_install_multiple_protocol_interfaces > (&efi_initrd_handle, > /* initramfs */ > @@ -196,3 +195,8 @@ efi_status_t efi_initrd_register(void) > > return ret; > } > + > +void efi_initrd_deregister(void) > +{ > + efi_delete_handle(efi_initrd_handle); > +} > -- > 2.30.0.rc2 >
Akashi-san, On Thu, Jan 14, 2021 at 01:23:30PM +0900, AKASHI Takahiro wrote: > Ilias, > > On Wed, Jan 13, 2021 at 01:11:48PM +0200, 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" > > I wonder what "OSV specific" does and should mean. Judging from the context is used, I assumed it meant OS vendor. > Apparently, using a "array of device paths" is U-Boot specific for now > and any distro who wants to use U-Boot as an EFI boot loader needs to > (at least, preferably) take care of this. It would be sad > that the installation process cannot be EFI-implementation agnostic > in terms of EFI's purpose. > That's the reason I changed the approach completely. This one adheres to the EFI spec compared to the previous version that used EFI variables. > So do you intend to propose your idea as a common practice to linux community? Yes, although that depends on the definition of the linux community. The kernel itself is irrelevant here. We need to make sure though that applications like efibootmgr follow the same principle. > > > 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 <ilias.apalodimas@linaro.org> > > --- > > cmd/bootefi.c | 3 + > > include/efi_loader.h | 1 + > > lib/efi_loader/Kconfig | 13 +-- > > lib/efi_loader/efi_bootmgr.c | 3 + > > lib/efi_loader/efi_load_initrd.c | 154 ++++++++++++++++--------------- > > 5 files changed, 91 insertions(+), 83 deletions(-) > > > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c > > index fdf909f8da2c..053927d5d986 100644 > > --- a/cmd/bootefi.c > > +++ b/cmd/bootefi.c > > @@ -357,6 +357,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(); > > + > > I understand why you want do "deregister" the initrd handle, > but the handle for the loaded image is still valid at this point. > So it looks inconsistent from the viewpoint of API's. > Hmm why? Won't this code be called after efi_exit()? In that case the loaded image handle is deleted as well. > > return ret; > > } [...] > > @@ -222,6 +222,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); > > + /* try to register load file2 for initrd's */ > > + if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD)) > > + ret |= efi_initrd_register(); > > I think that, even if the config(EFI_LOAD_FILE@_INITRD) is turned on, > we should be allowed to boot linux without initrd if we want. > In addition, we may want to boot non-linux images by using U-boot > with the config enabled. > That was the case with the previous patchset. The efi_initrd_register() was always returning EFI_SUCCESS. Heinrich asked me respect the bootmgr behavior, since a distro without initrd will most likely fail to boot, so he wanted to allow fallbacks on the bootmgr. The reasoning is that the kernel, in EFI mode, has 3 ways of discovering an iniitrd. 1. Fixups on initrd-start/end in the DTB. In that case the stub is unaware of how the DTB gets loaded 2. initrd=XXXX in the command line. In that case the kernel uses EFI_SIMPLE_FILESYSYSTEM_PROTOCOL to load the parsed file. 3. The load option this patch implements via LOAD_FILE2 If the user specifies an initrd, he needs it to boot and it's unlikely to specify both the initrd=xxxxx and add it in the Boot#### variable. I think I can fix the use case and return EFI_SUCCESS from efi_initrd_register() if the extra instance is not found in the device path array. In that case a user will be able to boot without it, but if the device path is specified, the file *must* be there. If it's not the bootmgr will fallback to the next available boot option. > > if (ret != EFI_SUCCESS) { > > + status = EFI_NOT_FOUND; [...] > > goto out; > > The logic here (efi_get_fp_from_boot, then efi_file_from_path) > is also seen in check_initrd(). Pls refactor the code. check_initrd() closes the file handle though and frees the device path. It's supposed to serve as a wrapper for 'is the file present', while in the second function we need the device path and the file handle to load the file. Thanks for taking the time to review this. Regards /Ilias
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index fdf909f8da2c..053927d5d986 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -357,6 +357,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 4719fa93f06d..5d2e161963c3 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -432,6 +432,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 fdf245dea30b..597a3ee86c88 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -307,14 +307,11 @@ config EFI_LOAD_FILE2_INITRD 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=<ramdisk> 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. + initrd=<ramdisk> will stop working. The protocol will only be + installed if bootmgr is used and the file is found on the defined + path. A boot entry of Boot0001 will try to match Initrd0001 and use + it. Initrd EFI variable format should be '<dev> <part> <filename>' + i.e 'mmc 0:1 boot/initrd' 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 0fe503a7f376..aa5d521535ee 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -222,6 +222,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); + /* try to register load file2 for initrd's */ + if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD)) + ret |= efi_initrd_register(); 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..bb0c5e63b65c 100644 --- a/lib/efi_loader/efi_load_initrd.c +++ b/lib/efi_loader/efi_load_initrd.c @@ -5,7 +5,9 @@ #include <common.h> #include <efi_loader.h> +#include <efi_helper.h> #include <efi_load_initrd.h> +#include <efi_variable.h> #include <fs.h> #include <malloc.h> #include <mapmem.h> @@ -39,41 +41,10 @@ static const struct efi_initrd_dp dp = { } }; -/** - * get_file_size() - retrieve the size of initramfs, set efi status on error - * - * @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 - * - * Return: size of file - */ -static loff_t get_file_size(const char *dev, const char *part, const char *file, - efi_status_t *status) -{ - loff_t sz = 0; - int ret; - - ret = fs_set_blk_dev(dev, part, FS_TYPE_ANY); - if (ret) { - *status = EFI_NO_MEDIA; - goto out; - } - - ret = fs_size(file, &sz); - if (ret) { - sz = 0; - *status = EFI_NOT_FOUND; - goto out; - } - -out: - return sz; -} +static efi_handle_t efi_initrd_handle; /** - * 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,21 +64,15 @@ 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_path = NULL; + struct efi_file_info *info = NULL; + 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; @@ -125,51 +90,82 @@ efi_load_file2_initrd(struct efi_load_file_protocol *this, 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) + initrd_path = efi_get_fp_from_boot(INITRD_DP); + if (!initrd_path) { + status = EFI_NOT_FOUND; goto out; - part = strsep(&pos, " "); - if (!part) + } + + /* Open file */ + f = efi_file_from_path(initrd_path); + if (!f) { + status = EFI_NOT_FOUND; goto out; - file = strsep(&pos, " "); - if (!file) + } + + /* Get file size */ + bs = 0; + EFI_CALL(status = f->getinfo(f, (efi_guid_t *)&efi_file_info_guid, + &bs, info)); + if (status != EFI_BUFFER_TOO_SMALL) { + status = EFI_DEVICE_ERROR; goto out; + } - file_sz = get_file_size(dev, part, file, &status); - if (!file_sz) + info = malloc(bs); + EFI_CALL(status = f->getinfo(f, (efi_guid_t *)&efi_file_info_guid, &bs, + info)); + if (status != EFI_SUCCESS) goto out; - if (!buffer || *buffer_size < file_sz) { + bs = info->file_size; + //efi_load_image_from_file + if (!buffer || *buffer_size < bs) { status = EFI_BUFFER_TOO_SMALL; - *buffer_size = file_sz; + *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(status = f->read(f, &bs, (void *)(uintptr_t)buffer)); + *buffer_size = bs; } out: - free(filespec); + free(info); + efi_free_pool(initrd_path); + EFI_CALL(f->close(f)); return EFI_EXIT(status); } +/** + * 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 *file_path; + struct efi_file_handle *f; + + /* + * if bootmgr is setup with and initrd, that will be the second + * device path instance in our load options located in Boot#### + */ + file_path = efi_get_fp_from_boot(INITRD_DP); + if (!file_path) + return EFI_NOT_FOUND; + + f = efi_file_from_path(file_path); + if (!f) { + efi_free_pool(file_path); + return EFI_NOT_FOUND; + } + + EFI_CALL(f->close(f)); + efi_free_pool(file_path); + + return EFI_SUCCESS; +} + /** * efi_initrd_register() - create handle for loading initial RAM disk * @@ -182,9 +178,12 @@ out: */ efi_status_t efi_initrd_register(void) { - efi_handle_t efi_initrd_handle = NULL; efi_status_t ret; + ret = check_initrd(); + if (ret != EFI_SUCCESS) + return ret; + ret = EFI_CALL(efi_install_multiple_protocol_interfaces (&efi_initrd_handle, /* initramfs */ @@ -196,3 +195,8 @@ efi_status_t efi_initrd_register(void) return ret; } + +void efi_initrd_deregister(void) +{ + efi_delete_handle(efi_initrd_handle); +}
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 <ilias.apalodimas@linaro.org> --- cmd/bootefi.c | 3 + include/efi_loader.h | 1 + lib/efi_loader/Kconfig | 13 +-- lib/efi_loader/efi_bootmgr.c | 3 + lib/efi_loader/efi_load_initrd.c | 154 ++++++++++++++++--------------- 5 files changed, 91 insertions(+), 83 deletions(-) -- 2.30.0.rc2