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

AKASHI Takahiro takahiro.akashi at linaro.org
Wed Feb 20 07:42:39 UTC 2019


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 at 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(
> >>>
> >>
> > 
> 


More information about the U-Boot mailing list