| Message ID | 20190115025437.11966-5-takahiro.akashi@linaro.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi_loader: run a specific efi application more easily | expand |
On 1/15/19 3:54 AM, AKASHI Takahiro wrote: > With this patch applied, we will be able to selectively execute > an EFI application by specifying a load option, say "1" for Boot0001, > "2" for Boot0002 and so on. > > => bootefi bootmgr <fdt addr> 1, or > bootefi bootmgr - 1 You already introduced the support for BootNext. So is there a real benefit? > > Please note that BootXXXX need not be included in "BootOrder". > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > cmd/bootefi.c | 39 ++++++++++++++++++++++++++++----------- > 1 file changed, 28 insertions(+), 11 deletions(-) > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c > index 3be01b49b589..241fd0f987ab 100644 > --- a/cmd/bootefi.c > +++ b/cmd/bootefi.c > @@ -471,16 +471,15 @@ static efi_status_t bootefi_test_prepare > > #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */ > > -static int do_bootefi_bootmgr_exec(void) > +static int do_bootefi_bootmgr_exec(int boot_id) > { > struct efi_device_path *device_path, *file_path; > void *addr; > efi_status_t r; > > - addr = efi_bootmgr_load(EFI_BOOTMGR_DEFAULT_ORDER, > - &device_path, &file_path); > + addr = efi_bootmgr_load(boot_id, &device_path, &file_path); > if (!addr) > - return 1; > + return CMD_RET_FAILURE; > > printf("## Starting EFI application at %p ...\n", addr); > r = do_bootefi_exec(addr, device_path, file_path); > @@ -488,9 +487,9 @@ static int do_bootefi_bootmgr_exec(void) > r & ~EFI_ERROR_MASK); > > if (r != EFI_SUCCESS) > - return 1; > + return CMD_RET_FAILURE; > > - return 0; > + return CMD_RET_SUCCESS; > } > > /* Interpreter command to boot an arbitrary EFI image from memory */ > @@ -546,10 +545,28 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > } else > #endif > if (!strcmp(argv[1], "bootmgr")) { > - if (efi_handle_fdt(argc > 2 ? argv[2] : NULL)) > - return CMD_RET_FAILURE; > + char *fdtstr, *endp; > + int boot_id = EFI_BOOTMGR_DEFAULT_ORDER; > + > + if (argc > 2) { > + fdtstr = argv[2]; > + /* Special address "-" means no device tree */ > + if (fdtstr[0] == '-') > + fdtstr = NULL; > + > + r = efi_handle_fdt(fdtstr); > + if (r) > + return CMD_RET_FAILURE; > + } > + > + if (argc > 3) { > + boot_id = (int)simple_strtoul(argv[3], &endp, 0); > + if ((argv[3] + strlen(argv[3]) != endp) || > + boot_id > 0xffff) > + return CMD_RET_USAGE; > + } > > - return do_bootefi_bootmgr_exec(); > + return do_bootefi_bootmgr_exec(boot_id); Why not communicate via the BootNext variable? > } else { > saddr = argv[1]; > > @@ -590,7 +607,7 @@ static char bootefi_help_text[] = > " Use environment variable efi_selftest to select a single test.\n" > " Use 'setenv efi_selftest list' to enumerate all tests.\n" > #endif > - "bootefi bootmgr [fdt addr]\n" > + "bootefi bootmgr [<fdt addr>|'-' [<boot id>]]\n" > " - load and boot EFI payload based on BootOrder/BootXXXX variables.\n" > "\n" > " If specified, the device tree located at <fdt address> gets\n" > @@ -598,7 +615,7 @@ static char bootefi_help_text[] = > #endif > > U_BOOT_CMD( > - bootefi, 3, 0, do_bootefi, > + bootefi, 5, 0, do_bootefi, Why 5? Best regards Heinrich > "Boots an EFI payload from memory", > bootefi_help_text > ); >
On Tue, Feb 26, 2019 at 08:30:50PM +0100, Heinrich Schuchardt wrote: > On 1/15/19 3:54 AM, AKASHI Takahiro wrote: > > With this patch applied, we will be able to selectively execute > > an EFI application by specifying a load option, say "1" for Boot0001, > > "2" for Boot0002 and so on. > > > > => bootefi bootmgr <fdt addr> 1, or > > bootefi bootmgr - 1 > > You already introduced the support for BootNext. So is there a real benefit? This is a convenient way of running EFI application directly, but I already removed this feature from the next version. > > > > Please note that BootXXXX need not be included in "BootOrder". > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > --- > > cmd/bootefi.c | 39 ++++++++++++++++++++++++++++----------- > > 1 file changed, 28 insertions(+), 11 deletions(-) > > > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c > > index 3be01b49b589..241fd0f987ab 100644 > > --- a/cmd/bootefi.c > > +++ b/cmd/bootefi.c > > @@ -471,16 +471,15 @@ static efi_status_t bootefi_test_prepare > > > > #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */ > > > > -static int do_bootefi_bootmgr_exec(void) > > +static int do_bootefi_bootmgr_exec(int boot_id) > > { > > struct efi_device_path *device_path, *file_path; > > void *addr; > > efi_status_t r; > > > > - addr = efi_bootmgr_load(EFI_BOOTMGR_DEFAULT_ORDER, > > - &device_path, &file_path); > > + addr = efi_bootmgr_load(boot_id, &device_path, &file_path); > > if (!addr) > > - return 1; > > + return CMD_RET_FAILURE; > > > > printf("## Starting EFI application at %p ...\n", addr); > > r = do_bootefi_exec(addr, device_path, file_path); > > @@ -488,9 +487,9 @@ static int do_bootefi_bootmgr_exec(void) > > r & ~EFI_ERROR_MASK); > > > > if (r != EFI_SUCCESS) > > - return 1; > > + return CMD_RET_FAILURE; > > > > - return 0; > > + return CMD_RET_SUCCESS; > > } > > > > /* Interpreter command to boot an arbitrary EFI image from memory */ > > @@ -546,10 +545,28 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > > } else > > #endif > > if (!strcmp(argv[1], "bootmgr")) { > > - if (efi_handle_fdt(argc > 2 ? argv[2] : NULL)) > > - return CMD_RET_FAILURE; > > + char *fdtstr, *endp; > > + int boot_id = EFI_BOOTMGR_DEFAULT_ORDER; > > + > > + if (argc > 2) { > > + fdtstr = argv[2]; > > + /* Special address "-" means no device tree */ > > + if (fdtstr[0] == '-') > > + fdtstr = NULL; > > + > > + r = efi_handle_fdt(fdtstr); > > + if (r) > > + return CMD_RET_FAILURE; > > + } > > + > > + if (argc > 3) { > > + boot_id = (int)simple_strtoul(argv[3], &endp, 0); > > + if ((argv[3] + strlen(argv[3]) != endp) || > > + boot_id > 0xffff) > > + return CMD_RET_USAGE; > > + } > > > > - return do_bootefi_bootmgr_exec(); > > + return do_bootefi_bootmgr_exec(boot_id); > > Why not communicate via the BootNext variable? I don't get your point. BootNext and BootOrder are both defined by UEFI specification. > > } else { > > saddr = argv[1]; > > > > @@ -590,7 +607,7 @@ static char bootefi_help_text[] = > > " Use environment variable efi_selftest to select a single test.\n" > > " Use 'setenv efi_selftest list' to enumerate all tests.\n" > > #endif > > - "bootefi bootmgr [fdt addr]\n" > > + "bootefi bootmgr [<fdt addr>|'-' [<boot id>]]\n" > > " - load and boot EFI payload based on BootOrder/BootXXXX variables.\n" > > "\n" > > " If specified, the device tree located at <fdt address> gets\n" > > @@ -598,7 +615,7 @@ static char bootefi_help_text[] = > > #endif > > > > U_BOOT_CMD( > > - bootefi, 3, 0, do_bootefi, > > + bootefi, 5, 0, do_bootefi, > > Why 5? For additional/optional '-' and <boot id>. But I removed this feature from bootefi. Thanks, -Takahiro Akashi > Best regards > > Heinrich > > > "Boots an EFI payload from memory", > > bootefi_help_text > > ); > > >
On 2/27/19 6:58 AM, AKASHI Takahiro wrote: > On Tue, Feb 26, 2019 at 08:30:50PM +0100, Heinrich Schuchardt wrote: >> On 1/15/19 3:54 AM, AKASHI Takahiro wrote: >>> With this patch applied, we will be able to selectively execute >>> an EFI application by specifying a load option, say "1" for Boot0001, >>> "2" for Boot0002 and so on. >>> >>> => bootefi bootmgr <fdt addr> 1, or >>> bootefi bootmgr - 1 >> >> You already introduced the support for BootNext. So is there a real benefit? > > This is a convenient way of running EFI application directly, > but I already removed this feature from the next version. Please, remove 'run -e' instead because it cannot specify the device tree needed for booting ARM boards. > >>> >>> Please note that BootXXXX need not be included in "BootOrder". >>> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >>> --- >>> cmd/bootefi.c | 39 ++++++++++++++++++++++++++++----------- >>> 1 file changed, 28 insertions(+), 11 deletions(-) >>> >>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c >>> index 3be01b49b589..241fd0f987ab 100644 >>> --- a/cmd/bootefi.c >>> +++ b/cmd/bootefi.c >>> @@ -471,16 +471,15 @@ static efi_status_t bootefi_test_prepare >>> >>> #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */ >>> >>> -static int do_bootefi_bootmgr_exec(void) >>> +static int do_bootefi_bootmgr_exec(int boot_id) >>> { >>> struct efi_device_path *device_path, *file_path; >>> void *addr; >>> efi_status_t r; >>> >>> - addr = efi_bootmgr_load(EFI_BOOTMGR_DEFAULT_ORDER, >>> - &device_path, &file_path); >>> + addr = efi_bootmgr_load(boot_id, &device_path, &file_path); >>> if (!addr) >>> - return 1; >>> + return CMD_RET_FAILURE; >>> >>> printf("## Starting EFI application at %p ...\n", addr); >>> r = do_bootefi_exec(addr, device_path, file_path); >>> @@ -488,9 +487,9 @@ static int do_bootefi_bootmgr_exec(void) >>> r & ~EFI_ERROR_MASK); >>> >>> if (r != EFI_SUCCESS) >>> - return 1; >>> + return CMD_RET_FAILURE; >>> >>> - return 0; >>> + return CMD_RET_SUCCESS; >>> } >>> >>> /* Interpreter command to boot an arbitrary EFI image from memory */ >>> @@ -546,10 +545,28 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >>> } else >>> #endif >>> if (!strcmp(argv[1], "bootmgr")) { >>> - if (efi_handle_fdt(argc > 2 ? argv[2] : NULL)) >>> - return CMD_RET_FAILURE; >>> + char *fdtstr, *endp; >>> + int boot_id = EFI_BOOTMGR_DEFAULT_ORDER; >>> + >>> + if (argc > 2) { >>> + fdtstr = argv[2]; >>> + /* Special address "-" means no device tree */ >>> + if (fdtstr[0] == '-') >>> + fdtstr = NULL; >>> + >>> + r = efi_handle_fdt(fdtstr); >>> + if (r) >>> + return CMD_RET_FAILURE; >>> + } >>> + >>> + if (argc > 3) { >>> + boot_id = (int)simple_strtoul(argv[3], &endp, 0); >>> + if ((argv[3] + strlen(argv[3]) != endp) || >>> + boot_id > 0xffff) >>> + return CMD_RET_USAGE; >>> + } >>> >>> - return do_bootefi_bootmgr_exec(); >>> + return do_bootefi_bootmgr_exec(boot_id); >> >> Why not communicate via the BootNext variable? > > I don't get your point. > BootNext and BootOrder are both defined by UEFI specification. Instead of changing the interface of do_bootefi_bootmgr_exec() you could simply set BootNext. Then the boot manager would pick up the option from the variable and finally delete the variable. This would result in less code. Best regards Heinrich > >>> } else { >>> saddr = argv[1]; >>> >>> @@ -590,7 +607,7 @@ static char bootefi_help_text[] = >>> " Use environment variable efi_selftest to select a single test.\n" >>> " Use 'setenv efi_selftest list' to enumerate all tests.\n" >>> #endif >>> - "bootefi bootmgr [fdt addr]\n" >>> + "bootefi bootmgr [<fdt addr>|'-' [<boot id>]]\n" >>> " - load and boot EFI payload based on BootOrder/BootXXXX variables.\n" >>> "\n" >>> " If specified, the device tree located at <fdt address> gets\n" >>> @@ -598,7 +615,7 @@ static char bootefi_help_text[] = >>> #endif >>> >>> U_BOOT_CMD( >>> - bootefi, 3, 0, do_bootefi, >>> + bootefi, 5, 0, do_bootefi, >> >> Why 5? > > For additional/optional '-' and <boot id>. > But I removed this feature from bootefi. > > Thanks, > -Takahiro Akashi > > >> Best regards >> >> Heinrich >> >>> "Boots an EFI payload from memory", >>> bootefi_help_text >>> ); >>> >> >
On Wed, Feb 27, 2019 at 07:31:06AM +0100, Heinrich Schuchardt wrote: > On 2/27/19 6:58 AM, AKASHI Takahiro wrote: > > On Tue, Feb 26, 2019 at 08:30:50PM +0100, Heinrich Schuchardt wrote: > >> On 1/15/19 3:54 AM, AKASHI Takahiro wrote: > >>> With this patch applied, we will be able to selectively execute > >>> an EFI application by specifying a load option, say "1" for Boot0001, > >>> "2" for Boot0002 and so on. > >>> > >>> => bootefi bootmgr <fdt addr> 1, or > >>> bootefi bootmgr - 1 > >> > >> You already introduced the support for BootNext. So is there a real benefit? > > > > This is a convenient way of running EFI application directly, > > but I already removed this feature from the next version. > > Please, remove 'run -e' instead because it cannot specify the device > tree needed for booting ARM boards. See my comment for patch#5 first. > > > >>> > >>> Please note that BootXXXX need not be included in "BootOrder". > >>> > >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > >>> --- > >>> cmd/bootefi.c | 39 ++++++++++++++++++++++++++++----------- > >>> 1 file changed, 28 insertions(+), 11 deletions(-) > >>> > >>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c > >>> index 3be01b49b589..241fd0f987ab 100644 > >>> --- a/cmd/bootefi.c > >>> +++ b/cmd/bootefi.c > >>> @@ -471,16 +471,15 @@ static efi_status_t bootefi_test_prepare > >>> > >>> #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */ > >>> > >>> -static int do_bootefi_bootmgr_exec(void) > >>> +static int do_bootefi_bootmgr_exec(int boot_id) > >>> { > >>> struct efi_device_path *device_path, *file_path; > >>> void *addr; > >>> efi_status_t r; > >>> > >>> - addr = efi_bootmgr_load(EFI_BOOTMGR_DEFAULT_ORDER, > >>> - &device_path, &file_path); > >>> + addr = efi_bootmgr_load(boot_id, &device_path, &file_path); > >>> if (!addr) > >>> - return 1; > >>> + return CMD_RET_FAILURE; > >>> > >>> printf("## Starting EFI application at %p ...\n", addr); > >>> r = do_bootefi_exec(addr, device_path, file_path); > >>> @@ -488,9 +487,9 @@ static int do_bootefi_bootmgr_exec(void) > >>> r & ~EFI_ERROR_MASK); > >>> > >>> if (r != EFI_SUCCESS) > >>> - return 1; > >>> + return CMD_RET_FAILURE; > >>> > >>> - return 0; > >>> + return CMD_RET_SUCCESS; > >>> } > >>> > >>> /* Interpreter command to boot an arbitrary EFI image from memory */ > >>> @@ -546,10 +545,28 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > >>> } else > >>> #endif > >>> if (!strcmp(argv[1], "bootmgr")) { > >>> - if (efi_handle_fdt(argc > 2 ? argv[2] : NULL)) > >>> - return CMD_RET_FAILURE; > >>> + char *fdtstr, *endp; > >>> + int boot_id = EFI_BOOTMGR_DEFAULT_ORDER; > >>> + > >>> + if (argc > 2) { > >>> + fdtstr = argv[2]; > >>> + /* Special address "-" means no device tree */ > >>> + if (fdtstr[0] == '-') > >>> + fdtstr = NULL; > >>> + > >>> + r = efi_handle_fdt(fdtstr); > >>> + if (r) > >>> + return CMD_RET_FAILURE; > >>> + } > >>> + > >>> + if (argc > 3) { > >>> + boot_id = (int)simple_strtoul(argv[3], &endp, 0); > >>> + if ((argv[3] + strlen(argv[3]) != endp) || > >>> + boot_id > 0xffff) > >>> + return CMD_RET_USAGE; > >>> + } > >>> > >>> - return do_bootefi_bootmgr_exec(); > >>> + return do_bootefi_bootmgr_exec(boot_id); > >> > >> Why not communicate via the BootNext variable? > > > > I don't get your point. > > BootNext and BootOrder are both defined by UEFI specification. > > Instead of changing the interface of do_bootefi_bootmgr_exec() Who care changing an *internal* function? > you could > simply set BootNext. Then the boot manager would pick up the option from > the variable and finally delete the variable. This would result in less > code. No. Even with "run -e," BootNext will disappear after execution. This is a requirement by UEFI spec. Thanks, -Takahiro Akashi > Best regards > > Heinrich > > > > >>> } else { > >>> saddr = argv[1]; > >>> > >>> @@ -590,7 +607,7 @@ static char bootefi_help_text[] = > >>> " Use environment variable efi_selftest to select a single test.\n" > >>> " Use 'setenv efi_selftest list' to enumerate all tests.\n" > >>> #endif > >>> - "bootefi bootmgr [fdt addr]\n" > >>> + "bootefi bootmgr [<fdt addr>|'-' [<boot id>]]\n" > >>> " - load and boot EFI payload based on BootOrder/BootXXXX variables.\n" > >>> "\n" > >>> " If specified, the device tree located at <fdt address> gets\n" > >>> @@ -598,7 +615,7 @@ static char bootefi_help_text[] = > >>> #endif > >>> > >>> U_BOOT_CMD( > >>> - bootefi, 3, 0, do_bootefi, > >>> + bootefi, 5, 0, do_bootefi, > >> > >> Why 5? > > > > For additional/optional '-' and <boot id>. > > But I removed this feature from bootefi. > > > > Thanks, > > -Takahiro Akashi > > > > > >> Best regards > >> > >> Heinrich > >> > >>> "Boots an EFI payload from memory", > >>> bootefi_help_text > >>> ); > >>> > >> > > >
On 2/27/19 7:47 AM, AKASHI Takahiro wrote: > On Wed, Feb 27, 2019 at 07:31:06AM +0100, Heinrich Schuchardt wrote: >> On 2/27/19 6:58 AM, AKASHI Takahiro wrote: >>> On Tue, Feb 26, 2019 at 08:30:50PM +0100, Heinrich Schuchardt wrote: >>>> On 1/15/19 3:54 AM, AKASHI Takahiro wrote: >>>>> With this patch applied, we will be able to selectively execute >>>>> an EFI application by specifying a load option, say "1" for Boot0001, >>>>> "2" for Boot0002 and so on. >>>>> >>>>> => bootefi bootmgr <fdt addr> 1, or >>>>> bootefi bootmgr - 1 >>>> >>>> You already introduced the support for BootNext. So is there a real benefit? >>> >>> This is a convenient way of running EFI application directly, >>> but I already removed this feature from the next version. >> >> Please, remove 'run -e' instead because it cannot specify the device >> tree needed for booting ARM boards. > > See my comment for patch#5 first. > >>> >>>>> >>>>> Please note that BootXXXX need not be included in "BootOrder". >>>>> >>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >>>>> --- >>>>> cmd/bootefi.c | 39 ++++++++++++++++++++++++++++----------- >>>>> 1 file changed, 28 insertions(+), 11 deletions(-) >>>>> >>>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c >>>>> index 3be01b49b589..241fd0f987ab 100644 >>>>> --- a/cmd/bootefi.c >>>>> +++ b/cmd/bootefi.c >>>>> @@ -471,16 +471,15 @@ static efi_status_t bootefi_test_prepare >>>>> >>>>> #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */ >>>>> >>>>> -static int do_bootefi_bootmgr_exec(void) >>>>> +static int do_bootefi_bootmgr_exec(int boot_id) >>>>> { >>>>> struct efi_device_path *device_path, *file_path; >>>>> void *addr; >>>>> efi_status_t r; >>>>> >>>>> - addr = efi_bootmgr_load(EFI_BOOTMGR_DEFAULT_ORDER, >>>>> - &device_path, &file_path); >>>>> + addr = efi_bootmgr_load(boot_id, &device_path, &file_path); >>>>> if (!addr) >>>>> - return 1; >>>>> + return CMD_RET_FAILURE; >>>>> >>>>> printf("## Starting EFI application at %p ...\n", addr); >>>>> r = do_bootefi_exec(addr, device_path, file_path); >>>>> @@ -488,9 +487,9 @@ static int do_bootefi_bootmgr_exec(void) >>>>> r & ~EFI_ERROR_MASK); >>>>> >>>>> if (r != EFI_SUCCESS) >>>>> - return 1; >>>>> + return CMD_RET_FAILURE; >>>>> >>>>> - return 0; >>>>> + return CMD_RET_SUCCESS; >>>>> } >>>>> >>>>> /* Interpreter command to boot an arbitrary EFI image from memory */ >>>>> @@ -546,10 +545,28 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >>>>> } else >>>>> #endif >>>>> if (!strcmp(argv[1], "bootmgr")) { >>>>> - if (efi_handle_fdt(argc > 2 ? argv[2] : NULL)) >>>>> - return CMD_RET_FAILURE; >>>>> + char *fdtstr, *endp; >>>>> + int boot_id = EFI_BOOTMGR_DEFAULT_ORDER; >>>>> + >>>>> + if (argc > 2) { >>>>> + fdtstr = argv[2]; >>>>> + /* Special address "-" means no device tree */ >>>>> + if (fdtstr[0] == '-') >>>>> + fdtstr = NULL; >>>>> + >>>>> + r = efi_handle_fdt(fdtstr); >>>>> + if (r) >>>>> + return CMD_RET_FAILURE; >>>>> + } >>>>> + >>>>> + if (argc > 3) { >>>>> + boot_id = (int)simple_strtoul(argv[3], &endp, 0); >>>>> + if ((argv[3] + strlen(argv[3]) != endp) || >>>>> + boot_id > 0xffff) >>>>> + return CMD_RET_USAGE; >>>>> + } >>>>> >>>>> - return do_bootefi_bootmgr_exec(); >>>>> + return do_bootefi_bootmgr_exec(boot_id); >>>> >>>> Why not communicate via the BootNext variable? >>> >>> I don't get your point. >>> BootNext and BootOrder are both defined by UEFI specification. >> >> Instead of changing the interface of do_bootefi_bootmgr_exec() > > Who care changing an *internal* function? > >> you could >> simply set BootNext. Then the boot manager would pick up the option from >> the variable and finally delete the variable. This would result in less >> code. > > No. Even with "run -e," BootNext will disappear after execution. > This is a requirement by UEFI spec. Shouldn't BootNext always be reset when executing bootefi no matter whether the boot manager is used or not? Regards Heinrich > > Thanks, > -Takahiro Akashi > >> Best regards >> >> Heinrich >> >>> >>>>> } else { >>>>> saddr = argv[1]; >>>>> >>>>> @@ -590,7 +607,7 @@ static char bootefi_help_text[] = >>>>> " Use environment variable efi_selftest to select a single test.\n" >>>>> " Use 'setenv efi_selftest list' to enumerate all tests.\n" >>>>> #endif >>>>> - "bootefi bootmgr [fdt addr]\n" >>>>> + "bootefi bootmgr [<fdt addr>|'-' [<boot id>]]\n" >>>>> " - load and boot EFI payload based on BootOrder/BootXXXX variables.\n" >>>>> "\n" >>>>> " If specified, the device tree located at <fdt address> gets\n" >>>>> @@ -598,7 +615,7 @@ static char bootefi_help_text[] = >>>>> #endif >>>>> >>>>> U_BOOT_CMD( >>>>> - bootefi, 3, 0, do_bootefi, >>>>> + bootefi, 5, 0, do_bootefi, >>>> >>>> Why 5? >>> >>> For additional/optional '-' and <boot id>. >>> But I removed this feature from bootefi. >>> >>> Thanks, >>> -Takahiro Akashi >>> >>> >>>> Best regards >>>> >>>> Heinrich >>>> >>>>> "Boots an EFI payload from memory", >>>>> bootefi_help_text >>>>> ); >>>>> >>>> >>> >> >
On Wed, Feb 27, 2019 at 08:33:17PM +0100, Heinrich Schuchardt wrote: > On 2/27/19 7:47 AM, AKASHI Takahiro wrote: > > On Wed, Feb 27, 2019 at 07:31:06AM +0100, Heinrich Schuchardt wrote: > >> On 2/27/19 6:58 AM, AKASHI Takahiro wrote: > >>> On Tue, Feb 26, 2019 at 08:30:50PM +0100, Heinrich Schuchardt wrote: > >>>> On 1/15/19 3:54 AM, AKASHI Takahiro wrote: > >>>>> With this patch applied, we will be able to selectively execute > >>>>> an EFI application by specifying a load option, say "1" for Boot0001, > >>>>> "2" for Boot0002 and so on. > >>>>> > >>>>> => bootefi bootmgr <fdt addr> 1, or > >>>>> bootefi bootmgr - 1 > >>>> > >>>> You already introduced the support for BootNext. So is there a real benefit? > >>> > >>> This is a convenient way of running EFI application directly, > >>> but I already removed this feature from the next version. > >> > >> Please, remove 'run -e' instead because it cannot specify the device > >> tree needed for booting ARM boards. > > > > See my comment for patch#5 first. > > > >>> > >>>>> > >>>>> Please note that BootXXXX need not be included in "BootOrder". > >>>>> > >>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > >>>>> --- > >>>>> cmd/bootefi.c | 39 ++++++++++++++++++++++++++++----------- > >>>>> 1 file changed, 28 insertions(+), 11 deletions(-) > >>>>> > >>>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c > >>>>> index 3be01b49b589..241fd0f987ab 100644 > >>>>> --- a/cmd/bootefi.c > >>>>> +++ b/cmd/bootefi.c > >>>>> @@ -471,16 +471,15 @@ static efi_status_t bootefi_test_prepare > >>>>> > >>>>> #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */ > >>>>> > >>>>> -static int do_bootefi_bootmgr_exec(void) > >>>>> +static int do_bootefi_bootmgr_exec(int boot_id) > >>>>> { > >>>>> struct efi_device_path *device_path, *file_path; > >>>>> void *addr; > >>>>> efi_status_t r; > >>>>> > >>>>> - addr = efi_bootmgr_load(EFI_BOOTMGR_DEFAULT_ORDER, > >>>>> - &device_path, &file_path); > >>>>> + addr = efi_bootmgr_load(boot_id, &device_path, &file_path); > >>>>> if (!addr) > >>>>> - return 1; > >>>>> + return CMD_RET_FAILURE; > >>>>> > >>>>> printf("## Starting EFI application at %p ...\n", addr); > >>>>> r = do_bootefi_exec(addr, device_path, file_path); > >>>>> @@ -488,9 +487,9 @@ static int do_bootefi_bootmgr_exec(void) > >>>>> r & ~EFI_ERROR_MASK); > >>>>> > >>>>> if (r != EFI_SUCCESS) > >>>>> - return 1; > >>>>> + return CMD_RET_FAILURE; > >>>>> > >>>>> - return 0; > >>>>> + return CMD_RET_SUCCESS; > >>>>> } > >>>>> > >>>>> /* Interpreter command to boot an arbitrary EFI image from memory */ > >>>>> @@ -546,10 +545,28 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > >>>>> } else > >>>>> #endif > >>>>> if (!strcmp(argv[1], "bootmgr")) { > >>>>> - if (efi_handle_fdt(argc > 2 ? argv[2] : NULL)) > >>>>> - return CMD_RET_FAILURE; > >>>>> + char *fdtstr, *endp; > >>>>> + int boot_id = EFI_BOOTMGR_DEFAULT_ORDER; > >>>>> + > >>>>> + if (argc > 2) { > >>>>> + fdtstr = argv[2]; > >>>>> + /* Special address "-" means no device tree */ > >>>>> + if (fdtstr[0] == '-') > >>>>> + fdtstr = NULL; > >>>>> + > >>>>> + r = efi_handle_fdt(fdtstr); > >>>>> + if (r) > >>>>> + return CMD_RET_FAILURE; > >>>>> + } > >>>>> + > >>>>> + if (argc > 3) { > >>>>> + boot_id = (int)simple_strtoul(argv[3], &endp, 0); > >>>>> + if ((argv[3] + strlen(argv[3]) != endp) || > >>>>> + boot_id > 0xffff) > >>>>> + return CMD_RET_USAGE; > >>>>> + } > >>>>> > >>>>> - return do_bootefi_bootmgr_exec(); > >>>>> + return do_bootefi_bootmgr_exec(boot_id); > >>>> > >>>> Why not communicate via the BootNext variable? > >>> > >>> I don't get your point. > >>> BootNext and BootOrder are both defined by UEFI specification. > >> > >> Instead of changing the interface of do_bootefi_bootmgr_exec() > > > > Who care changing an *internal* function? So do you agree? > > > >> you could > >> simply set BootNext. Then the boot manager would pick up the option from > >> the variable and finally delete the variable. This would result in less > >> code. > > > > No. Even with "run -e," BootNext will disappear after execution. > > This is a requirement by UEFI spec. > > Shouldn't BootNext always be reset when executing bootefi no matter > whether the boot manager is used or not? Didn't I say the same thing? Or do you expect that BootNext remain after "run -e"? -Takahiro Akashi > Regards > > Heinrich > > > > > Thanks, > > -Takahiro Akashi > > > >> Best regards > >> > >> Heinrich > >> > >>> > >>>>> } else { > >>>>> saddr = argv[1]; > >>>>> > >>>>> @@ -590,7 +607,7 @@ static char bootefi_help_text[] = > >>>>> " Use environment variable efi_selftest to select a single test.\n" > >>>>> " Use 'setenv efi_selftest list' to enumerate all tests.\n" > >>>>> #endif > >>>>> - "bootefi bootmgr [fdt addr]\n" > >>>>> + "bootefi bootmgr [<fdt addr>|'-' [<boot id>]]\n" > >>>>> " - load and boot EFI payload based on BootOrder/BootXXXX variables.\n" > >>>>> "\n" > >>>>> " If specified, the device tree located at <fdt address> gets\n" > >>>>> @@ -598,7 +615,7 @@ static char bootefi_help_text[] = > >>>>> #endif > >>>>> > >>>>> U_BOOT_CMD( > >>>>> - bootefi, 3, 0, do_bootefi, > >>>>> + bootefi, 5, 0, do_bootefi, > >>>> > >>>> Why 5? > >>> > >>> For additional/optional '-' and <boot id>. > >>> But I removed this feature from bootefi. > >>> > >>> Thanks, > >>> -Takahiro Akashi > >>> > >>> > >>>> Best regards > >>>> > >>>> Heinrich > >>>> > >>>>> "Boots an EFI payload from memory", > >>>>> bootefi_help_text > >>>>> ); > >>>>> > >>>> > >>> > >> > > >
On 2/28/19 5:28 AM, AKASHI Takahiro wrote: > On Wed, Feb 27, 2019 at 08:33:17PM +0100, Heinrich Schuchardt wrote: >> On 2/27/19 7:47 AM, AKASHI Takahiro wrote: >>> On Wed, Feb 27, 2019 at 07:31:06AM +0100, Heinrich Schuchardt wrote: >>>> On 2/27/19 6:58 AM, AKASHI Takahiro wrote: >>>>> On Tue, Feb 26, 2019 at 08:30:50PM +0100, Heinrich Schuchardt wrote: >>>>>> On 1/15/19 3:54 AM, AKASHI Takahiro wrote: >>>>>>> With this patch applied, we will be able to selectively execute >>>>>>> an EFI application by specifying a load option, say "1" for Boot0001, >>>>>>> "2" for Boot0002 and so on. >>>>>>> >>>>>>> => bootefi bootmgr <fdt addr> 1, or >>>>>>> bootefi bootmgr - 1 >>>>>> >>>>>> You already introduced the support for BootNext. So is there a real benefit? >>>>> >>>>> This is a convenient way of running EFI application directly, >>>>> but I already removed this feature from the next version. >>>> >>>> Please, remove 'run -e' instead because it cannot specify the device >>>> tree needed for booting ARM boards. >>> >>> See my comment for patch#5 first. >>> >>>>> >>>>>>> >>>>>>> Please note that BootXXXX need not be included in "BootOrder". >>>>>>> >>>>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >>>>>>> --- >>>>>>> cmd/bootefi.c | 39 ++++++++++++++++++++++++++++----------- >>>>>>> 1 file changed, 28 insertions(+), 11 deletions(-) >>>>>>> >>>>>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c >>>>>>> index 3be01b49b589..241fd0f987ab 100644 >>>>>>> --- a/cmd/bootefi.c >>>>>>> +++ b/cmd/bootefi.c >>>>>>> @@ -471,16 +471,15 @@ static efi_status_t bootefi_test_prepare >>>>>>> >>>>>>> #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */ >>>>>>> >>>>>>> -static int do_bootefi_bootmgr_exec(void) >>>>>>> +static int do_bootefi_bootmgr_exec(int boot_id) >>>>>>> { >>>>>>> struct efi_device_path *device_path, *file_path; >>>>>>> void *addr; >>>>>>> efi_status_t r; >>>>>>> >>>>>>> - addr = efi_bootmgr_load(EFI_BOOTMGR_DEFAULT_ORDER, >>>>>>> - &device_path, &file_path); >>>>>>> + addr = efi_bootmgr_load(boot_id, &device_path, &file_path); >>>>>>> if (!addr) >>>>>>> - return 1; >>>>>>> + return CMD_RET_FAILURE; >>>>>>> >>>>>>> printf("## Starting EFI application at %p ...\n", addr); >>>>>>> r = do_bootefi_exec(addr, device_path, file_path); >>>>>>> @@ -488,9 +487,9 @@ static int do_bootefi_bootmgr_exec(void) >>>>>>> r & ~EFI_ERROR_MASK); >>>>>>> >>>>>>> if (r != EFI_SUCCESS) >>>>>>> - return 1; >>>>>>> + return CMD_RET_FAILURE; >>>>>>> >>>>>>> - return 0; >>>>>>> + return CMD_RET_SUCCESS; >>>>>>> } >>>>>>> >>>>>>> /* Interpreter command to boot an arbitrary EFI image from memory */ >>>>>>> @@ -546,10 +545,28 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >>>>>>> } else >>>>>>> #endif >>>>>>> if (!strcmp(argv[1], "bootmgr")) { >>>>>>> - if (efi_handle_fdt(argc > 2 ? argv[2] : NULL)) >>>>>>> - return CMD_RET_FAILURE; >>>>>>> + char *fdtstr, *endp; >>>>>>> + int boot_id = EFI_BOOTMGR_DEFAULT_ORDER; >>>>>>> + >>>>>>> + if (argc > 2) { >>>>>>> + fdtstr = argv[2]; >>>>>>> + /* Special address "-" means no device tree */ >>>>>>> + if (fdtstr[0] == '-') >>>>>>> + fdtstr = NULL; >>>>>>> + >>>>>>> + r = efi_handle_fdt(fdtstr); >>>>>>> + if (r) >>>>>>> + return CMD_RET_FAILURE; >>>>>>> + } >>>>>>> + >>>>>>> + if (argc > 3) { >>>>>>> + boot_id = (int)simple_strtoul(argv[3], &endp, 0); >>>>>>> + if ((argv[3] + strlen(argv[3]) != endp) || >>>>>>> + boot_id > 0xffff) >>>>>>> + return CMD_RET_USAGE; >>>>>>> + } >>>>>>> >>>>>>> - return do_bootefi_bootmgr_exec(); >>>>>>> + return do_bootefi_bootmgr_exec(boot_id); >>>>>> >>>>>> Why not communicate via the BootNext variable? >>>>> >>>>> I don't get your point. >>>>> BootNext and BootOrder are both defined by UEFI specification. >>>> >>>> Instead of changing the interface of do_bootefi_bootmgr_exec() >>> >>> Who care changing an *internal* function? > > So do you agree? What is wrong about calling efi_set_variable(L"BootNext", ) instead? Wouldn't that result in less code? > >>> >>>> you could >>>> simply set BootNext. Then the boot manager would pick up the option from >>>> the variable and finally delete the variable. This would result in less >>>> code. >>> >>> No. Even with "run -e," BootNext will disappear after execution. >>> This is a requirement by UEFI spec. >> >> Shouldn't BootNext always be reset when executing bootefi no matter >> whether the boot manager is used or not? > > Didn't I say the same thing? > Or do you expect that BootNext remain after "run -e"? As described in the response to patch 5/5 'run -e' is not usable with ARM given the current state of device trees in Linux. Patch 1/5 only deletes variable BootNext if the boot manager is called. Is this really correct? Shouldn't we delete the BootNext variable when executing `bootefi $kernel_addr_r $fdt_addr_r`, too? Best regards Heinrich > > -Takahiro Akashi > >> Regards >> >> Heinrich >> >>> >>> Thanks, >>> -Takahiro Akashi >>> >>>> Best regards >>>> >>>> Heinrich >>>> >>>>> >>>>>>> } else { >>>>>>> saddr = argv[1]; >>>>>>> >>>>>>> @@ -590,7 +607,7 @@ static char bootefi_help_text[] = >>>>>>> " Use environment variable efi_selftest to select a single test.\n" >>>>>>> " Use 'setenv efi_selftest list' to enumerate all tests.\n" >>>>>>> #endif >>>>>>> - "bootefi bootmgr [fdt addr]\n" >>>>>>> + "bootefi bootmgr [<fdt addr>|'-' [<boot id>]]\n" >>>>>>> " - load and boot EFI payload based on BootOrder/BootXXXX variables.\n" >>>>>>> "\n" >>>>>>> " If specified, the device tree located at <fdt address> gets\n" >>>>>>> @@ -598,7 +615,7 @@ static char bootefi_help_text[] = >>>>>>> #endif >>>>>>> >>>>>>> U_BOOT_CMD( >>>>>>> - bootefi, 3, 0, do_bootefi, >>>>>>> + bootefi, 5, 0, do_bootefi, >>>>>> >>>>>> Why 5? >>>>> >>>>> For additional/optional '-' and <boot id>. >>>>> But I removed this feature from bootefi. >>>>> >>>>> Thanks, >>>>> -Takahiro Akashi >>>>> >>>>> >>>>>> Best regards >>>>>> >>>>>> Heinrich >>>>>> >>>>>>> "Boots an EFI payload from memory", >>>>>>> bootefi_help_text >>>>>>> ); >>>>>>> >>>>>> >>>>> >>>> >>> >> >
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 3be01b49b589..241fd0f987ab 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -471,16 +471,15 @@ static efi_status_t bootefi_test_prepare #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */ -static int do_bootefi_bootmgr_exec(void) +static int do_bootefi_bootmgr_exec(int boot_id) { struct efi_device_path *device_path, *file_path; void *addr; efi_status_t r; - addr = efi_bootmgr_load(EFI_BOOTMGR_DEFAULT_ORDER, - &device_path, &file_path); + addr = efi_bootmgr_load(boot_id, &device_path, &file_path); if (!addr) - return 1; + return CMD_RET_FAILURE; printf("## Starting EFI application at %p ...\n", addr); r = do_bootefi_exec(addr, device_path, file_path); @@ -488,9 +487,9 @@ static int do_bootefi_bootmgr_exec(void) r & ~EFI_ERROR_MASK); if (r != EFI_SUCCESS) - return 1; + return CMD_RET_FAILURE; - return 0; + return CMD_RET_SUCCESS; } /* Interpreter command to boot an arbitrary EFI image from memory */ @@ -546,10 +545,28 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) } else #endif if (!strcmp(argv[1], "bootmgr")) { - if (efi_handle_fdt(argc > 2 ? argv[2] : NULL)) - return CMD_RET_FAILURE; + char *fdtstr, *endp; + int boot_id = EFI_BOOTMGR_DEFAULT_ORDER; + + if (argc > 2) { + fdtstr = argv[2]; + /* Special address "-" means no device tree */ + if (fdtstr[0] == '-') + fdtstr = NULL; + + r = efi_handle_fdt(fdtstr); + if (r) + return CMD_RET_FAILURE; + } + + if (argc > 3) { + boot_id = (int)simple_strtoul(argv[3], &endp, 0); + if ((argv[3] + strlen(argv[3]) != endp) || + boot_id > 0xffff) + return CMD_RET_USAGE; + } - return do_bootefi_bootmgr_exec(); + return do_bootefi_bootmgr_exec(boot_id); } else { saddr = argv[1]; @@ -590,7 +607,7 @@ static char bootefi_help_text[] = " Use environment variable efi_selftest to select a single test.\n" " Use 'setenv efi_selftest list' to enumerate all tests.\n" #endif - "bootefi bootmgr [fdt addr]\n" + "bootefi bootmgr [<fdt addr>|'-' [<boot id>]]\n" " - load and boot EFI payload based on BootOrder/BootXXXX variables.\n" "\n" " If specified, the device tree located at <fdt address> gets\n" @@ -598,7 +615,7 @@ static char bootefi_help_text[] = #endif U_BOOT_CMD( - bootefi, 3, 0, do_bootefi, + bootefi, 5, 0, do_bootefi, "Boots an EFI payload from memory", bootefi_help_text );
With this patch applied, we will be able to selectively execute an EFI application by specifying a load option, say "1" for Boot0001, "2" for Boot0002 and so on. => bootefi bootmgr <fdt addr> 1, or bootefi bootmgr - 1 Please note that BootXXXX need not be included in "BootOrder". Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- cmd/bootefi.c | 39 ++++++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 11 deletions(-)