| Message ID | 20210113111149.64567-4-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: > The UEFI spec allow a packed array of UEFI device paths in the > FilePathList[] of an EFI_LOAD_OPTION. The first file path must > describe the laoded image but the rest are OS specific. > Previous patches parse the device path and try to use the second > member of the array as an initrd. So let's modify efidebug slightly > and install the second file described in the command line as the > initrd device path. Please, describe the syntax of the efidebug command in the commit message. > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > cmd/efidebug.c | 89 +++++++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 81 insertions(+), 8 deletions(-) > > diff --git a/cmd/efidebug.c b/cmd/efidebug.c > index 5fb7b1e3c6a9..8d62981aca92 100644 > --- a/cmd/efidebug.c > +++ b/cmd/efidebug.c > @@ -8,6 +8,7 @@ > #include <charset.h> > #include <common.h> > #include <command.h> > +#include <efi_helper.h> > #include <efi_loader.h> > #include <efi_rng.h> > #include <exports.h> > @@ -17,6 +18,7 @@ > #include <mapmem.h> > #include <search.h> > #include <linux/ctype.h> > +#include <linux/err.h> > > #define BS systab.boottime > #define RT systab.runtime > @@ -782,6 +784,42 @@ static int do_efi_show_tables(struct cmd_tbl *cmdtp, int flag, > return CMD_RET_SUCCESS; > } > > +/** > + * add_initrd_instance() - Append a device path to load_options pointing to an > + * inirtd > + * > + * @argc: Number of arguments > + * @argv: Argument array > + * @file_path Existing device path, the new instance will be appended > + * Return: Pointer to the device path or ERR_PTR > + * > + */ > +static struct efi_device_path *add_initrd_instance(int argc, char *const argv[], > + struct efi_device_path *file_path) > +{ > + struct efi_device_path *tmp_dp = NULL, *tmp_fp = NULL; > + struct efi_device_path *final_fp = NULL; > + efi_status_t ret; > + > + if (argc < 8) > + return ERR_PTR(-EINVAL); > + > + ret = efi_dp_from_name(argv[6], argv[7], argv[8], &tmp_dp, > + &tmp_fp); > + if (ret != EFI_SUCCESS) { > + printf("Cannot create device path for \"%s %s\"\n", > + argv[6], argv[7]); > + goto out; > + } > + > + final_fp = efi_dp_append_instance(file_path, tmp_fp); > + > +out: > + efi_free_pool(tmp_dp); > + efi_free_pool(tmp_fp); > + return final_fp ? final_fp : ERR_PTR(-EINVAL); > +} > + > /** > * do_efi_boot_add() - set UEFI load option > * > @@ -794,7 +832,11 @@ static int do_efi_show_tables(struct cmd_tbl *cmdtp, int flag, > * > * Implement efidebug "boot add" sub-command. Create or change UEFI load option. > * > - * efidebug boot add <id> <label> <interface> <devnum>[:<part>] <file> <options> > + * Without initrd: > + * efidebug boot add <id> <label> <interface> <devnum>[:<part>] <file> <options> > + * > + * With initrd: > + * efidebug boot add <id> <label> <interface> <devnum>[:<part>] <file> <interface2> <devnum2>[:<part>] <initrd> <options> > */ > static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, > int argc, char *const argv[]) > @@ -807,13 +849,14 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, > size_t label_len, label_len16; > u16 *label; > struct efi_device_path *device_path = NULL, *file_path = NULL; > + struct efi_device_path *final_fp = NULL; > struct efi_load_option lo; > void *data = NULL; > efi_uintn_t size; > efi_status_t ret; > int r = CMD_RET_SUCCESS; > > - if (argc < 6 || argc > 7) > + if (argc < 6 || argc > 9) > return CMD_RET_USAGE; > > id = (int)simple_strtoul(argv[1], &endp, 16); > @@ -829,6 +872,12 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, > /* attributes */ > lo.attributes = LOAD_OPTION_ACTIVE; /* always ACTIVE */ > > + /* optional data */ > + if (argc == 6) > + lo.optional_data = NULL; > + else > + lo.optional_data = (const u8 *)argv[6]; > + > /* label */ > label_len = strlen(argv[2]); > label_len16 = utf8_utf16_strnlen(argv[2], label_len); > @@ -847,15 +896,30 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, > r = CMD_RET_FAILURE; > goto out; > } > + > + /* add initrd instance in device path */ > + if (argc >= 9) { > + final_fp = add_initrd_instance(argc, argv, file_path); > + if (IS_ERR(final_fp)) { > + r = CMD_RET_FAILURE; > + goto out; > + } > + > + /* add_initrd_instance allocates a new device path */ > + efi_free_pool(file_path); > + file_path = final_fp; > + > + /* update optional data */ > + if (argc == 9) > + lo.optional_data = NULL; > + else > + lo.optional_data = (const u8 *)argv[9]; > + } > + > lo.file_path = file_path; > lo.file_path_length = efi_dp_size(file_path) > + sizeof(struct efi_device_path); /* for END */ > > - /* optional data */ > - if (argc == 6) > - lo.optional_data = NULL; > - else > - lo.optional_data = (const u8 *)argv[6]; > > size = efi_serialize_load_option(&lo, (u8 **)&data); > if (!size) { > @@ -939,11 +1003,13 @@ static int do_efi_boot_rm(struct cmd_tbl *cmdtp, int flag, > */ > static void show_efi_boot_opt_data(u16 *varname16, void *data, size_t *size) > { > + struct efi_device_path *initrd_path = NULL; > struct efi_load_option lo; > char *label, *p; > size_t label_len16, label_len; > u16 *dp_str; > efi_status_t ret; > + efi_uintn_t initrd_dp_size; > > ret = efi_deserialize_load_option(&lo, data, size); > if (ret != EFI_SUCCESS) { > @@ -973,6 +1039,13 @@ static void show_efi_boot_opt_data(u16 *varname16, void *data, size_t *size) > dp_str = efi_dp_str(lo.file_path); > printf(" file_path: %ls\n", dp_str); > efi_free_pool(dp_str); > + initrd_path = efi_dp_instance_by_idx(lo.file_path, &initrd_dp_size, INITRD_DP); > + if (initrd_path) { > + dp_str = efi_dp_str(initrd_path); > + printf(" initrd_path: %ls\n", dp_str); > + efi_free_pool(dp_str); > + efi_free_pool(initrd_path); > + } > > printf(" data:\n"); > print_hex_dump(" ", DUMP_PREFIX_OFFSET, 16, 1, > @@ -1583,7 +1656,7 @@ static char efidebug_help_text[] = Where is the change to efidebug_help_text[]? Best regards Heinrich > #endif > > U_BOOT_CMD( > - efidebug, 10, 0, do_efidebug, > + efidebug, CONFIG_SYS_MAXARGS, 0, do_efidebug, > "Configure UEFI environment", > efidebug_help_text > ); >
On Wed, Jan 13, 2021 at 02:13:44PM +0100, Heinrich Schuchardt wrote: > On 13.01.21 12:11, Ilias Apalodimas wrote: > > The UEFI spec allow a packed array of UEFI device paths in the > > FilePathList[] of an EFI_LOAD_OPTION. The first file path must > > describe the laoded image but the rest are OS specific. > > Previous patches parse the device path and try to use the second > > member of the array as an initrd. So let's modify efidebug slightly > > and install the second file described in the command line as the > > initrd device path. > > Please, describe the syntax of the efidebug command in the commit message. sure [...] > > + } > > > > printf(" data:\n"); > > print_hex_dump(" ", DUMP_PREFIX_OFFSET, 16, 1, > > @@ -1583,7 +1656,7 @@ static char efidebug_help_text[] = > > Where is the change to efidebug_help_text[]? Thanks, I'll add that on the next version Cheers /Ilias > > Best regards > > Heinrich > > > #endif > > > > U_BOOT_CMD( > > - efidebug, 10, 0, do_efidebug, > > + efidebug, CONFIG_SYS_MAXARGS, 0, do_efidebug, > > "Configure UEFI environment", > > efidebug_help_text > > ); > > >
Ilias, On Wed, Jan 13, 2021 at 01:11:49PM +0200, Ilias Apalodimas wrote: > The UEFI spec allow a packed array of UEFI device paths in the > FilePathList[] of an EFI_LOAD_OPTION. The first file path must > describe the laoded image but the rest are OS specific. > Previous patches parse the device path and try to use the second > member of the array as an initrd. So let's modify efidebug slightly > and install the second file described in the command line as the > initrd device path. I have a concern about your proposed command line syntax. It takes a lot of parameters as a whole which makes it hard to understand it at a glance, easily overflowing the width of terminal window. It will even get worse if we want to take dtb as a third device path, and what if we want to specify dtb, but not initrd? Moreover, if we want to add support for non-linux executabes which utilize extra device paths (neither initrd nor dtb) in a boot load option, the syntax will be problematic. > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > cmd/efidebug.c | 89 +++++++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 81 insertions(+), 8 deletions(-) > > diff --git a/cmd/efidebug.c b/cmd/efidebug.c > index 5fb7b1e3c6a9..8d62981aca92 100644 > --- a/cmd/efidebug.c > +++ b/cmd/efidebug.c > @@ -8,6 +8,7 @@ > #include <charset.h> > #include <common.h> > #include <command.h> > +#include <efi_helper.h> > #include <efi_loader.h> > #include <efi_rng.h> > #include <exports.h> > @@ -17,6 +18,7 @@ > #include <mapmem.h> > #include <search.h> > #include <linux/ctype.h> > +#include <linux/err.h> > > #define BS systab.boottime > #define RT systab.runtime > @@ -782,6 +784,42 @@ static int do_efi_show_tables(struct cmd_tbl *cmdtp, int flag, > return CMD_RET_SUCCESS; > } > > +/** > + * add_initrd_instance() - Append a device path to load_options pointing to an > + * inirtd > + * > + * @argc: Number of arguments > + * @argv: Argument array > + * @file_path Existing device path, the new instance will be appended > + * Return: Pointer to the device path or ERR_PTR > + * > + */ > +static struct efi_device_path *add_initrd_instance(int argc, char *const argv[], > + struct efi_device_path *file_path) > +{ > + struct efi_device_path *tmp_dp = NULL, *tmp_fp = NULL; > + struct efi_device_path *final_fp = NULL; > + efi_status_t ret; > + > + if (argc < 8) > + return ERR_PTR(-EINVAL); > + > + ret = efi_dp_from_name(argv[6], argv[7], argv[8], &tmp_dp, > + &tmp_fp); > + if (ret != EFI_SUCCESS) { > + printf("Cannot create device path for \"%s %s\"\n", > + argv[6], argv[7]); > + goto out; > + } > + > + final_fp = efi_dp_append_instance(file_path, tmp_fp); > + > +out: > + efi_free_pool(tmp_dp); > + efi_free_pool(tmp_fp); > + return final_fp ? final_fp : ERR_PTR(-EINVAL); > +} > + > /** > * do_efi_boot_add() - set UEFI load option > * > @@ -794,7 +832,11 @@ static int do_efi_show_tables(struct cmd_tbl *cmdtp, int flag, > * > * Implement efidebug "boot add" sub-command. Create or change UEFI load option. > * > - * efidebug boot add <id> <label> <interface> <devnum>[:<part>] <file> <options> > + * Without initrd: > + * efidebug boot add <id> <label> <interface> <devnum>[:<part>] <file> <options> > + * > + * With initrd: > + * efidebug boot add <id> <label> <interface> <devnum>[:<part>] <file> <interface2> <devnum2>[:<part>] <initrd> <options> > */ > static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, > int argc, char *const argv[]) > @@ -807,13 +849,14 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, > size_t label_len, label_len16; > u16 *label; > struct efi_device_path *device_path = NULL, *file_path = NULL; > + struct efi_device_path *final_fp = NULL; > struct efi_load_option lo; > void *data = NULL; > efi_uintn_t size; > efi_status_t ret; > int r = CMD_RET_SUCCESS; > > - if (argc < 6 || argc > 7) > + if (argc < 6 || argc > 9) > return CMD_RET_USAGE; > > id = (int)simple_strtoul(argv[1], &endp, 16); > @@ -829,6 +872,12 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, > /* attributes */ > lo.attributes = LOAD_OPTION_ACTIVE; /* always ACTIVE */ > > + /* optional data */ > + if (argc == 6) > + lo.optional_data = NULL; > + else > + lo.optional_data = (const u8 *)argv[6]; > + > /* label */ > label_len = strlen(argv[2]); > label_len16 = utf8_utf16_strnlen(argv[2], label_len); > @@ -847,15 +896,30 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, > r = CMD_RET_FAILURE; > goto out; > } > + > + /* add initrd instance in device path */ > + if (argc >= 9) { > + final_fp = add_initrd_instance(argc, argv, file_path); We'd better pass argv[6], argv[7] and argv[8] explicitly here, which will make the code readable and easily extended to support dtb in the future. then, argc -= 3; argv += 3; > + if (IS_ERR(final_fp)) { > + r = CMD_RET_FAILURE; > + goto out; > + } > + > + /* add_initrd_instance allocates a new device path */ > + efi_free_pool(file_path); > + file_path = final_fp; > + > + /* update optional data */ Then, the code should be moved out of this 'if' clause. > + if (argc == 9) > + lo.optional_data = NULL; > + else > + lo.optional_data = (const u8 *)argv[9]; > + } > + > lo.file_path = file_path; > lo.file_path_length = efi_dp_size(file_path) > + sizeof(struct efi_device_path); /* for END */ > > - /* optional data */ > - if (argc == 6) > - lo.optional_data = NULL; > - else > - lo.optional_data = (const u8 *)argv[6]; The code should remain here for non-initrd case. -Takahiro Akashi > > size = efi_serialize_load_option(&lo, (u8 **)&data); > if (!size) { > @@ -939,11 +1003,13 @@ static int do_efi_boot_rm(struct cmd_tbl *cmdtp, int flag, > */ > static void show_efi_boot_opt_data(u16 *varname16, void *data, size_t *size) > { > + struct efi_device_path *initrd_path = NULL; > struct efi_load_option lo; > char *label, *p; > size_t label_len16, label_len; > u16 *dp_str; > efi_status_t ret; > + efi_uintn_t initrd_dp_size; > > ret = efi_deserialize_load_option(&lo, data, size); > if (ret != EFI_SUCCESS) { > @@ -973,6 +1039,13 @@ static void show_efi_boot_opt_data(u16 *varname16, void *data, size_t *size) > dp_str = efi_dp_str(lo.file_path); > printf(" file_path: %ls\n", dp_str); > efi_free_pool(dp_str); > + initrd_path = efi_dp_instance_by_idx(lo.file_path, &initrd_dp_size, INITRD_DP); > + if (initrd_path) { > + dp_str = efi_dp_str(initrd_path); > + printf(" initrd_path: %ls\n", dp_str); > + efi_free_pool(dp_str); > + efi_free_pool(initrd_path); > + } > > printf(" data:\n"); > print_hex_dump(" ", DUMP_PREFIX_OFFSET, 16, 1, > @@ -1583,7 +1656,7 @@ static char efidebug_help_text[] = > #endif > > U_BOOT_CMD( > - efidebug, 10, 0, do_efidebug, > + efidebug, CONFIG_SYS_MAXARGS, 0, do_efidebug, > "Configure UEFI environment", > efidebug_help_text > ); > -- > 2.30.0.rc2 >
On Thu, Jan 14, 2021 at 02:11:33PM +0900, AKASHI Takahiro wrote: > Ilias, > > On Wed, Jan 13, 2021 at 01:11:49PM +0200, Ilias Apalodimas wrote: > > The UEFI spec allow a packed array of UEFI device paths in the > > FilePathList[] of an EFI_LOAD_OPTION. The first file path must > > describe the laoded image but the rest are OS specific. > > Previous patches parse the device path and try to use the second > > member of the array as an initrd. So let's modify efidebug slightly > > and install the second file described in the command line as the > > initrd device path. > > I have a concern about your proposed command line syntax. > It takes a lot of parameters as a whole which makes it > hard to understand it at a glance, easily overflowing > the width of terminal window. > > It will even get worse if we want to take dtb as a third > device path, and what if we want to specify dtb, but not initrd? > > Moreover, if we want to add support for non-linux executabes which > utilize extra device paths (neither initrd nor dtb) in a boot > load option, the syntax will be problematic. > Maybe we should add explicit commands in efidebug then? Something like: efidebug initrd add 0002 virtio 1 initrd_file efidebug dtb add 0002 virtio 1 dtb That would untangle the do_efi_boot_add() function, make our lives easier on adding things like 'kernel <no initrd> valid dtb' and should be much easier to use. The user will just have to make sure the boot order numbers match when adding files [...] Cheers /Ilias
On 14.01.21 10:55, Ilias Apalodimas wrote: > On Thu, Jan 14, 2021 at 02:11:33PM +0900, AKASHI Takahiro wrote: >> Ilias, >> >> On Wed, Jan 13, 2021 at 01:11:49PM +0200, Ilias Apalodimas wrote: >>> The UEFI spec allow a packed array of UEFI device paths in the >>> FilePathList[] of an EFI_LOAD_OPTION. The first file path must >>> describe the laoded image but the rest are OS specific. >>> Previous patches parse the device path and try to use the second >>> member of the array as an initrd. So let's modify efidebug slightly >>> and install the second file described in the command line as the >>> initrd device path. >> >> I have a concern about your proposed command line syntax. >> It takes a lot of parameters as a whole which makes it >> hard to understand it at a glance, easily overflowing >> the width of terminal window. >> >> It will even get worse if we want to take dtb as a third >> device path, and what if we want to specify dtb, but not initrd? >> >> Moreover, if we want to add support for non-linux executabes which >> utilize extra device paths (neither initrd nor dtb) in a boot >> load option, the syntax will be problematic. >> > > Maybe we should add explicit commands in efidebug then? > Something like: > efidebug initrd add 0002 virtio 1 initrd_file > efidebug dtb add 0002 virtio 1 dtb Why "add"? If no file is provided, we could empty the device path. All other boot related subcommands start with efidebug boot. So how about: efidebug boot add 00B1 'Linux' mmc 0:1 vmlinux-5.99 UUID=1234 efidebug boot initrd 00B1 mmc 0:1 initrd-5.99 efidebug boot dtb mmc 00B1 0:1 /dtbs/5.99/vendor/board.dtb efidebug boot order 00B1 What will happen if you pass the following command next: efidebug boot add 00B1 'Linux' mmc 0:1 vmlinux-5.99.1 UUID=1234 Will initrd and and dtb be kept? What will happen if you start with dtb or with initrd and not with add? A device path with kernel.efi, no initrd, and dtb would have an initrd device path with only an end node. Should we install a LOAD FILE2 protocol in this case to disallow initrd on the command line? The boot options are relevant for all users, while the rest of efidebug is more developers oriented. Should we separate the boot related commands from the rest of efidebug and build with the new command by default? bootopt add 00B1 'Linux' mmc 0:1 vmlinux-5.99 UUID=1234 bootopt initrd 00B1 mmc 0:1 initrd-5.99 bootopt dtb mmc 00B1 0:1 /dtbs/5.99/vendor/board.dtb bootopt order 00B1 Best regards Heinrich > > That would untangle the do_efi_boot_add() function, make our lives easier on > adding things like 'kernel <no initrd> valid dtb' and should be much easier > to use. The user will just have to make sure the boot order numbers match when > adding files > > [...] > > Cheers > /Ilias >
On Thu, Jan 14, 2021 at 01:07:42PM +0100, Heinrich Schuchardt wrote: > On 14.01.21 10:55, Ilias Apalodimas wrote: > > On Thu, Jan 14, 2021 at 02:11:33PM +0900, AKASHI Takahiro wrote: > >> Ilias, > >> > >> On Wed, Jan 13, 2021 at 01:11:49PM +0200, Ilias Apalodimas wrote: > >>> The UEFI spec allow a packed array of UEFI device paths in the > >>> FilePathList[] of an EFI_LOAD_OPTION. The first file path must > >>> describe the laoded image but the rest are OS specific. > >>> Previous patches parse the device path and try to use the second > >>> member of the array as an initrd. So let's modify efidebug slightly > >>> and install the second file described in the command line as the > >>> initrd device path. > >> > >> I have a concern about your proposed command line syntax. > >> It takes a lot of parameters as a whole which makes it > >> hard to understand it at a glance, easily overflowing > >> the width of terminal window. > >> > >> It will even get worse if we want to take dtb as a third > >> device path, and what if we want to specify dtb, but not initrd? > >> > >> Moreover, if we want to add support for non-linux executabes which > >> utilize extra device paths (neither initrd nor dtb) in a boot > >> load option, the syntax will be problematic. > >> > > > > Maybe we should add explicit commands in efidebug then? > > Something like: > > efidebug initrd add 0002 virtio 1 initrd_file > > efidebug dtb add 0002 virtio 1 dtb > > Why "add"? If no file is provided, we could empty the device path. > > All other boot related subcommands start with efidebug boot. So how about: > > efidebug boot add 00B1 'Linux' mmc 0:1 vmlinux-5.99 UUID=1234 > efidebug boot initrd 00B1 mmc 0:1 initrd-5.99 > efidebug boot dtb mmc 00B1 0:1 /dtbs/5.99/vendor/board.dtb > efidebug boot order 00B1 > > What will happen if you pass the following command next: > > efidebug boot add 00B1 'Linux' mmc 0:1 vmlinux-5.99.1 UUID=1234 > > Will initrd and and dtb be kept? That's one of the reasons the RFC had everything in one line. You enforce less options to the user, which imho is always good :). > > What will happen if you start with dtb or with initrd and not with add? IMHO we should return the usage in that case and explicitly request the boot option to be set first. In principle the dtb and initrd are not load options. They are appended DTB instances in an existing load option. If you dont do 'add' first, there's no device path to begin with. So to answer your first question, in order to simplify the command line, I'd suggest we overwrite the load_option when adding a new image. It's unlikely the initrd will remain as is anyway, the dtb might remain the same, but it's not worth the effort and complexity. You'll need a substantial amount of code parsing the DP instances and placing elements in top/middle/bottom etc. > > A device path with kernel.efi, no initrd, and dtb would have an initrd > device path with only an end node. Should we install a LOAD FILE2 > protocol in this case to disallow initrd on the command line? If you do that the user won't be able to load an initrd, unless a fixup is applied directly on the DTB. I'd avoid that, I would simply defer from installing the protocol overall if an end node is discovered on the initrd. > > The boot options are relevant for all users, while the rest of efidebug > is more developers oriented. Should we separate the boot related > commands from the rest of efidebug and build with the new command by > default? > > bootopt add 00B1 'Linux' mmc 0:1 vmlinux-5.99 UUID=1234 > bootopt initrd 00B1 mmc 0:1 initrd-5.99 > bootopt dtb mmc 00B1 0:1 /dtbs/5.99/vendor/board.dtb > bootopt order 00B1 +1 on that, efidebug feels a bit weird for that. This can go in on a different patchset I guess? It would merely mean c/p the code in a different file and edit a few makesfiles? Cheers /Ilias > > Best regards > > Heinrich > > > > > That would untangle the do_efi_boot_add() function, make our lives easier on > > adding things like 'kernel <no initrd> valid dtb' and should be much easier > > to use. The user will just have to make sure the boot order numbers match when > > adding files > > > > [...] > > > > Cheers > > /Ilias > > >
On Thu, Jan 14, 2021 at 02:36:23PM +0200, Ilias Apalodimas wrote: > On Thu, Jan 14, 2021 at 01:07:42PM +0100, Heinrich Schuchardt wrote: > > On 14.01.21 10:55, Ilias Apalodimas wrote: > > > On Thu, Jan 14, 2021 at 02:11:33PM +0900, AKASHI Takahiro wrote: > > >> Ilias, > > >> > > >> On Wed, Jan 13, 2021 at 01:11:49PM +0200, Ilias Apalodimas wrote: > > >>> The UEFI spec allow a packed array of UEFI device paths in the > > >>> FilePathList[] of an EFI_LOAD_OPTION. The first file path must > > >>> describe the laoded image but the rest are OS specific. > > >>> Previous patches parse the device path and try to use the second > > >>> member of the array as an initrd. So let's modify efidebug slightly > > >>> and install the second file described in the command line as the > > >>> initrd device path. > > >> > > >> I have a concern about your proposed command line syntax. > > >> It takes a lot of parameters as a whole which makes it > > >> hard to understand it at a glance, easily overflowing > > >> the width of terminal window. > > >> > > >> It will even get worse if we want to take dtb as a third > > >> device path, and what if we want to specify dtb, but not initrd? > > >> > > >> Moreover, if we want to add support for non-linux executabes which > > >> utilize extra device paths (neither initrd nor dtb) in a boot > > >> load option, the syntax will be problematic. > > >> > > > > > > Maybe we should add explicit commands in efidebug then? > > > Something like: > > > efidebug initrd add 0002 virtio 1 initrd_file > > > efidebug dtb add 0002 virtio 1 dtb > > > > Why "add"? If no file is provided, we could empty the device path. > > > > All other boot related subcommands start with efidebug boot. So how about: > > > > efidebug boot add 00B1 'Linux' mmc 0:1 vmlinux-5.99 UUID=1234 > > efidebug boot initrd 00B1 mmc 0:1 initrd-5.99 > > efidebug boot dtb mmc 00B1 0:1 /dtbs/5.99/vendor/board.dtb > > efidebug boot order 00B1 I have had the same idea in my mind. > > What will happen if you pass the following command next: > > > > efidebug boot add 00B1 'Linux' mmc 0:1 vmlinux-5.99.1 UUID=1234 > > > > Will initrd and and dtb be kept? > > That's one of the reasons the RFC had everything in one line. You enforce less > options to the user, which imho is always good :). > > > > > What will happen if you start with dtb or with initrd and not with add? > > IMHO we should return the usage in that case and explicitly request the boot > option to be set first. In principle the dtb and initrd are not load options. > They are appended DTB instances in an existing load option. If you dont do > 'add' first, there's no device path to begin with. > > So to answer your first question, in order to simplify the command line, I'd > suggest we overwrite the load_option when adding a new image. It's unlikely > the initrd will remain as is anyway, the dtb might remain the same, but it's > not worth the effort and complexity. You'll need a substantial amount of code > parsing the DP instances and placing elements in top/middle/bottom etc. > > > > > A device path with kernel.efi, no initrd, and dtb would have an initrd > > device path with only an end node. Should we install a LOAD FILE2 > > protocol in this case to disallow initrd on the command line? > > If you do that the user won't be able to load an initrd, unless a fixup is > applied directly on the DTB. I'd avoid that, I would simply defer from > installing the protocol overall if an end node is discovered on the initrd. > > > > > The boot options are relevant for all users, while the rest of efidebug > > is more developers oriented. Should we separate the boot related > > commands from the rest of efidebug and build with the new command by > > default? > > > > bootopt add 00B1 'Linux' mmc 0:1 vmlinux-5.99 UUID=1234 > > bootopt initrd 00B1 mmc 0:1 initrd-5.99 > > bootopt dtb mmc 00B1 0:1 /dtbs/5.99/vendor/board.dtb > > bootopt order 00B1 My long-standing hope was to step up the stage of "efidebug" from "debug" to "normal" command. Initially, I intended to implement this command as an alternative of EDK2's efishell (or poorman's shell), so most of the sub commands have a similar syntax and output formats to efisehell defined in EFI specification. But this idea was rejected by Alex :) I agree that "boot" sub command is a good candidate of a standalone command. Or alternatively, we may want to add options to bootefi command. > +1 on that, efidebug feels a bit weird for that. > This can go in on a different patchset I guess? > It would merely mean c/p the code in a different file and > edit a few makesfiles? Anyhow, when you go this way, please don't forget to update pytest scripts since efidebug is also used in secure boot/capsule update tests. -Takahiro Akashi > Cheers > /Ilias > > > > Best regards > > > > Heinrich > > > > > > > > That would untangle the do_efi_boot_add() function, make our lives easier on > > > adding things like 'kernel <no initrd> valid dtb' and should be much easier > > > to use. The user will just have to make sure the boot order numbers match when > > > adding files > > > > > > [...] > > > > > > Cheers > > > /Ilias > > > > >
diff --git a/cmd/efidebug.c b/cmd/efidebug.c index 5fb7b1e3c6a9..8d62981aca92 100644 --- a/cmd/efidebug.c +++ b/cmd/efidebug.c @@ -8,6 +8,7 @@ #include <charset.h> #include <common.h> #include <command.h> +#include <efi_helper.h> #include <efi_loader.h> #include <efi_rng.h> #include <exports.h> @@ -17,6 +18,7 @@ #include <mapmem.h> #include <search.h> #include <linux/ctype.h> +#include <linux/err.h> #define BS systab.boottime #define RT systab.runtime @@ -782,6 +784,42 @@ static int do_efi_show_tables(struct cmd_tbl *cmdtp, int flag, return CMD_RET_SUCCESS; } +/** + * add_initrd_instance() - Append a device path to load_options pointing to an + * inirtd + * + * @argc: Number of arguments + * @argv: Argument array + * @file_path Existing device path, the new instance will be appended + * Return: Pointer to the device path or ERR_PTR + * + */ +static struct efi_device_path *add_initrd_instance(int argc, char *const argv[], + struct efi_device_path *file_path) +{ + struct efi_device_path *tmp_dp = NULL, *tmp_fp = NULL; + struct efi_device_path *final_fp = NULL; + efi_status_t ret; + + if (argc < 8) + return ERR_PTR(-EINVAL); + + ret = efi_dp_from_name(argv[6], argv[7], argv[8], &tmp_dp, + &tmp_fp); + if (ret != EFI_SUCCESS) { + printf("Cannot create device path for \"%s %s\"\n", + argv[6], argv[7]); + goto out; + } + + final_fp = efi_dp_append_instance(file_path, tmp_fp); + +out: + efi_free_pool(tmp_dp); + efi_free_pool(tmp_fp); + return final_fp ? final_fp : ERR_PTR(-EINVAL); +} + /** * do_efi_boot_add() - set UEFI load option * @@ -794,7 +832,11 @@ static int do_efi_show_tables(struct cmd_tbl *cmdtp, int flag, * * Implement efidebug "boot add" sub-command. Create or change UEFI load option. * - * efidebug boot add <id> <label> <interface> <devnum>[:<part>] <file> <options> + * Without initrd: + * efidebug boot add <id> <label> <interface> <devnum>[:<part>] <file> <options> + * + * With initrd: + * efidebug boot add <id> <label> <interface> <devnum>[:<part>] <file> <interface2> <devnum2>[:<part>] <initrd> <options> */ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) @@ -807,13 +849,14 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, size_t label_len, label_len16; u16 *label; struct efi_device_path *device_path = NULL, *file_path = NULL; + struct efi_device_path *final_fp = NULL; struct efi_load_option lo; void *data = NULL; efi_uintn_t size; efi_status_t ret; int r = CMD_RET_SUCCESS; - if (argc < 6 || argc > 7) + if (argc < 6 || argc > 9) return CMD_RET_USAGE; id = (int)simple_strtoul(argv[1], &endp, 16); @@ -829,6 +872,12 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, /* attributes */ lo.attributes = LOAD_OPTION_ACTIVE; /* always ACTIVE */ + /* optional data */ + if (argc == 6) + lo.optional_data = NULL; + else + lo.optional_data = (const u8 *)argv[6]; + /* label */ label_len = strlen(argv[2]); label_len16 = utf8_utf16_strnlen(argv[2], label_len); @@ -847,15 +896,30 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, r = CMD_RET_FAILURE; goto out; } + + /* add initrd instance in device path */ + if (argc >= 9) { + final_fp = add_initrd_instance(argc, argv, file_path); + if (IS_ERR(final_fp)) { + r = CMD_RET_FAILURE; + goto out; + } + + /* add_initrd_instance allocates a new device path */ + efi_free_pool(file_path); + file_path = final_fp; + + /* update optional data */ + if (argc == 9) + lo.optional_data = NULL; + else + lo.optional_data = (const u8 *)argv[9]; + } + lo.file_path = file_path; lo.file_path_length = efi_dp_size(file_path) + sizeof(struct efi_device_path); /* for END */ - /* optional data */ - if (argc == 6) - lo.optional_data = NULL; - else - lo.optional_data = (const u8 *)argv[6]; size = efi_serialize_load_option(&lo, (u8 **)&data); if (!size) { @@ -939,11 +1003,13 @@ static int do_efi_boot_rm(struct cmd_tbl *cmdtp, int flag, */ static void show_efi_boot_opt_data(u16 *varname16, void *data, size_t *size) { + struct efi_device_path *initrd_path = NULL; struct efi_load_option lo; char *label, *p; size_t label_len16, label_len; u16 *dp_str; efi_status_t ret; + efi_uintn_t initrd_dp_size; ret = efi_deserialize_load_option(&lo, data, size); if (ret != EFI_SUCCESS) { @@ -973,6 +1039,13 @@ static void show_efi_boot_opt_data(u16 *varname16, void *data, size_t *size) dp_str = efi_dp_str(lo.file_path); printf(" file_path: %ls\n", dp_str); efi_free_pool(dp_str); + initrd_path = efi_dp_instance_by_idx(lo.file_path, &initrd_dp_size, INITRD_DP); + if (initrd_path) { + dp_str = efi_dp_str(initrd_path); + printf(" initrd_path: %ls\n", dp_str); + efi_free_pool(dp_str); + efi_free_pool(initrd_path); + } printf(" data:\n"); print_hex_dump(" ", DUMP_PREFIX_OFFSET, 16, 1, @@ -1583,7 +1656,7 @@ static char efidebug_help_text[] = #endif U_BOOT_CMD( - efidebug, 10, 0, do_efidebug, + efidebug, CONFIG_SYS_MAXARGS, 0, do_efidebug, "Configure UEFI environment", efidebug_help_text );
The UEFI spec allow a packed array of UEFI device paths in the FilePathList[] of an EFI_LOAD_OPTION. The first file path must describe the laoded image but the rest are OS specific. Previous patches parse the device path and try to use the second member of the array as an initrd. So let's modify efidebug slightly and install the second file described in the command line as the initrd device path. Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- cmd/efidebug.c | 89 +++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 81 insertions(+), 8 deletions(-) -- 2.30.0.rc2