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 DBE69C4332F for ; Fri, 2 Dec 2022 07:36:08 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 7536785221; Fri, 2 Dec 2022 08:36:06 +0100 (CET) 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="feno1i2x"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id EBDD38525C; Fri, 2 Dec 2022 08:36:04 +0100 (CET) Received: from mail-wr1-x433.google.com (mail-wr1-x433.google.com [IPv6:2a00:1450:4864:20::433]) (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 31859851D9 for ; Fri, 2 Dec 2022 08:36:01 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=ilias.apalodimas@linaro.org Received: by mail-wr1-x433.google.com with SMTP id u12so5557704wrr.11 for ; Thu, 01 Dec 2022 23:36:01 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=gmsvVVtIvAU/QrG+YFRDitNlkPmvvoCFzv04csIOxtQ=; b=feno1i2xTToU5BCA9FdRuzjz84e3jOqlceWPfAtw1lLdVinQTdN8VeQDXg6tLxrC/H tHsgld/DXsrF05/IdNpRXrPCXVzfi0buxcVZZBbRTDKKyJxcQspL/s3AfaR0GCx8viD7 o86+fHE0dJVY/0gOeXp4QV5QRApRsETu+J2l2tNSLTT1VlIofmVJ/Cuz7PRw4ngdGlEn Trzu4/B1b2SP0SBUgklUN5U7DqNYGrqE/9WAfzmzMqbdbeUvSvswaTBmH4AIpN9l1Lrg /atBx7Sqxug6HDzxiO6kFbbdVWvdE5QPxhS4ECoKytWiToLIl9plsnhtCTPKL5KqfHsE rpLg== 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:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=gmsvVVtIvAU/QrG+YFRDitNlkPmvvoCFzv04csIOxtQ=; b=zY31OC9twP6XnrdJ3axWVGaxzmlpN2A56T3IQ+PJ2SXVLztGaE3T8f8xcAnHXq3ByY 1zYifjbJ4d+DTMh5NVykRfy4Fsg7rp/px8iO86UEzAcsRqvjlEo9uu9PAL3KsnoHE2QL 0jCjPyJF8C+xhTYaj8xJf619S5coFTpVxNVt8tPbJxIGZzOmmKFozX1PvWyR+0ordDGw ypTYpoyXJXITCmr3QlF6gK3WjPrdCpUaZFFiGirQqNB6P7IyuYcqR+YFP+JNSDnXGO/l XUR77ilI6akREaSekmHVtHoy2WsAPp9q4RvaGRyMblPt2N0vaXPZm5HLGklZFd3bSXuJ WyJA== X-Gm-Message-State: ANoB5pl3byOqqNv7xx9/a5bSeXg28+EucRhZy3DoqM0waMxdFJv8sBo5 NPlvh9jG4+4YRHwQ0DPFSUWJMA== X-Google-Smtp-Source: AA0mqf5jYVnj8RC2KbArkw/zCvdBloOrvS+jUI+dxlwIFBAxgfrNG7pyZMQVWhtj9FNPdJYGqwPX1g== X-Received: by 2002:a05:6000:16c2:b0:241:c56c:b485 with SMTP id h2-20020a05600016c200b00241c56cb485mr38283782wrf.566.1669966560578; Thu, 01 Dec 2022 23:36:00 -0800 (PST) Received: from hera (ppp078087234022.access.hol.gr. [78.87.234.22]) by smtp.gmail.com with ESMTPSA id q128-20020a1c4386000000b003c71358a42dsm15515144wma.18.2022.12.01.23.35.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 01 Dec 2022 23:36:00 -0800 (PST) Date: Fri, 2 Dec 2022 09:35:57 +0200 From: Ilias Apalodimas To: Masahisa Kojima Cc: u-boot@lists.denx.de, Heinrich Schuchardt , Jerome Forissier Subject: Re: [PATCH v3 4/5] eficonfig: use efi_get_next_variable_name_int() Message-ID: References: <20221202045937.7846-1-masahisa.kojima@linaro.org> <20221202045937.7846-5-masahisa.kojima@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20221202045937.7846-5-masahisa.kojima@linaro.org> 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 Fri, Dec 02, 2022 at 01:59:36PM +0900, Masahisa Kojima wrote: > eficonfig command reads all possible UEFI load options > from 0x0000 to 0xFFFF to construct the menu. This takes too much > time in some environment. > This commit uses efi_get_next_variable_name_int() to read all > existing UEFI load options to significantlly reduce the count of > efi_get_var() call. > > Signed-off-by: Masahisa Kojima > --- > No update since v2 > > v1->v2: > - totaly change the implemention, remove new Kconfig introduced in v1. > - use efi_get_next_variable_name_int() to read the all existing > UEFI variables, then enumerate the "Boot####" variables > - this commit does not provide the common function to enumerate > all "Boot####" variables. I think common function does not > simplify the implementation, because caller of efi_get_next_variable_name_int() > needs to remember original buffer size, new buffer size and buffer address > and it is a bit complicated when we consider to create common function. > > cmd/eficonfig.c | 141 +++++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 117 insertions(+), 24 deletions(-) > > diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c > index 88d507d04c..394ae67cce 100644 > --- a/cmd/eficonfig.c > +++ b/cmd/eficonfig.c > @@ -1683,7 +1683,8 @@ static efi_status_t eficonfig_show_boot_selection(unsigned int *selected) > u32 i; > u16 *bootorder; > efi_status_t ret; > - efi_uintn_t num, size; > + u16 *var_name16 = NULL, *p; > + efi_uintn_t num, size, buf_size; > struct efimenu *efi_menu; > struct list_head *pos, *n; > struct eficonfig_entry *entry; > @@ -1707,14 +1708,43 @@ static efi_status_t eficonfig_show_boot_selection(unsigned int *selected) > } > > /* list the remaining load option not included in the BootOrder */ > - for (i = 0; i <= 0xFFFF; i++) { > - /* If the index is included in the BootOrder, skip it */ > - if (search_bootorder(bootorder, num, i, NULL)) > - continue; > + buf_size = 128; > + var_name16 = malloc(buf_size); > + if (!var_name16) > + return EFI_OUT_OF_RESOURCES; > > - ret = eficonfig_add_boot_selection_entry(efi_menu, i, selected); > - if (ret != EFI_SUCCESS) > - goto out; > + var_name16[0] = 0; Is it worth using calloc instead of malloc and get rid of this? > + for (;;) { > + int index; > + efi_guid_t guid; > + > + size = buf_size; > + ret = efi_get_next_variable_name_int(&size, var_name16, &guid); > + if (ret == EFI_NOT_FOUND) > + break; > + if (ret == EFI_BUFFER_TOO_SMALL) { > + buf_size = size; > + p = realloc(var_name16, buf_size); > + if (!p) { > + free(var_name16); > + return EFI_OUT_OF_RESOURCES; > + } > + var_name16 = p; > + ret = efi_get_next_variable_name_int(&size, var_name16, &guid); > + } > + if (ret != EFI_SUCCESS) { > + free(var_name16); > + return ret; > + } > + if (efi_varname_is_load_option(var_name16, &index)) { > + /* If the index is included in the BootOrder, skip it */ > + if (search_bootorder(bootorder, num, index, NULL)) > + continue; > + > + ret = eficonfig_add_boot_selection_entry(efi_menu, index, selected); > + if (ret != EFI_SUCCESS) > + goto out; > + } > > if (efi_menu->count >= EFICONFIG_ENTRY_NUM_MAX - 1) > break; > @@ -1732,6 +1762,8 @@ out: > } > eficonfig_destroy(efi_menu); > > + free(var_name16); > + > return ret; > } > > @@ -1994,6 +2026,8 @@ static efi_status_t eficonfig_create_change_boot_order_entry(struct efimenu *efi > u32 i; > char *title; > efi_status_t ret; > + u16 *var_name16 = NULL, *p; > + efi_uintn_t size, buf_size; > > /* list the load option in the order of BootOrder variable */ > for (i = 0; i < num; i++) { > @@ -2006,17 +2040,45 @@ static efi_status_t eficonfig_create_change_boot_order_entry(struct efimenu *efi > } > > /* list the remaining load option not included in the BootOrder */ > - for (i = 0; i < 0xFFFF; i++) { > + buf_size = 128; > + var_name16 = malloc(buf_size); > + if (!var_name16) > + return EFI_OUT_OF_RESOURCES; > + > + var_name16[0] = 0; > + for (;;) { > + int index; > + efi_guid_t guid; > + > if (efi_menu->count >= EFICONFIG_ENTRY_NUM_MAX - 2) > break; > > - /* If the index is included in the BootOrder, skip it */ > - if (search_bootorder(bootorder, num, i, NULL)) > - continue; > - > - ret = eficonfig_add_change_boot_order_entry(efi_menu, i, false); > + size = buf_size; > + ret = efi_get_next_variable_name_int(&size, var_name16, &guid); > + if (ret == EFI_NOT_FOUND) > + break; > + if (ret == EFI_BUFFER_TOO_SMALL) { > + buf_size = size; > + p = realloc(var_name16, buf_size); > + if (!p) { > + ret = EFI_OUT_OF_RESOURCES; > + goto out; > + } > + var_name16 = p; > + ret = efi_get_next_variable_name_int(&size, var_name16, &guid); > + } > if (ret != EFI_SUCCESS) > goto out; > + > + if (efi_varname_is_load_option(var_name16, &index)) { > + /* If the index is included in the BootOrder, skip it */ > + if (search_bootorder(bootorder, num, index, NULL)) > + continue; > + > + ret = eficonfig_add_change_boot_order_entry(efi_menu, index, false); > + if (ret != EFI_SUCCESS) > + goto out; > + } > } > > /* add "Save" and "Quit" entries */ > @@ -2035,9 +2097,9 @@ static efi_status_t eficonfig_create_change_boot_order_entry(struct efimenu *efi > goto out; > > efi_menu->active = 0; > - > - return EFI_SUCCESS; > out: > + free(var_name16); > + > return ret; > } > > @@ -2270,17 +2332,48 @@ out: > efi_status_t eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_option *opt, > efi_status_t count) > { > - u32 i, j; > + u32 i; > efi_uintn_t size; > void *load_option; > struct efi_load_option lo; > + u16 *var_name16 = NULL, *p; > u16 varname[] = u"Boot####"; > efi_status_t ret = EFI_SUCCESS; > + efi_uintn_t varname_size, buf_size; > > - for (i = 0; i <= 0xFFFF; i++) { > + buf_size = 128; > + var_name16 = malloc(buf_size); > + if (!var_name16) > + return EFI_OUT_OF_RESOURCES; > + > + var_name16[0] = 0; > + for (;;) { > + int index; > + efi_guid_t guid; > efi_uintn_t tmp; > > - efi_create_indexed_name(varname, sizeof(varname), "Boot", i); > + varname_size = buf_size; > + ret = efi_get_next_variable_name_int(&varname_size, var_name16, &guid); > + if (ret == EFI_NOT_FOUND) > + break; > + if (ret == EFI_BUFFER_TOO_SMALL) { > + buf_size = varname_size; > + p = realloc(var_name16, buf_size); > + if (!p) { > + free(var_name16); > + return EFI_OUT_OF_RESOURCES; > + } > + var_name16 = p; > + ret = efi_get_next_variable_name_int(&varname_size, var_name16, &guid); > + } > + if (ret != EFI_SUCCESS) { > + free(var_name16); > + return ret; > + } > + if (!efi_varname_is_load_option(var_name16, &index)) > + continue; > + > + efi_create_indexed_name(varname, sizeof(varname), "Boot", index); > load_option = efi_get_var(varname, &efi_global_variable_guid, &size); > if (!load_option) > continue; > @@ -2292,15 +2385,15 @@ efi_status_t eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_op > > if (size >= sizeof(efi_guid_bootmenu_auto_generated)) { > if (guidcmp(lo.optional_data, &efi_guid_bootmenu_auto_generated) == 0) { > - for (j = 0; j < count; j++) { > - if (opt[j].size == tmp && > - memcmp(opt[j].lo, load_option, tmp) == 0) { > - opt[j].exist = true; > + for (i = 0; i < count; i++) { > + if (opt[i].size == tmp && > + memcmp(opt[i].lo, load_option, tmp) == 0) { > + opt[i].exist = true; > break; > } > } > > - if (j == count) { > + if (i == count) { > ret = delete_boot_option(i); > if (ret != EFI_SUCCESS) { > free(load_option); > -- > 2.17.1 > Overall this looks correct and a good idea. The sequence of alloc -> efi_get_next_variable_name_int -> realloc -> efi_get_next_variable_name_int seems common and repeated though. Can we factor that out on a common function? Thanks /Ilias