[PATCH 1/1] cmd: simplify command efidebug

[PATCH 1/1] cmd: simplify command efidebug

[Heinrich Schuchardt]

Currently we have subcommands 'efidebug dh' which shows protocols per
handle and 'efidebug devices' which shows the device path. None shows which
U-Boot device matches the handle.

Change 'efidebug dh' to show the device path and the U-Boot device if any
is associated with the handle.

Remove 'efidebug devices'.

Old output of 'efidebug dh':

    Handle           Protocols
    ================ ====================
    000000001b22e690 Device Path, Block IO
    000000001b22e800 Device Path, Block IO, system, Simple File System

New output of 'efidebug dh':

    000000001b22e690 (host0)
      /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/VenHw(bbe4e671-5773-4ea1-9aab-3a7dbf40c482,00)
      Block IO

    000000001b22e800 (host0:1)
      /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/VenHw(bbe4e671-5773-4ea1-9aab-3a7dbf40c482,00)/HD(1,GPT,7e5c17c5-3f5f-49d0-ae96-511b21d7f273,0x800,0x3f7df)
      Block IO
      system
      Simple File System

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 cmd/efidebug.c | 102 ++++++++-----------------------------------------
 1 file changed, 15 insertions(+), 87 deletions(-)

diff --git a/cmd/efidebug.c b/cmd/efidebug.c
index 84e6ff5565..76a6b4d8eb 100644
--- a/cmd/efidebug.c
+++ b/cmd/efidebug.c
@@ -8,6 +8,7 @@
 #include <charset.h>
 #include <common.h>
 #include <command.h>
+#include <dm/device.h>
 #include <efi_dt_fixup.h>
 #include <efi_load_initrd.h>
 #include <efi_loader.h>
@@ -344,79 +345,11 @@ static int do_efi_capsule(struct cmd_tbl *cmdtp, int flag,
 }
 #endif /* CONFIG_EFI_HAVE_CAPSULE_SUPPORT */
 
-/**
- * efi_get_device_path_text() - get device path text
- *
- * Return the text representation of the device path of a handle.
- *
- * @handle:	handle of UEFI device
- * Return:
- * Pointer to the device path text or NULL.
- * The caller is responsible for calling FreePool().
- */
-static u16 *efi_get_device_path_text(efi_handle_t handle)
-{
-	struct efi_handler *handler;
-	efi_status_t ret;
-
-	ret = efi_search_protocol(handle, &efi_guid_device_path, &handler);
-	if (ret == EFI_SUCCESS && handler->protocol_interface) {
-		struct efi_device_path *dp = handler->protocol_interface;
-
-		return efi_dp_str(dp);
-	} else {
-		return NULL;
-	}
-}
-
 #define EFI_HANDLE_WIDTH ((int)sizeof(efi_handle_t) * 2)
 
 static const char spc[] = "                ";
 static const char sep[] = "================";
 
-/**
- * do_efi_show_devices() - show UEFI devices
- *
- * @cmdtp:	Command table
- * @flag:	Command flag
- * @argc:	Number of arguments
- * @argv:	Argument array
- * Return:	CMD_RET_SUCCESS on success, CMD_RET_RET_FAILURE on failure
- *
- * Implement efidebug "devices" sub-command.
- * Show all UEFI devices and their information.
- */
-static int do_efi_show_devices(struct cmd_tbl *cmdtp, int flag,
-			       int argc, char *const argv[])
-{
-	efi_handle_t *handles;
-	efi_uintn_t num, i;
-	u16 *dev_path_text;
-	efi_status_t ret;
-
-	ret = EFI_CALL(efi_locate_handle_buffer(ALL_HANDLES, NULL, NULL,
-						&num, &handles));
-	if (ret != EFI_SUCCESS)
-		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++) {
-		dev_path_text = efi_get_device_path_text(handles[i]);
-		if (dev_path_text) {
-			printf("%p %ls\n", handles[i], dev_path_text);
-			efi_free_pool(dev_path_text);
-		}
-	}
-
-	efi_free_pool(handles);
-
-	return CMD_RET_SUCCESS;
-}
-
 /**
  * efi_get_driver_handle_info() - get information of UEFI driver
  *
@@ -535,26 +468,25 @@ static int do_efi_show_handles(struct cmd_tbl *cmdtp, int flag,
 	if (!num)
 		return CMD_RET_SUCCESS;
 
-	printf("Handle%.*s Protocols\n", EFI_HANDLE_WIDTH - 6, spc);
-	printf("%.*s ====================\n", EFI_HANDLE_WIDTH, sep);
 	for (i = 0; i < num; i++) {
-		printf("%p", handles[i]);
+		struct efi_handler *handler;
+
+		printf("\n%p", handles[i]);
+		if (handles[i]->dev)
+			printf(" (%s)", handles[i]->dev->name);
+		printf("\n");
+		/* Print device path */
+		ret = efi_search_protocol(handles[i], &efi_guid_device_path,
+					  &handler);
+		if (ret == EFI_SUCCESS)
+			printf("  %pD\n", handler->protocol_interface);
 		ret = EFI_CALL(BS->protocols_per_handle(handles[i], &guid,
 							&count));
-		if (ret || !count) {
-			putc('\n');
-			continue;
-		}
-
+		/* Print other protocols */
 		for (j = 0; j < count; j++) {
-			if (j)
-				printf(", ");
-			else
-				putc(' ');
-
-			printf("%pUs", guid[j]);
+			if (guidcmp(guid[j], &efi_guid_device_path))
+				printf("  %pUs\n", guid[j]);
 		}
-		putc('\n');
 	}
 
 	efi_free_pool(handles);
@@ -1539,8 +1471,6 @@ static struct cmd_tbl cmd_efidebug_sub[] = {
 	U_BOOT_CMD_MKENT(capsule, CONFIG_SYS_MAXARGS, 1, do_efi_capsule,
 			 "", ""),
 #endif
-	U_BOOT_CMD_MKENT(devices, CONFIG_SYS_MAXARGS, 1, do_efi_show_devices,
-			 "", ""),
 	U_BOOT_CMD_MKENT(drivers, CONFIG_SYS_MAXARGS, 1, do_efi_show_drivers,
 			 "", ""),
 	U_BOOT_CMD_MKENT(dh, CONFIG_SYS_MAXARGS, 1, do_efi_show_handles,
@@ -1630,8 +1560,6 @@ static char efidebug_help_text[] =
 #endif
 	"\n"
 #endif
-	"efidebug devices\n"
-	"  - show UEFI devices\n"
 	"efidebug drivers\n"
 	"  - show UEFI drivers\n"
 	"efidebug dh\n"
-- 
2.37.2

Re: [PATCH 1/1] cmd: simplify command efidebug

[AKASHI Takahiro]

Heinrich,

On Tue, Oct 04, 2022 at 03:40:32PM +0200, Heinrich Schuchardt wrote:
> Currently we have subcommands 'efidebug dh' which shows protocols per
> handle and 'efidebug devices' which shows the device path. None shows which
> U-Boot device matches the handle.

If you know how a device path display format is constructed,
you can identify it. For instance,
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -> efi_root
/VenHw(bbe4e671-5773-4ea1-9aab-3a7dbf40c482,00)
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
       -> host                               0
/HD(1,GPT,7e5c17c5-3f5f-49d0-ae96-511b21d7f273,0x800,0x3f7df)
    ^ -> partition 1

> Change 'efidebug dh' to show the device path and the U-Boot device if any
> is associated with the handle.

There is a good reason for two separate subcommands. While "dh" shows
all the handles which currently exist on the system, "devices" shows
only ones with device path protocol.
The number of handles can possibly be huge (try "dh" on EDK2).
For those who are interested in what devices are connected at some point,
"devices" subcommand is still useful.

The problem is not "devices" subcommand itself, but the display format
of device path. UEFI spec says, in "10.6 EFI Device Path Display Format Overview",
"This section describes the recommended conversion between an EFI Device
                            ^^^^^^^^^^^
Path Protocol and text."

So if there is more readable/understandable format, we may want to use it
for U-Boot UEFI (at least, as an alternate option of format).
Say,
/VenHw(root)/VenHw(host, 0)/HD(1,GPT,....)
I believe that it is simple and unambiguous.

> Remove 'efidebug devices'.

So please don't remove "devices".
Instead, I recommend that the same change be applied to this subcommand,
displaying both formats at "devices".

-Takahiro Akashi


> Old output of 'efidebug dh':
> 
>     Handle           Protocols
>     ================ ====================
>     000000001b22e690 Device Path, Block IO
>     000000001b22e800 Device Path, Block IO, system, Simple File System
> 
> New output of 'efidebug dh':
> 
>     000000001b22e690 (host0)
>       /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/VenHw(bbe4e671-5773-4ea1-9aab-3a7dbf40c482,00)
>       Block IO
> 
>     000000001b22e800 (host0:1)
>       /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/VenHw(bbe4e671-5773-4ea1-9aab-3a7dbf40c482,00)/HD(1,GPT,7e5c17c5-3f5f-49d0-ae96-511b21d7f273,0x800,0x3f7df)
>       Block IO
>       system
>       Simple File System
> 
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  cmd/efidebug.c | 102 ++++++++-----------------------------------------
>  1 file changed, 15 insertions(+), 87 deletions(-)
> 
> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> index 84e6ff5565..76a6b4d8eb 100644
> --- a/cmd/efidebug.c
> +++ b/cmd/efidebug.c
> @@ -8,6 +8,7 @@
>  #include <charset.h>
>  #include <common.h>
>  #include <command.h>
> +#include <dm/device.h>
>  #include <efi_dt_fixup.h>
>  #include <efi_load_initrd.h>
>  #include <efi_loader.h>
> @@ -344,79 +345,11 @@ static int do_efi_capsule(struct cmd_tbl *cmdtp, int flag,
>  }
>  #endif /* CONFIG_EFI_HAVE_CAPSULE_SUPPORT */
>  
> -/**
> - * efi_get_device_path_text() - get device path text
> - *
> - * Return the text representation of the device path of a handle.
> - *
> - * @handle:	handle of UEFI device
> - * Return:
> - * Pointer to the device path text or NULL.
> - * The caller is responsible for calling FreePool().
> - */
> -static u16 *efi_get_device_path_text(efi_handle_t handle)
> -{
> -	struct efi_handler *handler;
> -	efi_status_t ret;
> -
> -	ret = efi_search_protocol(handle, &efi_guid_device_path, &handler);
> -	if (ret == EFI_SUCCESS && handler->protocol_interface) {
> -		struct efi_device_path *dp = handler->protocol_interface;
> -
> -		return efi_dp_str(dp);
> -	} else {
> -		return NULL;
> -	}
> -}
> -
>  #define EFI_HANDLE_WIDTH ((int)sizeof(efi_handle_t) * 2)
>  
>  static const char spc[] = "                ";
>  static const char sep[] = "================";
>  
> -/**
> - * do_efi_show_devices() - show UEFI devices
> - *
> - * @cmdtp:	Command table
> - * @flag:	Command flag
> - * @argc:	Number of arguments
> - * @argv:	Argument array
> - * Return:	CMD_RET_SUCCESS on success, CMD_RET_RET_FAILURE on failure
> - *
> - * Implement efidebug "devices" sub-command.
> - * Show all UEFI devices and their information.
> - */
> -static int do_efi_show_devices(struct cmd_tbl *cmdtp, int flag,
> -			       int argc, char *const argv[])
> -{
> -	efi_handle_t *handles;
> -	efi_uintn_t num, i;
> -	u16 *dev_path_text;
> -	efi_status_t ret;
> -
> -	ret = EFI_CALL(efi_locate_handle_buffer(ALL_HANDLES, NULL, NULL,
> -						&num, &handles));
> -	if (ret != EFI_SUCCESS)
> -		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++) {
> -		dev_path_text = efi_get_device_path_text(handles[i]);
> -		if (dev_path_text) {
> -			printf("%p %ls\n", handles[i], dev_path_text);
> -			efi_free_pool(dev_path_text);
> -		}
> -	}
> -
> -	efi_free_pool(handles);
> -
> -	return CMD_RET_SUCCESS;
> -}
> -
>  /**
>   * efi_get_driver_handle_info() - get information of UEFI driver
>   *
> @@ -535,26 +468,25 @@ static int do_efi_show_handles(struct cmd_tbl *cmdtp, int flag,
>  	if (!num)
>  		return CMD_RET_SUCCESS;
>  
> -	printf("Handle%.*s Protocols\n", EFI_HANDLE_WIDTH - 6, spc);
> -	printf("%.*s ====================\n", EFI_HANDLE_WIDTH, sep);
>  	for (i = 0; i < num; i++) {
> -		printf("%p", handles[i]);
> +		struct efi_handler *handler;
> +
> +		printf("\n%p", handles[i]);
> +		if (handles[i]->dev)
> +			printf(" (%s)", handles[i]->dev->name);
> +		printf("\n");
> +		/* Print device path */
> +		ret = efi_search_protocol(handles[i], &efi_guid_device_path,
> +					  &handler);
> +		if (ret == EFI_SUCCESS)
> +			printf("  %pD\n", handler->protocol_interface);
>  		ret = EFI_CALL(BS->protocols_per_handle(handles[i], &guid,
>  							&count));
> -		if (ret || !count) {
> -			putc('\n');
> -			continue;
> -		}
> -
> +		/* Print other protocols */
>  		for (j = 0; j < count; j++) {
> -			if (j)
> -				printf(", ");
> -			else
> -				putc(' ');
> -
> -			printf("%pUs", guid[j]);
> +			if (guidcmp(guid[j], &efi_guid_device_path))
> +				printf("  %pUs\n", guid[j]);
>  		}
> -		putc('\n');
>  	}
>  
>  	efi_free_pool(handles);
> @@ -1539,8 +1471,6 @@ static struct cmd_tbl cmd_efidebug_sub[] = {
>  	U_BOOT_CMD_MKENT(capsule, CONFIG_SYS_MAXARGS, 1, do_efi_capsule,
>  			 "", ""),
>  #endif
> -	U_BOOT_CMD_MKENT(devices, CONFIG_SYS_MAXARGS, 1, do_efi_show_devices,
> -			 "", ""),
>  	U_BOOT_CMD_MKENT(drivers, CONFIG_SYS_MAXARGS, 1, do_efi_show_drivers,
>  			 "", ""),
>  	U_BOOT_CMD_MKENT(dh, CONFIG_SYS_MAXARGS, 1, do_efi_show_handles,
> @@ -1630,8 +1560,6 @@ static char efidebug_help_text[] =
>  #endif
>  	"\n"
>  #endif
> -	"efidebug devices\n"
> -	"  - show UEFI devices\n"
>  	"efidebug drivers\n"
>  	"  - show UEFI drivers\n"
>  	"efidebug dh\n"
> -- 
> 2.37.2
> 

Re: [PATCH 1/1] cmd: simplify command efidebug

[Heinrich Schuchardt]

On 10/5/22 02:23, AKASHI Takahiro wrote:
> Heinrich,
> 
> On Tue, Oct 04, 2022 at 03:40:32PM +0200, Heinrich Schuchardt wrote:
>> Currently we have subcommands 'efidebug dh' which shows protocols per
>> handle and 'efidebug devices' which shows the device path. None shows which
>> U-Boot device matches the handle.
> 
> If you know how a device path display format is constructed,
> you can identify it. For instance,
> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
>         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -> efi_root
> /VenHw(bbe4e671-5773-4ea1-9aab-3a7dbf40c482,00)
>         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>         -> host                               0
> /HD(1,GPT,7e5c17c5-3f5f-49d0-ae96-511b21d7f273,0x800,0x3f7df)
>      ^ -> partition 1
> 
>> Change 'efidebug dh' to show the device path and the U-Boot device if any
>> is associated with the handle.
> 
> There is a good reason for two separate subcommands. While "dh" shows
> all the handles which currently exist on the system, "devices" shows
> only ones with device path protocol.
> The number of handles can possibly be huge (try "dh" on EDK2).

We only have to care about the low number of EFI handles in U-Boot which 
is block devices + 5.

> For those who are interested in what devices are connected at some point,
> "devices" subcommand is still useful.

Having to look up information in two separate commands instead of one 
place is inconvenient.

The output of efidebug devices does not contain any information which 
will not be shown by modified efidebug dh command. So the devices 
command becomes superfluous.

> 
> The problem is not "devices" subcommand itself, but the display format
> of device path. UEFI spec says, in "10.6 EFI Device Path Display Format Overview",
> "This section describes the recommended conversion between an EFI Device
>                              ^^^^^^^^^^^
> Path Protocol and text."
> 
> So if there is more readable/understandable format, we may want to use it
> for U-Boot UEFI (at least, as an alternate option of format).
> Say,
> /VenHw(root)/VenHw(host, 0)/HD(1,GPT,....)
> I believe that it is simple and unambiguous.

This might be implemented subject to the DisplayOnly argument of 
EFI_DEVICE_PATH_TO_TEXT_PROTOCOL.ConvertDevicePathToText() in a separate 
patch.

It would require adding the GUIDs to list_guid[] in lib/uuid.c and 
switching between '%pUl' and '%pUs' in efi_convert_device_node_to_text.

> 
>> Remove 'efidebug devices'.
> 
> So please don't remove "devices".
> Instead, I recommend that the same change be applied to this subcommand,
> displaying both formats at "devices".

There is no benefit in having two sub-commands showing the same 
information. Instead we should reduce the code size.

Best regards

Heinrich

> 
> -Takahiro Akashi
> 
> 
>> Old output of 'efidebug dh':
>>
>>      Handle           Protocols
>>      ================ ====================
>>      000000001b22e690 Device Path, Block IO
>>      000000001b22e800 Device Path, Block IO, system, Simple File System
>>
>> New output of 'efidebug dh':
>>
>>      000000001b22e690 (host0)
>>        /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/VenHw(bbe4e671-5773-4ea1-9aab-3a7dbf40c482,00)
>>        Block IO
>>
>>      000000001b22e800 (host0:1)
>>        /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/VenHw(bbe4e671-5773-4ea1-9aab-3a7dbf40c482,00)/HD(1,GPT,7e5c17c5-3f5f-49d0-ae96-511b21d7f273,0x800,0x3f7df)
>>        Block IO
>>        system
>>        Simple File System
>>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>> ---
>>   cmd/efidebug.c | 102 ++++++++-----------------------------------------
>>   1 file changed, 15 insertions(+), 87 deletions(-)
>>
>> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
>> index 84e6ff5565..76a6b4d8eb 100644
>> --- a/cmd/efidebug.c
>> +++ b/cmd/efidebug.c
>> @@ -8,6 +8,7 @@
>>   #include <charset.h>
>>   #include <common.h>
>>   #include <command.h>
>> +#include <dm/device.h>
>>   #include <efi_dt_fixup.h>
>>   #include <efi_load_initrd.h>
>>   #include <efi_loader.h>
>> @@ -344,79 +345,11 @@ static int do_efi_capsule(struct cmd_tbl *cmdtp, int flag,
>>   }
>>   #endif /* CONFIG_EFI_HAVE_CAPSULE_SUPPORT */
>>   
>> -/**
>> - * efi_get_device_path_text() - get device path text
>> - *
>> - * Return the text representation of the device path of a handle.
>> - *
>> - * @handle:	handle of UEFI device
>> - * Return:
>> - * Pointer to the device path text or NULL.
>> - * The caller is responsible for calling FreePool().
>> - */
>> -static u16 *efi_get_device_path_text(efi_handle_t handle)
>> -{
>> -	struct efi_handler *handler;
>> -	efi_status_t ret;
>> -
>> -	ret = efi_search_protocol(handle, &efi_guid_device_path, &handler);
>> -	if (ret == EFI_SUCCESS && handler->protocol_interface) {
>> -		struct efi_device_path *dp = handler->protocol_interface;
>> -
>> -		return efi_dp_str(dp);
>> -	} else {
>> -		return NULL;
>> -	}
>> -}
>> -
>>   #define EFI_HANDLE_WIDTH ((int)sizeof(efi_handle_t) * 2)
>>   
>>   static const char spc[] = "                ";
>>   static const char sep[] = "================";
>>   
>> -/**
>> - * do_efi_show_devices() - show UEFI devices
>> - *
>> - * @cmdtp:	Command table
>> - * @flag:	Command flag
>> - * @argc:	Number of arguments
>> - * @argv:	Argument array
>> - * Return:	CMD_RET_SUCCESS on success, CMD_RET_RET_FAILURE on failure
>> - *
>> - * Implement efidebug "devices" sub-command.
>> - * Show all UEFI devices and their information.
>> - */
>> -static int do_efi_show_devices(struct cmd_tbl *cmdtp, int flag,
>> -			       int argc, char *const argv[])
>> -{
>> -	efi_handle_t *handles;
>> -	efi_uintn_t num, i;
>> -	u16 *dev_path_text;
>> -	efi_status_t ret;
>> -
>> -	ret = EFI_CALL(efi_locate_handle_buffer(ALL_HANDLES, NULL, NULL,
>> -						&num, &handles));
>> -	if (ret != EFI_SUCCESS)
>> -		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++) {
>> -		dev_path_text = efi_get_device_path_text(handles[i]);
>> -		if (dev_path_text) {
>> -			printf("%p %ls\n", handles[i], dev_path_text);
>> -			efi_free_pool(dev_path_text);
>> -		}
>> -	}
>> -
>> -	efi_free_pool(handles);
>> -
>> -	return CMD_RET_SUCCESS;
>> -}
>> -
>>   /**
>>    * efi_get_driver_handle_info() - get information of UEFI driver
>>    *
>> @@ -535,26 +468,25 @@ static int do_efi_show_handles(struct cmd_tbl *cmdtp, int flag,
>>   	if (!num)
>>   		return CMD_RET_SUCCESS;
>>   
>> -	printf("Handle%.*s Protocols\n", EFI_HANDLE_WIDTH - 6, spc);
>> -	printf("%.*s ====================\n", EFI_HANDLE_WIDTH, sep);
>>   	for (i = 0; i < num; i++) {
>> -		printf("%p", handles[i]);
>> +		struct efi_handler *handler;
>> +
>> +		printf("\n%p", handles[i]);
>> +		if (handles[i]->dev)
>> +			printf(" (%s)", handles[i]->dev->name);
>> +		printf("\n");
>> +		/* Print device path */
>> +		ret = efi_search_protocol(handles[i], &efi_guid_device_path,
>> +					  &handler);
>> +		if (ret == EFI_SUCCESS)
>> +			printf("  %pD\n", handler->protocol_interface);
>>   		ret = EFI_CALL(BS->protocols_per_handle(handles[i], &guid,
>>   							&count));
>> -		if (ret || !count) {
>> -			putc('\n');
>> -			continue;
>> -		}
>> -
>> +		/* Print other protocols */
>>   		for (j = 0; j < count; j++) {
>> -			if (j)
>> -				printf(", ");
>> -			else
>> -				putc(' ');
>> -
>> -			printf("%pUs", guid[j]);
>> +			if (guidcmp(guid[j], &efi_guid_device_path))
>> +				printf("  %pUs\n", guid[j]);
>>   		}
>> -		putc('\n');
>>   	}
>>   
>>   	efi_free_pool(handles);
>> @@ -1539,8 +1471,6 @@ static struct cmd_tbl cmd_efidebug_sub[] = {
>>   	U_BOOT_CMD_MKENT(capsule, CONFIG_SYS_MAXARGS, 1, do_efi_capsule,
>>   			 "", ""),
>>   #endif
>> -	U_BOOT_CMD_MKENT(devices, CONFIG_SYS_MAXARGS, 1, do_efi_show_devices,
>> -			 "", ""),
>>   	U_BOOT_CMD_MKENT(drivers, CONFIG_SYS_MAXARGS, 1, do_efi_show_drivers,
>>   			 "", ""),
>>   	U_BOOT_CMD_MKENT(dh, CONFIG_SYS_MAXARGS, 1, do_efi_show_handles,
>> @@ -1630,8 +1560,6 @@ static char efidebug_help_text[] =
>>   #endif
>>   	"\n"
>>   #endif
>> -	"efidebug devices\n"
>> -	"  - show UEFI devices\n"
>>   	"efidebug drivers\n"
>>   	"  - show UEFI drivers\n"
>>   	"efidebug dh\n"
>> -- 
>> 2.37.2
>>

Re: [PATCH 1/1] cmd: simplify command efidebug

[AKASHI Takahiro]

On Wed, Oct 05, 2022 at 08:27:37AM +0200, Heinrich Schuchardt wrote:
> On 10/5/22 02:23, AKASHI Takahiro wrote:
> > Heinrich,
> > 
> > On Tue, Oct 04, 2022 at 03:40:32PM +0200, Heinrich Schuchardt wrote:
> > > Currently we have subcommands 'efidebug dh' which shows protocols per
> > > handle and 'efidebug devices' which shows the device path. None shows which
> > > U-Boot device matches the handle.
> > 
> > If you know how a device path display format is constructed,
> > you can identify it. For instance,
> > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
> >         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -> efi_root
> > /VenHw(bbe4e671-5773-4ea1-9aab-3a7dbf40c482,00)
> >         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >         -> host                               0
> > /HD(1,GPT,7e5c17c5-3f5f-49d0-ae96-511b21d7f273,0x800,0x3f7df)
> >      ^ -> partition 1
> > 
> > > Change 'efidebug dh' to show the device path and the U-Boot device if any
> > > is associated with the handle.
> > 
> > There is a good reason for two separate subcommands. While "dh" shows
> > all the handles which currently exist on the system, "devices" shows
> > only ones with device path protocol.
> > The number of handles can possibly be huge (try "dh" on EDK2).
> 
> We only have to care about the low number of EFI handles in U-Boot which is
> block devices + 5.

Who knows the future.

> > For those who are interested in what devices are connected at some point,
> > "devices" subcommand is still useful.
> 
> Having to look up information in two separate commands instead of one place
> is inconvenient.

I don't think so.
I sometimes felt comfortable only with the output from "devices" in my past development.

> The output of efidebug devices does not contain any information which will
> not be shown by modified efidebug dh command. So the devices command becomes
> superfluous.

That is why I suggested the same change be applied to "devices"
because such information is only meaningful for device paths.

-Takahiro Akashi

> > 
> > The problem is not "devices" subcommand itself, but the display format
> > of device path. UEFI spec says, in "10.6 EFI Device Path Display Format Overview",
> > "This section describes the recommended conversion between an EFI Device
> >                              ^^^^^^^^^^^
> > Path Protocol and text."
> > 
> > So if there is more readable/understandable format, we may want to use it
> > for U-Boot UEFI (at least, as an alternate option of format).
> > Say,
> > /VenHw(root)/VenHw(host, 0)/HD(1,GPT,....)
> > I believe that it is simple and unambiguous.
> 
> This might be implemented subject to the DisplayOnly argument of
> EFI_DEVICE_PATH_TO_TEXT_PROTOCOL.ConvertDevicePathToText() in a separate
> patch.
> 
> It would require adding the GUIDs to list_guid[] in lib/uuid.c and switching
> between '%pUl' and '%pUs' in efi_convert_device_node_to_text.
> 
> > 
> > > Remove 'efidebug devices'.
> > 
> > So please don't remove "devices".
> > Instead, I recommend that the same change be applied to this subcommand,
> > displaying both formats at "devices".
> 
> There is no benefit in having two sub-commands showing the same information.
> Instead we should reduce the code size.
> 
> Best regards
> 
> Heinrich
> 
> > 
> > -Takahiro Akashi
> > 
> > 
> > > Old output of 'efidebug dh':
> > > 
> > >      Handle           Protocols
> > >      ================ ====================
> > >      000000001b22e690 Device Path, Block IO
> > >      000000001b22e800 Device Path, Block IO, system, Simple File System
> > > 
> > > New output of 'efidebug dh':
> > > 
> > >      000000001b22e690 (host0)
> > >        /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/VenHw(bbe4e671-5773-4ea1-9aab-3a7dbf40c482,00)
> > >        Block IO
> > > 
> > >      000000001b22e800 (host0:1)
> > >        /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/VenHw(bbe4e671-5773-4ea1-9aab-3a7dbf40c482,00)/HD(1,GPT,7e5c17c5-3f5f-49d0-ae96-511b21d7f273,0x800,0x3f7df)
> > >        Block IO
> > >        system
> > >        Simple File System
> > > 
> > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > ---
> > >   cmd/efidebug.c | 102 ++++++++-----------------------------------------
> > >   1 file changed, 15 insertions(+), 87 deletions(-)
> > > 
> > > diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> > > index 84e6ff5565..76a6b4d8eb 100644
> > > --- a/cmd/efidebug.c
> > > +++ b/cmd/efidebug.c
> > > @@ -8,6 +8,7 @@
> > >   #include <charset.h>
> > >   #include <common.h>
> > >   #include <command.h>
> > > +#include <dm/device.h>
> > >   #include <efi_dt_fixup.h>
> > >   #include <efi_load_initrd.h>
> > >   #include <efi_loader.h>
> > > @@ -344,79 +345,11 @@ static int do_efi_capsule(struct cmd_tbl *cmdtp, int flag,
> > >   }
> > >   #endif /* CONFIG_EFI_HAVE_CAPSULE_SUPPORT */
> > > -/**
> > > - * efi_get_device_path_text() - get device path text
> > > - *
> > > - * Return the text representation of the device path of a handle.
> > > - *
> > > - * @handle:	handle of UEFI device
> > > - * Return:
> > > - * Pointer to the device path text or NULL.
> > > - * The caller is responsible for calling FreePool().
> > > - */
> > > -static u16 *efi_get_device_path_text(efi_handle_t handle)
> > > -{
> > > -	struct efi_handler *handler;
> > > -	efi_status_t ret;
> > > -
> > > -	ret = efi_search_protocol(handle, &efi_guid_device_path, &handler);
> > > -	if (ret == EFI_SUCCESS && handler->protocol_interface) {
> > > -		struct efi_device_path *dp = handler->protocol_interface;
> > > -
> > > -		return efi_dp_str(dp);
> > > -	} else {
> > > -		return NULL;
> > > -	}
> > > -}
> > > -
> > >   #define EFI_HANDLE_WIDTH ((int)sizeof(efi_handle_t) * 2)
> > >   static const char spc[] = "                ";
> > >   static const char sep[] = "================";
> > > -/**
> > > - * do_efi_show_devices() - show UEFI devices
> > > - *
> > > - * @cmdtp:	Command table
> > > - * @flag:	Command flag
> > > - * @argc:	Number of arguments
> > > - * @argv:	Argument array
> > > - * Return:	CMD_RET_SUCCESS on success, CMD_RET_RET_FAILURE on failure
> > > - *
> > > - * Implement efidebug "devices" sub-command.
> > > - * Show all UEFI devices and their information.
> > > - */
> > > -static int do_efi_show_devices(struct cmd_tbl *cmdtp, int flag,
> > > -			       int argc, char *const argv[])
> > > -{
> > > -	efi_handle_t *handles;
> > > -	efi_uintn_t num, i;
> > > -	u16 *dev_path_text;
> > > -	efi_status_t ret;
> > > -
> > > -	ret = EFI_CALL(efi_locate_handle_buffer(ALL_HANDLES, NULL, NULL,
> > > -						&num, &handles));
> > > -	if (ret != EFI_SUCCESS)
> > > -		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++) {
> > > -		dev_path_text = efi_get_device_path_text(handles[i]);
> > > -		if (dev_path_text) {
> > > -			printf("%p %ls\n", handles[i], dev_path_text);
> > > -			efi_free_pool(dev_path_text);
> > > -		}
> > > -	}
> > > -
> > > -	efi_free_pool(handles);
> > > -
> > > -	return CMD_RET_SUCCESS;
> > > -}
> > > -
> > >   /**
> > >    * efi_get_driver_handle_info() - get information of UEFI driver
> > >    *
> > > @@ -535,26 +468,25 @@ static int do_efi_show_handles(struct cmd_tbl *cmdtp, int flag,
> > >   	if (!num)
> > >   		return CMD_RET_SUCCESS;
> > > -	printf("Handle%.*s Protocols\n", EFI_HANDLE_WIDTH - 6, spc);
> > > -	printf("%.*s ====================\n", EFI_HANDLE_WIDTH, sep);
> > >   	for (i = 0; i < num; i++) {
> > > -		printf("%p", handles[i]);
> > > +		struct efi_handler *handler;
> > > +
> > > +		printf("\n%p", handles[i]);
> > > +		if (handles[i]->dev)
> > > +			printf(" (%s)", handles[i]->dev->name);
> > > +		printf("\n");
> > > +		/* Print device path */
> > > +		ret = efi_search_protocol(handles[i], &efi_guid_device_path,
> > > +					  &handler);
> > > +		if (ret == EFI_SUCCESS)
> > > +			printf("  %pD\n", handler->protocol_interface);
> > >   		ret = EFI_CALL(BS->protocols_per_handle(handles[i], &guid,
> > >   							&count));
> > > -		if (ret || !count) {
> > > -			putc('\n');
> > > -			continue;
> > > -		}
> > > -
> > > +		/* Print other protocols */
> > >   		for (j = 0; j < count; j++) {
> > > -			if (j)
> > > -				printf(", ");
> > > -			else
> > > -				putc(' ');
> > > -
> > > -			printf("%pUs", guid[j]);
> > > +			if (guidcmp(guid[j], &efi_guid_device_path))
> > > +				printf("  %pUs\n", guid[j]);
> > >   		}
> > > -		putc('\n');
> > >   	}
> > >   	efi_free_pool(handles);
> > > @@ -1539,8 +1471,6 @@ static struct cmd_tbl cmd_efidebug_sub[] = {
> > >   	U_BOOT_CMD_MKENT(capsule, CONFIG_SYS_MAXARGS, 1, do_efi_capsule,
> > >   			 "", ""),
> > >   #endif
> > > -	U_BOOT_CMD_MKENT(devices, CONFIG_SYS_MAXARGS, 1, do_efi_show_devices,
> > > -			 "", ""),
> > >   	U_BOOT_CMD_MKENT(drivers, CONFIG_SYS_MAXARGS, 1, do_efi_show_drivers,
> > >   			 "", ""),
> > >   	U_BOOT_CMD_MKENT(dh, CONFIG_SYS_MAXARGS, 1, do_efi_show_handles,
> > > @@ -1630,8 +1560,6 @@ static char efidebug_help_text[] =
> > >   #endif
> > >   	"\n"
> > >   #endif
> > > -	"efidebug devices\n"
> > > -	"  - show UEFI devices\n"
> > >   	"efidebug drivers\n"
> > >   	"  - show UEFI drivers\n"
> > >   	"efidebug dh\n"
> > > -- 
> > > 2.37.2
> > > 

Re: [PATCH 1/1] cmd: simplify command efidebug

[Ilias Apalodimas]

On Wed, Oct 05, 2022 at 08:27:37AM +0200, Heinrich Schuchardt wrote:
> On 10/5/22 02:23, AKASHI Takahiro wrote:
> > Heinrich,
> > 
> > On Tue, Oct 04, 2022 at 03:40:32PM +0200, Heinrich Schuchardt wrote:
> > > Currently we have subcommands 'efidebug dh' which shows protocols per
> > > handle and 'efidebug devices' which shows the device path. None shows which
> > > U-Boot device matches the handle.
> > 
> > If you know how a device path display format is constructed,
> > you can identify it. For instance,
> > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
> >         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -> efi_root
> > /VenHw(bbe4e671-5773-4ea1-9aab-3a7dbf40c482,00)
> >         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >         -> host                               0
> > /HD(1,GPT,7e5c17c5-3f5f-49d0-ae96-511b21d7f273,0x800,0x3f7df)
> >      ^ -> partition 1
> > 
> > > Change 'efidebug dh' to show the device path and the U-Boot device if any
> > > is associated with the handle.
> > 
> > There is a good reason for two separate subcommands. While "dh" shows
> > all the handles which currently exist on the system, "devices" shows
> > only ones with device path protocol.
> > The number of handles can possibly be huge (try "dh" on EDK2).
> 
> We only have to care about the low number of EFI handles in U-Boot which is
> block devices + 5.
> 
> > For those who are interested in what devices are connected at some point,
> > "devices" subcommand is still useful.
> 
> Having to look up information in two separate commands instead of one place
> is inconvenient.
> 
> The output of efidebug devices does not contain any information which will
> not be shown by modified efidebug dh command. So the devices command becomes
> superfluous.
> 
> > 
> > The problem is not "devices" subcommand itself, but the display format
> > of device path. UEFI spec says, in "10.6 EFI Device Path Display Format Overview",
> > "This section describes the recommended conversion between an EFI Device
> >                              ^^^^^^^^^^^
> > Path Protocol and text."
> > 
> > So if there is more readable/understandable format, we may want to use it
> > for U-Boot UEFI (at least, as an alternate option of format).
> > Say,
> > /VenHw(root)/VenHw(host, 0)/HD(1,GPT,....)
> > I believe that it is simple and unambiguous.
> 
> This might be implemented subject to the DisplayOnly argument of
> EFI_DEVICE_PATH_TO_TEXT_PROTOCOL.ConvertDevicePathToText() in a separate
> patch.
> 
> It would require adding the GUIDs to list_guid[] in lib/uuid.c and switching
> between '%pUl' and '%pUs' in efi_convert_device_node_to_text.
> 
> > 
> > > Remove 'efidebug devices'.
> > 
> > So please don't remove "devices".
> > Instead, I recommend that the same change be applied to this subcommand,
> > displaying both formats at "devices".
> 
> There is no benefit in having two sub-commands showing the same information.
> Instead we should reduce the code size.
> 
> Best regards
> 
> Heinrich
> 
> > 
> > -Takahiro Akashi
> > 
> > 
> > > Old output of 'efidebug dh':
> > > 
> > >      Handle           Protocols
> > >      ================ ====================
> > >      000000001b22e690 Device Path, Block IO
> > >      000000001b22e800 Device Path, Block IO, system, Simple File System
> > > 
> > > New output of 'efidebug dh':
> > > 
> > >      000000001b22e690 (host0)
> > >        /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/VenHw(bbe4e671-5773-4ea1-9aab-3a7dbf40c482,00)
> > >        Block IO
> > > 
> > >      000000001b22e800 (host0:1)
> > >        /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/VenHw(bbe4e671-5773-4ea1-9aab-3a7dbf40c482,00)/HD(1,GPT,7e5c17c5-3f5f-49d0-ae96-511b21d7f273,0x800,0x3f7df)
> > >        Block IO
> > >        system
> > >        Simple File System
> > > 
> > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > ---
> > >   cmd/efidebug.c | 102 ++++++++-----------------------------------------
> > >   1 file changed, 15 insertions(+), 87 deletions(-)
> > > 
> > > diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> > > index 84e6ff5565..76a6b4d8eb 100644
> > > --- a/cmd/efidebug.c
> > > +++ b/cmd/efidebug.c
> > > @@ -8,6 +8,7 @@
> > >   #include <charset.h>
> > >   #include <common.h>
> > >   #include <command.h>
> > > +#include <dm/device.h>
> > >   #include <efi_dt_fixup.h>
> > >   #include <efi_load_initrd.h>
> > >   #include <efi_loader.h>
> > > @@ -344,79 +345,11 @@ static int do_efi_capsule(struct cmd_tbl *cmdtp, int flag,
> > >   }
> > >   #endif /* CONFIG_EFI_HAVE_CAPSULE_SUPPORT */
> > > -/**
> > > - * efi_get_device_path_text() - get device path text
> > > - *
> > > - * Return the text representation of the device path of a handle.
> > > - *
> > > - * @handle:	handle of UEFI device
> > > - * Return:
> > > - * Pointer to the device path text or NULL.
> > > - * The caller is responsible for calling FreePool().
> > > - */
> > > -static u16 *efi_get_device_path_text(efi_handle_t handle)
> > > -{
> > > -	struct efi_handler *handler;
> > > -	efi_status_t ret;
> > > -
> > > -	ret = efi_search_protocol(handle, &efi_guid_device_path, &handler);
> > > -	if (ret == EFI_SUCCESS && handler->protocol_interface) {
> > > -		struct efi_device_path *dp = handler->protocol_interface;
> > > -
> > > -		return efi_dp_str(dp);
> > > -	} else {
> > > -		return NULL;
> > > -	}
> > > -}
> > > -
> > >   #define EFI_HANDLE_WIDTH ((int)sizeof(efi_handle_t) * 2)
> > >   static const char spc[] = "                ";
> > >   static const char sep[] = "================";
> > > -/**
> > > - * do_efi_show_devices() - show UEFI devices
> > > - *
> > > - * @cmdtp:	Command table
> > > - * @flag:	Command flag
> > > - * @argc:	Number of arguments
> > > - * @argv:	Argument array
> > > - * Return:	CMD_RET_SUCCESS on success, CMD_RET_RET_FAILURE on failure
> > > - *
> > > - * Implement efidebug "devices" sub-command.
> > > - * Show all UEFI devices and their information.
> > > - */
> > > -static int do_efi_show_devices(struct cmd_tbl *cmdtp, int flag,
> > > -			       int argc, char *const argv[])
> > > -{
> > > -	efi_handle_t *handles;
> > > -	efi_uintn_t num, i;
> > > -	u16 *dev_path_text;
> > > -	efi_status_t ret;
> > > -
> > > -	ret = EFI_CALL(efi_locate_handle_buffer(ALL_HANDLES, NULL, NULL,
> > > -						&num, &handles));
> > > -	if (ret != EFI_SUCCESS)
> > > -		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++) {
> > > -		dev_path_text = efi_get_device_path_text(handles[i]);
> > > -		if (dev_path_text) {
> > > -			printf("%p %ls\n", handles[i], dev_path_text);
> > > -			efi_free_pool(dev_path_text);
> > > -		}
> > > -	}
> > > -
> > > -	efi_free_pool(handles);
> > > -
> > > -	return CMD_RET_SUCCESS;
> > > -}
> > > -
> > >   /**
> > >    * efi_get_driver_handle_info() - get information of UEFI driver
> > >    *
> > > @@ -535,26 +468,25 @@ static int do_efi_show_handles(struct cmd_tbl *cmdtp, int flag,
> > >   	if (!num)
> > >   		return CMD_RET_SUCCESS;
> > > -	printf("Handle%.*s Protocols\n", EFI_HANDLE_WIDTH - 6, spc);
> > > -	printf("%.*s ====================\n", EFI_HANDLE_WIDTH, sep);
> > >   	for (i = 0; i < num; i++) {
> > > -		printf("%p", handles[i]);
> > > +		struct efi_handler *handler;
> > > +
> > > +		printf("\n%p", handles[i]);
> > > +		if (handles[i]->dev)
> > > +			printf(" (%s)", handles[i]->dev->name);
> > > +		printf("\n");
> > > +		/* Print device path */
> > > +		ret = efi_search_protocol(handles[i], &efi_guid_device_path,
> > > +					  &handler);
> > > +		if (ret == EFI_SUCCESS)
> > > +			printf("  %pD\n", handler->protocol_interface);
> > >   		ret = EFI_CALL(BS->protocols_per_handle(handles[i], &guid,
> > >   							&count));
> > > -		if (ret || !count) {
> > > -			putc('\n');
> > > -			continue;
> > > -		}
> > > -
> > > +		/* Print other protocols */
> > >   		for (j = 0; j < count; j++) {
> > > -			if (j)
> > > -				printf(", ");
> > > -			else
> > > -				putc(' ');
> > > -
> > > -			printf("%pUs", guid[j]);
> > > +			if (guidcmp(guid[j], &efi_guid_device_path))
> > > +				printf("  %pUs\n", guid[j]);
> > >   		}
> > > -		putc('\n');
> > >   	}
> > >   	efi_free_pool(handles);
> > > @@ -1539,8 +1471,6 @@ static struct cmd_tbl cmd_efidebug_sub[] = {
> > >   	U_BOOT_CMD_MKENT(capsule, CONFIG_SYS_MAXARGS, 1, do_efi_capsule,
> > >   			 "", ""),
> > >   #endif
> > > -	U_BOOT_CMD_MKENT(devices, CONFIG_SYS_MAXARGS, 1, do_efi_show_devices,
> > > -			 "", ""),
> > >   	U_BOOT_CMD_MKENT(drivers, CONFIG_SYS_MAXARGS, 1, do_efi_show_drivers,
> > >   			 "", ""),
> > >   	U_BOOT_CMD_MKENT(dh, CONFIG_SYS_MAXARGS, 1, do_efi_show_handles,
> > > @@ -1630,8 +1560,6 @@ static char efidebug_help_text[] =
> > >   #endif
> > >   	"\n"
> > >   #endif
> > > -	"efidebug devices\n"
> > > -	"  - show UEFI devices\n"
> > >   	"efidebug drivers\n"
> > >   	"  - show UEFI drivers\n"
> > >   	"efidebug dh\n"
> > > -- 
> > > 2.37.2
> > > 


FWIW the clean does make sense to me.  First of all displaying handles +
device is more convenient and it's going to become even more important as
the EFI<->DM integration moves on.  Not to mention that efidebug is a
misnomer (now) it is used for *configuring* boot options.  We should try
to make the command as lightweight as possible, since people are literally
expected to include it in their binary if they want to boot via EFI

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Regards
/Ilias

Re: [PATCH 1/1] cmd: simplify command efidebug

[AKASHI Takahiro]

On Wed, Oct 05, 2022 at 10:23:26AM +0300, Ilias Apalodimas wrote:
> On Wed, Oct 05, 2022 at 08:27:37AM +0200, Heinrich Schuchardt wrote:
> > On 10/5/22 02:23, AKASHI Takahiro wrote:
> > > Heinrich,
> > > 
> > > On Tue, Oct 04, 2022 at 03:40:32PM +0200, Heinrich Schuchardt wrote:
> > > > Currently we have subcommands 'efidebug dh' which shows protocols per
> > > > handle and 'efidebug devices' which shows the device path. None shows which
> > > > U-Boot device matches the handle.
> > > 
> > > If you know how a device path display format is constructed,
> > > you can identify it. For instance,
> > > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
> > >         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -> efi_root
> > > /VenHw(bbe4e671-5773-4ea1-9aab-3a7dbf40c482,00)
> > >         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > >         -> host                               0
> > > /HD(1,GPT,7e5c17c5-3f5f-49d0-ae96-511b21d7f273,0x800,0x3f7df)
> > >      ^ -> partition 1
> > > 
> > > > Change 'efidebug dh' to show the device path and the U-Boot device if any
> > > > is associated with the handle.
> > > 
> > > There is a good reason for two separate subcommands. While "dh" shows
> > > all the handles which currently exist on the system, "devices" shows
> > > only ones with device path protocol.
> > > The number of handles can possibly be huge (try "dh" on EDK2).
> > 
> > We only have to care about the low number of EFI handles in U-Boot which is
> > block devices + 5.
> > 
> > > For those who are interested in what devices are connected at some point,
> > > "devices" subcommand is still useful.
> > 
> > Having to look up information in two separate commands instead of one place
> > is inconvenient.
> > 
> > The output of efidebug devices does not contain any information which will
> > not be shown by modified efidebug dh command. So the devices command becomes
> > superfluous.
> > 
> > > 
> > > The problem is not "devices" subcommand itself, but the display format
> > > of device path. UEFI spec says, in "10.6 EFI Device Path Display Format Overview",
> > > "This section describes the recommended conversion between an EFI Device
> > >                              ^^^^^^^^^^^
> > > Path Protocol and text."
> > > 
> > > So if there is more readable/understandable format, we may want to use it
> > > for U-Boot UEFI (at least, as an alternate option of format).
> > > Say,
> > > /VenHw(root)/VenHw(host, 0)/HD(1,GPT,....)
> > > I believe that it is simple and unambiguous.
> > 
> > This might be implemented subject to the DisplayOnly argument of
> > EFI_DEVICE_PATH_TO_TEXT_PROTOCOL.ConvertDevicePathToText() in a separate
> > patch.
> > 
> > It would require adding the GUIDs to list_guid[] in lib/uuid.c and switching
> > between '%pUl' and '%pUs' in efi_convert_device_node_to_text.
> > 
> > > 
> > > > Remove 'efidebug devices'.
> > > 
> > > So please don't remove "devices".
> > > Instead, I recommend that the same change be applied to this subcommand,
> > > displaying both formats at "devices".
> > 
> > There is no benefit in having two sub-commands showing the same information.
> > Instead we should reduce the code size.
> > 
> > Best regards
> > 
> > Heinrich
> > 
> > > 
> > > -Takahiro Akashi
> > > 
> > > 
> > > > Old output of 'efidebug dh':
> > > > 
> > > >      Handle           Protocols
> > > >      ================ ====================
> > > >      000000001b22e690 Device Path, Block IO
> > > >      000000001b22e800 Device Path, Block IO, system, Simple File System
> > > > 
> > > > New output of 'efidebug dh':
> > > > 
> > > >      000000001b22e690 (host0)
> > > >        /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/VenHw(bbe4e671-5773-4ea1-9aab-3a7dbf40c482,00)
> > > >        Block IO
> > > > 
> > > >      000000001b22e800 (host0:1)
> > > >        /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/VenHw(bbe4e671-5773-4ea1-9aab-3a7dbf40c482,00)/HD(1,GPT,7e5c17c5-3f5f-49d0-ae96-511b21d7f273,0x800,0x3f7df)
> > > >        Block IO
> > > >        system
> > > >        Simple File System
> > > > 
> > > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > > ---
> > > >   cmd/efidebug.c | 102 ++++++++-----------------------------------------
> > > >   1 file changed, 15 insertions(+), 87 deletions(-)
> > > > 
> > > > diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> > > > index 84e6ff5565..76a6b4d8eb 100644
> > > > --- a/cmd/efidebug.c
> > > > +++ b/cmd/efidebug.c
> > > > @@ -8,6 +8,7 @@
> > > >   #include <charset.h>
> > > >   #include <common.h>
> > > >   #include <command.h>
> > > > +#include <dm/device.h>
> > > >   #include <efi_dt_fixup.h>
> > > >   #include <efi_load_initrd.h>
> > > >   #include <efi_loader.h>
> > > > @@ -344,79 +345,11 @@ static int do_efi_capsule(struct cmd_tbl *cmdtp, int flag,
> > > >   }
> > > >   #endif /* CONFIG_EFI_HAVE_CAPSULE_SUPPORT */
> > > > -/**
> > > > - * efi_get_device_path_text() - get device path text
> > > > - *
> > > > - * Return the text representation of the device path of a handle.
> > > > - *
> > > > - * @handle:	handle of UEFI device
> > > > - * Return:
> > > > - * Pointer to the device path text or NULL.
> > > > - * The caller is responsible for calling FreePool().
> > > > - */
> > > > -static u16 *efi_get_device_path_text(efi_handle_t handle)
> > > > -{
> > > > -	struct efi_handler *handler;
> > > > -	efi_status_t ret;
> > > > -
> > > > -	ret = efi_search_protocol(handle, &efi_guid_device_path, &handler);
> > > > -	if (ret == EFI_SUCCESS && handler->protocol_interface) {
> > > > -		struct efi_device_path *dp = handler->protocol_interface;
> > > > -
> > > > -		return efi_dp_str(dp);
> > > > -	} else {
> > > > -		return NULL;
> > > > -	}
> > > > -}
> > > > -
> > > >   #define EFI_HANDLE_WIDTH ((int)sizeof(efi_handle_t) * 2)
> > > >   static const char spc[] = "                ";
> > > >   static const char sep[] = "================";
> > > > -/**
> > > > - * do_efi_show_devices() - show UEFI devices
> > > > - *
> > > > - * @cmdtp:	Command table
> > > > - * @flag:	Command flag
> > > > - * @argc:	Number of arguments
> > > > - * @argv:	Argument array
> > > > - * Return:	CMD_RET_SUCCESS on success, CMD_RET_RET_FAILURE on failure
> > > > - *
> > > > - * Implement efidebug "devices" sub-command.
> > > > - * Show all UEFI devices and their information.
> > > > - */
> > > > -static int do_efi_show_devices(struct cmd_tbl *cmdtp, int flag,
> > > > -			       int argc, char *const argv[])
> > > > -{
> > > > -	efi_handle_t *handles;
> > > > -	efi_uintn_t num, i;
> > > > -	u16 *dev_path_text;
> > > > -	efi_status_t ret;
> > > > -
> > > > -	ret = EFI_CALL(efi_locate_handle_buffer(ALL_HANDLES, NULL, NULL,
> > > > -						&num, &handles));
> > > > -	if (ret != EFI_SUCCESS)
> > > > -		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++) {
> > > > -		dev_path_text = efi_get_device_path_text(handles[i]);
> > > > -		if (dev_path_text) {
> > > > -			printf("%p %ls\n", handles[i], dev_path_text);
> > > > -			efi_free_pool(dev_path_text);
> > > > -		}
> > > > -	}
> > > > -
> > > > -	efi_free_pool(handles);
> > > > -
> > > > -	return CMD_RET_SUCCESS;
> > > > -}
> > > > -
> > > >   /**
> > > >    * efi_get_driver_handle_info() - get information of UEFI driver
> > > >    *
> > > > @@ -535,26 +468,25 @@ static int do_efi_show_handles(struct cmd_tbl *cmdtp, int flag,
> > > >   	if (!num)
> > > >   		return CMD_RET_SUCCESS;
> > > > -	printf("Handle%.*s Protocols\n", EFI_HANDLE_WIDTH - 6, spc);
> > > > -	printf("%.*s ====================\n", EFI_HANDLE_WIDTH, sep);
> > > >   	for (i = 0; i < num; i++) {
> > > > -		printf("%p", handles[i]);
> > > > +		struct efi_handler *handler;
> > > > +
> > > > +		printf("\n%p", handles[i]);
> > > > +		if (handles[i]->dev)
> > > > +			printf(" (%s)", handles[i]->dev->name);
> > > > +		printf("\n");
> > > > +		/* Print device path */
> > > > +		ret = efi_search_protocol(handles[i], &efi_guid_device_path,
> > > > +					  &handler);
> > > > +		if (ret == EFI_SUCCESS)
> > > > +			printf("  %pD\n", handler->protocol_interface);
> > > >   		ret = EFI_CALL(BS->protocols_per_handle(handles[i], &guid,
> > > >   							&count));
> > > > -		if (ret || !count) {
> > > > -			putc('\n');
> > > > -			continue;
> > > > -		}
> > > > -
> > > > +		/* Print other protocols */
> > > >   		for (j = 0; j < count; j++) {
> > > > -			if (j)
> > > > -				printf(", ");
> > > > -			else
> > > > -				putc(' ');
> > > > -
> > > > -			printf("%pUs", guid[j]);
> > > > +			if (guidcmp(guid[j], &efi_guid_device_path))
> > > > +				printf("  %pUs\n", guid[j]);
> > > >   		}
> > > > -		putc('\n');
> > > >   	}
> > > >   	efi_free_pool(handles);
> > > > @@ -1539,8 +1471,6 @@ static struct cmd_tbl cmd_efidebug_sub[] = {
> > > >   	U_BOOT_CMD_MKENT(capsule, CONFIG_SYS_MAXARGS, 1, do_efi_capsule,
> > > >   			 "", ""),
> > > >   #endif
> > > > -	U_BOOT_CMD_MKENT(devices, CONFIG_SYS_MAXARGS, 1, do_efi_show_devices,
> > > > -			 "", ""),
> > > >   	U_BOOT_CMD_MKENT(drivers, CONFIG_SYS_MAXARGS, 1, do_efi_show_drivers,
> > > >   			 "", ""),
> > > >   	U_BOOT_CMD_MKENT(dh, CONFIG_SYS_MAXARGS, 1, do_efi_show_handles,
> > > > @@ -1630,8 +1560,6 @@ static char efidebug_help_text[] =
> > > >   #endif
> > > >   	"\n"
> > > >   #endif
> > > > -	"efidebug devices\n"
> > > > -	"  - show UEFI devices\n"
> > > >   	"efidebug drivers\n"
> > > >   	"  - show UEFI drivers\n"
> > > >   	"efidebug dh\n"
> > > > -- 
> > > > 2.37.2
> > > > 
> 
> 
> FWIW the clean does make sense to me.  First of all displaying handles +
> device is more convenient and it's going to become even more important as
> the EFI<->DM integration moves on.  Not to mention that efidebug is a
> misnomer (now) it is used for *configuring* boot options.

It's not true.
From the beginning, it was seen as a "debugging" tool.
In fact, when I implemented it at the first time, I named it efishell
and later proposed alternative names, efiutil and even eficonfig(!) but
all were rejected by then-maintainer simply because he saw the command
to be a debugging tool.
This positioning has not been changed since then.

> We should try
> to make the command as lightweight as possible, since people are literally
> expected to include it in their binary if they want to boot via EFI

I believe that Kojima-san's eficonfig can fill the requirements
as we all expect.

-Takahiro Akashi


> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> 
> Regards
> /Ilias

Re: [PATCH 1/1] cmd: simplify command efidebug

[Ilias Apalodimas]

Akashi-san

[...]

> > > > >         "\n"
> > > > >   #endif
> > > > > -       "efidebug devices\n"
> > > > > -       "  - show UEFI devices\n"
> > > > >         "efidebug drivers\n"
> > > > >         "  - show UEFI drivers\n"
> > > > >         "efidebug dh\n"
> > > > > --
> > > > > 2.37.2
> > > > >
> >
> >
> > FWIW the clean does make sense to me.  First of all displaying handles +
> > device is more convenient and it's going to become even more important as
> > the EFI<->DM integration moves on.  Not to mention that efidebug is a
> > misnomer (now) it is used for *configuring* boot options.
>
> It's not true.
> From the beginning, it was seen as a "debugging" tool.
> In fact, when I implemented it at the first time, I named it efishell
> and later proposed alternative names, efiutil and even eficonfig(!) but
> all were rejected by then-maintainer simply because he saw the command
> to be a debugging tool.

That's why I said *now*.  It was a debugging tool way back.  Right now
we use it to configure the efibootmgr and that's why I proposed
splitting efidebug and and 'efi' command way back.

> This positioning has not been changed since then.
>
> > We should try
> > to make the command as lightweight as possible, since people are literally
> > expected to include it in their binary if they want to boot via EFI
>
> I believe that Kojima-san's eficonfig can fill the requirements
> as we all expect.

Not really.  What about CI tools that rely on the cmd line to test the
efibootmgr?  We should force all of them to convert to a CI testing
that can interpret menus?

Cheers
/Ilias
>
> -Takahiro Akashi
>
>
> > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >
> > Regards
> > /Ilias