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.g...@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 mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
