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

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

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