* [PATCH v3 0/5] miscellaneous fixes of eficonfig
@ 2022-12-02 4:59 Masahisa Kojima
2022-12-02 4:59 ` [PATCH v3 1/5] eficonfig: fix going one directory up issue Masahisa Kojima
` (4 more replies)
0 siblings, 5 replies; 13+ messages in thread
From: Masahisa Kojima @ 2022-12-02 4:59 UTC (permalink / raw)
To: u-boot
Cc: Heinrich Schuchardt, Ilias Apalodimas, Jerome Forissier, Masahisa Kojima
This series includes bugfix, refactoring and documentation
updates.
Masahisa Kojima (5):
eficonfig: fix going one directory up issue
eficonfig: use u16_strsize() to get u16 string buffer size
efi_loader: utility function to check the variable name is "Boot####"
eficonfig: use efi_get_next_variable_name_int()
doc:eficonfig: add description for UEFI Secure Boot Configuration
cmd/eficonfig.c | 146 +++++++++++++++++++++++++++++-------
cmd/efidebug.c | 23 +-----
doc/usage/cmd/eficonfig.rst | 22 ++++++
include/efi_loader.h | 1 +
lib/efi_loader/efi_helper.c | 33 ++++++++
5 files changed, 176 insertions(+), 49 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 1/5] eficonfig: fix going one directory up issue
2022-12-02 4:59 [PATCH v3 0/5] miscellaneous fixes of eficonfig Masahisa Kojima
@ 2022-12-02 4:59 ` Masahisa Kojima
2022-12-02 4:59 ` [PATCH v3 2/5] eficonfig: use u16_strsize() to get u16 string buffer size Masahisa Kojima
` (3 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Masahisa Kojima @ 2022-12-02 4:59 UTC (permalink / raw)
To: u-boot
Cc: Heinrich Schuchardt, Ilias Apalodimas, Jerome Forissier, Masahisa Kojima
The directory name in eficonfig menu entry contains the
'\' separator. strcmp() argument ".." is wrong and one directory
up handling does not work correctly. strcmp() argument must
include '\' separator.
Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
No change since v1
cmd/eficonfig.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
index 97d35597a2..5529edc85e 100644
--- a/cmd/eficonfig.c
+++ b/cmd/eficonfig.c
@@ -488,7 +488,7 @@ static efi_status_t eficonfig_file_selected(void *data)
if (!info)
return EFI_INVALID_PARAMETER;
- if (!strcmp(info->file_name, "..")) {
+ if (!strcmp(info->file_name, "..\\")) {
struct eficonfig_filepath_info *iter;
struct list_head *pos, *n;
int is_last;
--
2.17.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 2/5] eficonfig: use u16_strsize() to get u16 string buffer size
2022-12-02 4:59 [PATCH v3 0/5] miscellaneous fixes of eficonfig Masahisa Kojima
2022-12-02 4:59 ` [PATCH v3 1/5] eficonfig: fix going one directory up issue Masahisa Kojima
@ 2022-12-02 4:59 ` Masahisa Kojima
2022-12-02 4:59 ` [PATCH v3 3/5] efi_loader: utility function to check the variable name is "Boot####" Masahisa Kojima
` (2 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Masahisa Kojima @ 2022-12-02 4:59 UTC (permalink / raw)
To: u-boot
Cc: Heinrich Schuchardt, Ilias Apalodimas, Jerome Forissier, Masahisa Kojima
Use u16_strsize() to simplify the u16 string buffer
size calculation.
Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
No update since v1.
cmd/eficonfig.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
index 5529edc85e..88d507d04c 100644
--- a/cmd/eficonfig.c
+++ b/cmd/eficonfig.c
@@ -452,8 +452,7 @@ struct efi_device_path *eficonfig_create_device_path(struct efi_device_path *dp_
struct efi_device_path *dp;
struct efi_device_path_file_path *fp;
- fp_size = sizeof(struct efi_device_path) +
- ((u16_strlen(current_path) + 1) * sizeof(u16));
+ fp_size = sizeof(struct efi_device_path) + u16_strsize(current_path);
buf = calloc(1, fp_size + sizeof(END));
if (!buf)
return NULL;
--
2.17.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 3/5] efi_loader: utility function to check the variable name is "Boot####"
2022-12-02 4:59 [PATCH v3 0/5] miscellaneous fixes of eficonfig Masahisa Kojima
2022-12-02 4:59 ` [PATCH v3 1/5] eficonfig: fix going one directory up issue Masahisa Kojima
2022-12-02 4:59 ` [PATCH v3 2/5] eficonfig: use u16_strsize() to get u16 string buffer size Masahisa Kojima
@ 2022-12-02 4:59 ` Masahisa Kojima
2022-12-02 4:59 ` [PATCH v3 4/5] eficonfig: use efi_get_next_variable_name_int() Masahisa Kojima
2022-12-02 4:59 ` [PATCH v3 5/5] doc:eficonfig: add description for UEFI Secure Boot Configuration Masahisa Kojima
4 siblings, 0 replies; 13+ messages in thread
From: Masahisa Kojima @ 2022-12-02 4:59 UTC (permalink / raw)
To: u-boot
Cc: Heinrich Schuchardt, Ilias Apalodimas, Jerome Forissier, Masahisa Kojima
Some commands need to enumerate the existing UEFI load
option variable("Boot####"). This commit transfers some code
from cmd/efidebug.c to lib/efi_loder/, then exposes
efi_varname_is_load_option() function to check whether
the UEFI variable name is "Boot####".
Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
v2->v3:
- add static qualifier to u16_tohex()
Newly created in v2
cmd/efidebug.c | 23 +----------------------
include/efi_loader.h | 1 +
lib/efi_loader/efi_helper.c | 33 +++++++++++++++++++++++++++++++++
3 files changed, 35 insertions(+), 22 deletions(-)
diff --git a/cmd/efidebug.c b/cmd/efidebug.c
index ef239bb34b..ceb3aa5cee 100644
--- a/cmd/efidebug.c
+++ b/cmd/efidebug.c
@@ -1010,17 +1010,6 @@ static void show_efi_boot_opt(u16 *varname16)
}
}
-static int u16_tohex(u16 c)
-{
- if (c >= '0' && c <= '9')
- return c - '0';
- if (c >= 'A' && c <= 'F')
- return c - 'A' + 10;
-
- /* not hexadecimal */
- return -1;
-}
-
/**
* show_efi_boot_dump() - dump all UEFI load options
*
@@ -1041,7 +1030,6 @@ static int do_efi_boot_dump(struct cmd_tbl *cmdtp, int flag,
u16 *var_name16, *p;
efi_uintn_t buf_size, size;
efi_guid_t guid;
- int id, i, digit;
efi_status_t ret;
if (argc > 1)
@@ -1074,16 +1062,7 @@ static int do_efi_boot_dump(struct cmd_tbl *cmdtp, int flag,
return CMD_RET_FAILURE;
}
- if (memcmp(var_name16, u"Boot", 8))
- continue;
-
- for (id = 0, i = 0; i < 4; i++) {
- digit = u16_tohex(var_name16[4 + i]);
- if (digit < 0)
- break;
- id = (id << 4) + digit;
- }
- if (i == 4 && !var_name16[8])
+ if (efi_varname_is_load_option(var_name16, NULL))
show_efi_boot_opt(var_name16);
}
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 0c6c95ba46..0899e293e5 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -707,6 +707,7 @@ int algo_to_len(const char *algo);
int efi_link_dev(efi_handle_t handle, struct udevice *dev);
int efi_unlink_dev(efi_handle_t handle);
+bool efi_varname_is_load_option(u16 *var_name16, int *index);
/**
* efi_size_in_pages() - convert size in bytes to size in pages
diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
index c71e87d118..788cb9faec 100644
--- a/lib/efi_loader/efi_helper.c
+++ b/lib/efi_loader/efi_helper.c
@@ -190,3 +190,36 @@ int efi_unlink_dev(efi_handle_t handle)
return 0;
}
+
+static int u16_tohex(u16 c)
+{
+ if (c >= '0' && c <= '9')
+ return c - '0';
+ if (c >= 'A' && c <= 'F')
+ return c - 'A' + 10;
+
+ /* not hexadecimal */
+ return -1;
+}
+
+bool efi_varname_is_load_option(u16 *var_name16, int *index)
+{
+ int id, i, digit;
+
+ if (memcmp(var_name16, u"Boot", 8))
+ return false;
+
+ for (id = 0, i = 0; i < 4; i++) {
+ digit = u16_tohex(var_name16[4 + i]);
+ if (digit < 0)
+ break;
+ id = (id << 4) + digit;
+ }
+ if (i == 4 && !var_name16[8]) {
+ if (index)
+ *index = id;
+ return true;
+ }
+
+ return false;
+}
--
2.17.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 4/5] eficonfig: use efi_get_next_variable_name_int()
2022-12-02 4:59 [PATCH v3 0/5] miscellaneous fixes of eficonfig Masahisa Kojima
` (2 preceding siblings ...)
2022-12-02 4:59 ` [PATCH v3 3/5] efi_loader: utility function to check the variable name is "Boot####" Masahisa Kojima
@ 2022-12-02 4:59 ` Masahisa Kojima
2022-12-02 7:35 ` Ilias Apalodimas
2022-12-02 4:59 ` [PATCH v3 5/5] doc:eficonfig: add description for UEFI Secure Boot Configuration Masahisa Kojima
4 siblings, 1 reply; 13+ messages in thread
From: Masahisa Kojima @ 2022-12-02 4:59 UTC (permalink / raw)
To: u-boot
Cc: Heinrich Schuchardt, Ilias Apalodimas, Jerome Forissier, Masahisa Kojima
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 <masahisa.kojima@linaro.org>
---
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;
+ 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 5/5] doc:eficonfig: add description for UEFI Secure Boot Configuration
2022-12-02 4:59 [PATCH v3 0/5] miscellaneous fixes of eficonfig Masahisa Kojima
` (3 preceding siblings ...)
2022-12-02 4:59 ` [PATCH v3 4/5] eficonfig: use efi_get_next_variable_name_int() Masahisa Kojima
@ 2022-12-02 4:59 ` Masahisa Kojima
2022-12-02 7:17 ` Ilias Apalodimas
4 siblings, 1 reply; 13+ messages in thread
From: Masahisa Kojima @ 2022-12-02 4:59 UTC (permalink / raw)
To: u-boot
Cc: Heinrich Schuchardt, Ilias Apalodimas, Jerome Forissier, Masahisa Kojima
This commits add the description for the UEFI Secure Boot
Configuration through the eficonfig menu.
Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
No update since v2
Newly created in v2
doc/usage/cmd/eficonfig.rst | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/doc/usage/cmd/eficonfig.rst b/doc/usage/cmd/eficonfig.rst
index 340ebc80db..67c859964f 100644
--- a/doc/usage/cmd/eficonfig.rst
+++ b/doc/usage/cmd/eficonfig.rst
@@ -31,6 +31,9 @@ Change Boot Order
Delete Boot Option
Delete the UEFI Boot Option
+Secure Boot Configuration
+ Edit UEFI Secure Boot Configuration
+
Configuration
-------------
@@ -44,6 +47,16 @@ U-Boot console. In this case, bootmenu can be used to invoke "eficonfig"::
CONFIG_USE_PREBOOT=y
CONFIG_PREBOOT="setenv bootmenu_0 UEFI Maintenance Menu=eficonfig"
+UEFI specification requires that UEFI Secure Boot Configuration (especially
+for PK and KEK) is stored in non-volatile storage which is tamper resident.
+CONFIG_EFI_MM_COMM_TEE is mandatory to provide the secure storage in U-Boot.
+UEFI Secure Boot Configuration menu entry is enabled when the following
+options are enabled::
+
+ CONFIG_EFI_SECURE_BOOT=y
+ CONFIG_EFI_MM_COMM_TEE=y
+
+
How to boot the system with newly added UEFI Boot Option
''''''''''''''''''''''''''''''''''''''''''''''''''''''''
@@ -66,6 +79,15 @@ add "bootefi bootmgr" entry as a default or first bootmenu entry::
CONFIG_PREBOOT="setenv bootmenu_0 UEFI Boot Manager=bootefi bootmgr; setenv bootmenu_1 UEFI Maintenance Menu=eficonfig"
+UEFI Secure Boot Configuration
+''''''''''''''''''''''''''''''
+
+User can enroll PK, KEK, db and dbx by selecting file.
+"eficonfig" command only accepts the signed EFI Signature List(s)
+with an authenticated header, typically ".auth" file.
+To clear the PK, KEK, db and dbx, user needs to enroll the null key
+signed by PK or KEK.
+
See also
--------
* :doc:`bootmenu<bootmenu>` provides a simple mechanism for creating menus with different boot items
--
2.17.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 5/5] doc:eficonfig: add description for UEFI Secure Boot Configuration
2022-12-02 4:59 ` [PATCH v3 5/5] doc:eficonfig: add description for UEFI Secure Boot Configuration Masahisa Kojima
@ 2022-12-02 7:17 ` Ilias Apalodimas
2022-12-02 13:23 ` Masahisa Kojima
0 siblings, 1 reply; 13+ messages in thread
From: Ilias Apalodimas @ 2022-12-02 7:17 UTC (permalink / raw)
To: Masahisa Kojima; +Cc: u-boot, Heinrich Schuchardt, Jerome Forissier
On Fri, Dec 02, 2022 at 01:59:37PM +0900, Masahisa Kojima wrote:
> This commits add the description for the UEFI Secure Boot
> Configuration through the eficonfig menu.
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
> No update since v2
>
> Newly created in v2
>
> doc/usage/cmd/eficonfig.rst | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
> diff --git a/doc/usage/cmd/eficonfig.rst b/doc/usage/cmd/eficonfig.rst
> index 340ebc80db..67c859964f 100644
> --- a/doc/usage/cmd/eficonfig.rst
> +++ b/doc/usage/cmd/eficonfig.rst
> @@ -31,6 +31,9 @@ Change Boot Order
> Delete Boot Option
> Delete the UEFI Boot Option
>
> +Secure Boot Configuration
> + Edit UEFI Secure Boot Configuration
> +
> Configuration
> -------------
>
> @@ -44,6 +47,16 @@ U-Boot console. In this case, bootmenu can be used to invoke "eficonfig"::
> CONFIG_USE_PREBOOT=y
> CONFIG_PREBOOT="setenv bootmenu_0 UEFI Maintenance Menu=eficonfig"
>
> +UEFI specification requires that UEFI Secure Boot Configuration (especially
> +for PK and KEK) is stored in non-volatile storage which is tamper resident.
s/resident/resistant
> +CONFIG_EFI_MM_COMM_TEE is mandatory to provide the secure storage in U-Boot.
Can we be a bit more clear here. Something along the lines of
"The only way U-Boot can currently store EFI variables on a tamper
resistant medium is via OP-TEE. The Kconfig option that enables that is
CONFIG_EFI_MM_COMM_TEE and ends up storing EFI variables on an RPMB
partition of an eMMC"
> +UEFI Secure Boot Configuration menu entry is enabled when the following
> +options are enabled::
> +
> + CONFIG_EFI_SECURE_BOOT=y
> + CONFIG_EFI_MM_COMM_TEE=y
> +
> +
> How to boot the system with newly added UEFI Boot Option
> ''''''''''''''''''''''''''''''''''''''''''''''''''''''''
>
> @@ -66,6 +79,15 @@ add "bootefi bootmgr" entry as a default or first bootmenu entry::
>
> CONFIG_PREBOOT="setenv bootmenu_0 UEFI Boot Manager=bootefi bootmgr; setenv bootmenu_1 UEFI Maintenance Menu=eficonfig"
>
> +UEFI Secure Boot Configuration
> +''''''''''''''''''''''''''''''
> +
> +User can enroll PK, KEK, db and dbx by selecting file.
selecting a file
> +"eficonfig" command only accepts the signed EFI Signature List(s)
> +with an authenticated header, typically ".auth" file.
> +To clear the PK, KEK, db and dbx, user needs to enroll the null key
> +signed by PK or KEK.
> +
> See also
> --------
> * :doc:`bootmenu<bootmenu>` provides a simple mechanism for creating menus with different boot items
> --
> 2.17.1
>
Thanks
/Ilias
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 4/5] eficonfig: use efi_get_next_variable_name_int()
2022-12-02 4:59 ` [PATCH v3 4/5] eficonfig: use efi_get_next_variable_name_int() Masahisa Kojima
@ 2022-12-02 7:35 ` Ilias Apalodimas
2022-12-02 16:59 ` Heinrich Schuchardt
2022-12-03 0:56 ` Masahisa Kojima
0 siblings, 2 replies; 13+ messages in thread
From: Ilias Apalodimas @ 2022-12-02 7:35 UTC (permalink / raw)
To: Masahisa Kojima; +Cc: u-boot, Heinrich Schuchardt, Jerome Forissier
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 <masahisa.kojima@linaro.org>
> ---
> 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 5/5] doc:eficonfig: add description for UEFI Secure Boot Configuration
2022-12-02 7:17 ` Ilias Apalodimas
@ 2022-12-02 13:23 ` Masahisa Kojima
0 siblings, 0 replies; 13+ messages in thread
From: Masahisa Kojima @ 2022-12-02 13:23 UTC (permalink / raw)
To: Ilias Apalodimas; +Cc: u-boot, Heinrich Schuchardt, Jerome Forissier
Hi Ilias,
On Fri, 2 Dec 2022 at 16:17, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Fri, Dec 02, 2022 at 01:59:37PM +0900, Masahisa Kojima wrote:
> > This commits add the description for the UEFI Secure Boot
> > Configuration through the eficonfig menu.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> > No update since v2
> >
> > Newly created in v2
> >
> > doc/usage/cmd/eficonfig.rst | 22 ++++++++++++++++++++++
> > 1 file changed, 22 insertions(+)
> >
> > diff --git a/doc/usage/cmd/eficonfig.rst b/doc/usage/cmd/eficonfig.rst
> > index 340ebc80db..67c859964f 100644
> > --- a/doc/usage/cmd/eficonfig.rst
> > +++ b/doc/usage/cmd/eficonfig.rst
> > @@ -31,6 +31,9 @@ Change Boot Order
> > Delete Boot Option
> > Delete the UEFI Boot Option
> >
> > +Secure Boot Configuration
> > + Edit UEFI Secure Boot Configuration
> > +
> > Configuration
> > -------------
> >
> > @@ -44,6 +47,16 @@ U-Boot console. In this case, bootmenu can be used to invoke "eficonfig"::
> > CONFIG_USE_PREBOOT=y
> > CONFIG_PREBOOT="setenv bootmenu_0 UEFI Maintenance Menu=eficonfig"
> >
> > +UEFI specification requires that UEFI Secure Boot Configuration (especially
> > +for PK and KEK) is stored in non-volatile storage which is tamper resident.
>
> s/resident/resistant
Thank you for pointing out the typo.
>
> > +CONFIG_EFI_MM_COMM_TEE is mandatory to provide the secure storage in U-Boot.
>
> Can we be a bit more clear here. Something along the lines of
> "The only way U-Boot can currently store EFI variables on a tamper
> resistant medium is via OP-TEE. The Kconfig option that enables that is
> CONFIG_EFI_MM_COMM_TEE and ends up storing EFI variables on an RPMB
> partition of an eMMC"
OK, it is clearer.
>
> > +UEFI Secure Boot Configuration menu entry is enabled when the following
> > +options are enabled::
> > +
> > + CONFIG_EFI_SECURE_BOOT=y
> > + CONFIG_EFI_MM_COMM_TEE=y
> > +
> > +
> > How to boot the system with newly added UEFI Boot Option
> > ''''''''''''''''''''''''''''''''''''''''''''''''''''''''
> >
> > @@ -66,6 +79,15 @@ add "bootefi bootmgr" entry as a default or first bootmenu entry::
> >
> > CONFIG_PREBOOT="setenv bootmenu_0 UEFI Boot Manager=bootefi bootmgr; setenv bootmenu_1 UEFI Maintenance Menu=eficonfig"
> >
> > +UEFI Secure Boot Configuration
> > +''''''''''''''''''''''''''''''
> > +
> > +User can enroll PK, KEK, db and dbx by selecting file.
>
> selecting a file
OK.
Thanks,
Masahisa Kojima
>
> > +"eficonfig" command only accepts the signed EFI Signature List(s)
> > +with an authenticated header, typically ".auth" file.
> > +To clear the PK, KEK, db and dbx, user needs to enroll the null key
> > +signed by PK or KEK.
> > +
> > See also
> > --------
> > * :doc:`bootmenu<bootmenu>` provides a simple mechanism for creating menus with different boot items
> > --
> > 2.17.1
> >
>
> Thanks
> /Ilias
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 4/5] eficonfig: use efi_get_next_variable_name_int()
2022-12-02 7:35 ` Ilias Apalodimas
@ 2022-12-02 16:59 ` Heinrich Schuchardt
2022-12-03 0:56 ` Masahisa Kojima
1 sibling, 0 replies; 13+ messages in thread
From: Heinrich Schuchardt @ 2022-12-02 16:59 UTC (permalink / raw)
To: Ilias Apalodimas; +Cc: u-boot, Jerome Forissier, Masahisa Kojima
On 12/2/22 08:35, Ilias Apalodimas wrote:
> 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 <masahisa.kojima@linaro.org>
>> ---
>> 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?
It doesn't change the binary code size as var_name16 is already in a
register.
Best regards
Heinrich
>
>> + 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 4/5] eficonfig: use efi_get_next_variable_name_int()
2022-12-02 7:35 ` Ilias Apalodimas
2022-12-02 16:59 ` Heinrich Schuchardt
@ 2022-12-03 0:56 ` Masahisa Kojima
2022-12-06 14:12 ` Ilias Apalodimas
1 sibling, 1 reply; 13+ messages in thread
From: Masahisa Kojima @ 2022-12-03 0:56 UTC (permalink / raw)
To: Ilias Apalodimas; +Cc: u-boot, Heinrich Schuchardt, Jerome Forissier
On Fri, 2 Dec 2022 at 16:36, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> 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 <masahisa.kojima@linaro.org>
> > ---
> > 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?
I tried to factor out these sequences into a common function, but I
could not find
proper common function interface.
Even if we factor out some of these sequences, the caller still should
take care the cases
of ret == EFI_NOT_FOUND and ret != EFI_SUCCESS.
We can factor out the EFI_BUFFER_TOO_SMALL and realloc sequence, but I
think it will not
make the caller simpler.
Thanks,
Masahisa Kojima
>
> Thanks
> /Ilias
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 4/5] eficonfig: use efi_get_next_variable_name_int()
2022-12-03 0:56 ` Masahisa Kojima
@ 2022-12-06 14:12 ` Ilias Apalodimas
2022-12-07 7:19 ` Masahisa Kojima
0 siblings, 1 reply; 13+ messages in thread
From: Ilias Apalodimas @ 2022-12-06 14:12 UTC (permalink / raw)
To: Masahisa Kojima; +Cc: u-boot, Heinrich Schuchardt, Jerome Forissier
On Sat, Dec 03, 2022 at 09:56:20AM +0900, Masahisa Kojima wrote:
> On Fri, 2 Dec 2022 at 16:36, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > 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 <masahisa.kojima@linaro.org>
> > > ---
> > > 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?
>
> I tried to factor out these sequences into a common function, but I
> could not find
> proper common function interface.
> Even if we factor out some of these sequences, the caller still should
> take care the cases
> of ret == EFI_NOT_FOUND and ret != EFI_SUCCESS.
> We can factor out the EFI_BUFFER_TOO_SMALL and realloc sequence, but I
> think it will not
> make the caller simpler.
I think mean the same thing here, but let me make sure.
This snippet seems like it could be it's own function, no?
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);
}
Regards
/Ilias
>
> Thanks,
> Masahisa Kojima
>
> >
> > Thanks
> > /Ilias
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 4/5] eficonfig: use efi_get_next_variable_name_int()
2022-12-06 14:12 ` Ilias Apalodimas
@ 2022-12-07 7:19 ` Masahisa Kojima
0 siblings, 0 replies; 13+ messages in thread
From: Masahisa Kojima @ 2022-12-07 7:19 UTC (permalink / raw)
To: Ilias Apalodimas; +Cc: u-boot, Heinrich Schuchardt, Jerome Forissier
On Tue, 6 Dec 2022 at 23:12, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Sat, Dec 03, 2022 at 09:56:20AM +0900, Masahisa Kojima wrote:
> > On Fri, 2 Dec 2022 at 16:36, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > 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 <masahisa.kojima@linaro.org>
> > > > ---
> > > > 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?
> >
> > I tried to factor out these sequences into a common function, but I
> > could not find
> > proper common function interface.
> > Even if we factor out some of these sequences, the caller still should
> > take care the cases
> > of ret == EFI_NOT_FOUND and ret != EFI_SUCCESS.
> > We can factor out the EFI_BUFFER_TOO_SMALL and realloc sequence, but I
> > think it will not
> > make the caller simpler.
>
> I think mean the same thing here, but let me make sure.
> This snippet seems like it could be it's own function, no?
>
> 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);
> }
OK, I will create a common function.
This patch was already merged, I will send a new patch.
Thanks,
Masahisa Kojima
>
> Regards
> /Ilias
> >
> > Thanks,
> > Masahisa Kojima
> >
> > >
> > > Thanks
> > > /Ilias
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-12-07 7:20 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-02 4:59 [PATCH v3 0/5] miscellaneous fixes of eficonfig Masahisa Kojima
2022-12-02 4:59 ` [PATCH v3 1/5] eficonfig: fix going one directory up issue Masahisa Kojima
2022-12-02 4:59 ` [PATCH v3 2/5] eficonfig: use u16_strsize() to get u16 string buffer size Masahisa Kojima
2022-12-02 4:59 ` [PATCH v3 3/5] efi_loader: utility function to check the variable name is "Boot####" Masahisa Kojima
2022-12-02 4:59 ` [PATCH v3 4/5] eficonfig: use efi_get_next_variable_name_int() Masahisa Kojima
2022-12-02 7:35 ` Ilias Apalodimas
2022-12-02 16:59 ` Heinrich Schuchardt
2022-12-03 0:56 ` Masahisa Kojima
2022-12-06 14:12 ` Ilias Apalodimas
2022-12-07 7:19 ` Masahisa Kojima
2022-12-02 4:59 ` [PATCH v3 5/5] doc:eficonfig: add description for UEFI Secure Boot Configuration Masahisa Kojima
2022-12-02 7:17 ` Ilias Apalodimas
2022-12-02 13:23 ` Masahisa Kojima
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.