[U-Boot] [PATCH v2 0/9] efi_loader: implement SetWatchdogTimer

[U-Boot] [PATCH v2 0/9] efi_loader: implement SetWatchdogTimer

[Heinrich Schuchardt]

The patch series is centered on implementing the SetWatchdogTimer
boottime service. Two tests are supplied. One that checks that
resetting the watchdog timer saves from a reboot. The other
checks that the watchdog timer actually leads to a reset.
Both are covered by Python test cases.

Additional fixes include missing comments and fixing typos.

Heinrich Schuchardt (9):
  efi_loader: move efi_search_obj up in code
  efi_loader: implement SetWatchdogTimer
  efi_selftest: provide test for watchdog timer
  efi_loader: new function utf8_to_utf16
  efi_loader: guard against double inclusion of efi_loader.h
  efi_selftest: allow to select a single test for exexution
  efi_selftest: test reboot by watchdog
  test/py: test reboot by EFI watchdog
  test/py: fix typo in test_efi_loader.py

 cmd/bootefi.c                            |  47 ++++++-
 include/charset.h                        |  15 ++
 include/efi_loader.h                     |  11 +-
 include/efi_selftest.h                   |  18 +++
 lib/charset.c                            |  57 +++++++-
 lib/efi_loader/Makefile                  |   2 +-
 lib/efi_loader/efi_boottime.c            |  58 ++++----
 lib/efi_loader/efi_watchdog.c            |  86 ++++++++++++
 lib/efi_selftest/Makefile                |   5 +-
 lib/efi_selftest/efi_selftest.c          |  89 +++++++++++-
 lib/efi_selftest/efi_selftest_console.c  |  11 ++
 lib/efi_selftest/efi_selftest_util.c     |  11 +-
 lib/efi_selftest/efi_selftest_watchdog.c | 230 +++++++++++++++++++++++++++++++
 test/py/tests/test_efi_loader.py         |   2 +-
 test/py/tests/test_efi_selftest.py       |  14 +-
 15 files changed, 609 insertions(+), 47 deletions(-)
 create mode 100644 lib/efi_loader/efi_watchdog.c
 create mode 100644 lib/efi_selftest/efi_selftest_watchdog.c

-- 
2.14.1

[U-Boot] [PATCH v2 1/9] efi_loader: move efi_search_obj up in code

[Heinrich Schuchardt]

To avoid a forward declaration move efi_search_obj before
all protocol services functions.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2
	no change
---
 lib/efi_loader/efi_boottime.c | 41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index f627340de4..30577f717e 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -690,6 +690,27 @@ static efi_status_t EFIAPI efi_check_event(struct efi_event *event)
 	return EFI_EXIT(EFI_INVALID_PARAMETER);
 }
 
+/*
+ * Find the internal EFI object for a handle.
+ *
+ * @handle	handle to find
+ * @return	EFI object
+ */
+static struct efi_object *efi_search_obj(void *handle)
+{
+	struct list_head *lhandle;
+
+	list_for_each(lhandle, &efi_obj_list) {
+		struct efi_object *efiobj;
+
+		efiobj = list_entry(lhandle, struct efi_object, link);
+		if (efiobj->handle == handle)
+			return efiobj;
+	}
+
+	return NULL;
+}
+
 /*
  * Install protocol interface.
  *
@@ -1355,26 +1376,6 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
 	panic("EFI application exited");
 }
 
-/*
- * Find the internal EFI object for a handle.
- *
- * @handle	handle to find
- * @return	EFI object
- */
-static struct efi_object *efi_search_obj(void *handle)
-{
-	struct list_head *lhandle;
-
-	list_for_each(lhandle, &efi_obj_list) {
-		struct efi_object *efiobj;
-		efiobj = list_entry(lhandle, struct efi_object, link);
-		if (efiobj->handle == handle)
-			return efiobj;
-	}
-
-	return NULL;
-}
-
 /*
  * Unload an EFI image.
  *
-- 
2.14.1

[U-Boot] [PATCH v2 2/9] efi_loader: implement SetWatchdogTimer

[Heinrich Schuchardt]

The watchdog is initialized with a 5 minute timeout period.
It can be reset by SetWatchdogTimer.
It is stopped by ExitBoottimeServices.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2
	code comments updated
---
 cmd/bootefi.c                 |  1 +
 include/efi_loader.h          |  4 ++
 lib/efi_loader/Makefile       |  2 +-
 lib/efi_loader/efi_boottime.c | 17 ++-------
 lib/efi_loader/efi_watchdog.c | 86 +++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 95 insertions(+), 15 deletions(-)
 create mode 100644 lib/efi_loader/efi_watchdog.c

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 478bc116e2..18176a1266 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -43,6 +43,7 @@ static void efi_init_obj_list(void)
 #ifdef CONFIG_GENERATE_SMBIOS_TABLE
 	efi_smbios_register();
 #endif
+	efi_watchdog_register();
 
 	/* Initialize EFI runtime services */
 	efi_reset_system_init();
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 1b92edbd77..af64b11cee 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -163,6 +163,8 @@ int efi_disk_register(void);
 int efi_gop_register(void);
 /* Called by bootefi to make the network interface available */
 int efi_net_register(void);
+/* Called by bootefi to make the watchdog available */
+int efi_watchdog_register(void);
 /* Called by bootefi to make SMBIOS tables available */
 void efi_smbios_register(void);
 
@@ -171,6 +173,8 @@ efi_fs_from_path(struct efi_device_path *fp);
 
 /* Called by networking code to memorize the dhcp ack package */
 void efi_net_set_dhcp_ack(void *pkt, int len);
+/* Called by efi_set_watchdog_timer to reset the timer */
+efi_status_t efi_set_watchdog(unsigned long timeout);
 
 /* Called from places to check whether a timer expired */
 void efi_timer_check(void);
diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
index ddb978f650..83d879b686 100644
--- a/lib/efi_loader/Makefile
+++ b/lib/efi_loader/Makefile
@@ -17,7 +17,7 @@ endif
 obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o
 obj-y += efi_image_loader.o efi_boottime.o efi_runtime.o efi_console.o
 obj-y += efi_memory.o efi_device_path_to_text.o efi_device_path.o
-obj-y += efi_file.o efi_variable.o efi_bootmgr.o
+obj-y += efi_file.o efi_variable.o efi_bootmgr.o efi_watchdog.o
 obj-$(CONFIG_LCD) += efi_gop.o
 obj-$(CONFIG_DM_VIDEO) += efi_gop.o
 obj-$(CONFIG_PARTITIONS) += efi_disk.o
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 30577f717e..fd8d15655b 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -155,18 +155,6 @@ void efi_signal_event(struct efi_event *event)
 	event->is_queued = false;
 }
 
-/*
- * Write a debug message for an EPI API service that is not implemented yet.
- *
- * @funcname	function that is not yet implemented
- * @return	EFI_UNSUPPORTED
- */
-static efi_status_t efi_unsupported(const char *funcname)
-{
-	debug("EFI: App called into unimplemented function %s\n", funcname);
-	return EFI_EXIT(EFI_UNSUPPORTED);
-}
-
 /*
  * Raise the task priority level.
  *
@@ -1454,6 +1442,7 @@ static efi_status_t EFIAPI efi_exit_boot_services(void *image_handle,
 	bootm_disable_interrupts();
 
 	/* Give the payload some time to boot */
+	efi_set_watchdog(0);
 	WATCHDOG_RESET();
 
 	return EFI_EXIT(EFI_SUCCESS);
@@ -1497,7 +1486,7 @@ static efi_status_t EFIAPI efi_stall(unsigned long microseconds)
 /*
  * Reset the watchdog timer.
  *
- * This function implements the WatchdogTimer service.
+ * This function implements the SetWatchdogTimer service.
  * See the Unified Extensible Firmware Interface (UEFI) specification
  * for details.
  *
@@ -1514,7 +1503,7 @@ static efi_status_t EFIAPI efi_set_watchdog_timer(unsigned long timeout,
 {
 	EFI_ENTRY("%ld, 0x%"PRIx64", %ld, %p", timeout, watchdog_code,
 		  data_size, watchdog_data);
-	return efi_unsupported(__func__);
+	return EFI_EXIT(efi_set_watchdog(timeout));
 }
 
 /*
diff --git a/lib/efi_loader/efi_watchdog.c b/lib/efi_loader/efi_watchdog.c
new file mode 100644
index 0000000000..eb437faf4b
--- /dev/null
+++ b/lib/efi_loader/efi_watchdog.c
@@ -0,0 +1,86 @@
+/*
+ *  EFI watchdog
+ *
+ *  Copyright (c) 2017 Heinrich Schuchardt
+ *
+ *  SPDX-License-Identifier:     GPL-2.0+
+ */
+
+#include <common.h>
+#include <efi_loader.h>
+
+static struct efi_event *watchdog_timer_event;
+
+/*
+ * Reset the system when the watchdog event is notified.
+ *
+ * @event:	the watchdog event
+ * @context:	not used
+ */
+static void EFIAPI efi_watchdog_timer_notify(struct efi_event *event,
+					     void *context)
+{
+	EFI_ENTRY("%p, %p", event, context);
+
+	printf("\nEFI: Watchdog timeout\n");
+	EFI_CALL_VOID(efi_runtime_services.reset_system(EFI_RESET_COLD,
+							EFI_SUCCESS, 0, NULL));
+
+	EFI_EXIT(EFI_UNSUPPORTED);
+}
+
+/*
+ * Reset the watchdog timer.
+ *
+ * This function is used by the SetWatchdogTimer service.
+ *
+ * @timeout:		seconds before reset by watchdog
+ * @return:		status code
+ */
+efi_status_t efi_set_watchdog(unsigned long timeout)
+{
+	efi_status_t r;
+
+	if (timeout)
+		/* Reset watchdog */
+		r = efi_set_timer(watchdog_timer_event, EFI_TIMER_RELATIVE,
+				  10000000 * timeout);
+	else
+		/* Deactivate watchdog */
+		r = efi_set_timer(watchdog_timer_event, EFI_TIMER_STOP, 0);
+	return r;
+}
+
+/*
+ * Initialize the EFI watchdog.
+ *
+ * This function is called by efi_init_obj_list()
+ */
+int efi_watchdog_register(void)
+{
+	efi_status_t r;
+
+	/*
+	 * Create a timer event.
+	 */
+	r = efi_create_event(EVT_TIMER | EVT_NOTIFY_SIGNAL, TPL_CALLBACK,
+			     efi_watchdog_timer_notify, NULL,
+			     &watchdog_timer_event);
+	if (r != EFI_SUCCESS) {
+		printf("ERROR: Failed to register watchdog event\n");
+		return r;
+	}
+	/*
+	 * The UEFI standard requires that the watchdog timer is set to five
+	 * minutes when invoking an EFI boot option.
+	 *
+	 * Unified Extensible Firmware Interface (UEFI), version 2.7 Errata A
+	 * 7.5. Miscellaneous Boot Services - EFI_BOOT_SERVICES.SetWatchdogTimer
+	 */
+	r = efi_set_watchdog(300);
+	if (r != EFI_SUCCESS) {
+		printf("ERROR: Failed to set watchdog timer\n");
+		return r;
+	}
+	return 0;
+}
-- 
2.14.1

[U-Boot] [PATCH v2 3/9] efi_selftest: provide test for watchdog timer

[Heinrich Schuchardt]

The test verifies that resetting the watchdog timer ensures
that it is not called during the timeout period.

Testing that the watchdog timer actually executes a reset
would require a test outside the efi_selftest framework.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2
	no change
---
 lib/efi_selftest/Makefile                |   5 +-
 lib/efi_selftest/efi_selftest_watchdog.c | 185 +++++++++++++++++++++++++++++++
 2 files changed, 189 insertions(+), 1 deletion(-)
 create mode 100644 lib/efi_selftest/efi_selftest_watchdog.c

diff --git a/lib/efi_selftest/Makefile b/lib/efi_selftest/Makefile
index e446046e02..3e5c9a6d16 100644
--- a/lib/efi_selftest/Makefile
+++ b/lib/efi_selftest/Makefile
@@ -21,6 +21,8 @@ CFLAGS_efi_selftest_tpl.o := $(CFLAGS_EFI)
 CFLAGS_REMOVE_efi_selftest_tpl.o := $(CFLAGS_NON_EFI)
 CFLAGS_efi_selftest_util.o := $(CFLAGS_EFI)
 CFLAGS_REMOVE_efi_selftest_util.o := $(CFLAGS_NON_EFI)
+CFLAGS_efi_selftest_watchdog.o := $(CFLAGS_EFI)
+CFLAGS_REMOVE_efi_selftest_watchdog.o := $(CFLAGS_NON_EFI)
 
 obj-$(CONFIG_CMD_BOOTEFI_SELFTEST) += \
 efi_selftest.o \
@@ -29,4 +31,5 @@ efi_selftest_events.o \
 efi_selftest_exitbootservices.o \
 efi_selftest_snp.o \
 efi_selftest_tpl.o \
-efi_selftest_util.o
+efi_selftest_util.o \
+efi_selftest_watchdog.o
diff --git a/lib/efi_selftest/efi_selftest_watchdog.c b/lib/efi_selftest/efi_selftest_watchdog.c
new file mode 100644
index 0000000000..f8c5404000
--- /dev/null
+++ b/lib/efi_selftest/efi_selftest_watchdog.c
@@ -0,0 +1,185 @@
+/*
+ * efi_selftest_watchdog
+ *
+ * Copyright (c) 2017 Heinrich Schuchardt <xypron.glpk@gmx.de>
+ *
+ * SPDX-License-Identifier:     GPL-2.0+
+ *
+ * This unit test checks that the watchdog timer will not cause
+ * a system restart during the timeout period after a timer reset.
+ *
+ * Testing that the watchdog timer actually will reset the system
+ * after a timeout is not possible within the used framework.
+ */
+
+#include <efi_selftest.h>
+
+/*
+ * This is the communication structure for the notification function.
+ */
+struct notify_context {
+	/* Status code returned when resetting watchdog */
+	efi_status_t status;
+	/* Number of invocations of the notification function */
+	unsigned int timer_ticks;
+};
+
+static struct efi_event *event_notify;
+static struct efi_event *event_wait;
+static struct efi_boot_services *boottime;
+static struct notify_context notification_context;
+
+/*
+ * Notification function, increments the notfication count if parameter
+ * context is provided.
+ *
+ * @event	notified event
+ * @context	pointer to the timeout
+ */
+static void EFIAPI notify(struct efi_event *event, void *context)
+{
+	struct notify_context *notify_context = context;
+	efi_status_t ret = EFI_SUCCESS;
+
+	if (!notify_context)
+		return;
+
+	/* Reset watchdog timer to one second */
+	ret = boottime->set_watchdog_timer(1, 0, 0, NULL);
+	if (ret != EFI_SUCCESS)
+		notify_context->status = ret;
+	/* Count number of calls */
+	notify_context->timer_ticks++;
+}
+
+/*
+ * 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:	EFI_ST_SUCCESS for success
+ */
+static int setup(const efi_handle_t handle,
+		 const struct efi_system_table *systable)
+{
+	efi_status_t ret;
+
+	boottime = systable->boottime;
+
+	notification_context.status = EFI_SUCCESS;
+	notification_context.timer_ticks = 0;
+	ret = boottime->create_event(EVT_TIMER | EVT_NOTIFY_SIGNAL,
+				     TPL_CALLBACK, notify,
+				     (void *)&notification_context,
+				     &event_notify);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("could not create event\n");
+		return EFI_ST_FAILURE;
+	}
+	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 EFI_ST_FAILURE;
+	}
+	return EFI_ST_SUCCESS;
+}
+
+/*
+ * Tear down unit test.
+ *
+ * Close the events created in setup.
+ *
+ * @return:	EFI_ST_SUCCESS for success
+ */
+static int teardown(void)
+{
+	efi_status_t ret;
+
+	/* Set the watchdog timer to the five minute default value */
+	ret = boottime->set_watchdog_timer(300, 0, 0, NULL);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("Setting watchdog timer failed\n");
+		return EFI_ST_FAILURE;
+	}
+	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 EFI_ST_FAILURE;
+		}
+	}
+	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 EFI_ST_FAILURE;
+		}
+	}
+	return EFI_ST_SUCCESS;
+}
+
+/*
+ * Execute unit test.
+ *
+ * Run a 600 ms periodic timer that resets the watchdog to one second
+ * on every timer tick.
+ *
+ * Run a 1350 ms single shot timer and check that the 600ms timer has
+ * been called 2 times.
+ *
+ * @return:	EFI_ST_SUCCESS for success
+ */
+static int execute(void)
+{
+	size_t index;
+	efi_status_t ret;
+
+	/* Set the watchdog timeout to one second */
+	ret = boottime->set_watchdog_timer(1, 0, 0, NULL);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("Setting watchdog timer failed\n");
+		return EFI_ST_FAILURE;
+	}
+	/* Set 600 ms timer */
+	ret = boottime->set_timer(event_notify, EFI_TIMER_PERIODIC, 6000000);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("Could not set timer\n");
+		return EFI_ST_FAILURE;
+	}
+	/* Set 1350 ms timer */
+	ret = boottime->set_timer(event_wait, EFI_TIMER_RELATIVE, 13500000);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("Could not set timer\n");
+		return EFI_ST_FAILURE;
+	}
+
+	ret = boottime->wait_for_event(1, &event_wait, &index);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("Could not wait for event\n");
+		return EFI_ST_FAILURE;
+	}
+	if (notification_context.status != EFI_SUCCESS) {
+		efi_st_error("Setting watchdog timer failed\n");
+		return EFI_ST_FAILURE;
+	}
+	if (notification_context.timer_ticks != 2) {
+		efi_st_error("The timer was called %u times, expected 2.\n",
+			     notification_context.timer_ticks);
+		return EFI_ST_FAILURE;
+	}
+	return EFI_ST_SUCCESS;
+}
+
+EFI_UNIT_TEST(watchdog) = {
+	.name = "watchdog timer",
+	.phase = EFI_EXECUTE_BEFORE_BOOTTIME_EXIT,
+	.setup = setup,
+	.execute = execute,
+	.teardown = teardown,
+};
-- 
2.14.1

[U-Boot] [PATCH v2 4/9] efi_loader: new function utf8_to_utf16

[Heinrich Schuchardt]

Provide a conversion function from utf8 to utf16.

Add missing #include <linux/types.h> in include/charset.h.
Remove superfluous #include <common.h> in lib/charset.c.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2
	new patch
---
 include/charset.h | 15 +++++++++++++++
 lib/charset.c     | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/include/charset.h b/include/charset.h
index 37a3278499..2662c2f7c9 100644
--- a/include/charset.h
+++ b/include/charset.h
@@ -9,6 +9,8 @@
 #ifndef __CHARSET_H_
 #define __CHARSET_H_
 
+#include <linux/types.h>
+
 #define MAX_UTF8_PER_UTF16 3
 
 /**
@@ -62,4 +64,17 @@ uint16_t *utf16_strdup(const uint16_t *s);
  */
 uint8_t *utf16_to_utf8(uint8_t *dest, const uint16_t *src, size_t size);
 
+/**
+ * utf8_to_utf16() - Convert an utf8 string to utf16
+ *
+ * Converts up to 'size' characters of the utf16 string 'src' to utf8
+ * written to the 'dest' buffer. Stops at 0x00.
+ *
+ * @dest   the destination buffer to write the utf8 characters
+ * @src    the source utf16 string
+ * @size   maximum number of utf16 characters to convert
+ * @return the pointer to the first unwritten byte in 'dest'
+ */
+uint16_t *utf8_to_utf16(uint16_t *dest, const uint8_t *src, size_t size);
+
 #endif /* __CHARSET_H_ */
diff --git a/lib/charset.c b/lib/charset.c
index ff76e88c77..8cd17ea1cb 100644
--- a/lib/charset.c
+++ b/lib/charset.c
@@ -6,7 +6,6 @@
  *  SPDX-License-Identifier:     GPL-2.0+
  */
 
-#include <common.h>
 #include <charset.h>
 #include <malloc.h>
 
@@ -99,3 +98,59 @@ uint8_t *utf16_to_utf8(uint8_t *dest, const uint16_t *src, size_t size)
 
 	return dest;
 }
+
+uint16_t *utf8_to_utf16(uint16_t *dest, const uint8_t *src, size_t size)
+{
+	while (size--) {
+		int extension_bytes;
+		uint32_t code;
+
+		extension_bytes = 0;
+		if (*src <= 0x7f) {
+			code = *src++;
+			/* Exit on zero byte */
+			if (!code)
+				size = 0;
+		} else if (*src <= 0xbf) {
+			/* Illegal code */
+			code = '?';
+		} else if (*src <= 0xdf) {
+			code = *src++ & 0x1f;
+			extension_bytes = 1;
+		} else if (*src <= 0xef) {
+			code = *src++ & 0x0f;
+			extension_bytes = 2;
+		} else if (*src <= 0xf7) {
+			code = *src++ & 0x07;
+			extension_bytes = 3;
+		} else {
+			/* Illegal code */
+			code = '?';
+		}
+
+		for (; extension_bytes && size; --size, --extension_bytes) {
+			if ((*src & 0xc0) == 0x80) {
+				code <<= 6;
+				code |= *src++ & 0x3f;
+			} else {
+				/* Illegal code */
+				code = '?';
+				++src;
+				--size;
+				break;
+			}
+		}
+
+		if (code < 0x10000) {
+			*dest++ = code;
+		} else {
+			/*
+			 * Simplified expression for
+			 * (((code - 0x10000) >> 10) & 0x3ff) | 0xd800
+			 */
+			*dest++ = (code >> 10) + 0xd7c0;
+			*dest++ = (code & 0x3ff) | 0xdc00;
+		}
+	}
+	return dest;
+}
-- 
2.14.1

[U-Boot] [PATCH v2 5/9] efi_loader: guard against double inclusion of efi_loader.h

[Heinrich Schuchardt]

Use a define to detect double inclusion of efi_loader.h.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2
	new patch
---
 include/efi_loader.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index af64b11cee..e506eeec61 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -6,6 +6,9 @@
  *  SPDX-License-Identifier:     GPL-2.0+
  */
 
+#ifndef _EFI_LOADER_H
+#define _EFI_LOADER_H 1
+
 #include <common.h>
 #include <part_efi.h>
 #include <efi_api.h>
@@ -345,4 +348,6 @@ static inline void efi_set_bootdev(const char *dev, const char *devnr,
 				   const char *path) { }
 static inline void efi_net_set_dhcp_ack(void *pkt, int len) { }
 
-#endif
+#endif /* CONFIG_EFI_LOADER && !CONFIG_SPL_BUILD */
+
+#endif /* _EFI_LOADER_H */
-- 
2.14.1

[U-Boot] [PATCH v2 6/9] efi_selftest: allow to select a single test for exexution

[Heinrich Schuchardt]

Environment variable efi_selftest is passed as load options
to the selftest application. It is used to select a single
test to be executed.

Special value 'list' displays a list of all available tests.

Tests get an on_request property. If this property is set
the tests are only executed if explicitly requested.

The invocation of efi_selftest is changed to reflect that
bootefi selftest with efi_selftest = 'list' will call the
Exit bootservice.

Environment variable bootargs is used as load options
for all other bootefi payloads.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2
	use an environment variable to choose a test
---
 cmd/bootefi.c                           | 46 ++++++++++++++++-
 include/efi_selftest.h                  | 18 +++++++
 lib/efi_selftest/efi_selftest.c         | 90 +++++++++++++++++++++++++++++++--
 lib/efi_selftest/efi_selftest_console.c | 10 ++++
 lib/efi_selftest/efi_selftest_util.c    | 11 +++-
 5 files changed, 168 insertions(+), 7 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 18176a1266..2d70137482 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -6,10 +6,12 @@
  *  SPDX-License-Identifier:     GPL-2.0+
  */
 
+#include <charset.h>
 #include <common.h>
 #include <command.h>
 #include <dm.h>
 #include <efi_loader.h>
+#include <efi_selftest.h>
 #include <errno.h>
 #include <libfdt.h>
 #include <libfdt_env.h>
@@ -50,6 +52,32 @@ static void efi_init_obj_list(void)
 	efi_get_time_init();
 }
 
+/*
+ * Set the load options of an image from an environment variable.
+ *
+ * @loaded_image_info:	the image
+ * @env_var:		name of the environment variable
+ */
+static void set_load_options(struct efi_loaded_image *loaded_image_info,
+			     const char *env_var)
+{
+	size_t size;
+	const char *env = env_get(env_var);
+
+	loaded_image_info->load_options = NULL;
+	loaded_image_info->load_options_size = 0;
+	if (!env)
+		return;
+	size = strlen(env) + 1;
+	loaded_image_info->load_options = calloc(size, sizeof(u16));
+	if (!loaded_image_info->load_options) {
+		printf("ERROR: Out of memory\n");
+		return;
+	}
+	utf8_to_utf16(loaded_image_info->load_options, (u8 *)env, size);
+	loaded_image_info->load_options_size = size * 2;
+}
+
 static void *copy_fdt(void *fdt)
 {
 	u64 fdt_size = fdt_totalsize(fdt);
@@ -190,6 +218,8 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt,
 		efi_install_configuration_table(&fdt_guid, NULL);
 	}
 
+	/* Transfer environment variable bootargs as load options */
+	set_load_options(&loaded_image_info, "bootargs");
 	/* Load the EFI payload */
 	entry = efi_load_pe(efi, &loaded_image_info);
 	if (!entry) {
@@ -237,6 +267,7 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt,
 
 exit:
 	/* image has returned, loaded-image obj goes *poof*: */
+	free(loaded_image_info.load_options);
 	list_del(&loaded_image_info_obj.link);
 
 	return ret;
@@ -301,17 +332,26 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 
 		efi_setup_loaded_image(&loaded_image_info,
 				       &loaded_image_info_obj,
-				       bootefi_device_path, bootefi_image_path);
+				       NULL, NULL);
 		/*
 		 * 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();
+		loaded_image_info.image_code_type = EFI_LOADER_CODE;
+		loaded_image_info.image_data_type = EFI_LOADER_DATA;
 		/* Initialize and populate EFI object list */
 		if (!efi_obj_list_initalized)
 			efi_init_obj_list();
-		return efi_selftest(&loaded_image_info, &systab);
+		/* Transfer environment variable efi_selftest as load options */
+		set_load_options(&loaded_image_info, "efi_selftest");
+		/* Execute the test */
+		r = efi_selftest(&loaded_image_info, &systab);
+		efi_restore_gd();
+		free(loaded_image_info.load_options);
+		list_del(&loaded_image_info_obj.link);
+		return r;
 	} else
 #endif
 	if (!strcmp(argv[1], "bootmgr")) {
@@ -357,6 +397,8 @@ static char bootefi_help_text[] =
 #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
 	"bootefi selftest\n"
 	"  - boot an EFI selftest application stored within U-Boot\n"
+	"    Use environment variable efi_selftest to select a single test.\n"
+	"    Use 'setenv efi_selftest list' to enumerate all tests.\n"
 #endif
 	"bootmgr [fdt addr]\n"
 	"  - load and boot EFI payload based on BootOrder/BootXXXX variables.\n"
diff --git a/include/efi_selftest.h b/include/efi_selftest.h
index 7ec42a0406..978ca2a7ea 100644
--- a/include/efi_selftest.h
+++ b/include/efi_selftest.h
@@ -12,6 +12,7 @@
 #include <common.h>
 #include <efi.h>
 #include <efi_api.h>
+#include <efi_loader.h>
 #include <linker_lists.h>
 
 #define EFI_ST_SUCCESS 0
@@ -71,6 +72,15 @@ void efi_st_printf(const char *fmt, ...)
  */
 int efi_st_memcmp(const void *buf1, const void *buf2, size_t length);
 
+/*
+ * Compare an u16 string to a char string.
+ *
+ * @buf1:	u16 string
+ * @buf2:	char string
+ * @return:	0 if both buffers contain the same bytes
+ */
+int efi_st_strcmp_16_8(const u16 *buf1, const char *buf2);
+
 /*
  * Reads an Unicode character from the input device.
  *
@@ -88,6 +98,7 @@ u16 efi_st_get_key(void);
  * @setup:	set up the unit test
  * @teardown:	tear down the unit test
  * @execute:	execute the unit test
+ * @on_request:	test is only executed on request
  */
 struct efi_unit_test {
 	const char *name;
@@ -96,10 +107,17 @@ struct efi_unit_test {
 		     const struct efi_system_table *systable);
 	int (*execute)(void);
 	int (*teardown)(void);
+	bool on_request;
 };
 
 /* Declare a new EFI unit test */
 #define EFI_UNIT_TEST(__name)						\
 	ll_entry_declare(struct efi_unit_test, __name, efi_unit_test)
 
+#define EFI_SELFTEST_TABLE_GUID \
+	EFI_GUID(0xbc3ebe57, 0x09e5, 0xa59d, 0xdb, 0x87, \
+		 0xf5, 0x79, 0x61, 0x62, 0x06, 0xde)
+
+extern const efi_guid_t efi_selftest_table_guid;
+
 #endif /* _EFI_SELFTEST_H */
diff --git a/lib/efi_selftest/efi_selftest.c b/lib/efi_selftest/efi_selftest.c
index 45d8d3d384..110284f9c7 100644
--- a/lib/efi_selftest/efi_selftest.c
+++ b/lib/efi_selftest/efi_selftest.c
@@ -15,6 +15,8 @@ static const struct efi_runtime_services *runtime;
 static efi_handle_t handle;
 static u16 reset_message[] = L"Selftest completed";
 
+const efi_guid_t efi_selftest_table_guid = EFI_SELFTEST_TABLE_GUID;
+
 /*
  * Exit the boot services.
  *
@@ -25,8 +27,8 @@ static u16 reset_message[] = L"Selftest completed";
  */
 void efi_st_exit_boot_services(void)
 {
-	unsigned long  map_size = 0;
-	unsigned long  map_key;
+	unsigned long map_size = 0;
+	unsigned long map_key;
 	unsigned long desc_size;
 	u32 desc_version;
 	efi_status_t ret;
@@ -133,6 +135,41 @@ static int teardown(struct efi_unit_test *test, unsigned int *failures)
 	return ret;
 }
 
+/*
+ * Check that a test exists.
+ *
+ * @testname:	name of the test
+ * @return:	test
+ */
+static struct efi_unit_test *find_test(const u16 *testname)
+{
+	struct efi_unit_test *test;
+
+	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 (!efi_st_strcmp_16_8(testname, test->name))
+			return test;
+	}
+	efi_st_printf("\nTest '%ps' not found\n", testname);
+	return NULL;
+}
+
+/*
+ * List all available tests.
+ */
+static void list_all_tests(void)
+{
+	struct efi_unit_test *test;
+
+	/* List all tests */
+	efi_st_printf("\nAvailable tests:\n");
+	for (test = ll_entry_start(struct efi_unit_test, efi_unit_test);
+	     test < ll_entry_end(struct efi_unit_test, efi_unit_test); ++test) {
+		efi_st_printf("'%s'%s\n", test->name,
+			      test->on_request ? " - on request" : "");
+	}
+}
+
 /*
  * Execute selftest of the EFI API
  *
@@ -155,6 +192,9 @@ efi_status_t EFIAPI efi_selftest(efi_handle_t image_handle,
 {
 	struct efi_unit_test *test;
 	unsigned int failures = 0;
+	const u16 *testname = NULL;
+	struct efi_loaded_image *loaded_image;
+	efi_status_t ret;
 
 	systable = systab;
 	boottime = systable->boottime;
@@ -163,14 +203,47 @@ efi_status_t EFIAPI efi_selftest(efi_handle_t image_handle,
 	con_out = systable->con_out;
 	con_in = systable->con_in;
 
+	ret = boottime->handle_protocol(image_handle, &efi_guid_loaded_image,
+					(void **)&loaded_image);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("Cannot open loaded image protocol");
+		return ret;
+	}
+
+	if (loaded_image->load_options)
+		testname = (u16 *)loaded_image->load_options;
+
+	if (testname) {
+		if (!efi_st_strcmp_16_8(testname, "list") ||
+		    !find_test(testname)) {
+			list_all_tests();
+			/*
+			 * TODO:
+			 * Once the Exit boottime service is correctly
+			 * implemented we should call
+			 *   boottime->exit(image_handle, EFI_SUCCESS, 0, NULL);
+			 * here, cf.
+			 * https://lists.denx.de/pipermail/u-boot/2017-October/308720.html
+			 */
+			return EFI_SUCCESS;
+		}
+	}
+
 	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));
+	if (testname)
+		efi_st_printf("\nSelected test: '%ps'\n", testname);
+	else
+		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 (testname ?
+		    efi_st_strcmp_16_8(testname, test->name) : test->on_request)
+			continue;
 		if (test->phase == EFI_EXECUTE_BEFORE_BOOTTIME_EXIT) {
 			setup(test, &failures);
 			execute(test, &failures);
@@ -181,6 +254,9 @@ efi_status_t EFIAPI efi_selftest(efi_handle_t image_handle,
 	/* 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 (testname ?
+		    efi_st_strcmp_16_8(testname, test->name) : test->on_request)
+			continue;
 		if (test->phase == EFI_SETUP_BEFORE_BOOTTIME_EXIT)
 			setup(test, &failures);
 	}
@@ -189,6 +265,9 @@ efi_status_t EFIAPI efi_selftest(efi_handle_t image_handle,
 
 	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 (testname ?
+		    efi_st_strcmp_16_8(testname, test->name) : test->on_request)
+			continue;
 		if (test->phase == EFI_SETUP_BEFORE_BOOTTIME_EXIT) {
 			execute(test, &failures);
 			teardown(test, &failures);
@@ -198,6 +277,9 @@ efi_status_t EFIAPI efi_selftest(efi_handle_t image_handle,
 	/* 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 (testname ?
+		    efi_st_strcmp_16_8(testname, test->name) : test->on_request)
+			continue;
 		if (test->phase == EFI_SETUP_AFTER_BOOTTIME_EXIT) {
 			setup(test, &failures);
 			execute(test, &failures);
diff --git a/lib/efi_selftest/efi_selftest_console.c b/lib/efi_selftest/efi_selftest_console.c
index 840e2290c6..6a7fd20da5 100644
--- a/lib/efi_selftest/efi_selftest_console.c
+++ b/lib/efi_selftest/efi_selftest_console.c
@@ -142,6 +142,7 @@ void efi_st_printf(const char *fmt, ...)
 	const char *c;
 	u16 *pos = buf;
 	const char *s;
+	const u16 *u;
 
 	va_start(args, fmt);
 
@@ -179,9 +180,18 @@ void efi_st_printf(const char *fmt, ...)
 			case 'p':
 				++c;
 				switch (*c) {
+				/* MAC address */
 				case 'm':
 					mac(va_arg(args, void*), &pos);
 					break;
+
+				/* u16 string */
+				case 's':
+					u = va_arg(args, u16*);
+					/* Ensure string fits into buffer */
+					for (; *u && pos < buf + 120; ++u)
+						*pos++ = *u;
+					break;
 				default:
 					--c;
 					pointer(va_arg(args, void*), &pos);
diff --git a/lib/efi_selftest/efi_selftest_util.c b/lib/efi_selftest/efi_selftest_util.c
index 5cffe383d8..1b17bf4d4b 100644
--- a/lib/efi_selftest/efi_selftest_util.c
+++ b/lib/efi_selftest/efi_selftest_util.c
@@ -21,5 +21,14 @@ int efi_st_memcmp(const void *buf1, const void *buf2, size_t length)
 		++pos1;
 		++pos2;
 	}
-	return EFI_ST_SUCCESS;
+	return 0;
+}
+
+int efi_st_strcmp_16_8(const u16 *buf1, const char *buf2)
+{
+	for (; *buf1 || *buf2; ++buf1, ++buf2) {
+		if (*buf1 != *buf2)
+			return *buf1 - *buf2;
+	}
+	return 0;
 }
-- 
2.14.1

[U-Boot] [PATCH v2 7/9] efi_selftest: test reboot by watchdog

[Heinrich Schuchardt]

A test is added that verifies that the watchdog timer actually
causes a reboot upon timeout. The test in only executed on
request using

    bootefi selftest 'watchdog reboot'

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2
	no change
---
 lib/efi_selftest/efi_selftest_watchdog.c | 65 +++++++++++++++++++++++++++-----
 1 file changed, 55 insertions(+), 10 deletions(-)

diff --git a/lib/efi_selftest/efi_selftest_watchdog.c b/lib/efi_selftest/efi_selftest_watchdog.c
index f8c5404000..a2c11ab1b9 100644
--- a/lib/efi_selftest/efi_selftest_watchdog.c
+++ b/lib/efi_selftest/efi_selftest_watchdog.c
@@ -5,11 +5,15 @@
  *
  * SPDX-License-Identifier:     GPL-2.0+
  *
- * This unit test checks that the watchdog timer will not cause
- * a system restart during the timeout period after a timer reset.
+ * The 'watchdog timer' unit test checks that the watchdog timer
+ * will not cause a system restart during the timeout period after
+ * a timer reset.
  *
- * Testing that the watchdog timer actually will reset the system
- * after a timeout is not possible within the used framework.
+ * The 'watchdog reboot' unit test checks that the watchdog timer
+ * actually reboots the system after a timeout. The test is only
+ * executed on explicit request. Use the following command:
+ *
+ *     bootefi selftest 'watchdog reboot'
  */
 
 #include <efi_selftest.h>
@@ -28,6 +32,7 @@ static struct efi_event *event_notify;
 static struct efi_event *event_wait;
 static struct efi_boot_services *boottime;
 static struct notify_context notification_context;
+static bool watchdog_reset;
 
 /*
  * Notification function, increments the notfication count if parameter
@@ -88,6 +93,34 @@ static int setup(const efi_handle_t handle,
 	return EFI_ST_SUCCESS;
 }
 
+/*
+ * Execute the test resetting the watchdog in a timely manner. No reboot occurs.
+ *
+ * @handle:	handle of the loaded image
+ * @systable:	system table
+ * @return:	EFI_ST_SUCCESS for success
+ */
+static int setup_timer(const efi_handle_t handle,
+		       const struct efi_system_table *systable)
+{
+	watchdog_reset = true;
+	return setup(handle, systable);
+}
+
+/*
+ * Execute the test without resetting the watchdog. A system reboot occurs.
+ *
+ * @handle:	handle of the loaded image
+ * @systable:	system table
+ * @return:	EFI_ST_SUCCESS for success
+ */
+static int setup_reboot(const efi_handle_t handle,
+			const struct efi_system_table *systable)
+{
+	watchdog_reset = false;
+	return setup(handle, systable);
+}
+
 /*
  * Tear down unit test.
  *
@@ -146,11 +179,14 @@ static int execute(void)
 		efi_st_error("Setting watchdog timer failed\n");
 		return EFI_ST_FAILURE;
 	}
+	if (watchdog_reset) {
 	/* Set 600 ms timer */
-	ret = boottime->set_timer(event_notify, EFI_TIMER_PERIODIC, 6000000);
-	if (ret != EFI_SUCCESS) {
-		efi_st_error("Could not set timer\n");
-		return EFI_ST_FAILURE;
+		ret = boottime->set_timer(event_notify, EFI_TIMER_PERIODIC,
+					  6000000);
+		if (ret != EFI_SUCCESS) {
+			efi_st_error("Could not set timer\n");
+			return EFI_ST_FAILURE;
+		}
 	}
 	/* Set 1350 ms timer */
 	ret = boottime->set_timer(event_wait, EFI_TIMER_RELATIVE, 13500000);
@@ -176,10 +212,19 @@ static int execute(void)
 	return EFI_ST_SUCCESS;
 }
 
-EFI_UNIT_TEST(watchdog) = {
+EFI_UNIT_TEST(watchdog1) = {
 	.name = "watchdog timer",
 	.phase = EFI_EXECUTE_BEFORE_BOOTTIME_EXIT,
-	.setup = setup,
+	.setup = setup_timer,
+	.execute = execute,
+	.teardown = teardown,
+};
+
+EFI_UNIT_TEST(watchdog2) = {
+	.name = "watchdog reboot",
+	.phase = EFI_EXECUTE_BEFORE_BOOTTIME_EXIT,
+	.setup = setup_reboot,
 	.execute = execute,
 	.teardown = teardown,
+	.on_request = true,
 };
-- 
2.14.1

[U-Boot] [PATCH v2 8/9] test/py: test reboot by EFI watchdog

[Heinrich Schuchardt]

Clear environment variable efi_selftest before executing the
default tests.

Provide a test verifying that the EFI watchdog
reboots the system upon timeout.

The test depends on CONFIG_CMD_EFI_SELFTEST=y.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2
	choose test via environment variable
---
 test/py/tests/test_efi_selftest.py | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/test/py/tests/test_efi_selftest.py b/test/py/tests/test_efi_selftest.py
index 76e282a6c7..66b799bed6 100644
--- a/test/py/tests/test_efi_selftest.py
+++ b/test/py/tests/test_efi_selftest.py
@@ -1,4 +1,3 @@
-# Copyright (c) 2016, NVIDIA CORPORATION. All rights reserved.
 # Copyright (c) 2017, Heinrich Schuchardt <xypron.glpk@gmx.de>
 #
 # SPDX-License-Identifier: GPL-2.0
@@ -14,6 +13,7 @@ def test_efi_selftest(u_boot_console):
 	Run bootefi selftest
 	"""
 
+	u_boot_console.run_command(cmd='setenv efi_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:
@@ -23,3 +23,15 @@ def test_efi_selftest(u_boot_console):
 	if m != 0:
 		raise Exception('Reset failed during the EFI selftest')
 	u_boot_console.restart_uboot();
+
+ at pytest.mark.buildconfigspec('cmd_bootefi_selftest')
+def test_efi_selftest_watchdog_reboot(u_boot_console):
+	u_boot_console.run_command(cmd='setenv efi_selftest list')
+	output = u_boot_console.run_command('bootefi selftest')
+	assert '\'watchdog reboot\'' in output
+	u_boot_console.run_command(cmd='setenv efi_selftest watchdog reboot')
+	u_boot_console.run_command(cmd='bootefi selftest', wait_for_prompt=False)
+	m = u_boot_console.p.expect(['resetting', 'U-Boot'])
+	if m != 0:
+		raise Exception('Reset failed in \'watchdog reboot\' test')
+	u_boot_console.restart_uboot();
-- 
2.14.1

[U-Boot] [PATCH v2 9/9] test/py: fix typo in test_efi_loader.py

[Heinrich Schuchardt]

Make a comment line easier to read.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2
	no change
---
 test/py/tests/test_efi_loader.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/py/tests/test_efi_loader.py b/test/py/tests/test_efi_loader.py
index 5d7f5dbfb2..4d391e13ef 100644
--- a/test/py/tests/test_efi_loader.py
+++ b/test/py/tests/test_efi_loader.py
@@ -12,7 +12,7 @@ import u_boot_utils
 
 """
 Note: This test relies on boardenv_* containing configuration values to define
-which the network environment available for testing. Without this, the parts
+which network environment is available for testing. Without this, the parts
 that rely on network will be automatically skipped.
 
 For example:
-- 
2.14.1

[U-Boot] [PATCH v2 2/9] efi_loader: implement SetWatchdogTimer

[Alexander Graf]

On 13.10.17 19:33, Heinrich Schuchardt wrote:
> The watchdog is initialized with a 5 minute timeout period.
> It can be reset by SetWatchdogTimer.
> It is stopped by ExitBoottimeServices.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v2
> 	code comments updated
> ---
>  cmd/bootefi.c                 |  1 +
>  include/efi_loader.h          |  4 ++
>  lib/efi_loader/Makefile       |  2 +-
>  lib/efi_loader/efi_boottime.c | 17 ++-------
>  lib/efi_loader/efi_watchdog.c | 86 +++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 95 insertions(+), 15 deletions(-)
>  create mode 100644 lib/efi_loader/efi_watchdog.c
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 478bc116e2..18176a1266 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -43,6 +43,7 @@ static void efi_init_obj_list(void)
>  #ifdef CONFIG_GENERATE_SMBIOS_TABLE
>  	efi_smbios_register();
>  #endif
> +	efi_watchdog_register();
>  
>  	/* Initialize EFI runtime services */
>  	efi_reset_system_init();
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 1b92edbd77..af64b11cee 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -163,6 +163,8 @@ int efi_disk_register(void);
>  int efi_gop_register(void);
>  /* Called by bootefi to make the network interface available */
>  int efi_net_register(void);
> +/* Called by bootefi to make the watchdog available */
> +int efi_watchdog_register(void);
>  /* Called by bootefi to make SMBIOS tables available */
>  void efi_smbios_register(void);
>  
> @@ -171,6 +173,8 @@ efi_fs_from_path(struct efi_device_path *fp);
>  
>  /* Called by networking code to memorize the dhcp ack package */
>  void efi_net_set_dhcp_ack(void *pkt, int len);
> +/* Called by efi_set_watchdog_timer to reset the timer */
> +efi_status_t efi_set_watchdog(unsigned long timeout);
>  
>  /* Called from places to check whether a timer expired */
>  void efi_timer_check(void);
> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> index ddb978f650..83d879b686 100644
> --- a/lib/efi_loader/Makefile
> +++ b/lib/efi_loader/Makefile
> @@ -17,7 +17,7 @@ endif
>  obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o
>  obj-y += efi_image_loader.o efi_boottime.o efi_runtime.o efi_console.o
>  obj-y += efi_memory.o efi_device_path_to_text.o efi_device_path.o
> -obj-y += efi_file.o efi_variable.o efi_bootmgr.o
> +obj-y += efi_file.o efi_variable.o efi_bootmgr.o efi_watchdog.o
>  obj-$(CONFIG_LCD) += efi_gop.o
>  obj-$(CONFIG_DM_VIDEO) += efi_gop.o
>  obj-$(CONFIG_PARTITIONS) += efi_disk.o
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 30577f717e..fd8d15655b 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -155,18 +155,6 @@ void efi_signal_event(struct efi_event *event)
>  	event->is_queued = false;
>  }
>  
> -/*
> - * Write a debug message for an EPI API service that is not implemented yet.
> - *
> - * @funcname	function that is not yet implemented
> - * @return	EFI_UNSUPPORTED
> - */
> -static efi_status_t efi_unsupported(const char *funcname)
> -{
> -	debug("EFI: App called into unimplemented function %s\n", funcname);
> -	return EFI_EXIT(EFI_UNSUPPORTED);
> -}
> -
>  /*
>   * Raise the task priority level.
>   *
> @@ -1454,6 +1442,7 @@ static efi_status_t EFIAPI efi_exit_boot_services(void *image_handle,
>  	bootm_disable_interrupts();
>  
>  	/* Give the payload some time to boot */
> +	efi_set_watchdog(0);
>  	WATCHDOG_RESET();
>  
>  	return EFI_EXIT(EFI_SUCCESS);
> @@ -1497,7 +1486,7 @@ static efi_status_t EFIAPI efi_stall(unsigned long microseconds)
>  /*
>   * Reset the watchdog timer.
>   *
> - * This function implements the WatchdogTimer service.
> + * This function implements the SetWatchdogTimer service.
>   * See the Unified Extensible Firmware Interface (UEFI) specification
>   * for details.
>   *
> @@ -1514,7 +1503,7 @@ static efi_status_t EFIAPI efi_set_watchdog_timer(unsigned long timeout,
>  {
>  	EFI_ENTRY("%ld, 0x%"PRIx64", %ld, %p", timeout, watchdog_code,
>  		  data_size, watchdog_data);
> -	return efi_unsupported(__func__);
> +	return EFI_EXIT(efi_set_watchdog(timeout));
>  }
>  
>  /*
> diff --git a/lib/efi_loader/efi_watchdog.c b/lib/efi_loader/efi_watchdog.c
> new file mode 100644
> index 0000000000..eb437faf4b
> --- /dev/null
> +++ b/lib/efi_loader/efi_watchdog.c
> @@ -0,0 +1,86 @@
> +/*
> + *  EFI watchdog
> + *
> + *  Copyright (c) 2017 Heinrich Schuchardt
> + *
> + *  SPDX-License-Identifier:     GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <efi_loader.h>
> +
> +static struct efi_event *watchdog_timer_event;
> +
> +/*
> + * Reset the system when the watchdog event is notified.
> + *
> + * @event:	the watchdog event
> + * @context:	not used
> + */
> +static void EFIAPI efi_watchdog_timer_notify(struct efi_event *event,
> +					     void *context)
> +{
> +	EFI_ENTRY("%p, %p", event, context);
> +
> +	printf("\nEFI: Watchdog timeout\n");
> +	EFI_CALL_VOID(efi_runtime_services.reset_system(EFI_RESET_COLD,
> +							EFI_SUCCESS, 0, NULL));
> +
> +	EFI_EXIT(EFI_UNSUPPORTED);
> +}
> +
> +/*
> + * Reset the watchdog timer.
> + *
> + * This function is used by the SetWatchdogTimer service.
> + *
> + * @timeout:		seconds before reset by watchdog
> + * @return:		status code
> + */
> +efi_status_t efi_set_watchdog(unsigned long timeout)
> +{
> +	efi_status_t r;
> +
> +	if (timeout)
> +		/* Reset watchdog */
> +		r = efi_set_timer(watchdog_timer_event, EFI_TIMER_RELATIVE,
> +				  10000000 * timeout);

Can we get a define for this number?

Also, wouldn't it make sense to make the constant ULL? That way we
extend the multiplication result to 64bit on 32bit hosts.

Otherwise we can only fit ~430 seconds (~7 minutes) into the resulting
number before we get an overflow. So if an application sets the timeout
to "in 60 minutes", we'll overflow and maybe trigger when the
application doesn't expect us to.


Alex

> +	else
> +		/* Deactivate watchdog */
> +		r = efi_set_timer(watchdog_timer_event, EFI_TIMER_STOP, 0);
> +	return r;
> +}
> +
> +/*
> + * Initialize the EFI watchdog.
> + *
> + * This function is called by efi_init_obj_list()
> + */
> +int efi_watchdog_register(void)
> +{
> +	efi_status_t r;
> +
> +	/*
> +	 * Create a timer event.
> +	 */
> +	r = efi_create_event(EVT_TIMER | EVT_NOTIFY_SIGNAL, TPL_CALLBACK,
> +			     efi_watchdog_timer_notify, NULL,
> +			     &watchdog_timer_event);
> +	if (r != EFI_SUCCESS) {
> +		printf("ERROR: Failed to register watchdog event\n");
> +		return r;
> +	}
> +	/*
> +	 * The UEFI standard requires that the watchdog timer is set to five
> +	 * minutes when invoking an EFI boot option.
> +	 *
> +	 * Unified Extensible Firmware Interface (UEFI), version 2.7 Errata A
> +	 * 7.5. Miscellaneous Boot Services - EFI_BOOT_SERVICES.SetWatchdogTimer
> +	 */
> +	r = efi_set_watchdog(300);
> +	if (r != EFI_SUCCESS) {
> +		printf("ERROR: Failed to set watchdog timer\n");
> +		return r;
> +	}
> +	return 0;
> +}
> 

[U-Boot] [PATCH v2 6/9] efi_selftest: allow to select a single test for exexution

[Alexander Graf]

On 13.10.17 19:33, Heinrich Schuchardt wrote:
> Environment variable efi_selftest is passed as load options
> to the selftest application. It is used to select a single
> test to be executed.
> 
> Special value 'list' displays a list of all available tests.
> 
> Tests get an on_request property. If this property is set
> the tests are only executed if explicitly requested.
> 
> The invocation of efi_selftest is changed to reflect that
> bootefi selftest with efi_selftest = 'list' will call the
> Exit bootservice.
> 
> Environment variable bootargs is used as load options
> for all other bootefi payloads.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v2
> 	use an environment variable to choose a test
> ---
>  cmd/bootefi.c                           | 46 ++++++++++++++++-
>  include/efi_selftest.h                  | 18 +++++++
>  lib/efi_selftest/efi_selftest.c         | 90 +++++++++++++++++++++++++++++++--
>  lib/efi_selftest/efi_selftest_console.c | 10 ++++
>  lib/efi_selftest/efi_selftest_util.c    | 11 +++-
>  5 files changed, 168 insertions(+), 7 deletions(-)
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 18176a1266..2d70137482 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -6,10 +6,12 @@
>   *  SPDX-License-Identifier:     GPL-2.0+
>   */
>  
> +#include <charset.h>
>  #include <common.h>
>  #include <command.h>
>  #include <dm.h>
>  #include <efi_loader.h>
> +#include <efi_selftest.h>
>  #include <errno.h>
>  #include <libfdt.h>
>  #include <libfdt_env.h>
> @@ -50,6 +52,32 @@ static void efi_init_obj_list(void)
>  	efi_get_time_init();
>  }
>  
> +/*
> + * Set the load options of an image from an environment variable.
> + *
> + * @loaded_image_info:	the image
> + * @env_var:		name of the environment variable
> + */
> +static void set_load_options(struct efi_loaded_image *loaded_image_info,
> +			     const char *env_var)
> +{
> +	size_t size;
> +	const char *env = env_get(env_var);
> +
> +	loaded_image_info->load_options = NULL;
> +	loaded_image_info->load_options_size = 0;
> +	if (!env)
> +		return;
> +	size = strlen(env) + 1;
> +	loaded_image_info->load_options = calloc(size, sizeof(u16));
> +	if (!loaded_image_info->load_options) {
> +		printf("ERROR: Out of memory\n");
> +		return;
> +	}
> +	utf8_to_utf16(loaded_image_info->load_options, (u8 *)env, size);
> +	loaded_image_info->load_options_size = size * 2;
> +}
> +
>  static void *copy_fdt(void *fdt)
>  {
>  	u64 fdt_size = fdt_totalsize(fdt);
> @@ -190,6 +218,8 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt,
>  		efi_install_configuration_table(&fdt_guid, NULL);
>  	}
>  
> +	/* Transfer environment variable bootargs as load options */
> +	set_load_options(&loaded_image_info, "bootargs");

While I really want to see that change, please try not to sneak it in
with the selftest one :).

Just split that hunk out to a following patch and give it its own patch
description. In case something goes wrong, we'd only need to revert a
small patch then.

>  	/* Load the EFI payload */
>  	entry = efi_load_pe(efi, &loaded_image_info);
>  	if (!entry) {
> @@ -237,6 +267,7 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt,
>  
>  exit:
>  	/* image has returned, loaded-image obj goes *poof*: */
> +	free(loaded_image_info.load_options);

This too is a change that doesn't fit the patch description?

>  	list_del(&loaded_image_info_obj.link);
>  
>  	return ret;
> @@ -301,17 +332,26 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  
>  		efi_setup_loaded_image(&loaded_image_info,
>  				       &loaded_image_info_obj,
> -				       bootefi_device_path, bootefi_image_path);
> +				       NULL, NULL);

Why?

>  		/*
>  		 * 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();
> +		loaded_image_info.image_code_type = EFI_LOADER_CODE;
> +		loaded_image_info.image_data_type = EFI_LOADER_DATA;

Also unrelated? Please split it out.

>  		/* Initialize and populate EFI object list */
>  		if (!efi_obj_list_initalized)
>  			efi_init_obj_list();
> -		return efi_selftest(&loaded_image_info, &systab);
> +		/* Transfer environment variable efi_selftest as load options */
> +		set_load_options(&loaded_image_info, "efi_selftest");
> +		/* Execute the test */
> +		r = efi_selftest(&loaded_image_info, &systab);
> +		efi_restore_gd();
> +		free(loaded_image_info.load_options);
> +		list_del(&loaded_image_info_obj.link);

That change too is unrelated to the patch description. Please split out.

> +		return r;
>  	} else
>  #endif
>  	if (!strcmp(argv[1], "bootmgr")) {
> @@ -357,6 +397,8 @@ static char bootefi_help_text[] =
>  #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
>  	"bootefi selftest\n"
>  	"  - boot an EFI selftest application stored within U-Boot\n"
> +	"    Use environment variable efi_selftest to select a single test.\n"
> +	"    Use 'setenv efi_selftest list' to enumerate all tests.\n"
>  #endif
>  	"bootmgr [fdt addr]\n"
>  	"  - load and boot EFI payload based on BootOrder/BootXXXX variables.\n"
> diff --git a/include/efi_selftest.h b/include/efi_selftest.h
> index 7ec42a0406..978ca2a7ea 100644
> --- a/include/efi_selftest.h
> +++ b/include/efi_selftest.h
> @@ -12,6 +12,7 @@
>  #include <common.h>
>  #include <efi.h>
>  #include <efi_api.h>
> +#include <efi_loader.h>
>  #include <linker_lists.h>
>  
>  #define EFI_ST_SUCCESS 0
> @@ -71,6 +72,15 @@ void efi_st_printf(const char *fmt, ...)
>   */
>  int efi_st_memcmp(const void *buf1, const void *buf2, size_t length);
>  
> +/*
> + * Compare an u16 string to a char string.
> + *
> + * @buf1:	u16 string
> + * @buf2:	char string
> + * @return:	0 if both buffers contain the same bytes
> + */
> +int efi_st_strcmp_16_8(const u16 *buf1, const char *buf2);
> +
>  /*
>   * Reads an Unicode character from the input device.
>   *
> @@ -88,6 +98,7 @@ u16 efi_st_get_key(void);
>   * @setup:	set up the unit test
>   * @teardown:	tear down the unit test
>   * @execute:	execute the unit test
> + * @on_request:	test is only executed on request
>   */
>  struct efi_unit_test {
>  	const char *name;
> @@ -96,10 +107,17 @@ struct efi_unit_test {
>  		     const struct efi_system_table *systable);
>  	int (*execute)(void);
>  	int (*teardown)(void);
> +	bool on_request;
>  };
>  
>  /* Declare a new EFI unit test */
>  #define EFI_UNIT_TEST(__name)						\
>  	ll_entry_declare(struct efi_unit_test, __name, efi_unit_test)
>  
> +#define EFI_SELFTEST_TABLE_GUID \
> +	EFI_GUID(0xbc3ebe57, 0x09e5, 0xa59d, 0xdb, 0x87, \
> +		 0xf5, 0x79, 0x61, 0x62, 0x06, 0xde)
> +
> +extern const efi_guid_t efi_selftest_table_guid;
> +
>  #endif /* _EFI_SELFTEST_H */
> diff --git a/lib/efi_selftest/efi_selftest.c b/lib/efi_selftest/efi_selftest.c
> index 45d8d3d384..110284f9c7 100644
> --- a/lib/efi_selftest/efi_selftest.c
> +++ b/lib/efi_selftest/efi_selftest.c
> @@ -15,6 +15,8 @@ static const struct efi_runtime_services *runtime;
>  static efi_handle_t handle;
>  static u16 reset_message[] = L"Selftest completed";
>  
> +const efi_guid_t efi_selftest_table_guid = EFI_SELFTEST_TABLE_GUID;
> +
>  /*
>   * Exit the boot services.
>   *
> @@ -25,8 +27,8 @@ static u16 reset_message[] = L"Selftest completed";
>   */
>  void efi_st_exit_boot_services(void)
>  {
> -	unsigned long  map_size = 0;
> -	unsigned long  map_key;
> +	unsigned long map_size = 0;
> +	unsigned long map_key;

Unrelated?

>  	unsigned long desc_size;
>  	u32 desc_version;
>  	efi_status_t ret;
> @@ -133,6 +135,41 @@ static int teardown(struct efi_unit_test *test, unsigned int *failures)
>  	return ret;
>  }
>  
> +/*
> + * Check that a test exists.
> + *
> + * @testname:	name of the test
> + * @return:	test
> + */
> +static struct efi_unit_test *find_test(const u16 *testname)
> +{
> +	struct efi_unit_test *test;
> +
> +	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 (!efi_st_strcmp_16_8(testname, test->name))

Why not just use UCS2 strings here and compare 16 to 16? Maybe you're
using the name more often in normal situations?

Either way, not a biggie :)

> +			return test;
> +	}
> +	efi_st_printf("\nTest '%ps' not found\n", testname);
> +	return NULL;
> +}
> +
> +/*
> + * List all available tests.
> + */
> +static void list_all_tests(void)
> +{
> +	struct efi_unit_test *test;
> +
> +	/* List all tests */
> +	efi_st_printf("\nAvailable tests:\n");
> +	for (test = ll_entry_start(struct efi_unit_test, efi_unit_test);
> +	     test < ll_entry_end(struct efi_unit_test, efi_unit_test); ++test) {
> +		efi_st_printf("'%s'%s\n", test->name,
> +			      test->on_request ? " - on request" : "");
> +	}
> +}
> +
>  /*
>   * Execute selftest of the EFI API
>   *
> @@ -155,6 +192,9 @@ efi_status_t EFIAPI efi_selftest(efi_handle_t image_handle,
>  {
>  	struct efi_unit_test *test;
>  	unsigned int failures = 0;
> +	const u16 *testname = NULL;
> +	struct efi_loaded_image *loaded_image;
> +	efi_status_t ret;
>  
>  	systable = systab;
>  	boottime = systable->boottime;
> @@ -163,14 +203,47 @@ efi_status_t EFIAPI efi_selftest(efi_handle_t image_handle,
>  	con_out = systable->con_out;
>  	con_in = systable->con_in;
>  
> +	ret = boottime->handle_protocol(image_handle, &efi_guid_loaded_image,
> +					(void **)&loaded_image);
> +	if (ret != EFI_SUCCESS) {
> +		efi_st_error("Cannot open loaded image protocol");
> +		return ret;
> +	}
> +
> +	if (loaded_image->load_options)
> +		testname = (u16 *)loaded_image->load_options;
> +
> +	if (testname) {
> +		if (!efi_st_strcmp_16_8(testname, "list") ||
> +		    !find_test(testname)) {
> +			list_all_tests();
> +			/*
> +			 * TODO:
> +			 * Once the Exit boottime service is correctly
> +			 * implemented we should call
> +			 *   boottime->exit(image_handle, EFI_SUCCESS, 0, NULL);
> +			 * here, cf.
> +			 * https://lists.denx.de/pipermail/u-boot/2017-October/308720.html
> +			 */
> +			return EFI_SUCCESS;
> +		}
> +	}
> +
>  	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));
> +	if (testname)
> +		efi_st_printf("\nSelected test: '%ps'\n", testname);
> +	else
> +		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 (testname ?
> +		    efi_st_strcmp_16_8(testname, test->name) : test->on_request)
> +			continue;
>  		if (test->phase == EFI_EXECUTE_BEFORE_BOOTTIME_EXIT) {
>  			setup(test, &failures);
>  			execute(test, &failures);
> @@ -181,6 +254,9 @@ efi_status_t EFIAPI efi_selftest(efi_handle_t image_handle,
>  	/* 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 (testname ?
> +		    efi_st_strcmp_16_8(testname, test->name) : test->on_request)
> +			continue;
>  		if (test->phase == EFI_SETUP_BEFORE_BOOTTIME_EXIT)
>  			setup(test, &failures);
>  	}
> @@ -189,6 +265,9 @@ efi_status_t EFIAPI efi_selftest(efi_handle_t image_handle,
>  
>  	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 (testname ?
> +		    efi_st_strcmp_16_8(testname, test->name) : test->on_request)
> +			continue;
>  		if (test->phase == EFI_SETUP_BEFORE_BOOTTIME_EXIT) {
>  			execute(test, &failures);
>  			teardown(test, &failures);
> @@ -198,6 +277,9 @@ efi_status_t EFIAPI efi_selftest(efi_handle_t image_handle,
>  	/* 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 (testname ?
> +		    efi_st_strcmp_16_8(testname, test->name) : test->on_request)

This pattern occurs a few time - maybe make a helper function out of it?

static bool test_is_active(const u16 *testname, struct efi_unit_test *test)
{
    if (testname)
        return !efi_st_strcmp_16_8(testname, test->name);
    else
        return !test->on_request;
}

> +			continue;
>  		if (test->phase == EFI_SETUP_AFTER_BOOTTIME_EXIT) {
>  			setup(test, &failures);
>  			execute(test, &failures);
> diff --git a/lib/efi_selftest/efi_selftest_console.c b/lib/efi_selftest/efi_selftest_console.c
> index 840e2290c6..6a7fd20da5 100644
> --- a/lib/efi_selftest/efi_selftest_console.c
> +++ b/lib/efi_selftest/efi_selftest_console.c
> @@ -142,6 +142,7 @@ void efi_st_printf(const char *fmt, ...)
>  	const char *c;
>  	u16 *pos = buf;
>  	const char *s;
> +	const u16 *u;
>  
>  	va_start(args, fmt);
>  
> @@ -179,9 +180,18 @@ void efi_st_printf(const char *fmt, ...)
>  			case 'p':
>  				++c;
>  				switch (*c) {
> +				/* MAC address */
>  				case 'm':
>  					mac(va_arg(args, void*), &pos);
>  					break;
> +
> +				/* u16 string */
> +				case 's':
> +					u = va_arg(args, u16*);
> +					/* Ensure string fits into buffer */
> +					for (; *u && pos < buf + 120; ++u)
> +						*pos++ = *u;
> +					break;
>  				default:
>  					--c;
>  					pointer(va_arg(args, void*), &pos);
> diff --git a/lib/efi_selftest/efi_selftest_util.c b/lib/efi_selftest/efi_selftest_util.c
> index 5cffe383d8..1b17bf4d4b 100644
> --- a/lib/efi_selftest/efi_selftest_util.c
> +++ b/lib/efi_selftest/efi_selftest_util.c
> @@ -21,5 +21,14 @@ int efi_st_memcmp(const void *buf1, const void *buf2, size_t length)
>  		++pos1;
>  		++pos2;
>  	}
> -	return EFI_ST_SUCCESS;
> +	return 0;

Also unrelated ;)


Alex

> +}
> +
> +int efi_st_strcmp_16_8(const u16 *buf1, const char *buf2)
> +{
> +	for (; *buf1 || *buf2; ++buf1, ++buf2) {
> +		if (*buf1 != *buf2)
> +			return *buf1 - *buf2;
> +	}
> +	return 0;
>  }
> 

[U-Boot] [PATCH v2 7/9] efi_selftest: test reboot by watchdog

[Alexander Graf]

On 13.10.17 19:33, Heinrich Schuchardt wrote:
> A test is added that verifies that the watchdog timer actually
> causes a reboot upon timeout. The test in only executed on
> request using
> 
>     bootefi selftest 'watchdog reboot'
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v2
> 	no change
> ---
>  lib/efi_selftest/efi_selftest_watchdog.c | 65 +++++++++++++++++++++++++++-----
>  1 file changed, 55 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/efi_selftest/efi_selftest_watchdog.c b/lib/efi_selftest/efi_selftest_watchdog.c
> index f8c5404000..a2c11ab1b9 100644
> --- a/lib/efi_selftest/efi_selftest_watchdog.c
> +++ b/lib/efi_selftest/efi_selftest_watchdog.c
> @@ -5,11 +5,15 @@
>   *
>   * SPDX-License-Identifier:     GPL-2.0+
>   *
> - * This unit test checks that the watchdog timer will not cause
> - * a system restart during the timeout period after a timer reset.
> + * The 'watchdog timer' unit test checks that the watchdog timer
> + * will not cause a system restart during the timeout period after
> + * a timer reset.
>   *
> - * Testing that the watchdog timer actually will reset the system
> - * after a timeout is not possible within the used framework.
> + * The 'watchdog reboot' unit test checks that the watchdog timer
> + * actually reboots the system after a timeout. The test is only
> + * executed on explicit request. Use the following command:
> + *
> + *     bootefi selftest 'watchdog reboot'
>   */
>  
>  #include <efi_selftest.h>
> @@ -28,6 +32,7 @@ static struct efi_event *event_notify;
>  static struct efi_event *event_wait;
>  static struct efi_boot_services *boottime;
>  static struct notify_context notification_context;
> +static bool watchdog_reset;
>  
>  /*
>   * Notification function, increments the notfication count if parameter
> @@ -88,6 +93,34 @@ static int setup(const efi_handle_t handle,
>  	return EFI_ST_SUCCESS;
>  }
>  
> +/*
> + * Execute the test resetting the watchdog in a timely manner. No reboot occurs.
> + *
> + * @handle:	handle of the loaded image
> + * @systable:	system table
> + * @return:	EFI_ST_SUCCESS for success
> + */
> +static int setup_timer(const efi_handle_t handle,
> +		       const struct efi_system_table *systable)
> +{
> +	watchdog_reset = true;
> +	return setup(handle, systable);
> +}
> +
> +/*
> + * Execute the test without resetting the watchdog. A system reboot occurs.
> + *
> + * @handle:	handle of the loaded image
> + * @systable:	system table
> + * @return:	EFI_ST_SUCCESS for success
> + */
> +static int setup_reboot(const efi_handle_t handle,
> +			const struct efi_system_table *systable)
> +{
> +	watchdog_reset = false;
> +	return setup(handle, systable);
> +}
> +
>  /*
>   * Tear down unit test.
>   *
> @@ -146,11 +179,14 @@ static int execute(void)
>  		efi_st_error("Setting watchdog timer failed\n");
>  		return EFI_ST_FAILURE;
>  	}
> +	if (watchdog_reset) {
>  	/* Set 600 ms timer */

Please indent :)


Alex

> -	ret = boottime->set_timer(event_notify, EFI_TIMER_PERIODIC, 6000000);
> -	if (ret != EFI_SUCCESS) {
> -		efi_st_error("Could not set timer\n");
> -		return EFI_ST_FAILURE;
> +		ret = boottime->set_timer(event_notify, EFI_TIMER_PERIODIC,
> +					  6000000);
> +		if (ret != EFI_SUCCESS) {
> +			efi_st_error("Could not set timer\n");
> +			return EFI_ST_FAILURE;
> +		}
>  	}
>  	/* Set 1350 ms timer */
>  	ret = boottime->set_timer(event_wait, EFI_TIMER_RELATIVE, 13500000);
> @@ -176,10 +212,19 @@ static int execute(void)
>  	return EFI_ST_SUCCESS;
>  }
>  
> -EFI_UNIT_TEST(watchdog) = {
> +EFI_UNIT_TEST(watchdog1) = {
>  	.name = "watchdog timer",
>  	.phase = EFI_EXECUTE_BEFORE_BOOTTIME_EXIT,
> -	.setup = setup,
> +	.setup = setup_timer,
> +	.execute = execute,
> +	.teardown = teardown,
> +};
> +
> +EFI_UNIT_TEST(watchdog2) = {
> +	.name = "watchdog reboot",
> +	.phase = EFI_EXECUTE_BEFORE_BOOTTIME_EXIT,
> +	.setup = setup_reboot,
>  	.execute = execute,
>  	.teardown = teardown,
> +	.on_request = true,
>  };
> 

[U-Boot] [PATCH v2 6/9] efi_selftest: allow to select a single test for exexution

[Heinrich Schuchardt]

On 10/17/2017 09:48 AM, Alexander Graf wrote:
> 
> 
> On 13.10.17 19:33, Heinrich Schuchardt wrote:
>> Environment variable efi_selftest is passed as load options
>> to the selftest application. It is used to select a single
>> test to be executed.
>>
>> Special value 'list' displays a list of all available tests.
>>
>> Tests get an on_request property. If this property is set
>> the tests are only executed if explicitly requested.
>>
>> The invocation of efi_selftest is changed to reflect that
>> bootefi selftest with efi_selftest = 'list' will call the
>> Exit bootservice.
>>
>> Environment variable bootargs is used as load options
>> for all other bootefi payloads.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>> v2
>> 	use an environment variable to choose a test
>> ---
>>  cmd/bootefi.c                           | 46 ++++++++++++++++-
>>  include/efi_selftest.h                  | 18 +++++++
>>  lib/efi_selftest/efi_selftest.c         | 90 +++++++++++++++++++++++++++++++--
>>  lib/efi_selftest/efi_selftest_console.c | 10 ++++
>>  lib/efi_selftest/efi_selftest_util.c    | 11 +++-
>>  5 files changed, 168 insertions(+), 7 deletions(-)
>>
>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>> index 18176a1266..2d70137482 100644
>> --- a/cmd/bootefi.c
>> +++ b/cmd/bootefi.c
>> @@ -6,10 +6,12 @@
>>   *  SPDX-License-Identifier:     GPL-2.0+
>>   */
>>  
>> +#include <charset.h>
>>  #include <common.h>
>>  #include <command.h>
>>  #include <dm.h>
>>  #include <efi_loader.h>
>> +#include <efi_selftest.h>
>>  #include <errno.h>
>>  #include <libfdt.h>
>>  #include <libfdt_env.h>
>> @@ -50,6 +52,32 @@ static void efi_init_obj_list(void)
>>  	efi_get_time_init();
>>  }
>>  
>> +/*
>> + * Set the load options of an image from an environment variable.
>> + *
>> + * @loaded_image_info:	the image
>> + * @env_var:		name of the environment variable
>> + */
>> +static void set_load_options(struct efi_loaded_image *loaded_image_info,
>> +			     const char *env_var)
>> +{
>> +	size_t size;
>> +	const char *env = env_get(env_var);
>> +
>> +	loaded_image_info->load_options = NULL;
>> +	loaded_image_info->load_options_size = 0;
>> +	if (!env)
>> +		return;
>> +	size = strlen(env) + 1;
>> +	loaded_image_info->load_options = calloc(size, sizeof(u16));
>> +	if (!loaded_image_info->load_options) {
>> +		printf("ERROR: Out of memory\n");
>> +		return;
>> +	}
>> +	utf8_to_utf16(loaded_image_info->load_options, (u8 *)env, size);
>> +	loaded_image_info->load_options_size = size * 2;
>> +}
>> +
>>  static void *copy_fdt(void *fdt)
>>  {
>>  	u64 fdt_size = fdt_totalsize(fdt);
>> @@ -190,6 +218,8 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt,
>>  		efi_install_configuration_table(&fdt_guid, NULL);
>>  	}
>>  
>> +	/* Transfer environment variable bootargs as load options */
>> +	set_load_options(&loaded_image_info, "bootargs");
> 
> While I really want to see that change, please try not to sneak it in
> with the selftest one :).
> 
> Just split that hunk out to a following patch and give it its own patch
> description. In case something goes wrong, we'd only need to revert a
> small patch then.
> 
>>  	/* Load the EFI payload */
>>  	entry = efi_load_pe(efi, &loaded_image_info);
>>  	if (!entry) {
>> @@ -237,6 +267,7 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt,
>>  
>>  exit:
>>  	/* image has returned, loaded-image obj goes *poof*: */
>> +	free(loaded_image_info.load_options);
> 
> This too is a change that doesn't fit the patch description?
> 
>>  	list_del(&loaded_image_info_obj.link);
>>  
>>  	return ret;
>> @@ -301,17 +332,26 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>  
>>  		efi_setup_loaded_image(&loaded_image_info,
>>  				       &loaded_image_info_obj,
>> -				       bootefi_device_path, bootefi_image_path);
>> +				       NULL, NULL);
> 
> Why?

We have neither a device nor an image path here. We do not want to use
the values that where set by a prior call.

bf19273e81eb efi_loader: Add mem-mapped for fallback
provided a logic for helloworld.

I will add a preceding patch to move that to efi_setup_loaded_image.

Regards

Heinrich

[U-Boot] [PATCH v2 6/9] efi_selftest: allow to select a single test for exexution

[Heinrich Schuchardt]

On 10/17/2017 09:48 AM, Alexander Graf wrote:
> 
> 
> On 13.10.17 19:33, Heinrich Schuchardt wrote:
>> Environment variable efi_selftest is passed as load options
>> to the selftest application. It is used to select a single
>> test to be executed.
>>
>> Special value 'list' displays a list of all available tests.
>>
>> Tests get an on_request property. If this property is set
>> the tests are only executed if explicitly requested.
>>
>> The invocation of efi_selftest is changed to reflect that
>> bootefi selftest with efi_selftest = 'list' will call the
>> Exit bootservice.
>>
>> Environment variable bootargs is used as load options
>> for all other bootefi payloads.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>> v2
>> 	use an environment variable to choose a test
>> ---
>>  cmd/bootefi.c                           | 46 ++++++++++++++++-
>>  include/efi_selftest.h                  | 18 +++++++
>>  lib/efi_selftest/efi_selftest.c         | 90 +++++++++++++++++++++++++++++++--
>>  lib/efi_selftest/efi_selftest_console.c | 10 ++++
>>  lib/efi_selftest/efi_selftest_util.c    | 11 +++-
>>  5 files changed, 168 insertions(+), 7 deletions(-)
>>
>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>> index 18176a1266..2d70137482 100644
>> --- a/cmd/bootefi.c
>> +++ b/cmd/bootefi.c
>> @@ -6,10 +6,12 @@
>>   *  SPDX-License-Identifier:     GPL-2.0+
>>   */
>>  
>> +#include <charset.h>
>>  #include <common.h>
>>  #include <command.h>
>>  #include <dm.h>
>>  #include <efi_loader.h>
>> +#include <efi_selftest.h>
>>  #include <errno.h>
>>  #include <libfdt.h>
>>  #include <libfdt_env.h>
>> @@ -50,6 +52,32 @@ static void efi_init_obj_list(void)
>>  	efi_get_time_init();
>>  }
>>  
>> +/*
>> + * Set the load options of an image from an environment variable.
>> + *
>> + * @loaded_image_info:	the image
>> + * @env_var:		name of the environment variable
>> + */
>> +static void set_load_options(struct efi_loaded_image *loaded_image_info,
>> +			     const char *env_var)
>> +{
>> +	size_t size;
>> +	const char *env = env_get(env_var);
>> +
>> +	loaded_image_info->load_options = NULL;
>> +	loaded_image_info->load_options_size = 0;
>> +	if (!env)
>> +		return;
>> +	size = strlen(env) + 1;
>> +	loaded_image_info->load_options = calloc(size, sizeof(u16));
>> +	if (!loaded_image_info->load_options) {
>> +		printf("ERROR: Out of memory\n");
>> +		return;
>> +	}
>> +	utf8_to_utf16(loaded_image_info->load_options, (u8 *)env, size);
>> +	loaded_image_info->load_options_size = size * 2;
>> +}
>> +
>>  static void *copy_fdt(void *fdt)
>>  {
>>  	u64 fdt_size = fdt_totalsize(fdt);
>> @@ -190,6 +218,8 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt,
>>  		efi_install_configuration_table(&fdt_guid, NULL);
>>  	}
>>  
>> +	/* Transfer environment variable bootargs as load options */
>> +	set_load_options(&loaded_image_info, "bootargs");
> 
> While I really want to see that change, please try not to sneak it in
> with the selftest one :).
> 
> Just split that hunk out to a following patch and give it its own patch
> description. In case something goes wrong, we'd only need to revert a
> small patch then.
> 
>>  	/* Load the EFI payload */
>>  	entry = efi_load_pe(efi, &loaded_image_info);
>>  	if (!entry) {
>> @@ -237,6 +267,7 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt,
>>  
>>  exit:
>>  	/* image has returned, loaded-image obj goes *poof*: */
>> +	free(loaded_image_info.load_options);
> 
> This too is a change that doesn't fit the patch description?
> 
>>  	list_del(&loaded_image_info_obj.link);
>>  
>>  	return ret;
>> @@ -301,17 +332,26 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>  
>>  		efi_setup_loaded_image(&loaded_image_info,
>>  				       &loaded_image_info_obj,
>> -				       bootefi_device_path, bootefi_image_path);
>> +				       NULL, NULL);
> 
> Why?
> 
>>  		/*
>>  		 * 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();
>> +		loaded_image_info.image_code_type = EFI_LOADER_CODE;
>> +		loaded_image_info.image_data_type = EFI_LOADER_DATA;
> 
> Also unrelated? Please split it out.
> 
>>  		/* Initialize and populate EFI object list */
>>  		if (!efi_obj_list_initalized)
>>  			efi_init_obj_list();
>> -		return efi_selftest(&loaded_image_info, &systab);
>> +		/* Transfer environment variable efi_selftest as load options */
>> +		set_load_options(&loaded_image_info, "efi_selftest");
>> +		/* Execute the test */
>> +		r = efi_selftest(&loaded_image_info, &systab);
>> +		efi_restore_gd();
>> +		free(loaded_image_info.load_options);
>> +		list_del(&loaded_image_info_obj.link);
> 
> That change too is unrelated to the patch description. Please split out.
> 
>> +		return r;
>>  	} else
>>  #endif
>>  	if (!strcmp(argv[1], "bootmgr")) {
>> @@ -357,6 +397,8 @@ static char bootefi_help_text[] =
>>  #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
>>  	"bootefi selftest\n"
>>  	"  - boot an EFI selftest application stored within U-Boot\n"
>> +	"    Use environment variable efi_selftest to select a single test.\n"
>> +	"    Use 'setenv efi_selftest list' to enumerate all tests.\n"
>>  #endif
>>  	"bootmgr [fdt addr]\n"
>>  	"  - load and boot EFI payload based on BootOrder/BootXXXX variables.\n"
>> diff --git a/include/efi_selftest.h b/include/efi_selftest.h
>> index 7ec42a0406..978ca2a7ea 100644
>> --- a/include/efi_selftest.h
>> +++ b/include/efi_selftest.h
>> @@ -12,6 +12,7 @@
>>  #include <common.h>
>>  #include <efi.h>
>>  #include <efi_api.h>
>> +#include <efi_loader.h>
>>  #include <linker_lists.h>
>>  
>>  #define EFI_ST_SUCCESS 0
>> @@ -71,6 +72,15 @@ void efi_st_printf(const char *fmt, ...)
>>   */
>>  int efi_st_memcmp(const void *buf1, const void *buf2, size_t length);
>>  
>> +/*
>> + * Compare an u16 string to a char string.
>> + *
>> + * @buf1:	u16 string
>> + * @buf2:	char string
>> + * @return:	0 if both buffers contain the same bytes
>> + */
>> +int efi_st_strcmp_16_8(const u16 *buf1, const char *buf2);
>> +
>>  /*
>>   * Reads an Unicode character from the input device.
>>   *
>> @@ -88,6 +98,7 @@ u16 efi_st_get_key(void);
>>   * @setup:	set up the unit test
>>   * @teardown:	tear down the unit test
>>   * @execute:	execute the unit test
>> + * @on_request:	test is only executed on request
>>   */
>>  struct efi_unit_test {
>>  	const char *name;
>> @@ -96,10 +107,17 @@ struct efi_unit_test {
>>  		     const struct efi_system_table *systable);
>>  	int (*execute)(void);
>>  	int (*teardown)(void);
>> +	bool on_request;
>>  };
>>  
>>  /* Declare a new EFI unit test */
>>  #define EFI_UNIT_TEST(__name)						\
>>  	ll_entry_declare(struct efi_unit_test, __name, efi_unit_test)
>>  
>> +#define EFI_SELFTEST_TABLE_GUID \
>> +	EFI_GUID(0xbc3ebe57, 0x09e5, 0xa59d, 0xdb, 0x87, \
>> +		 0xf5, 0x79, 0x61, 0x62, 0x06, 0xde)
>> +
>> +extern const efi_guid_t efi_selftest_table_guid;
>> +
>>  #endif /* _EFI_SELFTEST_H */
>> diff --git a/lib/efi_selftest/efi_selftest.c b/lib/efi_selftest/efi_selftest.c
>> index 45d8d3d384..110284f9c7 100644
>> --- a/lib/efi_selftest/efi_selftest.c
>> +++ b/lib/efi_selftest/efi_selftest.c
>> @@ -15,6 +15,8 @@ static const struct efi_runtime_services *runtime;
>>  static efi_handle_t handle;
>>  static u16 reset_message[] = L"Selftest completed";
>>  
>> +const efi_guid_t efi_selftest_table_guid = EFI_SELFTEST_TABLE_GUID;
>> +
>>  /*
>>   * Exit the boot services.
>>   *
>> @@ -25,8 +27,8 @@ static u16 reset_message[] = L"Selftest completed";
>>   */
>>  void efi_st_exit_boot_services(void)
>>  {
>> -	unsigned long  map_size = 0;
>> -	unsigned long  map_key;
>> +	unsigned long map_size = 0;
>> +	unsigned long map_key;
> 
> Unrelated?
> 
>>  	unsigned long desc_size;
>>  	u32 desc_version;
>>  	efi_status_t ret;
>> @@ -133,6 +135,41 @@ static int teardown(struct efi_unit_test *test, unsigned int *failures)
>>  	return ret;
>>  }
>>  
>> +/*
>> + * Check that a test exists.
>> + *
>> + * @testname:	name of the test
>> + * @return:	test
>> + */
>> +static struct efi_unit_test *find_test(const u16 *testname)
>> +{
>> +	struct efi_unit_test *test;
>> +
>> +	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 (!efi_st_strcmp_16_8(testname, test->name))
> 
> Why not just use UCS2 strings here and compare 16 to 16? Maybe you're
> using the name more often in normal situations?
> 
> Either way, not a biggie :)

I thought about defining the test names as UCS2. But wide strings need
double the space.

So I decided to stay with char * as far as possible to reduce code size.

I remember that reviewing one patch you asked me to rearrange function
arguments to save 16 bytes of compiled code size for a specific
architecture.

Best regards

Heinrich

[U-Boot] [PATCH v2 6/9] efi_selftest: allow to select a single test for exexution

[Alexander Graf]

On 17.10.17 22:11, Heinrich Schuchardt wrote:
> On 10/17/2017 09:48 AM, Alexander Graf wrote:
>>
>>
>> On 13.10.17 19:33, Heinrich Schuchardt wrote:
>>> Environment variable efi_selftest is passed as load options
>>> to the selftest application. It is used to select a single
>>> test to be executed.
>>>
>>> Special value 'list' displays a list of all available tests.
>>>
>>> Tests get an on_request property. If this property is set
>>> the tests are only executed if explicitly requested.
>>>
>>> The invocation of efi_selftest is changed to reflect that
>>> bootefi selftest with efi_selftest = 'list' will call the
>>> Exit bootservice.
>>>
>>> Environment variable bootargs is used as load options
>>> for all other bootefi payloads.
>>>
>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>> ---
>>> v2
>>> 	use an environment variable to choose a test
>>> ---
>>>  cmd/bootefi.c                           | 46 ++++++++++++++++-
>>>  include/efi_selftest.h                  | 18 +++++++
>>>  lib/efi_selftest/efi_selftest.c         | 90 +++++++++++++++++++++++++++++++--
>>>  lib/efi_selftest/efi_selftest_console.c | 10 ++++
>>>  lib/efi_selftest/efi_selftest_util.c    | 11 +++-
>>>  5 files changed, 168 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>> index 18176a1266..2d70137482 100644
>>> --- a/cmd/bootefi.c
>>> +++ b/cmd/bootefi.c
>>> @@ -6,10 +6,12 @@
>>>   *  SPDX-License-Identifier:     GPL-2.0+
>>>   */
>>>  
>>> +#include <charset.h>
>>>  #include <common.h>
>>>  #include <command.h>
>>>  #include <dm.h>
>>>  #include <efi_loader.h>
>>> +#include <efi_selftest.h>
>>>  #include <errno.h>
>>>  #include <libfdt.h>
>>>  #include <libfdt_env.h>
>>> @@ -50,6 +52,32 @@ static void efi_init_obj_list(void)
>>>  	efi_get_time_init();
>>>  }
>>>  
>>> +/*
>>> + * Set the load options of an image from an environment variable.
>>> + *
>>> + * @loaded_image_info:	the image
>>> + * @env_var:		name of the environment variable
>>> + */
>>> +static void set_load_options(struct efi_loaded_image *loaded_image_info,
>>> +			     const char *env_var)
>>> +{
>>> +	size_t size;
>>> +	const char *env = env_get(env_var);
>>> +
>>> +	loaded_image_info->load_options = NULL;
>>> +	loaded_image_info->load_options_size = 0;
>>> +	if (!env)
>>> +		return;
>>> +	size = strlen(env) + 1;
>>> +	loaded_image_info->load_options = calloc(size, sizeof(u16));
>>> +	if (!loaded_image_info->load_options) {
>>> +		printf("ERROR: Out of memory\n");
>>> +		return;
>>> +	}
>>> +	utf8_to_utf16(loaded_image_info->load_options, (u8 *)env, size);
>>> +	loaded_image_info->load_options_size = size * 2;
>>> +}
>>> +
>>>  static void *copy_fdt(void *fdt)
>>>  {
>>>  	u64 fdt_size = fdt_totalsize(fdt);
>>> @@ -190,6 +218,8 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt,
>>>  		efi_install_configuration_table(&fdt_guid, NULL);
>>>  	}
>>>  
>>> +	/* Transfer environment variable bootargs as load options */
>>> +	set_load_options(&loaded_image_info, "bootargs");
>>
>> While I really want to see that change, please try not to sneak it in
>> with the selftest one :).
>>
>> Just split that hunk out to a following patch and give it its own patch
>> description. In case something goes wrong, we'd only need to revert a
>> small patch then.
>>
>>>  	/* Load the EFI payload */
>>>  	entry = efi_load_pe(efi, &loaded_image_info);
>>>  	if (!entry) {
>>> @@ -237,6 +267,7 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt,
>>>  
>>>  exit:
>>>  	/* image has returned, loaded-image obj goes *poof*: */
>>> +	free(loaded_image_info.load_options);
>>
>> This too is a change that doesn't fit the patch description?
>>
>>>  	list_del(&loaded_image_info_obj.link);
>>>  
>>>  	return ret;
>>> @@ -301,17 +332,26 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>>  
>>>  		efi_setup_loaded_image(&loaded_image_info,
>>>  				       &loaded_image_info_obj,
>>> -				       bootefi_device_path, bootefi_image_path);
>>> +				       NULL, NULL);
>>
>> Why?
>>
>>>  		/*
>>>  		 * 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();
>>> +		loaded_image_info.image_code_type = EFI_LOADER_CODE;
>>> +		loaded_image_info.image_data_type = EFI_LOADER_DATA;
>>
>> Also unrelated? Please split it out.
>>
>>>  		/* Initialize and populate EFI object list */
>>>  		if (!efi_obj_list_initalized)
>>>  			efi_init_obj_list();
>>> -		return efi_selftest(&loaded_image_info, &systab);
>>> +		/* Transfer environment variable efi_selftest as load options */
>>> +		set_load_options(&loaded_image_info, "efi_selftest");
>>> +		/* Execute the test */
>>> +		r = efi_selftest(&loaded_image_info, &systab);
>>> +		efi_restore_gd();
>>> +		free(loaded_image_info.load_options);
>>> +		list_del(&loaded_image_info_obj.link);
>>
>> That change too is unrelated to the patch description. Please split out.
>>
>>> +		return r;
>>>  	} else
>>>  #endif
>>>  	if (!strcmp(argv[1], "bootmgr")) {
>>> @@ -357,6 +397,8 @@ static char bootefi_help_text[] =
>>>  #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
>>>  	"bootefi selftest\n"
>>>  	"  - boot an EFI selftest application stored within U-Boot\n"
>>> +	"    Use environment variable efi_selftest to select a single test.\n"
>>> +	"    Use 'setenv efi_selftest list' to enumerate all tests.\n"
>>>  #endif
>>>  	"bootmgr [fdt addr]\n"
>>>  	"  - load and boot EFI payload based on BootOrder/BootXXXX variables.\n"
>>> diff --git a/include/efi_selftest.h b/include/efi_selftest.h
>>> index 7ec42a0406..978ca2a7ea 100644
>>> --- a/include/efi_selftest.h
>>> +++ b/include/efi_selftest.h
>>> @@ -12,6 +12,7 @@
>>>  #include <common.h>
>>>  #include <efi.h>
>>>  #include <efi_api.h>
>>> +#include <efi_loader.h>
>>>  #include <linker_lists.h>
>>>  
>>>  #define EFI_ST_SUCCESS 0
>>> @@ -71,6 +72,15 @@ void efi_st_printf(const char *fmt, ...)
>>>   */
>>>  int efi_st_memcmp(const void *buf1, const void *buf2, size_t length);
>>>  
>>> +/*
>>> + * Compare an u16 string to a char string.
>>> + *
>>> + * @buf1:	u16 string
>>> + * @buf2:	char string
>>> + * @return:	0 if both buffers contain the same bytes
>>> + */
>>> +int efi_st_strcmp_16_8(const u16 *buf1, const char *buf2);
>>> +
>>>  /*
>>>   * Reads an Unicode character from the input device.
>>>   *
>>> @@ -88,6 +98,7 @@ u16 efi_st_get_key(void);
>>>   * @setup:	set up the unit test
>>>   * @teardown:	tear down the unit test
>>>   * @execute:	execute the unit test
>>> + * @on_request:	test is only executed on request
>>>   */
>>>  struct efi_unit_test {
>>>  	const char *name;
>>> @@ -96,10 +107,17 @@ struct efi_unit_test {
>>>  		     const struct efi_system_table *systable);
>>>  	int (*execute)(void);
>>>  	int (*teardown)(void);
>>> +	bool on_request;
>>>  };
>>>  
>>>  /* Declare a new EFI unit test */
>>>  #define EFI_UNIT_TEST(__name)						\
>>>  	ll_entry_declare(struct efi_unit_test, __name, efi_unit_test)
>>>  
>>> +#define EFI_SELFTEST_TABLE_GUID \
>>> +	EFI_GUID(0xbc3ebe57, 0x09e5, 0xa59d, 0xdb, 0x87, \
>>> +		 0xf5, 0x79, 0x61, 0x62, 0x06, 0xde)
>>> +
>>> +extern const efi_guid_t efi_selftest_table_guid;
>>> +
>>>  #endif /* _EFI_SELFTEST_H */
>>> diff --git a/lib/efi_selftest/efi_selftest.c b/lib/efi_selftest/efi_selftest.c
>>> index 45d8d3d384..110284f9c7 100644
>>> --- a/lib/efi_selftest/efi_selftest.c
>>> +++ b/lib/efi_selftest/efi_selftest.c
>>> @@ -15,6 +15,8 @@ static const struct efi_runtime_services *runtime;
>>>  static efi_handle_t handle;
>>>  static u16 reset_message[] = L"Selftest completed";
>>>  
>>> +const efi_guid_t efi_selftest_table_guid = EFI_SELFTEST_TABLE_GUID;
>>> +
>>>  /*
>>>   * Exit the boot services.
>>>   *
>>> @@ -25,8 +27,8 @@ static u16 reset_message[] = L"Selftest completed";
>>>   */
>>>  void efi_st_exit_boot_services(void)
>>>  {
>>> -	unsigned long  map_size = 0;
>>> -	unsigned long  map_key;
>>> +	unsigned long map_size = 0;
>>> +	unsigned long map_key;
>>
>> Unrelated?
>>
>>>  	unsigned long desc_size;
>>>  	u32 desc_version;
>>>  	efi_status_t ret;
>>> @@ -133,6 +135,41 @@ static int teardown(struct efi_unit_test *test, unsigned int *failures)
>>>  	return ret;
>>>  }
>>>  
>>> +/*
>>> + * Check that a test exists.
>>> + *
>>> + * @testname:	name of the test
>>> + * @return:	test
>>> + */
>>> +static struct efi_unit_test *find_test(const u16 *testname)
>>> +{
>>> +	struct efi_unit_test *test;
>>> +
>>> +	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 (!efi_st_strcmp_16_8(testname, test->name))
>>
>> Why not just use UCS2 strings here and compare 16 to 16? Maybe you're
>> using the name more often in normal situations?
>>
>> Either way, not a biggie :)
> 
> I thought about defining the test names as UCS2. But wide strings need
> double the space.
> 
> So I decided to stay with char * as far as possible to reduce code size.
> 
> I remember that reviewing one patch you asked me to rearrange function
> arguments to save 16 bytes of compiled code size for a specific
> architecture.

Yes, for code that is default =y, so we have much more potential to hit
size constraints.

Either way, I'm perfectly happy with leaving it at 8byte strings.
Certainly makes them more readable :)


Alex

[U-Boot] [PATCH v2 5/9] efi_loader: guard against double inclusion of efi_loader.h

[Simon Glass]

On 13 October 2017 at 19:33, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> Use a define to detect double inclusion of efi_loader.h.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v2
>         new patch
> ---
>  include/efi_loader.h | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

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

[U-Boot] [PATCH v2 9/9] test/py: fix typo in test_efi_loader.py

[Simon Glass]

On 13 October 2017 at 19:33, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> Make a comment line easier to read.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v2
>         no change
> ---
>  test/py/tests/test_efi_loader.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

[U-Boot] [PATCH v2 3/9] efi_selftest: provide test for watchdog timer

[Simon Glass]

On 13 October 2017 at 19:33, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> The test verifies that resetting the watchdog timer ensures
> that it is not called during the timeout period.
>
> Testing that the watchdog timer actually executes a reset
> would require a test outside the efi_selftest framework.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v2
>         no change
> ---
>  lib/efi_selftest/Makefile                |   5 +-
>  lib/efi_selftest/efi_selftest_watchdog.c | 185 +++++++++++++++++++++++++++++++
>  2 files changed, 189 insertions(+), 1 deletion(-)
>  create mode 100644 lib/efi_selftest/efi_selftest_watchdog.c

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

[U-Boot] [PATCH v2 8/9] test/py: test reboot by EFI watchdog

[Simon Glass]

On 13 October 2017 at 19:33, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> Clear environment variable efi_selftest before executing the
> default tests.
>
> Provide a test verifying that the EFI watchdog
> reboots the system upon timeout.
>
> The test depends on CONFIG_CMD_EFI_SELFTEST=y.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v2
>         choose test via environment variable
> ---
>  test/py/tests/test_efi_selftest.py | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)

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