diff mbox series

[U-Boot,v6,3/7] cmd: efidebug: add devices command

Message ID 20190124110437.6837-4-takahiro.akashi@linaro.org
State Superseded
Delegated to: Alexander Graf
Headers show
Series cmd: add efidebug for efi environment | expand

Commit Message

AKASHI Takahiro Jan. 24, 2019, 11:04 a.m. UTC
"devices" command prints all the uefi variables on the system.

=> efi devices
Scanning disk ahci_scsi.id0lun0...
Scanning disk ahci_scsi.id1lun0...
Found 4 disks
Device           Device Path

Comments

Heinrich Schuchardt Feb. 19, 2019, 7:46 p.m. UTC | #1
On 1/24/19 12:04 PM, AKASHI Takahiro wrote:
> "devices" command prints all the uefi variables on the system.
> 
> => efi devices
> Scanning disk ahci_scsi.id0lun0...
> Scanning disk ahci_scsi.id1lun0...
> Found 4 disks
> Device           Device Path
> ================ ====================
> 000000007ef07ea0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
> 000000007ef00c10 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)
> 000000007ef00dd0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)
> 000000007ef07be0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(1,MBR,0x086246ba,0x800,0x40000)
> 000000007ef07510 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(2,MBR,0x086246ba,0x40800,0x3f800)
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  cmd/efidebug.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 101 insertions(+), 1 deletion(-)
> 
> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> index d836576cf6e0..8a7f775b117a 100644
> --- a/cmd/efidebug.c
> +++ b/cmd/efidebug.c
> @@ -15,6 +15,102 @@
>  #include <search.h>
>  #include <linux/ctype.h>
>  
> +#define BS systab.boottime
> +

Please, add Sphinx style descriptions to all functions, their
parameters, and return values.

Why do you need this function? Can't you call LocateHandleBuffer() which
takes care of the allocation of the buffer?

> +static int efi_get_handles_by_proto(efi_guid_t *guid, efi_handle_t **handlesp,
> +				    int *num)
> +{
> +	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);

This will not work on i386. Please, always use EFI_CALL when invoking a
function which calls EFI_ENTRY().

> +		if (ret == EFI_BUFFER_TOO_SMALL) {
> +			handles = calloc(1, size);
> +			if (!handles)
> +				return -1;
> +
> +			ret = BS->locate_handle(BY_PROTOCOL, guid, NULL, &size,
> +						handles);
> +		}
> +		if (ret != EFI_SUCCESS) {
> +			free(handles);

The handles are not allocated by malloc(). Use FreePool() here.

> +			return -1;
> +		}
> +	} else {
> +		ret = BS->locate_handle(ALL_HANDLES, NULL, NULL, &size, NULL);
> +		if (ret == EFI_BUFFER_TOO_SMALL) {
> +			handles = calloc(1, size);
> +			if (!handles)
> +				return -1;
> +
> +			ret = BS->locate_handle(ALL_HANDLES, NULL, NULL, &size,
> +						handles);
> +		}
> +		if (ret != EFI_SUCCESS) {
> +			free(handles);

Same here.

> +			return -1;
> +		}
> +	}
> +
> +	*handlesp = handles;
> +	*num = size / sizeof(efi_handle_t);
> +
> +	return 0;
> +}
> +
> +static int efi_get_device_handle_info(efi_handle_t handle, u16 **dev_path_text)
> +{
> +	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) {
> +		*dev_path_text = efi_dp_str(dp);
> +		return 0;
> +	} else {
> +		return -1;
> +	}
> +}
> +
> +#define EFI_HANDLE_WIDTH ((int)sizeof(efi_handle_t) * 2)
> +
> +static const char spc[] = "                ";
> +static const char sep[] = "================";
> +
> +static int do_efi_show_devices(cmd_tbl_t *cmdtp, int flag,
> +			       int argc, char * const argv[])
> +{
> +	efi_handle_t *handles;
> +	u16 *dev_path_text;
> +	int num, i;
> +
> +	handles = NULL;
> +	num = 0;
> +	if (efi_get_handles_by_proto(NULL, &handles, &num))
> +		return CMD_RET_FAILURE;
> +
> +	if (!num)
> +		return CMD_RET_SUCCESS;
> +
> +	printf("Device%.*s Device Path\n", EFI_HANDLE_WIDTH - 6, spc);
> +	printf("%.*s ====================\n", EFI_HANDLE_WIDTH, sep);
> +	for (i = 0; i < num; i++) {
> +		if (!efi_get_device_handle_info(handles[i], &dev_path_text)) {
> +			printf("%p %ls\n", handles[i], dev_path_text);
> +			efi_free_pool(dev_path_text);
> +		}
> +	}
> +
> +	free(handles);

FreePool()!

Best regards

Heinrich

> +
> +	return CMD_RET_SUCCESS;
> +}
> +
>  static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag,
>  			   int argc, char * const argv[])
>  {
> @@ -406,6 +502,8 @@ static int do_efi_boot_opt(cmd_tbl_t *cmdtp, int flag,
>  
>  static cmd_tbl_t cmd_efidebug_sub[] = {
>  	U_BOOT_CMD_MKENT(boot, CONFIG_SYS_MAXARGS, 1, do_efi_boot_opt, "", ""),
> +	U_BOOT_CMD_MKENT(devices, CONFIG_SYS_MAXARGS, 1, do_efi_show_devices,
> +			 "", ""),
>  };
>  
>  /* Interpreter command to configure UEFI environment */
> @@ -451,7 +549,9 @@ static char efidebug_help_text[] =
>  	"  - set UEFI BootNext variable\n"
>  	"efidebug boot order [<bootid#1> [<bootid#2> [<bootid#3> [...]]]]\n"
>  	"  - set/show UEFI boot order\n"
> -	"\n";
> +	"\n"
> +	"efidebug devices\n"
> +	"  - show uefi devices\n";
>  #endif
>  
>  U_BOOT_CMD(
>
Heinrich Schuchardt Feb. 19, 2019, 7:53 p.m. UTC | #2
On 2/19/19 8:46 PM, Heinrich Schuchardt wrote:
> On 1/24/19 12:04 PM, AKASHI Takahiro wrote:
>> "devices" command prints all the uefi variables on the system.
>>
>> => efi devices
>> Scanning disk ahci_scsi.id0lun0...
>> Scanning disk ahci_scsi.id1lun0...
>> Found 4 disks
>> Device           Device Path
>> ================ ====================
>> 000000007ef07ea0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
>> 000000007ef00c10 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)
>> 000000007ef00dd0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)
>> 000000007ef07be0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(1,MBR,0x086246ba,0x800,0x40000)
>> 000000007ef07510 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(2,MBR,0x086246ba,0x40800,0x3f800)
>>
>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> ---
>>  cmd/efidebug.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 101 insertions(+), 1 deletion(-)
>>
>> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
>> index d836576cf6e0..8a7f775b117a 100644
>> --- a/cmd/efidebug.c
>> +++ b/cmd/efidebug.c
>> @@ -15,6 +15,102 @@
>>  #include <search.h>
>>  #include <linux/ctype.h>
>>  
>> +#define BS systab.boottime
>> +
> 
> Please, add Sphinx style descriptions to all functions, their
> parameters, and return values.
> 
> Why do you need this function? Can't you call LocateHandleBuffer() which
> takes care of the allocation of the buffer?
> 
>> +static int efi_get_handles_by_proto(efi_guid_t *guid, efi_handle_t **handlesp,
>> +				    int *num)
>> +{
>> +	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);
> 
> This will not work on i386. Please, always use EFI_CALL when invoking a
> function which calls EFI_ENTRY().
> 
>> +		if (ret == EFI_BUFFER_TOO_SMALL) {
>> +			handles = calloc(1, size);
>> +			if (!handles)
>> +				return -1;
>> +
>> +			ret = BS->locate_handle(BY_PROTOCOL, guid, NULL, &size,
>> +						handles);
>> +		}
>> +		if (ret != EFI_SUCCESS) {
>> +			free(handles);
> 
> The handles are not allocated by malloc(). Use FreePool() here.

Sorry I read this wrong. If you calloc(), free() is ok. But I would
rather use LocateHandleBuffer() and FreePool() further down.

Regards

Heinrich

> 
>> +			return -1;
>> +		}
>> +	} else {
>> +		ret = BS->locate_handle(ALL_HANDLES, NULL, NULL, &size, NULL);
>> +		if (ret == EFI_BUFFER_TOO_SMALL) {
>> +			handles = calloc(1, size);
>> +			if (!handles)
>> +				return -1;
>> +
>> +			ret = BS->locate_handle(ALL_HANDLES, NULL, NULL, &size,
>> +						handles);
>> +		}
>> +		if (ret != EFI_SUCCESS) {
>> +			free(handles);
> 
> Same here.
> 
>> +			return -1;
>> +		}
>> +	}
>> +
>> +	*handlesp = handles;
>> +	*num = size / sizeof(efi_handle_t);
>> +
>> +	return 0;
>> +}
>> +
>> +static int efi_get_device_handle_info(efi_handle_t handle, u16 **dev_path_text)
>> +{
>> +	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) {
>> +		*dev_path_text = efi_dp_str(dp);
>> +		return 0;
>> +	} else {
>> +		return -1;
>> +	}
>> +}
>> +
>> +#define EFI_HANDLE_WIDTH ((int)sizeof(efi_handle_t) * 2)
>> +
>> +static const char spc[] = "                ";
>> +static const char sep[] = "================";
>> +
>> +static int do_efi_show_devices(cmd_tbl_t *cmdtp, int flag,
>> +			       int argc, char * const argv[])
>> +{
>> +	efi_handle_t *handles;
>> +	u16 *dev_path_text;
>> +	int num, i;
>> +
>> +	handles = NULL;
>> +	num = 0;
>> +	if (efi_get_handles_by_proto(NULL, &handles, &num))
>> +		return CMD_RET_FAILURE;
>> +
>> +	if (!num)
>> +		return CMD_RET_SUCCESS;
>> +
>> +	printf("Device%.*s Device Path\n", EFI_HANDLE_WIDTH - 6, spc);
>> +	printf("%.*s ====================\n", EFI_HANDLE_WIDTH, sep);
>> +	for (i = 0; i < num; i++) {
>> +		if (!efi_get_device_handle_info(handles[i], &dev_path_text)) {
>> +			printf("%p %ls\n", handles[i], dev_path_text);
>> +			efi_free_pool(dev_path_text);
>> +		}
>> +	}
>> +
>> +	free(handles);
> 
> FreePool()!
> 
> Best regards
> 
> Heinrich
> 
>> +
>> +	return CMD_RET_SUCCESS;
>> +}
>> +
>>  static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag,
>>  			   int argc, char * const argv[])
>>  {
>> @@ -406,6 +502,8 @@ static int do_efi_boot_opt(cmd_tbl_t *cmdtp, int flag,
>>  
>>  static cmd_tbl_t cmd_efidebug_sub[] = {
>>  	U_BOOT_CMD_MKENT(boot, CONFIG_SYS_MAXARGS, 1, do_efi_boot_opt, "", ""),
>> +	U_BOOT_CMD_MKENT(devices, CONFIG_SYS_MAXARGS, 1, do_efi_show_devices,
>> +			 "", ""),
>>  };
>>  
>>  /* Interpreter command to configure UEFI environment */
>> @@ -451,7 +549,9 @@ static char efidebug_help_text[] =
>>  	"  - set UEFI BootNext variable\n"
>>  	"efidebug boot order [<bootid#1> [<bootid#2> [<bootid#3> [...]]]]\n"
>>  	"  - set/show UEFI boot order\n"
>> -	"\n";
>> +	"\n"
>> +	"efidebug devices\n"
>> +	"  - show uefi devices\n";
>>  #endif
>>  
>>  U_BOOT_CMD(
>>
> 
>
AKASHI Takahiro Feb. 20, 2019, 2:20 a.m. UTC | #3
On Tue, Feb 19, 2019 at 08:46:52PM +0100, Heinrich Schuchardt wrote:
> On 1/24/19 12:04 PM, AKASHI Takahiro wrote:
> > "devices" command prints all the uefi variables on the system.
> > 
> > => efi devices
> > Scanning disk ahci_scsi.id0lun0...
> > Scanning disk ahci_scsi.id1lun0...
> > Found 4 disks
> > Device           Device Path
> > ================ ====================
> > 000000007ef07ea0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
> > 000000007ef00c10 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)
> > 000000007ef00dd0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)
> > 000000007ef07be0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(1,MBR,0x086246ba,0x800,0x40000)
> > 000000007ef07510 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(2,MBR,0x086246ba,0x40800,0x3f800)
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  cmd/efidebug.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 101 insertions(+), 1 deletion(-)
> > 
> > diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> > index d836576cf6e0..8a7f775b117a 100644
> > --- a/cmd/efidebug.c
> > +++ b/cmd/efidebug.c
> > @@ -15,6 +15,102 @@
> >  #include <search.h>
> >  #include <linux/ctype.h>
> >  
> > +#define BS systab.boottime
> > +
> 
> Please, add Sphinx style descriptions to all functions, their
> parameters, and return values.

Sure, but please clearify what you mean by "all."
Do you expect descriptions for exactly all the functions,
even for some helper functions of a few lines of code?

> Why do you need this function? Can't you call LocateHandleBuffer() which
> takes care of the allocation of the buffer?
> 
> > +static int efi_get_handles_by_proto(efi_guid_t *guid, efi_handle_t **handlesp,
> > +				    int *num)

Right. I will use locate_handle_buffer here.
But why does UEFI spec define functionally-duplicated functions,
LocateHandle() and LocateHandleBuffer()?

> > +{
> > +	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);
> 
> This will not work on i386. Please, always use EFI_CALL when invoking a
> function which calls EFI_ENTRY().

OK, but let me make sure.
We don't have to use EFI_CALL() for any of (internal) efi_xxx() functions,
say, efi_add_protocol(). Right?

> > +		if (ret == EFI_BUFFER_TOO_SMALL) {
> > +			handles = calloc(1, size);
> > +			if (!handles)
> > +				return -1;
> > +
> > +			ret = BS->locate_handle(BY_PROTOCOL, guid, NULL, &size,
> > +						handles);
> > +		}
> > +		if (ret != EFI_SUCCESS) {
> > +			free(handles);
> 
> The handles are not allocated by malloc(). Use FreePool() here.

Will take care of that along with the change above.

-Takahiro Akashi


> > +			return -1;
> > +		}
> > +	} else {
> > +		ret = BS->locate_handle(ALL_HANDLES, NULL, NULL, &size, NULL);
> > +		if (ret == EFI_BUFFER_TOO_SMALL) {
> > +			handles = calloc(1, size);
> > +			if (!handles)
> > +				return -1;
> > +
> > +			ret = BS->locate_handle(ALL_HANDLES, NULL, NULL, &size,
> > +						handles);
> > +		}
> > +		if (ret != EFI_SUCCESS) {
> > +			free(handles);
> 
> Same here.
> 
> > +			return -1;
> > +		}
> > +	}
> > +
> > +	*handlesp = handles;
> > +	*num = size / sizeof(efi_handle_t);
> > +
> > +	return 0;
> > +}
> > +
> > +static int efi_get_device_handle_info(efi_handle_t handle, u16 **dev_path_text)
> > +{
> > +	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) {
> > +		*dev_path_text = efi_dp_str(dp);
> > +		return 0;
> > +	} else {
> > +		return -1;
> > +	}
> > +}
> > +
> > +#define EFI_HANDLE_WIDTH ((int)sizeof(efi_handle_t) * 2)
> > +
> > +static const char spc[] = "                ";
> > +static const char sep[] = "================";
> > +
> > +static int do_efi_show_devices(cmd_tbl_t *cmdtp, int flag,
> > +			       int argc, char * const argv[])
> > +{
> > +	efi_handle_t *handles;
> > +	u16 *dev_path_text;
> > +	int num, i;
> > +
> > +	handles = NULL;
> > +	num = 0;
> > +	if (efi_get_handles_by_proto(NULL, &handles, &num))
> > +		return CMD_RET_FAILURE;
> > +
> > +	if (!num)
> > +		return CMD_RET_SUCCESS;
> > +
> > +	printf("Device%.*s Device Path\n", EFI_HANDLE_WIDTH - 6, spc);
> > +	printf("%.*s ====================\n", EFI_HANDLE_WIDTH, sep);
> > +	for (i = 0; i < num; i++) {
> > +		if (!efi_get_device_handle_info(handles[i], &dev_path_text)) {
> > +			printf("%p %ls\n", handles[i], dev_path_text);
> > +			efi_free_pool(dev_path_text);
> > +		}
> > +	}
> > +
> > +	free(handles);
> 
> FreePool()!
> 
> Best regards
> 
> Heinrich
> 
> > +
> > +	return CMD_RET_SUCCESS;
> > +}
> > +
> >  static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag,
> >  			   int argc, char * const argv[])
> >  {
> > @@ -406,6 +502,8 @@ static int do_efi_boot_opt(cmd_tbl_t *cmdtp, int flag,
> >  
> >  static cmd_tbl_t cmd_efidebug_sub[] = {
> >  	U_BOOT_CMD_MKENT(boot, CONFIG_SYS_MAXARGS, 1, do_efi_boot_opt, "", ""),
> > +	U_BOOT_CMD_MKENT(devices, CONFIG_SYS_MAXARGS, 1, do_efi_show_devices,
> > +			 "", ""),
> >  };
> >  
> >  /* Interpreter command to configure UEFI environment */
> > @@ -451,7 +549,9 @@ static char efidebug_help_text[] =
> >  	"  - set UEFI BootNext variable\n"
> >  	"efidebug boot order [<bootid#1> [<bootid#2> [<bootid#3> [...]]]]\n"
> >  	"  - set/show UEFI boot order\n"
> > -	"\n";
> > +	"\n"
> > +	"efidebug devices\n"
> > +	"  - show uefi devices\n";
> >  #endif
> >  
> >  U_BOOT_CMD(
> > 
>
Heinrich Schuchardt Feb. 20, 2019, 6:49 a.m. UTC | #4
On 2/20/19 3:20 AM, AKASHI Takahiro wrote:
> On Tue, Feb 19, 2019 at 08:46:52PM +0100, Heinrich Schuchardt wrote:
>> On 1/24/19 12:04 PM, AKASHI Takahiro wrote:
>>> "devices" command prints all the uefi variables on the system.
>>>
>>> => efi devices
>>> Scanning disk ahci_scsi.id0lun0...
>>> Scanning disk ahci_scsi.id1lun0...
>>> Found 4 disks
>>> Device           Device Path
>>> ================ ====================
>>> 000000007ef07ea0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
>>> 000000007ef00c10 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)
>>> 000000007ef00dd0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)
>>> 000000007ef07be0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(1,MBR,0x086246ba,0x800,0x40000)
>>> 000000007ef07510 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(2,MBR,0x086246ba,0x40800,0x3f800)
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>> ---
>>>  cmd/efidebug.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 101 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
>>> index d836576cf6e0..8a7f775b117a 100644
>>> --- a/cmd/efidebug.c
>>> +++ b/cmd/efidebug.c
>>> @@ -15,6 +15,102 @@
>>>  #include <search.h>
>>>  #include <linux/ctype.h>
>>>  
>>> +#define BS systab.boottime
>>> +
>>
>> Please, add Sphinx style descriptions to all functions, their
>> parameters, and return values.
> 
> Sure, but please clearify what you mean by "all."
> Do you expect descriptions for exactly all the functions,
> even for some helper functions of a few lines of code?
> 
>> Why do you need this function? Can't you call LocateHandleBuffer() which
>> takes care of the allocation of the buffer?
>>
>>> +static int efi_get_handles_by_proto(efi_guid_t *guid, efi_handle_t **handlesp,
>>> +				    int *num)
> 
> Right. I will use locate_handle_buffer here.
> But why does UEFI spec define functionally-duplicated functions,
> LocateHandle() and LocateHandleBuffer()?
> 
>>> +{
>>> +	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);
>>
>> This will not work on i386. Please, always use EFI_CALL when invoking a
>> function which calls EFI_ENTRY().
> 
> OK, but let me make sure.
> We don't have to use EFI_CALL() for any of (internal) efi_xxx() functions,
> say, efi_add_protocol(). Right?

EFI_ENTRY() and EFI_EXIT() change the content of the register to which
gd is mapped and executes an assert. So only functions starting with
EFI_ENTRY() should be called via EFI_CALL. Please, do not call other
functions via EFI_CALL().

Please, check that efi_save_gd() is called (e.g. via
efi_init_obj_list()) before the first EFI_CALL().

To get rid of the systab.boottime dereference it might be preferable to
change static functions to global once.

Best regards

Heinrich

> 
>>> +		if (ret == EFI_BUFFER_TOO_SMALL) {
>>> +			handles = calloc(1, size);
>>> +			if (!handles)
>>> +				return -1;
>>> +
>>> +			ret = BS->locate_handle(BY_PROTOCOL, guid, NULL, &size,
>>> +						handles);
>>> +		}
>>> +		if (ret != EFI_SUCCESS) {
>>> +			free(handles);
>>
>> The handles are not allocated by malloc(). Use FreePool() here.
> 
> Will take care of that along with the change above.
> 
> -Takahiro Akashi
> 
> 
>>> +			return -1;
>>> +		}
>>> +	} else {
>>> +		ret = BS->locate_handle(ALL_HANDLES, NULL, NULL, &size, NULL);
>>> +		if (ret == EFI_BUFFER_TOO_SMALL) {
>>> +			handles = calloc(1, size);
>>> +			if (!handles)
>>> +				return -1;
>>> +
>>> +			ret = BS->locate_handle(ALL_HANDLES, NULL, NULL, &size,
>>> +						handles);
>>> +		}
>>> +		if (ret != EFI_SUCCESS) {
>>> +			free(handles);
>>
>> Same here.
>>
>>> +			return -1;
>>> +		}
>>> +	}
>>> +
>>> +	*handlesp = handles;
>>> +	*num = size / sizeof(efi_handle_t);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int efi_get_device_handle_info(efi_handle_t handle, u16 **dev_path_text)
>>> +{
>>> +	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) {
>>> +		*dev_path_text = efi_dp_str(dp);
>>> +		return 0;
>>> +	} else {
>>> +		return -1;
>>> +	}
>>> +}
>>> +
>>> +#define EFI_HANDLE_WIDTH ((int)sizeof(efi_handle_t) * 2)
>>> +
>>> +static const char spc[] = "                ";
>>> +static const char sep[] = "================";
>>> +
>>> +static int do_efi_show_devices(cmd_tbl_t *cmdtp, int flag,
>>> +			       int argc, char * const argv[])
>>> +{
>>> +	efi_handle_t *handles;
>>> +	u16 *dev_path_text;
>>> +	int num, i;
>>> +
>>> +	handles = NULL;
>>> +	num = 0;
>>> +	if (efi_get_handles_by_proto(NULL, &handles, &num))
>>> +		return CMD_RET_FAILURE;
>>> +
>>> +	if (!num)
>>> +		return CMD_RET_SUCCESS;
>>> +
>>> +	printf("Device%.*s Device Path\n", EFI_HANDLE_WIDTH - 6, spc);
>>> +	printf("%.*s ====================\n", EFI_HANDLE_WIDTH, sep);
>>> +	for (i = 0; i < num; i++) {
>>> +		if (!efi_get_device_handle_info(handles[i], &dev_path_text)) {
>>> +			printf("%p %ls\n", handles[i], dev_path_text);
>>> +			efi_free_pool(dev_path_text);
>>> +		}
>>> +	}
>>> +
>>> +	free(handles);
>>
>> FreePool()!
>>
>> Best regards
>>
>> Heinrich
>>
>>> +
>>> +	return CMD_RET_SUCCESS;
>>> +}
>>> +
>>>  static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag,
>>>  			   int argc, char * const argv[])
>>>  {
>>> @@ -406,6 +502,8 @@ static int do_efi_boot_opt(cmd_tbl_t *cmdtp, int flag,
>>>  
>>>  static cmd_tbl_t cmd_efidebug_sub[] = {
>>>  	U_BOOT_CMD_MKENT(boot, CONFIG_SYS_MAXARGS, 1, do_efi_boot_opt, "", ""),
>>> +	U_BOOT_CMD_MKENT(devices, CONFIG_SYS_MAXARGS, 1, do_efi_show_devices,
>>> +			 "", ""),
>>>  };
>>>  
>>>  /* Interpreter command to configure UEFI environment */
>>> @@ -451,7 +549,9 @@ static char efidebug_help_text[] =
>>>  	"  - set UEFI BootNext variable\n"
>>>  	"efidebug boot order [<bootid#1> [<bootid#2> [<bootid#3> [...]]]]\n"
>>>  	"  - set/show UEFI boot order\n"
>>> -	"\n";
>>> +	"\n"
>>> +	"efidebug devices\n"
>>> +	"  - show uefi devices\n";
>>>  #endif
>>>  
>>>  U_BOOT_CMD(
>>>
>>
>
AKASHI Takahiro Feb. 20, 2019, 7:42 a.m. UTC | #5
On Wed, Feb 20, 2019 at 07:49:01AM +0100, Heinrich Schuchardt wrote:
> On 2/20/19 3:20 AM, AKASHI Takahiro wrote:
> > On Tue, Feb 19, 2019 at 08:46:52PM +0100, Heinrich Schuchardt wrote:
> >> On 1/24/19 12:04 PM, AKASHI Takahiro wrote:
> >>> "devices" command prints all the uefi variables on the system.
> >>>
> >>> => efi devices
> >>> Scanning disk ahci_scsi.id0lun0...
> >>> Scanning disk ahci_scsi.id1lun0...
> >>> Found 4 disks
> >>> Device           Device Path
> >>> ================ ====================
> >>> 000000007ef07ea0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
> >>> 000000007ef00c10 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)
> >>> 000000007ef00dd0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)
> >>> 000000007ef07be0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(1,MBR,0x086246ba,0x800,0x40000)
> >>> 000000007ef07510 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(2,MBR,0x086246ba,0x40800,0x3f800)
> >>>
> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>> ---
> >>>  cmd/efidebug.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++-
> >>>  1 file changed, 101 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> >>> index d836576cf6e0..8a7f775b117a 100644
> >>> --- a/cmd/efidebug.c
> >>> +++ b/cmd/efidebug.c
> >>> @@ -15,6 +15,102 @@
> >>>  #include <search.h>
> >>>  #include <linux/ctype.h>
> >>>  
> >>> +#define BS systab.boottime
> >>> +
> >>
> >> Please, add Sphinx style descriptions to all functions, their
> >> parameters, and return values.
> > 
> > Sure, but please clearify what you mean by "all."
> > Do you expect descriptions for exactly all the functions,
> > even for some helper functions of a few lines of code?
> > 
> >> Why do you need this function? Can't you call LocateHandleBuffer() which
> >> takes care of the allocation of the buffer?
> >>
> >>> +static int efi_get_handles_by_proto(efi_guid_t *guid, efi_handle_t **handlesp,
> >>> +				    int *num)
> > 
> > Right. I will use locate_handle_buffer here.
> > But why does UEFI spec define functionally-duplicated functions,
> > LocateHandle() and LocateHandleBuffer()?
> > 
> >>> +{
> >>> +	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);
> >>
> >> This will not work on i386. Please, always use EFI_CALL when invoking a
> >> function which calls EFI_ENTRY().
> > 
> > OK, but let me make sure.
> > We don't have to use EFI_CALL() for any of (internal) efi_xxx() functions,
> > say, efi_add_protocol(). Right?
> 
> EFI_ENTRY() and EFI_EXIT() change the content of the register to which
> gd is mapped and executes an assert. So only functions starting with
> EFI_ENTRY() should be called via EFI_CALL. Please, do not call other
> functions via EFI_CALL().

Thank you for explanation.

> Please, check that efi_save_gd() is called (e.g. via
> efi_init_obj_list()) before the first EFI_CALL().

I didn't notice that.

Can you please leave some document about this kind of usage
so that future UEFI developers be aware of it.

> To get rid of the systab.boottime dereference it might be preferable to
> change static functions to global once.

Do you mean, for example, we should call efi_open_protocol()
instead of BS->open_protocol()?
I hesitate to agree with you as we should try to call exported *API's*
wherever possible, in particular, in EFI-application-like code, like efidebug.

-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> > 
> >>> +		if (ret == EFI_BUFFER_TOO_SMALL) {
> >>> +			handles = calloc(1, size);
> >>> +			if (!handles)
> >>> +				return -1;
> >>> +
> >>> +			ret = BS->locate_handle(BY_PROTOCOL, guid, NULL, &size,
> >>> +						handles);
> >>> +		}
> >>> +		if (ret != EFI_SUCCESS) {
> >>> +			free(handles);
> >>
> >> The handles are not allocated by malloc(). Use FreePool() here.
> > 
> > Will take care of that along with the change above.
> > 
> > -Takahiro Akashi
> > 
> > 
> >>> +			return -1;
> >>> +		}
> >>> +	} else {
> >>> +		ret = BS->locate_handle(ALL_HANDLES, NULL, NULL, &size, NULL);
> >>> +		if (ret == EFI_BUFFER_TOO_SMALL) {
> >>> +			handles = calloc(1, size);
> >>> +			if (!handles)
> >>> +				return -1;
> >>> +
> >>> +			ret = BS->locate_handle(ALL_HANDLES, NULL, NULL, &size,
> >>> +						handles);
> >>> +		}
> >>> +		if (ret != EFI_SUCCESS) {
> >>> +			free(handles);
> >>
> >> Same here.
> >>
> >>> +			return -1;
> >>> +		}
> >>> +	}
> >>> +
> >>> +	*handlesp = handles;
> >>> +	*num = size / sizeof(efi_handle_t);
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int efi_get_device_handle_info(efi_handle_t handle, u16 **dev_path_text)
> >>> +{
> >>> +	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) {
> >>> +		*dev_path_text = efi_dp_str(dp);
> >>> +		return 0;
> >>> +	} else {
> >>> +		return -1;
> >>> +	}
> >>> +}
> >>> +
> >>> +#define EFI_HANDLE_WIDTH ((int)sizeof(efi_handle_t) * 2)
> >>> +
> >>> +static const char spc[] = "                ";
> >>> +static const char sep[] = "================";
> >>> +
> >>> +static int do_efi_show_devices(cmd_tbl_t *cmdtp, int flag,
> >>> +			       int argc, char * const argv[])
> >>> +{
> >>> +	efi_handle_t *handles;
> >>> +	u16 *dev_path_text;
> >>> +	int num, i;
> >>> +
> >>> +	handles = NULL;
> >>> +	num = 0;
> >>> +	if (efi_get_handles_by_proto(NULL, &handles, &num))
> >>> +		return CMD_RET_FAILURE;
> >>> +
> >>> +	if (!num)
> >>> +		return CMD_RET_SUCCESS;
> >>> +
> >>> +	printf("Device%.*s Device Path\n", EFI_HANDLE_WIDTH - 6, spc);
> >>> +	printf("%.*s ====================\n", EFI_HANDLE_WIDTH, sep);
> >>> +	for (i = 0; i < num; i++) {
> >>> +		if (!efi_get_device_handle_info(handles[i], &dev_path_text)) {
> >>> +			printf("%p %ls\n", handles[i], dev_path_text);
> >>> +			efi_free_pool(dev_path_text);
> >>> +		}
> >>> +	}
> >>> +
> >>> +	free(handles);
> >>
> >> FreePool()!
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>> +
> >>> +	return CMD_RET_SUCCESS;
> >>> +}
> >>> +
> >>>  static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag,
> >>>  			   int argc, char * const argv[])
> >>>  {
> >>> @@ -406,6 +502,8 @@ static int do_efi_boot_opt(cmd_tbl_t *cmdtp, int flag,
> >>>  
> >>>  static cmd_tbl_t cmd_efidebug_sub[] = {
> >>>  	U_BOOT_CMD_MKENT(boot, CONFIG_SYS_MAXARGS, 1, do_efi_boot_opt, "", ""),
> >>> +	U_BOOT_CMD_MKENT(devices, CONFIG_SYS_MAXARGS, 1, do_efi_show_devices,
> >>> +			 "", ""),
> >>>  };
> >>>  
> >>>  /* Interpreter command to configure UEFI environment */
> >>> @@ -451,7 +549,9 @@ static char efidebug_help_text[] =
> >>>  	"  - set UEFI BootNext variable\n"
> >>>  	"efidebug boot order [<bootid#1> [<bootid#2> [<bootid#3> [...]]]]\n"
> >>>  	"  - set/show UEFI boot order\n"
> >>> -	"\n";
> >>> +	"\n"
> >>> +	"efidebug devices\n"
> >>> +	"  - show uefi devices\n";
> >>>  #endif
> >>>  
> >>>  U_BOOT_CMD(
> >>>
> >>
> > 
>
diff mbox series

Patch

================ ====================
000000007ef07ea0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
000000007ef00c10 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)
000000007ef00dd0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)
000000007ef07be0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(1,MBR,0x086246ba,0x800,0x40000)
000000007ef07510 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(2,MBR,0x086246ba,0x40800,0x3f800)

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

diff --git a/cmd/efidebug.c b/cmd/efidebug.c
index d836576cf6e0..8a7f775b117a 100644
--- a/cmd/efidebug.c
+++ b/cmd/efidebug.c
@@ -15,6 +15,102 @@ 
 #include <search.h>
 #include <linux/ctype.h>
 
+#define BS systab.boottime
+
+static int efi_get_handles_by_proto(efi_guid_t *guid, efi_handle_t **handlesp,
+				    int *num)
+{
+	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 -1;
+
+			ret = BS->locate_handle(BY_PROTOCOL, guid, NULL, &size,
+						handles);
+		}
+		if (ret != EFI_SUCCESS) {
+			free(handles);
+			return -1;
+		}
+	} else {
+		ret = BS->locate_handle(ALL_HANDLES, NULL, NULL, &size, NULL);
+		if (ret == EFI_BUFFER_TOO_SMALL) {
+			handles = calloc(1, size);
+			if (!handles)
+				return -1;
+
+			ret = BS->locate_handle(ALL_HANDLES, NULL, NULL, &size,
+						handles);
+		}
+		if (ret != EFI_SUCCESS) {
+			free(handles);
+			return -1;
+		}
+	}
+
+	*handlesp = handles;
+	*num = size / sizeof(efi_handle_t);
+
+	return 0;
+}
+
+static int efi_get_device_handle_info(efi_handle_t handle, u16 **dev_path_text)
+{
+	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) {
+		*dev_path_text = efi_dp_str(dp);
+		return 0;
+	} else {
+		return -1;
+	}
+}
+
+#define EFI_HANDLE_WIDTH ((int)sizeof(efi_handle_t) * 2)
+
+static const char spc[] = "                ";
+static const char sep[] = "================";
+
+static int do_efi_show_devices(cmd_tbl_t *cmdtp, int flag,
+			       int argc, char * const argv[])
+{
+	efi_handle_t *handles;
+	u16 *dev_path_text;
+	int num, i;
+
+	handles = NULL;
+	num = 0;
+	if (efi_get_handles_by_proto(NULL, &handles, &num))
+		return CMD_RET_FAILURE;
+
+	if (!num)
+		return CMD_RET_SUCCESS;
+
+	printf("Device%.*s Device Path\n", EFI_HANDLE_WIDTH - 6, spc);
+	printf("%.*s ====================\n", EFI_HANDLE_WIDTH, sep);
+	for (i = 0; i < num; i++) {
+		if (!efi_get_device_handle_info(handles[i], &dev_path_text)) {
+			printf("%p %ls\n", handles[i], dev_path_text);
+			efi_free_pool(dev_path_text);
+		}
+	}
+
+	free(handles);
+
+	return CMD_RET_SUCCESS;
+}
+
 static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag,
 			   int argc, char * const argv[])
 {
@@ -406,6 +502,8 @@  static int do_efi_boot_opt(cmd_tbl_t *cmdtp, int flag,
 
 static cmd_tbl_t cmd_efidebug_sub[] = {
 	U_BOOT_CMD_MKENT(boot, CONFIG_SYS_MAXARGS, 1, do_efi_boot_opt, "", ""),
+	U_BOOT_CMD_MKENT(devices, CONFIG_SYS_MAXARGS, 1, do_efi_show_devices,
+			 "", ""),
 };
 
 /* Interpreter command to configure UEFI environment */
@@ -451,7 +549,9 @@  static char efidebug_help_text[] =
 	"  - set UEFI BootNext variable\n"
 	"efidebug boot order [<bootid#1> [<bootid#2> [<bootid#3> [...]]]]\n"
 	"  - set/show UEFI boot order\n"
-	"\n";
+	"\n"
+	"efidebug devices\n"
+	"  - show uefi devices\n";
 #endif
 
 U_BOOT_CMD(