From mboxrd@z Thu Jan  1 00:00:00 1970
From: AKASHI Takahiro <takahiro.akashi@linaro.org>
Date: Fri, 6 Sep 2019 09:54:58 +0900
Subject: [U-Boot] [PATCH v5 18/19] env,
 efi_loader: define env context for UEFI variables
In-Reply-To: <24969f25-58d5-8ad6-1026-2b544107aade@gmx.de>
References: <20190905082133.18996-1-takahiro.akashi@linaro.org>
 <20190905082133.18996-19-takahiro.akashi@linaro.org>
 <24969f25-58d5-8ad6-1026-2b544107aade@gmx.de>
Message-ID: <20190906005457.GI4398@linaro.org>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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 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 <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);
> >
>