[U-Boot] [RFC, PATCH v4 16/16] efi_loader: variable: rework with new extended env interfaces
Heinrich Schuchardt
xypron.glpk at gmx.de
Wed Jul 17 18:53:53 UTC 2019
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 at 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;
> +}
>
More information about the U-Boot
mailing list