[PATCH 00/16] efi_loader: non-volatile and runtime variables

[PATCH 00/16] efi_loader: non-volatile and runtime variables

[Heinrich Schuchardt]

Up to UEFI variable where stored in U-Boot environment variables. Saving
UEFI variables was not possible without saving the U-Boot environment
variables.

With this patch series file ubootefi.var in the EFI system partition is
used for saving UEFI variables.

Furthermore the UEFI variables are exposed at runtime and are writable at
runtime.

The missing piece is transferring the variable changed at runtime back to
the firmware. I will evaluate the following options:

* using a fixed memory address: we could read the memory area after a
  reboot
* using a systemd service which is called before the system goes down

Many of the CCs are due to the changes in disk/part_efi.c. Here the logic
to detect the EFI system partition is introduced (patch 04/16).

Heinrich Schuchardt (16):
  cmd: efidebug: fix int to pointer cast
  efi_loader: only reserve memory if fdt node enabled
  efi_loader: eliminate EFI_CALL() for variable access
  part: detect EFI system partition
  efi_loader: identify EFI system partition
  efi_loader: keep attributes in efi_set_variable_int()
  efi_loader: export initialization state
  efi_loader: change setup sequence
  efi_loader: imply FAT, FAT_WRITE
  efi_loader: UEFI variable persistence
  efi_loader: export efi_convert_pointer()
  efi_loader: optional pointer for ConvertPointer
  efi_loader: memory buffer for variables
  efi_loader: use memory based variable storage
  efi_loader: enable UEFI variables at runtime
  efi_selftest: adjust runtime test for variables

 cmd/bootefi.c                                 |   3 +-
 cmd/efidebug.c                                |  71 +-
 cmd/nvedit_efi.c                              |  18 +-
 disk/part_dos.c                               |  10 +-
 disk/part_efi.c                               |  12 +-
 include/efi_api.h                             |   2 +
 include/efi_loader.h                          |  22 +
 include/efi_variable.h                        |  52 ++
 include/part.h                                |  11 +-
 lib/efi_loader/Kconfig                        |  10 +
 lib/efi_loader/Makefile                       |   2 +
 lib/efi_loader/efi_bootmgr.c                  |  20 +-
 lib/efi_loader/efi_disk.c                     |  20 +
 lib/efi_loader/efi_runtime.c                  |  20 +-
 lib/efi_loader/efi_setup.c                    |  54 +-
 lib/efi_loader/efi_variable.c                 | 635 ++++++------------
 lib/efi_loader/efi_variables_file.c           | 235 +++++++
 lib/efi_loader/efi_variables_mem.c            | 324 +++++++++
 .../efi_selftest_variables_runtime.c          |  47 +-
 19 files changed, 1037 insertions(+), 531 deletions(-)
 create mode 100644 include/efi_variable.h
 create mode 100644 lib/efi_loader/efi_variables_file.c
 create mode 100644 lib/efi_loader/efi_variables_mem.c

--
2.25.1

[PATCH 01/16] cmd: efidebug: fix int to pointer cast

[Heinrich Schuchardt]

On 32 bit systems fix
warning: cast to pointer from integer of different size
[-Wint-to-pointer-cast]

Fixes: a415d61eac26 ("cmd: map addresses to sysmem in efidebug memmap")
Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 cmd/efidebug.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/cmd/efidebug.c b/cmd/efidebug.c
index bb7c13d6a1..c1bb76477a 100644
--- a/cmd/efidebug.c
+++ b/cmd/efidebug.c
@@ -489,10 +489,12 @@ static int do_efi_show_memmap(cmd_tbl_t *cmdtp, int flag,

 		printf("%-16s %.*llx-%.*llx", type,
 		       EFI_PHYS_ADDR_WIDTH,
-		       (u64)map_to_sysmem((void *)map->physical_start),
+		       (u64)map_to_sysmem((void *)(uintptr_t)
+					  map->physical_start),
 		       EFI_PHYS_ADDR_WIDTH,
-		       (u64)map_to_sysmem((void *)map->physical_start +
-					  map->num_pages * EFI_PAGE_SIZE));
+		       (u64)map_to_sysmem((void *)(uintptr_t)
+					  (map->physical_start +
+					   map->num_pages * EFI_PAGE_SIZE)));

 		print_memory_attributes(map->attribute);
 		putc('\n');
--
2.25.1

[PATCH 02/16] efi_loader: only reserve memory if fdt node enabled

[Heinrich Schuchardt]

Sub-nodes of /reserved-memory may be disabled. In this case we should not
reserve memory in the memory map.

Reported-by: Patrick DELAUNAY <patrick.delaunay@st.com>
Fixes: fef907b2e440 ("efi_loader: create reservations after
ft_board_setup")
Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
Reviewed-by: Atish Patra <atish.patra@wdc.com>
---
 cmd/bootefi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 3bbe2d6a1a..aaed575505 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -199,7 +199,8 @@ static void efi_carve_out_dt_rsv(void *fdt)
 			 * The /reserved-memory node may have children with
 			 * a size instead of a reg property.
 			 */
-			if (addr != FDT_ADDR_T_NONE)
+			if (addr != FDT_ADDR_T_NONE &&
+			    fdtdec_get_is_enabled(fdt, subnode))
 				efi_reserve_memory(addr, size);
 			subnode = fdt_next_subnode(fdt, subnode);
 		}
--
2.25.1

[PATCH 03/16] efi_loader: eliminate EFI_CALL() for variable access

[Heinrich Schuchardt]

In several places of the UEFI sub-system UEFI variables as accessed via
runtime services functions. These functions require being called via
EFI_CALL() to restore the register holding the gd variable. Some code
even calls the functions via the runtime services table.

By making the functions exposing the variable runtime services wrappers for
exported functions that we can use internally we get rid of this clumsy
code.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 cmd/efidebug.c                |  63 +++++++++----------
 cmd/nvedit_efi.c              |  18 +++---
 include/efi_loader.h          |   9 +++
 lib/efi_loader/efi_bootmgr.c  |  20 +++---
 lib/efi_loader/efi_setup.c    |  42 ++++++-------
 lib/efi_loader/efi_variable.c | 114 +++++++++++++++++++++++++++-------
 6 files changed, 168 insertions(+), 98 deletions(-)

diff --git a/cmd/efidebug.c b/cmd/efidebug.c
index c1bb76477a..f89c1d2db7 100644
--- a/cmd/efidebug.c
+++ b/cmd/efidebug.c
@@ -17,7 +17,6 @@
 #include <linux/ctype.h>

 #define BS systab.boottime
-#define RT systab.runtime

 /**
  * efi_get_device_handle_info() - get information of UEFI device
@@ -614,11 +613,11 @@ static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag,
 		goto out;
 	}

-	ret = EFI_CALL(RT->set_variable(var_name16, &guid,
-					EFI_VARIABLE_NON_VOLATILE |
-					EFI_VARIABLE_BOOTSERVICE_ACCESS |
-					EFI_VARIABLE_RUNTIME_ACCESS,
-					size, data));
+	ret = efi_set_variable_int(var_name16, &guid,
+				   EFI_VARIABLE_NON_VOLATILE |
+				   EFI_VARIABLE_BOOTSERVICE_ACCESS |
+				   EFI_VARIABLE_RUNTIME_ACCESS,
+				   size, data);
 	if (ret != EFI_SUCCESS) {
 		printf("Cannot set %ls\n", var_name16);
 		r = CMD_RET_FAILURE;
@@ -669,7 +668,8 @@ static int do_efi_boot_rm(cmd_tbl_t *cmdtp, int flag,
 		p = var_name16;
 		utf8_utf16_strncpy(&p, var_name, 9);

-		ret = EFI_CALL(RT->set_variable(var_name16, &guid, 0, 0, NULL));
+		ret = efi_set_variable_int(var_name16, &guid, 0, 0,
+					   NULL);
 		if (ret) {
 			printf("Cannot remove %ls\n", var_name16);
 			return CMD_RET_FAILURE;
@@ -749,11 +749,11 @@ static void show_efi_boot_opt(int id)
 	guid = efi_global_variable_guid;

 	size = 0;
-	ret = EFI_CALL(RT->get_variable(var_name16, &guid, NULL, &size, NULL));
+	ret = efi_get_variable_int(var_name16, &guid, NULL, &size, NULL);
 	if (ret == EFI_BUFFER_TOO_SMALL) {
 		data = malloc(size);
-		ret = EFI_CALL(RT->get_variable(var_name16, &guid, NULL, &size,
-						data));
+		ret = efi_get_variable_int(var_name16, &guid, NULL, &size,
+					   data);
 	}
 	if (ret == EFI_SUCCESS)
 		show_efi_boot_opt_data(id, data, size);
@@ -808,8 +808,7 @@ static int do_efi_boot_dump(cmd_tbl_t *cmdtp, int flag,
 	var_name16[0] = 0;
 	for (;;) {
 		size = buf_size;
-		ret = EFI_CALL(efi_get_next_variable_name(&size, var_name16,
-							  &guid));
+		ret = efi_get_next_variable_name_int(&size, var_name16, &guid);
 		if (ret == EFI_NOT_FOUND)
 			break;
 		if (ret == EFI_BUFFER_TOO_SMALL) {
@@ -820,9 +819,8 @@ static int do_efi_boot_dump(cmd_tbl_t *cmdtp, int flag,
 				return CMD_RET_FAILURE;
 			}
 			var_name16 = p;
-			ret = EFI_CALL(efi_get_next_variable_name(&size,
-								  var_name16,
-								  &guid));
+			ret = efi_get_next_variable_name_int(&size, var_name16,
+							     &guid);
 		}
 		if (ret != EFI_SUCCESS) {
 			free(var_name16);
@@ -870,12 +868,11 @@ static int show_efi_boot_order(void)

 	guid = efi_global_variable_guid;
 	size = 0;
-	ret = EFI_CALL(RT->get_variable(L"BootOrder", &guid, NULL, &size,
-					NULL));
+	ret = efi_get_variable_int(L"BootOrder", &guid, NULL, &size, NULL);
 	if (ret == EFI_BUFFER_TOO_SMALL) {
 		bootorder = malloc(size);
-		ret = EFI_CALL(RT->get_variable(L"BootOrder", &guid, NULL,
-						&size, bootorder));
+		ret = efi_get_variable_int(L"BootOrder", &guid, NULL,
+					   &size, bootorder);
 	}
 	if (ret == EFI_NOT_FOUND) {
 		printf("BootOrder not defined\n");
@@ -893,8 +890,8 @@ static int show_efi_boot_order(void)
 		utf8_utf16_strncpy(&p16, var_name, 9);

 		size = 0;
-		ret = EFI_CALL(RT->get_variable(var_name16, &guid, NULL, &size,
-						NULL));
+		ret = efi_get_variable_int(var_name16, &guid, NULL, &size,
+					   NULL);
 		if (ret != EFI_BUFFER_TOO_SMALL) {
 			printf("%2d: Boot%04X: (not defined)\n",
 			       i + 1, bootorder[i]);
@@ -906,8 +903,8 @@ static int show_efi_boot_order(void)
 			ret = CMD_RET_FAILURE;
 			goto out;
 		}
-		ret = EFI_CALL(RT->get_variable(var_name16, &guid, NULL, &size,
-						data));
+		ret = efi_get_variable_int(var_name16, &guid, NULL, &size,
+					   data);
 		if (ret != EFI_SUCCESS) {
 			free(data);
 			ret = CMD_RET_FAILURE;
@@ -974,11 +971,11 @@ 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));
+	ret = efi_set_variable_int(L"BootNext", &guid,
+				   EFI_VARIABLE_NON_VOLATILE |
+				   EFI_VARIABLE_BOOTSERVICE_ACCESS |
+				   EFI_VARIABLE_RUNTIME_ACCESS,
+				   size, &bootnext);
 	if (ret != EFI_SUCCESS) {
 		printf("Cannot set BootNext\n");
 		r = CMD_RET_FAILURE;
@@ -1035,11 +1032,11 @@ 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));
+	ret = efi_set_variable_int(L"BootOrder", &guid,
+				   EFI_VARIABLE_NON_VOLATILE |
+				   EFI_VARIABLE_BOOTSERVICE_ACCESS |
+				   EFI_VARIABLE_RUNTIME_ACCESS,
+				   size, bootorder);
 	if (ret != EFI_SUCCESS) {
 		printf("Cannot set BootOrder\n");
 		r = CMD_RET_FAILURE;
diff --git a/cmd/nvedit_efi.c b/cmd/nvedit_efi.c
index 8ea0da0128..9d338478d3 100644
--- a/cmd/nvedit_efi.c
+++ b/cmd/nvedit_efi.c
@@ -87,14 +87,14 @@ static void efi_dump_single_var(u16 *name, const efi_guid_t *guid, bool verbose)

 	data = NULL;
 	size = 0;
-	ret = EFI_CALL(efi_get_variable(name, guid, &attributes, &size, data));
+	ret = efi_get_variable_int(name, guid, &attributes, &size, data);
 	if (ret == EFI_BUFFER_TOO_SMALL) {
 		data = malloc(size);
 		if (!data)
 			goto out;

-		ret = EFI_CALL(efi_get_variable(name, guid, &attributes, &size,
-						data));
+		ret = efi_get_variable_int(name, guid, &attributes, &size,
+					   data);
 	}
 	if (ret == EFI_NOT_FOUND) {
 		printf("Error: \"%ls\" not defined\n", name);
@@ -224,8 +224,7 @@ static int efi_dump_var_all(int argc,  char * const argv[],
 	var_name16[0] = 0;
 	for (;;) {
 		size = buf_size;
-		ret = EFI_CALL(efi_get_next_variable_name(&size, var_name16,
-							  &guid));
+		ret = efi_get_next_variable_name_int(&size, var_name16, &guid);
 		if (ret == EFI_NOT_FOUND)
 			break;
 		if (ret == EFI_BUFFER_TOO_SMALL) {
@@ -236,9 +235,8 @@ static int efi_dump_var_all(int argc,  char * const argv[],
 				return CMD_RET_FAILURE;
 			}
 			var_name16 = p;
-			ret = EFI_CALL(efi_get_next_variable_name(&size,
-								  var_name16,
-								  &guid));
+			ret = efi_get_next_variable_name_int(&size, var_name16,
+							     &guid);
 		}
 		if (ret != EFI_SUCCESS) {
 			free(var_name16);
@@ -576,8 +574,8 @@ int do_env_set_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	p = var_name16;
 	utf8_utf16_strncpy(&p, var_name, len + 1);

-	ret = EFI_CALL(efi_set_variable(var_name16, &guid, attributes,
-					size, value));
+	ret = efi_set_variable_int(var_name16, &guid, attributes, size,
+				   value);
 	unmap_sysmem(value);
 	if (ret == EFI_SUCCESS) {
 		ret = CMD_RET_SUCCESS;
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 3f2792892f..b1d2feab61 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -643,12 +643,21 @@ efi_status_t EFIAPI efi_selftest(efi_handle_t image_handle,
 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 efi_get_variable_int(u16 *variable_name, const efi_guid_t *vendor,
+				  u32 *attributes,
+				  efi_uintn_t *data_size, void *data);
 efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size,
 					       u16 *variable_name,
 					       efi_guid_t *vendor);
+efi_status_t efi_get_next_variable_name_int(efi_uintn_t *variable_name_size,
+					    u16 *variable_name,
+					    efi_guid_t *vendor);
 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 efi_set_variable_int(u16 *variable_name,
+				  const efi_guid_t *vendor, u32 attributes,
+				  efi_uintn_t data_size, const void *data);

 efi_status_t EFIAPI efi_query_variable_info(
 			u32 attributes, u64 *maximum_variable_storage_size,
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index 2ea21448f0..c87f38e46c 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -12,7 +12,6 @@
 #include <asm/unaligned.h>

 static const struct efi_boot_services *bs;
-static const struct efi_runtime_services *rs;

 /*
  * bootmgr implements the logic of trying to find a payload to boot
@@ -123,10 +122,10 @@ static void *get_var(u16 *name, const efi_guid_t *vendor,
 	void *buf = NULL;

 	*size = 0;
-	EFI_CALL(ret = rs->get_variable(name, v, NULL, size, buf));
+	ret = efi_get_variable_int(name, v, NULL, size, buf);
 	if (ret == EFI_BUFFER_TOO_SMALL) {
 		buf = malloc(*size);
-		EFI_CALL(ret = rs->get_variable(name, v, NULL, size, buf));
+		ret = efi_get_variable_int(name, v, NULL, size, buf);
 	}

 	if (ret != EFI_SUCCESS) {
@@ -186,10 +185,10 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle)
 		attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
 			     EFI_VARIABLE_RUNTIME_ACCESS;
 		size = sizeof(n);
-		ret = EFI_CALL(efi_set_variable(
+		ret = efi_set_variable_int(
 				L"BootCurrent",
 				(efi_guid_t *)&efi_global_variable_guid,
-				attributes, size, &n));
+				attributes, size, &n);
 		if (ret != EFI_SUCCESS) {
 			if (EFI_CALL(efi_unload_image(*handle))
 			    != EFI_SUCCESS)
@@ -226,25 +225,24 @@ efi_status_t efi_bootmgr_load(efi_handle_t *handle)
 	efi_status_t ret;

 	bs = systab.boottime;
-	rs = systab.runtime;

 	/* BootNext */
 	bootnext = 0;
 	size = sizeof(bootnext);
-	ret = EFI_CALL(efi_get_variable(L"BootNext",
-					(efi_guid_t *)&efi_global_variable_guid,
-					NULL, &size, &bootnext));
+	ret = efi_get_variable_int(L"BootNext",
+				   (efi_guid_t *)&efi_global_variable_guid,
+				   NULL, &size, &bootnext);
 	if (ret == EFI_SUCCESS || ret == EFI_BUFFER_TOO_SMALL) {
 		/* BootNext does exist here */
 		if (ret == EFI_BUFFER_TOO_SMALL || size != sizeof(u16))
 			printf("BootNext must be 16-bit integer\n");

 		/* delete BootNext */
-		ret = EFI_CALL(efi_set_variable(
+		ret = efi_set_variable_int(
 					L"BootNext",
 					(efi_guid_t *)&efi_global_variable_guid,
 					EFI_VARIABLE_NON_VOLATILE, 0,
-					&bootnext));
+					&bootnext);

 		/* load BootNext */
 		if (ret == EFI_SUCCESS) {
diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
index b458093dfb..d1884e4dae 100644
--- a/lib/efi_loader/efi_setup.c
+++ b/lib/efi_loader/efi_setup.c
@@ -40,12 +40,12 @@ static efi_status_t efi_init_platform_lang(void)
 	 * Variable PlatformLangCodes defines the language codes that the
 	 * machine can support.
 	 */
-	ret = EFI_CALL(efi_set_variable(L"PlatformLangCodes",
-					&efi_global_variable_guid,
-					EFI_VARIABLE_BOOTSERVICE_ACCESS |
-					EFI_VARIABLE_RUNTIME_ACCESS,
-					sizeof(CONFIG_EFI_PLATFORM_LANG_CODES),
-					CONFIG_EFI_PLATFORM_LANG_CODES));
+	ret = efi_set_variable_int(L"PlatformLangCodes",
+				   &efi_global_variable_guid,
+				   EFI_VARIABLE_BOOTSERVICE_ACCESS |
+				   EFI_VARIABLE_RUNTIME_ACCESS,
+				   sizeof(CONFIG_EFI_PLATFORM_LANG_CODES),
+				   CONFIG_EFI_PLATFORM_LANG_CODES);
 	if (ret != EFI_SUCCESS)
 		goto out;

@@ -53,9 +53,9 @@ static efi_status_t efi_init_platform_lang(void)
 	 * Variable PlatformLang defines the language that the machine has been
 	 * configured for.
 	 */
-	ret = EFI_CALL(efi_get_variable(L"PlatformLang",
-					&efi_global_variable_guid,
-					NULL, &data_size, &pos));
+	ret = efi_get_variable_int(L"PlatformLang",
+				   &efi_global_variable_guid,
+				   NULL, &data_size, &pos);
 	if (ret == EFI_BUFFER_TOO_SMALL) {
 		/* The variable is already set. Do not change it. */
 		ret = EFI_SUCCESS;
@@ -70,12 +70,12 @@ static efi_status_t efi_init_platform_lang(void)
 	if (pos)
 		*pos = 0;

-	ret = EFI_CALL(efi_set_variable(L"PlatformLang",
-					&efi_global_variable_guid,
-					EFI_VARIABLE_NON_VOLATILE |
-					EFI_VARIABLE_BOOTSERVICE_ACCESS |
-					EFI_VARIABLE_RUNTIME_ACCESS,
-					1 + strlen(lang), lang));
+	ret = efi_set_variable_int(L"PlatformLang",
+				   &efi_global_variable_guid,
+				   EFI_VARIABLE_NON_VOLATILE |
+				   EFI_VARIABLE_BOOTSERVICE_ACCESS |
+				   EFI_VARIABLE_RUNTIME_ACCESS,
+				   1 + strlen(lang), lang);
 out:
 	if (ret != EFI_SUCCESS)
 		printf("EFI: cannot initialize platform language settings\n");
@@ -113,12 +113,12 @@ efi_status_t efi_init_obj_list(void)
 		goto out;

 	/* Indicate supported features */
-	ret = EFI_CALL(efi_set_variable(L"OsIndicationsSupported",
-					&efi_global_variable_guid,
-					EFI_VARIABLE_BOOTSERVICE_ACCESS |
-					EFI_VARIABLE_RUNTIME_ACCESS,
-					sizeof(os_indications_supported),
-					&os_indications_supported));
+	ret = efi_set_variable_int(L"OsIndicationsSupported",
+				   &efi_global_variable_guid,
+				   EFI_VARIABLE_BOOTSERVICE_ACCESS |
+				   EFI_VARIABLE_RUNTIME_ACCESS,
+				   sizeof(os_indications_supported),
+				   &os_indications_supported);
 	if (ret != EFI_SUCCESS)
 		goto out;

diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
index fe2f264591..4fae1fa4c7 100644
--- a/lib/efi_loader/efi_variable.c
+++ b/lib/efi_loader/efi_variable.c
@@ -165,6 +165,30 @@ static const char *parse_attr(const char *str, u32 *attrp)
 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_ENTRY("\"%ls\" %pUl %p %p %p", variable_name, vendor, attributes,
+		  data_size, data);
+
+	return EFI_EXIT(efi_get_variable_int(variable_name, vendor, attributes,
+					     data_size, data));
+}
+
+/**
+ * efi_get_variable_int() - retrieve value of a UEFI variable
+ *
+ * This function allows to retrieve the value of a UEFI variable without using
+ * EFI_CALL().
+ *
+ * @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 efi_get_variable_int(u16 *variable_name,
+				  const efi_guid_t *vendor, u32 *attributes,
+				  efi_uintn_t *data_size, void *data)
 {
 	char *native_name;
 	efi_status_t ret;
@@ -172,22 +196,20 @@ 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);
+		return 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);
 	free(native_name);
 	if (!val)
-		return EFI_EXIT(EFI_NOT_FOUND);
+		return EFI_NOT_FOUND;

 	val = parse_attr(val, &attr);

@@ -198,7 +220,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 +232,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 +249,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,14 +257,14 @@ 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 EFI_EXIT(ret);
+	return ret;
 }

 static char *efi_variables_list;
@@ -331,6 +353,33 @@ static efi_status_t parse_uboot_variable(char *variable,
 efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size,
 					       u16 *variable_name,
 					       efi_guid_t *vendor)
+{
+	EFI_ENTRY("%p \"%ls\" %pUl", variable_name_size, variable_name, vendor);
+
+	return EFI_EXIT(efi_get_next_variable_name_int(variable_name_size,
+						       variable_name, vendor));
+}
+
+/**
+ * efi_get_next_variable_name_int() - enumerate the current variable names
+ *
+ * This function can be used to enumerate UEFI variable without using
+ * EFI_CALL().
+ *
+ * @variable_name_size:	size of variable_name buffer in byte
+ * @variable_name:	name of uefi variable's name in u16
+ * @vendor:		vendor's guid
+ *
+ * This function implements the GetNextVariableName service.
+ *
+ * See the Unified Extensible Firmware Interface (UEFI) specification for
+ * details.
+ *
+ * Return: status code
+ */
+efi_status_t efi_get_next_variable_name_int(efi_uintn_t *variable_name_size,
+					    u16 *variable_name,
+					    efi_guid_t *vendor)
 {
 	char *native_name, *variable;
 	ssize_t name_len, list_len;
@@ -340,10 +389,8 @@ efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size,
 	int i;
 	efi_status_t ret;

-	EFI_ENTRY("%p \"%ls\" %pUl", variable_name_size, variable_name, vendor);
-
 	if (!variable_name_size || !variable_name || !vendor)
-		return EFI_EXIT(EFI_INVALID_PARAMETER);
+		return EFI_INVALID_PARAMETER;

 	if (variable_name[0]) {
 		/* check null-terminated string */
@@ -351,12 +398,12 @@ efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size,
 			if (!variable_name[i])
 				break;
 		if (i >= *variable_name_size)
-			return EFI_EXIT(EFI_INVALID_PARAMETER);
+			return EFI_INVALID_PARAMETER;

 		/* search for the last-returned variable */
 		ret = efi_to_native(&native_name, variable_name, vendor);
 		if (ret)
-			return EFI_EXIT(ret);
+			return ret;

 		name_len = strlen(native_name);
 		for (variable = efi_variables_list; variable && *variable;) {
@@ -371,14 +418,14 @@ efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size,

 		free(native_name);
 		if (!(variable && *variable))
-			return EFI_EXIT(EFI_INVALID_PARAMETER);
+			return EFI_INVALID_PARAMETER;

 		/* next variable */
 		variable = strchr(variable, '\n');
 		if (variable)
 			variable++;
 		if (!(variable && *variable))
-			return EFI_EXIT(EFI_NOT_FOUND);
+			return EFI_NOT_FOUND;
 	} else {
 		/*
 		 *new search: free a list used in the previous search
@@ -393,7 +440,7 @@ efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size,
 				     &efi_variables_list, 0, 1, regexlist);
 		/* 1 indicates that no match was found */
 		if (list_len <= 1)
-			return EFI_EXIT(EFI_NOT_FOUND);
+			return EFI_NOT_FOUND;

 		variable = efi_variables_list;
 	}
@@ -401,7 +448,7 @@ efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size,
 	ret = parse_uboot_variable(variable, variable_name_size, variable_name,
 				   vendor, &attributes);

-	return EFI_EXIT(ret);
+	return ret;
 }

 /**
@@ -422,6 +469,29 @@ efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size,
 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_ENTRY("\"%ls\" %pUl %x %zu %p", variable_name, vendor, attributes,
+		  data_size, data);
+
+	return EFI_EXIT(efi_set_variable_int(variable_name, vendor, attributes,
+					     data_size, data));
+}
+
+/**
+ * efi_set_variable_int() - set value of a UEFI variable internal
+ *
+ * This function can be used to set a UEFI variable without using EFI_CALL().
+ *
+ * @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 efi_set_variable_int(u16 *variable_name,
+				  const efi_guid_t *vendor, u32 attributes,
+				  efi_uintn_t data_size, const void *data)
 {
 	char *native_name = NULL, *val = NULL, *s;
 	const char *old_val;
@@ -429,8 +499,6 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
 	efi_status_t ret = EFI_SUCCESS;
 	u32 attr;

-	EFI_ENTRY("\"%ls\" %pUl %x %zu %p", variable_name, vendor, attributes,
-		  data_size, data);

 	if (!variable_name || !*variable_name || !vendor ||
 	    ((attributes & EFI_VARIABLE_RUNTIME_ACCESS) &&
@@ -540,7 +608,7 @@ out:
 	free(native_name);
 	free(val);

-	return EFI_EXIT(ret);
+	return ret;
 }

 /**
--
2.25.1

[PATCH 04/16] part: detect EFI system partition

[Heinrich Schuchardt]

Up to now for MBR and GPT partitions the info field 'bootable' was set to 1
if either the partition was an EFI system partition or the bootable flag
was set.

Turn info field 'bootable' into a bit mask with separate bits for bootable
and EFI system partition.

This will allow us to identify the EFI system partition in the UEFI
sub-system.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 disk/part_dos.c | 10 ++++++++--
 disk/part_efi.c | 12 ++++++++----
 include/part.h  | 11 ++++++++++-
 3 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/disk/part_dos.c b/disk/part_dos.c
index 83ff40d310..0ec7f1628e 100644
--- a/disk/part_dos.c
+++ b/disk/part_dos.c
@@ -45,9 +45,15 @@ static inline int is_extended(int part_type)
 	    part_type == 0x85);
 }

-static inline int is_bootable(dos_partition_t *p)
+static int is_bootable(dos_partition_t *p)
 {
-	return (p->sys_ind == 0xef) || (p->boot_ind == 0x80);
+	int ret = 0;
+
+	if (p->sys_ind == 0xef)
+		ret |= PART_EFI_SYSTEM_PARTITION;
+	if (p->boot_ind == 0x80)
+		ret |= PART_BOOTABLE;
+	return ret;
 }

 static void print_one_part(dos_partition_t *p, lbaint_t ext_part_sector,
diff --git a/disk/part_efi.c b/disk/part_efi.c
index b2e157d9c1..19f1f43f4e 100644
--- a/disk/part_efi.c
+++ b/disk/part_efi.c
@@ -71,11 +71,15 @@ static char *print_efiname(gpt_entry *pte)

 static const efi_guid_t system_guid = PARTITION_SYSTEM_GUID;

-static inline int is_bootable(gpt_entry *p)
+static int is_bootable(gpt_entry *p)
 {
-	return p->attributes.fields.legacy_bios_bootable ||
-		!memcmp(&(p->partition_type_guid), &system_guid,
-			sizeof(efi_guid_t));
+	int ret = 0;
+
+	if (!memcmp(&p->partition_type_guid, &system_guid, sizeof(efi_guid_t)))
+		ret |=  PART_EFI_SYSTEM_PARTITION;
+	if (p->attributes.fields.legacy_bios_bootable)
+		ret |=  PART_BOOTABLE;
+	return ret;
 }

 static int validate_gpt_header(gpt_header *gpt_h, lbaint_t lba,
diff --git a/include/part.h b/include/part.h
index 0b5cf3d5e8..a63d1d0cda 100644
--- a/include/part.h
+++ b/include/part.h
@@ -51,13 +51,22 @@ struct block_drvr {
 #define PART_TYPE_LEN 32
 #define MAX_SEARCH_PARTITIONS 64

+#define PART_BOOTABLE			1
+#define PART_EFI_SYSTEM_PARTITION	2
+
 typedef struct disk_partition {
 	lbaint_t	start;	/* # of first block in partition	*/
 	lbaint_t	size;	/* number of blocks in partition	*/
 	ulong	blksz;		/* block size in bytes			*/
 	uchar	name[PART_NAME_LEN];	/* partition name			*/
 	uchar	type[PART_TYPE_LEN];	/* string type description		*/
-	int	bootable;	/* Active/Bootable flag is set		*/
+	/*
+	 * The bootable is a bitmask with the following fields:
+	 *
+	 * PART_BOOTABLE		the MBR bootable flag is set
+	 * PART_EFI_SYSTEM_PARTITION	the partition is an EFI system partition
+	 */
+	int	bootable;
 #if CONFIG_IS_ENABLED(PARTITION_UUIDS)
 	char	uuid[UUID_STR_LEN + 1];	/* filesystem UUID as string, if exists	*/
 #endif
--
2.25.1

[PATCH 04/16] part: detect EFI system partition

[Punit Agrawal]

Hi Heinrich,

Heinrich Schuchardt <xypron.glpk@gmx.de> writes:

> Up to now for MBR and GPT partitions the info field 'bootable' was set to 1
> if either the partition was an EFI system partition or the bootable flag
> was set.
>
> Turn info field 'bootable' into a bit mask with separate bits for bootable
> and EFI system partition.
>
> This will allow us to identify the EFI system partition in the UEFI
> sub-system.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  disk/part_dos.c | 10 ++++++++--
>  disk/part_efi.c | 12 ++++++++----
>  include/part.h  | 11 ++++++++++-
>  3 files changed, 26 insertions(+), 7 deletions(-)
>
> diff --git a/disk/part_dos.c b/disk/part_dos.c

[...]

> diff --git a/include/part.h b/include/part.h
> index 0b5cf3d5e8..a63d1d0cda 100644
> --- a/include/part.h
> +++ b/include/part.h
> @@ -51,13 +51,22 @@ struct block_drvr {
>  #define PART_TYPE_LEN 32
>  #define MAX_SEARCH_PARTITIONS 64
>
> +#define PART_BOOTABLE			1
> +#define PART_EFI_SYSTEM_PARTITION	2
> +

Maybe you missed my comments on the previous version [0].

As the above defines are used as bit positions, please consider using
BIT() for clarity.

Thanks,
Punit

[0] https://lists.denx.de/pipermail/u-boot/2020-March/404081.html

>  typedef struct disk_partition {
>  	lbaint_t	start;	/* # of first block in partition	*/
>  	lbaint_t	size;	/* number of blocks in partition	*/
>  	ulong	blksz;		/* block size in bytes			*/
>  	uchar	name[PART_NAME_LEN];	/* partition name			*/
>  	uchar	type[PART_TYPE_LEN];	/* string type description		*/
> -	int	bootable;	/* Active/Bootable flag is set		*/
> +	/*
> +	 * The bootable is a bitmask with the following fields:
> +	 *
> +	 * PART_BOOTABLE		the MBR bootable flag is set
> +	 * PART_EFI_SYSTEM_PARTITION	the partition is an EFI system partition
> +	 */
> +	int	bootable;
>  #if CONFIG_IS_ENABLED(PARTITION_UUIDS)
>  	char	uuid[UUID_STR_LEN + 1];	/* filesystem UUID as string, if exists	*/
>  #endif
> --
> 2.25.1

[PATCH 04/16] part: detect EFI system partition

[Heinrich Schuchardt]

On 3/27/20 7:35 AM, Punit Agrawal wrote:
> Hi Heinrich,
>
> Heinrich Schuchardt <xypron.glpk@gmx.de> writes:
>
>> Up to now for MBR and GPT partitions the info field 'bootable' was set to 1
>> if either the partition was an EFI system partition or the bootable flag
>> was set.
>>
>> Turn info field 'bootable' into a bit mask with separate bits for bootable
>> and EFI system partition.
>>
>> This will allow us to identify the EFI system partition in the UEFI
>> sub-system.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>   disk/part_dos.c | 10 ++++++++--
>>   disk/part_efi.c | 12 ++++++++----
>>   include/part.h  | 11 ++++++++++-
>>   3 files changed, 26 insertions(+), 7 deletions(-)
>>
>> diff --git a/disk/part_dos.c b/disk/part_dos.c
>
> [...]
>
>> diff --git a/include/part.h b/include/part.h
>> index 0b5cf3d5e8..a63d1d0cda 100644
>> --- a/include/part.h
>> +++ b/include/part.h
>> @@ -51,13 +51,22 @@ struct block_drvr {
>>   #define PART_TYPE_LEN 32
>>   #define MAX_SEARCH_PARTITIONS 64
>>
>> +#define PART_BOOTABLE			1
>> +#define PART_EFI_SYSTEM_PARTITION	2
>> +
>
> Maybe you missed my comments on the previous version [0].
>
> As the above defines are used as bit positions, please consider using
> BIT() for clarity.
>
> Thanks,
> Punit

Sorry to miss that. I will change it in the next round.

Thanks for reviewing.

Best regards

Heinrich

>
> [0] https://lists.denx.de/pipermail/u-boot/2020-March/404081.html
>
>>   typedef struct disk_partition {
>>   	lbaint_t	start;	/* # of first block in partition	*/
>>   	lbaint_t	size;	/* number of blocks in partition	*/
>>   	ulong	blksz;		/* block size in bytes			*/
>>   	uchar	name[PART_NAME_LEN];	/* partition name			*/
>>   	uchar	type[PART_TYPE_LEN];	/* string type description		*/
>> -	int	bootable;	/* Active/Bootable flag is set		*/
>> +	/*
>> +	 * The bootable is a bitmask with the following fields:
>> +	 *
>> +	 * PART_BOOTABLE		the MBR bootable flag is set
>> +	 * PART_EFI_SYSTEM_PARTITION	the partition is an EFI system partition
>> +	 */
>> +	int	bootable;
>>   #if CONFIG_IS_ENABLED(PARTITION_UUIDS)
>>   	char	uuid[UUID_STR_LEN + 1];	/* filesystem UUID as string, if exists	*/
>>   #endif
>> --
>> 2.25.1

[PATCH 05/16] efi_loader: identify EFI system partition

[Heinrich Schuchardt]

For capsule updates we need to identify the EFI system partition.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 include/efi_loader.h      |  7 +++++++
 lib/efi_loader/efi_disk.c | 20 ++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index b1d2feab61..5890871470 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -45,6 +45,13 @@ static inline void *guidcpy(void *dst, const void *src)
 /* Root node */
 extern efi_handle_t efi_root;

+/* EFI system partition */
+extern struct efi_system_partition {
+	enum if_type if_type;
+	int devnum;
+	u8 part;
+} efi_system_partition;
+
 int __efi_entry_check(void);
 int __efi_exit_check(void);
 const char *__efi_nesting(void);
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
index fc0682bc48..2f752a5e99 100644
--- a/lib/efi_loader/efi_disk.c
+++ b/lib/efi_loader/efi_disk.c
@@ -13,6 +13,8 @@
 #include <part.h>
 #include <malloc.h>

+struct efi_system_partition efi_system_partition;
+
 const efi_guid_t efi_block_io_guid = EFI_BLOCK_IO_PROTOCOL_GUID;

 /**
@@ -372,6 +374,24 @@ static efi_status_t efi_disk_add_dev(
 	diskobj->ops.media = &diskobj->media;
 	if (disk)
 		*disk = diskobj;
+
+	/* Store first EFI system partition */
+	if (part && !efi_system_partition.if_type) {
+		int r;
+		disk_partition_t info;
+
+		r = part_get_info(desc, part, &info);
+		if (r)
+			return EFI_DEVICE_ERROR;
+		if (info.bootable & PART_EFI_SYSTEM_PARTITION) {
+			efi_system_partition.if_type = desc->if_type;
+			efi_system_partition.devnum = desc->devnum;
+			efi_system_partition.part = part;
+			EFI_PRINT("EFI system partition: %s %d:%d\n",
+				  blk_get_if_type_name(desc->if_type),
+				  desc->devnum, part);
+		}
+	}
 	return EFI_SUCCESS;
 }

--
2.25.1

[PATCH 06/16] efi_loader: keep attributes in efi_set_variable_int()

[Heinrich Schuchardt]

Do not change the value of parameter attributes in function
efi_set_variable_int(). This allows to use it later.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_variable.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
index 4fae1fa4c7..d99ad6ddae 100644
--- a/lib/efi_loader/efi_variable.c
+++ b/lib/efi_loader/efi_variable.c
@@ -573,8 +573,8 @@ efi_status_t efi_set_variable_int(u16 *variable_name,
 		       EFI_VARIABLE_BOOTSERVICE_ACCESS |
 		       EFI_VARIABLE_RUNTIME_ACCESS);
 	s += sprintf(s, "{");
-	while (attributes) {
-		u32 attr = 1 << (ffs(attributes) - 1);
+	for (u32 attr_rem = attributes; attr_rem;) {
+		u32 attr = 1 << (ffs(attr_rem) - 1);

 		if (attr == EFI_VARIABLE_NON_VOLATILE)
 			s += sprintf(s, "nv");
@@ -583,8 +583,8 @@ efi_status_t efi_set_variable_int(u16 *variable_name,
 		else if (attr == EFI_VARIABLE_RUNTIME_ACCESS)
 			s += sprintf(s, "run");

-		attributes &= ~attr;
-		if (attributes)
+		attr_rem &= ~attr;
+		if (attr_rem)
 			s += sprintf(s, ",");
 	}
 	s += sprintf(s, "}");
--
2.25.1

[PATCH 07/16] efi_loader: export initialization state

[Heinrich Schuchardt]

Export the UEFI sub-system initialization state. This will allow to treat
the setting of UEFI variables during and after initialization differently.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 include/efi_loader.h       | 3 +++
 lib/efi_loader/efi_setup.c | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 5890871470..510f39fe9e 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -45,6 +45,9 @@ static inline void *guidcpy(void *dst, const void *src)
 /* Root node */
 extern efi_handle_t efi_root;

+/* Set to EFI_SUCCESS when initialized */
+extern efi_status_t efi_obj_list_initialized;
+
 /* EFI system partition */
 extern struct efi_system_partition {
 	enum if_type if_type;
diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
index d1884e4dae..f6b17b662c 100644
--- a/lib/efi_loader/efi_setup.c
+++ b/lib/efi_loader/efi_setup.c
@@ -11,7 +11,7 @@

 #define OBJ_LIST_NOT_INITIALIZED 1

-static efi_status_t efi_obj_list_initialized = OBJ_LIST_NOT_INITIALIZED;
+efi_status_t efi_obj_list_initialized = OBJ_LIST_NOT_INITIALIZED;

 /*
  * Allow unaligned memory access.
--
2.25.1

[PATCH 08/16] efi_loader: change setup sequence

[Heinrich Schuchardt]

If we want to restore variables from disk, we need to initialize block
devices before variables.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_setup.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
index f6b17b662c..1e3e474835 100644
--- a/lib/efi_loader/efi_setup.c
+++ b/lib/efi_loader/efi_setup.c
@@ -102,6 +102,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_PARTITIONS
+	ret = efi_disk_register();
+	if (ret != EFI_SUCCESS)
+		goto out;
+#endif
 	/* Initialize variable services */
 	ret = efi_init_variables();
 	if (ret != EFI_SUCCESS)
@@ -145,11 +150,6 @@ efi_status_t efi_init_obj_list(void)
 	ret = efi_console_register();
 	if (ret != EFI_SUCCESS)
 		goto out;
-#ifdef CONFIG_PARTITIONS
-	ret = efi_disk_register();
-	if (ret != EFI_SUCCESS)
-		goto out;
-#endif
 #if defined(CONFIG_LCD) || defined(CONFIG_DM_VIDEO)
 	ret = efi_gop_register();
 	if (ret != EFI_SUCCESS)
--
2.25.1

[PATCH 09/16] efi_loader: imply FAT, FAT_WRITE

[Heinrich Schuchardt]

The UEFI spec requires support for the FAT file system.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/Kconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index 9890144d41..e10ca05549 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -15,6 +15,8 @@ config EFI_LOADER
 	select HAVE_BLOCK_DEVICE
 	select REGEX
 	imply CFB_CONSOLE_ANSI
+	imply FAT
+	imply FAT_WRITE
 	imply USB_KEYBOARD_FN_KEYS
 	imply VIDEO_ANSI
 	help
--
2.25.1

[PATCH 09/16] efi_loader: imply FAT, FAT_WRITE

[AKASHI Takahiro]

On Fri, Mar 27, 2020 at 06:27:53AM +0100, Heinrich Schuchardt wrote:
> The UEFI spec requires support for the FAT file system.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_loader/Kconfig | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index 9890144d41..e10ca05549 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -15,6 +15,8 @@ config EFI_LOADER
>  	select HAVE_BLOCK_DEVICE
>  	select REGEX
>  	imply CFB_CONSOLE_ANSI
> +	imply FAT
> +	imply FAT_WRITE

Obviously, this *imply* doesn't enforce enabling FAT.
If it is absolutely necessary, another measure should be taken.

In addition, why do you treat FAT specifically here?
I remember that you insisted that other file system should be
allowed on U-Boot when I posted some patch.

-Takahiro Akashi


>  	imply USB_KEYBOARD_FN_KEYS
>  	imply VIDEO_ANSI
>  	help
> --
> 2.25.1
> 

[PATCH 09/16] efi_loader: imply FAT, FAT_WRITE

[Heinrich Schuchardt]

On  March 31, 2020, 5:28 a.m. UTC Takahiro Akashi wrote:
 > On Fri, Mar 27, 2020 at 06:27:53AM +0100, Heinrich Schuchardt wrote:
 > > The UEFI spec requires support for the FAT file system.
 > >
 > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
 > > ---
 > >  lib/efi_loader/Kconfig | 2 ++
 > >  1 file changed, 2 insertions(+)
 > >
 > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
 > > index 9890144d41..e10ca05549 100644
 > > --- a/lib/efi_loader/Kconfig
 > > +++ b/lib/efi_loader/Kconfig
 > > @@ -15,6 +15,8 @@ config EFI_LOADER
 > >  	select HAVE_BLOCK_DEVICE
 > >  	select REGEX
 > >  	imply CFB_CONSOLE_ANSI
 > > +	imply FAT
 > > +	imply FAT_WRITE
 >
 > Obviously, this *imply* doesn't enforce enabling FAT.
 > If it is absolutely necessary, another measure should be taken.

If somebody wants to minimize the U-Boot size it might be necessary to
do without FAT_WRITE or FAT support.

 >
 > In addition, why do you treat FAT specifically here?
 > I remember that you insisted that other file system should be
 > allowed on U-Boot when I posted some patch.

An EFI system partition is always FAT formatted. So if we want to safe
U-Boot variables to the EFI system partition we require FAT.

Best regards

Heinrich

 >
 > -Takahiro Akashi
 >
 >
 > >  	imply USB_KEYBOARD_FN_KEYS
 > >  	imply VIDEO_ANSI
 > >  	help
 > > --
 > > 2.25.1

[PATCH 09/16] efi_loader: imply FAT, FAT_WRITE

[AKASHI Takahiro]

On Tue, Mar 31, 2020 at 08:44:02AM +0200, Heinrich Schuchardt wrote:
> On  March 31, 2020, 5:28 a.m. UTC Takahiro Akashi wrote:
> > On Fri, Mar 27, 2020 at 06:27:53AM +0100, Heinrich Schuchardt wrote:
> > > The UEFI spec requires support for the FAT file system.
> > >
> > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > ---
> > >  lib/efi_loader/Kconfig | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > index 9890144d41..e10ca05549 100644
> > > --- a/lib/efi_loader/Kconfig
> > > +++ b/lib/efi_loader/Kconfig
> > > @@ -15,6 +15,8 @@ config EFI_LOADER
> > >  	select HAVE_BLOCK_DEVICE
> > >  	select REGEX
> > >  	imply CFB_CONSOLE_ANSI
> > > +	imply FAT
> > > +	imply FAT_WRITE
> >
> > Obviously, this *imply* doesn't enforce enabling FAT.
> > If it is absolutely necessary, another measure should be taken.
> 
> If somebody wants to minimize the U-Boot size it might be necessary to
> do without FAT_WRITE or FAT support.

If so, Get/SetVariable won't be supported even in boot time
with your patch applied. It is not practical for almost all users.

> >
> > In addition, why do you treat FAT specifically here?
> > I remember that you insisted that other file system should be
> > allowed on U-Boot when I posted some patch.
> 
> An EFI system partition is always FAT formatted. So if we want to safe
> U-Boot variables to the EFI system partition we require FAT.

As system partition is required to be in FAT, file system used on
other partitions must also be in FAT since, as I said before,
UEFI specification clearly defines its file system format based on FAT.
See section 13.3.

So,

> > I remember that you insisted that other file system should be
> > allowed on U-Boot when I posted some patch.

You reverted your statement above here.
That is my point.

-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> >
> > -Takahiro Akashi
> >
> >
> > >  	imply USB_KEYBOARD_FN_KEYS
> > >  	imply VIDEO_ANSI
> > >  	help
> > > --
> > > 2.25.1

[PATCH 09/16] efi_loader: imply FAT, FAT_WRITE

[Mark Kettenis]

> Date: Tue, 31 Mar 2020 16:44:34 +0900
> From: AKASHI Takahiro <takahiro.akashi@linaro.org>
> 
> On Tue, Mar 31, 2020 at 08:44:02AM +0200, Heinrich Schuchardt wrote:
> > On  March 31, 2020, 5:28 a.m. UTC Takahiro Akashi wrote:
> > > On Fri, Mar 27, 2020 at 06:27:53AM +0100, Heinrich Schuchardt wrote:
> > > > The UEFI spec requires support for the FAT file system.
> > > >
> > > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > > ---
> > > >  lib/efi_loader/Kconfig | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > > index 9890144d41..e10ca05549 100644
> > > > --- a/lib/efi_loader/Kconfig
> > > > +++ b/lib/efi_loader/Kconfig
> > > > @@ -15,6 +15,8 @@ config EFI_LOADER
> > > >  	select HAVE_BLOCK_DEVICE
> > > >  	select REGEX
> > > >  	imply CFB_CONSOLE_ANSI
> > > > +	imply FAT
> > > > +	imply FAT_WRITE
> > >
> > > Obviously, this *imply* doesn't enforce enabling FAT.
> > > If it is absolutely necessary, another measure should be taken.
> > 
> > If somebody wants to minimize the U-Boot size it might be necessary to
> > do without FAT_WRITE or FAT support.
> 
> If so, Get/SetVariable won't be supported even in boot time
> with your patch applied. It is not practical for almost all users.

I *strongly* disagree with that statement.  Most users don't care
about U-Boot providing a full EFI implementation.  They just want to
boot their OS.  The basic EFI support in U-Boot is good enough for
that and for OpenBSD and some Linux distros on arm/arm64 this is the
only bootpath that works.  If adding more code leads to board
maintainers disabling EFI support this isn't helpful.

[PATCH 09/16] efi_loader: imply FAT, FAT_WRITE

[AKASHI Takahiro]

On Tue, Mar 31, 2020 at 10:20:17AM +0200, Mark Kettenis wrote:
> > Date: Tue, 31 Mar 2020 16:44:34 +0900
> > From: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > 
> > On Tue, Mar 31, 2020 at 08:44:02AM +0200, Heinrich Schuchardt wrote:
> > > On  March 31, 2020, 5:28 a.m. UTC Takahiro Akashi wrote:
> > > > On Fri, Mar 27, 2020 at 06:27:53AM +0100, Heinrich Schuchardt wrote:
> > > > > The UEFI spec requires support for the FAT file system.
> > > > >
> > > > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > > > ---
> > > > >  lib/efi_loader/Kconfig | 2 ++
> > > > >  1 file changed, 2 insertions(+)
> > > > >
> > > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > > > index 9890144d41..e10ca05549 100644
> > > > > --- a/lib/efi_loader/Kconfig
> > > > > +++ b/lib/efi_loader/Kconfig
> > > > > @@ -15,6 +15,8 @@ config EFI_LOADER
> > > > >  	select HAVE_BLOCK_DEVICE
> > > > >  	select REGEX
> > > > >  	imply CFB_CONSOLE_ANSI
> > > > > +	imply FAT
> > > > > +	imply FAT_WRITE
> > > >
> > > > Obviously, this *imply* doesn't enforce enabling FAT.
> > > > If it is absolutely necessary, another measure should be taken.
> > > 
> > > If somebody wants to minimize the U-Boot size it might be necessary to
> > > do without FAT_WRITE or FAT support.
> > 
> > If so, Get/SetVariable won't be supported even in boot time
> > with your patch applied. It is not practical for almost all users.
> 
> I *strongly* disagree with that statement.  Most users don't care
> about U-Boot providing a full EFI implementation.  They just want to
> boot their OS.  The basic EFI support in U-Boot is good enough for
> that and for OpenBSD and some Linux distros on arm/arm64 this is the
> only bootpath that works.  If adding more code leads to board
> maintainers disabling EFI support this isn't helpful.

Okay, so can you please describe the minimum set of functionality
for you? Without that, the discussion will not be fair.

Anyhow, using "imply" to customize UEFI functionality would be
poor. Instead, we should introduce explicit configuration, say,

config EFI_CUSTOMIZE_RUNTIME_SERVICES

if EFI_CUSTOMIZE_RUNTIME_SERVICES

config EFI_RUNTIME_GET_VARIABLE
   select FAT

config EFI_RUNTIME_SET_VARIABLE
   depends on EFI_RUNTIME_GET_VARIABLE
   select FAT_WRITE

...

I proposed similar idea before, but it was not accepted.
It is time to do that.

Thanks,
-Takahiro Akashi

[PATCH 09/16] efi_loader: imply FAT, FAT_WRITE

[Heinrich Schuchardt]

On 4/1/20 2:31 AM, AKASHI Takahiro wrote:
> On Tue, Mar 31, 2020 at 10:20:17AM +0200, Mark Kettenis wrote:
>>> Date: Tue, 31 Mar 2020 16:44:34 +0900
>>> From: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>>
>>> On Tue, Mar 31, 2020 at 08:44:02AM +0200, Heinrich Schuchardt wrote:
>>>> On  March 31, 2020, 5:28 a.m. UTC Takahiro Akashi wrote:
>>>>> On Fri, Mar 27, 2020 at 06:27:53AM +0100, Heinrich Schuchardt wrote:
>>>>>> The UEFI spec requires support for the FAT file system.
>>>>>>
>>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>>> ---
>>>>>>  lib/efi_loader/Kconfig | 2 ++
>>>>>>  1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
>>>>>> index 9890144d41..e10ca05549 100644
>>>>>> --- a/lib/efi_loader/Kconfig
>>>>>> +++ b/lib/efi_loader/Kconfig
>>>>>> @@ -15,6 +15,8 @@ config EFI_LOADER
>>>>>>  	select HAVE_BLOCK_DEVICE
>>>>>>  	select REGEX
>>>>>>  	imply CFB_CONSOLE_ANSI
>>>>>> +	imply FAT
>>>>>> +	imply FAT_WRITE
>>>>>
>>>>> Obviously, this *imply* doesn't enforce enabling FAT.
>>>>> If it is absolutely necessary, another measure should be taken.
>>>>
>>>> If somebody wants to minimize the U-Boot size it might be necessary to
>>>> do without FAT_WRITE or FAT support.
>>>
>>> If so, Get/SetVariable won't be supported even in boot time
>>> with your patch applied. It is not practical for almost all users.
>>
>> I *strongly* disagree with that statement.  Most users don't care
>> about U-Boot providing a full EFI implementation.  They just want to
>> boot their OS.  The basic EFI support in U-Boot is good enough for
>> that and for OpenBSD and some Linux distros on arm/arm64 this is the
>> only bootpath that works.  If adding more code leads to board
>> maintainers disabling EFI support this isn't helpful.
>
> Okay, so can you please describe the minimum set of functionality
> for you? Without that, the discussion will not be fair.
>
> Anyhow, using "imply" to customize UEFI functionality would be
> poor. Instead, we should introduce explicit configuration, say,
>
> config EFI_CUSTOMIZE_RUNTIME_SERVICES
>
> if EFI_CUSTOMIZE_RUNTIME_SERVICES
>
> config EFI_RUNTIME_GET_VARIABLE
>    select FAT

Why should exposing the GetVariable() service depend on FAT?

At least for GetVariable() I see no real benefit of not exposing it always.

>
> config EFI_RUNTIME_SET_VARIABLE
>    depends on EFI_RUNTIME_GET_VARIABLE
>    select FAT_WRITE

We cannot use writing to FAT at runtime. So this dependency seems incorrect.

>
> ...
>
> I proposed similar idea before, but it was not accepted.
> It is time to do that.

Unfortunately your patches for adjusting the storage of environments
never made it into master.

Best regards

Heinrich

>
> Thanks,
> -Takahiro Akashi
>

[PATCH 09/16] efi_loader: imply FAT, FAT_WRITE

[Mark Kettenis]

> Date: Wed, 1 Apr 2020 09:31:03 +0900
> From: AKASHI Takahiro <takahiro.akashi@linaro.org>
> 
> On Tue, Mar 31, 2020 at 10:20:17AM +0200, Mark Kettenis wrote:
> > > Date: Tue, 31 Mar 2020 16:44:34 +0900
> > > From: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > 
> > > On Tue, Mar 31, 2020 at 08:44:02AM +0200, Heinrich Schuchardt wrote:
> > > > On  March 31, 2020, 5:28 a.m. UTC Takahiro Akashi wrote:
> > > > > On Fri, Mar 27, 2020 at 06:27:53AM +0100, Heinrich Schuchardt wrote:
> > > > > > The UEFI spec requires support for the FAT file system.
> > > > > >
> > > > > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > > > > ---
> > > > > >  lib/efi_loader/Kconfig | 2 ++
> > > > > >  1 file changed, 2 insertions(+)
> > > > > >
> > > > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > > > > index 9890144d41..e10ca05549 100644
> > > > > > --- a/lib/efi_loader/Kconfig
> > > > > > +++ b/lib/efi_loader/Kconfig
> > > > > > @@ -15,6 +15,8 @@ config EFI_LOADER
> > > > > >  	select HAVE_BLOCK_DEVICE
> > > > > >  	select REGEX
> > > > > >  	imply CFB_CONSOLE_ANSI
> > > > > > +	imply FAT
> > > > > > +	imply FAT_WRITE
> > > > >
> > > > > Obviously, this *imply* doesn't enforce enabling FAT.
> > > > > If it is absolutely necessary, another measure should be taken.
> > > > 
> > > > If somebody wants to minimize the U-Boot size it might be necessary to
> > > > do without FAT_WRITE or FAT support.
> > > 
> > > If so, Get/SetVariable won't be supported even in boot time
> > > with your patch applied. It is not practical for almost all users.
> > 
> > I *strongly* disagree with that statement.  Most users don't care
> > about U-Boot providing a full EFI implementation.  They just want to
> > boot their OS.  The basic EFI support in U-Boot is good enough for
> > that and for OpenBSD and some Linux distros on arm/arm64 this is the
> > only bootpath that works.  If adding more code leads to board
> > maintainers disabling EFI support this isn't helpful.
> 
> Okay, so can you please describe the minimum set of functionality
> for you? Without that, the discussion will not be fair.

For OpenBSD/arm and OpenBSD/amr64 we adopted the UEFI interfaces in
u-boot early on, even before most Linux distros did.  As a result our
requirements are very minimal:

Our bootloader uses the folowing boot services:

- HandleProtocol()
- AllocatePages()
- FreePages()
- LocateHandle()
- LocateHandleBuffer()
- LocateProtocol()
- CreateEvent()
- SetTimer()
- WaitForEvent()
- CloseEvent()
- GetMemoryMap()
- SetWatchdogTimer()
- Exit()

runtime services:

- ResetSystem()

and protocols:

- EFI_LOADED_IMAGE_PROTOCOL
- EFI_DEVICE_PATH_PROTOCOL
- EFI_BLOCK_IO_PROTOCOL
- EFI_GRAPHICS_OUTPUT_PROTOCOL
- EFI_RNG_PROTOCOL
- EFI_SIMPLE_NETWORK_PROTOCOL
- EFI_PXE_BASE_CODE_PROTOCOL

Obviously the last four are only used if there is actual hardware to
support these protocols.  But I'd consider them non-optional if such
hardware exists.

Our kernel only uses a couple if runtime services:

- SetVirtualAddressMap()
- GetTime() (optional)
- SetTime() (optional)

Cheers,

Mark

[PATCH 09/16] efi_loader: imply FAT, FAT_WRITE

[AKASHI Takahiro]

Hi Mark,

On Wed, Apr 01, 2020 at 07:56:31PM +0200, Mark Kettenis wrote:
> > Date: Wed, 1 Apr 2020 09:31:03 +0900
> > From: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > 
> > On Tue, Mar 31, 2020 at 10:20:17AM +0200, Mark Kettenis wrote:
> > > > Date: Tue, 31 Mar 2020 16:44:34 +0900
> > > > From: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > 
> > > > On Tue, Mar 31, 2020 at 08:44:02AM +0200, Heinrich Schuchardt wrote:
> > > > > On  March 31, 2020, 5:28 a.m. UTC Takahiro Akashi wrote:
> > > > > > On Fri, Mar 27, 2020 at 06:27:53AM +0100, Heinrich Schuchardt wrote:
> > > > > > > The UEFI spec requires support for the FAT file system.
> > > > > > >
> > > > > > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > > > > > ---
> > > > > > >  lib/efi_loader/Kconfig | 2 ++
> > > > > > >  1 file changed, 2 insertions(+)
> > > > > > >
> > > > > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > > > > > index 9890144d41..e10ca05549 100644
> > > > > > > --- a/lib/efi_loader/Kconfig
> > > > > > > +++ b/lib/efi_loader/Kconfig
> > > > > > > @@ -15,6 +15,8 @@ config EFI_LOADER
> > > > > > >  	select HAVE_BLOCK_DEVICE
> > > > > > >  	select REGEX
> > > > > > >  	imply CFB_CONSOLE_ANSI
> > > > > > > +	imply FAT
> > > > > > > +	imply FAT_WRITE
> > > > > >
> > > > > > Obviously, this *imply* doesn't enforce enabling FAT.
> > > > > > If it is absolutely necessary, another measure should be taken.
> > > > > 
> > > > > If somebody wants to minimize the U-Boot size it might be necessary to
> > > > > do without FAT_WRITE or FAT support.
> > > > 
> > > > If so, Get/SetVariable won't be supported even in boot time
> > > > with your patch applied. It is not practical for almost all users.
> > > 
> > > I *strongly* disagree with that statement.  Most users don't care
> > > about U-Boot providing a full EFI implementation.  They just want to
> > > boot their OS.  The basic EFI support in U-Boot is good enough for
> > > that and for OpenBSD and some Linux distros on arm/arm64 this is the
> > > only bootpath that works.  If adding more code leads to board
> > > maintainers disabling EFI support this isn't helpful.
> > 
> > Okay, so can you please describe the minimum set of functionality
> > for you? Without that, the discussion will not be fair.
> 
> For OpenBSD/arm and OpenBSD/amr64 we adopted the UEFI interfaces in
> u-boot early on, even before most Linux distros did.  As a result our
> requirements are very minimal:

Thank you for your quick feedback.
As I'm not familiar with OpenBSD things, this information gives me
much better view/understandings.

Just from my curiosity, let me ask some questions.

> Our bootloader uses the folowing boot services:

You don't reply on any features from "EFI Boot Manager,"
including BootXXXX variables, do you?

> - HandleProtocol()
> - AllocatePages()
> - FreePages()
> - LocateHandle()
> - LocateHandleBuffer()
> - LocateProtocol()
> - CreateEvent()
> - SetTimer()
> - WaitForEvent()
> - CloseEvent()
> - GetMemoryMap()
> - SetWatchdogTimer()
> - Exit()
> 
> runtime services:

+ load_image/start_image, obviously?

> - ResetSystem()
> 
> and protocols:
> 
> - EFI_LOADED_IMAGE_PROTOCOL
> - EFI_DEVICE_PATH_PROTOCOL
> - EFI_BLOCK_IO_PROTOCOL
> - EFI_GRAPHICS_OUTPUT_PROTOCOL
> - EFI_RNG_PROTOCOL
> - EFI_SIMPLE_NETWORK_PROTOCOL
> - EFI_PXE_BASE_CODE_PROTOCOL

What about EFI_FILE_PROTOCOL and/or EFI_FILE_LOAD_PROTOCOL?

Don't you have the assumption that there must be any EFI system
partition (and file system) on your boards?

-Takahiro Akashi

> Obviously the last four are only used if there is actual hardware to
> support these protocols.  But I'd consider them non-optional if such
> hardware exists.
> 
> Our kernel only uses a couple if runtime services:
> 
> - SetVirtualAddressMap()
> - GetTime() (optional)
> - SetTime() (optional)

As you might know, I'm working on UEFI secure boot for U-Boot.
Does OpenBSD support any kind of secure boot?
If so, is it based on UEFI feature (in that case, UEFI variables are
mandatory) or do you have your own solution?

Thanks,
-Takahiro Akashi


> Cheers,
> 
> Mark

[PATCH 09/16] efi_loader: imply FAT, FAT_WRITE

[Heinrich Schuchardt]

On 2020-03-31 09:44, AKASHI Takahiro wrote:
> On Tue, Mar 31, 2020 at 08:44:02AM +0200, Heinrich Schuchardt wrote:
>> On  March 31, 2020, 5:28 a.m. UTC Takahiro Akashi wrote:
>>> On Fri, Mar 27, 2020 at 06:27:53AM +0100, Heinrich Schuchardt wrote:
>>>> The UEFI spec requires support for the FAT file system.
>>>>
>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>> ---
>>>>  lib/efi_loader/Kconfig | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
>>>> index 9890144d41..e10ca05549 100644
>>>> --- a/lib/efi_loader/Kconfig
>>>> +++ b/lib/efi_loader/Kconfig
>>>> @@ -15,6 +15,8 @@ config EFI_LOADER
>>>>  	select HAVE_BLOCK_DEVICE
>>>>  	select REGEX
>>>>  	imply CFB_CONSOLE_ANSI
>>>> +	imply FAT
>>>> +	imply FAT_WRITE
>>>
>>> Obviously, this *imply* doesn't enforce enabling FAT.
>>> If it is absolutely necessary, another measure should be taken.
>>
>> If somebody wants to minimize the U-Boot size it might be necessary to
>> do without FAT_WRITE or FAT support.
>
> If so, Get/SetVariable won't be supported even in boot time
> with your patch applied. It is not practical for almost all users.

Hello Akashi,

without FAT_WRITE we will not have persistence for variables.
SetVariable and GetVariable are still usable.

Best regards

Heinrich

>
>>>
>>> In addition, why do you treat FAT specifically here?
>>> I remember that you insisted that other file system should be
>>> allowed on U-Boot when I posted some patch.
>>
>> An EFI system partition is always FAT formatted. So if we want to safe
>> U-Boot variables to the EFI system partition we require FAT.
>
> As system partition is required to be in FAT, file system used on
> other partitions must also be in FAT since, as I said before,
> UEFI specification clearly defines its file system format based on FAT.
> See section 13.3.
>
> So,
>
>>> I remember that you insisted that other file system should be
>>> allowed on U-Boot when I posted some patch.
>
> You reverted your statement above here.
> That is my point.
>
> -Takahiro Akashi
>
>> Best regards
>>
>> Heinrich
>>
>>>
>>> -Takahiro Akashi
>>>
>>>
>>>>  	imply USB_KEYBOARD_FN_KEYS
>>>>  	imply VIDEO_ANSI
>>>>  	help
>>>> --
>>>> 2.25.1

[PATCH 09/16] efi_loader: imply FAT, FAT_WRITE

[AKASHI Takahiro]

On Tue, Mar 31, 2020 at 03:08:06PM +0200, Heinrich Schuchardt wrote:
> On 2020-03-31 09:44, AKASHI Takahiro wrote:
> > On Tue, Mar 31, 2020 at 08:44:02AM +0200, Heinrich Schuchardt wrote:
> >> On  March 31, 2020, 5:28 a.m. UTC Takahiro Akashi wrote:
> >>> On Fri, Mar 27, 2020 at 06:27:53AM +0100, Heinrich Schuchardt wrote:
> >>>> The UEFI spec requires support for the FAT file system.
> >>>>
> >>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>>> ---
> >>>>  lib/efi_loader/Kconfig | 2 ++
> >>>>  1 file changed, 2 insertions(+)
> >>>>
> >>>> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> >>>> index 9890144d41..e10ca05549 100644
> >>>> --- a/lib/efi_loader/Kconfig
> >>>> +++ b/lib/efi_loader/Kconfig
> >>>> @@ -15,6 +15,8 @@ config EFI_LOADER
> >>>>  	select HAVE_BLOCK_DEVICE
> >>>>  	select REGEX
> >>>>  	imply CFB_CONSOLE_ANSI
> >>>> +	imply FAT
> >>>> +	imply FAT_WRITE
> >>>
> >>> Obviously, this *imply* doesn't enforce enabling FAT.
> >>> If it is absolutely necessary, another measure should be taken.
> >>
> >> If somebody wants to minimize the U-Boot size it might be necessary to
> >> do without FAT_WRITE or FAT support.
> >
> > If so, Get/SetVariable won't be supported even in boot time
> > with your patch applied. It is not practical for almost all users.
> 
> Hello Akashi,
> 
> without FAT_WRITE we will not have persistence for variables.
> SetVariable and GetVariable are still usable.

How about CONFIG_FAT?

-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> >
> >>>
> >>> In addition, why do you treat FAT specifically here?
> >>> I remember that you insisted that other file system should be
> >>> allowed on U-Boot when I posted some patch.
> >>
> >> An EFI system partition is always FAT formatted. So if we want to safe
> >> U-Boot variables to the EFI system partition we require FAT.
> >
> > As system partition is required to be in FAT, file system used on
> > other partitions must also be in FAT since, as I said before,
> > UEFI specification clearly defines its file system format based on FAT.
> > See section 13.3.
> >
> > So,
> >
> >>> I remember that you insisted that other file system should be
> >>> allowed on U-Boot when I posted some patch.
> >
> > You reverted your statement above here.
> > That is my point.
> >
> > -Takahiro Akashi
> >
> >> Best regards
> >>
> >> Heinrich
> >>
> >>>
> >>> -Takahiro Akashi
> >>>
> >>>
> >>>>  	imply USB_KEYBOARD_FN_KEYS
> >>>>  	imply VIDEO_ANSI
> >>>>  	help
> >>>> --
> >>>> 2.25.1
> 

[PATCH 09/16] efi_loader: imply FAT, FAT_WRITE

[AKASHI Takahiro]

On Wed, Apr 01, 2020 at 08:57:33AM +0900, AKASHI Takahiro wrote:
> On Tue, Mar 31, 2020 at 03:08:06PM +0200, Heinrich Schuchardt wrote:
> > On 2020-03-31 09:44, AKASHI Takahiro wrote:
> > > On Tue, Mar 31, 2020 at 08:44:02AM +0200, Heinrich Schuchardt wrote:
> > >> On  March 31, 2020, 5:28 a.m. UTC Takahiro Akashi wrote:
> > >>> On Fri, Mar 27, 2020 at 06:27:53AM +0100, Heinrich Schuchardt wrote:
> > >>>> The UEFI spec requires support for the FAT file system.
> > >>>>
> > >>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > >>>> ---
> > >>>>  lib/efi_loader/Kconfig | 2 ++
> > >>>>  1 file changed, 2 insertions(+)
> > >>>>
> > >>>> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > >>>> index 9890144d41..e10ca05549 100644
> > >>>> --- a/lib/efi_loader/Kconfig
> > >>>> +++ b/lib/efi_loader/Kconfig
> > >>>> @@ -15,6 +15,8 @@ config EFI_LOADER
> > >>>>  	select HAVE_BLOCK_DEVICE
> > >>>>  	select REGEX
> > >>>>  	imply CFB_CONSOLE_ANSI
> > >>>> +	imply FAT
> > >>>> +	imply FAT_WRITE
> > >>>
> > >>> Obviously, this *imply* doesn't enforce enabling FAT.
> > >>> If it is absolutely necessary, another measure should be taken.
> > >>
> > >> If somebody wants to minimize the U-Boot size it might be necessary to
> > >> do without FAT_WRITE or FAT support.
> > >
> > > If so, Get/SetVariable won't be supported even in boot time
> > > with your patch applied. It is not practical for almost all users.
> > 
> > Hello Akashi,
> > 
> > without FAT_WRITE we will not have persistence for variables.
> > SetVariable and GetVariable are still usable.
> 
> How about CONFIG_FAT?

(=> What if !CONFIG_FAT)


More fundamentally,
Why do you want to use a file as storage device for variables?
why not raw partition (or just part of partition) on, say,
NOR or eMMC?

As you know, EDK2 saves variables directly on NOR (or block device?
probably).

-Takahiro Akashi

> -Takahiro Akashi
> 
> > Best regards
> > 
> > Heinrich
> > 
> > >
> > >>>
> > >>> In addition, why do you treat FAT specifically here?
> > >>> I remember that you insisted that other file system should be
> > >>> allowed on U-Boot when I posted some patch.
> > >>
> > >> An EFI system partition is always FAT formatted. So if we want to safe
> > >> U-Boot variables to the EFI system partition we require FAT.
> > >
> > > As system partition is required to be in FAT, file system used on
> > > other partitions must also be in FAT since, as I said before,
> > > UEFI specification clearly defines its file system format based on FAT.
> > > See section 13.3.
> > >
> > > So,
> > >
> > >>> I remember that you insisted that other file system should be
> > >>> allowed on U-Boot when I posted some patch.
> > >
> > > You reverted your statement above here.
> > > That is my point.
> > >
> > > -Takahiro Akashi
> > >
> > >> Best regards
> > >>
> > >> Heinrich
> > >>
> > >>>
> > >>> -Takahiro Akashi
> > >>>
> > >>>
> > >>>>  	imply USB_KEYBOARD_FN_KEYS
> > >>>>  	imply VIDEO_ANSI
> > >>>>  	help
> > >>>> --
> > >>>> 2.25.1
> > 

[PATCH 09/16] efi_loader: imply FAT, FAT_WRITE

[Heinrich Schuchardt]

On 4/1/20 3:14 AM, AKASHI Takahiro wrote:
> On Wed, Apr 01, 2020 at 08:57:33AM +0900, AKASHI Takahiro wrote:
>> On Tue, Mar 31, 2020 at 03:08:06PM +0200, Heinrich Schuchardt wrote:
>>> On 2020-03-31 09:44, AKASHI Takahiro wrote:
>>>> On Tue, Mar 31, 2020 at 08:44:02AM +0200, Heinrich Schuchardt wrote:
>>>>> On  March 31, 2020, 5:28 a.m. UTC Takahiro Akashi wrote:
>>>>>> On Fri, Mar 27, 2020 at 06:27:53AM +0100, Heinrich Schuchardt wrote:
>>>>>>> The UEFI spec requires support for the FAT file system.
>>>>>>>
>>>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>>>> ---
>>>>>>>  lib/efi_loader/Kconfig | 2 ++
>>>>>>>  1 file changed, 2 insertions(+)
>>>>>>>
>>>>>>> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
>>>>>>> index 9890144d41..e10ca05549 100644
>>>>>>> --- a/lib/efi_loader/Kconfig
>>>>>>> +++ b/lib/efi_loader/Kconfig
>>>>>>> @@ -15,6 +15,8 @@ config EFI_LOADER
>>>>>>>  	select HAVE_BLOCK_DEVICE
>>>>>>>  	select REGEX
>>>>>>>  	imply CFB_CONSOLE_ANSI
>>>>>>> +	imply FAT
>>>>>>> +	imply FAT_WRITE
>>>>>>
>>>>>> Obviously, this *imply* doesn't enforce enabling FAT.
>>>>>> If it is absolutely necessary, another measure should be taken.
>>>>>
>>>>> If somebody wants to minimize the U-Boot size it might be necessary to
>>>>> do without FAT_WRITE or FAT support.
>>>>
>>>> If so, Get/SetVariable won't be supported even in boot time
>>>> with your patch applied. It is not practical for almost all users.
>>>
>>> Hello Akashi,
>>>
>>> without FAT_WRITE we will not have persistence for variables.
>>> SetVariable and GetVariable are still usable.
>>
>> How about CONFIG_FAT?
>
> (=> What if !CONFIG_FAT)
>
>
> More fundamentally,
> Why do you want to use a file as storage device for variables?
> why not raw partition (or just part of partition) on, say,
> NOR or eMMC?
>
> As you know, EDK2 saves variables directly on NOR (or block device?
> probably).

Yes, we may add further stores later on. Ilias wants to use the RPMB
area of eMMC devices. As an EFI system partition exists on any UEFI
compatible device I think implementing this first is a valid approach.

Best regards

Heinrich

>
> -Takahiro Akashi
>
>> -Takahiro Akashi
>>
>>> Best regards
>>>
>>> Heinrich
>>>
>>>>
>>>>>>
>>>>>> In addition, why do you treat FAT specifically here?
>>>>>> I remember that you insisted that other file system should be
>>>>>> allowed on U-Boot when I posted some patch.
>>>>>
>>>>> An EFI system partition is always FAT formatted. So if we want to safe
>>>>> U-Boot variables to the EFI system partition we require FAT.
>>>>
>>>> As system partition is required to be in FAT, file system used on
>>>> other partitions must also be in FAT since, as I said before,
>>>> UEFI specification clearly defines its file system format based on FAT.
>>>> See section 13.3.
>>>>
>>>> So,
>>>>
>>>>>> I remember that you insisted that other file system should be
>>>>>> allowed on U-Boot when I posted some patch.
>>>>
>>>> You reverted your statement above here.
>>>> That is my point.
>>>>
>>>> -Takahiro Akashi
>>>>
>>>>> Best regards
>>>>>
>>>>> Heinrich
>>>>>
>>>>>>
>>>>>> -Takahiro Akashi
>>>>>>
>>>>>>
>>>>>>>  	imply USB_KEYBOARD_FN_KEYS
>>>>>>>  	imply VIDEO_ANSI
>>>>>>>  	help
>>>>>>> --
>>>>>>> 2.25.1
>>>

[PATCH 09/16] efi_loader: imply FAT, FAT_WRITE

[AKASHI Takahiro]

On Wed, Apr 01, 2020 at 08:31:10AM +0200, Heinrich Schuchardt wrote:
> On 4/1/20 3:14 AM, AKASHI Takahiro wrote:
> > On Wed, Apr 01, 2020 at 08:57:33AM +0900, AKASHI Takahiro wrote:
> >> On Tue, Mar 31, 2020 at 03:08:06PM +0200, Heinrich Schuchardt wrote:
> >>> On 2020-03-31 09:44, AKASHI Takahiro wrote:
> >>>> On Tue, Mar 31, 2020 at 08:44:02AM +0200, Heinrich Schuchardt wrote:
> >>>>> On  March 31, 2020, 5:28 a.m. UTC Takahiro Akashi wrote:
> >>>>>> On Fri, Mar 27, 2020 at 06:27:53AM +0100, Heinrich Schuchardt wrote:
> >>>>>>> The UEFI spec requires support for the FAT file system.
> >>>>>>>
> >>>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>>>>>> ---
> >>>>>>>  lib/efi_loader/Kconfig | 2 ++
> >>>>>>>  1 file changed, 2 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> >>>>>>> index 9890144d41..e10ca05549 100644
> >>>>>>> --- a/lib/efi_loader/Kconfig
> >>>>>>> +++ b/lib/efi_loader/Kconfig
> >>>>>>> @@ -15,6 +15,8 @@ config EFI_LOADER
> >>>>>>>  	select HAVE_BLOCK_DEVICE
> >>>>>>>  	select REGEX
> >>>>>>>  	imply CFB_CONSOLE_ANSI
> >>>>>>> +	imply FAT
> >>>>>>> +	imply FAT_WRITE
> >>>>>>
> >>>>>> Obviously, this *imply* doesn't enforce enabling FAT.
> >>>>>> If it is absolutely necessary, another measure should be taken.
> >>>>>
> >>>>> If somebody wants to minimize the U-Boot size it might be necessary to
> >>>>> do without FAT_WRITE or FAT support.
> >>>>
> >>>> If so, Get/SetVariable won't be supported even in boot time
> >>>> with your patch applied. It is not practical for almost all users.
> >>>
> >>> Hello Akashi,
> >>>
> >>> without FAT_WRITE we will not have persistence for variables.
> >>> SetVariable and GetVariable are still usable.
> >>
> >> How about CONFIG_FAT?
> >
> > (=> What if !CONFIG_FAT)

?

> >
> >
> > More fundamentally,
> > Why do you want to use a file as storage device for variables?
> > why not raw partition (or just part of partition) on, say,
> > NOR or eMMC?
> >
> > As you know, EDK2 saves variables directly on NOR (or block device?
> > probably).
> 
> Yes, we may add further stores later on.

Why not now?
How will you generalize it?

> Ilias wants to use the RPMB
> area of eMMC devices.

I think that he plans to use RPMB as secure storage for Standalone MM.

> As an EFI system partition exists on any UEFI
> compatible device I think implementing this first is a valid approach.

I don't deny your approach here, but
I expect that more generic framework for back storage be
implemented and FAT driver be on top of that.

-Takahiro Akashi

> 
> Best regards
> 
> Heinrich
> 
> >
> > -Takahiro Akashi
> >
> >> -Takahiro Akashi
> >>
> >>> Best regards
> >>>
> >>> Heinrich
> >>>
> >>>>
> >>>>>>
> >>>>>> In addition, why do you treat FAT specifically here?
> >>>>>> I remember that you insisted that other file system should be
> >>>>>> allowed on U-Boot when I posted some patch.
> >>>>>
> >>>>> An EFI system partition is always FAT formatted. So if we want to safe
> >>>>> U-Boot variables to the EFI system partition we require FAT.
> >>>>
> >>>> As system partition is required to be in FAT, file system used on
> >>>> other partitions must also be in FAT since, as I said before,
> >>>> UEFI specification clearly defines its file system format based on FAT.
> >>>> See section 13.3.
> >>>>
> >>>> So,
> >>>>
> >>>>>> I remember that you insisted that other file system should be
> >>>>>> allowed on U-Boot when I posted some patch.
> >>>>
> >>>> You reverted your statement above here.
> >>>> That is my point.
> >>>>
> >>>> -Takahiro Akashi
> >>>>
> >>>>> Best regards
> >>>>>
> >>>>> Heinrich
> >>>>>
> >>>>>>
> >>>>>> -Takahiro Akashi
> >>>>>>
> >>>>>>
> >>>>>>>  	imply USB_KEYBOARD_FN_KEYS
> >>>>>>>  	imply VIDEO_ANSI
> >>>>>>>  	help
> >>>>>>> --
> >>>>>>> 2.25.1
> >>>
> 

[PATCH 09/16] efi_loader: imply FAT, FAT_WRITE

[Ilias Apalodimas]

On Wed, Apr 01, 2020 at 04:04:04PM +0900, AKASHI Takahiro wrote:
> > 
[...]
> > Yes, we may add further stores later on.
> 
> Why not now?
> How will you generalize it?
> 
> > Ilias wants to use the RPMB
> > area of eMMC devices.
> 
> I think that he plans to use RPMB as secure storage for Standalone MM.

Sorry for my really delayed replies, but I was over my head with EDK2 and RPMB
indeed :)
So my implementation is orthogonal to this patchset and I do have some code that
write UEFI variables on an RPMB via OP-TEE. 
> 
> > As an EFI system partition exists on any UEFI
> > compatible device I think implementing this first is a valid approach.
> 
> I don't deny your approach here, but
> I expect that more generic framework for back storage be
> implemented and FAT driver be on top of that.
> 
Akashi has a point here. On the other hand can't we just make the store a
callback in the future?
So for example is FAT_FS support is not included, we write the data into raw
flash sectors or a file.
That being said EDK2 uses Fvb and Fault Tolerant Writes to write the actual UEFI
variables. So you end up needing 3 partitions for the actual storage.  The
question here is should we implement something similar to u-boot EFI code since
this is only supposed to serve as a 'fallback'? If the board has OP-TEE then
that will be responsible for writing the variables (and it will use whatever
EDK2 uses).

I'll have a closer look to the patchset and try to give some feedback.

Regards
/Ilias
> -Takahiro Akashi
> 
> > 
> > Best regards
> > 
> > Heinrich
> > 
> > >
> > > -Takahiro Akashi
> > >
> > >> -Takahiro Akashi
> > >>
> > >>> Best regards
> > >>>
> > >>> Heinrich
> > >>>
> > >>>>
> > >>>>>>
> > >>>>>> In addition, why do you treat FAT specifically here?
> > >>>>>> I remember that you insisted that other file system should be
> > >>>>>> allowed on U-Boot when I posted some patch.
> > >>>>>
> > >>>>> An EFI system partition is always FAT formatted. So if we want to safe
> > >>>>> U-Boot variables to the EFI system partition we require FAT.
> > >>>>
> > >>>> As system partition is required to be in FAT, file system used on
> > >>>> other partitions must also be in FAT since, as I said before,
> > >>>> UEFI specification clearly defines its file system format based on FAT.
> > >>>> See section 13.3.
> > >>>>
> > >>>> So,
> > >>>>
> > >>>>>> I remember that you insisted that other file system should be
> > >>>>>> allowed on U-Boot when I posted some patch.
> > >>>>
> > >>>> You reverted your statement above here.
> > >>>> That is my point.
> > >>>>
> > >>>> -Takahiro Akashi
> > >>>>
> > >>>>> Best regards
> > >>>>>
> > >>>>> Heinrich
> > >>>>>
> > >>>>>>
> > >>>>>> -Takahiro Akashi
> > >>>>>>
> > >>>>>>
> > >>>>>>>  	imply USB_KEYBOARD_FN_KEYS
> > >>>>>>>  	imply VIDEO_ANSI
> > >>>>>>>  	help
> > >>>>>>> --
> > >>>>>>> 2.25.1
> > >>>
> > 

[PATCH 10/16] efi_loader: UEFI variable persistence

[Heinrich Schuchardt]

Persist non-volatile UEFI variables in a file on the EFI system partition.

The file is written:

* whenever a non-volatile UEFI variable is changed after initialization
  of the UEFI sub-system.
* upon ExitBootServices()

The file is read during the UEFI sub-system initialization to restore
non-volatile UEFI variables.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 include/efi_variable.h              |  36 +++++
 lib/efi_loader/Kconfig              |   8 +
 lib/efi_loader/Makefile             |   1 +
 lib/efi_loader/efi_variable.c       |  12 +-
 lib/efi_loader/efi_variables_file.c | 235 ++++++++++++++++++++++++++++
 5 files changed, 291 insertions(+), 1 deletion(-)
 create mode 100644 include/efi_variable.h
 create mode 100644 lib/efi_loader/efi_variables_file.c

diff --git a/include/efi_variable.h b/include/efi_variable.h
new file mode 100644
index 0000000000..fb8294fc2e
--- /dev/null
+++ b/include/efi_variable.h
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * File interface for UEFI variables
+ *
+ * Copyright (c) 2020, Heinrich Schuchardt
+ */
+
+#ifndef _EFI_VARIABLE_H
+#define _EFI_VARIABLE_H
+
+#define EFI_VAR_FILE_NAME "ubootefi.var"
+
+#define EFI_VAR_BUF_SIZE 0x4000
+
+#define EFI_VAR_FILE_MAGIC 0x7261566966456255 /* UbEfiVar */
+
+struct efi_var_entry {
+	u32 length;
+	u32 attr;
+	efi_guid_t guid;
+	u16 name[0];
+};
+
+struct efi_var_file {
+	u64 reserved; /* May be overwritten by memory probing */
+	u64 magic;
+	u32 length;
+	u32 crc32;
+	struct efi_var_entry var[0];
+};
+
+efi_status_t efi_var_to_file(void);
+
+efi_status_t efi_var_from_file(void);
+
+#endif
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index e10ca05549..41705fc252 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -27,6 +27,14 @@ config EFI_LOADER

 if EFI_LOADER

+config EFI_VARIABLE_FILE_STORE
+	bool "Store non-volatile UEFI variables as file"
+	depends on FAT_WRITE
+	default y
+	help
+	  Select tis option if you want non-volatile UEFI variables to be stored
+	  as file /ubootefi.var on the EFI system partition.
+
 config EFI_GET_TIME
 	bool "GetTime() runtime service"
 	depends on DM_RTC
diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
index 9b3b704473..621a767ab3 100644
--- a/lib/efi_loader/Makefile
+++ b/lib/efi_loader/Makefile
@@ -35,6 +35,7 @@ obj-y += efi_runtime.o
 obj-y += efi_setup.o
 obj-$(CONFIG_EFI_UNICODE_COLLATION_PROTOCOL2) += efi_unicode_collation.o
 obj-y += efi_variable.o
+obj-y += efi_variables_file.o
 obj-y += efi_watchdog.o
 obj-$(CONFIG_LCD) += efi_gop.o
 obj-$(CONFIG_DM_VIDEO) += efi_gop.o
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
index d99ad6ddae..952a0a0db7 100644
--- a/lib/efi_loader/efi_variable.c
+++ b/lib/efi_loader/efi_variable.c
@@ -7,6 +7,7 @@

 #include <common.h>
 #include <efi_loader.h>
+#include <efi_variable.h>
 #include <env_internal.h>
 #include <hexdump.h>
 #include <malloc.h>
@@ -604,6 +605,11 @@ efi_status_t efi_set_variable_int(u16 *variable_name,
 	if (env_set(native_name, val))
 		ret = EFI_DEVICE_ERROR;

+	/* Write non-volatile EFI variables to file */
+	if (attributes && EFI_VARIABLE_NON_VOLATILE &&
+	    ret == EFI_SUCCESS && efi_obj_list_initialized == EFI_SUCCESS)
+		efi_var_to_file();
+
 out:
 	free(native_name);
 	free(val);
@@ -694,6 +700,10 @@ efi_set_variable_runtime(u16 *variable_name, const efi_guid_t *vendor,
  */
 void efi_variables_boot_exit_notify(void)
 {
+	/* Write non-volatile EFI variables to file */
+	efi_var_to_file();
+
+	/* Switch variable services functions to runtime version */
 	efi_runtime_services.get_variable = efi_get_variable_runtime;
 	efi_runtime_services.get_next_variable_name =
 				efi_get_next_variable_name_runtime;
@@ -708,5 +718,5 @@ void efi_variables_boot_exit_notify(void)
  */
 efi_status_t efi_init_variables(void)
 {
-	return EFI_SUCCESS;
+	return efi_var_from_file();
 }
diff --git a/lib/efi_loader/efi_variables_file.c b/lib/efi_loader/efi_variables_file.c
new file mode 100644
index 0000000000..4a918d3fde
--- /dev/null
+++ b/lib/efi_loader/efi_variables_file.c
@@ -0,0 +1,235 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * File interface for UEFI variables
+ *
+ * Copyright (c) 2020, Heinrich Schuchardt
+ */
+
+#include <common.h>
+#include <charset.h>
+#include <fs.h>
+#include <malloc.h>
+#include <mapmem.h>
+#include <efi_loader.h>
+#include <efi_variable.h>
+#include <u-boot/crc.h>
+
+#define PART_STR_LEN 10
+
+/**
+ * efi_set_blk_dev_to_system_partition() - select EFI system partition
+ *
+ * Set the EFI system partition as current block device.
+ *
+ * Return:	status code
+ */
+static efi_status_t __maybe_unused efi_set_blk_dev_to_system_partition(void)
+{
+	char part_str[PART_STR_LEN];
+	int r;
+
+	if (!efi_system_partition.if_type)
+		return EFI_NOT_FOUND;
+	snprintf(part_str, PART_STR_LEN, "%u:%u",
+		 efi_system_partition.devnum, efi_system_partition.part);
+	r = fs_set_blk_dev(blk_get_if_type_name(efi_system_partition.if_type),
+			   part_str, FS_TYPE_ANY);
+	if (r) {
+		printf("Cannot read EFI system partition\n");
+		return EFI_DEVICE_ERROR;
+	}
+	return EFI_SUCCESS;
+}
+
+/**
+ * efi_var_collect() - collect non-volatile variables in buffer
+ *
+ * A buffer is allocated and filled with all non-volatile variables in a
+ * format ready to be written to disk.
+ *
+ * @bufp:	pointer to pointer of buffer with collected variables
+ * @lenp:	pointer to length of buffer
+ * Return:	status code
+ */
+static efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp,
+						   loff_t *lenp)
+{
+	size_t len = EFI_VAR_BUF_SIZE;
+	struct efi_var_file *buf;
+	struct efi_var_entry *var, *old_var;
+	size_t old_var_name_length = 2;
+
+	*bufp = NULL; /* Avoid double free() */
+	buf = calloc(1, len);
+	if (!buf)
+		return EFI_OUT_OF_RESOURCES;
+	var = buf->var;
+	old_var = var;
+	for (;;) {
+		efi_uintn_t data_length, var_name_length;
+		u8 *data;
+		efi_status_t ret;
+
+		if ((uintptr_t)buf + len <=
+		    (uintptr_t)var->name + old_var_name_length)
+			return EFI_BUFFER_TOO_SMALL;
+
+		var_name_length = (uintptr_t)buf + len - (uintptr_t)var->name;
+		memcpy(var->name, old_var->name, old_var_name_length);
+		guidcpy(&var->guid, &old_var->guid);
+		ret = efi_get_next_variable_name_int(
+				&var_name_length, var->name, &var->guid);
+		if (ret == EFI_NOT_FOUND)
+			break;
+		if (ret != EFI_SUCCESS) {
+			free(buf);
+			return ret;
+		}
+		old_var_name_length = var_name_length;
+		old_var = var;
+
+		data = (u8 *)var->name + old_var_name_length;
+		data_length = (uintptr_t)buf + len - (uintptr_t)data;
+		ret = efi_get_variable_int(var->name, &var->guid,
+					   &var->attr, &data_length, data);
+		if (ret != EFI_SUCCESS) {
+			free(buf);
+			return ret;
+		}
+		if (!(var->attr & EFI_VARIABLE_NON_VOLATILE))
+			continue;
+		var->length = data_length;
+		var = (struct efi_var_entry *)
+		      ALIGN((uintptr_t)data + data_length, 8);
+	}
+
+	buf->reserved = 0;
+	buf->magic = EFI_VAR_FILE_MAGIC;
+	len = (uintptr_t)var - (uintptr_t)buf;
+	buf->crc32 = crc32(0, (u8 *)buf->var,
+			   len - sizeof(struct efi_var_file));
+	buf->length = len;
+	*bufp = buf;
+	*lenp = len;
+
+	return EFI_SUCCESS;
+}
+
+/**
+ * efi_var_to_file() - save non-volatile variables as file
+ *
+ * File ubootefi.var is created on the EFI system partion.
+ *
+ * Return:	status code
+ */
+efi_status_t efi_var_to_file(void)
+{
+#ifdef CONFIG_EFI_VARIABLE_FILE_STORE
+	efi_status_t ret;
+	struct efi_var_file *buf;
+	loff_t len;
+	loff_t actlen;
+	int r;
+
+	ret = efi_var_collect(&buf, &len);
+	if (ret != EFI_SUCCESS)
+		goto error;
+
+	ret = efi_set_blk_dev_to_system_partition();
+	if (ret != EFI_SUCCESS)
+		goto error;
+
+	r = fs_write(EFI_VAR_FILE_NAME, map_to_sysmem(buf), 0, len, &actlen);
+	if (r || len != actlen)
+		ret =  EFI_DEVICE_ERROR;
+
+error:
+	if (ret != EFI_SUCCESS)
+		printf("Failed to persist EFI variables\n");
+	free(buf);
+	return ret;
+#else
+	return EFI_SUCCESS;
+#endif
+}
+
+/**
+ * efi_var_restore() - restore EFI variables from buffer
+ *
+ * @buf:	buffer
+ * Return:	status code
+ */
+static efi_status_t __maybe_unused efi_var_restore(struct efi_var_file *buf)
+{
+	struct efi_var_entry *var, *last_var;
+	efi_status_t ret;
+
+	if (buf->reserved || buf->magic != EFI_VAR_FILE_MAGIC ||
+	    buf->crc32 != crc32(0, (u8 *)buf->var,
+				buf->length - sizeof(struct efi_var_file))) {
+		printf("Invalid EFI variables file\n");
+		return EFI_INVALID_PARAMETER;
+	}
+
+	var = buf->var;
+	last_var = (struct efi_var_entry *)((u8 *)buf + buf->length);
+	while (var < last_var) {
+		u16 *data = var->name + u16_strlen(var->name) + 1;
+
+		if (var->attr & EFI_VARIABLE_NON_VOLATILE && var->length) {
+			ret = efi_set_variable_int(var->name, &var->guid,
+						   var->attr, var->length,
+						   data);
+			if (ret != EFI_SUCCESS)
+				printf("Failed to set EFI variable %ls\n",
+				       var->name);
+			}
+		var = (struct efi_var_entry *)
+		      ALIGN((uintptr_t)data + var->length, 8);
+	}
+	return EFI_SUCCESS;
+}
+
+/**
+ * efi_var_from_file() - read variables from file
+ *
+ * File ubootefi.var is read from the EFI system partitions and the variables
+ * stored in the file are created.
+ *
+ * In case the file does not exist yet or a variable cannot be set EFI_SUCCESS
+ * is returned
+ *
+ * Return:	status code
+ */
+efi_status_t efi_var_from_file(void)
+{
+#ifdef CONFIG_EFI_VARIABLE_FILE_STORE
+	struct efi_var_file *buf;
+	loff_t len;
+	efi_status_t ret;
+	int r;
+
+	buf = calloc(1, EFI_VAR_BUF_SIZE);
+	if (!buf) {
+		printf("Out of memory\n");
+		return EFI_OUT_OF_RESOURCES;
+	}
+
+	ret = efi_set_blk_dev_to_system_partition();
+	if (ret != EFI_SUCCESS) {
+		printf("No EFI system partition\n");
+		goto error;
+	}
+	r = fs_read(EFI_VAR_FILE_NAME, map_to_sysmem(buf), 0, EFI_VAR_BUF_SIZE,
+		    &len);
+	if (r || len < sizeof(struct efi_var_file)) {
+		printf("Failed to load EFI variables\n");
+		goto error;
+	}
+	if (buf->length != len || efi_var_restore(buf) != EFI_SUCCESS)
+		printf("Invalid EFI variables file\n");
+error:
+	free(buf);
+#endif
+	return EFI_SUCCESS;
+}
--
2.25.1

[PATCH 10/16] efi_loader: UEFI variable persistence

[Punit Agrawal]

Heinrich Schuchardt <xypron.glpk@gmx.de> writes:

> Persist non-volatile UEFI variables in a file on the EFI system partition.
>
> The file is written:
>
> * whenever a non-volatile UEFI variable is changed after initialization
>   of the UEFI sub-system.
> * upon ExitBootServices()

I might be missing something but how does this cope with the ESP being
on a storage medium access to which is owned by the OS at runtime? e.g.,
partition on eMMC or SATA drive.

>
> The file is read during the UEFI sub-system initialization to restore
> non-volatile UEFI variables.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  include/efi_variable.h              |  36 +++++
>  lib/efi_loader/Kconfig              |   8 +
>  lib/efi_loader/Makefile             |   1 +
>  lib/efi_loader/efi_variable.c       |  12 +-
>  lib/efi_loader/efi_variables_file.c | 235 ++++++++++++++++++++++++++++
>  5 files changed, 291 insertions(+), 1 deletion(-)
>  create mode 100644 include/efi_variable.h
>  create mode 100644 lib/efi_loader/efi_variables_file.c
>

[...]

> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index e10ca05549..41705fc252 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -27,6 +27,14 @@ config EFI_LOADER
>
>  if EFI_LOADER
>
> +config EFI_VARIABLE_FILE_STORE
> +	bool "Store non-volatile UEFI variables as file"
> +	depends on FAT_WRITE
> +	default y
> +	help
> +	  Select tis option if you want non-volatile UEFI variables to be stored

                 this

> +	  as file /ubootefi.var on the EFI system partition.
> +
>  config EFI_GET_TIME
>  	bool "GetTime() runtime service"
>  	depends on DM_RTC

[...]

> diff --git a/lib/efi_loader/efi_variables_file.c b/lib/efi_loader/efi_variables_file.c
> new file mode 100644
> index 0000000000..4a918d3fde
> --- /dev/null
> +++ b/lib/efi_loader/efi_variables_file.c
> @@ -0,0 +1,235 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * File interface for UEFI variables
> + *
> + * Copyright (c) 2020, Heinrich Schuchardt
> + */
> +
> +#include <common.h>
> +#include <charset.h>
> +#include <fs.h>
> +#include <malloc.h>
> +#include <mapmem.h>
> +#include <efi_loader.h>
> +#include <efi_variable.h>
> +#include <u-boot/crc.h>
> +
> +#define PART_STR_LEN 10
> +
> +/**
> + * efi_set_blk_dev_to_system_partition() - select EFI system partition
> + *
> + * Set the EFI system partition as current block device.
> + *
> + * Return:	status code
> + */
> +static efi_status_t __maybe_unused efi_set_blk_dev_to_system_partition(void)
> +{
> +	char part_str[PART_STR_LEN];
> +	int r;
> +
> +	if (!efi_system_partition.if_type)
> +		return EFI_NOT_FOUND;
> +	snprintf(part_str, PART_STR_LEN, "%u:%u",
> +		 efi_system_partition.devnum, efi_system_partition.part);
> +	r = fs_set_blk_dev(blk_get_if_type_name(efi_system_partition.if_type),
> +			   part_str, FS_TYPE_ANY);
> +	if (r) {
> +		printf("Cannot read EFI system partition\n");
> +		return EFI_DEVICE_ERROR;
> +	}
> +	return EFI_SUCCESS;
> +}
> +
> +/**
> + * efi_var_collect() - collect non-volatile variables in buffer
> + *
> + * A buffer is allocated and filled with all non-volatile variables in a
> + * format ready to be written to disk.
> + *
> + * @bufp:	pointer to pointer of buffer with collected variables
> + * @lenp:	pointer to length of buffer
> + * Return:	status code
> + */
> +static efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp,
> +						   loff_t *lenp)
> +{
> +	size_t len = EFI_VAR_BUF_SIZE;
> +	struct efi_var_file *buf;
> +	struct efi_var_entry *var, *old_var;
> +	size_t old_var_name_length = 2;
> +
> +	*bufp = NULL; /* Avoid double free() */
> +	buf = calloc(1, len);
> +	if (!buf)
> +		return EFI_OUT_OF_RESOURCES;
> +	var = buf->var;
> +	old_var = var;
> +	for (;;) {
> +		efi_uintn_t data_length, var_name_length;
> +		u8 *data;
> +		efi_status_t ret;
> +
> +		if ((uintptr_t)buf + len <=
> +		    (uintptr_t)var->name + old_var_name_length)
> +			return EFI_BUFFER_TOO_SMALL;
> +
> +		var_name_length = (uintptr_t)buf + len - (uintptr_t)var->name;
> +		memcpy(var->name, old_var->name, old_var_name_length);
> +		guidcpy(&var->guid, &old_var->guid);
> +		ret = efi_get_next_variable_name_int(
> +				&var_name_length, var->name, &var->guid);
> +		if (ret == EFI_NOT_FOUND)
> +			break;
> +		if (ret != EFI_SUCCESS) {
> +			free(buf);
> +			return ret;
> +		}
> +		old_var_name_length = var_name_length;
> +		old_var = var;
> +
> +		data = (u8 *)var->name + old_var_name_length;
> +		data_length = (uintptr_t)buf + len - (uintptr_t)data;
> +		ret = efi_get_variable_int(var->name, &var->guid,
> +					   &var->attr, &data_length, data);
> +		if (ret != EFI_SUCCESS) {
> +			free(buf);
> +			return ret;
> +		}
> +		if (!(var->attr & EFI_VARIABLE_NON_VOLATILE))
> +			continue;
> +		var->length = data_length;
> +		var = (struct efi_var_entry *)
> +		      ALIGN((uintptr_t)data + data_length, 8);
> +	}
> +
> +	buf->reserved = 0;
> +	buf->magic = EFI_VAR_FILE_MAGIC;
> +	len = (uintptr_t)var - (uintptr_t)buf;
> +	buf->crc32 = crc32(0, (u8 *)buf->var,
> +			   len - sizeof(struct efi_var_file));
> +	buf->length = len;
> +	*bufp = buf;
> +	*lenp = len;
> +
> +	return EFI_SUCCESS;
> +}
> +
> +/**
> + * efi_var_to_file() - save non-volatile variables as file
> + *
> + * File ubootefi.var is created on the EFI system partion.
> + *
> + * Return:	status code
> + */

The return value doesn't seem to be used in this patch. Is it really
needed?

Thanks,
Punit

> +efi_status_t efi_var_to_file(void)
> +{
> +#ifdef CONFIG_EFI_VARIABLE_FILE_STORE
> +	efi_status_t ret;
> +	struct efi_var_file *buf;
> +	loff_t len;
> +	loff_t actlen;
> +	int r;
> +
> +	ret = efi_var_collect(&buf, &len);
> +	if (ret != EFI_SUCCESS)
> +		goto error;
> +
> +	ret = efi_set_blk_dev_to_system_partition();
> +	if (ret != EFI_SUCCESS)
> +		goto error;
> +
> +	r = fs_write(EFI_VAR_FILE_NAME, map_to_sysmem(buf), 0, len, &actlen);
> +	if (r || len != actlen)
> +		ret =  EFI_DEVICE_ERROR;
> +
> +error:
> +	if (ret != EFI_SUCCESS)
> +		printf("Failed to persist EFI variables\n");
> +	free(buf);
> +	return ret;
> +#else
> +	return EFI_SUCCESS;
> +#endif
> +}
> +

[...]

[PATCH 10/16] efi_loader: UEFI variable persistence

[Heinrich Schuchardt]

On 3/27/20 9:07 AM, Punit Agrawal wrote:
> Heinrich Schuchardt <xypron.glpk@gmx.de> writes:
>
>> Persist non-volatile UEFI variables in a file on the EFI system partition.
>>
>> The file is written:
>>
>> * whenever a non-volatile UEFI variable is changed after initialization
>>    of the UEFI sub-system.
>> * upon ExitBootServices()
>
> I might be missing something but how does this cope with the ESP being
> on a storage medium access to which is owned by the OS at runtime? e.g.,
> partition on eMMC or SATA drive.

This development does not guard against manipulation by the OS.

Ilias is cureently working on a solution for ATF based devices that will
provide secure storage for variables.

>
>>
>> The file is read during the UEFI sub-system initialization to restore
>> non-volatile UEFI variables.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>   include/efi_variable.h              |  36 +++++
>>   lib/efi_loader/Kconfig              |   8 +
>>   lib/efi_loader/Makefile             |   1 +
>>   lib/efi_loader/efi_variable.c       |  12 +-
>>   lib/efi_loader/efi_variables_file.c | 235 ++++++++++++++++++++++++++++
>>   5 files changed, 291 insertions(+), 1 deletion(-)
>>   create mode 100644 include/efi_variable.h
>>   create mode 100644 lib/efi_loader/efi_variables_file.c
>>
>
> [...]
>
>> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
>> index e10ca05549..41705fc252 100644
>> --- a/lib/efi_loader/Kconfig
>> +++ b/lib/efi_loader/Kconfig
>> @@ -27,6 +27,14 @@ config EFI_LOADER
>>
>>   if EFI_LOADER
>>
>> +config EFI_VARIABLE_FILE_STORE
>> +	bool "Store non-volatile UEFI variables as file"
>> +	depends on FAT_WRITE
>> +	default y
>> +	help
>> +	  Select tis option if you want non-volatile UEFI variables to be stored
>
>                   this
>
>> +	  as file /ubootefi.var on the EFI system partition.
>> +
>>   config EFI_GET_TIME
>>   	bool "GetTime() runtime service"
>>   	depends on DM_RTC
>
> [...]
>
>> diff --git a/lib/efi_loader/efi_variables_file.c b/lib/efi_loader/efi_variables_file.c
>> new file mode 100644
>> index 0000000000..4a918d3fde
>> --- /dev/null
>> +++ b/lib/efi_loader/efi_variables_file.c
>> @@ -0,0 +1,235 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * File interface for UEFI variables
>> + *
>> + * Copyright (c) 2020, Heinrich Schuchardt
>> + */
>> +
>> +#include <common.h>
>> +#include <charset.h>
>> +#include <fs.h>
>> +#include <malloc.h>
>> +#include <mapmem.h>
>> +#include <efi_loader.h>
>> +#include <efi_variable.h>
>> +#include <u-boot/crc.h>
>> +
>> +#define PART_STR_LEN 10
>> +
>> +/**
>> + * efi_set_blk_dev_to_system_partition() - select EFI system partition
>> + *
>> + * Set the EFI system partition as current block device.
>> + *
>> + * Return:	status code
>> + */
>> +static efi_status_t __maybe_unused efi_set_blk_dev_to_system_partition(void)
>> +{
>> +	char part_str[PART_STR_LEN];
>> +	int r;
>> +
>> +	if (!efi_system_partition.if_type)
>> +		return EFI_NOT_FOUND;
>> +	snprintf(part_str, PART_STR_LEN, "%u:%u",
>> +		 efi_system_partition.devnum, efi_system_partition.part);
>> +	r = fs_set_blk_dev(blk_get_if_type_name(efi_system_partition.if_type),
>> +			   part_str, FS_TYPE_ANY);
>> +	if (r) {
>> +		printf("Cannot read EFI system partition\n");
>> +		return EFI_DEVICE_ERROR;
>> +	}
>> +	return EFI_SUCCESS;
>> +}
>> +
>> +/**
>> + * efi_var_collect() - collect non-volatile variables in buffer
>> + *
>> + * A buffer is allocated and filled with all non-volatile variables in a
>> + * format ready to be written to disk.
>> + *
>> + * @bufp:	pointer to pointer of buffer with collected variables
>> + * @lenp:	pointer to length of buffer
>> + * Return:	status code
>> + */
>> +static efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp,
>> +						   loff_t *lenp)
>> +{
>> +	size_t len = EFI_VAR_BUF_SIZE;
>> +	struct efi_var_file *buf;
>> +	struct efi_var_entry *var, *old_var;
>> +	size_t old_var_name_length = 2;
>> +
>> +	*bufp = NULL; /* Avoid double free() */
>> +	buf = calloc(1, len);
>> +	if (!buf)
>> +		return EFI_OUT_OF_RESOURCES;
>> +	var = buf->var;
>> +	old_var = var;
>> +	for (;;) {
>> +		efi_uintn_t data_length, var_name_length;
>> +		u8 *data;
>> +		efi_status_t ret;
>> +
>> +		if ((uintptr_t)buf + len <=
>> +		    (uintptr_t)var->name + old_var_name_length)
>> +			return EFI_BUFFER_TOO_SMALL;
>> +
>> +		var_name_length = (uintptr_t)buf + len - (uintptr_t)var->name;
>> +		memcpy(var->name, old_var->name, old_var_name_length);
>> +		guidcpy(&var->guid, &old_var->guid);
>> +		ret = efi_get_next_variable_name_int(
>> +				&var_name_length, var->name, &var->guid);
>> +		if (ret == EFI_NOT_FOUND)
>> +			break;
>> +		if (ret != EFI_SUCCESS) {
>> +			free(buf);
>> +			return ret;
>> +		}
>> +		old_var_name_length = var_name_length;
>> +		old_var = var;
>> +
>> +		data = (u8 *)var->name + old_var_name_length;
>> +		data_length = (uintptr_t)buf + len - (uintptr_t)data;
>> +		ret = efi_get_variable_int(var->name, &var->guid,
>> +					   &var->attr, &data_length, data);
>> +		if (ret != EFI_SUCCESS) {
>> +			free(buf);
>> +			return ret;
>> +		}
>> +		if (!(var->attr & EFI_VARIABLE_NON_VOLATILE))
>> +			continue;
>> +		var->length = data_length;
>> +		var = (struct efi_var_entry *)
>> +		      ALIGN((uintptr_t)data + data_length, 8);
>> +	}
>> +
>> +	buf->reserved = 0;
>> +	buf->magic = EFI_VAR_FILE_MAGIC;
>> +	len = (uintptr_t)var - (uintptr_t)buf;
>> +	buf->crc32 = crc32(0, (u8 *)buf->var,
>> +			   len - sizeof(struct efi_var_file));
>> +	buf->length = len;
>> +	*bufp = buf;
>> +	*lenp = len;
>> +
>> +	return EFI_SUCCESS;
>> +}
>> +
>> +/**
>> + * efi_var_to_file() - save non-volatile variables as file
>> + *
>> + * File ubootefi.var is created on the EFI system partion.
>> + *
>> + * Return:	status code
>> + */
>
> The return value doesn't seem to be used in this patch. Is it really
> needed?

It can be remove.

Thanks for reviewing.

Best regards

Heinrich

>
> Thanks,
> Punit
>
>> +efi_status_t efi_var_to_file(void)
>> +{
>> +#ifdef CONFIG_EFI_VARIABLE_FILE_STORE
>> +	efi_status_t ret;
>> +	struct efi_var_file *buf;
>> +	loff_t len;
>> +	loff_t actlen;
>> +	int r;
>> +
>> +	ret = efi_var_collect(&buf, &len);
>> +	if (ret != EFI_SUCCESS)
>> +		goto error;
>> +
>> +	ret = efi_set_blk_dev_to_system_partition();
>> +	if (ret != EFI_SUCCESS)
>> +		goto error;
>> +
>> +	r = fs_write(EFI_VAR_FILE_NAME, map_to_sysmem(buf), 0, len, &actlen);
>> +	if (r || len != actlen)
>> +		ret =  EFI_DEVICE_ERROR;
>> +
>> +error:
>> +	if (ret != EFI_SUCCESS)
>> +		printf("Failed to persist EFI variables\n");
>> +	free(buf);
>> +	return ret;
>> +#else
>> +	return EFI_SUCCESS;
>> +#endif
>> +}
>> +
>
> [...]
>

[PATCH 10/16] efi_loader: UEFI variable persistence

[Punit Agrawal]

Heinrich Schuchardt <xypron.glpk@gmx.de> writes:

> On 3/27/20 9:07 AM, Punit Agrawal wrote:
>> Heinrich Schuchardt <xypron.glpk@gmx.de> writes:
>>
>>> Persist non-volatile UEFI variables in a file on the EFI system partition.
>>>
>>> The file is written:
>>>
>>> * whenever a non-volatile UEFI variable is changed after initialization
>>>    of the UEFI sub-system.
>>> * upon ExitBootServices()
>>
>> I might be missing something but how does this cope with the ESP being
>> on a storage medium access to which is owned by the OS at runtime? e.g.,
>> partition on eMMC or SATA drive.
>
> This development does not guard against manipulation by the OS.
>
> Ilias is cureently working on a solution for ATF based devices that will
> provide secure storage for variables.

Thanks for the clarification.

So the current patches are more RFC material - as it would be worth
seeing the whole picture before things start getting baked in.

I only recently started looking at EFI features in u-boot and am trying
to piece the story together based on the patches in-flight.

Thanks,
Punit

[...]

[PATCH 10/16] efi_loader: UEFI variable persistence

[Ilias Apalodimas]

On Fri, Mar 27, 2020 at 11:30:47AM +0100, Heinrich Schuchardt wrote:
> On 3/27/20 9:07 AM, Punit Agrawal wrote:
> > Heinrich Schuchardt <xypron.glpk@gmx.de> writes:
> > 
> > > Persist non-volatile UEFI variables in a file on the EFI system partition.
> > > 
> > > The file is written:
> > > 
> > > * whenever a non-volatile UEFI variable is changed after initialization
> > >    of the UEFI sub-system.
> > > * upon ExitBootServices()
> > 
> > I might be missing something but how does this cope with the ESP being
> > on a storage medium access to which is owned by the OS at runtime? e.g.,
> > partition on eMMC or SATA drive.
> 
> This development does not guard against manipulation by the OS.
> 
> Ilias is cureently working on a solution for ATF based devices that will
> provide secure storage for variables.
> 

Ok sorry for jumping in late, I was actually coding what Heinrich mentioned and
I do have a rough prototype. 

I think it's time we (Linaro) expose the idea we had a while back a bit more.
The problem we identified is that the current EDK2 implementation on Arm uses
SPM (Secure Partition Manager). Although this works quite well, SPM and
SPD(Secure Payload Dispatcher) are mutually exclusive. So running SPM and
variable storage, means no OP-TEE.

What we ended up doing, is run the EDK2 StandAloneMM application, 
responsible for *all* the handling of UEFI variables in an isolated OP-TEE
partition. 
By doing so we can add variable storage on U-boot as an 'external' library which
also hapopens to be 'firmware agnostic'.
The only thing U-Boot has to do is route the Get/Set variable to the Secure World
and if there's an OP-TEE instance running the variables will be handled by that.
This includes authentication and writing them to a medium. The medium can either
be a dedicated flash on Secure world, an RPMB partiiton of the eMMC or a
file in some filesystem.

Storing them on a dedicated Secure world flash, means that the vendor has to
provide that driver in EDK2. 
Storing them on an eMMC has a single requirement. An eMMC driver in U-boot, since
OP-TEE uses an existing U-Boot supplicant to access the RPMB.
Storing them on a filesystem through OP-TEE is not implemented by us yet but it
might worth a try since at least the variable verification and authentication
will be executed in Secure World.
In any case this renders U-Boot agnostic to the actual variables, again, as long
it routes the requests to the secure world. 

That being said I do believe Heinrich's patch is very useful, since if U-Boot
doesn't run on an Arm core, you still have a way of providing some kind of
secure storage (and a fallback for Arm devices without options (1), (2)).

So, imho, the things need we need to discuss further is:
- EDK2 uses a firmware block protocol, with it's own defined hears and a
  Fault Tolerant Write backup partitions. By using the solution I mentioned, you
  inherit that as well. Should the 'raw' U-boot code follow that example and use
  the same format for the file? Or is Heinrich's format adequate?
  The obvious advantage is that systsems can switch between EDK2/U-Boot and
  still be able to use the same variable format.
  The disadvantage is that it's kind of complicated for my taste :)

- Should U-boot try to offer similar code for variable authentation and
  verification? The reason we decided to run StandAloneMM and get those from
  EDK2, is that the whole variable format, attributes and auth of UEFI variables
  is complex and big. Does it make sense to add that on U-Boot keeping in mind
  it's something running on Non-secure world?



Regards
/Ilias

> > 
> > > 
[...]
> > > +	if (r || len != actlen)
> > > +		ret =  EFI_DEVICE_ERROR;
> > > +
> > > +error:
> > > +	if (ret != EFI_SUCCESS)
> > > +		printf("Failed to persist EFI variables\n");
> > > +	free(buf);
> > > +	return ret;
> > > +#else
> > > +	return EFI_SUCCESS;
> > > +#endif
> > > +}
> > > +
> > 
> > [...]
> > 
> 

[PATCH 10/16] efi_loader: UEFI variable persistence

[Peter Robinson]

 Hi Heinrich,

In testing this I've noticed one thing, if the ESP partition has a
type of FAT16 (type 6) it doesn't detect the partition, the EFI spec
says the ESP partition can be FAT16 and some devices at least the rpi2
won't boot if the partition ID is set to the efi partition naming, but
it errors with "No EFI system partition" if it's set as such.

Peter

> Persist non-volatile UEFI variables in a file on the EFI system partition.
>
> The file is written:
>
> * whenever a non-volatile UEFI variable is changed after initialization
>   of the UEFI sub-system.
> * upon ExitBootServices()
>
> The file is read during the UEFI sub-system initialization to restore
> non-volatile UEFI variables.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  include/efi_variable.h              |  36 +++++
>  lib/efi_loader/Kconfig              |   8 +
>  lib/efi_loader/Makefile             |   1 +
>  lib/efi_loader/efi_variable.c       |  12 +-
>  lib/efi_loader/efi_variables_file.c | 235 ++++++++++++++++++++++++++++
>  5 files changed, 291 insertions(+), 1 deletion(-)
>  create mode 100644 include/efi_variable.h
>  create mode 100644 lib/efi_loader/efi_variables_file.c
>
> diff --git a/include/efi_variable.h b/include/efi_variable.h
> new file mode 100644
> index 0000000000..fb8294fc2e
> --- /dev/null
> +++ b/include/efi_variable.h
> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * File interface for UEFI variables
> + *
> + * Copyright (c) 2020, Heinrich Schuchardt
> + */
> +
> +#ifndef _EFI_VARIABLE_H
> +#define _EFI_VARIABLE_H
> +
> +#define EFI_VAR_FILE_NAME "ubootefi.var"
> +
> +#define EFI_VAR_BUF_SIZE 0x4000
> +
> +#define EFI_VAR_FILE_MAGIC 0x7261566966456255 /* UbEfiVar */
> +
> +struct efi_var_entry {
> +       u32 length;
> +       u32 attr;
> +       efi_guid_t guid;
> +       u16 name[0];
> +};
> +
> +struct efi_var_file {
> +       u64 reserved; /* May be overwritten by memory probing */
> +       u64 magic;
> +       u32 length;
> +       u32 crc32;
> +       struct efi_var_entry var[0];
> +};
> +
> +efi_status_t efi_var_to_file(void);
> +
> +efi_status_t efi_var_from_file(void);
> +
> +#endif
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index e10ca05549..41705fc252 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -27,6 +27,14 @@ config EFI_LOADER
>
>  if EFI_LOADER
>
> +config EFI_VARIABLE_FILE_STORE
> +       bool "Store non-volatile UEFI variables as file"
> +       depends on FAT_WRITE
> +       default y
> +       help
> +         Select tis option if you want non-volatile UEFI variables to be stored
> +         as file /ubootefi.var on the EFI system partition.
> +
>  config EFI_GET_TIME
>         bool "GetTime() runtime service"
>         depends on DM_RTC
> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> index 9b3b704473..621a767ab3 100644
> --- a/lib/efi_loader/Makefile
> +++ b/lib/efi_loader/Makefile
> @@ -35,6 +35,7 @@ obj-y += efi_runtime.o
>  obj-y += efi_setup.o
>  obj-$(CONFIG_EFI_UNICODE_COLLATION_PROTOCOL2) += efi_unicode_collation.o
>  obj-y += efi_variable.o
> +obj-y += efi_variables_file.o
>  obj-y += efi_watchdog.o
>  obj-$(CONFIG_LCD) += efi_gop.o
>  obj-$(CONFIG_DM_VIDEO) += efi_gop.o
> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> index d99ad6ddae..952a0a0db7 100644
> --- a/lib/efi_loader/efi_variable.c
> +++ b/lib/efi_loader/efi_variable.c
> @@ -7,6 +7,7 @@
>
>  #include <common.h>
>  #include <efi_loader.h>
> +#include <efi_variable.h>
>  #include <env_internal.h>
>  #include <hexdump.h>
>  #include <malloc.h>
> @@ -604,6 +605,11 @@ efi_status_t efi_set_variable_int(u16 *variable_name,
>         if (env_set(native_name, val))
>                 ret = EFI_DEVICE_ERROR;
>
> +       /* Write non-volatile EFI variables to file */
> +       if (attributes && EFI_VARIABLE_NON_VOLATILE &&
> +           ret == EFI_SUCCESS && efi_obj_list_initialized == EFI_SUCCESS)
> +               efi_var_to_file();
> +
>  out:
>         free(native_name);
>         free(val);
> @@ -694,6 +700,10 @@ efi_set_variable_runtime(u16 *variable_name, const efi_guid_t *vendor,
>   */
>  void efi_variables_boot_exit_notify(void)
>  {
> +       /* Write non-volatile EFI variables to file */
> +       efi_var_to_file();
> +
> +       /* Switch variable services functions to runtime version */
>         efi_runtime_services.get_variable = efi_get_variable_runtime;
>         efi_runtime_services.get_next_variable_name =
>                                 efi_get_next_variable_name_runtime;
> @@ -708,5 +718,5 @@ void efi_variables_boot_exit_notify(void)
>   */
>  efi_status_t efi_init_variables(void)
>  {
> -       return EFI_SUCCESS;
> +       return efi_var_from_file();
>  }
> diff --git a/lib/efi_loader/efi_variables_file.c b/lib/efi_loader/efi_variables_file.c
> new file mode 100644
> index 0000000000..4a918d3fde
> --- /dev/null
> +++ b/lib/efi_loader/efi_variables_file.c
> @@ -0,0 +1,235 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * File interface for UEFI variables
> + *
> + * Copyright (c) 2020, Heinrich Schuchardt
> + */
> +
> +#include <common.h>
> +#include <charset.h>
> +#include <fs.h>
> +#include <malloc.h>
> +#include <mapmem.h>
> +#include <efi_loader.h>
> +#include <efi_variable.h>
> +#include <u-boot/crc.h>
> +
> +#define PART_STR_LEN 10
> +
> +/**
> + * efi_set_blk_dev_to_system_partition() - select EFI system partition
> + *
> + * Set the EFI system partition as current block device.
> + *
> + * Return:     status code
> + */
> +static efi_status_t __maybe_unused efi_set_blk_dev_to_system_partition(void)
> +{
> +       char part_str[PART_STR_LEN];
> +       int r;
> +
> +       if (!efi_system_partition.if_type)
> +               return EFI_NOT_FOUND;
> +       snprintf(part_str, PART_STR_LEN, "%u:%u",
> +                efi_system_partition.devnum, efi_system_partition.part);
> +       r = fs_set_blk_dev(blk_get_if_type_name(efi_system_partition.if_type),
> +                          part_str, FS_TYPE_ANY);
> +       if (r) {
> +               printf("Cannot read EFI system partition\n");
> +               return EFI_DEVICE_ERROR;
> +       }
> +       return EFI_SUCCESS;
> +}
> +
> +/**
> + * efi_var_collect() - collect non-volatile variables in buffer
> + *
> + * A buffer is allocated and filled with all non-volatile variables in a
> + * format ready to be written to disk.
> + *
> + * @bufp:      pointer to pointer of buffer with collected variables
> + * @lenp:      pointer to length of buffer
> + * Return:     status code
> + */
> +static efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp,
> +                                                  loff_t *lenp)
> +{
> +       size_t len = EFI_VAR_BUF_SIZE;
> +       struct efi_var_file *buf;
> +       struct efi_var_entry *var, *old_var;
> +       size_t old_var_name_length = 2;
> +
> +       *bufp = NULL; /* Avoid double free() */
> +       buf = calloc(1, len);
> +       if (!buf)
> +               return EFI_OUT_OF_RESOURCES;
> +       var = buf->var;
> +       old_var = var;
> +       for (;;) {
> +               efi_uintn_t data_length, var_name_length;
> +               u8 *data;
> +               efi_status_t ret;
> +
> +               if ((uintptr_t)buf + len <=
> +                   (uintptr_t)var->name + old_var_name_length)
> +                       return EFI_BUFFER_TOO_SMALL;
> +
> +               var_name_length = (uintptr_t)buf + len - (uintptr_t)var->name;
> +               memcpy(var->name, old_var->name, old_var_name_length);
> +               guidcpy(&var->guid, &old_var->guid);
> +               ret = efi_get_next_variable_name_int(
> +                               &var_name_length, var->name, &var->guid);
> +               if (ret == EFI_NOT_FOUND)
> +                       break;
> +               if (ret != EFI_SUCCESS) {
> +                       free(buf);
> +                       return ret;
> +               }
> +               old_var_name_length = var_name_length;
> +               old_var = var;
> +
> +               data = (u8 *)var->name + old_var_name_length;
> +               data_length = (uintptr_t)buf + len - (uintptr_t)data;
> +               ret = efi_get_variable_int(var->name, &var->guid,
> +                                          &var->attr, &data_length, data);
> +               if (ret != EFI_SUCCESS) {
> +                       free(buf);
> +                       return ret;
> +               }
> +               if (!(var->attr & EFI_VARIABLE_NON_VOLATILE))
> +                       continue;
> +               var->length = data_length;
> +               var = (struct efi_var_entry *)
> +                     ALIGN((uintptr_t)data + data_length, 8);
> +       }
> +
> +       buf->reserved = 0;
> +       buf->magic = EFI_VAR_FILE_MAGIC;
> +       len = (uintptr_t)var - (uintptr_t)buf;
> +       buf->crc32 = crc32(0, (u8 *)buf->var,
> +                          len - sizeof(struct efi_var_file));
> +       buf->length = len;
> +       *bufp = buf;
> +       *lenp = len;
> +
> +       return EFI_SUCCESS;
> +}
> +
> +/**
> + * efi_var_to_file() - save non-volatile variables as file
> + *
> + * File ubootefi.var is created on the EFI system partion.
> + *
> + * Return:     status code
> + */
> +efi_status_t efi_var_to_file(void)
> +{
> +#ifdef CONFIG_EFI_VARIABLE_FILE_STORE
> +       efi_status_t ret;
> +       struct efi_var_file *buf;
> +       loff_t len;
> +       loff_t actlen;
> +       int r;
> +
> +       ret = efi_var_collect(&buf, &len);
> +       if (ret != EFI_SUCCESS)
> +               goto error;
> +
> +       ret = efi_set_blk_dev_to_system_partition();
> +       if (ret != EFI_SUCCESS)
> +               goto error;
> +
> +       r = fs_write(EFI_VAR_FILE_NAME, map_to_sysmem(buf), 0, len, &actlen);
> +       if (r || len != actlen)
> +               ret =  EFI_DEVICE_ERROR;
> +
> +error:
> +       if (ret != EFI_SUCCESS)
> +               printf("Failed to persist EFI variables\n");
> +       free(buf);
> +       return ret;
> +#else
> +       return EFI_SUCCESS;
> +#endif
> +}
> +
> +/**
> + * efi_var_restore() - restore EFI variables from buffer
> + *
> + * @buf:       buffer
> + * Return:     status code
> + */
> +static efi_status_t __maybe_unused efi_var_restore(struct efi_var_file *buf)
> +{
> +       struct efi_var_entry *var, *last_var;
> +       efi_status_t ret;
> +
> +       if (buf->reserved || buf->magic != EFI_VAR_FILE_MAGIC ||
> +           buf->crc32 != crc32(0, (u8 *)buf->var,
> +                               buf->length - sizeof(struct efi_var_file))) {
> +               printf("Invalid EFI variables file\n");
> +               return EFI_INVALID_PARAMETER;
> +       }
> +
> +       var = buf->var;
> +       last_var = (struct efi_var_entry *)((u8 *)buf + buf->length);
> +       while (var < last_var) {
> +               u16 *data = var->name + u16_strlen(var->name) + 1;
> +
> +               if (var->attr & EFI_VARIABLE_NON_VOLATILE && var->length) {
> +                       ret = efi_set_variable_int(var->name, &var->guid,
> +                                                  var->attr, var->length,
> +                                                  data);
> +                       if (ret != EFI_SUCCESS)
> +                               printf("Failed to set EFI variable %ls\n",
> +                                      var->name);
> +                       }
> +               var = (struct efi_var_entry *)
> +                     ALIGN((uintptr_t)data + var->length, 8);
> +       }
> +       return EFI_SUCCESS;
> +}
> +
> +/**
> + * efi_var_from_file() - read variables from file
> + *
> + * File ubootefi.var is read from the EFI system partitions and the variables
> + * stored in the file are created.
> + *
> + * In case the file does not exist yet or a variable cannot be set EFI_SUCCESS
> + * is returned
> + *
> + * Return:     status code
> + */
> +efi_status_t efi_var_from_file(void)
> +{
> +#ifdef CONFIG_EFI_VARIABLE_FILE_STORE
> +       struct efi_var_file *buf;
> +       loff_t len;
> +       efi_status_t ret;
> +       int r;
> +
> +       buf = calloc(1, EFI_VAR_BUF_SIZE);
> +       if (!buf) {
> +               printf("Out of memory\n");
> +               return EFI_OUT_OF_RESOURCES;
> +       }
> +
> +       ret = efi_set_blk_dev_to_system_partition();
> +       if (ret != EFI_SUCCESS) {
> +               printf("No EFI system partition\n");
> +               goto error;
> +       }
> +       r = fs_read(EFI_VAR_FILE_NAME, map_to_sysmem(buf), 0, EFI_VAR_BUF_SIZE,
> +                   &len);
> +       if (r || len < sizeof(struct efi_var_file)) {
> +               printf("Failed to load EFI variables\n");
> +               goto error;
> +       }
> +       if (buf->length != len || efi_var_restore(buf) != EFI_SUCCESS)
> +               printf("Invalid EFI variables file\n");
> +error:
> +       free(buf);
> +#endif
> +       return EFI_SUCCESS;
> +}
> --
> 2.25.1
>

[PATCH 11/16] efi_loader: export efi_convert_pointer()

[Heinrich Schuchardt]

We need ConvertPointer() to adjust pointers when implementing  runtime
services within U-Boot.

After ExitBootServices() gd is not available anymore. So we should not use
EFI_ENTRY() and EFI_EXIT().

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 include/efi_loader.h         | 3 +++
 lib/efi_loader/efi_runtime.c | 8 +++-----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 510f39fe9e..d17a3c5ae1 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -374,6 +374,9 @@ efi_status_t efi_root_node_register(void);
 efi_status_t efi_initialize_system_table(void);
 /* efi_runtime_detach() - detach unimplemented runtime functions */
 void efi_runtime_detach(void);
+/* efi_convert_pointer() - convert pointer to virtual address */
+efi_status_t EFIAPI efi_convert_pointer(efi_uintn_t debug_disposition,
+					void **address);
 /* Called by bootefi to make console interface available */
 efi_status_t efi_console_register(void);
 /* Called by bootefi to make all disk storage accessible as EFI objects */
diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
index 6a25acbbcd..67fa693e41 100644
--- a/lib/efi_loader/efi_runtime.c
+++ b/lib/efi_loader/efi_runtime.c
@@ -495,15 +495,13 @@ static __efi_runtime efi_status_t EFIAPI efi_convert_pointer_runtime(
  * @address:		pointer to be converted
  * Return:		status code
  */
-static __efi_runtime efi_status_t EFIAPI efi_convert_pointer(
-			efi_uintn_t debug_disposition, void **address)
+__efi_runtime efi_status_t EFIAPI
+efi_convert_pointer(efi_uintn_t debug_disposition, void **address)
 {
 	efi_physical_addr_t addr = (uintptr_t)*address;
 	efi_uintn_t i;
 	efi_status_t ret = EFI_NOT_FOUND;

-	EFI_ENTRY("%zu %p", debug_disposition, address);
-
 	if (!efi_virtmap) {
 		ret = EFI_UNSUPPORTED;
 		goto out;
@@ -531,7 +529,7 @@ static __efi_runtime efi_status_t EFIAPI efi_convert_pointer(
 	}

 out:
-	return EFI_EXIT(ret);
+	return ret;
 }

 static __efi_runtime void efi_relocate_runtime_table(ulong offset)
--
2.25.1

[PATCH 12/16] efi_loader: optional pointer for ConvertPointer

[Heinrich Schuchardt]

If the EFI_OPTIONAL_PTR is set in DebugDisposition, a NULL pointer does not
constitute an invalid parameter.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 include/efi_api.h            | 2 ++
 lib/efi_loader/efi_runtime.c | 6 ++++++
 2 files changed, 8 insertions(+)

diff --git a/include/efi_api.h b/include/efi_api.h
index 1c40ffc4f5..c56703fc5e 100644
--- a/include/efi_api.h
+++ b/include/efi_api.h
@@ -250,6 +250,8 @@ struct efi_rt_properties_table {
 	u32 runtime_services_supported;
 };

+#define EFI_OPTIONAL_PTR	0x00000001
+
 struct efi_runtime_services {
 	struct efi_table_hdr hdr;
 	efi_status_t (EFIAPI *get_time)(struct efi_time *time,
diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
index 67fa693e41..664a0422e2 100644
--- a/lib/efi_loader/efi_runtime.c
+++ b/lib/efi_loader/efi_runtime.c
@@ -511,6 +511,12 @@ efi_convert_pointer(efi_uintn_t debug_disposition, void **address)
 		ret = EFI_INVALID_PARAMETER;
 		goto out;
 	}
+	if (!*address) {
+		if (debug_disposition & EFI_OPTIONAL_PTR)
+			return EFI_SUCCESS;
+		else
+			return EFI_INVALID_PARAMETER;
+	}

 	for (i = 0; i < efi_descriptor_count; i++) {
 		struct efi_mem_desc *map = (void *)efi_virtmap +
--
2.25.1

[PATCH 12/16] efi_loader: optional pointer for ConvertPointer

[AKASHI Takahiro]

On Fri, Mar 27, 2020 at 06:27:56AM +0100, Heinrich Schuchardt wrote:
> If the EFI_OPTIONAL_PTR is set in DebugDisposition, a NULL pointer does not
> constitute an invalid parameter.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  include/efi_api.h            | 2 ++
>  lib/efi_loader/efi_runtime.c | 6 ++++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/include/efi_api.h b/include/efi_api.h
> index 1c40ffc4f5..c56703fc5e 100644
> --- a/include/efi_api.h
> +++ b/include/efi_api.h
> @@ -250,6 +250,8 @@ struct efi_rt_properties_table {
>  	u32 runtime_services_supported;
>  };
> 
> +#define EFI_OPTIONAL_PTR	0x00000001
> +
>  struct efi_runtime_services {
>  	struct efi_table_hdr hdr;
>  	efi_status_t (EFIAPI *get_time)(struct efi_time *time,
> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
> index 67fa693e41..664a0422e2 100644
> --- a/lib/efi_loader/efi_runtime.c
> +++ b/lib/efi_loader/efi_runtime.c
> @@ -511,6 +511,12 @@ efi_convert_pointer(efi_uintn_t debug_disposition, void **address)
>  		ret = EFI_INVALID_PARAMETER;
>  		goto out;
>  	}
> +	if (!*address) {
> +		if (debug_disposition & EFI_OPTIONAL_PTR)
> +			return EFI_SUCCESS;
> +		else
> +			return EFI_INVALID_PARAMETER;
> +	}

In either case, NULL won't be converted.
Is this a behavior UEFI specification expects?
(I haven't found any description there.)

-Takahiro Akashi


>  	for (i = 0; i < efi_descriptor_count; i++) {
>  		struct efi_mem_desc *map = (void *)efi_virtmap +
> --
> 2.25.1
> 

[PATCH 12/16] efi_loader: optional pointer for ConvertPointer

[Heinrich Schuchardt]

On March 31, 2020, 5:23 a.m. UTC Takahiro Akashi wrote
 >On Fri, Mar 27, 2020 at 06:27:56AM +0100, Heinrich Schuchardt wrote:
 >> If the EFI_OPTIONAL_PTR is set in DebugDisposition, a NULL pointer
does not
 >> constitute an invalid parameter.
 >>
 >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
 >> ---
 >>  include/efi_api.h            | 2 ++
 >>  lib/efi_loader/efi_runtime.c | 6 ++++++
 >>  2 files changed, 8 insertions(+)
 >>
 >> diff --git a/include/efi_api.h b/include/efi_api.h
 >> index 1c40ffc4f5..c56703fc5e 100644
 >> --- a/include/efi_api.h
 >> +++ b/include/efi_api.h
 >> @@ -250,6 +250,8 @@ struct efi_rt_properties_table {
 >>  	u32 runtime_services_supported;
 >>  };
 >>
 >> +#define EFI_OPTIONAL_PTR	0x00000001
 >> +
 >>  struct efi_runtime_services {
 >>  	struct efi_table_hdr hdr;
 >>  	efi_status_t (EFIAPI *get_time)(struct efi_time *time,
 >> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
 >> index 67fa693e41..664a0422e2 100644
 >> --- a/lib/efi_loader/efi_runtime.c
 >> +++ b/lib/efi_loader/efi_runtime.c
 >> @@ -511,6 +511,12 @@ efi_convert_pointer(efi_uintn_t
debug_disposition, void **address)
 >>  		ret = EFI_INVALID_PARAMETER;
 >>  		goto out;
 >>  	}
 >> +	if (!*address) {
 >> +		if (debug_disposition & EFI_OPTIONAL_PTR)
 >> +			return EFI_SUCCESS;
 >> +		else
 >> +			return EFI_INVALID_PARAMETER;
 >> +	}
 >
 >In either case, NULL won't be converted.
 >Is this a behavior UEFI specification expects?
 >(I haven't found any description there.)
 >
 >-Takahiro Akashi


The spec has:

EFI_INVALID_PARAMETER - *Address is NULL and DebugDisposition does not
have the EFI_OPTIONAL_PTR bit set.

EFI_OPTIONAL_PTR is handled in the same way in EDK2. An optional pointer
is one that should not be converted if is NULL. It should stay NULL, so
that the firmware can detect that it is not set.

Best regards

Heinrich

 >
 >
 >>  	for (i = 0; i < efi_descriptor_count; i++) {
 >>  		struct efi_mem_desc *map = (void *)efi_virtmap +
 >> --
 >> 2.25.1
 >>

[PATCH 12/16] efi_loader: optional pointer for ConvertPointer

[AKASHI Takahiro]

On Tue, Mar 31, 2020 at 08:52:23AM +0200, Heinrich Schuchardt wrote:
> On March 31, 2020, 5:23 a.m. UTC Takahiro Akashi wrote
> >On Fri, Mar 27, 2020 at 06:27:56AM +0100, Heinrich Schuchardt wrote:
> >> If the EFI_OPTIONAL_PTR is set in DebugDisposition, a NULL pointer
> does not
> >> constitute an invalid parameter.
> >>
> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >> ---
> >>  include/efi_api.h            | 2 ++
> >>  lib/efi_loader/efi_runtime.c | 6 ++++++
> >>  2 files changed, 8 insertions(+)
> >>
> >> diff --git a/include/efi_api.h b/include/efi_api.h
> >> index 1c40ffc4f5..c56703fc5e 100644
> >> --- a/include/efi_api.h
> >> +++ b/include/efi_api.h
> >> @@ -250,6 +250,8 @@ struct efi_rt_properties_table {
> >>  	u32 runtime_services_supported;
> >>  };
> >>
> >> +#define EFI_OPTIONAL_PTR	0x00000001
> >> +
> >>  struct efi_runtime_services {
> >>  	struct efi_table_hdr hdr;
> >>  	efi_status_t (EFIAPI *get_time)(struct efi_time *time,
> >> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
> >> index 67fa693e41..664a0422e2 100644
> >> --- a/lib/efi_loader/efi_runtime.c
> >> +++ b/lib/efi_loader/efi_runtime.c
> >> @@ -511,6 +511,12 @@ efi_convert_pointer(efi_uintn_t
> debug_disposition, void **address)
> >>  		ret = EFI_INVALID_PARAMETER;
> >>  		goto out;
> >>  	}
> >> +	if (!*address) {
> >> +		if (debug_disposition & EFI_OPTIONAL_PTR)
> >> +			return EFI_SUCCESS;
> >> +		else
> >> +			return EFI_INVALID_PARAMETER;
> >> +	}
> >
> >In either case, NULL won't be converted.
> >Is this a behavior UEFI specification expects?
> >(I haven't found any description there.)
> >
> >-Takahiro Akashi
> 
> 
> The spec has:
> 
> EFI_INVALID_PARAMETER - *Address is NULL and DebugDisposition does not
> have the EFI_OPTIONAL_PTR bit set.

That's it.
There is no description about how NULL be handled if OPTIONAL_PTR.

> EFI_OPTIONAL_PTR is handled in the same way in EDK2. An optional pointer
> is one that should not be converted if is NULL. It should stay NULL, so
> that the firmware can detect that it is not set.

Even so, when you implement it based on your own *interpretation,*
you should describe it explicitly.

-Takahiro Akashi

> 
> Best regards
> 
> Heinrich
> 
> >
> >
> >>  	for (i = 0; i < efi_descriptor_count; i++) {
> >>  		struct efi_mem_desc *map = (void *)efi_virtmap +
> >> --
> >> 2.25.1
> >>

[PATCH 13/16] efi_loader: memory buffer for variables

[Heinrich Schuchardt]

Saving UEFI variable as encoded U-Boot environment variables does not allow
support at runtime.

Provide functions to manage a memory buffer with UEFI variables.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 include/efi_variable.h             |  16 ++
 lib/efi_loader/Makefile            |   1 +
 lib/efi_loader/efi_variables_mem.c | 317 +++++++++++++++++++++++++++++
 3 files changed, 334 insertions(+)
 create mode 100644 lib/efi_loader/efi_variables_mem.c

diff --git a/include/efi_variable.h b/include/efi_variable.h
index fb8294fc2e..037d268085 100644
--- a/include/efi_variable.h
+++ b/include/efi_variable.h
@@ -33,4 +33,20 @@ efi_status_t efi_var_to_file(void);

 efi_status_t efi_var_from_file(void);

+void efi_var_mem_memcpy(void *dest, const void *src, size_t n);
+
+efi_status_t efi_var_mem_init(void);
+
+struct efi_var_entry *efi_var_mem_find(const efi_guid_t *guid, const u16 *name,
+				       struct efi_var_entry **next);
+
+void efi_var_mem_del(struct efi_var_entry *var);
+
+efi_status_t efi_var_mem_ins(u16 *variable_name,
+			     const efi_guid_t *vendor, u32 attributes,
+			     const efi_uintn_t size1, const void *data1,
+			     const efi_uintn_t size2, const void *data2);
+
+u64 efi_var_mem_free(void);
+
 #endif
diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
index 621a767ab3..14b210e189 100644
--- a/lib/efi_loader/Makefile
+++ b/lib/efi_loader/Makefile
@@ -36,6 +36,7 @@ obj-y += efi_setup.o
 obj-$(CONFIG_EFI_UNICODE_COLLATION_PROTOCOL2) += efi_unicode_collation.o
 obj-y += efi_variable.o
 obj-y += efi_variables_file.o
+obj-y += efi_variables_mem.o
 obj-y += efi_watchdog.o
 obj-$(CONFIG_LCD) += efi_gop.o
 obj-$(CONFIG_DM_VIDEO) += efi_gop.o
diff --git a/lib/efi_loader/efi_variables_mem.c b/lib/efi_loader/efi_variables_mem.c
new file mode 100644
index 0000000000..f70cc65f8b
--- /dev/null
+++ b/lib/efi_loader/efi_variables_mem.c
@@ -0,0 +1,317 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * File interface for UEFI variables
+ *
+ * Copyright (c) 2020, Heinrich Schuchardt
+ */
+
+#include <common.h>
+#include <efi_loader.h>
+#include <efi_variable.h>
+#include <u-boot/crc.h>
+
+static struct efi_var_file __efi_runtime_data *efi_var_buf;
+static struct efi_var_entry __efi_runtime_data *efi_current_var;
+
+/**
+ * memcpy() - copy memory area
+ *
+ * At runtime memcpy() is not available.
+ *
+ * @dest:	destination buffer
+ * @src:	source buffer
+ * @n:		number of bytes to copy
+ * Return:	pointer to destination buffer
+ */
+void __efi_runtime efi_var_mem_memcpy(void *dest, const void *src, size_t n)
+{
+	u8 *d = dest;
+	const u8 *s = src;
+
+	for (; n; --n)
+		*d++ = *s++;
+}
+
+/**
+ * efi_var_mem_compare() - compare GUID and name with a variable
+ *
+ * @var:	variable to compare
+ * @guid:	GUID to compare
+ * @name:	variable name to compare
+ * @next:	pointer to next variable
+ * Return:	true if match
+ */
+static bool __efi_runtime
+efi_var_mem_compare(struct efi_var_entry *var, const efi_guid_t *guid,
+		    const u16 *name, struct efi_var_entry **next)
+{
+	int i;
+	u8 *guid1, *guid2;
+	const u16 *data, *var_name;
+	bool match = true;
+
+	for (guid1 = (u8 *)&var->guid, guid2 = (u8 *)guid, i = 0;
+	     i < sizeof(efi_guid_t) && match; ++i)
+		match = (guid1[i] == guid2[i]);
+
+	for (data = var->name, var_name = name;; ++data, ++var_name) {
+		if (match)
+			match = (*data == *var_name);
+		if (!*data)
+			break;
+	}
+
+	++data;
+
+	if (next)
+		*next = (struct efi_var_entry *)
+			ALIGN((uintptr_t)data + var->length, 8);
+
+	return match;
+}
+
+/**
+ * efi_var_mem_find() - find a variable in the list
+ *
+ * @guid:	GUID of the variable
+ * @name:	name of the variable
+ * @next:	on exit pointer to the next variable after the found one
+ * Return:	found variable
+ */
+struct efi_var_entry __efi_runtime
+*efi_var_mem_find(const efi_guid_t *guid, const u16 *name,
+		  struct efi_var_entry **next)
+{
+	struct efi_var_entry *var, *last;
+
+	last = (struct efi_var_entry *)
+	       ((uintptr_t)efi_var_buf + efi_var_buf->length);
+
+	if (!*name) {
+		if (next) {
+			*next = efi_var_buf->var;
+			if (*next >= last)
+				*next = NULL;
+		}
+		return NULL;
+	}
+	if (efi_current_var &&
+	    efi_var_mem_compare(efi_current_var, guid, name, next)) {
+		if (next && *next >= last)
+			*next = NULL;
+		return efi_current_var;
+	}
+
+	var = efi_var_buf->var;
+	if (var < last) {
+		for (; var;) {
+			struct efi_var_entry *pos;
+			bool match;
+
+			match = efi_var_mem_compare(var, guid, name, &pos);
+			if (pos >= last)
+				pos = NULL;
+			if (match) {
+				if (next)
+					*next = pos;
+				return var;
+			}
+			var = pos;
+		}
+	}
+	if (next)
+		*next = NULL;
+	return NULL;
+}
+
+/**
+ * efi_var_mem_del() - delete a variable from the list of variables
+ *
+ * @var:	variable to delete
+ */
+void __efi_runtime efi_var_mem_del(struct efi_var_entry *var)
+{
+	u16 *data = var->name;
+	struct efi_var_entry *next, *last;
+	u64 *from, *to;
+
+	if (!var)
+		return;
+
+	last = (struct efi_var_entry *)
+	       ((uintptr_t)efi_var_buf + efi_var_buf->length);
+	if (var < efi_current_var)
+		efi_current_var = NULL;
+
+	for (data = var->name; *data; ++data)
+		;
+	++data;
+	next = (struct efi_var_entry *)
+	       ALIGN((uintptr_t)data + var->length, 8);
+	efi_var_buf->length -= (uintptr_t)next - (uintptr_t)var;
+
+	for (to = (u64 *)var, from = (u64 *)next; from < (u64 *)last;
+	     ++to, ++from)
+		*to = *from;
+	efi_var_buf->crc32 = crc32(0, (u8 *)efi_var_buf->var,
+				   efi_var_buf->length -
+				   sizeof(struct efi_var_file));
+}
+
+/**
+ * efi_var_mem_ins() - append a variable to the list of variables
+ *
+ * The variable is appended without checking if a variable of the same name
+ * already exists. The two data buffers are concatenated.
+ *
+ * @name:	variable name
+ * @vendor:	GUID
+ * @attributes:	variable attributes
+ * @size1:	size of the first data buffer
+ * @data1:	first data buffer
+ * @size2:	size of the second data field
+ * @data2:	second data buffer
+ * Result:	status code
+ */
+efi_status_t __efi_runtime efi_var_mem_ins(
+				u16 *variable_name,
+				const efi_guid_t *vendor, u32 attributes,
+				const efi_uintn_t size1, const void *data1,
+				const efi_uintn_t size2, const void *data2)
+{
+	u16 *data;
+	struct efi_var_entry *var;
+	u32 var_name_len;
+
+	var = (struct efi_var_entry *)
+	      ((uintptr_t)efi_var_buf + efi_var_buf->length);
+	for (var_name_len = 0; variable_name[var_name_len]; ++var_name_len)
+		;
+	++var_name_len;
+	data = var->name + var_name_len;
+
+	if ((uintptr_t)data - (uintptr_t)efi_var_buf + size1 + size2 >
+	    EFI_VAR_BUF_SIZE)
+		return EFI_OUT_OF_RESOURCES;
+
+	var->attr = attributes;
+	var->length = size1 + size2;
+
+	efi_var_mem_memcpy(&var->guid, vendor, sizeof(efi_guid_t));
+	efi_var_mem_memcpy(var->name, variable_name,
+			   sizeof(u16) * var_name_len);
+	efi_var_mem_memcpy(data, data1, size1);
+	efi_var_mem_memcpy((u8 *)data + size1, data2, size2);
+
+	var = (struct efi_var_entry *)
+	      ALIGN((uintptr_t)data + var->length, 8);
+	efi_var_buf->length = (uintptr_t)var - (uintptr_t)efi_var_buf;
+	efi_var_buf->crc32 = crc32(0, (u8 *)efi_var_buf->var,
+				   efi_var_buf->length -
+				   sizeof(struct efi_var_file));
+
+	return EFI_SUCCESS;
+}
+
+/**
+ * efi_var_mem_free() - determine free memory for variables
+ *
+ * Return:	maximum data size plus variable name size
+ */
+u64 __efi_runtime efi_var_mem_free(void)
+{
+	return EFI_VAR_BUF_SIZE - efi_var_buf->length -
+	       sizeof(struct efi_var_entry);
+}
+
+/**
+ * efi_var_mem_bs_del() - delete boot service only variables
+ */
+static void efi_var_mem_bs_del(void)
+{
+	struct efi_var_entry *var = efi_var_buf->var;
+
+	for (;;) {
+		struct efi_var_entry *last;
+
+		last = (struct efi_var_entry *)
+		       ((uintptr_t)efi_var_buf + efi_var_buf->length);
+		if (var >= last)
+			break;
+		if (var->attr & EFI_VARIABLE_RUNTIME_ACCESS) {
+			u16 *data;
+
+			/* skip variable */
+			for (data = var->name; *data; ++data)
+				;
+			++data;
+			var = (struct efi_var_entry *)
+			      ALIGN((uintptr_t)data + var->length, 8);
+		} else {
+			/* delete variable */
+			efi_var_mem_del(var);
+		}
+	}
+}
+
+/**
+ * efi_var_mem_notify_exit_boot_services() - ExitBootService callback
+ *
+ * @event:	callback event
+ * @context:	callback context
+ */
+static void EFIAPI __efi_runtime
+efi_var_mem_notify_exit_boot_services(struct efi_event *event, void *context)
+{
+	/* Delete boot service only variables */
+	efi_var_mem_bs_del();
+}
+
+/**
+ * efi_var_mem_notify_exit_boot_services() - SetVirtualMemoryMap callback
+ *
+ * @event:	callback event
+ * @context:	callback context
+ */
+static void EFIAPI __efi_runtime
+efi_var_mem_notify_virtual_address_map(struct efi_event *event, void *context)
+{
+	efi_convert_pointer(0, (void **)&efi_var_buf);
+}
+
+/**
+ * efi_var_mem_init() - set-up variable list
+ *
+ * Return:	status code
+ */
+efi_status_t efi_var_mem_init(void)
+{
+	u64 memory;
+	efi_status_t ret;
+	struct efi_event *event;
+
+	ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES,
+				 EFI_RUNTIME_SERVICES_DATA,
+				 efi_size_in_pages(EFI_VAR_BUF_SIZE),
+				 &memory);
+	if (ret != EFI_SUCCESS)
+		return ret;
+	efi_var_buf = (struct efi_var_file *)(uintptr_t)memory;
+	memset(efi_var_buf, 0, EFI_VAR_BUF_SIZE);
+	efi_var_buf->magic = EFI_VAR_FILE_MAGIC;
+	efi_var_buf->length = (uintptr_t)efi_var_buf->var -
+			      (uintptr_t)efi_var_buf;
+	/* crc32 for 0 bytes = 0 */
+
+	ret = efi_create_event(EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_CALLBACK,
+			       efi_var_mem_notify_exit_boot_services, NULL,
+			       NULL, &event);
+	if (ret != EFI_SUCCESS)
+		return ret;
+	ret = efi_create_event(EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE, TPL_CALLBACK,
+			       efi_var_mem_notify_virtual_address_map, NULL,
+			       NULL, &event);
+	if (ret != EFI_SUCCESS)
+		return ret;
+	return ret;
+}
--
2.25.1

[PATCH 13/16] efi_loader: memory buffer for variables

[Punit Agrawal]

Heinrich Schuchardt <xypron.glpk@gmx.de> writes:

> Saving UEFI variable as encoded U-Boot environment variables does not allow
> support at runtime.
>
> Provide functions to manage a memory buffer with UEFI variables.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  include/efi_variable.h             |  16 ++
>  lib/efi_loader/Makefile            |   1 +
>  lib/efi_loader/efi_variables_mem.c | 317 +++++++++++++++++++++++++++++
>  3 files changed, 334 insertions(+)
>  create mode 100644 lib/efi_loader/efi_variables_mem.c

[...]

> diff --git a/lib/efi_loader/efi_variables_mem.c b/lib/efi_loader/efi_variables_mem.c
> new file mode 100644
> index 0000000000..f70cc65f8b
> --- /dev/null
> +++ b/lib/efi_loader/efi_variables_mem.c
> @@ -0,0 +1,317 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * File interface for UEFI variables
> + *
> + * Copyright (c) 2020, Heinrich Schuchardt
> + */
> +
> +#include <common.h>
> +#include <efi_loader.h>
> +#include <efi_variable.h>
> +#include <u-boot/crc.h>
> +
> +static struct efi_var_file __efi_runtime_data *efi_var_buf;
> +static struct efi_var_entry __efi_runtime_data *efi_current_var;
> +
> +/**
> + * memcpy() - copy memory area
> + *
> + * At runtime memcpy() is not available.
> + *
> + * @dest:	destination buffer
> + * @src:	source buffer
> + * @n:		number of bytes to copy
> + * Return:	pointer to destination buffer
> + */
> +void __efi_runtime efi_var_mem_memcpy(void *dest, const void *src, size_t n)
> +{
> +	u8 *d = dest;
> +	const u8 *s = src;
> +
> +	for (; n; --n)
> +		*d++ = *s++;
> +}
> +
> +/**
> + * efi_var_mem_compare() - compare GUID and name with a variable
> + *
> + * @var:	variable to compare
> + * @guid:	GUID to compare
> + * @name:	variable name to compare
> + * @next:	pointer to next variable
> + * Return:	true if match
> + */
> +static bool __efi_runtime
> +efi_var_mem_compare(struct efi_var_entry *var, const efi_guid_t *guid,
> +		    const u16 *name, struct efi_var_entry **next)
> +{
> +	int i;
> +	u8 *guid1, *guid2;
> +	const u16 *data, *var_name;
> +	bool match = true;
> +
> +	for (guid1 = (u8 *)&var->guid, guid2 = (u8 *)guid, i = 0;
> +	     i < sizeof(efi_guid_t) && match; ++i)
> +		match = (guid1[i] == guid2[i]);
> +
> +	for (data = var->name, var_name = name;; ++data, ++var_name) {
> +		if (match)
> +			match = (*data == *var_name);
> +		if (!*data)
> +			break;
> +	}

Don't roll out two different versions of memcmp() - make it a generic
helper and use it here.

> +
> +	++data;
> +
> +	if (next)
> +		*next = (struct efi_var_entry *)
> +			ALIGN((uintptr_t)data + var->length, 8);

Also, instead of implementing iteration via carrying the iterator
around, the readability of patches would be improved if this was done as
a simple loop outside of this function.

If for some reason, this is considered to be better, please add it as a
comment in your next version.

Thanks,
Punit

> +
> +	return match;
> +}
> +
> +/**
> + * efi_var_mem_find() - find a variable in the list
> + *
> + * @guid:	GUID of the variable
> + * @name:	name of the variable
> + * @next:	on exit pointer to the next variable after the found one
> + * Return:	found variable
> + */
> +struct efi_var_entry __efi_runtime
> +*efi_var_mem_find(const efi_guid_t *guid, const u16 *name,
> +		  struct efi_var_entry **next)
> +{
> +	struct efi_var_entry *var, *last;
> +
> +	last = (struct efi_var_entry *)
> +	       ((uintptr_t)efi_var_buf + efi_var_buf->length);
> +
> +	if (!*name) {
> +		if (next) {
> +			*next = efi_var_buf->var;
> +			if (*next >= last)
> +				*next = NULL;
> +		}
> +		return NULL;
> +	}
> +	if (efi_current_var &&
> +	    efi_var_mem_compare(efi_current_var, guid, name, next)) {
> +		if (next && *next >= last)
> +			*next = NULL;
> +		return efi_current_var;
> +	}
> +
> +	var = efi_var_buf->var;
> +	if (var < last) {
> +		for (; var;) {
> +			struct efi_var_entry *pos;
> +			bool match;
> +
> +			match = efi_var_mem_compare(var, guid, name, &pos);
> +			if (pos >= last)
> +				pos = NULL;
> +			if (match) {
> +				if (next)
> +					*next = pos;
> +				return var;
> +			}
> +			var = pos;
> +		}
> +	}
> +	if (next)
> +		*next = NULL;
> +	return NULL;
> +}
> +
> +/**
> + * efi_var_mem_del() - delete a variable from the list of variables
> + *
> + * @var:	variable to delete
> + */
> +void __efi_runtime efi_var_mem_del(struct efi_var_entry *var)
> +{
> +	u16 *data = var->name;
> +	struct efi_var_entry *next, *last;
> +	u64 *from, *to;
> +
> +	if (!var)
> +		return;
> +
> +	last = (struct efi_var_entry *)
> +	       ((uintptr_t)efi_var_buf + efi_var_buf->length);
> +	if (var < efi_current_var)
> +		efi_current_var = NULL;
> +
> +	for (data = var->name; *data; ++data)
> +		;
> +	++data;
> +	next = (struct efi_var_entry *)
> +	       ALIGN((uintptr_t)data + var->length, 8);
> +	efi_var_buf->length -= (uintptr_t)next - (uintptr_t)var;
> +
> +	for (to = (u64 *)var, from = (u64 *)next; from < (u64 *)last;
> +	     ++to, ++from)
> +		*to = *from;
> +	efi_var_buf->crc32 = crc32(0, (u8 *)efi_var_buf->var,
> +				   efi_var_buf->length -
> +				   sizeof(struct efi_var_file));
> +}
> +
> +/**
> + * efi_var_mem_ins() - append a variable to the list of variables
> + *
> + * The variable is appended without checking if a variable of the same name
> + * already exists. The two data buffers are concatenated.
> + *
> + * @name:	variable name
> + * @vendor:	GUID
> + * @attributes:	variable attributes
> + * @size1:	size of the first data buffer
> + * @data1:	first data buffer
> + * @size2:	size of the second data field
> + * @data2:	second data buffer
> + * Result:	status code
> + */
> +efi_status_t __efi_runtime efi_var_mem_ins(
> +				u16 *variable_name,
> +				const efi_guid_t *vendor, u32 attributes,
> +				const efi_uintn_t size1, const void *data1,
> +				const efi_uintn_t size2, const void *data2)
> +{
> +	u16 *data;
> +	struct efi_var_entry *var;
> +	u32 var_name_len;
> +
> +	var = (struct efi_var_entry *)
> +	      ((uintptr_t)efi_var_buf + efi_var_buf->length);
> +	for (var_name_len = 0; variable_name[var_name_len]; ++var_name_len)
> +		;
> +	++var_name_len;
> +	data = var->name + var_name_len;
> +
> +	if ((uintptr_t)data - (uintptr_t)efi_var_buf + size1 + size2 >
> +	    EFI_VAR_BUF_SIZE)
> +		return EFI_OUT_OF_RESOURCES;
> +
> +	var->attr = attributes;
> +	var->length = size1 + size2;
> +
> +	efi_var_mem_memcpy(&var->guid, vendor, sizeof(efi_guid_t));
> +	efi_var_mem_memcpy(var->name, variable_name,
> +			   sizeof(u16) * var_name_len);
> +	efi_var_mem_memcpy(data, data1, size1);
> +	efi_var_mem_memcpy((u8 *)data + size1, data2, size2);
> +
> +	var = (struct efi_var_entry *)
> +	      ALIGN((uintptr_t)data + var->length, 8);
> +	efi_var_buf->length = (uintptr_t)var - (uintptr_t)efi_var_buf;
> +	efi_var_buf->crc32 = crc32(0, (u8 *)efi_var_buf->var,
> +				   efi_var_buf->length -
> +				   sizeof(struct efi_var_file));
> +
> +	return EFI_SUCCESS;
> +}
> +
> +/**
> + * efi_var_mem_free() - determine free memory for variables
> + *
> + * Return:	maximum data size plus variable name size
> + */
> +u64 __efi_runtime efi_var_mem_free(void)
> +{
> +	return EFI_VAR_BUF_SIZE - efi_var_buf->length -
> +	       sizeof(struct efi_var_entry);
> +}
> +
> +/**
> + * efi_var_mem_bs_del() - delete boot service only variables
> + */
> +static void efi_var_mem_bs_del(void)
> +{
> +	struct efi_var_entry *var = efi_var_buf->var;
> +
> +	for (;;) {
> +		struct efi_var_entry *last;
> +
> +		last = (struct efi_var_entry *)
> +		       ((uintptr_t)efi_var_buf + efi_var_buf->length);
> +		if (var >= last)
> +			break;
> +		if (var->attr & EFI_VARIABLE_RUNTIME_ACCESS) {
> +			u16 *data;
> +
> +			/* skip variable */
> +			for (data = var->name; *data; ++data)
> +				;
> +			++data;
> +			var = (struct efi_var_entry *)
> +			      ALIGN((uintptr_t)data + var->length, 8);
> +		} else {
> +			/* delete variable */
> +			efi_var_mem_del(var);
> +		}
> +	}
> +}
> +
> +/**
> + * efi_var_mem_notify_exit_boot_services() - ExitBootService callback
> + *
> + * @event:	callback event
> + * @context:	callback context
> + */
> +static void EFIAPI __efi_runtime
> +efi_var_mem_notify_exit_boot_services(struct efi_event *event, void *context)
> +{
> +	/* Delete boot service only variables */
> +	efi_var_mem_bs_del();
> +}
> +
> +/**
> + * efi_var_mem_notify_exit_boot_services() - SetVirtualMemoryMap callback
> + *
> + * @event:	callback event
> + * @context:	callback context
> + */
> +static void EFIAPI __efi_runtime
> +efi_var_mem_notify_virtual_address_map(struct efi_event *event, void *context)
> +{
> +	efi_convert_pointer(0, (void **)&efi_var_buf);
> +}
> +
> +/**
> + * efi_var_mem_init() - set-up variable list
> + *
> + * Return:	status code
> + */
> +efi_status_t efi_var_mem_init(void)
> +{
> +	u64 memory;
> +	efi_status_t ret;
> +	struct efi_event *event;
> +
> +	ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES,
> +				 EFI_RUNTIME_SERVICES_DATA,
> +				 efi_size_in_pages(EFI_VAR_BUF_SIZE),
> +				 &memory);
> +	if (ret != EFI_SUCCESS)
> +		return ret;
> +	efi_var_buf = (struct efi_var_file *)(uintptr_t)memory;
> +	memset(efi_var_buf, 0, EFI_VAR_BUF_SIZE);
> +	efi_var_buf->magic = EFI_VAR_FILE_MAGIC;
> +	efi_var_buf->length = (uintptr_t)efi_var_buf->var -
> +			      (uintptr_t)efi_var_buf;
> +	/* crc32 for 0 bytes = 0 */
> +
> +	ret = efi_create_event(EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_CALLBACK,
> +			       efi_var_mem_notify_exit_boot_services, NULL,
> +			       NULL, &event);
> +	if (ret != EFI_SUCCESS)
> +		return ret;
> +	ret = efi_create_event(EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE, TPL_CALLBACK,
> +			       efi_var_mem_notify_virtual_address_map, NULL,
> +			       NULL, &event);
> +	if (ret != EFI_SUCCESS)
> +		return ret;
> +	return ret;
> +}
> --
> 2.25.1

[PATCH 13/16] efi_loader: memory buffer for variables

[Heinrich Schuchardt]

On 3/27/20 9:09 AM, Punit Agrawal wrote:
> Heinrich Schuchardt <xypron.glpk@gmx.de> writes:
>
>> Saving UEFI variable as encoded U-Boot environment variables does not allow
>> support at runtime.
>>
>> Provide functions to manage a memory buffer with UEFI variables.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>   include/efi_variable.h             |  16 ++
>>   lib/efi_loader/Makefile            |   1 +
>>   lib/efi_loader/efi_variables_mem.c | 317 +++++++++++++++++++++++++++++
>>   3 files changed, 334 insertions(+)
>>   create mode 100644 lib/efi_loader/efi_variables_mem.c
>
> [...]
>
>> diff --git a/lib/efi_loader/efi_variables_mem.c b/lib/efi_loader/efi_variables_mem.c
>> new file mode 100644
>> index 0000000000..f70cc65f8b
>> --- /dev/null
>> +++ b/lib/efi_loader/efi_variables_mem.c
>> @@ -0,0 +1,317 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * File interface for UEFI variables
>> + *
>> + * Copyright (c) 2020, Heinrich Schuchardt
>> + */
>> +
>> +#include <common.h>
>> +#include <efi_loader.h>
>> +#include <efi_variable.h>
>> +#include <u-boot/crc.h>
>> +
>> +static struct efi_var_file __efi_runtime_data *efi_var_buf;
>> +static struct efi_var_entry __efi_runtime_data *efi_current_var;
>> +
>> +/**
>> + * memcpy() - copy memory area
>> + *
>> + * At runtime memcpy() is not available.
>> + *
>> + * @dest:	destination buffer
>> + * @src:	source buffer
>> + * @n:		number of bytes to copy
>> + * Return:	pointer to destination buffer
>> + */
>> +void __efi_runtime efi_var_mem_memcpy(void *dest, const void *src, size_t n)
>> +{
>> +	u8 *d = dest;
>> +	const u8 *s = src;
>> +
>> +	for (; n; --n)
>> +		*d++ = *s++;
>> +}
>> +
>> +/**
>> + * efi_var_mem_compare() - compare GUID and name with a variable
>> + *
>> + * @var:	variable to compare
>> + * @guid:	GUID to compare
>> + * @name:	variable name to compare
>> + * @next:	pointer to next variable
>> + * Return:	true if match
>> + */
>> +static bool __efi_runtime
>> +efi_var_mem_compare(struct efi_var_entry *var, const efi_guid_t *guid,
>> +		    const u16 *name, struct efi_var_entry **next)
>> +{
>> +	int i;
>> +	u8 *guid1, *guid2;
>> +	const u16 *data, *var_name;
>> +	bool match = true;
>> +
>> +	for (guid1 = (u8 *)&var->guid, guid2 = (u8 *)guid, i = 0;
>> +	     i < sizeof(efi_guid_t) && match; ++i)
>> +		match = (guid1[i] == guid2[i]);
>> +
>> +	for (data = var->name, var_name = name;; ++data, ++var_name) {
>> +		if (match)
>> +			match = (*data == *var_name);
>> +		if (!*data)
>> +			break;
>> +	}
>
> Don't roll out two different versions of memcmp() - make it a generic
> helper and use it here.

memcmp() has many architecture specific implementations. Currently all
of these are gone at UEFI runtime.

The loop above does not only do comparison but also seeks the end of the
u16 string.

>
>> +
>> +	++data;
>> +
>> +	if (next)
>> +		*next = (struct efi_var_entry *)
>> +			ALIGN((uintptr_t)data + var->length, 8);
>
> Also, instead of implementing iteration via carrying the iterator
> around, the readability of patches would be improved if this was done as
> a simple loop outside of this function.
>
> If for some reason, this is considered to be better, please add it as a
> comment in your next version.

I could extract a function for comparing two u16 strings and return the
end of the first string if they match and NULL otherwise.

Would this match your expectations?

Best regards

Heinrich

>
> Thanks,
> Punit
>
>> +
>> +	return match;
>> +}
>> +
>> +/**
>> + * efi_var_mem_find() - find a variable in the list
>> + *
>> + * @guid:	GUID of the variable
>> + * @name:	name of the variable
>> + * @next:	on exit pointer to the next variable after the found one
>> + * Return:	found variable
>> + */
>> +struct efi_var_entry __efi_runtime
>> +*efi_var_mem_find(const efi_guid_t *guid, const u16 *name,
>> +		  struct efi_var_entry **next)
>> +{
>> +	struct efi_var_entry *var, *last;
>> +
>> +	last = (struct efi_var_entry *)
>> +	       ((uintptr_t)efi_var_buf + efi_var_buf->length);
>> +
>> +	if (!*name) {
>> +		if (next) {
>> +			*next = efi_var_buf->var;
>> +			if (*next >= last)
>> +				*next = NULL;
>> +		}
>> +		return NULL;
>> +	}
>> +	if (efi_current_var &&
>> +	    efi_var_mem_compare(efi_current_var, guid, name, next)) {
>> +		if (next && *next >= last)
>> +			*next = NULL;
>> +		return efi_current_var;
>> +	}
>> +
>> +	var = efi_var_buf->var;
>> +	if (var < last) {
>> +		for (; var;) {
>> +			struct efi_var_entry *pos;
>> +			bool match;
>> +
>> +			match = efi_var_mem_compare(var, guid, name, &pos);
>> +			if (pos >= last)
>> +				pos = NULL;
>> +			if (match) {
>> +				if (next)
>> +					*next = pos;
>> +				return var;
>> +			}
>> +			var = pos;
>> +		}
>> +	}
>> +	if (next)
>> +		*next = NULL;
>> +	return NULL;
>> +}
>> +
>> +/**
>> + * efi_var_mem_del() - delete a variable from the list of variables
>> + *
>> + * @var:	variable to delete
>> + */
>> +void __efi_runtime efi_var_mem_del(struct efi_var_entry *var)
>> +{
>> +	u16 *data = var->name;
>> +	struct efi_var_entry *next, *last;
>> +	u64 *from, *to;
>> +
>> +	if (!var)
>> +		return;
>> +
>> +	last = (struct efi_var_entry *)
>> +	       ((uintptr_t)efi_var_buf + efi_var_buf->length);
>> +	if (var < efi_current_var)
>> +		efi_current_var = NULL;
>> +
>> +	for (data = var->name; *data; ++data)
>> +		;
>> +	++data;
>> +	next = (struct efi_var_entry *)
>> +	       ALIGN((uintptr_t)data + var->length, 8);
>> +	efi_var_buf->length -= (uintptr_t)next - (uintptr_t)var;
>> +
>> +	for (to = (u64 *)var, from = (u64 *)next; from < (u64 *)last;
>> +	     ++to, ++from)
>> +		*to = *from;
>> +	efi_var_buf->crc32 = crc32(0, (u8 *)efi_var_buf->var,
>> +				   efi_var_buf->length -
>> +				   sizeof(struct efi_var_file));
>> +}
>> +
>> +/**
>> + * efi_var_mem_ins() - append a variable to the list of variables
>> + *
>> + * The variable is appended without checking if a variable of the same name
>> + * already exists. The two data buffers are concatenated.
>> + *
>> + * @name:	variable name
>> + * @vendor:	GUID
>> + * @attributes:	variable attributes
>> + * @size1:	size of the first data buffer
>> + * @data1:	first data buffer
>> + * @size2:	size of the second data field
>> + * @data2:	second data buffer
>> + * Result:	status code
>> + */
>> +efi_status_t __efi_runtime efi_var_mem_ins(
>> +				u16 *variable_name,
>> +				const efi_guid_t *vendor, u32 attributes,
>> +				const efi_uintn_t size1, const void *data1,
>> +				const efi_uintn_t size2, const void *data2)
>> +{
>> +	u16 *data;
>> +	struct efi_var_entry *var;
>> +	u32 var_name_len;
>> +
>> +	var = (struct efi_var_entry *)
>> +	      ((uintptr_t)efi_var_buf + efi_var_buf->length);
>> +	for (var_name_len = 0; variable_name[var_name_len]; ++var_name_len)
>> +		;
>> +	++var_name_len;
>> +	data = var->name + var_name_len;
>> +
>> +	if ((uintptr_t)data - (uintptr_t)efi_var_buf + size1 + size2 >
>> +	    EFI_VAR_BUF_SIZE)
>> +		return EFI_OUT_OF_RESOURCES;
>> +
>> +	var->attr = attributes;
>> +	var->length = size1 + size2;
>> +
>> +	efi_var_mem_memcpy(&var->guid, vendor, sizeof(efi_guid_t));
>> +	efi_var_mem_memcpy(var->name, variable_name,
>> +			   sizeof(u16) * var_name_len);
>> +	efi_var_mem_memcpy(data, data1, size1);
>> +	efi_var_mem_memcpy((u8 *)data + size1, data2, size2);
>> +
>> +	var = (struct efi_var_entry *)
>> +	      ALIGN((uintptr_t)data + var->length, 8);
>> +	efi_var_buf->length = (uintptr_t)var - (uintptr_t)efi_var_buf;
>> +	efi_var_buf->crc32 = crc32(0, (u8 *)efi_var_buf->var,
>> +				   efi_var_buf->length -
>> +				   sizeof(struct efi_var_file));
>> +
>> +	return EFI_SUCCESS;
>> +}
>> +
>> +/**
>> + * efi_var_mem_free() - determine free memory for variables
>> + *
>> + * Return:	maximum data size plus variable name size
>> + */
>> +u64 __efi_runtime efi_var_mem_free(void)
>> +{
>> +	return EFI_VAR_BUF_SIZE - efi_var_buf->length -
>> +	       sizeof(struct efi_var_entry);
>> +}
>> +
>> +/**
>> + * efi_var_mem_bs_del() - delete boot service only variables
>> + */
>> +static void efi_var_mem_bs_del(void)
>> +{
>> +	struct efi_var_entry *var = efi_var_buf->var;
>> +
>> +	for (;;) {
>> +		struct efi_var_entry *last;
>> +
>> +		last = (struct efi_var_entry *)
>> +		       ((uintptr_t)efi_var_buf + efi_var_buf->length);
>> +		if (var >= last)
>> +			break;
>> +		if (var->attr & EFI_VARIABLE_RUNTIME_ACCESS) {
>> +			u16 *data;
>> +
>> +			/* skip variable */
>> +			for (data = var->name; *data; ++data)
>> +				;
>> +			++data;
>> +			var = (struct efi_var_entry *)
>> +			      ALIGN((uintptr_t)data + var->length, 8);
>> +		} else {
>> +			/* delete variable */
>> +			efi_var_mem_del(var);
>> +		}
>> +	}
>> +}
>> +
>> +/**
>> + * efi_var_mem_notify_exit_boot_services() - ExitBootService callback
>> + *
>> + * @event:	callback event
>> + * @context:	callback context
>> + */
>> +static void EFIAPI __efi_runtime
>> +efi_var_mem_notify_exit_boot_services(struct efi_event *event, void *context)
>> +{
>> +	/* Delete boot service only variables */
>> +	efi_var_mem_bs_del();
>> +}
>> +
>> +/**
>> + * efi_var_mem_notify_exit_boot_services() - SetVirtualMemoryMap callback
>> + *
>> + * @event:	callback event
>> + * @context:	callback context
>> + */
>> +static void EFIAPI __efi_runtime
>> +efi_var_mem_notify_virtual_address_map(struct efi_event *event, void *context)
>> +{
>> +	efi_convert_pointer(0, (void **)&efi_var_buf);
>> +}
>> +
>> +/**
>> + * efi_var_mem_init() - set-up variable list
>> + *
>> + * Return:	status code
>> + */
>> +efi_status_t efi_var_mem_init(void)
>> +{
>> +	u64 memory;
>> +	efi_status_t ret;
>> +	struct efi_event *event;
>> +
>> +	ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES,
>> +				 EFI_RUNTIME_SERVICES_DATA,
>> +				 efi_size_in_pages(EFI_VAR_BUF_SIZE),
>> +				 &memory);
>> +	if (ret != EFI_SUCCESS)
>> +		return ret;
>> +	efi_var_buf = (struct efi_var_file *)(uintptr_t)memory;
>> +	memset(efi_var_buf, 0, EFI_VAR_BUF_SIZE);
>> +	efi_var_buf->magic = EFI_VAR_FILE_MAGIC;
>> +	efi_var_buf->length = (uintptr_t)efi_var_buf->var -
>> +			      (uintptr_t)efi_var_buf;
>> +	/* crc32 for 0 bytes = 0 */
>> +
>> +	ret = efi_create_event(EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_CALLBACK,
>> +			       efi_var_mem_notify_exit_boot_services, NULL,
>> +			       NULL, &event);
>> +	if (ret != EFI_SUCCESS)
>> +		return ret;
>> +	ret = efi_create_event(EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE, TPL_CALLBACK,
>> +			       efi_var_mem_notify_virtual_address_map, NULL,
>> +			       NULL, &event);
>> +	if (ret != EFI_SUCCESS)
>> +		return ret;
>> +	return ret;
>> +}
>> --
>> 2.25.1

[PATCH 13/16] efi_loader: memory buffer for variables

[Punit Agrawal]

Heinrich Schuchardt <xypron.glpk@gmx.de> writes:

> On 3/27/20 9:09 AM, Punit Agrawal wrote:
>> Heinrich Schuchardt <xypron.glpk@gmx.de> writes:
>>
>>> Saving UEFI variable as encoded U-Boot environment variables does not allow
>>> support at runtime.
>>>
>>> Provide functions to manage a memory buffer with UEFI variables.
>>>
>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>> ---
>>>   include/efi_variable.h             |  16 ++
>>>   lib/efi_loader/Makefile            |   1 +
>>>   lib/efi_loader/efi_variables_mem.c | 317 +++++++++++++++++++++++++++++
>>>   3 files changed, 334 insertions(+)
>>>   create mode 100644 lib/efi_loader/efi_variables_mem.c
>>
>> [...]
>>
>>> diff --git a/lib/efi_loader/efi_variables_mem.c b/lib/efi_loader/efi_variables_mem.c
>>> new file mode 100644
>>> index 0000000000..f70cc65f8b
>>> --- /dev/null
>>> +++ b/lib/efi_loader/efi_variables_mem.c
>>> @@ -0,0 +1,317 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * File interface for UEFI variables
>>> + *
>>> + * Copyright (c) 2020, Heinrich Schuchardt
>>> + */
>>> +
>>> +#include <common.h>
>>> +#include <efi_loader.h>
>>> +#include <efi_variable.h>
>>> +#include <u-boot/crc.h>
>>> +
>>> +static struct efi_var_file __efi_runtime_data *efi_var_buf;
>>> +static struct efi_var_entry __efi_runtime_data *efi_current_var;
>>> +
>>> +/**
>>> + * memcpy() - copy memory area
>>> + *
>>> + * At runtime memcpy() is not available.
>>> + *
>>> + * @dest:	destination buffer
>>> + * @src:	source buffer
>>> + * @n:		number of bytes to copy
>>> + * Return:	pointer to destination buffer
>>> + */
>>> +void __efi_runtime efi_var_mem_memcpy(void *dest, const void *src, size_t n)
>>> +{
>>> +	u8 *d = dest;
>>> +	const u8 *s = src;
>>> +
>>> +	for (; n; --n)
>>> +		*d++ = *s++;
>>> +}
>>> +
>>> +/**
>>> + * efi_var_mem_compare() - compare GUID and name with a variable
>>> + *
>>> + * @var:	variable to compare
>>> + * @guid:	GUID to compare
>>> + * @name:	variable name to compare
>>> + * @next:	pointer to next variable
>>> + * Return:	true if match
>>> + */
>>> +static bool __efi_runtime
>>> +efi_var_mem_compare(struct efi_var_entry *var, const efi_guid_t *guid,
>>> +		    const u16 *name, struct efi_var_entry **next)
>>> +{
>>> +	int i;
>>> +	u8 *guid1, *guid2;
>>> +	const u16 *data, *var_name;
>>> +	bool match = true;
>>> +
>>> +	for (guid1 = (u8 *)&var->guid, guid2 = (u8 *)guid, i = 0;
>>> +	     i < sizeof(efi_guid_t) && match; ++i)
>>> +		match = (guid1[i] == guid2[i]);
>>> +
>>> +	for (data = var->name, var_name = name;; ++data, ++var_name) {
>>> +		if (match)
>>> +			match = (*data == *var_name);
>>> +		if (!*data)
>>> +			break;
>>> +	}
>>
>> Don't roll out two different versions of memcmp() - make it a generic
>> helper and use it here.
>
> memcmp() has many architecture specific implementations. Currently all
> of these are gone at UEFI runtime.
>
> The loop above does not only do comparison but also seeks the end of the
> u16 string.

... which is needed for iteration over the variables. Thanks for
pointing this out.

One way to avoid the complexity would be keep track of the variable
length in efi_var_entry. Same applies for the "value" as well. That ways
you can skip to the next variable without having to increment two bytes
at a time.

>>
>>> +
>>> +	++data;
>>> +
>>> +	if (next)
>>> +		*next = (struct efi_var_entry *)
>>> +			ALIGN((uintptr_t)data + var->length, 8);
>>
>> Also, instead of implementing iteration via carrying the iterator
>> around, the readability of patches would be improved if this was done as
>> a simple loop outside of this function.
>>
>> If for some reason, this is considered to be better, please add it as a
>> comment in your next version.
>
> I could extract a function for comparing two u16 strings and return the
> end of the first string if they match and NULL otherwise.
>
> Would this match your expectations?

Going by the function name, I was expecting something along the lines of
-

    return !memcmp(guid1, guid2, 16) && !memcmp(var->name, name, var->length);
    
depending on whether length is tracking bytes used.

Hope that makes sense.

Punit

[...]

[PATCH 14/16] efi_loader: use memory based variable storage

[Heinrich Schuchardt]

Saving UEFI variable as encoded U-Boot environment variables does not allow
implement run-time support.

Use a memory buffer for storing UEFI variables.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_variable.c      | 584 +++++++----------------------
 lib/efi_loader/efi_variables_mem.c |   7 +
 2 files changed, 147 insertions(+), 444 deletions(-)

diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
index 952a0a0db7..7c39542968 100644
--- a/lib/efi_loader/efi_variable.c
+++ b/lib/efi_loader/efi_variable.c
@@ -2,152 +2,15 @@
 /*
  * UEFI runtime variable services
  *
- * Copyright (c) 2017 Rob Clark
+ * Copyright (c) 2019 Heinrich Schuchardt
  */

 #include <common.h>
 #include <efi_loader.h>
 #include <efi_variable.h>
-#include <env_internal.h>
-#include <hexdump.h>
-#include <malloc.h>
-#include <search.h>
-#include <u-boot/crc.h>

 #define READ_ONLY BIT(31)

-/*
- * Mapping between EFI variables and u-boot variables:
- *
- *   efi_$guid_$varname = {attributes}(type)value
- *
- * For example:
- *
- *   efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported=
- *      "{ro,boot,run}(blob)0000000000000000"
- *   efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_BootOrder=
- *      "(blob)00010000"
- *
- * The attributes are a comma separated list of these possible
- * attributes:
- *
- *   + ro   - read-only
- *   + boot - boot-services access
- *   + run  - runtime access
- *
- * NOTE: with current implementation, no variables are available after
- * ExitBootServices, and all are persisted (if possible).
- *
- * If not specified, the attributes default to "{boot}".
- *
- * The required type is one of:
- *
- *   + utf8 - raw utf8 string
- *   + blob - arbitrary length hex string
- *
- * Maybe a utf16 type would be useful to for a string value to be auto
- * converted to utf16?
- */
-
-#define PREFIX_LEN (strlen("efi_xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx_"))
-
-/**
- * efi_to_native() - convert the UEFI variable name and vendor GUID to U-Boot
- *		     variable name
- *
- * The U-Boot variable name is a concatenation of prefix 'efi', the hexstring
- * encoded vendor GUID, and the UTF-8 encoded UEFI variable name separated by
- * underscores, e.g. 'efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_BootOrder'.
- *
- * @native:		pointer to pointer to U-Boot variable name
- * @variable_name:	UEFI variable name
- * @vendor:		vendor GUID
- * Return:		status code
- */
-static efi_status_t efi_to_native(char **native, const u16 *variable_name,
-				  const efi_guid_t *vendor)
-{
-	size_t len;
-	char *pos;
-
-	len = PREFIX_LEN + utf16_utf8_strlen(variable_name) + 1;
-	*native = malloc(len);
-	if (!*native)
-		return EFI_OUT_OF_RESOURCES;
-
-	pos = *native;
-	pos += sprintf(pos, "efi_%pUl_", vendor);
-	utf16_utf8_strcpy(&pos, variable_name);
-
-	return EFI_SUCCESS;
-}
-
-/**
- * prefix() - skip over prefix
- *
- * Skip over a prefix string.
- *
- * @str:	string with prefix
- * @prefix:	prefix string
- * Return:	string without prefix, or NULL if prefix not found
- */
-static const char *prefix(const char *str, const char *prefix)
-{
-	size_t n = strlen(prefix);
-	if (!strncmp(prefix, str, n))
-		return str + n;
-	return NULL;
-}
-
-/**
- * parse_attr() - decode attributes part of variable value
- *
- * Convert the string encoded attributes of a UEFI variable to a bit mask.
- * TODO: Several attributes are not supported.
- *
- * @str:	value of U-Boot variable
- * @attrp:	pointer to UEFI attributes
- * Return:	pointer to remainder of U-Boot variable value
- */
-static const char *parse_attr(const char *str, u32 *attrp)
-{
-	u32 attr = 0;
-	char sep = '{';
-
-	if (*str != '{') {
-		*attrp = EFI_VARIABLE_BOOTSERVICE_ACCESS;
-		return str;
-	}
-
-	while (*str == sep) {
-		const char *s;
-
-		str++;
-
-		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"))) {
-			attr |= EFI_VARIABLE_RUNTIME_ACCESS;
-		} else {
-			printf("invalid attribute: %s\n", str);
-			break;
-		}
-
-		str = s;
-		sep = ',';
-	}
-
-	str++;
-
-	*attrp = attr;
-
-	return str;
-}
-
 /**
  * efi_get_variable() - retrieve value of a UEFI variable
  *
@@ -167,11 +30,18 @@ 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);

-	return EFI_EXIT(efi_get_variable_int(variable_name, vendor, attributes,
-					     data_size, data));
+	ret = efi_get_variable_int(variable_name, vendor, attributes, data_size,
+				   data);
+	/* Remove read-only bit */
+	if (attributes)
+		*attributes &= EFI_VARIABLE_MASK;
+
+	return EFI_EXIT(ret);
 }

 /**
@@ -187,152 +57,37 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
  * @data:		buffer to which the variable value is copied
  * Return:		status code
  */
-efi_status_t efi_get_variable_int(u16 *variable_name,
-				  const efi_guid_t *vendor, u32 *attributes,
-				  efi_uintn_t *data_size, void *data)
+efi_status_t __efi_runtime
+efi_get_variable_int(u16 *variable_name, const efi_guid_t *vendor,
+		     u32 *attributes, efi_uintn_t *data_size, void *data)
 {
-	char *native_name;
-	efi_status_t ret;
-	unsigned long in_size;
-	const char *val, *s;
-	u32 attr;
-
+	efi_uintn_t old_size;
+	struct efi_var_entry *var;
+	u16 *pdata;

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

-	ret = efi_to_native(&native_name, variable_name, vendor);
-	if (ret)
-		return ret;
-
-	EFI_PRINT("get '%s'\n", native_name);
-
-	val = env_get(native_name);
-	free(native_name);
-	if (!val)
+	var = efi_var_mem_find(vendor, variable_name, NULL);
+	if (!var)
 		return EFI_NOT_FOUND;

-	val = parse_attr(val, &attr);
-
-	in_size = *data_size;
-
-	if ((s = prefix(val, "(blob)"))) {
-		size_t len = strlen(s);
-
-		/* number of hexadecimal digits must be even */
-		if (len & 1)
-			return EFI_DEVICE_ERROR;
-
-		/* two characters per byte: */
-		len /= 2;
-		*data_size = len;
-
-		if (in_size < len) {
-			ret = EFI_BUFFER_TOO_SMALL;
-			goto out;
-		}
-
-		if (!data)
-			return EFI_INVALID_PARAMETER;
-
-		if (hex2bin(data, s, len))
-			return EFI_DEVICE_ERROR;
-
-		EFI_PRINT("got value: \"%s\"\n", s);
-	} else if ((s = prefix(val, "(utf8)"))) {
-		unsigned len = strlen(s) + 1;
-
-		*data_size = len;
-
-		if (in_size < len) {
-			ret = EFI_BUFFER_TOO_SMALL;
-			goto out;
-		}
-
-		if (!data)
-			return EFI_INVALID_PARAMETER;
-
-		memcpy(data, s, len);
-		((char *)data)[len] = '\0';
-
-		EFI_PRINT("got value: \"%s\"\n", (char *)data);
-	} else {
-		EFI_PRINT("invalid value: '%s'\n", val);
-		return EFI_DEVICE_ERROR;
-	}
-
-out:
 	if (attributes)
-		*attributes = attr & EFI_VARIABLE_MASK;
+		*attributes = var->attr;

-	return ret;
-}
-
-static char *efi_variables_list;
-static char *efi_cur_variable;
-
-/**
- * parse_uboot_variable() - parse a u-boot variable and get uefi-related
- *			    information
- * @variable:		whole data of u-boot variable (ie. name=value)
- * @variable_name_size: size of variable_name buffer in byte
- * @variable_name:	name of uefi variable in u16, null-terminated
- * @vendor:		vendor's guid
- * @attributes:		attributes
- *
- * A uefi variable is encoded into a u-boot variable as described above.
- * This function parses such a u-boot variable and retrieve uefi-related
- * information into respective parameters. In return, variable_name_size
- * is the size of variable name including NULL.
- *
- * Return:		EFI_SUCCESS if parsing is OK, EFI_NOT_FOUND when
- *			the entire variable list has been returned,
- *			otherwise non-zero status code
- */
-static efi_status_t parse_uboot_variable(char *variable,
-					 efi_uintn_t *variable_name_size,
-					 u16 *variable_name,
-					 const efi_guid_t *vendor,
-					 u32 *attributes)
-{
-	char *guid, *name, *end, c;
-	size_t name_len;
-	efi_uintn_t old_variable_name_size;
-	u16 *p;
-
-	guid = strchr(variable, '_');
-	if (!guid)
-		return EFI_INVALID_PARAMETER;
-	guid++;
-	name = strchr(guid, '_');
-	if (!name)
-		return EFI_INVALID_PARAMETER;
-	name++;
-	end = strchr(name, '=');
-	if (!end)
-		return EFI_INVALID_PARAMETER;
-
-	name_len = end - name;
-	old_variable_name_size = *variable_name_size;
-	*variable_name_size = sizeof(u16) * (name_len + 1);
-	if (old_variable_name_size < *variable_name_size)
+	old_size = *data_size;
+	*data_size = var->length;
+	if (old_size < var->length)
 		return EFI_BUFFER_TOO_SMALL;

-	end++; /* point to value */
-
-	/* variable name */
-	p = variable_name;
-	utf8_utf16_strncpy(&p, name, name_len);
-	variable_name[name_len] = 0;
+	if (!data)
+		return EFI_INVALID_PARAMETER;

-	/* guid */
-	c = *(name - 1);
-	*(name - 1) = '\0'; /* guid need be null-terminated here */
-	uuid_str_to_bin(guid, (unsigned char *)vendor, UUID_STR_FORMAT_GUID);
-	*(name - 1) = c;
+	for (pdata = var->name; *pdata; ++pdata)
+		;
+	++pdata;

-	/* attributes */
-	parse_attr(end, attributes);
+	efi_var_mem_memcpy(data, pdata, var->length);

 	return EFI_SUCCESS;
 }
@@ -378,78 +133,38 @@ efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size,
  *
  * Return: status code
  */
-efi_status_t efi_get_next_variable_name_int(efi_uintn_t *variable_name_size,
-					    u16 *variable_name,
-					    efi_guid_t *vendor)
+efi_status_t __efi_runtime
+efi_get_next_variable_name_int(efi_uintn_t *variable_name_size,
+			       u16 *variable_name, efi_guid_t *vendor)
 {
-	char *native_name, *variable;
-	ssize_t name_len, list_len;
-	char regex[256];
-	char * const regexlist[] = {regex};
-	u32 attributes;
-	int i;
-	efi_status_t ret;
+	struct efi_var_entry *var;
+	efi_uintn_t old_size;
+	u16 *pdata;

 	if (!variable_name_size || !variable_name || !vendor)
 		return EFI_INVALID_PARAMETER;

-	if (variable_name[0]) {
-		/* check null-terminated string */
-		for (i = 0; i < *variable_name_size; i++)
-			if (!variable_name[i])
-				break;
-		if (i >= *variable_name_size)
-			return EFI_INVALID_PARAMETER;
-
-		/* search for the last-returned variable */
-		ret = efi_to_native(&native_name, variable_name, vendor);
-		if (ret)
-			return ret;
-
-		name_len = strlen(native_name);
-		for (variable = efi_variables_list; variable && *variable;) {
-			if (!strncmp(variable, native_name, name_len) &&
-			    variable[name_len] == '=')
-				break;
+	if (!variable_name[0])
+		var = NULL;

-			variable = strchr(variable, '\n');
-			if (variable)
-				variable++;
-		}
+	efi_var_mem_find(vendor, variable_name, &var);
+	if (!var)
+		return EFI_NOT_FOUND;

-		free(native_name);
-		if (!(variable && *variable))
-			return EFI_INVALID_PARAMETER;
+	for (pdata = var->name; *pdata; ++pdata)
+		;
+	++pdata;

-		/* next variable */
-		variable = strchr(variable, '\n');
-		if (variable)
-			variable++;
-		if (!(variable && *variable))
-			return EFI_NOT_FOUND;
-	} else {
-		/*
-		 *new search: free a list used in the previous search
-		 */
-		free(efi_variables_list);
-		efi_variables_list = NULL;
-		efi_cur_variable = NULL;
-
-		snprintf(regex, 256, "efi_.*-.*-.*-.*-.*_.*");
-		list_len = hexport_r(&env_htab, '\n',
-				     H_MATCH_REGEX | H_MATCH_KEY,
-				     &efi_variables_list, 0, 1, regexlist);
-		/* 1 indicates that no match was found */
-		if (list_len <= 1)
-			return EFI_NOT_FOUND;
+	old_size = *variable_name_size;
+	*variable_name_size = (uintptr_t)pdata - (uintptr_t)var->name;

-		variable = efi_variables_list;
-	}
+	if (old_size < *variable_name_size)
+		return EFI_BUFFER_TOO_SMALL;

-	ret = parse_uboot_variable(variable, variable_name_size, variable_name,
-				   vendor, &attributes);
+	efi_var_mem_memcpy(variable_name, var->name, *variable_name_size);
+	efi_var_mem_memcpy(vendor, &var->guid, sizeof(efi_guid_t));

-	return ret;
+	return EFI_SUCCESS;
 }

 /**
@@ -471,17 +186,26 @@ 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);

-	return EFI_EXIT(efi_set_variable_int(variable_name, vendor, attributes,
-					     data_size, data));
+	ret = efi_set_variable_int(variable_name, vendor, attributes,
+					    data_size, data);
+
+	/* Write non-volatile EFI variables to file */
+	if (ret == EFI_SUCCESS && (attributes & EFI_VARIABLE_NON_VOLATILE) &&
+	    efi_obj_list_initialized == EFI_SUCCESS)
+		efi_var_to_file();
+
+	return EFI_EXIT(ret);
 }

 /**
- * efi_set_variable_int() - set value of a UEFI variable internal
+ * efi_set_variable_rt_int() - set value of a UEFI variable internal
  *
- * This function can be used to set a UEFI variable without using EFI_CALL().
+ * This implements SetVariable() without persistence.
  *
  * @variable_name:	name of the variable
  * @vendor:		vendor GUID
@@ -490,130 +214,91 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
  * @data:		buffer with the variable value
  * Return:		status code
  */
-efi_status_t efi_set_variable_int(u16 *variable_name,
-				  const efi_guid_t *vendor, u32 attributes,
-				  efi_uintn_t data_size, const void *data)
+static efi_status_t __efi_runtime
+efi_set_variable_rt_int(u16 *variable_name, const efi_guid_t *vendor,
+			u32 attributes, efi_uintn_t data_size, const void *data)
 {
-	char *native_name = NULL, *val = NULL, *s;
-	const char *old_val;
-	size_t old_size;
-	efi_status_t ret = EFI_SUCCESS;
-	u32 attr;
-
+	struct efi_var_entry *var;
+	efi_uintn_t ret;
+	bool append;

 	if (!variable_name || !*variable_name || !vendor ||
 	    ((attributes & EFI_VARIABLE_RUNTIME_ACCESS) &&
 	     !(attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS))) {
-		ret = EFI_INVALID_PARAMETER;
-		goto out;
+		return EFI_INVALID_PARAMETER;
 	}

-	ret = efi_to_native(&native_name, variable_name, vendor);
-	if (ret)
-		goto out;
-
-	old_val = env_get(native_name);
-	if (old_val) {
-		old_val = parse_attr(old_val, &attr);
-
-		/* check read-only first */
-		if (attr & READ_ONLY) {
-			ret = EFI_WRITE_PROTECTED;
-			goto out;
-		}
-
-		if ((data_size == 0 &&
-		     !(attributes & EFI_VARIABLE_APPEND_WRITE)) ||
-		    !attributes) {
-			/* delete the variable: */
-			env_set(native_name, NULL);
-			ret = EFI_SUCCESS;
-			goto out;
-		}
-
-		/* attributes won't be changed */
-		if (attr != (attributes & ~EFI_VARIABLE_APPEND_WRITE)) {
-			ret = EFI_INVALID_PARAMETER;
+	var = efi_var_mem_find(vendor, variable_name, NULL);
+	append = attributes & EFI_VARIABLE_APPEND_WRITE;
+	attributes &= ~EFI_VARIABLE_APPEND_WRITE;
+
+	/*
+	 * TODO:
+	 * * authentication
+	 * * checks for EFI_VARIABLE_HARDWARE_ERROR_RECORD
+	 */
+	if (var) {
+		if (attributes && var->attr != attributes)
+			return EFI_INVALID_PARAMETER;
+		if (var->attr & READ_ONLY)
+			return EFI_WRITE_PROTECTED;
+		if ((!attributes || !data_size) && !append) {
+			attributes = var->attr;
 			goto out;
 		}
-
-		if (attributes & EFI_VARIABLE_APPEND_WRITE) {
-			if (!prefix(old_val, "(blob)")) {
-				ret = EFI_DEVICE_ERROR;
-				goto out;
-			}
-			old_size = strlen(old_val);
-		} else {
-			old_size = 0;
-		}
 	} else {
-		if (data_size == 0 || !attributes ||
-		    (attributes & EFI_VARIABLE_APPEND_WRITE)) {
-			/*
-			 * Trying to delete or to update a non-existent
-			 * variable.
-			 */
-			ret = EFI_NOT_FOUND;
-			goto out;
-		}
-
-		old_size = 0;
+		if (!attributes || !data_size || append)
+			return EFI_NOT_FOUND;
 	}

-	val = malloc(old_size + 2 * data_size
-		     + strlen("{ro,run,boot,nv}(blob)") + 1);
-	if (!val) {
-		ret = EFI_OUT_OF_RESOURCES;
-		goto out;
-	}
+	if (append) {
+		u16 *old_data = var->name;
+
+		for (; *old_data; ++old_data)
+			;
+		++old_data;

-	s = val;
-
-	/* store attributes */
-	attributes &= (EFI_VARIABLE_NON_VOLATILE |
-		       EFI_VARIABLE_BOOTSERVICE_ACCESS |
-		       EFI_VARIABLE_RUNTIME_ACCESS);
-	s += sprintf(s, "{");
-	for (u32 attr_rem = attributes; attr_rem;) {
-		u32 attr = 1 << (ffs(attr_rem) - 1);
-
-		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");
-
-		attr_rem &= ~attr;
-		if (attr_rem)
-			s += sprintf(s, ",");
+		ret = efi_var_mem_ins(variable_name, vendor, attributes,
+				      var->length, old_data, data_size, data);
+	} else {
+		ret = efi_var_mem_ins(variable_name, vendor, attributes,
+				      data_size, data, 0, NULL);
 	}
-	s += sprintf(s, "}");
+	if (ret != EFI_SUCCESS)
+		return ret;

-	if (old_size)
-		/* APPEND_WRITE */
-		s += sprintf(s, old_val);
-	else
-		s += sprintf(s, "(blob)");
+out:
+	efi_var_mem_del(var);

-	/* store payload: */
-	s = bin2hex(s, data, data_size);
-	*s = '\0';
+	return EFI_SUCCESS;
+}

-	EFI_PRINT("setting: %s=%s\n", native_name, val);
+/**
+ * efi_set_variable_int() - set value of a UEFI variable internal
+ *
+ * This function can be used to set a UEFI variable without using EFI_CALL().
+ *
+ * @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
+efi_set_variable_int(u16 *variable_name, const efi_guid_t *vendor,
+		     u32 attributes, efi_uintn_t data_size, const void *data)
+{
+	efi_status_t ret;

-	if (env_set(native_name, val))
-		ret = EFI_DEVICE_ERROR;
+	ret = efi_set_variable_rt_int(variable_name, vendor, attributes,
+				      data_size, data);

 	/* Write non-volatile EFI variables to file */
-	if (attributes && EFI_VARIABLE_NON_VOLATILE &&
-	    ret == EFI_SUCCESS && efi_obj_list_initialized == EFI_SUCCESS)
+	if (ret == EFI_SUCCESS && (attributes & EFI_VARIABLE_NON_VOLATILE) &&
+	    efi_obj_list_initialized == EFI_SUCCESS)
 		efi_var_to_file();

-out:
-	free(native_name);
-	free(val);
-
 	return ret;
 }

@@ -629,7 +314,7 @@ out:
  *					queried
  * @maximum_variable_storage_size:	maximum size of storage area for the
  *					selected variable types
- * @remaining_variable_storage_size:	remaining size of storage are for the
+ * @remaining_variable_storage_size:	remaining size of storage for the
  *					selected variable types
  * @maximum_variable_size:		maximum size of a variable of the
  *					selected type
@@ -641,6 +326,14 @@ efi_status_t __efi_runtime EFIAPI efi_query_variable_info(
 			u64 *remaining_variable_storage_size,
 			u64 *maximum_variable_size)
 {
+	/*
+	*maximum_variable_storage_size = EFI_VAR_BUF_SIZE -
+					 sizeof(struct efi_var_file);
+	*remaining_variable_storage_size = efi_var_mem_free();
+	*maximum_variable_size = EFI_VAR_BUF_SIZE -
+				 sizeof(struct efi_var_file) -
+				 sizeof(struct efi_var_entry);
+	*/
 	return EFI_UNSUPPORTED;
 }

@@ -700,9 +393,6 @@ efi_set_variable_runtime(u16 *variable_name, const efi_guid_t *vendor,
  */
 void efi_variables_boot_exit_notify(void)
 {
-	/* Write non-volatile EFI variables to file */
-	efi_var_to_file();
-
 	/* Switch variable services functions to runtime version */
 	efi_runtime_services.get_variable = efi_get_variable_runtime;
 	efi_runtime_services.get_next_variable_name =
@@ -718,5 +408,11 @@ void efi_variables_boot_exit_notify(void)
  */
 efi_status_t efi_init_variables(void)
 {
+	efi_status_t ret;
+
+	ret = efi_var_mem_init();
+	if (ret != EFI_SUCCESS)
+		return ret;
+
 	return efi_var_from_file();
 }
diff --git a/lib/efi_loader/efi_variables_mem.c b/lib/efi_loader/efi_variables_mem.c
index f70cc65f8b..939fbaa972 100644
--- a/lib/efi_loader/efi_variables_mem.c
+++ b/lib/efi_loader/efi_variables_mem.c
@@ -263,8 +263,15 @@ static void efi_var_mem_bs_del(void)
 static void EFIAPI __efi_runtime
 efi_var_mem_notify_exit_boot_services(struct efi_event *event, void *context)
 {
+	EFI_ENTRY("%p, %p", event, context);
+
+	/* Write non-volatile EFI variables to file */
+	efi_var_to_file();
+
 	/* Delete boot service only variables */
 	efi_var_mem_bs_del();
+
+	EFI_EXIT(EFI_SUCCESS);
 }

 /**
--
2.25.1

[PATCH 00/16] efi_loader: non-volatile and runtime variables

[Simon Glass]

Hi Heinrich,

On Thu, 26 Mar 2020 at 23:28, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Up to UEFI variable where stored in U-Boot environment variables. Saving

That doesn't read right to me.

> UEFI variables was not possible without saving the U-Boot environment
> variables.
>
> With this patch series file ubootefi.var in the EFI system partition is
> used for saving UEFI variables.
>
> Furthermore the UEFI variables are exposed at runtime and are writable at
> runtime.
>
> The missing piece is transferring the variable changed at runtime back to
> the firmware. I will evaluate the following options:
>
> * using a fixed memory address: we could read the memory area after a
>   reboot
> * using a systemd service which is called before the system goes down
>
> Many of the CCs are due to the changes in disk/part_efi.c. Here the logic
> to detect the EFI system partition is introduced (patch 04/16).
>
> Heinrich Schuchardt (16):
>   cmd: efidebug: fix int to pointer cast
>   efi_loader: only reserve memory if fdt node enabled
>   efi_loader: eliminate EFI_CALL() for variable access
>   part: detect EFI system partition
>   efi_loader: identify EFI system partition
>   efi_loader: keep attributes in efi_set_variable_int()
>   efi_loader: export initialization state
>   efi_loader: change setup sequence
>   efi_loader: imply FAT, FAT_WRITE
>   efi_loader: UEFI variable persistence
>   efi_loader: export efi_convert_pointer()
>   efi_loader: optional pointer for ConvertPointer
>   efi_loader: memory buffer for variables
>   efi_loader: use memory based variable storage
>   efi_loader: enable UEFI variables at runtime
>   efi_selftest: adjust runtime test for variables
>
>  cmd/bootefi.c                                 |   3 +-
>  cmd/efidebug.c                                |  71 +-
>  cmd/nvedit_efi.c                              |  18 +-
>  disk/part_dos.c                               |  10 +-
>  disk/part_efi.c                               |  12 +-
>  include/efi_api.h                             |   2 +
>  include/efi_loader.h                          |  22 +
>  include/efi_variable.h                        |  52 ++
>  include/part.h                                |  11 +-
>  lib/efi_loader/Kconfig                        |  10 +
>  lib/efi_loader/Makefile                       |   2 +
>  lib/efi_loader/efi_bootmgr.c                  |  20 +-
>  lib/efi_loader/efi_disk.c                     |  20 +
>  lib/efi_loader/efi_runtime.c                  |  20 +-
>  lib/efi_loader/efi_setup.c                    |  54 +-
>  lib/efi_loader/efi_variable.c                 | 635 ++++++------------
>  lib/efi_loader/efi_variables_file.c           | 235 +++++++
>  lib/efi_loader/efi_variables_mem.c            | 324 +++++++++
>  .../efi_selftest_variables_runtime.c          |  47 +-
>  19 files changed, 1037 insertions(+), 531 deletions(-)
>  create mode 100644 include/efi_variable.h
>  create mode 100644 lib/efi_loader/efi_variables_file.c
>  create mode 100644 lib/efi_loader/efi_variables_mem.c

Can you use 'vars' instead of variables as these filenames are too long.

Regards,
Simon

[PATCH 00/16] efi_loader: non-volatile and runtime variables

[Heinrich Schuchardt]

On 3/27/20 8:44 PM, Simon Glass wrote:
> Hi Heinrich,
>
> On Thu, 26 Mar 2020 at 23:28, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> Up to UEFI variable where stored in U-Boot environment variables. Saving
>
> That doesn't read right to me.

This should be:

Up to now UEFI variable were stored in U-Boot environment variables

>
>> UEFI variables was not possible without saving the U-Boot environment
>> variables.
>>
>> With this patch series file ubootefi.var in the EFI system partition is
>> used for saving UEFI variables.
>>
>> Furthermore the UEFI variables are exposed at runtime and are writable at
>> runtime.
>>
>> The missing piece is transferring the variable changed at runtime back to
>> the firmware. I will evaluate the following options:
>>
>> * using a fixed memory address: we could read the memory area after a
>>    reboot
>> * using a systemd service which is called before the system goes down
>>
>> Many of the CCs are due to the changes in disk/part_efi.c. Here the logic
>> to detect the EFI system partition is introduced (patch 04/16).
>>
>> Heinrich Schuchardt (16):
>>    cmd: efidebug: fix int to pointer cast
>>    efi_loader: only reserve memory if fdt node enabled
>>    efi_loader: eliminate EFI_CALL() for variable access
>>    part: detect EFI system partition
>>    efi_loader: identify EFI system partition
>>    efi_loader: keep attributes in efi_set_variable_int()
>>    efi_loader: export initialization state
>>    efi_loader: change setup sequence
>>    efi_loader: imply FAT, FAT_WRITE
>>    efi_loader: UEFI variable persistence
>>    efi_loader: export efi_convert_pointer()
>>    efi_loader: optional pointer for ConvertPointer
>>    efi_loader: memory buffer for variables
>>    efi_loader: use memory based variable storage
>>    efi_loader: enable UEFI variables at runtime
>>    efi_selftest: adjust runtime test for variables
>>
>>   cmd/bootefi.c                                 |   3 +-
>>   cmd/efidebug.c                                |  71 +-
>>   cmd/nvedit_efi.c                              |  18 +-
>>   disk/part_dos.c                               |  10 +-
>>   disk/part_efi.c                               |  12 +-
>>   include/efi_api.h                             |   2 +
>>   include/efi_loader.h                          |  22 +
>>   include/efi_variable.h                        |  52 ++
>>   include/part.h                                |  11 +-
>>   lib/efi_loader/Kconfig                        |  10 +
>>   lib/efi_loader/Makefile                       |   2 +
>>   lib/efi_loader/efi_bootmgr.c                  |  20 +-
>>   lib/efi_loader/efi_disk.c                     |  20 +
>>   lib/efi_loader/efi_runtime.c                  |  20 +-
>>   lib/efi_loader/efi_setup.c                    |  54 +-
>>   lib/efi_loader/efi_variable.c                 | 635 ++++++------------
>>   lib/efi_loader/efi_variables_file.c           | 235 +++++++
>>   lib/efi_loader/efi_variables_mem.c            | 324 +++++++++
>>   .../efi_selftest_variables_runtime.c          |  47 +-
>>   19 files changed, 1037 insertions(+), 531 deletions(-)
>>   create mode 100644 include/efi_variable.h
>>   create mode 100644 lib/efi_loader/efi_variables_file.c
>>   create mode 100644 lib/efi_loader/efi_variables_mem.c
>
> Can you use 'vars' instead of variables as these filenames are too long.

Sure

Regards

Heinrich

[PATCH 15/16] efi_loader: enable UEFI variables at runtime

[Heinrich Schuchardt]

Enable UEFI variables at runtime.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_runtime.c  |  6 +++++-
 lib/efi_loader/efi_variable.c | 23 +++++++++++++++++------
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
index 664a0422e2..acd644202d 100644
--- a/lib/efi_loader/efi_runtime.c
+++ b/lib/efi_loader/efi_runtime.c
@@ -120,8 +120,12 @@ efi_status_t efi_init_runtime_supported(void)
 	rt_table->version = EFI_RT_PROPERTIES_TABLE_VERSION;
 	rt_table->length = sizeof(struct efi_rt_properties_table);
 	rt_table->runtime_services_supported =
+				EFI_RT_SUPPORTED_GET_VARIABLE |
+				EFI_RT_SUPPORTED_GET_NEXT_VARIABLE_NAME |
+				EFI_RT_SUPPORTED_SET_VARIABLE |
 				EFI_RT_SUPPORTED_SET_VIRTUAL_ADDRESS_MAP |
-				EFI_RT_SUPPORTED_CONVERT_POINTER;
+				EFI_RT_SUPPORTED_CONVERT_POINTER |
+				EFI_RT_SUPPORTED_QUERY_VARIABLE_INFO;

 	/*
 	 * This value must be synced with efi_runtime_detach_list
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
index 7c39542968..cf8b44c535 100644
--- a/lib/efi_loader/efi_variable.c
+++ b/lib/efi_loader/efi_variable.c
@@ -326,15 +326,13 @@ efi_status_t __efi_runtime EFIAPI efi_query_variable_info(
 			u64 *remaining_variable_storage_size,
 			u64 *maximum_variable_size)
 {
-	/*
 	*maximum_variable_storage_size = EFI_VAR_BUF_SIZE -
 					 sizeof(struct efi_var_file);
 	*remaining_variable_storage_size = efi_var_mem_free();
 	*maximum_variable_size = EFI_VAR_BUF_SIZE -
 				 sizeof(struct efi_var_file) -
 				 sizeof(struct efi_var_entry);
-	*/
-	return EFI_UNSUPPORTED;
+	return EFI_SUCCESS;
 }

 /**
@@ -351,7 +349,8 @@ static efi_status_t __efi_runtime EFIAPI
 efi_get_variable_runtime(u16 *variable_name, const efi_guid_t *vendor,
 			 u32 *attributes, efi_uintn_t *data_size, void *data)
 {
-	return EFI_UNSUPPORTED;
+	return efi_get_variable_int(variable_name, vendor, attributes,
+				    data_size, data);
 }

 /**
@@ -367,7 +366,8 @@ static efi_status_t __efi_runtime EFIAPI
 efi_get_next_variable_name_runtime(efi_uintn_t *variable_name_size,
 				   u16 *variable_name, efi_guid_t *vendor)
 {
-	return EFI_UNSUPPORTED;
+	return efi_get_next_variable_name_int(variable_name_size, variable_name,
+					      vendor);
 }

 /**
@@ -385,7 +385,18 @@ efi_set_variable_runtime(u16 *variable_name, const efi_guid_t *vendor,
 			 u32 attributes, efi_uintn_t data_size,
 			 const void *data)
 {
-	return EFI_UNSUPPORTED;
+	const u32 required_attributes = EFI_VARIABLE_NON_VOLATILE |
+					EFI_VARIABLE_BOOTSERVICE_ACCESS |
+					EFI_VARIABLE_RUNTIME_ACCESS;
+
+	if (attributes &&
+	    (attributes & required_attributes) != required_attributes)
+		return EFI_INVALID_PARAMETER;
+	if ((attributes & ~(u32)EFI_VARIABLE_MASK))
+		return EFI_INVALID_PARAMETER;
+
+	return efi_set_variable_rt_int(variable_name, vendor, attributes,
+				       data_size, data);
 }

 /**
--
2.25.1

[PATCH 15/16] efi_selftest: adjust runtime test for variables

[Heinrich Schuchardt]

As variable services are available at runtime we have to expect EFI_SUCCESS
when calling the services.

Check the SetVariable() only succeeds with EFI_VARIABLE_RUNTIME_ACCESS and
EFI_VARIABLE_NON_VOLATILE set.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 .../efi_selftest_variables_runtime.c          | 47 +++++++++++++++----
 1 file changed, 39 insertions(+), 8 deletions(-)

diff --git a/lib/efi_selftest/efi_selftest_variables_runtime.c b/lib/efi_selftest/efi_selftest_variables_runtime.c
index b3b40ad2cf..c6005eeeaf 100644
--- a/lib/efi_selftest/efi_selftest_variables_runtime.c
+++ b/lib/efi_selftest/efi_selftest_variables_runtime.c
@@ -20,8 +20,8 @@ static const efi_guid_t guid_vendor0 =
 	EFI_GUID(0x67029eb5, 0x0af2, 0xf6b1,
 		 0xda, 0x53, 0xfc, 0xb5, 0x66, 0xdd, 0x1c, 0xe6);

-/*
- * Setup unit test.
+/**
+ * setup() - set up unit test.
  *
  * @handle	handle of the loaded image
  * @systable	system table
@@ -38,7 +38,7 @@ static int setup(const efi_handle_t img_handle,
 /**
  * execute() - execute unit test
  *
- * As runtime support is not implmented expect EFI_UNSUPPORTED to be returned.
+ * Test variable services at runtime.
  */
 static int execute(void)
 {
@@ -52,37 +52,68 @@ static int execute(void)
 	efi_guid_t guid;
 	u64 max_storage, rem_storage, max_size;

-	ret = runtime->query_variable_info(EFI_VARIABLE_BOOTSERVICE_ACCESS,
+	ret = runtime->query_variable_info(EFI_VARIABLE_BOOTSERVICE_ACCESS |
+					   EFI_VARIABLE_RUNTIME_ACCESS |
+					   EFI_VARIABLE_NON_VOLATILE,
 					   &max_storage, &rem_storage,
 					   &max_size);
-	if (ret != EFI_UNSUPPORTED) {
+	if (ret != EFI_SUCCESS) {
 		efi_st_error("QueryVariableInfo failed\n");
 		return EFI_ST_FAILURE;
 	}

+	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0,
+				    EFI_VARIABLE_BOOTSERVICE_ACCESS |
+				    EFI_VARIABLE_NON_VOLATILE,
+				    3, v + 4);
+	if (ret != EFI_INVALID_PARAMETER) {
+		efi_st_error("SetVariable succeeded w/o EFI_VARIABLE_RUNTIME_ACCESS\n");
+		return EFI_ST_FAILURE;
+	}
+
 	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0,
 				    EFI_VARIABLE_BOOTSERVICE_ACCESS |
 				    EFI_VARIABLE_RUNTIME_ACCESS,
 				    3, v + 4);
-	if (ret != EFI_UNSUPPORTED) {
+	if (ret != EFI_INVALID_PARAMETER) {
+		efi_st_error("SetVariable succeeded w/o EFI_VARIABLE_NON_VOLATILE\n");
+		return EFI_ST_FAILURE;
+	}
+
+	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0,
+				    EFI_VARIABLE_BOOTSERVICE_ACCESS |
+				    EFI_VARIABLE_RUNTIME_ACCESS |
+				    EFI_VARIABLE_NON_VOLATILE,
+				    3, v + 4);
+	if (ret != EFI_SUCCESS) {
 		efi_st_error("SetVariable failed\n");
 		return EFI_ST_FAILURE;
 	}
+
 	len = 3;
 	ret = runtime->get_variable(L"efi_st_var0", &guid_vendor0,
 				    &attr, &len, data);
-	if (ret != EFI_UNSUPPORTED) {
+	if (ret != EFI_SUCCESS) {
 		efi_st_error("GetVariable failed\n");
 		return EFI_ST_FAILURE;
 	}
+
 	memset(&guid, 0, 16);
 	*varname = 0;
+	len = EFI_ST_MAX_VARNAME_SIZE * sizeof(u16);
 	ret = runtime->get_next_variable_name(&len, varname, &guid);
-	if (ret != EFI_UNSUPPORTED) {
+	if (ret != EFI_SUCCESS) {
 		efi_st_error("GetNextVariableName failed\n");
 		return EFI_ST_FAILURE;
 	}

+	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0, 0,
+				    3, v + 4);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("Variable deletion failed\n");
+		return EFI_ST_FAILURE;
+	}
+
 	return EFI_ST_SUCCESS;
 }

--
2.25.1

[PATCH 15/16] efi_loader: enable UEFI variables at runtime

[AKASHI Takahiro]

On Tue, Mar 31, 2020 at 08:05:40AM +0200, Heinrich Schuchardt wrote:
> Enable UEFI variables at runtime.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_loader/efi_runtime.c  |  6 +++++-
>  lib/efi_loader/efi_variable.c | 23 +++++++++++++++++------
>  2 files changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
> index 664a0422e2..acd644202d 100644
> --- a/lib/efi_loader/efi_runtime.c
> +++ b/lib/efi_loader/efi_runtime.c
> @@ -120,8 +120,12 @@ efi_status_t efi_init_runtime_supported(void)
>  	rt_table->version = EFI_RT_PROPERTIES_TABLE_VERSION;
>  	rt_table->length = sizeof(struct efi_rt_properties_table);
>  	rt_table->runtime_services_supported =
> +				EFI_RT_SUPPORTED_GET_VARIABLE |
> +				EFI_RT_SUPPORTED_GET_NEXT_VARIABLE_NAME |
> +				EFI_RT_SUPPORTED_SET_VARIABLE |
>  				EFI_RT_SUPPORTED_SET_VIRTUAL_ADDRESS_MAP |
> -				EFI_RT_SUPPORTED_CONVERT_POINTER;
> +				EFI_RT_SUPPORTED_CONVERT_POINTER |
> +				EFI_RT_SUPPORTED_QUERY_VARIABLE_INFO;
> 
>  	/*
>  	 * This value must be synced with efi_runtime_detach_list
> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> index 7c39542968..cf8b44c535 100644
> --- a/lib/efi_loader/efi_variable.c
> +++ b/lib/efi_loader/efi_variable.c
> @@ -326,15 +326,13 @@ efi_status_t __efi_runtime EFIAPI efi_query_variable_info(
>  			u64 *remaining_variable_storage_size,
>  			u64 *maximum_variable_size)
>  {
> -	/*
>  	*maximum_variable_storage_size = EFI_VAR_BUF_SIZE -
>  					 sizeof(struct efi_var_file);
>  	*remaining_variable_storage_size = efi_var_mem_free();
>  	*maximum_variable_size = EFI_VAR_BUF_SIZE -
>  				 sizeof(struct efi_var_file) -
>  				 sizeof(struct efi_var_entry);
> -	*/
> -	return EFI_UNSUPPORTED;
> +	return EFI_SUCCESS;
>  }
> 
>  /**
> @@ -351,7 +349,8 @@ static efi_status_t __efi_runtime EFIAPI
>  efi_get_variable_runtime(u16 *variable_name, const efi_guid_t *vendor,
>  			 u32 *attributes, efi_uintn_t *data_size, void *data)
>  {
> -	return EFI_UNSUPPORTED;
> +	return efi_get_variable_int(variable_name, vendor, attributes,
> +				    data_size, data);
>  }
> 
>  /**
> @@ -367,7 +366,8 @@ static efi_status_t __efi_runtime EFIAPI
>  efi_get_next_variable_name_runtime(efi_uintn_t *variable_name_size,
>  				   u16 *variable_name, efi_guid_t *vendor)
>  {
> -	return EFI_UNSUPPORTED;
> +	return efi_get_next_variable_name_int(variable_name_size, variable_name,
> +					      vendor);
>  }
> 
>  /**
> @@ -385,7 +385,18 @@ efi_set_variable_runtime(u16 *variable_name, const efi_guid_t *vendor,
>  			 u32 attributes, efi_uintn_t data_size,
>  			 const void *data)
>  {
> -	return EFI_UNSUPPORTED;
> +	const u32 required_attributes = EFI_VARIABLE_NON_VOLATILE |
> +					EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +					EFI_VARIABLE_RUNTIME_ACCESS;
> +

Why not support APPEND_WRITE?


> +	if (attributes &&
> +	    (attributes & required_attributes) != required_attributes)
> +		return EFI_INVALID_PARAMETER;

UEFI specification says,
===8<====
Variables that have runtime access but that are not nonvolatile are read-
only data variables once ExitBootServices() is performed.
[snip]
EFI_WRITE_PROTECTED The variable in question is read-only.
===>8===

So in this case, we should return WRITE_PROTECTED instead of
INVALID_PARAMETER.

> +	if ((attributes & ~(u32)EFI_VARIABLE_MASK))
> +		return EFI_INVALID_PARAMETER;
> +
> +	return efi_set_variable_rt_int(variable_name, vendor, attributes,
> +				       data_size, data);

I didn't follow detailed logic of this function, but
does it really return UNSUPPORTED if !FAT_WRITE?

-Takahiro Akashi

>  }
> 
>  /**
> --
> 2.25.1
> 

[PATCH 15/16] efi_loader: enable UEFI variables at runtime

[Heinrich Schuchardt]

On 4/1/20 3:41 AM, AKASHI Takahiro wrote:
> On Tue, Mar 31, 2020 at 08:05:40AM +0200, Heinrich Schuchardt wrote:
>> Enable UEFI variables at runtime.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>  lib/efi_loader/efi_runtime.c  |  6 +++++-
>>  lib/efi_loader/efi_variable.c | 23 +++++++++++++++++------
>>  2 files changed, 22 insertions(+), 7 deletions(-)
>>
>> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
>> index 664a0422e2..acd644202d 100644
>> --- a/lib/efi_loader/efi_runtime.c
>> +++ b/lib/efi_loader/efi_runtime.c
>> @@ -120,8 +120,12 @@ efi_status_t efi_init_runtime_supported(void)
>>  	rt_table->version = EFI_RT_PROPERTIES_TABLE_VERSION;
>>  	rt_table->length = sizeof(struct efi_rt_properties_table);
>>  	rt_table->runtime_services_supported =
>> +				EFI_RT_SUPPORTED_GET_VARIABLE |
>> +				EFI_RT_SUPPORTED_GET_NEXT_VARIABLE_NAME |
>> +				EFI_RT_SUPPORTED_SET_VARIABLE |
>>  				EFI_RT_SUPPORTED_SET_VIRTUAL_ADDRESS_MAP |
>> -				EFI_RT_SUPPORTED_CONVERT_POINTER;
>> +				EFI_RT_SUPPORTED_CONVERT_POINTER |
>> +				EFI_RT_SUPPORTED_QUERY_VARIABLE_INFO;
>>
>>  	/*
>>  	 * This value must be synced with efi_runtime_detach_list
>> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
>> index 7c39542968..cf8b44c535 100644
>> --- a/lib/efi_loader/efi_variable.c
>> +++ b/lib/efi_loader/efi_variable.c
>> @@ -326,15 +326,13 @@ efi_status_t __efi_runtime EFIAPI efi_query_variable_info(
>>  			u64 *remaining_variable_storage_size,
>>  			u64 *maximum_variable_size)
>>  {
>> -	/*
>>  	*maximum_variable_storage_size = EFI_VAR_BUF_SIZE -
>>  					 sizeof(struct efi_var_file);
>>  	*remaining_variable_storage_size = efi_var_mem_free();
>>  	*maximum_variable_size = EFI_VAR_BUF_SIZE -
>>  				 sizeof(struct efi_var_file) -
>>  				 sizeof(struct efi_var_entry);
>> -	*/
>> -	return EFI_UNSUPPORTED;
>> +	return EFI_SUCCESS;
>>  }
>>
>>  /**
>> @@ -351,7 +349,8 @@ static efi_status_t __efi_runtime EFIAPI
>>  efi_get_variable_runtime(u16 *variable_name, const efi_guid_t *vendor,
>>  			 u32 *attributes, efi_uintn_t *data_size, void *data)
>>  {
>> -	return EFI_UNSUPPORTED;
>> +	return efi_get_variable_int(variable_name, vendor, attributes,
>> +				    data_size, data);
>>  }
>>
>>  /**
>> @@ -367,7 +366,8 @@ static efi_status_t __efi_runtime EFIAPI
>>  efi_get_next_variable_name_runtime(efi_uintn_t *variable_name_size,
>>  				   u16 *variable_name, efi_guid_t *vendor)
>>  {
>> -	return EFI_UNSUPPORTED;
>> +	return efi_get_next_variable_name_int(variable_name_size, variable_name,
>> +					      vendor);
>>  }
>>
>>  /**
>> @@ -385,7 +385,18 @@ efi_set_variable_runtime(u16 *variable_name, const efi_guid_t *vendor,
>>  			 u32 attributes, efi_uintn_t data_size,
>>  			 const void *data)
>>  {
>> -	return EFI_UNSUPPORTED;
>> +	const u32 required_attributes = EFI_VARIABLE_NON_VOLATILE |
>> +					EFI_VARIABLE_BOOTSERVICE_ACCESS |
>> +					EFI_VARIABLE_RUNTIME_ACCESS;
>> +
>
> Why not support APPEND_WRITE?

This is the list of attributes that have to be set as a *minimum* in a
valid call to SetVariable. APPEND_WRITE is not required but optional.

>
>
>> +	if (attributes &&
>> +	    (attributes & required_attributes) != required_attributes)
>> +		return EFI_INVALID_PARAMETER;
>
> UEFI specification says,
> ===8<====
> Variables that have runtime access but that are not nonvolatile are read-
> only data variables once ExitBootServices() is performed.
> [snip]
> EFI_WRITE_PROTECTED The variable in question is read-only.
> ===>8===
>
> So in this case, we should return WRITE_PROTECTED instead of
> INVALID_PARAMETER.

Here I check the parameters of the call which may create a new variable.

We should later in the code check if the variable already exists and if
it is volatile return EFI_WRITE_PROTECTED.

Best regards

Heinrich

>
>> +	if ((attributes & ~(u32)EFI_VARIABLE_MASK))
>> +		return EFI_INVALID_PARAMETER;
>> +
>> +	return efi_set_variable_rt_int(variable_name, vendor, attributes,
>> +				       data_size, data);
>
> I didn't follow detailed logic of this function, but
> does it really return UNSUPPORTED if !FAT_WRITE?
>
> -Takahiro Akashi
>
>>  }
>>
>>  /**
>> --
>> 2.25.1
>>

[PATCH 15/16] efi_loader: enable UEFI variables at runtime

[AKASHI Takahiro]

On Wed, Apr 01, 2020 at 08:26:24AM +0200, Heinrich Schuchardt wrote:
> On 4/1/20 3:41 AM, AKASHI Takahiro wrote:
> > On Tue, Mar 31, 2020 at 08:05:40AM +0200, Heinrich Schuchardt wrote:
> >> Enable UEFI variables at runtime.
> >>
> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >> ---
> >>  lib/efi_loader/efi_runtime.c  |  6 +++++-
> >>  lib/efi_loader/efi_variable.c | 23 +++++++++++++++++------
> >>  2 files changed, 22 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
> >> index 664a0422e2..acd644202d 100644
> >> --- a/lib/efi_loader/efi_runtime.c
> >> +++ b/lib/efi_loader/efi_runtime.c
> >> @@ -120,8 +120,12 @@ efi_status_t efi_init_runtime_supported(void)
> >>  	rt_table->version = EFI_RT_PROPERTIES_TABLE_VERSION;
> >>  	rt_table->length = sizeof(struct efi_rt_properties_table);
> >>  	rt_table->runtime_services_supported =
> >> +				EFI_RT_SUPPORTED_GET_VARIABLE |
> >> +				EFI_RT_SUPPORTED_GET_NEXT_VARIABLE_NAME |
> >> +				EFI_RT_SUPPORTED_SET_VARIABLE |
> >>  				EFI_RT_SUPPORTED_SET_VIRTUAL_ADDRESS_MAP |
> >> -				EFI_RT_SUPPORTED_CONVERT_POINTER;
> >> +				EFI_RT_SUPPORTED_CONVERT_POINTER |
> >> +				EFI_RT_SUPPORTED_QUERY_VARIABLE_INFO;
> >>
> >>  	/*
> >>  	 * This value must be synced with efi_runtime_detach_list
> >> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> >> index 7c39542968..cf8b44c535 100644
> >> --- a/lib/efi_loader/efi_variable.c
> >> +++ b/lib/efi_loader/efi_variable.c
> >> @@ -326,15 +326,13 @@ efi_status_t __efi_runtime EFIAPI efi_query_variable_info(
> >>  			u64 *remaining_variable_storage_size,
> >>  			u64 *maximum_variable_size)
> >>  {
> >> -	/*
> >>  	*maximum_variable_storage_size = EFI_VAR_BUF_SIZE -
> >>  					 sizeof(struct efi_var_file);
> >>  	*remaining_variable_storage_size = efi_var_mem_free();
> >>  	*maximum_variable_size = EFI_VAR_BUF_SIZE -
> >>  				 sizeof(struct efi_var_file) -
> >>  				 sizeof(struct efi_var_entry);
> >> -	*/
> >> -	return EFI_UNSUPPORTED;
> >> +	return EFI_SUCCESS;
> >>  }
> >>
> >>  /**
> >> @@ -351,7 +349,8 @@ static efi_status_t __efi_runtime EFIAPI
> >>  efi_get_variable_runtime(u16 *variable_name, const efi_guid_t *vendor,
> >>  			 u32 *attributes, efi_uintn_t *data_size, void *data)
> >>  {
> >> -	return EFI_UNSUPPORTED;
> >> +	return efi_get_variable_int(variable_name, vendor, attributes,
> >> +				    data_size, data);
> >>  }
> >>
> >>  /**
> >> @@ -367,7 +366,8 @@ static efi_status_t __efi_runtime EFIAPI
> >>  efi_get_next_variable_name_runtime(efi_uintn_t *variable_name_size,
> >>  				   u16 *variable_name, efi_guid_t *vendor)
> >>  {
> >> -	return EFI_UNSUPPORTED;
> >> +	return efi_get_next_variable_name_int(variable_name_size, variable_name,
> >> +					      vendor);
> >>  }
> >>
> >>  /**
> >> @@ -385,7 +385,18 @@ efi_set_variable_runtime(u16 *variable_name, const efi_guid_t *vendor,
> >>  			 u32 attributes, efi_uintn_t data_size,
> >>  			 const void *data)
> >>  {
> >> -	return EFI_UNSUPPORTED;
> >> +	const u32 required_attributes = EFI_VARIABLE_NON_VOLATILE |
> >> +					EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >> +					EFI_VARIABLE_RUNTIME_ACCESS;
> >> +
> >
> > Why not support APPEND_WRITE?
> 
> This is the list of attributes that have to be set as a *minimum* in a
> valid call to SetVariable. APPEND_WRITE is not required but optional.
> 
> >
> >
> >> +	if (attributes &&
> >> +	    (attributes & required_attributes) != required_attributes)
> >> +		return EFI_INVALID_PARAMETER;
> >
> > UEFI specification says,
> > ===8<====
> > Variables that have runtime access but that are not nonvolatile are read-
> > only data variables once ExitBootServices() is performed.
> > [snip]
> > EFI_WRITE_PROTECTED The variable in question is read-only.
> > ===>8===
> >
> > So in this case, we should return WRITE_PROTECTED instead of
> > INVALID_PARAMETER.
> 
> Here I check the parameters of the call which may create a new variable.

If !NON_VOLATILE, 
> >> +	if (attributes &&
> >> +	    (attributes & required_attributes) != required_attributes)
> >> +		return EFI_INVALID_PARAMETER;

this check will return INVALID_PARAMETER whether the variable exists
or not.

> We should later in the code check if the variable already exists and if
> it is volatile return EFI_WRITE_PROTECTED.
> 
> Best regards
> 
> Heinrich
> 
> >
> >> +	if ((attributes & ~(u32)EFI_VARIABLE_MASK))
> >> +		return EFI_INVALID_PARAMETER;
> >> +
> >> +	return efi_set_variable_rt_int(variable_name, vendor, attributes,
> >> +				       data_size, data);
> >
> > I didn't follow detailed logic of this function, but
> > does it really return UNSUPPORTED if !FAT_WRITE?

?

-Takahiro Akashi


> > -Takahiro Akashi
> >
> >>  }
> >>
> >>  /**
> >> --
> >> 2.25.1
> >>
> 

[PATCH 16/16] efi_selftest: adjust runtime test for variables

[Heinrich Schuchardt]

As variable services are available at runtime we have to expect EFI_SUCCESS
when calling the services.

Check the SetVariable() only succeeds with EFI_VARIABLE_RUNTIME_ACCESS and
EFI_VARIABLE_NON_VOLATILE set.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 .../efi_selftest_variables_runtime.c          | 47 +++++++++++++++----
 1 file changed, 39 insertions(+), 8 deletions(-)

diff --git a/lib/efi_selftest/efi_selftest_variables_runtime.c b/lib/efi_selftest/efi_selftest_variables_runtime.c
index b3b40ad2cf..c6005eeeaf 100644
--- a/lib/efi_selftest/efi_selftest_variables_runtime.c
+++ b/lib/efi_selftest/efi_selftest_variables_runtime.c
@@ -20,8 +20,8 @@ static const efi_guid_t guid_vendor0 =
 	EFI_GUID(0x67029eb5, 0x0af2, 0xf6b1,
 		 0xda, 0x53, 0xfc, 0xb5, 0x66, 0xdd, 0x1c, 0xe6);

-/*
- * Setup unit test.
+/**
+ * setup() - set up unit test.
  *
  * @handle	handle of the loaded image
  * @systable	system table
@@ -38,7 +38,7 @@ static int setup(const efi_handle_t img_handle,
 /**
  * execute() - execute unit test
  *
- * As runtime support is not implmented expect EFI_UNSUPPORTED to be returned.
+ * Test variable services at runtime.
  */
 static int execute(void)
 {
@@ -52,37 +52,68 @@ static int execute(void)
 	efi_guid_t guid;
 	u64 max_storage, rem_storage, max_size;

-	ret = runtime->query_variable_info(EFI_VARIABLE_BOOTSERVICE_ACCESS,
+	ret = runtime->query_variable_info(EFI_VARIABLE_BOOTSERVICE_ACCESS |
+					   EFI_VARIABLE_RUNTIME_ACCESS |
+					   EFI_VARIABLE_NON_VOLATILE,
 					   &max_storage, &rem_storage,
 					   &max_size);
-	if (ret != EFI_UNSUPPORTED) {
+	if (ret != EFI_SUCCESS) {
 		efi_st_error("QueryVariableInfo failed\n");
 		return EFI_ST_FAILURE;
 	}

+	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0,
+				    EFI_VARIABLE_BOOTSERVICE_ACCESS |
+				    EFI_VARIABLE_NON_VOLATILE,
+				    3, v + 4);
+	if (ret != EFI_INVALID_PARAMETER) {
+		efi_st_error("SetVariable succeeded w/o EFI_VARIABLE_RUNTIME_ACCESS\n");
+		return EFI_ST_FAILURE;
+	}
+
 	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0,
 				    EFI_VARIABLE_BOOTSERVICE_ACCESS |
 				    EFI_VARIABLE_RUNTIME_ACCESS,
 				    3, v + 4);
-	if (ret != EFI_UNSUPPORTED) {
+	if (ret != EFI_INVALID_PARAMETER) {
+		efi_st_error("SetVariable succeeded w/o EFI_VARIABLE_NON_VOLATILE\n");
+		return EFI_ST_FAILURE;
+	}
+
+	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0,
+				    EFI_VARIABLE_BOOTSERVICE_ACCESS |
+				    EFI_VARIABLE_RUNTIME_ACCESS |
+				    EFI_VARIABLE_NON_VOLATILE,
+				    3, v + 4);
+	if (ret != EFI_SUCCESS) {
 		efi_st_error("SetVariable failed\n");
 		return EFI_ST_FAILURE;
 	}
+
 	len = 3;
 	ret = runtime->get_variable(L"efi_st_var0", &guid_vendor0,
 				    &attr, &len, data);
-	if (ret != EFI_UNSUPPORTED) {
+	if (ret != EFI_SUCCESS) {
 		efi_st_error("GetVariable failed\n");
 		return EFI_ST_FAILURE;
 	}
+
 	memset(&guid, 0, 16);
 	*varname = 0;
+	len = EFI_ST_MAX_VARNAME_SIZE * sizeof(u16);
 	ret = runtime->get_next_variable_name(&len, varname, &guid);
-	if (ret != EFI_UNSUPPORTED) {
+	if (ret != EFI_SUCCESS) {
 		efi_st_error("GetNextVariableName failed\n");
 		return EFI_ST_FAILURE;
 	}

+	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0, 0,
+				    3, v + 4);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("Variable deletion failed\n");
+		return EFI_ST_FAILURE;
+	}
+
 	return EFI_ST_SUCCESS;
 }

--
2.25.1

[PATCH 16/16] efi_selftest: adjust runtime test for variables

[AKASHI Takahiro]

On Tue, Mar 31, 2020 at 08:07:39AM +0200, Heinrich Schuchardt wrote:
> As variable services are available at runtime we have to expect EFI_SUCCESS
> when calling the services.

Why not run variables test *after* ExitBootServices as well as
*before* that event?

Then we should have more test cases.

> Check the SetVariable() only succeeds with EFI_VARIABLE_RUNTIME_ACCESS and
> EFI_VARIABLE_NON_VOLATILE set.

I'm not sure if this claim(check) is true.
At least, you need provide more description about specific test conditions.

-Takahiro Akashi

>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  .../efi_selftest_variables_runtime.c          | 47 +++++++++++++++----
>  1 file changed, 39 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/efi_selftest/efi_selftest_variables_runtime.c b/lib/efi_selftest/efi_selftest_variables_runtime.c
> index b3b40ad2cf..c6005eeeaf 100644
> --- a/lib/efi_selftest/efi_selftest_variables_runtime.c
> +++ b/lib/efi_selftest/efi_selftest_variables_runtime.c
> @@ -20,8 +20,8 @@ static const efi_guid_t guid_vendor0 =
>  	EFI_GUID(0x67029eb5, 0x0af2, 0xf6b1,
>  		 0xda, 0x53, 0xfc, 0xb5, 0x66, 0xdd, 0x1c, 0xe6);
> 
> -/*
> - * Setup unit test.
> +/**
> + * setup() - set up unit test.
>   *
>   * @handle	handle of the loaded image
>   * @systable	system table
> @@ -38,7 +38,7 @@ static int setup(const efi_handle_t img_handle,
>  /**
>   * execute() - execute unit test
>   *
> - * As runtime support is not implmented expect EFI_UNSUPPORTED to be returned.
> + * Test variable services at runtime.
>   */
>  static int execute(void)
>  {
> @@ -52,37 +52,68 @@ static int execute(void)
>  	efi_guid_t guid;
>  	u64 max_storage, rem_storage, max_size;
> 
> -	ret = runtime->query_variable_info(EFI_VARIABLE_BOOTSERVICE_ACCESS,
> +	ret = runtime->query_variable_info(EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +					   EFI_VARIABLE_RUNTIME_ACCESS |
> +					   EFI_VARIABLE_NON_VOLATILE,
>  					   &max_storage, &rem_storage,
>  					   &max_size);
> -	if (ret != EFI_UNSUPPORTED) {
> +	if (ret != EFI_SUCCESS) {
>  		efi_st_error("QueryVariableInfo failed\n");
>  		return EFI_ST_FAILURE;
>  	}
> 
> +	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0,
> +				    EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +				    EFI_VARIABLE_NON_VOLATILE,
> +				    3, v + 4);
> +	if (ret != EFI_INVALID_PARAMETER) {
> +		efi_st_error("SetVariable succeeded w/o EFI_VARIABLE_RUNTIME_ACCESS\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
>  	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0,
>  				    EFI_VARIABLE_BOOTSERVICE_ACCESS |
>  				    EFI_VARIABLE_RUNTIME_ACCESS,
>  				    3, v + 4);
> -	if (ret != EFI_UNSUPPORTED) {
> +	if (ret != EFI_INVALID_PARAMETER) {
> +		efi_st_error("SetVariable succeeded w/o EFI_VARIABLE_NON_VOLATILE\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0,
> +				    EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +				    EFI_VARIABLE_RUNTIME_ACCESS |
> +				    EFI_VARIABLE_NON_VOLATILE,
> +				    3, v + 4);
> +	if (ret != EFI_SUCCESS) {
>  		efi_st_error("SetVariable failed\n");
>  		return EFI_ST_FAILURE;
>  	}
> +
>  	len = 3;
>  	ret = runtime->get_variable(L"efi_st_var0", &guid_vendor0,
>  				    &attr, &len, data);
> -	if (ret != EFI_UNSUPPORTED) {
> +	if (ret != EFI_SUCCESS) {
>  		efi_st_error("GetVariable failed\n");
>  		return EFI_ST_FAILURE;
>  	}
> +
>  	memset(&guid, 0, 16);
>  	*varname = 0;
> +	len = EFI_ST_MAX_VARNAME_SIZE * sizeof(u16);
>  	ret = runtime->get_next_variable_name(&len, varname, &guid);
> -	if (ret != EFI_UNSUPPORTED) {
> +	if (ret != EFI_SUCCESS) {
>  		efi_st_error("GetNextVariableName failed\n");
>  		return EFI_ST_FAILURE;
>  	}
> 
> +	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0, 0,
> +				    3, v + 4);
> +	if (ret != EFI_SUCCESS) {
> +		efi_st_error("Variable deletion failed\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
>  	return EFI_ST_SUCCESS;
>  }
> 
> --
> 2.25.1
> 

[PATCH 16/16] efi_selftest: adjust runtime test for variables

[Heinrich Schuchardt]

On 4/1/20 3:05 AM, AKASHI Takahiro wrote:
> On Tue, Mar 31, 2020 at 08:07:39AM +0200, Heinrich Schuchardt wrote:
>> As variable services are available at runtime we have to expect EFI_SUCCESS
>> when calling the services.
>
> Why not run variables test *after* ExitBootServices as well as
> *before* that event?

We have a separate test for before ExitBootServices
(lib/efi_selftest/efi_selftest_variables.c).

>
> Then we should have more test cases.

Please, feel free to supply some.

Unfortunately SCT does not provide runtime testing. I have been running
my code successfully against Ubuntu's firmware test suite
(https://git.launchpad.net/fwts).

>
>> Check the SetVariable() only succeeds with EFI_VARIABLE_RUNTIME_ACCESS and
>> EFI_VARIABLE_NON_VOLATILE set.
>
> I'm not sure if this claim(check) is true.
> At least, you need provide more description about specific test conditions.

At runtime non-volatile variables and variables without runtime access
cannot be changed according to the UEFI spec.

Best regards

Heinrich

>
> -Takahiro Akashi
>
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>  .../efi_selftest_variables_runtime.c          | 47 +++++++++++++++----
>>  1 file changed, 39 insertions(+), 8 deletions(-)
>>
>> diff --git a/lib/efi_selftest/efi_selftest_variables_runtime.c b/lib/efi_selftest/efi_selftest_variables_runtime.c
>> index b3b40ad2cf..c6005eeeaf 100644
>> --- a/lib/efi_selftest/efi_selftest_variables_runtime.c
>> +++ b/lib/efi_selftest/efi_selftest_variables_runtime.c
>> @@ -20,8 +20,8 @@ static const efi_guid_t guid_vendor0 =
>>  	EFI_GUID(0x67029eb5, 0x0af2, 0xf6b1,
>>  		 0xda, 0x53, 0xfc, 0xb5, 0x66, 0xdd, 0x1c, 0xe6);
>>
>> -/*
>> - * Setup unit test.
>> +/**
>> + * setup() - set up unit test.
>>   *
>>   * @handle	handle of the loaded image
>>   * @systable	system table
>> @@ -38,7 +38,7 @@ static int setup(const efi_handle_t img_handle,
>>  /**
>>   * execute() - execute unit test
>>   *
>> - * As runtime support is not implmented expect EFI_UNSUPPORTED to be returned.
>> + * Test variable services at runtime.
>>   */
>>  static int execute(void)
>>  {
>> @@ -52,37 +52,68 @@ static int execute(void)
>>  	efi_guid_t guid;
>>  	u64 max_storage, rem_storage, max_size;
>>
>> -	ret = runtime->query_variable_info(EFI_VARIABLE_BOOTSERVICE_ACCESS,
>> +	ret = runtime->query_variable_info(EFI_VARIABLE_BOOTSERVICE_ACCESS |
>> +					   EFI_VARIABLE_RUNTIME_ACCESS |
>> +					   EFI_VARIABLE_NON_VOLATILE,
>>  					   &max_storage, &rem_storage,
>>  					   &max_size);
>> -	if (ret != EFI_UNSUPPORTED) {
>> +	if (ret != EFI_SUCCESS) {
>>  		efi_st_error("QueryVariableInfo failed\n");
>>  		return EFI_ST_FAILURE;
>>  	}
>>
>> +	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0,
>> +				    EFI_VARIABLE_BOOTSERVICE_ACCESS |
>> +				    EFI_VARIABLE_NON_VOLATILE,
>> +				    3, v + 4);
>> +	if (ret != EFI_INVALID_PARAMETER) {
>> +		efi_st_error("SetVariable succeeded w/o EFI_VARIABLE_RUNTIME_ACCESS\n");
>> +		return EFI_ST_FAILURE;
>> +	}
>> +
>>  	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0,
>>  				    EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>  				    EFI_VARIABLE_RUNTIME_ACCESS,
>>  				    3, v + 4);
>> -	if (ret != EFI_UNSUPPORTED) {
>> +	if (ret != EFI_INVALID_PARAMETER) {
>> +		efi_st_error("SetVariable succeeded w/o EFI_VARIABLE_NON_VOLATILE\n");
>> +		return EFI_ST_FAILURE;
>> +	}
>> +
>> +	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0,
>> +				    EFI_VARIABLE_BOOTSERVICE_ACCESS |
>> +				    EFI_VARIABLE_RUNTIME_ACCESS |
>> +				    EFI_VARIABLE_NON_VOLATILE,
>> +				    3, v + 4);
>> +	if (ret != EFI_SUCCESS) {
>>  		efi_st_error("SetVariable failed\n");
>>  		return EFI_ST_FAILURE;
>>  	}
>> +
>>  	len = 3;
>>  	ret = runtime->get_variable(L"efi_st_var0", &guid_vendor0,
>>  				    &attr, &len, data);
>> -	if (ret != EFI_UNSUPPORTED) {
>> +	if (ret != EFI_SUCCESS) {
>>  		efi_st_error("GetVariable failed\n");
>>  		return EFI_ST_FAILURE;
>>  	}
>> +
>>  	memset(&guid, 0, 16);
>>  	*varname = 0;
>> +	len = EFI_ST_MAX_VARNAME_SIZE * sizeof(u16);
>>  	ret = runtime->get_next_variable_name(&len, varname, &guid);
>> -	if (ret != EFI_UNSUPPORTED) {
>> +	if (ret != EFI_SUCCESS) {
>>  		efi_st_error("GetNextVariableName failed\n");
>>  		return EFI_ST_FAILURE;
>>  	}
>>
>> +	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0, 0,
>> +				    3, v + 4);
>> +	if (ret != EFI_SUCCESS) {
>> +		efi_st_error("Variable deletion failed\n");
>> +		return EFI_ST_FAILURE;
>> +	}
>> +
>>  	return EFI_ST_SUCCESS;
>>  }
>>
>> --
>> 2.25.1
>>

[PATCH 16/16] efi_selftest: adjust runtime test for variables

[AKASHI Takahiro]

On Wed, Apr 01, 2020 at 08:37:38AM +0200, Heinrich Schuchardt wrote:
> On 4/1/20 3:05 AM, AKASHI Takahiro wrote:
> > On Tue, Mar 31, 2020 at 08:07:39AM +0200, Heinrich Schuchardt wrote:
> >> As variable services are available at runtime we have to expect EFI_SUCCESS
> >> when calling the services.
> >
> > Why not run variables test *after* ExitBootServices as well as
> > *before* that event?
> 
> We have a separate test for before ExitBootServices
> (lib/efi_selftest/efi_selftest_variables.c).

Ah, I misundersttood the file to be patched.
(EXECUTE_BEFORE_BOOTTIME_EXIT and SETUP_BEFORE_BOOTTIME_EXIT
are quite  confusing.)

> >
> > Then we should have more test cases.
> 
> Please, feel free to supply some.

?
You re-invented UEFI variable implementation almost from the scratch,
then you should cover as many corner cases as possible in general.

-Takahiro Akashi

> Unfortunately SCT does not provide runtime testing. I have been running
> my code successfully against Ubuntu's firmware test suite
> (https://git.launchpad.net/fwts).
> 
> >
> >> Check the SetVariable() only succeeds with EFI_VARIABLE_RUNTIME_ACCESS and
> >> EFI_VARIABLE_NON_VOLATILE set.
> >
> > I'm not sure if this claim(check) is true.
> > At least, you need provide more description about specific test conditions.
> 
> At runtime non-volatile variables and variables without runtime access
> cannot be changed according to the UEFI spec.

> Best regards
> 
> Heinrich
> 
> >
> > -Takahiro Akashi
> >
> >>
> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >> ---
> >>  .../efi_selftest_variables_runtime.c          | 47 +++++++++++++++----
> >>  1 file changed, 39 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/lib/efi_selftest/efi_selftest_variables_runtime.c b/lib/efi_selftest/efi_selftest_variables_runtime.c
> >> index b3b40ad2cf..c6005eeeaf 100644
> >> --- a/lib/efi_selftest/efi_selftest_variables_runtime.c
> >> +++ b/lib/efi_selftest/efi_selftest_variables_runtime.c
> >> @@ -20,8 +20,8 @@ static const efi_guid_t guid_vendor0 =
> >>  	EFI_GUID(0x67029eb5, 0x0af2, 0xf6b1,
> >>  		 0xda, 0x53, 0xfc, 0xb5, 0x66, 0xdd, 0x1c, 0xe6);
> >>
> >> -/*
> >> - * Setup unit test.
> >> +/**
> >> + * setup() - set up unit test.
> >>   *
> >>   * @handle	handle of the loaded image
> >>   * @systable	system table
> >> @@ -38,7 +38,7 @@ static int setup(const efi_handle_t img_handle,
> >>  /**
> >>   * execute() - execute unit test
> >>   *
> >> - * As runtime support is not implmented expect EFI_UNSUPPORTED to be returned.
> >> + * Test variable services at runtime.
> >>   */
> >>  static int execute(void)
> >>  {
> >> @@ -52,37 +52,68 @@ static int execute(void)
> >>  	efi_guid_t guid;
> >>  	u64 max_storage, rem_storage, max_size;
> >>
> >> -	ret = runtime->query_variable_info(EFI_VARIABLE_BOOTSERVICE_ACCESS,
> >> +	ret = runtime->query_variable_info(EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >> +					   EFI_VARIABLE_RUNTIME_ACCESS |
> >> +					   EFI_VARIABLE_NON_VOLATILE,
> >>  					   &max_storage, &rem_storage,
> >>  					   &max_size);
> >> -	if (ret != EFI_UNSUPPORTED) {
> >> +	if (ret != EFI_SUCCESS) {
> >>  		efi_st_error("QueryVariableInfo failed\n");
> >>  		return EFI_ST_FAILURE;
> >>  	}
> >>
> >> +	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0,
> >> +				    EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >> +				    EFI_VARIABLE_NON_VOLATILE,
> >> +				    3, v + 4);
> >> +	if (ret != EFI_INVALID_PARAMETER) {
> >> +		efi_st_error("SetVariable succeeded w/o EFI_VARIABLE_RUNTIME_ACCESS\n");
> >> +		return EFI_ST_FAILURE;
> >> +	}
> >> +
> >>  	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0,
> >>  				    EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >>  				    EFI_VARIABLE_RUNTIME_ACCESS,
> >>  				    3, v + 4);
> >> -	if (ret != EFI_UNSUPPORTED) {
> >> +	if (ret != EFI_INVALID_PARAMETER) {
> >> +		efi_st_error("SetVariable succeeded w/o EFI_VARIABLE_NON_VOLATILE\n");
> >> +		return EFI_ST_FAILURE;
> >> +	}
> >> +
> >> +	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0,
> >> +				    EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >> +				    EFI_VARIABLE_RUNTIME_ACCESS |
> >> +				    EFI_VARIABLE_NON_VOLATILE,
> >> +				    3, v + 4);
> >> +	if (ret != EFI_SUCCESS) {
> >>  		efi_st_error("SetVariable failed\n");
> >>  		return EFI_ST_FAILURE;
> >>  	}
> >> +
> >>  	len = 3;
> >>  	ret = runtime->get_variable(L"efi_st_var0", &guid_vendor0,
> >>  				    &attr, &len, data);
> >> -	if (ret != EFI_UNSUPPORTED) {
> >> +	if (ret != EFI_SUCCESS) {
> >>  		efi_st_error("GetVariable failed\n");
> >>  		return EFI_ST_FAILURE;
> >>  	}
> >> +
> >>  	memset(&guid, 0, 16);
> >>  	*varname = 0;
> >> +	len = EFI_ST_MAX_VARNAME_SIZE * sizeof(u16);
> >>  	ret = runtime->get_next_variable_name(&len, varname, &guid);
> >> -	if (ret != EFI_UNSUPPORTED) {
> >> +	if (ret != EFI_SUCCESS) {
> >>  		efi_st_error("GetNextVariableName failed\n");
> >>  		return EFI_ST_FAILURE;
> >>  	}
> >>
> >> +	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0, 0,
> >> +				    3, v + 4);
> >> +	if (ret != EFI_SUCCESS) {
> >> +		efi_st_error("Variable deletion failed\n");
> >> +		return EFI_ST_FAILURE;
> >> +	}
> >> +
> >>  	return EFI_ST_SUCCESS;
> >>  }
> >>
> >> --
> >> 2.25.1
> >>
>