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;#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