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

On 6/4/19 8:52 AM, AKASHI Takahiro wrote:

The attribute, EFI_VARIABLE_NON_VOLATILE, should be encoded as "nv" flag in U-Boot variable if specified.

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

Reviewed-by: Heinrich Schuchardt

I will cherry-pick this patch. It is already showing relevant output:

Warning: e1000#0 using MAC address from ROM OsIndicationsSupported: BS|RT, DataSize = 0x8 00000000: 00 00 00 00 00 00 00 00 ........ PlatformLang: NV|BS|RT, DataSize = 0x6 00000000: 65 6e 2d 55 53 00 en-US. PlatformLangCodes: BS|RT, DataSize = 0x6 00000000: 65 6e 2d 55 53 00 en-US.

---

lib/efi_loader/efi_variable.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c index 50bc10537f40..e56053194dae 100644 --- a/lib/efi_loader/efi_variable.c +++ b/lib/efi_loader/efi_variable.c @@ -125,6 +125,8 @@ static const char *parse_attr(const char *str, u32 *attrp)

if ((s = prefix(str, "ro"))) {
	attr |= READ_ONLY;

• } else if ((s = prefix(str, "nv"))) {
  

• 	attr |= EFI_VARIABLE_NON_VOLATILE;
  

  } else if ((s = prefix(str, "boot"))) { attr |= EFI_VARIABLE_BOOTSERVICE_ACCESS; } else if ((s = prefix(str, "run"))) {

@@ -468,7 +470,7 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name, } }

• val = malloc(2 * data_size + strlen("{ro,run,boot}(blob)") + 1);

• val = malloc(2 * data_size + strlen("{ro,run,boot,nv}(blob)") + 1); if (!val) { ret = EFI_OUT_OF_RESOURCES; goto out;

@@ -480,12 +482,16 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name, * store attributes * TODO: several attributes are not supported */

• attributes &= (EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS);

• attributes &= (EFI_VARIABLE_NON_VOLATILE |

•        EFI_VARIABLE_BOOTSERVICE_ACCESS |
  

•        EFI_VARIABLE_RUNTIME_ACCESS);
  

  s += sprintf(s, "{"); while (attributes) { u32 attr = 1 << (ffs(attributes) - 1);

• if (attr == EFI_VARIABLE_BOOTSERVICE_ACCESS)
  

• if (attr == EFI_VARIABLE_NON_VOLATILE)
  

• 	s += sprintf(s, "nv");
  

• else if (attr == EFI_VARIABLE_BOOTSERVICE_ACCESS)
  s += sprintf(s, "boot");
  

  else if (attr == EFI_VARIABLE_RUNTIME_ACCESS) s += sprintf(s, "run");

On 6/4/19 8:52 AM, AKASHI Takahiro wrote:

UEFI volatile variables are managed in efi_var_htab while UEFI non-volatile variables are in efi_nv_var_htab. At every SetVariable API, env_efi_save() will also be called to save data cache (hash table) to persistent storage.

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

lib/efi_loader/Kconfig | 10 + lib/efi_loader/efi_variable.c | 342 ++++++++++++++++++++++++++-------- 2 files changed, 275 insertions(+), 77 deletions(-)

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index cd5436c576b1..8bf4b1754d06 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -18,6 +18,16 @@ config EFI_LOADER

if EFI_LOADER

+choice

• prompt "Select variables storage"
• default EFI_VARIABLE_USE_ENV

+config EFI_VARIABLE_USE_ENV

• bool "Same device as U-Boot environment"
• select ENV_EFI

+endchoice

• config EFI_GET_TIME bool "GetTime() runtime service" depends on DM_RTC

diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c index e56053194dae..d9887be938c2 100644 --- a/lib/efi_loader/efi_variable.c +++ b/lib/efi_loader/efi_variable.c @@ -48,6 +48,66 @@

• converted to utf16?

*/

+/*

• • We will maintain two variable database: one for volatile variables,
• • the other for non-volatile variables. The former exists only in memory
• • and will go away at re-boot. The latter is currently backed up by the same
• • device as U-Boot environment and also works as variables cache.
• • struct hsearch_data efi_var_htab
• • struct hsearch_data efi_nv_var_htab
• */

+static char *env_efi_get(const char *name, bool is_non_volatile) +{

• struct hsearch_data *htab;
• ENTRY e, *ep;
• /* WATCHDOG_RESET(); */
• if (is_non_volatile)

• htab = &efi_nv_var_htab;
  

• else

• htab = &efi_var_htab;
  

• e.key = name;
• e.data = NULL;
• hsearch_r(e, FIND, &ep, htab, 0);
• return ep ? ep->data : NULL;

+}

+static int env_efi_set(const char *name, const char *value,

•        bool is_non_volatile)
  

+{

• struct hsearch_data *htab;
• ENTRY e, *ep;
• int ret;
• if (is_non_volatile)

• htab = &efi_nv_var_htab;
  

• else

• htab = &efi_var_htab;
  

• /* delete */
• if (!value || *value == '\0') {

• ret = hdelete_r(name, htab, H_PROGRAMMATIC);
  

• return !ret;
  

• }
• /* set */
• e.key = name;
• e.data = (char *)value;
• hsearch_r(e, ENTER, &ep, htab, H_PROGRAMMATIC);
• if (!ep) {

• printf("## Error inserting \"%s\" variable, errno=%d\n",
  

•        name, errno);
  

• return 1;
  

• }
• return 0;

+}

• #define PREFIX_LEN (strlen("efi_xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx_"))

  /**

@@ -147,24 +207,12 @@ static const char *parse_attr(const char *str, u32 *attrp) return str; }

-/**

• • efi_efi_get_variable() - retrieve value of a UEFI variable
• • This function implements the GetVariable runtime service.
• • See the Unified Extensible Firmware Interface (UEFI) specification for
• • details.
• • @variable_name: name of the variable
• • @vendor: vendor GUID
• • @attributes: attributes of the variable
• • @data_size: size of the buffer to which the variable value is copied
• • @data: buffer to which the variable value is copied
• • Return: status code
• */

-efi_status_t EFIAPI efi_get_variable(u16 *variable_name,

• 		     const efi_guid_t *vendor, u32 *attributes,
  

• 		     efi_uintn_t *data_size, void *data)
  

+static +efi_status_t EFIAPI efi_get_variable_common(u16 *variable_name,

• 			    const efi_guid_t *vendor,
  

• 			    u32 *attributes,
  

• 			    efi_uintn_t *data_size, void *data,
  

• 			    bool is_non_volatile)
  

  { char *native_name; efi_status_t ret;

@@ -172,22 +220,19 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name, const char *val, *s; u32 attr;

• EFI_ENTRY(""%ls" %pUl %p %p %p", variable_name, vendor, attributes,

•   data_size, data);
  

• if (!variable_name || !vendor || !data_size) return EFI_EXIT(EFI_INVALID_PARAMETER);

  ret = efi_to_native(&native_name, variable_name, vendor); if (ret)

• return EFI_EXIT(ret);
  

• return ret;
  
  

  EFI_PRINT("get '%s'\n", native_name);

• val = env_get(native_name);

• val = env_efi_get(native_name, is_non_volatile); free(native_name); if (!val)

• return EFI_EXIT(EFI_NOT_FOUND);
  

• return EFI_NOT_FOUND;
  
  

  val = parse_attr(val, &attr);

@@ -198,7 +243,7 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name,

/* number of hexadecimal digits must be even */
if (len & 1)

• 	return EFI_EXIT(EFI_DEVICE_ERROR);
  

• 	return EFI_DEVICE_ERROR;
  
  

  /* two characters per byte: */ len /= 2;

@@ -210,10 +255,10 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name, }

if (!data)

• 	return EFI_EXIT(EFI_INVALID_PARAMETER);
  

• 	return EFI_INVALID_PARAMETER;
  
  

  if (hex2bin(data, s, len))

• 	return EFI_EXIT(EFI_DEVICE_ERROR);
  

• 	return EFI_DEVICE_ERROR;
  
  

  EFI_PRINT("got value: "%s"\n", s); } else if ((s = prefix(val, "(utf8)"))) {

@@ -227,7 +272,7 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name, }

if (!data)

• 	return EFI_EXIT(EFI_INVALID_PARAMETER);
  

• 	return EFI_INVALID_PARAMETER;
  
  

  memcpy(data, s, len); ((char *)data)[len] = '\0';

@@ -235,13 +280,67 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name, EFI_PRINT("got value: "%s"\n", (char *)data); } else { EFI_PRINT("invalid value: '%s'\n", val);

• return EFI_EXIT(EFI_DEVICE_ERROR);
  

• return EFI_DEVICE_ERROR;
  

  }

  out: if (attributes) *attributes = attr & EFI_VARIABLE_MASK;

• return ret;

+}

+static +efi_status_t EFIAPI efi_get_volatile_variable(u16 *variable_name,

• 			      const efi_guid_t *vendor,
  

• 			      u32 *attributes,
  

• 			      efi_uintn_t *data_size,
  

• 			      void *data)
  

+{

• return efi_get_variable_common(variable_name, vendor, attributes,

• 		       data_size, data, false);
  

+}

+efi_status_t EFIAPI efi_get_nonvolatile_variable(u16 *variable_name,

• 				 const efi_guid_t *vendor,
  

• 				 u32 *attributes,
  

• 				 efi_uintn_t *data_size,
  

• 				 void *data)
  

+{

• return efi_get_variable_common(variable_name, vendor, attributes,

• 		       data_size, data, true);
  

+}

+/**

• • efi_efi_get_variable() - retrieve value of a UEFI variable
• • This function implements the GetVariable runtime service.
• • See the Unified Extensible Firmware Interface (UEFI) specification for
• • details.
• • @variable_name: name of the variable
• • @vendor: vendor GUID
• • @attributes: attributes of the variable
• • @data_size: size of the buffer to which the variable value is copied
• • @data: buffer to which the variable value is copied
• • Return: status code
• */

+efi_status_t EFIAPI efi_get_variable(u16 *variable_name,

• 		     const efi_guid_t *vendor, u32 *attributes,
  

• 		     efi_uintn_t *data_size, void *data)
  

+{

• efi_status_t ret;
• EFI_ENTRY(""%ls" %pUl %p %p %p", variable_name, vendor, attributes,

•   data_size, data);
  

• ret = efi_get_volatile_variable(variable_name, vendor, attributes,

• 			data_size, data);
  

• if (ret == EFI_NOT_FOUND)

• ret = efi_get_nonvolatile_variable(variable_name, vendor,
  

• 				   attributes, data_size, data);
  

• return EFI_EXIT(ret); }

@@ -331,7 +430,7 @@ efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size, u16 *variable_name, const efi_guid_t *vendor) {

• char *native_name, *variable;

• char *native_name, *variable, *tmp_list, *merged_list; ssize_t name_len, list_len; char regex[256]; char * const regexlist[] = {regex};

@@ -387,10 +486,39 @@ efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size, efi_cur_variable = NULL;

snprintf(regex, 256, "efi_.*-.*-.*-.*-.*_.*");

• list_len = hexport_r(&env_htab, '\n',
  

• list_len = hexport_r(&efi_var_htab, '\n',
  	     H_MATCH_REGEX | H_MATCH_KEY,
  	     &efi_variables_list, 0, 1, regexlist);
  

• /* 1 indicates that no match was found */
  

• /*
  

•  * Note: '1' indicates that nothing is matched
  

•  */
  

• if (list_len <= 1) {
  

• 	free(efi_variables_list);
  

• 	efi_variables_list = NULL;
  

• 	list_len = hexport_r(&efi_nv_var_htab, '\n',
  

• 			     H_MATCH_REGEX | H_MATCH_KEY,
  

• 			     &efi_variables_list, 0, 1,
  

• 			     regexlist);
  

• } else {
  

• 	tmp_list = NULL;
  

• 	list_len = hexport_r(&efi_nv_var_htab, '\n',
  

• 			     H_MATCH_REGEX | H_MATCH_KEY,
  

• 			     &tmp_list, 0, 1,
  

• 			     regexlist);
  

• 	if (list_len <= 1) {
  

• 		list_len = 2; /* don't care actual number */
  

• 	} else {
  

• 		/* merge two variables lists */
  

• 		merged_list = malloc(strlen(efi_variables_list)
  

• 					+ strlen(tmp_list) + 1);
  

• 		strcpy(merged_list, efi_variables_list);
  

• 		strcat(merged_list, tmp_list);
  

• 		free(efi_variables_list);
  

• 		free(tmp_list);
  

• 		efi_variables_list = merged_list;
  

• 	}
  

• }
  

• if (list_len <= 1) return EFI_EXIT(EFI_NOT_FOUND);

@@ -403,77 +531,71 @@ efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size, return EFI_EXIT(ret); }

-/**

• • efi_efi_set_variable() - set value of a UEFI variable
• • This function implements the SetVariable runtime service.
• • See the Unified Extensible Firmware Interface (UEFI) specification for
• • details.
• • @variable_name: name of the variable
• • @vendor: vendor GUID
• • @attributes: attributes of the variable
• • @data_size: size of the buffer with the variable value
• • @data: buffer with the variable value
• • Return: status code
• */

-efi_status_t EFIAPI efi_set_variable(u16 *variable_name,

• 		     const efi_guid_t *vendor, u32 attributes,
  

• 		     efi_uintn_t data_size, const void *data)
  

+static +efi_status_t EFIAPI efi_set_variable_common(u16 *variable_name,

• 			    const efi_guid_t *vendor,
  

• 			    u32 attributes,
  

• 			    efi_uintn_t data_size,
  

• 			    const void *data,
  

• 			    bool is_non_volatile)
  

  { char *native_name = NULL, *val = NULL, *s;

• efi_status_t ret = EFI_SUCCESS;

• efi_uintn_t size; u32 attr;

• EFI_ENTRY(""%ls" %pUl %x %zu %p", variable_name, vendor, attributes,

•   data_size, data);
  

• efi_status_t ret = EFI_SUCCESS;

  /* TODO: implement APPEND_WRITE */ if (!variable_name || !vendor || (attributes & EFI_VARIABLE_APPEND_WRITE)) { ret = EFI_INVALID_PARAMETER;

• goto out;
  

• goto err;
  

  }

  ret = efi_to_native(&native_name, variable_name, vendor); if (ret)

• goto out;
  

• goto err;
  
  

  #define ACCESS_ATTR (EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_BOOTSERVICE_ACCESS)

• if ((data_size == 0) || !(attributes & ACCESS_ATTR)) {

• /* delete the variable: */
  

• env_set(native_name, NULL);
  

• ret = EFI_SUCCESS;
  

• goto out;
  

• /* check if a variable exists */
• size = 0;
• ret = EFI_CALL(efi_get_variable(variable_name, vendor, &attr,

• 			&size, NULL));
  

• if (ret == EFI_BUFFER_TOO_SMALL) {

• if ((is_non_volatile && !(attr & EFI_VARIABLE_NON_VOLATILE)) ||
  

•     (!is_non_volatile && (attr & EFI_VARIABLE_NON_VOLATILE))) {
  

• 	ret = EFI_INVALID_PARAMETER;
  

• 	goto err;
  

• }
  

  }

• val = env_get(native_name);
• if (val) {

• parse_attr(val, &attr);
  

• /* We should not free val */
  

• val = NULL;
  

• if (attr & READ_ONLY) {
  

• 	ret = EFI_WRITE_PROTECTED;
  

• /* delete a variable */

• if (data_size == 0 || !(attributes & ACCESS_ATTR)) {

• if (size) {
  

• 	if (attr & READ_ONLY) {
  

• 		ret = EFI_WRITE_PROTECTED;
  

• 		goto err;
  

• 	}
  goto out;
  

  }

• ret = EFI_SUCCESS;
  

• goto err; /* not error, but nothing to do */
  

• }

• /* create/modify a variable */

• if (size && attr != attributes) { /*

  • attributes won't be changed
  • TODO: take care of APPEND_WRITE once supported

  */

• if (attr != attributes) {
  

• 	ret = EFI_INVALID_PARAMETER;
  

• 	goto out;
  

• }
  

• ret = EFI_INVALID_PARAMETER;
  

• goto err;
  

  }

  val = malloc(2 * data_size + strlen("{ro,run,boot,nv}(blob)") + 1); if (!val) { ret = EFI_OUT_OF_RESOURCES;

• goto out;
  

• goto err;
  

  }

  s = val;

@@ -487,7 +609,7 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name, EFI_VARIABLE_RUNTIME_ACCESS); s += sprintf(s, "{"); while (attributes) {

• u32 attr = 1 << (ffs(attributes) - 1);
  

• attr = 1 << (ffs(attributes) - 1);
  
  

  if (attr == EFI_VARIABLE_NON_VOLATILE) s += sprintf(s, "nv");

@@ -509,12 +631,78 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,

EFI_PRINT("setting: %s=%s\n", native_name, val);

• if (env_set(native_name, val))

+out:

• ret = EFI_SUCCESS;
• if (env_efi_set(native_name, val, is_non_volatile)) ret = EFI_DEVICE_ERROR;

-out: +err: free(native_name); free(val);

• return ret;

+}

+static +efi_status_t EFIAPI efi_set_volatile_variable(u16 *variable_name,

Once you have implemented this we can make the variable service available at runtime. So I suggest to define everything here as __runtime.

Why do you any of these three functions (efi_set_volatile_variable(), efi_set_nonvolatile_variable(), efi_set_nonvolatile_variable()). Just use an if based on attributes in efi_set_variable() to call env_efi_save, when a non-volatile function is changed.

What is the advantage of having two separate RAM stores? Can't the save function sort out what to save and what not to save according to the NV flag?

• 			      const efi_guid_t *vendor,
  

• 			      u32 attributes,
  

• 			      efi_uintn_t data_size,
  

• 			      const void *data)
  

+{

• return efi_set_variable_common(variable_name, vendor, attributes,

• 		       data_size, data, false);
  

+}

+efi_status_t EFIAPI efi_set_nonvolatile_variable(u16 *variable_name,

• 				 const efi_guid_t *vendor,
  

• 				 u32 attributes,
  

• 				 efi_uintn_t data_size,
  

• 				 const void *data)
  

+{

• efi_status_t ret;
• ret = efi_set_variable_common(variable_name, vendor, attributes,

• 		      data_size, data, true);
  

• if (ret == EFI_SUCCESS)

• /* FIXME: what if save failed? */
  

• if (env_efi_save())
  

Somewhere we need the ability to switch between different backends. Will that be in env_efi_save()?

• 	ret = EFI_DEVICE_ERROR;
  

• return ret;

+}

+/**

• • efi_efi_set_variable() - set value of a UEFI variable
• • This function implements the SetVariable runtime service.
• • See the Unified Extensible Firmware Interface (UEFI) specification for
• • details.
• • @variable_name: name of the variable
• • @vendor: vendor GUID
• • @attributes: attributes of the variable
• • @data_size: size of the buffer with the variable value
• • @data: buffer with the variable value
• • Return: status code
• */

+efi_status_t EFIAPI efi_set_variable(u16 *variable_name,

• 		     const efi_guid_t *vendor, u32 attributes,
  

• 		     efi_uintn_t data_size, const void *data)
  

+{

• efi_status_t ret;
• EFI_ENTRY(""%ls" %pUl %x %zu %p", variable_name, vendor, attributes,

•   data_size, data);
  

• if (attributes & EFI_VARIABLE_NON_VOLATILE)

• ret = efi_set_nonvolatile_variable(variable_name, vendor,
  

• 				   attributes,
  

• 				   data_size, data);
  

• else

• ret = efi_set_volatile_variable(variable_name, vendor,
  

• 				attributes, data_size, data);
  

• return EFI_EXIT(ret); }

On Tue, Jun 04, 2019 at 11:38:27PM +0200, Heinrich Schuchardt wrote:

On 6/4/19 8:52 AM, AKASHI Takahiro wrote:

...

Data cache will be read in from persistent storage after (re)boot to restore UEFI non-volatile variables.

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

lib/efi_loader/efi_setup.c | 6 ++++++ 1 file changed, 6 insertions(+)

diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c index 8691d686d29d..45d6aca051f3 100644 --- a/lib/efi_loader/efi_setup.c +++ b/lib/efi_loader/efi_setup.c @@ -8,6 +8,7 @@ #include <common.h> #include <bootm.h> #include <efi_loader.h> +#include <environment.h>

#define OBJ_LIST_NOT_INITIALIZED 1

@@ -102,6 +103,11 @@ efi_status_t efi_init_obj_list(void) /* On ARM switch from EL3 or secure mode to EL2 or non-secure mode */ switch_to_non_secure_mode();

+#ifdef CONFIG_EFI_VARIABLE_USE_ENV

No clue what ENV refers to here as we are not talking about U-Boot environment variables anymore. How about CONFIG_EFI_PERSISTENT_VARIABLES.

It will be trivial once you take a look at "menuconfig."

...

• /* Load non-volatile variables */
• env_efi_load();

Can't we make env_efi_load() a __weak function which does nothing. If we have a backend, that backend replaces the weak function. That way we restrict the config variables to the Makefile.

This is a discussion. There can be different approaches here, so I would like to deter to a developer who will implement a next backing storage (other than U-Boot env), which is likely to be Standalone MM services for secure boot. Unfortunately I'm not responsible for that(StMM).

I hope that some Linaro engineers may have comments here.

Thanks, -Takahiro Akashi

Regards

Heinrich

...

+#endif

• /* Define supported languages */ ret = efi_init_platform_lang(); if (ret != EFI_SUCCESS)

On 6/4/19 8:52 AM, AKASHI Takahiro wrote:

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

Reviewed by: Heinrich Schuchardt xypron.glpk@gmx.de

---

lib/efi_loader/efi_bootmgr.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index 43791422c819..b2102c5b5af2 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -210,7 +210,8 @@ efi_status_t efi_bootmgr_load(efi_handle_t *handle) ret = EFI_CALL(efi_set_variable( L"BootNext", (efi_guid_t *)&efi_global_variable_guid,

• 			0, 0, &bootnext));
  

• 			EFI_VARIABLE_NON_VOLATILE, 0,
  

• 			&bootnext));
  
  

  /* load BootNext */ if (ret == EFI_SUCCESS) {

On Tue, Jun 04, 2019 at 03:52:10PM +0900, AKASHI Takahiro wrote:

Boot####, BootOrder and BootNext should be non-volatile.

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org Reviewed-by: Heinrich Schuchardt

---

cmd/efidebug.c | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/cmd/efidebug.c b/cmd/efidebug.c index c4ac9dd634e2..e65722625455 100644 --- a/cmd/efidebug.c +++ b/cmd/efidebug.c @@ -558,6 +558,7 @@ static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag, }

ret = EFI_CALL(RT->set_variable(var_name16, &guid,

• 			EFI_VARIABLE_NON_VOLATILE |
  		EFI_VARIABLE_BOOTSERVICE_ACCESS |
  		EFI_VARIABLE_RUNTIME_ACCESS,
  		size, data));
  

@@ -909,6 +910,7 @@ static int do_efi_boot_next(cmd_tbl_t *cmdtp, int flag, guid = efi_global_variable_guid; size = sizeof(u16); ret = EFI_CALL(RT->set_variable(L"BootNext", &guid,

• 			EFI_VARIABLE_NON_VOLATILE |
  		EFI_VARIABLE_BOOTSERVICE_ACCESS |
  		EFI_VARIABLE_RUNTIME_ACCESS,
  		size, &bootnext));
  

@@ -964,6 +966,7 @@ static int do_efi_boot_order(cmd_tbl_t *cmdtp, int flag,

guid = efi_global_variable_guid; ret = EFI_CALL(RT->set_variable(L"BootOrder", &guid,

• 			EFI_VARIABLE_NON_VOLATILE |
  		EFI_VARIABLE_BOOTSERVICE_ACCESS |
  		EFI_VARIABLE_RUNTIME_ACCESS,
  		size, bootorder));
  

Acked-by: Ilias Apalodimas ilias.apalodimas@linaro.org