Message ID | 20201228122440.316403-4-ilias.apalodimas@linaro.org |
---|---|
State | New |
Headers | show |
Series | Change logic of EFI LoadFile2 protocol for initrd loading | expand |
Hi Heinrich, [...] > bootorder[i]); > ret = try_load_entry(bootorder[i], handle, load_options); > - if (ret == EFI_SUCCESS) > + if (ret == EFI_SUCCESS) { > + if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD)) > + ret = efi_initrd_register(); I just realized registering the protocol here won't work for BootNext since the protocol won't regsiter. I'll just move the protocol registration in try_load_entry() when I send a v2 Cheers /Ilias
On 12/28/20 1:24 PM, Ilias Apalodimas wrote: > Up to now we register EFI_LOAD_FILE2_PROTOCOL to load an initrd > unconditionally. Although we correctly return various EFI return 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 registered. > > This creates a problem for EFI installers, since they won't be able to > load their own initrd and continue the installation. > > So let's introduce a different logic that will decopouple the initrd > path from the config option we currently support. > When the EFI application is launched through the bootmgr, we'll try to > match the BootCurrent value to an Initrd#### EFI variable. > i.e 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 register the > appropriate protocol so the kernel's efi-stub 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 registered. > > 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> > --- > lib/efi_loader/Kconfig | 12 ++--- > lib/efi_loader/Makefile | 2 +- > lib/efi_loader/efi_bootmgr.c | 6 ++- > lib/efi_loader/efi_load_initrd.c | 86 +++++++++----------------------- > 4 files changed, 33 insertions(+), 73 deletions(-) > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig > index dd8b93bd3c5a..eca24e82b8b1 100644 > --- a/lib/efi_loader/Kconfig > +++ b/lib/efi_loader/Kconfig > @@ -212,14 +212,10 @@ 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 > + registered if bootmgr is used and the file is found on the defined In UEFI speak protocols are not registered but installed. further comment below > + path. A boot entry of Boot0001 will try to match Initrd0001 and use > + it. Initrd format 'mmc 0:1 <filename>' > > config EFI_SECURE_BOOT > bool "Enable EFI secure boot support" > diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile > index cd4b252a417c..793e5b7f8730 100644 > --- a/lib/efi_loader/Makefile > +++ b/lib/efi_loader/Makefile > @@ -54,7 +54,7 @@ obj-$(CONFIG_GENERATE_ACPI_TABLE) += efi_acpi.o > obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += efi_smbios.o > obj-$(CONFIG_EFI_RNG_PROTOCOL) += efi_rng.o > obj-$(CONFIG_EFI_TCG2_PROTOCOL) += efi_tcg2.o > -obj-$(CONFIG_EFI_LOAD_FILE2_INITRD) += efi_load_initrd.o > +obj-$(CONFIG_EFI_LOAD_FILE2_INITRD) += efi_helper.o efi_load_initrd.o > obj-y += efi_signature.o > > EFI_VAR_SEED_FILE := $(subst $\",,$(CONFIG_EFI_VAR_SEED_FILE)) > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c > index 61dc72a23da8..ceca5c5b1bf3 100644 > --- a/lib/efi_loader/efi_bootmgr.c > +++ b/lib/efi_loader/efi_bootmgr.c > @@ -12,6 +12,7 @@ > #include <log.h> > #include <malloc.h> > #include <efi_loader.h> > +#include <efi_load_initrd.h> > #include <efi_variable.h> > #include <asm/unaligned.h> > > @@ -348,8 +349,11 @@ efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options) > log_debug("%s trying to load Boot%04X\n", __func__, > bootorder[i]); > ret = try_load_entry(bootorder[i], handle, load_options); > - if (ret == EFI_SUCCESS) > + if (ret == EFI_SUCCESS) { > + if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD)) > + ret = efi_initrd_register(); I think this is not enough. * We should uninstall the protocol once 'bootefi bootmgr' returns to U-Boot. * The EFI_LOAD_FILE2_PROTOCOL should be installed on the handle of the loaded image for this purpose. Best regards Heinrich > break; > + } > } > > free(bootorder); > diff --git a/lib/efi_loader/efi_load_initrd.c b/lib/efi_loader/efi_load_initrd.c > index d517d686c330..984fea1bd679 100644 > --- a/lib/efi_loader/efi_load_initrd.c > +++ b/lib/efi_loader/efi_load_initrd.c > @@ -10,7 +10,9 @@ > #include <dm.h> > #include <fs.h> > #include <efi_loader.h> > +#include <efi_variable.h> > #include <efi_load_initrd.h> > +#include <efi_helper.h> > > static const efi_guid_t efi_guid_load_file2_protocol = > EFI_LOAD_FILE2_PROTOCOL_GUID; > @@ -45,40 +47,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. > @@ -98,21 +67,14 @@ 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; > > 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; > @@ -130,24 +92,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(L"Initrd####", 6, &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; > > @@ -155,23 +104,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); > } > > @@ -189,6 +140,15 @@ efi_status_t efi_initrd_register(void) > { > efi_handle_t efi_initrd_handle = NULL; > efi_status_t ret; > + struct load_file_info info; > + > + ret = efi_get_fp_from_var(L"Initrd####", 6, &info); > + /* > + * Don't fail here. If we don't register 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, >
Hi Heinrich, On Mon, Dec 28, 2020 at 03:55:49PM +0100, Heinrich Schuchardt wrote: [...] > > ret = try_load_entry(bootorder[i], handle, load_options); > > - if (ret == EFI_SUCCESS) > > + if (ret == EFI_SUCCESS) { > > + if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD)) > > + ret = efi_initrd_register(); > > I think this is not enough. > > * We should uninstall the protocol once 'bootefi bootmgr' returns > to U-Boot. > * The EFI_LOAD_FILE2_PROTOCOL should be installed on the handle of > the loaded image for this purpose. > Yep you are right. I completely missed that case, I'll fix it on v2 Regards /Ilias
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index dd8b93bd3c5a..eca24e82b8b1 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -212,14 +212,10 @@ 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 + registered 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 format 'mmc 0:1 <filename>' config EFI_SECURE_BOOT bool "Enable EFI secure boot support" diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile index cd4b252a417c..793e5b7f8730 100644 --- a/lib/efi_loader/Makefile +++ b/lib/efi_loader/Makefile @@ -54,7 +54,7 @@ obj-$(CONFIG_GENERATE_ACPI_TABLE) += efi_acpi.o obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += efi_smbios.o obj-$(CONFIG_EFI_RNG_PROTOCOL) += efi_rng.o obj-$(CONFIG_EFI_TCG2_PROTOCOL) += efi_tcg2.o -obj-$(CONFIG_EFI_LOAD_FILE2_INITRD) += efi_load_initrd.o +obj-$(CONFIG_EFI_LOAD_FILE2_INITRD) += efi_helper.o efi_load_initrd.o obj-y += efi_signature.o EFI_VAR_SEED_FILE := $(subst $\",,$(CONFIG_EFI_VAR_SEED_FILE)) diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index 61dc72a23da8..ceca5c5b1bf3 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -12,6 +12,7 @@ #include <log.h> #include <malloc.h> #include <efi_loader.h> +#include <efi_load_initrd.h> #include <efi_variable.h> #include <asm/unaligned.h> @@ -348,8 +349,11 @@ efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options) log_debug("%s trying to load Boot%04X\n", __func__, bootorder[i]); ret = try_load_entry(bootorder[i], handle, load_options); - if (ret == EFI_SUCCESS) + if (ret == EFI_SUCCESS) { + if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD)) + ret = efi_initrd_register(); break; + } } free(bootorder); diff --git a/lib/efi_loader/efi_load_initrd.c b/lib/efi_loader/efi_load_initrd.c index d517d686c330..984fea1bd679 100644 --- a/lib/efi_loader/efi_load_initrd.c +++ b/lib/efi_loader/efi_load_initrd.c @@ -10,7 +10,9 @@ #include <dm.h> #include <fs.h> #include <efi_loader.h> +#include <efi_variable.h> #include <efi_load_initrd.h> +#include <efi_helper.h> static const efi_guid_t efi_guid_load_file2_protocol = EFI_LOAD_FILE2_PROTOCOL_GUID; @@ -45,40 +47,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. @@ -98,21 +67,14 @@ 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; 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; @@ -130,24 +92,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(L"Initrd####", 6, &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; @@ -155,23 +104,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); } @@ -189,6 +140,15 @@ efi_status_t efi_initrd_register(void) { efi_handle_t efi_initrd_handle = NULL; efi_status_t ret; + struct load_file_info info; + + ret = efi_get_fp_from_var(L"Initrd####", 6, &info); + /* + * Don't fail here. If we don't register 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,
Up to now we register EFI_LOAD_FILE2_PROTOCOL to load an initrd unconditionally. Although we correctly return various EFI return 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 registered. This creates a problem for EFI installers, since they won't be able to load their own initrd and continue the installation. So let's introduce a different logic that will decopouple the initrd path from the config option we currently support. When the EFI application is launched through the bootmgr, we'll try to match the BootCurrent value to an Initrd#### EFI variable. i.e 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 register the appropriate protocol so the kernel's efi-stub 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 registered. 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> --- lib/efi_loader/Kconfig | 12 ++--- lib/efi_loader/Makefile | 2 +- lib/efi_loader/efi_bootmgr.c | 6 ++- lib/efi_loader/efi_load_initrd.c | 86 +++++++++----------------------- 4 files changed, 33 insertions(+), 73 deletions(-) -- 2.30.0.rc2