Re: [U-Boot] [RFC, PATCH v4 16/16] efi_loader: variable: rework with new extended env interfaces

On 7/17/19 10:25 AM, AKASHI Takahiro wrote:

In UEFI variable implementation, ENVCTX_UEFI is used for context and VARSTORAGE_VOLATILE and VARSTORAGE_NON_VOLATILE_AUTOSAVE are allowed for variable storage attribute.

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

include/efi_loader.h | 1 + lib/efi_loader/Kconfig | 12 +++++++++ lib/efi_loader/Makefile | 2 +- lib/efi_loader/efi_setup.c | 5 ++++ lib/efi_loader/efi_variable.c | 50 ++++++++++++++++++++++++++--------- 5 files changed, 57 insertions(+), 13 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h index b07155cecb7c..e119d2d2a802 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -617,6 +617,7 @@ efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size, efi_status_t EFIAPI efi_set_variable(u16 *variable_name, const efi_guid_t *vendor, u32 attributes, efi_uintn_t data_size, const void *data); +efi_status_t efi_variable_init(void);

This initialization function is already defined in origin/master as

efi_status_t efi_init_variables();

/*

• See section 3.1.3 in the v2.7 UEFI spec for more details on

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index cd5436c576b1..60085a81144d 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -18,6 +18,18 @@ config EFI_LOADER

if EFI_LOADER

+choice

• prompt "Select variables storage"
• default EFI_VARIABLE_USE_ENV
• help

•  With this option, UEFI variables are implemented using
  

•  U-Boot environment variables.
  

+config EFI_VARIABLE_USE_ENV

• bool "Dedicated context in U-Boot environment"

I could not find any place in the code where EFI_VARIABLE_USE_ENV is set. Why do you introduce it?

+endchoice

• config EFI_GET_TIME bool "GetTime() runtime service" depends on DM_RTC

diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile index 01769ea58ba6..a1fb0f112e5c 100644 --- a/lib/efi_loader/Makefile +++ b/lib/efi_loader/Makefile @@ -31,7 +31,7 @@ obj-y += efi_root_node.o obj-y += efi_runtime.o obj-y += efi_setup.o obj-$(CONFIG_EFI_UNICODE_COLLATION_PROTOCOL2) += efi_unicode_collation.o -obj-y += efi_variable.o +obj-$(CONFIG_EFI_VARIABLE_USE_ENV) += efi_variable.o obj-y += efi_watchdog.o obj-$(CONFIG_LCD) += efi_gop.o obj-$(CONFIG_DM_VIDEO) += efi_gop.o diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c index bfb57836fa9f..e96c92474467 100644 --- a/lib/efi_loader/efi_setup.c +++ b/lib/efi_loader/efi_setup.c @@ -102,6 +102,11 @@ efi_status_t efi_init_obj_list(void) /* On ARM switch from EL3 or secure mode to EL2 or non-secure mode */ switch_to_non_secure_mode();

• /* Initialize UEFI variables */
• ret = efi_variable_init();

We already call efi_init_variables() here.

• if (ret != EFI_SUCCESS)

• goto out;
  

• /* Define supported languages */ ret = efi_init_platform_lang(); if (ret != EFI_SUCCESS)

diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c index d6b75ca02e45..115a1f593058 100644 --- a/lib/efi_loader/efi_variable.c +++ b/lib/efi_loader/efi_variable.c @@ -8,6 +8,7 @@ #include <malloc.h> #include <charset.h> #include <efi_loader.h> +#include <exports.h> #include <hexdump.h> #include <environment.h> #include <search.h> @@ -184,7 +185,7 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name,

EFI_PRINT("get '%s'\n", native_name);

• val = env_get(native_name);

• val = env_get_ext(ENVCTX_UEFI, native_name, NULL); free(native_name); if (!val) return EFI_EXIT(EFI_NOT_FOUND);

@@ -272,7 +273,7 @@ static efi_status_t parse_uboot_variable(char *variable, const efi_guid_t *vendor, u32 *attributes) {

• char *guid, *name, *end, c;

• char *guid, *name, *env_attr, *end, c; unsigned long name_len; u16 *p;

@@ -284,11 +285,14 @@ static efi_status_t parse_uboot_variable(char *variable, if (!name) return EFI_INVALID_PARAMETER; name++;

• end = strchr(name, '=');

• env_attr = strchr(name, ':');
• if (!env_attr)

• return EFI_INVALID_PARAMETER;
  

• end = strchr(env_attr, '='); if (!end) return EFI_INVALID_PARAMETER;

• name_len = end - name;

• name_len = env_attr - name; if (*variable_name_size < (name_len + 1)) { *variable_name_size = name_len + 1; return EFI_BUFFER_TOO_SMALL;

@@ -360,7 +364,7 @@ efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size, name_len = strlen(native_name); for (variable = efi_variables_list; variable && *variable;) { if (!strncmp(variable, native_name, name_len) &&

• 	    variable[name_len] == '=')
  

• 	    variable[name_len] == ':')
  	break;
  
  variable = strchr(variable, '\n');
  

@@ -387,9 +391,9 @@ efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size, efi_cur_variable = NULL;

snprintf(regex, 256, "efi_.*-.*-.*-.*-.*_.*");

• list_len = hexport_r(&env_htab, '\n',
  

• 		     H_MATCH_REGEX | H_MATCH_KEY,
  

• 		     &efi_variables_list, 0, 1, regexlist);
  

• list_len = hexport_ext(&env_htab, ENVCTX_UEFI, '\n',
  

• 		       H_MATCH_REGEX | H_MATCH_KEY,
  

• 		       &efi_variables_list, 0, 1, regexlist);
  

  /* 1 indicates that no match was found */ if (list_len <= 1) return EFI_EXIT(EFI_NOT_FOUND);

@@ -424,7 +428,7 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name, { char *native_name = NULL, *val = NULL, *s; efi_status_t ret = EFI_SUCCESS;

• u32 attr;

• u32 attr, flags;

  EFI_ENTRY(""%ls" %pUl %x %zu %p", variable_name, vendor, attributes, data_size, data);

@@ -446,12 +450,12 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,

if ((data_size == 0) || !(attributes & ACCESS_ATTR)) { /* delete the variable: */

• env_set(native_name, NULL);
  

• env_set_ext(ENVCTX_UEFI, native_name, 0, NULL);
  

  ret = EFI_SUCCESS; goto out; }

• val = env_get(native_name);

• val = env_get_ext(ENVCTX_UEFI, native_name, NULL); if (val) { parse_attr(val, &attr);

@@ -484,6 +488,8 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name, * store attributes * TODO: several attributes are not supported */

• flags = (attributes & EFI_VARIABLE_NON_VOLATILE) ?

• 	ENV_FLAGS_VARSTORAGE_NON_VOLATILE_AUTOSAVE : 0;
  

  attributes &= (EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS);

@@ -511,7 +517,8 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,

EFI_PRINT("setting: %s=%s\n", native_name, val);

• if (env_set(native_name, val))

• /* TODO: type: string, access: READ/WRITE/CREATE/DELETE */

• if (env_set_ext(ENVCTX_UEFI, native_name, flags, val)) ret = EFI_DEVICE_ERROR;

  out:

@@ -520,3 +527,22 @@ out:

return EFI_EXIT(ret); }

+efi_status_t efi_variable_init(void)

Please, rebase your patch on the existing coding.

+{

• int ret;
• ret = env_load_ext(ENVCTX_UEFI);
• if (ret)

+#if 1

There is no need for the #if. Please, remove the unused code.

• /*
  

•  * FIXME:
  

•  * It is valid even if initial file in FAT doesn't exist
  

What will the correct handling look like?

Best regards

Heinrich

•  */
  

• return EFI_SUCCESS;
  

+#else

• return EFI_DEVICE_ERROR;
  

+#endif

• return EFI_SUCCESS;

+}