[5/5] cmd: run: add "-e" option to run an EFI application

Message ID 20181218050257.20142-6-takahiro.akashi@linaro.org
State New
Headers show
Series
  • efi_loader: run a specific efi application more easily
Related show

Commit Message

AKASHI Takahiro Dec. 18, 2018, 5:02 a.m.
"run -e" allows for executing EFI application with a specific "BootXXXX"
variable. If no "BootXXXX" is specified or "BootOrder" is specified,
it tries to run an EFI application specified in the order of "bootOrder."

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 cmd/bootefi.c     |  2 +-
 cmd/nvedit.c      |  5 +++++
 common/cli.c      | 31 +++++++++++++++++++++++++++++++
 include/command.h |  3 +++
 4 files changed, 40 insertions(+), 1 deletion(-)

Comments

Alexander Graf Dec. 23, 2018, 2:19 a.m. | #1
On 18.12.18 06:02, AKASHI Takahiro wrote:
> "run -e" allows for executing EFI application with a specific "BootXXXX"
> variable. If no "BootXXXX" is specified or "BootOrder" is specified,
> it tries to run an EFI application specified in the order of "bootOrder."
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  cmd/bootefi.c     |  2 +-
>  cmd/nvedit.c      |  5 +++++
>  common/cli.c      | 31 +++++++++++++++++++++++++++++++
>  include/command.h |  3 +++
>  4 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 2fc52e3056d2..8122793d11c5 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -471,7 +471,7 @@ static efi_status_t bootefi_test_prepare
>  
>  #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
>  
> -static int do_bootefi_bootmgr_exec(int boot_id)
> +int do_bootefi_bootmgr_exec(int boot_id)
>  {
>  	struct efi_device_path *device_path, *file_path;
>  	void *addr;
> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> index de16c72c23f2..c0facabfc4fe 100644
> --- a/cmd/nvedit.c
> +++ b/cmd/nvedit.c
> @@ -1343,8 +1343,13 @@ U_BOOT_CMD(
>  U_BOOT_CMD_COMPLETE(
>  	run,	CONFIG_SYS_MAXARGS,	1,	do_run,
>  	"run commands in an environment variable",
> +#if defined(CONFIG_CMD_BOOTEFI)
> +	"var -e [BootXXXX]\n"
> +	"    - load and run UEFI app based on 'BootXXXX' UEFI variable",
> +#else
>  	"var [...]\n"
>  	"    - run the commands in the environment variable(s) 'var'",
> +#endif
>  	var_complete
>  );
>  #endif
> diff --git a/common/cli.c b/common/cli.c
> index 51b8d5f85cbb..013dd2e51936 100644
> --- a/common/cli.c
> +++ b/common/cli.c
> @@ -12,8 +12,10 @@
>  #include <cli.h>
>  #include <cli_hush.h>
>  #include <console.h>
> +#include <efi_loader.h>
>  #include <fdtdec.h>
>  #include <malloc.h>
> +#include "../cmd/bootefi.h"
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> @@ -125,6 +127,35 @@ int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  	if (argc < 2)
>  		return CMD_RET_USAGE;
>  
> +#ifdef CONFIG_CMD_BOOTEFI
> +	if (!strcmp(argv[1], "-e")) {
> +		int boot_id = -1;
> +		char *endp;
> +
> +		if (argc == 3) {
> +			if (!strcmp(argv[2], "BootOrder")) {
> +				boot_id = -1;
> +			} else if (!strncmp(argv[2], "Boot", 4)) {
> +				boot_id = (int)simple_strtoul(&argv[2][4],
> +							      &endp, 0);
> +				if ((argv[2] + strlen(argv[2]) != endp) ||
> +				    boot_id > 0xffff)
> +					return CMD_RET_USAGE;

This duplicates the same logic you added to bootefi.c. Better reuse it.
I guess you can just call a function inside bootefi.c from here if you
detect -e:

  if (!strcmp(argv[1], "-e"))
    return do_bootefi_run(cmdtp, flag, argc, argv);

and just handle it all inside bootefi.c at that point.

> +			} else {
> +				return CMD_RET_USAGE;
> +			}
> +		}
> +
> +		if (efi_init_obj_list())
> +			return CMD_RET_FAILURE;
> +
> +		if (efi_handle_fdt(NULL))
> +			return CMD_RET_FAILURE;
> +
> +		return do_bootefi_bootmgr_exec(boot_id);
> +	}
> +#endif
> +
>  	for (i = 1; i < argc; ++i) {
>  		char *arg;
>  
> diff --git a/include/command.h b/include/command.h
> index 200c7a5e9f4e..9b7b876585d9 100644
> --- a/include/command.h
> +++ b/include/command.h
> @@ -48,6 +48,9 @@ typedef struct cmd_tbl_s	cmd_tbl_t;
>  #if defined(CONFIG_CMD_RUN)
>  extern int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
>  #endif
> +#if defined(CONFIG_CMD_BOOTEFI)
> +int do_bootefi_bootmgr_exec(int boot_id);
> +#endif

I would prefer if nvedit.c includes efi_loader.h and we define it there?


Alex
AKASHI Takahiro Dec. 25, 2018, 11:29 a.m. | #2
On Sun, Dec 23, 2018 at 03:19:17AM +0100, Alexander Graf wrote:
> 
> 
> On 18.12.18 06:02, AKASHI Takahiro wrote:
> > "run -e" allows for executing EFI application with a specific "BootXXXX"
> > variable. If no "BootXXXX" is specified or "BootOrder" is specified,
> > it tries to run an EFI application specified in the order of "bootOrder."
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  cmd/bootefi.c     |  2 +-
> >  cmd/nvedit.c      |  5 +++++
> >  common/cli.c      | 31 +++++++++++++++++++++++++++++++
> >  include/command.h |  3 +++
> >  4 files changed, 40 insertions(+), 1 deletion(-)
> > 
> > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > index 2fc52e3056d2..8122793d11c5 100644
> > --- a/cmd/bootefi.c
> > +++ b/cmd/bootefi.c
> > @@ -471,7 +471,7 @@ static efi_status_t bootefi_test_prepare
> >  
> >  #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
> >  
> > -static int do_bootefi_bootmgr_exec(int boot_id)
> > +int do_bootefi_bootmgr_exec(int boot_id)
> >  {
> >  	struct efi_device_path *device_path, *file_path;
> >  	void *addr;
> > diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> > index de16c72c23f2..c0facabfc4fe 100644
> > --- a/cmd/nvedit.c
> > +++ b/cmd/nvedit.c
> > @@ -1343,8 +1343,13 @@ U_BOOT_CMD(
> >  U_BOOT_CMD_COMPLETE(
> >  	run,	CONFIG_SYS_MAXARGS,	1,	do_run,
> >  	"run commands in an environment variable",
> > +#if defined(CONFIG_CMD_BOOTEFI)
> > +	"var -e [BootXXXX]\n"
> > +	"    - load and run UEFI app based on 'BootXXXX' UEFI variable",
> > +#else
> >  	"var [...]\n"
> >  	"    - run the commands in the environment variable(s) 'var'",
> > +#endif
> >  	var_complete
> >  );
> >  #endif
> > diff --git a/common/cli.c b/common/cli.c
> > index 51b8d5f85cbb..013dd2e51936 100644
> > --- a/common/cli.c
> > +++ b/common/cli.c
> > @@ -12,8 +12,10 @@
> >  #include <cli.h>
> >  #include <cli_hush.h>
> >  #include <console.h>
> > +#include <efi_loader.h>
> >  #include <fdtdec.h>
> >  #include <malloc.h>
> > +#include "../cmd/bootefi.h"
> >  
> >  DECLARE_GLOBAL_DATA_PTR;
> >  
> > @@ -125,6 +127,35 @@ int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >  	if (argc < 2)
> >  		return CMD_RET_USAGE;
> >  
> > +#ifdef CONFIG_CMD_BOOTEFI
> > +	if (!strcmp(argv[1], "-e")) {
> > +		int boot_id = -1;
> > +		char *endp;
> > +
> > +		if (argc == 3) {
> > +			if (!strcmp(argv[2], "BootOrder")) {
> > +				boot_id = -1;
> > +			} else if (!strncmp(argv[2], "Boot", 4)) {
> > +				boot_id = (int)simple_strtoul(&argv[2][4],
> > +							      &endp, 0);
> > +				if ((argv[2] + strlen(argv[2]) != endp) ||
> > +				    boot_id > 0xffff)
> > +					return CMD_RET_USAGE;
> 
> This duplicates the same logic you added to bootefi.c. Better reuse it.
> I guess you can just call a function inside bootefi.c from here if you
> detect -e:
> 
>   if (!strcmp(argv[1], "-e"))
>     return do_bootefi_run(cmdtp, flag, argc, argv);
> 
> and just handle it all inside bootefi.c at that point.

Hmm, OK.
In this case, the command syntax of bootmgr will be changed so as to
accept "BootXXXX" instead of just an integer (as boot id).

Thanks,
-Takahiro Akashi

> > +			} else {
> > +				return CMD_RET_USAGE;
> > +			}
> > +		}
> > +
> > +		if (efi_init_obj_list())
> > +			return CMD_RET_FAILURE;
> > +
> > +		if (efi_handle_fdt(NULL))
> > +			return CMD_RET_FAILURE;
> > +
> > +		return do_bootefi_bootmgr_exec(boot_id);
> > +	}
> > +#endif
> > +
> >  	for (i = 1; i < argc; ++i) {
> >  		char *arg;
> >  
> > diff --git a/include/command.h b/include/command.h
> > index 200c7a5e9f4e..9b7b876585d9 100644
> > --- a/include/command.h
> > +++ b/include/command.h
> > @@ -48,6 +48,9 @@ typedef struct cmd_tbl_s	cmd_tbl_t;
> >  #if defined(CONFIG_CMD_RUN)
> >  extern int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
> >  #endif
> > +#if defined(CONFIG_CMD_BOOTEFI)
> > +int do_bootefi_bootmgr_exec(int boot_id);
> > +#endif
> 
> I would prefer if nvedit.c includes efi_loader.h and we define it there?
> 
> 
> Alex
Alexander Graf Dec. 26, 2018, 9:24 p.m. | #3
On 25.12.18 12:29, AKASHI Takahiro wrote:
> On Sun, Dec 23, 2018 at 03:19:17AM +0100, Alexander Graf wrote:
>>
>>
>> On 18.12.18 06:02, AKASHI Takahiro wrote:
>>> "run -e" allows for executing EFI application with a specific "BootXXXX"
>>> variable. If no "BootXXXX" is specified or "BootOrder" is specified,
>>> it tries to run an EFI application specified in the order of "bootOrder."
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>> ---
>>>  cmd/bootefi.c     |  2 +-
>>>  cmd/nvedit.c      |  5 +++++
>>>  common/cli.c      | 31 +++++++++++++++++++++++++++++++
>>>  include/command.h |  3 +++
>>>  4 files changed, 40 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>> index 2fc52e3056d2..8122793d11c5 100644
>>> --- a/cmd/bootefi.c
>>> +++ b/cmd/bootefi.c
>>> @@ -471,7 +471,7 @@ static efi_status_t bootefi_test_prepare
>>>  
>>>  #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
>>>  
>>> -static int do_bootefi_bootmgr_exec(int boot_id)
>>> +int do_bootefi_bootmgr_exec(int boot_id)
>>>  {
>>>  	struct efi_device_path *device_path, *file_path;
>>>  	void *addr;
>>> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
>>> index de16c72c23f2..c0facabfc4fe 100644
>>> --- a/cmd/nvedit.c
>>> +++ b/cmd/nvedit.c
>>> @@ -1343,8 +1343,13 @@ U_BOOT_CMD(
>>>  U_BOOT_CMD_COMPLETE(
>>>  	run,	CONFIG_SYS_MAXARGS,	1,	do_run,
>>>  	"run commands in an environment variable",
>>> +#if defined(CONFIG_CMD_BOOTEFI)
>>> +	"var -e [BootXXXX]\n"
>>> +	"    - load and run UEFI app based on 'BootXXXX' UEFI variable",
>>> +#else
>>>  	"var [...]\n"
>>>  	"    - run the commands in the environment variable(s) 'var'",
>>> +#endif
>>>  	var_complete
>>>  );
>>>  #endif
>>> diff --git a/common/cli.c b/common/cli.c
>>> index 51b8d5f85cbb..013dd2e51936 100644
>>> --- a/common/cli.c
>>> +++ b/common/cli.c
>>> @@ -12,8 +12,10 @@
>>>  #include <cli.h>
>>>  #include <cli_hush.h>
>>>  #include <console.h>
>>> +#include <efi_loader.h>
>>>  #include <fdtdec.h>
>>>  #include <malloc.h>
>>> +#include "../cmd/bootefi.h"
>>>  
>>>  DECLARE_GLOBAL_DATA_PTR;
>>>  
>>> @@ -125,6 +127,35 @@ int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>>  	if (argc < 2)
>>>  		return CMD_RET_USAGE;
>>>  
>>> +#ifdef CONFIG_CMD_BOOTEFI
>>> +	if (!strcmp(argv[1], "-e")) {
>>> +		int boot_id = -1;
>>> +		char *endp;
>>> +
>>> +		if (argc == 3) {
>>> +			if (!strcmp(argv[2], "BootOrder")) {
>>> +				boot_id = -1;
>>> +			} else if (!strncmp(argv[2], "Boot", 4)) {
>>> +				boot_id = (int)simple_strtoul(&argv[2][4],
>>> +							      &endp, 0);
>>> +				if ((argv[2] + strlen(argv[2]) != endp) ||
>>> +				    boot_id > 0xffff)
>>> +					return CMD_RET_USAGE;
>>
>> This duplicates the same logic you added to bootefi.c. Better reuse it.
>> I guess you can just call a function inside bootefi.c from here if you
>> detect -e:
>>
>>   if (!strcmp(argv[1], "-e"))
>>     return do_bootefi_run(cmdtp, flag, argc, argv);
>>
>> and just handle it all inside bootefi.c at that point.
> 
> Hmm, OK.
> In this case, the command syntax of bootmgr will be changed so as to
> accept "BootXXXX" instead of just an integer (as boot id).

Not necessarily. You can just move all the code you have here into a new
function in bootefi.c. The parsing logic really should just be shared.


Alex
AKASHI Takahiro Jan. 7, 2019, 7:40 a.m. | #4
On Wed, Dec 26, 2018 at 10:24:45PM +0100, Alexander Graf wrote:
> 
> 
> On 25.12.18 12:29, AKASHI Takahiro wrote:
> > On Sun, Dec 23, 2018 at 03:19:17AM +0100, Alexander Graf wrote:
> >>
> >>
> >> On 18.12.18 06:02, AKASHI Takahiro wrote:
> >>> "run -e" allows for executing EFI application with a specific "BootXXXX"
> >>> variable. If no "BootXXXX" is specified or "BootOrder" is specified,
> >>> it tries to run an EFI application specified in the order of "bootOrder."
> >>>
> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>> ---
> >>>  cmd/bootefi.c     |  2 +-
> >>>  cmd/nvedit.c      |  5 +++++
> >>>  common/cli.c      | 31 +++++++++++++++++++++++++++++++
> >>>  include/command.h |  3 +++
> >>>  4 files changed, 40 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >>> index 2fc52e3056d2..8122793d11c5 100644
> >>> --- a/cmd/bootefi.c
> >>> +++ b/cmd/bootefi.c
> >>> @@ -471,7 +471,7 @@ static efi_status_t bootefi_test_prepare
> >>>  
> >>>  #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
> >>>  
> >>> -static int do_bootefi_bootmgr_exec(int boot_id)
> >>> +int do_bootefi_bootmgr_exec(int boot_id)
> >>>  {
> >>>  	struct efi_device_path *device_path, *file_path;
> >>>  	void *addr;
> >>> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> >>> index de16c72c23f2..c0facabfc4fe 100644
> >>> --- a/cmd/nvedit.c
> >>> +++ b/cmd/nvedit.c
> >>> @@ -1343,8 +1343,13 @@ U_BOOT_CMD(
> >>>  U_BOOT_CMD_COMPLETE(
> >>>  	run,	CONFIG_SYS_MAXARGS,	1,	do_run,
> >>>  	"run commands in an environment variable",
> >>> +#if defined(CONFIG_CMD_BOOTEFI)
> >>> +	"var -e [BootXXXX]\n"
> >>> +	"    - load and run UEFI app based on 'BootXXXX' UEFI variable",
> >>> +#else
> >>>  	"var [...]\n"
> >>>  	"    - run the commands in the environment variable(s) 'var'",
> >>> +#endif
> >>>  	var_complete
> >>>  );
> >>>  #endif
> >>> diff --git a/common/cli.c b/common/cli.c
> >>> index 51b8d5f85cbb..013dd2e51936 100644
> >>> --- a/common/cli.c
> >>> +++ b/common/cli.c
> >>> @@ -12,8 +12,10 @@
> >>>  #include <cli.h>
> >>>  #include <cli_hush.h>
> >>>  #include <console.h>
> >>> +#include <efi_loader.h>
> >>>  #include <fdtdec.h>
> >>>  #include <malloc.h>
> >>> +#include "../cmd/bootefi.h"
> >>>  
> >>>  DECLARE_GLOBAL_DATA_PTR;
> >>>  
> >>> @@ -125,6 +127,35 @@ int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >>>  	if (argc < 2)
> >>>  		return CMD_RET_USAGE;
> >>>  
> >>> +#ifdef CONFIG_CMD_BOOTEFI
> >>> +	if (!strcmp(argv[1], "-e")) {
> >>> +		int boot_id = -1;
> >>> +		char *endp;
> >>> +
> >>> +		if (argc == 3) {
> >>> +			if (!strcmp(argv[2], "BootOrder")) {
> >>> +				boot_id = -1;
> >>> +			} else if (!strncmp(argv[2], "Boot", 4)) {
> >>> +				boot_id = (int)simple_strtoul(&argv[2][4],
> >>> +							      &endp, 0);
> >>> +				if ((argv[2] + strlen(argv[2]) != endp) ||
> >>> +				    boot_id > 0xffff)
> >>> +					return CMD_RET_USAGE;
> >>
> >> This duplicates the same logic you added to bootefi.c. Better reuse it.
> >> I guess you can just call a function inside bootefi.c from here if you
> >> detect -e:
> >>
> >>   if (!strcmp(argv[1], "-e"))
> >>     return do_bootefi_run(cmdtp, flag, argc, argv);
> >>
> >> and just handle it all inside bootefi.c at that point.
> > 
> > Hmm, OK.
> > In this case, the command syntax of bootmgr will be changed so as to
> > accept "BootXXXX" instead of just an integer (as boot id).
> 
> Not necessarily. You can just move all the code you have here into a new
> function in bootefi.c. The parsing logic really should just be shared.

Ah ha, OK.

-Takahiro Akashi

> 
> Alex

Patch

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 2fc52e3056d2..8122793d11c5 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -471,7 +471,7 @@  static efi_status_t bootefi_test_prepare
 
 #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
 
-static int do_bootefi_bootmgr_exec(int boot_id)
+int do_bootefi_bootmgr_exec(int boot_id)
 {
 	struct efi_device_path *device_path, *file_path;
 	void *addr;
diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index de16c72c23f2..c0facabfc4fe 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -1343,8 +1343,13 @@  U_BOOT_CMD(
 U_BOOT_CMD_COMPLETE(
 	run,	CONFIG_SYS_MAXARGS,	1,	do_run,
 	"run commands in an environment variable",
+#if defined(CONFIG_CMD_BOOTEFI)
+	"var -e [BootXXXX]\n"
+	"    - load and run UEFI app based on 'BootXXXX' UEFI variable",
+#else
 	"var [...]\n"
 	"    - run the commands in the environment variable(s) 'var'",
+#endif
 	var_complete
 );
 #endif
diff --git a/common/cli.c b/common/cli.c
index 51b8d5f85cbb..013dd2e51936 100644
--- a/common/cli.c
+++ b/common/cli.c
@@ -12,8 +12,10 @@ 
 #include <cli.h>
 #include <cli_hush.h>
 #include <console.h>
+#include <efi_loader.h>
 #include <fdtdec.h>
 #include <malloc.h>
+#include "../cmd/bootefi.h"
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -125,6 +127,35 @@  int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	if (argc < 2)
 		return CMD_RET_USAGE;
 
+#ifdef CONFIG_CMD_BOOTEFI
+	if (!strcmp(argv[1], "-e")) {
+		int boot_id = -1;
+		char *endp;
+
+		if (argc == 3) {
+			if (!strcmp(argv[2], "BootOrder")) {
+				boot_id = -1;
+			} else if (!strncmp(argv[2], "Boot", 4)) {
+				boot_id = (int)simple_strtoul(&argv[2][4],
+							      &endp, 0);
+				if ((argv[2] + strlen(argv[2]) != endp) ||
+				    boot_id > 0xffff)
+					return CMD_RET_USAGE;
+			} else {
+				return CMD_RET_USAGE;
+			}
+		}
+
+		if (efi_init_obj_list())
+			return CMD_RET_FAILURE;
+
+		if (efi_handle_fdt(NULL))
+			return CMD_RET_FAILURE;
+
+		return do_bootefi_bootmgr_exec(boot_id);
+	}
+#endif
+
 	for (i = 1; i < argc; ++i) {
 		char *arg;
 
diff --git a/include/command.h b/include/command.h
index 200c7a5e9f4e..9b7b876585d9 100644
--- a/include/command.h
+++ b/include/command.h
@@ -48,6 +48,9 @@  typedef struct cmd_tbl_s	cmd_tbl_t;
 #if defined(CONFIG_CMD_RUN)
 extern int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
 #endif
+#if defined(CONFIG_CMD_BOOTEFI)
+int do_bootefi_bootmgr_exec(int boot_id);
+#endif
 
 /* common/command.c */
 int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int