Re: [U-Boot] [PATCH v3 1/7] env: save UEFI non-volatile variables in dedicated storage

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

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

Where do you specify on which subsystem (mmc, scsi, sata, nvme) the file is stored?

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.

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

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

"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

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

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

Essentially I expect we will end up with a class of drivers which we can use as backends for nv variables, e.g.

* 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_ */