[U-Boot] [PATCH 00/10] efi_loader: event services & API test

[U-Boot] [PATCH 00/10] efi_loader: event services & API test

[Heinrich Schuchardt]

This patch series provides:
* corrections for the EFI event services
* a test framework to check the EFI API implementation
* unit tests covering the event services

The EFI selftest is written such that it could be easily turned
into a standalone EFI application. But this would require
modifying the build procedures for EFI. Objcopy cannot generate
the necessary relocations.

The unit tests are identified by entries in a linker generated
array to make them as self sufficient as possible.

A Python test case is supplied to call run the EFI tests.

Tested with Travis CI
https://travis-ci.org/xypron2/u-boot/jobs/275733784

Of all my efi_loader patches these are the first I would like
to see merged.

Simon has commented on some other patches that he misses
comments for all EFI API functions. I will add these with
a separate patch.

Heinrich Schuchardt (10):
  efi_loader: allow return value in EFI_CALL
  efi_selftest: provide an EFI selftest application
  test/py: add a test calling the EFI selftest
  efi_loader: implement queueing of the notification function
  efi_loader: efi_set_timer: reset signaled state
  efi_selftest: provide unit test for event services
  efi_loader: implement task priority level (TPL)
  efi_selftest: test task priority levels
  efi_loader: notify when ExitBootServices is invoked
  efi_selftest: check notification of ExitBootServices

 MAINTAINERS                                      |   5 +-
 cmd/Kconfig                                      |   2 +
 cmd/bootefi.c                                    |  25 ++-
 include/efi_loader.h                             |  30 +++-
 include/efi_selftest.h                           |  91 ++++++++++
 lib/Makefile                                     |   1 +
 lib/efi_loader/efi_boottime.c                    |  83 +++++++--
 lib/efi_loader/efi_console.c                     |   4 +-
 lib/efi_selftest/Kconfig                         |   7 +
 lib/efi_selftest/Makefile                        |  26 +++
 lib/efi_selftest/efi_selftest.c                  | 219 +++++++++++++++++++++++
 lib/efi_selftest/efi_selftest_console.c          | 187 +++++++++++++++++++
 lib/efi_selftest/efi_selftest_events.c           | 195 ++++++++++++++++++++
 lib/efi_selftest/efi_selftest_exitbootservices.c | 106 +++++++++++
 lib/efi_selftest/efi_selftest_tpl.c              | 214 ++++++++++++++++++++++
 test/py/tests/test_efi_selftest.py               |  24 +++
 16 files changed, 1197 insertions(+), 22 deletions(-)
 create mode 100644 include/efi_selftest.h
 create mode 100644 lib/efi_selftest/Kconfig
 create mode 100644 lib/efi_selftest/Makefile
 create mode 100644 lib/efi_selftest/efi_selftest.c
 create mode 100644 lib/efi_selftest/efi_selftest_console.c
 create mode 100644 lib/efi_selftest/efi_selftest_events.c
 create mode 100644 lib/efi_selftest/efi_selftest_exitbootservices.c
 create mode 100644 lib/efi_selftest/efi_selftest_tpl.c
 create mode 100644 test/py/tests/test_efi_selftest.py

-- 
2.11.0

[U-Boot] [PATCH 01/10] efi_loader: allow return value in EFI_CALL

[Heinrich Schuchardt]

Macro EFI_CALL was introduced to call an UEFI function.
Unfortunately it does not support return values.
Most UEFI functions have a return value.

So let's rename EFI_CALL to EFI_CALL_VOID and introduce a
new EFI_CALL macro that supports return values.

Cc: Rob Clark <robdclark@gmail.com>
Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 include/efi_loader.h          | 17 +++++++++++++++--
 lib/efi_loader/efi_boottime.c |  3 ++-
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 46d684f6df..f27192555e 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -42,9 +42,22 @@ const char *__efi_nesting_dec(void);
 	})
 
 /*
- * Callback into UEFI world from u-boot:
+ * Call non-void UEFI function from u-boot and retrieve return value:
  */
-#define EFI_CALL(exp) do { \
+#define EFI_CALL(exp) ({ \
+	debug("%sEFI: Call: %s\n", __efi_nesting_inc(), #exp); \
+	assert(__efi_exit_check()); \
+	typeof(exp) _r = exp; \
+	assert(__efi_entry_check()); \
+	debug("%sEFI: %lu returned by %s\n", __efi_nesting_dec(), \
+	      (unsigned long)((uintptr_t)_r & ~EFI_ERROR_MASK), #exp); \
+	_r; \
+})
+
+/*
+ * Call void UEFI function from u-boot:
+ */
+#define EFI_CALL_VOID(exp) do { \
 	debug("%sEFI: Call: %s\n", __efi_nesting_inc(), #exp); \
 	assert(__efi_exit_check()); \
 	exp; \
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 90e9ead7b2..2c9379a8ae 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -163,7 +163,8 @@ void efi_signal_event(struct efi_event *event)
 		return;
 	event->signaled = 1;
 	if (event->type & EVT_NOTIFY_SIGNAL) {
-		EFI_CALL(event->notify_function(event, event->notify_context));
+		EFI_CALL_VOID(event->notify_function(event,
+						     event->notify_context));
 	}
 }
 
-- 
2.11.0

[U-Boot] [PATCH 02/10] efi_selftest: provide an EFI selftest application

[Heinrich Schuchardt]

A testing framework for the EFI API is provided.
It can be executed with the 'bootefi selftest' command.

It is coded in a way that at a later stage we may turn it
into a standalone EFI application. The current build system
does not allow this yet.

All tests use a driver model and are run in three phases:
setup, execute, teardown.

A test may be setup and executed at boottime,
it may be setup at boottime and executed at runtime,
or it may be setup and executed at runtime.

After executing all tests the system is reset.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 MAINTAINERS                             |   5 +-
 cmd/Kconfig                             |   2 +
 cmd/bootefi.c                           |  25 +++-
 include/efi_loader.h                    |   9 ++
 include/efi_selftest.h                  |  91 +++++++++++++
 lib/Makefile                            |   1 +
 lib/efi_selftest/Kconfig                |   7 +
 lib/efi_selftest/Makefile               |  17 +++
 lib/efi_selftest/efi_selftest.c         | 219 ++++++++++++++++++++++++++++++++
 lib/efi_selftest/efi_selftest_console.c | 187 +++++++++++++++++++++++++++
 10 files changed, 558 insertions(+), 5 deletions(-)
 create mode 100644 include/efi_selftest.h
 create mode 100644 lib/efi_selftest/Kconfig
 create mode 100644 lib/efi_selftest/Makefile
 create mode 100644 lib/efi_selftest/efi_selftest.c
 create mode 100644 lib/efi_selftest/efi_selftest_console.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 04acf2b89d..0b7b2bbeb2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -259,8 +259,9 @@ EFI PAYLOAD
 M:	Alexander Graf <agraf@suse.de>
 S:	Maintained
 T:	git git://github.com/agraf/u-boot.git
-F:	include/efi_loader.h
-F:	lib/efi_loader/
+F:	include/efi*
+F:	lib/efi*
+F:	test/py/tests/test_efi*
 F:	cmd/bootefi.c
 
 FLATTENED DEVICE TREE
diff --git a/cmd/Kconfig b/cmd/Kconfig
index d6d130edfa..3ef9b16b08 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -222,6 +222,8 @@ config CMD_BOOTEFI_HELLO
 	  for testing that EFI is working at a basic level, and for bringing
 	  up EFI support on a new architecture.
 
+source lib/efi_selftest/Kconfig
+
 config CMD_BOOTMENU
 	bool "bootmenu"
 	select MENU
diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index ffd50ba159..788f869479 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -285,7 +285,6 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt)
 	return efi_do_enter(&loaded_image_info, &systab, entry);
 }
 
-
 /* Interpreter command to boot an arbitrary EFI image from memory */
 static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
@@ -307,6 +306,22 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		memcpy((char *)addr, __efi_helloworld_begin, size);
 	} else
 #endif
+#ifdef CONFIG_CMD_BOOTEFI_SELFTEST
+	if (!strcmp(argv[1], "selftest")) {
+		/*
+		 * gd lives in a fixed register which may get clobbered while we
+		 * execute the payload. So save it here and restore it on every
+		 * callback entry
+		 */
+		efi_save_gd();
+		/* Initialize and populate EFI object list */
+		if (!efi_obj_list_initalized)
+			efi_init_obj_list();
+		loaded_image_info.device_handle = bootefi_device_path;
+		loaded_image_info.file_path = bootefi_image_path;
+		return efi_selftest(&loaded_image_info, &systab);
+	} else
+#endif
 	{
 		saddr = argv[1];
 
@@ -336,8 +351,12 @@ static char bootefi_help_text[] =
 	"    If specified, the device tree located at <fdt address> gets\n"
 	"    exposed as EFI configuration table.\n"
 #ifdef CONFIG_CMD_BOOTEFI_HELLO
-	"hello\n"
-	"  - boot a sample Hello World application stored within U-Boot"
+	"bootefi hello\n"
+	"  - boot a sample Hello World application stored within U-Boot\n"
+#endif
+#ifdef CONFIG_CMD_BOOTEFI_SELFTEST
+	"bootefi selftest\n"
+	"  - boot an EFI selftest application stored within U-Boot\n"
 #endif
 	;
 #endif
diff --git a/include/efi_loader.h b/include/efi_loader.h
index f27192555e..f74b33d589 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -254,6 +254,15 @@ efi_status_t __efi_runtime EFIAPI efi_get_time(
 			struct efi_time_cap *capabilities);
 void efi_get_time_init(void);
 
+#ifdef CONFIG_CMD_BOOTEFI_SELFTEST
+/*
+ * Entry point for the tests of the EFI API.
+ * It is called by 'bootefi selftest'
+ */
+efi_status_t EFIAPI efi_selftest(efi_handle_t image_handle,
+				 struct efi_system_table *systab);
+#endif
+
 #else /* defined(EFI_LOADER) && !defined(CONFIG_SPL_BUILD) */
 
 /* Without CONFIG_EFI_LOADER we don't have a runtime section, stub it out */
diff --git a/include/efi_selftest.h b/include/efi_selftest.h
new file mode 100644
index 0000000000..76304a2b2a
--- /dev/null
+++ b/include/efi_selftest.h
@@ -0,0 +1,91 @@
+/*
+ *  EFI application loader
+ *
+ *  Copyright (c) 2017 Heinrich Schuchardt <xypron.glpk@gmx.de>
+ *
+ *  SPDX-License-Identifier:     GPL-2.0+
+ */
+
+#ifndef _EFI_SELFTEST_H
+#define _EFI_SELFTEST_H
+
+#include <common.h>
+#include <efi.h>
+#include <efi_api.h>
+#include <linker_lists.h>
+
+/*
+ * Prints an error message.
+ *
+ * @...	format string followed by fields to print
+ */
+#define efi_st_error(...) \
+	efi_st_printf("%s(%u):\nERROR: ", __FILE__, __LINE__); \
+	efi_st_printf(__VA_ARGS__) \
+
+/*
+ * A test may be setup and executed at boottime,
+ * it may be setup at boottime and executed at runtime,
+ * or it may be setup and executed at runtime.
+ */
+enum efi_test_phase {
+	EFI_EXECUTE_BEFORE_BOOTTIME_EXIT = 1,
+	EFI_SETUP_BEFORE_BOOTTIME_EXIT,
+	EFI_SETUP_AFTER_BOOTTIME_EXIT,
+};
+
+extern struct efi_simple_text_output_protocol *con_out;
+extern struct efi_simple_input_interface *con_in;
+
+/*
+ * Exit the boot services.
+ *
+ * The size of the memory map is determined.
+ * Pool memory is allocated to copy the memory map.
+ * The memory amp is copied and the map key is obtained.
+ * The map key is used to exit the boot services.
+ */
+void efi_st_exit_boot_services(void);
+
+/*
+ * Print a pointer to an u16 string
+ *
+ * @pointer: pointer
+ * @buf: pointer to buffer address
+ * on return position of terminating zero word
+ */
+void efi_st_printf(const char *fmt, ...)
+		 __attribute__ ((format (__printf__, 1, 2)));
+
+/*
+ * Reads an Unicode character from the input device.
+ *
+ * @return: Unicode character
+ */
+u16 efi_st_get_key(void);
+
+/**
+ * struct efi_unit_test - EFI unit test
+ *
+ * An efi_unit_test provides a interface to an EFI unit test.
+ *
+ * @name:	name of unit test
+ * @phase:	specifies when setup and execute are executed
+ * @setup:	set up the unit test
+ * @teardown:	tear down the unit test
+ * @execute:	execute the unit test
+ */
+struct efi_unit_test {
+	const char *name;
+	const enum efi_test_phase phase;
+	int (*setup)(const efi_handle_t handle,
+		     const struct efi_system_table *systable);
+	int (*execute)(void);
+	int (*teardown)(void);
+};
+
+/* Declare a new EFI unit test */
+#define EFI_UNIT_TEST(__name)						\
+	ll_entry_declare(struct efi_unit_test, __name, efi_unit_test)
+
+#endif /* _EFI_SELFTEST_H */
diff --git a/lib/Makefile b/lib/Makefile
index 15bba9eac2..8b339dfa22 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -9,6 +9,7 @@ ifndef CONFIG_SPL_BUILD
 
 obj-$(CONFIG_EFI) += efi/
 obj-$(CONFIG_EFI_LOADER) += efi_loader/
+obj-$(CONFIG_EFI_LOADER) += efi_selftest/
 obj-$(CONFIG_LZMA) += lzma/
 obj-$(CONFIG_LZO) += lzo/
 obj-$(CONFIG_BZIP2) += bzip2/
diff --git a/lib/efi_selftest/Kconfig b/lib/efi_selftest/Kconfig
new file mode 100644
index 0000000000..3b5f3a1230
--- /dev/null
+++ b/lib/efi_selftest/Kconfig
@@ -0,0 +1,7 @@
+config CMD_BOOTEFI_SELFTEST
+	bool "Allow booting an EFI efi_selftest"
+	depends on CMD_BOOTEFI
+	help
+	  This adds an EFI test application to U-Boot that can be executed
+	  with the 'bootefi selftest' command. It provides extended tests of
+	  the EFI API implementation.
diff --git a/lib/efi_selftest/Makefile b/lib/efi_selftest/Makefile
new file mode 100644
index 0000000000..34f5ff1838
--- /dev/null
+++ b/lib/efi_selftest/Makefile
@@ -0,0 +1,17 @@
+:
+# (C) Copyright 2017, Heinrich Schuchardt <xypron.glpk@gmx.de>
+#
+#  SPDX-License-Identifier:     GPL-2.0+
+#
+
+# This file only gets included with CONFIG_EFI_LOADER set, so all
+# object inclusion implicitly depends on it
+
+CFLAGS_efi_selftest.o := $(CFLAGS_EFI)
+CFLAGS_REMOVE_efi_selftest.o := $(CFLAGS_NON_EFI)
+CFLAGS_efi_selftest_console.o := $(CFLAGS_EFI)
+CFLAGS_REMOVE_efi_selftest_console.o := $(CFLAGS_NON_EFI)
+
+obj-$(CONFIG_CMD_BOOTEFI_SELFTEST) += \
+efi_selftest.o \
+efi_selftest_console.o
diff --git a/lib/efi_selftest/efi_selftest.c b/lib/efi_selftest/efi_selftest.c
new file mode 100644
index 0000000000..efec832e98
--- /dev/null
+++ b/lib/efi_selftest/efi_selftest.c
@@ -0,0 +1,219 @@
+/*
+ * EFI efi_selftest
+ *
+ * Copyright (c) 2017 Heinrich Schuchardt <xypron.glpk@gmx.de>
+ *
+ * SPDX-License-Identifier:     GPL-2.0+
+ */
+
+#include <efi_selftest.h>
+#include <vsprintf.h>
+
+static const struct efi_system_table *systable;
+static const struct efi_boot_services *boottime;
+static const struct efi_runtime_services *runtime;
+static efi_handle_t handle;
+static u16 reset_message[] = L"Selftest completed";
+
+/*
+ * Exit the boot services.
+ *
+ * The size of the memory map is determined.
+ * Pool memory is allocated to copy the memory map.
+ * The memory amp is copied and the map key is obtained.
+ * The map key is used to exit the boot services.
+ */
+void efi_st_exit_boot_services(void)
+{
+	unsigned long  map_size = 0;
+	unsigned long  map_key;
+	unsigned long desc_size;
+	u32 desc_version;
+	efi_status_t ret;
+	struct efi_mem_desc *memory_map;
+
+	ret = boottime->get_memory_map(&map_size, NULL, &map_key, &desc_size,
+				       &desc_version);
+	if (ret != EFI_BUFFER_TOO_SMALL) {
+		efi_st_printf("ERROR: GetMemoryMap did not return "
+			      "EFI_BUFFER_TOO_SMALL\n");
+		return;
+	}
+	/* Allocate extra space for newly allocated memory */
+	map_size += sizeof(struct efi_mem_desc);
+	ret = boottime->allocate_pool(EFI_BOOT_SERVICES_DATA, map_size,
+				      (void **)&memory_map);
+	if (ret != EFI_SUCCESS) {
+		efi_st_printf("ERROR: AllocatePool did not return "
+			      "EFI_SUCCESS\n");
+		return;
+	}
+	ret = boottime->get_memory_map(&map_size, memory_map, &map_key,
+				       &desc_size, &desc_version);
+	if (ret != EFI_SUCCESS) {
+		efi_st_printf("ERROR: GetMemoryMap did not return "
+			      "EFI_SUCCESS\n");
+		return;
+	}
+	ret = boottime->exit_boot_services(handle, map_key);
+	if (ret != EFI_SUCCESS) {
+		efi_st_printf("ERROR: ExitBootServices did not return "
+			      "EFI_SUCCESS\n");
+		return;
+	}
+	efi_st_printf("\nBoot services terminated\n");
+}
+
+/*
+ * Set up a test.
+ *
+ * @test	the test to be executed
+ * @failures	counter that will be incremented if a failure occurs
+ */
+static int setup(struct efi_unit_test *test, unsigned int *failures)
+{
+	int ret;
+
+	if (!test->setup)
+		return 0;
+	efi_st_printf("\nSetting up '%s'\n", test->name);
+	ret = test->setup(handle, systable);
+	if (ret) {
+		efi_st_printf("ERROR: Setting up '%s' failed\n", test->name);
+		++*failures;
+	} else {
+		efi_st_printf("Setting up '%s' succeeded\n", test->name);
+	}
+	return ret;
+}
+
+/*
+ * Execute a test.
+ *
+ * @test	the test to be executed
+ * @failures	counter that will be incremented if a failure occurs
+ */
+static int execute(struct efi_unit_test *test, unsigned int *failures)
+{
+	int ret;
+
+	if (!test->execute)
+		return 0;
+	efi_st_printf("\nExecuting '%s'\n", test->name);
+	ret = test->execute();
+	if (ret) {
+		efi_st_printf("ERROR: Executing '%s' failed\n", test->name);
+		++*failures;
+	} else {
+		efi_st_printf("Executing '%s' succeeded\n", test->name);
+	}
+	return ret;
+}
+
+/*
+ * Tear down a test.
+ *
+ * @test	the test to be torn down
+ * @failures	counter that will be incremented if a failure occurs
+ */
+static int teardown(struct efi_unit_test *test, unsigned int *failures)
+{
+	int ret;
+
+	if (!test->teardown)
+		return 0;
+	efi_st_printf("\nTearing down '%s'\n", test->name);
+	ret = test->teardown();
+	if (ret) {
+		efi_st_printf("ERROR: Tearing down '%s' failed\n", test->name);
+		++*failures;
+	} else {
+		efi_st_printf("Tearing down '%s' succeeded\n", test->name);
+	}
+	return ret;
+}
+
+/*
+ * Execute selftest of the EFI API
+ *
+ * This is the main entry point of the EFI selftest application.
+ *
+ * All tests use a driver model and are run in three phases:
+ * setup, execute, teardown.
+ *
+ * A test may be setup and executed at boottime,
+ * it may be setup at boottime and executed at runtime,
+ * or it may be setup and executed at runtime.
+ *
+ * After executing all tests the system is reset.
+ *
+ * @image_handle:	handle of the loaded EFI image
+ * @systab:		EFI system table
+ */
+efi_status_t EFIAPI efi_selftest(efi_handle_t image_handle,
+				 struct efi_system_table *systab)
+{
+	struct efi_unit_test *test;
+	unsigned int failures = 0;
+
+	systable = systab;
+	boottime = systable->boottime;
+	runtime = systable->runtime;
+	handle = image_handle;
+	con_out = systable->con_out;
+	con_in = systable->con_in;
+
+	efi_st_printf("\nTesting EFI API implementation\n");
+
+	efi_st_printf("\nNumber of tests to execute: %u\n",
+		      ll_entry_count(struct efi_unit_test, efi_unit_test));
+
+	/* Execute boottime tests */
+	for (test = ll_entry_start(struct efi_unit_test, efi_unit_test);
+	     test < ll_entry_end(struct efi_unit_test, efi_unit_test); ++test) {
+		if (test->phase == EFI_EXECUTE_BEFORE_BOOTTIME_EXIT) {
+			setup(test, &failures);
+			execute(test, &failures);
+			teardown(test, &failures);
+		}
+	}
+
+	/* Execute mixed tests */
+	for (test = ll_entry_start(struct efi_unit_test, efi_unit_test);
+	     test < ll_entry_end(struct efi_unit_test, efi_unit_test); ++test) {
+		if (test->phase == EFI_SETUP_BEFORE_BOOTTIME_EXIT)
+			setup(test, &failures);
+	}
+
+	efi_st_exit_boot_services();
+
+	for (test = ll_entry_start(struct efi_unit_test, efi_unit_test);
+	     test < ll_entry_end(struct efi_unit_test, efi_unit_test); ++test) {
+		if (test->phase == EFI_SETUP_BEFORE_BOOTTIME_EXIT) {
+			execute(test, &failures);
+			teardown(test, &failures);
+		}
+	}
+
+	/* Execute runtime tests */
+	for (test = ll_entry_start(struct efi_unit_test, efi_unit_test);
+	     test < ll_entry_end(struct efi_unit_test, efi_unit_test); ++test) {
+		if (test->phase == EFI_SETUP_AFTER_BOOTTIME_EXIT) {
+			setup(test, &failures);
+			execute(test, &failures);
+			teardown(test, &failures);
+		}
+	}
+
+	/* Give feedback */
+	efi_st_printf("\nSummary: %u failures\n\n", failures);
+
+	/* Reset system */
+	efi_st_printf("Preparing for reset. Press any key.\n");
+	efi_st_get_key();
+	runtime->reset_system(EFI_RESET_WARM, EFI_NOT_READY,
+			      sizeof(reset_message), reset_message);
+	efi_st_printf("\nERROR: reset failed.\n");
+
+	return EFI_UNSUPPORTED;
+}
diff --git a/lib/efi_selftest/efi_selftest_console.c b/lib/efi_selftest/efi_selftest_console.c
new file mode 100644
index 0000000000..7b5b724a61
--- /dev/null
+++ b/lib/efi_selftest/efi_selftest_console.c
@@ -0,0 +1,187 @@
+/*
+ * EFI efi_selftest
+ *
+ * Copyright (c) 2017 Heinrich Schuchardt <xypron.glpk@gmx.de>
+ *
+ * SPDX-License-Identifier:     GPL-2.0+
+ */
+
+#include <efi_selftest.h>
+#include <vsprintf.h>
+
+struct efi_simple_text_output_protocol *con_out;
+struct efi_simple_input_interface *con_in;
+
+/*
+ * Print a pointer to an u16 string
+ *
+ * @pointer: pointer
+ * @buf: pointer to buffer address
+ * on return position of terminating zero word
+ */
+static void pointer(void *pointer, u16 **buf)
+{
+	int i;
+	u16 c;
+	uintptr_t p = (uintptr_t)pointer;
+	u16 *pos = *buf;
+
+	for (i = 8 * sizeof(p) - 4; i >= 0; i -= 4) {
+		c = (p >> i) & 0x0f;
+		c += '0';
+		if (c > '9')
+			c += 'a' - '9' - 1;
+		*pos++ = c;
+	}
+	*pos = 0;
+	*buf = pos;
+}
+
+/*
+ * Print an unsigned 32bit value as decimal number to an u16 string
+ *
+ * @value: value to be printed
+ * @buf: pointer to buffer address
+ * on return position of terminating zero word
+ */
+static void uint2dec(u32 value, u16 **buf)
+{
+	u16 *pos = *buf;
+	int i;
+	u16 c;
+	u64 f;
+
+	/*
+	 * Increment by .5 and multiply with
+	 * (2 << 60) / 1,000,000,000 = 0x44B82FA0.9B5A52CC
+	 * to move the first digit to bit 60-63.
+	 */
+	f = 0x225C17D0;
+	f += (0x9B5A52DULL * value) >> 28;
+	f += 0x44B82FA0ULL * value;
+
+	for (i = 0; i < 10; ++i) {
+		/* Write current digit */
+		c = f >> 60;
+		if (c || pos != *buf)
+			*pos++ = c + '0';
+		/* Eliminate current digit */
+		f &= 0xfffffffffffffff;
+		/* Get next digit */
+		f *= 0xaULL;
+	}
+	if (pos == *buf)
+		*pos++ = '0';
+	*pos = 0;
+	*buf = pos;
+}
+
+/*
+ * Print a signed 32bit value as decimal number to an u16 string
+ *
+ * @value: value to be printed
+ * @buf: pointer to buffer address
+ * on return position of terminating zero word
+ */
+static void int2dec(s32 value, u16 **buf)
+{
+	u32 u;
+	u16 *pos = *buf;
+
+	if (value < 0) {
+		*pos++ = '-';
+		u = -value;
+	} else {
+		u = value;
+	}
+	uint2dec(u, &pos);
+	*buf = pos;
+}
+
+/*
+ * Print a formatted string to the EFI console
+ *
+ * @fmt: format string
+ * @...: optional arguments
+ */
+void efi_st_printf(const char *fmt, ...)
+{
+	va_list args;
+	u16 buf[160];
+	const char *c;
+	u16 *pos = buf;
+	const char *s;
+
+	va_start(args, fmt);
+
+	c = fmt;
+	for (; *c; ++c) {
+		switch (*c) {
+		case '\\':
+			++c;
+			switch (*c) {
+			case '\0':
+				--c;
+				break;
+			case 'n':
+				*pos++ = '\n';
+				break;
+			case 'r':
+				*pos++ = '\r';
+				break;
+			case 't':
+				*pos++ = '\t';
+				break;
+			default:
+				*pos++ = *c;
+			}
+			break;
+		case '%':
+			++c;
+			switch (*c) {
+			case '\0':
+				--c;
+				break;
+			case 'd':
+				int2dec(va_arg(args, s32), &pos);
+				break;
+			case 'p':
+				pointer(va_arg(args, void*), &pos);
+				break;
+			case 's':
+				s = va_arg(args, const char *);
+				for (; *s; ++s)
+					*pos++ = *s;
+				break;
+			case 'u':
+				uint2dec(va_arg(args, u32), &pos);
+				break;
+			default:
+				break;
+			}
+			break;
+		default:
+			*pos++ = *c;
+		}
+	}
+	va_end(args);
+	*pos = 0;
+	con_out->output_string(con_out, buf);
+}
+
+/*
+ * Reads an Unicode character from the input device.
+ *
+ * @return: Unicode character
+ */
+u16 efi_st_get_key(void)
+{
+	struct efi_input_key input_key;
+	efi_status_t ret;
+
+	/* Wait for next key */
+	do {
+		ret = con_in->read_key_stroke(con_in, &input_key);
+	} while (ret == EFI_NOT_READY);
+	return input_key.unicode_char;
+}
-- 
2.11.0

[U-Boot] [PATCH 03/10] test/py: add a test calling the EFI selftest

[Heinrich Schuchardt]

A Python test script is provided that runs the EFI selftest
if CONFIG_CMD_EFI_SELFTEST=y.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 test/py/tests/test_efi_selftest.py | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)
 create mode 100644 test/py/tests/test_efi_selftest.py

diff --git a/test/py/tests/test_efi_selftest.py b/test/py/tests/test_efi_selftest.py
new file mode 100644
index 0000000000..76e282a6c7
--- /dev/null
+++ b/test/py/tests/test_efi_selftest.py
@@ -0,0 +1,25 @@
+# Copyright (c) 2016, NVIDIA CORPORATION. All rights reserved.
+# Copyright (c) 2017, Heinrich Schuchardt <xypron.glpk@gmx.de>
+#
+# SPDX-License-Identifier: GPL-2.0
+
+# Test efi API implementation
+
+import pytest
+import u_boot_utils
+
+ at pytest.mark.buildconfigspec('cmd_bootefi_selftest')
+def test_efi_selftest(u_boot_console):
+	"""
+	Run bootefi selftest
+	"""
+
+	u_boot_console.run_command(cmd='bootefi selftest', wait_for_prompt=False)
+	m = u_boot_console.p.expect(['Summary: 0 failures', 'Press any key'])
+	if m != 0:
+		raise Exception('Failures occured during the EFI selftest')
+	u_boot_console.run_command(cmd='', wait_for_echo=False, wait_for_prompt=False);
+	m = u_boot_console.p.expect(['resetting', 'U-Boot'])
+	if m != 0:
+		raise Exception('Reset failed during the EFI selftest')
+	u_boot_console.restart_uboot();
-- 
2.11.0

[U-Boot] [PATCH 04/10] efi_loader: implement queueing of the notification function

[Heinrich Schuchardt]

For the correct implementation of the task priority level (TPL)
calling the notification function must be queued.

Add a status field 'queued' to events.

In function efi_signal_event set status queued if a notification
function exists and reset it after we have called the function.
A later patch will add a check of the TPL here.

In efi_create_event and efi_close_event unset the queued status.

In function efi_wait_for_event and efi_check_event
queue the notification function.

In efi_timer_check call the efi_notify_event
if the status queued is set.
For all timer events set status signaled.

In efi_console_timer_notify set the signaled state of the
WaitForKey event.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 include/efi_loader.h          |  4 +++-
 lib/efi_loader/efi_boottime.c | 40 ++++++++++++++++++++++++++++++----------
 lib/efi_loader/efi_console.c  |  4 +++-
 3 files changed, 36 insertions(+), 12 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index f74b33d589..25398ba40c 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -131,7 +131,8 @@ struct efi_object {
  * @nofify_function:	Function to call when the event is triggered
  * @notify_context:	Data to be passed to the notify function
  * @trigger_type:	Type of timer, see efi_set_timer
- * @signaled:		The notify function was already called
+ * @queued:		The notification functionis queued
+ * @signaled:		The event occured
  */
 struct efi_event {
 	uint32_t type;
@@ -141,6 +142,7 @@ struct efi_event {
 	u64 trigger_next;
 	u64 trigger_time;
 	enum efi_timer_delay trigger_type;
+	int queued;
 	int signaled;
 };
 
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 2c9379a8ae..408b4a9097 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -159,13 +159,13 @@ static u64 efi_div10(u64 a)
 
 void efi_signal_event(struct efi_event *event)
 {
-	if (event->signaled)
-		return;
-	event->signaled = 1;
-	if (event->type & EVT_NOTIFY_SIGNAL) {
+	if (event->notify_function) {
+		event->queued = 1;
+		/* Put missing TPL check here */
 		EFI_CALL_VOID(event->notify_function(event,
 						     event->notify_context));
 	}
+	event->queued = 0;
 }
 
 static efi_status_t efi_unsupported(const char *funcname)
@@ -276,6 +276,7 @@ efi_status_t efi_create_event(uint32_t type, UINTN notify_tpl,
 		efi_events[i].notify_context = notify_context;
 		/* Disable timers on bootup */
 		efi_events[i].trigger_next = -1ULL;
+		efi_events[i].queued = 0;
 		efi_events[i].signaled = 0;
 		*event = &efi_events[i];
 		return EFI_SUCCESS;
@@ -307,16 +308,25 @@ void efi_timer_check(void)
 	u64 now = timer_get_us();
 
 	for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
-		if (!efi_events[i].type ||
-		    !(efi_events[i].type & EVT_TIMER) ||
-		    efi_events[i].trigger_type == EFI_TIMER_STOP ||
+		if (!efi_events[i].type)
+			continue;
+		if (efi_events[i].queued)
+			efi_signal_event(&efi_events[i]);
+		if (!(efi_events[i].type & EVT_TIMER) ||
 		    now < efi_events[i].trigger_next)
 			continue;
-		if (efi_events[i].trigger_type == EFI_TIMER_PERIODIC) {
+		switch (efi_events[i].trigger_type) {
+		case EFI_TIMER_RELATIVE:
+			efi_events[i].trigger_type = EFI_TIMER_STOP;
+			break;
+		case EFI_TIMER_PERIODIC:
 			efi_events[i].trigger_next +=
 				efi_events[i].trigger_time;
-			efi_events[i].signaled = 0;
+			break;
+		default:
+			continue;
 		}
+		efi_events[i].signaled = 1;
 		efi_signal_event(&efi_events[i]);
 	}
 	WATCHDOG_RESET();
@@ -377,6 +387,7 @@ static efi_status_t EFIAPI efi_wait_for_event(unsigned long num_events,
 	/* Check parameters */
 	if (!num_events || !event)
 		return EFI_EXIT(EFI_INVALID_PARAMETER);
+	/* Put missing TPL check here */
 	for (i = 0; i < num_events; ++i) {
 		for (j = 0; j < ARRAY_SIZE(efi_events); ++j) {
 			if (event[i] == &efi_events[j])
@@ -386,6 +397,8 @@ static efi_status_t EFIAPI efi_wait_for_event(unsigned long num_events,
 known_event:
 		if (!event[i]->type || event[i]->type & EVT_NOTIFY_SIGNAL)
 			return EFI_EXIT(EFI_INVALID_PARAMETER);
+		if (!event[i]->signaled)
+			efi_signal_event(event[i]);
 	}
 
 	/* Wait for signal */
@@ -418,7 +431,11 @@ static efi_status_t EFIAPI efi_signal_event_ext(struct efi_event *event)
 	for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
 		if (event != &efi_events[i])
 			continue;
-		efi_signal_event(event);
+		if (event->signaled)
+			break;
+		event->signaled = 1;
+		if (event->type & EVT_NOTIFY_SIGNAL)
+			efi_signal_event(event);
 		break;
 	}
 	return EFI_EXIT(EFI_SUCCESS);
@@ -433,6 +450,7 @@ static efi_status_t EFIAPI efi_close_event(struct efi_event *event)
 		if (event == &efi_events[i]) {
 			event->type = 0;
 			event->trigger_next = -1ULL;
+			event->queued = 0;
 			event->signaled = 0;
 			return EFI_EXIT(EFI_SUCCESS);
 		}
@@ -451,6 +469,8 @@ static efi_status_t EFIAPI efi_check_event(struct efi_event *event)
 			continue;
 		if (!event->type || event->type & EVT_NOTIFY_SIGNAL)
 			break;
+		if (!event->signaled)
+			efi_signal_event(event);
 		if (event->signaled)
 			return EFI_EXIT(EFI_SUCCESS);
 		return EFI_EXIT(EFI_NOT_READY);
diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
index 3fc82b8726..65c07fdf37 100644
--- a/lib/efi_loader/efi_console.c
+++ b/lib/efi_loader/efi_console.c
@@ -426,8 +426,10 @@ static void EFIAPI efi_console_timer_notify(struct efi_event *event,
 					    void *context)
 {
 	EFI_ENTRY("%p, %p", event, context);
-	if (tstc())
+	if (tstc()) {
+		efi_con_in.wait_for_key->signaled = 1;
 		efi_signal_event(efi_con_in.wait_for_key);
+		}
 	EFI_EXIT(EFI_SUCCESS);
 }
 
-- 
2.11.0

[U-Boot] [PATCH 05/10] efi_loader: efi_set_timer: reset signaled state

[Heinrich Schuchardt]

We should be able to call efi_set_timer repeatedly.
So let us reset the signaled state here.

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

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 408b4a9097..73793cd986 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -363,6 +363,7 @@ efi_status_t efi_set_timer(struct efi_event *event, enum efi_timer_delay type,
 		}
 		event->trigger_type = type;
 		event->trigger_time = trigger_time;
+		event->signaled = 0;
 		return EFI_SUCCESS;
 	}
 	return EFI_INVALID_PARAMETER;
-- 
2.11.0

[U-Boot] [PATCH 06/10] efi_selftest: provide unit test for event services

[Heinrich Schuchardt]

This unit test uses timer events to check the implementation
of the following boottime services:
CreateEvent, CloseEvent, WaitForEvent, CheckEvent, SetTimer

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_selftest/Makefile              |   5 +-
 lib/efi_selftest/efi_selftest_events.c | 195 +++++++++++++++++++++++++++++++++
 2 files changed, 199 insertions(+), 1 deletion(-)
 create mode 100644 lib/efi_selftest/efi_selftest_events.c

diff --git a/lib/efi_selftest/Makefile b/lib/efi_selftest/Makefile
index 34f5ff1838..a71c8bf937 100644
--- a/lib/efi_selftest/Makefile
+++ b/lib/efi_selftest/Makefile
@@ -11,7 +11,10 @@ CFLAGS_efi_selftest.o := $(CFLAGS_EFI)
 CFLAGS_REMOVE_efi_selftest.o := $(CFLAGS_NON_EFI)
 CFLAGS_efi_selftest_console.o := $(CFLAGS_EFI)
 CFLAGS_REMOVE_efi_selftest_console.o := $(CFLAGS_NON_EFI)
+CFLAGS_efi_selftest_events.o := $(CFLAGS_EFI)
+CFLAGS_REMOVE_efi_selftest_events.o := $(CFLAGS_NON_EFI)
 
 obj-$(CONFIG_CMD_BOOTEFI_SELFTEST) += \
 efi_selftest.o \
-efi_selftest_console.o
+efi_selftest_console.o \
+efi_selftest_events.o
diff --git a/lib/efi_selftest/efi_selftest_events.c b/lib/efi_selftest/efi_selftest_events.c
new file mode 100644
index 0000000000..c4f66952b9
--- /dev/null
+++ b/lib/efi_selftest/efi_selftest_events.c
@@ -0,0 +1,195 @@
+/*
+ * efi_selftest_events
+ *
+ * Copyright (c) 2017 Heinrich Schuchardt <xypron.glpk@gmx.de>
+ *
+ * SPDX-License-Identifier:     GPL-2.0+
+ *
+ * This unit test uses timer events to check the implementation
+ * of the following boottime services:
+ * CreateEvent, CloseEvent, WaitForEvent, CheckEvent, SetTimer.
+ */
+
+#include <efi_selftest.h>
+
+static struct efi_event *event_notify;
+static struct efi_event *event_wait;
+static unsigned int counter;
+static struct efi_boot_services *boottime;
+
+/*
+ * Notification function, increments a counter.
+ *
+ * @event	notified event
+ * @context	pointer to the counter
+ */
+static void EFIAPI notify(struct efi_event *event, void *context)
+{
+	if (!context)
+		return;
+	++*(unsigned int *)context;
+}
+
+/*
+ * Setup unit test.
+ *
+ * Create two timer events.
+ * One with EVT_NOTIFY_SIGNAL, the other with EVT_NOTIFY_WAIT.
+ *
+ * @handle:	handle of the loaded image
+ * @systable:	system table
+ */
+static int setup(const efi_handle_t handle,
+		 const struct efi_system_table *systable)
+{
+	efi_status_t ret;
+
+	boottime = systable->boottime;
+
+	ret = boottime->create_event(EVT_TIMER | EVT_NOTIFY_SIGNAL,
+				     TPL_CALLBACK, notify, (void *)&counter,
+				     &event_notify);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("could not create event\n");
+		return 1;
+	}
+	ret = boottime->create_event(EVT_TIMER | EVT_NOTIFY_WAIT,
+				     TPL_CALLBACK, notify, NULL, &event_wait);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("could not create event\n");
+		return 1;
+	}
+	return 0;
+}
+
+/*
+ * Tear down unit test.
+ *
+ * Close the events created in setup.
+ */
+static int teardown(void)
+{
+	efi_status_t ret;
+
+	if (event_notify) {
+		ret = boottime->close_event(event_notify);
+		event_notify = NULL;
+		if (ret != EFI_SUCCESS) {
+			efi_st_error("could not close event\n");
+			return 1;
+		}
+	}
+	if (event_wait) {
+		ret = boottime->close_event(event_wait);
+		event_wait = NULL;
+		if (ret != EFI_SUCCESS) {
+			efi_st_error("could not close event\n");
+			return 1;
+		}
+	}
+	return 0;
+}
+
+/*
+ * Execute unit test.
+ *
+ * Run a 10 ms periodic timer and check that it is called 10 times
+ * while waiting for 100 ms single shot timer.
+ *
+ * Run a 100 ms single shot timer and check that it is called once
+ * while waiting for 100 ms periodic timer for two periods.
+ */
+static int execute(void)
+{
+	unsigned long index;
+	efi_status_t ret;
+
+	/* Set 10 ms timer */
+	counter = 0;
+	ret = boottime->set_timer(event_notify, EFI_TIMER_PERIODIC, 100000);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("Could not set timer\n");
+		return 1;
+	}
+	/* Set 100 ms timer */
+	ret = boottime->set_timer(event_wait, EFI_TIMER_RELATIVE, 1000000);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("Could not set timer\n");
+		return 1;
+	}
+
+	index = 5;
+	ret = boottime->wait_for_event(1, &event_wait, &index);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("Could not wait for event\n");
+		return 1;
+	}
+	ret = boottime->check_event(event_wait);
+	if (ret != EFI_NOT_READY) {
+		efi_st_error("Signaled state was not cleared.\n");
+		efi_st_printf("ret = %u\n", (unsigned int)ret);
+		return 1;
+	}
+	if (index != 0) {
+		efi_st_error("WaitForEvent returned wrong index\n");
+		return 1;
+	}
+	efi_st_printf("Counter periodic: %u\n", counter);
+	if (counter < 8 || counter > 12) {
+		efi_st_error("Incorrect timing of events\n");
+		return 1;
+	}
+	ret = boottime->set_timer(event_notify, EFI_TIMER_STOP, 0);
+	if (index != 0) {
+		efi_st_error("Could not cancel timer\n");
+		return 1;
+	}
+	/* Set 10 ms timer */
+	counter = 0;
+	ret = boottime->set_timer(event_notify, EFI_TIMER_RELATIVE, 100000);
+	if (index != 0) {
+		efi_st_error("Could not set timer\n");
+		return 1;
+	}
+	/* Set 100 ms timer */
+	ret = boottime->set_timer(event_wait, EFI_TIMER_PERIODIC, 1000000);
+	if (index != 0) {
+		efi_st_error("Could not set timer\n");
+		return 1;
+	}
+	ret = boottime->wait_for_event(1, &event_wait, &index);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("Could not wait for event\n");
+		return 1;
+	}
+	efi_st_printf("Counter single shot: %u\n", counter);
+	if (counter != 1) {
+		efi_st_error("Single shot timer failed\n");
+		return 1;
+	}
+	ret = boottime->wait_for_event(1, &event_wait, &index);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("Could not wait for event\n");
+		return 1;
+	}
+	efi_st_printf("Stopped counter: %u\n", counter);
+	if (counter != 1) {
+		efi_st_error("Stopped timer fired\n");
+		return 1;
+	}
+	ret = boottime->set_timer(event_wait, EFI_TIMER_STOP, 0);
+	if (index != 0) {
+		efi_st_error("Could not cancel timer\n");
+		return 1;
+	}
+
+	return 0;
+}
+
+EFI_UNIT_TEST(events) = {
+	.name = "event services",
+	.phase = EFI_EXECUTE_BEFORE_BOOTTIME_EXIT,
+	.setup = setup,
+	.execute = execute,
+	.teardown = teardown,
+};
-- 
2.11.0

[U-Boot] [PATCH 07/10] efi_loader: implement task priority level (TPL)

[Heinrich Schuchardt]

Define variable holding tpl.
Implement RaiseTPL and RestoreTPL.
Implement TPL check in efi_signal_event.
Implement TPL check in efi_wait_for_event.

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

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 73793cd986..7c92a68bf4 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -18,6 +18,9 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
+/* Task priority level */
+static UINTN efi_tpl = TPL_APPLICATION;
+
 /* This list contains all the EFI objects our payload has access to */
 LIST_HEAD(efi_obj_list);
 
@@ -161,7 +164,9 @@ void efi_signal_event(struct efi_event *event)
 {
 	if (event->notify_function) {
 		event->queued = 1;
-		/* Put missing TPL check here */
+		/* Check TPL */
+		if (efi_tpl >= event->notify_tpl)
+			return;
 		EFI_CALL_VOID(event->notify_function(event,
 						     event->notify_context));
 	}
@@ -176,14 +181,31 @@ static efi_status_t efi_unsupported(const char *funcname)
 
 static unsigned long EFIAPI efi_raise_tpl(UINTN new_tpl)
 {
+	UINTN old_tpl = efi_tpl;
+
 	EFI_ENTRY("0x%zx", new_tpl);
-	return EFI_EXIT(0);
+
+	if (new_tpl < efi_tpl)
+		debug("WARNING: new_tpl < current_tpl in %s\n", __func__);
+	efi_tpl = new_tpl;
+	if (efi_tpl > TPL_HIGH_LEVEL)
+		efi_tpl = TPL_HIGH_LEVEL;
+
+	EFI_EXIT(EFI_SUCCESS);
+	return old_tpl;
 }
 
 static void EFIAPI efi_restore_tpl(UINTN old_tpl)
 {
 	EFI_ENTRY("0x%zx", old_tpl);
-	efi_unsupported(__func__);
+
+	if (old_tpl > efi_tpl)
+		debug("WARNING: old_tpl > current_tpl in %s\n", __func__);
+	efi_tpl = old_tpl;
+	if (efi_tpl > TPL_HIGH_LEVEL)
+		efi_tpl = TPL_HIGH_LEVEL;
+
+	EFI_EXIT(EFI_SUCCESS);
 }
 
 static efi_status_t EFIAPI efi_allocate_pages_ext(int type, int memory_type,
@@ -388,7 +410,9 @@ static efi_status_t EFIAPI efi_wait_for_event(unsigned long num_events,
 	/* Check parameters */
 	if (!num_events || !event)
 		return EFI_EXIT(EFI_INVALID_PARAMETER);
-	/* Put missing TPL check here */
+	/* Check TPL */
+	if (efi_tpl != TPL_APPLICATION)
+		return EFI_EXIT(EFI_UNSUPPORTED);
 	for (i = 0; i < num_events; ++i) {
 		for (j = 0; j < ARRAY_SIZE(efi_events); ++j) {
 			if (event[i] == &efi_events[j])
-- 
2.11.0

[U-Boot] [PATCH 08/10] efi_selftest: test task priority levels

[Heinrich Schuchardt]

Run a 10 ms periodic timer and check that it is called 10 times
while waiting for 100 ms single shot timer.

Raise the TPL level to the level of the 10 ms timer and observe
that the notification function is not called again.

Lower the TPL level and check that the queued notification
function is called.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_selftest/Makefile           |   5 +-
 lib/efi_selftest/efi_selftest_tpl.c | 214 ++++++++++++++++++++++++++++++++++++
 2 files changed, 218 insertions(+), 1 deletion(-)
 create mode 100644 lib/efi_selftest/efi_selftest_tpl.c

diff --git a/lib/efi_selftest/Makefile b/lib/efi_selftest/Makefile
index a71c8bf937..ddf304e1fa 100644
--- a/lib/efi_selftest/Makefile
+++ b/lib/efi_selftest/Makefile
@@ -13,8 +13,11 @@ CFLAGS_efi_selftest_console.o := $(CFLAGS_EFI)
 CFLAGS_REMOVE_efi_selftest_console.o := $(CFLAGS_NON_EFI)
 CFLAGS_efi_selftest_events.o := $(CFLAGS_EFI)
 CFLAGS_REMOVE_efi_selftest_events.o := $(CFLAGS_NON_EFI)
+CFLAGS_efi_selftest_tpl.o := $(CFLAGS_EFI)
+CFLAGS_REMOVE_efi_selftest_tpl.o := $(CFLAGS_NON_EFI)
 
 obj-$(CONFIG_CMD_BOOTEFI_SELFTEST) += \
 efi_selftest.o \
 efi_selftest_console.o \
-efi_selftest_events.o
+efi_selftest_events.o \
+efi_selftest_tpl.o
diff --git a/lib/efi_selftest/efi_selftest_tpl.c b/lib/efi_selftest/efi_selftest_tpl.c
new file mode 100644
index 0000000000..90ace0f51e
--- /dev/null
+++ b/lib/efi_selftest/efi_selftest_tpl.c
@@ -0,0 +1,214 @@
+/*
+ * efi_selftest_events
+ *
+ * Copyright (c) 2017 Heinrich Schuchardt <xypron.glpk@gmx.de>
+ *
+ * SPDX-License-Identifier:     GPL-2.0+
+ *
+ * This unit test uses timer events to check the handling of
+ * task priority levels.
+ */
+
+#include <efi_selftest.h>
+
+static struct efi_event *event_notify;
+static struct efi_event *event_wait;
+static unsigned int counter;
+static struct efi_boot_services *boottime;
+
+/*
+ * Notification function, increments a counter.
+ *
+ * @event	notified event
+ * @context	pointer to the counter
+ */
+static void EFIAPI notify(struct efi_event *event, void *context)
+{
+	if (!context)
+		return;
+	++*(unsigned int *)context;
+}
+
+/*
+ * Setup unit test.
+ *
+ * Create two timer events.
+ * One with EVT_NOTIFY_SIGNAL, the other with EVT_NOTIFY_WAIT.
+ *
+ * @handle:	handle of the loaded image
+ * @systable:	system table
+ */
+static int setup(const efi_handle_t handle,
+		 const struct efi_system_table *systable)
+{
+	efi_status_t ret;
+
+	boottime = systable->boottime;
+
+	ret = boottime->create_event(EVT_TIMER | EVT_NOTIFY_SIGNAL,
+				     TPL_CALLBACK, notify, (void *)&counter,
+				     &event_notify);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("could not create event\n");
+		return 1;
+	}
+	ret = boottime->create_event(EVT_TIMER | EVT_NOTIFY_WAIT,
+				     TPL_HIGH_LEVEL, notify, NULL, &event_wait);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("could not create event\n");
+		return 1;
+	}
+	return 0;
+}
+
+/*
+ * Tear down unit test.
+ *
+ * Close the events created in setup.
+ */
+static int teardown(void)
+{
+	efi_status_t ret;
+
+	if (event_notify) {
+		ret = boottime->close_event(event_notify);
+		event_notify = NULL;
+		if (ret != EFI_SUCCESS) {
+			efi_st_error("could not close event\n");
+			return 1;
+		}
+	}
+	if (event_wait) {
+		ret = boottime->close_event(event_wait);
+		event_wait = NULL;
+		if (ret != EFI_SUCCESS) {
+			efi_st_error("could not close event\n");
+			return 1;
+		}
+	}
+	boottime->restore_tpl(TPL_APPLICATION);
+	return 0;
+}
+
+/*
+ * Execute unit test.
+ *
+ * Run a 10 ms periodic timer and check that it is called 10 times
+ * while waiting for 100 ms single shot timer.
+ *
+ * Raise the TPL level to the level of the 10 ms timer and observe
+ * that the notification function is not called again.
+ *
+ * Lower the TPL level and check that the queued notification
+ * function is called.
+ */
+static int execute(void)
+{
+	unsigned long index;
+	efi_status_t ret;
+	UINTN old_tpl;
+
+	/* Set 10 ms timer */
+	counter = 0;
+	ret = boottime->set_timer(event_notify, EFI_TIMER_PERIODIC, 100000);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("Could not set timer\n");
+		return 1;
+	}
+	/* Set 100 ms timer */
+	ret = boottime->set_timer(event_wait, EFI_TIMER_RELATIVE, 1000000);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("Could not set timer\n");
+		return 1;
+	}
+	index = 5;
+	ret = boottime->wait_for_event(1, &event_wait, &index);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("Could not wait for event\n");
+		return 1;
+	}
+	ret = boottime->check_event(event_wait);
+	if (ret != EFI_NOT_READY) {
+		efi_st_error("Signaled state was not cleared.\n");
+		efi_st_printf("ret = %u\n", (unsigned int)ret);
+		return 1;
+	}
+	if (index != 0) {
+		efi_st_error("WaitForEvent returned wrong index\n");
+		return 1;
+	}
+	efi_st_printf("Counter with TPL level TPL_APPLICATION: %u\n", counter);
+	if (counter < 8 || counter > 12) {
+		efi_st_error("Incorrect timing of events\n");
+		return 1;
+	}
+	ret = boottime->set_timer(event_notify, EFI_TIMER_STOP, 0);
+	if (index != 0) {
+		efi_st_error("Could not cancel timer\n");
+		return 1;
+	}
+	/* Raise TPL level */
+	old_tpl = boottime->raise_tpl(TPL_CALLBACK);
+	if (old_tpl != TPL_APPLICATION) {
+		efi_st_error("Initial TPL level was not TPL_APPLICATION");
+		return 1;
+	}
+	/* Set 10 ms timer */
+	counter = 0;
+	ret = boottime->set_timer(event_notify, EFI_TIMER_PERIODIC, 100000);
+	if (index != 0) {
+		efi_st_error("Could not set timer\n");
+		return 1;
+	}
+	/* Set 100 ms timer */
+	ret = boottime->set_timer(event_wait, EFI_TIMER_RELATIVE, 1000000);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("Could not set timer\n");
+		return 1;
+	}
+	do {
+		ret = boottime->check_event(event_wait);
+	} while (ret == EFI_NOT_READY);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("Could not check event\n");
+		return 1;
+	}
+	efi_st_printf("Counter with TPL level TPL_CALLBACK: %u\n", counter);
+	if (counter != 0) {
+		efi_st_error("Suppressed timer fired\n");
+		return 1;
+	}
+	/* Set 1 ms timer */
+	ret = boottime->set_timer(event_wait, EFI_TIMER_RELATIVE, 1000);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("Could not set timer\n");
+		return 1;
+	}
+	/* Restore the old TPL level */
+	boottime->restore_tpl(TPL_APPLICATION);
+	ret = boottime->wait_for_event(1, &event_wait, &index);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("Could not wait for event\n");
+		return 1;
+	}
+	efi_st_printf("Counter with TPL level TPL_APPLICATION: %u\n", counter);
+	if (counter < 1) {
+		efi_st_error("Queued timer event did not fire\n");
+		return 1;
+	}
+	ret = boottime->set_timer(event_wait, EFI_TIMER_STOP, 0);
+	if (index != 0) {
+		efi_st_error("Could not cancel timer\n");
+		return 1;
+	}
+
+	return 0;
+}
+
+EFI_UNIT_TEST(tpl) = {
+	.name = "task priority levels",
+	.phase = EFI_EXECUTE_BEFORE_BOOTTIME_EXIT,
+	.setup = setup,
+	.execute = execute,
+	.teardown = teardown,
+};
-- 
2.11.0

[U-Boot] [PATCH 09/10] efi_loader: notify when ExitBootServices is invoked

[Heinrich Schuchardt]

All events of type EVT_SIGNAL_EXIT_BOOT_SERVICES have to be
notified when ExitBootServices is invoked.

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

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 7c92a68bf4..0d234146d7 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -900,8 +900,19 @@ static void efi_exit_caches(void)
 static efi_status_t EFIAPI efi_exit_boot_services(void *image_handle,
 						  unsigned long map_key)
 {
+	int i;
+
 	EFI_ENTRY("%p, %ld", image_handle, map_key);
 
+	/* Notify that ExitBootServices is invoked. */
+	for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
+		if (efi_events[i].type != EVT_SIGNAL_EXIT_BOOT_SERVICES)
+			continue;
+		efi_signal_event(&efi_events[i]);
+	}
+	/* Make sure that notification functions are not called anymore */
+	efi_tpl = TPL_HIGH_LEVEL;
+
 	board_quiesce_devices();
 
 	/* Fix up caches for EFI payloads if necessary */
-- 
2.11.0

[U-Boot] [PATCH 10/10] efi_selftest: check notification of ExitBootServices

[Heinrich Schuchardt]

Check that the notification function of an
EVT_SIGNAL_EXIT_BOOT_SERVICES event is called
exactly once.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_selftest/Makefile                        |   3 +
 lib/efi_selftest/efi_selftest_exitbootservices.c | 106 +++++++++++++++++++++++
 2 files changed, 109 insertions(+)
 create mode 100644 lib/efi_selftest/efi_selftest_exitbootservices.c

diff --git a/lib/efi_selftest/Makefile b/lib/efi_selftest/Makefile
index ddf304e1fa..30f1960933 100644
--- a/lib/efi_selftest/Makefile
+++ b/lib/efi_selftest/Makefile
@@ -13,6 +13,8 @@ CFLAGS_efi_selftest_console.o := $(CFLAGS_EFI)
 CFLAGS_REMOVE_efi_selftest_console.o := $(CFLAGS_NON_EFI)
 CFLAGS_efi_selftest_events.o := $(CFLAGS_EFI)
 CFLAGS_REMOVE_efi_selftest_events.o := $(CFLAGS_NON_EFI)
+CFLAGS_efi_selftest_exitbootservices.o := $(CFLAGS_EFI)
+CFLAGS_REMOVE_efi_selftest_exitbootservices.o := $(CFLAGS_NON_EFI)
 CFLAGS_efi_selftest_tpl.o := $(CFLAGS_EFI)
 CFLAGS_REMOVE_efi_selftest_tpl.o := $(CFLAGS_NON_EFI)
 
@@ -20,4 +22,5 @@ obj-$(CONFIG_CMD_BOOTEFI_SELFTEST) += \
 efi_selftest.o \
 efi_selftest_console.o \
 efi_selftest_events.o \
+efi_selftest_exitbootservices.o \
 efi_selftest_tpl.o
diff --git a/lib/efi_selftest/efi_selftest_exitbootservices.c b/lib/efi_selftest/efi_selftest_exitbootservices.c
new file mode 100644
index 0000000000..60271e6180
--- /dev/null
+++ b/lib/efi_selftest/efi_selftest_exitbootservices.c
@@ -0,0 +1,106 @@
+/*
+ * efi_selftest_events
+ *
+ * Copyright (c) 2017 Heinrich Schuchardt <xypron.glpk@gmx.de>
+ *
+ * SPDX-License-Identifier:     GPL-2.0+
+ *
+ * This unit test checks that the notification function of an
+ * EVT_SIGNAL_EXIT_BOOT_SERVICES event is called exactly once.
+ */
+
+#include <efi_selftest.h>
+
+static struct efi_boot_services *boottime;
+static struct efi_event *event_notify;
+static unsigned int counter;
+
+/*
+ * Notification function, increments a counter.
+ *
+ * @event	notified event
+ * @context	pointer to the counter
+ */
+static void EFIAPI notify(struct efi_event *event, void *context)
+{
+	if (!context)
+		return;
+	++*(unsigned int *)context;
+}
+
+/*
+ * Setup unit test.
+ *
+ * Create an EVT_SIGNAL_EXIT_BOOT_SERVICES event.
+ *
+ * @handle:	handle of the loaded image
+ * @systable:	system table
+ */
+static int setup(const efi_handle_t handle,
+		 const struct efi_system_table *systable)
+{
+	efi_status_t ret;
+
+	boottime = systable->boottime;
+
+	counter = 0;
+	ret = boottime->create_event(EVT_SIGNAL_EXIT_BOOT_SERVICES,
+				     TPL_CALLBACK, notify, (void *)&counter,
+				     &event_notify);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("could not create event\n");
+		return 1;
+	}
+	return 0;
+}
+
+/*
+ * Tear down unit test.
+ *
+ * Close the event created in setup.
+ */
+static int teardown(void)
+{
+	efi_status_t ret;
+
+	if (event_notify) {
+		ret = boottime->close_event(event_notify);
+		event_notify = NULL;
+		if (ret != EFI_SUCCESS) {
+			efi_st_error("could not close event\n");
+			return 1;
+		}
+	}
+	return 0;
+}
+
+/*
+ * Execute unit test.
+ *
+ * Check that the notification function of the EVT_SIGNAL_EXIT_BOOT_SERVICES
+ * event has been called.
+ *
+ * Call ExitBootServices again and check that the notification function is
+ * not called again.
+ */
+static int execute(void)
+{
+	if (counter != 1) {
+		efi_st_error("ExitBootServices was not notified");
+		return 1;
+	}
+	efi_st_exit_boot_services();
+	if (counter != 1) {
+		efi_st_error("ExitBootServices was notified twice");
+		return 1;
+	}
+	return 0;
+}
+
+EFI_UNIT_TEST(exitbootservices) = {
+	.name = "ExitBootServices",
+	.phase = EFI_SETUP_BEFORE_BOOTTIME_EXIT,
+	.setup = setup,
+	.execute = execute,
+	.teardown = teardown,
+};
-- 
2.11.0

[U-Boot] [PATCH 00/10] efi_loader: event services & API test

[Simon Glass]

Hi Heinrich,

On 15 September 2017 at 02:06, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> This patch series provides:
> * corrections for the EFI event services
> * a test framework to check the EFI API implementation
> * unit tests covering the event services
>
> The EFI selftest is written such that it could be easily turned
> into a standalone EFI application. But this would require
> modifying the build procedures for EFI. Objcopy cannot generate
> the necessary relocations.
>
> The unit tests are identified by entries in a linker generated
> array to make them as self sufficient as possible.
>
> A Python test case is supplied to call run the EFI tests.
>
> Tested with Travis CI
> https://travis-ci.org/xypron2/u-boot/jobs/275733784
>
> Of all my efi_loader patches these are the first I would like
> to see merged.
>
> Simon has commented on some other patches that he misses
> comments for all EFI API functions. I will add these with
> a separate patch.
>
> Heinrich Schuchardt (10):
>   efi_loader: allow return value in EFI_CALL
>   efi_selftest: provide an EFI selftest application
>   test/py: add a test calling the EFI selftest
>   efi_loader: implement queueing of the notification function
>   efi_loader: efi_set_timer: reset signaled state
>   efi_selftest: provide unit test for event services
>   efi_loader: implement task priority level (TPL)
>   efi_selftest: test task priority levels
>   efi_loader: notify when ExitBootServices is invoked
>   efi_selftest: check notification of ExitBootServices
>

This progress makes significant progress on EFI testing at last. I'm
very pleased to see it. Thank you for all the work you have put into
this.

In addition to this (not instead of) I would like to see EFI code
running under sandbox. I don't at present see a good reason why this
cannot be done. I am going to try to enable EFI loader support in
sandbox to a basic level and then we can see how hard it is to get
some of your tests running directly in sandbox. If that works out then
we can add that into the mix.

I think this would make for an easier development environment for new
EFI features. For some years I have developed all new features in
sandbox and find it painful and unproductive when I need to test every
change manually on a board. It should also allow us to run your tests
(perhaps with some adaptation) with 'make tests' on a local machine
using sandbox. Ultimately it should be possible to expand test
coverage to cover all significant EFI logic.

[..]

Regards,
Simon

[U-Boot] [PATCH 00/10] efi_loader: event services & API test

[Rob Clark]

On Sun, Sep 17, 2017 at 1:58 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi Heinrich,
>
> On 15 September 2017 at 02:06, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> This patch series provides:
>> * corrections for the EFI event services
>> * a test framework to check the EFI API implementation
>> * unit tests covering the event services
>>
>> The EFI selftest is written such that it could be easily turned
>> into a standalone EFI application. But this would require
>> modifying the build procedures for EFI. Objcopy cannot generate
>> the necessary relocations.
>>
>> The unit tests are identified by entries in a linker generated
>> array to make them as self sufficient as possible.
>>
>> A Python test case is supplied to call run the EFI tests.
>>
>> Tested with Travis CI
>> https://travis-ci.org/xypron2/u-boot/jobs/275733784
>>
>> Of all my efi_loader patches these are the first I would like
>> to see merged.
>>
>> Simon has commented on some other patches that he misses
>> comments for all EFI API functions. I will add these with
>> a separate patch.
>>
>> Heinrich Schuchardt (10):
>>   efi_loader: allow return value in EFI_CALL
>>   efi_selftest: provide an EFI selftest application
>>   test/py: add a test calling the EFI selftest
>>   efi_loader: implement queueing of the notification function
>>   efi_loader: efi_set_timer: reset signaled state
>>   efi_selftest: provide unit test for event services
>>   efi_loader: implement task priority level (TPL)
>>   efi_selftest: test task priority levels
>>   efi_loader: notify when ExitBootServices is invoked
>>   efi_selftest: check notification of ExitBootServices
>>
>
> This progress makes significant progress on EFI testing at last. I'm
> very pleased to see it. Thank you for all the work you have put into
> this.
>
> In addition to this (not instead of) I would like to see EFI code
> running under sandbox. I don't at present see a good reason why this
> cannot be done. I am going to try to enable EFI loader support in
> sandbox to a basic level and then we can see how hard it is to get
> some of your tests running directly in sandbox. If that works out then
> we can add that into the mix.

fwiw, I started on trying to get EFI_LOADER working in sandbox earlier
today.. but ran into issues w/ setjmp.  I probably should have kept my
WIP but it was nothing too hard to reproduce (kconfig adds an "||
SANDBOX" to depends, and one or two "#elif defined(CONFIG_SANDBOX)"..
nothing that would take too long to get back to the point I was at
stuck on setjmp/longjmp (and lack of standard system hdrs).. I just
switched to qemu

Other than booting a real OS, seems theoretically possible to get
EFI_LOADER working in sandbox.  It should be enough to (w/ suitable
'host bind') load/run Shell.efi and eventually SCT.efi.

BR,
-R

> I think this would make for an easier development environment for new
> EFI features. For some years I have developed all new features in
> sandbox and find it painful and unproductive when I need to test every
> change manually on a board. It should also allow us to run your tests
> (perhaps with some adaptation) with 'make tests' on a local machine
> using sandbox. Ultimately it should be possible to expand test
> coverage to cover all significant EFI logic.
>
> [..]
>
> Regards,
> Simon

[U-Boot] [PATCH 00/10] efi_loader: event services & API test

[Heinrich Schuchardt]

On 09/17/2017 07:58 PM, Simon Glass wrote:
> Hi Heinrich,
> 
> On 15 September 2017 at 02:06, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> This patch series provides:
>> * corrections for the EFI event services
>> * a test framework to check the EFI API implementation
>> * unit tests covering the event services
>>
>> The EFI selftest is written such that it could be easily turned
>> into a standalone EFI application. But this would require
>> modifying the build procedures for EFI. Objcopy cannot generate
>> the necessary relocations.
>>
>> The unit tests are identified by entries in a linker generated
>> array to make them as self sufficient as possible.
>>
>> A Python test case is supplied to call run the EFI tests.
>>
>> Tested with Travis CI
>> https://travis-ci.org/xypron2/u-boot/jobs/275733784
>>
>> Of all my efi_loader patches these are the first I would like
>> to see merged.
>>
>> Simon has commented on some other patches that he misses
>> comments for all EFI API functions. I will add these with
>> a separate patch.
>>
>> Heinrich Schuchardt (10):
>>   efi_loader: allow return value in EFI_CALL
>>   efi_selftest: provide an EFI selftest application
>>   test/py: add a test calling the EFI selftest
>>   efi_loader: implement queueing of the notification function
>>   efi_loader: efi_set_timer: reset signaled state
>>   efi_selftest: provide unit test for event services
>>   efi_loader: implement task priority level (TPL)
>>   efi_selftest: test task priority levels
>>   efi_loader: notify when ExitBootServices is invoked
>>   efi_selftest: check notification of ExitBootServices
>>
> 
> This progress makes significant progress on EFI testing at last. I'm
> very pleased to see it. Thank you for all the work you have put into
> this.
> 
> In addition to this (not instead of) I would like to see EFI code
> running under sandbox. I don't at present see a good reason why this
> cannot be done. I am going to try to enable EFI loader support in
> sandbox to a basic level and then we can see how hard it is to get
> some of your tests running directly in sandbox. If that works out then
> we can add that into the mix.
> 
> I think this would make for an easier development environment for new
> EFI features. For some years I have developed all new features in
> sandbox and find it painful and unproductive when I need to test every
> change manually on a board. It should also allow us to run your tests
> (perhaps with some adaptation) with 'make tests' on a local machine
> using sandbox. Ultimately it should be possible to expand test
> coverage to cover all significant EFI logic.
> 
> [..]
> 
> Regards,
> Simon
> 
For local testing I have been using qemu-x86_defconfig with
CONFIG_CMD_BOOTEFI_SELFTEST=y.

Cf. https://lists.denx.de/pipermail/u-boot/2017-September/306510.html

As Rob pointed out enabling EFI_LOADER in the sandbox we require an
implementation of arch/sandbox/include/asm/setjmp.h Probably this has to
be based on the host architecture.

arch/x86/cpu/x86_64/setjmp.c teaches that setjmp.c is not yet
implemented in U-Boot for this architecture.

Linux ./arch/x86/um/setjmp_64.S is probably a good starting point.

Regards

Heinrich

[U-Boot] [PATCH 00/10] efi_loader: event services & API test

[Simon Glass]

Hi,

On 17 September 2017 at 13:36, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> On 09/17/2017 07:58 PM, Simon Glass wrote:
>> Hi Heinrich,
>>
>> On 15 September 2017 at 02:06, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>> This patch series provides:
>>> * corrections for the EFI event services
>>> * a test framework to check the EFI API implementation
>>> * unit tests covering the event services
>>>
>>> The EFI selftest is written such that it could be easily turned
>>> into a standalone EFI application. But this would require
>>> modifying the build procedures for EFI. Objcopy cannot generate
>>> the necessary relocations.
>>>
>>> The unit tests are identified by entries in a linker generated
>>> array to make them as self sufficient as possible.
>>>
>>> A Python test case is supplied to call run the EFI tests.
>>>
>>> Tested with Travis CI
>>> https://travis-ci.org/xypron2/u-boot/jobs/275733784
>>>
>>> Of all my efi_loader patches these are the first I would like
>>> to see merged.
>>>
>>> Simon has commented on some other patches that he misses
>>> comments for all EFI API functions. I will add these with
>>> a separate patch.
>>>
>>> Heinrich Schuchardt (10):
>>>   efi_loader: allow return value in EFI_CALL
>>>   efi_selftest: provide an EFI selftest application
>>>   test/py: add a test calling the EFI selftest
>>>   efi_loader: implement queueing of the notification function
>>>   efi_loader: efi_set_timer: reset signaled state
>>>   efi_selftest: provide unit test for event services
>>>   efi_loader: implement task priority level (TPL)
>>>   efi_selftest: test task priority levels
>>>   efi_loader: notify when ExitBootServices is invoked
>>>   efi_selftest: check notification of ExitBootServices
>>>
>>
>> This progress makes significant progress on EFI testing at last. I'm
>> very pleased to see it. Thank you for all the work you have put into
>> this.
>>
>> In addition to this (not instead of) I would like to see EFI code
>> running under sandbox. I don't at present see a good reason why this
>> cannot be done. I am going to try to enable EFI loader support in
>> sandbox to a basic level and then we can see how hard it is to get
>> some of your tests running directly in sandbox. If that works out then
>> we can add that into the mix.
>>
>> I think this would make for an easier development environment for new
>> EFI features. For some years I have developed all new features in
>> sandbox and find it painful and unproductive when I need to test every
>> change manually on a board. It should also allow us to run your tests
>> (perhaps with some adaptation) with 'make tests' on a local machine
>> using sandbox. Ultimately it should be possible to expand test
>> coverage to cover all significant EFI logic.
>>
>> [..]
>>
>> Regards,
>> Simon
>>
> For local testing I have been using qemu-x86_defconfig with
> CONFIG_CMD_BOOTEFI_SELFTEST=y.
>
> Cf. https://lists.denx.de/pipermail/u-boot/2017-September/306510.html
>
> As Rob pointed out enabling EFI_LOADER in the sandbox we require an
> implementation of arch/sandbox/include/asm/setjmp.h Probably this has to
> be based on the host architecture.
>
> arch/x86/cpu/x86_64/setjmp.c teaches that setjmp.c is not yet
> implemented in U-Boot for this architecture.
>
> Linux ./arch/x86/um/setjmp_64.S is probably a good starting point.

I don't think we should implement this in U-Boot, but instead we
should use the host C library version.

I will send some WIP patches later today for discussion.

Regards,
Simon

[U-Boot] [PATCH 01/10] efi_loader: allow return value in EFI_CALL

[Simon Glass]

On 15 September 2017 at 02:06, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> Macro EFI_CALL was introduced to call an UEFI function.
> Unfortunately it does not support return values.
> Most UEFI functions have a return value.
>
> So let's rename EFI_CALL to EFI_CALL_VOID and introduce a
> new EFI_CALL macro that supports return values.
>
> Cc: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  include/efi_loader.h          | 17 +++++++++++++++--
>  lib/efi_loader/efi_boottime.c |  3 ++-
>  2 files changed, 17 insertions(+), 3 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 02/10] efi_selftest: provide an EFI selftest application

[Simon Glass]

On 15 September 2017 at 02:06, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> A testing framework for the EFI API is provided.
> It can be executed with the 'bootefi selftest' command.
>
> It is coded in a way that at a later stage we may turn it
> into a standalone EFI application. The current build system
> does not allow this yet.
>
> All tests use a driver model and are run in three phases:
> setup, execute, teardown.
>
> A test may be setup and executed at boottime,
> it may be setup at boottime and executed at runtime,
> or it may be setup and executed at runtime.
>
> After executing all tests the system is reset.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  MAINTAINERS                             |   5 +-
>  cmd/Kconfig                             |   2 +
>  cmd/bootefi.c                           |  25 +++-
>  include/efi_loader.h                    |   9 ++
>  include/efi_selftest.h                  |  91 +++++++++++++
>  lib/Makefile                            |   1 +
>  lib/efi_selftest/Kconfig                |   7 +
>  lib/efi_selftest/Makefile               |  17 +++
>  lib/efi_selftest/efi_selftest.c         | 219 ++++++++++++++++++++++++++++++++
>  lib/efi_selftest/efi_selftest_console.c | 187 +++++++++++++++++++++++++++
>  10 files changed, 558 insertions(+), 5 deletions(-)
>  create mode 100644 include/efi_selftest.h
>  create mode 100644 lib/efi_selftest/Kconfig
>  create mode 100644 lib/efi_selftest/Makefile
>  create mode 100644 lib/efi_selftest/efi_selftest.c
>  create mode 100644 lib/efi_selftest/efi_selftest_console.c

Reviewed-by: Simon Glass <sjg@chromium.org>

One comment: for the error strings, you should not split them even if
that means that you violate the 80col rule. Otherwise people get
confused when they search for the message and cannot find it in the
code.

- Simon

[U-Boot] [PATCH 04/10] efi_loader: implement queueing of the notification function

[Simon Glass]

Hi Heinrich,

On 15 September 2017 at 02:06, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> For the correct implementation of the task priority level (TPL)
> calling the notification function must be queued.
>
> Add a status field 'queued' to events.
>
> In function efi_signal_event set status queued if a notification
> function exists and reset it after we have called the function.
> A later patch will add a check of the TPL here.
>
> In efi_create_event and efi_close_event unset the queued status.
>
> In function efi_wait_for_event and efi_check_event
> queue the notification function.
>
> In efi_timer_check call the efi_notify_event
> if the status queued is set.
> For all timer events set status signaled.
>
> In efi_console_timer_notify set the signaled state of the
> WaitForKey event.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  include/efi_loader.h          |  4 +++-
>  lib/efi_loader/efi_boottime.c | 40 ++++++++++++++++++++++++++++++----------
>  lib/efi_loader/efi_console.c  |  4 +++-
>  3 files changed, 36 insertions(+), 12 deletions(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index f74b33d589..25398ba40c 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -131,7 +131,8 @@ struct efi_object {
>   * @nofify_function:   Function to call when the event is triggered
>   * @notify_context:    Data to be passed to the notify function
>   * @trigger_type:      Type of timer, see efi_set_timer
> - * @signaled:          The notify function was already called
> + * @queued:            The notification functionis queued

functions

What does this actually mean? Can you expand the comment a bit? I'm
not sure what a value of (for example) 3 would mean. Or if it is just
*whether* the function is queued, then you could use a bool.

> + * @signaled:          The event occured

occurred

[U-Boot] [PATCH 05/10] efi_loader: efi_set_timer: reset signaled state

[Simon Glass]

On 15 September 2017 at 02:06, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> We should be able to call efi_set_timer repeatedly.
> So let us reset the signaled state here.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_loader/efi_boottime.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

Wondering if that should be a bool.

>
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 408b4a9097..73793cd986 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -363,6 +363,7 @@ efi_status_t efi_set_timer(struct efi_event *event, enum efi_timer_delay type,
>                 }
>                 event->trigger_type = type;
>                 event->trigger_time = trigger_time;
> +               event->signaled = 0;
>                 return EFI_SUCCESS;
>         }
>         return EFI_INVALID_PARAMETER;
> --
> 2.11.0
>

[U-Boot] [PATCH 06/10] efi_selftest: provide unit test for event services

[Simon Glass]

On 15 September 2017 at 02:06, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> This unit test uses timer events to check the implementation
> of the following boottime services:
> CreateEvent, CloseEvent, WaitForEvent, CheckEvent, SetTimer
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_selftest/Makefile              |   5 +-
>  lib/efi_selftest/efi_selftest_events.c | 195 +++++++++++++++++++++++++++++++++
>  2 files changed, 199 insertions(+), 1 deletion(-)
>  create mode 100644 lib/efi_selftest/efi_selftest_events.c
>
> diff --git a/lib/efi_selftest/Makefile b/lib/efi_selftest/Makefile
> index 34f5ff1838..a71c8bf937 100644
> --- a/lib/efi_selftest/Makefile
> +++ b/lib/efi_selftest/Makefile
> @@ -11,7 +11,10 @@ CFLAGS_efi_selftest.o := $(CFLAGS_EFI)
>  CFLAGS_REMOVE_efi_selftest.o := $(CFLAGS_NON_EFI)
>  CFLAGS_efi_selftest_console.o := $(CFLAGS_EFI)
>  CFLAGS_REMOVE_efi_selftest_console.o := $(CFLAGS_NON_EFI)
> +CFLAGS_efi_selftest_events.o := $(CFLAGS_EFI)
> +CFLAGS_REMOVE_efi_selftest_events.o := $(CFLAGS_NON_EFI)
>
>  obj-$(CONFIG_CMD_BOOTEFI_SELFTEST) += \
>  efi_selftest.o \
> -efi_selftest_console.o
> +efi_selftest_console.o \
> +efi_selftest_events.o
> diff --git a/lib/efi_selftest/efi_selftest_events.c b/lib/efi_selftest/efi_selftest_events.c
> new file mode 100644
> index 0000000000..c4f66952b9
> --- /dev/null
> +++ b/lib/efi_selftest/efi_selftest_events.c
> @@ -0,0 +1,195 @@
> +/*
> + * efi_selftest_events
> + *
> + * Copyright (c) 2017 Heinrich Schuchardt <xypron.glpk@gmx.de>
> + *
> + * SPDX-License-Identifier:     GPL-2.0+
> + *
> + * This unit test uses timer events to check the implementation
> + * of the following boottime services:
> + * CreateEvent, CloseEvent, WaitForEvent, CheckEvent, SetTimer.
> + */
> +
> +#include <efi_selftest.h>
> +
> +static struct efi_event *event_notify;
> +static struct efi_event *event_wait;
> +static unsigned int counter;
> +static struct efi_boot_services *boottime;
> +
> +/*
> + * Notification function, increments a counter.
> + *
> + * @event      notified event
> + * @context    pointer to the counter
> + */
> +static void EFIAPI notify(struct efi_event *event, void *context)
> +{
> +       if (!context)
> +               return;
> +       ++*(unsigned int *)context;

This is pretty ugly I think. Instead can you create a struct with a
single int member and do something like:

struct notify_contact *ctx = context;

ctx->counter++;

(a better name for counter, too if you can think of one)

> +}
> +
> +/*
> + * Setup unit test.
> + *
> + * Create two timer events.
> + * One with EVT_NOTIFY_SIGNAL, the other with EVT_NOTIFY_WAIT.
> + *
> + * @handle:    handle of the loaded image
> + * @systable:  system table
> + */
> +static int setup(const efi_handle_t handle,
> +                const struct efi_system_table *systable)
> +{
> +       efi_status_t ret;
> +
> +       boottime = systable->boottime;
> +
> +       ret = boottime->create_event(EVT_TIMER | EVT_NOTIFY_SIGNAL,
> +                                    TPL_CALLBACK, notify, (void *)&counter,
> +                                    &event_notify);
> +       if (ret != EFI_SUCCESS) {
> +               efi_st_error("could not create event\n");
> +               return 1;
> +       }
> +       ret = boottime->create_event(EVT_TIMER | EVT_NOTIFY_WAIT,
> +                                    TPL_CALLBACK, notify, NULL, &event_wait);
> +       if (ret != EFI_SUCCESS) {
> +               efi_st_error("could not create event\n");
> +               return 1;
> +       }
> +       return 0;
> +}
> +
> +/*
> + * Tear down unit test.
> + *
> + * Close the events created in setup.
> + */
> +static int teardown(void)
> +{
> +       efi_status_t ret;
> +
> +       if (event_notify) {
> +               ret = boottime->close_event(event_notify);
> +               event_notify = NULL;
> +               if (ret != EFI_SUCCESS) {
> +                       efi_st_error("could not close event\n");
> +                       return 1;
> +               }
> +       }
> +       if (event_wait) {
> +               ret = boottime->close_event(event_wait);
> +               event_wait = NULL;
> +               if (ret != EFI_SUCCESS) {
> +                       efi_st_error("could not close event\n");
> +                       return 1;
> +               }
> +       }
> +       return 0;
> +}
> +
> +/*
> + * Execute unit test.
> + *
> + * Run a 10 ms periodic timer and check that it is called 10 times
> + * while waiting for 100 ms single shot timer.
> + *
> + * Run a 100 ms single shot timer and check that it is called once
> + * while waiting for 100 ms periodic timer for two periods.
> + */
> +static int execute(void)
> +{
> +       unsigned long index;
> +       efi_status_t ret;
> +
> +       /* Set 10 ms timer */
> +       counter = 0;
> +       ret = boottime->set_timer(event_notify, EFI_TIMER_PERIODIC, 100000);
> +       if (ret != EFI_SUCCESS) {
> +               efi_st_error("Could not set timer\n");
> +               return 1;
> +       }
> +       /* Set 100 ms timer */
> +       ret = boottime->set_timer(event_wait, EFI_TIMER_RELATIVE, 1000000);
> +       if (ret != EFI_SUCCESS) {
> +               efi_st_error("Could not set timer\n");
> +               return 1;
> +       }
> +
> +       index = 5;

Is this an non-zero arbitrary value? Please add a comment.

> +       ret = boottime->wait_for_event(1, &event_wait, &index);
> +       if (ret != EFI_SUCCESS) {
> +               efi_st_error("Could not wait for event\n");
> +               return 1;
> +       }
> +       ret = boottime->check_event(event_wait);
> +       if (ret != EFI_NOT_READY) {
> +               efi_st_error("Signaled state was not cleared.\n");
> +               efi_st_printf("ret = %u\n", (unsigned int)ret);
> +               return 1;
> +       }
> +       if (index != 0) {
> +               efi_st_error("WaitForEvent returned wrong index\n");
> +               return 1;
> +       }
> +       efi_st_printf("Counter periodic: %u\n", counter);
> +       if (counter < 8 || counter > 12) {
> +               efi_st_error("Incorrect timing of events\n");
> +               return 1;
> +       }
> +       ret = boottime->set_timer(event_notify, EFI_TIMER_STOP, 0);
> +       if (index != 0) {
> +               efi_st_error("Could not cancel timer\n");
> +               return 1;
> +       }
> +       /* Set 10 ms timer */
> +       counter = 0;
> +       ret = boottime->set_timer(event_notify, EFI_TIMER_RELATIVE, 100000);
> +       if (index != 0) {
> +               efi_st_error("Could not set timer\n");
> +               return 1;
> +       }
> +       /* Set 100 ms timer */
> +       ret = boottime->set_timer(event_wait, EFI_TIMER_PERIODIC, 1000000);
> +       if (index != 0) {
> +               efi_st_error("Could not set timer\n");
> +               return 1;
> +       }
> +       ret = boottime->wait_for_event(1, &event_wait, &index);
> +       if (ret != EFI_SUCCESS) {
> +               efi_st_error("Could not wait for event\n");
> +               return 1;
> +       }

One problem with this test is that it takes 100ms to run. I don't know
if the EFI API has any test facilities, but with sandbox we can update
the timer to make the test take no time.

(that's just a comment, no change needed here)

> +       efi_st_printf("Counter single shot: %u\n", counter);
> +       if (counter != 1) {
> +               efi_st_error("Single shot timer failed\n");
> +               return 1;
> +       }
> +       ret = boottime->wait_for_event(1, &event_wait, &index);
> +       if (ret != EFI_SUCCESS) {
> +               efi_st_error("Could not wait for event\n");
> +               return 1;
> +       }
> +       efi_st_printf("Stopped counter: %u\n", counter);
> +       if (counter != 1) {
> +               efi_st_error("Stopped timer fired\n");
> +               return 1;
> +       }
> +       ret = boottime->set_timer(event_wait, EFI_TIMER_STOP, 0);
> +       if (index != 0) {
> +               efi_st_error("Could not cancel timer\n");
> +               return 1;
> +       }
> +
> +       return 0;
> +}
> +
> +EFI_UNIT_TEST(events) = {
> +       .name = "event services",
> +       .phase = EFI_EXECUTE_BEFORE_BOOTTIME_EXIT,
> +       .setup = setup,
> +       .execute = execute,
> +       .teardown = teardown,
> +};
> --
> 2.11.0
>

Regards,
Simon

[U-Boot] [PATCH 07/10] efi_loader: implement task priority level (TPL)

[Simon Glass]

On 15 September 2017 at 02:06, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> Define variable holding tpl.
> Implement RaiseTPL and RestoreTPL.
> Implement TPL check in efi_signal_event.
> Implement TPL check in efi_wait_for_event.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_loader/efi_boottime.c | 32 ++++++++++++++++++++++++++++----
>  1 file changed, 28 insertions(+), 4 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Looking forward to dropping UINTN!

[U-Boot] [PATCH 08/10] efi_selftest: test task priority levels

[Simon Glass]

On 15 September 2017 at 02:06, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> Run a 10 ms periodic timer and check that it is called 10 times
> while waiting for 100 ms single shot timer.
>
> Raise the TPL level to the level of the 10 ms timer and observe
> that the notification function is not called again.
>
> Lower the TPL level and check that the queued notification
> function is called.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_selftest/Makefile           |   5 +-
>  lib/efi_selftest/efi_selftest_tpl.c | 214 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 218 insertions(+), 1 deletion(-)
>  create mode 100644 lib/efi_selftest/efi_selftest_tpl.c
>
> diff --git a/lib/efi_selftest/Makefile b/lib/efi_selftest/Makefile
> index a71c8bf937..ddf304e1fa 100644
> --- a/lib/efi_selftest/Makefile
> +++ b/lib/efi_selftest/Makefile
> @@ -13,8 +13,11 @@ CFLAGS_efi_selftest_console.o := $(CFLAGS_EFI)
>  CFLAGS_REMOVE_efi_selftest_console.o := $(CFLAGS_NON_EFI)
>  CFLAGS_efi_selftest_events.o := $(CFLAGS_EFI)
>  CFLAGS_REMOVE_efi_selftest_events.o := $(CFLAGS_NON_EFI)
> +CFLAGS_efi_selftest_tpl.o := $(CFLAGS_EFI)
> +CFLAGS_REMOVE_efi_selftest_tpl.o := $(CFLAGS_NON_EFI)
>
>  obj-$(CONFIG_CMD_BOOTEFI_SELFTEST) += \
>  efi_selftest.o \
>  efi_selftest_console.o \
> -efi_selftest_events.o
> +efi_selftest_events.o \
> +efi_selftest_tpl.o
> diff --git a/lib/efi_selftest/efi_selftest_tpl.c b/lib/efi_selftest/efi_selftest_tpl.c
> new file mode 100644
> index 0000000000..90ace0f51e
> --- /dev/null
> +++ b/lib/efi_selftest/efi_selftest_tpl.c
> @@ -0,0 +1,214 @@
> +/*
> + * efi_selftest_events
> + *
> + * Copyright (c) 2017 Heinrich Schuchardt <xypron.glpk@gmx.de>
> + *
> + * SPDX-License-Identifier:     GPL-2.0+
> + *
> + * This unit test uses timer events to check the handling of
> + * task priority levels.
> + */
> +
> +#include <efi_selftest.h>
> +
> +static struct efi_event *event_notify;
> +static struct efi_event *event_wait;
> +static unsigned int counter;
> +static struct efi_boot_services *boottime;
> +
> +/*
> + * Notification function, increments a counter.
> + *
> + * @event      notified event
> + * @context    pointer to the counter
> + */
> +static void EFIAPI notify(struct efi_event *event, void *context)
> +{
> +       if (!context)
> +               return;

Does this ever happen in practice? If not, why check it?

> +       ++*(unsigned int *)context;

Similar comment to previous patch about using a struct here.

> +}
> +
> +/*
> + * Setup unit test.
> + *
> + * Create two timer events.
> + * One with EVT_NOTIFY_SIGNAL, the other with EVT_NOTIFY_WAIT.
> + *
> + * @handle:    handle of the loaded image
> + * @systable:  system table

@return ...

> + */
> +static int setup(const efi_handle_t handle,
> +                const struct efi_system_table *systable)
> +{
> +       efi_status_t ret;
> +
> +       boottime = systable->boottime;
> +
> +       ret = boottime->create_event(EVT_TIMER | EVT_NOTIFY_SIGNAL,
> +                                    TPL_CALLBACK, notify, (void *)&counter,
> +                                    &event_notify);
> +       if (ret != EFI_SUCCESS) {
> +               efi_st_error("could not create event\n");
> +               return 1;

Why not return ret?

> +       }
> +       ret = boottime->create_event(EVT_TIMER | EVT_NOTIFY_WAIT,
> +                                    TPL_HIGH_LEVEL, notify, NULL, &event_wait);
> +       if (ret != EFI_SUCCESS) {
> +               efi_st_error("could not create event\n");
> +               return 1;
> +       }
> +       return 0;
> +}
> +
> +/*
> + * Tear down unit test.
> + *
> + * Close the events created in setup.
> + */
> +static int teardown(void)
> +{
> +       efi_status_t ret;
> +
> +       if (event_notify) {
> +               ret = boottime->close_event(event_notify);
> +               event_notify = NULL;
> +               if (ret != EFI_SUCCESS) {
> +                       efi_st_error("could not close event\n");
> +                       return 1;
> +               }
> +       }
> +       if (event_wait) {
> +               ret = boottime->close_event(event_wait);
> +               event_wait = NULL;
> +               if (ret != EFI_SUCCESS) {
> +                       efi_st_error("could not close event\n");
> +                       return 1;
> +               }
> +       }
> +       boottime->restore_tpl(TPL_APPLICATION);
> +       return 0;
> +}
> +
> +/*
> + * Execute unit test.
> + *
> + * Run a 10 ms periodic timer and check that it is called 10 times
> + * while waiting for 100 ms single shot timer.
> + *
> + * Raise the TPL level to the level of the 10 ms timer and observe
> + * that the notification function is not called again.
> + *
> + * Lower the TPL level and check that the queued notification
> + * function is called.
> + */
> +static int execute(void)
> +{
> +       unsigned long index;
> +       efi_status_t ret;
> +       UINTN old_tpl;
> +
> +       /* Set 10 ms timer */
> +       counter = 0;
> +       ret = boottime->set_timer(event_notify, EFI_TIMER_PERIODIC, 100000);
> +       if (ret != EFI_SUCCESS) {
> +               efi_st_error("Could not set timer\n");
> +               return 1;
> +       }
> +       /* Set 100 ms timer */
> +       ret = boottime->set_timer(event_wait, EFI_TIMER_RELATIVE, 1000000);
> +       if (ret != EFI_SUCCESS) {
> +               efi_st_error("Could not set timer\n");
> +               return 1;
> +       }
> +       index = 5;
> +       ret = boottime->wait_for_event(1, &event_wait, &index);
> +       if (ret != EFI_SUCCESS) {
> +               efi_st_error("Could not wait for event\n");
> +               return 1;
> +       }
> +       ret = boottime->check_event(event_wait);
> +       if (ret != EFI_NOT_READY) {
> +               efi_st_error("Signaled state was not cleared.\n");
> +               efi_st_printf("ret = %u\n", (unsigned int)ret);
> +               return 1;
> +       }
> +       if (index != 0) {
> +               efi_st_error("WaitForEvent returned wrong index\n");
> +               return 1;
> +       }
> +       efi_st_printf("Counter with TPL level TPL_APPLICATION: %u\n", counter);
> +       if (counter < 8 || counter > 12) {
> +               efi_st_error("Incorrect timing of events\n");
> +               return 1;
> +       }
> +       ret = boottime->set_timer(event_notify, EFI_TIMER_STOP, 0);
> +       if (index != 0) {
> +               efi_st_error("Could not cancel timer\n");
> +               return 1;
> +       }
> +       /* Raise TPL level */
> +       old_tpl = boottime->raise_tpl(TPL_CALLBACK);
> +       if (old_tpl != TPL_APPLICATION) {
> +               efi_st_error("Initial TPL level was not TPL_APPLICATION");
> +               return 1;
> +       }
> +       /* Set 10 ms timer */
> +       counter = 0;
> +       ret = boottime->set_timer(event_notify, EFI_TIMER_PERIODIC, 100000);
> +       if (index != 0) {
> +               efi_st_error("Could not set timer\n");
> +               return 1;
> +       }
> +       /* Set 100 ms timer */
> +       ret = boottime->set_timer(event_wait, EFI_TIMER_RELATIVE, 1000000);
> +       if (ret != EFI_SUCCESS) {
> +               efi_st_error("Could not set timer\n");
> +               return 1;
> +       }
> +       do {
> +               ret = boottime->check_event(event_wait);
> +       } while (ret == EFI_NOT_READY);
> +       if (ret != EFI_SUCCESS) {
> +               efi_st_error("Could not check event\n");
> +               return 1;
> +       }
> +       efi_st_printf("Counter with TPL level TPL_CALLBACK: %u\n", counter);
> +       if (counter != 0) {
> +               efi_st_error("Suppressed timer fired\n");
> +               return 1;
> +       }
> +       /* Set 1 ms timer */
> +       ret = boottime->set_timer(event_wait, EFI_TIMER_RELATIVE, 1000);
> +       if (ret != EFI_SUCCESS) {
> +               efi_st_error("Could not set timer\n");
> +               return 1;
> +       }
> +       /* Restore the old TPL level */
> +       boottime->restore_tpl(TPL_APPLICATION);
> +       ret = boottime->wait_for_event(1, &event_wait, &index);
> +       if (ret != EFI_SUCCESS) {
> +               efi_st_error("Could not wait for event\n");
> +               return 1;
> +       }
> +       efi_st_printf("Counter with TPL level TPL_APPLICATION: %u\n", counter);
> +       if (counter < 1) {
> +               efi_st_error("Queued timer event did not fire\n");
> +               return 1;
> +       }
> +       ret = boottime->set_timer(event_wait, EFI_TIMER_STOP, 0);
> +       if (index != 0) {
> +               efi_st_error("Could not cancel timer\n");
> +               return 1;
> +       }
> +
> +       return 0;
> +}
> +
> +EFI_UNIT_TEST(tpl) = {
> +       .name = "task priority levels",
> +       .phase = EFI_EXECUTE_BEFORE_BOOTTIME_EXIT,
> +       .setup = setup,
> +       .execute = execute,
> +       .teardown = teardown,

In general I suggest you put the filename as a prefix on these
function names. It makes __func__ much more useful in the function,
plus the name is more meaningful.

E.g. efi_selftest_tpl_execute()

Regards,
Simon

[U-Boot] [PATCH 09/10] efi_loader: notify when ExitBootServices is invoked

[Simon Glass]

On 15 September 2017 at 02:06, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> All events of type EVT_SIGNAL_EXIT_BOOT_SERVICES have to be
> notified when ExitBootServices is invoked.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_loader/efi_boottime.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 10/10] efi_selftest: check notification of ExitBootServices

[Simon Glass]

Hi Heinrich,

On 15 September 2017 at 02:06, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> Check that the notification function of an
> EVT_SIGNAL_EXIT_BOOT_SERVICES event is called
> exactly once.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_selftest/Makefile                        |   3 +
>  lib/efi_selftest/efi_selftest_exitbootservices.c | 106 +++++++++++++++++++++++
>  2 files changed, 109 insertions(+)
>  create mode 100644 lib/efi_selftest/efi_selftest_exitbootservices.c
>
> diff --git a/lib/efi_selftest/Makefile b/lib/efi_selftest/Makefile
> index ddf304e1fa..30f1960933 100644
> --- a/lib/efi_selftest/Makefile
> +++ b/lib/efi_selftest/Makefile
> @@ -13,6 +13,8 @@ CFLAGS_efi_selftest_console.o := $(CFLAGS_EFI)
>  CFLAGS_REMOVE_efi_selftest_console.o := $(CFLAGS_NON_EFI)
>  CFLAGS_efi_selftest_events.o := $(CFLAGS_EFI)
>  CFLAGS_REMOVE_efi_selftest_events.o := $(CFLAGS_NON_EFI)
> +CFLAGS_efi_selftest_exitbootservices.o := $(CFLAGS_EFI)
> +CFLAGS_REMOVE_efi_selftest_exitbootservices.o := $(CFLAGS_NON_EFI)
>  CFLAGS_efi_selftest_tpl.o := $(CFLAGS_EFI)
>  CFLAGS_REMOVE_efi_selftest_tpl.o := $(CFLAGS_NON_EFI)
>
> @@ -20,4 +22,5 @@ obj-$(CONFIG_CMD_BOOTEFI_SELFTEST) += \
>  efi_selftest.o \
>  efi_selftest_console.o \
>  efi_selftest_events.o \
> +efi_selftest_exitbootservices.o \
>  efi_selftest_tpl.o
> diff --git a/lib/efi_selftest/efi_selftest_exitbootservices.c b/lib/efi_selftest/efi_selftest_exitbootservices.c
> new file mode 100644
> index 0000000000..60271e6180
> --- /dev/null
> +++ b/lib/efi_selftest/efi_selftest_exitbootservices.c
> @@ -0,0 +1,106 @@
> +/*
> + * efi_selftest_events
> + *
> + * Copyright (c) 2017 Heinrich Schuchardt <xypron.glpk@gmx.de>
> + *
> + * SPDX-License-Identifier:     GPL-2.0+
> + *
> + * This unit test checks that the notification function of an
> + * EVT_SIGNAL_EXIT_BOOT_SERVICES event is called exactly once.
> + */
> +
> +#include <efi_selftest.h>
> +
> +static struct efi_boot_services *boottime;
> +static struct efi_event *event_notify;
> +static unsigned int counter;

I wonder if the solution to the context thin in the notify() function
is to put all of these in a struct?

It is nice to group globals into a struct to allow future one-to-many
conversion, reduce the number of symbols in the map and provide a
logical grouping. So this would kill two birds with one stone.

Another idea is to have setup() return the context (e.g. as a void **
final arg). Then that same context can be passed to execute and
teardown. This is similar to how the unit tests work in U-Boot.

Other than that, see my comments to the previous patch which also apply here.

> +
> +/*
> + * Notification function, increments a counter.

You don't need the .

> + *
> + * @event      notified event
> + * @context    pointer to the counter
> + */
> +static void EFIAPI notify(struct efi_event *event, void *context)
> +{
> +       if (!context)
> +               return;
> +       ++*(unsigned int *)context;
> +}
> +
> +/*
> + * Setup unit test.
> + *
> + * Create an EVT_SIGNAL_EXIT_BOOT_SERVICES event.
> + *
> + * @handle:    handle of the loaded image
> + * @systable:  system table

@return...

> + */
> +static int setup(const efi_handle_t handle,
> +                const struct efi_system_table *systable)
> +{
> +       efi_status_t ret;
> +
> +       boottime = systable->boottime;
> +
> +       counter = 0;
> +       ret = boottime->create_event(EVT_SIGNAL_EXIT_BOOT_SERVICES,
> +                                    TPL_CALLBACK, notify, (void *)&counter,
> +                                    &event_notify);
> +       if (ret != EFI_SUCCESS) {
> +               efi_st_error("could not create event\n");
> +               return 1;
> +       }
> +       return 0;
> +}
> +
> +/*
> + * Tear down unit test.
> + *
> + * Close the event created in setup.
> + */
> +static int teardown(void)
> +{
> +       efi_status_t ret;
> +
> +       if (event_notify) {
> +               ret = boottime->close_event(event_notify);
> +               event_notify = NULL;
> +               if (ret != EFI_SUCCESS) {
> +                       efi_st_error("could not close event\n");
> +                       return 1;
> +               }
> +       }
> +       return 0;
> +}
> +
> +/*
> + * Execute unit test.
> + *
> + * Check that the notification function of the EVT_SIGNAL_EXIT_BOOT_SERVICES
> + * event has been called.
> + *
> + * Call ExitBootServices again and check that the notification function is
> + * not called again.
> + */
> +static int execute(void)
> +{
> +       if (counter != 1) {
> +               efi_st_error("ExitBootServices was not notified");
> +               return 1;
> +       }
> +       efi_st_exit_boot_services();
> +       if (counter != 1) {
> +               efi_st_error("ExitBootServices was notified twice");
> +               return 1;
> +       }
> +       return 0;
> +}
> +
> +EFI_UNIT_TEST(exitbootservices) = {
> +       .name = "ExitBootServices",
> +       .phase = EFI_SETUP_BEFORE_BOOTTIME_EXIT,
> +       .setup = setup,
> +       .execute = execute,
> +       .teardown = teardown,
> +};
> --
> 2.11.0
>

[U-Boot] [PATCH 03/10] test/py: add a test calling the EFI selftest

[Simon Glass]

On 15 September 2017 at 02:06, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> A Python test script is provided that runs the EFI selftest
> if CONFIG_CMD_EFI_SELFTEST=y.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  test/py/tests/test_efi_selftest.py | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>  create mode 100644 test/py/tests/test_efi_selftest.py

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 10/10] efi_selftest: check notification of ExitBootServices

[Heinrich Schuchardt]

On 09/25/2017 04:12 AM, Simon Glass wrote:
> Hi Heinrich,
> 
> On 15 September 2017 at 02:06, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> Check that the notification function of an
>> EVT_SIGNAL_EXIT_BOOT_SERVICES event is called
>> exactly once.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>  lib/efi_selftest/Makefile                        |   3 +
>>  lib/efi_selftest/efi_selftest_exitbootservices.c | 106 +++++++++++++++++++++++
>>  2 files changed, 109 insertions(+)
>>  create mode 100644 lib/efi_selftest/efi_selftest_exitbootservices.c
>>
>> diff --git a/lib/efi_selftest/Makefile b/lib/efi_selftest/Makefile
>> index ddf304e1fa..30f1960933 100644
>> --- a/lib/efi_selftest/Makefile
>> +++ b/lib/efi_selftest/Makefile
>> @@ -13,6 +13,8 @@ CFLAGS_efi_selftest_console.o := $(CFLAGS_EFI)
>>  CFLAGS_REMOVE_efi_selftest_console.o := $(CFLAGS_NON_EFI)
>>  CFLAGS_efi_selftest_events.o := $(CFLAGS_EFI)
>>  CFLAGS_REMOVE_efi_selftest_events.o := $(CFLAGS_NON_EFI)
>> +CFLAGS_efi_selftest_exitbootservices.o := $(CFLAGS_EFI)
>> +CFLAGS_REMOVE_efi_selftest_exitbootservices.o := $(CFLAGS_NON_EFI)
>>  CFLAGS_efi_selftest_tpl.o := $(CFLAGS_EFI)
>>  CFLAGS_REMOVE_efi_selftest_tpl.o := $(CFLAGS_NON_EFI)
>>
>> @@ -20,4 +22,5 @@ obj-$(CONFIG_CMD_BOOTEFI_SELFTEST) += \
>>  efi_selftest.o \
>>  efi_selftest_console.o \
>>  efi_selftest_events.o \
>> +efi_selftest_exitbootservices.o \
>>  efi_selftest_tpl.o
>> diff --git a/lib/efi_selftest/efi_selftest_exitbootservices.c b/lib/efi_selftest/efi_selftest_exitbootservices.c
>> new file mode 100644
>> index 0000000000..60271e6180
>> --- /dev/null
>> +++ b/lib/efi_selftest/efi_selftest_exitbootservices.c
>> @@ -0,0 +1,106 @@
>> +/*
>> + * efi_selftest_events
>> + *
>> + * Copyright (c) 2017 Heinrich Schuchardt <xypron.glpk@gmx.de>
>> + *
>> + * SPDX-License-Identifier:     GPL-2.0+
>> + *
>> + * This unit test checks that the notification function of an
>> + * EVT_SIGNAL_EXIT_BOOT_SERVICES event is called exactly once.
>> + */
>> +
>> +#include <efi_selftest.h>
>> +
>> +static struct efi_boot_services *boottime;
>> +static struct efi_event *event_notify;
>> +static unsigned int counter;
> 
> I wonder if the solution to the context thin in the notify() function
> is to put all of these in a struct?

This would mean replacing
boottime->something
by
config->boottime->something.

This does not make the code easier to read.

> 
> It is nice to group globals into a struct to allow future one-to-many
> conversion, reduce the number of symbols in the map and provide a
> logical grouping. So this would kill two birds with one stone.

I typically do not read map files.

With your suggestion the code will be slower and the binary will be larger.

How would putting all private variables into a structure provide logical
grouping?

> 
> Another idea is to have setup() return the context (e.g. as a void **
> final arg). Then that same context can be passed to execute and
> teardown. This is similar to how the unit tests work in U-Boot.

Passing structures as void* is error prone.

Private variables should stay private. There is no reason to make these
accessible outside the unit test.

Passing a void *this would make sense if we would envision having
multiple instances of the same unit test. I can't see that.

> 
> Other than that, see my comments to the previous patch which also apply here.

@Alex
You already accepted the patch series to efi-next and afterwards merged
a bunch of other patches.

I could not see any comment by Simon concerning functionality.
Everything seemed to focus on style.

Shall I provide add-on patches covering Simon's comments or should I
create a new version of the patch series.

Best regards

Heinich

> 
>> +
>> +/*
>> + * Notification function, increments a counter.
> 
> You don't need the .

Should we ever decide to use Doxygen we will need a dot to separate the
first line comment from the rest.

I think it is a pity that U-Boot does not use Doxygen for API documentation.

Regards

Heinrich

> 
>> + *
>> + * @event      notified event
>> + * @context    pointer to the counter
>> + */
>> +static void EFIAPI notify(struct efi_event *event, void *context)
>> +{
>> +       if (!context)
>> +               return;
>> +       ++*(unsigned int *)context;
>> +}
>> +
>> +/*
>> + * Setup unit test.
>> + *
>> + * Create an EVT_SIGNAL_EXIT_BOOT_SERVICES event.
>> + *
>> + * @handle:    handle of the loaded image
>> + * @systable:  system table
> 
> @return...
> 
>> + */
>> +static int setup(const efi_handle_t handle,
>> +                const struct efi_system_table *systable)
>> +{
>> +       efi_status_t ret;
>> +
>> +       boottime = systable->boottime;
>> +
>> +       counter = 0;
>> +       ret = boottime->create_event(EVT_SIGNAL_EXIT_BOOT_SERVICES,
>> +                                    TPL_CALLBACK, notify, (void *)&counter,
>> +                                    &event_notify);
>> +       if (ret != EFI_SUCCESS) {
>> +               efi_st_error("could not create event\n");
>> +               return 1;
>> +       }
>> +       return 0;
>> +}
>> +
>> +/*
>> + * Tear down unit test.
>> + *
>> + * Close the event created in setup.
>> + */
>> +static int teardown(void)
>> +{
>> +       efi_status_t ret;
>> +
>> +       if (event_notify) {
>> +               ret = boottime->close_event(event_notify);
>> +               event_notify = NULL;
>> +               if (ret != EFI_SUCCESS) {
>> +                       efi_st_error("could not close event\n");
>> +                       return 1;
>> +               }
>> +       }
>> +       return 0;
>> +}
>> +
>> +/*
>> + * Execute unit test.
>> + *
>> + * Check that the notification function of the EVT_SIGNAL_EXIT_BOOT_SERVICES
>> + * event has been called.
>> + *
>> + * Call ExitBootServices again and check that the notification function is
>> + * not called again.
>> + */
>> +static int execute(void)
>> +{
>> +       if (counter != 1) {
>> +               efi_st_error("ExitBootServices was not notified");
>> +               return 1;
>> +       }
>> +       efi_st_exit_boot_services();
>> +       if (counter != 1) {
>> +               efi_st_error("ExitBootServices was notified twice");
>> +               return 1;
>> +       }
>> +       return 0;
>> +}
>> +
>> +EFI_UNIT_TEST(exitbootservices) = {
>> +       .name = "ExitBootServices",
>> +       .phase = EFI_SETUP_BEFORE_BOOTTIME_EXIT,
>> +       .setup = setup,
>> +       .execute = execute,
>> +       .teardown = teardown,
>> +};
>> --
>> 2.11.0
>>
> 

[U-Boot] [PATCH 04/10] efi_loader: implement queueing of the notification function

[Heinrich Schuchardt]

On 09/25/2017 04:11 AM, Simon Glass wrote:
> Hi Heinrich,
> 
> On 15 September 2017 at 02:06, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> For the correct implementation of the task priority level (TPL)
>> calling the notification function must be queued.
>>
>> Add a status field 'queued' to events.
>>
>> In function efi_signal_event set status queued if a notification
>> function exists and reset it after we have called the function.
>> A later patch will add a check of the TPL here.
>>
>> In efi_create_event and efi_close_event unset the queued status.
>>
>> In function efi_wait_for_event and efi_check_event
>> queue the notification function.
>>
>> In efi_timer_check call the efi_notify_event
>> if the status queued is set.
>> For all timer events set status signaled.
>>
>> In efi_console_timer_notify set the signaled state of the
>> WaitForKey event.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>  include/efi_loader.h          |  4 +++-
>>  lib/efi_loader/efi_boottime.c | 40 ++++++++++++++++++++++++++++++----------
>>  lib/efi_loader/efi_console.c  |  4 +++-
>>  3 files changed, 36 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index f74b33d589..25398ba40c 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -131,7 +131,8 @@ struct efi_object {
>>   * @nofify_function:   Function to call when the event is triggered
>>   * @notify_context:    Data to be passed to the notify function
>>   * @trigger_type:      Type of timer, see efi_set_timer
>> - * @signaled:          The notify function was already called
>> + * @queued:            The notification functionis queued
> 
> functions

function is

> 
> What does this actually mean? Can you expand the comment a bit? I'm
> not sure what a value of (for example) 3 would mean. Or if it is just
> *whether* the function is queued, then you could use a bool.

Yes
bool is_signaled
bool is_queued
would make sense.

Regards

Heinrich

> 
>> + * @signaled:          The event occured
> 
> occurred
> 

[U-Boot] [PATCH 08/10] efi_selftest: test task priority levels

[Heinrich Schuchardt]

On 09/25/2017 04:12 AM, Simon Glass wrote:
> On 15 September 2017 at 02:06, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> Run a 10 ms periodic timer and check that it is called 10 times
>> while waiting for 100 ms single shot timer.
>>
>> Raise the TPL level to the level of the 10 ms timer and observe
>> that the notification function is not called again.
>>
>> Lower the TPL level and check that the queued notification
>> function is called.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>  lib/efi_selftest/Makefile           |   5 +-
>>  lib/efi_selftest/efi_selftest_tpl.c | 214 ++++++++++++++++++++++++++++++++++++
>>  2 files changed, 218 insertions(+), 1 deletion(-)
>>  create mode 100644 lib/efi_selftest/efi_selftest_tpl.c
>>
>> diff --git a/lib/efi_selftest/Makefile b/lib/efi_selftest/Makefile
>> index a71c8bf937..ddf304e1fa 100644
>> --- a/lib/efi_selftest/Makefile
>> +++ b/lib/efi_selftest/Makefile
>> @@ -13,8 +13,11 @@ CFLAGS_efi_selftest_console.o := $(CFLAGS_EFI)
>>  CFLAGS_REMOVE_efi_selftest_console.o := $(CFLAGS_NON_EFI)
>>  CFLAGS_efi_selftest_events.o := $(CFLAGS_EFI)
>>  CFLAGS_REMOVE_efi_selftest_events.o := $(CFLAGS_NON_EFI)
>> +CFLAGS_efi_selftest_tpl.o := $(CFLAGS_EFI)
>> +CFLAGS_REMOVE_efi_selftest_tpl.o := $(CFLAGS_NON_EFI)
>>
>>  obj-$(CONFIG_CMD_BOOTEFI_SELFTEST) += \
>>  efi_selftest.o \
>>  efi_selftest_console.o \
>> -efi_selftest_events.o
>> +efi_selftest_events.o \
>> +efi_selftest_tpl.o
>> diff --git a/lib/efi_selftest/efi_selftest_tpl.c b/lib/efi_selftest/efi_selftest_tpl.c
>> new file mode 100644
>> index 0000000000..90ace0f51e
>> --- /dev/null
>> +++ b/lib/efi_selftest/efi_selftest_tpl.c
>> @@ -0,0 +1,214 @@
>> +/*
>> + * efi_selftest_events
>> + *
>> + * Copyright (c) 2017 Heinrich Schuchardt <xypron.glpk@gmx.de>
>> + *
>> + * SPDX-License-Identifier:     GPL-2.0+
>> + *
>> + * This unit test uses timer events to check the handling of
>> + * task priority levels.
>> + */
>> +
>> +#include <efi_selftest.h>
>> +
>> +static struct efi_event *event_notify;
>> +static struct efi_event *event_wait;
>> +static unsigned int counter;
>> +static struct efi_boot_services *boottime;
>> +
>> +/*
>> + * Notification function, increments a counter.
>> + *
>> + * @event      notified event
>> + * @context    pointer to the counter
>> + */
>> +static void EFIAPI notify(struct efi_event *event, void *context)
>> +{
>> +       if (!context)
>> +               return;
> 
> Does this ever happen in practice? If not, why check it?
> 
>> +       ++*(unsigned int *)context;
> 
> Similar comment to previous patch about using a struct here.
> 
>> +}
>> +
>> +/*
>> + * Setup unit test.
>> + *
>> + * Create two timer events.
>> + * One with EVT_NOTIFY_SIGNAL, the other with EVT_NOTIFY_WAIT.
>> + *
>> + * @handle:    handle of the loaded image
>> + * @systable:  system table
> 
> @return ...
> 
>> + */
>> +static int setup(const efi_handle_t handle,
>> +                const struct efi_system_table *systable)
>> +{
>> +       efi_status_t ret;
>> +
>> +       boottime = systable->boottime;
>> +
>> +       ret = boottime->create_event(EVT_TIMER | EVT_NOTIFY_SIGNAL,
>> +                                    TPL_CALLBACK, notify, (void *)&counter,
>> +                                    &event_notify);
>> +       if (ret != EFI_SUCCESS) {
>> +               efi_st_error("could not create event\n");
>> +               return 1;
> 
> Why not return ret?

We return int an not efi_status_t which is UINTN (= size_t).

The code might be more readable by using

#define EFI_ST_SUCCESS 0
#define EFI_ST_FAILURE 1

Regards

Heinrich

> 
>> +       }
>> +       ret = boottime->create_event(EVT_TIMER | EVT_NOTIFY_WAIT,
>> +                                    TPL_HIGH_LEVEL, notify, NULL, &event_wait);
>> +       if (ret != EFI_SUCCESS) {
>> +               efi_st_error("could not create event\n");
>> +               return 1;
>> +       }
>> +       return 0;
>> +}
>> +
>> +/*
>> + * Tear down unit test.
>> + *
>> + * Close the events created in setup.
>> + */
>> +static int teardown(void)
>> +{
>> +       efi_status_t ret;
>> +
>> +       if (event_notify) {
>> +               ret = boottime->close_event(event_notify);
>> +               event_notify = NULL;
>> +               if (ret != EFI_SUCCESS) {
>> +                       efi_st_error("could not close event\n");
>> +                       return 1;
>> +               }
>> +       }
>> +       if (event_wait) {
>> +               ret = boottime->close_event(event_wait);
>> +               event_wait = NULL;
>> +               if (ret != EFI_SUCCESS) {
>> +                       efi_st_error("could not close event\n");
>> +                       return 1;
>> +               }
>> +       }
>> +       boottime->restore_tpl(TPL_APPLICATION);
>> +       return 0;
>> +}
>> +
>> +/*
>> + * Execute unit test.
>> + *
>> + * Run a 10 ms periodic timer and check that it is called 10 times
>> + * while waiting for 100 ms single shot timer.
>> + *
>> + * Raise the TPL level to the level of the 10 ms timer and observe
>> + * that the notification function is not called again.
>> + *
>> + * Lower the TPL level and check that the queued notification
>> + * function is called.
>> + */
>> +static int execute(void)
>> +{
>> +       unsigned long index;
>> +       efi_status_t ret;
>> +       UINTN old_tpl;
>> +
>> +       /* Set 10 ms timer */
>> +       counter = 0;
>> +       ret = boottime->set_timer(event_notify, EFI_TIMER_PERIODIC, 100000);
>> +       if (ret != EFI_SUCCESS) {
>> +               efi_st_error("Could not set timer\n");
>> +               return 1;
>> +       }
>> +       /* Set 100 ms timer */
>> +       ret = boottime->set_timer(event_wait, EFI_TIMER_RELATIVE, 1000000);
>> +       if (ret != EFI_SUCCESS) {
>> +               efi_st_error("Could not set timer\n");
>> +               return 1;
>> +       }
>> +       index = 5;
>> +       ret = boottime->wait_for_event(1, &event_wait, &index);
>> +       if (ret != EFI_SUCCESS) {
>> +               efi_st_error("Could not wait for event\n");
>> +               return 1;
>> +       }
>> +       ret = boottime->check_event(event_wait);
>> +       if (ret != EFI_NOT_READY) {
>> +               efi_st_error("Signaled state was not cleared.\n");
>> +               efi_st_printf("ret = %u\n", (unsigned int)ret);
>> +               return 1;
>> +       }
>> +       if (index != 0) {
>> +               efi_st_error("WaitForEvent returned wrong index\n");
>> +               return 1;
>> +       }
>> +       efi_st_printf("Counter with TPL level TPL_APPLICATION: %u\n", counter);
>> +       if (counter < 8 || counter > 12) {
>> +               efi_st_error("Incorrect timing of events\n");
>> +               return 1;
>> +       }
>> +       ret = boottime->set_timer(event_notify, EFI_TIMER_STOP, 0);
>> +       if (index != 0) {
>> +               efi_st_error("Could not cancel timer\n");
>> +               return 1;
>> +       }
>> +       /* Raise TPL level */
>> +       old_tpl = boottime->raise_tpl(TPL_CALLBACK);
>> +       if (old_tpl != TPL_APPLICATION) {
>> +               efi_st_error("Initial TPL level was not TPL_APPLICATION");
>> +               return 1;
>> +       }
>> +       /* Set 10 ms timer */
>> +       counter = 0;
>> +       ret = boottime->set_timer(event_notify, EFI_TIMER_PERIODIC, 100000);
>> +       if (index != 0) {
>> +               efi_st_error("Could not set timer\n");
>> +               return 1;
>> +       }
>> +       /* Set 100 ms timer */
>> +       ret = boottime->set_timer(event_wait, EFI_TIMER_RELATIVE, 1000000);
>> +       if (ret != EFI_SUCCESS) {
>> +               efi_st_error("Could not set timer\n");
>> +               return 1;
>> +       }
>> +       do {
>> +               ret = boottime->check_event(event_wait);
>> +       } while (ret == EFI_NOT_READY);
>> +       if (ret != EFI_SUCCESS) {
>> +               efi_st_error("Could not check event\n");
>> +               return 1;
>> +       }
>> +       efi_st_printf("Counter with TPL level TPL_CALLBACK: %u\n", counter);
>> +       if (counter != 0) {
>> +               efi_st_error("Suppressed timer fired\n");
>> +               return 1;
>> +       }
>> +       /* Set 1 ms timer */
>> +       ret = boottime->set_timer(event_wait, EFI_TIMER_RELATIVE, 1000);
>> +       if (ret != EFI_SUCCESS) {
>> +               efi_st_error("Could not set timer\n");
>> +               return 1;
>> +       }
>> +       /* Restore the old TPL level */
>> +       boottime->restore_tpl(TPL_APPLICATION);
>> +       ret = boottime->wait_for_event(1, &event_wait, &index);
>> +       if (ret != EFI_SUCCESS) {
>> +               efi_st_error("Could not wait for event\n");
>> +               return 1;
>> +       }
>> +       efi_st_printf("Counter with TPL level TPL_APPLICATION: %u\n", counter);
>> +       if (counter < 1) {
>> +               efi_st_error("Queued timer event did not fire\n");
>> +               return 1;
>> +       }
>> +       ret = boottime->set_timer(event_wait, EFI_TIMER_STOP, 0);
>> +       if (index != 0) {
>> +               efi_st_error("Could not cancel timer\n");
>> +               return 1;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +EFI_UNIT_TEST(tpl) = {
>> +       .name = "task priority levels",
>> +       .phase = EFI_EXECUTE_BEFORE_BOOTTIME_EXIT,
>> +       .setup = setup,
>> +       .execute = execute,
>> +       .teardown = teardown,
> 
> In general I suggest you put the filename as a prefix on these
> function names. It makes __func__ much more useful in the function,
> plus the name is more meaningful.
> 
> E.g. efi_selftest_tpl_execute()
> 
> Regards,
> Simon
> 

[U-Boot] [PATCH 10/10] efi_selftest: check notification of ExitBootServices

[Simon Glass]

Hi Heinrich,

On 25 September 2017 at 00:01, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 09/25/2017 04:12 AM, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On 15 September 2017 at 02:06, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >> Check that the notification function of an
> >> EVT_SIGNAL_EXIT_BOOT_SERVICES event is called
> >> exactly once.
> >>
> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >> ---
> >>  lib/efi_selftest/Makefile                        |   3 +
> >>  lib/efi_selftest/efi_selftest_exitbootservices.c | 106 +++++++++++++++++++++++
> >>  2 files changed, 109 insertions(+)
> >>  create mode 100644 lib/efi_selftest/efi_selftest_exitbootservices.c
> >>
> >> diff --git a/lib/efi_selftest/Makefile b/lib/efi_selftest/Makefile
> >> index ddf304e1fa..30f1960933 100644
> >> --- a/lib/efi_selftest/Makefile
> >> +++ b/lib/efi_selftest/Makefile
> >> @@ -13,6 +13,8 @@ CFLAGS_efi_selftest_console.o := $(CFLAGS_EFI)
> >>  CFLAGS_REMOVE_efi_selftest_console.o := $(CFLAGS_NON_EFI)
> >>  CFLAGS_efi_selftest_events.o := $(CFLAGS_EFI)
> >>  CFLAGS_REMOVE_efi_selftest_events.o := $(CFLAGS_NON_EFI)
> >> +CFLAGS_efi_selftest_exitbootservices.o := $(CFLAGS_EFI)
> >> +CFLAGS_REMOVE_efi_selftest_exitbootservices.o := $(CFLAGS_NON_EFI)
> >>  CFLAGS_efi_selftest_tpl.o := $(CFLAGS_EFI)
> >>  CFLAGS_REMOVE_efi_selftest_tpl.o := $(CFLAGS_NON_EFI)
> >>
> >> @@ -20,4 +22,5 @@ obj-$(CONFIG_CMD_BOOTEFI_SELFTEST) += \
> >>  efi_selftest.o \
> >>  efi_selftest_console.o \
> >>  efi_selftest_events.o \
> >> +efi_selftest_exitbootservices.o \
> >>  efi_selftest_tpl.o
> >> diff --git a/lib/efi_selftest/efi_selftest_exitbootservices.c b/lib/efi_selftest/efi_selftest_exitbootservices.c
> >> new file mode 100644
> >> index 0000000000..60271e6180
> >> --- /dev/null
> >> +++ b/lib/efi_selftest/efi_selftest_exitbootservices.c
> >> @@ -0,0 +1,106 @@
> >> +/*
> >> + * efi_selftest_events
> >> + *
> >> + * Copyright (c) 2017 Heinrich Schuchardt <xypron.glpk@gmx.de>
> >> + *
> >> + * SPDX-License-Identifier:     GPL-2.0+
> >> + *
> >> + * This unit test checks that the notification function of an
> >> + * EVT_SIGNAL_EXIT_BOOT_SERVICES event is called exactly once.
> >> + */
> >> +
> >> +#include <efi_selftest.h>
> >> +
> >> +static struct efi_boot_services *boottime;
> >> +static struct efi_event *event_notify;
> >> +static unsigned int counter;
> >
> > I wonder if the solution to the context thin in the notify() function
> > is to put all of these in a struct?
>
> This would mean replacing
> boottime->something
> by
> config->boottime->something.
>
> This does not make the code easier to read.

I don't understand that comment. In general only the outermost
function accesses the global, then passes what is needed to the inner
functions. So in practice you seldom see config->boottime->something.

You might define a local variable boottime as config->boottime,
perhaps. But I agree that two levels of access would be bad.

>
> >
> > It is nice to group globals into a struct to allow future one-to-many
> > conversion, reduce the number of symbols in the map and provide a
> > logical grouping. So this would kill two birds with one stone.
>
> I typically do not read map files.
>
> With your suggestion the code will be slower and the binary will be larger.

Actually it is often faster and smaller. E.g. on ARM every global
access requires a literal pool entry and access. You can compare the
compiler output and see what you find.

>
> How would putting all private variables into a structure provide logical
> grouping?

A structure is a way of grouping.

>
> >
> > Another idea is to have setup() return the context (e.g. as a void **
> > final arg). Then that same context can be passed to execute and
> > teardown. This is similar to how the unit tests work in U-Boot.
>
> Passing structures as void* is error prone.
>
> Private variables should stay private. There is no reason to make these
> accessible outside the unit test.

That was not my suggestion.

>
> Passing a void *this would make sense if we would envision having
> multiple instances of the same unit test. I can't see that.

Well you have set up a test framework of sorts, but it does not use
the existing unit test code in U-Boot. That framework uses a test
struct which is passed to all functions. It works quite well.

>
> >
> > Other than that, see my comments to the previous patch which also apply here.
>
> @Alex
> You already accepted the patch series to efi-next and afterwards merged
> a bunch of other patches.
>
> I could not see any comment by Simon concerning functionality.
> Everything seemed to focus on style.
>
> Shall I provide add-on patches covering Simon's comments or should I
> create a new version of the patch series.

If you think there is value in these changes then I suggest doing a
follow-on patch.

Regards,
Simon