[U-Boot] [PATCH v3 1/7] env: save UEFI non-volatile variables in dedicated storage
AKASHI Takahiro
takahiro.akashi at linaro.org
Wed Jun 5 00:36:06 UTC 2019
On Tue, Jun 04, 2019 at 11:09:56PM +0200, Heinrich Schuchardt wrote:
> On 6/4/19 8:52 AM, AKASHI Takahiro wrote:
> >We need a variant of env_save()/env_load() to handle dedicated storage
> >for UEFI variables.
> >It is assumed that env_efi_load() will be called only ince at init
> >and that env_efi_save() will be called at every SetVariable.
> >
> >In this patch, new parameters will be expected to be configured:
> > CONFIG_ENV_EFI_FAT_DEVICE_AND_PART
> > CONFIG_ENV_EFI_FAT_FILE
> >in case of CONFIG_ENV_IS_IN_FAT.
> >
> >Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> >---
> > env/Kconfig | 39 ++++++++
> > env/env.c | 155 +++++++++++++++++++++++++++++-
> > env/fat.c | 105 ++++++++++++++++++++
> > include/asm-generic/global_data.h | 3 +
> > include/environment.h | 31 ++++++
> > 5 files changed, 332 insertions(+), 1 deletion(-)
> >
> >diff --git a/env/Kconfig b/env/Kconfig
> >index 1e10c7a4c46b..a36b6ee48f10 100644
> >--- a/env/Kconfig
> >+++ b/env/Kconfig
> >@@ -399,6 +399,10 @@ config ENV_IS_IN_UBI
> > the environment in. This will enable redundant environments in UBI.
> > It is assumed that both volumes are in the same MTD partition.
> >
>
> Our store will be completely unrelated to U-Boot environment variables.
> So why should we put anything into this Kconfig?
This is a discussion.
Some of my code still mimics the logic of U-Boot environment,
for instance, backing driver lookup.
I think that U-Boot maintainers may want U-Boot and UEFI code
to stay as close as possible.
> >+config ENV_EFI
> >+ bool
> >+ default n
> >+
> > config ENV_FAT_INTERFACE
> > string "Name of the block device for the environment"
> > depends on ENV_IS_IN_FAT
> >@@ -438,6 +442,34 @@ config ENV_FAT_FILE
> > It's a string of the FAT file name. This file use to store the
> > environment.
> >
> >+config ENV_EFI_FAT_DEVICE_AND_PART
> >+ string "Device and partition for where to store the UEFI non-volatile variables in FAT"
> >+ depends on ENV_IS_IN_FAT
> >+ depends on ENV_EFI
> >+ help
> >+ Define this to a string to specify the partition of the device. It can
> >+ be as following:
>
> The following information is not specific to this variable. So we can
> cut it short:
>
> "String defining the device number and the partition number in the
> format <device number>:<partition number>, e.g. 0:1."
The text is just a copy of the one as for CONFIG_ENV_FAT_DEVICE_AND_PART.
This is another reason that I put the code under "env."
After your comment, the text can be modified as it refers to
CONFIG_ENV_FAT_DEVICE_AND_PART for details.
>
> Where do you specify on which subsystem (mmc, scsi, sata, nvme) the file
> is stored?
Haven't you read the cover letter? Please read it first.
I always try to put bunch of information regarding my patch there.
> I would prefer nopt to have a restriction to FAT. If the EXT4 driver is
> enabled writing and reading to an ext4 valume should work as well. When
> calling fs_set_blk_dev() you will not specify any file system.
I remember that I have answered this comment before:
This is just an example for backing storage.
It is a matter of time to support other devices as U-Boot environment does.
This is another reason that I put the code under "env."
> >+
> >+ "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 UEFI non-volatile variables"
>
> %s/UEFI non-volatile variables/non-volatile UEFI variables/
>
> No need for the file system being FAT.
ditto.
> >+ depends on ENV_IS_IN_FAT
> >+ depends on ENV_EFI
> >+ default "uboot_efi.env"
> >+ help
> >+ It's a string of the FAT file name. This file use to store the
>
> It is obvious that afile name is a string. But why restrict to a file
> name? This could also be a path to a non-root file:
Again, this is a copy from CONFIG_ENV_FAT_FILE.
> "Path of the file used to store non-volatile UEFI variables."
>
> But can't we simply have a single variable, where we put all parts of
> the definition, e.g.
>
> mmc 0:1 /EFI/nv-var.store
Again, my code follows U-Boot's configuration style.
> >+ UEFI non-volatile variables.
> >+
> > config ENV_EXT4_INTERFACE
> > string "Name of the block device for the environment"
> > depends on ENV_IS_IN_EXT4
> >@@ -470,6 +502,13 @@ config ENV_EXT4_FILE
> > It's a string of the EXT4 file name. This file use to store the
> > environment (explicit path to the file)
> >
> >+config ENV_EFI_SIZE
> >+ hex "UEFI Variables Environment Size"
> >+ depends on ENV_EFI
> >+ default 0x20000
>
> If we are writing to a file, the available space on the partition and in
> RAM is the limit. I see no need for this variable in this case.
The configuration is not properly used in my code yet.
Anyway, it is not the size of written file, but the size of buffer
in my intent.
> >+ help
> >+ Size of the UEFI variables storage area
> >+
> > if ARCH_ROCKCHIP || ARCH_SUNXI || ARCH_ZYNQ || ARCH_ZYNQMP || ARCH_VERSAL || ARC || ARCH_STM32MP
> >
> > config ENV_OFFSET
> >diff --git a/env/env.c b/env/env.c
> >index 4b417b90a291..202079f6d4c0 100644
> >--- a/env/env.c
> >+++ b/env/env.c
>
> We are not creating a driver for U-Boot environment variables. So I
> would keep away from the code in this directory. You can put the load
> and save function into lib/efi_loader/ and use the normal file-system
> functions there to read and write the file.
ditto.
> Essentially I expect we will end up with a class of drivers which we can
> use as backends for nv variables, e.g.
I heavily disagree.
We have no needs to implement UEFI features only with UEFI interfaces.
-Takahiro Akashi
> * file storage
> * OP-TEE module
> * U-Boot variables
> * none
>
> Best regards
>
> Heinrich
>
> >@@ -24,6 +24,12 @@ void env_fix_drivers(void)
> > entry->load += gd->reloc_off;
> > if (entry->save)
> > entry->save += gd->reloc_off;
> >+#ifdef CONFIG_ENV_EFI
> >+ if (entry->efi_load)
> >+ entry->efi_load += gd->reloc_off;
> >+ if (entry->efi_save)
> >+ entry->efi_save += gd->reloc_off;
> >+#endif
> > if (entry->init)
> > entry->init += gd->reloc_off;
> > }
> >@@ -125,7 +131,8 @@ __weak enum env_location env_get_location(enum env_operation op, int prio)
> > if (prio >= ARRAY_SIZE(env_locations))
> > return ENVL_UNKNOWN;
> >
> >- gd->env_load_prio = prio;
> >+ if (op != ENVOP_EFI)
> >+ gd->env_load_prio = prio;
> >
> > return env_locations[prio];
> > }
> >@@ -280,3 +287,149 @@ int env_init(void)
> >
> > return ret;
> > }
> >+
> >+#ifdef CONFIG_ENV_EFI
> >+struct hsearch_data efi_var_htab;
> >+struct hsearch_data efi_nv_var_htab;
> >+
> >+int env_efi_import(const char *buf, int check)
> >+{
> >+ env_t *ep = (env_t *)buf;
> >+
> >+ if (check) {
> >+ u32 crc;
> >+
> >+ memcpy(&crc, &ep->crc, sizeof(crc));
> >+
> >+ if (crc32(0, ep->data, CONFIG_ENV_EFI_SIZE - ENV_HEADER_SIZE)
> >+ != crc) {
> >+ pr_err("bad CRC of UEFI variables\n");
> >+ return -ENOMSG; /* needed for env_load() */
> >+ }
> >+ }
> >+
> >+ if (himport_r(&efi_nv_var_htab, (char *)ep->data,
> >+ CONFIG_ENV_EFI_SIZE - ENV_HEADER_SIZE,
> >+ '\0', 0, 0, 0, NULL))
> >+ return 0;
> >+
> >+ pr_err("Cannot import environment: errno = %d\n", errno);
> >+
> >+ /* set_default_env("import failed", 0); */
> >+
> >+ return -EIO;
> >+}
> >+
> >+int env_efi_export(env_t *env_out)
> >+{
> >+ char *res;
> >+ ssize_t len;
> >+
> >+ res = (char *)env_out->data;
> >+ len = hexport_r(&efi_nv_var_htab, '\0', 0, &res,
> >+ CONFIG_ENV_EFI_SIZE - ENV_HEADER_SIZE,
> >+ 0, NULL);
> >+ if (len < 0) {
> >+ pr_err("Cannot export environment: errno = %d\n", errno);
> >+ return 1;
> >+ }
> >+
> >+ env_out->crc = crc32(0, env_out->data,
> >+ CONFIG_ENV_EFI_SIZE - ENV_HEADER_SIZE);
> >+
> >+ return 0;
> >+}
> >+
> >+int env_efi_save(void)
> >+{
> >+#ifdef CONFIG_ENV_IS_NOWHERE
> >+ return 0;
> >+#else
> >+ struct env_driver *drv = NULL;
> >+ int ret;
> >+
> >+ if (!efi_nv_var_htab.table)
> >+ return 0;
> >+
> >+ if (gd->env_efi_prio == -1) {
> >+ pr_warn("No UEFI non-volatile variable storage\n");
> >+ return -1;
> >+ }
> >+
> >+ drv = _env_driver_lookup(env_get_location(ENVOP_EFI, gd->env_efi_prio));
> >+ if (!drv) {
> >+ pr_warn("No UEFI non-volatile variable storage\n");
> >+ return -1;
> >+ }
> >+
> >+ ret = drv->efi_save();
> >+ if (ret)
> >+ pr_err("Saving UEFI non-volatile variable failed\n");
> >+
> >+ return ret;
> >+#endif
> >+}
> >+
> >+/* This function should be called only once at init */
> >+int env_efi_load(void)
> >+{
> >+#ifndef CONFIG_ENV_IS_NOWHERE
> >+ struct env_driver *drv;
> >+ int prio;
> >+ enum env_location loc;
> >+#endif
> >+ int ret;
> >+
> >+ /* volatile variables */
> >+ if (!efi_var_htab.table) {
> >+ ret = himport_r(&efi_var_htab, NULL, 0, '\0', 0, 0, 0, NULL);
> >+ if (!ret) {
> >+ pr_err("Creating UEFI volatile variables failed\n");
> >+ return -1;
> >+ }
> >+ }
> >+
> >+#ifndef CONFIG_ENV_IS_NOWHERE
> >+ gd->env_efi_prio = -1;
> >+
> >+ /* non-volatile variables */
> >+ if (efi_nv_var_htab.table)
> >+ return 0;
> >+
> >+ for (drv = NULL, prio = 0; prio < ARRAY_SIZE(env_locations); prio++) {
> >+ loc = env_get_location(ENVOP_EFI, prio);
> >+ drv = _env_driver_lookup(loc);
> >+ if (!drv)
> >+ continue;
> >+
> >+ if (drv->efi_load && drv->efi_save)
> >+ break;
> >+ }
> >+ if (!drv || prio == ARRAY_SIZE(env_locations)) {
> >+ pr_warn("No UEFI non-volatile variable storage\n");
> >+ goto skip_load;
> >+ }
> >+
> >+ gd->env_efi_prio = prio;
> >+
> >+ ret = drv->efi_load();
> >+ if (ret) {
> >+ pr_err("Loading UEFI non-volatile variables failed\n");
> >+ return -1;
> >+ }
> >+skip_load:
> >+#endif /* CONFIG_ENV_IS_NOWHERE */
> >+
> >+ if (!efi_nv_var_htab.table) {
> >+ ret = himport_r(&efi_nv_var_htab, NULL, 0, '\0', 0, 0, 0, NULL);
> >+ if (!ret) {
> >+ pr_err("Creating UEFI non-volatile variables failed\n");
> >+ return -1;
> >+ }
> >+
> >+ return 0;
> >+ }
> >+
> >+ return 0;
> >+}
> >+#endif /* CONFIG_ENV_EFI */
> >diff --git a/env/fat.c b/env/fat.c
> >index 7f74c64dfe7e..7cd6dc51baea 100644
> >--- a/env/fat.c
> >+++ b/env/fat.c
> >@@ -14,6 +14,7 @@
> > #include <malloc.h>
> > #include <memalign.h>
> > #include <search.h>
> >+#include <efi_loader.h>
> > #include <errno.h>
> > #include <fat.h>
> > #include <mmc.h>
> >@@ -128,6 +129,106 @@ err_env_relocate:
> > }
> > #endif /* LOADENV */
> >
> >+#ifdef CONFIG_ENV_EFI
> >+static int env_fat_efi_save(void)
> >+{
> >+ env_t __aligned(ARCH_DMA_MINALIGN) env_new;
> >+ struct blk_desc *dev_desc = NULL;
> >+ disk_partition_t info;
> >+ int dev, part;
> >+ int err;
> >+ loff_t size;
> >+
> >+ err = env_efi_export(&env_new);
> >+ if (err)
> >+ return err;
> >+
> >+ part = blk_get_device_part_str(CONFIG_ENV_FAT_INTERFACE,
> >+ CONFIG_ENV_EFI_FAT_DEVICE_AND_PART,
> >+ &dev_desc, &info, 1);
> >+ if (part < 0)
> >+ return 1;
> >+
> >+ dev = dev_desc->devnum;
> >+ if (fat_set_blk_dev(dev_desc, &info) != 0) {
> >+ /*
> >+ * This printf is embedded in the messages from env_save that
> >+ * will calling it. The missing \n is intentional.
> >+ */
> >+ printf("Unable to use %s %d:%d... ",
> >+ CONFIG_ENV_FAT_INTERFACE, dev, part);
> >+ return 1;
> >+ }
> >+
> >+ err = file_fat_write(CONFIG_ENV_EFI_FAT_FILE,
> >+ (void *)&env_new, 0, sizeof(env_t), &size);
> >+ if (err < 0) {
> >+ /*
> >+ * This printf is embedded in the messages from env_save that
> >+ * will calling it. The missing \n is intentional.
> >+ */
> >+ printf("Unable to write \"%s\" from %s%d:%d... ",
> >+ CONFIG_ENV_EFI_FAT_FILE, CONFIG_ENV_FAT_INTERFACE,
> >+ dev, part);
> >+ return 1;
> >+ }
> >+
> >+ return 0;
> >+}
> >+
> >+static int env_fat_efi_load(void)
> >+{
> >+ ALLOC_CACHE_ALIGN_BUFFER(char, buf, CONFIG_ENV_EFI_SIZE);
> >+ struct blk_desc *dev_desc = NULL;
> >+ disk_partition_t info;
> >+ int dev, part;
> >+ int err;
> >+
> >+#ifdef CONFIG_MMC
> >+ if (!strcmp(CONFIG_ENV_FAT_INTERFACE, "mmc"))
> >+ mmc_initialize(NULL);
> >+#endif
> >+
> >+ part = blk_get_device_part_str(CONFIG_ENV_FAT_INTERFACE,
> >+ CONFIG_ENV_EFI_FAT_DEVICE_AND_PART,
> >+ &dev_desc, &info, 1);
> >+ if (part < 0)
> >+ goto err_env_relocate;
> >+
> >+ dev = dev_desc->devnum;
> >+ if (fat_set_blk_dev(dev_desc, &info) != 0) {
> >+ /*
> >+ * This printf is embedded in the messages from env_save that
> >+ * will calling it. The missing \n is intentional.
> >+ */
> >+ printf("Unable to use %s %d:%d...\n",
> >+ CONFIG_ENV_FAT_INTERFACE, dev, part);
> >+ goto err_env_relocate;
> >+ }
> >+
> >+ err = file_fat_read(CONFIG_ENV_EFI_FAT_FILE, buf, CONFIG_ENV_EFI_SIZE);
> >+ if (err <= 0 && (err != -ENOENT)) {
> >+ /*
> >+ * This printf is embedded in the messages from env_save that
> >+ * will calling it. The missing \n is intentional.
> >+ */
> >+ printf("Unable to read \"%s\" from %s %d:%d...\n",
> >+ CONFIG_ENV_EFI_FAT_FILE, CONFIG_ENV_FAT_INTERFACE,
> >+ dev, part);
> >+ goto err_env_relocate;
> >+ }
> >+
> >+ if (err > 0)
> >+ return env_efi_import(buf, 1);
> >+ else
> >+ return 0;
> >+
> >+err_env_relocate:
> >+
> >+ return -EIO;
> >+}
> >+#endif /* CONFIG_ENV_EFI */
> >+
> > U_BOOT_ENV_LOCATION(fat) = {
> > .location = ENVL_FAT,
> > ENV_NAME("FAT")
> >@@ -137,4 +238,8 @@ U_BOOT_ENV_LOCATION(fat) = {
> > #ifdef CMD_SAVEENV
> > .save = env_save_ptr(env_fat_save),
> > #endif
> >+#ifdef CONFIG_ENV_EFI
> >+ .efi_load = env_fat_efi_load,
> >+ .efi_save = env_fat_efi_save,
> >+#endif
> > };
> >diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
> >index 02a3ed683821..5ed390cc31c7 100644
> >--- a/include/asm-generic/global_data.h
> >+++ b/include/asm-generic/global_data.h
> >@@ -52,6 +52,9 @@ typedef struct global_data {
> > unsigned long env_valid; /* Environment valid? enum env_valid */
> > unsigned long env_has_init; /* Bitmask of boolean of struct env_location offsets */
> > int env_load_prio; /* Priority of the loaded environment */
> >+#ifdef CONFIG_ENV_EFI
> >+ int env_efi_prio; /* Priority of the UEFI variables */
> >+#endif
> >
> > unsigned long ram_base; /* Base address of RAM used by U-Boot */
> > unsigned long ram_top; /* Top address of RAM used by U-Boot */
> >diff --git a/include/environment.h b/include/environment.h
> >index cd966761416e..32a03014690f 100644
> >--- a/include/environment.h
> >+++ b/include/environment.h
> >@@ -200,6 +200,7 @@ enum env_operation {
> > ENVOP_INIT, /* we want to call the init function */
> > ENVOP_LOAD, /* we want to call the load function */
> > ENVOP_SAVE, /* we want to call the save function */
> >+ ENVOP_EFI, /* we want to call the efi_load/save function */
> > };
> >
> > struct env_driver {
> >@@ -225,6 +226,26 @@ struct env_driver {
> > */
> > int (*save)(void);
> >
> >+#ifdef CONFIG_ENV_EFI
> >+ /**
> >+ * efi_load() - Load UEFI non-volatile variables from storage
> >+ *
> >+ * This method is required for UEFI non-volatile variables
> >+ *
> >+ * @return 0 if OK, -ve on error
> >+ */
> >+ int (*efi_load)(void);
> >+
> >+ /**
> >+ * efi_save() - Save UEFI non-volatile variables to storage
> >+ *
> >+ * This method is required for UEFI non-volatile variables
> >+ *
> >+ * @return 0 if OK, -ve on error
> >+ */
> >+ int (*efi_save)(void);
> >+#endif
> >+
> > /**
> > * init() - Set up the initial pre-relocation environment
> > *
> >@@ -312,6 +333,16 @@ void eth_parse_enetaddr(const char *addr, uint8_t *enetaddr);
> > int eth_env_get_enetaddr(const char *name, uint8_t *enetaddr);
> > int eth_env_set_enetaddr(const char *name, const uint8_t *enetaddr);
> >
> >+#ifdef CONFIG_ENV_EFI
> >+extern struct hsearch_data efi_var_htab;
> >+extern struct hsearch_data efi_nv_var_htab;
> >+
> >+int env_efi_import(const char *buf, int check);
> >+int env_efi_export(env_t *env_out);
> >+int env_efi_load(void);
> >+int env_efi_save(void);
> >+#endif
> >+
> > #endif /* DO_DEPS_ONLY */
> >
> > #endif /* _ENVIRONMENT_H_ */
> >
>
More information about the U-Boot
mailing list