[U-Boot] [PATCH v2 05/11] env: save UEFI non-volatile variables in dedicated storage

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

On 4/24/19 8:30 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

%s/ince/once/

> 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                       |  34 ++++++++++
>   env/env.c                         |  98 ++++++++++++++++++++++++++-
>   env/fat.c                         | 109 ++++++++++++++++++++++++++++++
>   include/asm-generic/global_data.h |   1 +
>   include/environment.h             |  24 +++++++
>   5 files changed, 265 insertions(+), 1 deletion(-)
>
> diff --git a/env/Kconfig b/env/Kconfig
> index 78300660c720..8f59e9347d4b 100644
> --- a/env/Kconfig
> +++ b/env/Kconfig
> @@ -438,6 +438,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

API wise there is no reason why we should prefer FAT over other
file-systemes.

> +	string "Device and partition for where to store the UEFI non-volatile variables in FAT"
> +	depends on ENV_IS_IN_FAT

It should be possible to store the U-Boot variable on a different medium
than the EFI variable.

> +	depends on EFI_LOADER
> +	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 UEFI non-volatile variables"
> +	depends on ENV_IS_IN_FAT
> +	depends on EFI_LOADER
> +	default "uboot_efi.env"
> +	help
> +	  It's a string of the FAT file name. This file use to store the
> +	  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 +498,12 @@ 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"
> +	default 0x20000
> +	help
> +	  Size of the UEFI variables storage area
> +
>   if ARCH_ROCKCHIP || ARCH_SUNXI || ARCH_ZYNQ || ARCH_ZYNQMP || ARCH_VERSAL || ARC
>
>   config ENV_OFFSET
> diff --git a/env/env.c b/env/env.c
> index 4b417b90a291..d5af761ba57e 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -24,6 +24,10 @@ void env_fix_drivers(void)
>   			entry->load += gd->reloc_off;
>   		if (entry->save)
>   			entry->save += gd->reloc_off;
> +		if (entry->efi_load)
> +			entry->efi_load += gd->reloc_off;
> +		if (entry->efi_save)
> +			entry->efi_save += gd->reloc_off;
>   		if (entry->init)
>   			entry->init += gd->reloc_off;
>   	}
> @@ -125,7 +129,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 +285,94 @@ int env_init(void)
>
>   	return ret;
>   }
> +
> +#ifdef CONFIG_EFI_LOADER
> +extern struct hsearch_data efi_var_htab;
> +extern struct hsearch_data efi_nv_var_htab;
> +
> +/* TODO: experimental */
> +int env_efi_save(void)

This function should be __efi_runtime.

> +{
> +	struct env_driver *drv = NULL;
> +	int ret;
> +
> +	if (!efi_nv_var_htab.table)
> +		return 0;
> +
> +	if (gd->env_efi_prio == -1) {
> +		printf("Cannot save UEFI non-volatile variable\n");
> +		return -1;
> +	}
> +
> +	drv = _env_driver_lookup(env_get_location(ENVOP_EFI, gd->env_efi_prio));

Some drivers will support writing at runtime some will not.

I think at initialization you should decide on the one and only
environment driver. And the link to it should be shored in an
__efi_runtime variable.

> +	if (!drv) {
> +		printf("Cannot save UEFI non-volatile variable\n");
> +		return -1;
> +	}
> +
> +	ret = drv->efi_save();
> +	if (ret)
> +		printf("Saving UEFI non-volatile variable failed\n");
> +
> +	return ret;
> +}
> +
> +/* TODO: experimental */
> +/* This function should be called only once at init */
> +int env_efi_load(void)

This function needs to be __efi_runtime.

> +{
> +	struct env_driver *drv = NULL;
> +	int prio, ret;
> +	enum env_location loc;
> +
> +	gd->env_efi_prio = -1;
> +
> +	/* volatile variables */
> +	if (!efi_var_htab.table) {
> +		ret = himport_r(&efi_var_htab, NULL, 0, '\0', 0, 0, 0, NULL);
> +		if (!ret) {
> +			printf("Creating default UEFI variables failed\n");
> +			return -1;
> +		}
> +	}
> +
> +	/* non-loratile variables */
> +	if (efi_nv_var_htab.table)
> +		return 0;
> +
> +	for (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)) {
> +		printf("No driver for UEFI storage is available\n");
> +		return -1;
> +	}
> +
> +	gd->env_efi_prio = prio;
> +
> +	ret = drv->efi_load();
> +	if (ret) {
> +		printf("Loading UEFI non-volatile variables failed\n");
> +		return -1;
> +	}
> +
> +	/* FIXME: init needed here? */
> +	if (!efi_nv_var_htab.table) {
> +		ret = himport_r(&efi_nv_var_htab, NULL, 0, '\0', 0, 0, 0, NULL);
> +		if (!ret) {
> +			printf("Creating default UEFI non-volatile variables failed\n");
> +			return -1;
> +		}
> +
> +		return 0;
> +	}
> +
> +	return 0;
> +}
> +#endif /* CONFIG_EFI_LOADER */
> diff --git a/env/fat.c b/env/fat.c
> index 7f74c64dfe7e..e2b050d4553d 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,110 @@ err_env_relocate:
>   }
>   #endif /* LOADENV */
>
> +#ifdef CONFIG_EFI_LOADER
> +#if 1
> +extern int efi_variable_import(const char *buf, int check);
> +extern int efi_variable_export(env_t *env_out);
> +#endif
> +static int env_fat_efi_save(void)

Please, define a u-class for the different EFI variable stores.
Currently there should be two of them:

- u-boot variables
- EFI env file (on any block device supporting file writes)

The driver should provide an indication if it is supporting runtime access.

> +{
> +	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 = efi_variable_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 efi_variable_import(buf, 1);
> +	else
> +		return 0;
> +
> +err_env_relocate:
> +
> +	return -EIO;
> +}
> +#endif /* CONFIG_EFI_LOADER */
> +
>   U_BOOT_ENV_LOCATION(fat) = {
>   	.location	= ENVL_FAT,
>   	ENV_NAME("FAT")
> @@ -137,4 +242,8 @@ U_BOOT_ENV_LOCATION(fat) = {
>   #ifdef CMD_SAVEENV
>   	.save		= env_save_ptr(env_fat_save),
>   #endif
> +#ifdef CONFIG_EFI_LOADER
> +	.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 78dcf40bff48..8c4d56c346f3 100644
> --- a/include/asm-generic/global_data.h
> +++ b/include/asm-generic/global_data.h
> @@ -51,6 +51,7 @@ 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 */
> +	int env_efi_prio;		/* Priority of the UEFI variables */
>
>   	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..f62399863ef9 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,24 @@ struct env_driver {

If you would want to reuse the env driver you could simply change load()
and save() to accept a filename as parameter.

Buf why mess with env_driver?

I think the EFI variable driver can simply use the functions provided in
include/fs.h: fs_set_blk_dev(), fs_write(), fs_read().

Best regards

Heinrich

>   	 */
>   	int (*save)(void);
>
> +	/**
> +	 * 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);
> +
>   	/**
>   	 * init() - Set up the initial pre-relocation environment
>   	 *
> @@ -312,6 +331,11 @@ 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_EFI_LOADER
> +int env_efi_load(void);
> +int env_efi_save(void);
> +#endif
> +
>   #endif /* DO_DEPS_ONLY */
>
>   #endif /* _ENVIRONMENT_H_ */
>