[6/6] efi_loader: bootmgr: run an EFI application of a given load option

Message ID 20181017073207.13150-7-takahiro.akashi@linaro.org
State New
Headers show
Series
  • efi: make efi and bootmgr more usable
Related show

Commit Message

AKASHI Takahiro Oct. 17, 2018, 7:32 a.m.
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 -1 <fdt addr>

Please note that BootXXXX need not be included in "BootOrder".

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 cmd/bootefi.c                | 19 ++++++++++++++++---
 include/efi_loader.h         |  3 ++-
 lib/efi_loader/efi_bootmgr.c |  8 +++++++-
 3 files changed, 25 insertions(+), 5 deletions(-)

Comments

Alexander Graf Oct. 17, 2018, 8:43 a.m. | #1
On 17.10.18 09:32, 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 -1 <fdt addr>

I don't think -1 is very good user experience :). How about

  => bootefi bootmgr Boot0001 <fdt addr>


Alex
AKASHI Takahiro Oct. 18, 2018, 5:48 a.m. | #2
On Wed, Oct 17, 2018 at 10:43:22AM +0200, Alexander Graf wrote:
> 
> 
> On 17.10.18 09:32, 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 -1 <fdt addr>
> 
> I don't think -1 is very good user experience :). How about
>   => bootefi bootmgr Boot0001 <fdt addr>

It looks like u-boot's run command with six more characters!
How about this:

 => bootefi bootmgr #1 <fdt addr>

or allowing "-" as empty fdt,

 => bootefi bootmgr - 1

Otherwise, a new sub command?

 => bootefi run 1, or
 => efi(shell) run 1

# Discussing UI is a fun or mess.

Thanks,
-Takahiro Akashi

> 
> Alex
Alexander Graf Oct. 18, 2018, 8:46 a.m. | #3
On 18.10.18 07:48, AKASHI Takahiro wrote:
> On Wed, Oct 17, 2018 at 10:43:22AM +0200, Alexander Graf wrote:
>>
>>
>> On 17.10.18 09:32, 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 -1 <fdt addr>
>>
>> I don't think -1 is very good user experience :). How about
>>   => bootefi bootmgr Boot0001 <fdt addr>
> 
> It looks like u-boot's run command with six more characters!
> How about this:
> 
>  => bootefi bootmgr #1 <fdt addr>

So what is the problem with making it Boot0001? That way at least the
variable name is consistent across the board ;).

> or allowing "-" as empty fdt,
> 
>  => bootefi bootmgr - 1
> 
> Otherwise, a new sub command?
> 
>  => bootefi run 1, or
>  => efi(shell) run 1
> 
> # Discussing UI is a fun or mess.

Yeah :(. What we really need would be that "bootefi bootmgr" becomes
"efiboot". But that would be even more confusing ;).

So the whole rationale of why "bootefi" is the way it is today is that
it's trying to lean on the existing "bootm", "booti", "bootz" etc syntax
as much as it can. In other words, it's trying to fit into the U-Boot
ecosystem much rather than the existing edk2 one.

I would like to keep following that path going forward. Whenever there
is an option between "U-Boot like" and "edk2 like" I would always opt
for the "U-Boot like" user experience, because if they want edk2 they
may as well use edk2 ;).


Alex
AKASHI Takahiro Oct. 22, 2018, 5:37 a.m. | #4
On Thu, Oct 18, 2018 at 10:46:36AM +0200, Alexander Graf wrote:
> 
> 
> On 18.10.18 07:48, AKASHI Takahiro wrote:
> > On Wed, Oct 17, 2018 at 10:43:22AM +0200, Alexander Graf wrote:
> >>
> >>
> >> On 17.10.18 09:32, 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 -1 <fdt addr>
> >>
> >> I don't think -1 is very good user experience :). How about
> >>   => bootefi bootmgr Boot0001 <fdt addr>
> > 
> > It looks like u-boot's run command with six more characters!
> > How about this:
> > 
> >  => bootefi bootmgr #1 <fdt addr>
> 
> So what is the problem with making it Boot0001? That way at least the
> variable name is consistent across the board ;).

More typing!

> > or allowing "-" as empty fdt,
> > 
> >  => bootefi bootmgr - 1

(Please notice that this is NOT "-1.")
I also like this one as it maintains upward-compatibility:
    bootefi bootmgr [<fdt addr> [<boot id>]]

> > Otherwise, a new sub command?
> > 
> >  => bootefi run 1, or
> >  => efi(shell) run 1

Well, if you stick to "setenv -e"-like syntax, how about
    => run -e Boot0001

Given that "run" takes an environment variable, this syntax
is perfectly fit to U-boot, isn't it?

> > # Discussing UI is a fun or mess.

# I hope that this is not fruitless discussion.

> Yeah :(. What we really need would be that "bootefi bootmgr" becomes
> "efiboot". But that would be even more confusing ;).

So I think that we should not add anything more to "bootefi bootmgr"
to extend functionality.

> So the whole rationale of why "bootefi" is the way it is today is that
> it's trying to lean on the existing "bootm", "booti", "bootz" etc syntax
> as much as it can. In other words, it's trying to fit into the U-Boot
> ecosystem much rather than the existing edk2 one.

IMO, "boot*" variants are already a mess.

> I would like to keep following that path going forward. Whenever there
> is an option between "U-Boot like" and "edk2 like" I would always opt
> for the "U-Boot like" user experience, because if they want edk2 they
> may as well use edk2 ;).

Well, BootXXXX is quite UEFI-specific and people who are not familiar
with UEFI need to consult UEFI specification anyway and this means, to me,
that UEFI shell's similarity would be favorable.
(See "setvar" syntax, not mine but UEFI shell's, which can be
quite different and complicated.)

Does anybody else have any opinions?

Thanks,
-Takahiro Akashi

> 
> Alex
Alexander Graf Oct. 22, 2018, 6:58 a.m. | #5
On 22.10.18 06:37, AKASHI Takahiro wrote:
> On Thu, Oct 18, 2018 at 10:46:36AM +0200, Alexander Graf wrote:
>>
>>
>> On 18.10.18 07:48, AKASHI Takahiro wrote:
>>> On Wed, Oct 17, 2018 at 10:43:22AM +0200, Alexander Graf wrote:
>>>>
>>>>
>>>> On 17.10.18 09:32, 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 -1 <fdt addr>
>>>>
>>>> I don't think -1 is very good user experience :). How about
>>>>   => bootefi bootmgr Boot0001 <fdt addr>
>>>
>>> It looks like u-boot's run command with six more characters!
>>> How about this:
>>>
>>>  => bootefi bootmgr #1 <fdt addr>
>>
>> So what is the problem with making it Boot0001? That way at least the
>> variable name is consistent across the board ;).
> 
> More typing!
> 
>>> or allowing "-" as empty fdt,
>>>
>>>  => bootefi bootmgr - 1
> 
> (Please notice that this is NOT "-1.")
> I also like this one as it maintains upward-compatibility:
>     bootefi bootmgr [<fdt addr> [<boot id>]]
> 
>>> Otherwise, a new sub command?
>>>
>>>  => bootefi run 1, or
>>>  => efi(shell) run 1
> 
> Well, if you stick to "setenv -e"-like syntax, how about
>     => run -e Boot0001
> 
> Given that "run" takes an environment variable, this syntax
> is perfectly fit to U-boot, isn't it?

Ooooh, that is an amazing suggestion! And "run -e" without an explicit
target could just invoke efibootmgr directly, looping through the BootOrder.

> 
>>> # Discussing UI is a fun or mess.
> 
> # I hope that this is not fruitless discussion.
> 
>> Yeah :(. What we really need would be that "bootefi bootmgr" becomes
>> "efiboot". But that would be even more confusing ;).
> 
> So I think that we should not add anything more to "bootefi bootmgr"
> to extend functionality.
> 
>> So the whole rationale of why "bootefi" is the way it is today is that
>> it's trying to lean on the existing "bootm", "booti", "bootz" etc syntax
>> as much as it can. In other words, it's trying to fit into the U-Boot
>> ecosystem much rather than the existing edk2 one.
> 
> IMO, "boot*" variants are already a mess.
> 
>> I would like to keep following that path going forward. Whenever there
>> is an option between "U-Boot like" and "edk2 like" I would always opt
>> for the "U-Boot like" user experience, because if they want edk2 they
>> may as well use edk2 ;).
> 
> Well, BootXXXX is quite UEFI-specific and people who are not familiar
> with UEFI need to consult UEFI specification anyway and this means, to me,
> that UEFI shell's similarity would be favorable.
> (See "setvar" syntax, not mine but UEFI shell's, which can be
> quite different and complicated.)

Well my thinking there is that if someone likes the UEFI Shell, they may
as well just run it :).


Alex

> 
> Does anybody else have any opinions?
> 
> Thanks,
> -Takahiro Akashi
> 
>>
>> Alex
AKASHI Takahiro Oct. 23, 2018, 3:18 a.m. | #6
On Mon, Oct 22, 2018 at 07:58:29AM +0100, Alexander Graf wrote:
> 
> 
> On 22.10.18 06:37, AKASHI Takahiro wrote:
> > On Thu, Oct 18, 2018 at 10:46:36AM +0200, Alexander Graf wrote:
> >>
> >>
> >> On 18.10.18 07:48, AKASHI Takahiro wrote:
> >>> On Wed, Oct 17, 2018 at 10:43:22AM +0200, Alexander Graf wrote:
> >>>>
> >>>>
> >>>> On 17.10.18 09:32, 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 -1 <fdt addr>
> >>>>
> >>>> I don't think -1 is very good user experience :). How about
> >>>>   => bootefi bootmgr Boot0001 <fdt addr>
> >>>
> >>> It looks like u-boot's run command with six more characters!
> >>> How about this:
> >>>
> >>>  => bootefi bootmgr #1 <fdt addr>
> >>
> >> So what is the problem with making it Boot0001? That way at least the
> >> variable name is consistent across the board ;).
> > 
> > More typing!
> > 
> >>> or allowing "-" as empty fdt,
> >>>
> >>>  => bootefi bootmgr - 1
> > 
> > (Please notice that this is NOT "-1.")
> > I also like this one as it maintains upward-compatibility:
> >     bootefi bootmgr [<fdt addr> [<boot id>]]
> > 
> >>> Otherwise, a new sub command?
> >>>
> >>>  => bootefi run 1, or
> >>>  => efi(shell) run 1
> > 
> > Well, if you stick to "setenv -e"-like syntax, how about
> >     => run -e Boot0001
> > 
> > Given that "run" takes an environment variable, this syntax
> > is perfectly fit to U-boot, isn't it?
> 
> Ooooh, that is an amazing suggestion! And "run -e" without an explicit
> target could just invoke efibootmgr directly, looping through the BootOrder.

We agree here :)

> > 
> >>> # Discussing UI is a fun or mess.
> > 
> > # I hope that this is not fruitless discussion.
> > 
> >> Yeah :(. What we really need would be that "bootefi bootmgr" becomes
> >> "efiboot". But that would be even more confusing ;).
> > 
> > So I think that we should not add anything more to "bootefi bootmgr"
> > to extend functionality.
> > 
> >> So the whole rationale of why "bootefi" is the way it is today is that
> >> it's trying to lean on the existing "bootm", "booti", "bootz" etc syntax
> >> as much as it can. In other words, it's trying to fit into the U-Boot
> >> ecosystem much rather than the existing edk2 one.
> > 
> > IMO, "boot*" variants are already a mess.
> > 
> >> I would like to keep following that path going forward. Whenever there
> >> is an option between "U-Boot like" and "edk2 like" I would always opt
> >> for the "U-Boot like" user experience, because if they want edk2 they
> >> may as well use edk2 ;).
> > 
> > Well, BootXXXX is quite UEFI-specific and people who are not familiar
> > with UEFI need to consult UEFI specification anyway and this means, to me,
> > that UEFI shell's similarity would be favorable.
> > (See "setvar" syntax, not mine but UEFI shell's, which can be
> > quite different and complicated.)
> 
> Well my thinking there is that if someone likes the UEFI Shell, they may
> as well just run it :).

My aim here is to provide poor man's uefi shell!
For example, I think there are a few useful commands to be supported:
* devices
* devtree
* dh
* (dis)connect
* drivers
* memmap
* unload
They must be useful even now for debugging and understanding
internal status of UEFI environment.
# Please note that some of those commands in edk2's shell
  will still cause a panic.

That's is why I want to have efi(shell) command.

Thanks,
-Takahiro Akashi

> 
> Alex
> 
> > 
> > Does anybody else have any opinions?
> > 
> > Thanks,
> > -Takahiro Akashi
> > 
> >>
> >> Alex

Patch

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 5772f7422e5f..434a6a07c26a 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -508,7 +508,7 @@  exit:
 	return ret;
 }
 
-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;
@@ -520,7 +520,7 @@  static int do_bootefi_bootmgr_exec(void)
 	 */
 	efi_save_gd();
 
-	addr = efi_bootmgr_load(&device_path, &file_path);
+	addr = efi_bootmgr_load(boot_id, &device_path, &file_path);
 	if (!addr)
 		return 1;
 
@@ -605,6 +605,19 @@  static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	} else
 #endif
 	if (!strcmp(argv[1], "bootmgr")) {
+		char *endp;
+		int boot_id = -1;
+
+		if ((argc > 2) && (argv[2][0] == '-')) {
+			boot_id = (int)simple_strtoul(&argv[2][1], &endp, 0);
+			if ((argv[2] + strlen(argv[2]) != endp) ||
+			    (boot_id > 0xffff))
+				return CMD_RET_USAGE;
+
+			argc--;
+			argv++;
+		}
+
 		if (efi_handle_fdt(argc > 2 ? argv[2] : NULL))
 			return CMD_RET_FAILURE;
 
@@ -649,7 +662,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 [-XXXX] [fdt addr]\n"
 	"  - load and boot EFI payload based on BootOrder/BootXXXX variables.\n"
 	"\n"
 	"    If specified, the device tree located at <fdt address> gets\n"
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 1cabb1680d20..5804c2b5015d 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -527,7 +527,8 @@  void efi_parse_load_option(struct load_option *lo, void *ptr);
 unsigned long efi_marshal_load_option(u32 attr, u16 *label,
 				      struct efi_device_path *file_path,
 				      char *option, void **data);
-void *efi_bootmgr_load(struct efi_device_path **device_path,
+void *efi_bootmgr_load(int boot_id,
+		       struct efi_device_path **device_path,
 		       struct efi_device_path **file_path);
 
 #else /* CONFIG_IS_ENABLED(EFI_LOADER) */
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index c2d29f956065..348f99a9ca25 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -165,7 +165,8 @@  error:
  * available load-options, finding and returning the first one that can
  * be loaded successfully.
  */
-void *efi_bootmgr_load(struct efi_device_path **device_path,
+void *efi_bootmgr_load(int boot_id,
+		       struct efi_device_path **device_path,
 		       struct efi_device_path **file_path)
 {
 	uint16_t *bootorder;
@@ -178,6 +179,11 @@  void *efi_bootmgr_load(struct efi_device_path **device_path,
 	bs = systab.boottime;
 	rs = systab.runtime;
 
+	if (boot_id != -1) {
+		image = try_load_entry(boot_id, device_path, file_path);
+		goto error;
+	}
+
 	bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size);
 	if (!bootorder)
 		goto error;