From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id D4DB6C433FE for ; Wed, 5 Oct 2022 08:47:26 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 05AE684CEF; Wed, 5 Oct 2022 10:47:24 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="nwY3RImz"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 214A084CA9; Wed, 5 Oct 2022 10:47:22 +0200 (CEST) Received: from mail-pj1-x1033.google.com (mail-pj1-x1033.google.com [IPv6:2607:f8b0:4864:20::1033]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id D915784CF4 for ; Wed, 5 Oct 2022 10:47:16 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=takahiro.akashi@linaro.org Received: by mail-pj1-x1033.google.com with SMTP id h8-20020a17090a054800b00205ccbae31eso1017070pjf.5 for ; Wed, 05 Oct 2022 01:47:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:cc:to:from:date:from:to:cc :subject:date:message-id:reply-to; bh=AsEq7Y3a2TW2OPUQUz6Ms50Xx1vLRVgV6llmaytjO7M=; b=nwY3RImzpHwLW1GGHw03LrXYDVYXAITK3hVuz2JSklUq0nwVYclnTckQTs87zCvYxM Hk04o4RRi2gNuf97ga9QHkuGPw1L7KHbSSxDkzKBkx4+8Cx6MaGyUzEJQcV3rC271x/p 57RyrbXg71cohaPv3omk6VSOEWl3cPts7cd3gqg+6G3QJFgy0o4MODeHBoQGDzuAjqx7 t2729WURJ5+bf8ZfBc+DOqv9FWU2l2vb+pN+yLS1PStS02Tbi9+mTU8VjaUBh8dDyJqr 4w40N6emaT95X2XOP8Z63ZljA2Zvv89mwCk5055hBUmZ63wLzyqRjhsKpWc0CYxerehj 0NCA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=AsEq7Y3a2TW2OPUQUz6Ms50Xx1vLRVgV6llmaytjO7M=; b=SAc0OQ5xFzLRFkp9sDV1q+Z2CK86iUxLTtvkjHqlZue1yU5N5o4kXw4ceFnJHw8NwT +Wf75/eiSH6S1rw/BUGoU3IpnDR24H/U++3KM2s4zaa5tixS+g6V5KwpR1IaCpS2a973 zs73TTAjoFP/KeGMjfk9SMB8wvhpM74RgxChAOYlAWm9QKZaEu8CO0MJ+RHudtQDW2Zk WHh87elX4Br4gAL4f72wZM7E0FoLyPiL45kIXHT0epO+jBwXpxjbzVGfer1VKpUVnSZE yn8TeJRN3Re9ZBWXKe7EUfd8wbTdBcEJqAtyVjmEAk8Lnsxygiu8CtJXVlwOO/c+fB6W u/Qw== X-Gm-Message-State: ACrzQf0WF5VMSWxANbO6+ngjB4nWGcM80Vet/XRTwo49b5OgCicSlUnb xPgVccwzD3jMw1gmkxi5nySTaA== X-Google-Smtp-Source: AMsMyM725OyOdbDz8sZLK2KG5LPa1rSuMK4Gt8+SEqu/uDSegfqit71IL+id9rR8hoLeRAzVZYnZvg== X-Received: by 2002:a17:902:ce8f:b0:176:d5af:a175 with SMTP id f15-20020a170902ce8f00b00176d5afa175mr32000462plg.123.1664959634625; Wed, 05 Oct 2022 01:47:14 -0700 (PDT) Received: from laputa ([2400:4050:c3e1:100:39e7:db31:fd97:9c8d]) by smtp.gmail.com with ESMTPSA id n10-20020a65488a000000b0042bf6034b3fsm9777381pgs.55.2022.10.05.01.47.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 05 Oct 2022 01:47:13 -0700 (PDT) Date: Wed, 5 Oct 2022 17:47:10 +0900 From: AKASHI Takahiro To: Ilias Apalodimas Cc: Heinrich Schuchardt , u-boot@lists.denx.de Subject: Re: [PATCH 1/1] cmd: simplify command efidebug Message-ID: <20221005084710.GA61125@laputa> Mail-Followup-To: AKASHI Takahiro , Ilias Apalodimas , Heinrich Schuchardt , u-boot@lists.denx.de References: <20221004134032.128142-1-heinrich.schuchardt@canonical.com> <20221005002330.GA34350@laputa> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean 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 > > > > --- > > > > 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 > > > > #include > > > > #include > > > > +#include > > > > #include > > > > #include > > > > #include > > > > @@ -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 > > Regards > /Ilias