From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heinrich Schuchardt Date: Thu, 5 Sep 2019 21:37:37 +0200 Subject: [U-Boot] [PATCH v5 18/19] env, efi_loader: define env context for UEFI variables In-Reply-To: <20190905082133.18996-19-takahiro.akashi@linaro.org> References: <20190905082133.18996-1-takahiro.akashi@linaro.org> <20190905082133.18996-19-takahiro.akashi@linaro.org> Message-ID: <24969f25-58d5-8ad6-1026-2b544107aade@gmx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 9/5/19 10:21 AM, AKASHI Takahiro wrote: > We will use two environment contexts for implementing UEFI variables, > one (ctx_efi) for non-volatile variables and the other for volatile > variables. The latter doesn't have a backing storage. > > Those two contexts are currently used only in efi_variable.c and > can be moved into there if desired. But I let it under env/ for > future use elsewhere. > > In this commit, FAT file system and flash device are only supported > devices for backing storages, but extending to other devices will be > quite straightforward. > > Signed-off-by: AKASHI Takahiro > --- > env/Kconfig | 1 + > env/Kconfig.efi | 152 ++++++++++++++++++++++++++++++++++++++++++++++ > env/Makefile | 1 + > env/env_ctx_efi.c | 131 +++++++++++++++++++++++++++++++++++++++ > include/env.h | 3 + > 5 files changed, 288 insertions(+) > create mode 100644 env/Kconfig.efi > create mode 100644 env/env_ctx_efi.c > > diff --git a/env/Kconfig b/env/Kconfig > index ae96cf75bbaa..0cd3caa67376 100644 > --- a/env/Kconfig > +++ b/env/Kconfig > @@ -47,5 +47,6 @@ config ENV_DRV_UBI > bool > > source "env/Kconfig.uboot" > +source "env/Kconfig.efi" > > endmenu > diff --git a/env/Kconfig.efi b/env/Kconfig.efi > new file mode 100644 > index 000000000000..cd4e78989df6 > --- /dev/null > +++ b/env/Kconfig.efi > @@ -0,0 +1,152 @@ > +if EFI_LOADER > + > +menu "Configure UEFI context" > + Essentially the 3 variable below exclude each other. Please, use a 'choice' statement. drivers/video/Kconfig has an example. > +config ENV_EFI_IS_NOWHERE > + bool > + default y if !ENV_EFI_IS_INFLASH && !ENV_EFI_IS_IN_FAT > + > +config ENV_EFI_IS_IN_FAT > + bool "EFI Environment is in a FAT filesystem" > + select ENV_DRV_FAT > + help > + Define this if you want to use the FAT file system for > + the environment. > + > +config ENV_EFI_IS_IN_FLASH > + bool "EFI Environment in flash memory" > + select ENV_DRV_FLASH > + help > + Define this if you have a flash device which you want to use > + for the environment. > + > + a) The environment occupies one whole flash sector, which is > + "embedded" in the text segment with the U-Boot code. This Are we really limited to exactly one sector. Or is this the minimum size? > + happens usually with "bottom boot sector" or "top boot > + sector" type flash chips, which have several smaller > + sectors at the start or the end. For instance, such a > + layout can have sector sizes of 8, 2x4, 16, Nx32 kB. In > + such a case you would place the environment in one of the > + 4 kB sectors - with U-Boot code before and after it. With > + "top boot sector" type flash chips, you would put the > + environment in one of the last sectors, leaving a gap > + between U-Boot and the environment. > + > + CONFIG_ENV_EFI_OFFSET: > + > + Offset of environment data (variable area) to the > + beginning of flash memory; for instance, with bottom boot > + type flash chips the second sector can be used: the offset > + for this sector is given here. > + > + CONFIG_ENV_EFI_OFFSET is used relative to CONFIG_SYS_FLASH_BASE. > + > + CONFIG_ENV_EFI_ADDR: > + > + This is just another way to specify the start address of > + the flash sector containing the environment (instead of > + CONFIG_ENV_OFFSET). > + > + CONFIG_ENV_EFI_SECT_SIZE: > + > + Size of the sector containing the environment. > + > + > + b) Sometimes flash chips have few, equal sized, BIG sectors. > + In such a case you don't want to spend a whole sector for > + the environment. > + > + CONFIG_ENV_EFI_SIZE: > + > + If you use this in combination with CONFIG_ENV_IS_IN_FLASH > + and CONFIG_ENV_SECT_SIZE, you can specify to use only a part > + of this flash sector for the environment. This saves > + memory for the RAM copy of the environment. > + > + It may also save flash memory if you decide to use this > + when your environment is "embedded" within U-Boot code, > + since then the remainder of the flash sector could be used > + for U-Boot code. It should be pointed out that this is > + STRONGLY DISCOURAGED from a robustness point of view: > + updating the environment in flash makes it always > + necessary to erase the WHOLE sector. If something goes > + wrong before the contents has been restored from a copy in > + RAM, your target system will be dead. > + > + CONFIG_ENV_EFI_ADDR_REDUND > + CONFIG_ENV_EFI_SIZE_REDUND > + > + These settings describe a second storage area used to hold > + a redundant copy of the environment data, so that there is > + a valid backup copy in case there is a power failure during > + a "saveenv" operation. > + > + BE CAREFUL! Any changes to the flash layout, and some changes to the > + source code will make it necessary to adapt /u-boot.lds* > + accordingly! > + > +config ENV_EFI_FAT_INTERFACE > + string "Name of the block device for the environment" > + depends on ENV_EFI_IS_IN_FAT > + help > + Define this to a string that is the name of the block device. > + > +config ENV_EFI_FAT_DEVICE_AND_PART > + string "Device and partition for where to store the environment in FAT" > + depends on ENV_EFI_IS_IN_FAT > + help > + Define this to a string to specify the partition of the device. > + It can be as following: > + > + "D:P", "D:0", "D", "D:" or "D:auto" (D, P are integers. And P >= 1) > + - "D:P": device D partition P. Error occurs if device D has no > + partition table. > + - "D:0": device D. > + - "D" or "D:": device D partition 1 if device D has partition > + table, or the whole device D if has no partition > + table. > + - "D:auto": first partition in device D with bootable flag set. > + If none, first valid partition in device D. If no > + partition table then means device D. > + > +config ENV_EFI_FAT_FILE > + string "Name of the FAT file to use for the environment" > + depends on ENV_EFI_IS_IN_FAT > + default "uboot_efi.env" > + help > + It's a string of the FAT file name. This file use to store the > + environment. > + > +config ENV_EFI_ADDR 'depends on' is missing for this variable and all below. > + hex "EFI Environment Address" > + help > + Start address of the device (or partition) > + > +config ENV_EFI_OFFSET > + hex "EFI Environment Offset" > + help > + Offset from the start of the device (or partition) > + > +config ENV_EFI_SIZE > + hex "EFI Environment Size" > + help > + Size of the environment storage area > + > +config ENV_EFI_SECT_SIZE > + hex "EFI Environment Sector-Size" > + help > + Size of the sector containing the environment. > + > +config ENV_EFI_ADDR_REDUND > + hex "EFI Environment Address for second area" > + help > + Start address of the device (or partition) > + > +config ENV_EFI_SIZE_REDUND > + hex "EFI Environment Size for second area" > + help > + Size of the environment second storage area > + > +endmenu > + > +endif > diff --git a/env/Makefile b/env/Makefile > index 0168eb47f00d..b6690c73b17f 100644 > --- a/env/Makefile > +++ b/env/Makefile > @@ -4,6 +4,7 @@ > # Wolfgang Denk, DENX Software Engineering, wd at denx.de. > > obj-y += common.o env.o env_ctx_uboot.o > +obj-$(CONFIG_EFI_LOADER) += env_ctx_efi.o > > ifndef CONFIG_SPL_BUILD > obj-y += attr.o > diff --git a/env/env_ctx_efi.c b/env/env_ctx_efi.c > new file mode 100644 > index 000000000000..9b5b2f392179 > --- /dev/null > +++ b/env/env_ctx_efi.c > @@ -0,0 +1,131 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (C) 2019 Linaro Limited > + * Author: AKASHI Takahiro > + */ > + > +#include > +#include > +#include > +#include > + > +DECLARE_GLOBAL_DATA_PTR; > + > +struct hsearch_data efi_htab = { > +#if CONFIG_IS_ENABLED(ENV_SUPPORT) > + /* defined in flags.c, only compile with ENV_SUPPORT */ > + .change_ok = env_flags_validate, > +#endif > +}; > + > +struct hsearch_data efi_volatile_htab = { > +#if CONFIG_IS_ENABLED(ENV_SUPPORT) > + /* defined in flags.c, only compile with ENV_SUPPORT */ > + .change_ok = env_flags_validate, > +#endif > +}; > + > +static int env_drv_init_efi(struct env_context *ctx, enum env_location loc) > +{ > + __maybe_unused int ret; > + > + switch (loc) { > +#ifdef CONFIG_ENV_EFI_IS_IN_FLASH > + case ENVL_FLASH: { > + env_t *env_ptr; > + env_t *flash_addr; > + ulong end_addr; > + env_t *flash_addr_new; > + ulong end_addr_new; > + > +#if defined(CONFIG_ENV_EFI_ADDR_REDUND) && defined(CMD_SAVEENV) || \ > + !defined(CONFIG_ENV_EFI_ADDR_REDUND) && defined(INITENV) > +#ifdef ENV_IS_EMBEDDED > + /* FIXME: not allowed */ Is something wrong in Kconfig that an non-allowed situation can occur? > + env_ptr = NULL; > +#else /* ! ENV_IS_EMBEDDED */ > + > + env_ptr = (env_t *)CONFIG_ENV_EFI_ADDR; > +#endif /* ENV_IS_EMBEDDED */ > +#else > + env_ptr = NULL; > +#endif > + flash_addr = (env_t *)CONFIG_ENV_EFI_ADDR; > + > +/* CONFIG_ENV_EFI_ADDR is supposed to be on sector boundary */ > + end_addr = CONFIG_ENV_EFI_ADDR + CONFIG_ENV_EFI_SECT_SIZE - 1; > + > +#ifdef CONFIG_ENV_EFI_ADDR_REDUND > + flash_addr_new = (env_t *)CONFIG_ENV_EFI_ADDR_REDUND; > +/* CONFIG_ENV_EFI_ADDR_REDUND is supposed to be on sector boundary */ > + end_addr_new = CONFIG_ENV_EFI_ADDR_REDUND > + + CONFIG_ENV_EFI_SECT_SIZE - 1; > +#else > + flash_addr_new = NULL; > + end_addr_new = 0; > +#endif /* CONFIG_ENV_EFI_ADDR_REDUND */ > + > + ret = env_flash_init_params(ctx, env_ptr, flash_addr, end_addr, > + flash_addr_new, end_addr_new, > + NULL); The code above needs some comments. If one area is the old one and the other the new one, how do you toggle between the two? > + if (ret) > + return -ENOENT; > + > + return 0; > + } > +#endif > +#ifdef CONFIG_ENV_EFI_IS_IN_FAT > + case ENVL_FAT: { > + ret = env_fat_init_params(ctx, > + CONFIG_ENV_EFI_FAT_INTERFACE, > + CONFIG_ENV_EFI_FAT_DEVICE_AND_PART, > + CONFIG_ENV_EFI_FAT_FILE); > + > + return 0; > + } > +#endif > +#ifdef CONFIG_ENV_DRV_NONE > + case ENVL_NOWHERE: > +#ifdef CONFIG_ENV_EFI_IS_NOWHERE > + /* TODO: what we should do */ > + > + return -ENOENT; > +#else > + return -ENOENT; > +#endif > +#endif > + default: > + return -ENOENT; > + } > +} > + > +/* > + * Env context for UEFI variables > + */ > +U_BOOT_ENV_CONTEXT(efi) = { > + .name = "efi", > + .htab = &efi_htab, > + .env_size = 0x10000, /* TODO: make this configurable */ > + .drv_init = env_drv_init_efi, > +}; From the name of this context it is unclear that this is for the non-volatile variables. Please, adjust the comment. > + > +static int env_ctx_init_efi_volatile(struct env_context *ctx) > +{ > + /* Dummy table creation, or hcreate_r()? */ > + if (!himport_r(ctx->htab, NULL, 0, 0, 0, 0, 0, NULL)) { > + debug("%s: Creating entry tables failed (ret=%d)\n", __func__, > + errno); > + return errno; > + } > + > + env_set_ready(ctx); > + > + return 0; > +} > + > +U_BOOT_ENV_CONTEXT(efi_volatile) = { > + .name = "efi_volatile", > + .htab = &efi_volatile_htab, > + .env_size = 0, > + .init = env_ctx_init_efi_volatile, > +}; > diff --git a/include/env.h b/include/env.h > index 8045dda9f811..ec241d419a8a 100644 > --- a/include/env.h > +++ b/include/env.h > @@ -66,6 +66,9 @@ enum env_redund_flags { > }; > > #define ctx_uboot ll_entry_get(struct env_context, uboot, env_contexts) > +#define ctx_efi ll_entry_get(struct env_context, efi, env_contexts) > +#define ctx_efi_volatile ll_entry_get(struct env_context, efi_volatile, \ > + env_contexts) I do not like that ll_entry_get() is called again and again in patch 19/19. Please, call ll_entry_get() once for each context and store the reference in a static variable. Than you do not need any of these 3 defines. Best regards Heinrich > > /* Accessor functions */ > void env_set_ready(struct env_context *ctx); >