[PATCH v2 0/5] miscellaneous fixes of eficonfig

[PATCH v2 0/5] miscellaneous fixes of eficonfig

[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        |   2 +
 lib/efi_loader/efi_helper.c |  33 ++++++++
 5 files changed, 177 insertions(+), 49 deletions(-)

-- 
2.17.1

[PATCH v2 1/5] eficonfig: fix going one directory up issue

[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>
---
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

[PATCH v2 2/5] eficonfig: use u16_strsize() to get u16 string buffer size

[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>
---
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

[PATCH v2 3/5] efi_loader: utility function to check the variable name is "Boot####"

[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 u16_tohex() and
efi_varname_is_load_option() function to check whether the
UEFI variable name is "Boot####".

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
Newly created in v2

 cmd/efidebug.c              | 23 +----------------------
 include/efi_loader.h        |  2 ++
 lib/efi_loader/efi_helper.c | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 36 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..b1ded811e7 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -707,6 +707,8 @@ 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);
+int u16_tohex(u16 c);
+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..8a7afcc381 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;
 }
+
+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

[PATCH v2 4/5] eficonfig: use efi_get_next_variable_name_int()

[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>
---
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

[PATCH v2 5/5] doc:eficonfig: add description for UEFI Secure Boot Configuration

[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>
---
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

Re: [PATCH v2 2/5] eficonfig: use u16_strsize() to get u16 string buffer size

[Ilias Apalodimas]

On Mon, Nov 28, 2022 at 09:45:06PM +0900, Masahisa Kojima wrote:
> 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>
> ---
> 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
> 

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

Re: [PATCH v2 1/5] eficonfig: fix going one directory up issue

[Ilias Apalodimas]

On Mon, Nov 28, 2022 at 09:45:05PM +0900, Masahisa Kojima wrote:
> 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>
> ---
> 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
> 

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

Re: [PATCH v2 3/5] efi_loader: utility function to check the variable name is "Boot####"

[Ilias Apalodimas]

On Mon, Nov 28, 2022 at 09:45:07PM +0900, Masahisa Kojima wrote:
> 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 u16_tohex() and
> efi_varname_is_load_option() function to check whether the
> UEFI variable name is "Boot####".
> 
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
> Newly created in v2
> 
>  cmd/efidebug.c              | 23 +----------------------
>  include/efi_loader.h        |  2 ++
>  lib/efi_loader/efi_helper.c | 33 +++++++++++++++++++++++++++++++++
>  3 files changed, 36 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..b1ded811e7 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -707,6 +707,8 @@ 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);
> +int u16_tohex(u16 c);
> +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..8a7afcc381 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;
>  }
> +
> +int u16_tohex(u16 c)

This needs static

> +{
> +	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
> 

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

Re: [PATCH v2 3/5] efi_loader: utility function to check the variable name is "Boot####"

[Masahisa Kojima]

Hi Ilias,

On Tue, 29 Nov 2022 at 16:33, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Mon, Nov 28, 2022 at 09:45:07PM +0900, Masahisa Kojima wrote:
> > 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 u16_tohex() and
> > efi_varname_is_load_option() function to check whether the
> > UEFI variable name is "Boot####".
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> > Newly created in v2
> >
> >  cmd/efidebug.c              | 23 +----------------------
> >  include/efi_loader.h        |  2 ++
> >  lib/efi_loader/efi_helper.c | 33 +++++++++++++++++++++++++++++++++
> >  3 files changed, 36 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..b1ded811e7 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -707,6 +707,8 @@ 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);
> > +int u16_tohex(u16 c);
> > +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..8a7afcc381 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;
> >  }
> > +
> > +int u16_tohex(u16 c)
>
> This needs static

OK, I will fix it.

>
> > +{
> > +     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
> >
>
> With that
> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Thank you for the review.

Regards,
Masahisa Kojima

>