| Message ID | 20201230150722.154663-7-ilias.apalodimas@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Change logic of EFI LoadFile2 protocol for initrd loading | expand |
On 12/30/20 4:07 PM, 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 the EFI application is launched through the bootmgr, we'll try to > match the BootCurrent value to an Initrd#### EFI variable. > e.g Boot0000 -> Initrd0000, Boot0010 -> Initrd0010 etc. > > The Initrd#### EFI variable is expected to include the full file path, > i.e 'mmc 0:1 initrd'. If the file is found, we'll install the > appropriate protocol so the kernel's efi-stub can use it and load our > initrd. If the file is not found the kernel will still try to load an > initrd parsing the kernel cmdline, since the protocol won't be installed. > > 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. So we can base the whole boot flow on the Boot#### > and Initrd#### paired values. > > Suggested-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > cmd/bootefi.c | 13 +++++ > include/efi_loader.h | 2 +- > lib/efi_loader/Kconfig | 13 ++--- > lib/efi_loader/efi_load_initrd.c | 93 ++++++++++---------------------- > 4 files changed, 46 insertions(+), 75 deletions(-) > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c > index fdf909f8da2c..0234b0df2258 100644 > --- a/cmd/bootefi.c > +++ b/cmd/bootefi.c > @@ -377,6 +377,19 @@ static int do_efibootmgr(void) > return CMD_RET_FAILURE; > } > > + /* efi_exit() will delete all protocols and the handle itself on exit */ > + if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD)) { > + ret = efi_initrd_register(&handle); > + /* Just warn the user. If the installation fails, the kernel > + * either load an initrd specificied in the cmdline or fail > + * to boot. > + * If we decide to fail the command here we need to uninstall > + * the protocols efi_bootmgr_load() installed > + */ > + if (ret != EFI_SUCCESS) > + log_warning("EFI boot manager: Failed to install Loadfile2 for initrd\n"); > + } > + > ret = do_bootefi_exec(handle, load_options); > > if (ret != EFI_SUCCESS) > diff --git a/include/efi_loader.h b/include/efi_loader.h > index af30dbafab77..000cc942e31c 100644 > --- a/include/efi_loader.h > +++ b/include/efi_loader.h > @@ -422,7 +422,7 @@ efi_status_t efi_gop_register(void); > 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); > +efi_status_t efi_initrd_register(efi_handle_t *initrd_handle); > /* 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 dd8b93bd3c5a..96b5934a221d 100644 > --- a/lib/efi_loader/Kconfig > +++ b/lib/efi_loader/Kconfig > @@ -212,14 +212,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_load_initrd.c b/lib/efi_loader/efi_load_initrd.c > index 4ec55d70979d..6289957c97bc 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> > @@ -43,40 +45,7 @@ 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; > -} > - > -/** > - * 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. Please, describe how UEFI variables are used here. > @@ -96,21 +65,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 load_file_info info; > + char initrd[] = "Initrd"; > > 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; > @@ -128,24 +91,11 @@ 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) > - goto out; > - part = strsep(&pos, " "); > - if (!part) > - goto out; > - file = strsep(&pos, " "); > - if (!file) > + status = efi_get_fp_from_var(initrd, &info); > + if (status != EFI_SUCCESS) > goto out; > > - file_sz = get_file_size(dev, part, file, &status); > + file_sz = get_file_size(&info, &status); > if (!file_sz) > goto out; > > @@ -153,23 +103,25 @@ efi_load_file2_initrd(struct efi_load_file_protocol *this, > status = EFI_BUFFER_TOO_SMALL; > *buffer_size = file_sz; > } else { > - ret = fs_set_blk_dev(dev, part, FS_TYPE_ANY); > + ret = fs_set_blk_dev(info.dev, info.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) > + ret = fs_read(info.filename, map_to_sysmem(buffer), 0, > + *buffer_size, &read_sz); > + if (ret || read_sz != file_sz) { > + status = EFI_DEVICE_ERROR; > goto out; > + } > *buffer_size = read_sz; > > status = EFI_SUCCESS; > } > > out: > - free(filespec); > return EFI_EXIT(status); > } > > @@ -183,13 +135,22 @@ out: > * > * Return: status code > */ > -efi_status_t efi_initrd_register(void) > +efi_status_t efi_initrd_register(efi_handle_t *efi_initrd_handle) Please, describe the new function parameter. Best regards Heinrich > { > - efi_handle_t efi_initrd_handle = NULL; > efi_status_t ret; > + struct load_file_info info; > + char initrd[] = "Initrd"; > + > + ret = efi_get_fp_from_var(initrd, &info); > + /* > + * Don't fail here. If we don't install the protocol the efi-stub will > + * try to load and initrd parsing the kernel cmdline > + */ > + if (ret != EFI_SUCCESS) > + return EFI_SUCCESS; > > ret = EFI_CALL(efi_install_multiple_protocol_interfaces > - (&efi_initrd_handle, > + (efi_initrd_handle, > /* initramfs */ > &efi_guid_device_path, &dp, > /* LOAD_FILE2 */ >
On Wed, Dec 30, 2020 at 05:07:18PM +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 the EFI application is launched through the bootmgr, we'll try to > match the BootCurrent value to an Initrd#### EFI variable. > e.g Boot0000 -> Initrd0000, Boot0010 -> Initrd0010 etc. Do you think this semantics be kept U-boot specific? It seems that it can work for any other EFI firmware (implementation). > The Initrd#### EFI variable is expected to include the full file path, > i.e 'mmc 0:1 initrd'. If the file is found, we'll install the If so, the content of "Initrd####" should contain a generic EFI representation of file path. Please note that "Boot####" internally contains a flattened string of "EFI device path" to the image while efidebug command accepts a style of "mmc 0:1 image" as arguments to "efidebug boot add" sub-command. -Takahiro Akashi > appropriate protocol so the kernel's efi-stub can use it and load our > initrd. If the file is not found the kernel will still try to load an > initrd parsing the kernel cmdline, since the protocol won't be installed. > > 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. So we can base the whole boot flow on the Boot#### > and Initrd#### paired values. > > Suggested-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > cmd/bootefi.c | 13 +++++ > include/efi_loader.h | 2 +- > lib/efi_loader/Kconfig | 13 ++--- > lib/efi_loader/efi_load_initrd.c | 93 ++++++++++---------------------- > 4 files changed, 46 insertions(+), 75 deletions(-) > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c > index fdf909f8da2c..0234b0df2258 100644 > --- a/cmd/bootefi.c > +++ b/cmd/bootefi.c > @@ -377,6 +377,19 @@ static int do_efibootmgr(void) > return CMD_RET_FAILURE; > } > > + /* efi_exit() will delete all protocols and the handle itself on exit */ > + if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD)) { > + ret = efi_initrd_register(&handle); > + /* Just warn the user. If the installation fails, the kernel > + * either load an initrd specificied in the cmdline or fail > + * to boot. > + * If we decide to fail the command here we need to uninstall > + * the protocols efi_bootmgr_load() installed > + */ > + if (ret != EFI_SUCCESS) > + log_warning("EFI boot manager: Failed to install Loadfile2 for initrd\n"); > + } > + > ret = do_bootefi_exec(handle, load_options); > > if (ret != EFI_SUCCESS) > diff --git a/include/efi_loader.h b/include/efi_loader.h > index af30dbafab77..000cc942e31c 100644 > --- a/include/efi_loader.h > +++ b/include/efi_loader.h > @@ -422,7 +422,7 @@ efi_status_t efi_gop_register(void); > 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); > +efi_status_t efi_initrd_register(efi_handle_t *initrd_handle); > /* 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 dd8b93bd3c5a..96b5934a221d 100644 > --- a/lib/efi_loader/Kconfig > +++ b/lib/efi_loader/Kconfig > @@ -212,14 +212,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_load_initrd.c b/lib/efi_loader/efi_load_initrd.c > index 4ec55d70979d..6289957c97bc 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> > @@ -43,40 +45,7 @@ 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; > -} > - > -/** > - * 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. > @@ -96,21 +65,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 load_file_info info; > + char initrd[] = "Initrd"; > > 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; > @@ -128,24 +91,11 @@ 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) > - goto out; > - part = strsep(&pos, " "); > - if (!part) > - goto out; > - file = strsep(&pos, " "); > - if (!file) > + status = efi_get_fp_from_var(initrd, &info); > + if (status != EFI_SUCCESS) > goto out; > > - file_sz = get_file_size(dev, part, file, &status); > + file_sz = get_file_size(&info, &status); > if (!file_sz) > goto out; > > @@ -153,23 +103,25 @@ efi_load_file2_initrd(struct efi_load_file_protocol *this, > status = EFI_BUFFER_TOO_SMALL; > *buffer_size = file_sz; > } else { > - ret = fs_set_blk_dev(dev, part, FS_TYPE_ANY); > + ret = fs_set_blk_dev(info.dev, info.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) > + ret = fs_read(info.filename, map_to_sysmem(buffer), 0, > + *buffer_size, &read_sz); > + if (ret || read_sz != file_sz) { > + status = EFI_DEVICE_ERROR; > goto out; > + } > *buffer_size = read_sz; > > status = EFI_SUCCESS; > } > > out: > - free(filespec); > return EFI_EXIT(status); > } > > @@ -183,13 +135,22 @@ out: > * > * Return: status code > */ > -efi_status_t efi_initrd_register(void) > +efi_status_t efi_initrd_register(efi_handle_t *efi_initrd_handle) > { > - efi_handle_t efi_initrd_handle = NULL; > efi_status_t ret; > + struct load_file_info info; > + char initrd[] = "Initrd"; > + > + ret = efi_get_fp_from_var(initrd, &info); > + /* > + * Don't fail here. If we don't install the protocol the efi-stub will > + * try to load and initrd parsing the kernel cmdline > + */ > + if (ret != EFI_SUCCESS) > + return EFI_SUCCESS; > > ret = EFI_CALL(efi_install_multiple_protocol_interfaces > - (&efi_initrd_handle, > + (efi_initrd_handle, > /* initramfs */ > &efi_guid_device_path, &dp, > /* LOAD_FILE2 */ > -- > 2.30.0 >
Akashi san, On Tue, Jan 05, 2021 at 11:42:22AM +0900, AKASHI Takahiro wrote: > On Wed, Dec 30, 2020 at 05:07:18PM +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 the EFI application is launched through the bootmgr, we'll try to > > match the BootCurrent value to an Initrd#### EFI variable. > > e.g Boot0000 -> Initrd0000, Boot0010 -> Initrd0010 etc. > > Do you think this semantics be kept U-boot specific? > It seems that it can work for any other EFI firmware (implementation). > The GUID is very linux specific and it's meant to load an initrd, but I guess any OS that uses a 'helper' file to load the final OS can be supported. > > The Initrd#### EFI variable is expected to include the full file path, > > i.e 'mmc 0:1 initrd'. If the file is found, we'll install the > > If so, the content of "Initrd####" should contain a generic EFI > representation of file path. > Please note that "Boot####" internally contains a flattened string of > "EFI device path" to the image while efidebug command accepts a style of > "mmc 0:1 image" as arguments to "efidebug boot add" sub-command. Thanks for the pointers. I've already changed the patchset and using exactly what you described. This has another advantage, all the helper files for the string parsing previous patches introduce, went away as well. Cheers /Ilias > > -Takahiro Akashi > > > appropriate protocol so the kernel's efi-stub can use it and load our > > initrd. If the file is not found the kernel will still try to load an > > initrd parsing the kernel cmdline, since the protocol won't be installed. > > > > 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. So we can base the whole boot flow on the Boot#### > > and Initrd#### paired values. > > > > Suggested-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > --- > > cmd/bootefi.c | 13 +++++ > > include/efi_loader.h | 2 +- > > lib/efi_loader/Kconfig | 13 ++--- > > lib/efi_loader/efi_load_initrd.c | 93 ++++++++++---------------------- > > 4 files changed, 46 insertions(+), 75 deletions(-) > > > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c > > index fdf909f8da2c..0234b0df2258 100644 > > --- a/cmd/bootefi.c > > +++ b/cmd/bootefi.c > > @@ -377,6 +377,19 @@ static int do_efibootmgr(void) > > return CMD_RET_FAILURE; > > } > > > > + /* efi_exit() will delete all protocols and the handle itself on exit */ > > + if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD)) { > > + ret = efi_initrd_register(&handle); > > + /* Just warn the user. If the installation fails, the kernel > > + * either load an initrd specificied in the cmdline or fail > > + * to boot. > > + * If we decide to fail the command here we need to uninstall > > + * the protocols efi_bootmgr_load() installed > > + */ > > + if (ret != EFI_SUCCESS) > > + log_warning("EFI boot manager: Failed to install Loadfile2 for initrd\n"); > > + } > > + > > ret = do_bootefi_exec(handle, load_options); > > > > if (ret != EFI_SUCCESS) > > diff --git a/include/efi_loader.h b/include/efi_loader.h > > index af30dbafab77..000cc942e31c 100644 > > --- a/include/efi_loader.h > > +++ b/include/efi_loader.h > > @@ -422,7 +422,7 @@ efi_status_t efi_gop_register(void); > > 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); > > +efi_status_t efi_initrd_register(efi_handle_t *initrd_handle); > > /* 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 dd8b93bd3c5a..96b5934a221d 100644 > > --- a/lib/efi_loader/Kconfig > > +++ b/lib/efi_loader/Kconfig > > @@ -212,14 +212,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_load_initrd.c b/lib/efi_loader/efi_load_initrd.c > > index 4ec55d70979d..6289957c97bc 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> > > @@ -43,40 +45,7 @@ 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; > > -} > > - > > -/** > > - * 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. > > @@ -96,21 +65,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 load_file_info info; > > + char initrd[] = "Initrd"; > > > > 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; > > @@ -128,24 +91,11 @@ 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) > > - goto out; > > - part = strsep(&pos, " "); > > - if (!part) > > - goto out; > > - file = strsep(&pos, " "); > > - if (!file) > > + status = efi_get_fp_from_var(initrd, &info); > > + if (status != EFI_SUCCESS) > > goto out; > > > > - file_sz = get_file_size(dev, part, file, &status); > > + file_sz = get_file_size(&info, &status); > > if (!file_sz) > > goto out; > > > > @@ -153,23 +103,25 @@ efi_load_file2_initrd(struct efi_load_file_protocol *this, > > status = EFI_BUFFER_TOO_SMALL; > > *buffer_size = file_sz; > > } else { > > - ret = fs_set_blk_dev(dev, part, FS_TYPE_ANY); > > + ret = fs_set_blk_dev(info.dev, info.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) > > + ret = fs_read(info.filename, map_to_sysmem(buffer), 0, > > + *buffer_size, &read_sz); > > + if (ret || read_sz != file_sz) { > > + status = EFI_DEVICE_ERROR; > > goto out; > > + } > > *buffer_size = read_sz; > > > > status = EFI_SUCCESS; > > } > > > > out: > > - free(filespec); > > return EFI_EXIT(status); > > } > > > > @@ -183,13 +135,22 @@ out: > > * > > * Return: status code > > */ > > -efi_status_t efi_initrd_register(void) > > +efi_status_t efi_initrd_register(efi_handle_t *efi_initrd_handle) > > { > > - efi_handle_t efi_initrd_handle = NULL; > > efi_status_t ret; > > + struct load_file_info info; > > + char initrd[] = "Initrd"; > > + > > + ret = efi_get_fp_from_var(initrd, &info); > > + /* > > + * Don't fail here. If we don't install the protocol the efi-stub will > > + * try to load and initrd parsing the kernel cmdline > > + */ > > + if (ret != EFI_SUCCESS) > > + return EFI_SUCCESS; > > > > ret = EFI_CALL(efi_install_multiple_protocol_interfaces > > - (&efi_initrd_handle, > > + (efi_initrd_handle, > > /* initramfs */ > > &efi_guid_device_path, &dp, > > /* LOAD_FILE2 */ > > -- > > 2.30.0 > >
On Tue, 5 Jan 2021 at 10:50, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Akashi san, > > On Tue, Jan 05, 2021 at 11:42:22AM +0900, AKASHI Takahiro wrote: > > On Wed, Dec 30, 2020 at 05:07:18PM +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 the EFI application is launched through the bootmgr, we'll try to > > > match the BootCurrent value to an Initrd#### EFI variable. > > > e.g Boot0000 -> Initrd0000, Boot0010 -> Initrd0010 etc. > > > > Do you think this semantics be kept U-boot specific? > > It seems that it can work for any other EFI firmware (implementation). > > > > The GUID is very linux specific and it's meant to load an initrd, but I guess > any OS that uses a 'helper' file to load the final OS can be supported. > > > > The Initrd#### EFI variable is expected to include the full file path, > > > i.e 'mmc 0:1 initrd'. If the file is found, we'll install the > > > > If so, the content of "Initrd####" should contain a generic EFI > > representation of file path. > > Please note that "Boot####" internally contains a flattened string of > > "EFI device path" to the image while efidebug command accepts a style of > > "mmc 0:1 image" as arguments to "efidebug boot add" sub-command. > > Thanks for the pointers. I've already changed the patchset and using exactly > what you described. This has another advantage, all the helper files for the > string parsing previous patches introduce, went away as well. > While I was trying to code this I came across a few things that we need to resolve before posting my v3. This feature is supposed to be very specific in linux so each OS loader can decide on how to expose and load the corresponding initrd. Moving the contents to a device path adds more problems that it solves at the moment. First of all we'll be forced to use efi_load_image_from_file(), which uses EFI_SIMPLE_FILESYSTEM_PROTOCOL and EFI_FILE_PROTOCOL to eventually load the file. This has 2 problems. We'll have to place the initrd on the same filesystem the image we load resides (as opposed to my patch where the initrd can be anywhere). Apart from that calling efi_load_image_from_file() will try to free the memory on errors, but that memory is allocated and managed by the efi-stub. I'd prefer keeping the implementation as is, unbless someone has a better idea. Thoughts? Cheers /Ilias [...]
On 06.01.21 11:43, Ilias Apalodimas wrote: > On Tue, 5 Jan 2021 at 10:50, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: >> >> Akashi san, >> >> On Tue, Jan 05, 2021 at 11:42:22AM +0900, AKASHI Takahiro wrote: >>> On Wed, Dec 30, 2020 at 05:07:18PM +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 the EFI application is launched through the bootmgr, we'll try to >>>> match the BootCurrent value to an Initrd#### EFI variable. >>>> e.g Boot0000 -> Initrd0000, Boot0010 -> Initrd0010 etc. >>> >>> Do you think this semantics be kept U-boot specific? >>> It seems that it can work for any other EFI firmware (implementation). >>> >> >> The GUID is very linux specific and it's meant to load an initrd, but I guess >> any OS that uses a 'helper' file to load the final OS can be supported. >> >>>> The Initrd#### EFI variable is expected to include the full file path, >>>> i.e 'mmc 0:1 initrd'. If the file is found, we'll install the >>> >>> If so, the content of "Initrd####" should contain a generic EFI >>> representation of file path. >>> Please note that "Boot####" internally contains a flattened string of >>> "EFI device path" to the image while efidebug command accepts a style of >>> "mmc 0:1 image" as arguments to "efidebug boot add" sub-command. >> >> Thanks for the pointers. I've already changed the patchset and using exactly >> what you described. This has another advantage, all the helper files for the >> string parsing previous patches introduce, went away as well. >> > > While I was trying to code this I came across a few things that we > need to resolve before posting my v3. > This feature is supposed to be very specific in linux so each OS > loader can decide on how to expose and > load the corresponding initrd. > > Moving the contents to a device path adds more problems that it solves > at the moment. > First of all we'll be forced to use efi_load_image_from_file(), which You are not obliged to call efi_load_image_from_file(). You could implement your own function. Or you could add a check *buffer!=NULL to detect a pre-allocated buffer and neither allocate nor free the buffer in this case. > uses EFI_SIMPLE_FILESYSTEM_PROTOCOL > and EFI_FILE_PROTOCOL to eventually load the file. This has 2 > problems. We'll have to place the initrd on the same > filesystem the image we load resides (as opposed to my patch where the > initrd can be anywhere). Given Boot#### contains dp1<sep>dp2<sep>dp3<end> when calling efi_load_image_from_file() three times, once for dp1, dp2, dp3, each of these device paths can point to a different block device. Best regards Heinrich > Apart from that calling efi_load_image_from_file() will try to free > the memory on errors, but that memory is allocated > and managed by the efi-stub. > > I'd prefer keeping the implementation as is, unless someone has a better idea. > > Thoughts? > > Cheers > /Ilias > > > [...] >
> >>> [...] > >>> If so, the content of "Initrd####" should contain a generic EFI > >>> representation of file path. > >>> Please note that "Boot####" internally contains a flattened string of > >>> "EFI device path" to the image while efidebug command accepts a style of > >>> "mmc 0:1 image" as arguments to "efidebug boot add" sub-command. > >> > >> Thanks for the pointers. I've already changed the patchset and using exactly > >> what you described. This has another advantage, all the helper files for the > >> string parsing previous patches introduce, went away as well. > >> > > > > While I was trying to code this I came across a few things that we > > need to resolve before posting my v3. > > This feature is supposed to be very specific in linux so each OS > > loader can decide on how to expose and > > load the corresponding initrd. > > > > Moving the contents to a device path adds more problems that it solves > > at the moment. > > First of all we'll be forced to use efi_load_image_from_file(), which > > You are not obliged to call efi_load_image_from_file(). You could > implement your own function. > > Or you could add a check *buffer!=NULL to detect a pre-allocated buffer > and neither allocate nor free the buffer in this case. Right obliged was a poor choice of words. I was just trying to point out issues with using the current U-Boot functions. > > > uses EFI_SIMPLE_FILESYSTEM_PROTOCOL > > and EFI_FILE_PROTOCOL to eventually load the file. This has 2 > > problems. We'll have to place the initrd on the same > > filesystem the image we load resides (as opposed to my patch where the > > initrd can be anywhere). > > Given Boot#### contains > > dp1<sep>dp2<sep>dp3<end> Let's clarify this a bit first so other people can join the discussion. What I proposed yesterday to Heinrich, since we can use a device path directly, is store it in the EFI_LOAD_OPTION we construct when we add the Boot#### variables. According to the EFI spec FilePathList is an array of device paths. The first element of the array is a device path that describes the device and location of the Image for this load option, but the rest are OS specific, so we might as well use them instead of using a custom EFI variable. Ok I'll send a v3 implementing this idea + a new function that can consume the device path and load a file without using EFI_SIMPLE_FILESYSTEM_PROTOCOL. That would probablty solve all our problems. Cheers /Ilias > > when calling efi_load_image_from_file() three times, once for dp1, dp2, > dp3, each of these device paths can point to a different block device. > > Best regards > > Heinrich > > > Apart from that calling efi_load_image_from_file() will try to free > > the memory on errors, but that memory is allocated > > and managed by the efi-stub. > > > > I'd prefer keeping the implementation as is, unless someone has a better idea. > > > > Thoughts? > > > > Cheers > > /Ilias > > > > > > [...] > > >
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index fdf909f8da2c..0234b0df2258 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -377,6 +377,19 @@ static int do_efibootmgr(void) return CMD_RET_FAILURE; } + /* efi_exit() will delete all protocols and the handle itself on exit */ + if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD)) { + ret = efi_initrd_register(&handle); + /* Just warn the user. If the installation fails, the kernel + * either load an initrd specificied in the cmdline or fail + * to boot. + * If we decide to fail the command here we need to uninstall + * the protocols efi_bootmgr_load() installed + */ + if (ret != EFI_SUCCESS) + log_warning("EFI boot manager: Failed to install Loadfile2 for initrd\n"); + } + ret = do_bootefi_exec(handle, load_options); if (ret != EFI_SUCCESS) diff --git a/include/efi_loader.h b/include/efi_loader.h index af30dbafab77..000cc942e31c 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -422,7 +422,7 @@ efi_status_t efi_gop_register(void); 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); +efi_status_t efi_initrd_register(efi_handle_t *initrd_handle); /* 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 dd8b93bd3c5a..96b5934a221d 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -212,14 +212,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_load_initrd.c b/lib/efi_loader/efi_load_initrd.c index 4ec55d70979d..6289957c97bc 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> @@ -43,40 +45,7 @@ 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; -} - -/** - * 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. @@ -96,21 +65,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 load_file_info info; + char initrd[] = "Initrd"; 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; @@ -128,24 +91,11 @@ 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) - goto out; - part = strsep(&pos, " "); - if (!part) - goto out; - file = strsep(&pos, " "); - if (!file) + status = efi_get_fp_from_var(initrd, &info); + if (status != EFI_SUCCESS) goto out; - file_sz = get_file_size(dev, part, file, &status); + file_sz = get_file_size(&info, &status); if (!file_sz) goto out; @@ -153,23 +103,25 @@ efi_load_file2_initrd(struct efi_load_file_protocol *this, status = EFI_BUFFER_TOO_SMALL; *buffer_size = file_sz; } else { - ret = fs_set_blk_dev(dev, part, FS_TYPE_ANY); + ret = fs_set_blk_dev(info.dev, info.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) + ret = fs_read(info.filename, map_to_sysmem(buffer), 0, + *buffer_size, &read_sz); + if (ret || read_sz != file_sz) { + status = EFI_DEVICE_ERROR; goto out; + } *buffer_size = read_sz; status = EFI_SUCCESS; } out: - free(filespec); return EFI_EXIT(status); } @@ -183,13 +135,22 @@ out: * * Return: status code */ -efi_status_t efi_initrd_register(void) +efi_status_t efi_initrd_register(efi_handle_t *efi_initrd_handle) { - efi_handle_t efi_initrd_handle = NULL; efi_status_t ret; + struct load_file_info info; + char initrd[] = "Initrd"; + + ret = efi_get_fp_from_var(initrd, &info); + /* + * Don't fail here. If we don't install the protocol the efi-stub will + * try to load and initrd parsing the kernel cmdline + */ + if (ret != EFI_SUCCESS) + return EFI_SUCCESS; ret = EFI_CALL(efi_install_multiple_protocol_interfaces - (&efi_initrd_handle, + (efi_initrd_handle, /* initramfs */ &efi_guid_device_path, &dp, /* LOAD_FILE2 */
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 the EFI application is launched through the bootmgr, we'll try to match the BootCurrent value to an Initrd#### EFI variable. e.g Boot0000 -> Initrd0000, Boot0010 -> Initrd0010 etc. The Initrd#### EFI variable is expected to include the full file path, i.e 'mmc 0:1 initrd'. If the file is found, we'll install the appropriate protocol so the kernel's efi-stub can use it and load our initrd. If the file is not found the kernel will still try to load an initrd parsing the kernel cmdline, since the protocol won't be installed. 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. So we can base the whole boot flow on the Boot#### and Initrd#### paired values. Suggested-by: Heinrich Schuchardt <xypron.glpk@gmx.de> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- cmd/bootefi.c | 13 +++++ include/efi_loader.h | 2 +- lib/efi_loader/Kconfig | 13 ++--- lib/efi_loader/efi_load_initrd.c | 93 ++++++++++---------------------- 4 files changed, 46 insertions(+), 75 deletions(-) -- 2.30.0