[PATCH v3 0/5] miscellaneous fixes of eficonfig

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

-- 
2.17.1

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

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

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

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

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

[Ilias Apalodimas]

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

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

[Heinrich Schuchardt]

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

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

[Masahisa Kojima]

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

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

[Ilias Apalodimas]

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

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

[Masahisa Kojima]

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

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

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

[Ilias Apalodimas]

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

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

[Masahisa Kojima]

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