Re: [U-Boot] [PATCH v5 18/19] env, efi_loader: define env context for UEFI variables

On Thu, Sep 05, 2019 at 09:37:37PM +0200, Heinrich Schuchardt wrote:

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 takahiro.akashi@linaro.org

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.

The same comment as uboot context.

...

+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?

I have not changed this 'help' text from the original env/Kconfig. So I don't know whether or not you are right.

...

•   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 <board>/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.

Okay.

...

• 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@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 <common.h> +#include <env_flags.h> +#include <env_internal.h> +#include <search.h>

+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?

Again, I just mimicked the original init code in env/flash.c. AFAIK, this is not old-or-new stuff, but *redundant* or original-and-copy storages for robustness. The logic exists in env/flash.c.

...

• 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.

Okay.

...

+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.

Okay.

Thank you for your review, -Takahiro Akashi

Best regards

Heinrich

...

/* Accessor functions */ void env_set_ready(struct env_context *ctx);