diff mbox series

[v2,05/14] cmd: efishell: add devices command

Message ID 20181105090653.7409-6-takahiro.akashi@linaro.org
State Superseded
Headers show
Series efi: make efi and bootmgr more usable | expand

Commit Message

AKASHI Takahiro Nov. 5, 2018, 9:06 a.m. UTC 
"devices" command prints all the uefi variables on the system.
=> efishell devices
Device Name

Comments

Alexander Graf Dec. 2, 2018, 11:46 p.m. UTC | #1 
On 05.11.18 10:06, AKASHI Takahiro wrote:
> "devices" command prints all the uefi variables on the system.
> => efishell devices
> Device Name
> ============================================
> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)
> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/\
> 				HD(2,MBR,0x086246ba,0x40800,0x3f800)
> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/\
> 				HD(1,MBR,0x086246ba,0x800,0x40000)
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  cmd/efishell.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 87 insertions(+), 1 deletion(-)
> 
> diff --git a/cmd/efishell.c b/cmd/efishell.c
> index abc8216c7bd6..f4fa3fdf28a7 100644
> --- a/cmd/efishell.c
> +++ b/cmd/efishell.c
> @@ -21,6 +21,8 @@
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> +static const struct efi_boot_services *bs;

Why do you need a local copy of this?


Alex
AKASHI Takahiro Dec. 3, 2018, 7:02 a.m. UTC | #2 
On Mon, Dec 03, 2018 at 12:46:20AM +0100, Alexander Graf wrote:
> 
> 
> On 05.11.18 10:06, AKASHI Takahiro wrote:
> > "devices" command prints all the uefi variables on the system.
> > => efishell devices
> > Device Name
> > ============================================
> > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
> > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)
> > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/\
> > 				HD(2,MBR,0x086246ba,0x40800,0x3f800)
> > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/\
> > 				HD(1,MBR,0x086246ba,0x800,0x40000)
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  cmd/efishell.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 87 insertions(+), 1 deletion(-)
> > 
> > diff --git a/cmd/efishell.c b/cmd/efishell.c
> > index abc8216c7bd6..f4fa3fdf28a7 100644
> > --- a/cmd/efishell.c
> > +++ b/cmd/efishell.c
> > @@ -21,6 +21,8 @@
> >  
> >  DECLARE_GLOBAL_DATA_PTR;
> >  
> > +static const struct efi_boot_services *bs;
> 
> Why do you need a local copy of this?

Good point. It's because I followed the way boot manager does :)

I think that it would be good to do so since either boot manager or
efishell should ultimately be an independent efi application
in its nature.

What do you think?

FYI, one of the reasons why efishell cannot be an application
is that we lack an runtime service interface of GetNextVariableName()
which can be used to enumerate variables in dumpvar sub-command.

I also have a patch for adding GetNextVariableName() in my local dev branch.
I intend to post this patch along with capsule-on-disk support,
but I may be able to submit it separately if you like.

Thanks,
-Takahiro Akashi

> 
> Alex
Alexander Graf Dec. 23, 2018, 3:11 a.m. UTC | #3 
On 03.12.18 08:02, AKASHI Takahiro wrote:
> On Mon, Dec 03, 2018 at 12:46:20AM +0100, Alexander Graf wrote:
>>
>>
>> On 05.11.18 10:06, AKASHI Takahiro wrote:
>>> "devices" command prints all the uefi variables on the system.
>>> => efishell devices
>>> Device Name
>>> ============================================
>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)
>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/\
>>> 				HD(2,MBR,0x086246ba,0x40800,0x3f800)
>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/\
>>> 				HD(1,MBR,0x086246ba,0x800,0x40000)
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>> ---
>>>  cmd/efishell.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 87 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/cmd/efishell.c b/cmd/efishell.c
>>> index abc8216c7bd6..f4fa3fdf28a7 100644
>>> --- a/cmd/efishell.c
>>> +++ b/cmd/efishell.c
>>> @@ -21,6 +21,8 @@
>>>  
>>>  DECLARE_GLOBAL_DATA_PTR;
>>>  
>>> +static const struct efi_boot_services *bs;
>>
>> Why do you need a local copy of this?
> 
> Good point. It's because I followed the way boot manager does :)
> 
> I think that it would be good to do so since either boot manager or
> efishell should ultimately be an independent efi application
> in its nature.
> 
> What do you think?

As mentioned in the other email thread, I think that we should
definitely evaluate to add the edk2 shell as built-in option.

That way for the "full-fledged" shell experience, your built-in code is
not needed. But I still believe it would be useful for quick and
built-in debugging.


Alex
AKASHI Takahiro Dec. 25, 2018, noon UTC | #4 
On Sun, Dec 23, 2018 at 04:11:14AM +0100, Alexander Graf wrote:
> 
> 
> On 03.12.18 08:02, AKASHI Takahiro wrote:
> > On Mon, Dec 03, 2018 at 12:46:20AM +0100, Alexander Graf wrote:
> >>
> >>
> >> On 05.11.18 10:06, AKASHI Takahiro wrote:
> >>> "devices" command prints all the uefi variables on the system.
> >>> => efishell devices
> >>> Device Name
> >>> ============================================
> >>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
> >>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)
> >>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/\
> >>> 				HD(2,MBR,0x086246ba,0x40800,0x3f800)
> >>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/\
> >>> 				HD(1,MBR,0x086246ba,0x800,0x40000)
> >>>
> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>> ---
> >>>  cmd/efishell.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++-
> >>>  1 file changed, 87 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/cmd/efishell.c b/cmd/efishell.c
> >>> index abc8216c7bd6..f4fa3fdf28a7 100644
> >>> --- a/cmd/efishell.c
> >>> +++ b/cmd/efishell.c
> >>> @@ -21,6 +21,8 @@
> >>>  
> >>>  DECLARE_GLOBAL_DATA_PTR;
> >>>  
> >>> +static const struct efi_boot_services *bs;
> >>
> >> Why do you need a local copy of this?
> > 
> > Good point. It's because I followed the way boot manager does :)
> > 
> > I think that it would be good to do so since either boot manager or
> > efishell should ultimately be an independent efi application
> > in its nature.
> > 
> > What do you think?

Back to your original comment: Why do you need a local copy of this?

Do you think we should use systab.boottime,runtime instead?

> As mentioned in the other email thread, I think that we should
> definitely evaluate to add the edk2 shell as built-in option.

Do you expect that I will add a new config, say, CONFIG_EFI_SHELL_PATH? 

-Takahiro Akashi

> That way for the "full-fledged" shell experience, your built-in code is
> not needed. But I still believe it would be useful for quick and
> built-in debugging.
> 
> 
> Alex
Alexander Graf Dec. 26, 2018, 8 a.m. UTC | #5 
On 25.12.18 13:00, AKASHI Takahiro wrote:
> On Sun, Dec 23, 2018 at 04:11:14AM +0100, Alexander Graf wrote:
>>
>>
>> On 03.12.18 08:02, AKASHI Takahiro wrote:
>>> On Mon, Dec 03, 2018 at 12:46:20AM +0100, Alexander Graf wrote:
>>>>
>>>>
>>>> On 05.11.18 10:06, AKASHI Takahiro wrote:
>>>>> "devices" command prints all the uefi variables on the system.
>>>>> => efishell devices
>>>>> Device Name
>>>>> ============================================
>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)
>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/\
>>>>> 				HD(2,MBR,0x086246ba,0x40800,0x3f800)
>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/\
>>>>> 				HD(1,MBR,0x086246ba,0x800,0x40000)
>>>>>
>>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>>>> ---
>>>>>  cmd/efishell.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>>  1 file changed, 87 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/cmd/efishell.c b/cmd/efishell.c
>>>>> index abc8216c7bd6..f4fa3fdf28a7 100644
>>>>> --- a/cmd/efishell.c
>>>>> +++ b/cmd/efishell.c
>>>>> @@ -21,6 +21,8 @@
>>>>>  
>>>>>  DECLARE_GLOBAL_DATA_PTR;
>>>>>  
>>>>> +static const struct efi_boot_services *bs;
>>>>
>>>> Why do you need a local copy of this?
>>>
>>> Good point. It's because I followed the way boot manager does :)
>>>
>>> I think that it would be good to do so since either boot manager or
>>> efishell should ultimately be an independent efi application
>>> in its nature.
>>>
>>> What do you think?
> 
> Back to your original comment: Why do you need a local copy of this?
> 
> Do you think we should use systab.boottime,runtime instead?

Sure, that's one thing. Or you could make it a local variable. Or you
could even do a #define in the file. But that static const here will
most likely waste space in .bss and does not take into account when
someone patches the systab.

>> As mentioned in the other email thread, I think that we should
>> definitely evaluate to add the edk2 shell as built-in option.
> 
> Do you expect that I will add a new config, say, CONFIG_EFI_SHELL_PATH? 

Doesn't have to be you, but it might be useful to have something like
that, yes.

CONFIG_CMD_BOOTEFI_SHELL=y
CONFIG_CMD_BOOTEFI_SHELL_PATH=shell.efi

which then would work the same as CONFIG_CMD_BOOTEFI_HELLO, but simply
execute a precompiled version of the UEFI shell. IIRC the UEFI shell
also supports passing arguments via the command line (load_options ->
"bootargs" variable).

So with that you should be able to run

U-Boot# setenv bootargs memmap; bootefi shell

and get a list of the UEFI memory map.


Alex
AKASHI Takahiro Jan. 7, 2019, 8:22 a.m. UTC | #6 
On Wed, Dec 26, 2018 at 09:00:42AM +0100, Alexander Graf wrote:
> 
> 
> On 25.12.18 13:00, AKASHI Takahiro wrote:
> > On Sun, Dec 23, 2018 at 04:11:14AM +0100, Alexander Graf wrote:
> >>
> >>
> >> On 03.12.18 08:02, AKASHI Takahiro wrote:
> >>> On Mon, Dec 03, 2018 at 12:46:20AM +0100, Alexander Graf wrote:
> >>>>
> >>>>
> >>>> On 05.11.18 10:06, AKASHI Takahiro wrote:
> >>>>> "devices" command prints all the uefi variables on the system.
> >>>>> => efishell devices
> >>>>> Device Name
> >>>>> ============================================
> >>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
> >>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)
> >>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/\
> >>>>> 				HD(2,MBR,0x086246ba,0x40800,0x3f800)
> >>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/\
> >>>>> 				HD(1,MBR,0x086246ba,0x800,0x40000)
> >>>>>
> >>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>>>> ---
> >>>>>  cmd/efishell.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++-
> >>>>>  1 file changed, 87 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/cmd/efishell.c b/cmd/efishell.c
> >>>>> index abc8216c7bd6..f4fa3fdf28a7 100644
> >>>>> --- a/cmd/efishell.c
> >>>>> +++ b/cmd/efishell.c
> >>>>> @@ -21,6 +21,8 @@
> >>>>>  
> >>>>>  DECLARE_GLOBAL_DATA_PTR;
> >>>>>  
> >>>>> +static const struct efi_boot_services *bs;
> >>>>
> >>>> Why do you need a local copy of this?
> >>>
> >>> Good point. It's because I followed the way boot manager does :)
> >>>
> >>> I think that it would be good to do so since either boot manager or
> >>> efishell should ultimately be an independent efi application
> >>> in its nature.
> >>>
> >>> What do you think?
> > 
> > Back to your original comment: Why do you need a local copy of this?
> > 
> > Do you think we should use systab.boottime,runtime instead?
> 
> Sure, that's one thing. Or you could make it a local variable. Or you
> could even do a #define in the file. But that static const here will
> most likely waste space in .bss and does not take into account when
> someone patches the systab.

OK, I will add a macro definition.

-Takahiro Akashi

> >> As mentioned in the other email thread, I think that we should
> >> definitely evaluate to add the edk2 shell as built-in option.
> > 
> > Do you expect that I will add a new config, say, CONFIG_EFI_SHELL_PATH? 
> 
> Doesn't have to be you, but it might be useful to have something like
> that, yes.
> 
> CONFIG_CMD_BOOTEFI_SHELL=y
> CONFIG_CMD_BOOTEFI_SHELL_PATH=shell.efi
> 
> which then would work the same as CONFIG_CMD_BOOTEFI_HELLO, but simply
> execute a precompiled version of the UEFI shell. IIRC the UEFI shell
> also supports passing arguments via the command line (load_options ->
> "bootargs" variable).
> 
> So with that you should be able to run
> 
> U-Boot# setenv bootargs memmap; bootefi shell
> 
> and get a list of the UEFI memory map.
> 
> 
> Alex
diff mbox series

Patch

============================================
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/\
				HD(2,MBR,0x086246ba,0x40800,0x3f800)
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/\
				HD(1,MBR,0x086246ba,0x800,0x40000)

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 cmd/efishell.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 87 insertions(+), 1 deletion(-)

diff --git a/cmd/efishell.c b/cmd/efishell.c
index abc8216c7bd6..f4fa3fdf28a7 100644
--- a/cmd/efishell.c
+++ b/cmd/efishell.c
@@ -21,6 +21,8 @@ 
 
 DECLARE_GLOBAL_DATA_PTR;
 
+static const struct efi_boot_services *bs;
+
 static void dump_var_data(char *data, unsigned long len)
 {
 	char *start, *end, *p;
@@ -266,6 +268,84 @@  out:
 	return ret;
 }
 
+static efi_handle_t *efi_get_handles_by_proto(efi_guid_t *guid)
+{
+	efi_handle_t *handles = NULL;
+	efi_uintn_t size = 0;
+	efi_status_t ret;
+
+	if (guid) {
+		ret = bs->locate_handle(BY_PROTOCOL, guid, NULL, &size,
+					handles);
+		if (ret == EFI_BUFFER_TOO_SMALL) {
+			handles = calloc(1, size);
+			if (!handles)
+				return NULL;
+
+			ret = bs->locate_handle(BY_PROTOCOL, guid, NULL, &size,
+						handles);
+		}
+		if (ret != EFI_SUCCESS) {
+			free(handles);
+			return NULL;
+		}
+	} else {
+		ret = bs->locate_handle(ALL_HANDLES, NULL, NULL, &size, NULL);
+		if (ret == EFI_BUFFER_TOO_SMALL) {
+			handles = calloc(1, size);
+			if (!handles)
+				return NULL;
+
+			ret = bs->locate_handle(ALL_HANDLES, NULL, NULL, &size,
+						handles);
+		}
+		if (ret != EFI_SUCCESS) {
+			free(handles);
+			return NULL;
+		}
+	}
+
+	return handles;
+}
+
+static int efi_get_device_handle_info(efi_handle_t handle, u16 **name)
+{
+	struct efi_device_path *dp;
+	efi_status_t ret;
+
+	ret = bs->open_protocol(handle, &efi_guid_device_path,
+				(void **)&dp, NULL /* FIXME */, NULL,
+				EFI_OPEN_PROTOCOL_GET_PROTOCOL);
+	if (ret == EFI_SUCCESS) {
+		*name = efi_dp_str(dp);
+		return 0;
+	} else {
+		return -1;
+	}
+}
+
+static int do_efi_show_devices(int argc, char * const argv[])
+{
+	efi_handle_t *handles = NULL, *handle;
+	u16 *devname;
+
+	handles = efi_get_handles_by_proto(NULL);
+	if (!handles)
+		return CMD_RET_SUCCESS;
+
+	printf("Device Name\n");
+	printf("============================================\n");
+	for (handle = handles; *handle; handle++) {
+		if (!efi_get_device_handle_info(*handle, &devname)) {
+			printf("%ls\n", devname);
+			efi_free_pool(devname);
+		}
+		handle++;
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
 static int do_efi_boot_add(int argc, char * const argv[])
 {
 	int id;
@@ -625,6 +705,8 @@  static int do_efishell(cmd_tbl_t *cmdtp, int flag,
 	if (argc < 2)
 		return CMD_RET_USAGE;
 
+	bs = systab.boottime;
+
 	argc--; argv++;
 	command = argv[0];
 
@@ -642,6 +724,8 @@  static int do_efishell(cmd_tbl_t *cmdtp, int flag,
 		return do_efi_dump_var(argc, argv);
 	else if (!strcmp(command, "setvar"))
 		return do_efi_set_var(argc, argv);
+	else if (!strcmp(command, "devices"))
+		return do_efi_show_devices(argc, argv);
 	else
 		return CMD_RET_USAGE;
 }
@@ -663,7 +747,9 @@  static char efishell_help_text[] =
 	"  - get uefi variable's value\n"
 	"efishell setvar <name> [<value>]\n"
 	"  - set/delete uefi variable's value\n"
-	"    <value> may be \"=\"...\"\", \"=0x...\" (set) or \"=\" (delete)\n";
+	"    <value> may be \"=\"...\"\", \"=0x...\" (set) or \"=\" (delete)\n"
+	"efishell devices\n"
+	"  - show uefi devices\n";
 #endif
 
 U_BOOT_CMD(