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(