[PATCH 0/3] miscellaneous fixes of eficonfig
This series includes bugfix, new Kconfig option to add the limit of boot option index, and refactoring.
Masahisa Kojima (3): eficonfig: fix going one directory up issue cmd: add maximum boot option index Kconfig option eficonfig: use u16_strsize() to get u16 string buffer size
cmd/Kconfig | 12 ++++++++++++ cmd/eficonfig.c | 15 +++++++-------- 2 files changed, 19 insertions(+), 8 deletions(-)
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 --- 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;
eficonfig command reads the all possible UEFI boot options from 0x0000 to 0xFFFF to construct the menu. This takes too much time in some environment, especially for the platform using OP-TEE and RPMB based secure storage. For example, on Socionext Developerbox, it takes more than 30 seconds to draw the boot option menu.
To reduce the time to draw the menu, this patch introduces the maximum UEFI boot option index that eficonfig can manage.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org --- cmd/Kconfig | 12 ++++++++++++ cmd/eficonfig.c | 10 +++++----- 2 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/cmd/Kconfig b/cmd/Kconfig index 1092fb9c91..3978a5ef30 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -1981,6 +1981,18 @@ config CMD_EFICONFIG Enable the 'eficonfig' command which provides the menu-driven UEFI variable maintenance interface.
+config EFICONFIG_BOOT_OPTION_MAX_INDEX + int "Maximum index of UEFI Boot Option" + depends on CMD_EFICONFIG + default 99 + help + Define the number of boot option entry that 'eficonfig' command + can manage. + In UEFI specification, the boot option is indexed from 0x0000 + to 0xFFFF. 'eficonfig' command read the boot option from uefi + variable storage, reading all possible boot option may + significantly affect the performance of boot option enumeration. + config CMD_EXCEPTION bool "exception - raise exception" depends on ARM || RISCV || SANDBOX || X86 diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c index 5529edc85e..20b9a29d3a 100644 --- a/cmd/eficonfig.c +++ b/cmd/eficonfig.c @@ -1079,7 +1079,7 @@ efi_status_t eficonfig_get_unused_bootoption(u16 *buf, efi_uintn_t buf_size, if (buf_size < u16_strsize(u"Boot####")) return EFI_BUFFER_TOO_SMALL;
- for (i = 0; i <= 0xFFFF; i++) { + for (i = 0; i <= CONFIG_EFICONFIG_BOOT_OPTION_MAX_INDEX; i++) { size = 0; efi_create_indexed_name(buf, buf_size, "Boot", i); ret = efi_get_variable_int(buf, &efi_global_variable_guid, @@ -1090,7 +1090,7 @@ efi_status_t eficonfig_get_unused_bootoption(u16 *buf, efi_uintn_t buf_size, break; }
- if (i > 0xFFFF) + if (i > CONFIG_EFICONFIG_BOOT_OPTION_MAX_INDEX) return EFI_OUT_OF_RESOURCES;
*index = i; @@ -1708,7 +1708,7 @@ 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++) { + for (i = 0; i <= CONFIG_EFICONFIG_BOOT_OPTION_MAX_INDEX; i++) { /* If the index is included in the BootOrder, skip it */ if (search_bootorder(bootorder, num, i, NULL)) continue; @@ -2007,7 +2007,7 @@ 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++) { + for (i = 0; i < CONFIG_EFICONFIG_BOOT_OPTION_MAX_INDEX; i++) { if (efi_menu->count >= EFICONFIG_ENTRY_NUM_MAX - 2) break;
@@ -2278,7 +2278,7 @@ efi_status_t eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_op u16 varname[] = u"Boot####"; efi_status_t ret = EFI_SUCCESS;
- for (i = 0; i <= 0xFFFF; i++) { + for (i = 0; i <= CONFIG_EFICONFIG_BOOT_OPTION_MAX_INDEX; i++) { efi_uintn_t tmp;
efi_create_indexed_name(varname, sizeof(varname), "Boot", i);
On 11/23/22 08:17, Masahisa Kojima wrote:
eficonfig command reads the all possible UEFI boot options from 0x0000 to 0xFFFF to construct the menu. This takes too much time in some environment, especially for the platform using OP-TEE and RPMB based secure storage. For example, on Socionext Developerbox, it takes more than 30 seconds to draw the boot option menu.
To reduce the time to draw the menu, this patch introduces the maximum UEFI boot option index that eficonfig can manage.
We should avoid deviations from the UEFI specification.
There cannot be more boot options than UEFI variables. Instead of iterating over Boot0000 to BootFFFF just iterate over all UEFI variables to collect Boot#### entries using GetNextVariableName(). Afterwards decide which ones are mentioned in BootOrder.
Best regards
Heinrich
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
cmd/Kconfig | 12 ++++++++++++ cmd/eficonfig.c | 10 +++++----- 2 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/cmd/Kconfig b/cmd/Kconfig index 1092fb9c91..3978a5ef30 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -1981,6 +1981,18 @@ config CMD_EFICONFIG Enable the 'eficonfig' command which provides the menu-driven UEFI variable maintenance interface.
+config EFICONFIG_BOOT_OPTION_MAX_INDEX
- int "Maximum index of UEFI Boot Option"
- depends on CMD_EFICONFIG
- default 99
- help
Define the number of boot option entry that 'eficonfig' commandcan manage.In UEFI specification, the boot option is indexed from 0x0000to 0xFFFF. 'eficonfig' command read the boot option from uefivariable storage, reading all possible boot option maysignificantly affect the performance of boot option enumeration.- config CMD_EXCEPTION bool "exception - raise exception" depends on ARM || RISCV || SANDBOX || X86
diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c index 5529edc85e..20b9a29d3a 100644 --- a/cmd/eficonfig.c +++ b/cmd/eficonfig.c @@ -1079,7 +1079,7 @@ efi_status_t eficonfig_get_unused_bootoption(u16 *buf, efi_uintn_t buf_size, if (buf_size < u16_strsize(u"Boot####")) return EFI_BUFFER_TOO_SMALL;
- for (i = 0; i <= 0xFFFF; i++) {
- for (i = 0; i <= CONFIG_EFICONFIG_BOOT_OPTION_MAX_INDEX; i++) { size = 0; efi_create_indexed_name(buf, buf_size, "Boot", i); ret = efi_get_variable_int(buf, &efi_global_variable_guid,
@@ -1090,7 +1090,7 @@ efi_status_t eficonfig_get_unused_bootoption(u16 *buf, efi_uintn_t buf_size, break; }
- if (i > 0xFFFF)
if (i > CONFIG_EFICONFIG_BOOT_OPTION_MAX_INDEX) return EFI_OUT_OF_RESOURCES;
*index = i;
@@ -1708,7 +1708,7 @@ 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++) {
- for (i = 0; i <= CONFIG_EFICONFIG_BOOT_OPTION_MAX_INDEX; i++) { /* If the index is included in the BootOrder, skip it */ if (search_bootorder(bootorder, num, i, NULL)) continue;
@@ -2007,7 +2007,7 @@ 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++) {
- for (i = 0; i < CONFIG_EFICONFIG_BOOT_OPTION_MAX_INDEX; i++) { if (efi_menu->count >= EFICONFIG_ENTRY_NUM_MAX - 2) break;
@@ -2278,7 +2278,7 @@ efi_status_t eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_op u16 varname[] = u"Boot####"; efi_status_t ret = EFI_SUCCESS;
- for (i = 0; i <= 0xFFFF; i++) {
for (i = 0; i <= CONFIG_EFICONFIG_BOOT_OPTION_MAX_INDEX; i++) { efi_uintn_t tmp;
efi_create_indexed_name(varname, sizeof(varname), "Boot", i);
Hi Heinrich,
On Wed, 23 Nov 2022 at 16:26, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 11/23/22 08:17, Masahisa Kojima wrote:
eficonfig command reads the all possible UEFI boot options from 0x0000 to 0xFFFF to construct the menu. This takes too much time in some environment, especially for the platform using OP-TEE and RPMB based secure storage. For example, on Socionext Developerbox, it takes more than 30 seconds to draw the boot option menu.
To reduce the time to draw the menu, this patch introduces the maximum UEFI boot option index that eficonfig can manage.
We should avoid deviations from the UEFI specification.
There cannot be more boot options than UEFI variables. Instead of iterating over Boot0000 to BootFFFF just iterate over all UEFI variables to collect Boot#### entries using GetNextVariableName(). Afterwards decide which ones are mentioned in BootOrder.
Using GetNextVariableName() will address the current issue. Thank you for your comment.
Regards, Masahisa Kojima
Best regards
Heinrich
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
cmd/Kconfig | 12 ++++++++++++ cmd/eficonfig.c | 10 +++++----- 2 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/cmd/Kconfig b/cmd/Kconfig index 1092fb9c91..3978a5ef30 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -1981,6 +1981,18 @@ config CMD_EFICONFIG Enable the 'eficonfig' command which provides the menu-driven UEFI variable maintenance interface.
+config EFICONFIG_BOOT_OPTION_MAX_INDEX
int "Maximum index of UEFI Boot Option"depends on CMD_EFICONFIGdefault 99helpDefine the number of boot option entry that 'eficonfig' commandcan manage.In UEFI specification, the boot option is indexed from 0x0000to 0xFFFF. 'eficonfig' command read the boot option from uefivariable storage, reading all possible boot option maysignificantly affect the performance of boot option enumeration.- config CMD_EXCEPTION bool "exception - raise exception" depends on ARM || RISCV || SANDBOX || X86
diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c index 5529edc85e..20b9a29d3a 100644 --- a/cmd/eficonfig.c +++ b/cmd/eficonfig.c @@ -1079,7 +1079,7 @@ efi_status_t eficonfig_get_unused_bootoption(u16 *buf, efi_uintn_t buf_size, if (buf_size < u16_strsize(u"Boot####")) return EFI_BUFFER_TOO_SMALL;
for (i = 0; i <= 0xFFFF; i++) {
for (i = 0; i <= CONFIG_EFICONFIG_BOOT_OPTION_MAX_INDEX; i++) { size = 0; efi_create_indexed_name(buf, buf_size, "Boot", i); ret = efi_get_variable_int(buf, &efi_global_variable_guid,@@ -1090,7 +1090,7 @@ efi_status_t eficonfig_get_unused_bootoption(u16 *buf, efi_uintn_t buf_size, break; }
if (i > 0xFFFF)
if (i > CONFIG_EFICONFIG_BOOT_OPTION_MAX_INDEX) return EFI_OUT_OF_RESOURCES; *index = i;@@ -1708,7 +1708,7 @@ 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++) {
for (i = 0; i <= CONFIG_EFICONFIG_BOOT_OPTION_MAX_INDEX; i++) { /* If the index is included in the BootOrder, skip it */ if (search_bootorder(bootorder, num, i, NULL)) continue;@@ -2007,7 +2007,7 @@ 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++) {
for (i = 0; i < CONFIG_EFICONFIG_BOOT_OPTION_MAX_INDEX; i++) { if (efi_menu->count >= EFICONFIG_ENTRY_NUM_MAX - 2) break;@@ -2278,7 +2278,7 @@ efi_status_t eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_op u16 varname[] = u"Boot####"; efi_status_t ret = EFI_SUCCESS;
for (i = 0; i <= 0xFFFF; i++) {
for (i = 0; i <= CONFIG_EFICONFIG_BOOT_OPTION_MAX_INDEX; i++) { efi_uintn_t tmp; efi_create_indexed_name(varname, sizeof(varname), "Boot", i);
Hi,
On 11/23/22 08:17, Masahisa Kojima wrote:
eficonfig command reads the all possible UEFI boot options from 0x0000 to 0xFFFF to construct the menu. This takes too much time in some environment, especially for the platform using OP-TEE and RPMB based secure storage. For example, on Socionext Developerbox, it takes more than 30 seconds to draw the boot option menu.
How are you configuring the RPMB FS driver in OP-TEE [1]? CFG_RPMB_FS_RD_ENTRIES and CFG_RPMB_FS_CACHE_ENTRIES affect how OP-TEE reads and caches the File Allocation Table from RPMB, they have an impact on the performance of file lookup. The default values are quite conservative in terms of memory, with no caching and a read buffer capable of holding entries for 8 files only. This can result is poor performance but was considered a reasonable default considering memory-constrained platforms.
Since Developerbox has lots of RAM, I suggest experimenting with larger values. For the initial file enumeration, I think increasing CFG_RPMB_FS_RD_ENTRIES should be enough to improve the situation quite a bit. Subsequent enumerations and access to the file contents could be improved by also increasing CFG_RPMB_FS_CACHE_ENTRIES. I would recommend setting both to the same "rather large" value (128?). You might need to increase the TEE core heap size though (CFG_CORE_HEAP_SIZE).
Then it would be nice to have these values defined as platform defaults upstream rather than in some downstream fork of OP-TEE (I am saying this because I suppose you already have custom values as part of some Yocto build or something).
[1] https://github.com/OP-TEE/optee_os/blob/master/mk/config.mk#L170-L201
Thanks,
Hi Jerome,
On Wed, 23 Nov 2022 at 19:57, Jerome Forissier jerome.forissier@linaro.org wrote:
Hi,
On 11/23/22 08:17, Masahisa Kojima wrote:
eficonfig command reads the all possible UEFI boot options from 0x0000 to 0xFFFF to construct the menu. This takes too much time in some environment, especially for the platform using OP-TEE and RPMB based secure storage. For example, on Socionext Developerbox, it takes more than 30 seconds to draw the boot option menu.
How are you configuring the RPMB FS driver in OP-TEE [1]? CFG_RPMB_FS_RD_ENTRIES and CFG_RPMB_FS_CACHE_ENTRIES affect how OP-TEE reads and caches the File Allocation Table from RPMB, they have an impact on the performance of file lookup. The default values are quite conservative in terms of memory, with no caching and a read buffer capable of holding entries for 8 files only. This can result is poor performance but was considered a reasonable default considering memory-constrained platforms.
Since Developerbox has lots of RAM, I suggest experimenting with larger values. For the initial file enumeration, I think increasing CFG_RPMB_FS_RD_ENTRIES should be enough to improve the situation quite a bit. Subsequent enumerations and access to the file contents could be improved by also increasing CFG_RPMB_FS_CACHE_ENTRIES. I would recommend setting both to the same "rather large" value (128?). You might need to increase the TEE core heap size though (CFG_CORE_HEAP_SIZE).
Then it would be nice to have these values defined as platform defaults upstream rather than in some downstream fork of OP-TEE (I am saying this because I suppose you already have custom values as part of some Yocto build or something).
Yes, I'm using default configuration. Thank you for the information to improve the performance. The problem I'm facing will be addressed by Heinrich's comment(using GetNextVariableName()), but OP-TEE configuration is worth trying.
Thanks, Masahisa Kojima
[1] https://github.com/OP-TEE/optee_os/blob/master/mk/config.mk#L170-L201
Thanks,
Jerome
Use u16_strsize() to simplify the u16 string buffer size calculation.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org --- cmd/eficonfig.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c index 20b9a29d3a..282b5d2cf2 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;
On 11/23/22 08:17, 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
cmd/eficonfig.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c index 20b9a29d3a..282b5d2cf2 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;
participants (3)
-
Heinrich Schuchardt -
Jerome Forissier -
Masahisa Kojima