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

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;

}